On Sun, Feb 21, 2021 at 7:43 PM Stefan Beller <stefanbeller@xxxxxxxxx> wrote:
> Different ref backends will have different ways to write out the invalid 00..00
> ref when starting a new worktree. Encapsulate this into a function and expose
> the function in the refs API.
>
> Signed-off-by: Stefan Beller <stefanbeller@xxxxxxxxx>
> ---
> it's been a while since I looked at git source code, but today is the day!
> I was actually looking how the refs table work progresses and this patch
> caught my attention. I think the changes in builtin/worktree.c (that
> if/else depending on the actual refs backend used)
> demonstrate that the refs API layer is leaking implementation details.
Welcome back. I have a few comments in response to this proposal. See below...
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -330,9 +330,9 @@ static int add_worktree(const char *path, const char *refname,
> strbuf_reset(&sb);
> - strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
> - write_file(sb.buf, "%s", oid_to_hex(&null_oid));
> - strbuf_reset(&sb);
> +
> + write_invalid_head(get_main_ref_store(the_repository), sb_repo.buf);
> +
> strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
> write_file(sb.buf, "../..");
Nit: This change ends up making the strbuf_reset() unnecessarily
distant from the location where it has most meaning (just before we
start using it again, which is the pattern in this function):
strbuf_release(&sb);
write_invalid_head(..., sb_repo.buf);
strbuf_add(&sb, ...);
It would be clearer for the end result to be:
write_invalid_head(..., sb_repo.buf);
strbuf_release(&sb);
strbuf_add(&sb, ...);
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> @@ -3169,6 +3169,18 @@ static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
> +static int files_write_invalid_head_ref(struct ref_store *ref_store,
> + const char *dir)
> +{
> + struct strbuf sb = STRBUF_INIT;
> +
> + strbuf_addf(&sb, "%s/HEAD", dir);
> + write_file(sb.buf, "%s", oid_to_hex(&null_oid));
> + strbuf_release(&sb);
> +
> + return 0;
> +}
I'm not super enthused about the name write_invalid_head_ref(). Even
if given a different name, I'm not necessarily enthused about it
unconditionally writing a zero-OID. Do you foresee other cases in
which callers will need to perform this precise operation?
The reason I ask is that the bit of code in
builtin/worktree.c:add_worktree() which this patch targets is itself a
hack to work around the shortcoming in which is_git_directory() won't
consider the newly-created worktree as being legitimate if it doesn't
have a well-formed HEAD ref. It's not visible in the context of this
patch, but there is a comment just above the changes to
builtin/worktree.c made by this patch:
/*
* This is to keep resolve_ref() happy. We need a valid HEAD
* or is_git_directory() will reject the directory. Any value which
* looks like an object ID will do since it will be immediately
* replaced by the symbolic-ref or update-ref invocation in the new
* worktree.
*/
As mentioned in the comment, it doesn't matter what the value of HEAD
is; it just needs to be something that looks like a well-formed OID.
So, my lack of enthusiasm for write_invalid_head_ref() is twofold.
First, this function is being introduced to paper over a hack, which
leaves the new function smelling funny. Second, the "invalid head" in
the name and the unconditional zero-OID feel too special-purpose and
focus too much on the particular current implementation in
builtin/worktree.c which happens to assign a zero-OID to HEAD. (At an
earlier time, builtin/worktree.h assigned the value of HEAD from the
current worktree to HEAD in the newly-created worktree instead. But,
as noted, the value is arbitrary -- it doesn't matter what it is -- it
just needs to exist long enough to pacify is_git_directory() and is
then immediately overwritten.)
On the other hand, I could see this as acceptable if "invalid" is
removed from the function name and if it accepts an OID to write
rather than unconditionally writing a zero-ID. In that case, it would
become a generally useful function without the bad smells associated
with the too-special-purpose write_invalid_head_ref(). (But I haven't
been paying attention to the ref backends work, so perhaps such a
function already exists? I don't know.)
