From: Jeff King <peff@peff.net>
Date: Mon, 11 Nov 2019 08:39:40 GMT
Subject: Re: [PATCH v6 9/9] sequencer: fallback to sane label in making rebase todo list
Message-ID: <20191111083940.GC17984@sigill.intra.peff.net>
In-Reply-To: <78daf050de8c0cdc81fed4adc8fef92d1768c63a.1573452046.git.congdanhqx@gmail.com>

On Mon, Nov 11, 2019 at 01:03:42PM +0700, Doan Tran Cong Danh wrote:

> In later stage of rebasing, we will make a ref in
> refs/rewritten/<label>, this label is extracted from the subject of
> current merge commit.
> 
> If that subject has forbidden character for refname, git couldn't create
> the rewritten ref. One such case is the merge commit subject has
> a multibyte character encoded in ISO-2022-JP.
> 
> Provide a sane fallback in order to help git-rebase works in such case

Good find. Not having worked much with this part of the sequencer code,
I don't have a strong opinion on the fallback label. But it seems better
than the current behavior would be.

I suspect there may be other subtle problems, too, for filesystems that
can't represent some part of the subject (e.g., I wonder if some of your
ISO-2022-JP tests might already have trouble on HFS+, which excepts all
paths to be UTF-8).

> @@ -4607,9 +4608,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  			if (isspace(*p1))
>  				*(char *)p1 = '-';
>  
> +		hex_oid = oid_to_hex(&commit->object.oid);
> +
> +		if (check_refname_format(label.buf, REFNAME_ALLOW_ONELEVEL) < 0) {
> +			strbuf_reset(&label);
> +			strbuf_addf(&label, "label-%s", hex_oid);
> +		}
> +
>  		strbuf_reset(&buf);
> -		strbuf_addf(&buf, "%s -C %s",
> -			    cmd_merge, oid_to_hex(&commit->object.oid));
> +		strbuf_addf(&buf, "%s -C %s", cmd_merge, hex_oid);

A minor nit, but I think it's better here to just call oid_to_hex()
twice, rather than assigning to the hex_oid variable. It's returning a
pointer to a static buffer, so holding onto the pointers for too long is
dangerous. What you have here is definitely OK, because nothing
interesting happens in between, but seeing any assignment of the result
of "oid_to_hex" makes auditing harder.

And it's not like it's that expensive, or that this is
performance-critical code (and if it were, we'd do better to use
oid_to_hex_r() directly into the strbuf anyway).

-Peff