| Message ID | 20210222004112.99268-1-stefanbeller@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs: introduce API function to write invalid null ref | expand |
On Sun, Feb 21, 2021 at 7:43 PM Stefan Beller <stefanbeller@gmail.com> 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@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... > 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.)
On Sun, Feb 21, 2021 at 8:20 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > 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. [...] By the way, the only reason the hack of creating a temporary HEAD (containing arbitrary OID) is needed is that add_worktree() shells out to invoke one of git-update-ref or git-symbolic-ref. It is that shell-out to invoke a Git command which triggers the necessity of pacifying is_git_directory()... > 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(). ... so, a much cleaner fix would be to stop shelling out to set up HEAD in the newly-created worktree, and instead call C API to perform those functions (update-ref and symbolic-ref) directly. If that C API does not yet exist, then it should be added.
On Mon, Feb 22, 2021 at 1:41 AM Stefan Beller <stefanbeller@gmail.com> 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@gmail.com> > --- > > Hi Han-Wen, > > 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. > > What do you think about rolling this patch first, and then implementing > the following part inside the reftable as a function? The "invalid HEAD" hack is there to avoid confusing historical git implementations. It's not a part of the "modern" refs API layer, so I think we shouldn't add it as a method on the ref backend API.
diff --git a/builtin/worktree.c b/builtin/worktree.c index 1cd5c2016e..6f5d79e5ec 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -330,9 +330,9 @@ static int add_worktree(const char *path, const char *refname, * worktree. */ 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, "../.."); diff --git a/refs.c b/refs.c index a665ed5e10..a7b415c386 100644 --- a/refs.c +++ b/refs.c @@ -2454,3 +2454,8 @@ int copy_existing_ref(const char *oldref, const char *newref, const char *logmsg { return refs_copy_existing_ref(get_main_ref_store(the_repository), oldref, newref, logmsg); } + +int write_invalid_head(struct ref_store *refs, const char *dir) +{ + return refs->be->write_invalid_head_ref(refs, dir); +} diff --git a/refs.h b/refs.h index 48970dfc7e..7a64f777d5 100644 --- a/refs.h +++ b/refs.h @@ -456,6 +456,9 @@ int refs_delete_refs(struct ref_store *refs, const char *msg, int delete_refs(const char *msg, struct string_list *refnames, unsigned int flags); + +int write_invalid_head(struct ref_store *refs, const char *dir); + /** Delete a reflog */ int refs_delete_reflog(struct ref_store *refs, const char *refname); int delete_reflog(const char *refname); diff --git a/refs/debug.c b/refs/debug.c index 922e64fa6a..9cfc3ca9f3 100644 --- a/refs/debug.c +++ b/refs/debug.c @@ -175,6 +175,15 @@ static int debug_copy_ref(struct ref_store *ref_store, const char *oldref, return res; } +static int debug_write_invalid_head_ref(struct ref_store *ref_store, + const char *dir) +{ + struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; + int res = drefs->refs->be->write_invalid_head_ref(drefs->refs, dir); + trace_printf_key(&trace_refs, "write_invalid_head: %s\n", dir); + return res; +} + struct debug_ref_iterator { struct ref_iterator base; struct ref_iterator *iter; @@ -384,6 +393,7 @@ struct ref_storage_be refs_be_debug = { debug_delete_refs, debug_rename_ref, debug_copy_ref, + debug_write_invalid_head_ref, debug_ref_iterator_begin, debug_read_raw_ref, diff --git a/refs/files-backend.c b/refs/files-backend.c index 4fdc68810b..b1c82a6f61 100644 --- 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) return 0; } +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; +} + struct ref_storage_be refs_be_files = { NULL, "files", @@ -3184,6 +3196,7 @@ struct ref_storage_be refs_be_files = { files_delete_refs, files_rename_ref, files_copy_ref, + files_write_invalid_head_ref, files_ref_iterator_begin, files_read_raw_ref, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index b912f2505f..a1adb81777 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1591,6 +1591,12 @@ static int packed_copy_ref(struct ref_store *ref_store, BUG("packed reference store does not support copying references"); } +static int packed_write_invalid_head_ref(struct ref_store *ref_store, + const char *dir) +{ + BUG("packed reference store does not support writing an invalid head ref"); +} + static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_store) { return empty_ref_iterator_begin(); @@ -1656,6 +1662,7 @@ struct ref_storage_be refs_be_packed = { packed_delete_refs, packed_rename_ref, packed_copy_ref, + packed_write_invalid_head_ref, packed_ref_iterator_begin, packed_read_raw_ref, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 467f4b3c93..5055226f75 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -556,6 +556,8 @@ typedef int rename_ref_fn(struct ref_store *ref_store, typedef int copy_ref_fn(struct ref_store *ref_store, const char *oldref, const char *newref, const char *logmsg); +typedef int write_invalid_head_ref_fn(struct ref_store *ref_store, + const char *dir); /* * Iterate over the references in `ref_store` whose names start with @@ -655,6 +657,7 @@ struct ref_storage_be { delete_refs_fn *delete_refs; rename_ref_fn *rename_ref; copy_ref_fn *copy_ref; + write_invalid_head_ref_fn *write_invalid_head_ref; ref_iterator_begin_fn *iterator_begin; read_raw_ref_fn *read_raw_ref;
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> --- Hi Han-Wen, 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. What do you think about rolling this patch first, and then implementing the following part inside the reftable as a function? int reftable_write_invalid_head_ref(struct *ref_store, const char *dir) { struct sb = STRBUF_INIT; /* the following adapted from this patch: */ + /* XXX this is cut & paste from reftable_init_db. */ + strbuf_addf(&sb, "%s/HEAD", dir); + write_file(sb.buf, "%s", "ref: refs/heads/.invalid\n"); + strbuf_reset(&sb); + + strbuf_addf(&sb, "%s/refs", sb_repo.buf); + safe_create_dir(sb.buf, 1); + strbuf_reset(&sb); + + strbuf_addf(&sb, "%s/refs/heads", sb_repo.buf); + write_file(sb.buf, "this repository uses the reftable format"); + strbuf_reset(&sb); + + strbuf_addf(&sb, "%s/reftable", sb_repo.buf); + safe_create_dir(sb.buf, 1); + strbuf_reset(&sb); return 0; } What do you think? Cheers, Stefan builtin/worktree.c | 6 +++--- refs.c | 5 +++++ refs.h | 3 +++ refs/debug.c | 10 ++++++++++ refs/files-backend.c | 13 +++++++++++++ refs/packed-backend.c | 7 +++++++ refs/refs-internal.h | 3 +++ 7 files changed, 44 insertions(+), 3 deletions(-)