git add -p and unresolved conflicts

I'm running git version 1.7.9.msysgit.0.390.g01fca.dirty, and I've noticed a puzzling behavior with "git add -p" on a conflicted file: it seems to simply spew the diff and exit the process.

Is this the expected behavior? It doesn't seem very useful to me...

Here's how to reproduce the behavior:

git init /tmp/git-add--p-conflict cd /tmp/git-add--p-conflict/ echo "foo" > bar.txt git add bar.txt git commit -m. echo "bar" >> bar.txt git stash save echo "baz" >> bar.txt git commit -am. git stash pop git add -p bar.txt

Erik Faye-Lund <kusmabite@gmail.com> writes:

Yeah, when I wrote the 'p'atch mode in "add--interactive", I wasn't interested at all in letting it be used on a conflicted path, so that is not a designed-in behaviour but merely whatever it happens to do. So at least it should not allow the user to pick a path that has conflicts in it.

What the behaviour for people who *do* want to use the patch mode for conflicted paths should be is a separate matter. As I said, I am not interested in it, so I wouldn't be the best person to design it.

I mildly suspect nobody would come up with a sane behaviour, but what would I know...

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

Designing a per-hunk behavior would clearly be very tricky.

But a simple way to deal with this would be to show the whole-file patch, and ask "stage this file (yes/no)?".

My fingers are used to typing

git add -p y y y git commit

and they would appreciate if it worked in the presence of conflicts. Most of the time, I have small conflicts, so a whole-file behavior wouldn't be disturbing, but I like seeing the changes before commiting them.

On 28.03.2012 17:21, Matthieu Moy wrote:

I don't think it's that tricky:

For a conflicted file show any conflicted hunks first(!) and only provide the following options for them: q - quit, do not stage... d - do not stage this hunk nor any of the remaining hunks in the file e - manually edit the current hunk ? - print help

and additionally the following three options:
b - choose the base version
< - choose our version
 > - choose their version

< and > correspond to the conflict markers, alternatives would be o and t

The user now has the option to remove the conflicts (through editing or choosing one of the versions) or abort adding the file.

The tricky part might be sorting the hunks so that conflicts are first (depending on how the code works now), choosing the right version in a hunk (the three new options) should be relatively easy.

Holger Hellmuth <hellmuth@ira.uka.de> writes:

Code it and then tell us if it is tricky or not. Until then, especially when you have to say "depending on how the code works now", implying you do not know, why should we even listen to you?

On 28.03.2012 21:19, Junio C Hamano wrote:

I was assuming this "tricky" was about a sensible user interface.

As it is perl I'm already looking at it. I'll try to find out if my meagre programming skills can do something here.

On Wed, Mar 28, 2012 at 09:52:55PM +0200, Holger Hellmuth wrote:

I think maybe the discussion elsewhere in this thread has given you a taste of the trickiness, but if you want to read more, here is a similar discussion from the same time last year (ahh, Spring, when a young man's thoughts turn to partial staging of conflicted hunks):

  http://thread.gmane.org/gmane.comp.version-control.git/168403/focus=168842

-Peff

On 29.03.2012 08:08, Jeff King wrote:

It already dawned on me that to add the proposed conflict resolution would also mean changing the working copy of the file which none of the other options needs. Which would change the work from (assumed) surgical changes to completely new routines and special cases and....

Holger Hellmuth <hellmuth@ira.uka.de> writes:

What does it mean to "choose" in this context? In general, it means "put this into the index", but if the file is in conflicted state, you have 3 different versions of the file in the index.

And you miss the most usefull (to me at least): "choose the version in the worktree".

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

I tend to think that "add -p" whose sole purpose is to make partial commit is fundamentally incompatible with "am", "merge" and (mostly) "rebase", which you do not want to record partial commit.

When "rebase -i" or "cherry-pick" that is used for the purpose of replaying only a part of a larger commit, letting "add -p" show the difference between "HEAD" and the working tree version, and letting the user pick hunks, would make a lot of sense, though.

For such an application, however, the user could first "reset $path" and use "add -p" normally.

On 28.03.2012 21:38, Matthieu Moy wrote:

The only ways to exit with the proposed reduced options is to either quit adding this file (which is comparable to Eriks option 4) or resolving all conflicts in the file thereby allowing to add this file to the index.

But the conflicted chunks are of the form "<<<< our ... |||||||||| theirs >>>>>>" in your work tree. So there are two cases:

a) You have removed the markers thereby removing the conflict -> this means the chunk will not be offered to you as a conflicting chunk

