threads / patch / 48977

patch, 2 partsfix "rebase -i --root" corrupting root commit

Subject: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

## tl;dr

13 messages between Jul 30, 2018 and Jul 30, 2018. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Eric Sunshine· Jul 30, 2018, 09:29 UTC · lore

This series fixes bugs causing corruption of the root commit when "rebase -i --root" is used to swap in a new root commit. In particular, the "author" header has trailing garbage. Some tools handle the corruption somewhat gracefully by showing a bogus date, but others barf on it (gitk, for instance). git-fsck correctly identifies the corruption. I discovered this after git-rebase corrupted one of my own projects.

Unfortunately, these bugs (from js/sequencer-and-root-commits) made it into the v2.18.0 release. It's worrying that a released Git can be creating corrupt commits, but fortunately "rebase -i --root" is not likely used often (especially on well-established projects). Nevertheless, it may be 'maint' worthy and applies cleanly there.

It was only after I diagnosed and fixed these bugs that I thought to check 'pu' and discovered that Akinori MUSHA already made a stab[1] at fixing one of the three bugs which this series fixes. Akinori's fix has the somewhat undesirable property that it adds an extra blank line to the end of the script, as Phillip correctly pointed out in review[2]. Patch 2/2 of this series has the more "correct" fix, in addition to fixing another bug.

Moreover, patch 2/2 of this series provides a more thorough fix overall than Akinori, so it may make sense to replace his patch with this series, though perhaps keep the test his patch adds to augment the strict test of the "author" header added by this series.

[1]: https://public-inbox.org/git/86a7qwpt9g.knu@iDaemons.org/ [2]: https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889d06@talktalk.net/

Eric Sunshine (2):
  sequencer: fix "rebase -i --root" corrupting author header
  sequencer: fix "rebase -i --root" corrupting author header timezone
 sequencer.c                   |  9 +++++++--
 t/t3404-rebase-interactive.sh | 10 +++++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)
-- 
2.18.0.597.ga71716f1ad
Eric Sunshine· Jul 30, 2018, 09:29 UTC · re: Eric Sunshine · lore

[PATCH 1/2] sequencer: fix "rebase -i --root" corrupting author header

When "git rebase -i --root" creates a new root commit (say, by swapping in a different commit for the root), it corrupts the commit's "author" header with trailing garbage:

    author A U Thor <author@example.com> @1112912773 -07000or@example.com

This is a result of read_author_ident() neglecting to NUL-terminate the buffer into which it composes the "author" header.

(Note that the extra "0" in the timezone is a separate bug which will be fixed subsequently.)

Security considerations: Construction of the "author" header by read_author_ident() happens in-place and in parallel with parsing the content of "rebase-merge/author-script" which occupies the same buffer. This is possible because the constructed "author" header is always smaller than the content of "rebase-merge/author-script". Despite neglecting to NUL-terminate the constructed "author" header, memory is never accessed (either by read_author_ident() or its caller) beyond the allocated buffer since a NUL-terminator is present at the end of the loaded "rebase-merge/author-script" content, and additional NUL's are inserted as part of the parsing process.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)
Show changes to 2 files +10 −2

sequencer.c, t/t3404-rebase-interactive.sh

diff --git a/sequencer.c b/sequencer.c
index 16c1411054..78864d9072 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -744,7 +744,7 @@ static const char *read_author_ident(struct strbuf *buf)
 		return NULL;
 	}
 
-	buf->len = out - buf->buf;
+	strbuf_setlen(buf, out - buf->buf);
 	return buf->buf;
 }
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 01616901bd..8509c89a26 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1238,7 +1238,7 @@ rebase_setup_and_clean () {
 		test_might_fail git branch -D $1 &&
 		test_might_fail git rebase --abort
 	" &&
-	git checkout -b $1 master
+	git checkout -b $1 ${2:-master}
 }
 
 test_expect_success 'drop' '
@@ -1415,4 +1415,12 @@ test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '
 	test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
 '
 
+test_expect_success 'valid author header after --root swap' '
+	rebase_setup_and_clean author-header no-conflict-branch &&
+	set_fake_editor &&
+	FAKE_LINES="2 1" git rebase -i --root &&
+	git cat-file commit HEAD^ >out &&
+	grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$" out
+'
+
 test_done

