patchMake git rebase -r's label generation more resilient

> From: Matt R <mattr94@gmail.com>
> 
> The `label` todo command in interactive rebases creates temporary refs
> in the `refs/rewritten/` namespace. These refs are stored as loose refs,
> i.e. as files in `.git/refs/rewritten/`, therefore they have to conform
> with file name limitations on the current filesystem.
> 
> This poses a problem in particular on NTFS/FAT, where e.g. the colon
> character is not a valid part of a file name.

> Let's safeguard against this by replacing not only white-space
> characters by dashes, but all non-alpha-numeric ones.
> 
> However, we exempt non-ASCII UTF-8 characters from that, as it should be
> quite possible to reflect branch names such as `↯↯↯` in refs/file names.
> 
> Signed-off-by: Matthew Rogers <mattr94@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   sequencer.c              | 12 +++++++++++-
>   t/t3430-rebase-merges.sh |  5 +++++
>   2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 34ebf8ed94..23f4a0876a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>   		else
>   			strbuf_addbuf(&label, &oneline);
>   
> +		/*
> +		 * Sanitize labels by replacing non-alpha-numeric characters
> +		 * (including white-space ones) by dashes, as they might be
> +		 * illegal in file names (and hence in ref names).
> +		 *
> +		 * Note that we retain non-ASCII UTF-8 characters (identified
> +		 * via the most significant bit). They should be all acceptable
> +		 * in file names. We do not validate the UTF-8 here, that's not
> +		 * the job of this function.
> +		 */
>   		for (p1 = label.buf; *p1; p1++)
> -			if (isspace(*p1))
> +			if (!(*p1 & 0x80) && !isalnum(*p1))
>   				*(char *)p1 = '-';

>   		strbuf_reset(&buf);
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 7b6c4847ad..737396f944 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -441,4 +441,9 @@ test_expect_success '--continue after resolving conflicts after a merge' '
>   	test_path_is_missing .git/MERGE_HEAD
>   '
>   
> +test_expect_success '--rebase-merges with commit that can generate bad characters for filename' '
> +	git checkout -b colon-in-label E &&
> +	git merge -m "colon: this should work" G &&
> +	git rebase --rebase-merges --force-rebase E
> +'
>   test_done
> 

> Being picking I'll point out that ':' is not a valid in refs
> either. Looking at
> https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I
> think only " and | are not allowed on NTFS/FAT but are valid in refs
> (see the man page for git check-ref-format for all the details). So
> the main limitation is actually what git allows in refs.

>>   		for (p1 = label.buf; *p1; p1++)
>> -			if (isspace(*p1))
>> +			if (!(*p1 & 0x80) && !isalnum(*p1))
>>   				*(char *)p1 = '-';
>
> I'm sightly concerned that this opens the possibility for unexpected
> effects if two different labels get sanitized to the same string. I
> suspect it's unlikely to happen in practice but doing something like
> percent encoding non-alphanumeric characters would avoid the problem
> entirely.

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > Being picking I'll point out that ':' is not a valid in refs
> > either. Looking at
> > https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I
> > think only " and | are not allowed on NTFS/FAT but are valid in refs
> > (see the man page for git check-ref-format for all the details). So
> > the main limitation is actually what git allows in refs.
> 
> Yeah, trying to use the contents of the log message without
> sufficient sanitization is looking for trouble.
> 
> >>   		for (p1 = label.buf; *p1; p1++)
> >> -			if (isspace(*p1))
> >> +			if (!(*p1 & 0x80) && !isalnum(*p1))
> >>   				*(char *)p1 = '-';

> > I'm sightly concerned that this opens the possibility for unexpected
> > effects if two different labels get sanitized to the same string. I
> > suspect it's unlikely to happen in practice but doing something like
> > percent encoding non-alphanumeric characters would avoid the problem
> > entirely.
> 
> I see there are "lets make sure it is unique by suffixing "-%d" in
> other codepaths; would that help if this piece of code yields a
> label that is not unique?

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >>   		for (p1 = label.buf; *p1; p1++)
> >> -			if (isspace(*p1))
> >> +			if (!(*p1 & 0x80) && !isalnum(*p1))
> >>   				*(char *)p1 = '-';
> >
> > I'm sightly concerned that this opens the possibility for unexpected
> > effects if two different labels get sanitized to the same string. I
> > suspect it's unlikely to happen in practice but doing something like
> > percent encoding non-alphanumeric characters would avoid the problem
> > entirely.
>
> I'd rather see 'x' used instead of '-' (double-or-more dashes and
> leading dash in refnames may currently be allowed but double-or-more
> exes and leading ex would be much more likely to stay valid) if we
> just want to redact invalid characters.