b) You haven't removed the markers. Then there is a choice between base, our and their version. If you had edited one of the versions between the conflict markers in your working tree without removing the markers (which wouldn't be a sensible thing) then it depends on whether the chunk in the working tree or the versions in the index are used as a basis for the choosing, but I suspect using the working tree version is easier to do.

Holger Hellmuth <hellmuth@ira.uka.de> writes:

I think Matthieu is not thinking about the "resolve conflicted merge (or rebase, am) and make a single commit" use case, for which your "pick one of the sides" makes sense. His is more about "I like some change in that unrelated commit---while I have no intention to replay that change as a whole, I want to use "cherry-pick --no-commit" and pick only the bits out of the change that are useful for what I am doing" use case.

In such a context, after resolving conflicts in the working tree, "add -p" would be a good tool to pick hunks in the diff between the HEAD and the working tree.

I tend to think that use case, while valid, may be a minority case and giving a version of "add -p" to make that mode too easy to use by mistake during a true conflict resolution is inviting trouble for newbies. As I said, "reset $path" followed by regular "add -p $path" would be a usable workaround for Matthieu's workflow.

Am 28.03.2012 22:50, schrieb Junio C Hamano:

Ok. Something like this (which is not really that different from Eriks example, so I might still be missing something) ?

git init . echo "foo" > bar.txt git add bar.txt git commit -m. git checkout -b new echo "bar" >> bar.txt git commit -am. git checkout master git reset --hard echo "baz" >> bar.txt git commit -am. git cherry-pick --no-commit new git add -p bar.txt

So he would be interested in keeping his previous working tree version (identical to HEAD commit, otherwise cherry-pick fails) for conflicting chunks as they (in most cases) don't have anything to do with the chunks he is interested in. In this case that is "baz"

Which would always be the "ours" side in any conflicting junks.

That suggests that he could just type '<' respective 'o' whenever a conflicting junk comes up as it would be the previous working tree version

Holger Hellmuth <hellmuth@ira.uka.de> writes:

After re-reading what I wrote, I find that I misspoke.

There is a crucial step missing. After running "cherry-pick --no-commit", you would eyeball the result, and typically edit the file. The conflict may not be in the part of the file you are interested in picking, in which case you may even leave that part as-is. And then, you would pick what you want to apply to the index during per-hunk interactive patching session.

In other words,...

Here, there *will* be "edit bar.txt" before "add -p".  Which makes...

...this part of your analysys (and your conclusion that he can take '<') invalid.

Now I am not Matthieu, so he may have a different use case in mind when he said "I want to take the version from my working tree", but the sequence "cherry-pick --no-commit ; edit ; add ; commit" is a natural way to pick only a part relevant to your current work from unrelated commit, and I would understand why "Taking the version from my working tree" can be useful in such a scenario.

In short, these two are totally different workflows and call for two different semantics.

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

Strictly speaking, you do not even have to edit the file. If the conflict does not overlap the part of the change you are interested in, a hypothetical version of "add -p" that is oblivious to merge conflicts (i.e. running "reset $path" before starting "add -p") could help you apply the changes the picked commit makes relative to its parents to your current state.

Am 29.03.2012 01:01, schrieb Junio C Hamano:

