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

Hi Phillip,

On Mon, 30 Jul 2018, Phillip Wood wrote:

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

quoted

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.

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

quoted

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