[PATCH] Add abbreviated commit hash to rebase conflict message

[PATCH] Add abbreviated commit hash to rebase conflict message

[Sverre Rabbelier]

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(-)

diff --git a/git-am.sh b/git-am.sh
index 9042432..9d70588 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -837,7 +837,8 @@ did you forget to use 'git add'?"
 	fi
 	if test $apply_status != 0
 	then
-		eval_gettextln 'Patch failed at $msgnum $FIRSTLINE'
+		abbrev_commit=$(git log -1 --pretty=%h $commit)
+		eval_gettextln 'Patch $msgnum failed at [$abbrev_commit] $FIRSTLINE'
 		stop_here_user_resolve $this
 	fi
 
-- 
1.7.8.rc0.36.g67522.dirty

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Also move the $msgnum to a more sensible location.
>
> Before:
> 	Patch failed at 0001 msg
> After:
> 	Patch 0001 failed at [da65151] msg

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?

> 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>

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?

> ---
>  git-am.sh |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 9042432..9d70588 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -837,7 +837,8 @@ did you forget to use 'git add'?"
>  	fi
>  	if test $apply_status != 0
>  	then
> -		eval_gettextln 'Patch failed at $msgnum $FIRSTLINE'
> +		abbrev_commit=$(git log -1 --pretty=%h $commit)
> +		eval_gettextln 'Patch $msgnum failed at [$abbrev_commit] $FIRSTLINE'

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Sverre Rabbelier]

Heya,

On Sun, Nov 6, 2011 at 01:31, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

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.

>> 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>
>
> 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?

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.

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

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.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> We had a little Git hackathon in Amsterdam today,...

Ah, that was the context I was missing.

>> What happens when threeway is not enabled, and especially when "git am" is
>> used for applying patches, not within rebase?
>
> 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.

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")?

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Sverre Rabbelier]

Heya,

On Sun, Nov 6, 2011 at 05:14, Junio C Hamano <gitster@pobox.com> wrote:
> 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")?

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?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Sun, Nov 6, 2011 at 05:14, Junio C Hamano <gitster@pobox.com> wrote:
>> 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")?
>
> 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?

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.

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Sverre Rabbelier]

Heya,

On Sun, Nov 6, 2011 at 21:27, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Sun, Nov 6, 2011 at 21:27, Junio C Hamano <gitster@pobox.com> wrote:
>> 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.
>
> 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?

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.

Re: [PATCH] Add abbreviated commit hash to rebase conflict message

[Junio C Hamano]

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

> Sverre Rabbelier <srabbelier@gmail.com> writes:
>
>> On Sun, Nov 6, 2011 at 21:27, Junio C Hamano <gitster@pobox.com> wrote:
>>> 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.
>>
>> 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?
>
> 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.

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.