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.
