possible to checkout same branch in different worktree

classic Classic list List threaded Threaded
117 messages Options
1 ... 3456
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/5] worktree.c: add find_worktree_by_path()

Duy Nguyen
So far we haven't needed to identify an existing worktree from command
line. Future commands such as lock or move will need it. There are of
course other options for identifying a worktree, for example by branch
or even by internal id. They may be added later if proved useful.

Helped-by: Eric Sunshine <[hidden email]>
Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 worktree.c | 12 ++++++++++++
 worktree.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/worktree.c b/worktree.c
index f4a4f38..c5d45a6 100644
--- a/worktree.c
+++ b/worktree.c
@@ -214,6 +214,18 @@ const char *get_worktree_git_dir(const struct worktree *wt)
  return git_common_path("worktrees/%s", wt->id);
 }
 
+struct worktree *find_worktree_by_path(struct worktree **list,
+       const char *path_)
+{
+ char *path = xstrdup(real_path(path_));
+
+ for (; *list; list++)
+ if (!fspathcmp(path, real_path((*list)->path)))
+ break;
+ free(path);
+ return *list;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
       const char *target)
 {
diff --git a/worktree.h b/worktree.h
index 1394909..91627fd 100644
--- a/worktree.h
+++ b/worktree.h
@@ -29,6 +29,12 @@ extern struct worktree **get_worktrees(void);
  */
 extern const char *get_worktree_git_dir(const struct worktree *wt);
 
+/*
+ * Search a worktree by its path. Paths are normalized internally.
+ */
+extern struct worktree *find_worktree_by_path(struct worktree **list,
+      const char *path_);
+
 /*
  * Free up the memory for worktree(s)
  */
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/5] worktree.c: add is_main_worktree()

Duy Nguyen
In reply to this post by Duy Nguyen
Main worktree _is_ different. You can lock a linked worktree but not the
main one, for example. Provide an API for checking that.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 worktree.c | 5 +++++
 worktree.h | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/worktree.c b/worktree.c
index c5d45a6..6b3015b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -226,6 +226,11 @@ struct worktree *find_worktree_by_path(struct worktree **list,
  return *list;
 }
 
+int is_main_worktree(const struct worktree *wt)
+{
+ return !wt->id;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
       const char *target)
 {
diff --git a/worktree.h b/worktree.h
index 91627fd..39f1aa2 100644
--- a/worktree.h
+++ b/worktree.h
@@ -35,6 +35,11 @@ extern const char *get_worktree_git_dir(const struct worktree *wt);
 extern struct worktree *find_worktree_by_path(struct worktree **list,
       const char *path_);
 
+/*
+ * Return true if the given worktree is the main one.
+ */
+extern int is_main_worktree(const struct worktree *wt);
+
 /*
  * Free up the memory for worktree(s)
  */
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 3/5] worktree.c: add is_worktree_locked()

Duy Nguyen
In reply to this post by Duy Nguyen
This provides an API for checking if a worktree is locked. We need to
check this to avoid double locking a worktree, or try to unlock one when
it's not even locked.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 worktree.c | 21 +++++++++++++++++++++
 worktree.h |  6 ++++++
 2 files changed, 27 insertions(+)

diff --git a/worktree.c b/worktree.c
index 6b3015b..cb30af3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -231,6 +231,27 @@ int is_main_worktree(const struct worktree *wt)
  return !wt->id;
 }
 
