Re: [PATCH 1/1] branch.c: ammend error messages for die()

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

>> @@ -640,16 +640,16 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>>  	if (!copy && (oldref_usage & IS_HEAD) &&
>>  	    replace_each_worktree_head_symref(worktrees, oldref.buf, newref.buf,
>>  					      logmsg.buf))
>> -		die(_("Branch renamed to %s, but HEAD is not updated!"), newname);
>> +		die(_("branch renamed to %s, but HEAD is not updated!"), newname);
>
> IMO we can go further here and also remove that final "!".  But it's OK
> the way you have done it.

Thanks for good eyes.  I do not think '!' is adding any value in
this case, so removing it is probably a good idea, even if it is
done outside the scope of this patch.

>>  		else if ((argc == 1) && filter.detached)
>> -			die(copy? _("cannot copy the current branch while not on any.")
>> +			die(copy? _("cannot copy the current branch while not on any")
>>  				: _("cannot rename the current branch while not on any."));
>
> OK.  But I think you want to modify also the second message, to remove
> its full stop as well.

Nice spotting.

>> @@ -965,11 +965,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>>  		const char *start_name = argc == 2 ? argv[1] : head;
>>  
>>  		if (filter.kind != FILTER_REFS_BRANCHES)
>> -			die(_("The -a, and -r, options to 'git branch' do not take a branch name.\n"
>> +			die(_("the -a, and -r, options to 'git branch' do not take a branch name\n"
>>  				  "Did you mean to use: -a|-r --list <pattern>?"));
>
> Good; the full stop removed and here that question mark is valuable.  And ...

This one is not a single sentence, so retaining the full stop at the
end of the first sentence would actually be good, in addition to the
capitalization of the second sentence.

In a modern world, I would suspect that we would use die_message()
interface to emit the first sentence, show the "Did you mean..."
with advice_if_enabled(), and then exit with the status returned by
die_message(), similar to how branch.c::setup_tracking() does.  When
it happens, the die() message will be only the first sentence, and
what this patch did can be retained.  The second message will have
to be reworked to make it more appropriate as an advice message.

Thanks for a prompt review.