Re: MERGE_RR droppings

Re: MERGE_RR droppings

[Phil Hord]

git mergetool decides to use rerere-remaining based on the existence
of the .git/MERGE_RR file:

    if test -e "$GIT_DIR/MERGE_RR"
    then
        files=$(git rerere remaining)
    else
        files=$(git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u)
    fi

But when I disabled rerere, I still had MERGE_RR files left over in my
.git directory.   This causes git-mergetool to do the wrong thing.

I do not know if the correct fix for this is to check rerere.enabled instead.

Is MERGE_RR leaking somewhere it should have been cleaned up?  I often
see leftover MERGE_RR files even though I am certain the project is
not in the middle of a merge.

Is this a bad semaphore to check?  I do not see it being used this way
anywhere else.


Phil

Re: MERGE_RR droppings

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> git mergetool decides to use rerere-remaining based on the existence
> of the .git/MERGE_RR file:
>
>     if test -e "$GIT_DIR/MERGE_RR"

This is correct, I would probably write it with "test -f" if I were
writing this line today, though.  After you commit to conclude the
merge, the MERGE_RR marker should disappear.  Isn't it happening for
you?

Re: MERGE_RR droppings

[Phil Hord]

On Mon, Jun 18, 2012 at 7:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> git mergetool decides to use rerere-remaining based on the existence
>> of the .git/MERGE_RR file:
>>
>>     if test -e "$GIT_DIR/MERGE_RR"
>
> This is correct, I would probably write it with "test -f" if I were
> writing this line today, though.  After you commit to conclude the
> merge, the MERGE_RR marker should disappear.  Isn't it happening for
> you?

I was able to isolate this today to cherry-pick.

When I successfully cherry-pick a change (no conflicts, no merges), I
get a .git/MERGE_RR file left over.  After this happens, git-mergetool
will think there are no unresolved conflicts unless git (or I) first
removes the .git/MERGE_RR.

Maybe this only causes trouble for 'git stash apply' since it does not
employ rerere machinery, or because it takes some other code path
which does not begin by removing .git/MERGE_RR.

A naive fix would be to remove MERGE_RR at the conclusion of
'stash-apply', but I do not think that is the right place for it to
go.

Suggestions?

Phil

Re: MERGE_RR droppings

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Phil Hord <phil.hord@gmail.com> writes:
>
>> git mergetool decides to use rerere-remaining based on the existence
>> of the .git/MERGE_RR file:
>>
>>     if test -e "$GIT_DIR/MERGE_RR"
>
> This is correct, I would probably write it with "test -f" if I were
> writing this line today, though.  After you commit to conclude the
> merge, the MERGE_RR marker should disappear.  Isn't it happening for
> you?

Oops.  It actually does not happen for _me_. An empty MERGE_RR is
left in the working tree after rerere has dealt with the conflicts.
There seems to be only three codepaths that explicitly remove
MERGE_RR that is stale:

 - (obviously) "rerere clear"
 - "git reset"
 - "git checkout otherbranch"

Perhaps mergetool should consider a missing MERGE_RR and an empty
one the same way?

Re: MERGE_RR droppings

[David Aguilar]

On Mon, Jun 25, 2012 at 12:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Phil Hord <phil.hord@gmail.com> writes:
>>
>>> git mergetool decides to use rerere-remaining based on the existence
>>> of the .git/MERGE_RR file:
>>>
>>>     if test -e "$GIT_DIR/MERGE_RR"
>>
>> This is correct, I would probably write it with "test -f" if I were
>> writing this line today, though.  After you commit to conclude the
>> merge, the MERGE_RR marker should disappear.  Isn't it happening for
>> you?
>
> Oops.  It actually does not happen for _me_. An empty MERGE_RR is
> left in the working tree after rerere has dealt with the conflicts.
> There seems to be only three codepaths that explicitly remove
> MERGE_RR that is stale:
>
>  - (obviously) "rerere clear"
>  - "git reset"
>  - "git checkout otherbranch"
>
> Perhaps mergetool should consider a missing MERGE_RR and an empty
> one the same way?