+const char *is_worktree_locked(const struct worktree *wt)
+{
+ static struct strbuf sb = STRBUF_INIT;
+ struct strbuf path = STRBUF_INIT;
+
+ strbuf_git_common_path(&path, "worktrees/%s/locked", wt->id);
+
+ if (!file_exists(path.buf)) {
+ strbuf_release(&path);
+ return NULL;
+ }
+
+ strbuf_reset(&sb);
+ if (strbuf_read_file(&sb, path.buf, 0) < 0)
+ die_errno(_("failed to read '%s'"), path.buf);
+ strbuf_release(&path);
+
+ strbuf_trim(&sb);
+ return sb.buf;
+}
+
 int is_worktree_being_rebased(const struct worktree *wt,
       const char *target)
 {
diff --git a/worktree.h b/worktree.h
index 39f1aa2..3a780c2 100644
--- a/worktree.h
+++ b/worktree.h
@@ -40,6 +40,12 @@ extern struct worktree *find_worktree_by_path(struct worktree **list,
  */
 extern int is_main_worktree(const struct worktree *wt);
 
+/*
+ * Return the reason string if the given worktree is locked. Return
+ * NULL otherwise. Return value is only valid until the next invocation.
+ */
+extern const char *is_worktree_locked(const struct worktree *wt);
+
 /*
  * Free up the memory for worktree(s)
  */
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 4/5] worktree: add "lock" command

Duy Nguyen
In reply to this post by Duy Nguyen
Helped-by: Eric Sunshine <[hidden email]>
Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 Documentation/git-worktree.txt         | 22 ++++++++++----
 builtin/worktree.c                     | 43 +++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  5 +++-
 t/t2028-worktree-move.sh (new +x)      | 54 ++++++++++++++++++++++++++++++++++
 4 files changed, 117 insertions(+), 7 deletions(-)
 create mode 100755 t/t2028-worktree-move.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 27feff6..004d770 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git worktree add' [-f] [--detach] [--checkout] [-b <new-branch>] <path> [<branch>]
 'git worktree list' [--porcelain]
+'git worktree lock' [--reason <string>] <path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
 
 DESCRIPTION
@@ -38,9 +39,8 @@ section "DETAILS" for more information.
 
 If a linked working tree is stored on a portable device or network share
 which is not always mounted, you can prevent its administrative files from
-being pruned by creating a file named 'locked' alongside the other
-administrative files, optionally containing a plain text reason that
-pruning should be suppressed. See section "DETAILS" for more information.
+being pruned by issuing the `git worktree lock` command, optionally
+specifying `--reason` to explain why the working tree is locked.
 
 COMMANDS
 --------
@@ -61,6 +61,14 @@ 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).
 
+lock::
+
+If a working tree is on a portable device or network share which
+is not always mounted, lock it to prevent its administrative
+files from being pruned automatically. This also prevents it from
+being moved or deleted. Optionally, specify a reason for the lock
+with `--reason`.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -110,6 +118,9 @@ OPTIONS
 --expire <time>::
  With `prune`, only expire unused working trees older than <time>.
 
+--reason <string>:
+ With `lock`, an explanation why the worktree is locked.
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's
@@ -150,7 +161,8 @@ instead.
 
 To prevent a $GIT_DIR/worktrees entry from being pruned (which
 can be useful in some situations, such as when the
-entry's working tree is stored on a portable device), add a file named
+entry's working tree is stored on a portable device), use the
+`git worktree lock` comamnd, which adds a file named
 'locked' to the entry's directory. The file contains the reason in
 plain text. For example, if a linked working tree's `.git` file points
 to `/path/main/.git/worktrees/test-next` then a file named
@@ -226,8 +238,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
-- `lock` to prevent automatic pruning of administrative files (for instance,
-  for a working tree on a portable device)
 
 GIT
 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index f9dac37..9008dcd 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -14,6 +14,7 @@
 static const char * const worktree_usage[] = {
  N_("git worktree add [<options>] <path> [<branch>]"),
  N_("git worktree list [<options>]"),
+ N_("git worktree lock [<options>] <path>"),
  N_("git worktree prune [<options>]"),
  NULL
 };
@@ -459,6 +460,46 @@ static int list(int ac, const char **av, const char *prefix)
  return 0;
 }
 
