Fix git-gc losing objects in multi worktree
v4 cover Fix git-gc losing objects in multi worktree
"git gc" when used in multiple worktrees ignore some per-worktree
references: object references in the index, HEAD and reflog. This series fixes it by making the revision walker include these from all worktrees by default (and the series is basically split in three parts in the same order). There's a couple more cleanups in refs.c. Luckily it does not conflict with anything in 'pu'.
Compared to v3 [1], the largest change is supporting multi worktree in the reflog iterator. The merge iterator is now used (Micheal was right all along).
[1] https://public-inbox.org/git/[email protected]/
Nguyễn Thái Ngọc Duy (16):
revision.h: new flag in struct rev_info wrt. worktree-related refs refs.c: use is_dir_sep() in resolve_gitlink_ref() revision.c: refactor add_index_objects_to_pending() revision.c: --indexed-objects add objects from all worktrees refs.c: refactor get_submodule_ref_store(), share common free block
refs: move submodule slash stripping code to get_submodule_ref_store
refs: add refs_head_ref()
revision.c: use refs_for_each*() instead of for_each_*_submodule() refs.c: move for_each_remote_ref_submodule() to submodule.c
refs: remove dead for_each_*_submodule()
revision.c: --all adds HEAD from all worktrees
files-backend: make reflog iterator go through per-worktree reflog revision.c: --reflog add HEAD reflog from all worktrees
rev-list: expose and document --single-worktree
refs.c: remove fallback-to-main-store code get_submodule_ref_store() refs.c: reindent get_submodule_ref_store()
Documentation/rev-list-options.txt | 8 ++ Documentation/technical/api-ref-iteration.txt | 7 +- reachable.c | 2 + refs.c | 110 ++++++--------------- refs.h | 20 +--- refs/files-backend.c | 59 +++++++++--- revision.c | 131 +++++++++++++++++++++----- revision.h | 1 + submodule.c | 9 ++ t/t1407-worktree-ref-store.sh | 30 ++++++ t/t5304-prune.sh | 37 ++++++++ worktree.c | 22 +++++ worktree.h | 8 ++ 13 files changed, 308 insertions(+), 136 deletions(-)
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 1/16 revision.h: new flag in struct rev_info wrt. worktree-related refs
The revision walker can walk through per-worktree refs like HEAD or SHA-1 references in the index. These currently are from the current worktree only. This new flag is added to change rev-list behavior in this regard:
When single_worktree is set, only current worktree is considered. When it is not set (which is the default), all worktrees are considered.
The default is chosen so because the two big components that rev-list works with are object database (entirely shared between worktrees) and refs (mostly shared). It makes sense that default behavior goes per-repo too instead of per-worktree.
The flag will eventually be exposed as a rev-list argument with documents. For now it stays internal until the new behavior is fully implemented.
--- revision.h | 1 + 1 file changed, 1 insertion(+)
revision.h +1 -0
@@ -96,6 +96,7 @@ struct rev_info { topo_order:1, simplify_merges:1, simplify_by_decoration:1, + single_worktree:1, tag_objects:1, tree_objects:1, blob_objects:1,
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 2/16 refs.c: use is_dir_sep() in resolve_gitlink_ref()
In reply to Nguyễn Thái Ngọc DuyThe "submodule" argument in this function is a path, which can have either '/' or '\\' as a separator. Use is_dir_sep() to support both.
--- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
refs.c +1 -1
@@ -1507,7 +1507,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,struct ref_store *refs;int flags; -while (len && submodule[len -1 ] =='/' ) +while (len &&is_dir_sep (submodule[len -1 ])) len--;if (!len)
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()
Quoted 5 lines · Nguyễn Thái Ngọc Duy
The "submodule" argument in this function is a path, which can have either '/' or '\\' as a separator. Use
is_dir_sep()to support both.Noticed-by Johannes Sixt <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
Immediate questions that come to mind:
* Could this go in as an independent bug fix?
-> If so do we have any test that fails or stops failing on Windows? -> If not, do we need one?
* Assuming this is not an independent bug fix:
Why do we need this patch in this series here?
(I thought this is about worktrees, not submodules.
So does this fix a potential bug that will be exposed later in this series, but was harmless up to now?)
I'll read the next patches to see if I will be enlightened.
Thanks,
Stefan
Quoted 20 lines
--- refs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)refs.c +1
-2@@ -1507,7 +1507,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,struct ref_store *refs;int flags; -while (len && submodule[len -1 ] =='/' ) +while (len &&is_dir_sep (submodule[len -1 ])) len--;if (!len) --2.11.0.157.gd943d85
Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()
Quoted 9 lines · Stefan Beller
Quoted 5 lines · Nguyễn Thái Ngọc Duy
The "submodule" argument in this function is a path, which can have either '/' or '\\' as a separator. Use
is_dir_sep()to support both.Noticed-by Johannes Sixt <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>Immediate questions that come to mind:
* Could this go in as an independent bug fix?
It probably could. But it may depend on other submodule changes in this series.
Quoted 1 line
-> If so do we have any test that fails or stops failing on Windows?
It was spotted during the review [1] so I guess no test fails on Windows (not that I would know because I can't run tests on Windows)
Quoted 1 line
-> If not, do we need one?
Since I don't use Windows, I don't particularly care. And I can't add one anyway because I can't run it.
[1] https://public-inbox.org/git/%[email protected]%3E/
Quoted 5 lines
* Assuming this is not an independent bug fix:
Why do we need this patch in this series here? (I thought this is about worktrees, not submodules. So does this fix a potential bug that will be exposed later in this series, but was harmless up to now?)
The series could probably be split in two. The first part is about using the new refs_* API in revision.c. This helps clean up refs API a bit, all *_submodule() functions will be one. Not strictly needed to be part of this, it's just a continuation of my previous series that introduces *_refs. Since submodule code is touched, this is found.
The second part is actually fixing the prune bug.
Should I split the series in two? I think I separated two parts in the past (maybe I misremember) at least in the description...
Signature · 1 line
Duy
Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()
Quoted 18 lines · Duy Nguyen
Quoted 9 lines · Stefan Beller
Quoted 5 lines · Nguyễn Thái Ngọc Duy
The "submodule" argument in this function is a path, which can have either '/' or '\\' as a separator. Use
is_dir_sep()to support both.Noticed-by Johannes Sixt <[email protected]>Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>Immediate questions that come to mind:
* Could this go in as an independent bug fix?It probably could. But it may depend on other submodule changes in this series.
Quoted 1 line
-> If so do we have any test that fails or stops failing on Windows?
It was spotted during the review [1] so I guess no test fails on Windows (not that I would know because I can't run tests on Windows)
Quoted 6 lines
Quoted 1 line
-> If not, do we need one?
Since I don't use Windows, I don't particularly care. And I can't add one anyway because I can't run it.
IIRC I asked these questions as I was genuinely confused, I do not mind too much either.
Quoted 13 lines
Quoted 6 lines
* Assuming this is not an independent bug fix:
Why do we need this patch in this series here? (I thought this is about worktrees, not submodules. So does this fix a potential bug that will be exposed later in this series, but was harmless up to now?)The series could probably be split in two. The first part is about using the new refs_* API in
revision.c. This helps clean up refs API a bit, all*_submodule()functions will be one. Not strictly needed to be part of this, it's just a continuation of my previous series that introduces*_refs. Since submodule code is touched, this is found.
Quoted 4 lines
The second part is actually fixing the prune bug.
Should I split the series in two? I think I separated two parts in the past (maybe I misremember) at least in the description...
I had to reread the coverletter
https://public-inbox.org/git/[email protected]/
to get that part. I would not be opposed to splitting the series, but I'll review an unsplit series as well.
Thanks,
Stefan
Quoted 2 lines
--
Duy
PATCH v4 3/16 revision.c: refactor add_index_objects_to_pending()
In reply to Nguyễn Thái Ngọc DuyThe core code is factored out and take 'struct index_state *' instead so that we can reuse it to add objects from index files other than .git/index in the next patch.
--- revision.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
revision.c +12 -6
@@ -1262,13 +1262,13 @@ static void add_cache_tree(struct cache_tree *it, struct rev_info *revs, } -void add_index_objects_to_pending (struct rev_info *revs,unsigned flags) +static void do_add_index_objects_to_pending (struct rev_info *revs, +struct index_state *istate) {int i; -read_cache (); -for (i =0 ; i < active_nr; i++) { -struct cache_entry *ce = active_cache[i]; +for (i =0 ; i < istate->cache_nr; i++) { +struct cache_entry *ce = istate->cache[i];struct blob *blob;if (S_ISGITLINK (ce->ce_mode))
@@ -1281,13 +1281,19 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags) ce->ce_mode, ce->name); } -if (active_cache_tree) { +if (istate->cache_tree) {struct strbuf path =STRBUF_INIT ; -add_cache_tree (active_cache_tree, revs, &path); +add_cache_tree (istate->cache_tree, revs, &path);strbuf_release (&path); } } +void add_index_objects_to_pending (struct rev_info *revs,unsigned int flags) +{ +read_cache (); +do_add_index_objects_to_pending (revs, &the_index); +} +static int add_parents_only (struct rev_info *revs,const char *arg_,int flags,int exclude_parent) {
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 4/16 revision.c: --indexed-objects add objects from all worktrees
In reply to Nguyễn Thái Ngọc DuyThis is the result of single_worktree flag never being set (no way to up until now). To get objects from current index only, set single_worktree.
The other add_index_objects_to_pending's caller is mark_reachable_objects() (e.g. "git prune") which also mark objects from all indexes.
--- revision.c | 21 +++++++++++++++++++++ t/t5304-prune.sh | 9 +++++++++ 2 files changed, 30 insertions(+)
revision.c +21 -0
@@ -19,6 +19,7 @@#include "dir.h" #include "cache-tree.h" #include "bisect.h" +#include "worktree.h" volatile show_early_output_fn_t show_early_output;
@@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,void add_index_objects_to_pending (struct rev_info *revs,unsigned int flags) { +struct worktree **worktrees, **p; +read_cache ();do_add_index_objects_to_pending (revs, &the_index); + +if (revs->single_worktree) +return ; + + worktrees =get_worktrees (0 ); +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; +struct index_state istate = {NULL }; + +if (wt->is_current) +continue ;/* current index already taken care of */ + +if (read_index_from (&istate, +worktree_git_path (wt,"index" )) >0 ) +do_add_index_objects_to_pending (revs, &istate); +discard_index (&istate); + } +free_worktrees (worktrees); } static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
t/t5304-prune.sh +9 -0
@@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object database' ' git -C B prune ' +test_expect_success 'prune: handle index in multiple worktrees' ' + git worktree add second-worktree && + echo "new blob for second-worktree" >second-worktree/blob && + git -C second-worktree add blob && + git prune --expire=now && + git -C second-worktree show :blob >actual && + test_cmp second-worktree/blob actual +' + test_done
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 04/16] revision.c: --indexed-objects add objects from all worktrees
Quoted 41 lines · Nguyễn Thái Ngọc Duy
This is the result of
single_worktreeflag never being set (no way to up until now). To get objects from current index only, setsingle_worktree.The other add_index_objects_to_pending's caller is
mark_reachable_objects()(e.g. "git prune") which also mark objects from all indexes.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- revision.c | 21 +++++++++++++++++++++ t/t5304-prune.sh | 9 +++++++++ 2 files changed, 30 insertions(+)revision.c +12
-0@@ -19,6 +19,7 @@#include "dir.h" #include "cache-tree.h" #include "bisect.h" +#include "worktree.h" volatile show_early_output_fn_t show_early_output;@@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,void add_index_objects_to_pending (struct rev_info *revs,unsigned int flags) { +struct worktree **worktrees, **p; +read_cache ();do_add_index_objects_to_pending (revs, &the_index); + +if (revs->single_worktree) +return ; + + worktrees =get_worktrees (0 ); +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; +struct index_state istate = {NULL }; +
revision.c static void do_add_index_objects_to_pending(struct rev_info *revs, · line 1316
Quoted 10 lines
revision.c +9
-0+if (wt->is_current) +continue ;/* current index already taken care of */ + +if (read_index_from (&istate, +worktree_git_path (wt,"index" )) >0 ) +do_add_index_objects_to_pending (revs, &istate); +discard_index (&istate); + } +free_worktrees (worktrees); }
The commit (message and code) looks acceptable and easy to read. I wonder though if another approach would be more feasible:
* factor out anything after the early return into a new function "add_wt_index_objects_to_pending", maybe?
* introduce a new function for_each_wt_except_current(function, callback_cookie) such that this code could use this predefined iterator. (I have the impression I have seen this code pattern before, hence it may be useful to have such a foreach function; we have foreach functions already for various ref things, string list items, reflogs, and soon submodule lists)
Quoted 23 lines
revision.c +0
-0static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,t/t5304-prune.sh +9
-1@@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object database' ' git -C B prune ' +test_expect_success 'prune: handle index in multiple worktrees' ' + git worktree add second-worktree && + echo "new blob for second-worktree" >second-worktree/blob && + git -C second-worktree add blob && + git prune --expire=now && + git -C second-worktree show :blob >actual && + test_cmp second-worktree/blob actual +' + test_done --2.11.0.157.gd943d85
PATCH v4 5/16 refs.c: refactor get_submodule_ref_store(), share common free block
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)
refs.c +6 -11
@@ -1636,7 +1636,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; -int ret;if (!submodule || !*submodule) {/*
@@ -1648,19 +1647,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule) refs =lookup_ref_store_map (&submodule_ref_stores, submodule);if (refs) -return refs; + goto done;strbuf_addstr (&submodule_sb, submodule); - ret =is_nonbare_repository_dir (&submodule_sb); -strbuf_release (&submodule_sb); -if (!ret) -return NULL ; +if (!is_nonbare_repository_dir (&submodule_sb)) + goto done; - ret =submodule_to_gitdir (&submodule_sb, submodule); -if (ret) { -strbuf_release (&submodule_sb); -return NULL ; - } +if (submodule_to_gitdir (&submodule_sb, submodule)) + goto done;/* assume that add_submodule_odb() has been called */ refs = ref_store_init(submodule_sb.buf,
@@ -1668,6 +1662,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)register_ref_store_map (&submodule_ref_stores,"submodule" , refs, submodule); +done:strbuf_release (&submodule_sb);return refs; }
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 05/16] refs.c: refactor get_submodule_ref_store(), share common free block
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1638
Quoted 13 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)refs.c +0
-0@@ -1636,7 +1636,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs;
Do we need to assign NULL here, to cover the early outs?
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1648
Quoted 7 lines
refs.c +0
-1-int ret;if (!submodule || !*submodule) {/* @@ -1648,19 +1647,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule) refs =lookup_ref_store_map (&submodule_ref_stores, submodule);
Answering the question from above, no we do not need to assign NULL as we have an unconditional assignment here and any early outs are after this.
Thanks,
Stefan
PATCH v4 6/16 refs: move submodule slash stripping code to get_submodule_ref_store
In reply to Nguyễn Thái Ngọc DuyThis is a better place that will benefit all submodule callers instead of just resolve_gitlink_ref()
--- refs.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
refs.c +17 -16
@@ -1503,25 +1503,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,int resolve_gitlink_ref (const char *submodule,const char *refname,unsigned char *sha1) { -size_t len =strlen (submodule);struct ref_store *refs;int flags; -while (len &&is_dir_sep (submodule[len -1 ])) - len--; - -if (!len) -return -1 ; - -if (submodule[len]) { -/* We need to strip off one or more trailing slashes */ -char *stripped =xmemdupz (submodule, len); - - refs =get_submodule_ref_store (stripped); -free (stripped); - }else { - refs =get_submodule_ref_store (submodule); - } + refs =get_submodule_ref_store (submodule);if (!refs)return -1 ;
@@ -1636,6 +1621,16 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; +char *to_free =NULL ; +size_t len; + +if (submodule) { + len =strlen (submodule); +while (len &&is_dir_sep (submodule[len -1 ])) + len--; +if (!len) +return NULL ; + }if (!submodule || !*submodule) {/*
@@ -1645,6 +1640,10 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return get_main_ref_store (); } +if (submodule[len]) +/* We need to strip off one or more trailing slashes */ + submodule = to_free =xmemdupz (submodule, len); + refs =lookup_ref_store_map (&submodule_ref_stores, submodule);if (refs) goto done;
@@ -1664,6 +1663,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule) done:strbuf_release (&submodule_sb); +free (to_free); +return refs; }
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 06/16] refs: move submodule slash stripping code to get_submodule_ref_store
Quoted 2 lines · Nguyễn Thái Ngọc Duy
This is a better place that will benefit all submodule callers instead of just
resolve_gitlink_ref()
This is a nice sentiment, but I have to wonder whether we should rather be requiring callers to use the "canonical" submodule name rather than covering up their sloppiness for them (perhaps multiple times?). I vaguely remember intentionally limiting the number of functions that do this canonicalization, and having in mind the goal that the number should become smaller over time, not larger.
For example, there could be some kind of canonicalize_submodule_name() function that callers can use to clean up whatever submodule string they have in hand, then let them use the cleaned up value from then on.
I don't really know much about the callers, though, so that is more of a lazy philosophical speculation than a concrete suggestion.
Michael
PATCH v4 7/16 refs: add refs_head_ref()
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 23 +++++++++++++---------- refs.h | 2 ++ 2 files changed, 15 insertions(+), 10 deletions(-)
refs.c +13 -10
@@ -1248,27 +1248,30 @@ int refs_rename_ref_available(struct ref_store *refs,return ok; } -int head_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data) +int refs_head_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data) {struct object_id oid;int flag; -if (submodule) { -if (resolve_gitlink_ref (submodule,"HEAD" , oid.hash) ==0 ) -return fn ("HEAD" , &oid,0 , cb_data); - -return 0 ; - } - -if (!read_ref_full ("HEAD" ,RESOLVE_REF_READING , oid.hash, &flag)) +if (!refs_read_ref_full (refs,"HEAD" ,RESOLVE_REF_READING , + oid.hash, &flag))return fn ("HEAD" , &oid, flag, cb_data);return 0 ; } +int head_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data) +{ +struct ref_store *refs =get_submodule_ref_store (submodule); + +if (!refs) +return -1 ; +return refs_head_ref (refs, fn, cb_data); +} +int head_ref (each_ref_fn fn,void *cb_data) { -return head_ref_submodule (NULL , fn, cb_data); +return refs_head_ref (get_main_ref_store (), fn, cb_data); } struct ref_iterator *refs_ref_iterator_begin(
refs.h +2 -0
@@ -275,6 +275,8 @@ typedef int each_ref_fn(const char *refname,* modifies the reference also returns a nonzero value to immediately * stop the iteration. Returned references are sorted. */ +int refs_head_ref (struct ref_store *refs, +each_ref_fn fn,void *cb_data);int refs_for_each_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data); int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 07/16] refs: add refs_head_ref()
Nguyễn Thái Ngọc Duy <[email protected]> writes:
... which does what?
Unlike refs_for_each_ref() and friends, this does not iterate. It just uses the same function signature to make a single call of fn on the "HEAD" ref.
Did I capture what it does right?
Thanks.
Re: [PATCH v4 07/16] refs: add refs_head_ref()
Quoted 9 lines · Junio C Hamano
Nguyễn Thái Ngọc Duy <[email protected]> writes:
... which does what?
Unlike
refs_for_each_ref()and friends, this does not iterate. It just uses the same function signature to make a single call of fn on the "HEAD" ref.Did I capture what it does right?
It's basically head_ref() but can take any ref store (while head_ref() always uses the main ref store). I'll update the commit message. What you describe is correct, but I think I'll leave that out and assume people know what head_ref() does.
Signature · 1 line
Duy
PATCH v4 8/16 revision.c: use refs_for_each*() instead of for_each_*_submodule()
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 9 ++++----- refs.h | 6 +++--- revision.c | 48 ++++++++++++++++++++++++++++++++---------------- 3 files changed, 39 insertions(+), 24 deletions(-)
refs.c +4 -5
@@ -1362,16 +1362,15 @@ int for_each_ref_in_submodule(const char *submodule, const char *prefix, prefix, fn, cb_data); } -int for_each_fullref_in_submodule (const char *submodule,const char *prefix, -each_ref_fn fn,void *cb_data, -unsigned int broken) +int refs_for_each_fullref_in (struct ref_store *refs,const char *prefix, +each_ref_fn fn,void *cb_data, +unsigned int broken) {unsigned int flag =0 ;if (broken) flag =DO_FOR_EACH_INCLUDE_BROKEN ; -return do_for_each_ref (get_submodule_ref_store (submodule), - prefix, fn,0 , flag, cb_data); +return do_for_each_ref (refs, prefix, fn,0 , flag, cb_data); }int for_each_replace_ref (each_ref_fn fn,void *cb_data)
refs.h +3 -3
@@ -291,6 +291,9 @@ int refs_for_each_remote_ref(struct ref_store *refs,int head_ref (each_ref_fn fn,void *cb_data);int for_each_ref (each_ref_fn fn,void *cb_data);int for_each_ref_in (const char *prefix,each_ref_fn fn,void *cb_data); +int refs_for_each_fullref_in (struct ref_store *refs,const char *prefix, +each_ref_fn fn,void *cb_data, +unsigned int broken);int for_each_fullref_in (const char *prefix,each_ref_fn fn,void *cb_data,unsigned int broken);int for_each_tag_ref (each_ref_fn fn,void *cb_data);
@@ -306,9 +309,6 @@ int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);int for_each_ref_in_submodule (const char *submodule,const char *prefix,each_ref_fn fn,void *cb_data); -int for_each_fullref_in_submodule (const char *submodule,const char *prefix, -each_ref_fn fn,void *cb_data, -unsigned int broken);int for_each_tag_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data); int for_each_branch_ref_submodule(const char *submodule,
revision.c +32 -16
97 lines not on this page · raw message
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 08/16] revision.c: use refs_for_each*() instead of for_each_*_submodule()
Quoted 2 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>---
So... the idea is that the caller gives a ref-store and tells these functions to iterate over refs in it, and the existing submodule related callers can prepare a ref-store for the submodule---that way, refs.[ch] layer does not have to _care_ that the set of refs it was asked to look at is for submodule processing.
Nice. Very nice.
Quoted 12 lines
revision.c +6
-0@@ -2120,8 +2129,14 @@ static int handle_revision_pseudo_opt(const char *submodule, {const char *arg = argv[0 ];const char *optarg; +struct ref_store *refs;int argcount; +if (submodule) { + refs =get_submodule_ref_store (submodule); + }else + refs =get_main_ref_store (); +
PATCH v4 9/16 refs.c: move for_each_remote_ref_submodule() to submodule.c
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 6 ------ refs.h | 2 -- submodule.c | 7 +++++++ 3 files changed, 7 insertions(+), 8 deletions(-)
refs.c +0 -6
@@ -368,12 +368,6 @@ int for_each_remote_ref(each_ref_fn fn, void *cb_data)return refs_for_each_remote_ref (get_main_ref_store (), fn, cb_data); } -int for_each_remote_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data) -{ -return refs_for_each_remote_ref (get_submodule_ref_store (submodule), - fn, cb_data); -} -int head_ref_namespaced (each_ref_fn fn,void *cb_data) {struct strbuf buf =STRBUF_INIT ;
refs.h +0 -2
@@ -313,8 +313,6 @@ int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);int for_each_branch_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data); -int for_each_remote_ref_submodule (const char *submodule, -each_ref_fn fn,void *cb_data);int head_ref_namespaced (each_ref_fn fn,void *cb_data);int for_each_namespaced_ref (each_ref_fn fn,void *cb_data);
submodule.c +7 -0
@@ -69,6 +69,13 @@ int is_staging_gitmodules_ok(const struct index_state *istate)return 1 ; } +static int for_each_remote_ref_submodule (const char *submodule, +each_ref_fn fn,void *cb_data) +{ +return refs_for_each_remote_ref (get_submodule_ref_store (submodule), + fn, cb_data); +} +/* * Try to update the "path" entry in the "submodule.<name>" section of the * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 10/16 refs: remove dead for_each_*_submodule()
In reply to Nguyễn Thái Ngọc DuyThese are used in revision.c. After the last patch they are replaced with the refs_ version. Delete them.
--- Documentation/technical/api-ref-iteration.txt | 7 ++---- refs.c | 33 --------------------------- refs.h | 10 -------- 3 files changed, 2 insertions(+), 48 deletions(-)
Documentation/technical/api-ref-iteration.txt +2 -5
@@ -32,11 +32,8 @@ Iteration functions * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined. -* `head_ref_submodule()`, `for_each_ref_submodule()`, - `for_each_ref_in_submodule()`, `for_each_tag_ref_submodule()`, - `for_each_branch_ref_submodule()`, `for_each_remote_ref_submodule()` - do the same as the functions described above but for a specified - submodule. +* Use `refs_` API for accessing submodules. The submodule ref store could + be obtained with `get_submodule_ref_store()`. * `for_each_rawref()` can be used to learn about broken ref and symref.
refs.c +0 -33
@@ -336,12 +336,6 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)return refs_for_each_tag_ref (get_main_ref_store (), fn, cb_data); } -int for_each_tag_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data) -{ -return refs_for_each_tag_ref (get_submodule_ref_store (submodule), - fn, cb_data); -} -int refs_for_each_branch_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data) {return refs_for_each_ref_in (refs,"refs/heads/" , fn, cb_data);
@@ -352,12 +346,6 @@ int for_each_branch_ref(each_ref_fn fn, void *cb_data)return refs_for_each_branch_ref (get_main_ref_store (), fn, cb_data); } -int for_each_branch_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data) -{ -return refs_for_each_branch_ref (get_submodule_ref_store (submodule), - fn, cb_data); -} -int refs_for_each_remote_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data) {return refs_for_each_ref_in (refs,"refs/remotes/" , fn, cb_data);
@@ -1254,15 +1242,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)return 0 ; } -int head_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data) -{ -struct ref_store *refs =get_submodule_ref_store (submodule); - -if (!refs) -return -1 ; -return refs_head_ref (refs, fn, cb_data); -} -int head_ref (each_ref_fn fn,void *cb_data) {return refs_head_ref (get_main_ref_store (), fn, cb_data);
@@ -1323,11 +1302,6 @@ int for_each_ref(each_ref_fn fn, void *cb_data)return refs_for_each_ref (get_main_ref_store (), fn, cb_data); } -int for_each_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data) -{ -return refs_for_each_ref (get_submodule_ref_store (submodule), fn, cb_data); -} -int refs_for_each_ref_in (struct ref_store *refs,const char *prefix,each_ref_fn fn,void *cb_data) {
@@ -1349,13 +1323,6 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsig prefix, fn, 0, flag, cb_data); } -int for_each_ref_in_submodule (const char *submodule,const char *prefix, -each_ref_fn fn,void *cb_data) -{ -return refs_for_each_ref_in (get_submodule_ref_store (submodule), - prefix, fn, cb_data); -} -int refs_for_each_fullref_in (struct ref_store *refs,const char *prefix,each_ref_fn fn,void *cb_data,unsigned int broken)
refs.h +0 -10
@@ -304,16 +304,6 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);int for_each_glob_ref_in (each_ref_fn fn,const char *pattern,const char *prefix,void *cb_data); -int head_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data); -int for_each_ref_submodule (const char *submodule, -each_ref_fn fn,void *cb_data); -int for_each_ref_in_submodule (const char *submodule,const char *prefix, -each_ref_fn fn,void *cb_data); -int for_each_tag_ref_submodule (const char *submodule, -each_ref_fn fn,void *cb_data); -int for_each_branch_ref_submodule (const char *submodule, -each_ref_fn fn,void *cb_data); -int head_ref_namespaced (each_ref_fn fn,void *cb_data);int for_each_namespaced_ref (each_ref_fn fn,void *cb_data);
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 10/16] refs: remove dead for_each_*_submodule()
Quoted 4 lines · Nguyễn Thái Ngọc Duy
These are used in
revision.c. After the last patch they are replaced with therefs_version. Delete them.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>
The refactoring up to this patch looks good to me.
Thanks,
Stefan
Re: [PATCH v4 10/16] refs: remove dead for_each_*_submodule()
In reply to Nguyễn Thái Ngọc DuyQuoted 9 lines · Nguyễn Thái Ngọc Duy
These are used in
revision.c. After the last patch they are replaced with therefs_version. Delete them.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- Documentation/technical/api-ref-iteration.txt | 7 ++---- refs.c | 33 --------------------------- refs.h | 10 -------- 3 files changed, 2 insertions(+), 48 deletions(-)
What a lovely diffstat :-)
Michael
PATCH v4 11/16 revision.c: --all adds HEAD from all worktrees
In reply to Nguyễn Thái Ngọc DuyUnless single_worktree is set, --all now adds HEAD from all worktrees.
Since reachable.c code does not use setup_revisions(), we need to call other_head_refs_submodule() explicitly there to have the same effect on "git prune", so that we won't accidentally delete objects needed by some other HEADs.
A new FIXME is added because we would need something like
int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);
in addition to other_head_refs() to handle it, which might require
int get_submodule_worktrees(const char *submodule, int flags);
It could be a separate topic to reduce the scope of this one.
--- reachable.c | 2 ++ revision.c | 14 ++++++++++++++ submodule.c | 2 ++ t/t5304-prune.sh | 12 ++++++++++++ worktree.c | 22 ++++++++++++++++++++++ worktree.h | 8 ++++++++ 6 files changed, 60 insertions(+)
reachable.c +2 -0
@@ -9,6 +9,7 @@#include "cache-tree.h" #include "progress.h" #include "list-objects.h" +#include "worktree.h" struct connectivity_progress {struct progress *progress;
@@ -176,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,/* detached HEAD is not included in the list above */ head_ref (add_one_ref, revs); +other_head_refs (add_one_ref, revs);/* Add all reflog info */ if (mark_reflog)
revision.c +14 -0
@@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char *submodule,int argcount;if (submodule) { +/* +* We need some something like get_submodule_worktrees() +* before we can go through all worktrees of a submodule, +* .e.g with adding all HEADs from --all, which is not +* supported right now, so stick to single worktree. +*/ +if (!revs->single_worktree) +die ("BUG: --single-worktree cannot be used together with submodule" ); refs =get_submodule_ref_store (submodule); }else refs =get_main_ref_store ();
@@ -2150,6 +2158,12 @@ static int handle_revision_pseudo_opt(const char *submodule,if (!strcmp (arg,"--all" )) {handle_refs (refs, revs, *flags, refs_for_each_ref);handle_refs (refs, revs, *flags, refs_head_ref); +if (!revs->single_worktree) { +struct all_refs_cb cb; + +init_all_refs_cb (&cb, revs, *flags); +other_head_refs (handle_one_ref, &cb); + }clear_ref_exclusion (&revs->ref_excludes); }else if (!strcmp (arg,"--branches" )) {handle_refs (refs, revs, *flags, refs_for_each_branch_ref);
submodule.c +2 -0
@@ -1685,6 +1685,8 @@ static int find_first_merges(struct object_array *result, const char *path, oid_to_hex(&a->object.oid));init_revisions (&revs,NULL ); rev_opts.submodule = path; +/* FIXME: can't handle linked worktrees in submodules yet */ + revs.single_worktree = path !=NULL ;setup_revisions (ARRAY_SIZE (rev_args)-1 , rev_args, &revs, &rev_opts);/* save all revisions from the above list that contain b */
t/t5304-prune.sh +12 -0
@@ -292,4 +292,16 @@ test_expect_success 'prune: handle index in multiple worktrees' ' test_cmp second-worktree/blob actual ' +test_expect_success 'prune: handle HEAD in multiple worktrees' ' + git worktree add --detach third-worktree && + echo "new blob for third-worktree" >third-worktree/blob && + git -C third-worktree add blob && + git -C third-worktree commit -m "third" && + rm .git/worktrees/third-worktree/index && + test_must_fail git -C third-worktree show :blob && + git prune --expire=now && + git -C third-worktree show HEAD:blob >actual && + test_cmp third-worktree/blob actual +' + test_done
worktree.c +22 -0
@@ -386,3 +386,25 @@ int submodule_uses_worktrees(const char *path)closedir (dir);return ret; } + +int other_head_refs (each_ref_fn fn,void *cb_data) +{ +struct worktree **worktrees, **p; +int ret =0 ; + + worktrees =get_worktrees (0 ); +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; +struct ref_store *refs; + +if (wt->is_current) +continue ; + + refs =get_worktree_ref_store (wt); + ret =refs_head_ref (refs, fn, cb_data); +if (ret) +break ; + } +free_worktrees (worktrees); +return ret; +}
worktree.h +8 -0
@@ -1,6 +1,8 @@ #ifndef WORKTREE_H #define WORKTREE_H +#include "refs.h" + struct worktree { char *path; char *id;
@@ -70,6 +72,12 @@ extern void free_worktrees(struct worktree **);extern const struct worktree *find_shared_symref (const char *symref,const char *target); +/* +* Similar to head_ref() for all HEADs _except_ one from the current +* worktree, which is covered by head_ref(). +*/ +int other_head_refs (each_ref_fn fn,void *cb_data); +int is_worktree_being_rebased (const struct worktree *wt,const char *target);int is_worktree_being_bisected (const struct worktree *wt,const char *target);
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees
worktree.c int submodule_uses_worktrees(const char *path) · line 401
Quoted 12 lines
worktree.c +12
-0+int other_head_refs (each_ref_fn fn,void *cb_data) +{ +struct worktree **worktrees, **p; +int ret =0 ; + + worktrees =get_worktrees (0 ); +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; +struct ref_store *refs; + +if (wt->is_current) +continue ;
As said in an earlier patch (and now this pattern coming up twice in this patch series alone), the lines of this function up to here, could become part of a worktree iterator function?
worktree.c int submodule_uses_worktrees(const char *path) · line 406
Quoted 4 lines
worktree.c +4
-0+ refs =get_worktree_ref_store (wt); + ret =refs_head_ref (refs, fn, cb_data); +if (ret) +break ;
with these 4 lines in the callback function.
worktree.h extern void free_worktrees(struct worktree **); · line 79
Quoted 5 lines
worktree.h +5
-0+/* +* Similar to head_ref() for all HEADs _except_ one from the current +* worktree, which is covered by head_ref(). +*/ +int other_head_refs (each_ref_fn fn,void *cb_data);
This is already such an iterator function, just at another abstraction level.
Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees
Quoted 9 lines · Stefan Beller
Quoted 9 lines
worktree.c +9
-0+int other_head_refs (each_ref_fn fn,void *cb_data) +{ +struct worktree **worktrees, **p; +int ret =0 ; + + worktrees =get_worktrees (0 ); +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; +struct ref_store *refs;
Quoted 8 lines
worktree.c int submodule_uses_worktrees(const char *path) · line 401
Quoted 3 lines
worktree.c +3
-0+ +if (wt->is_current) +continue ;As said in an earlier patch (and now this pattern coming up twice in this patch series alone), the lines of this function up to here, could become part of a worktree iterator function?
There are a couple "loop through all worktrees" code so far, but the filter condition is not always this.
While I like the idea of iterator function (especially if it does "struct worktree" memory management internally), I think it's a bit overkill to go for_each_worktree() with a callback function when the equivalent for(;;) is so short. We would need to declare struct to pass callback data, and the reader may have to got read for_each_worktree() code then come back here.
So, probably no worktree iterator (yet).
Quoted 7 lines
worktree.c int submodule_uses_worktrees(const char *path) · line 406
Quoted 4 lines
worktree.c +4
-0+ refs =get_worktree_ref_store (wt); + ret =refs_head_ref (refs, fn, cb_data); +if (ret) +break ;with these 4 lines in the callback function.
Quoted 8 lines
worktree.h extern void free_worktrees(struct worktree **); · line 79
Quoted 5 lines
worktree.h +5
-0+/* +* Similar to head_ref() for all HEADs _except_ one from the current +* worktree, which is covered by head_ref(). +*/ +int other_head_refs (each_ref_fn fn,void *cb_data);This is already such an iterator function, just at another abstraction level.
yeah.. but we can't mix and match (or combine) ref/worktree iterator callbacks
Signature · 1 line
Duy
Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees
Quoted 2 lines · Duy Nguyen
So, probably no worktree iterator (yet).
Ok, thanks for considering it.
Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees
In reply to Nguyễn Thái Ngọc DuyQuoted 10 lines · Nguyễn Thái Ngọc Duy
[...]
revision.c +1
-0@@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char *submodule,int argcount;if (submodule) { +/*
revision.c static int handle_revision_pseudo_opt(const char *submodule, · line 2146
Quoted 10 lines
revision.c +7
-0+* We need some something like get_submodule_worktrees() +* before we can go through all worktrees of a submodule, +* .e.g with adding all HEADs from --all, which is not +* supported right now, so stick to single worktree. +*/ +if (!revs->single_worktree) +die ("BUG: --single-worktree cannot be used together with submodule" ); refs =get_submodule_ref_store (submodule); }else refs =get_main_ref_store ();
Tiny nit: s/.e.g/e.g./
Quoted 1 line
[...]
Michael
PATCH v4 12/16 files-backend: make reflog iterator go through per-worktree reflog
In reply to Nguyễn Thái Ngọc Duyrefs/bisect is unfortunately per-worktree, so we need to look in per-worktree logs/refs/bisect in addition to per-repo logs/refs. The current iterator only goes through per-repo logs/refs.
Use merge iterator to walk two ref stores at the same time and pick per-worktree refs from the right iterator.
PS. Note the unsorted order of for_each_reflog in the test. This is supposed to be OK, for now. If we enforce order on for_each_reflog() then some more work will be required.
--- refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++---------- t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 14 deletions(-)
refs/files-backend.c +45 -14
@@ -106,15 +106,6 @@ static void files_reflog_path(struct files_ref_store *refs,struct strbuf *sb, const char *refname) { -if (!refname) { -/* -* FIXME: of course this is wrong in multi worktree -* setting. To be fixed real soon. -*/ -strbuf_addf (sb,"%s/logs" , refs->gitcommondir); -return ; - } - switch (ref_type(refname)) { case REF_TYPE_PER_WORKTREE: case REF_TYPE_PSEUDOREF:
@@ -2055,23 +2046,63 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {files_reflog_iterator_abort }; -static struct ref_iterator *files_reflog_iterator_begin (struct ref_store *ref_store) +static struct ref_iterator *reflog_iterator_begin (struct ref_store *ref_store, +const char *gitdir) { -struct files_ref_store *refs = -files_downcast (ref_store,REF_STORE_READ , -"reflog_iterator_begin" );struct files_reflog_iterator *iter =xcalloc (1 ,sizeof (*iter));struct ref_iterator *ref_iterator = &iter->base;struct strbuf sb =STRBUF_INIT ;base_ref_iterator_init (ref_iterator, &files_reflog_iterator_vtable); -files_reflog_path (refs, &sb,NULL ); +strbuf_addf (&sb,"%s/logs" , gitdir); iter->dir_iterator =dir_iterator_begin (sb.buf); iter->ref_store = ref_store;strbuf_release (&sb); +return ref_iterator; } +static enum iterator_selection reflog_iterator_select ( +struct ref_iterator *iter_worktree, +struct ref_iterator *iter_common, +void *cb_data) +{ +if (iter_worktree) { +/* +* We're a bit loose here. We probably should ignore +* common refs if they are accidentally added as +* per-worktree refs. +*/ +return ITER_SELECT_0 ; + }else if (iter_common) { +if (ref_type (iter_common->refname) ==REF_TYPE_NORMAL ) +return ITER_SELECT_1 ; + +/* +* The main ref store may contain main worktree's +* per-worktree refs, which should be ignored +*/ +return ITER_SKIP_1 ; + }else +return ITER_DONE ; +} + +static struct ref_iterator *files_reflog_iterator_begin (struct ref_store *ref_store) +{ +struct files_ref_store *refs = +files_downcast (ref_store,REF_STORE_READ , +"reflog_iterator_begin" ); + +if (!strcmp (refs->gitdir, refs->gitcommondir)) { +return reflog_iterator_begin (ref_store, refs->gitcommondir); + }else { +return merge_ref_iterator_begin ( +reflog_iterator_begin (ref_store, refs->gitdir), +reflog_iterator_begin (ref_store, refs->gitcommondir), + reflog_iterator_select, refs); + } +} +/* * If update is a direct update of head_ref (the reference pointed to * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
t/t1407-worktree-ref-store.sh +30 -0
@@ -49,4 +49,34 @@ test_expect_success 'create_symref(FOO, refs/heads/master)' ' test_cmp expected actual ' +test_expect_success 'for_each_reflog()' ' + echo $_z40 > .git/logs/PSEUDO-MAIN && + mkdir -p .git/logs/refs/bisect && + echo $_z40 > .git/logs/refs/bisect/random && + + echo $_z40 > .git/worktrees/wt/logs/PSEUDO-WT && + mkdir -p .git/worktrees/wt/logs/refs/bisect && + echo $_z40 > .git/worktrees/wt/logs/refs/bisect/wt-random && + + $RWT for-each-reflog | cut -c 42- | sort >actual && + cat >expected <<-\EOF && + HEAD 0x1 + PSEUDO-WT 0x0 + refs/bisect/wt-random 0x0 + refs/heads/master 0x0 + refs/heads/wt-master 0x0 + EOF + test_cmp expected actual && + + $RMAIN for-each-reflog | cut -c 42- | sort >actual && + cat >expected <<-\EOF && + HEAD 0x1 + PSEUDO-MAIN 0x0 + refs/bisect/random 0x0 + refs/heads/master 0x0 + refs/heads/wt-master 0x0 + EOF + test_cmp expected actual +' + test_done
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog
Quoted 22 lines · Nguyễn Thái Ngọc Duy
refs/bisect is unfortunately per-worktree, so we need to look in per-worktree logs/refs/bisect in addition to per-repo logs/refs. The current iterator only goes through per-repo logs/refs.
Use merge iterator to walk two ref stores at the same time and pick per-worktree refs from the right iterator.
PS. Note the unsorted order of
for_each_reflogin the test. This is supposed to be OK, for now. If we enforce order onfor_each_reflog()then some more work will be required.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++---------- t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 14 deletions(-)refs/files-backend.c +0
-0@@ -2055,23 +2046,63 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {
<snip>
Quoted 9 lines
refs/files-backend.c +9
-0+static struct ref_iterator *files_reflog_iterator_begin (struct ref_store *ref_store) +{ +struct files_ref_store *refs = +files_downcast (ref_store,REF_STORE_READ , +"reflog_iterator_begin" ); + +if (!strcmp (refs->gitdir, refs->gitcommondir)) { +return reflog_iterator_begin (ref_store, refs->gitcommondir); + }else {
refs/files-backend.c static struct ref_iterator_vtable files_reflog_iterator_vtable = { · line 2108
Quoted 10 lines
refs/files-backend.c +7
-0+return merge_ref_iterator_begin ( +reflog_iterator_begin (ref_store, refs->gitdir), +reflog_iterator_begin (ref_store, refs->gitcommondir), + reflog_iterator_select, refs); + } +} +/* * If update is a direct update of head_ref (the reference pointed to * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
Whilst trying to use ref backends to implement ref namespaces one of the issues I had was that namespaced bisect refs weren't being found.
This turned out to be for the same reason their reflogs weren't found, that by only iterating through the commondir refs it was missing the gitdir ones and that it only worked normally because of a special case in loose_fill_ref_dir
/*
* Manually add refs/bisect, which, being per-worktree, might
* not appear in the directory listing for refs/ in the main
* repo.
*/
if (!strcmp(dirname, "refs/")) {
int pos = search_ref_dir(dir, "refs/bisect/", 12);
if (pos < 0) {
struct ref_entry *child_entry = create_dir_entry(
dir->cache, "refs/bisect/", 12, 1);
add_entry_to_dir(dir, child_entry);
}
}If files_ref_iterator_begin was made to use a merged or overlay ref iterator too then this special case could be removed and bisecting in a namespaced workspace would work.
I've yet to work out whether namespaced bisect refs makes sense, but that's a problem for the next time I have time to work on it :).
Re: [PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog
In reply to Nguyễn Thái Ngọc DuyQuoted 24 lines · Nguyễn Thái Ngọc Duy
refs/bisect is unfortunately per-worktree, so we need to look in per-worktree logs/refs/bisect in addition to per-repo logs/refs. The current iterator only goes through per-repo logs/refs.
Use merge iterator to walk two ref stores at the same time and pick per-worktree refs from the right iterator.
PS. Note the unsorted order of
for_each_reflogin the test. This is supposed to be OK, for now. If we enforce order onfor_each_reflog()then some more work will be required.Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++---------- t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++ 2 files changed, 75 insertions(+), 14 deletions(-)refs/files-backend.c +2
-0+static enum iterator_selection reflog_iterator_select( + struct ref_iterator *iter_worktree,
refs/files-backend.c static struct ref_iterator_vtable files_reflog_iterator_vtable = { · line 2076
Quoted 10 lines
refs/files-backend.c +10
-0+ struct ref_iterator *iter_common, + void *cb_data) +{ + if (iter_worktree) { +/* +* We're a bit loose here. We probably should ignore +* common refs if they are accidentally added as +* per-worktree refs. +*/ +return ITER_SELECT_0 ;
I don't understand the point of the comment. If we should ignore common refs here, why not do it rather than commenting about it? Wouldn't it be really easy to implement? OTOH if it's not needed, then why the comment?
Quoted 1 line
[...]
Michael
PATCH v4 13/16 revision.c: --reflog add HEAD reflog from all worktrees
In reply to Nguyễn Thái Ngọc DuyNote that add_other_reflogs_to_pending() is a bit inefficient, since it scans reflog for all refs of each worktree, including shared refs, so the shared ref's reflog is scanned over and over again.
We could update refs API to pass "per-worktree only" flag to avoid that. But long term we should be able to obtain a "per-worktree only" ref store and would need to revert the changes in reflog iteration API. So let's just wait until then.
add_reflogs_to_pending() is called by reachable.c so by default "git prune" will examine reflog from all worktrees.
--- revision.c | 28 +++++++++++++++++++++++++++- t/t5304-prune.sh | 16 ++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-)
revision.c +27 -1
@@ -1132,6 +1132,7 @@ struct all_refs_cb {int warned_bad_reflog;struct rev_info *all_revs;const char *name_for_errormsg; +struct ref_store *refs; };int ref_excluded (struct string_list *ref_excludes,const char *path)
@@ -1168,6 +1169,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs, cb->all_revs = revs; cb->all_flags = flags; revs->rev_input_given =1 ; + cb->refs =NULL ; }void clear_ref_exclusion (struct string_list **ref_excludes_p)
@@ -1236,17 +1238,41 @@ static int handle_one_reflog(const char *path, const struct object_id *oid,struct all_refs_cb *cb = cb_data; cb->warned_bad_reflog =0 ; cb->name_for_errormsg = path; -for_each_reflog_ent (path, handle_one_reflog_ent, cb_data); +refs_for_each_reflog_ent (cb->refs, path, + handle_one_reflog_ent, cb_data);return 0 ; } +static void add_other_reflogs_to_pending (struct all_refs_cb *cb) +{ +struct worktree **worktrees, **p; + + worktrees =get_worktrees (0 ); +for (p = worktrees; *p; p++) { +struct worktree *wt = *p; + +if (wt->is_current) +continue ; + + cb->refs =get_worktree_ref_store (wt); +refs_for_each_reflog (cb->refs, + handle_one_reflog, + cb); + } +free_worktrees (worktrees); +} +void add_reflogs_to_pending (struct rev_info *revs,unsigned flags) {struct all_refs_cb cb; cb.all_revs = revs; cb.all_flags = flags; + cb.refs =get_main_ref_store ();for_each_reflog (handle_one_reflog, &cb); + +if (!revs->single_worktree) +add_other_reflogs_to_pending (&cb); } static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
t/t5304-prune.sh +16 -0
@@ -304,4 +304,20 @@ test_expect_success 'prune: handle HEAD in multiple worktrees' ' test_cmp third-worktree/blob actual ' +test_expect_success 'prune: handle HEAD reflog in multiple worktrees' ' + git config core.logAllRefUpdates true && + echo "lost blob for third-worktree" >expected && + ( + cd third-worktree && + cat ../expected >blob && + git add blob && + git commit -m "second commit in third" && + git reset --hard HEAD^ + ) && + git prune --expire=now && + SHA1=`git hash-object expected` && + git -C third-worktree show "$SHA1" >actual && + test_cmp expected actual +' + test_done
Signature · 1 line
2.11.0.157.gd943d85
PATCH v4 14/16 rev-list: expose and document --single-worktree
In reply to Nguyễn Thái Ngọc Duy--- Documentation/rev-list-options.txt | 8 ++++++++ revision.c | 2 ++ 2 files changed, 10 insertions(+)
Documentation/rev-list-options.txt +8 -0
@@ -184,6 +184,14 @@ explicitly. Pretend as if all objects mentioned by reflogs are listed on the command line as `<commit>`. +--single-worktree:: + By default, all working trees will be examined by the + following options when there are more than one (see + linkgit:git-worktree[1]): `--all`, `--reflog` and + `--indexed-objects`. + This option forces them to examine the current working tree + only. + --ignore-missing:: Upon seeing an invalid object name in the input, pretend as if the bad input was not given.
revision.c +2 -0
@@ -2251,6 +2251,8 @@ static int handle_revision_pseudo_opt(const char *submodule,return error ("invalid argument to --no-walk" ); }else if (!strcmp (arg,"--do-walk" )) { revs->no_walk =0 ; + }else if (!strcmp (arg,"--single-worktree" )) { + revs->single_worktree =1 ; }else {return 0 ; }
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 14/16] rev-list: expose and document --single-worktree
Quoted 30 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- Documentation/rev-list-options.txt | 8 ++++++++ revision.c | 2 ++ 2 files changed, 10 insertions(+)Documentation/rev-list-options.txt +8
-0@@ -184,6 +184,14 @@ explicitly. Pretend as if all objects mentioned by reflogs are listed on the command line as `<commit>`. +--single-worktree:: + By default, all working trees will be examined by the + following options when there are more than one (see + linkgit:git-worktree[1]): `--all`, `--reflog` and + `--indexed-objects`. + This option forces them to examine the current working tree + only. + --ignore-missing:: Upon seeing an invalid object name in the input, pretend as if the bad input was not given.revision.c +0
-0
revision.c static int handle_revision_pseudo_opt(const char *submodule, · line 2255
Quoted 5 lines
revision.c +2
-0@@ -2251,6 +2251,8 @@ static int handle_revision_pseudo_opt(const char *submodule,return error ("invalid argument to --no-walk" ); }else if (!strcmp (arg,"--do-walk" )) { revs->no_walk =0 ; + }else if (!strcmp (arg,"--single-worktree" )) { + revs->single_worktree =1 ;
This is in handle_revision_pseudo_opt, that has the note
/*
* NOTE!
*
* Commands like "git shortlog" will not accept the options below
* unless parse_revision_opt queues them (as opposed to erroring
* out).
*
* When implementing your new pseudo-option, remember to
* register it in the list at the top of handle_revision_opt. */
The registration needs to be done at around line 1700.
But come to think of it, is it really a pseudo opt? Could it be a "real" (non pseudo) opt in handle_revision_opt? The reasoning (either way) would be of interest in the commit message, IMHO.
PATCH v4 15/16 refs.c: remove fallback-to-main-store code get_submodule_ref_store()
In reply to Nguyễn Thái Ngọc DuyAt this state, there are three get_submodule_ref_store() callers:
- for_each_remote_ref_submodule()
- handle_revision_pseudo_opt()
- resolve_gitlink_ref()
The first two deal explicitly with submodules (and we should never fall back to the main ref store as a result). They are only called from submodule.c:
- find_first_merges()
- submodule_needs_pushing()
- push_submodule()
The last one, as its name implies, deals only with submodules too, and the "submodule" (path) argument must be a non-NULL, non-empty string.
So, this "if NULL or empty string" code block should never ever trigger. And it's wrong to fall back to the main ref store anyway. Delete it.
--- refs.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
refs.c +3 -8
@@ -1587,6 +1587,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)char *to_free =NULL ;size_t len; +if (!submodule) +return NULL ; +if (submodule) { len =strlen (submodule);while (len &&is_dir_sep (submodule[len -1 ]))
@@ -1595,14 +1598,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)return NULL ; } -if (!submodule || !*submodule) { -/* -* FIXME: This case is ideally not allowed. But that -* can't happen until we clean up all the callers. -*/ -return get_main_ref_store (); - } -if (submodule[len])/* We need to strip off one or more trailing slashes */ submodule = to_free =xmemdupz (submodule, len);
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 15/16] refs.c: remove fallback-to-main-store code get_submodule_ref_store()
Quoted 14 lines · Nguyễn Thái Ngọc Duy
At this state, there are three
get_submodule_ref_store()callers:-
for_each_remote_ref_submodule()
-handle_revision_pseudo_opt()
-resolve_gitlink_ref()The first two deal explicitly with submodules (and we should never fall back to the main ref store as a result). They are only called from
submodule.c:-
find_first_merges()
-submodule_needs_pushing()
-push_submodule()
Quoted 6 lines
The last one, as its name implies, deals only with submodules too, and the "submodule" (path) argument must be a non-NULL, non-empty string.
So, this "if NULL or empty string" code block should never ever trigger. And it's wrong to fall back to the main ref store anyway. Delete it.
Nice! Thanks for the cleanup.
Michael
PATCH v4 16/16 refs.c: reindent get_submodule_ref_store()
In reply to Nguyễn Thái Ngọc DuyWith the new "if (!submodule) return NULL;" code added in the previous commit, we don't need to check if submodule is not NULL anymore.
--- refs.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
refs.c +5 -7
@@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char *submodule)if (!submodule)return NULL ; -if (submodule) { - len =strlen (submodule); -while (len &&is_dir_sep (submodule[len -1 ])) - len--; -if (!len) -return NULL ; - } + len =strlen (submodule); +while (len &&is_dir_sep (submodule[len -1 ])) + len--; +if (!len) +return NULL ;if (submodule[len])/* We need to strip off one or more trailing slashes */
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v4 16/16] refs.c: reindent get_submodule_ref_store()
Quoted 22 lines · Nguyễn Thái Ngọc Duy
With the new "if (!submodule) return NULL;" code added in the previous commit, we don't need to check if submodule is not NULL anymore.
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)refs.c +0
-5@@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char *submodule)if (!submodule)return NULL ; -if (submodule) { - len =strlen (submodule); -while (len &&is_dir_sep (submodule[len -1 ])) - len--; -if (!len)
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1600
Quoted 10 lines
refs.c +5
-2-return NULL ; - } + len =strlen (submodule); +while (len &&is_dir_sep (submodule[len -1 ])) + len--; +if (!len) +return NULL ;if (submodule[len])/* We need to strip off one or more trailing slashes */
Now I'm confused. Is it still allowed to call get_submodule_ref_store(NULL)? If not, then the previous commit should probably do
if (!submdule) BUG(...);
Either way, it seems like it would be natural to combine this commit with the previous one.
Michael
Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.08.2017 14:36:
Quoted 22 lines
"git gc" when used in multiple worktrees ignore some per-worktree
references: object references in the index, HEAD and reflog. This series fixes it by making the revision walker include these from all worktrees by default (and the series is basically split in three parts in the same order). There's a couple more cleanups inrefs.c. Luckily it does not conflict with anything in 'pu'.Compared to v3 [1], the largest change is supporting multi worktree in the reflog iterator. The merge iterator is now used (Micheal was right all along).
[1] https://public-inbox.org/git/[email protected]/
Nguyễn Thái Ngọc Duy (16):
revision.h: new flag in structrev_infowrt. worktree-related refsrefs.c: useis_dir_sep()inresolve_gitlink_ref()revision.c: refactoradd_index_objects_to_pending()revision.c: --indexed-objects add objects from all worktreesrefs.c: refactorget_submodule_ref_store(), share common free block
refs: move submodule slash stripping code toget_submodule_ref_store
refs: addrefs_head_ref()
revision.c: use refs_for_each*() instead of for_each_*_submodule()
Quoted 8 lines
refs.c: movefor_each_remote_ref_submodule()tosubmodule.c
refs: remove dead for_each_*_submodule()
revision.c: --all adds HEAD from all worktrees
files-backend: make reflog iterator go through per-worktree reflogrevision.c: --reflog add HEAD reflog from all worktrees
rev-list: expose and document --single-worktree
refs.c: remove fallback-to-main-store codeget_submodule_ref_store()refs.c: reindentget_submodule_ref_store()
I probably won't be able to review this (many commits without commit message), but I'm happy to see progress on the "--all" and prune front (and will test). I suggest we think about the UI exposure a bit when it comes to including all heads or naming options, though:
* HEAD is "the current head"
* refs/heads is where all local branch heads are
* --branches is the rev-list/log option for refs/heads/*
* --all is the rev-list/log option for refs/* plus HEAD
* HEAD is the rev-list/log argument for HEAD
* --heads is the show-ref option limiting to refs/heads/*
* --head is the show-ref option which adds HEAD
* refs/heads is the for-each-ref-pattern for refs/heads/*
* HEAD is not the for-each-ref-pattern for HEAD [I'll suggest a patch to change the latter, shortly.]
I would hope that the result of this series and other efforts will be:
* consistent way to specify "all local branch heads"
* consistent way to specify "the head" aka HEAD
* consistent way to specify "all linked worktree heads" [* maybe something for submodules...]
This may require changing the misnamed show-ref option, but also thinking twice before changing the meaning of "--all" for the rev-list/log family: it's easy to say "--all --linked" or "--all --heads" to get everything plus all linked worktree heads when "--all" == "--branches --tags HEAD", but it's more cumbersome with a changed --all that is "really everything". I gues my suggestion would be:
--all as it is now (refs/* plus HEAD)
--head alternative way to say HEAD (as it is now for show-ref)
--heads HEAD for all linked worktrees (incompatible change for show-ref)
And all of them should work the same for the rev-list/log family as well as for-each-ref/show-ref.
I thinking that changing show-ref (porcelain, not quite as commonly used) should do the least harm compared to all other options.
Michael
Quoted 9 lines · Michael J Gruber
I suggest we think about the UI exposure a bit when it comes to including all heads or naming options, though:
* HEAD is "the current head"
* refs/heads is where all local branch heads are* --branches is the rev-list/log option for refs/heads/*
* --all is the rev-list/log option for refs/* plus HEAD
* HEAD is the rev-list/log argument for HEAD
It also covers object references from the index file aka --indexed-objects
Quoted 7 lines
* --heads is the show-ref option limiting to refs/heads/*
* --head is the show-ref option which adds HEAD* refs/heads is the for-each-ref-pattern for refs/heads/*
* HEAD is not the for-each-ref-pattern for HEAD [I'll suggest a patch to change the latter, shortly.]
Quoted 6 lines
I would hope that the result of this series and other efforts will be:
* consistent way to specify "all local branch heads"
* consistent way to specify "the head" aka HEAD
* consistent way to specify "all linked worktree heads" [* maybe something for submodules...]
Hmm.. I admit that I completely overlooked 'git show-ref'.
Quoted 7 lines
This may require changing the misnamed show-ref option, but also thinking twice before changing the meaning of "--all" for the rev-list/log family: it's easy to say "--all --linked" or "--all --heads" to get everything plus all linked worktree heads when "--all" == "--branches --tags HEAD", but it's more cumbersome with a changed --all that is "really everything". I gues my suggestion would be:
Quoted 8 lines
--all as it is now (refs/* plus HEAD)
--head alternative way to say HEAD (as it is now for show-ref)
--heads HEAD for all linked worktrees (incompatible change for show-ref)
And all of them should work the same for the rev-list/log family as well as for-each-ref/show-ref.
How about: show-ref learns a new option to let it list HEAD (and other per-worktree refs) of one/current worktree, or all worktrees. This is what the --single-worktree option is for, which is added by this series (but I need to make sure if's exposed in show-ref as well). For showing refs as viewed by another worktree, we could have the global option similar to --git-dir to select that worktree, e.g. "git --work-tree-id=XXX show-ref ..."?
Since this seems a good thing to do, but not necessary to fix the "prune" bug, I'll do it separately instead of adding in this series. I may need to look at "git for-each-ref" too for that matter.
Quoted 2 lines
I thinking that changing show-ref (porcelain, not quite as commonly used) should do the least harm compared to all other options.
Signature · 1 line
Duy
Quoted 10 lines · Nguyễn Thái Ngọc Duy
"git gc" when used in multiple worktrees ignore some per-worktree
references: object references in the index, HEAD and reflog. This series fixes it by making the revision walker include these from all worktrees by default (and the series is basically split in three parts in the same order). There's a couple more cleanups inrefs.c. Luckily it does not conflict with anything in 'pu'.Compared to v3 [1], the largest change is supporting multi worktree in the reflog iterator. The merge iterator is now used (Micheal was right all along).
I read over all of the refs-related changes in this patch series. Aside from the comments that I left, they look good to me. Thanks!
Michael