> If you care deeply about double dashes and leading dashes, how about
> this instead?
>
> 		char *from, *to;
>
>    		for (from = to = label.buf; *from; from++)
> 			if ((*from & 0x80) || isalnum(*from))
> 				*(to++) = *from;
> 			/* avoid leading dash and double-dashes */
> 			else if (to != label.buf && to[-1] != '-')
>    				*(to++) = '-';
> 		strbuf_setlen(&label, to - label.buf);

>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > If you care deeply about double dashes and leading dashes, how about
> > this instead?
> >
> >               char *from, *to;
> >
> >               for (from = to = label.buf; *from; from++)
> >                       if ((*from & 0x80) || isalnum(*from))
> >                               *(to++) = *from;
> >                       /* avoid leading dash and double-dashes */
> >                       else if (to != label.buf && to[-1] != '-')
> >                               *(to++) = '-';
> >               strbuf_setlen(&label, to - label.buf);
>
> Simple enough and is a good change when judged locally.
>
> It still would cause readers to wonder if label_oid() later append
> '-%d' to end up with double-dash near the end, etc., which made me
> wonder if the resulting code becomes better if sanitization and
> uniquefying are done at the same single place in the other message.

> > The `label` todo command in interactive rebases creates temporary refs
> > in the `refs/rewritten/` namespace. These refs are stored as loose refs,
> > i.e. as files in `.git/refs/rewritten/`, therefore they have to conform
> > with file name limitations on the current filesystem.
> >
> > This poses a problem in particular on NTFS/FAT, where e.g. the colon
> > character is not a valid part of a file name.
>
> Being picking I'll point out that ':' is not a valid in refs either.

> Looking at
> https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file I
> think only " and | are not allowed on NTFS/FAT but are valid in refs
> (see the man page for git check-ref-format for all the details). So
> the main limitation is actually what git allows in refs.

> > Let's safeguard against this by replacing not only white-space
> > characters by dashes, but all non-alpha-numeric ones.
> >
> > However, we exempt non-ASCII UTF-8 characters from that, as it should be
> > quite possible to reflect branch names such as `↯↯↯` in refs/file names.
> >
> > Signed-off-by: Matthew Rogers <mattr94@gmail.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >   sequencer.c              | 12 +++++++++++-
> >   t/t3430-rebase-merges.sh |  5 +++++
> >   2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 34ebf8ed94..23f4a0876a 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -4635,8 +4635,18 @@ static int make_script_with_merges(struct
> > pretty_print_context *pp,
> >     else
> >      strbuf_addbuf(&label, &oneline);
> >   +		/*
> > +		 * Sanitize labels by replacing non-alpha-numeric characters
> > +		 * (including white-space ones) by dashes, as they might be
> > +		 * illegal in file names (and hence in ref names).
> > +		 *
> > +		 * Note that we retain non-ASCII UTF-8 characters (identified
> > +		 * via the most significant bit). They should be all
> > acceptable
> > +		 * in file names. We do not validate the UTF-8 here, that's
> > not
> > +		 * the job of this function.
> > +		 */
> >   		for (p1 = label.buf; *p1; p1++)
> > -			if (isspace(*p1))
> > +			if (!(*p1 & 0x80) && !isalnum(*p1))
> >       *(char *)p1 = '-';
>
> I'm sightly concerned that this opens the possibility for unexpected effects
> if two different labels get sanitized to the same string. I suspect it's
> unlikely to happen in practice but doing something like percent encoding
> non-alphanumeric characters would avoid the problem entirely.

>> I'm sightly concerned that this opens the possibility for unexpected effects
>> if two different labels get sanitized to the same string. I suspect it's
>> unlikely to happen in practice but doing something like percent encoding
>> non-alphanumeric characters would avoid the problem entirely.
>
> Oh, but we make sure that the labels are unique, via the `label_oid()`
> function! Otherwise, we would not be able to label more than one merge
> parent ;-)

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> I'm sightly concerned that this opens the possibility for unexpected effects
> >> if two different labels get sanitized to the same string. I suspect it's
> >> unlikely to happen in practice but doing something like percent encoding
> >> non-alphanumeric characters would avoid the problem entirely.
> >
> > Oh, but we make sure that the labels are unique, via the `label_oid()`
> > function! Otherwise, we would not be able to label more than one merge
> > parent ;-)
>
> It somewhat feels suboptimal, from code followability's point of
> view, to have this "pre-sanitization" to replace isspace() to a
> dash, which is being extended to "all non-alnums", and the uniquefy
> of labels in label_oid(), in two separate places.  I wonder if the
> resulting code becomes easier to follow and harder to introduce new
> bugs, if this part is made to just yield label.buf it obtained form
> the log message as-is and leave the munging to label_oid()?