+static int lock_worktree(int ac, const char **av, const char *prefix)
+{
+ const char *reason = "", *old_reason;
+ struct option options[] = {
+ OPT_STRING(0, "reason", &reason, N_("string"),
+   N_("reason for locking")),
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf dst = STRBUF_INIT;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac != 1)
+ usage_with_options(worktree_usage, options);
+
+ strbuf_addstr(&dst, prefix_filename(prefix,
+    strlen(prefix),
+    av[0]));
+
+ worktrees = get_worktrees();
+ wt = find_worktree_by_path(worktrees, dst.buf);
+ strbuf_release(&dst);
+ if (!wt)
+ die(_("'%s' is not a working directory"), av[0]);
+ if (is_main_worktree(wt))
+ die(_("'%s' is a main working directory"), av[0]);
+
+ old_reason = is_worktree_locked(wt);
+ if (old_reason) {
+ if (*old_reason)
+ die(_("already locked, reason: %s"), old_reason);
+ die(_("already locked"));
+ }
+
+ write_file(git_common_path("worktrees/%s/locked", wt->id),
+   "%s", reason);
+ free_worktrees(worktrees);
+ return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
  struct option options[] = {
@@ -475,5 +516,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
  return prune(ac - 1, av + 1, prefix);
  if (!strcmp(av[1], "list"))
  return list(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "lock"))
+ return lock_worktree(ac - 1, av + 1, prefix);
  usage_with_options(worktree_usage, options);
 }
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 951a186..f88727d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2597,7 +2597,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
- local subcommands="add list prune"
+ local subcommands="add list lock prune"
  local subcommand="$(__git_find_on_cmdline "$subcommands")"
  if [ -z "$subcommand" ]; then
  __gitcomp "$subcommands"
@@ -2609,6 +2609,9 @@ _git_worktree ()
  list,--*)
  __gitcomp "--porcelain"
  ;;
+ lock,--*)
+ __gitcomp "--reason"
+ ;;
  prune,--*)
  __gitcomp "--dry-run --expire --verbose"
  ;;
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
new file mode 100755
index 0000000..aeac848
--- /dev/null
+++ b/t/t2028-worktree-move.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='test git worktree move, remove, lock and unlock'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit init &&
+ git worktree add source &&
+ git worktree list --porcelain | grep "^worktree" >actual &&
+ cat <<-EOF >expected &&
+ worktree $TRASH_DIRECTORY
+ worktree $TRASH_DIRECTORY/source
+ EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'lock main worktree' '
+ test_must_fail git worktree lock .
+'
+
+test_expect_success 'lock linked worktree' '
+ git worktree lock --reason hahaha source &&
+ echo hahaha >expected &&
+ test_cmp expected .git/worktrees/source/locked
+'
+
+test_expect_success 'lock linked worktree from another worktree' '
+ rm .git/worktrees/source/locked &&
+ git worktree add elsewhere &&
+ (
+ cd elsewhere &&
+ git worktree lock --reason hahaha ../source
+ ) &&
+ echo hahaha >expected &&
+ test_cmp expected .git/worktrees/source/locked
+'
+
+test_expect_success 'lock worktree twice' '
+ test_must_fail git worktree lock source &&
+ echo hahaha >expected &&
+ test_cmp expected .git/worktrees/source/locked
+'
+
+test_expect_success 'lock worktree twice (from the locked worktree)' '
+ (
+ cd source &&
+ test_must_fail git worktree lock .
+ ) &&
+ echo hahaha >expected &&
+ test_cmp expected .git/worktrees/source/locked
+'
+
+test_done
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 5/5] worktree: add "unlock" command

Duy Nguyen
In reply to this post by Duy Nguyen
Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 Documentation/git-worktree.txt         |  5 +++++
 builtin/worktree.c                     | 34 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 14 ++++++++++++++
 4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 004d770..e0f2605 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -13,6 +13,7 @@ SYNOPSIS
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason <string>] <path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree unlock' <path>
 
 DESCRIPTION
 -----------
@@ -73,6 +74,10 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+unlock::
+
+Unlock a worktree, allowing it to be pruned, moved or deleted.
+
 OPTIONS
 -------
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9008dcd..da9f771 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -16,6 +16,7 @@ static const char * const worktree_usage[] = {
  N_("git worktree list [<options>]"),
  N_("git worktree lock [<options>] <path>"),
  N_("git worktree prune [<options>]"),
+ N_("git worktree unlock <path>"),
  NULL
 };
 
@@ -500,6 +501,37 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
  return 0;
 }
 
