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

Re: [PATCH] t3429: try to protect against a potential racy todo file problem

From
Junio C Hamano <gitster@pobox.com>
Date
Nov 25, 2019, 01:28 UTC
Message-ID
<xmqqk17ozqvy.fsf@gitster-ct.c.googlers.com>
In-Reply-To
<20191124211021.GB23183@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 15 quoted lines
> On Sun, Nov 24, 2019 at 10:44:10AM +0000, Phillip Wood wrote:
>> On 24/11/2019 04:49, Junio C Hamano wrote:
>>  ...
>> >Hmph, that makes it sound as if the right fix is to re-read after
>> >writing the first version of the todo file out, so that the stat
>> >data matches reality and tells us that it has never been modified?
>> 
>> I think we should update the stat data after we write the todo list.
>
> Well, yes and no.
>
> No, because we are dealing with regression in v2.24.0 here, so the
> simpler the fix the better it is for maint.  I don't think a fix can
> get any simpler than my patch, with or without the suggestions from
> Phillip.

Of course, the simplest "fix" for regression is to revert the offending one, and anything else is a band-aid ;-). The question is which band-aid is the least risky and which one takes us the closest to the real solution. I tend to agree that forcing to skip checking no matter what the variable "check_todo" says unless is_rebase_i() qualifies as the band-aid that is the least risky.

Show 7 quoted lines
> Yes, we should definitely consider updating the stat data after the
> sequencer writes the todo list, or any other options with which the
> sequencer could notice a modified todo list file with less subtlety.
> Alas, there is a big can of worms in that direction, see the patch
> below, and we have to be very careful going that way, so I think it's
> only viable in the long term, but less suitable as a regression fix
> for maint.

Yes, I agree that it is much less suitable than even reverting the offending one outright.

> (Hrm, perhaps I spent too many words on the all zeroed out stat data,
> and managed to sidetrack you a bit...)

No, I do not think so. Thinking about what we need to do in the longer term, while coming up with a shorter term fix, is a necessary step of gaining confidence in the latter.

Again, thanks both for thinking about this issue.
Previous: SZEDER GáborNext: SZEDER Gábor
Message 9 of 16 in “git 2.24: git revert <commit1> <commit2> requires extra '--continue'?”
  1. Brian NorrisNov 22, 2019
  2. SZEDER GáborNov 23, 2019
  3. Phillip WoodNov 23, 2019
  4. sequencer: don't re-read todo for revert and cherry-pickSZEDER Gábor, Nov 23, 2019
  5. Johannes SchindelinNov 23, 2019
  6. Junio C HamanoNov 24, 2019
  7. Phillip WoodNov 24, 2019
  8. t3429: try to protect against a potential racy todo file problemSZEDER Gábor, Nov 24, 2019
  9. Junio C HamanoNov 25, 2019
  10. SZEDER GáborNov 25, 2019
  11. SZEDER GáborNov 25, 2019
  12. Phillip WoodNov 25, 2019
  13. SZEDER GáborNov 25, 2019
  14. Phillip WoodNov 25, 2019
  15. Junio C HamanoNov 25, 2019
  16. Phillip WoodNov 25, 2019

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.