refs: unify parse_worktree_ref() and ref_type()
PATCH 1/1 refs: unify parse_worktree_ref() and ref_type()
From: Han-Wen Nienhuys <[email protected]>
The logic to handle worktree refs (worktrees/NAME/REF and main-worktree/REF) existed in two places:
* ref_type() in refs.c
* parse_worktree_ref() in worktree.c
Collapse this logic together in one function parse_worktree_ref():
this avoids having to cross-check the result of parse_worktree_ref() and ref_type().
Introduce enum ref_worktree_type, which is slightly different from enum ref_type. The latter is a misleading name (one would think that 'ref_type' would have the symref option).
Instead, enum ref_worktree_type only makes explicit how a refname relates to a worktree. From this point of view, HEAD and refs/bisect/abc are the same: they specify the current worktree implicitly.
The files-backend must avoid packing refs/bisect/* and friends into packed-refs, so expose is_per_worktree_ref() separately.
---
refs: unify parse_worktree_ref() and ref_type()
The logic to handle worktree refs (worktrees/NAME/REF and main-worktree/REF) existed in two places:
* ref_type() in refs.c
* parse_worktree_ref() in worktree.c
Collapse this logic together in one function parse_worktree_ref(): this avoids having to cross-check the result of parse_worktree_ref() and ref_type().
Introduce enum ref_worktree_type, which is slightly different from enum ref_type. The latter is a misleading name (one would think that 'ref_type' would have the symref option).
Instead, enum ref_worktree_type only makes explicit how a refname relates to a worktree. From this point of view, HEAD and refs/bisect/abc are the same: they specify the current worktree implicitly.
The files-backend must avoid packing refs/bisect/* and friends into packed-refs, so expose is_per_worktree_ref() separately.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1325%2Fhanwen%2Fparse-worktree-ref-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1325/hanwen/parse-worktree-ref-v1
Pull-Request: https://github.com/git/git/pull/1325
builtin/reflog.c | 3 +- reflog.c | 13 ++----- refs.c | 75 ++++++++++++++++++++++++++-------------- refs.h | 29 ++++++++++++---- refs/files-backend.c | 80 +++++++++++++++++++------------------------ refs/packed-backend.c | 2 +- worktree.c | 59 ++++--------------------------- worktree.h | 10 ------ 8 files changed, 120 insertions(+), 151 deletions(-)
builtin/reflog.c +2 -1
@@ -26,7 +26,8 @@ static int collect_reflog(const char *ref, const struct object_id *oid, int unus* Avoid collecting the same shared ref multiple times because * they are available via all worktrees. */ -if (!worktree->is_current &&ref_type (ref) ==REF_TYPE_NORMAL ) +if (!worktree->is_current && +parse_worktree_ref (ref,NULL ,NULL ,NULL ) ==REF_WORKTREE_SHARED )return 0 ;strbuf_worktree_ref (worktree, &newref, ref);
reflog.c +3 -10
@@ -310,16 +310,9 @@ static int push_tip_to_list(const char *refname, const struct object_id *oid,static int is_head (const char *refname) { -switch (ref_type (refname)) { -case REF_TYPE_OTHER_PSEUDOREF : -case REF_TYPE_MAIN_PSEUDOREF : -if (parse_worktree_ref (refname,NULL ,NULL , &refname)) -BUG ("not a worktree ref: %s" , refname); -break ; -default : -break ; - } -return !strcmp (refname,"HEAD" ); +const char *stripped_refname; +parse_worktree_ref (refname,NULL ,NULL , &stripped_refname); +return !strcmp (stripped_refname,"HEAD" ); } void reflog_expiry_prepare(const char *refname,
refs.c +49 -26
93 lines not on this page · raw message
refs.h +22 -7
@@ -825,15 +825,30 @@ int parse_hide_refs_config(const char *var, const char *value, const char *);*/ int ref_is_hidden (const char *,const char *); -enum ref_type { -REF_TYPE_PER_WORKTREE ,/* refs inside refs/ but not shared */ -REF_TYPE_PSEUDOREF ,/* refs outside refs/ in current worktree */ -REF_TYPE_MAIN_PSEUDOREF ,/* pseudo refs from the main worktree */ -REF_TYPE_OTHER_PSEUDOREF ,/* pseudo refs from other worktrees */ -REF_TYPE_NORMAL ,/* normal/shared refs inside refs/ */ +/* Is this a per-worktree ref living in the refs/ namespace? */ +int is_per_worktree_ref (const char *refname); + +/* Describes how a refname relates to worktrees */ +enum ref_worktree_type { +REF_WORKTREE_CURRENT ,/* implicitly per worktree, eg. HEAD or +refs/bisect/SOMETHING */ +REF_WORKTREE_MAIN ,/* explicitly in main worktree, eg. +refs/main-worktree/HEAD */ +REF_WORKTREE_OTHER ,/* explicitly in named worktree, eg. +refs/worktrees/bla/HEAD */ +REF_WORKTREE_SHARED ,/* the default, eg. refs/heads/main */ }; -enum ref_type ref_type (const char *refname); +/* Parse a ref that possibly explicitly refers to a worktree ref +* (ie. either REFNAME, main-worktree/REFNAME or +* worktree/WORKTREE/REFNAME). If the name references a worktree +* implicitly or explicitly, return what kind it was. The +* worktree_name, worktree_name_length and refname argument maybe NULL. +*/ +enum ref_worktree_type parse_worktree_ref (const char *maybe_worktree_ref, +const char **worktree_name, +int *worktree_name_length, +const char **refname);enum expire_reflog_flags {EXPIRE_REFLOGS_DRY_RUN =1 <<0 ,
refs/files-backend.c +36 -44
123 lines not on this page · raw message
refs/packed-backend.c +1 -1
@@ -862,7 +862,7 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)while ((ok =next_record (iter)) ==ITER_OK ) {if (iter->flags &DO_FOR_EACH_PER_WORKTREE_ONLY && -ref_type (iter->base.refname) !=REF_TYPE_PER_WORKTREE ) + !is_per_worktree_ref (iter->base.refname))continue ; if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
worktree.c +7 -52
69 lines not on this page · raw message
worktree.h +0 -10
@@ -166,16 +166,6 @@ const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...) __attribute__((format (printf, 2, 3))); -/* -* Parse a worktree ref (i.e. with prefix main-worktree/ or -* worktrees/) and return the position of the worktree's name and -* length (or NULL and zero if it's main worktree), and ref. -* -* All name, name_length and ref arguments could be NULL. -*/ -int parse_worktree_ref (const char *worktree_ref,const char **name, -int *name_length,const char **ref); -/* * Return a refname suitable for access from the current ref store. */
base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b
Signature · 1 line
gitgitgadget
Quoted 7 lines · Han-Wen Nienhuys via GitGitGadget
From: Han-Wen Nienhuys <[email protected]>
The logic to handle worktree refs (worktrees/NAME/REF and main-worktree/REF) existed in two places:
*
ref_type()inrefs.c
Quoted 8 lines
*
parse_worktree_ref()inworktree.cCollapse this logic together in one function
parse_worktree_ref():
this avoids having to cross-check the result ofparse_worktree_ref()andref_type().
...
The files-backend must avoid packing refs/bisect/* and friends into packed-refs, so exposeis_per_worktree_ref()separately.
A sensible goal.
builtin/reflog.c static int collect_reflog(const char *ref, const struct object_id *oid, int unus · line 31
Quoted 12 lines
builtin/reflog.c +2
-1@@ -26,7 +26,8 @@ static int collect_reflog(const char *ref, const struct object_id *oid, int unus* Avoid collecting the same shared ref multiple times because * they are available via all worktrees. */ -if (!worktree->is_current &&ref_type (ref) ==REF_TYPE_NORMAL ) +if (!worktree->is_current && +parse_worktree_ref (ref,NULL ,NULL ,NULL ) ==REF_WORKTREE_SHARED )return 0 ;
We used to say "for a ref without anything magical" but now we say "for a ref that is not per worktree at all", and they mean the same thing. OK.
Quoted 9 lines
builtin/reflog.c +0
-0strbuf_worktree_ref (worktree, &newref, ref);reflog.c +0
-0@@ -310,16 +310,9 @@ static int push_tip_to_list(const char *refname, const struct object_id *oid,static int is_head (const char *refname) {
reflog.c static int push_tip_to_list(const char *refname, const struct object_id *oid, · line 322
Quoted 10 lines
reflog.c +0
-10-switch (ref_type (refname)) { -case REF_TYPE_OTHER_PSEUDOREF : -case REF_TYPE_MAIN_PSEUDOREF : -if (parse_worktree_ref (refname,NULL ,NULL , &refname)) -BUG ("not a worktree ref: %s" , refname); -break ; -default : -break ; - } -return !strcmp (refname,"HEAD" );
It used to do some sort of input validation with BUG() but it no longer does. Intended?
The new call to parse_worktree_ref() is only to strip the possible "main-worktree/" and/or "worktrees/$name/" names.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 742
Quoted 12 lines
reflog.c +5
-0+const char *stripped_refname; +parse_worktree_ref (refname,NULL ,NULL , &stripped_refname); +return !strcmp (stripped_refname,"HEAD" ); }@@ -738,37 +738,60 @@ static int is_pseudoref_syntax(const char *refname)return 0 ; } +/* HEAD is not a pseudoref, but it certainly uses the +* pseudoref syntax. */
/*
* Our multi-line comments have opening slash-aster
* and closing aster-slash on their own line. */
Not your fault but the fault of the original file contents and diff algorithms; the patch around here is simply unreadble because almost nothing in the preimage contributes to the result. So I'll remove '-' lines first before commenting.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 745
Quoted 3 lines
reflog.c +3
-0+enum ref_worktree_type parse_worktree_ref (const char *worktree_ref, +const char **name,int *name_length, +const char **ref)
It is hard to see what's input and what's output. You have comments in <refs.h> but the prototype uses different parameter names and they do not tell what is input and what is output, either.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 749
Quoted 4 lines
reflog.c +3
-0{ +const char *name_dummy; +int name_length_dummy; +const char *ref_dummy;
Style: Have blank here between the end of decls and the beginning of statements.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 755
Quoted 6 lines
reflog.c +6
-0+if (!name) + name = &name_dummy; +if (!name_length) + name_length = &name_length_dummy; +if (!ref) + ref = &ref_dummy;
I can guess from these that worktree_ref is the input and all three are to point at variables that receive optional output. The patch author shouldn't make the code reader guess.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 759
Quoted 4 lines
reflog.c +4
-0+ *ref = worktree_ref; +if (is_pseudoref_syntax (worktree_ref)) { +return REF_WORKTREE_CURRENT ; + }
Style: No braces around a single statement block.
OK, given a worktree_ref, FETCH_HEAD, MERGE_HEAD, etc. are all valid only for the current worktree.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 762
Quoted 3 lines
reflog.c +3
-0+if (is_per_worktree_ref (worktree_ref)) { +return REF_WORKTREE_CURRENT ; + }
Also the ones that are within a certain known namespaces, like "refs/bisect/*".
In the above two cases, name/name_length are undefined. Presumably the caller behaves well enough not to peek into them unless it is told to by the returned value REF_WORKTREE_OTHER?
reflog.c static int is_pseudoref_syntax(const char *refname) · line 763
Quoted 1 line
reflog.c +1
-0+if (skip_prefix (worktree_ref,"main-worktree/" , &worktree_ref)) {
The special syntax "main-worktree/$their_ref". We skip the prefix and then ...
reflog.c static int is_pseudoref_syntax(const char *refname) · line 765
Quoted 2 lines
reflog.c +2
-0+if (!*worktree_ref) +return -1 ;
Do we know if, or do we have control over, "enum ref_worktree_type" ends up being signed or unsigned? I am not sure if the callers in this patch even pays attention to the error condition.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 772
Quoted 7 lines
reflog.c +7
-0+ *name =NULL ; + *name_length =0 ; + *ref = worktree_ref; + +if (parse_worktree_ref (*ref,NULL ,NULL ,NULL ) == +REF_WORKTREE_CURRENT ) +return REF_WORKTREE_MAIN ;
In "main-worktree/$their_ref", if $their_ref is the per worktree name, then it is a ref specific to the primary worktree.
It is not clear what *name and *name_length are trying to return to the caller here.
Otherwise we fall through with worktree_ref that we have stripped main-worktree/ prefix, which means the original input
main-worktree/worktrees/foo/blah
is now
worktrees/foo/blah
and the next skip_prefix() will see that it begins with "worktrees/". Of course, if the initial input were
worktrees/foo/blah
then we wouldn't have skipped main-worktree/ prefix from it, and go to the next skip_prefix(). So from here on, we cannot tell which case the original input was.
But that is OK. Asking "give me the ref 'blah' in the worktree 'foo'" in the current worktree should yield the same answer to the question "give me the ref 'blah' in the worktree 'foo', as if I asked you to do so in the main worktree".
reflog.c static int is_pseudoref_syntax(const char *refname) · line 781
Quoted 9 lines
reflog.c +9
-0+ } +if (skip_prefix (worktree_ref,"worktrees/" , &worktree_ref)) { +const char *slash =strchr (worktree_ref,'/' ); + +if (!slash || slash == worktree_ref || !slash[1 ]) +return -1 ; + *name = worktree_ref; + *name_length = slash - worktree_ref; + *ref = slash +1 ;
OK, name and name_length are giving the name of the worktree to the caller here. But this is done even when we do not end up returning REF_WORKTREE_OTHER, which sounds eh, ugly.
In any case, the part after worktrees/$worktreename/ is a per worktree name, then we say "that's a per-worktree ref that belongs to $worktreename" by returning _OTHER and with *name/*name_length.
reflog.c static int is_pseudoref_syntax(const char *refname) · line 788
Quoted 7 lines
reflog.c +6
-0+if (parse_worktree_ref (*ref,NULL ,NULL ,NULL ) == +REF_WORKTREE_CURRENT ) +return REF_WORKTREE_OTHER ; + } + +return REF_WORKTREE_SHARED ;
And everything else is _SHARED.
The logic sounds sane enough, but the name/name_length being sometimes undefined and sometimes cleared feels yucky to me.
Quoted 8 lines
reflog.c +0
-0}long get_files_ref_lock_timeout_ms (void )refs.h +0
-0
refs.h int parse_hide_refs_config(const char *var, const char *value, const char *); · line 834
Quoted 10 lines
refs.h +7
-0@@ -825,15 +825,30 @@ int parse_hide_refs_config(const char *var, const char *value, const char *);*/ int ref_is_hidden (const char *,const char *); +/* Is this a per-worktree ref living in the refs/ namespace? */ +int is_per_worktree_ref (const char *refname); + +/* Describes how a refname relates to worktrees */ +enum ref_worktree_type { +REF_WORKTREE_CURRENT ,/* implicitly per worktree, eg. HEAD or +refs/bisect/SOMETHING */
OK.
refs.h int parse_hide_refs_config(const char *var, const char *value, const char *); · line 838
Quoted 4 lines
refs.h +4
-0+REF_WORKTREE_MAIN ,/* explicitly in main worktree, eg. +refs/main-worktree/HEAD */ + REF_WORKTREE_OTHER, /* explicitly in named worktree, eg. +refs/worktrees/bla/HEAD */
Hmph. Do we need refs/ prefix before main-worktree/ and worktrees/bla? The code we reviewed above seemed to skip "main-worktree/" and "worktrees/" with the assumption that there will be no "refs/" prefix before them. Probably just two typoes, as the comment before parse_worktree_ref() below also seems to assume that there is no refs/ before them.
refs.h int parse_hide_refs_config(const char *var, const char *value, const char *); · line 847
Quoted 10 lines
refs.h +7
-1+ REF_WORKTREE_SHARED, /* the default, eg. refs/heads/main */ }; -enum ref_type ref_type (const char *refname); +/* Parse a ref that possibly explicitly refers to a worktree ref +* (ie. either REFNAME, main-worktree/REFNAME or +* worktree/WORKTREE/REFNAME). If the name references a worktree +* implicitly or explicitly, return what kind it was. The +* worktree_name, worktree_name_length and refname argument maybe NULL. +*/
Here is my attempt to clarify what the comment tries to explain.
/*
* Parse maybe_worktree_ref (input) that possibly explicitly
* refers to a worktree ref (i.e. REFNAME, main-worktree/REFNAME,
* main-worktree/worktrees/WORKTREE/REFNAME, or
* worktrees/WORKTREE/REFNAME). Return what kind of ref
* (among the four kinds listed above) it is.
*
* The location pointed at by worktree_name and worktree_name_length
* are modified to point to "WORKTREE" part of such an input string
* when the returned value is REF_WORKTREE_OTHER.
* Otherwise they are undefined (they may still be smudged). *
* refname is made to piont to REFNAME part of the input string in
* all cases.
*
* These three output parameters are optional and NULL can
* be given for them.
*/
Quoted 4 lines
refs.h +4
-0+enum ref_worktree_type parse_worktree_ref (const char *maybe_worktree_ref, +const char **worktree_name, +int *worktree_name_length, +const char **refname);
Quoted 14 lines · Junio C Hamano
Otherwise we fall through with
worktree_refthat we have stripped main-worktree/ prefix, which means the original inputmain-worktree/worktrees/foo/blah
is now
worktrees/foo/blah
and the next
skip_prefix()will see that it begins with "worktrees/". Of course, if the initial input wereworktrees/foo/blah
Quoted 8 lines
then we wouldn't have skipped main-worktree/ prefix from it, and go to the next
skip_prefix(). So from here on, we cannot tell which case the original input was.But that is OK. Asking "give me the ref 'blah' in the worktree 'foo'" in the current worktree should yield the same answer to the question "give me the ref 'blah' in the worktree 'foo', as if I asked you to do so in the main worktree".
This makes me wonder...
I wonder if it makes the resulting code clearer to go fully recursive, unlike the posted code that says "if a recursive call says it is for current, that means it is for main worktree, and otherwise pretend as if the input did not have the prefix".
That is, something like
parse_worktree_ref(...) {
... prepare name, name_len and ref ...
if (skip_prefix(worktree_ref, "main-worktree/", &worktree_ref)) {
parsed = parse_worktree_ref(worktree_ref, name, name_len, ref);
switch (parsed) {
case REF_WORKTREE_CURRENT:
case REF_WORKTREE_MAIN:
return REF_WORKTREE_MAIN;
case REF_WORKTREE_OTHER:
case REF_WORKTREE_SHARED:
return parsed;
}
}
if (skip_prefix(worktree_ref, "worktrees/", &worktree_ref)) {
slash = strchr(worktree_ref, '/');
parsed = parse_worktree_ref(slash + 1, name, name_len, ref);
switch (parsed) {
case REF_WORKTREE_CURRENT:
return WORKTREE_OTHER; /* iffy */
case REF_WORKTREE_MAIN:
return REF_WORKTREE_MAIN;
case REF_WORKTREE_OTHER:
case REF_WORKTREE_SHARED:
return parsed;
}
}
/* Otherwise the input is like HEAD, MERGE_HEAD, refs/$BLAH */
... do whatever for these trivial cases ...}
And while composing this follow-up, I found another thing that is iffy. When you ask for "worktrees/foo/refs/bisect/good", the recursive call in "worktrees/foo" for the remainder says "current", and the posted patch and the above pseudocode would take CURRENT to mean "that other worktree's", and turns it into OTHER i.e. "not ours". But the code does so without even checking what our worktree is called. What happens if we are the owner of the "worktrees/foo" namespace? The same thing for "The main worktree says the given ref is 'current', so we can turn that into 'main'"---if our worktree is 'main', it is also 'current' and does not have to be turned into 'main' (even though it does not hurt).
Quoted 27 lines · Junio C Hamano
Quoted 14 lines · Junio C Hamano
Otherwise we fall through with
worktree_refthat we have stripped main-worktree/ prefix, which means the original inputmain-worktree/worktrees/foo/blah
is now
worktrees/foo/blah
and the next
skip_prefix()will see that it begins with "worktrees/". Of course, if the initial input wereworktrees/foo/blah
Quoted 8 lines
then we wouldn't have skipped main-worktree/ prefix from it, and go to the next
skip_prefix(). So from here on, we cannot tell which case the original input was.But that is OK. Asking "give me the ref 'blah' in the worktree 'foo'" in the current worktree should yield the same answer to the question "give me the ref 'blah' in the worktree 'foo', as if I asked you to do so in the main worktree".
This makes me wonder...
Quoted 7 lines
I wonder if it makes the resulting code clearer to go fully recursive, unlike the posted code that says "if a recursive call says it is for current, that means it is for main worktree, and otherwise pretend as if the input did not have the prefix".
That is, something like
...
The above may be a wrong suggestion, as it was solely guided by my reading of the posted code that looked as if it wanted to support something like "main-worktree/worktrees/foo/refs/heads/main". If that wasn't the intention, and we only want to support
1. a ref spelled in the traditional way before multiple worktree feature,
2. 1., with "main-worktree/" prefixed, or
3. 1., with "worktrees/<worktreename>/" prefixed
then a better approach would be to have a small helper parse_local_worktree_ref() and make the primary one into something like
parse_worktree_ref()
{
if (begins with "main-worktree/") {
strip main-worktree/ prefix;
switch (parse_local_worktree_ref(input minus prefix)) {
... the same switch to turn "ours" into "main's" ...
... but we probably want to special case if we are ...
... the main worktree ...
}
}
else if (begins with "worktrees/") {
strip worktrees/ prefix and learn worktree name;
switch (parse_local_worktree_ref(input minus prefix)) {
... the same switch to turn "ours" into "theirs" ...
... but we probably want to special case if we are ...
... the worktree in question ...
}
}
return parse_local_worktree_ref(input);
}and have parse_local_worktree_ref() handle psuedorefs, per-worktree-refs.
Quoted 4 lines · Junio C Hamano
then a better approach would be to have a small helper
parse_local_worktree_ref()and make the primary one into something like
...
Thanks, good idea. I'm sending you a v2.
Signature · 7 lines
Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Liana Sebastian
Quoted 7 lines · Han-Wen Nienhuys
Quoted 4 lines · Junio C Hamano
then a better approach would be to have a small helper
parse_local_worktree_ref()and make the primary one into something like
...Thanks, good idea. I'm sending you a v2.
Hmph, is that "v2" <[email protected]> or is there another version of it?
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Junio C Hamano
Quoted 4 lines
then a better approach would be to have a small helper
parse_local_worktree_ref()and make the primary one into something like
...Thanks, good idea. I'm sending you a v2.
is that "v2" <[email protected]> or is there another version of it?
I think so.
Quoted 1 line
Hmph,
Did I do something wrong?
Signature · 7 lines
Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Liana Sebastian
Quoted 9 lines · Han-Wen Nienhuys
Quoted 8 lines · Junio C Hamano
Quoted 7 lines · Junio C Hamano
Quoted 4 lines
then a better approach would be to have a small helper
parse_local_worktree_ref()and make the primary one into something like
...Thanks, good idea. I'm sending you a v2.
Quoted 8 lines
Quoted 2 lines
is that "v2" <[email protected]> or is there another version of it?
I think so.
Quoted 1 line
Hmph,
Did I do something wrong?
Not "wrong" per-se, but I was surprised by the patch that looked quite different from what I expected from your response to the "small helper" suggestion.
PATCH v2 1/1 refs: unify parse_worktree_ref() and ref_type()
In reply to Han-Wen Nienhuys via GitGitGadgetFrom: Han-Wen Nienhuys <[email protected]>
The logic to handle worktree refs (worktrees/NAME/REF and main-worktree/REF) existed in two places:
* ref_type() in refs.c
* parse_worktree_ref() in worktree.c
Collapse this logic together in one function parse_worktree_ref():
this avoids having to cross-check the result of parse_worktree_ref() and ref_type().
Introduce enum ref_worktree_type, which is slightly different from enum ref_type. The latter is a misleading name (one would think that 'ref_type' would have the symref option).
Instead, enum ref_worktree_type only makes explicit how a refname relates to a worktree. From this point of view, HEAD and refs/bisect/abc are the same: they specify the current worktree implicitly.
The files-backend must avoid packing refs/bisect/* and friends into packed-refs, so expose is_per_worktree_ref() separately.
---
refs: unify parse_worktree_ref() and ref_type()
The logic to handle worktree refs (worktrees/NAME/REF and main-worktree/REF) existed in two places:
* ref_type() in refs.c
* parse_worktree_ref() in worktree.c
Collapse this logic together in one function parse_worktree_ref(): this avoids having to cross-check the result of parse_worktree_ref() and ref_type().
Introduce enum ref_worktree_type, which is slightly different from enum ref_type. The latter is a misleading name (one would think that 'ref_type' would have the symref option).
Instead, enum ref_worktree_type only makes explicit how a refname relates to a worktree. From this point of view, HEAD and refs/bisect/abc are the same: they specify the current worktree implicitly.
The files-backend must avoid packing refs/bisect/* and friends into packed-refs, so expose is_per_worktree_ref() separately.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1325%2Fhanwen%2Fparse-worktree-ref-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1325/hanwen/parse-worktree-ref-v2
Pull-Request: https://github.com/git/git/pull/1325
Range-diff vs v1:
1: e2d19b80f76 ! 1: b1e6dc0edc7 refs: unify parse_worktree_ref() and ref_type()
@@ refs.c: static int is_pseudoref_syntax(const char *refname)
return 0;
}
-+ /* HEAD is not a pseudoref, but it certainly uses the
-+ * pseudoref syntax. */
++ /*
++ * HEAD is not a pseudoref, but it certainly uses the
++ * pseudoref syntax.
++ */
return 1;
}
-static int is_main_pseudoref_syntax(const char *refname)
-+enum ref_worktree_type parse_worktree_ref(const char *worktree_ref,
-+ const char **name, int *name_length,
-+ const char **ref)
- {
+-{
- return skip_prefix(refname, "main-worktree/", &refname) &&
- *refname &&
- is_pseudoref_syntax(refname);
--}
-+ const char *name_dummy;
-+ int name_length_dummy;
-+ const char *ref_dummy;
-+ if (!name)
-+ name = &name_dummy;
-+ if (!name_length)
-+ name_length = &name_length_dummy;
-+ if (!ref)
-+ ref = &ref_dummy;
++static int is_current_worktree_ref(const char *ref) {
++ return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
+ }
-static int is_other_pseudoref_syntax(const char *refname)
--{
++enum ref_worktree_type parse_worktree_ref(const char *maybe_worktree_ref,
++ const char **worktree_name, int *worktree_name_length,
++ const char **bare_refname)
+ {
- if (!skip_prefix(refname, "worktrees/", &refname))
- return 0;
- refname = strchr(refname, '/');
@@ refs.c: static int is_pseudoref_syntax(const char *refname)
- return 0;
- return is_pseudoref_syntax(refname + 1);
-}
-+ *ref = worktree_ref;
-+ if (is_pseudoref_syntax(worktree_ref)) {
-+ return REF_WORKTREE_CURRENT;
-+ }
++ const char *name_dummy;
++ int name_length_dummy;
++ const char *ref_dummy;
-enum ref_type ref_type(const char *refname)
-{
@@ refs.c: static int is_pseudoref_syntax(const char *refname)
- if (is_other_pseudoref_syntax(refname))
- return REF_TYPE_OTHER_PSEUDOREF;
- return REF_TYPE_NORMAL;
-+ if (is_per_worktree_ref(worktree_ref)) {
-+ return REF_WORKTREE_CURRENT;
-+ }
++ if (!worktree_name)
++ worktree_name = &name_dummy;
++ if (!worktree_name_length)
++ worktree_name_length = &name_length_dummy;
++ if (!bare_refname)
++ bare_refname = &ref_dummy;
+
-+ if (skip_prefix(worktree_ref, "main-worktree/", &worktree_ref)) {
-+ if (!*worktree_ref)
-+ return -1;
-+ *name = NULL;
-+ *name_length = 0;
-+ *ref = worktree_ref;
++ if (skip_prefix(maybe_worktree_ref, "worktrees/", bare_refname)) {
++ const char *slash = strchr(*bare_refname, '/');
+
-+ if (parse_worktree_ref(*ref, NULL, NULL, NULL) ==
-+ REF_WORKTREE_CURRENT)
-+ return REF_WORKTREE_MAIN;
-+ }
-+ if (skip_prefix(worktree_ref, "worktrees/", &worktree_ref)) {
-+ const char *slash = strchr(worktree_ref, '/');
++ *worktree_name = *bare_refname;
++ if (!slash) {
++ *worktree_name_length = strlen(*worktree_name);
+
-+ if (!slash || slash == worktree_ref || !slash[1])
-+ return -1;
-+ *name = worktree_ref;
-+ *name_length = slash - worktree_ref;
-+ *ref = slash + 1;
++ /* This is an error condition, and the caller tell because the bare_refname is "" */
++ *bare_refname = *worktree_name + *worktree_name_length;
++ return REF_WORKTREE_OTHER;
++ }
++
++ *worktree_name_length = slash - *bare_refname;
++ *bare_refname = slash + 1;
+
-+ if (parse_worktree_ref(*ref, NULL, NULL, NULL) ==
-+ REF_WORKTREE_CURRENT)
++ if (is_current_worktree_ref(*bare_refname))
+ return REF_WORKTREE_OTHER;
+ }
+
++ *worktree_name = NULL;
++ *worktree_name_length = 0;
++
++ if (skip_prefix(maybe_worktree_ref, "main-worktree/", bare_refname)
++ && is_current_worktree_ref(*bare_refname))
++ return REF_WORKTREE_MAIN;
++
++ *bare_refname = maybe_worktree_ref;
++ if (is_current_worktree_ref(maybe_worktree_ref))
++ return REF_WORKTREE_CURRENT;
++
+ return REF_WORKTREE_SHARED;
}
@@ refs.h: int parse_hide_refs_config(const char *var, const char *value, const cha
+ REF_WORKTREE_CURRENT, /* implicitly per worktree, eg. HEAD or
+ refs/bisect/SOMETHING */
+ REF_WORKTREE_MAIN, /* explicitly in main worktree, eg.
-+ refs/main-worktree/HEAD */
++ main-worktree/HEAD */
+ REF_WORKTREE_OTHER, /* explicitly in named worktree, eg.
-+ refs/worktrees/bla/HEAD */
++ worktrees/bla/HEAD */
+ REF_WORKTREE_SHARED, /* the default, eg. refs/heads/main */
};
-enum ref_type ref_type(const char *refname);
-+/* Parse a ref that possibly explicitly refers to a worktree ref
-+ * (ie. either REFNAME, main-worktree/REFNAME or
-+ * worktree/WORKTREE/REFNAME). If the name references a worktree
-+ * implicitly or explicitly, return what kind it was. The
-+ * worktree_name, worktree_name_length and refname argument maybe NULL.
++/*
++ * Parse a `maybe_worktree_ref` as a ref that possibly refers to a worktree ref
++ * (ie. either REFNAME, main-worktree/REFNAME or worktree/WORKTREE/REFNAME). It
++ * returns what kind of ref was found, and in case of REF_WORKTREE_OTHER, the
++ * worktree name is returned in `worktree_name` (pointing into
++ * `maybe_worktree_ref`) and `worktree_name_length`. The bare refname (the
++ * refname stripped of prefixes) is returned in `bare_refname`. The
++ * `worktree_name`, `worktree_name_length` and `bare_refname` arguments may be
++ * NULL.
+ */
+enum ref_worktree_type parse_worktree_ref(const char *maybe_worktree_ref,
+ const char **worktree_name,
+ int *worktree_name_length,
-+ const char **refname);
++ const char **bare_refname);
enum expire_reflog_flags {
EXPIRE_REFLOGS_DRY_RUN = 1 << 0,builtin/reflog.c | 3 +- reflog.c | 13 ++----- refs.c | 76 ++++++++++++++++++++++++++-------------- refs.h | 33 ++++++++++++++---- refs/files-backend.c | 80 +++++++++++++++++++------------------------ refs/packed-backend.c | 2 +- worktree.c | 59 ++++--------------------------- worktree.h | 10 ------ 8 files changed, 126 insertions(+), 150 deletions(-)
builtin/reflog.c +2 -1
@@ -26,7 +26,8 @@ static int collect_reflog(const char *ref, const struct object_id *oid, int unus* Avoid collecting the same shared ref multiple times because * they are available via all worktrees. */ -if (!worktree->is_current &&ref_type (ref) ==REF_TYPE_NORMAL ) +if (!worktree->is_current && +parse_worktree_ref (ref,NULL ,NULL ,NULL ) ==REF_WORKTREE_SHARED )return 0 ;strbuf_worktree_ref (worktree, &newref, ref);
reflog.c +3 -10
@@ -310,16 +310,9 @@ static int push_tip_to_list(const char *refname, const struct object_id *oid,static int is_head (const char *refname) { -switch (ref_type (refname)) { -case REF_TYPE_OTHER_PSEUDOREF : -case REF_TYPE_MAIN_PSEUDOREF : -if (parse_worktree_ref (refname,NULL ,NULL , &refname)) -BUG ("not a worktree ref: %s" , refname); -break ; -default : -break ; - } -return !strcmp (refname,"HEAD" ); +const char *stripped_refname; +parse_worktree_ref (refname,NULL ,NULL , &stripped_refname); +return !strcmp (stripped_refname,"HEAD" ); } void reflog_expiry_prepare(const char *refname,
refs.c +51 -25
95 lines not on this page · raw message
refs.h +26 -7
@@ -825,15 +825,34 @@ int parse_hide_refs_config(const char *var, const char *value, const char *);*/ int ref_is_hidden (const char *,const char *); -enum ref_type { -REF_TYPE_PER_WORKTREE ,/* refs inside refs/ but not shared */ -REF_TYPE_PSEUDOREF ,/* refs outside refs/ in current worktree */ -REF_TYPE_MAIN_PSEUDOREF ,/* pseudo refs from the main worktree */ -REF_TYPE_OTHER_PSEUDOREF ,/* pseudo refs from other worktrees */ -REF_TYPE_NORMAL ,/* normal/shared refs inside refs/ */ +/* Is this a per-worktree ref living in the refs/ namespace? */ +int is_per_worktree_ref (const char *refname); + +/* Describes how a refname relates to worktrees */ +enum ref_worktree_type { +REF_WORKTREE_CURRENT ,/* implicitly per worktree, eg. HEAD or +refs/bisect/SOMETHING */ +REF_WORKTREE_MAIN ,/* explicitly in main worktree, eg. +main-worktree/HEAD */ +REF_WORKTREE_OTHER ,/* explicitly in named worktree, eg. +worktrees/bla/HEAD */ +REF_WORKTREE_SHARED ,/* the default, eg. refs/heads/main */ }; -enum ref_type ref_type (const char *refname); +/* +* Parse a `maybe_worktree_ref` as a ref that possibly refers to a worktree ref +* (ie. either REFNAME, main-worktree/REFNAME or worktree/WORKTREE/REFNAME). It +* returns what kind of ref was found, and in case of REF_WORKTREE_OTHER, the +* worktree name is returned in `worktree_name` (pointing into +* `maybe_worktree_ref`) and `worktree_name_length`. The bare refname (the +* refname stripped of prefixes) is returned in `bare_refname`. The +* `worktree_name`, `worktree_name_length` and `bare_refname` arguments may be +* NULL. +*/ +enum ref_worktree_type parse_worktree_ref (const char *maybe_worktree_ref, +const char **worktree_name, +int *worktree_name_length, +const char **bare_refname);enum expire_reflog_flags {EXPIRE_REFLOGS_DRY_RUN =1 <<0 ,
refs/files-backend.c +36 -44
123 lines not on this page · raw message
refs/packed-backend.c +1 -1
@@ -862,7 +862,7 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)while ((ok =next_record (iter)) ==ITER_OK ) {if (iter->flags &DO_FOR_EACH_PER_WORKTREE_ONLY && -ref_type (iter->base.refname) !=REF_TYPE_PER_WORKTREE ) + !is_per_worktree_ref (iter->base.refname))continue ; if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&
worktree.c +7 -52
69 lines not on this page · raw message
worktree.h +0 -10
@@ -166,16 +166,6 @@ const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...) __attribute__((format (printf, 2, 3))); -/* -* Parse a worktree ref (i.e. with prefix main-worktree/ or -* worktrees/) and return the position of the worktree's name and -* length (or NULL and zero if it's main worktree), and ref. -* -* All name, name_length and ref arguments could be NULL. -*/ -int parse_worktree_ref (const char *worktree_ref,const char **name, -int *name_length,const char **ref); -/* * Return a refname suitable for access from the current ref store. */
base-commit: 805e0a68082a217f0112db9ee86a022227a9c81b
Signature · 1 line
gitgitgadget