[PATCH 00/25] worktree lock, move, remove and unlock

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[PATCH 20/25] worktree: simplify prefixing paths

Duy Nguyen
Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/worktree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index e041d7f..a9e91ab 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -330,7 +330,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;
@@ -460,6 +460,8 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
 
  if (ac < 2)
  usage_with_options(worktree_usage, options);
+ if (!prefix)
+ prefix = "";
  if (!strcmp(av[1], "add"))
  return add(ac - 1, av + 1, prefix);
  if (!strcmp(av[1], "prune"))
--
2.8.0.rc0.210.gd302cd2

--
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 21/25] worktree: add "lock" 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         | 12 ++++++++--
 builtin/worktree.c                     | 41 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  5 ++++-
 t/t2028-worktree-move.sh (new +x)      | 34 ++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 3 deletions(-)
 create mode 100755 t/t2028-worktree-move.sh

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 1c9d7c1..9f0c9f0 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 list' [--porcelain]
+'git worktree lock' [--reason <string>] <path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
 
 DESCRIPTION
@@ -61,6 +62,12 @@ 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::
+
+When a worktree is locked, it cannot be pruned, moved or deleted. For
+example, if the worktree is on portable device that is not available
+when "git worktree <command>" is executed.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -104,6 +111,9 @@ OPTIONS
 --expire <time>::
  With `prune`, only expire unused working trees older than <time>.
 
+--reason <string>:
+ An explanation why the worktree is locked.
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's
@@ -220,8 +230,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 a9e91ab..736caff 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
 };
@@ -452,6 +453,44 @@ 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);
+ 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, no reason"));
+ }
+
+ write_file(git_common_path("worktrees/%s/locked", wt->id),
+   "%s", reason);
+ return 0;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
  struct option options[] = {
@@ -468,5 +507,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 f66db2d..cdae408 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,7 +2596,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"
@@ -2608,6 +2608,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..97434be
--- /dev/null
+++ b/t/t2028-worktree-move.sh
@@ -0,0 +1,34 @@
+#!/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 worktree twice' '
+ test_must_fail git worktree lock source &&
+ echo hahaha >expected &&
+ test_cmp expected .git/worktrees/source/locked
+'
+
+test_done
--
2.8.0.rc0.210.gd302cd2

--
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 22/25] 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                     | 31 +++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 14 ++++++++++++++
 4 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9f0c9f0..8315e5b 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
 -----------
@@ -72,6 +73,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 736caff..13f4083 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
 };
 
@@ -491,6 +492,34 @@ 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;
+
+ 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);
+ 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"));
+
+ return unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
  struct option options[] = {
@@ -509,5 +538,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 cdae408..d7d92ac 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,7 +2596,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 97434be..f4b2816 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -31,4 +31,18 @@ test_expect_success 'lock worktree twice' '
  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.0.rc0.210.gd302cd2

--
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 23/25] worktree: add "move" commmand

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         |  6 +++-
 builtin/worktree.c                     | 60 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  2 +-
 t/t2028-worktree-move.sh               | 29 ++++++++++++++++
 4 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 8315e5b..a302f0a 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
 'git worktree add' [-f] [--detach] [-b <new-branch>] <path> [<branch>]
 'git worktree list' [--porcelain]
 'git worktree lock' [--reason <string>] <path>
+'git worktree move' <path> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
 'git worktree unlock' <path>
 
@@ -69,6 +70,10 @@ When a worktree is locked, it cannot be pruned, moved or deleted. For
 example, if the worktree is on portable device that is not available
 when "git worktree <command>" is executed.
 
+move::
+
+Move a worktree to a new location. Note that the main worktree cannot be moved.
+
 prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
@@ -234,7 +239,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
 
 GIT
 ---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 13f4083..a988913 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -15,6 +15,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 move <path> <new-path>"),
  N_("git worktree prune [<options>]"),
  N_("git worktree unlock <path>"),
  NULL
@@ -520,6 +521,63 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
  return unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
 }
 