> Those labels must be valid ref names, and therefore valid file names. The
> initial patch came in via Git for Windows.
>
> Change since v1:
>
>  * moved the entire sanitizing logic to label_oid(), as a preparatory step.
>
> Johannes Schindelin (1):
>   rebase-merges: move labels' whitespace mangling into `label_oid()`
>
> Matthew Rogers (1):
>   rebase -r: let `label` generate safer labels
>
>  sequencer.c              | 72 +++++++++++++++++++++++++---------------
>  t/t3430-rebase-merges.sh |  6 ++++
>  2 files changed, 51 insertions(+), 27 deletions(-)

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > Those labels must be valid ref names, and therefore valid file names. The
> > initial patch came in via Git for Windows.
> >
> > Change since v1:
> >
> >  * moved the entire sanitizing logic to label_oid(), as a preparatory step.
> >
> > Johannes Schindelin (1):
> >   rebase-merges: move labels' whitespace mangling into `label_oid()`
> >
> > Matthew Rogers (1):
> >   rebase -r: let `label` generate safer labels
> >
> >  sequencer.c              | 72 +++++++++++++++++++++++++---------------
> >  t/t3430-rebase-merges.sh |  6 ++++
> >  2 files changed, 51 insertions(+), 27 deletions(-)
> 
> I think Dscho meant to Cc you as these two patches are meant to be a
> more complete solution to supersede your [*1*].

> In order to work correctly, git-rebase --rebase-merges needs to make
> initial todo list with unique labels.
>
> Those unique labels is being handled by employing a hashmap and
> appending an unique number if any duplicate is found.
>
> But, we forget that beside those labels for side branches,
> we also have a special label `onto' for our so-called new-base.
>
> In a special case that any of those labels for side branches named
> `onto', git will run into trouble.
>
> Correct it.
>
> Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
> ---

