Status of conflicted files resolved with rerere

I played around with git rerere today and was surprised by the results. When a conflict has been resolved automatically by rerere, the file isn't added to the index like other files are where Git just used one of the regular merge resolution algorithms. What's worse, if git mergetool is invoked -- which is what I normally do when git merge needs help -- it has no idea that the file actually has been merged already, and launches the merge tool with the three files involved in the merge. If the user hasn't been paying attention to each line of the git merge output (stating the files who were automatically resolved) it's easy to trash rerere's work.

Would it make sense for git merge to add rerere'd files (where all hunks were recognized and resolved) to the index automatically? That would make it possible for the user to run git mergetool without reading every single line of output from previous commands just to figure out which files should be added to the index straight off and which files require merging.

For users resolving conflicts by editing the file and fiddling with the <<<<<<<<< marks etc such a change wouldn't make that big difference, but for mergetool users it seems like a quite useful improvement. Or is there something I'm failing to grasp here? This is on Git 1.7.0.3 by the way.

On Thu, Aug 12, 2010 at 5:28 PM, Magnus Bäck <magnus.back@sonyericsson.com> wrote:

The motivation behind the current behaviour, as I understand it, is that rerere speeds things up, but you don't necessarily want to trust that it has resolved your merge conflicts correctly. After all, they were unarguably *conflicts*, not just normal merge results, so you can't quite trust them.

That said, I've never had a problem where rerere did the wrong thing for me. Maybe there could be an option to override it.

Anyway, I never use a mergetool, so like you suspected, this has never been a major problem for me.

It sounds like the real problem here though it the mergetool stuff. Why is it disregarding all the automated merging that git has done and starting over from scratch? If git, in its infinite cleverness, has resolved *some* parts of the file but not others, wouldn't we want it to keep those resolutions? It sounds like mergetool is actually making things *more* work instead of less.

Is there some way to teach the mergetool stuff to be smarter? At the very least, having it auto-skip files that have no *remaining* conflicts might be a good idea.

Have fun,

Avery

On Thu, Aug 12, 2010 at 5:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote:

$ git config rerere.autoupdate true

j.

On Aug 12, 2010, at 2:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote:

Right, that would be a great enhancement.

The problem is that mergetool always uses local, remote, and base. It uses the unmerged flag in the index when deciding which files to consider.

Here's what we'd need in order to improve rerere and mergetool interaction: the ability to answer the question, "has this file been rerere merged?"

Once we have that then we can make mergetool skip these files by default. We would also need a command line flag to override the behaviour.

Perhaps a naive grep for merge markers in the worktree file would suffice? Heuristics have gone a long way in git so doing something like that wouldn't seem too atrocious.

It'd also have the benefit that mergetool would skip files merged by $EDITOR.

If anyone has any tips I'm all ears. Does this seem reasonable, and if so, what would be a good name for the command line flag to get the existing behavior? My gut feeling is that it shouldn't have a corresponding config variable.

Cheers,

David Aguilar <davvid@gmail.com> writes:

I do not quite understand why the user _runs_ mergetool on a file that has been already merged; isn't it an option not to do so in the first place?

Having said that.

I think you can use the fact that:

 - "ls-files -u" will list paths with conflicts; and

 - "rerere status" won't mention the ones that have been autoresolved

if rerere is in effect (check for presense of .git/MERGE_RR).

On Sunday, August 15, 2010 at 08:59 CEST,
     Junio C Hamano <gitster@pobox.com> wrote:

You have a point, but if there are conflicts in many files where only a subset were autoresolved I think it would be prudent to help the user. Grepping after remaining conflict markers or keeping the "git merge" output somewhere to see which files actually were autoresolved works but I think we can do better.

On the other hand, hinting mergetool users about rerere.autoupdate is perhaps good enough? Doesn't help users who want to inspect the autoresolved results yet also want hassle-free mergetool usage, though.

Okay, I'll have a stab at a patch.

When mergetool is run without path limiters it loops over each entry in 'git ls-files -u'. This includes autoresolved paths.

Teach mergetool to only merge files listed in 'rerere status' when rerere is enabled.

There are some subtle but harmless changes in behavior. We now call cd_to_toplevel when no paths are given. We do this because 'rerere status' paths are always relative to the root. This is beneficial for the non-rerere use as well in that mergetool now runs against all unmerged files regardless of the current directory.

This also slightly tweaks the output when run without paths to be more readable.

The old output:

Merging the files: foo bar baz

The new output:

Merging: foo bar baz

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool.sh     |   28 +++++++++++++++++++++++-----
 t/t7610-mergetool.sh |   46 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 57 insertions(+), 17 deletions(-)

David Aguilar wrote:

[...]

This test never worked in my automatic testing (it fails and bisects to this commit).

It might be because the cronjob doesn't have a tty, as I'm seeing the output below (note the error at the end). Any insights?

