"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/20170419110145.5086-1-pclouds@gmail.com/
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(-)
--
2.11.0.157.gd943d85
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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
revision.h | 1 +
1 file changed, 1 insertion(+)
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 [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Stefan Beller <hidden> Date: 2017-08-23 19:14:21
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
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 [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
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
On Thu, Aug 24, 2017 at 2:14 AM, Stefan Beller [off-list ref] wrote:
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
quoted
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 [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
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.
-> 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)
* 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...
--
Duy
From: Stefan Beller <hidden> Date: 2017-09-06 17:41:39
On Wed, Sep 6, 2017 at 4:08 AM, Duy Nguyen [off-list ref] wrote:
On Thu, Aug 24, 2017 at 2:14 AM, Stefan Beller [off-list ref] wrote:
quoted
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
quoted
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 [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
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
-> 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)
IIRC I asked these questions as I was genuinely confused,
I do not mind too much either.
quoted
* 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...
The 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
revision.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
This 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
revision.c | 21 +++++++++++++++++++++
t/t5304-prune.sh | 9 +++++++++
2 files changed, 30 insertions(+)
@@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,voidadd_index_objects_to_pending(structrev_info*revs,unsignedintflags){+structworktree**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++){+structworktree*wt=*p;+structindex_stateistate={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);}staticintadd_parents_only(structrev_info*revs,constchar*arg_,intflags,
From: Stefan Beller <hidden> Date: 2017-08-23 19:25:28
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
quoted hunk
This 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
revision.c | 21 +++++++++++++++++++++
t/t5304-prune.sh | 9 +++++++++
2 files changed, 30 insertions(+)
@@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,voidadd_index_objects_to_pending(structrev_info*revs,unsignedintflags){+structworktree**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++){+structworktree*wt=*p;+structindex_stateistate={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);}
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 hunk
static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
@@ -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)-returnrefs;+gotodone;strbuf_addstr(&submodule_sb,submodule);-ret=is_nonbare_repository_dir(&submodule_sb);-strbuf_release(&submodule_sb);-if(!ret)-returnNULL;+if(!is_nonbare_repository_dir(&submodule_sb))+gotodone;-ret=submodule_to_gitdir(&submodule_sb,submodule);-if(ret){-strbuf_release(&submodule_sb);-returnNULL;-}+if(submodule_to_gitdir(&submodule_sb,submodule))+gotodone;/* assume that add_submodule_odb() has been called */refs=ref_store_init(submodule_sb.buf,
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
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 <redacted>
---
refs.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
@@ -1503,25 +1503,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,intresolve_gitlink_ref(constchar*submodule,constchar*refname,unsignedchar*sha1){-size_tlen=strlen(submodule);structref_store*refs;intflags;-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;
@@ -1645,6 +1640,10 @@ struct ref_store *get_submodule_ref_store(const char *submodule)returnget_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)gotodone;
From: Michael Haggerty <hidden> Date: 2017-09-09 05:45:14
On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
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
These are used in revision.c. After the last patch they are replaced
with the refs_ version. Delete them.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
Documentation/technical/api-ref-iteration.txt | 7 ++----
refs.c | 33 ---------------------------
refs.h | 10 --------
3 files changed, 2 insertions(+), 48 deletions(-)
@@ -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.
From: Stefan Beller <hidden> Date: 2017-08-23 19:45:41
On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:
These are used in revision.c. After the last patch they are replaced
with the refs_ version. Delete them.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
The refactoring up to this patch looks good to me.
Thanks,
Stefan
From: Michael Haggerty <hidden> Date: 2017-09-09 05:59:13
On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
These are used in revision.c. After the last patch they are replaced
with the refs_ version. Delete them.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
Documentation/technical/api-ref-iteration.txt | 7 ++----
refs.c | 33 ---------------------------
refs.h | 10 --------
3 files changed, 2 insertions(+), 48 deletions(-)
Unless 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
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(+)
@@ -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)
@@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char *submodule,intargcount;if(submodule){+/*+*Weneedsomesomethinglikeget_submodule_worktrees()+*beforewecangothroughallworktreesofasubmodule,+*.e.gwithaddingallHEADsfrom--all,whichisnot+*supportedrightnow,sosticktosingleworktree.+*/+if(!revs->single_worktree)+die("BUG: --single-worktree cannot be used together with submodule");refs=get_submodule_ref_store(submodule);}elserefs=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){+structall_refs_cbcb;++init_all_refs_cb(&cb,revs,*flags);+other_head_refs(handle_one_ref,&cb);+}clear_ref_exclusion(&revs->ref_excludes);}elseif(!strcmp(arg,"--branches")){handle_refs(refs,revs,*flags,refs_for_each_branch_ref);
@@ -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 */
@@ -292,4 +292,16 @@ test_expect_success 'prune: handle index in multiple worktrees' 'test_cmpsecond-worktree/blobactual'+test_expect_success'prune: handle HEAD in multiple worktrees''+gitworktreeadd--detachthird-worktree&&+echo"new blob for third-worktree">third-worktree/blob&&+git-Cthird-worktreeaddblob&&+git-Cthird-worktreecommit-m"third"&&+rm.git/worktrees/third-worktree/index&&+test_must_failgit-Cthird-worktreeshow:blob&&+gitprune--expire=now&&+git-Cthird-worktreeshowHEAD:blob>actual&&+test_cmpthird-worktree/blobactual+'+ test_done
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?
+ 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.
+/*
+ * 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.
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
+ 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
+/*
+ * 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
--
Duy
@@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char *submodule,intargcount;if(submodule){+/*+*Weneedsomesomethinglikeget_submodule_worktrees()+*beforewecangothroughallworktreesofasubmodule,+*.e.gwithaddingallHEADsfrom--all,whichisnot+*supportedrightnow,sosticktosingleworktree.+*/+if(!revs->single_worktree)+die("BUG: --single-worktree cannot be used together with submodule");refs=get_submodule_ref_store(submodule);}elserefs=get_main_ref_store();
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_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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++----------
t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++
2 files changed, 75 insertions(+), 14 deletions(-)
From: Richard Maw <hidden> Date: 2017-08-24 14:13:58
On Wed, Aug 23, 2017 at 07:37:00PM +0700, Nguyễn Thái Ngọc Duy wrote:
quoted hunk
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_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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++----------
t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++
2 files changed, 75 insertions(+), 14 deletions(-)
+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.
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 :).
From: Michael Haggerty <hidden> Date: 2017-09-09 06:30:27
On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
quoted hunk
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_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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
refs/files-backend.c | 59 +++++++++++++++++++++++++++++++++----------
t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++
2 files changed, 75 insertions(+), 14 deletions(-)
[...]
+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;
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?
Note 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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
revision.c | 28 +++++++++++++++++++++++++++-
t/t5304-prune.sh | 16 ++++++++++++++++
2 files changed, 43 insertions(+), 1 deletion(-)
@@ -304,4 +304,20 @@ test_expect_success 'prune: handle HEAD in multiple worktrees' 'test_cmpthird-worktree/blobactual'+test_expect_success'prune: handle HEAD reflog in multiple worktrees''+gitconfigcore.logAllRefUpdatestrue&&+echo"lost blob for third-worktree">expected&&+(+cdthird-worktree&&+cat../expected>blob&&+gitaddblob&&+gitcommit-m"second commit in third"&&+gitreset--hardHEAD^+)&&+gitprune--expire=now&&+SHA1=`githash-objectexpected`&&+git-Cthird-worktreeshow"$SHA1">actual&&+test_cmpexpectedactual+'+ test_done
@@ -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.
@@ -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.
@@ -2251,6 +2251,8 @@ static int handle_revision_pseudo_opt(const char *submodule,returnerror("invalid argument to --no-walk");}elseif(!strcmp(arg,"--do-walk")){revs->no_walk=0;+}elseif(!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.
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()
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.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
refs.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
@@ -1595,14 +1598,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)returnNULL;}-if(!submodule||!*submodule){-/*-*FIXME:Thiscaseisideallynotallowed.Butthat-*can'thappenuntilwecleanupallthecallers.-*/-returnget_main_ref_store();-}-if(submodule[len])/* We need to strip off one or more trailing slashes */submodule=to_free=xmemdupz(submodule,len);
From: Michael Haggerty <hidden> Date: 2017-09-09 06:36:40
On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
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()
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.
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 <redacted>
---
refs.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
@@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char *submodule)if(!submodule)returnNULL;-if(submodule){-len=strlen(submodule);-while(len&&is_dir_sep(submodule[len-1]))-len--;-if(!len)-returnNULL;-}+len=strlen(submodule);+while(len&&is_dir_sep(submodule[len-1]))+len--;+if(!len)+returnNULL;if(submodule[len])/* We need to strip off one or more trailing slashes */
From: Michael Haggerty <hidden> Date: 2017-09-09 06:42:02
On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
quoted hunk
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 <redacted>
---
refs.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
@@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char *submodule)if(!submodule)returnNULL;-if(submodule){-len=strlen(submodule);-while(len&&is_dir_sep(submodule[len-1]))-len--;-if(!len)-returnNULL;-}+len=strlen(submodule);+while(len&&is_dir_sep(submodule[len-1]))+len--;+if(!len)+returnNULL;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
From: Michael J Gruber <hidden> Date: 2017-08-25 11:21:21
Nguyễn Thái Ngọc Duy venit, vidit, dixit 23.08.2017 14:36:
"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/20170419110145.5086-1-pclouds@gmail.com/
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()
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
On Fri, Aug 25, 2017 at 6:21 PM, Michael J Gruber [off-list ref] wrote:
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
* --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...]
Hmm.. I admit that I completely overlooked 'git show-ref'.
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.
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.
I thinking that changing show-ref (porcelain, not quite as commonly
used) should do the least harm compared to all other options.
From: Michael Haggerty <hidden> Date: 2017-09-09 06:46:01
On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
"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).
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