[PATCH v4 00/16] Fix git-gc losing objects in multi worktree

[PATCH v4 00/16] Fix git-gc losing objects in multi worktree

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

"git gc" when used in multiple worktrees ignore some per-worktree
references: object references in the index, HEAD and reflog. This
series fixes it by making the revision walker include these from all
worktrees by default (and the series is basically split in three parts
in the same order). There's a couple more cleanups 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

[PATCH v4 01/16] revision.h: new flag in struct rev_info wrt. worktree-related refs

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

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 <pclouds@gmail.com>
---
 revision.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/revision.h b/revision.h
index bc18487d6f..3a3d3e2cf8 100644
--- a/revision.h
+++ b/revision.h
@@ -96,6 +96,7 @@ struct rev_info {
 			topo_order:1,
 			simplify_merges:1,
 			simplify_by_decoration:1,
+			single_worktree:1,
 			tag_objects:1,
 			tree_objects:1,
 			blob_objects:1,
-- 
2.11.0.157.gd943d85

[PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()

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

The "submodule" argument in this function is a path, which can have
either '/' or '\\' as a separator. Use is_dir_sep() to support both.

Noticed-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 3d549a8970..dec899a57a 100644
--- a/refs.c
+++ b/refs.c
@@ -1507,7 +1507,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
 	struct ref_store *refs;
 	int flags;
 
-	while (len && submodule[len - 1] == '/')
+	while (len && is_dir_sep(submodule[len - 1]))
 		len--;
 
 	if (!len)
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()

[Stefan Beller]

On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <j6t@kdbg.org>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

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

> ---
>  refs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs.c b/refs.c
> index 3d549a8970..dec899a57a 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1507,7 +1507,7 @@ int resolve_gitlink_ref(const char *submodule, const char *refname,
>         struct ref_store *refs;
>         int flags;
>
> -       while (len && submodule[len - 1] == '/')
> +       while (len && is_dir_sep(submodule[len - 1]))
>                 len--;
>
>         if (!len)
> --
> 2.11.0.157.gd943d85
>

Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()

[Duy Nguyen]

On Thu, Aug 24, 2017 at 2:14 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <j6t@kdbg.org>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> 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)

>   -> If not, do we need one?

Since I don't use Windows, I don't particularly care. And I can't add
one anyway because I can't run it.

[1] https://public-inbox.org/git/%3Ca74cf309-fb16-2f45-8189-d1d0c655dea4@kdbg.org%3E/

> * 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

Re: [PATCH v4 02/16] refs.c: use is_dir_sep() in resolve_gitlink_ref()

[Stefan Beller]

On Wed, Sep 6, 2017 at 4:08 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Thu, Aug 24, 2017 at 2:14 AM, Stefan Beller <sbeller@google.com> wrote:
>> On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <j6t@kdbg.org>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>
>> 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)
>
>>   -> If not, do we need one?
>
> Since I don't use Windows, I don't particularly care. And I can't add
> one anyway because I can't run it.
>
> [1] https://public-inbox.org/git/%3Ca74cf309-fb16-2f45-8189-d1d0c655dea4@kdbg.org%3E/

IIRC I asked these questions as I was genuinely confused,
I do not mind too much either.

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

I had to reread the coverletter
https://public-inbox.org/git/20170823123704.16518-1-pclouds@gmail.com/
to get that part. I would not be opposed to splitting the series, but
I'll review an unsplit series as well.

Thanks,
Stefan

> --
> Duy

[PATCH v4 03/16] revision.c: refactor add_index_objects_to_pending()

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

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 <pclouds@gmail.com>
---
 revision.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/revision.c b/revision.c
index aa3b946a8d..6c46ad6c8a 100644
--- a/revision.c
+++ b/revision.c
@@ -1262,13 +1262,13 @@ static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
 
 }
 
-void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
+static void do_add_index_objects_to_pending(struct rev_info *revs,
+					    struct index_state *istate)
 {
 	int i;
 
-	read_cache();
-	for (i = 0; i < active_nr; i++) {
-		struct cache_entry *ce = active_cache[i];
+	for (i = 0; i < istate->cache_nr; i++) {
+		struct cache_entry *ce = istate->cache[i];
 		struct blob *blob;
 
 		if (S_ISGITLINK(ce->ce_mode))
@@ -1281,13 +1281,19 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned flags)
 					     ce->ce_mode, ce->name);
 	}
 
-	if (active_cache_tree) {
+	if (istate->cache_tree) {
 		struct strbuf path = STRBUF_INIT;
-		add_cache_tree(active_cache_tree, revs, &path);
+		add_cache_tree(istate->cache_tree, revs, &path);
 		strbuf_release(&path);
 	}
 }
 
