[PATCH 0/3] Minor fixes on 'git worktree'

[PATCH 0/3] Minor fixes on 'git worktree'

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

This fixes two things:

 - make sure the first item is always the main worktree even if we
   fail to retrieve some info

 - keep 'worktree list' order stable (which in turn fixes the random
   failure on my 'worktree-move' series

Nguyễn Thái Ngọc Duy (3):
  worktree.c: zero new 'struct worktree' on allocation
  get_worktrees() must return main worktree as first item even on error
  worktree list: keep the list sorted

 builtin/worktree.c | 26 ++++++++++++++++++++++----
 worktree.c         | 20 ++++----------------
 2 files changed, 26 insertions(+), 20 deletions(-)

-- 
2.8.2.524.g6ff3d78

[PATCH 1/3] worktree.c: zero new 'struct worktree' on allocation

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

This keeps things a bit simpler when we add more fields, knowing that
default values are always zero.

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

diff --git a/worktree.c b/worktree.c
index f7869f8..f7c1b5e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
 
-	worktree = xmalloc(sizeof(struct worktree));
+	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
-	worktree->id = NULL;
 	worktree->is_bare = is_bare;
-	worktree->head_ref = NULL;
 	worktree->is_detached = is_detached;
-	worktree->is_current = 0;
 	add_head_info(&head_ref, worktree);
-	worktree->lock_reason = NULL;
-	worktree->lock_reason_valid = 0;
 
 done:
 	strbuf_release(&path);
@@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char *id)
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
 
-	worktree = xmalloc(sizeof(struct worktree));
+	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
 	worktree->id = xstrdup(id);
-	worktree->is_bare = 0;
-	worktree->head_ref = NULL;
 	worktree->is_detached = is_detached;
-	worktree->is_current = 0;
 	add_head_info(&head_ref, worktree);
-	worktree->lock_reason = NULL;
-	worktree->lock_reason_valid = 0;
 
 done:
 	strbuf_release(&path);
-- 
2.8.2.524.g6ff3d78

Re: [PATCH 1/3] worktree.c: zero new 'struct worktree' on allocation

[Junio C Hamano]

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

> This keeps things a bit simpler when we add more fields, knowing that
> default values are always zero.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Looks sensible.  Thanks.


>  worktree.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index f7869f8..f7c1b5e 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
>  	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
>  		goto done;
>  
> -	worktree = xmalloc(sizeof(struct worktree));
> +	worktree = xcalloc(1, sizeof(*worktree));
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
> -	worktree->id = NULL;
>  	worktree->is_bare = is_bare;
> -	worktree->head_ref = NULL;
>  	worktree->is_detached = is_detached;
> -	worktree->is_current = 0;
>  	add_head_info(&head_ref, worktree);
> -	worktree->lock_reason = NULL;
> -	worktree->lock_reason_valid = 0;
>  
>  done:
>  	strbuf_release(&path);
> @@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char *id)
>  	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
>  		goto done;
>  
> -	worktree = xmalloc(sizeof(struct worktree));
> +	worktree = xcalloc(1, sizeof(*worktree));
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
>  	worktree->id = xstrdup(id);
> -	worktree->is_bare = 0;
> -	worktree->head_ref = NULL;
>  	worktree->is_detached = is_detached;
> -	worktree->is_current = 0;
>  	add_head_info(&head_ref, worktree);
> -	worktree->lock_reason = NULL;
> -	worktree->lock_reason_valid = 0;
>  
>  done:
>  	strbuf_release(&path);

[PATCH 2/3] get_worktrees() must return main worktree as first item even on error

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

This is required by git-worktree.txt, stating that the main worktree is
the first line (especially in --porcelain mode when we can't just change
behavior at will).

There's only one case when get_worktrees() may skip main worktree, when
parse_ref() fails. Update the code so that we keep first item as main
worktree and return something sensible in this case:

 - In user-friendly mode, since we're not constraint by anything,
   returning "(error)" should do the job (we already show "(detached
   HEAD)" which is not machine-friendly). Actually errors should be
   printed on stderr by parse_ref() (*)

 - In plumbing mode, we do not show neither 'bare', 'detached' or
   'branch ...', which is possible by the format description if I read
   it right.

Careful readers may realize that when the local variable "head_ref" in
get_main_worktree() is emptied, add_head_info() will do nothing to
wt->head_sha1. But that's ok because head_sha1 is zero-ized in the
previous patch.

