From: Tyler Cipriani Date: Tue, 29 Sep 2026 01:10:08 GMT Subject: Re: [PATCH v3] push: fix --force-if-includes when remote-tracking ref has no reflog Message-ID: In-Reply-To: <20260905171330.34646-1-f@lex.la> On 26-09-05 20:13:30, Aleksei Sviridkin wrote: Code looks right to me with the date=0 fallback. push.useForceIfIncludes is meant to tighten --force-with-lease's checks. Getting a wrong answer with advice telling me to pull would push me (pun intended) to drop the config and ditch the feature. For --force-if-includes, being wrong here is worse than being slow here. Re: being slow. I tried to recreate some numbers from this thread with my own test case using linux.git and 2k reflog entries. In the cases I tried: - With a commit-graph (which gc should write), walking + merge-base checks on 2k entries took 15ms. So a few microseconds per reflog entry roughly jibes with numbers from this thread. - Without a commit-graph, each batched call to repo_in_merge_bases_many() walks the commit history from scratch: rejection took two minutes for 2k entries. But my tests were artificial worst-case scenarios. And today, on my build, "date" already happens to be a low number. For folks like me, setting date=0 is a non-change and I've been unable to find any complaints of slowness on the mailing list (or by searching the web). For folks where date happens to be a high number: this gets the feature working correctly. Bonus: doubling batch size after each call to repo_in_merge_bases_many took my 2min down to 10s, locally; a viable speed up if needed (but separate from this change). >Since 99a1f9ae10 (push: add reflog check for "--force-if-includes", >2020-10-03), is_reachable_in_reflog() stops walking the reflog of the >local branch at entries older than the newest reflog entry of the >remote-tracking ref. That timestamp is read by a callback of >refs_for_each_reflog_ent_reverse(), so when the remote-tracking ref >has no reflog, the variable that holds the timestamp stays >uninitialized. > >With the files backend a remote-tracking ref created by "git clone" >has no reflog and does not get one until it moves. On my machine the >leftover value exceeds any real timestamp: the walk stops at the very >first entry, never reaches the "Created from" entry that "checkout >--track" wrote, and the push is rejected with "remote ref updated >since checkout" although nothing on the remote has changed. > >The cut-off is an optimization that rests on an assumption: an entry >older than the moment the remote-tracking ref last moved is not >expected to be the one being looked for. Without a reflog there is >no such moment, hence no cut-off to apply. Initialize the timestamp >to zero to say exactly that: timestamp_t is unsigned, so no entry >compares older than zero and the comparison never fires. Using >"now", or any fixed age, would instead cut the walk off at the first >entry older than that bound, which is how the failure happens in >the first place. The price is paid only when no matching entry is >found: the walk then reaches the oldest entry and falls back to the >merge-base check over what it collected, where the cut-off would >have stopped it earlier. The last paragraph of this log message is hard to read for me; I think people could come away from reading it with the wrong information. Nits: - The final paragraph of the log message starts with "The cut-off", but it's the first time you've used "cut-off." What cut-off? - "an entry older than [...] is not expected to be the one being looked for" - passive voice, stacked verb phrases ("is not expected/to be"), and a subject separated from its verb by 9 words made this hard to follow. And it leaves questions: Why is not looking at entry? - "Without a reflog" - which reflog? remote-tracking or local? - Unclear referents: - "exactly that" - "that bound" Problems (with more nits :)): - "there is no such moment" - Readability: referring back to "moment" that came 23 words before this "moment" made me re-read this a few times. - Inaccuracy: there may have been a moment when the remote-tracking ref last moved, but there is no reliable record of it because there is no remote-tracking reflog. That is, someone may have removed the reflog, or the reflog could have been GC'd (neither case is mentioned in your message). - Most importantly, since the way I parse it is technically incorrect: "the walk then reaches the oldest entry and falls back to the merge-base check...where the cut-off would have stopped it earlier." - Stopped what earlier? I read this sentence split on "where" (i.e., Y does this, whereas X does that). Read that way, the final sentence reads as: Walk without a cut-off: (a) "reaches the oldest entry" (b) "falls back to the merge-base check" vs. Walk with a cut-off: stops earlier and therefore does neither. But a walk with a cut-off falls back to a merge-base check, too. The difference is that without a cut-off you reach the oldest entry and therefore pass more local reflog entries to the merge-base check; i.e., potentially more calls to repo_in_merge_bases_many() > >Signed-off-by: Aleksei Sviridkin >--- >Changes since v2: > - reworded the first paragraph as you suggested > - explain why zero is the fallback rather than "now" or a fixed age > - dropped the Assisted-by trailer > > remote.c | 2 +- > t/t5533-push-cas.sh | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > >diff --git a/remote.c b/remote.c >index 00723b385e..6d301698ca 100644 >--- a/remote.c >+++ b/remote.c >@@ -2751,7 +2751,7 @@ static int check_and_collect_until(const char *refname UNUSED, > */ > static int is_reachable_in_reflog(const char *local, const struct ref *remote) > { >- timestamp_t date; >+ timestamp_t date = 0; > struct commit *commit; > struct commit **chunk; > struct check_and_collect_until_cb_data cb; >diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh >index cba26a872d..bb8878c593 100755 >--- a/t/t5533-push-cas.sh >+++ b/t/t5533-push-cas.sh >@@ -396,4 +396,22 @@ test_expect_success '"--force-if-includes" should allow deletes' ' > ) > ' > >+test_expect_success '"--force-if-includes" should allow forced update when remote-tracking ref has no reflog' ' >+ rm -fr dst src && >+ test_when_finished "rm -fr dst src" && >+ git init --bare dst && >+ git push dst main main:branch && >+ git clone --no-local dst src && >+ ( >+ cd src && >+ # a clone leaves the remote-tracking refs without reflog >+ # entries with the files backend, but not with reftable >+ git reflog expire --all --expire=all && >+ git switch -c branch --track origin/branch && >+ git reset --hard HEAD^ && >+ test_commit D && >+ git push --force-if-includes --force-with-lease="branch" >+ ) >+' >+ > test_done Tested: passes with the fix. Without the fix it also passes on my machine. gdb says that the value of date is 2 for me (Linux x86_64, gcc (Debian 14.2.0-19) 14.2.0, on Trixie). To get the test to fail reliably, had to build with: make CFLAGS_APPEND=-ftrivial-auto-var-init=pattern So, CI probably would miss date becoming uninitialized again. It also fails with a date set to a timestamp 90 days ago due, since test dates are 2005. Minor nit: surrounding tests in t/t5533-push-cas.sh use setup_src_dup_dst, which would simplify the test setup. I'd be happy to give a Reviewed-by once the log message is clearer. Thanks.