Re: [PATCH] refs: introduce API function to write invalid null ref

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 22, 2021, 01:20 UTC
Message-ID: <CAPig+cT9ZUqkOZWZS3+gEd3soh-xyfxu2yvQ_gY-LMgVV-rAiw@mail.gmail.com>
In-Reply-To: <20210222004112.99268-1-stefanbeller@gmail.com>

On Sun, Feb 21, 2021 at 7:43 PM Stefan Beller <stefanbeller@gmail.com> wrote:

Show 11 quoted lines

> 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@gmail.com>
> ---
> 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...

Show 11 quoted lines

> 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, ...);

Show 13 quoted lines

> 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.)