[PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Ralf Thielow]

Printing a 40 character sha1 hash in the middle of a message
stretches the sentence a lot. Print the abbreviated version
instead.

Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
---
 sequencer.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c4f4b7d..2a39ab6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -498,20 +498,21 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		struct commit_list *p;
 
 		if (!opts->mainline)
-			return error(_("Commit %s is a merge but no -m option was given."),
-				sha1_to_hex(commit->object.sha1));
+			return error(_("Commit %s... is a merge but no -m option was given."),
+				find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
 
 		for (cnt = 1, p = commit->parents;
 		     cnt != opts->mainline && p;
 		     cnt++)
 			p = p->next;
 		if (cnt != opts->mainline || !p)
-			return error(_("Commit %s does not have parent %d"),
-				sha1_to_hex(commit->object.sha1), opts->mainline);
+			return error(_("Commit %s... does not have parent %d"),
+				find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
+				opts->mainline);
 		parent = p->item;
 	} else if (0 < opts->mainline)
-		return error(_("Mainline was specified but commit %s is not a merge."),
-			sha1_to_hex(commit->object.sha1));
+		return error(_("Mainline was specified but commit %s... is not a merge."),
+			find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
 	else
 		parent = commit->parents->item;
 
-- 
2.4.0.228.gc627b12

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Eric Sunshine]

On Fri, May 8, 2015 at 12:53 PM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
> sequencer.c: abbreviate hashs placed in the middle of messages

s/hashs/hashes/

> Printing a 40 character sha1 hash in the middle of a message
> stretches the sentence a lot. Print the abbreviated version
> instead.
>
> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
> diff --git a/sequencer.c b/sequencer.c
> index c4f4b7d..2a39ab6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -498,20 +498,21 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>                 struct commit_list *p;
>
>                 if (!opts->mainline)
> -                       return error(_("Commit %s is a merge but no -m option was given."),
> -                               sha1_to_hex(commit->object.sha1));
> +                       return error(_("Commit %s... is a merge but no -m option was given."),
> +                               find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));

Are short SHA1's followed by "..." anywhere else in the project? It
seems strange to introduce such usage here.

>                 for (cnt = 1, p = commit->parents;
>                      cnt != opts->mainline && p;
>                      cnt++)
>                         p = p->next;
>                 if (cnt != opts->mainline || !p)
> -                       return error(_("Commit %s does not have parent %d"),
> -                               sha1_to_hex(commit->object.sha1), opts->mainline);
> +                       return error(_("Commit %s... does not have parent %d"),
> +                               find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
> +                               opts->mainline);
>                 parent = p->item;
>         } else if (0 < opts->mainline)
> -               return error(_("Mainline was specified but commit %s is not a merge."),
> -                       sha1_to_hex(commit->object.sha1));
> +               return error(_("Mainline was specified but commit %s... is not a merge."),
> +                       find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
>         else
>                 parent = commit->parents->item;
>
> --
> 2.4.0.228.gc627b12
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Ralf Thielow]

Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Fri, May 8, 2015 at 12:53 PM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
>> sequencer.c: abbreviate hashs placed in the middle of messages
>
> s/hashs/hashes/

Thanks

