Interactive session when rebasing from Magit is broken

[dabrahams]

Under magit in Emacs, I just did a "rebase onto…" and it invoked git-subline-merge's user interaction in the magit-process buffer, which kinda doesn't work:

run git … rebase davea/x-y-z
First, rewinding head to replay your work on top of it...
Applying: WIP
Using index info to reconstruct a base tree...
M   _XYZ.xcodeproj/project.pbxproj
.git/rebase-apply/patch:30: trailing whitespace.

.git/rebase-apply/patch:47: trailing whitespace.

.git/rebase-apply/patch:105: trailing whitespace.

.git/rebase-apply/patch:128: trailing whitespace.

.git/rebase-apply/patch:133: trailing whitespace.

warning: squelched 3 whitespace errors
warning: 8 lines add whitespace errors.
Falling back to patching base and 3-way merge...

git-subline-merge v1.0


Conflicted hunk 1 of 2 (spans 5/1/5 lines) in SomeProject.xcodeproj/project.pbxproj...

<<<<<<< Current version
+         5538698B2210D5B40005EA6E /* XYZ.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5538698A2210D5B40005EA6E /* XYZ.swift */; };
+         5538698C2210D5B40005EA6E /* XYZ.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5538698A2210D5B40005EA6E /* XYZ.swift */; };
+         5538698D2210D5B40005EA6E /* XYZ.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5538698A2210D5B40005EA6E /* XYZ.swift */; };
+         5538698E2210D5B40005EA6E /* XYZ.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5538698A2210D5B40005EA6E /* XYZ.swift */; };
+         5538698F2210D5B40005EA6E /* XYZ.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5538698A2210D5B40005EA6E /* XYZ.swift */; };
||||||| Base version
======= Other version
+         553869852210D3180005EA6E /* XYZZY.swift in Sources */ = {isa = PBXBuildFile; fileRef = 553869842210D3180005EA6E /* XYZZY.swift */; };
+         553869862210D3180005EA6E /* XYZZY.swift in Sources */ = {isa = PBXBuildFile; fileRef = 553869842210D3180005EA6E /* XYZZY.swift */; };
+         553869872210D3180005EA6E /* XYZZY.swift in Sources */ = {isa = PBXBuildFile; fileRef = 553869842210D3180005EA6E /* XYZZY.swift */; };
+         553869882210D3180005EA6E /* XYZZY.swift in Sources */ = {isa = PBXBuildFile; fileRef = 553869842210D3180005EA6E /* XYZZY.swift */; };
+         553869892210D3180005EA6E /* XYZZY.swift in Sources */ = {isa = PBXBuildFile; fileRef = 553869842210D3180005EA6E /* XYZZY.swift */; };
>>>>>>>

   v - view entire hunk
   x - view hunk in context
   s - attempt sub-line merge
   m - resolve manually
   c - use current version
   b - use base version
   o - use other version
   k - skip this hunk
   q - skip all hunks in this file
Resolve this hunk (v/x/s/m/c/b/o/k/q)? 

[paulaltin]

Thanks for posting this!

I'm not familiar with magit myself; is the issue the weird characters appearing in the output, or the fact that magit-process buffer isn't interactive?

The characters are the ANSI escape codes that make the text output colored in (most) terminals. I'm not sure if it's possible to detect whether these are supported by the current target of stdout, but I'll look into it.

If the problem is that the buffer isn't interactive, it should also be possible to detect that, and then the script can simply abort if it isn't being run from an interactive terminal.

[dabrahams]

On Feb 11, 2019, at 12:32 AM, paulaltin notifications@github.com wrote:

Thanks for posting this!

I'm not familiar with magit myself; is

Try it; you’ll never go back. the issue the weird characters appearing in the output, or the fact that magit-process buffer isn't interactive?

All of that, but the latter is the bigger problem. It’s not expecting git to prompt me for anything, so I am stuck. The characters are the ANSI escape codes that make the text output colored in (most) terminals. I'm not sure if it's possible to detect whether these are supported by the current target of stdout, but I'll look into it.

If the problem is that the buffer isn't interactive, it should also be possible to detect that, and then the script can simply abort if it isn't being run from an interactive terminal.

As long as that leaves my merge somewhere useful, SGTM!

[paulaltin]

Try it; you’ll never go back.

Easier said than done, apparently! I'm not having any luck installing it on my machine, either through Homebrew or via the instructions on the website. How did you get it running?

If you have time and wouldn't mind helping me debug this, would you be able to try adding these lines just after the ### MAIN ### heading in git-subline-merge, running it again through magit, and posting the output? I think the problem has to do with stdin not being configured correctly, and this information might help.

Thanks!

############
### MAIN ###
############

print(sys.stdin)
print(sys.stdin.isatty())

print(open('/dev/tty'))
sys.stdin = open('/dev/tty')
print(sys.stdin)
print(sys.__stdin__)

[paulaltin]

The issue of ANSI escape sequences in the output should be fixed by #10.

[paulaltin]

The issue of interactivity is a little trickier.

The simplest thing to do would be to check whether stdin is connected to a terminal, and if it isn't then display a warning (with instructions on how to use non-interactive mode, see #5) and exit.

The problem is that this can result in both false negatives and false positives. In fact, git itself calls custom merge drivers in such a way that sys.stdin.isatty() returns False. We already have to work around this with sys.stdin = open('/dev/tty') when using Python's input command.

Could we perhaps use the ability to open /dev/tty as the test for an interactive terminal? For example, when run from an emacs shell, python -c 'open("/dev/tty")' throws IOError: [Errno 6] Device not configured: '/dev/tty'.

Even if this worked, we would probably also want a way for the user to override the auto-detect in case it gives a false negative (something like a FORCE_INTERACTIVE_MODE environment variable).