-- 
2.18.0.597.ga71716f1ad
Johannes Schindelin· Jul 30, 2018, 15:44 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/2] sequencer: fix "rebase -i --root" corrupting author header

Hi Eric,
On Mon, 30 Jul 2018, Eric Sunshine wrote:
Show 24 quoted lines
> When "git rebase -i --root" creates a new root commit (say, by swapping
> in a different commit for the root), it corrupts the commit's "author"
> header with trailing garbage:
> 
>     author A U Thor <author@example.com> @1112912773 -07000or@example.com
> 
> This is a result of read_author_ident() neglecting to NUL-terminate the
> buffer into which it composes the "author" header.
> 
> (Note that the extra "0" in the timezone is a separate bug which will be
> fixed subsequently.)
> 
> Security considerations: Construction of the "author" header by
> read_author_ident() happens in-place and in parallel with parsing the
> content of "rebase-merge/author-script" which occupies the same buffer.
> This is possible because the constructed "author" header is always
> smaller than the content of "rebase-merge/author-script". Despite
> neglecting to NUL-terminate the constructed "author" header, memory is
> never accessed (either by read_author_ident() or its caller) beyond the
> allocated buffer since a NUL-terminator is present at the end of the
> loaded "rebase-merge/author-script" content, and additional NUL's are
> inserted as part of the parsing process.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
ACK!

Thanks, Dscho

Show 48 quoted lines
> ---
>  sequencer.c                   |  2 +-
>  t/t3404-rebase-interactive.sh | 10 +++++++++-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 16c1411054..78864d9072 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -744,7 +744,7 @@ static const char *read_author_ident(struct strbuf *buf)
>  		return NULL;
>  	}
>  
> -	buf->len = out - buf->buf;
> +	strbuf_setlen(buf, out - buf->buf);
>  	return buf->buf;
>  }
>  
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 01616901bd..8509c89a26 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1238,7 +1238,7 @@ rebase_setup_and_clean () {
>  		test_might_fail git branch -D $1 &&
>  		test_might_fail git rebase --abort
>  	" &&
> -	git checkout -b $1 master
> +	git checkout -b $1 ${2:-master}
>  }
>  
>  test_expect_success 'drop' '
> @@ -1415,4 +1415,12 @@ test_expect_success 'rebase -i --gpg-sign=<key-id> overrides commit.gpgSign' '
>  	test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err
>  '
>  
> +test_expect_success 'valid author header after --root swap' '
> +	rebase_setup_and_clean author-header no-conflict-branch &&
> +	set_fake_editor &&
> +	FAKE_LINES="2 1" git rebase -i --root &&
> +	git cat-file commit HEAD^ >out &&
> +	grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$" out
> +'
> +
>  test_done
> -- 
> 2.18.0.597.ga71716f1ad
> 
>
Eric Sunshine· Jul 30, 2018, 09:29 UTC · re: Eric Sunshine · lore

[PATCH 2/2] sequencer: fix "rebase -i --root" corrupting author header timezone

When "git rebase -i --root" creates a new root commit, it corrupts the "author" header's timezone by repeating the last digit:

    author A U Thor <author@example.com> @1112912773 -07000
This is due to two bugs.

First, write_author_script() neglects to add the closing quote to the value of GIT_AUTHOR_DATE when generating "rebase-merge/author-script".

Second, although sq_dequote() correctly diagnoses the missing closing quote, read_author_ident() ignores sq_dequote()'s return value and blindly uses the result of the aborted dequote.

sq_dequote() performs dequoting in-place by removing quoting and shifting content downward. When it detects misquoting (lack of closing quote, in this case), it gives up and returns an error without inserting a NUL-terminator at the end of the shifted content, which explains the duplicated last digit in the timezone.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
 sequencer.c                   | 7 ++++++-
 t/t3404-rebase-interactive.sh | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)
Show changes to 2 files +7 −2

sequencer.c, t/t3404-rebase-interactive.sh

diff --git a/sequencer.c b/sequencer.c
index 78864d9072..1008f6d71a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -654,6 +654,7 @@ static int write_author_script(const char *message)
 			strbuf_addch(&buf, *(message++));
 		else
 			strbuf_addf(&buf, "'\\\\%c'", *(message++));
+	strbuf_addch(&buf, '\'');
 	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
 	strbuf_release(&buf);
 	return res;
