Re: [PATCH v3] push: fix --force-if-includes when remote-tracking ref has no reflog

From: Tyler Cipriani <tyler@tylercipriani.com>
Date: Sep 29, 2026, 01:10 UTC
Message-ID: <arsP8IE6LuAKzYE6@localhost.localdomain>
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).

Show 27 quoted lines

>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 <who or what> not looking at
   <what> 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()

Show 52 quoted lines

>
>Signed-off-by: Aleksei Sviridkin <f@lex.la>
>---
>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.