+void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
+{
+	read_cache();
+	do_add_index_objects_to_pending(revs, &the_index);
+}
+
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
 			    int exclude_parent)
 {
-- 
2.11.0.157.gd943d85

[PATCH v4 04/16] revision.c: --indexed-objects add objects from all worktrees

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

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 <pclouds@gmail.com>
---
 revision.c       | 21 +++++++++++++++++++++
 t/t5304-prune.sh |  9 +++++++++
 2 files changed, 30 insertions(+)

diff --git a/revision.c b/revision.c
index 6c46ad6c8a..f35cb49af5 100644
--- a/revision.c
+++ b/revision.c
@@ -19,6 +19,7 @@
 #include "dir.h"
 #include "cache-tree.h"
 #include "bisect.h"
+#include "worktree.h"
 
 volatile show_early_output_fn_t show_early_output;
 
@@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,
 
 void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 {
+	struct worktree **worktrees, **p;
+
 	read_cache();
 	do_add_index_objects_to_pending(revs, &the_index);
+
+	if (revs->single_worktree)
+		return;
+
+	worktrees = get_worktrees(0);
+	for (p = worktrees; *p; p++) {
+		struct worktree *wt = *p;
+		struct index_state istate = { NULL };
+
+		if (wt->is_current)
+			continue; /* current index already taken care of */
+
+		if (read_index_from(&istate,
+				    worktree_git_path(wt, "index")) > 0)
+			do_add_index_objects_to_pending(revs, &istate);
+		discard_index(&istate);
+	}
+	free_worktrees(worktrees);
 }
 
 static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 133b5842b1..cba45c7be9 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object database' '
 	git -C B prune
 '
 
+test_expect_success 'prune: handle index in multiple worktrees' '
+	git worktree add second-worktree &&
+	echo "new blob for second-worktree" >second-worktree/blob &&
+	git -C second-worktree add blob &&
+	git prune --expire=now &&
+	git -C second-worktree show :blob >actual &&
+	test_cmp second-worktree/blob actual
+'
+
 test_done
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 04/16] revision.c: --indexed-objects add objects from all worktrees

[Stefan Beller]

On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> 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 <pclouds@gmail.com>
> ---
>  revision.c       | 21 +++++++++++++++++++++
>  t/t5304-prune.sh |  9 +++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 6c46ad6c8a..f35cb49af5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -19,6 +19,7 @@
>  #include "dir.h"
>  #include "cache-tree.h"
>  #include "bisect.h"
> +#include "worktree.h"
>
>  volatile show_early_output_fn_t show_early_output;
>
> @@ -1290,8 +1291,28 @@ static void do_add_index_objects_to_pending(struct rev_info *revs,
>
>  void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
>  {
> +       struct worktree **worktrees, **p;
> +
>         read_cache();
>         do_add_index_objects_to_pending(revs, &the_index);
> +
> +       if (revs->single_worktree)
> +               return;
> +
> +       worktrees = get_worktrees(0);
> +       for (p = worktrees; *p; p++) {
> +               struct worktree *wt = *p;
> +               struct index_state istate = { NULL };
> +
> +               if (wt->is_current)
> +                       continue; /* current index already taken care of */
> +
> +               if (read_index_from(&istate,
> +                                   worktree_git_path(wt, "index")) > 0)
> +                       do_add_index_objects_to_pending(revs, &istate);
> +               discard_index(&istate);
> +       }
> +       free_worktrees(worktrees);
>  }

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)