+static int unlock_worktree(int ac, const char **av, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf dst = STRBUF_INIT;
+ int ret;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac != 1)
+ usage_with_options(worktree_usage, options);
+
+ strbuf_addstr(&dst, prefix_filename(prefix,
+    strlen(prefix),
+    av[0]));
+
+ worktrees = get_worktrees();
+ wt = find_worktree_by_path(worktrees, dst.buf);
+ strbuf_release(&dst);
+ if (!wt)
+ die(_("'%s' is not a working directory"), av[0]);
+ if (is_main_worktree(wt))
+ die(_("'%s' is a main working directory"), av[0]);
+ if (!is_worktree_locked(wt))
+ die(_("not locked"));
+ ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
+ free_worktrees(worktrees);
+ return ret;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
  struct option options[] = {
@@ -518,5 +550,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
  return list(ac - 1, av + 1, prefix);
  if (!strcmp(av[1], "lock"))
  return lock_worktree(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "unlock"))
+ return unlock_worktree(ac - 1, av + 1, prefix);
  usage_with_options(worktree_usage, options);
 }
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f88727d..0e3841d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2597,7 +2597,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
- local subcommands="add list lock prune"
+ local subcommands="add list lock prune unlock"
  local subcommand="$(__git_find_on_cmdline "$subcommands")"
  if [ -z "$subcommand" ]; then
  __gitcomp "$subcommands"
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index aeac848..1927537 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -51,4 +51,18 @@ test_expect_success 'lock worktree twice (from the locked worktree)' '
  test_cmp expected .git/worktrees/source/locked
 '
 
+test_expect_success 'unlock main worktree' '
+ test_must_fail git worktree unlock .
+'
+
+test_expect_success 'unlock linked worktree' '
+ git worktree unlock source &&
+ test_path_is_missing .git/worktrees/source/locked
+'
+
+test_expect_success 'unlock worktree twice' '
+ test_must_fail git worktree unlock source &&
+ test_path_is_missing .git/worktrees/source/locked
+'
+
 test_done
--
2.8.2.524.g6ff3d78

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 6/6] worktree: simplify prefixing paths

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 5:33 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
> This also makes slash conversion always happen on Windows (a side effect
> of prefix_filename). Which is a good thing.

Selling this patch as a mere simplification seems misguided
(especially as it's subjective whether this is indeed simpler). I'd
find it more easy to buy this change if it was described primarily as
benefiting Windows, in the same vein as[1].

More below...

[1]: http://git.661346.n2.nabble.com/PATCH-Do-not-skip-prefix-filename-when-there-is-no-prefix-tp7657038.html

> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -337,7 +337,7 @@ static int add(int ac, const char **av, const char *prefix)
>         if (ac < 1 || ac > 2)
>                 usage_with_options(worktree_usage, options);
>
> -       path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
> +       path = prefix_filename(prefix, strlen(prefix), av[0]);
>         branch = ac < 2 ? "HEAD" : av[1];
>
>         opts.force_new_branch = !!new_branch_force;
> @@ -467,6 +467,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
>
>         if (ac < 2)
>                 usage_with_options(worktree_usage, options);
> +       if (!prefix)
> +               prefix = "";

Considering that all the other existing git-worktree subcommands
already handle NULL prefix acceptably, it makes me a bit uncomfortable
that this "fix", which could be local to add(), leaks into all the
other subcommands, thus making the assumption that they (or other new
subcommands) will never care about the distinction between NULL and
"".

Not a big objection; just a minor niggle, probably not worth a re-roll.

>         if (!strcmp(av[1], "add"))
>                 return add(ac - 1, av + 1, prefix);
>         if (!strcmp(av[1], "prune"))
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/6] nd/worktree-cleanup-post-head-protection update

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 5:33 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:

> This is mostly commit message update plus
>
>  - dropping "--force" from the completion patch (a comment from Szeder
>    Gabor in a previous attempt of worktree completion support).
>
>  - the removal of clear_worktree() patch.
>
> I keep the completion patch anyway even though two versions of it were
> posted by Eric and Szeder (and didn't get discussed much) because I
> needed this for new commands and because I didn't aim high. It's meant
> to provide very basic completion support. Fancy stuff like ref
> completion can be added later by people who are more experienced in
> bash completion stuff.

Thanks. Dropping the clear_worktree() patch does indeed seem like a
good idea. I've re-read the entire series, and aside from my minor
comments on 6/6, everything looks fine and the series is:

    Reviewed by: Eric Sunshine <[hidden email]>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/5] worktree.c: add find_worktree_by_path()

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 6:43 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:

