* [PATCH v8 1/4] worktree: add top-level worktree.c
2015-09-18 13:30 [PATCH v8 0/4] worktree: list functions and command Michael Rappazzo
@ 2015-09-18 13:30 ` Michael Rappazzo
2015-09-22 17:16 ` Junio C Hamano
2015-09-18 13:30 ` [PATCH v8 2/4] worktree: refactor find_linked_symref function Michael Rappazzo
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Michael Rappazzo @ 2015-09-18 13:30 UTC (permalink / raw)
To: gitster, sunshine, dturner; +Cc: git, Michael Rappazzo
worktree.c contains functions to work with and get information from
worktrees. This introduction moves functions related to worktrees
from branch.c into worktree.c
Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
Makefile | 1 +
branch.c | 79 +-----------------------------------------------------
branch.h | 8 ------
builtin/notes.c | 1 +
worktree.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
worktree.h | 12 +++++++++
6 files changed, 97 insertions(+), 86 deletions(-)
create mode 100644 worktree.c
create mode 100644 worktree.h
diff --git a/Makefile b/Makefile
index 8d5df7e..f4ee2d2 100644
--- a/Makefile
+++ b/Makefile
@@ -807,6 +807,7 @@ LIB_OBJS += version.o
LIB_OBJS += versioncmp.o
LIB_OBJS += walker.o
LIB_OBJS += wildmatch.o
+LIB_OBJS += worktree.o
LIB_OBJS += wrapper.o
LIB_OBJS += write_or_die.o
LIB_OBJS += ws.o
diff --git a/branch.c b/branch.c
index d013374..77d7f2a 100644
--- a/branch.c
+++ b/branch.c
@@ -4,6 +4,7 @@
#include "refs.h"
#include "remote.h"
#include "commit.h"
+#include "worktree.h"
struct tracking {
struct refspec spec;
@@ -311,84 +312,6 @@ void remove_branch_state(void)
unlink(git_path_squash_msg());
}
-static char *find_linked_symref(const char *symref, const char *branch,
- const char *id)
-{
- struct strbuf sb = STRBUF_INIT;
- struct strbuf path = STRBUF_INIT;
- struct strbuf gitdir = STRBUF_INIT;
- char *existing = NULL;
-
- /*
- * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside
- * $GIT_DIR so resolve_ref_unsafe() won't work (it uses
- * git_path). Parse the ref ourselves.
- */
- if (id)
- strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
- else
- strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
-
- if (!strbuf_readlink(&sb, path.buf, 0)) {
- if (!starts_with(sb.buf, "refs/") ||
- check_refname_format(sb.buf, 0))
- goto done;
- } else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&
- starts_with(sb.buf, "ref:")) {
- strbuf_remove(&sb, 0, strlen("ref:"));
- strbuf_trim(&sb);
- } else
- goto done;
- if (strcmp(sb.buf, branch))
- goto done;
- if (id) {
- strbuf_reset(&path);
- strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
- if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
- goto done;
- strbuf_rtrim(&gitdir);
- } else
- strbuf_addstr(&gitdir, get_git_common_dir());
- strbuf_strip_suffix(&gitdir, ".git");
-
- existing = strbuf_detach(&gitdir, NULL);
-done:
- strbuf_release(&path);
- strbuf_release(&sb);
- strbuf_release(&gitdir);
-
- return existing;
-}
-
-char *find_shared_symref(const char *symref, const char *target)
-{
- struct strbuf path = STRBUF_INIT;
- DIR *dir;
- struct dirent *d;
- char *existing;
-
- if ((existing = find_linked_symref(symref, target, NULL)))
- return existing;
-
- strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
- dir = opendir(path.buf);
- strbuf_release(&path);
- if (!dir)
- return NULL;
-
- while ((d = readdir(dir)) != NULL) {
- if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
- continue;
- existing = find_linked_symref(symref, target, d->d_name);
- if (existing)
- goto done;
- }
-done:
- closedir(dir);
-
- return existing;
-}
-
void die_if_checked_out(const char *branch)
{
char *existing;
diff --git a/branch.h b/branch.h
index d3446ed..58aa45f 100644
--- a/branch.h
+++ b/branch.h
@@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
*/
extern void die_if_checked_out(const char *branch);
-/*
- * Check if a per-worktree symref points to a ref in the main worktree
- * or any linked worktree, and return the path to the exising worktree
- * if it is. Returns NULL if there is no existing ref. The caller is
- * responsible for freeing the returned path.
- */
-extern char *find_shared_symref(const char *symref, const char *target);
-
#endif
diff --git a/builtin/notes.c b/builtin/notes.c
index 3608c64..8b30334 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -20,6 +20,7 @@
#include "notes-merge.h"
#include "notes-utils.h"
#include "branch.h"
+#include "worktree.h"
static const char * const git_notes_usage[] = {
N_("git notes [--ref <notes-ref>] [list [<object>]]"),
diff --git a/worktree.c b/worktree.c
new file mode 100644
index 0000000..10e1496
--- /dev/null
+++ b/worktree.c
@@ -0,0 +1,82 @@
+#include "cache.h"
+#include "refs.h"
+#include "strbuf.h"
+#include "worktree.h"
+
+static char *find_linked_symref(const char *symref, const char *branch,
+ const char *id)
+{
+ struct strbuf sb = STRBUF_INIT;
+ struct strbuf path = STRBUF_INIT;
+ struct strbuf gitdir = STRBUF_INIT;
+ char *existing = NULL;
+
+ /*
+ * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside
+ * $GIT_DIR so resolve_ref_unsafe() won't work (it uses
+ * git_path). Parse the ref ourselves.
+ */
+ if (id)
+ strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
+ else
+ strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
+
+ if (!strbuf_readlink(&sb, path.buf, 0)) {
+ if (!starts_with(sb.buf, "refs/") ||
+ check_refname_format(sb.buf, 0))
+ goto done;
+ } else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&
+ starts_with(sb.buf, "ref:")) {
+ strbuf_remove(&sb, 0, strlen("ref:"));
+ strbuf_trim(&sb);
+ } else
+ goto done;
+ if (strcmp(sb.buf, branch))
+ goto done;
+ if (id) {
+ strbuf_reset(&path);
+ strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
+ if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
+ goto done;
+ strbuf_rtrim(&gitdir);
+ } else
+ strbuf_addstr(&gitdir, get_git_common_dir());
+ strbuf_strip_suffix(&gitdir, ".git");
+
+ existing = strbuf_detach(&gitdir, NULL);
+done:
+ strbuf_release(&path);
+ strbuf_release(&sb);
+ strbuf_release(&gitdir);
+
+ return existing;
+}
+
+char *find_shared_symref(const char *symref, const char *target)
+{
+ struct strbuf path = STRBUF_INIT;
+ DIR *dir;
+ struct dirent *d;
+ char *existing;
+
+ if ((existing = find_linked_symref(symref, target, NULL)))
+ return existing;
+
+ strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+ dir = opendir(path.buf);
+ strbuf_release(&path);
+ if (!dir)
+ return NULL;
+
+ while ((d = readdir(dir)) != NULL) {
+ if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+ continue;
+ existing = find_linked_symref(symref, target, d->d_name);
+ if (existing)
+ goto done;
+ }
+done:
+ closedir(dir);
+
+ return existing;
+}
diff --git a/worktree.h b/worktree.h
new file mode 100644
index 0000000..71b1409
--- /dev/null
+++ b/worktree.h
@@ -0,0 +1,12 @@
+#ifndef WORKTREE_H
+#define WORKTREE_H
+
+/*
+ * Check if a per-worktree symref points to a ref in the main worktree
+ * or any linked worktree, and return the path to the exising worktree
+ * if it is. Returns NULL if there is no existing ref. The caller is
+ * responsible for freeing the returned path.
+ */
+extern char *find_shared_symref(const char *symref, const char *target);
+
+#endif
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v8 1/4] worktree: add top-level worktree.c
2015-09-18 13:30 ` [PATCH v8 1/4] worktree: add top-level worktree.c Michael Rappazzo
@ 2015-09-22 17:16 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2015-09-22 17:16 UTC (permalink / raw)
To: Michael Rappazzo; +Cc: sunshine, dturner, git
Michael Rappazzo <rappazzo@gmail.com> writes:
> worktree.c contains functions to work with and get information from
> worktrees. This introduction moves functions related to worktrees
> from branch.c into worktree.c
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> Makefile | 1 +
> branch.c | 79 +-----------------------------------------------------
> branch.h | 8 ------
> builtin/notes.c | 1 +
> worktree.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> worktree.h | 12 +++++++++
> 6 files changed, 97 insertions(+), 86 deletions(-)
> create mode 100644 worktree.c
> create mode 100644 worktree.h
> ...
> diff --git a/branch.h b/branch.h
> index d3446ed..58aa45f 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -59,12 +59,4 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
> */
> extern void die_if_checked_out(const char *branch);
>
> -/*
> - * Check if a per-worktree symref points to a ref in the main worktree
> - * or any linked worktree, and return the path to the exising worktree
> - * if it is. Returns NULL if there is no existing ref. The caller is
> - * responsible for freeing the returned path.
> - */
> -extern char *find_shared_symref(const char *symref, const char *target);
> -
> #endif
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 3608c64..8b30334 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -20,6 +20,7 @@
> #include "notes-merge.h"
> #include "notes-utils.h"
> #include "branch.h"
> +#include "worktree.h"
I think you no longer need to include branch.h after this change.
Other than that, this step looks uncontroversial.
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v8 2/4] worktree: refactor find_linked_symref function
2015-09-18 13:30 [PATCH v8 0/4] worktree: list functions and command Michael Rappazzo
2015-09-18 13:30 ` [PATCH v8 1/4] worktree: add top-level worktree.c Michael Rappazzo
@ 2015-09-18 13:30 ` Michael Rappazzo
2015-09-22 17:44 ` Junio C Hamano
2015-09-18 13:30 ` [PATCH v8 3/4] worktree: add functions to get worktree details Michael Rappazzo
2015-09-18 13:30 ` [PATCH v8 4/4] worktree: add 'list' command Michael Rappazzo
3 siblings, 1 reply; 11+ messages in thread
From: Michael Rappazzo @ 2015-09-18 13:30 UTC (permalink / raw)
To: gitster, sunshine, dturner; +Cc: git, Michael Rappazzo
Refactoring will help transition this code to provide additional useful
worktree functions.
Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
worktree.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 64 insertions(+), 22 deletions(-)
diff --git a/worktree.c b/worktree.c
index 10e1496..5c75875 100644
--- a/worktree.c
+++ b/worktree.c
@@ -3,6 +3,60 @@
#include "strbuf.h"
#include "worktree.h"
+/*
+ * read 'path_to_ref' into 'ref'. Also if is_detached is not NULL,
+ * set is_detached to 1 if the ref is detatched.
+ *
+ * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so
+ * for linked worktrees, `resolve_ref_unsafe()` won't work (it uses
+ * git_path). Parse the ref ourselves.
+ *
+ * return -1 if the ref is not a proper ref, 0 otherwise (success)
+ */
+static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
+{
+ if (is_detached)
+ *is_detached = 0;
+ if (!strbuf_readlink(ref, path_to_ref, 0)) {
+ if (!starts_with(ref->buf, "refs/")
+ || check_refname_format(ref->buf, 0))
+ return -1;
+
+ } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
+ if (starts_with(ref->buf, "ref:")) {
+ strbuf_remove(ref, 0, strlen("ref:"));
+ strbuf_trim(ref);
+ if (check_refname_format(ref->buf, 0))
+ return -1;
+ } else if (is_detached)
+ *is_detached = 1;
+ }
+ return 0;
+}
+
+static char *find_main_symref(const char *symref, const char *branch)
+{
+ struct strbuf sb = STRBUF_INIT;
+ struct strbuf path = STRBUF_INIT;
+ struct strbuf gitdir = STRBUF_INIT;
+ char *existing = NULL;
+
+ strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
+ if (parse_ref(path.buf, &sb, NULL) == -1)
+ goto done;
+ if (strcmp(sb.buf, branch))
+ goto done;
+ strbuf_addstr(&gitdir, get_git_common_dir());
+ strbuf_strip_suffix(&gitdir, ".git");
+ existing = strbuf_detach(&gitdir, NULL);
+done:
+ strbuf_release(&path);
+ strbuf_release(&sb);
+ strbuf_release(&gitdir);
+
+ return existing;
+}
+
static char *find_linked_symref(const char *symref, const char *branch,
const char *id)
{
@@ -11,36 +65,24 @@ static char *find_linked_symref(const char *symref, const char *branch,
struct strbuf gitdir = STRBUF_INIT;
char *existing = NULL;
+ if (!id)
+ goto done;
/*
* $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside
* $GIT_DIR so resolve_ref_unsafe() won't work (it uses
* git_path). Parse the ref ourselves.
*/
- if (id)
- strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
- else
- strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
+ strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
- if (!strbuf_readlink(&sb, path.buf, 0)) {
- if (!starts_with(sb.buf, "refs/") ||
- check_refname_format(sb.buf, 0))
- goto done;
- } else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&
- starts_with(sb.buf, "ref:")) {
- strbuf_remove(&sb, 0, strlen("ref:"));
- strbuf_trim(&sb);
- } else
+ if (parse_ref(path.buf, &sb, NULL) == -1)
goto done;
if (strcmp(sb.buf, branch))
goto done;
- if (id) {
- strbuf_reset(&path);
- strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
- if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
- goto done;
- strbuf_rtrim(&gitdir);
- } else
- strbuf_addstr(&gitdir, get_git_common_dir());
+ strbuf_reset(&path);
+ strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
+ if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
+ goto done;
+ strbuf_rtrim(&gitdir);
strbuf_strip_suffix(&gitdir, ".git");
existing = strbuf_detach(&gitdir, NULL);
@@ -59,7 +101,7 @@ char *find_shared_symref(const char *symref, const char *target)
struct dirent *d;
char *existing;
- if ((existing = find_linked_symref(symref, target, NULL)))
+ if ((existing = find_main_symref(symref, target)))
return existing;
strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v8 2/4] worktree: refactor find_linked_symref function
2015-09-18 13:30 ` [PATCH v8 2/4] worktree: refactor find_linked_symref function Michael Rappazzo
@ 2015-09-22 17:44 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2015-09-22 17:44 UTC (permalink / raw)
To: Michael Rappazzo; +Cc: sunshine, dturner, git
Michael Rappazzo <rappazzo@gmail.com> writes:
> Refactoring will help transition this code to provide additional useful
> worktree functions.
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
> worktree.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 64 insertions(+), 22 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index 10e1496..5c75875 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -3,6 +3,60 @@
> #include "strbuf.h"
> #include "worktree.h"
>
> +/*
> + * read 'path_to_ref' into 'ref'. Also if is_detached is not NULL,
> + * set is_detached to 1 if the ref is detatched.
set is_detached to 1 (0) if the ref is detatched (is not detached).
> + *
> + * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so
> + * for linked worktrees, `resolve_ref_unsafe()` won't work (it uses
> + * git_path). Parse the ref ourselves.
> + *
> + * return -1 if the ref is not a proper ref, 0 otherwise (success)
> + */
> +static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
> +{
> + if (is_detached)
> + *is_detached = 0;
> + if (!strbuf_readlink(ref, path_to_ref, 0)) {
> + if (!starts_with(ref->buf, "refs/")
> + || check_refname_format(ref->buf, 0))
Don't try to be creative with the format and stick to the original.
if (!starts_with(ref->buf, "refs/") ||
check_refname_format(ref->buf, 0))
> + return -1;
> +
This blank makes a strange code by making the "return -1" have no
blank above and one blank below.
> + } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> + if (starts_with(ref->buf, "ref:")) {
> + strbuf_remove(ref, 0, strlen("ref:"));
> + strbuf_trim(ref);
> + if (check_refname_format(ref->buf, 0))
> + return -1;
> + } else if (is_detached)
> + *is_detached = 1;
Minor: I have a suspicion that this would be easier to follow:
if (!starts_with(...)) {
if (is_detached)
*is_detached = 1;
} else {
strbuf_remove(...);
strbuf_trim(...);
if (check_refname_format(...))
return -1;
}
> + }
What should happen when strbuf_read_file() above fails? Would it be
a bug (i.e. the caller shouldn't have called us in the first place
with such a broken ref), would it be a repository inconsistency
(i.e. it is worth warning and stop the caller from doing further
damage), or is it OK to silently succeed?
> + return 0;
> +}
> +
> +static char *find_main_symref(const char *symref, const char *branch)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + struct strbuf path = STRBUF_INIT;
> + struct strbuf gitdir = STRBUF_INIT;
> + char *existing = NULL;
> +
> + strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
> + if (parse_ref(path.buf, &sb, NULL) == -1)
> + goto done;
I know you described it to "return -1 on an error", but as a general
style, for a function that signals a success by returning 0 and
negative on error (or on various kinds of errors), it is easier to
follow if you followed a more common pattern:
if (parse_ref(...) < 0)
goto done;
> + if (strcmp(sb.buf, branch))
> + goto done;
> + strbuf_addstr(&gitdir, get_git_common_dir());
> + strbuf_strip_suffix(&gitdir, ".git");
> + existing = strbuf_detach(&gitdir, NULL);
> +done:
> + strbuf_release(&path);
> + strbuf_release(&sb);
> + strbuf_release(&gitdir);
> +
> + return existing;
> +}
> +
> static char *find_linked_symref(const char *symref, const char *branch,
> const char *id)
> {
> @@ -11,36 +65,24 @@ static char *find_linked_symref(const char *symref, const char *branch,
> struct strbuf gitdir = STRBUF_INIT;
> char *existing = NULL;
>
> + if (!id)
> + goto done;
A caller that calls this function with id==NULL would always get a
no-op. Is that what the caller intended, or should it have called
the new find_main_symref() instead? I'd imagine it is the latter,
in which case
if (!id)
die("BUG");
is more appropriate. On the other hand, if you are trying to keep
the old interface to allow id==NULL to mean "get the information for
the primary one", I'd expect this to call find_main_symref() for the
(old-style) callers.
In either case, this "no id? no-op" looks funny.
> /*
> * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside
> * $GIT_DIR so resolve_ref_unsafe() won't work (it uses
> * git_path). Parse the ref ourselves.
> */
You moved this comment to parse_ref(), which is a more appropriate
home for it. Perhaps you want to remove this copy from here?
> - if (id)
> - strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
> - else
> - strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
> + strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
>
> - if (!strbuf_readlink(&sb, path.buf, 0)) {
> - if (!starts_with(sb.buf, "refs/") ||
> - check_refname_format(sb.buf, 0))
> - goto done;
> - } else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&
> - starts_with(sb.buf, "ref:")) {
> - strbuf_remove(&sb, 0, strlen("ref:"));
> - strbuf_trim(&sb);
> - } else
> + if (parse_ref(path.buf, &sb, NULL) == -1)
> goto done;
Same comment as in find_main_symref() applies here.
I can see the value of splitting out parse_ref() into a separate
function, but it is not immediately obvious how it would be an
overall win to duplicate major parts of the logic that used to be
shared for "primary" and "linked" cases in a single function into
two separate, simpler and similar functions at this point in the
series (note that I did not say "it clearly made it worse").
Perhaps reviews on the callers will help us answer that.
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v8 3/4] worktree: add functions to get worktree details
2015-09-18 13:30 [PATCH v8 0/4] worktree: list functions and command Michael Rappazzo
2015-09-18 13:30 ` [PATCH v8 1/4] worktree: add top-level worktree.c Michael Rappazzo
2015-09-18 13:30 ` [PATCH v8 2/4] worktree: refactor find_linked_symref function Michael Rappazzo
@ 2015-09-18 13:30 ` Michael Rappazzo
2015-09-22 18:10 ` Junio C Hamano
2015-09-18 13:30 ` [PATCH v8 4/4] worktree: add 'list' command Michael Rappazzo
3 siblings, 1 reply; 11+ messages in thread
From: Michael Rappazzo @ 2015-09-18 13:30 UTC (permalink / raw)
To: gitster, sunshine, dturner; +Cc: git, Michael Rappazzo
The worktree structure provided for an individual worktree includes the
absolute path, the location of the git dir, the head ref (if not
detached), the head revision sha1, whether or not head is detached, and
whether or not the worktree is a bare repo.
Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
worktree.c | 224 ++++++++++++++++++++++++++++++++++++++++++++++---------------
worktree.h | 26 +++++++
2 files changed, 195 insertions(+), 55 deletions(-)
diff --git a/worktree.c b/worktree.c
index 5c75875..74a2ffa 100644
--- a/worktree.c
+++ b/worktree.c
@@ -3,6 +3,17 @@
#include "strbuf.h"
#include "worktree.h"
+void free_worktrees(struct worktree **worktrees)
+{
+ for (int i = 0; worktrees[i]; i++) {
+ free(worktrees[i]->path);
+ free(worktrees[i]->git_dir);
+ free(worktrees[i]->head_ref);
+ free(worktrees[i]);
+ }
+ free (worktrees);
+}
+
/*
* read 'path_to_ref' into 'ref'. Also if is_detached is not NULL,
* set is_detached to 1 if the ref is detatched.
@@ -34,91 +45,194 @@ static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
return 0;
}
-static char *find_main_symref(const char *symref, const char *branch)
+/**
+ * Add the head_sha1 and head_ref (if not detached) to the given worktree
+ */
+static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
{
- struct strbuf sb = STRBUF_INIT;
+ if (head_ref->len) {
+ if (worktree->is_detached) {
+ get_sha1_hex(head_ref->buf, worktree->head_sha1);
+ } else {
+ resolve_ref_unsafe(head_ref->buf, 0, worktree->head_sha1, NULL);
+ worktree->head_ref = strbuf_detach(head_ref, NULL);
+ }
+ }
+}
+
+/**
+ * get the main worktree
+ */
+static struct worktree *get_main_worktree()
+{
+ struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
- struct strbuf gitdir = STRBUF_INIT;
- char *existing = NULL;
+ struct strbuf worktree_path = STRBUF_INIT;
+ struct strbuf git_dir = STRBUF_INIT;
+ struct strbuf head_ref = STRBUF_INIT;
+ int is_bare = 0;
+ int is_detached = 0;
- strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
- if (parse_ref(path.buf, &sb, NULL) == -1)
- goto done;
- if (strcmp(sb.buf, branch))
- goto done;
- strbuf_addstr(&gitdir, get_git_common_dir());
- strbuf_strip_suffix(&gitdir, ".git");
- existing = strbuf_detach(&gitdir, NULL);
-done:
- strbuf_release(&path);
- strbuf_release(&sb);
- strbuf_release(&gitdir);
+ strbuf_addf(&git_dir, "%s", absolute_path(get_git_common_dir()));
+ strbuf_addf(&worktree_path, "%s", absolute_path(get_git_common_dir()));
+ is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
+ if (is_bare)
+ strbuf_strip_suffix(&worktree_path, "/.");
- return existing;
+ strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+
+ if (!parse_ref(path.buf, &head_ref, &is_detached)) {
+ worktree = xmalloc(sizeof(struct worktree));
+ worktree->path = strbuf_detach(&worktree_path, NULL);
+ worktree->git_dir = strbuf_detach(&git_dir, NULL);
+ worktree->is_bare = is_bare;
+ worktree->head_ref = NULL;
+ worktree->is_detached = is_detached;
+ add_head_info(&head_ref, worktree);
+ }
+ return worktree;
}
-static char *find_linked_symref(const char *symref, const char *branch,
- const char *id)
+static struct worktree *get_linked_worktree(const char *id)
{
- struct strbuf sb = STRBUF_INIT;
+ struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
- struct strbuf gitdir = STRBUF_INIT;
- char *existing = NULL;
+ struct strbuf worktree_path = STRBUF_INIT;
+ struct strbuf git_dir = STRBUF_INIT;
+ struct strbuf head_ref = STRBUF_INIT;
+ int is_detached = 0;
if (!id)
goto done;
- /*
- * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside
- * $GIT_DIR so resolve_ref_unsafe() won't work (it uses
- * git_path). Parse the ref ourselves.
- */
- strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
-
- if (parse_ref(path.buf, &sb, NULL) == -1)
- goto done;
- if (strcmp(sb.buf, branch))
+
+ strbuf_addf(&git_dir, "%s/worktrees/%s",
+ absolute_path(get_git_common_dir()), id);
+ strbuf_addf(&path, "%s/gitdir", git_dir.buf);
+ if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
+ /* invalid gitdir file */
goto done;
+
+ strbuf_rtrim(&worktree_path);
+ if (!strbuf_strip_suffix(&worktree_path, "/.git")) {
+ strbuf_reset(&worktree_path);
+ strbuf_addstr(&worktree_path, absolute_path("."));
+ strbuf_strip_suffix(&worktree_path, "/.");
+ }
+
strbuf_reset(&path);
- strbuf_addf(&path, "%s/worktrees/%s/gitdir", get_git_common_dir(), id);
- if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
+ strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+
+ if (parse_ref(path.buf, &head_ref, &is_detached))
goto done;
- strbuf_rtrim(&gitdir);
- strbuf_strip_suffix(&gitdir, ".git");
- existing = strbuf_detach(&gitdir, NULL);
+ worktree = xmalloc(sizeof(struct worktree));
+ worktree->path = strbuf_detach(&worktree_path, NULL);
+ worktree->git_dir = strbuf_detach(&git_dir, NULL);
+ worktree->is_bare = 0;
+ worktree->head_ref = NULL;
+ worktree->is_detached = is_detached;
+ add_head_info(&head_ref, worktree);
+
done:
strbuf_release(&path);
- strbuf_release(&sb);
- strbuf_release(&gitdir);
+ strbuf_release(&git_dir);
+ strbuf_release(&head_ref);
+ strbuf_release(&worktree_path);
+ return worktree;
+}
- return existing;
+/**
+ * get the estimated worktree count for use in sizing the worktree array
+ * Note that the minimum count should be 2 (main worktree + NULL). Using the
+ * file count in $GIT_COMMON_DIR/worktrees includes '.' and '..' so the
+ * minimum is satisfied by counting those entries.
+ */
+static int get_estimated_worktree_count()
+{
+ struct strbuf path = STRBUF_INIT;
+ DIR *dir;
+ int count = 0;
+
+ strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+ dir = opendir(path.buf);
+ strbuf_release(&path);
+ if (dir) {
+ while (readdir(dir))
+ count++;
+ closedir(dir);
+ }
+
+ if (!count)
+ count = 2;
+
+ return count;
}
-char *find_shared_symref(const char *symref, const char *target)
+struct worktree **get_worktrees(void)
{
+ struct worktree **list = NULL;
struct strbuf path = STRBUF_INIT;
DIR *dir;
struct dirent *d;
- char *existing;
+ int counter = 0;
+
+ list = xmalloc(get_estimated_worktree_count() * sizeof(struct worktree *));
- if ((existing = find_main_symref(symref, target)))
- return existing;
+ if ((list[counter] = get_main_worktree()))
+ counter++;
strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
dir = opendir(path.buf);
strbuf_release(&path);
- if (!dir)
- return NULL;
-
- while ((d = readdir(dir)) != NULL) {
- if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
- continue;
- existing = find_linked_symref(symref, target, d->d_name);
- if (existing)
- goto done;
+ if (dir) {
+ while ((d = readdir(dir)) != NULL) {
+ struct worktree *linked = NULL;
+ if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
+ continue;
+
+ if ((linked = get_linked_worktree(d->d_name))) {
+ list[counter++] = linked;
+ }
+ }
+ closedir(dir);
}
-done:
- closedir(dir);
+ list[counter] = NULL;
+ return list;
+}
+
+char *find_shared_symref(const char *symref, const char *target)
+{
+ char *existing = NULL;
+ struct strbuf path = STRBUF_INIT;
+ struct strbuf sb = STRBUF_INIT;
+ struct worktree **worktrees = get_worktrees();
+ int symref_is_head = !strcmp("HEAD", symref);
+
+ for (int i = 0; worktrees[i]; i++) {
+ if (!symref_is_head) {
+ strbuf_reset(&path);
+ strbuf_reset(&sb);
+ strbuf_addf(&path, "%s/%s", worktrees[i]->git_dir, symref);
+
+ if (parse_ref(path.buf, &sb, NULL)) {
+ continue;
+ }
+
+ if (!strcmp(sb.buf, target)) {
+ existing = xstrdup(worktrees[i]->path);
+ break;
+ }
+ } else {
+ if (worktrees[i]->head_ref && !strcmp(worktrees[i]->head_ref, target)) {
+ existing = xstrdup(worktrees[i]->path);
+ break;
+ }
+ }
+ }
+
+ strbuf_release(&path);
+ strbuf_release(&sb);
+ free_worktrees(worktrees);
return existing;
}
diff --git a/worktree.h b/worktree.h
index 71b1409..b4b3dda 100644
--- a/worktree.h
+++ b/worktree.h
@@ -1,6 +1,32 @@
#ifndef WORKTREE_H
#define WORKTREE_H
+struct worktree {
+ char *path;
+ char *git_dir;
+ char *head_ref;
+ unsigned char head_sha1[20];
+ int is_detached;
+ int is_bare;
+};
+
+/* Functions for acting on the information about worktrees. */
+
+/*
+ * Get the worktrees. The primary worktree will always be the first returned,
+ * and linked worktrees will be pointed to by 'next' in each subsequent
+ * worktree. No specific ordering is done on the linked worktrees.
+ *
+ * The caller is responsible for freeing the memory from the returned
+ * worktree(s).
+ */
+extern struct worktree **get_worktrees(void);
+
+/*
+ * Free up the memory for worktree(s)
+ */
+extern void free_worktrees(struct worktree **);
+
/*
* Check if a per-worktree symref points to a ref in the main worktree
* or any linked worktree, and return the path to the exising worktree
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v8 3/4] worktree: add functions to get worktree details
2015-09-18 13:30 ` [PATCH v8 3/4] worktree: add functions to get worktree details Michael Rappazzo
@ 2015-09-22 18:10 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2015-09-22 18:10 UTC (permalink / raw)
To: Michael Rappazzo; +Cc: sunshine, dturner, git
Michael Rappazzo <rappazzo@gmail.com> writes:
> +/**
> + * get the main worktree
> + */
> +static struct worktree *get_main_worktree()
static struct worktree *get_main_worktree(void)
> +{
> + struct worktree *worktree = NULL;
> struct strbuf path = STRBUF_INIT;
> + struct strbuf worktree_path = STRBUF_INIT;
> + struct strbuf git_dir = STRBUF_INIT;
> + struct strbuf head_ref = STRBUF_INIT;
> + int is_bare = 0;
> + int is_detached = 0;
>
> + strbuf_addf(&git_dir, "%s", absolute_path(get_git_common_dir()));
> + strbuf_addf(&worktree_path, "%s", absolute_path(get_git_common_dir()));
Why not strbuf_add(&git_dir, absolute_path(get_git_common_dir()))
followed by strbuf_addbuf(&git_dir, &worktree_path)?
> + is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
> + if (is_bare)
> + strbuf_strip_suffix(&worktree_path, "/.");
Hmm, it is not immediately obvious which part is meant to be a
faithful translations from the old find_main_symref() that grab the
same kind of information the old code used to discover (and record
in a new mechanism that is "struct worktree"), and which part is a
new code that is needed to grab new pieces of information. The
effort in [PATCH 2/4] to make it easier to follow sort of lost by
this rewrite.
I presume that discovery of the git_dir is from the original and
everything else including is_bare bit, head_ref, is_detached are
new?
Splitting this patch further into two may help showing the changes
better. First move to the "grab information into an element of the
worktree array" code structure without adding new information, and
then "now make the worktree structure richer" as a follow up,
perhaps?
> +/**
> + * get the estimated worktree count for use in sizing the worktree array
> + * Note that the minimum count should be 2 (main worktree + NULL). Using the
> + * file count in $GIT_COMMON_DIR/worktrees includes '.' and '..' so the
> + * minimum is satisfied by counting those entries.
> + */
> +static int get_estimated_worktree_count()
> +{
> +...
> }
This is new. My gut-feeling is that we would probably not want to
do this (see below).
> +struct worktree **get_worktrees(void)
> {
> + struct worktree **list = NULL;
> struct strbuf path = STRBUF_INIT;
> DIR *dir;
> struct dirent *d;
> - char *existing;
> + int counter = 0;
> +
> + list = xmalloc(get_estimated_worktree_count() * sizeof(struct worktree *));
Here you scanned the directory to see how many possible worktrees
there are "at this moment".
Time passes.
How do you know that nobody added more worktrees in the meantime?
You don't.
>
> - if ((existing = find_main_symref(symref, target)))
> - return existing;
> + if ((list[counter] = get_main_worktree()))
> + counter++;
>
> strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
> dir = opendir(path.buf);
> strbuf_release(&path);
> + if (dir) {
> + while ((d = readdir(dir)) != NULL) {
> + struct worktree *linked = NULL;
> + if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> + continue;
> +
> + if ((linked = get_linked_worktree(d->d_name))) {
> + list[counter++] = linked;
Which means that nothing stops you from overrunning the list[] with
this iteration.
By the way, when the body of "if" and "else" both consists of a
single statement, we tend to drop braces.
> + }
And in order to avoid overrunning, you would need to do the usual
ALLOC_GROW() dance on list[] _anyway_. So there is no much point,
other than to optimize for a case where you have thousands of
worktrees and the usual ALLOC_GROW() approach would have to do
realloc(3) too many times, to have the initial "guestimate" scan.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v8 4/4] worktree: add 'list' command
2015-09-18 13:30 [PATCH v8 0/4] worktree: list functions and command Michael Rappazzo
` (2 preceding siblings ...)
2015-09-18 13:30 ` [PATCH v8 3/4] worktree: add functions to get worktree details Michael Rappazzo
@ 2015-09-18 13:30 ` Michael Rappazzo
2015-09-22 19:42 ` Junio C Hamano
3 siblings, 1 reply; 11+ messages in thread
From: Michael Rappazzo @ 2015-09-18 13:30 UTC (permalink / raw)
To: gitster, sunshine, dturner; +Cc: git, Michael Rappazzo
'git worktree list' iterates through the worktree list, and outputs
details of the worktree including the path to the worktree, the currently
checked out revision and branch, and if the work tree is bare. There is
also porcelain format option available.
Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
Documentation/git-worktree.txt | 15 +++++++-
builtin/worktree.c | 78 ++++++++++++++++++++++++++++++++++++++
t/t2027-worktree-list.sh | 86 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 178 insertions(+), 1 deletion(-)
create mode 100755 t/t2027-worktree-list.sh
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index fb68156..8ee65c6 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -11,6 +11,7 @@ SYNOPSIS
[verse]
'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree list' [--porcelain]
DESCRIPTION
-----------
@@ -59,6 +60,13 @@ prune::
Prune working tree information in $GIT_DIR/worktrees.
+list::
+
+List details of each worktree. The main worktree is listed first, followed by
+each of the linked worktrees. The output details include if the worktree is
+bare, the revision currently checked out, and the branch currently checked out
+(or 'detached HEAD' if none).
+
OPTIONS
-------
@@ -86,6 +94,11 @@ OPTIONS
With `prune`, do not remove anything; just report what it would
remove.
+--porcelain::
+ With `list`, output in an easy-to-parse format for scripts.
+ This format will remain stable across Git versions and regardless of user
+ configuration.
+
-v::
--verbose::
With `prune`, report all removals.
@@ -93,6 +106,7 @@ OPTIONS
--expire <time>::
With `prune`, only expire unused working trees older than <time>.
+
DETAILS
-------
Each linked working tree has a private sub-directory in the repository's
@@ -167,7 +181,6 @@ performed manually, such as:
- `remove` to remove a linked working tree and its administrative files (and
warn if the working tree is dirty)
- `mv` to move or rename a working tree and update its administrative files
-- `list` to list linked working trees
- `lock` to prevent automatic pruning of administrative files (for instance,
for a working tree on a portable device)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 71bb770..e6e36ac 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -8,10 +8,13 @@
#include "run-command.h"
#include "sigchain.h"
#include "refs.h"
+#include "utf8.h"
+#include "worktree.h"
static const char * const worktree_usage[] = {
N_("git worktree add [<options>] <path> <branch>"),
N_("git worktree prune [<options>]"),
+ N_("git worktree list [<options>]"),
NULL
};
@@ -359,6 +362,79 @@ static int add(int ac, const char **av, const char *prefix)
return add_worktree(path, branch, &opts);
}
+static void show_worktree_porcelain(struct worktree *worktree)
+{
+ struct strbuf sb = STRBUF_INIT;
+
+ strbuf_addf(&sb, "worktree %s\n", worktree->path);
+ if (worktree->is_bare)
+ strbuf_addstr(&sb, "bare");
+ else {
+ if (worktree->is_detached)
+ strbuf_addf(&sb, "detached at %s", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));
+ else
+ strbuf_addf(&sb, "branch %s", shorten_unambiguous_ref(worktree->head_ref, 0));
+ }
+
+ printf("%s\n\n", sb.buf);
+
+ strbuf_release(&sb);
+}
+static void show_worktree(struct worktree *worktree, int path_maxlen)
+{
+ struct strbuf sb = STRBUF_INIT;
+
+ int cur_len = strlen(worktree->path);
+ int utf8_adj = cur_len - utf8_strwidth(worktree->path);
+ strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree->path);
+ if (worktree->is_bare)
+ strbuf_addstr(&sb, "(bare)");
+ else {
+ strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));
+ if (!worktree->is_detached)
+ strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree->head_ref, 0));
+ else
+ strbuf_addstr(&sb, "(detached HEAD)");
+ }
+ printf("%s\n", sb.buf);
+
+ strbuf_release(&sb);
+}
+
+static int list(int ac, const char **av, const char *prefix)
+{
+ int porcelain = 0;
+
+ struct option options[] = {
+ OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+ OPT_END()
+ };
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac)
+ usage_with_options(worktree_usage, options);
+ else {
+ struct worktree **worktrees = get_worktrees();
+ int path_maxlen = 0;
+
+ if (!porcelain) {
+ for (int i = 0; worktrees[i]; i++) {
+ int len = strlen(worktrees[i]->path);
+ if (len > path_maxlen)
+ path_maxlen = len;
+ }
+ }
+ for (int i = 0; worktrees[i]; i++) {
+ if (porcelain)
+ show_worktree_porcelain(worktrees[i]);
+ else
+ show_worktree(worktrees[i], path_maxlen);
+ }
+ free_worktrees(worktrees);
+ }
+ return 0;
+}
+
int cmd_worktree(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -371,5 +447,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
return add(ac - 1, av + 1, prefix);
if (!strcmp(av[1], "prune"))
return prune(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "list"))
+ return list(ac - 1, av + 1, prefix);
usage_with_options(worktree_usage, options);
}
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
new file mode 100755
index 0000000..b68dfb4
--- /dev/null
+++ b/t/t2027-worktree-list.sh
@@ -0,0 +1,86 @@
+#!/bin/sh
+
+test_description='test git worktree list'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit init
+'
+
+test_expect_success '"list" all worktrees from main' '
+ echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
+ git worktree add --detach here master &&
+ test_when_finished "rm -rf here && git worktree prune" &&
+ echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
+ git worktree list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees from linked' '
+ echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
+ git worktree add --detach here master &&
+ test_when_finished "rm -rf here && git worktree prune" &&
+ echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
+ git -C here worktree list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees --porcelain' '
+ echo "worktree $(git rev-parse --show-toplevel)" >expect &&
+ echo "branch $(git symbolic-ref --short HEAD)" >>expect &&
+ echo >>expect &&
+ git worktree add --detach here master &&
+ test_when_finished "rm -rf here && git worktree prune" &&
+ echo "worktree $(git -C here rev-parse --show-toplevel)" >>expect &&
+ echo "detached at $(git rev-parse --short HEAD)" >>expect &&
+ echo >>expect &&
+ git worktree list --porcelain >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'bare repo setup' '
+ git init --bare bare1 &&
+ echo "data" >file1 &&
+ git add file1 &&
+ git commit -m"File1: add data" &&
+ git push bare1 master &&
+ git reset --hard HEAD^
+'
+
+test_expect_success '"list" all worktrees from bare main' '
+ git -C bare1 worktree add --detach ../there master &&
+ test_when_finished "rm -rf there && git -C bare1 worktree prune" &&
+ echo "$(pwd)/bare1 (bare)" >expect &&
+ echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
+ git -C bare1 worktree list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees --porcelain from bare main' '
+ git -C bare1 worktree add --detach ../there master &&
+ test_when_finished "rm -rf there && git -C bare1 worktree prune" &&
+ echo "worktree $(pwd)/bare1" >expect &&
+ echo "bare" >>expect &&
+ echo >>expect &&
+ echo "worktree $(git -C there rev-parse --show-toplevel)" >>expect &&
+ echo "detached at $(git -C there rev-parse --short HEAD)" >>expect &&
+ echo >>expect &&
+ git -C bare1 worktree list --porcelain >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees from linked with a bare main' '
+ git -C bare1 worktree add --detach ../there master &&
+ test_when_finished "rm -rf there && git -C bare1 worktree prune" &&
+ echo "$(pwd)/bare1 (bare)" >expect &&
+ echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
+ git -C there worktree list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'bare repo cleanup' '
+ rm -rf bare1
+'
+
+test_done
--
2.5.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v8 4/4] worktree: add 'list' command
2015-09-18 13:30 ` [PATCH v8 4/4] worktree: add 'list' command Michael Rappazzo
@ 2015-09-22 19:42 ` Junio C Hamano
2015-09-24 2:58 ` Mike Rappazzo
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2015-09-22 19:42 UTC (permalink / raw)
To: Michael Rappazzo; +Cc: sunshine, dturner, git
Michael Rappazzo <rappazzo@gmail.com> writes:
>
> +--porcelain::
> + With `list`, output in an easy-to-parse format for scripts.
> + This format will remain stable across Git versions and regardless of user
> + configuration.
... and exactly what does it output? That would be the first
question a reader of this documentation would ask.
> @@ -93,6 +106,7 @@ OPTIONS
> --expire <time>::
> With `prune`, only expire unused working trees older than <time>.
>
> +
?
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 71bb770..e6e36ac 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -8,10 +8,13 @@
> #include "run-command.h"
> #include "sigchain.h"
> #include "refs.h"
> +#include "utf8.h"
> +#include "worktree.h"
>
> static const char * const worktree_usage[] = {
> N_("git worktree add [<options>] <path> <branch>"),
> N_("git worktree prune [<options>]"),
> + N_("git worktree list [<options>]"),
> NULL
> };
>
> @@ -359,6 +362,79 @@ static int add(int ac, const char **av, const char *prefix)
> return add_worktree(path, branch, &opts);
> }
>
> +static void show_worktree_porcelain(struct worktree *worktree)
> +{
> + struct strbuf sb = STRBUF_INIT;
> +
> + strbuf_addf(&sb, "worktree %s\n", worktree->path);
> + if (worktree->is_bare)
> + strbuf_addstr(&sb, "bare");
> + else {
> + if (worktree->is_detached)
> + strbuf_addf(&sb, "detached at %s", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));
> + else
> + strbuf_addf(&sb, "branch %s", shorten_unambiguous_ref(worktree->head_ref, 0));
> + }
Writing the above like this:
if (worktree->is_bare)
...
else if (worktree->is_detached)
...
else
...
would make it a lot more clear that there are three cases.
Also, I doubt --porcelain output wants shorten or abbrev.
Human-readability is not a goal. Reproducibility is. When you run
"worktree list" today and save the output, you want the output from
"worktree list" taken tomorrow to exactly match it, even after
creating many objects and tags with conflicting names with branches,
as long as you didn't change their HEADs in the meantime.
> +
> + printf("%s\n\n", sb.buf);
> +
> + strbuf_release(&sb);
I am not sure what the point of use of a strbuf is in this function,
though. Two printf's for each case (one for the common "worktree
%s", the other inside if/elseif/else cascade) should be sufficient.
> +static void show_worktree(struct worktree *worktree, int path_maxlen)
> +{
> + struct strbuf sb = STRBUF_INIT;
> +
Remove this blank line. You are still declaring variables.
> + int cur_len = strlen(worktree->path);
> + int utf8_adj = cur_len - utf8_strwidth(worktree->path);
Have a blank line here, instead, as now you start your statements.
> + strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree->path);
> + if (worktree->is_bare)
> + strbuf_addstr(&sb, "(bare)");
> + else {
> + strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));
Personally I am not a big fan of the the alignment and use of
utf8_strwidth(), but by using find_unique_abbrev() here, you are
breaking the alignment, aren't you? " [branchname]" that follows
the commit object name would not start at the same column, when
you have many objects that default-abbrev is not enough to uniquely
identify them.
And it can easily be fixed by computing the unique-abbrev length for
all the non-bare worktree's HEADs in the same loop you computed
path_maxlen() in the caller, passing that to this function, and use
that as mininum abbrev length when computing the unique-abbrev.
> + if (!worktree->is_detached)
> + strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree->head_ref, 0));
> + else
> + strbuf_addstr(&sb, "(detached HEAD)");
> + }
> + printf("%s\n", sb.buf);
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> new file mode 100755
> index 0000000..b68dfb4
> --- /dev/null
> +++ b/t/t2027-worktree-list.sh
> @@ -0,0 +1,86 @@
> +#!/bin/sh
> +
> +test_description='test git worktree list'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + test_commit init
> +'
> +
> +test_expect_success '"list" all worktrees from main' '
> + echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
Are the number of SPs here significant and if so in what way? Does
it depend on your environment or will there always be six of them?
Either way feels like an indication of a problem.
> + git worktree add --detach here master &&
> + test_when_finished "rm -rf here && git worktree prune" &&
Aren't these two the other way around? When "add" fails in the
middle, you would want it to be removed to proceed to the next test
without leaving 'here' in the list of worktrees, no?
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v8 4/4] worktree: add 'list' command
2015-09-22 19:42 ` Junio C Hamano
@ 2015-09-24 2:58 ` Mike Rappazzo
2015-09-24 5:00 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Mike Rappazzo @ 2015-09-24 2:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, David Turner, Git List
On Tue, Sep 22, 2015 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Rappazzo <rappazzo@gmail.com> writes:
>
>>
>> +--porcelain::
>> + With `list`, output in an easy-to-parse format for scripts.
>> + This format will remain stable across Git versions and regardless of user
>> + configuration.
>
> ... and exactly what does it output? That would be the first
> question a reader of this documentation would ask.
>
I will add that description. I have mostly followed what Eric
suggested in the v7 series for a porcelain format. Does the porcelain
format restrict additive changes? That is, is it OK for a future
patch to add another field in the format, as long as it doesn't alter
the other values? Is the format that I have used here acceptable
(assuming the changes proposed below are made)?
>
>> @@ -93,6 +106,7 @@ OPTIONS
>> --expire <time>::
>> With `prune`, only expire unused working trees older than <time>.
>>
>> +
>
> ?
>
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index 71bb770..e6e36ac 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -8,10 +8,13 @@
>> #include "run-command.h"
>> #include "sigchain.h"
>> #include "refs.h"
>> +#include "utf8.h"
>> +#include "worktree.h"
>>
>> static const char * const worktree_usage[] = {
>> N_("git worktree add [<options>] <path> <branch>"),
>> N_("git worktree prune [<options>]"),
>> + N_("git worktree list [<options>]"),
>> NULL
>> };
>>
>> @@ -359,6 +362,79 @@ static int add(int ac, const char **av, const char *prefix)
>> return add_worktree(path, branch, &opts);
>> }
>>
>> +static void show_worktree_porcelain(struct worktree *worktree)
>> +{
>> + struct strbuf sb = STRBUF_INIT;
>> +
>> + strbuf_addf(&sb, "worktree %s\n", worktree->path);
>> + if (worktree->is_bare)
>> + strbuf_addstr(&sb, "bare");
>> + else {
>> + if (worktree->is_detached)
>> + strbuf_addf(&sb, "detached at %s", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));
>> + else
>> + strbuf_addf(&sb, "branch %s", shorten_unambiguous_ref(worktree->head_ref, 0));
>> + }
>
> Writing the above like this:
>
> if (worktree->is_bare)
> ...
> else if (worktree->is_detached)
> ...
> else
> ...
>
> would make it a lot more clear that there are three cases.
>
> Also, I doubt --porcelain output wants shorten or abbrev.
>
> Human-readability is not a goal. Reproducibility is. When you run
> "worktree list" today and save the output, you want the output from
> "worktree list" taken tomorrow to exactly match it, even after
> creating many objects and tags with conflicting names with branches,
> as long as you didn't change their HEADs in the meantime.
>
>> +
>> + printf("%s\n\n", sb.buf);
>> +
>> + strbuf_release(&sb);
>
> I am not sure what the point of use of a strbuf is in this function,
> though. Two printf's for each case (one for the common "worktree
> %s", the other inside if/elseif/else cascade) should be sufficient.
>
>> +static void show_worktree(struct worktree *worktree, int path_maxlen)
>> +{
>> + struct strbuf sb = STRBUF_INIT;
>> +
>
> Remove this blank line. You are still declaring variables.
>
>> + int cur_len = strlen(worktree->path);
>> + int utf8_adj = cur_len - utf8_strwidth(worktree->path);
>
> Have a blank line here, instead, as now you start your statements.
>
>> + strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + utf8_adj, worktree->path);
>> + if (worktree->is_bare)
>> + strbuf_addstr(&sb, "(bare)");
>> + else {
>> + strbuf_addf(&sb, "%s ", find_unique_abbrev(worktree->head_sha1, DEFAULT_ABBREV));
>
> Personally I am not a big fan of the the alignment and use of
> utf8_strwidth(), but by using find_unique_abbrev() here, you are
> breaking the alignment, aren't you? " [branchname]" that follows
> the commit object name would not start at the same column, when
> you have many objects that default-abbrev is not enough to uniquely
> identify them.
>
> And it can easily be fixed by computing the unique-abbrev length for
> all the non-bare worktree's HEADs in the same loop you computed
> path_maxlen() in the caller, passing that to this function, and use
> that as mininum abbrev length when computing the unique-abbrev.
>
I only intended for the path to be right padded, since paths can vary
in length so widely. I found it much easier to read with the path
right-padded. I think that doing a full column align isn't the best
looking output in this case. I tried to model this output after `git
branch -vv`. I didn't notice if that does a full align if the
shortened refs are differently sized. I will try it out, and see if
it makes a significant visual impact.
>> + if (!worktree->is_detached)
>> + strbuf_addf(&sb, "[%s]", shorten_unambiguous_ref(worktree->head_ref, 0));
>> + else
>> + strbuf_addstr(&sb, "(detached HEAD)");
>> + }
>> + printf("%s\n", sb.buf);
>
>
>> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
>> new file mode 100755
>> index 0000000..b68dfb4
>> --- /dev/null
>> +++ b/t/t2027-worktree-list.sh
>> @@ -0,0 +1,86 @@
>> +#!/bin/sh
>> +
>> +test_description='test git worktree list'
>> +
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup' '
>> + test_commit init
>> +'
>> +
>> +test_expect_success '"list" all worktrees from main' '
>> + echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect &&
>
> Are the number of SPs here significant and if so in what way? Does
> it depend on your environment or will there always be six of them?
> Either way feels like an indication of a problem.
The number of spaces is significant in this case, but it should not be
platform dependent. It is just the padding for the different worktree
paths.
>
>> + git worktree add --detach here master &&
>> + test_when_finished "rm -rf here && git worktree prune" &&
>
> Aren't these two the other way around? When "add" fails in the
> middle, you would want it to be removed to proceed to the next test
> without leaving 'here' in the list of worktrees, no?
Sure, I'll switch it.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v8 4/4] worktree: add 'list' command
2015-09-24 2:58 ` Mike Rappazzo
@ 2015-09-24 5:00 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2015-09-24 5:00 UTC (permalink / raw)
To: Mike Rappazzo; +Cc: Eric Sunshine, David Turner, Git List
Mike Rappazzo <rappazzo@gmail.com> writes:
> ... Does the porcelain
> format restrict additive changes? That is, is it OK for a future
> patch to add another field in the format, as long as it doesn't alter
> the other values? Is the format that I have used here acceptable
> (assuming the changes proposed below are made)?
It for you, as the designer of the format, to decide what to put in
the proposed specification, but I'm sure that we would want it to be
extensible. I guess your design is essentially a series of records,
each of which begins with a "worktree <path>" line, followed by
various attributes, one per line, about that worktree, and each
attribute line begins with some fixed keyword so that the reader can
tell what attribute the line is talking about (or if the line
describes an attribute that the reader does not yet know about), and
I think that is an acceptable format.
You need to decide and describe if the value for some attribute can
span multiple lines (and define the quoting mechanism if that is the
case), what the set of keywords currently defined and what each of
these keywords means, and document that the readers are expected to
skip a line that begins with an unknown keyword.
>> Are the number of SPs here significant and if so in what way? Does
>> it depend on your environment or will there always be six of them?
>> Either way feels like an indication of a problem.
>
> The number of spaces is significant in this case, but it should not be
> platform dependent. It is just the padding for the different worktree
> paths.
By environment, I didn't just mean 'platform'. For example, the
output from --show-toplevel will be different for me and you as it
is likely that the absolute path where I have my repository and you
have yours would be different, so the contents of "expect" would
also be. I was wondering where that SIX comes from.
The reason why I said it feels like an indication of a problem is
because it invites this question: If it is uniformly SIX no matter
what the user's environment is, why does it have to be SIX and not
other value, say, ONE, for example?
^ permalink raw reply [flat|nested] 11+ messages in thread