Ediff overwrites variants during merge conflict resolution

[richkinder]

While resolving a merge with conflicts, I typed e to open ediff while point was on the unmerged line in the status buffer, then immediately quit out of ediff with q. When I returned to run ediff again the variant buffers match the ediff-merge buffer instead of actually being variants.

I'm sort of new to this and I might be doing something wrong because the other thing that is strange is that during the conflict the status buffer shows the unmerged changes as both staged and unstaged. Is this normal?

While testing the above, every once in awhile exiting Ediff returned to the main frame, but Emacs did not appear to accept any keyboard input. I can't reproduce this right now but I will open a separate issue if I can.

[kyleam]

When I returned to run ediff again the variant buffers match the
ediff-merge buffer instead of actually being variants.

The changes in 79cba42 should fix that. What do you think,
@tarsius?

[tarsius]

Heh. I think I taught Ediff to not even mess with the separators at one time, before I even used Magit. But it seems I have lost that code. :-)

[tarsius]

The changes in 79cba42 should fix that. What do you think, @tarsius?

Looks reasonable and safe. So go ahead.

[kyleam]

Looks reasonable and safe. So go ahead.

Pushed to master (with typo fix in commit message).

[tarsius]

Thanks.

[richkinder]

Now it works if while quitting ediff you respond no to the "Conflict resolution finished, save the file?" query. If you respond yes it still overwrites the variants.

[kyleam]

I personally use smerge rather than ediff for resolving conflicts, so perhaps I'm missing something. If you're responding y to say that you have resolved the conflicts, doesn't that mean that your changes would have removed the markers?

[richkinder]

Well sometimes in merge with a lot of conflicts you get part way through and quit ediff to come back later. So you've removed some of the markers but not others and want to save your progress. IIRC this worked in Magit 1.x. I admit it's a narrow use case though so if it's a difficult change I'm ok keeping this closed because the fix you made is a good improvement.

[tarsius]

Well I would consider it a bug in Ediff that it replaces Git's perfectly fine separators with something else. To fix that we'll likely have to patch Ediff and eventually those changes should land upstream.

[johnmastro]

It looks like the markers Ediff uses are controlled by ediff-combination-pattern. Maybe Magit can let-bind that when calling Ediff.

[tarsius]

Could you please investigate that?

[kyleam]

@richkinder I see. Thanks.

@johnmastro Thanks for the information. That certainly looks like the variable to tweak. May be a bit more involved if the markers need to match the whole line since the git markers contain actual commit information. If so, the values would need to be constructed at the time of the call.

[tarsius]

Some preliminary work can be found in https://github.com/magit/magit/compare/jb/ediff-combination-pattern?expand=1.

[tarsius]

It would be nice if someone else could tackle this. None of the Magit maintainers actually regularly use Ediff and the issue is in Ediff, not Magit.

[tarsius]

I've added this to the list of things I might work on once I start contributing to Emacs itself more. And I am closing this issue because it's an Emacs issue, not a Magit issue. You might want to report this as an Emacs issue.

[tarsius]

I've now deleted the branch I mentioned above. In case someone wants to continue based on that, here are my cleaned up version of some ediff functions

;; (setq ediff-combination-pattern
;;       '("<<<<<<< variant A" A
;;         ">>>>>>> variant B" B
;;         "####### Ancestor" Ancestor
;;         "======= end"))

;; Cleaned up version of the functions in ediff-merg.el
;; (Yes, I also removed the error handling.)
;; (And no, so far I have not tested this at all.)

;; Used by ediff-set-state-of-diff-in-all-buffers,
;; ediff-combine-diffs, and ediff-merge-changed-from-default-p
;;
(defun ediff-get-combined-region (n)
  (let (ret (pattern ediff-combination-pattern))
    (while pattern
      (let ((delim (pop pattern))
            (spec  (pop pattern)))
        (when (ignore-errors
                (ediff-get-region-contents n spec ediff-control-buffer))
          (setq ret (concat ret delim "\n"))))
      (unless (cdr pattern)
        (setq reg (concat ret (pop pattern) "\n"))))))

;; Used by ediff-make-fine-diffs and ediff-set-fine-overlays-in-one-buffer
;;
(defun ediff-looks-like-combined-merge (region-num)
  (when (and ediff-merge-job
             (--when-let (ediff-get-state-of-diff region-num 'C)
               (string-match (regexp-quote "(A+B)") it)))
    (cl-destructuring-bind (beg . end)
        (ediff-get-diff-posn 'C nil region-num)
      (goto-char beg)
      (let (ret (pattern ediff-combination-pattern))
        (ediff-with-current-buffer ediff-buffer-C
          (while pattern
            (search-forward (pop pattern) end t)
            (--when-let (match-beginning 0)
              (setq ret (nconc ret (list it (match-end 0)))))
            (pop pattern)))
        ret))))

;; Used by many
;;
(defun ediff-get-diff-posn (buf-type pos &optional n control-buf)
  (unless control-buf
    (setq control-buf (current-buffer)))
  (let (diff-overlay)
    (ediff-with-current-buffer control-buf
      (unless n
        (setq n ediff-current-difference))
      (when (or (< n 0) (>= n ediff-number-of-differences))
        (if (> ediff-number-of-differences 0)
            (error ediff-BAD-DIFF-NUMBER
                   this-command (1+ n) ediff-number-of-differences)
          (error ediff-NO-DIFFERENCES)))
      (setq diff-overlay (ediff-get-diff-overlay n buf-type)))
    (unless (ediff-buffer-live-p (ediff-overlay-buffer diff-overlay))
      (error ediff-KILLED-VITAL-BUFFER))
    (pcase pos
      (`beg (ediff-overlay-start diff-overlay))
      (`end (ediff-overlay-end diff-overlay))
      (_ (cons (ediff-overlay-start diff-overlay)
               (ediff-overlay-end diff-overlay))))))