Would that test look (roughly) like this?

    if test -f "$GIT_DIR/MERGE_RR" && test -n "$(cat "$GIT_DIR/MERGE_RR")"
    then
	files=$(git rerere remaining)
    else
	files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
    fi
    ...

If so I'll throw together a patch.
-- 
David

Re: MERGE_RR droppings

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>> Perhaps mergetool should consider a missing MERGE_RR and an empty
>> one the same way?
>
> Would that test look (roughly) like this?
>
>     if test -f "$GIT_DIR/MERGE_RR" && test -n "$(cat "$GIT_DIR/MERGE_RR")"

Hrm, would "test -s $that_file" be simpler?


>     then
> 	files=$(git rerere remaining)
>     else
> 	files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
>     fi
>     ...
>
> If so I'll throw together a patch.

Re: MERGE_RR droppings

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> git mergetool decides to use rerere-remaining based on the existence
> of the .git/MERGE_RR file:
>
>     if test -e "$GIT_DIR/MERGE_RR"
>     then
>         files=$(git rerere remaining)
>     else
>         files=$(git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u)
>     fi
>
> But when I disabled rerere, I still had MERGE_RR files left over in my
> .git directory.   This causes git-mergetool to do the wrong thing.
>
> I do not know if the correct fix for this is to check rerere.enabled instead.

I do not think basing the decision to use "ls-files -u" output upon
the use of "rerere" is a good thing for this script to do in the
first place.  Doesn't it close the door to future enhancements and
third-party mechanisms that do better job than rerere to help the
users auto-resolve conflicted merges?  They certainly do not have to
use GIT_DIR/MERGE_RR nor GIT_DIR/rr-cache, and "rerere remaining"
would not know which paths they handled already.

After all, the user may have resolved the conflicts in his editor
but hasn't recorded the resolution to the index yet when he runs
mergetool, and the tool should handle such a path exactly like how
it currently handles the paths resolved by rerere in the working
tree.  But the above code will include such paths in the $files list
and throw them at the mergetool backends when rerere is not in use.

This part of the code probably should be _inspecting_ the files and
the index to find out what still needs to be fed to the mergetools
itself.  Asking "rerere remaining" about things that it may not have
done (either the user edited the paths in the editor, or some
third-party mechanism resolved the conflicts already) does not make
much sense, methinks.

Re: MERGE_RR droppings

[Phil Hord]

On Tue, Jun 26, 2012 at 2:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> git mergetool decides to use rerere-remaining based on the existence
>> of the .git/MERGE_RR file:
>>
>>     if test -e "$GIT_DIR/MERGE_RR"
>>     then
>>         files=$(git rerere remaining)
>>     else
>>         files=$(git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u)
>>     fi
>>
>> But when I disabled rerere, I still had MERGE_RR files left over in my
>> .git directory.   This causes git-mergetool to do the wrong thing.
>>
>> I do not know if the correct fix for this is to check rerere.enabled instead.
>
> I do not think basing the decision to use "ls-files -u" output upon
> the use of "rerere" is a good thing for this script to do in the
> first place.  Doesn't it close the door to future enhancements and
> third-party mechanisms that do better job than rerere to help the
> users auto-resolve conflicted merges?  They certainly do not have to
> use GIT_DIR/MERGE_RR nor GIT_DIR/rr-cache, and "rerere remaining"
> would not know which paths they handled already.
>
> After all, the user may have resolved the conflicts in his editor
> but hasn't recorded the resolution to the index yet when he runs
> mergetool, and the tool should handle such a path exactly like how
> it currently handles the paths resolved by rerere in the working
> tree.  But the above code will include such paths in the $files list
> and throw them at the mergetool backends when rerere is not in use.
>
> This part of the code probably should be _inspecting_ the files and
> the index to find out what still needs to be fed to the mergetools
> itself.  Asking "rerere remaining" about things that it may not have
> done (either the user edited the paths in the editor, or some
> third-party mechanism resolved the conflicts already) does not make
> much sense, methinks.


