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

From: Rubén Justo <rjusto@gmail.com>
Date: Sep 26, 2022, 23:35 UTC
Message-ID: <26a5cbe2-d821-e7f6-e56f-4ad90ef2cf2d@gmail.com>
In-Reply-To: <xmqqleq6ovh4.fsf@gitster.g>

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

Show 11 quoted lines

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