+static int move_worktree(int ac, const char **av, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf dst = STRBUF_INIT;
+ struct strbuf src = STRBUF_INIT;
+ const char *reason;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac != 2)
+ usage_with_options(worktree_usage, options);
+
+ strbuf_addstr(&dst, prefix_filename(prefix,
+    strlen(prefix),
+    av[1]));
+ if (file_exists(dst.buf))
+ die(_("target '%s' already exists"), av[1]);
+
+ worktrees = get_worktrees();
+ strbuf_addstr(&src, prefix_filename(prefix,
+     strlen(prefix),
+     av[0]));
+ wt = find_worktree_by_path(worktrees, src.buf);
+ 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 ((reason = is_worktree_locked(wt))) {
+ if (*reason)
+ die(_("already locked, reason: %s"), reason);
+ die(_("already locked, no reason"));
+ }
+ if (validate_worktree(wt, 0))
+ return -1;
+
+ /*
+ * First try. Atomically move, and probably cheaper, if both
+ * source and target are on the same file system.
+ */
+ if (rename(src.buf, dst.buf) == -1) {
+ if (errno != EXDEV)
+ die_errno(_("failed to move '%s' to '%s'"),
+  src.buf, dst.buf);
+
+ /* second try.. */
+ if (copy_dir_recursively(src.buf, dst.buf))
+ die(_("failed to copy '%s' to '%s'"),
+    src.buf, dst.buf);
+ else
+ (void)remove_dir_recursively(&src, 0);
+ }
+
+ return update_worktree_location(wt, dst.buf);
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
  struct option options[] = {
@@ -540,5 +598,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
  return lock_worktree(ac - 1, av + 1, prefix);
  if (!strcmp(av[1], "unlock"))
  return unlock_worktree(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "move"))
+ return move_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 d7d92ac..022d990 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,7 +2596,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
- local subcommands="add list lock prune unlock"
+ local subcommands="add list lock move 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 f4b2816..83fbab5 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -45,4 +45,33 @@ test_expect_success 'unlock worktree twice' '
  test_path_is_missing .git/worktrees/source/locked
 '
 
+test_expect_success 'move non-worktree' '
+ mkdir abc &&
+ test_must_fail git worktree move abc def
+'
+
+test_expect_success 'move locked worktree' '
+ git worktree lock source &&
+ test_must_fail git worktree move source destination &&
+ git worktree unlock source
+'
+
+test_expect_success 'move worktree' '
+ git worktree move source destination &&
+ test_path_is_missing source &&
+ git worktree list --porcelain | grep "^worktree" >actual &&
+ cat <<-EOF >expected &&
+ worktree $TRASH_DIRECTORY
+ worktree $TRASH_DIRECTORY/destination
+ EOF
+ test_cmp expected actual &&
+ git -C destination log --format=%s >actual2 &&
+ echo init >expected2 &&
+ test_cmp expected2 actual2
+'
+
+test_expect_success 'move main worktree' '
+ test_must_fail git worktree move . def
+'
+
 test_done
--
2.8.0.rc0.210.gd302cd2

--
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 24/25] worktree move: accept destination as directory

Duy Nguyen
In reply to this post by Duy Nguyen
Similar to "mv a b/", which is actually "mv a b/a", we extract basename
of source worktree and create a directory of the same name at
destination if dst path is a directory.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/worktree.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index a988913..5402a4e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -538,7 +538,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
  strbuf_addstr(&dst, prefix_filename(prefix,
     strlen(prefix),
     av[1]));
- if (file_exists(dst.buf))
+ if (is_directory(dst.buf))
+ /*
+ * keep going, dst will be appended after we get the
+ * source's absolute path
+ */
+ ;
+ else if (file_exists(dst.buf))
  die(_("target '%s' already exists"), av[1]);
 
  worktrees = get_worktrees();
@@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
  if (validate_worktree(wt, 0))
  return -1;
 