>
>  static int add_parents_only(struct rev_info *revs, const char *arg_, int flags,
> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index 133b5842b1..cba45c7be9 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -283,4 +283,13 @@ test_expect_success 'prune: handle alternate object database' '
>         git -C B prune
>  '
>
> +test_expect_success 'prune: handle index in multiple worktrees' '
> +       git worktree add second-worktree &&
> +       echo "new blob for second-worktree" >second-worktree/blob &&
> +       git -C second-worktree add blob &&
> +       git prune --expire=now &&
> +       git -C second-worktree show :blob >actual &&
> +       test_cmp second-worktree/blob actual
> +'
> +
>  test_done
> --
> 2.11.0.157.gd943d85
>

[PATCH v4 05/16] refs.c: refactor get_submodule_ref_store(), share common free block

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/refs.c b/refs.c
index dec899a57a..522c4ab74f 100644
--- a/refs.c
+++ b/refs.c
@@ -1636,7 +1636,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 {
 	struct strbuf submodule_sb = STRBUF_INIT;
 	struct ref_store *refs;
-	int ret;
 
 	if (!submodule || !*submodule) {
 		/*
@@ -1648,19 +1647,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 
 	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
 	if (refs)
-		return refs;
+		goto done;
 
 	strbuf_addstr(&submodule_sb, submodule);
-	ret = is_nonbare_repository_dir(&submodule_sb);
-	strbuf_release(&submodule_sb);
-	if (!ret)
-		return NULL;
+	if (!is_nonbare_repository_dir(&submodule_sb))
+		goto done;
 
-	ret = submodule_to_gitdir(&submodule_sb, submodule);
-	if (ret) {
-		strbuf_release(&submodule_sb);
-		return NULL;
-	}
+	if (submodule_to_gitdir(&submodule_sb, submodule))
+		goto done;
 
 	/* assume that add_submodule_odb() has been called */
 	refs = ref_store_init(submodule_sb.buf,
@@ -1668,6 +1662,7 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	register_ref_store_map(&submodule_ref_stores, "submodule",
 			       refs, submodule);
 
+done:
 	strbuf_release(&submodule_sb);
 	return refs;
 }
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 05/16] refs.c: refactor get_submodule_ref_store(), share common free block

[Stefan Beller]

On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  refs.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index dec899a57a..522c4ab74f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1636,7 +1636,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  {
>         struct strbuf submodule_sb = STRBUF_INIT;
>         struct ref_store *refs;

Do we need to assign NULL here, to cover the early outs?

> -       int ret;
>
>         if (!submodule || !*submodule) {
>                 /*
> @@ -1648,19 +1647,14 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>
>         refs = lookup_ref_store_map(&submodule_ref_stores, submodule);

Answering the question from above, no we do not need to
assign NULL as we have an unconditional assignment
here and any early outs are after this.

Thanks,
Stefan

[PATCH v4 06/16] refs: move submodule slash stripping code to get_submodule_ref_store

[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 <pclouds@gmail.com>
---
 refs.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 522c4ab74f..ea8e6b9f42 100644
--- a/refs.c
+++ b/refs.c
@@ -1503,25 +1503,10 @@ const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
 int resolve_gitlink_ref(const char *submodule, const char *refname,
 			unsigned char *sha1)
 {
-	size_t len = strlen(submodule);
 	struct ref_store *refs;
 	int flags;
 
-	while (len && is_dir_sep(submodule[len - 1]))
-		len--;
-
-	if (!len)
-		return -1;
-
-	if (submodule[len]) {
-		/* We need to strip off one or more trailing slashes */
-		char *stripped = xmemdupz(submodule, len);
-
-		refs = get_submodule_ref_store(stripped);
-		free(stripped);
-	} else {
-		refs = get_submodule_ref_store(submodule);
-	}
+	refs = get_submodule_ref_store(submodule);
 
 	if (!refs)
 		return -1;
@@ -1636,6 +1621,16 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 {
 	struct strbuf submodule_sb = STRBUF_INIT;
 	struct ref_store *refs;
+	char *to_free = NULL;
+	size_t len;
+
+	if (submodule) {
+		len = strlen(submodule);
+		while (len && is_dir_sep(submodule[len - 1]))
+			len--;
+		if (!len)
+			return NULL;
+	}
 
 	if (!submodule || !*submodule) {
 		/*
@@ -1645,6 +1640,10 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 		return get_main_ref_store();
 	}
 
+	if (submodule[len])
+		/* We need to strip off one or more trailing slashes */
+		submodule = to_free = xmemdupz(submodule, len);
+
 	refs = lookup_ref_store_map(&submodule_ref_stores, submodule);
 	if (refs)
 		goto done;
@@ -1664,6 +1663,8 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 
 done:
 	strbuf_release(&submodule_sb);
+	free(to_free);
+
 	return refs;
 }
 
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 06/16] refs: move submodule slash stripping code to get_submodule_ref_store

[Michael Haggerty]

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

[PATCH v4 07/16] refs: add refs_head_ref()

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 23 +++++++++++++----------
 refs.h |  2 ++
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index ea8e6b9f42..b3a0a24469 100644
--- a/refs.c
+++ b/refs.c
@@ -1248,27 +1248,30 @@ int refs_rename_ref_available(struct ref_store *refs,
 	return ok;
 }
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	struct object_id oid;
 	int flag;
 
-	if (submodule) {
-		if (resolve_gitlink_ref(submodule, "HEAD", oid.hash) == 0)
-			return fn("HEAD", &oid, 0, cb_data);
-
-		return 0;
-	}
-
-	if (!read_ref_full("HEAD", RESOLVE_REF_READING, oid.hash, &flag))
+	if (!refs_read_ref_full(refs, "HEAD", RESOLVE_REF_READING,
+				oid.hash, &flag))
 		return fn("HEAD", &oid, flag, cb_data);
 
 	return 0;
 }
 
+int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
+{
+	struct ref_store *refs = get_submodule_ref_store(submodule);
+
+	if (!refs)
+		return -1;
+	return refs_head_ref(refs, fn, cb_data);
+}
+
 int head_ref(each_ref_fn fn, void *cb_data)
 {
-	return head_ref_submodule(NULL, fn, cb_data);
+	return refs_head_ref(get_main_ref_store(), fn, cb_data);
 }
 
 struct ref_iterator *refs_ref_iterator_begin(
diff --git a/refs.h b/refs.h
index 6daa78eb50..8073f8ab56 100644
--- a/refs.h
+++ b/refs.h
@@ -275,6 +275,8 @@ typedef int each_ref_fn(const char *refname,
  * modifies the reference also returns a nonzero value to immediately
  * stop the iteration. Returned references are sorted.
  */
+int refs_head_ref(struct ref_store *refs,
+		  each_ref_fn fn, void *cb_data);
 int refs_for_each_ref(struct ref_store *refs,
 		      each_ref_fn fn, void *cb_data);
 int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 07/16] refs: add refs_head_ref()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

... which does what?

    Unlike refs_for_each_ref() and friends, this does not iterate.
    It just uses the same function signature to make a single call
    of fn on the "HEAD" ref.

Did I capture what it does right?

Thanks.

Re: [PATCH v4 07/16] refs: add refs_head_ref()

[Duy Nguyen]

On Fri, Aug 25, 2017 at 4:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
> ... which does what?
>
>     Unlike refs_for_each_ref() and friends, this does not iterate.
>     It just uses the same function signature to make a single call
>     of fn on the "HEAD" ref.
>
> Did I capture what it does right?

It's basically head_ref() but can take any ref store (while head_ref()
always uses the main ref store). I'll update the commit message. What
you describe is correct, but I think I'll leave that out and assume
people know what head_ref() does.
-- 
Duy

[PATCH v4 08/16] revision.c: use refs_for_each*() instead of for_each_*_submodule()

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c     |  9 ++++-----
 refs.h     |  6 +++---
 revision.c | 48 ++++++++++++++++++++++++++++++++----------------
 3 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/refs.c b/refs.c
index b3a0a24469..cd61509bc8 100644
--- a/refs.c
+++ b/refs.c
@@ -1362,16 +1362,15 @@ int for_each_ref_in_submodule(const char *submodule, const char *prefix,
 				    prefix, fn, cb_data);
 }
 
-int for_each_fullref_in_submodule(const char *submodule, const char *prefix,
-				  each_ref_fn fn, void *cb_data,
-				  unsigned int broken)
+int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
+			     each_ref_fn fn, void *cb_data,
+			     unsigned int broken)
 {
 	unsigned int flag = 0;
 
 	if (broken)
 		flag = DO_FOR_EACH_INCLUDE_BROKEN;
-	return do_for_each_ref(get_submodule_ref_store(submodule),
-			       prefix, fn, 0, flag, cb_data);
+	return do_for_each_ref(refs, prefix, fn, 0, flag, cb_data);
 }
 
 int for_each_replace_ref(each_ref_fn fn, void *cb_data)