I'll buy that.  But what should we be inspecting the files for?  What
indicates the conflicts are resolved if not 'git add/rm'?

Meanwhile, I run into this trouble when I get a conflicted
git-stash-pop.  The first thing I do is invoke git-mergetool.  This
fails with "No files need merging" in the case of a GIT_DIR/MERGE_RR
leave-behind from some previous activity.  This is quite itchy, and so
I am looking for the correct thing to do.

Does the MERGE_RR file need to be deleted more often?
Do we need some extensible way to indicate currently-unresolved files?
Or something in-between these two?

P

Re: MERGE_RR droppings

[David Aguilar]

On Tue, Jun 26, 2012 at 12:00 PM, Phil Hord <phil.hord@gmail.com> wrote:
> On Tue, Jun 26, 2012 at 2:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Phil Hord <phil.hord@gmail.com> writes:
>>
>>> git mergetool decides to use rerere-remaining based on the existence
>>> of the .git/MERGE_RR file:
>>>
>>>     if test -e "$GIT_DIR/MERGE_RR"
>>>     then
>>>         files=$(git rerere remaining)
>>>     else
>>>         files=$(git ls-files -u | sed -e 's/^[^ ]*      //' | sort -u)
>>>     fi
>>>
>>> But when I disabled rerere, I still had MERGE_RR files left over in my
>>> .git directory.   This causes git-mergetool to do the wrong thing.
>>>
>>> I do not know if the correct fix for this is to check rerere.enabled instead.
>>
>> I do not think basing the decision to use "ls-files -u" output upon
>> the use of "rerere" is a good thing for this script to do in the
>> first place.  Doesn't it close the door to future enhancements and
>> third-party mechanisms that do better job than rerere to help the
>> users auto-resolve conflicted merges?  They certainly do not have to
>> use GIT_DIR/MERGE_RR nor GIT_DIR/rr-cache, and "rerere remaining"
>> would not know which paths they handled already.
>>
>> After all, the user may have resolved the conflicts in his editor
>> but hasn't recorded the resolution to the index yet when he runs
>> mergetool, and the tool should handle such a path exactly like how
>> it currently handles the paths resolved by rerere in the working
>> tree.  But the above code will include such paths in the $files list
>> and throw them at the mergetool backends when rerere is not in use.
>>
>> This part of the code probably should be _inspecting_ the files and
>> the index to find out what still needs to be fed to the mergetools
>> itself.  Asking "rerere remaining" about things that it may not have
>> done (either the user edited the paths in the editor, or some
>> third-party mechanism resolved the conflicts already) does not make
>> much sense, methinks.
>
>
> I'll buy that.  But what should we be inspecting the files for?  What
> indicates the conflicts are resolved if not 'git add/rm'?
>
> Meanwhile, I run into this trouble when I get a conflicted
> git-stash-pop.  The first thing I do is invoke git-mergetool.  This
> fails with "No files need merging" in the case of a GIT_DIR/MERGE_RR
> leave-behind from some previous activity.  This is quite itchy, and so
> I am looking for the correct thing to do.
>
> [1]Does the MERGE_RR file need to be deleted more often?
> [2]Do we need some extensible way to indicate currently-unresolved files?
> Or something in-between these two?
>
> P

>From a mergetool perspective, just doing #2 would be nice.
We can have a "default" unmerged files provider that does the
`test -s .../MERGE_RR` check since that would probably deal with
this ugly edge case.

We can then teach this default thing to be smarter and skip text files
with no merge markers. Junio, is this what you were thinking?
This sounds like a nice way to me.
-- 
David

Re: MERGE_RR droppings

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> We can have a "default" unmerged files provider that does the
> `test -s .../MERGE_RR` check since that would probably deal with
> this ugly edge case.
>
> We can then teach this default thing to be smarter and skip text files
> with no merge markers. Junio, is this what you were thinking?
> This sounds like a nice way to me.

