Fix git-gc losing objects in multi worktree
v3 cover Fix git-gc losing objects in multi worktree
Changes since v2 [1] is relatively small. It still needs nd/worktree-kill-parse-ref of course.
[1] http://public-inbox.org/git/[email protected]/
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 -5
@@ -1208,11 +1208,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) -{ -return refs_head_ref (get_submodule_ref_store (submodule), fn, cb_data); -} -int head_ref (each_ref_fn fn,void *cb_data) {return refs_head_ref (get_main_ref_store (), fn, cb_data);
revision.c +1 -1
@@ -1336,7 +1336,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags) worktrees =get_worktrees (0 );for (p = worktrees; *p; p++) {struct worktree *wt = *p; -struct index_state istate = {0 }; +struct index_state istate = {NULL };if (wt->is_current)continue ;/* current index already taken care of */
Nguyễn Thái Ngọc Duy (12):
revision.h: new flag in struct rev_info wrt. worktree-related refs 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: 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
Documentation/rev-list-options.txt | 8 ++ Documentation/technical/api-ref-iteration.txt | 7 +- reachable.c | 1 + refs.c | 105 ++++++++++----------- refs.h | 12 +-- refs/files-backend.c | 46 ++++++--- revision.c | 130 +++++++++++++++++++++----- revision.h | 1 + submodule.c | 2 + t/t1407-worktree-ref-store.sh | 30 ++++++ t/t5304-prune.sh | 37 ++++++++ 11 files changed, 274 insertions(+), 105 deletions(-)
Signature · 1 line
2.11.0.157.gd943d85
PATCH v3 1/12 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
@@ -88,6 +88,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 v3 2/12 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
@@ -1263,13 +1263,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))
@@ -1282,13 +1282,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 v3 3/12 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;
@@ -1291,8 +1292,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
PATCH v3 4/12 refs.c: refactor get_submodule_ref_store(), share common free block
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
refs.c +5 -7
@@ -1570,19 +1570,16 @@ 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 ; + goto done; ret =submodule_to_gitdir (&submodule_sb, submodule); -if (ret) { -strbuf_release (&submodule_sb); -return NULL ; - } +if (ret) + goto done;/* assume that add_submodule_odb() has been called */ refs = ref_store_init(submodule_sb.buf,
@@ -1590,6 +1587,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 v3 04/12] refs.c: refactor get_submodule_ref_store(), share common free block
Quoted 20 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)refs.c +1
-2@@ -1570,19 +1570,16 @@ 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)
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1582
Quoted 10 lines
refs.c +3
-5-return NULL ; + goto done; ret =submodule_to_gitdir (&submodule_sb, submodule); -if (ret) { -strbuf_release (&submodule_sb); -return NULL ; - } +if (ret) + goto done;
After this change, the temporary variable ret could be eliminated.
Quoted 1 line
[...]
Michael
PATCH v3 5/12 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
@@ -1422,25 +1422,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 && 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 ;
@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; +char *to_free =NULL ;int ret; +size_t len; + +if (submodule) { + len =strlen (submodule); +while (len && submodule[len -1 ] =='/' ) + len--; +if (!len) + submodule =NULL ; + }if (!submodule || !*submodule) {/*
@@ -1568,6 +1563,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;
@@ -1589,6 +1588,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 v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store
Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1552
Quoted 11 lines
refs.c +6
-0@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; +char *to_free =NULL ;int ret; +size_t len; + +if (submodule) { + len =strlen (submodule); +while (len && submodule[len -1 ] =='/' )
What is the source of the value of 'submodule'? Is it an index entry? Or did it pass through parse_pathspec? In these cases it is correct to compare against literal '/'. Otherwise, is_dir_sep() is preferred.
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1559
Quoted 7 lines
refs.c +4
-0+ len--; +if (!len) + submodule =NULL ; + }if (!submodule || !*submodule) {/*
-- Hannes
Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store
Quoted 16 lines · Johannes Sixt
Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1552
Quoted 11 lines
refs.c +6
-0@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; +char *to_free =NULL ;int ret; +size_t len; + +if (submodule) { + len =strlen (submodule); +while (len && submodule[len -1 ] =='/' )What is the source of the value of 'submodule'? Is it an index entry? Or did it pass through
parse_pathspec? In these cases it is correct to compare against literal '/'. Otherwise,is_dir_sep()is preferred.
This is a code move from resolve_gitlink_ref(), which goes back to 0ebde32c87 (Add 'resolve_gitlink_ref()' helper function - 2007-04-09) and it looks like a dir separator back then.
Can I convert that in a separate topic? I think Michael even wanted to kill all these path manipulation in refs code, which makes sense, but I would need to audit the callers carefully before making that move. --
Duy
Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store
In reply to Johannes SixtQuoted 11 lines · Johannes Sixt
Am 19.04.2017 um 13:01 schrieb Nguyễn Thái Ngọc Duy:
Quoted 10 lines
refs.c +3
-0@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; +char *to_free =NULL ;int ret; +size_t len; +
Quoted 8 lines
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1552
Quoted 3 lines
refs.c +3
-0+if (submodule) { + len =strlen (submodule); +while (len && submodule[len -1 ] =='/' )What is the source of the value of 'submodule'? Is it an index entry? Or did it pass through
parse_pathspec? In these cases it is correct to compare against literal '/'. Otherwise,is_dir_sep()is preferred.
I've looked at the callers. Yes it is a path and is_dir_sep() should be used. Since this has been like this in the current code, unless there's some more changes in this series or you insist, I would hold this change back, wait for this series to settle and submit it later (I'll have to do that anyway to kill the get_main_store() call in this function).
Signature · 1 line
Duy
Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store
Am 20.04.2017 um 13:56 schrieb Duy Nguyen:
Quoted 9 lines · Johannes Sixt
Quoted 3 lines
What is the source of the value of 'submodule'? Is it an index entry? Or did it pass through
parse_pathspec? In these cases it is correct to compare against literal '/'. Otherwise,is_dir_sep()is preferred.I've looked at the callers. Yes it is a path and
is_dir_sep()should be used. Since this has been like this in the current code, unless there's some more changes in this series or you insist, I would hold this change back,
I won't insist because we have it the old way since a decade without issue reports.
-- Hannes
Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store
In reply to Nguyễn Thái Ngọc DuyQuoted 44 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()Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)refs.c +2
-16@@ -1422,25 +1422,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 && 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 ;@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; +char *to_free =NULL ;
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1556
Quoted 10 lines
refs.c +9
-0int ret; +size_t len; + +if (submodule) { + len =strlen (submodule); +while (len && submodule[len -1 ] =='/' ) + len--; +if (!len) + submodule =NULL ; + }
Ugh. Should a submodule named "///" *really* be considered to refer to the main ref_store? I understand that's what the code did before this patch, but it seems to me more like an accident of the old design rather than something worth supporting. In other words, if a caller would really pass us such a string, it seems like we could declare the caller buggy, no?
Quoted 1 line
[...]
Otherwise, looks good and makes a lot of sense.
Michael
Re: [PATCH v3 05/12] refs: move submodule slash stripping code to get_submodule_ref_store
Quoted 54 lines · Michael Haggerty
Quoted 53 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()Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)refs.c +10
-16@@ -1422,25 +1422,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 && 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 ;@@ -1558,7 +1543,17 @@ struct ref_store *get_submodule_ref_store(const char *submodule) {struct strbuf submodule_sb =STRBUF_INIT ;struct ref_store *refs; +char *to_free =NULL ;int ret; +size_t len; + +if (submodule) { + len =strlen (submodule); +while (len && submodule[len -1 ] =='/' ) + len--; +if (!len) + submodule =NULL ;
Quoted 8 lines
refs.c struct ref_store *get_submodule_ref_store(const char *submodule) · line 1556
Quoted 1 line
refs.c +1
-0+ }Ugh. Should a submodule named "///" *really* be considered to refer to the main
ref_store? I understand that's what the code did before this patch, but it seems to me more like an accident of the old design rather than something worth supporting. In other words, if a caller would really pass us such a string, it seems like we could declare the caller buggy, no?
In a nearby thread we discussed whether we want to tighten the submodule names and paths as some of the code path do funny things on funny input. As 'submodule' here refers to a path (and not a name), I would think '///' is not possible/buggy, rather we'd want to discuss if we want to extend the check to is_dir_sep, which would include '\' on Windows.
Thanks,
Stefan
PATCH v3 6/12 refs: add refs_head_ref()
In reply to Nguyễn Thái Ngọc Duy--- refs.c | 19 +++++++++---------- refs.h | 2 ++ 2 files changed, 11 insertions(+), 10 deletions(-)
refs.c +9 -10
@@ -1208,27 +1208,26 @@ 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) +{ +return refs_head_ref (get_submodule_ref_store (submodule), 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); }/*
refs.h +2 -0
@@ -233,6 +233,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 v3 06/12] refs: add refs_head_ref()
Quoted 53 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 19 +++++++++---------- refs.h | 2 ++ 2 files changed, 11 insertions(+), 10 deletions(-)refs.c +9
-10@@ -1208,27 +1208,26 @@ 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) +{ +return refs_head_ref (get_submodule_ref_store (submodule), 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); }/* refs.h +0
-0
refs.h typedef int each_ref_fn(const char *refname, · line 240
Quoted 8 lines
refs.h +2
-0@@ -233,6 +233,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,
I'm seeing segfaults in t3600 after this patch, apparently because refs==NULL gets passed from head_ref_submodule() to refs_head_ref().
Michael
Re: [PATCH v3 06/12] refs: add refs_head_ref()
Quoted 57 lines · Michael Haggerty
Quoted 56 lines · Nguyễn Thái Ngọc Duy
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- refs.c | 19 +++++++++---------- refs.h | 2 ++ 2 files changed, 11 insertions(+), 10 deletions(-)refs.c +9
-10@@ -1208,27 +1208,26 @@ 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) +{ +return refs_head_ref (get_submodule_ref_store (submodule), 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); }/* refs.h +1
-0@@ -233,6 +233,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,
Quoted 8 lines
refs.h typedef int each_ref_fn(const char *refname, · line 240
Quoted 5 lines
refs.h +1
-0+ 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,I'm seeing segfaults in t3600 after this patch, apparently because
refs==NULLgets passed fromhead_ref_submodule()torefs_head_ref().
Eventually it's the caller's responsibility not to pass NULL ref store to refs_* functions. That happens in "revision.c: use refs_for_each*() instead of for_each_*_submodule()" patch, which is too late and would break bisect. Will fix.
Signature · 1 line
Duy
PATCH v3 7/12 revision.c: use refs_for_each*() instead of for_each_*_submodule()
In reply to Nguyễn Thái Ngọc Duy--- revision.c | 48 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 16 deletions(-)
revision.c +32 -16
@@ -1189,12 +1189,19 @@ void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)string_list_append (*ref_excludes_p, exclude); } -static void handle_refs (const char *submodule,struct rev_info *revs,unsigned flags, -int (*for_each)(const char *,each_ref_fn ,void *)) +static void handle_refs (struct ref_store *refs, +struct rev_info *revs,unsigned flags, +int (*for_each)(struct ref_store *,each_ref_fn ,void *)) {struct all_refs_cb cb; + +if (!refs) { +/* this could happen with uninitialized submodules */ +return ; + } +init_all_refs_cb (&cb, revs, flags); -for_each (submodule, handle_one_ref, &cb); +for_each (refs, handle_one_ref, &cb); }static void handle_one_reflog_commit (struct object_id *oid,void *cb_data)
@@ -2067,23 +2074,25 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx, ctx->argc -= n; } -static int for_each_bisect_ref (const char *submodule,each_ref_fn fn,void *cb_data,const char *term) { +static int for_each_bisect_ref (struct ref_store *refs,each_ref_fn fn, +void *cb_data,const char *term) +{struct strbuf bisect_refs =STRBUF_INIT ;int status;strbuf_addf (&bisect_refs,"refs/bisect/%s" , term); - status =for_each_ref_in_submodule (submodule, bisect_refs.buf, fn, cb_data); + status =refs_for_each_ref_in (refs, bisect_refs.buf, fn, cb_data);strbuf_release (&bisect_refs);return status; } -static int for_each_bad_bisect_ref (const char *submodule,each_ref_fn fn,void *cb_data) +static int for_each_bad_bisect_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data) { -return for_each_bisect_ref (submodule, fn, cb_data, term_bad); +return for_each_bisect_ref (refs, fn, cb_data, term_bad); } -static int for_each_good_bisect_ref (const char *submodule,each_ref_fn fn,void *cb_data) +static int for_each_good_bisect_ref (struct ref_store *refs,each_ref_fn fn,void *cb_data) { -return for_each_bisect_ref (submodule, fn, cb_data, term_good); +return for_each_bisect_ref (refs, fn, cb_data, term_good); } static int handle_revision_pseudo_opt(const char *submodule,
@@ -2092,8 +2101,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 (); +/* * NOTE! *
@@ -2105,22 +2120,23 @@ static int handle_revision_pseudo_opt(const char *submodule,* register it in the list at the top of handle_revision_opt. */ if (!strcmp (arg,"--all" )) { -handle_refs (submodule, revs, *flags, for_each_ref_submodule); -handle_refs (submodule, revs, *flags, head_ref_submodule); +handle_refs (refs, revs, *flags, refs_for_each_ref); +handle_refs (refs, revs, *flags, refs_head_ref);clear_ref_exclusion (&revs->ref_excludes); }else if (!strcmp (arg,"--branches" )) { -handle_refs (submodule, revs, *flags, for_each_branch_ref_submodule); +handle_refs (refs, revs, *flags, refs_for_each_branch_ref);clear_ref_exclusion (&revs->ref_excludes); }else if (!strcmp (arg,"--bisect" )) {read_bisect_terms (&term_bad, &term_good); -handle_refs (submodule, revs, *flags, for_each_bad_bisect_ref); -handle_refs (submodule, revs, *flags ^ (UNINTERESTING |BOTTOM ), for_each_good_bisect_ref); +handle_refs (refs, revs, *flags, for_each_bad_bisect_ref); +handle_refs (refs, revs, *flags ^ (UNINTERESTING |BOTTOM ), + for_each_good_bisect_ref); revs->bisect =1 ; }else if (!strcmp (arg,"--tags" )) { -handle_refs (submodule, revs, *flags, for_each_tag_ref_submodule); +handle_refs (refs, revs, *flags, refs_for_each_tag_ref);clear_ref_exclusion (&revs->ref_excludes); }else if (!strcmp (arg,"--remotes" )) { -handle_refs (submodule, revs, *flags, for_each_remote_ref_submodule); +handle_refs (refs, revs, *flags, refs_for_each_remote_ref);clear_ref_exclusion (&revs->ref_excludes); }else if ((argcount =parse_long_opt ("glob" , argv, &optarg))) {struct all_refs_cb cb;
Signature · 1 line
2.11.0.157.gd943d85
PATCH v3 8/12 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 (except for_each_remote_ref_submodule which is still used by submodule.c)
--- Documentation/technical/api-ref-iteration.txt | 7 ++----- refs.c | 29 --------------------------- refs.h | 9 --------- 3 files changed, 2 insertions(+), 43 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 -29
@@ -316,12 +316,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);
@@ -332,12 +326,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);
@@ -1220,11 +1208,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) -{ -return refs_head_ref (get_submodule_ref_store (submodule), fn, cb_data); -} -int head_ref (each_ref_fn fn,void *cb_data) {return refs_head_ref (get_main_ref_store (), fn, cb_data);
@@ -1263,11 +1246,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) {
@@ -1289,13 +1267,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 for_each_replace_ref (each_ref_fn fn,void *cb_data) { return do_for_each_ref(get_main_ref_store(),
refs.h +0 -9
@@ -259,15 +259,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 for_each_remote_ref_submodule (const char *submodule,each_ref_fn fn,void *cb_data);
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()
Quoted 16 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 (exceptfor_each_remote_ref_submodulewhich is still used bysubmodule.c)Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- Documentation/technical/api-ref-iteration.txt | 7 ++----- refs.c | 29 --------------------------- refs.h | 9 --------- 3 files changed, 2 insertions(+), 43 deletions(-)Documentation/technical/api-ref-iteration.txt +0
-0
Documentation/technical/api-ref-iteration.txt Iteration functions · line 36
Quoted 10 lines
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().
missing ` ? ------------------------------------^
ATB,
Ramsay Jones
Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()
Quoted 20 lines · Ramsay Jones
Quoted 19 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 (exceptfor_each_remote_ref_submodulewhich is still used bysubmodule.c)Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- Documentation/technical/api-ref-iteration.txt | 7 ++----- refs.c | 29 --------------------------- refs.h | 9 --------- 3 files changed, 2 insertions(+), 43 deletions(-)Documentation/technical/api-ref-iteration.txt +0
-0@@ -32,11 +32,8 @@ Iteration functions * `for_each_glob_ref_in()` the previous and `for_each_ref_in()` combined.
Quoted 9 lines
Documentation/technical/api-ref-iteration.txt Iteration functions · line 36
Quoted 7 lines
Documentation/technical/api-ref-iteration.txt +2
-5-* `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().missing ` ? ------------------------------------^
Indeed. Will tweak while queuing.
Re: [PATCH v3 08/12] refs: remove dead for_each_*_submodule()
In reply to Nguyễn Thái Ngọc DuyQuoted 3 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 (exceptfor_each_remote_ref_submodulewhich is still used bysubmodule.c)
❤❤❤❤❤❤
I love the way this is going.
Michael
PATCH v3 9/12 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 | 1 + refs.c | 22 ++++++++++++++++++++++ refs.h | 1 + revision.c | 13 +++++++++++++ submodule.c | 2 ++ t/t5304-prune.sh | 12 ++++++++++++ 6 files changed, 51 insertions(+)
reachable.c +1 -0
@@ -177,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)
refs.c +22 -0
@@ -1780,3 +1780,25 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg) {return refs_rename_ref (get_main_ref_store (), oldref, newref, logmsg); } + +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; +}
refs.h +1 -0
@@ -247,6 +247,7 @@ int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);int head_ref (each_ref_fn fn,void *cb_data); +int other_head_refs (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 for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
revision.c +13 -0
@@ -2105,6 +2105,13 @@ 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. +*/ +assert (revs->single_worktree !=0 ); refs =get_submodule_ref_store (submodule); }else refs =get_main_ref_store ();
@@ -2122,6 +2129,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
@@ -1225,6 +1225,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
Signature · 1 line
2.11.0.157.gd943d85
Re: [PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees
Quoted 59 lines · Nguyễn Thái Ngọc Duy
Unless
single_worktreeis set, --all now adds HEAD from all worktrees.Since
reachable.ccode does not usesetup_revisions(), we need to callother_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 requireint get_submodule_worktrees(const char *submodule, int flags);It could be a separate topic to reduce the scope of this one.
Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>--- reachable.c | 1 + refs.c | 22 ++++++++++++++++++++++ refs.h | 1 + revision.c | 13 +++++++++++++ submodule.c | 2 ++ t/t5304-prune.sh | 12 ++++++++++++ 6 files changed, 51 insertions(+)reachable.c +1
-0@@ -177,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)refs.c +12
-0@@ -1780,3 +1780,25 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg) {return refs_rename_ref (get_main_ref_store (), oldref, newref, logmsg); } + +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)
refs.c int rename_ref(const char *oldref, const char *newref, const char *logmsg) · line 1804
Quoted 10 lines
refs.c +10
-0+continue ; + + refs =get_worktree_ref_store (wt); + ret =refs_head_ref (refs, fn, cb_data); +if (ret) +break ; + } +free_worktrees (worktrees); +return ret; +}
This function is mainly about iterating through all worktrees. Therefore it feels out of place in the refs module. But I don't have a strong feeling about it.
Quoted 17 lines
refs.h +1
-0@@ -247,6 +247,7 @@ int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data);int head_ref (each_ref_fn fn,void *cb_data); +int other_head_refs (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 for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,revision.c +0
-0
revision.c static int handle_revision_pseudo_opt(const char *submodule, · line 2114
Quoted 10 lines
revision.c +7
-0@@ -2105,6 +2105,13 @@ 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. +*/ +assert (revs->single_worktree !=0 );
You don't need != 0 above.
We usually don't use assert(foo) but rather if (!foo) die("BUG:..."), which gives a better error message and isn't switched off if a distribution compiles with NDEBUG.
But here I'm confused about whether this check failing really indicates a bug within git, or whether it could indicate a situation that the user has set up that git just can't handle right now. For example, maybe the user will set up a submodule that itself uses worktrees (even if that's not possible now, maybe they will have done it with some future version of git or with another tool). If the problem is only that this version of git is too stupid to handle pruning safely in that situation, it would be more appropriate to use something more like
if (!refs->single_worktree)
die("error: git is currently unable to handle submodules that uselinked worktrees");
revision.c static int handle_revision_pseudo_opt(const char *submodule, · line 2117
Quoted 4 lines
revision.c +0
-0refs =get_submodule_ref_store (submodule); }else refs =get_main_ref_store ();
Michael
PATCH v3 10/12 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.
Ideally we should have something like merge_ref_iterator_begin (and maybe with a predicate), but for dir_iterator. Since there's only one use case for this pattern, let's not add a bunch more code for merge_dir_iterator_begin just yet.
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 | 46 ++++++++++++++++++++++++++++++++----------- t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-)
refs/files-backend.c +35 -11
@@ -1171,15 +1171,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:
@@ -3368,6 +3359,7 @@ struct files_reflog_iterator {struct ref_store *ref_store;struct dir_iterator *dir_iterator; +struct dir_iterator *worktree_dir_iterator;struct object_id oid; };
@@ -3388,6 +3380,21 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)if (ends_with (diter->basename,".lock" ))continue ; +if (iter->worktree_dir_iterator) { +const char *refname = diter->relative_path; + +switch (ref_type (refname)) { +case REF_TYPE_PER_WORKTREE : +case REF_TYPE_PSEUDOREF : +continue ; +case REF_TYPE_NORMAL : +break ; +default : +die ("BUG: unknown ref type %d of ref %s" , +ref_type (refname), refname); + } + } +if (refs_read_ref_full (iter->ref_store, diter->relative_path,0 , iter->oid.hash, &flags)) {
@@ -3401,7 +3408,11 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)return ITER_OK ; } - iter->dir_iterator =NULL ; + iter->dir_iterator = iter->worktree_dir_iterator; +if (iter->worktree_dir_iterator) { + iter->worktree_dir_iterator =NULL ; +return files_reflog_iterator_advance (ref_iterator); + }if (ref_iterator_abort (ref_iterator) ==ITER_ERROR ) ok =ITER_ERROR ;return ok;
@@ -3422,6 +3433,12 @@ static int files_reflog_iterator_abort(struct ref_iterator *ref_iterator)if (iter->dir_iterator) ok =dir_iterator_abort (iter->dir_iterator); +if (iter->worktree_dir_iterator) { +int ok2 =dir_iterator_abort (iter->worktree_dir_iterator); +if (ok2 ==ITER_ERROR ) + ok = ok2; + } +base_ref_iterator_free (ref_iterator);return ok; }
@@ -3442,10 +3459,17 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_ststruct 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" , refs->gitcommondir); iter->dir_iterator =dir_iterator_begin (sb.buf); iter->ref_store = ref_store;strbuf_release (&sb); + +if (strcmp (refs->gitdir, refs->gitcommondir)) { +strbuf_addf (&sb,"%s/logs" , refs->gitdir); + iter->worktree_dir_iterator =dir_iterator_begin (sb.buf); +strbuf_release (&sb); + } +return ref_iterator; }
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 v3 10/12] files-backend: make reflog iterator go through per-worktree reflog
Quoted 70 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.
Ideally we should have something like
merge_ref_iterator_begin(and maybe with a predicate), but fordir_iterator. Since there's only one use case for this pattern, let's not add a bunch more code formerge_dir_iterator_beginjust yet.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 | 46 ++++++++++++++++++++++++++++++++----------- t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-)refs/files-backend.c +16
-9@@ -1171,15 +1171,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:@@ -3368,6 +3359,7 @@ struct files_reflog_iterator {struct ref_store *ref_store;struct dir_iterator *dir_iterator; +struct dir_iterator *worktree_dir_iterator;struct object_id oid; };@@ -3388,6 +3380,21 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)if (ends_with (diter->basename,".lock" ))continue ; +if (iter->worktree_dir_iterator) { +const char *refname = diter->relative_path; + +switch (ref_type (refname)) { +case REF_TYPE_PER_WORKTREE : +case REF_TYPE_PSEUDOREF : +continue ; +case REF_TYPE_NORMAL : +break ; +default : +die ("BUG: unknown ref type %d of ref %s" , +ref_type (refname), refname); + } + } +if (refs_read_ref_full (iter->ref_store, diter->relative_path,0 , iter->oid.hash, &flags)) {
refs/files-backend.c static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) · line 3415
Quoted 9 lines
refs/files-backend.c +5
-1@@ -3401,7 +3408,11 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)return ITER_OK ; } - iter->dir_iterator =NULL ; + iter->dir_iterator = iter->worktree_dir_iterator; +if (iter->worktree_dir_iterator) { + iter->worktree_dir_iterator =NULL ; +return files_reflog_iterator_advance (ref_iterator); + }
I find this implementation confusing:
* if (iter->worktree_dir_iterator) sounds like it should mean that we are iterating over worktree references but it really means that we are iterating over the common references in a repository that is a linked worktree.
* files_reflog_iterator_advance() is called recursively, but only for the first worktree reference.
* iter->worktree_dir_iterator is moved over to iter->dir_iterator when the common refs are exhausted.
Do you find it more readable as follows?:
static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator)
{
struct files_reflog_iterator *iter =
(struct files_reflog_iterator *)ref_iterator;
int ok;
while (1) {
struct dir_iterator **diter;
int normal_only, flags;
if (iter->dir_iterator) {
diter = &iter->dir_iterator;
/*
* If we are in a worktree, then we only
* include "normal" common references:
*/
normal_only = !!iter->worktree_dir_iterator;
} else if (iter->worktree_dir_iterator) {
diter = &iter->worktree_dir_iterator;
normal_only = 0;
} else {
ok = ITER_DONE;
break;
}
ok = dir_iterator_advance(*diter);
if (ok == ITER_ERROR) {
*diter = NULL;
break;
} else if (ok == ITER_DONE) {
*diter = NULL;
/* There might still be worktree refs left: */
continue;
}
if (!S_ISREG((*diter)->st.st_mode))
continue;
if ((*diter)->basename[0] == '.')
continue;
if (ends_with((*diter)->basename, ".lock"))
continue;
iter->base.refname = (*diter)->relative_path;
if (normal_only &&
ref_type(iter->base.refname) != REF_TYPE_NORMAL)
continue;
if (refs_read_ref_full(iter->ref_store,
iter->base.refname, 0,
iter->oid.hash, &flags)) {
error("bad ref for %s", (*diter)->path.buf);
continue;
}
iter->base.oid = &iter->oid;
iter->base.flags = flags;
return ITER_OK;
}
if (ref_iterator_abort(ref_iterator) == ITER_ERROR)
return ITER_ERROR;
return ok;
}refs/files-backend.c static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) · line 3418
Quoted 4 lines
refs/files-backend.c +0
-0if (ref_iterator_abort (ref_iterator) ==ITER_ERROR ) ok =ITER_ERROR ;return ok;
Michael
Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog
Quoted 11 lines · Michael Haggerty
I find this implementation confusing:
*
if (iter->worktree_dir_iterator)sounds like it should mean that we are iterating over worktree references but it really means that we are iterating over the common references in a repository that is a linked worktree.
*files_reflog_iterator_advance()is called recursively, but only for the first worktree reference.
*iter->worktree_dir_iteratoris moved over toiter->dir_iteratorwhen the common refs are exhausted.
Quoted 1 line
Do you find it more readable as follows?:
It's a bit better, but while we're at it, why not take full advantage of iterator abstraction?
This replacement patch (with some unrelated bits removed to reduce distraction) adds a new meta ref-iterator that combine a per-repo and a per-worktree iterators together. The new iterator walks through both sub-iterators and drop the per-worktree results from the per-repo iterator, which will be replaced with results from per-worktree iterator.
You probably see where I'm going with this. When the new "linked worktree ref store" comes, it will combine two per-worktree and per-repo ref stores together and this iterator will come handy.
At that point, files-backend can go back to being oblivious about $GIT_DIR vs $GIT_COMMON_DIR and files_reflog_iterator_begin() will be reverted back to the version before this patch.
refs/files-backend.c +19 -5
@@ -3432,23 +3423,37 @@ 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 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 worktree_ref_iterator_begin ( +reflog_iterator_begin (ref_store, refs->gitcommondir), +reflog_iterator_begin (ref_store, refs->gitdir)); + } +} +static int ref_update_reject_duplicates (struct string_list *refnames,struct strbuf *err) {
refs/iterator.c +105 -0
114 lines not on this page · raw message
refs/refs-internal.h +8 -0
@@ -387,6 +387,14 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,const char *prefix, int trim); +/* +* Wrap per_repo and per_worktree iterators. Traverse per_repo +* iterator, drop per-worktree refs. Then traverse per_worktree +* iterator. +*/ +struct ref_iterator *worktree_ref_iterator_begin (struct ref_iterator *per_repo, +struct ref_iterator *per_worktree); +/* Internal implementation of reference iteration: */ /*
Re: [PATCH v3 10/12] files-backend: make reflog iterator go through per-worktree reflog
Hi,
I put off reviewing this patch, thinking that it would appear in a re-roll, then never came back to it. :-(
Quoted 24 lines · Duy Nguyen
Quoted 11 lines · Michael Haggerty
I find this implementation confusing:
*
if (iter->worktree_dir_iterator)sounds like it should mean that we are iterating over worktree references but it really means that we are iterating over the common references in a repository that is a linked worktree.
*files_reflog_iterator_advance()is called recursively, but only for the first worktree reference.
*iter->worktree_dir_iteratoris moved over toiter->dir_iteratorwhen the common refs are exhausted.Quoted 1 line
Do you find it more readable as follows?:
It's a bit better, but while we're at it, why not take full advantage of iterator abstraction?
This replacement patch (with some unrelated bits removed to reduce distraction) adds a new meta ref-iterator that combine a per-repo and a per-worktree iterators together. The new iterator walks through both sub-iterators and drop the per-worktree results from the per-repo iterator, which will be replaced with results from per-worktree iterator.
Quoted 7 lines
You probably see where I'm going with this. When the new "linked worktree ref store" comes, it will combine two per-worktree and per-repo ref stores together and this iterator will come handy.
At that point, files-backend can go back to being oblivious about $GIT_DIR vs $GIT_COMMON_DIR and
files_reflog_iterator_begin()will be reverted back to the version before this patch.
Yes, that's even better.
Quoted 18 lines
refs/files-backend.c +2
-4@@ -3432,23 +3423,37 @@ 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 ;
refs/files-backend.c static struct ref_iterator_vtable files_reflog_iterator_vtable = { · line 3440
Quoted 10 lines
refs/files-backend.c +2
-1base_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; }
Makes sense.
Quoted 5 lines
refs/files-backend.c +5
-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" );
refs/files-backend.c static struct ref_iterator_vtable files_reflog_iterator_vtable = { · line 3456
Quoted 10 lines
refs/files-backend.c +10
-0+ +if (!strcmp (refs->gitdir, refs->gitcommondir)) { +return reflog_iterator_begin (ref_store, refs->gitcommondir); + }else { +return worktree_ref_iterator_begin ( +reflog_iterator_begin (ref_store, refs->gitcommondir), +reflog_iterator_begin (ref_store, refs->gitdir)); + } +} +
Yes.
Quoted 134 lines
refs/files-backend.c +0
-0static int ref_update_reject_duplicates (struct string_list *refnames,struct strbuf *err) {refs/iterator.c +105
-0@@ -40,6 +40,14 @@ void base_ref_iterator_free(struct ref_iterator *iter)free (iter); } +static void ref_iterator_copy_result (struct ref_iterator *dst, +const struct ref_iterator *src) +{ + dst->refname = src->refname; + dst->oid = src->oid; + dst->flags = src->flags; +} +struct empty_ref_iterator {struct ref_iterator base; };@@ -382,3 +390,100 @@ int do_for_each_ref_iterator(struct ref_iterator *iter,return -1 ;return retval; } + +struct worktree_ref_iterator { +struct ref_iterator base; +struct ref_iterator *per_repo_iterator; +struct ref_iterator *per_worktree_iterator; +}; + +static int worktree_ref_iterator_advance (struct ref_iterator *ref_iterator) +{ +struct worktree_ref_iterator *iter = + (struct worktree_ref_iterator *)ref_iterator; +int ok; + +while (1 ) { +struct ref_iterator **subiter; +int normal_only; + +if (iter->per_repo_iterator) { + subiter = &iter->per_repo_iterator; +/* +* If we are in a worktree, then we only +* include "normal" common references: +*/ + normal_only = !!iter->per_worktree_iterator; + }else if (iter->per_worktree_iterator) { + subiter = &iter->per_worktree_iterator; + normal_only =0 ; + }else { + ok =ITER_DONE ; +break ; + } + + ok =ref_iterator_advance (*subiter); +if (ok ==ITER_ERROR ) { + *subiter =NULL ; +break ; + }else if (ok ==ITER_DONE ) { + *subiter =NULL ; +/* There might still be worktree refs left: */ +continue ; + } + +if (normal_only && +ref_type ((*subiter)->refname) !=REF_TYPE_NORMAL ) +continue ; + +ref_iterator_copy_result (&iter->base, *subiter); +return ITER_OK ; + } + +if (ref_iterator_abort (ref_iterator) ==ITER_ERROR ) +return ITER_ERROR ; + +return ok; +} + +static int worktree_ref_iterator_peel (struct ref_iterator *ref_iterator, +struct object_id *peeled) +{ +die ("BUG: ref_iterator_peel() called for reflog_iterator" ); +} + +static int worktree_ref_iterator_abort (struct ref_iterator *ref_iterator) +{ +struct worktree_ref_iterator *iter = + (struct worktree_ref_iterator *)ref_iterator; +int ok =ITER_DONE ; + +if (iter->per_repo_iterator) + ok =ref_iterator_abort (iter->per_repo_iterator); + +if (iter->per_worktree_iterator) { +int ok2 =ref_iterator_abort (iter->per_worktree_iterator); +if (ok2 ==ITER_ERROR ) + ok = ok2; + } + +base_ref_iterator_free (ref_iterator); +return ok; +} + +static struct ref_iterator_vtable worktree_ref_iterator_vtable = { + worktree_ref_iterator_advance, + worktree_ref_iterator_peel, + worktree_ref_iterator_abort +}; + +struct ref_iterator *worktree_ref_iterator_begin (struct ref_iterator *per_repo, +struct ref_iterator *per_worktree) +{ +struct worktree_ref_iterator *iter =xcalloc (1 ,sizeof (*iter)); + +base_ref_iterator_init (&iter->base, &worktree_ref_iterator_vtable); + iter->per_repo_iterator = per_repo; + iter->per_worktree_iterator = per_worktree; +return &iter->base; +}refs/refs-internal.h +1
-0@@ -387,6 +387,14 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,const char *prefix, int trim); +/*
refs/refs-internal.h struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0, · line 400
Quoted 10 lines
refs/refs-internal.h +7
-0+* Wrap per_repo and per_worktree iterators. Traverse per_repo +* iterator, drop per-worktree refs. Then traverse per_worktree +* iterator. +*/ +struct ref_iterator *worktree_ref_iterator_begin (struct ref_iterator *per_repo, +struct ref_iterator *per_worktree); +/* Internal implementation of reference iteration: */ /*
I think worktree_ref_iterator_begin() could be implemented with a lot less code via a merge_ref_iterator. You would need to define a ref_iterator_select_fn, which could look like something like this:
enum iterator_selection worktree_iterator_select_fn(
struct ref_iterator *iter0, struct ref_iterator *iter1, void *cb_data)
{
if (iter0) {
if (ref_type(iter0->refname) != REF_TYPE_NORMAL)
return ITER_SKIP_0;
return ITER_SELECT_0;
} else if (iter1) {
return ITER_SELECT_1;
} else {
return ITER_SELECT_DONE;
}
}Then worktree_ref_iterator_begin() is simply
struct ref_iterator *worktree_ref_iterator_begin(struct ref_iterator *per_repo,
struct ref_iterator *per_worktree)
{
return merge_ref_iterator_begin(per_repo, per_worktree,
worktree_iterator_select_fn, NULL);
}For references (as opposed to reflogs) you would want to interleave the outputs from the two input iterators in the correct order. See overlay_iterator_select() for how that can be done.
Michael
PATCH v3 11/12 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
@@ -1134,6 +1134,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)
@@ -1169,6 +1170,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs, { cb->all_revs = revs; cb->all_flags = flags; + cb->refs =NULL ; }void clear_ref_exclusion (struct string_list **ref_excludes_p)
@@ -1237,17 +1239,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 v3 12/12 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
@@ -179,6 +179,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
@@ -2222,6 +2222,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
Quoted 2 lines · Nguyễn Thái Ngọc Duy
Changes since v2 [1] is relatively small. It still needs nd/worktree-kill-parse-ref of course.
I read the whole series. Aside from the comments I made, it looks good to me.
This will be able to be cleaned up once we have a cleaner distinction between "logical" and "physical" ref_stores. But given the current state of the code, your implementation is reasonable.
Michael