diff --git a/refs.h b/refs.h
index 8073f8ab56..a8d6f33703 100644
--- a/refs.h
+++ b/refs.h
@@ -291,6 +291,9 @@ int refs_for_each_remote_ref(struct ref_store *refs,
 int head_ref(each_ref_fn fn, void *cb_data);
 int for_each_ref(each_ref_fn fn, void *cb_data);
 int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
+int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
+			     each_ref_fn fn, void *cb_data,
+			     unsigned int broken);
 int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
 			unsigned int broken);
 int for_each_tag_ref(each_ref_fn fn, void *cb_data);
@@ -306,9 +309,6 @@ int for_each_ref_submodule(const char *submodule,
 			   each_ref_fn fn, void *cb_data);
 int for_each_ref_in_submodule(const char *submodule, const char *prefix,
 			      each_ref_fn fn, void *cb_data);
-int for_each_fullref_in_submodule(const char *submodule, const char *prefix,
-				  each_ref_fn fn, void *cb_data,
-				  unsigned int broken);
 int for_each_tag_ref_submodule(const char *submodule,
 			       each_ref_fn fn, void *cb_data);
 int for_each_branch_ref_submodule(const char *submodule,
diff --git a/revision.c b/revision.c
index f35cb49af5..8d04516266 100644
--- a/revision.c
+++ b/revision.c
@@ -1188,12 +1188,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)
@@ -2095,23 +2102,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_fullref_in_submodule(submodule, bisect_refs.buf, fn, cb_data, 0);
+	status = refs_for_each_fullref_in(refs, bisect_refs.buf, fn, cb_data, 0);
 	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,