+ if (is_directory(dst.buf)) {
+ const char *sep = strrchr(wt->path, '/');
+
+ if (!sep)
+ die(_("could not figure out destination name from '%s'"),
+    wt->path);
+ strbuf_addstr(&dst, sep);
+ if (file_exists(dst.buf))
+ die(_("target '%s' already exists"), dst.buf);
+ }
+
  /*
  * First try. Atomically move, and probably cheaper, if both
  * source and target are on the same file system.
--
2.8.0.rc0.210.gd302cd2

--
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 25/25] worktree: add "remove" 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         | 22 +++++----
 builtin/worktree.c                     | 81 ++++++++++++++++++++++++++++++++++
 contrib/completion/git-completion.bash |  5 ++-
 t/t2028-worktree-move.sh               | 26 +++++++++++
 4 files changed, 124 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index a302f0a..60ad465 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 'git worktree lock' [--reason <string>] <path>
 'git worktree move' <path> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]
+'git worktree remove' [--force] <path>
 'git worktree unlock' <path>
 
 DESCRIPTION
@@ -78,6 +79,14 @@ prune::
 
 Prune working tree information in $GIT_DIR/worktrees.
 
+remove::
+
+Remove a worktree. Only clean worktrees, no untracked files and no
+modification in tracked files, can be removed. Unclean worktrees can
+be removed with `--force`. Main worktree cannot be removed. It needs
+to be converted to a linked worktree first by moving the repository
+away.
+
 unlock::
 
 Unlock a worktree, allowing it to be pruned, moved or deleted.
@@ -87,9 +96,10 @@ OPTIONS
 
 -f::
 --force::
- By default, `add` refuses to create a new working tree when `<branch>`
- is already checked out by another working tree. This option overrides
- that safeguard.
+ By default, `add` refuses to create a new working tree when
+ `<branch>` is already checked out by another working tree and
+ `remove` refuses to remove an unclean worktree. This option
+ overrides that safeguard.
 
 -b <new-branch>::
 -B <new-branch>::
@@ -234,12 +244,6 @@ Multiple checkout in general is still experimental, and the support
 for submodules is incomplete. It is NOT recommended to make multiple
 checkouts of a superproject.
 
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
-  warn if the working tree is dirty)
-
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5402a4e..084f8fd 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {
  N_("git worktree lock [<options>] <path>"),
  N_("git worktree move <path> <new-path>"),
  N_("git worktree prune [<options>]"),
+ N_("git worktree remove [<options>] <path>"),
  N_("git worktree unlock <path>"),
  NULL
 };
@@ -595,6 +596,84 @@ static int move_worktree(int ac, const char **av, const char *prefix)
  return update_worktree_location(wt, dst.buf);
 }
 
+static int remove_worktree(int ac, const char **av, const char *prefix)
+{
+ int force = 0;
+ struct option options[] = {
+ OPT_BOOL(0, "force", &force,
+ N_("force removing even if the worktree is dirty")),
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf dst = STRBUF_INIT;
+ const char *reason;
+ int ret = 0;
+
+ 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);
+ 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 ((reason = is_worktree_locked(wt))) {
+ if (*reason)
+ die(_("already locked, reason: %s"), reason);
+ die(_("already locked, no reason"));
+ }
+ if (validate_worktree(wt, 0))
+ return -1;
+
+ if (!force) {
+ struct argv_array child_env = ARGV_ARRAY_INIT;
+ struct child_process cp;
+ char buf[1];
+
+ argv_array_pushf(&child_env, "%s=%s/.git",
+ GIT_DIR_ENVIRONMENT, wt->path);
+ argv_array_pushf(&child_env, "%s=%s",
+ GIT_WORK_TREE_ENVIRONMENT, wt->path);
+ memset(&cp, 0, sizeof(cp));
+ argv_array_pushl(&cp.args, "status", "--porcelain", NULL);
+ cp.env = child_env.argv;
+ cp.git_cmd = 1;
+ cp.dir = wt->path;
+ cp.out = -1;
+ ret = start_command(&cp);
+ if (ret)
+ die_errno(_("failed to run git-status on '%s', code %d"),
+  av[0], ret);
+ ret = xread(cp.out, buf, sizeof(buf));
+ if (ret)
+ die(_("'%s' is dirty, use --force to delete it"), av[0]);
+ close(cp.out);
+ ret = finish_command(&cp);
+ if (ret)
+ die_errno(_("failed to run git-status on '%s', code %d"),
+  av[0], ret);
+ }
+ if (remove_dir_recursively(&dst, 0)) {
+ sys_error(_("failed to delete '%s'"), wt->path);
+ ret = -1;
+ }
+ strbuf_reset(&dst);
+ strbuf_addstr(&dst, git_common_path("worktrees/%s", wt->id));
+ if (remove_dir_recursively(&dst, 0)) {
+ sys_error(_("failed to delete '%s'"), dst.buf);
+ ret = -1;
+ }
+ strbuf_release(&dst);
+ free_worktrees(worktrees);
+ return ret;
+}
+
 int cmd_worktree(int ac, const char **av, const char *prefix)
 {
  struct option options[] = {
@@ -617,5 +696,7 @@ int cmd_worktree(int ac, const char **av, const char *prefix)
  return unlock_worktree(ac - 1, av + 1, prefix);
  if (!strcmp(av[1], "move"))
  return move_worktree(ac - 1, av + 1, prefix);
+ if (!strcmp(av[1], "remove"))
+ return remove_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 022d990..6fb45ea 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2596,7 +2596,7 @@ _git_whatchanged ()
 
 _git_worktree ()
 {
- local subcommands="add list lock move prune unlock"
+ local subcommands="add list lock move prune remove unlock"
  local subcommand="$(__git_find_on_cmdline "$subcommands")"
  if [ -z "$subcommand" ]; then
  __gitcomp "$subcommands"
@@ -2614,6 +2614,9 @@ _git_worktree ()
  prune,--*)
  __gitcomp "--dry-run --expire --verbose"
  ;;
+ remove,--*)
+ __gitcomp "--force"
+ ;;
  *)
  ;;
  esac
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 83fbab5..668ee05 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -74,4 +74,30 @@ test_expect_success 'move main worktree' '
  test_must_fail git worktree move . def
 '
 
+test_expect_success 'remove main worktree' '
+ test_must_fail git worktree remove .
+'
+
+test_expect_success 'remove locked worktree' '
+ git worktree lock destination &&
+ test_must_fail git worktree remove destination &&
+ git worktree unlock destination
+'
+
+test_expect_success 'remove worktree with dirty tracked file' '
+ echo dirty >>destination/init.t &&
+ test_must_fail git worktree remove destination
+'
+
+test_expect_success 'remove worktree with untracked file' '
+ git -C destination checkout init.t &&
+ : >destination/untracked &&
+ test_must_fail git worktree remove destination
+'
+
+test_expect_success 'force remove worktree with untracked file' '
+ git worktree remove --force destination &&
+ test_path_is_missing destination
+'
+
 test_done
--
2.8.0.rc0.210.gd302cd2

--
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 00/25] worktree lock, move, remove and unlock

Junio C Hamano
In reply to this post by Duy Nguyen
Nguyễn Thái Ngọc Duy  <[hidden email]> writes:

> This is basically a resend from last time, which happened during rc
> time.

It would have made them a much more pleasant read if you re-read
them during that time and added the missing "why" to many of the
commit log message.

> It adds 4 more commands, basically cleaning up the "TODO" list
> in git-worktree.txt.
>
> So far I've only actually used move and remove (and maybe unlock once
> because worktree-add failed on me and I had to unlock it manually).
> And I don't get to move worktrees a lot either so not really extensive
> testing.
>
>   [01/25] usage.c: move format processing out of die_errno()
>   [02/25] usage.c: add sys_error() that prints strerror() automatically

This looks parallel to die_errno(); isn't error_errno() a better name?

>   [03/25] copy.c: import copy_file() from busybox
>   [04/25] copy.c: delete unused code in copy_file()
>   [05/25] copy.c: convert bb_(p)error_msg to (sys_)error
>   [06/25] copy.c: style fix
>   [07/25] copy.c: convert copy_file() to copy_dir_recursively()

Somewhere among these, there needs to be a single overview of why we
want "cp" implementation of busybox, e.g. what part of "cp" we want?
the whole thing?  or "because this is to be used from this and that
codepaths to make copy of these things, we only need these parts and
can remove other features like this and that?"

>   [08/25] completion: support git-worktree
>   [09/25] git-worktree.txt: keep subcommand listing in alphabetical order

I'd defer doing this immediately before 21 if I were doing this
series.

Offhand, I think it makes it easier to look things up in an
alphabetical list in the description section, but it probably is
easier to get an overview if the synopsis part groups things along
concepts and/or lists things along the order in typical workflows
(e.g. "create, list, rename, remove" would be a list along
lifecycle), not alphabetical.

But such judgement is better done when we know what are the final
elements that are to be listed, i.e. closer to where new things are
introduced.  This is especially true, as the log messages of patches
leading to 21 are all sketchy and do not give the readers a good
birds-view picture.

>   [10/25] path.c: add git_common_path() and strbuf_git_common_path()

Write "what for" when adding a new API function.  "Wanting to learn
X is very common and there are many existing code or new code that
repeats sequence A, B and C to compute it.  Give a helper function
to do so to refactor the existing codepaths" or something like that?

Move some part of [12/25] that is not about "store 'id'" but is
about this refactoring to this commit.

>   [11/25] worktree.c: use is_dot_or_dotdot()
>   [12/25] worktree.c: store "id" instead of "git_dir"

It is better to have these (and other conceptually "small and
obvious" ones) as preliminary clean-up to make the main body of the
series that may need to go through iterations smaller.

>   [13/25] worktree.c: add clear_worktree()
>   [14/25] worktree.c: add find_worktree_by_path()
>   [15/25] worktree.c: add is_main_worktree()
>   [16/25] worktree.c: add validate_worktree()
>   [17/25] worktree.c: add update_worktree_location()
>   [18/25] worktree.c: add is_worktree_locked()
>   [19/25] worktree: avoid 0{40}, too many zeroes, hard to read
>   [20/25] worktree: simplify prefixing paths
>   [21/25] worktree: add "lock" command
>   [22/25] worktree: add "unlock" command
>   [23/25] worktree: add "move" commmand
>   [24/25] worktree move: accept destination as directory
>   [25/25] worktree: add "remove" command
>
> Total 11 files changed, 1028 insertions(+), 48 deletions(-)
--
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 00/25] worktree lock, move, remove and unlock

Duy Nguyen
On Thu, Apr 14, 2016 at 11:08 PM, Junio C Hamano <[hidden email]> wrote:
> Nguyễn Thái Ngọc Duy  <[hidden email]> writes:
>
>> This is basically a resend from last time, which happened during rc
>> time.
>
> It would have made them a much more pleasant read if you re-read
> them during that time and added the missing "why" to many of the
> commit log message.

Hmm... I thought I didn't receive any comments last time.

>> It adds 4 more commands, basically cleaning up the "TODO" list
>> in git-worktree.txt.
>>
>> So far I've only actually used move and remove (and maybe unlock once
>> because worktree-add failed on me and I had to unlock it manually).
>> And I don't get to move worktrees a lot either so not really extensive
>> testing.
>>
>>   [01/25] usage.c: move format processing out of die_errno()
>>   [02/25] usage.c: add sys_error() that prints strerror() automatically
>
> This looks parallel to die_errno(); isn't error_errno() a better name?

To me, no. Duplicating the "err" looks weird. error_no() does not look
good either. Though there's a couple of warning(..., strerror()),
which could become warning_errno(). Then maybe error_errno() makes
more sense because all three follow the same naming convention.

>>   [03/25] copy.c: import copy_file() from busybox
>>   [04/25] copy.c: delete unused code in copy_file()
>>   [05/25] copy.c: convert bb_(p)error_msg to (sys_)error
>>   [06/25] copy.c: style fix
>>   [07/25] copy.c: convert copy_file() to copy_dir_recursively()
>
> Somewhere among these, there needs to be a single overview of why we
> want "cp" implementation of busybox, e.g. what part of "cp" we want?
> the whole thing?  or "because this is to be used from this and that
> codepaths to make copy of these things, we only need these parts and
> can remove other features like this and that?"

We need directory move functionality. In the worst case when
rename(<dir>, <dir>) wouldn't do the job, we have to fall back to
copying the whole directory over, preserving metadata as much as
possible, then delete the old directory. I don't want to write new
code for this because I think it shows in busybox code that there are
pitfalls in dealing with filesystems. And I don't want to fall back to
/bin/cp either. Windows won't have one (long term I hope we won't need
msys) and *nix is not famous for consistent command line behavior.
Plus, if we want to clean up a failed cp operation, it's easier to do
it in core by keeping track of what files have been copied.

>>   [08/25] completion: support git-worktree
>>   [09/25] git-worktree.txt: keep subcommand listing in alphabetical order
>
> I'd defer doing this immediately before 21 if I were doing this
> series.

Will do.

> Offhand, I think it makes it easier to look things up in an
> alphabetical list in the description section, but it probably is
> easier to get an overview if the synopsis part groups things along
> concepts and/or lists things along the order in typical workflows
> (e.g. "create, list, rename, remove" would be a list along
> lifecycle), not alphabetical.
>
> But such judgement is better done when we know what are the final
> elements that are to be listed, i.e. closer to where new things are
> introduced.  This is especially true, as the log messages of patches
> leading to 21 are all sketchy and do not give the readers a good
> birds-view picture.

Well. I think all the commands are there now at the end of this
series. So we have add, list, prune, move, remove, lock and unlock. I
guess we can group list/add/move/remove together and the rest as
support commands. I might add "git worktree migrate" for converting
between worktree v0 and v1. But it's not clear yet.

>>   [10/25] path.c: add git_common_path() and strbuf_git_common_path()
>
> Write "what for" when adding a new API function.  "Wanting to learn
> X is very common and there are many existing code or new code that
> repeats sequence A, B and C to compute it.  Give a helper function
> to do so to refactor the existing codepaths" or something like that?

Pretty much for convenience. Will add some more in commit message.

> Move some part of [12/25] that is not about "store 'id'" but is
> about this refactoring to this commit.
>
>>   [11/25] worktree.c: use is_dot_or_dotdot()
>>   [12/25] worktree.c: store "id" instead of "git_dir"
>
> It is better to have these (and other conceptually "small and
> obvious" ones) as preliminary clean-up to make the main body of the
> series that may need to go through iterations smaller.

Sure.
--
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 00/25] worktree lock, move, remove and unlock

Junio C Hamano
On Thu, Apr 14, 2016 at 5:40 PM, Duy Nguyen <[hidden email]> wrote:

> On Thu, Apr 14, 2016 at 11:08 PM, Junio C Hamano <[hidden email]> wrote:
>> Nguyễn Thái Ngọc Duy  <[hidden email]> writes:
>>
>>> This is basically a resend from last time, which happened during rc
>>> time.
>>
>> It would have made them a much more pleasant read if you re-read
>> them during that time and added the missing "why" to many of the
>> commit log message.
>
> Hmm... I thought I didn't receive any comments last time.

I think you've been here long enough to know that absense of comments
does not mean anything more than that: lack of interest at that moment in time.

>> This looks parallel to die_errno(); isn't error_errno() a better name?
>
> To me, no. Duplicating the "err" looks weird. error_no() does not look
> good either. Though there's a couple of warning(..., strerror()),
> which could become warning_errno(). Then maybe error_errno() makes
> more sense because all three follow the same naming convention.

So in the end error_errno() would be a better name to you after all ;-)
I agree the stuttering sound coming from repeating error twice feels
somewhat odd, but warning_errno() would be so natural and obvious
future direction, so...

>>>   [03/25] copy.c: import copy_file() from busybox
>>>   [04/25] copy.c: delete unused code in copy_file()
>>>   [05/25] copy.c: convert bb_(p)error_msg to (sys_)error
>>>   [06/25] copy.c: style fix
>>>   [07/25] copy.c: convert copy_file() to copy_dir_recursively()
>>
>> Somewhere among these, there needs to be a single overview of why we
>> want "cp" implementation of busybox, e.g. what part of "cp" we want?
>> the whole thing?  or "because this is to be used from this and that
>> codepaths to make copy of these things, we only need these parts and
>> can remove other features like this and that?"
>
> We need directory move functionality.

Yeah, I know all that but you do not want to explain that to me only when
I asked in a mailing list response. You want to get into the habit of having
that in the log message to help reviewers, not just me, before they ask such
a question.

>> But such judgement is better done when we know what are the final
>> elements that are to be listed, i.e. closer to where new things are
>> introduced.  This is especially true, as the log messages of patches
>> leading to 21 are all sketchy and do not give the readers a good
>> birds-view picture.
>
> Well. I think all the commands are there now at the end of this
> series. So we have add, list, prune, move, remove, lock and unlock. I
> guess we can group list/add/move/remove together and the rest as
> support commands. I might add "git worktree migrate" for converting
> between worktree v0 and v1. But it's not clear yet.

Just so that there is no confusion, I am not opposed to reordering.
I was just saying that the decision on what the right ordering is can
be easier to make when the readers more or less know what the final
set of things in the set are, and because that becomes only clear when
they start reading 21, and because the log messages of patches
leading to 21 do not show the direction clearly enough, it is hard to
make that judgment at this point so early in the series.
--
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 24/25] worktree move: accept destination as directory

Eric Sunshine
In reply to this post by Duy Nguyen
On Wed, Apr 13, 2016 at 9:15 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:

> Similar to "mv a b/", which is actually "mv a b/a", we extract basename
> of source worktree and create a directory of the same name at
> destination if dst path is a directory.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -538,7 +538,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
> -       if (file_exists(dst.buf))
> +       if (is_directory(dst.buf))
> +               /*
> +                * keep going, dst will be appended after we get the
> +                * source's absolute path
> +                */
> +               ;
> +       else if (file_exists(dst.buf))
>                 die(_("target '%s' already exists"), av[1]);
> @@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
> +       if (is_directory(dst.buf)) {
> +               const char *sep = strrchr(wt->path, '/');

Does this need to take Windows into account? Perhaps git_find_last_dir_sep()?

> +
> +               if (!sep)
> +                       die(_("could not figure out destination name from '%s'"),
> +                           wt->path);
> +               strbuf_addstr(&dst, sep);
> +               if (file_exists(dst.buf))
> +                       die(_("target '%s' already exists"), dst.buf);
> +       }
--
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 24/25] worktree move: accept destination as directory

