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

On Mon, Jul 30, 2018 at 8:14 AM Phillip Wood [off-list ref] wrote:

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

quoted

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.

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.