patchAdd abbreviated commit hash to rebase conflict message

Also move the $msgnum to a more sensible location.

Before:
	Patch failed at 0001 msg
After:
	Patch 0001 failed at [da65151] msg

Reviewed-by: Eric Herman <eric@freesa.org>
Reviewed-by: Fernando Vezzosi <buccia@repnz.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---
 git-am.sh |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Sverre Rabbelier <srabbelier@gmail.com> writes:

We can guess that 7-hexdigit is an abbreviated commit object name but the above description and the title do not tell the most important thing. What commit are you trying to describe, and why is it a good idea to show it?

I wouldn't have issues if these were Helped-by or Asked-by or something, but a patch with Reviewed-by for which I do not see any trace of discussion on this list triggers some WTF at least for me.

Where did these reviews take place? What were their inputs and how was the patch improved based on them? Why I should trust the judgements of these people?

What happens when threeway is not enabled, and especially when "git am" is used for applying patches, not within rebase?

Heya,

On Sun, Nov 6, 2011 at 01:31, Junio C Hamano <gitster@pobox.com> wrote:

The same commit that the title and number are already being displayed for. It's a good idea to show that as that's a lot more convenient way to look up the commit that failed to apply than just a rather arbitrary number and the title.

We had a little Git hackathon in Amsterdam today, the review was done IRL. In this case it consisted of Fernando pointing out that we should stick to the git cherry-pick format of displaying the hash/title (with the hash in square brackets before the title), rather than in parenthesis after the title like I had before. I wanted to give credit to their offline review somehow. If you'd prefer the "Helped-by" nomer for this case I'm fine with that.

The same thing that already happens. I'm not sure what it is, but whatever title/number is shown, the matching hash is now shown as well. This patch does not change that behavior.

Sverre Rabbelier <srabbelier@gmail.com> writes:

Ah, that was the context I was missing.

I am puzzled, but that cannot be true. The existing message uses $msgnum and $FIRSTLINE but does not use $commit because it does not necessarily exist.

What a value would the variable contain when I am applying your original patch message using "git am -s" (or "git am -s3")?

Heya,

On Sun, Nov 6, 2011 at 05:14, Junio C Hamano <gitster@pobox.com> wrote:

Aaah, I understand the concern you raise now. In that case a spurious [] would be printed, which I agree is less than desirable. Would checking 'if test -n $commit' be sufficient?

Sverre Rabbelier <srabbelier@gmail.com> writes:

In what situation does it make sense to say "It came from _this_ commit"?

I think there is a separate variable that allows any part of the script if we are being run as a backend of rebase or not, and that is the condition you are looking for.

Heya,

On Sun, Nov 6, 2011 at 21:27, Junio C Hamano <gitster@pobox.com> wrote:

The closest I could find is:

                if test -f "$dotest/rebasing"

Which is exactly the case when commit is set. Do you prefer the "-f $dotest/rebasing" test or the "-n $commit" one?

Sverre Rabbelier <srabbelier@gmail.com> writes:

Given the variable scoping rules of vanilla shell script, relying on the variable $commit is a very bad idea to begin with. I think the variable also is used to hold the final commit object name produced by patch application elsewhere in the script in the same loop, and I do not think existing code clears it before each iteration, as each part of the exiting code uses the variable only immediately after that part assigns to the variable for its own purpose, and they all know that nobody uses the variable as a way for long haul communication media between different parts of the script. Unless your patch updated that aspect of the lifetime rule for the variable, which I doubt you did, using $commit would introduce yet another bug without solving anything, I would think.

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

I was looking at git-am today for a separate topic. Doesn't it appear to you that $dotest/original-commit is what serves your purpose the best?

The file is removed before starting to process a new input (i.e. message in the mbox), created only after we read the from line and determine it is really the commit we are rebasing, and is left intact until we decide the patch was applied correctly and write the result out as a tree.

It might be a clean-up to get rid of $dotest/original-commit file, rename the variable to $original_commit and initialize it to an empty string where we currently have 'rm -f "$dotest/original-commit"' (and replace the check 'test -f "$dotest/original-commit"' later in the script with a check 'test -n "$original_commit"'), though.