Make git rebase -r's label generation more resilient

From: Matt R <[email protected]>

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
  sequencer.c              | 12 +++++++++++-
  t/t3430-rebase-merges.sh |  5 +++++
  2 files changed, 16 insertions(+), 1 deletion(-)

sequencer.c +4 -0

index 34ebf8ed94..23f4a0876a 100644
@@ -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).

sequencer.c +0 -0

  		strbuf_reset(&buf);

t/t3430-rebase-merges.sh +5 -0

index 7b6c4847ad..737396f944 100755
@@ -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.

sequencer.c static int make_script_with_merges(struct pretty_print_context *pp, · line 4650

Quoted 4 lines

for (p1 = label.buf; *p1; p1++)

sequencer.c +1 -1

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

This is definitely worth fixing, I've got a couple of comments below

Quoted 1 line · Matt R via GitGitGadget

From: Matt R <[email protected]>

Quoted 7 lines

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.

Quoted 34 lines

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

---
  sequencer.c              | 12 +++++++++++-
  t/t3430-rebase-merges.sh |  5 +++++
  2 files changed, 16 insertions(+), 1 deletion(-)

sequencer.c +6 -0

index 34ebf8ed94..23f4a0876a 100644
@@ -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

sequencer.c +1 -0

+		 * in file names. We do not validate the UTF-8 here, that's

not

sequencer.c +2 -0

+		 * the job of this function.
+		 */
  		for (p1 = label.buf; *p1; p1++)

sequencer.c static int make_script_with_merges(struct pretty_print_context *pp, · line 4650

Quoted 3 lines

sequencer.c +1 -1

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

Quoted 6 lines · Phillip Wood

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.

Quoted 4 lines

sequencer.c static int make_script_with_merges(struct pretty_print_context *pp, · line 4650

Quoted 4 lines

for (p1 = label.buf; *p1; p1++)

sequencer.c +1 -1

-			if (isspace(*p1))
+			if (!(*p1 & 0x80) && !isalnum(*p1))
  				*(char *)p1 = '-';

Quoted 5 lines

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?

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?

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

Also the code given doesn't resolve onelines that consist only of restricted file names (e.g. COM, NUL, etc. On windows)

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

Quoted 10 lines · Phillip Wood

sequencer.c static int make_script_with_merges(struct pretty_print_context *pp, · line 4650

Quoted 4 lines

for (p1 = label.buf; *p1; p1++)

sequencer.c +1 -1

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

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?

Quoted 4 lines

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 ;-)

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);

Quoted 12 lines · Johannes Schindelin

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.

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" <[email protected]>

Quoted 13 lines · 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

Quoted 3 lines

 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 <[email protected]>

---

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 <[email protected]>

## 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(+)

sequencer.c +5 -0

index 350045b1b4..fc81e43f0f 100644
@@ -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);
 	}
 
 	/*

t/t3430-rebase-merges.sh +21 -1

index f728aba995..4e2c0ede51 100755
@@ -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

Quoted 8 lines · Johannes Schindelin

Quoted 4 lines

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()?

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 13 lines · 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

Quoted 3 lines

 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>

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 <[email protected]>
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 <[email protected]>

---
 sequencer.c              |  5 +++++
 t/t3430-rebase-merges.sh | 21 +++++++++++++++++++++
 2 files changed, 26 insertions(+)

sequencer.c +5 -0

index 350045b1b4..fc81e43f0f 100644
@@ -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);
 	}
 
 	/*

t/t3430-rebase-merges.sh +21 -0

index f728aba995..4e2c0ede51 100755
@@ -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