Minor fixes on 'git worktree'

PATCH Superseded Nguyễn Thái Ngọc Duy 2 participants 19 messages Nov 22, 2016 – Nov 28, 2016

lore

Share & tools

lore

Nguyễn Thái Ngọc Duy

author

2016-11-22 10:01 UTC #0 lore raw

cover Minor fixes on 'git worktree'

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

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-22 10:01 UTC #1 lore raw

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

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 <[email protected]>

---
 worktree.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

worktree.c +2 ~~-12~~

index f7869f8..f7c1b5e 100644
@@ -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);

Signature · 1 line

2.8.2.524.g6ff3d78

Junio C Hamano

2016-11-23 16:52 UTC #2 lore raw

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

Quoted 5 lines · 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 <[email protected]>
---

Looks sensible. Thanks.

Quoted 43 lines

 worktree.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

worktree.c +2 ~~-12~~

index f7869f8..f7c1b5e 100644
@@ -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);

Nguyễn Thái Ngọc Duy

author

2016-11-22 10:01 UTC #3 lore raw

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

In reply to 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 <[email protected]>

---
 builtin/worktree.c | 8 +++++---
 worktree.c         | 6 ++----
 2 files changed, 7 insertions(+), 7 deletions(-)

builtin/worktree.c +5 -3

index 5c4854d..b835b91 100644
@@ -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);

worktree.c +2 -4

index f7c1b5e..a674efa 100644
@@ -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);

Signature · 1 line

2.8.2.524.g6ff3d78

Junio C Hamano

2016-11-23 17:06 UTC #4 lore raw

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

builtin/worktree.c static void show_worktree_porcelain(struct worktree *wt) · line 392

Quoted 11 lines · Nguyễn Thái Ngọc Duy

builtin/worktree.c +1 -1

index 5c4854d..b835b91 100644
@@ -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.

builtin/worktree.c static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) · line 415

Quoted 13 lines

builtin/worktree.c +4 -2

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

Quoted 14 lines

worktree.c +1 -1

index f7c1b5e..a674efa 100644
@@ -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);

worktree.c static struct worktree *get_main_worktree(void) · line 98

Quoted 2 lines

worktree.c +0 -0

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

worktree.c static struct worktree *get_main_worktree(void) · line 102

Quoted 4 lines

worktree.c +0 -1

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

worktree.c struct worktree **get_worktrees(void) · line 175

Quoted 7 lines

worktree.c +1 -2

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

Nguyễn Thái Ngọc Duy

author

2016-11-22 10:01 UTC #5 lore raw

PATCH 3/3 worktree list: keep the list sorted

In reply to 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 <[email protected]>

---
 builtin/worktree.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

builtin/worktree.c +17 -1

index b835b91..80ccc51 100644
@@ -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]);

Signature · 1 line

2.8.2.524.g6ff3d78

Junio C Hamano

2016-11-23 17:16 UTC #6 lore raw

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

builtin/worktree.c static int list(int ac, const char **av, const char *prefix) · line 471

Quoted 9 lines · Nguyễn Thái Ngọc Duy

builtin/worktree.c +9 -0

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

Duy Nguyen

author

2016-11-25 12:20 UTC #7 lore raw

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

Quoted 2 lines · Junio C Hamano

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

Quoted 5 lines

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.

Signature · 1 line

Duy

Junio C Hamano

2016-11-23 16:52 UTC #8 lore raw

In reply to Nguyễn Thái Ngọc Duy

Quoted 12 lines · 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

Quoted 3 lines

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

Any tests?

Duy Nguyen

author

2016-11-25 12:25 UTC #9 lore raw

Quoted 14 lines · Junio C Hamano

Quoted 12 lines · 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

Quoted 5 lines

Quoted 3 lines

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

Signature · 1 line

Duy

Duy Nguyen

author

2016-11-25 13:45 UTC #10 lore raw

Quoted 6 lines · Duy Nguyen

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.

Signature · 1 line

Duy

Junio C Hamano

2016-11-28 19:36 UTC #11 lore raw

Quoted 5 lines · Duy Nguyen

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.

Quoted 5 lines

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.

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:37 UTC #12 lore raw

v2 cover nd/worktree-list-fixup

In reply to 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(-)

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:37 UTC #13 lore raw

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

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 <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 worktree.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

worktree.c +2 ~~-12~~

index f7869f8..f7c1b5e 100644
@@ -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);

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:37 UTC #14 lore raw

PATCH v2 2/5 worktree: reorder an if statement

In reply to 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 <[email protected]>

---
 builtin/worktree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

builtin/worktree.c +3 -3

index 5c4854d..8a654e4 100644
@@ -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);

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:38 UTC #15 lore raw

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

In reply to 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 plumbing mode, we do not show neither 'bare', 'detached' or 'branch ...', which is possible by the format description if I read it right.

(*) 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 <[email protected]>

---
 builtin/worktree.c       |  6 ++++--
 t/t2027-worktree-list.sh | 21 +++++++++++++++++++++
 worktree.c               | 10 +++-------
 3 files changed, 28 insertions(+), 9 deletions(-)

builtin/worktree.c +4 -2

index 8a654e4..b835b91 100644
@@ -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);

t/t2027-worktree-list.sh +21 -0

index 1b1b65a..98b5f34 100755
@@ -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

worktree.c +3 -7

index f7c1b5e..3145522 100644
@@ -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);

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:38 UTC #16 lore raw

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

In reply to 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 <[email protected]>

---
 branch.c           | 2 +-
 builtin/branch.c   | 2 +-
 builtin/worktree.c | 6 +++---
 worktree.c         | 4 ++--
 worktree.h         | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

branch.c +1 -1

index 0d459b3..c431cbf 100644
@@ -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++) {

builtin/branch.c +1 -1

index 60cc5c8..4757075 100644
@@ -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++) {

builtin/worktree.c +3 -3

index b835b91..d7d195c 100644
@@ -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]);

worktree.c +2 -2

index 3145522..ead088e 100644
@@ -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];

worktree.h +1 -1

index 90e1311..2e68d4a 100644
@@ -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.

Signature · 1 line

2.8.2.524.g6ff3d78

Nguyễn Thái Ngọc Duy

author

2016-11-28 09:38 UTC #17 lore raw

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

In reply to 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 <[email protected]>

---
 builtin/worktree.c       |  2 +-
 t/t2027-worktree-list.sh | 19 +++++++++++++++++++
 worktree.c               | 14 ++++++++++++++
 worktree.h               |  2 ++
 4 files changed, 36 insertions(+), 1 deletion(-)

builtin/worktree.c +1 -1

index d7d195c..9a97e37 100644
@@ -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)

t/t2027-worktree-list.sh +19 -0

index 98b5f34..465eeea 100755
@@ -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

worktree.c +14 -0

index ead088e..eb61212 100644
@@ -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;
 }

worktree.h +2 -0

index 2e68d4a..d59ce1f 100644
@@ -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

Signature · 1 line

2.8.2.524.g6ff3d78

Junio C Hamano

2016-11-28 21:26 UTC #18 lore raw

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

In reply to Nguyễn Thái Ngọc Duy

Quoted 18 lines · 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

Quoted 7 lines

 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.