Kill manual ref parsing code in worktree.c
cover Kill manual ref parsing code in worktree.c
A hundred years ago I added this code because a "standalone" ref parsing code was not available from refs.c and the file was going through some heavy changes that refactoring its ref parsing code out was not an option. I promised to kill this parse_ref() eventually. I'm fulfilling it today (or soon).
I would really like to you double check the approach I'm using here (using submodule interface for accessing refs from another worktree) since that may be the way forward to fix the "gc losing objects" in multi worktrees. I've given it lots of thoughts in the last 24 hours. Still can't find any fundamental flaw...
Nguyễn Thái Ngọc Duy (2):
refs.c: add resolve_ref_submodule()
worktree.c: use submodule interface to access refs from another worktree
branch.c | 3 +- refs.c | 20 +++++++++---- refs.h | 3 ++ worktree.c | 99 +++++++++++++++----------------------------------------------- worktree.h | 2 +- 5 files changed, 44 insertions(+), 83 deletions(-)
Signature · 1 line
2.11.0.157.gd943d85
PATCH 1/2 refs.c: add resolve_ref_submodule()
This is basically the extended version of resolve_gitlink_ref() where we have access to more info from the underlying resolve_ref_recursively() call.
--- refs.c | 20 ++++++++++++++------ refs.h | 3 +++ 2 files changed, 17 insertions(+), 6 deletions(-)
refs.c +14 -6
@@ -1325,18 +1325,18 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, resolve_flags, sha1, flags); } -int resolve_gitlink_ref (const char *submodule,const char *refname, -unsigned char *sha1) +const char *resolve_ref_submodule (const char *submodule,const char *refname, +int resolve_flags,unsigned char *sha1, +int *flags) {size_t len =strlen (submodule);struct ref_store *refs; -int flags;while (len && submodule[len -1 ] =='/' ) len--;if (!len) -return -1 ; +return NULL ;if (submodule[len]) {/* We need to strip off one or more trailing slashes */
@@ -1349,9 +1349,17 @@ int resolve_gitlink_ref(const char *submodule, const char *refname, }if (!refs) -return -1 ; +return NULL ; + +return resolve_ref_recursively (refs, refname, resolve_flags, sha1, flags); +} + +int resolve_gitlink_ref (const char *submodule,const char *refname, +unsigned char *sha1) +{ +int flags; -if (!resolve_ref_recursively (refs, refname,0 , sha1, &flags) || +if (!resolve_ref_submodule (submodule, refname,0 , sha1, &flags) ||is_null_sha1 (sha1))return -1 ;return 0 ;
refs.h +3 -0
@@ -88,6 +88,9 @@ int peel_ref(const char *refname, unsigned char *sha1);*/ int resolve_gitlink_ref (const char *submodule,const char *refname,unsigned char *sha1); +const char *resolve_ref_submodule (const char *submodule,const char *refname, +int resolve_flags,unsigned char *sha1, +int *flags);/* * Return true iff abbrev_name is a possible abbreviation for
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH 1/2] refs.c: add resolve_ref_submodule()
Quoted 61 lines · Nguyễn Thái Ngọc Duy
This is basically the extended version of
resolve_gitlink_ref()where we have access to more info from the underlyingresolve_ref_recursively()call.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 20 ++++++++++++++------ refs.h | 3 +++ 2 files changed, 17 insertions(+), 6 deletions(-)refs.c +14
-6@@ -1325,18 +1325,18 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags, resolve_flags, sha1, flags); } -int resolve_gitlink_ref (const char *submodule,const char *refname, -unsigned char *sha1) +const char *resolve_ref_submodule (const char *submodule,const char *refname, +int resolve_flags,unsigned char *sha1, +int *flags) {size_t len =strlen (submodule);struct ref_store *refs; -int flags;while (len && submodule[len -1 ] =='/' ) len--;if (!len) -return -1 ; +return NULL ;if (submodule[len]) {/* We need to strip off one or more trailing slashes */ @@ -1349,9 +1349,17 @@ int resolve_gitlink_ref(const char *submodule, const char *refname, }if (!refs) -return -1 ; +return NULL ; + +return resolve_ref_recursively (refs, refname, resolve_flags, sha1, flags); +} + +int resolve_gitlink_ref (const char *submodule,const char *refname, +unsigned char *sha1) +{ +int flags; -if (!resolve_ref_recursively (refs, refname,0 , sha1, &flags) || +if (!resolve_ref_submodule (submodule, refname,0 , sha1, &flags) ||is_null_sha1 (sha1))return -1 ;return 0 ;refs.h +0
-0
refs.h int peel_ref(const char *refname, unsigned char *sha1); · line 93
Quoted 6 lines
refs.h +3
-0@@ -88,6 +88,9 @@ int peel_ref(const char *refname, unsigned char *sha1);*/ int resolve_gitlink_ref (const char *submodule,const char *refname,unsigned char *sha1); +const char *resolve_ref_submodule (const char *submodule,const char *refname, +int resolve_flags,unsigned char *sha1, +int *flags);
This function is the analog of resolve_ref_unsafe(); i.e., it returns a pointer to either a static buffer or a pointer into the refname argument. Therefore, I think it should have "unsafe" in its name. And/or maybe there should be a safe version of the function analogous to resolve_refdup().
Moreover, this function has inherited the code for stripping trailing slashes from the submodule name. I have the feeling that this is a wart, not a feature, and that it would be sad to see it spread. How about moving the slash-stripping code to resolve_gitlink_ref() and making resolve_ref_submodule() assume that its submodule name is already clean?
It would be nice to have a docstring here.
I also have some higher-level concerns about the approach of this patch series, which I'll write about in a comment to patch 2/2.
Michael
PATCH 2/2 worktree.c: use submodule interface to access refs from another worktree
In reply to Nguyễn Thái Ngọc DuyThe patch itself is relatively simple: manual parsing code is replaced with a call to resolve_ref_submodule(). The manual parsing code must die because only refs/files-backend.c should do that. Why submodule here is a more interesting question.
From an outside look, any .git/worktrees/foo is seen as a "normal" repository. You can set GIT_DIR to it and have access to everything, even shared things that are not literally inside that directory, like object db or shared refs.
On top of that, linked worktrees point to those directories with ".git" files. These two make a linked worktree's path "X" a "submodule" (*) (**) because X/.git is a file that points to a repository somewhere.
As such, we can just linked worktree's path as a submodule. We just need to make sure they are unique because they are used to lookup submodule refs store.
Main worktree is a a bit trickier. If we stand at a linked worktree, we may still need to peek into main worktree's HEAD, for example. We can treat main worktree's path as submodule as well since git_path_submodule() can tolerate ".git" dirs, in addition to ".git" files.
The constraint is, if main worktree is X, then the git repo must be at X/.git. If the user separates .git repo far away and tell git to point to it via GIT_DIR or something else, then the "main worktree as submodule" trick fails. Within multiple worktree context, I think we can limit support to "standard" layout, at least for now.
(*) The differences in sharing object database and refs between submodules and linked worktrees don't really matter in this context.
(**) At this point, we may want to rename refs *_submodule API to something more neutral, maybe s/_submodule/_remote/
--- branch.c | 3 +- worktree.c | 99 +++++++++++++++----------------------------------------------- worktree.h | 2 +- 3 files changed, 27 insertions(+), 77 deletions(-)
branch.c +2 -1
@@ -354,7 +354,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)for (i =0 ; worktrees[i]; i++) {if (worktrees[i]->is_detached)continue ; -if (strcmp (oldref, worktrees[i]->head_ref)) +if (worktrees[i]->head_ref && +strcmp (oldref, worktrees[i]->head_ref))continue ; if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
worktree.c +24 -75
166 lines not on this page · raw message
worktree.h +1 -1
@@ -4,7 +4,7 @@struct worktree {char *path;char *id; -char *head_ref; +char *head_ref;/* NULL if HEAD is broken or detached */ char *lock_reason;/* internal use */ unsigned char head_sha1[20 ];int is_detached;
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
Quoted 2 lines · Nguyễn Thái Ngọc Duy
(**) At this point, we may want to rename refs
*_submoduleAPI to something more neutral, maybe s/_submodule/_remote/
I agree on (**), except that I am not sure if /_remote/ is a better name, because there is already a concept of a "remote" as well as "remote-tracking" in Git. (Usually it is not reachable on the same FS, but resides on another machine).
So if we were to do s/_submodule/_remote/, we'd have e.g.
for_each_ref_remote
which could mean that we do networking things to obtain the actual remote refs or just talk about remote tracking refs.
My gut reaction would be to s/submodule/alternative/ here, but we also have a thing called alternates already.
So we're looking for a name that describes refs for both worktrees as well as submodules. (Not sure if we can generalize to also include alternates, too)
And the one thing they share is that they have their refs "not at the usual place", e.g. not at .git/refs but rather at .git/{modules,worktrees}.
Recently we had a tangential discussion in submodule land about the different places, when adding the
"git submodule absorbgitdirs" command, that moves the submodule/.git directory into .git/modules/<name>. We chose "absorb" here as the name, because it was moved into the .git/ area.
So maybe one of:
s/submodule_ref/unusual_ref/
(to emphasize it is not a regular ref inside the repo, so:) s/submodule_ref/irregular_ref/
s/resolve_ref_submodule/resolve_ref_out_of_place/ s/resolve_ref_submodule/resolve_ref_gitlink_pointed/ s/resolve_ref_submodule/resolve_ref_linked_pointer/
would be my current thinking.
Thanks,
Stefan
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
Quoted 8 lines · Stefan Beller
Quoted 2 lines · Nguyễn Thái Ngọc Duy
(**) At this point, we may want to rename refs
*_submoduleAPI to something more neutral, maybe s/_submodule/_remote/I agree on (**), except that I am not sure if /_remote/ is a better name, because there is already a concept of a "remote" as well as "remote-tracking" in Git. (Usually it is not reachable on the same FS, but resides on another machine).
I agree with you that the concept of remote is quite detached from the concept of wt and submodule whose refs need to be peeked at from the local repository. After all, "remote" tracking branches are part of local repository's refs.
Quoted 2 lines
My gut reaction would be to s/submodule/alternative/ here, but we also have a thing called alternates already.
... and I tend to think that is far closer a concept. You borrow objects from your alternate object store, and that alternate object store may have its own set of refs you would need to peek when you are computing reachability from refs.
Also don't we already enumerate such refs that pin objects in the alternate object store when doing object transfer negotiation in order to send ".have" entries for their tips? What API do we use to do that, I wonder.
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
In reply to Nguyễn Thái Ngọc DuyQuoted 1 line · Nguyễn Thái Ngọc Duy
As such, we can just linked worktree's path as a submodule. We just need
Lack of verb made me read this three times. "We can just treat linked worktree's path as if it were a submodule"?
I agree with you that the "submodule" in the name is misleading. We would want to be able to walk refs from other local repositories (and repository-like entities, like the .git/ thing in a liked worktree), like our own submodule, or the repository we borrow objects from via the alternate mechanism.
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
In reply to Nguyễn Thái Ngọc DuyQuoted 29 lines · Nguyễn Thái Ngọc Duy
The patch itself is relatively simple: manual parsing code is replaced with a call to
resolve_ref_submodule(). The manual parsing code must die because onlyrefs/files-backend.cshould do that. Why submodule here is a more interesting question.From an outside look, any .git/worktrees/foo is seen as a "normal" repository. You can set
GIT_DIRto it and have access to everything, even shared things that are not literally inside that directory, like object db or shared refs.On top of that, linked worktrees point to those directories with ".git" files. These two make a linked worktree's path "X" a "submodule" (*) (**) because X/.git is a file that points to a repository somewhere.
As such, we can just linked worktree's path as a submodule. We just need to make sure they are unique because they are used to lookup submodule refs store.
Main worktree is a a bit trickier. If we stand at a linked worktree, we may still need to peek into main worktree's HEAD, for example. We can treat main worktree's path as submodule as well since
git_path_submodule()can tolerate ".git" dirs, in addition to ".git" files.The constraint is, if main worktree is X, then the git repo must be at X/.git. If the user separates .git repo far away and tell git to point to it via
GIT_DIRor something else, then the "main worktree as submodule" trick fails. Within multiple worktree context, I think we can limit support to "standard" layout, at least for now.
Quoted 5 lines
(*) The differences in sharing object database and refs between submodules and linked worktrees don't really matter in this context.
(**) At this point, we may want to rename refs
*_submoduleAPI to something more neutral, maybe s/_submodule/_remote/
It is unquestionably a good goal to avoid parsing references outside of refs/files-backend.c. But I'm not a fan of this approach.
There are two meanings of the concept of a "ref store", and I think this change muddles them:
1. The references that happen to be *physically* stored in a particular location, for example the refs/bisect/* references in a worktree.
2. The references that *logically* should be considered part of a particular repository. This might require stitching together references from multiple sources, for example HEAD and refs/bisect from a worktree's own directory with other references from the main repository.
Either of these concepts can be implemented via the ref_store abstraction.
The ref_store for a submodule should represent the references logically visible from the submodule. The main program shouldn't care whether the references are stored in a single physical location or spread across multiple locations (for example, if the submodule were itself a linked worktree).
The ref_store that you want here for a worktree is not the worktree's *logical* ref_store. You want the worktree's *physical* ref_store. Mixing logical and physical reference stores together is a bad idea (even if we were willing to ignore the fact that worktrees are not submodules in the accepted sense of the word).
The point of my submodule-hash branch [1] was to separate these concepts better by breaking the current 1:1 connection between ref_stores and submodules. This would allow ref_stores to be created for other purposes, such as to represent worktree refs. If you want the *logical* ref_store for a submodule, you access it through the submodule_ref_stores table. If you want the *physical* ref_store for a worktree, you should access it through a different table.
I think the best solution would be to expose the concept of ref_store in the public refs API. Then users of submodules would essentially do
struct ref_store *refs = get_submodule_refs(submodule_path);
... resolve_ref_recursively(refs, refname, 0, sha1, &flags) ...
... for_each_ref(refs, fn, cb_data) ...whereas for a worktree you'd have to look up the ref_store instance somewhere else (or maybe keep it as part of some worktree structure, if there is one) but you would use it via the same API.
Michael
[1] https://github.com/mhagger/git
Quoted 213 lines
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- branch.c | 3 +- worktree.c | 99 +++++++++++++++----------------------------------------------- worktree.h | 2 +- 3 files changed, 27 insertions(+), 77 deletions(-)branch.c +2
-1@@ -354,7 +354,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)for (i =0 ; worktrees[i]; i++) {if (worktrees[i]->is_detached)continue ; -if (strcmp (oldref, worktrees[i]->head_ref)) +if (worktrees[i]->head_ref && +strcmp (oldref, worktrees[i]->head_ref))continue ; if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),worktree.c +24
-75@@ -19,54 +19,24 @@ void free_worktrees(struct worktree **worktrees)free (worktrees); } -/* -* read 'path_to_ref' into 'ref'. Also if is_detached is not NULL, -* set is_detached to 1 (0) if the ref is detached (is not detached). -* -* $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so -* for linked worktrees, `resolve_ref_unsafe()` won't work (it uses -* git_path). Parse the ref ourselves. -* -* return -1 if the ref is not a proper ref, 0 otherwise (success) -*/ -static int parse_ref (char *path_to_ref,struct strbuf *ref,int *is_detached) -{ -if (is_detached) - *is_detached =0 ; -if (!strbuf_readlink (ref, path_to_ref,0 )) { -/* HEAD is symbolic link */ -if (!starts_with (ref->buf,"refs/" ) || -check_refname_format (ref->buf,0 )) -return -1 ; - }else if (strbuf_read_file (ref, path_to_ref,0 ) >=0 ) { -/* textual symref or detached */ -if (!starts_with (ref->buf,"ref:" )) { -if (is_detached) - *is_detached =1 ; - }else { -strbuf_remove (ref,0 ,strlen ("ref:" )); -strbuf_trim (ref); -if (check_refname_format (ref->buf,0 )) -return -1 ; - } - }else -return -1 ; -return 0 ; -} -/** -* Add the head_sha1 and head_ref (if not detached) to the given worktree +* Update head_sha1, head_ref and is_detached of the given worktree */ -static void add_head_info (struct strbuf *head_ref,struct worktree *worktree) +static void add_head_info (struct worktree *wt) { -if (head_ref->len) { -if (worktree->is_detached) { -get_sha1_hex (head_ref->buf, worktree->head_sha1); - }else { -resolve_ref_unsafe (head_ref->buf,0 , worktree->head_sha1,NULL ); - worktree->head_ref =strbuf_detach (head_ref,NULL ); - } - } +int flags; +const char *target; + + target =resolve_ref_submodule (wt->path,"HEAD" , +RESOLVE_REF_READING , + wt->head_sha1, &flags); +if (!target) +return ; + +if (flags &REF_ISSYMREF ) + wt->head_ref =xstrdup (target); +else + wt->is_detached =1 ; }/** @@ -77,9 +47,7 @@ static struct worktree *get_main_worktree(void)struct worktree *worktree =NULL ;struct strbuf path =STRBUF_INIT ;struct strbuf worktree_path =STRBUF_INIT ; -struct strbuf head_ref =STRBUF_INIT ;int is_bare =0 ; -int is_detached =0 ;strbuf_add_absolute_path (&worktree_path,get_git_common_dir ()); is_bare = !strbuf_strip_suffix (&worktree_path,"/.git" );@@ -91,13 +59,10 @@ static struct worktree *get_main_worktree(void) worktree =xcalloc (1 ,sizeof (*worktree)); worktree->path =strbuf_detach (&worktree_path,NULL ); worktree->is_bare = is_bare; - worktree->is_detached = is_detached; -if (!parse_ref (path.buf, &head_ref, &is_detached)) -add_head_info (&head_ref, worktree); +add_head_info (worktree);strbuf_release (&path);strbuf_release (&worktree_path); -strbuf_release (&head_ref);return worktree; }@@ -106,8 +71,6 @@ static struct worktree *get_linked_worktree(const char *id)struct worktree *worktree =NULL ;struct strbuf path =STRBUF_INIT ;struct strbuf worktree_path =STRBUF_INIT ; -struct strbuf head_ref =STRBUF_INIT ; -int is_detached =0 ;if (!id)die ("Missing linked worktree name" );@@ -127,19 +90,14 @@ static struct worktree *get_linked_worktree(const char *id)strbuf_reset (&path);strbuf_addf (&path,"%s/worktrees/%s/HEAD" ,get_git_common_dir (), id); -if (parse_ref (path.buf, &head_ref, &is_detached) <0 ) - goto done; - worktree =xcalloc (1 ,sizeof (*worktree)); worktree->path =strbuf_detach (&worktree_path,NULL ); worktree->id =xstrdup (id); - worktree->is_detached = is_detached; -add_head_info (&head_ref, worktree); +add_head_info (worktree); done:strbuf_release (&path);strbuf_release (&worktree_path); -strbuf_release (&head_ref);return worktree; }@@ -334,8 +292,6 @@ const struct worktree *find_shared_symref(const char *symref, const char *target) {const struct worktree *existing =NULL ; -struct strbuf path =STRBUF_INIT ; -struct strbuf sb =STRBUF_INIT ;static struct worktree **worktrees;int i =0 ;@@ -345,6 +301,10 @@ const struct worktree *find_shared_symref(const char *symref,for (i =0 ; worktrees[i]; i++) {struct worktree *wt = worktrees[i]; +const char *symref_target; +unsigned char sha1[20 ]; +int flags; +if (wt->is_bare)continue ;@@ -359,25 +319,14 @@ const struct worktree *find_shared_symref(const char *symref, } } -strbuf_reset (&path); -strbuf_reset (&sb); -strbuf_addf (&path,"%s/%s" , -get_worktree_git_dir (wt), - symref); - -if (parse_ref (path.buf, &sb,NULL )) { -continue ; - } - -if (!strcmp (sb.buf, target)) { + symref_target =resolve_ref_submodule (wt->path, symref,0 , + sha1, &flags); +if ((flags &REF_ISSYMREF ) && !strcmp (symref_target, target)) { existing = wt;break ; } } -strbuf_release (&path); -strbuf_release (&sb); -return existing; }worktree.h +1
-1@@ -4,7 +4,7 @@struct worktree {char *path;char *id; -char *head_ref; +char *head_ref;/* NULL if HEAD is broken or detached */ char *lock_reason;/* internal use */ unsigned char head_sha1[20 ];int is_detached;
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
Quoted 12 lines · Michael Haggerty
There are two meanings of the concept of a "ref store", and I think this change muddles them:
1. The references that happen to be *physically* stored in a particular location, for example the
refs/bisect/*references in a worktree.2. The references that *logically* should be considered part of a particular repository. This might require stitching together references from multiple sources, for example
HEADandrefs/bisectfrom a worktree's own directory with other references from the main repository.
Quoted 7 lines
Either of these concepts can be implemented via the
ref_storeabstraction.
...
Theref_storethat you want here for a worktree is not the worktree's *logical*ref_store. You want the worktree's *physical*ref_store. Mixing logical and physical reference stores together is a bad idea (even if we were willing to ignore the fact that worktrees are not submodules in the accepted sense of the word).
I am not quite sure what mental model you are suggesting as a preferred solution. We can
- represent a set of refs stored for a particular worktree (i.e. HEAD, refs/bisect, and refs/worktree/<name>, iirc), as bunch of ref_stores;
- represent a set of refs shared across a set of worktrees that share the primary one, as another ref_store;
- a caller who wants to get a "logical" view of a single worktree user can pick one of the first kind and union that with the second one, and represent the result as a (synthetic) ref_store.
The third one is "stitching together from multiple sources". By "mixing logical and physical is a bad idea", do you mean that the same abstraction "ref_store" should not be used for the first two (which are physical) and the third one (which is logical)? Do you want to call the first two "physical_ref_store"and the last one "ref_store" and keep them distinct?
For the purpose of anchoring objects in the object store shared by multiple worktrees, we can either iterate over all the ref_stores of the first two kind, or iterate over all the ref_stores of the third kind for all worktrees. The latter of course is less efficient as the enumeration
for worktree in all worktrees:
for ref in get_ref_store(worktree)
mark tip of ref reachable
will work on all the shared refs multiple times, but as an abstraction that may be simpler. The alternative of working at the physical level would be more efficient
for worktree in all worktrees:
for ref in get_ref_store_specific_to_worktree(worktree):
mark tip of ref reachable
for ref in get_ref_store_shared_across_worktrees():
mark tip of ref reachable
but this consumer now _knows_ how the logical ref_store of a worktree is constructed (i.e. by combining the two ref_stores), which appears as a layering violation.
I am however not sure if these issues are what you are driving at, and what exact design you are suggesting.
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
Quoted 36 lines · Junio C Hamano
Quoted 12 lines · Michael Haggerty
There are two meanings of the concept of a "ref store", and I think this change muddles them:
1. The references that happen to be *physically* stored in a particular location, for example the
refs/bisect/*references in a worktree.2. The references that *logically* should be considered part of a particular repository. This might require stitching together references from multiple sources, for example
HEADandrefs/bisectfrom a worktree's own directory with other references from the main repository.Quoted 7 lines
Either of these concepts can be implemented via the
ref_storeabstraction.
...
Theref_storethat you want here for a worktree is not the worktree's *logical*ref_store. You want the worktree's *physical*ref_store. Mixing logical and physical reference stores together is a bad idea (even if we were willing to ignore the fact that worktrees are not submodules in the accepted sense of the word).I am not quite sure what mental model you are suggesting as a preferred solution. We can
- represent a set of refs stored for a particular worktree (i.e. HEAD, refs/bisect, and refs/worktree/<name>, iirc), as bunch of
ref_stores;- represent a set of refs shared across a set of worktrees that share the primary one, as another
ref_store;- a caller who wants to get a "logical" view of a single worktree user can pick one of the first kind and union that with the second one, and represent the result as a (synthetic)
ref_store.
Quoted 6 lines
The third one is "stitching together from multiple sources". By "mixing logical and physical is a bad idea", do you mean that the same abstraction "
ref_store" should not be used for the first two (which are physical) and the third one (which is logical)? Do you want to call the first two "physical_ref_store"and the last one "ref_store" and keep them distinct?
The existing ref_store abstraction, I think, is capable of representing either kind of reference store. The stitching together to get the "logical" view of a worktree should probably happen within the refs code rather than forcing callers to deal with it. But yes, I think that code should put together a compound ref_store object that delegates to multiple underlying ref_store objects as you've described.
Which kind of ref_store * you have in your hand would depend on where you got it. If you call the hypothetical get_submodule_refs() function, you would get a ref_store * representing the references that are logically visible from that submodule. There might be a separate get_worktree_specific_refs() that returns a ref_store * representing the worktree-specific references physically stored for the worktree. But maybe the latter is not even necessary; see below.
Quoted 20 lines
For the purpose of anchoring objects in the object store shared by multiple worktrees, we can either iterate over all the
ref_storesof the first two kind, or iterate over all theref_storesof the third kind for all worktrees. The latter of course is less efficient as the enumerationfor worktree in all worktrees:
for ref inget_ref_store(worktree)
mark tip of ref reachablewill work on all the shared refs multiple times, but as an abstraction that may be simpler. The alternative of working at the physical level would be more efficient
for worktree in all worktrees:
for ref inget_ref_store_specific_to_worktree(worktree):
mark tip of ref reachable
for ref inget_ref_store_shared_across_worktrees():
mark tip of ref reachable
Quoted 6 lines
but this consumer now _knows_ how the logical
ref_storeof a worktree is constructed (i.e. by combining the tworef_stores), which appears as a layering violation.I am however not sure if these issues are what you are driving at, and what exact design you are suggesting.
Reachability is a special case, because it needs all of the references that refer to a particular object store, even though the reference names might overlap. I personally think that reachability roots should be requested via a new refs API call separate from for_each_rawref() (or whatever is used now). Internally it would be implemented much like your second "efficient" algorithm, but the implementation would be within the refs code, and the caller could remain ignorant of those details.
Externally, it might not even want to pass the caller the real reference names (I assume that callers mainly only use the reference names for diagnostic messages). For example, it might want to report references HEAD and refs/bisect/bad in worktree foo under the pseudonyms worktree/foo/HEAD and worktree/foo/refs/bisect/bad, so that they can be distinguished from any homonyms in the main repo and in other worktrees. If you ask for the reachability roots while in a worktree, it would either automatically crawl up to the main repository and across to sibling worktrees to get the full set of reachability roots, or maybe it would refuse to run at all (if we want to require such commands to be executed from the main repo).
I don't know exactly who would be the consumers of the reachability roots, so maybe there are some problems with this suggestion.
Michael
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
In reply to Michael HaggertyQuoted 2 lines · Michael Haggerty
It is unquestionably a good goal to avoid parsing references outside of
refs/files-backend.c. But I'm not a fan of this approach.
Yes. But in this context it was more of a guinea pig. I wanted something simple enough to code up show we can see what the approach looked like. Good thing I did it.
Quoted 15 lines
There are two meanings of the concept of a "ref store", and I think this change muddles them:
1. The references that happen to be *physically* stored in a particular location, for example the
refs/bisect/*references in a worktree.2. The references that *logically* should be considered part of a particular repository. This might require stitching together references from multiple sources, for example
HEADandrefs/bisectfrom a worktree's own directory with other references from the main repository.Either of these concepts can be implemented via the
ref_storeabstraction.
Quoted 8 lines
The
ref_storefor a submodule should represent the references logically visible from the submodule. The main program shouldn't care whether the references are stored in a single physical location or spread across multiple locations (for example, if the submodule were itself a linked worktree).The
ref_storethat you want here for a worktree is not the worktree's *logical*ref_store. You want the worktree's *physical*ref_store.
Yep.
Quoted 9 lines
Mixing logical and physical reference stores together is a bad idea (even if we were willing to ignore the fact that worktrees are not submodules in the accepted sense of the word).
...
I think the best solution would be to expose the concept of
ref_storein the public refs API. Then users of submodules would essentially do
Quoted 7 lines
struct ref_store *refs = get_submodule_refs(submodule_path); ... resolve_ref_recursively(refs, refname, 0, sha1, &flags) ... ... for_each_ref(refs, fn, cb_data) ...whereas for a worktree you'd have to look up the
ref_storeinstance somewhere else (or maybe keep it as part of some worktree structure, if there is one) but you would use it via the same API.
Oh I was going to reply to Stefan about his comment to my (**) footnote. Something along the this line
"Ideally we would introduce a new set of api, maybe with refs_ prefix, that takes a refs_store. Then submodule people can get a ref store somewhere and pass to it. Worktree people get maybe some other refs store for it. The "old" api like for_each_ref() is a thin wrapper around it, just like read_cache() vs read_index(&the_index). If the *_submodule does not see much use, we might as well kill it and use the generic refs_*".
If I didn't misunderstood anything else, then I think we're on the same page.
Now I need to see if I can get there in a reasonable time frame (so I can fix my "gc in worktree" problem properly) or I would need something temporary but not so hacky. I'll try to make this new api and see how it works out. If you think I should not do it right away, for whatever reason, stop me now.
Signature · 1 line
Duy
Re: [PATCH 2/2] worktree.c: use submodule interface to access refs from another worktree
Quoted 62 lines · Duy Nguyen
Quoted 2 lines · Michael Haggerty
It is unquestionably a good goal to avoid parsing references outside of
refs/files-backend.c. But I'm not a fan of this approach.Yes. But in this context it was more of a guinea pig. I wanted something simple enough to code up show we can see what the approach looked like. Good thing I did it.
Quoted 15 lines
There are two meanings of the concept of a "ref store", and I think this change muddles them:
1. The references that happen to be *physically* stored in a particular location, for example the
refs/bisect/*references in a worktree.2. The references that *logically* should be considered part of a particular repository. This might require stitching together references from multiple sources, for example
HEADandrefs/bisectfrom a worktree's own directory with other references from the main repository.Either of these concepts can be implemented via the
ref_storeabstraction.Quoted 8 lines
The
ref_storefor a submodule should represent the references logically visible from the submodule. The main program shouldn't care whether the references are stored in a single physical location or spread across multiple locations (for example, if the submodule were itself a linked worktree).The
ref_storethat you want here for a worktree is not the worktree's *logical*ref_store. You want the worktree's *physical*ref_store.Yep.
Quoted 9 lines
Mixing logical and physical reference stores together is a bad idea (even if we were willing to ignore the fact that worktrees are not submodules in the accepted sense of the word).
...
I think the best solution would be to expose the concept of
ref_storein the public refs API. Then users of submodules would essentially doQuoted 7 lines
struct ref_store *refs = get_submodule_refs(submodule_path); ... resolve_ref_recursively(refs, refname, 0, sha1, &flags) ... ... for_each_ref(refs, fn, cb_data) ...whereas for a worktree you'd have to look up the
ref_storeinstance somewhere else (or maybe keep it as part of some worktree structure, if there is one) but you would use it via the same API.Oh I was going to reply to Stefan about his comment to my (**) footnote. Something along the this line
"Ideally we would introduce a new set of api, maybe with
refs_prefix, that takes arefs_store. Then submodule people can get a ref store somewhere and pass to it. Worktree people get maybe some other refs store for it. The "old" api likefor_each_ref()is a thin wrapper around it, just likeread_cache()vsread_index(&the_index). If the*_submoduledoes not see much use, we might as well kill it and use the generic refs_*".
Quoted 7 lines
If I didn't misunderstood anything else, then I think we're on the same page.
Now I need to see if I can get there in a reasonable time frame (so I can fix my "gc in worktree" problem properly) or I would need something temporary but not so hacky. I'll try to make this new api and see how it works out. If you think I should not do it right away, for whatever reason, stop me now.
Sounds good. A couple of hints and points to remember:
* I think struct ref_store * should remain opaque outside of the refs code.
* The virtual function interface embodied in struct ref_storage_be isn't meant to be an external interface (i.e., don't just expose it and have external callers use it directly).
* One important distinction between the main reference store and submodule reference stores is that we know the object store for the former but not the latter. That implies that some operations are, or should be, impossible for submodules (e.g., anything that involves looking up objects, including peeling refs). However, we *do* know the object store for linked worktrees. So they don't have to be as dumbed-down as submodule ref stores. (This might be irrelevant if you don't need any additional features for your current purposes.)
Also, I just sent my submodule-hash patch series to the mailing list in case you want to build on that.
Michael
Quoted 5 lines · Nguyễn Thái Ngọc Duy
A hundred years ago I added this code because a "standalone" ref parsing code was not available from
refs.cand the file was going through some heavy changes that refactoring its ref parsing code out was not an option. I promised to kill thisparse_ref()eventually. I'm fulfilling it today (or soon).
;-)
Thanks. And thanks for looping Michael in. I'd appreciate his input in this area.
Quoted 5 lines
I would really like to you double check the approach I'm using here (using submodule interface for accessing refs from another worktree) since that may be the way forward to fix the "gc losing objects" in multi worktrees. I've given it lots of thoughts in the last 24 hours. Still can't find any fundamental flaw...
I see that you posted a separate message outlining the idea yesterday and I didn't see any response (and I was sick and lacked energy to think it through); I think the basic approach to use "an API to bring set of refs hidden from your normal view" is sensible. Except for the unfortunate naming of the interface that makes it sound as if it is only to access submodules, but that is where the feature original came from, so let's not complain too loudly ;-)
Quoted 10 lines
Nguyễn Thái Ngọc Duy (2):
refs.c: addresolve_ref_submodule()
worktree.c: use submodule interface to access refs from another worktreebranch.c | 3 +- refs.c | 20 +++++++++---- refs.h | 3 ++ worktree.c | 99 +++++++++++++++----------------------------------------------- worktree.h | 2 +- 5 files changed, 44 insertions(+), 83 deletions(-)
v2 cover Kill manual ref parsing code in worktree.c
In reply to Nguyễn Thái Ngọc Duyv2 still kills parse_ref(), but the series now depends on my other series [1] and as a result does not make any confusing "submodule" calls any more.
It also kills another function, introduced for multi-worktree, that should not have been there to begin with. Good riddance.
Again, the naming convention with refs_ prefix for new APIs may not be the best idea...
[1] public-inbox.org/git/[email protected]
Nguyễn Thái Ngọc Duy (5):
refs: introduce get_worktree_ref_store()
refs.c: add refs_resolve_ref_unsafe()
worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
refs: add refs_create_symref()
refs: kill set_worktree_head_symref()
branch.c | 15 ++++---- refs.c | 58 ++++++++++++++++++++++++----- refs.h | 21 ++++++----- refs/files-backend.c | 43 +--------------------- refs/refs-internal.h | 5 --- worktree.c | 102 ++++++++++++++------------------------------------- worktree.h | 2 +- 7 files changed, 98 insertions(+), 148 deletions(-)
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 1/5 refs: introduce get_worktree_ref_store()
files-backend at this point is still aware of the per-repo/worktree separation in refs, so it can handle a linked worktree.
Note: accessing a worktree of a submodule remains unaddressed. Perhaps after get_worktrees() can access submodule (or rather a new function get_submodule_worktrees(), that lists worktrees of a submodule), we can update this function to work with submodules as well.
--- refs.c | 27 +++++++++++++++++++++++++++ refs.h | 2 ++ 2 files changed, 29 insertions(+)
refs.c +27 -0
@@ -10,6 +10,7 @@#include "object.h" #include "tag.h" #include "submodule.h" +#include "worktree.h" /* * List of all available backends
@@ -1486,6 +1487,32 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return refs; } +struct ref_store *get_worktree_ref_store (const struct worktree *wt) +{ +struct ref_store *refs; + +if (wt->is_current) +return get_main_ref_store (); + +/* +* We share the same hash map with submodules for +* now. submodule paths are always relative (to topdir) while +* worktree paths are always absolute. No chance of conflict. +*/ + refs =lookup_submodule_ref_store (wt->path); +if (refs) +return refs; + +if (wt->id) + refs =ref_store_init (git_common_path ("worktrees/%s" , wt->id)); +else + refs =ref_store_init (get_git_common_dir ()); + +if (refs) +register_submodule_ref_store (refs, wt->path); +return refs; +} +void base_ref_store_init (struct ref_store *refs,const struct ref_storage_be *be) {
refs.h +2 -0
@@ -565,5 +565,7 @@ struct ref_store *get_main_ref_store(void);* submodule==NULL. */ struct ref_store *get_submodule_ref_store (const char *submodule); +struct worktree ; +struct ref_store *get_worktree_ref_store (const struct worktree *wt);#endif /* REFS_H */
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 2/5 refs.c: add refs_resolve_ref_unsafe()
In reply to Nguyễn Thái Ngọc DuySurprise surprise. This is just rename and introduce resolve_ref_recursively() to the public.
--- refs.c | 9 +++++---- refs.h | 6 ++++++ refs/files-backend.c | 2 +- refs/refs-internal.h | 5 ----- 4 files changed, 12 insertions(+), 10 deletions(-)
refs.c +5 -4
@@ -1226,7 +1226,7 @@ int for_each_rawref(each_ref_fn fn, void *cb_data) }/* This function needs to return a meaningful errno on failure */ -const char *resolve_ref_recursively (struct ref_store *refs, +const char *refs_resolve_ref_unsafe (struct ref_store *refs,const char *refname,int resolve_flags,unsigned char *sha1,int *flags)
@@ -1313,8 +1313,9 @@ int refs_init_db(struct strbuf *err)const char *resolve_ref_unsafe (const char *refname,int resolve_flags,unsigned char *sha1,int *flags) { -return resolve_ref_recursively (get_main_ref_store (), refname, - resolve_flags, sha1, flags); +return refs_resolve_ref_unsafe (get_main_ref_store (), + refname, resolve_flags, + sha1, flags); } int resolve_gitlink_ref(const char *submodule, const char *refname,
@@ -1343,7 +1344,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,if (!refs)return -1 ; -if (!resolve_ref_recursively (refs, refname,0 , sha1, &flags) || +if (!refs_resolve_ref_unsafe (refs, refname,0 , sha1, &flags) ||is_null_sha1 (sha1))return -1 ;return 0 ;
refs.h +6 -0
@@ -568,4 +568,10 @@ struct ref_store *get_submodule_ref_store(const char *submodule);struct worktree ;struct ref_store *get_worktree_ref_store (const struct worktree *wt); +const char *refs_resolve_ref_unsafe (struct ref_store *refs, +const char *refname, +int resolve_flags, +unsigned char *sha1, +int *flags); +#endif /* REFS_H */
refs/files-backend.c +1 -1
@@ -1276,7 +1276,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)create_dir_entry (refs, refname.buf, refname.len, 1)); }else { -if (!resolve_ref_recursively (&refs->base, +if (!refs_resolve_ref_unsafe (&refs->base, refname.buf,RESOLVE_REF_READING , sha1, &flag)) {
refs/refs-internal.h +0 -5
@@ -634,11 +634,6 @@ struct ref_store {void base_ref_store_init (struct ref_store *refs,const struct ref_storage_be *be); -const char *resolve_ref_recursively (struct ref_store *refs, -const char *refname, -int resolve_flags, -unsigned char *sha1,int *flags); -static inline int is_per_worktree_ref (const char *refname) { return !starts_with(refname, "refs/") ||
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 3/5 worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
In reply to Nguyễn Thái Ngọc DuyThe manual parsing code is replaced with a call to refs_resolve_ref_unsafe(). The manual parsing code must die because only refs/files-backend.c should do that.
--- branch.c | 3 +- worktree.c | 102 ++++++++++++++++--------------------------------------------- worktree.h | 2 +- 3 files changed, 30 insertions(+), 77 deletions(-)
branch.c +2 -1
@@ -354,7 +354,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)for (i =0 ; worktrees[i]; i++) {if (worktrees[i]->is_detached)continue ; -if (strcmp (oldref, worktrees[i]->head_ref)) +if (worktrees[i]->head_ref && +strcmp (oldref, worktrees[i]->head_ref))continue ; if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
worktree.c +27 -75
168 lines not on this page · raw message
worktree.h +1 -1
@@ -4,7 +4,7 @@struct worktree {char *path;char *id; -char *head_ref; +char *head_ref;/* NULL if HEAD is broken or detached */ char *lock_reason;/* internal use */ unsigned char head_sha1[20 ];int is_detached;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 4/5 refs: add refs_create_symref()
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 22 +++++++++++++++++----- refs.h | 4 ++++ 2 files changed, 21 insertions(+), 5 deletions(-)
refs.c +17 -5
@@ -1535,13 +1535,25 @@ int peel_ref(const char *refname, unsigned char *sha1)return refs->be->peel_ref (refs, refname, sha1); } -int create_symref (const char *ref_target,const char *refs_heads_master, +int refs_create_symref (struct ref_store *refs, +const char *ref_target, +const char *refs_heads_master, +const char *logmsg) +{ +return refs->be->create_symref (refs, + ref_target, + refs_heads_master, + logmsg); +} + +int create_symref (const char *ref_target, +const char *refs_heads_master,const char *logmsg) { -struct ref_store *refs =get_main_ref_store (); - -return refs->be->create_symref (refs, ref_target, refs_heads_master, - logmsg); +return refs_create_symref (get_main_ref_store (), + ref_target, + refs_heads_master, + logmsg); } int ref_transaction_commit(struct ref_transaction *transaction,
refs.h +4 -0
@@ -573,5 +573,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,int resolve_flags, unsigned char *sha1, int *flags); +int refs_create_symref (struct ref_store *refs, +const char *refname, +const char *target, +const char *logmsg);#endif /* REFS_H */
Signature · 1 line
2.11.0.157.gd943d85
PATCH v2 5/5 refs: kill set_worktree_head_symref()
In reply to Nguyễn Thái Ngọc Duy70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27) added this function in order to update HEADs of all relevant worktrees, when a branch is renamed.
It, as a public ref api, kind of breaks abstraction when it uses internal functions of files backend. With the introduction of refs_create_symref(), we can move back pretty close to the code before 70999e9cec, where create_symref() was used for updating HEAD.
--- branch.c | 12 ++++++------ refs.h | 9 --------- refs/files-backend.c | 41 ----------------------------------------- 3 files changed, 6 insertions(+), 56 deletions(-)
branch.c +6 -6
@@ -352,18 +352,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)int i;for (i =0 ; worktrees[i]; i++) { +struct ref_store *refs; +if (worktrees[i]->is_detached)continue ;if (worktrees[i]->head_ref &&strcmp (oldref, worktrees[i]->head_ref))continue ; -if (set_worktree_head_symref (get_worktree_git_dir (worktrees[i]), - newref)) { - ret =-1 ; -error (_ ("HEAD of working tree %s is not updated" ), - worktrees[i]->path); - } + refs =get_worktree_ref_store (worktrees[i]); +if (refs_create_symref (refs,"HEAD" , newref,NULL )) + ret =error (_ ("HEAD of working tree %s is not updated" ), + worktrees[i]->path); }free_worktrees (worktrees);
refs.h +0 -9
@@ -325,15 +325,6 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg);int create_symref (const char *refname,const char *target,const char *logmsg); -/* -* Update HEAD of the specified gitdir. -* Similar to create_symref("relative-git-dir/HEAD", target, NULL), but -* this can update the main working tree's HEAD regardless of where -* $GIT_DIR points to. -* Return 0 if successful, non-zero otherwise. -* */ -int set_worktree_head_symref (const char *gitdir,const char *target); -enum action_on_err {UPDATE_REFS_MSG_ON_ERR ,UPDATE_REFS_DIE_ON_ERR ,
refs/files-backend.c +0 -41
47 lines not on this page · raw message
Signature · 1 line
2.11.0.157.gd943d85
v3 cover Kill manual ref parsing code in worktree.c
In reply to Nguyễn Thái Ngọc Duyv3 is a rebased version on latest nd/files-backend-git-dir [1]. Since that series added a bunch of new refs_* functions, v2's 02/05 and 04/05 are removed. The new 01/04 could be an indepedent fix, but at test-ref-store.c requires it, so I put it here. More tests are added now that we have test-ref-store.c (yay!)
[1] http://public-inbox.org/git/%[email protected]%3E/
Nguyễn Thái Ngọc Duy (4):
environment.c: fix potential segfault by get_git_common_dir()
refs: introduce get_worktree_ref_store()
worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
refs: kill set_worktree_head_symref()
branch.c | 15 ++--- environment.c | 2 + refs.c | 32 +++++++++++ refs.h | 12 +--- refs/files-backend.c | 44 -------------- t/helper/test-ref-store.c | 19 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 52 +++++++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 9 files changed, 143 insertions(+), 137 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh
Signature · 1 line
2.11.0.157.gd943d85
PATCH v3 1/4 environment.c: fix potential segfault by get_git_common_dir()
setup_git_env() must be called before this function to initialize git_common_dir so that it returns a non NULL string. And it must return a non NULL string or segfault can happen because all callers expect so.
Normally if somebody has called get_git_dir(), or set_git_dir() then setup_git_env() is already called. But if you do setup_git_directory() at top dir (which skips set_git_dir) and never call get_git_dir, you'll get NULL here.
test-ref-store.c will hit this problem because it's very lightweight, just enough initialization to exercise refs code, and get_git_dir() will never be called until get_worktrees() is, which uses get_git_common_dir().
--- environment.c | 2 ++ 1 file changed, 2 insertions(+)
environment.c +2 -0
@@ -214,6 +214,8 @@ const char *get_git_dir(void)const char *get_git_common_dir (void ) { +if (!git_dir) +setup_git_env ();return git_common_dir; }
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()
Quoted 8 lines · Nguyễn Thái Ngọc Duy
setup_git_env()must be called before this function to initializegit_common_dirso that it returns a non NULL string. And it must return a non NULL string or segfault can happen because all callers expect so.Normally if somebody has called
get_git_dir(), orset_git_dir()thensetup_git_env()is already called. But if you dosetup_git_directory()at top dir (which skipsset_git_dir) and never callget_git_dir, you'll get NULL here.
Hmph, and the solution for the problem not being "so let's make sure get_git_dir() is called even when the command is started at the top directory" is because...?
Quoted 4 lines
test-ref-store.cwill hit this problem because it's very lightweight, just enough initialization to exercise refs code, andget_git_dir()will never be called untilget_worktrees()is, which usesget_git_common_dir().
---
Missing sign-off.
Quoted 15 lines
environment.c | 2 ++ 1 file changed, 2 insertions(+)environment.c +2
-0@@ -214,6 +214,8 @@ const char *get_git_dir(void)const char *get_git_common_dir (void ) { +if (!git_dir) +setup_git_env ();return git_common_dir; }
Re: [PATCH v3 1/4] environment.c: fix potential segfault by get_git_common_dir()
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Nguyễn Thái Ngọc Duy
setup_git_env()must be called before this function to initializegit_common_dirso that it returns a non NULL string. And it must return a non NULL string or segfault can happen because all callers expect so.
Quoted 8 lines
Quoted 4 lines
Normally if somebody has called
get_git_dir(), orset_git_dir()thensetup_git_env()is already called. But if you dosetup_git_directory()at top dir (which skipsset_git_dir) and never callget_git_dir, you'll get NULL here.Hmph, and the solution for the problem not being "so let's make sure
get_git_dir()is called even when the command is started at the top directory" is because...?
-EHARDTOPARSE. There's a hidden dependency between get_git_dir() and get_git_common_dir() which is not good. If we lazily call set_git_env(), make sure we do it lazily but consistently at all relevant function calls (i.e. including get_git_common_dir).
Alternatively (I was thinking of this but didn't really follow up because this was side issue) we should make sure setup_git_env() is always called at the end of setup_git_dir...() and remove the laziness in get_git_dir(). This may be more in line of recent attempts to catch repo access without calling setup_git_directory..() first. But sadly I haven't read Jeff's series, so I can't say whether it's true.
Signature · 1 line
Duy
PATCH v3 2/4 refs: introduce get_worktree_ref_store()
In reply to Nguyễn Thái Ngọc Duyfiles-backend at this point is still aware of the per-repo/worktree separation in refs, so it can handle a linked worktree.
Some refs operations are known not working when current files-backend is used in a linked worktree (e.g. reflog). Tests will be written when refs_* functions start to be called with worktree backend to verify that they work as expected.
Note: accessing a worktree of a submodule remains unaddressed. Perhaps after get_worktrees() can access submodule (or rather a new function get_submodule_worktrees(), that lists worktrees of a submodule), we can update this function to work with submodules as well.
--- refs.c | 32 ++++++++++++++++++++++++++++++++ refs.h | 2 ++ 2 files changed, 34 insertions(+)
refs.c +32 -0
@@ -10,6 +10,7 @@#include "object.h" #include "tag.h" #include "submodule.h" +#include "worktree.h" /* * List of all available backends
@@ -1593,6 +1594,37 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return refs; } +struct ref_store *get_worktree_ref_store (const struct worktree *wt) +{ +struct ref_store *refs; +unsigned int refs_all_capabilities = +REF_STORE_READ |REF_STORE_WRITE | +REF_STORE_ODB |REF_STORE_MAIN ; + +if (wt->is_current) +return get_main_ref_store (); + +/* +* We share the same hash map with submodules for +* now. submodule paths are always relative (to topdir) while +* worktree paths are always absolute. No chance of conflict. +*/ + refs =lookup_submodule_ref_store (wt->path); +if (refs) +return refs; + +if (wt->id) + refs =ref_store_init (git_common_path ("worktrees/%s" , wt->id), + refs_all_capabilities); +else + refs =ref_store_init (get_git_common_dir (), + refs_all_capabilities); + +if (refs) +register_submodule_ref_store (refs, wt->path); +return refs; +} +void base_ref_store_init (struct ref_store *refs,const struct ref_storage_be *be) {
refs.h +2 -0
@@ -5,6 +5,7 @@ struct object_id;struct ref_store ;struct strbuf ;struct string_list ; +struct worktree ;/* * Resolve a reference, recursively following symbolic refererences.
@@ -655,5 +656,6 @@ struct ref_store *get_main_ref_store(void);* submodule==NULL. */ struct ref_store *get_submodule_ref_store (const char *submodule); +struct ref_store *get_worktree_ref_store (const struct worktree *wt);#endif /* REFS_H */
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()
Quoted 54 lines · Nguyễn Thái Ngọc Duy
files-backend at this point is still aware of the per-repo/worktree separation in refs, so it can handle a linked worktree.
Some refs operations are known not working when current files-backend is used in a linked worktree (e.g. reflog). Tests will be written when refs_* functions start to be called with worktree backend to verify that they work as expected.
Note: accessing a worktree of a submodule remains unaddressed. Perhaps after
get_worktrees()can access submodule (or rather a new functionget_submodule_worktrees(), that lists worktrees of a submodule), we can update this function to work with submodules as well.--- refs.c | 32 ++++++++++++++++++++++++++++++++ refs.h | 2 ++ 2 files changed, 34 insertions(+)refs.c +22
-0@@ -10,6 +10,7 @@#include "object.h" #include "tag.h" #include "submodule.h" +#include "worktree.h" /* * List of all available backends @@ -1593,6 +1594,37 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return refs; } +struct ref_store *get_worktree_ref_store (const struct worktree *wt) +{ +struct ref_store *refs; +unsigned int refs_all_capabilities = +REF_STORE_READ |REF_STORE_WRITE | +REF_STORE_ODB |REF_STORE_MAIN ; + +if (wt->is_current) +return get_main_ref_store (); + +/* +* We share the same hash map with submodules for +* now. submodule paths are always relative (to topdir) while +* worktree paths are always absolute. No chance of conflict. +*/ + refs =lookup_submodule_ref_store (wt->path); +if (refs) +return refs; + +if (wt->id) + refs = ref_store_init(git_common_path("worktrees/%s", wt->id),
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1627
Quoted 10 lines
refs.c +10
-0+ refs_all_capabilities); + else + refs =ref_store_init (get_git_common_dir (), + refs_all_capabilities); + +if (refs) +register_submodule_ref_store (refs, wt->path); +return refs; +} +
I guess I can hold my nose and accept storing worktree and submodule ref_stores together in a single hashmap, but not if the code continues to use "submodule" in the names, because that will be a continuing source of confusion to readers. For example,
* lookup_submodule_ref_store()
* register_submodule_ref_store()
* submodule_hash_entry
* submodule_hash_cmp()
* alloc_submodule_hash_entry()
* submodule_ref_stores
Any docstrings would also have to be updated.
As a new naming scheme, maybe use s/submodule/other/, for example lookup_other_ref_store() etc.
Quoted 1 line
[...]
Michael
Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()
Quoted 2 lines · Michael Haggerty
I guess I can hold my nose and accept storing worktree and submodule
ref_stores together in a single hashmap
Release your nose, I'll add a new hashmap :) But while we're here, what are your thoughts about moving submodule-specific to submodule.c? That includes the hashmap, get_submodule_ref_store() and all other submodule stuff, to submodule.c where submodule-specific stuff stays. The same for worktree stuff, to worktree.c. That keeps refs.c to core refs business. And the hashmap can be used for more than just refs. For example, submodule has configs, and worktree also has bunch of other stuff that I would like to just cache and not readdir() and parse every time.
Signature · 1 line
Duy
Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()
Quoted 5 lines · Duy Nguyen
Quoted 2 lines · Michael Haggerty
I guess I can hold my nose and accept storing worktree and submodule
ref_stores together in a single hashmapRelease your nose, I'll add a new hashmap :)
Thanks :-)
Quoted 9 lines
But while we're here,
what are your thoughts about moving submodule-specific tosubmodule.c? That includes the hashmap,get_submodule_ref_store()and all other submodule stuff, tosubmodule.cwhere submodule-specific stuff stays. The same for worktree stuff, toworktree.c. That keepsrefs.cto core refs business. And the hashmap can be used for more than just refs. For example, submodule has configs, and worktree also has bunch of other stuff that I would like to just cache and notreaddir()and parse every time.
I can't think of any objections, though there might be some devils in the details. I assume you mean that the submodule-specific hashmap wouldn't be {name -> struct ref_store} anymore, but rather {name -> struct submodule}, and struct submodule would hold the associated ref_store? That sounds very reasonable.
Instead of moving all of the for_each_*_submodule() functions over, I encourage you to consider getting rid of them entirely and let the end-users call the refs_for_each_*() versions of the functions. Again, I'm not sure that there won't be friction in doing so, but it seems like it's worth a try.
Michael
Re: [PATCH v3 2/4] refs: introduce get_worktree_ref_store()
Quoted 5 lines · Michael Haggerty
Instead of moving all of the
for_each_*_submodule()functions over, I encourage you to consider getting rid of them entirely and let the end-users call therefs_for_each_*()versions of the functions. Again, I'm not sure that there won't be friction in doing so, but it seems like it's worth a try.
They are getting rid of. If you look at pu (or nd/prune-in-worktree actually) there's only head_ref_submodule() and for_each_remote_ref_submodule() left. head_ref_submodule() has no caller but is still there, I'll need to kill it. Killing the latter can be done separately since the callers in submodule.c need some update.
Signature · 1 line
Duy
PATCH v3 3/4 worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
In reply to Nguyễn Thái Ngọc DuyThe manual parsing code is replaced with a call to refs_resolve_ref_unsafe(). The manual parsing code must die because only refs/files-backend.c should do that.
--- branch.c | 3 +- t/helper/test-ref-store.c | 19 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 40 +++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 5 files changed, 89 insertions(+), 77 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh
branch.c +2 -1
@@ -355,7 +355,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,for (i =0 ; worktrees[i]; i++) {if (worktrees[i]->is_detached)continue ; -if (strcmp (oldref, worktrees[i]->head_ref)) +if (worktrees[i]->head_ref && +strcmp (oldref, worktrees[i]->head_ref))continue ; if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
t/helper/test-ref-store.c +19 -0
@@ -1,5 +1,6 @@#include "cache.h" #include "refs.h" +#include "worktree.h" static const char *notnull (const char *arg,const char *name) {
@@ -32,6 +33,23 @@ static const char **get_store(const char **argv, struct ref_store **refs)strbuf_release (&sb); *refs =get_submodule_ref_store (gitdir); + }else if (skip_prefix (argv[0 ],"worktree:" , &gitdir)) { +struct worktree **p, **worktrees =get_worktrees (0 ); + +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; + +if (!wt->id) { +/* special case for main worktree */ +if (!strcmp (gitdir,"main" )) +break ; + }else if (!strcmp (gitdir, wt->id)) +break ; + } +if (!*p) +die ("no such worktree: %s" , gitdir); + + *refs =get_worktree_ref_store (*p); }else die ("unknown backend %s" , argv[0 ]);
@@ -261,6 +279,7 @@ int cmd_main(int argc, const char **argv)const char *func;struct command *cmd; +setup_git_directory (); argv =get_store (argv +1 , &refs); func = *argv++;
t/t1407-worktree-ref-store.sh new file +40 -0
@@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='test worktree ref store api' + +. ./test-lib.sh + +RWT="test-ref-store worktree:wt" +RMAIN="test-ref-store worktree:main" + +test_expect_success 'setup' ' + test_commit first && + git worktree add -b wt-master wt && + ( + cd wt && + test_commit second + ) +' + +test_expect_success 'resolve_ref(<shared-ref>)' ' + SHA1=`git rev-parse master` && + echo "$SHA1 refs/heads/master 0x0" >expected && + $RWT resolve-ref refs/heads/master 0 >actual && + test_cmp expected actual && + $RMAIN resolve-ref refs/heads/master 0 >actual && + test_cmp expected actual +' + +test_expect_success 'resolve_ref(<per-worktree-ref>)' ' + SHA1=`git -C wt rev-parse HEAD` && + echo "$SHA1 refs/heads/wt-master 0x1" >expected && + $RWT resolve-ref HEAD 0 >actual && + test_cmp expected actual && + + SHA1=`git rev-parse HEAD` && + echo "$SHA1 refs/heads/master 0x1" >expected && + $RMAIN resolve-ref HEAD 0 >actual && + test_cmp expected actual +' + +test_done
worktree.c +27 -75
169 lines not on this page · raw message
worktree.h +1 -1
@@ -4,7 +4,7 @@struct worktree {char *path;char *id; -char *head_ref; +char *head_ref;/* NULL if HEAD is broken or detached */ char *lock_reason;/* internal use */ unsigned char head_sha1[20 ];int is_detached;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v3 4/4 refs: kill set_worktree_head_symref()
In reply to Nguyễn Thái Ngọc Duy70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27) added this function in order to update HEADs of all relevant worktrees, when a branch is renamed.
It, as a public ref api, kind of breaks abstraction when it uses internal functions of files backend. With the introduction of refs_create_symref(), we can move back pretty close to the code before 70999e9cec, where create_symref() was used for updating HEAD.
--- branch.c | 12 ++++++------ refs.h | 10 ---------- refs/files-backend.c | 44 ------------------------------------------- t/t1407-worktree-ref-store.sh | 12 ++++++++++++ 4 files changed, 18 insertions(+), 60 deletions(-)
branch.c +6 -6
@@ -353,18 +353,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,int i;for (i =0 ; worktrees[i]; i++) { +struct ref_store *refs; +if (worktrees[i]->is_detached)continue ;if (worktrees[i]->head_ref &&strcmp (oldref, worktrees[i]->head_ref))continue ; -if (set_worktree_head_symref (get_worktree_git_dir (worktrees[i]), - newref, logmsg)) { - ret =-1 ; -error (_ ("HEAD of working tree %s is not updated" ), - worktrees[i]->path); - } + refs =get_worktree_ref_store (worktrees[i]); +if (refs_create_symref (refs,"HEAD" , newref, logmsg)) + ret =error (_ ("HEAD of working tree %s is not updated" ), + worktrees[i]->path); }free_worktrees (worktrees);
refs.h +0 -10
@@ -402,16 +402,6 @@ int refs_create_symref(struct ref_store *refs, const char *refname, const char *target, const char *logmsg);int create_symref (const char *refname,const char *target,const char *logmsg); -/* -* Update HEAD of the specified gitdir. -* Similar to create_symref("relative-git-dir/HEAD", target, NULL), but -* this can update the main working tree's HEAD regardless of where -* $GIT_DIR points to. -* Return 0 if successful, non-zero otherwise. -* */ -int set_worktree_head_symref (const char *gitdir,const char *target, -const char *logmsg); -enum action_on_err {UPDATE_REFS_MSG_ON_ERR ,UPDATE_REFS_DIE_ON_ERR ,
refs/files-backend.c +0 -44
50 lines not on this page · raw message
t/t1407-worktree-ref-store.sh +12 -0
@@ -37,4 +37,16 @@ test_expect_success 'resolve_ref(<per-worktree-ref>)' ' test_cmp expected actual ' +test_expect_success 'create_symref(FOO, refs/heads/master)' ' + $RWT create-symref FOO refs/heads/master nothing && + echo refs/heads/master >expected && + git -C wt symbolic-ref FOO >actual && + test_cmp expected actual && + + $RMAIN create-symref FOO refs/heads/wt-master nothing && + echo refs/heads/wt-master >expected && + git symbolic-ref FOO >actual && + test_cmp expected actual +' + test_done
Signature · 1 line
2.11.0.157.gd943d85
Quoted 5 lines · Nguyễn Thái Ngọc Duy
v3 is a rebased version on latest nd/files-backend-git-dir [1]. Since that series added a bunch of new refs_* functions, v2's 02/05 and 04/05 are removed. The new 01/04 could be an indepedent fix, but at
test-ref-store.crequires it, so I put it here. More tests are added now that we havetest-ref-store.c(yay!)
I think the corresponding ones were queued on a separate topic nd/worktree-kill-parse-ref that is based on nd/files-backend-git-dir but I do not mind making these 4 patches just part of the latter topic.
Will send comments on individual patches separately.
Thanks.
Quoted 20 lines
[1] http://public-inbox.org/git/%[email protected]%3E/
Nguyễn Thái Ngọc Duy (4):
environment.c: fix potential segfault byget_git_common_dir()
refs: introduceget_worktree_ref_store()
worktree.c: killparse_ref()in favor ofrefs_resolve_ref_unsafe()
refs: killset_worktree_head_symref()branch.c | 15 ++--- environment.c | 2 + refs.c | 32 +++++++++++ refs.h | 12 +--- refs/files-backend.c | 44 -------------- t/helper/test-ref-store.c | 19 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 52 +++++++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 9 files changed, 143 insertions(+), 137 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh
v4 cover Kill manual ref parsing code in worktree.c
In reply to Nguyễn Thái Ngọc Duyv4 adds a new patch, 2/5, which makes the hashmap related functions in refs.c generic, so I could add a new map for worktree ref stores and not abuse submodule hashmap.
I mentioned about moving these hashmap back to submodule.c and worktree.c where it can map more than just ref stores (in worktree case, we can cache 'struct worktree', for example). But I'm not doing it now to keep things small.
The commit message in 1/5 is rephrased a bit, hopefully clearer.
Nguyễn Thái Ngọc Duy (5):
environment.c: fix potential segfault by get_git_common_dir() refs.c: make submodule ref store hashmap generic
refs: introduce get_worktree_ref_store()
worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
refs: kill set_worktree_head_symref()
branch.c | 15 ++--- environment.c | 2 + refs.c | 95 +++++++++++++++++++----------- refs.h | 12 +--- refs/files-backend.c | 44 -------------- t/helper/test-ref-store.c | 18 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 52 +++++++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 9 files changed, 173 insertions(+), 169 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 1/5 environment.c: fix potential segfault by get_git_common_dir()
setup_git_env() must be called before this function to initialize git_common_dir so that it returns a non NULL string. And it must return a non NULL string or segfault can happen because all callers expect so.
It does not do so explicitly though and depends on get_git_dir() being called first (which will guarantee setup_git_env()). Avoid this dependency and call setup_git_env() by itself.
test-ref-store.c will hit this problem because it's very lightweight, just enough initialization to exercise refs code, and get_git_dir() will never be called until get_worktrees() is, which uses get_git_common_dir and hits a segfault.
--- environment.c | 2 ++ 1 file changed, 2 insertions(+)
environment.c +2 -0
@@ -214,6 +214,8 @@ const char *get_git_dir(void)const char *get_git_common_dir (void ) { +if (!git_dir) +setup_git_env ();return git_common_dir; }
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 2/5 refs.c: make submodule ref store hashmap generic
In reply to Nguyễn Thái Ngọc DuyThis removes the "submodule" from submodule_hash_entry and other function names. The goal is to reuse the same code and data structure for other ref store types. The first one is worktree ref stores.
--- refs.c | 64 ++++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 32 insertions(+), 32 deletions(-)
refs.c +32 -32
@@ -1450,32 +1450,32 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,return 0 ; } -struct submodule_hash_entry +struct ref_store_hash_entry {struct hashmap_entry ent;/* must be the first member! */ struct ref_store *refs; -/* NUL-terminated name of submodule: */ -char submodule[FLEX_ARRAY ]; +/* NUL-terminated identifier of the ref store: */ +char name[FLEX_ARRAY ]; }; -static int submodule_hash_cmp (const void *entry,const void *entry_or_key, +static int ref_store_hash_cmp (const void *entry,const void *entry_or_key,const void *keydata) { -const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key; -const char *submodule = keydata ? keydata : e2->submodule; +const struct ref_store_hash_entry *e1 = entry, *e2 = entry_or_key; +const char *name = keydata ? keydata : e2->name; -return strcmp (e1->submodule, submodule); +return strcmp (e1->name, name); } -static struct submodule_hash_entry *alloc_submodule_hash_entry ( -const char *submodule,struct ref_store *refs) +static struct ref_store_hash_entry *alloc_ref_store_hash_entry ( +const char *name,struct ref_store *refs) { -struct submodule_hash_entry *entry; +struct ref_store_hash_entry *entry; -FLEX_ALLOC_STR (entry, submodule, submodule); -hashmap_entry_init (entry,strhash (submodule)); +FLEX_ALLOC_STR (entry, name, name); +hashmap_entry_init (entry,strhash (name)); entry->refs = refs;return entry; }
@@ -1487,19 +1487,19 @@ static struct ref_store *main_ref_store;static struct hashmap submodule_ref_stores;/* -* Return the ref_store instance for the specified submodule. If that -* ref_store hasn't been initialized yet, return NULL. +* Look up a ref store by name. If that ref_store hasn't been +* registered yet, return NULL. */ -static struct ref_store *lookup_submodule_ref_store (const char *submodule) +static struct ref_store *lookup_ref_store_map (struct hashmap *map, +const char *name) { -struct submodule_hash_entry *entry; +struct ref_store_hash_entry *entry; -if (!submodule_ref_stores.tablesize) +if (!map->tablesize)/* It's initialized on demand in register_ref_store(). */ return NULL ; - entry =hashmap_get_from_hash (&submodule_ref_stores, -strhash (submodule), submodule); + entry =hashmap_get_from_hash (map,strhash (name), name);return entry ? entry->refs :NULL ; }
@@ -1535,20 +1535,19 @@ struct ref_store *get_main_ref_store(void) }/* -* Register the specified ref_store to be the one that should be used -* for submodule. It is a fatal error to call this function twice for -* the same submodule. +* Associate a ref store with a name. It is a fatal error to call this +* function twice for the same name. */ -static void register_submodule_ref_store (struct ref_store *refs, -const char *submodule) +static void register_ref_store_map (struct hashmap *map, +const char *type, +struct ref_store *refs, +const char *name) { -if (!submodule_ref_stores.tablesize) -hashmap_init (&submodule_ref_stores, submodule_hash_cmp,0 ); +if (!map->tablesize) +hashmap_init (map, ref_store_hash_cmp,0 ); -if (hashmap_put (&submodule_ref_stores, -alloc_submodule_hash_entry (submodule, refs))) -die ("BUG: ref_store for submodule '%s' initialized twice" , - submodule); +if (hashmap_put (map,alloc_ref_store_hash_entry (name, refs))) +die ("BUG: %s ref_store '%s' initialized twice" , type, name); }struct ref_store *get_submodule_ref_store (const char *submodule)
@@ -1565,7 +1564,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return get_main_ref_store (); } - refs =lookup_submodule_ref_store (submodule); + refs =lookup_ref_store_map (&submodule_ref_stores, submodule);if (refs)return refs;
@@ -1584,7 +1583,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule)/* pretend that add_submodule_odb() has been called */ refs =ref_store_init (submodule_sb.buf,REF_STORE_READ |REF_STORE_ODB ); -register_submodule_ref_store (refs, submodule); +register_ref_store_map (&submodule_ref_stores,"submodule" , + refs, submodule);strbuf_release (&submodule_sb);return refs;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 3/5 refs: introduce get_worktree_ref_store()
In reply to Nguyễn Thái Ngọc Duyfiles-backend at this point is still aware of the per-repo/worktree separation in refs, so it can handle a linked worktree.
Some refs operations are known not working when current files-backend is used in a linked worktree (e.g. reflog). Tests will be written when refs_* functions start to be called with worktree backend to verify that they work as expected.
Note: accessing a worktree of a submodule remains unaddressed. Perhaps after get_worktrees() can access submodule (or rather a new function get_submodule_worktrees(), that lists worktrees of a submodule), we can update this function to work with submodules as well.
--- refs.c | 31 +++++++++++++++++++++++++++++++ refs.h | 2 ++ 2 files changed, 33 insertions(+)
refs.c +31 -0
@@ -10,6 +10,7 @@#include "object.h" #include "tag.h" #include "submodule.h" +#include "worktree.h" /* * List of all available backends
@@ -1486,6 +1487,9 @@ static struct ref_store *main_ref_store;/* A hashmap of ref_stores, stored by submodule name: */ static struct hashmap submodule_ref_stores; +/* A hashmap of ref_stores, stored by worktree id: */ +static struct hashmap worktree_ref_stores; +/* * Look up a ref store by name. If that ref_store hasn't been * registered yet, return NULL.
@@ -1590,6 +1594,33 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return refs; } +struct ref_store *get_worktree_ref_store (const struct worktree *wt) +{ +struct ref_store *refs; +unsigned int refs_all_capabilities = +REF_STORE_READ |REF_STORE_WRITE | +REF_STORE_ODB |REF_STORE_MAIN ; + +if (wt->is_current) +return get_main_ref_store (); + + refs =lookup_ref_store_map (&worktree_ref_stores, wt->id); +if (refs) +return refs; + +if (wt->id) + refs =ref_store_init (git_common_path ("worktrees/%s" , wt->id), + refs_all_capabilities); +else + refs =ref_store_init (get_git_common_dir (), + refs_all_capabilities); + +if (refs) +register_ref_store_map (&worktree_ref_stores,"worktree" , + refs, wt->id); +return refs; +} +void base_ref_store_init (struct ref_store *refs,const struct ref_storage_be *be) {
refs.h +2 -0
@@ -5,6 +5,7 @@ struct object_id;struct ref_store ;struct strbuf ;struct string_list ; +struct worktree ;/* * Resolve a reference, recursively following symbolic refererences.
@@ -655,5 +656,6 @@ struct ref_store *get_main_ref_store(void);* submodule==NULL. */ struct ref_store *get_submodule_ref_store (const char *submodule); +struct ref_store *get_worktree_ref_store (const struct worktree *wt);#endif /* REFS_H */
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 3/5] refs: introduce get_worktree_ref_store()
Quoted 42 lines · Nguyễn Thái Ngọc Duy
files-backend at this point is still aware of the per-repo/worktree separation in refs, so it can handle a linked worktree.
Some refs operations are known not working when current files-backend is used in a linked worktree (e.g. reflog). Tests will be written when refs_* functions start to be called with worktree backend to verify that they work as expected.
Note: accessing a worktree of a submodule remains unaddressed. Perhaps after
get_worktrees()can access submodule (or rather a new functionget_submodule_worktrees(), that lists worktrees of a submodule), we can update this function to work with submodules as well.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 31 +++++++++++++++++++++++++++++++ refs.h | 2 ++ 2 files changed, 33 insertions(+)refs.c +4
-0@@ -10,6 +10,7 @@#include "object.h" #include "tag.h" #include "submodule.h" +#include "worktree.h" /* * List of all available backends @@ -1486,6 +1487,9 @@ static struct ref_store *main_ref_store;/* A hashmap of ref_stores, stored by submodule name: */ static struct hashmap submodule_ref_stores; +/* A hashmap of ref_stores, stored by worktree id: */ +static struct hashmap worktree_ref_stores; +/* * Look up a ref store by name. If that ref_store hasn't been * registered yet, return NULL.
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1602
Quoted 9 lines
refs.c +6
-0@@ -1590,6 +1594,33 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return refs; } +struct ref_store *get_worktree_ref_store (const struct worktree *wt) +{ +struct ref_store *refs; +unsigned int refs_all_capabilities = +REF_STORE_READ |REF_STORE_WRITE | +REF_STORE_ODB |REF_STORE_MAIN ;
This constant appears another place, too. It might make sense to define a constant REF_STORE_ALL_CAPABILITIES in refs-internal.h alongside the individual bit values.
If you prefer not to, please at least declare this variable const to spare the reader the trouble of looking to see whether it is modified before it is used.
Otherwise, looks fine to me.
Quoted 1 line
[...]
Michael
PATCH v4 4/5 worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
In reply to Nguyễn Thái Ngọc DuyThe manual parsing code is replaced with a call to refs_resolve_ref_unsafe(). The manual parsing code must die because only refs/files-backend.c should do that.
--- branch.c | 3 +- t/helper/test-ref-store.c | 18 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 40 +++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 5 files changed, 88 insertions(+), 77 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh
branch.c +2 -1
@@ -355,7 +355,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,for (i =0 ; worktrees[i]; i++) {if (worktrees[i]->is_detached)continue ; -if (strcmp (oldref, worktrees[i]->head_ref)) +if (worktrees[i]->head_ref && +strcmp (oldref, worktrees[i]->head_ref))continue ; if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
t/helper/test-ref-store.c +18 -0
@@ -1,5 +1,6 @@#include "cache.h" #include "refs.h" +#include "worktree.h" static const char *notnull (const char *arg,const char *name) {
@@ -32,6 +33,23 @@ static const char **get_store(const char **argv, struct ref_store **refs)strbuf_release (&sb); *refs =get_submodule_ref_store (gitdir); + }else if (skip_prefix (argv[0 ],"worktree:" , &gitdir)) { +struct worktree **p, **worktrees =get_worktrees (0 ); + +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; + +if (!wt->id) { +/* special case for main worktree */ +if (!strcmp (gitdir,"main" )) +break ; + }else if (!strcmp (gitdir, wt->id)) +break ; + } +if (!*p) +die ("no such worktree: %s" , gitdir); + + *refs =get_worktree_ref_store (*p); }else die ("unknown backend %s" , argv[0 ]);
t/t1407-worktree-ref-store.sh new file +40 -0
@@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='test worktree ref store api' + +. ./test-lib.sh + +RWT="test-ref-store worktree:wt" +RMAIN="test-ref-store worktree:main" + +test_expect_success 'setup' ' + test_commit first && + git worktree add -b wt-master wt && + ( + cd wt && + test_commit second + ) +' + +test_expect_success 'resolve_ref(<shared-ref>)' ' + SHA1=`git rev-parse master` && + echo "$SHA1 refs/heads/master 0x0" >expected && + $RWT resolve-ref refs/heads/master 0 >actual && + test_cmp expected actual && + $RMAIN resolve-ref refs/heads/master 0 >actual && + test_cmp expected actual +' + +test_expect_success 'resolve_ref(<per-worktree-ref>)' ' + SHA1=`git -C wt rev-parse HEAD` && + echo "$SHA1 refs/heads/wt-master 0x1" >expected && + $RWT resolve-ref HEAD 0 >actual && + test_cmp expected actual && + + SHA1=`git rev-parse HEAD` && + echo "$SHA1 refs/heads/master 0x1" >expected && + $RMAIN resolve-ref HEAD 0 >actual && + test_cmp expected actual +' + +test_done
worktree.c +27 -75
169 lines not on this page · raw message
worktree.h +1 -1
@@ -4,7 +4,7 @@struct worktree {char *path;char *id; -char *head_ref; +char *head_ref;/* NULL if HEAD is broken or detached */ char *lock_reason;/* internal use */ unsigned char head_sha1[20 ];int is_detached;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 5/5 refs: kill set_worktree_head_symref()
In reply to Nguyễn Thái Ngọc Duy70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27) added this function in order to update HEADs of all relevant worktrees, when a branch is renamed.
It, as a public ref api, kind of breaks abstraction when it uses internal functions of files backend. With the introduction of refs_create_symref(), we can move back pretty close to the code before 70999e9cec, where create_symref() was used for updating HEAD.
--- branch.c | 12 ++++++------ refs.h | 10 ---------- refs/files-backend.c | 44 ------------------------------------------- t/t1407-worktree-ref-store.sh | 12 ++++++++++++ 4 files changed, 18 insertions(+), 60 deletions(-)
branch.c +6 -6
@@ -353,18 +353,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,int i;for (i =0 ; worktrees[i]; i++) { +struct ref_store *refs; +if (worktrees[i]->is_detached)continue ;if (worktrees[i]->head_ref &&strcmp (oldref, worktrees[i]->head_ref))continue ; -if (set_worktree_head_symref (get_worktree_git_dir (worktrees[i]), - newref, logmsg)) { - ret =-1 ; -error (_ ("HEAD of working tree %s is not updated" ), - worktrees[i]->path); - } + refs =get_worktree_ref_store (worktrees[i]); +if (refs_create_symref (refs,"HEAD" , newref, logmsg)) + ret =error (_ ("HEAD of working tree %s is not updated" ), + worktrees[i]->path); }free_worktrees (worktrees);
refs.h +0 -10
@@ -402,16 +402,6 @@ int refs_create_symref(struct ref_store *refs, const char *refname, const char *target, const char *logmsg);int create_symref (const char *refname,const char *target,const char *logmsg); -/* -* Update HEAD of the specified gitdir. -* Similar to create_symref("relative-git-dir/HEAD", target, NULL), but -* this can update the main working tree's HEAD regardless of where -* $GIT_DIR points to. -* Return 0 if successful, non-zero otherwise. -* */ -int set_worktree_head_symref (const char *gitdir,const char *target, -const char *logmsg); -enum action_on_err {UPDATE_REFS_MSG_ON_ERR ,UPDATE_REFS_DIE_ON_ERR ,
refs/files-backend.c +0 -44
50 lines not on this page · raw message
t/t1407-worktree-ref-store.sh +12 -0
@@ -37,4 +37,16 @@ test_expect_success 'resolve_ref(<per-worktree-ref>)' ' test_cmp expected actual ' +test_expect_success 'create_symref(FOO, refs/heads/master)' ' + $RWT create-symref FOO refs/heads/master nothing && + echo refs/heads/master >expected && + git -C wt symbolic-ref FOO >actual && + test_cmp expected actual && + + $RMAIN create-symref FOO refs/heads/wt-master nothing && + echo refs/heads/wt-master >expected && + git symbolic-ref FOO >actual && + test_cmp expected actual +' + test_done
Signature · 1 line
2.11.0.157.gd943d85
Quoted 10 lines · Nguyễn Thái Ngọc Duy
v4 adds a new patch, 2/5, which makes the hashmap related functions in
refs.cgeneric, so I could add a new map for worktree ref stores and not abuse submodule hashmap.I mentioned about moving these hashmap back to
submodule.candworktree.cwhere it can map more than just ref stores (in worktree case, we can cache 'struct worktree', for example). But I'm not doing it now to keep things small.The commit message in 1/5 is rephrased a bit, hopefully clearer.
Michael, does this look good to replace what has been queued?
I did not spot any discussion on this topic after v4 when I skimmed the backlog messages, so I am hoping that I can pick this up without having to worry about seeing another reroll immediately after that (in which case I'd have been better off if I tended other topics in flight in the meantime).
Thanks, both.
Quoted 15 lines · Junio C Hamano
Quoted 10 lines · Nguyễn Thái Ngọc Duy
v4 adds a new patch, 2/5, which makes the hashmap related functions in
refs.cgeneric, so I could add a new map for worktree ref stores and not abuse submodule hashmap.I mentioned about moving these hashmap back to
submodule.candworktree.cwhere it can map more than just ref stores (in worktree case, we can cache 'struct worktree', for example). But I'm not doing it now to keep things small.The commit message in 1/5 is rephrased a bit, hopefully clearer.
Michael, does this look good to replace what has been queued?
Quoted 7 lines
I did not spot any discussion on this topic after v4 when I skimmed the backlog messages, so I am hoping that I can pick this up without having to worry about seeing another reroll immediately after that (in which case I'd have been better off if I tended other topics in flight in the meantime).
Thanks, both.
Oops, I shouldn't have done that. When applied on top of the files-backend thing (or have you updated that one and is my tree lacking it???), this breaks quite a few tests.
t0001#41 dumps core from "git worktree add ../linked-worktree" for example.
Quoted 6 lines · Junio C Hamano
Oops, I shouldn't have done that. When applied on top of the files-backend thing (or have you updated that one and is my tree lacking it???), this breaks quite a few tests.
t0001#41 dumps core from "git worktree add ../linked-worktree" for example.
This is embarassing. I worked on and off on this series over a long period of time. I guess at the end I thought everything was ok "since the last time" and just sent it away without realizing I hadn't actually run the test suite, because it does fail here, now.
This fixup patch should make the test suite pass. I'm sorry for wasting your time on this.
-- 8< --
From 09c6d432490b8e9de99eca09cdbdaebf53bbad2c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?=
<[email protected]>
Date: Fri, 14 Apr 2017 19:39:53 +0700
Subject: [PATCH] fixup! refs: introduce get_worktree_ref_store()
--- refs.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
refs.c +4 -2
@@ -1600,11 +1600,13 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)unsigned int refs_all_capabilities =REF_STORE_READ |REF_STORE_WRITE |REF_STORE_ODB |REF_STORE_MAIN ; +const char *id;if (wt->is_current)return get_main_ref_store (); - refs =lookup_ref_store_map (&worktree_ref_stores, wt->id); + id = wt->id ? wt->id :"/" ; + refs =lookup_ref_store_map (&worktree_ref_stores, id);if (refs)return refs;
@@ -1617,7 +1619,7 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)if (refs)register_ref_store_map (&worktree_ref_stores,"worktree" , - refs, wt->id); + refs, id);return refs; }
Signature · 3 lines
2.11.0.157.gd943d85 -- 8< --
Quoted 7 lines · Duy Nguyen
This is embarassing. I worked on and off on this series over a long period of time. I guess at the end I thought everything was ok "since the last time" and just sent it away without realizing I hadn't actually run the test suite, because it does fail here, now.
This fixup patch should make the test suite pass. I'm sorry for wasting your time on this.
Thanks. Will squash it in.
Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Nguyễn Thái Ngọc Duy
v4 adds a new patch, 2/5, which makes the hashmap related functions in
refs.cgeneric, so I could add a new map for worktree ref stores and not abuse submodule hashmap.
Quoted 8 lines
Quoted 6 lines
I mentioned about moving these hashmap back to
submodule.candworktree.cwhere it can map more than just ref stores (in worktree case, we can cache 'struct worktree', for example). But I'm not doing it now to keep things small.The commit message in 1/5 is rephrased a bit, hopefully clearer.
Michael, does this look good to replace what has been queued?
I finally reviewed this patch series. The refs-related changes look fine, and the submodule-related changes (which I'm not so familiar with) looks plausible.
It's a nice cleanup :-)
Michael
v5 cover Kill manual ref parsing code in worktree.c
In reply to Nguyễn Thái Ngọc Duyv5 chanes are mostly cosmetic:
refs.c +3 -10
@@ -1530,11 +1530,7 @@ struct ref_store *get_main_ref_store(void)if (main_ref_store)return main_ref_store; - main_ref_store =ref_store_init (get_git_dir (), - (REF_STORE_READ | -REF_STORE_WRITE | -REF_STORE_ODB | -REF_STORE_MAIN )); + main_ref_store =ref_store_init (get_git_dir (),REF_STORE_ALL_CAPS );return main_ref_store; }
@@ -1597,9 +1593,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)struct ref_store *get_worktree_ref_store (const struct worktree *wt) {struct ref_store *refs; -unsigned int refs_all_capabilities = -REF_STORE_READ |REF_STORE_WRITE | -REF_STORE_ODB |REF_STORE_MAIN ;const char *id;if (wt->is_current)
@@ -1612,10 +1605,10 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)if (wt->id) refs =ref_store_init (git_common_path ("worktrees/%s" , wt->id), - refs_all_capabilities); +REF_STORE_ALL_CAPS );else refs =ref_store_init (get_git_common_dir (), - refs_all_capabilities); +REF_STORE_ALL_CAPS );if (refs) register_ref_store_map(&worktree_ref_stores, "worktree",
refs/refs-internal.h +4 -0
@@ -467,6 +467,10 @@ struct ref_store;#define REF_STORE_WRITE (1 << 1)/* can perform update operations */ #define REF_STORE_ODB (1 << 2)/* has access to object database */ #define REF_STORE_MAIN (1 << 3) +#define REF_STORE_ALL_CAPS (REF_STORE_READ | \ + REF_STORE_WRITE | \ + REF_STORE_ODB | \ + REF_STORE_MAIN)/* * Initialize the ref_store for the specified gitdir. These functions
Nguyễn Thái Ngọc Duy (6):
environment.c: fix potential segfault by get_git_common_dir() refs.c: make submodule ref store hashmap generic
refs: add REFS_STORE_ALL_CAPS
refs: introduce get_worktree_ref_store()
worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
refs: kill set_worktree_head_symref()
branch.c | 15 ++--- environment.c | 2 + refs.c | 100 ++++++++++++++++++++------------ refs.h | 12 +--- refs/files-backend.c | 44 -------------- refs/refs-internal.h | 4 ++ t/helper/test-ref-store.c | 18 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 52 +++++++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 10 files changed, 177 insertions(+), 174 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh
Signature · 1 line
2.11.0.157.gd943d85
PATCH v5 1/6 environment.c: fix potential segfault by get_git_common_dir()
setup_git_env() must be called before this function to initialize git_common_dir so that it returns a non NULL string. And it must return a non NULL string or segfault can happen because all callers expect so.
It does not do so explicitly though and depends on get_git_dir() being called first (which will guarantee setup_git_env()). Avoid this dependency and call setup_git_env() by itself.
test-ref-store.c will hit this problem because it's very lightweight, just enough initialization to exercise refs code, and get_git_dir() will never be called until get_worktrees() is, which uses get_git_common_dir and hits a segfault.
--- environment.c | 2 ++ 1 file changed, 2 insertions(+)
environment.c +2 -0
@@ -214,6 +214,8 @@ const char *get_git_dir(void)const char *get_git_common_dir (void ) { +if (!git_dir) +setup_git_env ();return git_common_dir; }
Signature · 1 line
2.11.0.157.gd943d85
PATCH v5 2/6 refs.c: make submodule ref store hashmap generic
In reply to Nguyễn Thái Ngọc DuyThis removes the "submodule" from submodule_hash_entry and other function names. The goal is to reuse the same code and data structure for other ref store types. The first one is worktree ref stores.
--- refs.c | 64 ++++++++++++++++++++++++++++++++-------------------------------- 1 file changed, 32 insertions(+), 32 deletions(-)
refs.c +32 -32
@@ -1450,32 +1450,32 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,return 0 ; } -struct submodule_hash_entry +struct ref_store_hash_entry {struct hashmap_entry ent;/* must be the first member! */ struct ref_store *refs; -/* NUL-terminated name of submodule: */ -char submodule[FLEX_ARRAY ]; +/* NUL-terminated identifier of the ref store: */ +char name[FLEX_ARRAY ]; }; -static int submodule_hash_cmp (const void *entry,const void *entry_or_key, +static int ref_store_hash_cmp (const void *entry,const void *entry_or_key,const void *keydata) { -const struct submodule_hash_entry *e1 = entry, *e2 = entry_or_key; -const char *submodule = keydata ? keydata : e2->submodule; +const struct ref_store_hash_entry *e1 = entry, *e2 = entry_or_key; +const char *name = keydata ? keydata : e2->name; -return strcmp (e1->submodule, submodule); +return strcmp (e1->name, name); } -static struct submodule_hash_entry *alloc_submodule_hash_entry ( -const char *submodule,struct ref_store *refs) +static struct ref_store_hash_entry *alloc_ref_store_hash_entry ( +const char *name,struct ref_store *refs) { -struct submodule_hash_entry *entry; +struct ref_store_hash_entry *entry; -FLEX_ALLOC_STR (entry, submodule, submodule); -hashmap_entry_init (entry,strhash (submodule)); +FLEX_ALLOC_STR (entry, name, name); +hashmap_entry_init (entry,strhash (name)); entry->refs = refs;return entry; }
@@ -1487,19 +1487,19 @@ static struct ref_store *main_ref_store;static struct hashmap submodule_ref_stores;/* -* Return the ref_store instance for the specified submodule. If that -* ref_store hasn't been initialized yet, return NULL. +* Look up a ref store by name. If that ref_store hasn't been +* registered yet, return NULL. */ -static struct ref_store *lookup_submodule_ref_store (const char *submodule) +static struct ref_store *lookup_ref_store_map (struct hashmap *map, +const char *name) { -struct submodule_hash_entry *entry; +struct ref_store_hash_entry *entry; -if (!submodule_ref_stores.tablesize) +if (!map->tablesize)/* It's initialized on demand in register_ref_store(). */ return NULL ; - entry =hashmap_get_from_hash (&submodule_ref_stores, -strhash (submodule), submodule); + entry =hashmap_get_from_hash (map,strhash (name), name);return entry ? entry->refs :NULL ; }
@@ -1535,20 +1535,19 @@ struct ref_store *get_main_ref_store(void) }/* -* Register the specified ref_store to be the one that should be used -* for submodule. It is a fatal error to call this function twice for -* the same submodule. +* Associate a ref store with a name. It is a fatal error to call this +* function twice for the same name. */ -static void register_submodule_ref_store (struct ref_store *refs, -const char *submodule) +static void register_ref_store_map (struct hashmap *map, +const char *type, +struct ref_store *refs, +const char *name) { -if (!submodule_ref_stores.tablesize) -hashmap_init (&submodule_ref_stores, submodule_hash_cmp,0 ); +if (!map->tablesize) +hashmap_init (map, ref_store_hash_cmp,0 ); -if (hashmap_put (&submodule_ref_stores, -alloc_submodule_hash_entry (submodule, refs))) -die ("BUG: ref_store for submodule '%s' initialized twice" , - submodule); +if (hashmap_put (map,alloc_ref_store_hash_entry (name, refs))) +die ("BUG: %s ref_store '%s' initialized twice" , type, name); }struct ref_store *get_submodule_ref_store (const char *submodule)
@@ -1565,7 +1564,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return get_main_ref_store (); } - refs =lookup_submodule_ref_store (submodule); + refs =lookup_ref_store_map (&submodule_ref_stores, submodule);if (refs)return refs;
@@ -1584,7 +1583,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule)/* assume that add_submodule_odb() has been called */ refs =ref_store_init (submodule_sb.buf,REF_STORE_READ |REF_STORE_ODB ); -register_submodule_ref_store (refs, submodule); +register_ref_store_map (&submodule_ref_stores,"submodule" , + refs, submodule);strbuf_release (&submodule_sb);return refs;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v5 3/6 refs: add REFS_STORE_ALL_CAPS
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 6 +----- refs/refs-internal.h | 4 ++++ 2 files changed, 5 insertions(+), 5 deletions(-)
refs.c +1 -5
@@ -1526,11 +1526,7 @@ struct ref_store *get_main_ref_store(void)if (main_ref_store)return main_ref_store; - main_ref_store =ref_store_init (get_git_dir (), - (REF_STORE_READ | -REF_STORE_WRITE | -REF_STORE_ODB | -REF_STORE_MAIN )); + main_ref_store =ref_store_init (get_git_dir (),REF_STORE_ALL_CAPS );return main_ref_store; }
refs/refs-internal.h +4 -0
@@ -467,6 +467,10 @@ struct ref_store;#define REF_STORE_WRITE (1 << 1)/* can perform update operations */ #define REF_STORE_ODB (1 << 2)/* has access to object database */ #define REF_STORE_MAIN (1 << 3) +#define REF_STORE_ALL_CAPS (REF_STORE_READ | \ + REF_STORE_WRITE | \ + REF_STORE_ODB | \ + REF_STORE_MAIN)/* * Initialize the ref_store for the specified gitdir. These functions
Signature · 1 line
2.11.0.157.gd943d85
PATCH v5 4/6 refs: introduce get_worktree_ref_store()
In reply to Nguyễn Thái Ngọc Duyfiles-backend at this point is still aware of the per-repo/worktree separation in refs, so it can handle a linked worktree.
Some refs operations are known not working when current files-backend is used in a linked worktree (e.g. reflog). Tests will be written when refs_* functions start to be called with worktree backend to verify that they work as expected.
Note: accessing a worktree of a submodule remains unaddressed. Perhaps after get_worktrees() can access submodule (or rather a new function get_submodule_worktrees(), that lists worktrees of a submodule), we can update this function to work with submodules as well.
--- refs.c | 30 ++++++++++++++++++++++++++++++ refs.h | 2 ++ 2 files changed, 32 insertions(+)
refs.c +30 -0
@@ -10,6 +10,7 @@#include "object.h" #include "tag.h" #include "submodule.h" +#include "worktree.h" /* * List of all available backends
@@ -1486,6 +1487,9 @@ static struct ref_store *main_ref_store;/* A hashmap of ref_stores, stored by submodule name: */ static struct hashmap submodule_ref_stores; +/* A hashmap of ref_stores, stored by worktree id: */ +static struct hashmap worktree_ref_stores; +/* * Look up a ref store by name. If that ref_store hasn't been * registered yet, return NULL.
@@ -1586,6 +1590,32 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return refs; } +struct ref_store *get_worktree_ref_store (const struct worktree *wt) +{ +struct ref_store *refs; +const char *id; + +if (wt->is_current) +return get_main_ref_store (); + + id = wt->id ? wt->id :"/" ; + refs =lookup_ref_store_map (&worktree_ref_stores, id); +if (refs) +return refs; + +if (wt->id) + refs =ref_store_init (git_common_path ("worktrees/%s" , wt->id), +REF_STORE_ALL_CAPS ); +else + refs =ref_store_init (get_git_common_dir (), +REF_STORE_ALL_CAPS ); + +if (refs) +register_ref_store_map (&worktree_ref_stores,"worktree" , + refs, id); +return refs; +} +void base_ref_store_init (struct ref_store *refs,const struct ref_storage_be *be) {
refs.h +2 -0
@@ -5,6 +5,7 @@ struct object_id;struct ref_store ;struct strbuf ;struct string_list ; +struct worktree ;/* * Resolve a reference, recursively following symbolic refererences.
@@ -655,5 +656,6 @@ struct ref_store *get_main_ref_store(void);* submodule==NULL. */ struct ref_store *get_submodule_ref_store (const char *submodule); +struct ref_store *get_worktree_ref_store (const struct worktree *wt);#endif /* REFS_H */
Signature · 1 line
2.11.0.157.gd943d85
PATCH v5 5/6 worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
In reply to Nguyễn Thái Ngọc DuyThe manual parsing code is replaced with a call to refs_resolve_ref_unsafe(). The manual parsing code must die because only refs/files-backend.c should do that.
--- branch.c | 3 +- t/helper/test-ref-store.c | 18 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 40 +++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 5 files changed, 88 insertions(+), 77 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh
branch.c +2 -1
@@ -355,7 +355,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,for (i =0 ; worktrees[i]; i++) {if (worktrees[i]->is_detached)continue ; -if (strcmp (oldref, worktrees[i]->head_ref)) +if (worktrees[i]->head_ref && +strcmp (oldref, worktrees[i]->head_ref))continue ; if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
t/helper/test-ref-store.c +18 -0
@@ -1,5 +1,6 @@#include "cache.h" #include "refs.h" +#include "worktree.h" static const char *notnull (const char *arg,const char *name) {
@@ -32,6 +33,23 @@ static const char **get_store(const char **argv, struct ref_store **refs)strbuf_release (&sb); *refs =get_submodule_ref_store (gitdir); + }else if (skip_prefix (argv[0 ],"worktree:" , &gitdir)) { +struct worktree **p, **worktrees =get_worktrees (0 ); + +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; + +if (!wt->id) { +/* special case for main worktree */ +if (!strcmp (gitdir,"main" )) +break ; + }else if (!strcmp (gitdir, wt->id)) +break ; + } +if (!*p) +die ("no such worktree: %s" , gitdir); + + *refs =get_worktree_ref_store (*p); }else die ("unknown backend %s" , argv[0 ]);
t/t1407-worktree-ref-store.sh new file +40 -0
@@ -0,0 +1,40 @@ +#!/bin/sh + +test_description='test worktree ref store api' + +. ./test-lib.sh + +RWT="test-ref-store worktree:wt" +RMAIN="test-ref-store worktree:main" + +test_expect_success 'setup' ' + test_commit first && + git worktree add -b wt-master wt && + ( + cd wt && + test_commit second + ) +' + +test_expect_success 'resolve_ref(<shared-ref>)' ' + SHA1=`git rev-parse master` && + echo "$SHA1 refs/heads/master 0x0" >expected && + $RWT resolve-ref refs/heads/master 0 >actual && + test_cmp expected actual && + $RMAIN resolve-ref refs/heads/master 0 >actual && + test_cmp expected actual +' + +test_expect_success 'resolve_ref(<per-worktree-ref>)' ' + SHA1=`git -C wt rev-parse HEAD` && + echo "$SHA1 refs/heads/wt-master 0x1" >expected && + $RWT resolve-ref HEAD 0 >actual && + test_cmp expected actual && + + SHA1=`git rev-parse HEAD` && + echo "$SHA1 refs/heads/master 0x1" >expected && + $RMAIN resolve-ref HEAD 0 >actual && + test_cmp expected actual +' + +test_done
worktree.c +27 -75
169 lines not on this page · raw message
worktree.h +1 -1
@@ -4,7 +4,7 @@struct worktree {char *path;char *id; -char *head_ref; +char *head_ref;/* NULL if HEAD is broken or detached */ char *lock_reason;/* internal use */ unsigned char head_sha1[20 ];int is_detached;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v5 6/6 refs: kill set_worktree_head_symref()
In reply to Nguyễn Thái Ngọc Duy70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27) added this function in order to update HEADs of all relevant worktrees, when a branch is renamed.
It, as a public ref api, kind of breaks abstraction when it uses internal functions of files backend. With the introduction of refs_create_symref(), we can move back pretty close to the code before 70999e9cec, where create_symref() was used for updating HEAD.
--- branch.c | 12 ++++++------ refs.h | 10 ---------- refs/files-backend.c | 44 ------------------------------------------- t/t1407-worktree-ref-store.sh | 12 ++++++++++++ 4 files changed, 18 insertions(+), 60 deletions(-)
branch.c +6 -6
@@ -353,18 +353,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref,int i;for (i =0 ; worktrees[i]; i++) { +struct ref_store *refs; +if (worktrees[i]->is_detached)continue ;if (worktrees[i]->head_ref &&strcmp (oldref, worktrees[i]->head_ref))continue ; -if (set_worktree_head_symref (get_worktree_git_dir (worktrees[i]), - newref, logmsg)) { - ret =-1 ; -error (_ ("HEAD of working tree %s is not updated" ), - worktrees[i]->path); - } + refs =get_worktree_ref_store (worktrees[i]); +if (refs_create_symref (refs,"HEAD" , newref, logmsg)) + ret =error (_ ("HEAD of working tree %s is not updated" ), + worktrees[i]->path); }free_worktrees (worktrees);
refs.h +0 -10
@@ -402,16 +402,6 @@ int refs_create_symref(struct ref_store *refs, const char *refname, const char *target, const char *logmsg);int create_symref (const char *refname,const char *target,const char *logmsg); -/* -* Update HEAD of the specified gitdir. -* Similar to create_symref("relative-git-dir/HEAD", target, NULL), but -* this can update the main working tree's HEAD regardless of where -* $GIT_DIR points to. -* Return 0 if successful, non-zero otherwise. -* */ -int set_worktree_head_symref (const char *gitdir,const char *target, -const char *logmsg); -enum action_on_err {UPDATE_REFS_MSG_ON_ERR ,UPDATE_REFS_DIE_ON_ERR ,
refs/files-backend.c +0 -44
50 lines not on this page · raw message
t/t1407-worktree-ref-store.sh +12 -0
@@ -37,4 +37,16 @@ test_expect_success 'resolve_ref(<per-worktree-ref>)' ' test_cmp expected actual ' +test_expect_success 'create_symref(FOO, refs/heads/master)' ' + $RWT create-symref FOO refs/heads/master nothing && + echo refs/heads/master >expected && + git -C wt symbolic-ref FOO >actual && + test_cmp expected actual && + + $RMAIN create-symref FOO refs/heads/wt-master nothing && + echo refs/heads/wt-master >expected && + git symbolic-ref FOO >actual && + test_cmp expected actual +' + test_done
Signature · 1 line
2.11.0.157.gd943d85
Quoted 1 line · Nguyễn Thái Ngọc Duy
v5 chanes are mostly cosmetic:
Good changes. Will re-queue.
Thanks.
Quoted 77 lines
refs.c +3
-10@@ -1530,11 +1530,7 @@ struct ref_store *get_main_ref_store(void)if (main_ref_store)return main_ref_store; - main_ref_store =ref_store_init (get_git_dir (), - (REF_STORE_READ | -REF_STORE_WRITE | -REF_STORE_ODB | -REF_STORE_MAIN )); + main_ref_store =ref_store_init (get_git_dir (),REF_STORE_ALL_CAPS );return main_ref_store; }@@ -1597,9 +1593,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)struct ref_store *get_worktree_ref_store (const struct worktree *wt) {struct ref_store *refs; -unsigned int refs_all_capabilities = -REF_STORE_READ |REF_STORE_WRITE | -REF_STORE_ODB |REF_STORE_MAIN ;const char *id;if (wt->is_current)@@ -1612,10 +1605,10 @@ struct ref_store *get_worktree_ref_store(const struct worktree *wt)if (wt->id) refs =ref_store_init (git_common_path ("worktrees/%s" , wt->id), - refs_all_capabilities); +REF_STORE_ALL_CAPS );else refs =ref_store_init (get_git_common_dir (), - refs_all_capabilities); +REF_STORE_ALL_CAPS );if (refs) register_ref_store_map(&worktree_ref_stores, "worktree",refs/refs-internal.h +4
-0@@ -467,6 +467,10 @@ struct ref_store;#define REF_STORE_WRITE (1 << 1)/* can perform update operations */ #define REF_STORE_ODB (1 << 2)/* has access to object database */ #define REF_STORE_MAIN (1 << 3) +#define REF_STORE_ALL_CAPS (REF_STORE_READ | \ + REF_STORE_WRITE | \ + REF_STORE_ODB | \ + REF_STORE_MAIN)/* * Initialize the ref_store for the specified gitdir. These functions Nguyễn Thái Ngọc Duy (6):
environment.c: fix potential segfault byget_git_common_dir()refs.c: make submodule ref store hashmap generic
refs: addREFS_STORE_ALL_CAPS
refs: introduceget_worktree_ref_store()
worktree.c: killparse_ref()in favor ofrefs_resolve_ref_unsafe()
refs: killset_worktree_head_symref()branch.c | 15 ++--- environment.c | 2 + refs.c | 100 ++++++++++++++++++++------------ refs.h | 12 +--- refs/files-backend.c | 44 -------------- refs/refs-internal.h | 4 ++ t/helper/test-ref-store.c | 18 ++++++ t/t1407-worktree-ref-store.sh (new +x) | 52 +++++++++++++++++ worktree.c | 102 +++++++++------------------------ worktree.h | 2 +- 10 files changed, 177 insertions(+), 174 deletions(-) create mode 100755 t/t1407-worktree-ref-store.sh