> So far we haven't needed to identify an existing worktree from command
> line. Future commands such as lock or move will need it. There are of
> course other options for identifying a worktree, for example by branch
> or even by internal id. They may be added later if proved useful.
>
> Helped-by: Eric Sunshine <[hidden email]>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/worktree.h b/worktree.h
> @@ -29,6 +29,12 @@ extern struct worktree **get_worktrees(void);
> +/*
> + * Search a worktree by its path. Paths are normalized internally.
> + */
> +extern struct worktree *find_worktree_by_path(struct worktree **list,
> +                                             const char *path_);
> +

Sorry for not noticing last time, but I'd probably have named this
argument 'path' rather than 'path_' here in the header.

Not worth a re-roll, of course.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/5] worktree.c: add is_worktree_locked()

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 5:53 AM, Duy Nguyen <[hidden email]> wrote:

> On Fri, May 13, 2016 at 11:52 PM, Eric Sunshine <[hidden email]> wrote:
>> Actually, I recall that when I suggested the idea of 'struct worktree'
>> and get_worktrees() to Mike that it would be natural for the structure
>> to have a 'locked' (or 'locked_reason') field, in which case the
>> reason could be stored there instead of in this static strbuf. In
>> fact, my idea at the time was that get_worktrees() would populate that
>> field automatically, rather than having to do so via a separate
>> on-demand function call as in this patch.
>
> I'm keeping this as a separate function for now. I don't like
> get_worktrees() doing extra work unless it has to. We soon will see
> the complete picture of "git worktree" then we can merge it back to
> get_worktrees() if it turns out checking "locked" is the common
> operation. is_worktree_locked() then may become a thin wrapper to
> access this "locked" field.

is_worktree_locked() could also just go away since the 'struct
worktree::locked' field would be non-NULL for locked, else NULL.

>>> +extern const char *is_worktree_locked(const struct worktree *wt);
>>
>> I was wondering if builtin/worktree.c:prune_worktree() should be
>> updated to invoke this new function instead of consulting
>> "worktrees/<id>/locked" manually, but I see that the entire "prune
>> worktrees" functionality in builting/worktree.c first needs to be
>> updated to the get_worktrees() API for that to happen.
>
> I thought about updating prune too. But it is in a bit special
> situation where it may need to consider invalid (or partly invalid)
> worktrees as well. So far worktree api is about valid worktrees only
> if I'm not mistaken and we probably should keep it that way, otherwise
> all callers may need to check "is this worktree valid" all over the
> place.

Yes and no. In addition to suggesting to Mike that 'struct worktree'
should have a 'locked' field, I also suggested a 'prunable' field
which would indicate whether a worktree was a candidate for pruning.
In fact, the field would be a 'const char *' where non-NULL would mean
prunable and give a reason (one of the reasons already determined by
builtin/worktree.c:prune_worktree()). Alternately it could be an enum.
Like the 'locked' (or 'lock_reason') field, 'prunable' (or
'prune_reason') is likely the sort of information which should be
presented by "git worktree list". And, as with 'locked', I think it
makes sense for the 'prunable' field to be populated automatically by
get_worktrees().

As for your concern about clients having to take care with valid vs.
invalid worktrees, get_worktrees() could be updated to return all
worktrees or only valid worktrees (for some definition of "valid").
"git worktree list" is an example of a client which would want to see
all worktrees assuming its updated to show 'locked' and 'prunable'
status.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 1/5] worktree.c: add find_worktree_by_path()

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 6:43 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
> So far we haven't needed to identify an existing worktree from command
> line. Future commands such as lock or move will need it. There are of
> course other options for identifying a worktree, for example by branch
> or even by internal id. They may be added later if proved useful.

Beyond the above methods for specifying a worktree, [1] adds
$(basename $path) as a possibility if not ambiguous. Excerpt:

    For git-worktree commands such as "lock", "mv", "remove", it
    likely will be nice to allow people to specify the linked
    worktree not only by path, but also by tag, and possibly even by
    $(basename $path) if not ambiguous.

[1]: http://article.gmane.org/gmane.comp.version-control.git/275528

> Helped-by: Eric Sunshine <[hidden email]>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/5] worktree: add "lock" command

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 6:31 AM, Duy Nguyen <[hidden email]> wrote:

> On Mon, May 16, 2016 at 7:09 AM, Eric Sunshine <[hidden email]> wrote:
>>> +       old_reason = is_worktree_locked(wt);
>>> +       if (old_reason) {
>>> +               if (*old_reason)
>>> +                       die(_("already locked, reason: %s"), old_reason);
>>> +               die(_("already locked, no reason"));
>>> +       }
>>
>> As a convenience, would it make sense to allow a worktree to be
>> re-locked with a different reason rather than erroring out?
>
> For now I think the user can just unlock then relock, after examining
> the previous reason and maybe incorporating it to the new reason. Yes
> there is a race but for these operations I don't think it really
> matters. If it's done often enough, we can lift this restriction.

Agreed. It's easy to imagine someone wanting to change the lock
message to correct a typo, but that's likely a rare occurrence, and
other conceivable reasons for wanting to change the message are
probably even more unusual, thus not worth worrying about now.

>>> +       write_file(git_common_path("worktrees/%s/locked", wt->id),
>>> +                  "%s", reason);
>>> +       return 0;
>>> +}
>>
>> This is a tangent, but it would be nice for the "git worktree list"
>> command to show whether a worktree is locked, and the reason (if
>> available), both in pretty and porcelain formats. (That was another
>> reason why I suggested to Mike, back when he was adding the "list"
>> command, that 'struct worktree' should have a 'locked' field and
>> get_worktrees() should populate it automatically.)
>
> Yes. And a good reason for get_worktrees() to read lock status
> automatically too. I will not do it right away though or at least
> until after I'm done with move/remove and the shared config file
> problem.

For the record (for future readers of this thread), [1] itemized
various useful bits of information that would be handy as fields in
'struct worktree' and populated automatically by get_worktrees(), all
of which (I think) could be nice to have in the output of "git
worktree list". Excerpt:

    For git-worktree commands such as "lock", "mv", "remove", it
    likely will be nice to allow people to specify the linked
    worktree not only by path, but also by tag, and possibly even by
    $(basename $path) if not ambiguous. Therefore, to support such
    usage, at minimum, I think you also want to show the worktree's
    tag (d->d_name) in addition to the path.

    Other information which would be nice to display for each
    worktree (possibly controlled by a --verbose flag):

       * the checked out branch or detached head
       * whether it is locked
            - the lock reason (if available)
            - and whether the worktree is currently accessible
        * whether it can be pruned
            - and the prune reason if so

    The prune reason could be obtained by factoring out the
    reason-determination code from worktree.c:prune_worktree() to
    make it re-usable.

[1]: http://article.gmane.org/gmane.comp.version-control.git/275528
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 6/6] worktree: simplify prefixing paths

Eric Sunshine
In reply to this post by Eric Sunshine
On Sun, May 22, 2016 at 7:32 PM, Eric Sunshine <[hidden email]> wrote:

> On Sun, May 22, 2016 at 5:33 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>> @@ -467,6 +467,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
>>         if (ac < 2)
>>                 usage_with_options(worktree_usage, options);
>> +       if (!prefix)
>> +               prefix = "";
>
> Considering that all the other existing git-worktree subcommands
> already handle NULL prefix acceptably, it makes me a bit uncomfortable
> that this "fix", which could be local to add(), leaks into all the
> other subcommands, thus making the assumption that they (or other new
> subcommands) will never care about the distinction between NULL and
> "".
>
> Not a big objection; just a minor niggle, probably not worth a re-roll.

Okay, I see that you're taking advantage of the prefix="" in patch
5/6, as well, so it's not benefitting only add().
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 4/5] worktree: add "lock" command

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 6:43 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:

