Re: [PATCH 2/3] t/lib-httpd: make http-429 first-request check atomic
- From
Michael Montalbo <mmontalbo@gmail.com>
- Date
- Jul 9, 2026, 18:10 UTC
- Message-ID
- <CAC2QwmKuHUP6_287T9SOLdjLdb=b4EqV4qJ_NnYCkGP0-d6qHA@mail.gmail.com>
- In-Reply-To
- <xmqqcxwxtfkp.fsf@gitster.g>
On Wed, Jul 8, 2026 at 1:02 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
> > "Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > -# Check if this is the first call (no state file exists) > > -if test -f "$state_file" > > +# Apache can run this CGI for concurrent requests, so the script decides > > +# whether this is the first call with a single atomic "mkdir": it succeeds for > > +# exactly one of any racing requests and fails for the rest. "permanent" > > +# always rate-limits and records no state. > > +if test "$retry_after" != permanent && ! mkdir "$state" 2>/dev/null > > I think the last sentence in the above comment was meant to explain > why the new code checks the value of "$retry_after", but it is not > clear if it is needed for correctness (in other words, the original > was wrong to do "test -f && touch" but also was wrong to do so even > when "$retry_after" is set to "permanent), or if it is a mere > "optimization opportunity" you are taking advantage of. In either > case, it would be nice to see it explained in the proposed commit > log message. >
It is needed for correctness, and I agree it is not very clear from the log message / comment. I will spell out the reasoning for the change more clearly in both.
Thanks for taking a look at this!