@@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)
 
 		eol = strchrnul(in, '\n');
 		*eol = '\0';
-		sq_dequote(in);
+		if (!sq_dequote(in)) {
+			warning(_("bad quoting on %s value in '%s'"),
+				keys[i], rebase_path_author_script());
+			return NULL;
+		}
 		len = strlen(in);
 
 		if (i > 0) /* separate values by spaces */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 8509c89a26..37796bb4c1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1420,7 +1420,7 @@ test_expect_success 'valid author header after --root swap' '
 	set_fake_editor &&
 	FAKE_LINES="2 1" git rebase -i --root &&
 	git cat-file commit HEAD^ >out &&
-	grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$" out
+	grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
 '
 
 test_done

-- 
2.18.0.597.ga71716f1ad
Phillip Wood· Jul 30, 2018, 12:20 UTC · re: Eric Sunshine · lore

Re: [PATCH 2/2] sequencer: fix "rebase -i --root" corrupting author header timezone

Hi Eric On 30/07/18 10:29, Eric Sunshine wrote:

Show 47 quoted lines
> When "git rebase -i --root" creates a new root commit, it corrupts the
> "author" header's timezone by repeating the last digit:
> 
>     author A U Thor <author@example.com> @1112912773 -07000
> 
> This is due to two bugs.
> 
> First, write_author_script() neglects to add the closing quote to the
> value of GIT_AUTHOR_DATE when generating "rebase-merge/author-script".
> 
> Second, although sq_dequote() correctly diagnoses the missing closing
> quote, read_author_ident() ignores sq_dequote()'s return value and
> blindly uses the result of the aborted dequote.
> 
> sq_dequote() performs dequoting in-place by removing quoting and
> shifting content downward. When it detects misquoting (lack of closing
> quote, in this case), it gives up and returns an error without inserting
> a NUL-terminator at the end of the shifted content, which explains the
> duplicated last digit in the timezone.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  sequencer.c                   | 7 ++++++-
>  t/t3404-rebase-interactive.sh | 2 +-
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 78864d9072..1008f6d71a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -654,6 +654,7 @@ static int write_author_script(const char *message)
>  			strbuf_addch(&buf, *(message++));
>  		else
>  			strbuf_addf(&buf, "'\\\\%c'", *(message++));
> +	strbuf_addch(&buf, '\'');
>  	res = write_message(buf.buf, buf.len, rebase_path_author_script(), 1);
>  	strbuf_release(&buf);
>  	return res;
> @@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)
>  
>  		eol = strchrnul(in, '\n');
>  		*eol = '\0';
> -		sq_dequote(in);
> +		if (!sq_dequote(in)) {
> +			warning(_("bad quoting on %s value in '%s'"),
> +				keys[i], rebase_path_author_script());
> +			return NULL;

I think we want to handle the broken author script properly rather than returning NULL. If we had a single function int read_author_script(const char **name, const char **author, const char **date) to read the author script that tried sq_dequote() and then fell back to code based on read_env_script() that handled the missing "'" at the end and also the bad quoting of "'" if sq_dequote() failed it would make it easier to fix the existing bugs, rather than having to fix read_author_ident() and read_env_script() separately. What do you think?

Best Wishes
Phillip
Show 18 quoted lines
> +		}
>  		len = strlen(in);
>  
>  		if (i > 0) /* separate values by spaces */
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8509c89a26..37796bb4c1 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1420,7 +1420,7 @@ test_expect_success 'valid author header after --root swap' '
>  	set_fake_editor &&
>  	FAKE_LINES="2 1" git rebase -i --root &&
>  	git cat-file commit HEAD^ >out &&
> -	grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9]*$" out
> +	grep "^author ..* @[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$" out
>  '
>  
>  test_done
>
Eric Sunshine· Jul 30, 2018, 18:45 UTC · re: Phillip Wood · lore

Re: [PATCH 2/2] sequencer: fix "rebase -i --root" corrupting author header timezone

