Neil Horman [off-list ref] writes:
... What do you say to my proposal regarding the splitting of the device
dependent on how we were executed? It seems we can't use a single advice string
in this case, as no matter which we choose there is a use case in which it fails
to make sense.
When your cherry-pick got --allow-empty, it should pass --allow-empty to
its inner invocation of commit if it is picking an originally empty
commit, and this advice will not trigger because commit will happily
commit the no-change change.
When your cherry-pick got --allow-empty but not --keep-unnecessary-commit,
its inner invocation of commit must not pass --allow-empty if it is _not_
picking an originally empty commit. Then the inner commit will fail if it
auto resolves to no change, and the user sees the advice. The current
advice text is appropriate for this case.
When your cherry-pick did not get either of these flags, its inner
invocation of commit must not pass --allow-empty. The user sees the
advice when the auto resolved result matches HEAD from the commit invoked
by cherry-pick. The current advice text is fine for this case, as we say
"possibly", not "we definitely know it was due to conflict resolution".
Or your cherry-pick may have failed due to a conflict, regardless of the
options like --allow-empty or --keep-unnecessary-commit given to it, and
the user may have run commit after resolving the conflict. The current
advice text is fine for this case, too, as we say "possibly", and it
indeed is what just happened.
So I do not think you need to change anything with respect to the advice
message.
Am I missing some other cases?
On Tue, Apr 10, 2012 at 02:09:17PM -0700, Junio C Hamano wrote:
Neil Horman [off-list ref] writes:
quoted
... What do you say to my proposal regarding the splitting of the device
dependent on how we were executed? It seems we can't use a single advice string
in this case, as no matter which we choose there is a use case in which it fails
to make sense.
When your cherry-pick got --allow-empty, it should pass --allow-empty to
its inner invocation of commit if it is picking an originally empty
commit, and this advice will not trigger because commit will happily
commit the no-change change.
When your cherry-pick got --allow-empty but not --keep-unnecessary-commit,
its inner invocation of commit must not pass --allow-empty if it is _not_
picking an originally empty commit. Then the inner commit will fail if it
auto resolves to no change, and the user sees the advice. The current
advice text is appropriate for this case.
When your cherry-pick did not get either of these flags, its inner
invocation of commit must not pass --allow-empty. The user sees the
advice when the auto resolved result matches HEAD from the commit invoked
by cherry-pick. The current advice text is fine for this case, as we say
"possibly", not "we definitely know it was due to conflict resolution".
Or your cherry-pick may have failed due to a conflict, regardless of the
options like --allow-empty or --keep-unnecessary-commit given to it, and
the user may have run commit after resolving the conflict. The current
advice text is fine for this case, too, as we say "possibly", and it
indeed is what just happened.
So I do not think you need to change anything with respect to the advice
message.
Am I missing some other cases?
No, you covered all the cases, but I disagree with your assertion that the advice
is correct (or at least optimal) in any of these cases. If a cherry-pick without
any options is preformed and the commit is empty (regardless of the reason), the
advice given is that git commit --allow-empty should be used. With the addition
of these new options, thats not true any longer. Instead of using git commit
--allow-empty, you can use git cherry-pick --allow-empty.
Given your description above though, I'm ok rolling this back. Regardless of my
disagreement, git commit --allow-empty still works just as well after the
addition of these options, so while I still think its awkward to give git commit
advice on the result of a git cherry-pick operation, its not any more
problematic than the issues you've pointed out with my change. I'll roll this
back in my next version and will look for a way to provide better advice in the
future.
Regards
Neil
Neil Horman [off-list ref] writes:
No, you covered all the cases, but I disagree with your assertion that the advice
is correct (or at least optimal) in any of these cases. If a cherry-pick without
any options is preformed and the commit is empty (regardless of the reason), the
advice given is that git commit --allow-empty should be used. With the addition
of these new options, thats not true any longer. Instead of using git commit
--allow-empty, you can use git cherry-pick --allow-empty.
Sorry, I am confused. Do you mean that the sequence goes like this (with
concrete examples of command line args)?
$ git cherry-pick nh/empty-rebase
... stops because "git show nh/empty-rebase" is empty
$ git cherry-pick --allow-empty
But that cannot be correct, without --continue [*1*], i.e.
$ git cherry-pick --allow-empty --continue
no? I didn't check, but if the command without --continue in the above
sequence does not error out, I think it is a bug.
I am actually OK with suggesting "git cherry-pick --continue", but then
"cherry-pick --allow-empty" (or "--keep-unnecessary-commits") that punts
and gives the control back to the user should leave enough clue for a
later invocation of itself so that it can realize that the original
invocation was made with "--allow-empty". In other words, I am OK if the
interaction goes like this:
$ git cherry-pick --keep-unnecessary-commits nh/empty-rebase
... stops due to a conflict
$ edit builtin/revert.c
... the result ends up being empty
$ git add -u ;# resolved
$ git cherry-pick --continue
[Side note]
*1* It was an original UI mistake to make the users conclude a "git merge"
that asked the user to help resolving the conflict with "git commit",
which was inherited by "git cherry-pick" and "git revert", especially when
these three commands are merely a special "possibly zero or one stoppage"
case of more general sequencing commands like "am" and "rebase" that can
stop zero or more times to ask the user for help and the way to resume
them is to re-run the same command with "--continue" option (and without
any other arguments), e.g.
$ git am -3 ./+nh.mbox
... stops due to conflict and asks to resolve them
$ edit builtin/revert.c
$ git add builtin/revert.c
$ git am --continue
and also discussed that in the longer-term it would be nice to teach the
oddball commands to honor "--continue". "am" originally took "--resolved"
(and it still does, and it will do so in the future) for the same purpose,
and we taught it and "cherry-pick" and "revert" to honor "--continue".
Probably we should start teaching "merge" to honor it as well to complete
the vision.
On Wed, Apr 11, 2012 at 09:52:21AM -0700, Junio C Hamano wrote:
Neil Horman [off-list ref] writes:
quoted
No, you covered all the cases, but I disagree with your assertion that the advice
is correct (or at least optimal) in any of these cases. If a cherry-pick without
any options is preformed and the commit is empty (regardless of the reason), the
advice given is that git commit --allow-empty should be used. With the addition
of these new options, thats not true any longer. Instead of using git commit
--allow-empty, you can use git cherry-pick --allow-empty.
Sorry, I am confused. Do you mean that the sequence goes like this (with
concrete examples of command line args)?
$ git cherry-pick nh/empty-rebase
... stops because "git show nh/empty-rebase" is empty
$ git cherry-pick --allow-empty
No, currently what happens is the following:
$ git cherry-pick nh/empty-rebase
... either gets accepted as empty if the cherry-pick qualifies for
fast-forward, or stops if it doesn not, indicating the empty_cherry_pick advice
that git commit --allow-empty should be used.
While that advice is accurate, in that a git commit --allow-empty will accept
the empty commit, it would seem (at least to me) preferable to offer guidance
that git cherry-pick should be used instead in this case, because that is the
command the user was issuing.
But that cannot be correct, without --continue [*1*], i.e.
$ git cherry-pick --allow-empty --continue
no? I didn't check, but if the command without --continue in the above
sequence does not error out, I think it is a bug.
No, it errors out. I'm sorry to have confused you. The only point that I was
trying to make here is that, when running git cherry-pick, its seems awkward to
a user to get advice indicating that git commit --allow-empty should be run. My
change was intended to resolve that so that advice no how to use cherry-pick
options to avoid the error was issued instead. Thats all. I hadn't considered
the fact that manual resolution of a cherry-pick (where getting advice about git
commit makes more sense) was also a factor here
I am actually OK with suggesting "git cherry-pick --continue", but then
"cherry-pick --allow-empty" (or "--keep-unnecessary-commits") that punts
and gives the control back to the user should leave enough clue for a
later invocation of itself so that it can realize that the original
invocation was made with "--allow-empty". In other words, I am OK if the
interaction goes like this:
$ git cherry-pick --keep-unnecessary-commits nh/empty-rebase
... stops due to a conflict
$ edit builtin/revert.c
... the result ends up being empty
$ git add -u ;# resolved
$ git cherry-pick --continue
No, Id rather not do that thanks. The intent of these options we really to
automate the rebase process, so rebasing doesn't stop on empty commits. Using
the model above seems to disagree with that.
[Side note]
*1* It was an original UI mistake to make the users conclude a "git merge"
that asked the user to help resolving the conflict with "git commit",
which was inherited by "git cherry-pick" and "git revert", especially when
these three commands are merely a special "possibly zero or one stoppage"
case of more general sequencing commands like "am" and "rebase" that can
stop zero or more times to ask the user for help and the way to resume
them is to re-run the same command with "--continue" option (and without
any other arguments), e.g.
$ git am -3 ./+nh.mbox
... stops due to conflict and asks to resolve them
$ edit builtin/revert.c
$ git add builtin/revert.c
$ git am --continue
and also discussed that in the longer-term it would be nice to teach the
oddball commands to honor "--continue". "am" originally took "--resolved"
(and it still does, and it will do so in the future) for the same purpose,
and we taught it and "cherry-pick" and "revert" to honor "--continue".
Probably we should start teaching "merge" to honor it as well to complete
the vision.
I won't pretend to fully understand the implications of what you said on your
side note here, but yes, from the ways I've used merge in the past, allowing it
to continue would be very nice I think.
Neil