git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] branch: error and informative messages

From
Rubén Justo <rjusto@gmail.com>
Date
Oct 24, 2022, 23:39 UTC
Message-ID
<faf7a985-f6ef-f20a-3857-031396124d60@gmail.com>
In-Reply-To
<xmqq5ygaul5k.fsf@gitster.g>
On 24/10/22 2:20, Junio C Hamano wrote:
Show 15 quoted lines
>> Beside applying this rules, some unneeded messages have been removed and
>> others reworded to normalize and simplify.
>>
>>  - Four similar messages has been merged into the most common, the last
>>    one:
>> 	- "no such branch '%s'"
>> 	- "branch '%s' does not exist"
>> 	- "invalid branch name: '%s'"
>> 	+ "no branch named '%s'"
> 
> The third one could be more appropriate than the fourth one when the
> error is about failing to create a new branch due to invalid name.
> But all the other three seem to refer to a branch that ought to
> exist but doesn't, and the one you picked seems reasonable one among
> the three (and it is also the shortest).

Yes, I had doubts with the third. The error is referring to the source branch in the copy/rename operation, so I think it makes sense to say that the branch doesn't exist, even if it couldn't.

Show 12 quoted lines
>>  - Two specific messages has been reworded and merged:
>> 	- "could not set upstream of HEAD to %s when
>> 		it does not point to any branch."
>> 	- "could not unset upstream of HEAD when it
>> 		does not point to any branch."
>> 	+ "cannot modify upstream to detached HEAD"
> 
> OK.  I have no strong opinion between "modify" and "set or unset"
> but the latter may be closer to the spirit of the former.  I agree
> that "HEAD when it does not ..." is quite a mouthful and to those
> who understand what a 'detached HEAD' is, the updated phrasing may
> be easier to read.

I prefer a single term like 'modify' as I find it less confuse than 'set or unset'.

Show 9 quoted lines
>>  - An error message reworded
>> 	- "options '%s' and '%s' cannot be used together",
>> 		"--column", "--verbose"
>> 	+ "options --column and --verbose used together"
> 
> This is arguably a regression.  The original ensures that l10n/i18n
> folks cannot frob the option names that must be literally spelled,
> but the "reworded" one lacks the protection (it also loses the quotes
> around dashed options).

OK. I didn't think about that, I'll revert to the parametrized form. About the quotes, I did that on purpose. As I said in a comment below, for branches the quotes are commonly used and make sense as it is a variable value and the quotes helps to delimit, but with options the literal is well-known and we use the form with dashes, ie --verbose, so it is easily identifiable. Also, we do not use quotes with option literals in other places, like the documentation, --help,..

Show 5 quoted lines
>>  - Two error messages not needed, removed:
>> 	- "cannot copy the current branch while not on any."
>> 	- "cannot rename the current branch while not on any."
> 
> OK (assuming that these are unreachable code).
 
It is, you reviewed it below.  I'll comment there.
Show 7 quoted lines
>>  - A deprecation message has been reworded, note the '\n':
>> 	- "the '--set-upstream' option is no longer supported."
>> 		"Please use '--track' or '--set-upstream-to' instead."
>> 	+ "option --set-upstream is no longer supported\n"
>> 		"Please use --track or --set-upstream-to instead"
> 
> Loss of quotes around option names, and the definite article "the"?
 
The loss of the article, it sounds good to me and in line with the previous
"options --column ...".  Dunno.
Show 9 quoted lines
>>  - "%s" and "'%s'" was used to format a branch name in different
>>    messages.  "'%s'" has been used to normalize as it's the more
>>    frequently used in this file and very common in the rest of the
>>    codebase.  The opposite has been done for options: "-a" used vs
>>    "'-a'".
> 
> As the way to display the exact literal string, I think it is good
> to have quotes around both of them, the user-chosen names like
> branch names, and the system-chosen names like option names.
 
Same reasoning as above.  It is a system-chosen term, but the message
has not a placeholder to put a value, we're using a literal.
Show 15 quoted lines
>> A new informative, non-error, message has been introduced for
>> create_branch:
>> 	+ "New branch '%s' created from '%s'\n"
> 
> OK.
> 
>> The tests for the modified messages have been updated and a new test for
>> the create_branch message has been added.
>>
>> Finally, let's change the return code on error for --edit-description,
>> from -1 to 1.
> 
> OK.  That last one may be better to be a separate patch, as these
> wording changes are subject to discussion and bikeshedding.
>

Mmm, I thought about that. This change is one that we've been delaying because it might break something due to a change in the way we report errors. We're specifically changing this here and the change is small, so I found appropriate to do it here.

