bug#29366: gitmerge to handle NEWS better

bug#29366: gitmerge to handle NEWS better

[Glenn Morris]

Package: emacs
Version: 25.3
Severity: wishlist

Hi,

Thanks for gitmerge.el, it's a great help.
It would be nice if it could handle NEWS better.
Currently etc/NEWS in the emacs-26 branch has been renamed to
etc/NEWS.26 in master. Running M-x gitmerge, this is not detected.
emacs-26:etc/NEWS changes are merged to master:etc/NEWS, resulting in a
merge conflict every time NEWS is touched in emacs-26.
Can gitmerge do something to help here?

bug#29366: gitmerge to handle NEWS better

[Glenn Morris]

Glenn Morris wrote:

> Currently etc/NEWS in the emacs-26 branch has been renamed to
> etc/NEWS.26 in master. Running M-x gitmerge, this is not detected.
> emacs-26:etc/NEWS changes are merged to master:etc/NEWS, resulting in a
> merge conflict every time NEWS is touched in emacs-26.
> Can gitmerge do something to help here?

I added some code for this in 0b6f4f2 that I thought worked.
There's pre-existing gitmerge code that uses smerge to try and
auto-resolve conflicts. I just added a special-case for NEWS at the same
place.

However, it's missing a step that will automatically continue the merge
(ie, do the equivalent of running M-x gitmerge after manually fixing a
conflict). I cannot see how this would work for the "smerge fixed a
conflict automatically" case either. I don't know how to simulate an
smerge-fixable conflict to test.

Am I overlooking something, or is the code missing something like the
following:

--- a/admin/gitmerge.el
+++ b/admin/gitmerge.el
@@ -431,7 +431,8 @@ gitmerge-resolve-unmerged
 	      (setq conflicted t)
 	    ;; Mark as resolved
 	    (call-process "git" nil t nil "add" file)))
