mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Rubén Justo" <>
To: Junio C Hamano <>
Subject: Re: [PATCH] branch: description for non-existent branch errors
Date: Tue, 27 Sep 2022 01:35:13 +0200	[thread overview]
Message-ID: <> (raw)
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..

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

  reply	other threads:[~2022-09-26 23:36 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-22 22:37 [PATCH] branch: description for non-existent branch errors Rubén Justo
2022-09-24 22:52 ` Rubén Justo
2022-09-26 18:12   ` Junio C Hamano
2022-09-26 23:35     ` Rubén Justo [this message]
2022-09-27 22:24       ` Junio C Hamano
2022-09-28 17:41         ` Junio C Hamano
2022-09-28 17:50         ` Junio C Hamano
2022-09-28 23:59           ` Rubén Justo
2022-09-29  1:56             ` Junio C Hamano
2022-09-30 22:47 ` [PATCH v2] " Rubén Justo
2022-10-01 12:43   ` Bagas Sanjaya
2022-10-02 21:28     ` Rubén Justo
2022-10-08  0:39   ` [PATCH v3] " Rubén Justo
2022-10-08  3:27     ` Eric Sunshine
2022-10-08  8:54       ` Rubén Justo
2022-10-09  5:05         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).