Duy Nguyen
On Wed, May 11, 2016 at 11:43 AM, Eric Sunshine <[hidden email]> wrote:

> On Wed, Apr 13, 2016 at 9:15 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>> Similar to "mv a b/", which is actually "mv a b/a", we extract basename
>> of source worktree and create a directory of the same name at
>> destination if dst path is a directory.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -538,7 +538,13 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>> -       if (file_exists(dst.buf))
>> +       if (is_directory(dst.buf))
>> +               /*
>> +                * keep going, dst will be appended after we get the
>> +                * source's absolute path
>> +                */
>> +               ;
>> +       else if (file_exists(dst.buf))
>>                 die(_("target '%s' already exists"), av[1]);
>> @@ -558,6 +564,17 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>> +       if (is_directory(dst.buf)) {
>> +               const char *sep = strrchr(wt->path, '/');
>
> Does this need to take Windows into account?

wt->path comes from $GIT_DIR/worktrees/xxx/gitdir, which normally uses
forward slashes, so we should be safe. We already rely on forward
slashes in get_linked_worktree()

> Perhaps git_find_last_dir_sep()?

But this is probably a good thing to do anyway, to be more robust in
future. But it could confuse the reader later on why it's necessary
when backward slashes can't exist in wt->path. I don't know. Maybe
just have a comment that backward slashes can't never appear here?

There is also a potential problem with find_worktree_by_path(). I was
counting on real_path() to normalize paths and could simply do
strcmp_icase (or its new name, fspathcmp). But real_path() does not
seem to convert unify slashes. I will need to have a closer look at
this. Hopefully prefix_filename() already makes sure everything uses
forward slashes. Or maybe we could improve fspathcmp to see '/' and
'\' the same thing on Windows.
--
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 24/25] worktree move: accept destination as directory

