On Sun, Feb 21, 2021 at 7:43 PM Stefan Beller [off-list ref] 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 <redacted>
---
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...

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

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