I am not sure what I were thinking ;-) so let me think aloud.

In any case, once the user says (either via directly using "git add"
or using the command via the mergetool) the path is resolved,
mergetool would no longer want to touch it, until the user says
"oops, the resolution was a mistake; pleae make it unresolved again"
with something like "git checkout -m".  So in this message, I'll
think only about the case where the index is still unmerged.

The working tree files may be in one of these states:

 1. It may still be the same as the conflicted state after the
    three-way merge proper gave the user, perhaps with conflict
    markers in the text.

 2. rerere or some other mechanism (this includes manual editing or
    previous run of "mergetool") may have resolved the conflict
    fully.

 3. Or such a mechanism may have only half-resolved the conflict.

As far as I can see, the mergetool machinery, once it decides to
feed a path to the mergetool backend, ignores what is in the working
tree file, and uses the stage #2 (i.e. the state before merge
happened) as LOCAL (and stage #1 as BASE, stage #3 as REMOTE) to
redo the entire three-way merge from scratch.

So "ask rerere what it hasn't resolved" would only safely work for
the first kind, where the state in the working tree file is known to
be expendable.

If it can somehow determine paths that have been fully resolved, it
can (and should) skip redoing the three-way merge.  And that is
probably what the "if test -s MERGE_RR; then rerere remaining; fi"
logic under discussion is about.

But that will still risk losing any half-resolution done by external
means (the third category), including manual editing.  Is that a
good thing?

Perhaps there were two conflicting hunks and one of them the user
has resolved manually, and then the other one was a bit more complex
than the user wanted to resolve in the editor and that is why the
user is now running "mergetool" to resolve it in a GUI.  "rerere"
will say "I didn't touch that path", and "mergetool" will happily
ignore user's manual resolution of one hunk in the working tree, and
the user ends up redoing the whole three-way merge for the path, no?

Even if you switch that logic from "asking rerere" to "look for
conflict markers", it wouldn't change the story.  Perhaps "rerere
remaining" was a useless addition, and instead we probably should
have a way to ask "rerere" if the working tree file is different
from what the mergy operation left before "rerere" tried to work on
it?  If it is unchanged, then we know nobody (either the user, any
third-party mechanism, or rerere) touched it and it is safe for a
mergetool backend to redo the three-way merge from scratch (i.e. we
know that is in the first category).

Otherwise, you are risking to stomp on the change that may have been
done between the time the mergy operation was done and "mergetool"
was started.

Re: MERGE_RR droppings

[Phil Hord]

On Mon, Jul 2, 2012 at 3:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> We can have a "default" unmerged files provider that does the
>> `test -s .../MERGE_RR` check since that would probably deal with
>> this ugly edge case.
>>
>> We can then teach this default thing to be smarter and skip text files
>> with no merge markers. Junio, is this what you were thinking?
>> This sounds like a nice way to me.
>
> I am not sure what I were thinking ;-) so let me think aloud.
>
> In any case, once the user says (either via directly using "git add"
> or using the command via the mergetool) the path is resolved,
> mergetool would no longer want to touch it, until the user says
> "oops, the resolution was a mistake; pleae make it unresolved again"
> with something like "git checkout -m".  So in this message, I'll
> think only about the case where the index is still unmerged.
>
> The working tree files may be in one of these states:
>
>  1. It may still be the same as the conflicted state after the
>     three-way merge proper gave the user, perhaps with conflict
>     markers in the text.
>
>  2. rerere or some other mechanism (this includes manual editing or
>     previous run of "mergetool") may have resolved the conflict
>     fully.
>
>  3. Or such a mechanism may have only half-resolved the conflict.
>
> As far as I can see, the mergetool machinery, once it decides to
> feed a path to the mergetool backend, ignores what is in the working
> tree file, and uses the stage #2 (i.e. the state before merge
> happened) as LOCAL (and stage #1 as BASE, stage #3 as REMOTE) to
> redo the entire three-way merge from scratch.
>
> So "ask rerere what it hasn't resolved" would only safely work for
> the first kind, where the state in the working tree file is known to
> be expendable.

Yes, and specifically it would only work with 'rerere' and not the
"future enhancements and third-party mechanisms" you imagined earlier.

> If it can somehow determine paths that have been fully resolved, it
> can (and should) skip redoing the three-way merge.  And that is
> probably what the "if test -s MERGE_RR; then rerere remaining; fi"
> logic under discussion is about.
>
> But that will still risk losing any half-resolution done by external
> means (the third category), including manual editing.  Is that a
> good thing?
>
> Perhaps there were two conflicting hunks and one of them the user
> has resolved manually, and then the other one was a bit more complex
> than the user wanted to resolve in the editor and that is why the
> user is now running "mergetool" to resolve it in a GUI.  "rerere"
> will say "I didn't touch that path", and "mergetool" will happily
> ignore user's manual resolution of one hunk in the working tree, and
> the user ends up redoing the whole three-way merge for the path, no?

I think this is the case in practice, but the workdir file should
still be available to the mergetool if it is smart enough to consider
it.  It is not quite stomped on, though the machinery does expect the
merge-result to wind up in the workdir file.

Consider also that after a "successful" mergetool execution, the file
is automatically marked as resolved by being added to the index.
Therefore, it is not practical for mergetool to be used first to
partially merge the file in the hopes of using another tool to
complete the merge in a later step.  This is opposite from the default
and recommended operation mode of rerere, which does NOT add the
resulting resolution to the index.  And this results in more "states"
for a file to be in after a merge.

> Even if you switch that logic from "asking rerere" to "look for
> conflict markers", it wouldn't change the story.

No, this really calls for some meta-information storage about the
file's status, such as marking the file as "fully resolved" or
"partially resolved" in some sort of checklist, maybe even with line
numbers indicating regions in :1:file.txt and :3:file.txt which are
still TBD (though this "resolved regions" business seems useless to me
right now). Maybe some merge tool can learn to preserve the
partially-resolved file as a partial target in some kind of iterative
process.  But to do that, we need to have some way to manage this
meta-info.  The index comes to mind as a likely container, but a state
file similar to MERGE_RR is easier and safer (unless the index already
has some extensibility here).

> Perhaps "rerere
> remaining" was a useless addition, and instead we probably should
> have a way to ask "rerere" if the working tree file is different
> from what the mergy operation left before "rerere" tried to work on
> it?  If it is unchanged, then we know nobody (either the user, any
> third-party mechanism, or rerere) touched it and it is safe for a
> mergetool backend to redo the three-way merge from scratch (i.e. we
> know that is in the first category).

Yes, though technically the user could have previously recorded the
file as "resolved" with the change markers in place, so an unchanged
file might NOT mean that it still needs to be resolved.  This seems
like a silly case to consider, but I can see it extending to other
kinds of diff-markers which do not show up in the file itself, for
example, and we would be no further.

> Otherwise, you are risking to stomp on the change that may have been
> done between the time the mergy operation was done and "mergetool"
> was started.

So  'mergetool' is not useful to examine the result of a successful
rerere-merge, for example. because it will certainly ignore the
already-merged bits in the workdir.  We also do not want to ask the
user to reconsider the rerere resolved files during her mergetool
session.

There certainly are way more corner cases in this niche than I ever expected.

The normal rerere action is to apply previous resolutions in the
workdir but not in the index.  This is so the user can recognize that
the change needs closer inspection because she does not trust rerere
to always merge the project perfectly.  These files still appear in
ls-files --unmerged.  rerere-remaining allows git to recognize those
files. It allows git-mergetool to merge the files rerere did not touch
and also avoid stomping on the rerere resolutions which are waiting in
the workdir for the user's attention.

I personally do not like all these slightly-different cases.  It
causes the rebase-mergtool-continue cycle to lose all consistency for
me.    I am eager to help sort this out into something reasonable. I
hope we can decide what that should look like.

In the meantime, I will try to find a way to clean out stray MERGE_RR
files before I stash-pop.

Phil