> Helped-by: Eric Sunshine <[hidden email]>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -150,7 +161,8 @@ instead.
>
>  To prevent a $GIT_DIR/worktrees entry from being pruned (which
>  can be useful in some situations, such as when the
> -entry's working tree is stored on a portable device), add a file named
> +entry's working tree is stored on a portable device), use the
> +`git worktree lock` comamnd, which adds a file named

s/comamnd/command/

>  'locked' to the entry's directory. The file contains the reason in
>  plain text. For example, if a linked working tree's `.git` file points
>  to `/path/main/.git/worktrees/test-next` then a file named
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -0,0 +1,54 @@
> +test_expect_success 'lock linked worktree from another worktree' '
> +       rm .git/worktrees/source/locked &&
> +       git worktree add elsewhere &&
> +       (
> +               cd elsewhere &&
> +               git worktree lock --reason hahaha ../source

Could use "git -C elsewhere worktree lock ..." and drop the subshell,
but not worth a re-roll.

> +       ) &&
> +       echo hahaha >expected &&
> +       test_cmp expected .git/worktrees/source/locked
> +'
> +
> +test_expect_success 'lock worktree twice (from the locked worktree)' '
> +       (
> +               cd source &&
> +               test_must_fail git worktree lock .

Likewise, -C.

> +       ) &&
> +       echo hahaha >expected &&
> +       test_cmp expected .git/worktrees/source/locked
> +'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 5/5] worktree: add "unlock" command

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 6:43 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:

> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -500,6 +501,37 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
> +static int unlock_worktree(int ac, const char **av, const char *prefix)
> +{
> +       [...]
> +       wt = find_worktree_by_path(worktrees, dst.buf);
> +       strbuf_release(&dst);
> +       if (!wt)
> +               die(_("'%s' is not a working directory"), av[0]);
> +       if (is_main_worktree(wt))
> +               die(_("'%s' is a main working directory"), av[0]);
> +       if (!is_worktree_locked(wt))
> +               die(_("not locked"));

Could be _("'%s' is not locked"), but not worth a re-roll.

> +       ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
> +       free_worktrees(worktrees);
> +       return ret;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/5] worktree lock/unlock

Eric Sunshine
In reply to this post by Duy Nguyen
On Sun, May 22, 2016 at 6:43 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
> This should address all of Eric's comments (thanks!). An extra change
> I made is free_worktrees() at the end of {,un}lock_worktree() to avoid
> leaking. This series depends on nd/worktree-cleanup-post-head-protection.

Thanks, this addresses all my comments from the previous round (aside
from the suggestion to add a 'locked' field to 'struct worktree' and
populate it automatically, which you elected to defer for the
present).

I re-read the entire series and, aside from the typo in the
documentation update of patch 4/5, everything looks fine, and the
series is:

    Reviewed-by: Eric Sunshine <[hidden email]>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/5] worktree lock/unlock

Duy Nguyen
On Mon, May 23, 2016 at 11:51 AM, Eric Sunshine <[hidden email]> wrote:
> On Sun, May 22, 2016 at 6:43 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>> This should address all of Eric's comments (thanks!). An extra change
>> I made is free_worktrees() at the end of {,un}lock_worktree() to avoid
>> leaking. This series depends on nd/worktree-cleanup-post-head-protection.
>
> Thanks, this addresses all my comments from the previous round (aside
> from the suggestion to add a 'locked' field to 'struct worktree' and
> populate it automatically, which you elected to defer for the
> present).

If there's another re-roll (likely so), I'm tempted to do that too as
it's quite clear now that "locked" belongs in struct worktree.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 6/6] worktree: simplify prefixing paths

Duy Nguyen
In reply to this post by Eric Sunshine
On Mon, May 23, 2016 at 11:31 AM, Eric Sunshine <[hidden email]> wrote:

> On Sun, May 22, 2016 at 7:32 PM, Eric Sunshine <[hidden email]> wrote:
>> On Sun, May 22, 2016 at 5:33 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>>> @@ -467,6 +467,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
>>>         if (ac < 2)
>>>                 usage_with_options(worktree_usage, options);
>>> +       if (!prefix)
>>> +               prefix = "";
>>
>> Considering that all the other existing git-worktree subcommands
>> already handle NULL prefix acceptably, it makes me a bit uncomfortable
>> that this "fix", which could be local to add(), leaks into all the
>> other subcommands, thus making the assumption that they (or other new
>> subcommands) will never care about the distinction between NULL and
>> "".
>>
>> Not a big objection; just a minor niggle, probably not worth a re-roll.
>
> Okay, I see that you're taking advantage of the prefix="" in patch
> 5/6, as well, so it's not benefitting only add().

Yep. 'add' so far is the only command taking paths. The next four,
lock, unlock, move and delete, all deal with paths and need to deal
with NULL prefix otherwise.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
1 ... 3456