expecting success: 
    cd subdir && (
    git config rerere.enabled false &&
    test_must_fail git merge master &&
    git mergetool --no-prompt &&
    test "$(cat ../file1)" = "master updated" &&
    test "$(cat ../file2)" = "master new" &&
    test "$(cat file3)" = "master new sub" &&
    git add ../file1 ../file2 file3 &&
    git commit -m "branch2 resolved by mergetool from subdir") &&
    cd ..

Merging: a8bf666 branch1 changes virtual master found 1 common ancestor(s): 775c381 added file1 Auto-merging file1 CONFLICT (content): Merge conflict in file1 Auto-merging file2 CONFLICT (add/add): Merge conflict in file2 Auto-merging subdir/file3 CONFLICT (content): Merge conflict in subdir/file3 Automatic merge failed; fix conflicts and then commit the result. Merging: file1 file2 subdir/file3

/local/home/trast/git/t/valgrind/bin/git-mergetool: line 302: /dev/tty: No such device
 or address
/local/home/trast/git/t/valgrind/bin/git-mergetool: line 299: /dev/tty: No such device
 or address

On Thu, Aug 19, 2010 at 12:02:36PM +0200, Thomas Rast wrote:

It must be the tty.

git-mergetool lines 295-307:

    files_to_merge |
    while IFS= read i
    do
	if test $last_status -ne 0; then
	    prompt_after_failed_merge < /dev/tty || exit 1
	fi
	printf "\n"
	merge_file "$i" < /dev/tty > /dev/tty
	last_status=$?
	if test $last_status -ne 0; then
	    rollup_status=1
	fi
    done

The reason the test fails without a tty is that we've never exercised this code in the past.

This commit did not introduce the "< /dev/tty > /dev/tty" idiom. It was introduced in b0169d84 by Charles Bailey. What this commit did do was add test coverage to it, which is good because it uncovered this problem :-)

Charles, is there another way we can write this? Is there a reason why we need the tty redirection? Can we drop it or is there a portability concern?

FWIW, the merge_file call in the else clause that follows this section does not use tty redirection.

On 20/08/2010 04:52, David Aguilar wrote:

Actually, it's been like this since c4b4a5af which is when mergetool was introduced.

(b0169d84 didn't change this line, 0eea3451 but made only whitespace changes, it comes from the original mergetool code.)

When you say "drop it" what are you proposing to replace it with? We're in the middle of a shell pipe which has replaced stdin and merge_file needs access to the human on it's stdin; hence the </dev/tty. Strictly. I believe that the >/dev/tty isn't needed.

Is there some way of juggling file descriptors in shell? I had a quick play with this but suspect it's a bashism (and it might make mergetool less readable!).

echo hidden | { echo lost | cat 0<&3- ; } 3<&0

mergetool has never really been very approachable for automatic testing as it's fundamentally an interactive script. It would be nice if sufficient of the guts of mergetool were in testable library code and mergetool was just an obviously correct slim shell UI.

merge_file in the 'else' doesn't need the redirection as nobody has redirected the original stdin.

Charles.

Charles Bailey wrote:

[...]

You can duplicate important fds, like so:

 exec 3<&0

 foo |
 (
	bar
	baz
	quuz <&3
 )

It's standard, luckily.  See http://unix.org/2008edition/

Hope that helps.

mergetool used /dev/tty to switch back to receiving input from the user via inside a block with a redirected stdin.

This harms testability, so change mergetool to save its original stdin to an alternative fd in this block and restore it for those sub-commands that need the original stdin.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---

This works on my fedora 12 box with bash. The redirects should be standard but this could do with some testing on other bourne shell implementations.

 git-mergetool--lib.sh |    2 +-
 git-mergetool.sh      |    7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Charles Bailey wrote:

Sounds good.

Part of the run_merge_tool codepath. The only place this is called with TOOL_MODE=merge is by merge_file which has stdin redirected, so this should be safe. Good.

I would think this should work, but it doesn't feel idiomatic. Why not save stdin a little earlier, so the reader does not have to track down whether it has been redirected?

The test quietly passes for me with dash but fails with ksh:

 /home/jrn/src/git4/git-mergetool: line 303: 3: cannot open [Bad file descriptor]

With the patch below on top, it passes with dash and ksh. ---

On 20/08/2010 13:27, Jonathan Nieder wrote:

No special reason, I just thought it was more natural to save it at the time that we do the redirect..

... but given that this approach is evidently less portable your way is clearly better.

Thanks, I'll re-roll in a bit at squash your fixes in, if that's OK?

Charles.

Charles Bailey wrote:

Yeah, that's okay. :)

Thanks for your work.

mergetool used /dev/tty to switch back to receiving input from the user via inside a block with a redirected stdin.

This harms testability, so change mergetool to save its original stdin to an alternative fd in this block and restore it for those sub-commands that need the original stdin.

Includes additional compatibility fix from Jonathan Nieder.

Tested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Charles Bailey <charles@hashpling.org>
---

Now works on ksh as well as bash and dash.

 git-mergetool--lib.sh |    2 +-
 git-mergetool.sh      |    7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)