Also relevant: https://github.com/magit/magit/issues/3549

[paulaltin]

@dabrahams: I've pushed a commit to the same branch as the fixes to #9 and #10, which attempts to detect whether the script can be run interactively. I've tested it with the script run manually and invoked automatically by git during rebasing, both in a normal terminal and in an emacs shell. I don't have magit to test with, but given that it does the right thing in an emacs shell I'm hopeful that this fixes the issue.

[paulaltin]

Changes which attempt to auto-detect whether the script can run interactively have been merged to master, since they are strictly an improvement and did work in an emacs shell, but I'll leave this issue open until I can confirm that they work with magit also.

[danielmartin]

For me the colored text output is working correctly in Magit. Thanks.

About the prompt problem in Magit, I think it can be solved with a little Elisp. Something like this (rough experiment):

(defun magit-subline-merge-prompt (process string)
  "Forward git subline merge prompts to the user."
  (when (string-match-p "Resolve this hunk " string)
    (magit-process-buffer)
    (process-send-string
     process
     (concat
      (string (magit-process-kill-on-abort process
       (let* ((read-answer-short t)
              (git-subline-prompt-answer
               (read-answer "Resolve this hunk " git-subline-prompt)))
         (cadr (assoc git-subline-prompt-answer git-subline-prompt)))))
      "\n"))))

This function uses read-answer to prompt the user with the available git subline merge actions. Here's the git-subline-prompt alist:

(defconst git-subline-prompt '(("view"  ?v "view entire hunk")
                           ("view-context"   ?x "view hunk in context")
                           ("attempt"  ?s "attempt sub-line merge")
                           ("manually" ?m "resolve manually")
                           ("current" ?c "use current version")
                           ("base" ?b "use base version")
                           ("other" ?o "use other version")
                           ("skip" ?k "skip this hunk")
                           ("skip-all" ?q "skip all hunks in this file")))

Finally, we need to add magit-subline-merge-prompt as a hook in magit-process-prompt-functions:

(add-hook 'magit-process-prompt-functions #'magit-subline-merge-prompt)

[paulaltin]

Hi @danielmartin, thanks for looking at this! If it works I'd be happy to add your code to the Readme for other magit users.

GSM does prompt the user for other information as well, for example when choosing the "current", "base" or "other" options it asks if you are sure you want to discard the information on the other branches. I'm guessing similar functions would need to be written for each possible prompt text? Or is there a more general way of detecting when GSM is waiting for user input? (Sorry, I don't know any Elisp).

[danielmartin]

GSM does prompt the user for other information as well, for example when choosing the "current", "base" or "other" options it asks if you are sure you want to discard the information on the other branches. I'm guessing similar functions would need to be written for each possible prompt text? Or is there a more general way of detecting when GSM is waiting for user input? (Sorry, I don't know any Elisp).

I haven't checked, but if they are simple "yes or no" questions, Magit should already handle those well. See https://github.com/magit/magit/blob/00dbf89fb71872356b0c1084af3559fe20925e5d/lisp/magit-process.el#L752

[paulaltin]

OK, great! Yes, all the others are yes/no questions. Thanks!

[jscheid]

If I understand correctly, the reason that this isn't a problem with Magit and the default merge drivers is that the default drivers never enter interactive mode in the first place.

And further, quoting the README, that "[NON_INTERACTIVE_MODE] is strongly discouraged in most situations as it is likely to lead to erroneous merges."

This makes me wonder if there couldn't be another, more conservative setting for NON_INTERACTIVE_MODE, one where it would fall back to the default merge strategy if an automatic merge can't be performed. This would sidestep the issue with Magit, never lead to erroneous merges (where they wouldn't have occurred with default settings), and leave the user off no worse than if they didn't use this package in the first place.

Apologies if this a silly suggestion, I haven't actually used this package much so far and I might be missing something obvious. All the existing solutions sound a bit brittle, that's why I'm asking.

[paulaltin]

This makes me wonder if there couldn't be another, more conservative setting for NON_INTERACTIVE_MODE, one where it would fall back to the default merge strategy if an automatic merge can't be performed.

I'm not sure I understand what you mean, sorry. git-subline-merge won't be invoked unless a conflict arises, i.e. unless the default merge strategy has already failed. In the existing non-interactive mode, it will resolve all the conflicts that it can resolve, and leave all others untouched. What behavior are you suggesting for a more conservative setting?

[jscheid]

Thanks for your quick reply. I guess I'm suggesting a mode where if git-subline-merge can't resolve all conflicts automatically it simply bails out (with conflict markers as per the default strategy), rather than enter interactive mode.

Since posting my comment yesterday I discovered that git-subline-merge isn't a good fit for my use case, so I don't need it to work with Magit after all. I hope it's still a useful suggestion.

[paulaltin]

I guess I'm suggesting a mode where if git-subline-merge can't resolve all conflicts automatically it simply bails out (with conflict markers as per the default strategy), rather than enter interactive mode.

If I'm understanding you correctly, this is what the existing non-interactive mode does already.

Since posting my comment yesterday I discovered that git-subline-merge isn't a good fit for my use case, so I don't need it to work with Magit after all.

Sorry to hear that it isn't useful for you. Please feel free to make a suggestion if there's some way it could be improved to fit your use case.

[jscheid]

If I'm understanding you correctly, this is what the existing non-interactive mode does already.

Right, disregard me. I should have spent some more time evaluating this before piping up.

Sorry to hear that it isn't useful for you

Nothing wrong with it, it's just not a good match for my use case. All good, thanks for your help.

[paulaltin]

No worries at all, thanks for your interest and good luck finding a solution for your use case :)