On Mon, Jul 30, 2018 at 8:20 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
Show 25 quoted lines
> On 30/07/18 10:29, Eric Sunshine wrote:
> > When "git rebase -i --root" creates a new root commit, it corrupts the
> > "author" header's timezone by repeating the last digit:
> > [...]
> > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -654,6 +654,7 @@ static int write_author_script(const char *message)
> > +     strbuf_addch(&buf, '\'');
> > @@ -724,7 +725,11 @@ static const char *read_author_ident(struct strbuf *buf)
> > -             sq_dequote(in);
> > +             if (!sq_dequote(in)) {
> > +                     warning(_("bad quoting on %s value in '%s'"),
> > +                             keys[i], rebase_path_author_script());
> > +                     return NULL;
>
> I think we want to handle the broken author script properly rather than
> returning NULL. If we had a single function
> int read_author_script(const char **name, const char **author, const
> char **date)
> to read the author script that tried sq_dequote() and then fell back to
> code based on read_env_script() that handled the missing "'" at the end
> and also the bad quoting of "'" if sq_dequote() failed it would make it
> easier to fix the existing bugs, rather than having to fix
> read_author_ident() and read_env_script() separately. What do you think?

That makes sense as a long-term plan, however, I'm concerned with the immediate problem that a released version of Git can (and did, in my case) corrupt commit objects. So, in the short term, I think it makes sense to get this minimal fix landed, and build the more "correctly engineered" solution on top of it, without the pressure of worrying about corruption spreading.

Eric Sunshine· Jul 30, 2018, 10:06 UTC · re: Eric Sunshine · lore

Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

On Mon, Jul 30, 2018 at 5:30 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 7 quoted lines
> This series fixes bugs causing corruption of the root commit when
> "rebase -i --root" is used to swap in a new root commit. In particular,
> the "author" header has trailing garbage. Some tools handle the
> corruption somewhat gracefully by showing a bogus date, but others barf
> on it (gitk, for instance). git-fsck correctly identifies the
> corruption. I discovered this after git-rebase corrupted one of my own
> projects.

Unfortunately, after sending this series, I see that there is additional corruption that needs to be addressed. In particular, the "author" header time is incorrectly prefixed with "@", so this series is going to need a re-roll to fix that, as well.

Johannes Schindelin· Jul 30, 2018, 15:47 UTC · re: Eric Sunshine · lore

Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

Hi Eric,
On Mon, 30 Jul 2018, Eric Sunshine wrote:
Show 13 quoted lines
> On Mon, Jul 30, 2018 at 5:30 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > This series fixes bugs causing corruption of the root commit when
> > "rebase -i --root" is used to swap in a new root commit. In particular,
> > the "author" header has trailing garbage. Some tools handle the
> > corruption somewhat gracefully by showing a bogus date, but others barf
> > on it (gitk, for instance). git-fsck correctly identifies the
> > corruption. I discovered this after git-rebase corrupted one of my own
> > projects.
> 
> Unfortunately, after sending this series, I see that there is
> additional corruption that needs to be addressed. In particular, the
> "author" header time is incorrectly prefixed with "@", so this series
> is going to need a re-roll to fix that, as well.

AFAIR the `@` was not an oversight, but required so that we could pass in the Unix epoch.

Ciao, Dscho

Eric Sunshine· Jul 30, 2018, 19:19 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

On Mon, Jul 30, 2018 at 11:47 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 8 quoted lines
> On Mon, 30 Jul 2018, Eric Sunshine wrote:
> > Unfortunately, after sending this series, I see that there is
> > additional corruption that needs to be addressed. In particular, the
> > "author" header time is incorrectly prefixed with "@", so this series
> > is going to need a re-roll to fix that, as well.
>
> AFAIR the `@` was not an oversight, but required so that we could pass in
> the Unix epoch.

I don't think it was an oversight either, but it is nevertheless incorrect to use the "@" in the commit object's "author" header.

As I understand it, you do "need" the "@" to distinguish a Unix epoch value assigned to GIT_AUTHOR_DATE, but the commit object format is very exacting in the datestamp format it accepts, and it does not allow "@". So, the date from GIT_AUTHOR_DATE needs to be converted to a format acceptable to the commit object, otherwise the commit is considered corrupt.

Phillip Wood· Jul 30, 2018, 12:14 UTC · re: Eric Sunshine · lore

Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

On 30/07/18 10:29, Eric Sunshine wrote:
Show 26 quoted lines
> This series fixes bugs causing corruption of the root commit when
> "rebase -i --root" is used to swap in a new root commit. In particular,
> the "author" header has trailing garbage. Some tools handle the
> corruption somewhat gracefully by showing a bogus date, but others barf
> on it (gitk, for instance). git-fsck correctly identifies the
> corruption. I discovered this after git-rebase corrupted one of my own
> projects.
> 
> Unfortunately, these bugs (from js/sequencer-and-root-commits) made it
> into the v2.18.0 release. It's worrying that a released Git can be
> creating corrupt commits, but fortunately "rebase -i --root" is not
> likely used often (especially on well-established projects).
> Nevertheless, it may be 'maint' worthy and applies cleanly there.
> 
> It was only after I diagnosed and fixed these bugs that I thought to
> check 'pu' and discovered that Akinori MUSHA already made a stab[1] at
> fixing one of the three bugs which this series fixes. Akinori's fix has
> the somewhat undesirable property that it adds an extra blank line to
> the end of the script, as Phillip correctly pointed out in review[2].
> Patch 2/2 of this series has the more "correct" fix, in addition to
> fixing another bug.
> 
> Moreover, patch 2/2 of this series provides a more thorough fix overall
> than Akinori, so it may make sense to replace his patch with this
> series, though perhaps keep the test his patch adds to augment the
> strict test of the "author" header added by this series.

Johannes and I have some fixups for Akinori's patch on the branch fix-t3403-author-script-test at https://github.com/phillipwood/git

That branch also contains a fix for the bad quoting of names with "'" in them. I think it would be good to somehow try and combine this series with those patches.

I'd really like to see a single function to read and another to write the author script that is shared by 'git am' and 'git rebase -i', rather than the two writers and three readers we have at the moment. I was thinking of doing that in the longer term, but given the extra bug you've found in read_author_script() maybe we should do that sooner rather than later.

Show 11 quoted lines
> [1]: https://public-inbox.org/git/86a7qwpt9g.knu@iDaemons.org/
> [2]: https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889d06@talktalk.net/
> 
> Eric Sunshine (2):
>   sequencer: fix "rebase -i --root" corrupting author header
>   sequencer: fix "rebase -i --root" corrupting author header timezone
> 
>  sequencer.c                   |  9 +++++++--
>  t/t3404-rebase-interactive.sh | 10 +++++++++-
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
Junio C Hamano· Jul 30, 2018, 15:29 UTC · re: Phillip Wood · lore

Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

Phillip Wood <phillip.wood@talktalk.net> writes:
Show 11 quoted lines
>> Moreover, patch 2/2 of this series provides a more thorough fix overall
>> than Akinori, so it may make sense to replace his patch with this
>> series, though perhaps keep the test his patch adds to augment the
>> strict test of the "author" header added by this series.
>
> Johannes and I have some fixups for Akinori's patch on the branch
> fix-t3403-author-script-test at https://github.com/phillipwood/git
>
> That branch also contains a fix for the bad quoting of names with "'" in
> them. I think it would be good to somehow try and combine this series
> with those patches.

Thanks for working well together. Always nice to see contributors thinking beyond immediate band-aid and for longer term ;-)