> Sorry for the noise, I forgot to check spelling for v1
>
> And I forgot to delete From line when append to my MUA.
>
> Range-diff against v1:
> 1:  48205889b4 ! 1:  9246beacf2 sequencer: handle rebase-merge for "onto" message
>     @@ Metadata
>      Author: Doan Tran Cong Danh <congdanhqx@gmail.com>
>
>       ## Commit message ##
>     -    sequencer: handle rebase-merge for "onto" message
>     +    sequencer: handle rebase-merges for "onto" message
>
>          In order to work correctly, git-rebase --rebase-merges needs to make
>          initial todo list with unique labels.
>
>          Those unique labels is being handled by employing a hashmap and
>     -    suffixing an unique number if any duplicate is found.
>     +    appending an unique number if any duplicate is found.
>
>     -    But we forgat that beside of those labels for side branches,
>     -    we also make a special label `onto' for our so-called new-base.
>     +    But, we forget that beside those labels for side branches,
>     +    we also have a special label `onto' for our so-called new-base.
>
>          In a special case that any of those labels for side branches named
>          `onto', git will run into trouble.
>
>  sequencer.c              |  5 +++++
>  t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 350045b1b4..fc81e43f0f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4569,10 +4569,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  	strbuf_init(&state.buf, 32);
>
>  	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
> +		struct labels_entry *onto_label_entry;
>  		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
>  		FLEX_ALLOC_STR(entry, string, "onto");
>  		oidcpy(&entry->entry.oid, oid);
>  		oidmap_put(&state.commit2label, entry);
> +
> +		FLEX_ALLOC_STR(onto_label_entry, label, "onto");
> +		hashmap_entry_init(&onto_label_entry->entry, strihash("onto"));
> +		hashmap_add(&state.labels, &onto_label_entry->entry);
>  	}
>
>  	/*
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index f728aba995..4e2c0ede51 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -474,4 +474,25 @@ test_expect_success '--rebase-merges with commit that can generate bad character
>  	git rebase --rebase-merges --force-rebase E
>  '
>
> +test_expect_success '--rebase-merges with message matched with onto label' '
> +	git checkout -b onto-label E &&
> +	git merge -m onto G &&
> +	git rebase --rebase-merges --force-rebase E &&
> +	test_cmp_graph <<-\EOF
> +	*   onto
> +	|\
> +	| * G
> +	| * F
> +	* |   E
> +	|\ \
> +	| * | B
> +	* | | D
> +	| |/
> +	|/|
> +	* | C
> +	|/
> +	* A
> +	EOF
> +'
> +
>  test_done
> --
> 2.24.0.10.gc61c3b979f.dirty
>
>

> Anyway, when reading their patch, I discovered a problem with
> git rebase --rebase-merges when its message is onto.
>
> Here is a patch to fix it.
>
> I applied Dscho's patches over my dd/sequencer-utf8 then writing this
> patch, in case you have problem applying it.
>
> The context for the diff is coming from Dscho's patches.

> -------8<--------------------
> From 48205889b404b82baa4b30c2eedd52243c349e3e Mon Sep 17 00:00:00 2001
> From: Doan Tran Cong Danh <congdanhqx@gmail.com>
> Date: Mon, 18 Nov 2019 18:02:05 +0700
> Subject: [PATCH] sequencer: handle rebase-merge for "onto" message
>
> In order to work correctly, git-rebase --rebase-merges needs to make
> initial todo list with unique labels.
>
> Those unique labels is being handled by employing a hashmap and
> suffixing an unique number if any duplicate is found.
>
> But we forgat that beside of those labels for side branches,
> we also make a special label `onto' for our so-called new-base.
>
> In a special case that any of those labels for side branches named
> `onto', git will run into trouble.
>
> Correct it.
>
> Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
> ---
>  sequencer.c              |  5 +++++
>  t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index 350045b1b4..fc81e43f0f 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4569,10 +4569,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  	strbuf_init(&state.buf, 32);
>  
>  	if (revs->cmdline.nr && (revs->cmdline.rev[0].flags & BOTTOM)) {
> +		struct labels_entry *onto_label_entry;
>  		struct object_id *oid = &revs->cmdline.rev[0].item->oid;
>  		FLEX_ALLOC_STR(entry, string, "onto");
>  		oidcpy(&entry->entry.oid, oid);
>  		oidmap_put(&state.commit2label, entry);
> +
> +		FLEX_ALLOC_STR(onto_label_entry, label, "onto");
> +		hashmap_entry_init(&onto_label_entry->entry, strihash("onto"));
> +		hashmap_add(&state.labels, &onto_label_entry->entry);
>  	}
>  
>  	/*
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index f728aba995..4e2c0ede51 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -474,4 +474,25 @@ test_expect_success '--rebase-merges with commit that can generate bad character
>  	git rebase --rebase-merges --force-rebase E
>  '
>  
> +test_expect_success '--rebase-merges with message matched with onto label' '
> +	git checkout -b onto-label E &&
> +	git merge -m onto G &&
> +	git rebase --rebase-merges --force-rebase E &&
> +	test_cmp_graph <<-\EOF
> +	*   onto
> +	|\
> +	| * G
> +	| * F
> +	* |   E
> +	|\ \
> +	| * | B
> +	* | | D
> +	| |/
> +	|/|
> +	* | C
> +	|/
> +	* A
> +	EOF
> +'
> +
>  test_done

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > Those labels must be valid ref names, and therefore valid file names. The
> > initial patch came in via Git for Windows.
> >
> > Change since v1:
> >
> >  * moved the entire sanitizing logic to label_oid(), as a preparatory step.
> >
> > Johannes Schindelin (1):
> >   rebase-merges: move labels' whitespace mangling into `label_oid()`
> >
> > Matthew Rogers (1):
> >   rebase -r: let `label` generate safer labels
> >
> >  sequencer.c              | 72 +++++++++++++++++++++++++---------------
> >  t/t3430-rebase-merges.sh |  6 ++++
> >  2 files changed, 51 insertions(+), 27 deletions(-)
>
> I think Dscho meant to Cc you as these two patches are meant to be a
> more complete solution to supersede your [*1*].

>
>
> [Reference]
>
> *1* <860dee65f49ea7eacf5a0c7c8ffe59095a51b1ce.1573205699.git.congdanhqx@gmail.com>
>

> I can redo the commit, I had thought that I had previously fixed the 
> author but I guess I was mistaken.
>
> As for issues with non utf-8 encodings, I don't know of any simple way 
> to check for those except for restricting to asci alphanumeric characters

>
>
>
> On Mon, Sep 2, 2019, 5:24 PM Philip Oakley <philipoakley@iee.email> wrote:
>
>     On 02/09/2019 19:29, Junio C Hamano wrote:
>     > I see there are "lets make sure it is unique by suffixing "-%d" in
>     > other codepaths; would that help if this piece of code yields a
>     > label that is not unique?
>     maybe use a trailing 4 characters  of the oid to get a reasonably
>     unique
>     label?
>
>     Oh, just seen dscho's "we make sure that the labels are unique,
>     via the
>     `label_oid()` function!", maybe needs mentioning in the commit
>     message
>     if re-rolled.
>
>     Philip
>