-	(when conflicted
+	(if (not conflicted)
+	    (if files (gitmerge-maybe-resume 'noask))
 	  (with-current-buffer (get-buffer-create gitmerge-warning-buffer)
 	    (erase-buffer)
 	    (insert "For the following files, conflicts could\n"
@@ -457,7 +458,7 @@ gitmerge-repo-clean
 		    "diff" "--name-only")
       (zerop (buffer-size))))
 
-(defun gitmerge-maybe-resume ()
+(defun gitmerge-maybe-resume (&optional noask)
   "Check if we have to resume a merge.
 If so, add no longer conflicted files and commit."
   (let ((mergehead (file-exists-p
@@ -469,7 +470,7 @@ gitmerge-maybe-resume
 	       (not (gitmerge-repo-clean)))
       (user-error "Repository is not clean"))
     (when statusexist
-      (if (not (y-or-n-p "Resume merge? "))
+      (if (not (or noask (y-or-n-p "Resume merge? ")))
 	  (progn
 	    (delete-file gitmerge-status-file)
 	    ;; No resume.

bug#29366: gitmerge to handle NEWS better

[David Engster]

Hi Glenn,

I'm not sure about that smerge stuff either, that was something I took
over from bzrmerge. It may be that it is not functional. I hope I can
make time to take a look at this in the coming days.

-David

bug#29366: gitmerge to handle NEWS better

[Glenn Morris]

David Engster wrote:

> I'm not sure about that smerge stuff either, that was something I took
> over from bzrmerge. It may be that it is not functional. I hope I can
> make time to take a look at this in the coming days.

Thanks! Here's my revised patch for this. It just does a commit after a
successful auto-conflict resolution. It seems to work...

--- a/admin/gitmerge.el
+++ b/admin/gitmerge.el
@@ -316,11 +316,7 @@ gitmerge-resolve
                                    (gitmerge-emacs-version gitmerge--from))))
                    (file-exists-p temp)
                    (or noninteractive
-                       (and
-                        (y-or-n-p "Try to fix NEWS conflict? ")
-                        ;; FIXME
-                        (y-or-n-p "This is buggy, really try? ")
-                        )))
+                       (y-or-n-p "Try to fix NEWS conflict? ")))
               (let ((relfile (file-name-nondirectory file))
                     (tempfile (make-temp-file "gitmerge")))
                 (unwind-protect
@@ -431,7 +427,9 @@ gitmerge-resolve-unmerged
 	      (setq conflicted t)
 	    ;; Mark as resolved
 	    (call-process "git" nil t nil "add" file)))
-	(when conflicted
+	(if (not conflicted)
+	    (and files (not (gitmerge-commit))
+		 (error "Error committing resolution - fix it manually"))
 	  (with-current-buffer (get-buffer-create gitmerge-warning-buffer)
 	    (erase-buffer)
 	    (insert "For the following files, conflicts could\n"
@@ -457,6 +455,12 @@ gitmerge-repo-clean
 		    "diff" "--name-only")
       (zerop (buffer-size))))
 
+(defun gitmerge-commit ()
+  "Commit, and return non-nil if it succeeds."
+  (with-current-buffer (get-buffer-create gitmerge-output-buffer)
+    (erase-buffer)
+    (eq 0 (call-process "git" nil t nil "commit" "--no-edit"))))
+
 (defun gitmerge-maybe-resume ()
   "Check if we have to resume a merge.
 If so, add no longer conflicted files and commit."
@@ -478,11 +482,8 @@ gitmerge-maybe-resume
 	(gitmerge-resolve-unmerged)
 	;; Commit the merge.
 	(when mergehead
-	  (with-current-buffer (get-buffer-create gitmerge-output-buffer)
-	    (erase-buffer)
-	    (unless (zerop (call-process "git" nil t nil
-					 "commit" "--no-edit"))
-	      (error "Git error during merge - fix it manually"))))
+	  (or (gitmerge-commit)
+	      (error "Git error during merge - fix it manually")))
 	;; Successfully resumed.
 	t))))
 

bug#29366: gitmerge to handle NEWS better

[David Engster]

Glenn Morris writes:
> David Engster wrote:
>
>> I'm not sure about that smerge stuff either, that was something I took
>> over from bzrmerge. It may be that it is not functional. I hope I can
>> make time to take a look at this in the coming days.
>
> Thanks! Here's my revised patch for this. It just does a commit after a
> successful auto-conflict resolution. It seems to work...

Indeed, that final commit was missing. It works for me as well.

-David

bug#29366: gitmerge to handle NEWS better

[Glenn Morris]

David Engster wrote:

> Indeed, that final commit was missing. It works for me as well.

Thanks for checking.

bug#29366: gitmerge to handle NEWS better

[Glenn Morris]

Today I noticed that this still goes wrong sometimes.
See the obvious mistake in

http://lists.gnu.org/rl/emacs-diffs/2018-10/msg00163.html

where a change ends up in etc/NEWS when it should have gone to NEWS.26.
(Presumably this means git somehow merged it without a conflict?
I have no idea how it could merge it to that position.)

See also the other corrections I then found to be needed in
http://lists.gnu.org/r/emacs-diffs/2018-10/msg00165.html

I guess I repeat my comments from
http://lists.gnu.org/r/emacs-devel/2017-12/msg00340.html

1) It's a shame there's no proper rename tracking
2) Perhaps etc/NEWS should always be called etc/NEWS.MAJORVERSION right
from the start, with etc/NEWS being just a symlink.

bug#29366: gitmerge to handle NEWS better

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Tue, 23 Oct 2018 13:10:09 -0400
> 
> 2) Perhaps etc/NEWS should always be called etc/NEWS.MAJORVERSION right
> from the start, with etc/NEWS being just a symlink.

I cannot use symlinks, sorry.

bug#29366: gitmerge to handle NEWS better

[Eli Zaretskii]

> Date: Tue, 23 Oct 2018 20:23:36 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 29366@debbugs.gnu.org
> 
> > 2) Perhaps etc/NEWS should always be called etc/NEWS.MAJORVERSION right
> > from the start, with etc/NEWS being just a symlink.
> 
> I cannot use symlinks, sorry.

But maybe we could keep a NEWS.XYZ file in the repository, and rename
it as part of make-tarball.