But that changes nothing. To make it clear: We have a file with chunks A, B and C. Only B is conflicting, but the user wants to add A or an edited A´ . And maybe also a new edit D.

When he calls "git add -p", he gets presented with B. By picking the "ours" option (aka "<") he gets the version in HEAD, resolving the conflict and thereby allowing the add. Next he will be presented with A (or A´) where he can type in "y" and C where he can type in "n".

Result: chunk A is added, C not, and B doesn't change from HEAD. Note he 
had the option to use ">" instead if it was B he wanted to cherry-pick.

The same happens if he further edited somewhere else to produce chunk D. Again, after resolving the conflict of B, he is presented with A,B and D and can add these chunks with "y" or not with "n".

The only case I haven't talked about now is if he edits B, but that is the trivial case because he would normally remove the conflict markers as well and git add -p would work conflict-free after the edit.

Holger Hellmuth <hellmuth@ira.uka.de> writes:

If you have removed the markers, then the file is still marked as conflicted in the index, and the user may still want to see the combined diff.

My use-case is actually quite simple: I see "git add -p" both as a way to make partial commits, and as a way to manually validate what I'm about to commit. I review the diff, validate them with "y", and if I see something wrong, I quit "git add -p", fix the issue, and next "git add -p" won't show it again. This flow could work also with conflicts:

$ git add -p # see an unresolved diff for file foo.txt n # see a resolved diff for file bar.txt y <-- the "missing" possibility for me $ edit foo.txt $ git add -p # see resolution for file foo.txt y $ git commit

The "'git reset $path' before 'add -p'" workaround is not really handy there: it requires typing the path, while my workflow with "git add -p" does not, and it wouldn't show the combined diff, which is usually the best tool to see if a merge has been resolved properly.

The current behavior is not that bad for me: I see the combined diff in the output, I just have one extra "git add $path" to type. Being able to just say "y" instead would be handy, but not fundamentally different.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

That is exactly why "add -i" was written without first giving "add -p" shorthand, which spoiled you and at the same time robbed you the other parts of "add -i".

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

Quite frankly, I never liked the interface of "git add -i". It saves very little compared to typing "git status" and then doing stuff directly from the shell. "git add -p" on the other hand just requires "y" and "n" most of the time, and that really does what I want.

In the case of conflicts, I don't think "git add -i" solves the problem "view the diff and mark it as resolved" smoothly.

On Wed, Mar 28, 2012 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:

OK, that's pretty much what I suspected ;)

So, the options are:
 1) Keep the current behavior
 2) Abort with an error immediately if one of the picked paths are in
a conflicted state
 3) Abort with an error when first encounter a path that is in a
conflicted state
 4) Warn and skip any paths that are in a conflicted state
 5) Implement "git add -p" for conflicted files (if we can agree on a behavior)

1) and 3) makes little sense to me. 5) only makes sense if we can
agree on what should have happened.

Well, I can explain what I expected to happen:

I had stashed away some of my "hacking"-code to rebase a branch to the current master. I was going to stage parts of the stashed changes as a commit on top. However after rebasing, "git stash pop" lead to a conflict. So I manually resolved the conflicts, but only wanted to stage some of the hunks to be committed.

Now, what is obvious in hindsight, is that I should simply have done "git reset" after resolving the conflicts manually. When I do "git add -p", I add hunks to the index. But for a conflicted file, the index isn't in a well-defined state (or, the contents of the file is in a not-yet-defined state).

However, as it happened it made me confused (and a little bit frustrated). I suspect this could confuse other users as well. Perhaps we should implement 1) or 3) and come up with a helpful error/warning message? I don't have a very strong opinion on which one is better, but I suspect 3) is easier.

Erik Faye-Lund <kusmabite@gmail.com> writes:

I do not think 1, 2, or 3 makes any sense to me if you are proposing to change anything. Why did you even bother to list them?

Given that you wrote "(if we can...)" for #5, in the short term, #4 is the only sensible thing to do, I would think.