Eric Sunshine
On Wed, May 11, 2016 at 9:34 AM, Duy Nguyen <[hidden email]> wrote:

> On Wed, May 11, 2016 at 11:43 AM, Eric Sunshine <[hidden email]> wrote:
>> On Wed, Apr 13, 2016 at 9:15 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>>> +       if (is_directory(dst.buf)) {
>>> +               const char *sep = strrchr(wt->path, '/');
>>
>> Does this need to take Windows into account?
>
> wt->path comes from $GIT_DIR/worktrees/xxx/gitdir, which normally uses
> forward slashes, so we should be safe. We already rely on forward
> slashes in get_linked_worktree()
>
>> Perhaps git_find_last_dir_sep()?
>
> But this is probably a good thing to do anyway, to be more robust in
> future. But it could confuse the reader later on why it's necessary
> when backward slashes can't exist in wt->path. I don't know. Maybe
> just have a comment that backward slashes can't never appear here?

As this path is read from a file git itself creates, and if we know
that it will always contain forward slashes, then I agree that it
could be potentially confusing to later readers to see
git_find_last_dir_sep(). So, keeping it as-is seems correct.

Not sure if it needs a comment. I reviewed this rather quickly since
(I think) you plan on re-rolling it and I'm far behind on my reviews.
Consequently, I didn't check the existing code, and reviewed only
within the context of the patch itself. If the end result is that it's
clear from reading the code that it will always contain forward
slashes, then a comment would be redundant. You could perhaps mention
in the commit message that the slash will always be forward, which
should satisfy future reviewers and readers of the code once its in
the tree.

> There is also a potential problem with find_worktree_by_path(). I was
> counting on real_path() to normalize paths and could simply do
> strcmp_icase (or its new name, fspathcmp). But real_path() does not
> seem to convert unify slashes. I will need to have a closer look at
> this. Hopefully prefix_filename() already makes sure everything uses
> forward slashes. Or maybe we could improve fspathcmp to see '/' and
> '\' the same thing on Windows.

If we look at fspathcmp() as a function which performs whatever magic
is needed to make comparisons work on a platform/filesystem, then it
might indeed be reasonable to enhance it to recognize '/' and '\' as
equivalent (with possible caveats for Windows corner cases).
--
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 24/25] worktree move: accept destination as directory

