Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees

[Michael Haggerty]

On 08/23/2017 02:36 PM, Nguyễn Thái Ngọc Duy wrote:
> [...]
> diff --git a/revision.c b/revision.c
> index 8d04516266..0e98444857 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
>   int argcount;
>  
>   if (submodule) {
> + /*
> +  * We need some something like get_submodule_worktrees()
> +  * before we can go through all worktrees of a submodule,
> +  * .e.g with adding all HEADs from --all, which is not
> +  * supported right now, so stick to single worktree.
> +  */
> + if (!revs->single_worktree)
> + die("BUG: --single-worktree cannot be used together 
> with submodule");
>   refs = get_submodule_ref_store(submodule);
>   } else
>   refs = get_main_ref_store();

Tiny nit: s/.e.g/e.g./

> [...]

Michael

Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees

[Stefan Beller]

On Wed, Sep 6, 2017 at 4:19 AM, Duy Nguyen  wrote:
>
> So, probably no worktree iterator (yet).

Ok, thanks for considering it.

Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees

[Duy Nguyen]

On Thu, Aug 24, 2017 at 2:54 AM, Stefan Beller  wrote:
>> +int other_head_refs(each_ref_fn fn, void *cb_data)
>> +{
>> +   struct worktree **worktrees, **p;
>> +   int ret = 0;
>> +
>> +   worktrees = get_worktrees(0);
>> +   for (p = worktrees; *p; p++) {
>> +   struct worktree *wt = *p;
>> +   struct ref_store *refs;
>> +
>> +   if (wt->is_current)
>> +   continue;
>
> As said in an earlier patch (and now this pattern
> coming up twice in this patch series alone), the lines
> of this function up to here, could become
> part of a worktree iterator function?

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).

>> +   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.

yeah.. but we can't mix and match (or combine) ref/worktree iterator callbacks

-- 
Duy

Re: [PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees

[Stefan Beller]

> +int other_head_refs(each_ref_fn fn, void *cb_data)
> +{
> +   struct worktree **worktrees, **p;
> +   int ret = 0;
> +
> +   worktrees = get_worktrees(0);
> +   for (p = worktrees; *p; p++) {
> +   struct worktree *wt = *p;
> +   struct ref_store *refs;
> +
> +   if (wt->is_current)
> +   continue;

As said in an earlier patch (and now this pattern
coming up twice in this patch series alone), the lines
of this function up to here, could become
part of a worktree iterator function?

> +   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.

[PATCH v4 11/16] revision.c: --all adds HEAD from all worktrees

[Nguyễn Thái Ngọc Duy]

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 
---
 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(+)

diff --git a/reachable.c b/reachable.c
index c62efbfd43..492e87b9fa 100644
--- a/reachable.c
+++ b/reachable.c
@@ -9,6 +9,7 @@
 #include "cache-tree.h"
 #include "progress.h"
 #include "list-objects.h"
+#include "worktree.h"
 
 struct connectivity_progress {
struct progress *progress;
@@ -176,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int 
mark_reflog,
 
/* detached HEAD is not included in the list above */
head_ref(add_one_ref, revs);
+   other_head_refs(add_one_ref, revs);
 
/* Add all reflog info */
if (mark_reflog)
diff --git a/revision.c b/revision.c
index 8d04516266..0e98444857 100644
--- a/revision.c
+++ b/revision.c
@@ -2133,6 +2133,14 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
int argcount;
 
if (submodule) {
+   /*
+* We need some something like get_submodule_worktrees()
+* before we can go through all worktrees of a submodule,
+* .e.g with adding all HEADs from --all, which is not
+* supported right now, so stick to single worktree.
+*/
+   if (!revs->single_worktree)
+   die("BUG: --single-worktree cannot be used together 
with submodule");
refs = get_submodule_ref_store(submodule);
} else
refs = get_main_ref_store();
@@ -2150,6 +2158,12 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
if (!strcmp(arg, "--all")) {
handle_refs(refs, revs, *flags, refs_for_each_ref);
handle_refs(refs, revs, *flags, refs_head_ref);
+   if (!revs->single_worktree) {
+   struct all_refs_cb cb;
+
+   init_all_refs_cb(, revs, *flags);
+   other_head_refs(handle_one_ref, );
+   }
clear_ref_exclusion(>ref_excludes);
} else if (!strcmp(arg, "--branches")) {
handle_refs(refs, revs, *flags, refs_for_each_branch_ref);
diff --git a/submodule.c b/submodule.c
index 98e1f9d3c7..61a38adcd4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1685,6 +1685,8 @@ static int find_first_merges(struct object_array *result, 
const char *path,
oid_to_hex(>object.oid));
init_revisions(, 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, , _opts);
 
/* save all revisions from the above list that contain b */
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index cba45c7be9..683bdb031c 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -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
diff --git a/worktree.c b/worktree.c
index e28ffbeb09..389e2e952c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -386,3 +386,25 @@ int submodule_uses_worktrees(const char *path)
closedir(dir);
return ret;
 }
+
+int other_head_refs(each_ref_fn fn, void *cb_data)
+{
+   struct worktree **worktrees, **p;
+   int ret = 0;
+
+   worktrees = get_worktrees(0);
+   for (p = worktrees; *p; p++) {
+   struct worktree *wt = *p;
+   struct ref_store *refs;
+
+   if (wt->is_current)
+   continue;
+
+   refs =