Hi Phillip, On Mon, 30 Jul 2018, Phillip Wood wrote:
> 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. > 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 > > > [1]: https://public-inbox.org/git/86a7qwpt9g....@idaemons.org/ > > [2]: > > https://public-inbox.org/git/f5b56540-d26a-044e-5f46-1d975f889...@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(-) > > > >