Show 6 quoted lines
> I'd really like to see a single function to read and another to write
> the author script that is shared by 'git am' and 'git rebase -i', rather
> than the two writers and three readers we have at the moment. I was
> thinking of doing that in the longer term, but given the extra bug
> you've found in read_author_script() maybe we should do that sooner
> rather than later.
Johannes Schindelin· Jul 30, 2018, 15:49 UTC · re: Phillip Wood · lore

Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

Hi Phillip,
On Mon, 30 Jul 2018, Phillip Wood wrote:
Show 34 quoted lines
> On 30/07/18 10:29, Eric Sunshine wrote:
> > This series fixes bugs causing corruption of the root commit when
> > "rebase -i --root" is used to swap in a new root commit. In particular,
> > the "author" header has trailing garbage. Some tools handle the
> > corruption somewhat gracefully by showing a bogus date, but others barf
> > on it (gitk, for instance). git-fsck correctly identifies the
> > corruption. I discovered this after git-rebase corrupted one of my own
> > projects.
> > 
> > Unfortunately, these bugs (from js/sequencer-and-root-commits) made it
> > into the v2.18.0 release. It's worrying that a released Git can be
> > creating corrupt commits, but fortunately "rebase -i --root" is not
> > likely used often (especially on well-established projects).
> > Nevertheless, it may be 'maint' worthy and applies cleanly there.
> > 
> > It was only after I diagnosed and fixed these bugs that I thought to
> > check 'pu' and discovered that Akinori MUSHA already made a stab[1] at
> > fixing one of the three bugs which this series fixes. Akinori's fix has
> > the somewhat undesirable property that it adds an extra blank line to
> > the end of the script, as Phillip correctly pointed out in review[2].
> > Patch 2/2 of this series has the more "correct" fix, in addition to
> > fixing another bug.
> > 
> > Moreover, patch 2/2 of this series provides a more thorough fix overall
> > than Akinori, so it may make sense to replace his patch with this
> > series, though perhaps keep the test his patch adds to augment the
> > strict test of the "author" header added by this series.
> 
> Johannes and I have some fixups for Akinori's patch on the branch
> fix-t3403-author-script-test at https://github.com/phillipwood/git
> 
> That branch also contains a fix for the bad quoting of names with "'" in
> them. I think it would be good to somehow try and combine this series
> with those patches.
I would like that, too.
Show 6 quoted lines
> I'd really like to see a single function to read and another to write
> the author script that is shared by 'git am' and 'git rebase -i', rather
> than the two writers and three readers we have at the moment. I was
> thinking of doing that in the longer term, but given the extra bug
> you've found in read_author_script() maybe we should do that sooner
> rather than later.
You are at least the second person (after Junio) with that wish.

