Re: [PATCH v3 2/8] branch: consider refs under 'update-refs'

From: Derrick Stolee <derrickstolee@github.com>
To: Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, johannes.schindelin@gmx.de, me@ttaylorr.com,
	Jeff Hostetler <git@jeffhostetler.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v3 2/8] branch: consider refs under 'update-refs'
Date: Wed, 29 Jun 2022 08:58:48 -0400	[thread overview]
Message-ID: <9d188e11-7bb4-cae7-b38a-f9f38c8c2fe1@github.com> (raw)
In-Reply-To: <xmqqpmisikex.fsf@gitster.g>

On 6/28/22 4:48 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> The branch_checked_out() helper helps commands like 'git branch' and
>> 'git fetch' from overwriting refs that are currently checked out in
>> other worktrees.
>>
>> A future update to 'git rebase' will introduce a new '--update-refs'
>> option which will update the local refs that point to commits that are
>> being rebased. To avoid collisions as the rebase completes, we want to
>> make the future data store for these refs to be considered by
>> branch_checked_out().
>>
>> The data store is a plaintext file inside the 'rebase-merge' directory
>> for that worktree. The file alternates refnames and OIDs. The OIDs will
>> be used to store the to-be-written values as the rebase progresses, but
>> can be ignored at the moment.
>>
>> Create a new sequencer_get_update_refs_state() method that parses this
>> file and populates a struct string_list with the ref-OID pairs. We can
>> then use this list to add to the current_checked_out_branches strmap
>> used by branch_checked_out().
>>
>> To properly navigate to the rebase directory for a given worktree,
>> extract the static strbuf_worktree_gitdir() method to a public API
>> method.
>>
>> We can test that this works without having Git write this file by
>> artificially creating one in our test script.
> 
> Hmph, I am not thrilled to see an ad-hoc text file for things like
> this.  Do the objects that appear in this file otherwise already
> anchored by existing refs, or can some of them be "floating" without
> any refs pointing at them, i.e. making the "connected" and "fsck"
> machinery also being aware of them to protect them from collected as
> garbage and reported as dangling/unreachable?

You're right that we could have this sequence of events in a todo
file:

  pick deadbeef Here is a commit that will become unreachable
  update-ref will-be-lost

  squash 1234567 make will-be-lost unreachable
  ...

So if a GC runs after this 'update-ref' step but before the rebase
completes, then that commit can be lost. The ref-update at the end
of the rebase process would fail.

I wonder how important such a situation is, though. But I'm willing
to add the extra lookups in 'git gc' and 'git fsck' to make this a
non-issue.

I'll take a look to see where the other refs in rebase-<backend>/*
are handled by these processes.

Thanks,
-Stolee