@@ -2120,8 +2129,14 @@ static int handle_revision_pseudo_opt(const char *submodule,
 {
 	const char *arg = argv[0];
 	const char *optarg;
+	struct ref_store *refs;
 	int argcount;
 
+	if (submodule) {
+		refs = get_submodule_ref_store(submodule);
+	} else
+		refs = get_main_ref_store();
+
 	/*
 	 * NOTE!
 	 *
@@ -2133,22 +2148,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;
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 08/16] revision.c: use refs_for_each*() instead of for_each_*_submodule()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

So... the idea is that the caller gives a ref-store and tells these
functions to iterate over refs in it, and the existing submodule
related callers can prepare a ref-store for the submodule---that
way, refs.[ch] layer does not have to _care_ that the set of refs it
was asked to look at is for submodule processing.

Nice.  Very nice.

> @@ -2120,8 +2129,14 @@ static int handle_revision_pseudo_opt(const char *submodule,
>  {
>  	const char *arg = argv[0];
>  	const char *optarg;
> +	struct ref_store *refs;
>  	int argcount;
>  
> +	if (submodule) {
> +		refs = get_submodule_ref_store(submodule);
> +	} else
> +		refs = get_main_ref_store();
> +

[PATCH v4 09/16] refs.c: move for_each_remote_ref_submodule() to submodule.c

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c      | 6 ------
 refs.h      | 2 --
 submodule.c | 7 +++++++
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index cd61509bc8..7fa19e9309 100644
--- a/refs.c
+++ b/refs.c
@@ -368,12 +368,6 @@ int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_remote_ref(get_main_ref_store(), fn, cb_data);
 }
 
-int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_remote_ref(get_submodule_ref_store(submodule),
-					fn, cb_data);
-}
-
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
 {
 	struct strbuf buf = STRBUF_INIT;
diff --git a/refs.h b/refs.h
index a8d6f33703..5d25da227a 100644
--- a/refs.h
+++ b/refs.h
@@ -313,8 +313,6 @@ int for_each_tag_ref_submodule(const char *submodule,
 			       each_ref_fn fn, void *cb_data);
 int for_each_branch_ref_submodule(const char *submodule,
 				  each_ref_fn fn, void *cb_data);
-int for_each_remote_ref_submodule(const char *submodule,
-				  each_ref_fn fn, void *cb_data);
 
 int head_ref_namespaced(each_ref_fn fn, void *cb_data);
 int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
diff --git a/submodule.c b/submodule.c
index e072036e79..98e1f9d3c7 100644
--- a/submodule.c
+++ b/submodule.c
@@ -69,6 +69,13 @@ int is_staging_gitmodules_ok(const struct index_state *istate)
 	return 1;
 }
 
+static int for_each_remote_ref_submodule(const char *submodule,
+					 each_ref_fn fn, void *cb_data)
+{
+	return refs_for_each_remote_ref(get_submodule_ref_store(submodule),
+					fn, cb_data);
+}
+
 /*
  * Try to update the "path" entry in the "submodule.<name>" section of the
  * .gitmodules file. Return 0 only if a .gitmodules file was found, a section
-- 
2.11.0.157.gd943d85

[PATCH v4 10/16] refs: remove dead for_each_*_submodule()

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

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 <pclouds@gmail.com>
---
 Documentation/technical/api-ref-iteration.txt |  7 ++----
 refs.c                                        | 33 ---------------------------
 refs.h                                        | 10 --------
 3 files changed, 2 insertions(+), 48 deletions(-)

diff --git a/Documentation/technical/api-ref-iteration.txt b/Documentation/technical/api-ref-iteration.txt
index 37379d8337..46c3d5c355 100644
--- a/Documentation/technical/api-ref-iteration.txt
+++ b/Documentation/technical/api-ref-iteration.txt
@@ -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.
 
diff --git a/refs.c b/refs.c
index 7fa19e9309..8c989ffec7 100644
--- a/refs.c
+++ b/refs.c
@@ -336,12 +336,6 @@ int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data);
 }
 
-int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_tag_ref(get_submodule_ref_store(submodule),
-				     fn, cb_data);
-}
-
 int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data);
@@ -352,12 +346,6 @@ int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data);
 }
 
-int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_branch_ref(get_submodule_ref_store(submodule),
-					fn, cb_data);
-}
-
 int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 {
 	return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
@@ -1254,15 +1242,6 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data)
 	return 0;
 }
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	struct ref_store *refs = get_submodule_ref_store(submodule);
-
-	if (!refs)
-		return -1;
-	return refs_head_ref(refs, fn, cb_data);
-}
-
 int head_ref(each_ref_fn fn, void *cb_data)
 {
 	return refs_head_ref(get_main_ref_store(), fn, cb_data);
@@ -1323,11 +1302,6 @@ int for_each_ref(each_ref_fn fn, void *cb_data)
 	return refs_for_each_ref(get_main_ref_store(), fn, cb_data);
 }
 
-int for_each_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_ref(get_submodule_ref_store(submodule), fn, cb_data);
-}
-
 int refs_for_each_ref_in(struct ref_store *refs, const char *prefix,
 			 each_ref_fn fn, void *cb_data)
 {
@@ -1349,13 +1323,6 @@ int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data, unsig
 			       prefix, fn, 0, flag, cb_data);
 }
 
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-			      each_ref_fn fn, void *cb_data)
-{
-	return refs_for_each_ref_in(get_submodule_ref_store(submodule),
-				    prefix, fn, cb_data);
-}
-
 int refs_for_each_fullref_in(struct ref_store *refs, const char *prefix,
 			     each_ref_fn fn, void *cb_data,
 			     unsigned int broken)
diff --git a/refs.h b/refs.h
index 5d25da227a..78a26400b6 100644
--- a/refs.h
+++ b/refs.h
@@ -304,16 +304,6 @@ int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data);
 int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
 			 const char *prefix, void *cb_data);
 
-int head_ref_submodule(const char *submodule, each_ref_fn fn, void *cb_data);
-int for_each_ref_submodule(const char *submodule,
-			   each_ref_fn fn, void *cb_data);
-int for_each_ref_in_submodule(const char *submodule, const char *prefix,
-			      each_ref_fn fn, void *cb_data);
-int for_each_tag_ref_submodule(const char *submodule,
-			       each_ref_fn fn, void *cb_data);
-int for_each_branch_ref_submodule(const char *submodule,
-				  each_ref_fn fn, void *cb_data);
-
 int head_ref_namespaced(each_ref_fn fn, void *cb_data);
 int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
 
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 10/16] refs: remove dead for_each_*_submodule()

[Stefan Beller]

On Wed, Aug 23, 2017 at 5:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com>

The refactoring up to this patch looks good to me.

Thanks,
Stefan

Re: [PATCH v4 10/16] refs: remove dead for_each_*_submodule()

[Michael Haggerty]

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 <pclouds@gmail.com>
> ---
>  Documentation/technical/api-ref-iteration.txt |  7 ++----
>  refs.c                                        | 33 ---------------------------
>  refs.h                                        | 10 --------
>  3 files changed, 2 insertions(+), 48 deletions(-)

What a lovely diffstat :-)

Michael

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

[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 <pclouds@gmail.com>
---
 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(&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);
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(&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 */
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 = get_worktree_ref_store(wt);
+		ret = refs_head_ref(refs, fn, cb_data);
+		if (ret)
+			break;
+	}
+	free_worktrees(worktrees);
+	return ret;
+}
diff --git a/worktree.h b/worktree.h
index 5ea5e503fb..9276c81ae7 100644
--- a/worktree.h
+++ b/worktree.h
@@ -1,6 +1,8 @@
 #ifndef WORKTREE_H
 #define WORKTREE_H
 