Show 23 quoted lines
>> -	if ((head_rev != reference_rev) &&
>> -	    in_merge_bases(rev, head_rev) != merged) {
>> -		if (merged)
>> -			warning(_("deleting branch '%s' that has been merged to\n"
>> -				"         '%s', but not yet merged to HEAD."),
>> -				name, reference_name);
>> -		else
>> -			warning(_("not deleting branch '%s' that is not yet merged to\n"
>> +	if ((head_rev != reference_rev) && in_merge_bases(rev, head_rev) != merged)
>> +		warning(merged
>> +			? _("deleting branch '%s' that has been merged to\n"
>> +				"         '%s', but not yet merged to HEAD.")
>> +			: _("not deleting branch '%s' that is not yet merged to\n"
>>  				"         '%s', even though it is merged to HEAD."),
>>  				name, reference_name);
>> -	}
> 
> This does not fall into any of the categories the proposed log
> message discussed.  Rather, it looks more like "the code
> subjectively looks better this way".  It happens to much my
> subjective taste, but that does not change the fact that we
> shouldn't distract reviewers with such an unrelated change in the
> same patch.
 
It certainly looks subjectively better, and in less lines :).  That ternary
form is already used in the file,  this patch is an opportunity to uniformize
that in the file.  I did this change in two commits, a preparatory one with the
switch to the ternary op and other changes, and then the rewording.  Finally I
squased into one.  I can go back to the two patches and/or revert to the
if/else, but my preference is to keep the ternary.
In fact, I missed there the removal of two '.',  I'll fix that.
Show 46 quoted lines
>> @@ -520,13 +512,6 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int
>>  	const char *interpreted_newname = NULL;
>>  	int recovery = 0;
>>  
>> -	if (!oldname) {
>> -		if (copy)
>> -			die(_("cannot copy the current branch while not on any."));
>> -		else
>> -			die(_("cannot rename the current branch while not on any."));
>> -	}
>> -
> 
> Something like
> 
>     copy_or_rename_branch() never gets NULL in oldname, because its
>     only callers in cmd_branch() calls it with either the end-user's
>     command line argument or the result of resolve_refdup("HEAD"),
>     and neither can ever be NULL because ...
> 
> needs to go into the proposed log message to explain why it is safe
> to remove these two messages.
> 
> Having said that, the messages may actually be correct and it is the
> logic that makes it unreachable that is wrong in this case, I think.
> It looks like the code expects that oldname is NULL when we are on
> detached HEAD (it could be the old caller did have NULL in head when
> this code was written, and it is possible that we regressed over the
> course of history).  I.e. Isn't it trying to protect us against
> 
>    $ git checkout --detach HEAD^0
>    $ git branch -m foo		;# rename .git/HEAD to .git/foo???
> 
> (or "-c" for copy)?  The current code happens to catch it a bit
> later in this function, when it notices "HEAD" is not a refname in
> "refs/heads/" hierarchy, but the user never attempted to rename
> refs/heads/HEAD to refs/heads/foo, and "Invalid branch name: 'HEAD'"
> is us being wrong, insulting and confusing to the users.
> 
> I suspect that this needs to be fixed at the caller's level, just
> like the caller reacts to "edit_description" by checking the
> filter.detached bit and rejects the request, the caller should
> notice that we are on a detached HEAD and die with one of the above
> two messages without calling this function.
> 
> And that should be a separate patch, that can be reviewed and
> applied regardless of the rest of "error messages cleanup" topic.

Good point. I didn't think about that and it also goes in the line of the previous patches in this file. I'll review that. Also gives a good opportunity to fix that repeated code /... if (copy) ... else if (rename)/.

Show 8 quoted lines
> The hunks before this point that I omitted from quoting and
> commenting looked all OK.
> 
> The remainder of the patch I didn't look at.  There may be similar
> issues like this "oops, the messages are dead for a wrong reason and
> code needs to be fixed" or "let's leave subjective improvements out
> of this topic that has clearly described rules" to be discovered, so
> I may come back to them later, or others may find them before I do.
I'll prepare a reroll.
Thank you for you review.
Previous: Junio C HamanoNext: Junio C Hamano
Message 3 of 6 in “branch: error and informative messages”
  1. branch: error and informative messagesRubén Justo, Oct 23, 2022
  2. Junio C HamanoOct 24, 2022
  3. Rubén JustoOct 24, 2022
  4. Junio C HamanoOct 25, 2022
  5. Rubén JustoOct 25, 2022
  6. branch: error and informative messagesRubén Justo, Nov 6, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.