(*) Well, it does not. But it's supposed to be a stop gap
    implementation until refs we can reuse code, to parse "ref: " stuff
    in HEAD, from resolve_refs_unsafe(). Now may be the time since
    refs refactoring is mostly done.

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

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c4854d..b835b91 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
 		printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
 		if (wt->is_detached)
 			printf("detached\n");
-		else
+		else if (wt->head_ref)
 			printf("branch %s\n", wt->head_ref);
 	}
 	printf("\n");
@@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
 	else {
 		strbuf_addf(&sb, "%-*s ", abbrev_len,
 				find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
-		if (!wt->is_detached)
+		if (wt->is_detached)
+			strbuf_addstr(&sb, "(detached HEAD)");
+		else if (wt->head_ref)
 			strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
 		else
-			strbuf_addstr(&sb, "(detached HEAD)");
+			strbuf_addstr(&sb, "(error)");
 	}
 	printf("%s\n", sb.buf);
 
diff --git a/worktree.c b/worktree.c
index f7c1b5e..a674efa 100644
--- a/worktree.c
+++ b/worktree.c
@@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
 	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
 
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
-		goto done;
+		strbuf_reset(&head_ref);
 
 	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
 	worktree->is_detached = is_detached;
 	add_head_info(&head_ref, worktree);
 
-done:
 	strbuf_release(&path);
 	strbuf_release(&worktree_path);
 	strbuf_release(&head_ref);
@@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
 
 	list = xmalloc(alloc * sizeof(struct worktree *));
 
-	if ((list[counter] = get_main_worktree()))
-		counter++;
+	list[counter++] = get_main_worktree();
 
 	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
 	dir = opendir(path.buf);
-- 
2.8.2.524.g6ff3d78

Re: [PATCH 2/3] get_worktrees() must return main worktree as first item even on error

[Junio C Hamano]

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

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 5c4854d..b835b91 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
>  		printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
>  		if (wt->is_detached)
>  			printf("detached\n");
> -		else
> +		else if (wt->head_ref)
>  			printf("branch %s\n", wt->head_ref);

This change looks somewhat unrelated to what the title and the log
message claims to do, but the fix is to indicate an error condition
by leaving wt->head_ref as NULL, so this is a necessary adjustment.

Good.

> @@ -406,10 +406,12 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
>  	else {
>  		strbuf_addf(&sb, "%-*s ", abbrev_len,
>  				find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
> -		if (!wt->is_detached)
> +		if (wt->is_detached)
> +			strbuf_addstr(&sb, "(detached HEAD)");
> +		else if (wt->head_ref)
>  			strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
>  		else
> -			strbuf_addstr(&sb, "(detached HEAD)");
> +			strbuf_addstr(&sb, "(error)");
>  	}

Likewise.

> diff --git a/worktree.c b/worktree.c
> index f7c1b5e..a674efa 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -89,7 +89,7 @@ static struct worktree *get_main_worktree(void)
>  	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
>  
>  	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
> -		goto done;
> +		strbuf_reset(&head_ref);
>  
>  	worktree = xcalloc(1, sizeof(*worktree));
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
> @@ -97,7 +97,6 @@ static struct worktree *get_main_worktree(void)
>  	worktree->is_detached = is_detached;
>  	add_head_info(&head_ref, worktree);

OK.  The earlier call to _reset() and add_head_info() function
itself may want to be clarified that a zero-length strbuf signals an
error condition with additional comment.  It is all too unclear in
the code with this patch as it stands.

> -done:
>  	strbuf_release(&path);
>  	strbuf_release(&worktree_path);
>  	strbuf_release(&head_ref);

After this there is "return worktree" which used to return NULL
because of the "goto", but we never return NULL from the function
after this change, which is the whole point of this change.  Good.

> @@ -173,8 +172,7 @@ struct worktree **get_worktrees(void)
>  
>  	list = xmalloc(alloc * sizeof(struct worktree *));
>  
> -	if ((list[counter] = get_main_worktree()))
> -		counter++;
> +	list[counter++] = get_main_worktree();

Hence the conditional, while it does not hurt, becomes unnecessary
and we can unconditionally throw the primary one to the list.

Good.

Other than the "these need in-code commenting", and also that this
should have a new test, the patch makes sense to me.

Thanks.

[PATCH 3/3] worktree list: keep the list sorted

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

It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index b835b91..80ccc51 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -434,6 +434,13 @@ static void measure_widths(struct worktree **wt, int *abbrev, int *maxlen)
 	}
 }
 