>
>> Printing a 40 character sha1 hash in the middle of a message
>> stretches the sentence a lot. Print the abbreviated version
>> instead.
>>
>> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
>> ---
>> diff --git a/sequencer.c b/sequencer.c
>> index c4f4b7d..2a39ab6 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -498,20 +498,21 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>>                 struct commit_list *p;
>>
>>                 if (!opts->mainline)
>> -                       return error(_("Commit %s is a merge but no -m option was given."),
>> -                               sha1_to_hex(commit->object.sha1));
>> +                       return error(_("Commit %s... is a merge but no -m option was given."),
>> +                               find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
>
> Are short SHA1's followed by "..." anywhere else in the project? It
> seems strange to introduce such usage here.
>

Yes, that's why I used it.

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 8, 2015 at 12:53 PM, Ralf Thielow <ralf.thielow@gmail.com> wrote:
>> sequencer.c: abbreviate hashs placed in the middle of messages
>
> s/hashs/hashes/
> ...
>> + return error(_("Commit %s... is a merge but no -m option was
>> given."),
>> +                               find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV));
>
> Are short SHA1's followed by "..." anywhere else in the project? It
> seems strange to introduce such usage here.

There are (and used to be the norm), as in "git diff --raw", for
example.

But I doubt the value of pointing out exact commit in the first
place, which leads me to say that "no -m option was given but
history has a merge" might be a viable alternative.

If identifying the exact commit has value, on the other hand, we can
rephrase it like this:

	error(_("no -m option was given to pick a merge '%s'", ...));

to place it not in the middle.  We can do similar rephrasing for
other messages as well.

>> -                       return error(_("Commit %s does not have parent %d"),
>> -                               sha1_to_hex(commit->object.sha1), opts->mainline);

	error(_("No parent %d for commit '%s'", opts->mainline, ...);

>> -               return error(_("Mainline was specified but commit %s is not a merge."),
>> -                       sha1_to_hex(commit->object.sha1));

	error(_("-m option was given for non-merge commit '%s'", ...);

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Ralf Thielow]

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

> If identifying the exact commit has value, on the other hand, we can
> rephrase it like this:
>
>         error(_("no -m option was given to pick a merge '%s'", ...));
>
> to place it not in the middle.  We can do similar rephrasing for
> other messages as well.
>
>>> -                       return error(_("Commit %s does not have parent %d"),
>>> -                               sha1_to_hex(commit->object.sha1), opts->mainline);
>
>         error(_("No parent %d for commit '%s'", opts->mainline, ...);
>
>>> -               return error(_("Mainline was specified but commit %s is not a merge."),
>>> -                       sha1_to_hex(commit->object.sha1));
>
>         error(_("-m option was given for non-merge commit '%s'", ...);

The %s can be placed to somewhere else in translations.

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

>>         error(_("-m option was given for non-merge commit '%s'", ...);
>
> The %s can be placed to somewhere else in translations.

That is true for all the messages, and is something the l10n people
need to be aware of.

We've seen those who are careful to keep '%s' for longer strings
(e.g. end-user controlled ones strings) at the end for this exact
reason. I do not offhand recall if there were cases where we
committed one version that had %s in the middle and then fixed
later to change that message in a separate commit, but there were
cases where earlier messages with %s in the middle were fixed for
this exact reason in their rerolls.

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Ralf Thielow]

Junio C Hamano <gitster@pobox.com> wrote:
> But I doubt the value of pointing out exact commit in the first
> place, which leads me to say that "no -m option was given but
> history has a merge" might be a viable alternative.
>
> If identifying the exact commit has value, on the other hand, we can
> rephrase it like this:
>

It has value since you see the hash and can check if you have
passed a wrong commit accidently.

I don't see why you suggest to rephrase the messages over an
abbreviation of the hash. Is it because I wrote "in the middle of"?
I think it's a good change anyway.

>         error(_("no -m option was given to pick a merge '%s'", ...));
>
> to place it not in the middle.  We can do similar rephrasing for
> other messages as well.
>
>>> -                       return error(_("Commit %s does not have parent %d"),
>>> -                               sha1_to_hex(commit->object.sha1), opts->mainline);
>
>         error(_("No parent %d for commit '%s'", opts->mainline, ...);
>
>>> -               return error(_("Mainline was specified but commit %s is not a merge."),
>>> -                       sha1_to_hex(commit->object.sha1));
>
>         error(_("-m option was given for non-merge commit '%s'", ...);

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>> But I doubt the value of pointing out exact commit in the first
>> place, which leads me to say that "no -m option was given but
>> history has a merge" might be a viable alternative.
>>
>> If identifying the exact commit has value, on the other hand, we can
>> rephrase it like this:
>
> It has value since you see the hash and can check if you have
> passed a wrong commit accidently.

Let's say by mistake I applied your patch while I had 'master'
checked out, and I want to cherry-pick it to its own branch, by
doing something like

    $ git checkout -b rt/sequencer-error-messages maint
    $ git cherry-pick <branch-name-here>

and typed next by mistake instead of master in the above.  I should
get "that commit is a merge but you didn't tell me relative to which
of its parents you want the changes ported".

Does it make it any clearer to say "commit 38e707... is a merge"
compared to "commit you gave me is a merge", with or without
abbreviation?

I do not think so.  I know I said "git cherry-pick next".

> I don't see why you suggest to rephrase the messages over an
> abbreviation of the hash. Is it because I wrote "in the middle of"?

No.

I am merely making sure that the original problem is well analysed
and we looked at other possibilities before picking one, i.e. making
sure that we did not pick the one merely because it happened to be
an expedient thing to do.

And shortening the output feels to me a more expedient thing to do
because we do not have to analyse the ramification of possible
information loss (your "It has value since...").  If we analyse the
issue well, we might realize that there is little point in showing
the commit object name in hexadecimal, be it in full or in shortened
form.

If the proposal were to parrot what the user typed on the command
line, e.g. one of these (or their rephrased versions)

	error: commit 'next' is a merge but no -m option was given.
	error: no -m option was given to pick a merge 'next'.

against the above example, then I would say that would be an
improvement, but that is not what is being discussed, so...

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Ralf Thielow]

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

> And shortening the output feels to me a more expedient thing to do
> because we do not have to analyse the ramification of possible
> information loss (your "It has value since...").  If we analyse the
> issue well, we might realize that there is little point in showing
> the commit object name in hexadecimal, be it in full or in shortened
> form.
>
> If the proposal were to parrot what the user typed on the command
> line, e.g. one of these (or their rephrased versions)
>
>         error: commit 'next' is a merge but no -m option was given.
>         error: no -m option was given to pick a merge 'next'.
>
> against the above example, then I would say that would be an
> improvement, but that is not what is being discussed, so...

Commits to be merged don't have to be branches.

OK. So let's stay with 40 character hash in the messages.

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> wrote:
>
>> And shortening the output feels to me a more expedient thing to do
>> because we do not have to analyse the ramification of possible
>> information loss (your "It has value since...").  If we analyse the
>> issue well, we might realize that there is little point in showing
>> the commit object name in hexadecimal, be it in full or in shortened
>> form.
>>
>> If the proposal were to parrot what the user typed on the command
>> line, e.g. one of these (or their rephrased versions)
>>
>>         error: commit 'next' is a merge but no -m option was given.
>>         error: no -m option was given to pick a merge 'next'.
>>
>> against the above example, then I would say that would be an
>> improvement, but that is not what is being discussed, so...
>
> Commits to be merged don't have to be branches.

I am not sure how that changes anything.

    $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
    error: that commit is a merge and you didn't give me -m <which-parent>

    $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
    error: the commit 38e707... is a merge and you didn't give me -m <which-parent>

Puzzled.

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Ralf Thielow]

Junio C Hamano <gitster@pobox.com> wrote:
>
> I am not sure how that changes anything.
>
>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>     error: that commit is a merge and you didn't give me -m <which-parent>
>
>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>     error: the commit 38e707... is a merge and you didn't give me -m <which-parent>
>
> Puzzled.

It doesn't change anything, obviously.
The patch was about showing sha1 in 40 character or 10 characters or so,
in the current messages.

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Matthieu Moy]

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

> I am not sure how that changes anything.
>
>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>     error: that commit is a merge and you didn't give me -m <which-parent>
>
>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>     error: the commit 38e707... is a merge and you didn't give me -m <which-parent>

But ...

    ./myscript.sh
    error: that commit is a merge and you didn't give me -m <which-parent>

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> I am not sure how that changes anything.
>>
>>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>>     error: that commit is a merge and you didn't give me -m <which-parent>
>>
>>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>>     error: the commit 38e707... is a merge and you didn't give me -m <which-parent>
>
> But ...
>
>     ./myscript.sh
>     error: that commit is a merge and you didn't give me -m <which-parent>

If myscript.sh did not take the user input, what would the first
thing you (who tried to run the script) would do?  At that point,
figuring out which wrong commit was fed to underlying cherry-pick
becomes a lot less important issue than figuring out _why_ the buggy
script fed a wrong commit to it, doesn't it?

Re: [PATCH] sequencer.c: abbreviate hashs placed in the middle of messages

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> I am not sure how that changes anything.
>>>
>>>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>>>     error: that commit is a merge and you didn't give me -m <which-parent>
>>>
>>>     $ git cherry-pick 38e70713119c25ab5699df6b2fb13e4133d399ab
>>>     error: the commit 38e707... is a merge and you didn't give me -m <which-parent>
>>
>> But ...
>>
>>     ./myscript.sh
>>     error: that commit is a merge and you didn't give me -m <which-parent>
>
> If myscript.sh did not take the user input, what would the first
> thing you (who tried to run the script) would do?  At that point,
> figuring out which wrong commit was fed to underlying cherry-pick
> becomes a lot less important issue than figuring out _why_ the buggy
> script fed a wrong commit to it, doesn't it?

True, but knowing which commit was fed to cherry-pick may explain why it
did so. Sure, a developer could find out, but a user asking a question
or reporting an issue is more effective when the error message contains
as much information as possible.

(Admittedly, in this senario, I should have written
./someone-else-s-script.sh ;-) )

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/