Re: [PATCH] branch: description for non-existent branch errors

["Rubén Justo" <rjusto@gmail.com>]

On 26/9/22 20:12, Junio C Hamano wrote:

Thank you for the review. I will do a reroll with your comments, but about..

>> +	if (copy && !ref_exists(oldref.buf)) {
>> +		if (!strcmp(head, oldname))
>> +			die(_("No commit on branch '%s' yet."), oldname);
>> +		else
>> +			die(_("No branch named '%s'."), oldname);
>> +	}
> 
> Let's not make it worse by starting the die() message with capital
> letters, even though the existing "git branch" error messages are
> already mixture that they need to be cleaned up later.
> 

I chose these literals for the errors because they are already translated,
appear below in that same file. I thought that would make the patch easier to
review and apply, for the translators too.

Maybe we can maintain those capitalized literals to have a simpler patch to
merge and leave the "mixtures" for a posterior patch.  I have already 
identified a leak in that command:

	static const char *head;
	...
	int cmd_branch()
	...
		head = resolve_refdup("HEAD", 0, &head_oid, NULL);

"head" is leaked but there is no easy free, because some exists are like:

		if (delete) {
			if (!argc)
				die(_("branch name required"));
			return delete_branches(argc, argv, delete > 1, filter.kind, quiet);

Without entering in this too much, maybe an atexit() approach, as in some
others commands... but maybe a more clear flow.. and that would need some
changes that can carry out the "mixtures".

Anyway, if you think there is no problem with the new literal in that file,
I will add it to the reroll with the rest of the comments in your review.

I pointed out in the first mail of this thread, there is already a patch in
'seen' that touches builtin/branch.c [1].  I would like to keep the patches
separated, but I don't know how to proceed: make the change from 'seen', keep
it from 'master'... Maybe you can give me some guidance in this.

Thank you.

Un saludo.