Johannes Sixt-3
Am 11.05.2016 um 19:32 schrieb Eric Sunshine:

> On Wed, May 11, 2016 at 9:34 AM, Duy Nguyen <[hidden email]> wrote:
>> On Wed, May 11, 2016 at 11:43 AM, Eric Sunshine <[hidden email]> wrote:
>>> On Wed, Apr 13, 2016 at 9:15 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>>>> +       if (is_directory(dst.buf)) {
>>>> +               const char *sep = strrchr(wt->path, '/');
>>>
>>> Does this need to take Windows into account?
>>
>> wt->path comes from $GIT_DIR/worktrees/xxx/gitdir, which normally uses
>> forward slashes, so we should be safe. We already rely on forward
>> slashes in get_linked_worktree()
>>
>>> Perhaps git_find_last_dir_sep()?
>>
>> But this is probably a good thing to do anyway, to be more robust in
>> future. But it could confuse the reader later on why it's necessary
>> when backward slashes can't exist in wt->path. I don't know. Maybe
>> just have a comment that backward slashes can't never appear here?
>
> As this path is read from a file git itself creates, and if we know
> that it will always contain forward slashes, then I agree that it
> could be potentially confusing to later readers to see
> git_find_last_dir_sep(). So, keeping it as-is seems correct.

Please allow me to disagree. There should not be any assumption that a
path uses forward slashes as directory separator, except when the path is

- a pathspec
- a ref
- a path found in the object database including the index

In particular, everything concerning paths in the file system (including
paths pointing to Git's own files) must not assume forward slashes.

We do convert backslashes to forward slashes in a number of places, but
this is only for cosmetic reasons, not to maintain an invariant.

> If we look at fspathcmp() as a function which performs whatever magic
> is needed to make comparisons work on a platform/filesystem, then it
> might indeed be reasonable to enhance it to recognize '/' and '\' as
> equivalent (with possible caveats for Windows corner cases).

That sounds reasonable.

-- Hannes

--
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 24/25] worktree move: accept destination as directory

Junio C Hamano
Johannes Sixt <[hidden email]> writes:

>> As this path is read from a file git itself creates, and if we know
>> that it will always contain forward slashes, then I agree that it
>> could be potentially confusing to later readers to see
>> git_find_last_dir_sep(). So, keeping it as-is seems correct.
>
> Please allow me to disagree. There should not be any assumption that a
> path uses forward slashes as directory separator, except when the path
> is
>
> - a pathspec
> - a ref
> - a path found in the object database including the index

I think standardising on one way for what we write out would give
less hassle to users.  The human end users should not be opening
these files in their editors and futzing with their contents, but
there are third-party tools and reimplementations of Git.  Forcing
them to be prepared for input with slashes and backslashes does not
make much sense to me.

Is there an upside for us to accept both slashes in this file?



--
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 24/25] worktree move: accept destination as directory

Johannes Sixt-3
Am 11.05.2016 um 23:34 schrieb Junio C Hamano:

> Johannes Sixt <[hidden email]> writes:
>
>>> As this path is read from a file git itself creates, and if we know
>>> that it will always contain forward slashes, then I agree that it
>>> could be potentially confusing to later readers to see
>>> git_find_last_dir_sep(). So, keeping it as-is seems correct.
>>
>> Please allow me to disagree. There should not be any assumption that a
>> path uses forward slashes as directory separator, except when the path
>> is
>>
>> - a pathspec
>> - a ref
>> - a path found in the object database including the index
>
> I think standardising on one way for what we write out would give
> less hassle to users.  The human end users should not be opening
> these files in their editors and futzing with their contents, but
> there are third-party tools and reimplementations of Git.  Forcing
> them to be prepared for input with slashes and backslashes does not
> make much sense to me.

It is the opposite: We would force other tools to write slashes even
though on Windows both types of slashes are allowed as directory separators.

> Is there an upside for us to accept both slashes in this file?

Obviously, yes: We can accept what third-party tools write.

BTW, we also have to accept absolute paths in the file, no? Then we
cannot assume that the path begins with a slash on Windows; absolute
paths would come in drive letter style on Windows.

-- Hannes

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