Please note, however, that the purpose of author-script reading/writing is very different in git-am vs rebase -i, or at least it used to be: read_env_script() in sequencer.c very specifically wants to construct an env parameter for use in run_command().

Don't let me stop you from trying to consolidate the two different code paths, though.

Ciao, Dscho

Show 14 quoted lines
> 
> > [1]: https://public-inbox.org/git/86a7qwpt9g.knu@iDaemons.org/
> > [2]: https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889d06@talktalk.net/
> > 
> > Eric Sunshine (2):
> >   sequencer: fix "rebase -i --root" corrupting author header
> >   sequencer: fix "rebase -i --root" corrupting author header timezone
> > 
> >  sequencer.c                   |  9 +++++++--
> >  t/t3404-rebase-interactive.sh | 10 +++++++++-
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> 
>
Eric Sunshine· Jul 30, 2018, 19:15 UTC · re: Phillip Wood · lore

Re: [PATCH 0/2] fix "rebase -i --root" corrupting root commit

On Mon, Jul 30, 2018 at 8:14 AM Phillip Wood <phillip.wood@talktalk.net> wrote:
Show 16 quoted lines
> On 30/07/18 10:29, Eric Sunshine wrote:
> > It was only after I diagnosed and fixed these bugs that I thought to
> > check 'pu' and discovered that Akinori MUSHA already made a stab[1] at
> > fixing one of the three bugs which this series fixes. Akinori's fix has
> > the somewhat undesirable property that it adds an extra blank line to
> > the end of the script, as Phillip correctly pointed out in review[2].
> > Patch 2/2 of this series has the more "correct" fix, in addition to
> > fixing another bug.
> >
> > Moreover, patch 2/2 of this series provides a more thorough fix overall
> > than Akinori, so it may make sense to replace his patch with this
> > series, though perhaps keep the test his patch adds to augment the
> > strict test of the "author" header added by this series.
>
> Johannes and I have some fixups for Akinori's patch on the branch
> fix-t3403-author-script-test at https://github.com/phillipwood/git

I don't see a branch with that name there. There are a couple "wip" branches, however, named wip/fix-t3403-author-script-test and wip/fix-t3404-author-script-test. I'm guessing you wanted me to look at the former.

> That branch also contains a fix for the bad quoting of names with "'" in
> them. I think it would be good to somehow try and combine this series
> with those patches.

It appears that your patches are fixing issues and a test outside the issues fixed by my series (aside from the one line inserting the missing closing quote). As such, I think your patches can be built atop this series without worrying about conflicts. That would allow this commit-corruption-bug-fixing series to land without being tied to those "wip" patches which address lower-priority problems.

Show 6 quoted lines
> I'd really like to see a single function to read and another to write
> the author script that is shared by 'git am' and 'git rebase -i', rather
> than the two writers and three readers we have at the moment. I was
> thinking of doing that in the longer term, but given the extra bug
> you've found in read_author_script() maybe we should do that sooner
> rather than later.

Agreed. That seems a reasonable long-term goal but needn't hold up this series which addresses very real bugs leading to object corruption.

← back to recent threads