+#include "refs.h"
+
 struct worktree {
 	char *path;
 	char *id;
@@ -70,6 +72,12 @@ extern void free_worktrees(struct worktree **);
 extern const struct worktree *find_shared_symref(const char *symref,
 						 const char *target);
 
+/*
+ * Similar to head_ref() for all HEADs _except_ one from the current
+ * worktree, which is covered by head_ref().
+ */
+int other_head_refs(each_ref_fn fn, void *cb_data);
+
 int is_worktree_being_rebased(const struct worktree *wt, const char *target);
 int is_worktree_being_bisected(const struct worktree *wt, const char *target);
 
-- 
2.11.0.157.gd943d85

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.

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 <sbeller@google.com> 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]

On Wed, Sep 6, 2017 at 4:19 AM, Duy Nguyen <pclouds@gmail.com> 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

[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

[PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog

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

refs/bisect is unfortunately per-worktree, so we need to look in
per-worktree logs/refs/bisect in addition to per-repo logs/refs. The
current iterator only goes through per-repo logs/refs.

Use merge iterator to walk two ref stores at the same time and pick
per-worktree refs from the right iterator.

PS. Note the unsorted order of for_each_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 <pclouds@gmail.com>
---
 refs/files-backend.c          | 59 +++++++++++++++++++++++++++++++++----------
 t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++
 2 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5cca55510b..d4d22882ef 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -106,15 +106,6 @@ static void files_reflog_path(struct files_ref_store *refs,
 			      struct strbuf *sb,
 			      const char *refname)
 {
-	if (!refname) {
-		/*
-		 * FIXME: of course this is wrong in multi worktree
-		 * setting. To be fixed real soon.
-		 */
-		strbuf_addf(sb, "%s/logs", refs->gitcommondir);
-		return;
-	}
-
 	switch (ref_type(refname)) {
 	case REF_TYPE_PER_WORKTREE:
 	case REF_TYPE_PSEUDOREF:
@@ -2055,23 +2046,63 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {
 	files_reflog_iterator_abort
 };
 
-static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
+static struct ref_iterator *reflog_iterator_begin(struct ref_store *ref_store,
+						  const char *gitdir)
 {
-	struct files_ref_store *refs =
-		files_downcast(ref_store, REF_STORE_READ,
-			       "reflog_iterator_begin");
 	struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
 	struct ref_iterator *ref_iterator = &iter->base;
 	struct strbuf sb = STRBUF_INIT;
 
 	base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
-	files_reflog_path(refs, &sb, NULL);
+	strbuf_addf(&sb, "%s/logs", gitdir);
 	iter->dir_iterator = dir_iterator_begin(sb.buf);
 	iter->ref_store = ref_store;
 	strbuf_release(&sb);
+
 	return ref_iterator;
 }
 
+static enum iterator_selection reflog_iterator_select(
+	struct ref_iterator *iter_worktree,
+	struct ref_iterator *iter_common,
+	void *cb_data)
+{
+	if (iter_worktree) {
+		/*
+		 * We're a bit loose here. We probably should ignore
+		 * common refs if they are accidentally added as
+		 * per-worktree refs.
+		 */
+		return ITER_SELECT_0;
+	} else if (iter_common) {
+		if (ref_type(iter_common->refname) == REF_TYPE_NORMAL)
+			return ITER_SELECT_1;
+
+		/*
+		 * The main ref store may contain main worktree's
+		 * per-worktree refs, which should be ignored
+		 */
+		return ITER_SKIP_1;
+	} else
+		return ITER_DONE;
+}
+
+static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
+{
+	struct files_ref_store *refs =
+		files_downcast(ref_store, REF_STORE_READ,
+			       "reflog_iterator_begin");
+
+	if (!strcmp(refs->gitdir, refs->gitcommondir)) {
+		return reflog_iterator_begin(ref_store, refs->gitcommondir);
+	} else {
+		return merge_ref_iterator_begin(
+			reflog_iterator_begin(ref_store, refs->gitdir),
+			reflog_iterator_begin(ref_store, refs->gitcommondir),
+			reflog_iterator_select, refs);
+	}
+}
+
 /*
  * If update is a direct update of head_ref (the reference pointed to
  * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.
diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
index 5df06f3556..8842d0329f 100755
--- a/t/t1407-worktree-ref-store.sh
+++ b/t/t1407-worktree-ref-store.sh
@@ -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
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog

[Richard Maw]

On Wed, Aug 23, 2017 at 07:37:00PM +0700, Nguyễn Thái Ngọc Duy wrote:
> 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 <pclouds@gmail.com>
> ---
>  refs/files-backend.c          | 59 +++++++++++++++++++++++++++++++++----------
>  t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++
>  2 files changed, 75 insertions(+), 14 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 5cca55510b..d4d22882ef 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2055,23 +2046,63 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {
<snip>
> +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 :).

Re: [PATCH v4 12/16] files-backend: make reflog iterator go through per-worktree reflog

[Michael Haggerty]

On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
> 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 <pclouds@gmail.com>
> ---
>  refs/files-backend.c          | 59 +++++++++++++++++++++++++++++++++----------
>  t/t1407-worktree-ref-store.sh | 30 ++++++++++++++++++++++
>  2 files changed, 75 insertions(+), 14 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 5cca55510b..d4d22882ef 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> +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?

> [...]

Michael

[PATCH v4 13/16] revision.c: --reflog add HEAD reflog from all worktrees

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

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 <pclouds@gmail.com>
---
 revision.c       | 28 +++++++++++++++++++++++++++-
 t/t5304-prune.sh | 16 ++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 0e98444857..d100b3a3be 100644
--- a/revision.c
+++ b/revision.c
@@ -1132,6 +1132,7 @@ struct all_refs_cb {
 	int warned_bad_reflog;
 	struct rev_info *all_revs;
 	const char *name_for_errormsg;
+	struct ref_store *refs;
 };
 
 int ref_excluded(struct string_list *ref_excludes, const char *path)
@@ -1168,6 +1169,7 @@ static void init_all_refs_cb(struct all_refs_cb *cb, struct rev_info *revs,
 	cb->all_revs = revs;
 	cb->all_flags = flags;
 	revs->rev_input_given = 1;
+	cb->refs = NULL;
 }
 
 void clear_ref_exclusion(struct string_list **ref_excludes_p)
@@ -1236,17 +1238,41 @@ static int handle_one_reflog(const char *path, const struct object_id *oid,
 	struct all_refs_cb *cb = cb_data;
 	cb->warned_bad_reflog = 0;
 	cb->name_for_errormsg = path;
-	for_each_reflog_ent(path, handle_one_reflog_ent, cb_data);
+	refs_for_each_reflog_ent(cb->refs, path,
+				 handle_one_reflog_ent, cb_data);
 	return 0;
 }
 
+static void add_other_reflogs_to_pending(struct all_refs_cb *cb)
+{
+	struct worktree **worktrees, **p;
+
+	worktrees = get_worktrees(0);
+	for (p = worktrees; *p; p++) {
+		struct worktree *wt = *p;
+
+		if (wt->is_current)
+			continue;
+
+		cb->refs = get_worktree_ref_store(wt);
+		refs_for_each_reflog(cb->refs,
+				     handle_one_reflog,
+				     cb);
+	}
+	free_worktrees(worktrees);
+}
+
 void add_reflogs_to_pending(struct rev_info *revs, unsigned flags)
 {
 	struct all_refs_cb cb;
 
 	cb.all_revs = revs;
 	cb.all_flags = flags;
+	cb.refs = get_main_ref_store();
 	for_each_reflog(handle_one_reflog, &cb);
+
+	if (!revs->single_worktree)
+		add_other_reflogs_to_pending(&cb);
 }
 
 static void add_cache_tree(struct cache_tree *it, struct rev_info *revs,
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 683bdb031c..6694c19a1e 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -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
-- 
2.11.0.157.gd943d85

[PATCH v4 14/16] rev-list: expose and document --single-worktree

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/rev-list-options.txt | 8 ++++++++
 revision.c                         | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a6cf9eb380..7d860bfca1 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -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.
diff --git a/revision.c b/revision.c
index d100b3a3be..6eba4131b4 100644
--- a/revision.c
+++ b/revision.c
@@ -2251,6 +2251,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
 			return error("invalid argument to --no-walk");
 	} else if (!strcmp(arg, "--do-walk")) {
 		revs->no_walk = 0;
+	} else if (!strcmp(arg, "--single-worktree")) {
+		revs->single_worktree = 1;
 	} else {
 		return 0;
 	}
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 14/16] rev-list: expose and document --single-worktree

[Stefan Beller]

On Wed, Aug 23, 2017 at 5:37 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/rev-list-options.txt | 8 ++++++++
>  revision.c                         | 2 ++
>  2 files changed, 10 insertions(+)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a6cf9eb380..7d860bfca1 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -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.
> diff --git a/revision.c b/revision.c
> index d100b3a3be..6eba4131b4 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2251,6 +2251,8 @@ static int handle_revision_pseudo_opt(const char *submodule,
>                         return error("invalid argument to --no-walk");
>         } else if (!strcmp(arg, "--do-walk")) {
>                 revs->no_walk = 0;
> +       } else if (!strcmp(arg, "--single-worktree")) {
> +               revs->single_worktree = 1;

This is in handle_revision_pseudo_opt, that has the note

/*
* NOTE!
*
* Commands like "git shortlog" will not accept the options below
* unless parse_revision_opt queues them (as opposed to erroring
* out).
*
* When implementing your new pseudo-option, remember to
* register it in the list at the top of handle_revision_opt.
*/

The registration needs to be done at around line 1700.

But come to think of it, is it really a pseudo opt?
Could it be a "real" (non pseudo) opt in handle_revision_opt?
The reasoning (either way) would be of interest in the
commit message, IMHO.

[PATCH v4 15/16] refs.c: remove fallback-to-main-store code get_submodule_ref_store()

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

At this state, there are three get_submodule_ref_store() callers:

 - for_each_remote_ref_submodule()
 - handle_revision_pseudo_opt()
 - resolve_gitlink_ref()

The first two deal explicitly with submodules (and we should never fall
back to the main ref store as a result). They are only called from
submodule.c:

 - find_first_merges()
 - submodule_needs_pushing()
 - push_submodule()

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 <pclouds@gmail.com>
---
 refs.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 8c989ffec7..a0c5078901 100644
--- a/refs.c
+++ b/refs.c
@@ -1587,6 +1587,9 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	char *to_free = NULL;
 	size_t len;
 
+	if (!submodule)
+		return NULL;
+
 	if (submodule) {
 		len = strlen(submodule);
 		while (len && is_dir_sep(submodule[len - 1]))
@@ -1595,14 +1598,6 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 			return NULL;
 	}
 
-	if (!submodule || !*submodule) {
-		/*
-		 * FIXME: This case is ideally not allowed. But that
-		 * can't happen until we clean up all the callers.
-		 */
-		return get_main_ref_store();
-	}
-
 	if (submodule[len])
 		/* We need to strip off one or more trailing slashes */
 		submodule = to_free = xmemdupz(submodule, len);
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 15/16] refs.c: remove fallback-to-main-store code get_submodule_ref_store()

[Michael Haggerty]

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.

Nice! Thanks for the cleanup.

Michael

[PATCH v4 16/16] refs.c: reindent get_submodule_ref_store()

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

With the new "if (!submodule) return NULL;" code added in the previous
commit, we don't need to check if submodule is not NULL anymore.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index a0c5078901..206af61d62 100644
--- a/refs.c
+++ b/refs.c
@@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
 	if (!submodule)
 		return NULL;
 
-	if (submodule) {
-		len = strlen(submodule);
-		while (len && is_dir_sep(submodule[len - 1]))
-			len--;
-		if (!len)
-			return NULL;
-	}
+	len = strlen(submodule);
+	while (len && is_dir_sep(submodule[len - 1]))
+		len--;
+	if (!len)
+		return NULL;
 
 	if (submodule[len])
 		/* We need to strip off one or more trailing slashes */
-- 
2.11.0.157.gd943d85

Re: [PATCH v4 16/16] refs.c: reindent get_submodule_ref_store()

[Michael Haggerty]

On 08/23/2017 02:37 PM, Nguyễn Thái Ngọc Duy wrote:
> 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 <pclouds@gmail.com>
> ---
>  refs.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index a0c5078901..206af61d62 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1590,13 +1590,11 @@ struct ref_store *get_submodule_ref_store(const char *submodule)
>  	if (!submodule)
>  		return NULL;
>  
> -	if (submodule) {
> -		len = strlen(submodule);
> -		while (len && is_dir_sep(submodule[len - 1]))
> -			len--;
> -		if (!len)
> -			return NULL;
> -	}
> +	len = strlen(submodule);
> +	while (len && is_dir_sep(submodule[len - 1]))
> +		len--;
> +	if (!len)
> +		return NULL;
>  
>  	if (submodule[len])
>  		/* We need to strip off one or more trailing slashes */
> 

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

Re: [PATCH v4 00/16] Fix git-gc losing objects in multi worktree

[Michael J Gruber]

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

Re: [PATCH v4 00/16] Fix git-gc losing objects in multi worktree

[Duy Nguyen]

On Fri, Aug 25, 2017 at 6:21 PM, Michael J Gruber <git@grubix.eu> 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.
-- 
Duy

Re: [PATCH v4 00/16] Fix git-gc losing objects in multi worktree

[Michael Haggerty]

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