+static int compare_worktree(const void *a_, const void *b_)
+{
+	const struct worktree *const *a = a_;
+	const struct worktree *const *b = b_;
+	return fspathcmp((*a)->path, (*b)->path);
+}
+
 static int list(int ac, const char **av, const char *prefix)
 {
 	int porcelain = 0;
@@ -448,11 +455,20 @@ static int list(int ac, const char **av, const char *prefix)
 		usage_with_options(worktree_usage, options);
 	else {
 		struct worktree **worktrees = get_worktrees();
-		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
+		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i, nr;
 
 		if (!porcelain)
 			measure_widths(worktrees, &abbrev, &path_maxlen);
 
+		for (i = nr = 0; worktrees[i]; i++)
+			nr++;
+
+		/*
+		 * don't sort the first item (main worktree), which will
+		 * always be the first
+		 */
+		QSORT(worktrees + 1, nr - 1, compare_worktree);
+
 		for (i = 0; worktrees[i]; i++) {
 			if (porcelain)
 				show_worktree_porcelain(worktrees[i]);
-- 
2.8.2.524.g6ff3d78

Re: [PATCH 3/3] worktree list: keep the list sorted

[Junio C Hamano]

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

> +		for (i = nr = 0; worktrees[i]; i++)
> +			nr++;
> +
> +		/*
> +		 * don't sort the first item (main worktree), which will
> +		 * always be the first
> +		 */
> +		QSORT(worktrees + 1, nr - 1, compare_worktree);
> +

This is somewhat curious.

	for (i = 0; worktrees[i]; i++)
		; /* just counting */
	QSORT(worktrees + 1, i - 1, compare_worktree);

would have been a lot more idiomatic (you do not use nr after sorting).

More importantly, perhaps get_worktrees() should learn to take an
optional pointer to int that returns how many items are in the list?

Alternatively, other existing callers of the function do not care
about the order, so it may not be such a good idea to always sort
the result, but it may be a good idea to teach it to take a boolean
that signals that the list should be sorted in a "natural order",
which is how "worktree list" would show them to the user?

This should be easily protectable with a new test.  Please do.

Re: [PATCH 3/3] worktree list: keep the list sorted

[Duy Nguyen]

On Thu, Nov 24, 2016 at 12:16 AM, Junio C Hamano <gitster@pobox.com> wrote:
> More importantly, perhaps get_worktrees() should learn to take an
> optional pointer to int that returns how many items are in the list?

My first thought was "yeah I remember there are many counting loop
like this" then grepped and realized this is the only place. For now I
think it's ok to leave it as is. When the second place does the same,
we'll change get_worktrees()

> Alternatively, other existing callers of the function do not care
> about the order, so it may not be such a good idea to always sort
> the result, but it may be a good idea to teach it to take a boolean
> that signals that the list should be sorted in a "natural order",
> which is how "worktree list" would show them to the user?

OK. I'll make it a bit flag, not a boolean though, to avoid changing
all call sites next time.
-- 
Duy

Re: [PATCH 0/3] Minor fixes on 'git worktree'

[Junio C Hamano]

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

> This fixes two things:
>
>  - make sure the first item is always the main worktree even if we
>    fail to retrieve some info
>
>  - keep 'worktree list' order stable (which in turn fixes the random
>    failure on my 'worktree-move' series
> Nguyễn Thái Ngọc Duy (3):
>   worktree.c: zero new 'struct worktree' on allocation
>   get_worktrees() must return main worktree as first item even on error
>   worktree list: keep the list sorted
>
>  builtin/worktree.c | 26 ++++++++++++++++++++++----
>  worktree.c         | 20 ++++----------------
>  2 files changed, 26 insertions(+), 20 deletions(-)

Any tests?

Re: [PATCH 0/3] Minor fixes on 'git worktree'

[Duy Nguyen]

On Wed, Nov 23, 2016 at 11:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This fixes two things:
>>
>>  - make sure the first item is always the main worktree even if we
>>    fail to retrieve some info
>>
>>  - keep 'worktree list' order stable (which in turn fixes the random
>>    failure on my 'worktree-move' series
>> Nguyễn Thái Ngọc Duy (3):
>>   worktree.c: zero new 'struct worktree' on allocation
>>   get_worktrees() must return main worktree as first item even on error
>>   worktree list: keep the list sorted
>>
>>  builtin/worktree.c | 26 ++++++++++++++++++++++----
>>  worktree.c         | 20 ++++----------------
>>  2 files changed, 26 insertions(+), 20 deletions(-)
>
> Any tests?

I discarded the idea of adding test for sorting because it relies on
filesystems. A passed test may just mean the filesystem happens to
return files in "good" order. But I guess a test wouldn't hurt.
Somewhere out there some user may still have a "bad" filesystem and
the test could help catch breakage then.

Adding the test for the failed parse_ref() is possible, I think. But
since that function is destined to die, as I promised to use
refs-provided api instead of rolling out a custom ref parser, and I'm
going to have another hard look at refs subsystem for the
gc-not-looking-at-worktree-refs problem, may I make another promise to
add tests after this function is gone? It should happen "soon".
-- 
Duy

Re: [PATCH 0/3] Minor fixes on 'git worktree'

[Duy Nguyen]

On Fri, Nov 25, 2016 at 7:24 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> Adding the test for the failed parse_ref() is possible, I think. But
> since that function is destined to die, as I promised to use
> refs-provided api instead of rolling out a custom ref parser, and I'm
> going to have another hard look at refs subsystem for the
> gc-not-looking-at-worktree-refs problem, may I make another promise to
> add tests after this function is gone?

Never mind. I went ahead and made a test case anyway because why not.

But that's not worth an email. An interesting thing maybe worth
sharing is, if HEAD is broken (the only reason we would fail to create
"struct worktree" for main worktree), then "git worktree list" from
main worktree would fail too, because repo setup code fails to parse
HEAD as well and die(). That makes the "always show main worktree"
patch useless, since we won't get far enough to execute
get_worktrees() in the first place. However, you can still do "git
worktree list" from a linked worktree, gitdir setup code will not stop
you (how can it). And the patch is still needed.
-- 
Duy

Re: [PATCH 0/3] Minor fixes on 'git worktree'

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> An interesting thing maybe worth
> sharing is, if HEAD is broken (the only reason we would fail to create
> "struct worktree" for main worktree), then "git worktree list" from
> main worktree would fail too, because repo setup code fails to parse
> HEAD as well and die().

Yeah, that makes sense.

> That makes the "always show main worktree"
> patch useless, since we won't get far enough to execute
> get_worktrees() in the first place. However, you can still do "git
> worktree list" from a linked worktree, gitdir setup code will not stop
> you (how can it). And the patch is still needed.

Yes.

[PATCH v2 0/5] nd/worktree-list-fixup

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

This version

* changes get_worktrees() to take a flag, and adds one flag for
  sorting.

* adds tests for both the 'main worktree always present' and the
  sorting problems.

* reworks 3/5 a bit, keep changes closer, easier to see the cause and
  consequence.

Nguyễn Thái Ngọc Duy (5):
  worktree.c: zero new 'struct worktree' on allocation
  worktree: reorder an if statement
  get_worktrees() must return main worktree as first item even on error
  worktree.c: get_worktrees() takes a new flag argument
  worktree list: keep the list sorted

 branch.c                 |  2 +-
 builtin/branch.c         |  2 +-
 builtin/worktree.c       | 14 ++++++++------
 t/t2027-worktree-list.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 worktree.c               | 42 +++++++++++++++++++++---------------------
 worktree.h               |  4 +++-
 6 files changed, 74 insertions(+), 30 deletions(-)

-- 
2.8.2.524.g6ff3d78

[PATCH v2 1/5] worktree.c: zero new 'struct worktree' on allocation

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

This keeps things a bit simpler when we add more fields, knowing that
default values are always zero.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 worktree.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/worktree.c b/worktree.c
index f7869f8..f7c1b5e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -91,16 +91,11 @@ static struct worktree *get_main_worktree(void)
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
 
-	worktree = xmalloc(sizeof(struct worktree));
+	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
-	worktree->id = NULL;
 	worktree->is_bare = is_bare;
-	worktree->head_ref = NULL;
 	worktree->is_detached = is_detached;
-	worktree->is_current = 0;
 	add_head_info(&head_ref, worktree);
-	worktree->lock_reason = NULL;
-	worktree->lock_reason_valid = 0;
 
 done:
 	strbuf_release(&path);
@@ -138,16 +133,11 @@ static struct worktree *get_linked_worktree(const char *id)
 	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
 		goto done;
 
-	worktree = xmalloc(sizeof(struct worktree));
+	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
 	worktree->id = xstrdup(id);
-	worktree->is_bare = 0;
-	worktree->head_ref = NULL;
 	worktree->is_detached = is_detached;
-	worktree->is_current = 0;
 	add_head_info(&head_ref, worktree);
-	worktree->lock_reason = NULL;
-	worktree->lock_reason_valid = 0;
 
 done:
 	strbuf_release(&path);
-- 
2.8.2.524.g6ff3d78

[PATCH v2 2/5] worktree: reorder an if statement

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

This is no-op. But it helps reduce diff noise in the next patch.

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

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c4854d..8a654e4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -406,10 +406,10 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
 	else {
 		strbuf_addf(&sb, "%-*s ", abbrev_len,
 				find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
-		if (!wt->is_detached)
-			strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
-		else
+		if (wt->is_detached)
 			strbuf_addstr(&sb, "(detached HEAD)");
+		else
+			strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
 	}
 	printf("%s\n", sb.buf);
 
-- 
2.8.2.524.g6ff3d78

[PATCH v2 3/5] get_worktrees() must return main worktree as first item even on error

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

This is required by git-worktree.txt, stating that the main worktree is
the first line (especially in --porcelain mode when we can't just change
behavior at will).

There's only one case when get_worktrees() may skip main worktree, when
parse_ref() fails. Update the code so that we keep first item as main
worktree and return something sensible in this case:

 - In user-friendly mode, since we're not constraint by anything,
   returning "(error)" should do the job (we already show "(detached
   HEAD)" which is not machine-friendly). Actually errors should be
   printed on stderr by parse_ref() (*)

 - In plumbing mode, we do not show neither 'bare', 'detached' or
   'branch ...', which is possible by the format description if I read
   it right.

Careful readers may realize that when the local variable "head_ref" in
get_main_worktree() is emptied, add_head_info() will do nothing to
wt->head_sha1. But that's ok because head_sha1 is zero-ized in the
previous patch.

(*) Well, it does not. But it's supposed to be a stop gap implementation
    until we can reuse refs code to parse "ref: " stuff in HEAD, from
    resolve_refs_unsafe(). Now may be the time since refs refactoring is
    mostly done.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c       |  6 ++++--
 t/t2027-worktree-list.sh | 21 +++++++++++++++++++++
 worktree.c               | 10 +++-------
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 8a654e4..b835b91 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -388,7 +388,7 @@ static void show_worktree_porcelain(struct worktree *wt)
 		printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
 		if (wt->is_detached)
 			printf("detached\n");
-		else
+		else if (wt->head_ref)
 			printf("branch %s\n", wt->head_ref);
 	}
 	printf("\n");
@@ -408,8 +408,10 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
 				find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
 		if (wt->is_detached)
 			strbuf_addstr(&sb, "(detached HEAD)");
-		else
+		else if (wt->head_ref)
 			strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(wt->head_ref, 0));
+		else
+			strbuf_addstr(&sb, "(error)");
 	}
 	printf("%s\n", sb.buf);
 
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..98b5f34 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -96,4 +96,25 @@ test_expect_success 'bare repo cleanup' '
 	rm -rf bare1
 '
 
+test_expect_success 'broken main worktree still at the top' '
+	git init broken-main &&
+	(
+		cd broken-main &&
+		test_commit new &&
+		git worktree add linked &&
+		cat >expected <<-EOF &&
+		worktree $(pwd)
+		HEAD $_z40
+
+		EOF
+		cd linked &&
+		echo "worktree $(pwd)" >expected &&
+		echo "ref: .broken" >../.git/HEAD &&
+		git worktree list --porcelain | head -n 3 >actual &&
+		test_cmp ../expected actual &&
+		git worktree list | head -n 1 >actual.2 &&
+		grep -F "(error)" actual.2
+	)
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index f7c1b5e..3145522 100644
--- a/worktree.c
+++ b/worktree.c
@@ -88,16 +88,13 @@ static struct worktree *get_main_worktree(void)
 
 	strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
 
-	if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
-		goto done;
-
 	worktree = xcalloc(1, sizeof(*worktree));
 	worktree->path = strbuf_detach(&worktree_path, NULL);
 	worktree->is_bare = is_bare;
 	worktree->is_detached = is_detached;
-	add_head_info(&head_ref, worktree);
+	if (!parse_ref(path.buf, &head_ref, &is_detached))
+		add_head_info(&head_ref, worktree);
 
-done:
 	strbuf_release(&path);
 	strbuf_release(&worktree_path);
 	strbuf_release(&head_ref);
@@ -173,8 +170,7 @@ struct worktree **get_worktrees(void)
 
 	list = xmalloc(alloc * sizeof(struct worktree *));
 
-	if ((list[counter] = get_main_worktree()))
-		counter++;
+	list[counter++] = get_main_worktree();
 
 	strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
 	dir = opendir(path.buf);
-- 
2.8.2.524.g6ff3d78

[PATCH v2 4/5] worktree.c: get_worktrees() takes a new flag argument

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

This is another no-op patch, in preparation for get_worktrees() to do
optional things, like sorting.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c           | 2 +-
 builtin/branch.c   | 2 +-
 builtin/worktree.c | 6 +++---
 worktree.c         | 4 ++--
 worktree.h         | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/branch.c b/branch.c
index 0d459b3..c431cbf 100644
--- a/branch.c
+++ b/branch.c
@@ -348,7 +348,7 @@ void die_if_checked_out(const char *branch, int ignore_current_worktree)
 int replace_each_worktree_head_symref(const char *oldref, const char *newref)
 {
 	int ret = 0;
-	struct worktree **worktrees = get_worktrees();
+	struct worktree **worktrees = get_worktrees(0);
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
diff --git a/builtin/branch.c b/builtin/branch.c
index 60cc5c8..4757075 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -531,7 +531,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
 
 static void reject_rebase_or_bisect_branch(const char *target)
 {
-	struct worktree **worktrees = get_worktrees();
+	struct worktree **worktrees = get_worktrees(0);
 	int i;
 
 	for (i = 0; worktrees[i]; i++) {
diff --git a/builtin/worktree.c b/builtin/worktree.c
index b835b91..d7d195c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -447,7 +447,7 @@ static int list(int ac, const char **av, const char *prefix)
 	if (ac)
 		usage_with_options(worktree_usage, options);
 	else {
-		struct worktree **worktrees = get_worktrees();
+		struct worktree **worktrees = get_worktrees(0);
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
 
 		if (!porcelain)
@@ -478,7 +478,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
 	if (ac != 1)
 		usage_with_options(worktree_usage, options);
 
-	worktrees = get_worktrees();
+	worktrees = get_worktrees(0);
 	wt = find_worktree(worktrees, prefix, av[0]);
 	if (!wt)
 		die(_("'%s' is not a working tree"), av[0]);
@@ -511,7 +511,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
 	if (ac != 1)
 		usage_with_options(worktree_usage, options);
 
-	worktrees = get_worktrees();
+	worktrees = get_worktrees(0);
 	wt = find_worktree(worktrees, prefix, av[0]);
 	if (!wt)
 		die(_("'%s' is not a working tree"), av[0]);
diff --git a/worktree.c b/worktree.c
index 3145522..ead088e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -160,7 +160,7 @@ static void mark_current_worktree(struct worktree **worktrees)
 	free(git_dir);
 }
 
-struct worktree **get_worktrees(void)
+struct worktree **get_worktrees(unsigned flags)
 {
 	struct worktree **list = NULL;
 	struct strbuf path = STRBUF_INIT;
@@ -327,7 +327,7 @@ const struct worktree *find_shared_symref(const char *symref,
 
 	if (worktrees)
 		free_worktrees(worktrees);
-	worktrees = get_worktrees();
+	worktrees = get_worktrees(0);
 
 	for (i = 0; worktrees[i]; i++) {
 		struct worktree *wt = worktrees[i];
diff --git a/worktree.h b/worktree.h
index 90e1311..2e68d4a 100644
--- a/worktree.h
+++ b/worktree.h
@@ -23,7 +23,7 @@ struct worktree {
  * The caller is responsible for freeing the memory from the returned
  * worktree(s).
  */
-extern struct worktree **get_worktrees(void);
+extern struct worktree **get_worktrees(unsigned flags);
 
 /*
  * Return git dir of the worktree. Note that the path may be relative.
-- 
2.8.2.524.g6ff3d78

[PATCH v2 5/5] worktree list: keep the list sorted

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

It makes it easier to write tests for. But it should also be good for
the user since locating a worktree by eye would be easier once they
notice this.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/worktree.c       |  2 +-
 t/t2027-worktree-list.sh | 19 +++++++++++++++++++
 worktree.c               | 14 ++++++++++++++
 worktree.h               |  2 ++
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index d7d195c..9a97e37 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -447,7 +447,7 @@ static int list(int ac, const char **av, const char *prefix)
 	if (ac)
 		usage_with_options(worktree_usage, options);
 	else {
-		struct worktree **worktrees = get_worktrees(0);
+		struct worktree **worktrees = get_worktrees(GWT_SORT_LINKED);
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
 
 		if (!porcelain)
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 98b5f34..465eeea 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -117,4 +117,23 @@ test_expect_success 'broken main worktree still at the top' '
 	)
 '
 
+test_expect_success 'linked worktrees are sorted' '
+	mkdir sorted &&
+	git init sorted/main &&
+	(
+		cd sorted/main &&
+		test_tick &&
+		test_commit new &&
+		git worktree add ../first &&
+		git worktree add ../second &&
+		git worktree list --porcelain | grep ^worktree >actual
+	) &&
+	cat >expected <<-EOF &&
+	worktree $(pwd)/sorted/main
+	worktree $(pwd)/sorted/first
+	worktree $(pwd)/sorted/second
+	EOF
+	test_cmp expected sorted/main/actual
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index ead088e..eb61212 100644
--- a/worktree.c
+++ b/worktree.c
@@ -160,6 +160,13 @@ static void mark_current_worktree(struct worktree **worktrees)
 	free(git_dir);
 }
 
+static int compare_worktree(const void *a_, const void *b_)
+{
+	const struct worktree *const *a = a_;
+	const struct worktree *const *b = b_;
+	return fspathcmp((*a)->path, (*b)->path);
+}
+
 struct worktree **get_worktrees(unsigned flags)
 {
 	struct worktree **list = NULL;
@@ -191,6 +198,13 @@ struct worktree **get_worktrees(unsigned flags)
 	ALLOC_GROW(list, counter + 1, alloc);
 	list[counter] = NULL;
 
+	if (flags & GWT_SORT_LINKED)
+		/*
+		 * don't sort the first item (main worktree), which will
+		 * always be the first
+		 */
+		QSORT(list + 1, counter - 1, compare_worktree);
+
 	mark_current_worktree(list);
 	return list;
 }
diff --git a/worktree.h b/worktree.h
index 2e68d4a..d59ce1f 100644
--- a/worktree.h
+++ b/worktree.h
@@ -15,6 +15,8 @@ struct worktree {
 
 /* Functions for acting on the information about worktrees. */
 
+#define GWT_SORT_LINKED (1 << 0) /* keeps linked worktrees sorted */
+
 /*
  * Get the worktrees.  The primary worktree will always be the first returned,
  * and linked worktrees will be pointed to by 'next' in each subsequent
-- 
2.8.2.524.g6ff3d78

Re: [PATCH v2 0/5] nd/worktree-list-fixup

[Junio C Hamano]

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

> This version
>
> * changes get_worktrees() to take a flag, and adds one flag for
>   sorting.
>
> * adds tests for both the 'main worktree always present' and the
>   sorting problems.
>
> * reworks 3/5 a bit, keep changes closer, easier to see the cause and
>   consequence.
>
> Nguyễn Thái Ngọc Duy (5):
>   worktree.c: zero new 'struct worktree' on allocation
>   worktree: reorder an if statement
>   get_worktrees() must return main worktree as first item even on error
>   worktree.c: get_worktrees() takes a new flag argument
>   worktree list: keep the list sorted
>
>  branch.c                 |  2 +-
>  builtin/branch.c         |  2 +-
>  builtin/worktree.c       | 14 ++++++++------
>  t/t2027-worktree-list.sh | 40 ++++++++++++++++++++++++++++++++++++++++
>  worktree.c               | 42 +++++++++++++++++++++---------------------
>  worktree.h               |  4 +++-
>  6 files changed, 74 insertions(+), 30 deletions(-)

Thanks for a pleasant read.  Will replace what has been queued.