git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Jul 30, 2018, 19:15 UTC
Message-ID
<CAPig+cRaxgNVkvjumgiMY_ssmvKWRh6USBdu27WbBrT7y7SP3g@mail.gmail.com>
In-Reply-To
<1f172fc1-4b51-cdf7-e841-5ca41e209be4@talktalk.net>
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.

Previous: Johannes Schindelin
Message 13 of 13 in “fix "rebase -i --root" corrupting root commit”
  1. 0/2 fix "rebase -i --root" corrupting root commitEric Sunshine, Jul 30, 2018
  2. 1/2 sequencer: fix "rebase -i --root" corrupting author headerEric Sunshine, Jul 30, 2018
  3. Johannes SchindelinJul 30, 2018
  4. 2/2 sequencer: fix "rebase -i --root" corrupting author header timezoneEric Sunshine, Jul 30, 2018
  5. Phillip WoodJul 30, 2018
  6. Eric SunshineJul 30, 2018
  7. Eric SunshineJul 30, 2018
  8. Johannes SchindelinJul 30, 2018
  9. Eric SunshineJul 30, 2018
  10. Phillip WoodJul 30, 2018
  11. Junio C HamanoJul 30, 2018
  12. Johannes SchindelinJul 30, 2018
  13. Eric SunshineJul 30, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.