possible to checkout same branch in different worktree

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

[PATCH v2 06/12] wt-status.c: split rebase detection out of wt_status_get_state()

Duy Nguyen
worktree.c:find_shared_symref() later needs to know if a branch is being
rebased, and only rebased only. Split this code so it can be used
independently from other in-progress tests.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 wt-status.c | 23 +++++++++++++++++------
 wt-status.h |  1 +
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 1ea2ebe..35787ec 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1360,15 +1360,11 @@ static void wt_status_get_detached_from(struct wt_status_state *state)
  strbuf_release(&cb.buf);
 }
 
-void wt_status_get_state(struct wt_status_state *state,
- int get_detached_from)
+int wt_status_check_rebase(struct wt_status_state *state)
 {
  struct stat st;
- unsigned char sha1[20];
 
- if (!stat(git_path_merge_head(), &st)) {
- state->merge_in_progress = 1;
- } else if (!stat(git_path("rebase-apply"), &st)) {
+ if (!stat(git_path("rebase-apply"), &st)) {
  if (!stat(git_path("rebase-apply/applying"), &st)) {
  state->am_in_progress = 1;
  if (!stat(git_path("rebase-apply/patch"), &st) && !st.st_size)
@@ -1385,6 +1381,21 @@ void wt_status_get_state(struct wt_status_state *state,
  state->rebase_in_progress = 1;
  state->branch = read_and_strip_branch("rebase-merge/head-name");
  state->onto = read_and_strip_branch("rebase-merge/onto");
+ } else
+ return 0;
+ return 1;
+}
+
+void wt_status_get_state(struct wt_status_state *state,
+ int get_detached_from)
+{
+ struct stat st;
+ unsigned char sha1[20];
+
+ if (!stat(git_path_merge_head(), &st)) {
+ state->merge_in_progress = 1;
+ } else if (wt_status_check_rebase(state)) {
+ /* all set */
  } else if (!stat(git_path_cherry_pick_head(), &st) &&
  !get_sha1("CHERRY_PICK_HEAD", sha1)) {
  state->cherry_pick_in_progress = 1;
diff --git a/wt-status.h b/wt-status.h
index c9b3b74..b398353 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -100,6 +100,7 @@ void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
 void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
+int wt_status_check_rebase(struct wt_status_state *state);
 
 void wt_shortstatus_print(struct wt_status *s);
 void wt_porcelain_print(struct wt_status *s);
--
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 v2 07/12] wt-status.c: make wt_status_check_rebase() work on any worktree

Duy Nguyen
In reply to this post by Duy Nguyen
This is a preparation step for find_shared_symref() to detect if any
worktree is being rebased.

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

diff --git a/wt-status.c b/wt-status.c
index 35787ec..2295682 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -15,6 +15,7 @@
 #include "column.h"
 #include "strbuf.h"
 #include "utf8.h"
+#include "worktree.h"
 
 static const char cut_line[] =
 "------------------------ >8 ------------------------\n";
@@ -1262,13 +1263,13 @@ static void show_bisect_in_progress(struct wt_status *s,
 /*
  * Extract branch information from rebase/bisect
  */
-static char *read_and_strip_branch(const char *path)
+static char *get_branch(const struct worktree *wt, const char *path)
 {
  struct strbuf sb = STRBUF_INIT;
  unsigned char sha1[20];
  const char *branch_name;
 
- if (strbuf_read_file(&sb, git_path("%s", path), 0) <= 0)
+ if (strbuf_read_file(&sb, worktree_git_path(wt, "%s", path), 0) <= 0)
  goto got_nothing;
 
  while (sb.len && sb.buf[sb.len - 1] == '\n')
@@ -1295,6 +1296,11 @@ got_nothing:
  return NULL;
 }
 
+static char *read_and_strip_branch(const char *path)
+{
+ return get_branch(NULL, path);
+}
+
 struct grab_1st_switch_cbdata {
  struct strbuf buf;
  unsigned char nsha1[20];
@@ -1360,27 +1366,28 @@ static void wt_status_get_detached_from(struct wt_status_state *state)
  strbuf_release(&cb.buf);
 }
 
-int wt_status_check_rebase(struct wt_status_state *state)
+int wt_status_check_rebase(const struct worktree *wt,
+   struct wt_status_state *state)
 {
  struct stat st;
 
- if (!stat(git_path("rebase-apply"), &st)) {
- if (!stat(git_path("rebase-apply/applying"), &st)) {
+ if (!stat(worktree_git_path(wt, "rebase-apply"), &st)) {
+ if (!stat(worktree_git_path(wt, "rebase-apply/applying"), &st)) {
  state->am_in_progress = 1;
- if (!stat(git_path("rebase-apply/patch"), &st) && !st.st_size)
+ if (!stat(worktree_git_path(wt, "rebase-apply/patch"), &st) && !st.st_size)
  state->am_empty_patch = 1;
  } else {
  state->rebase_in_progress = 1;
- state->branch = read_and_strip_branch("rebase-apply/head-name");
- state->onto = read_and_strip_branch("rebase-apply/onto");
+ state->branch = get_branch(wt, "rebase-apply/head-name");
+ state->onto = get_branch(wt, "rebase-apply/onto");
  }
- } else if (!stat(git_path("rebase-merge"), &st)) {
- if (!stat(git_path("rebase-merge/interactive"), &st))
+ } else if (!stat(worktree_git_path(wt, "rebase-merge"), &st)) {
+ if (!stat(worktree_git_path(wt, "rebase-merge/interactive"), &st))
  state->rebase_interactive_in_progress = 1;
  else
  state->rebase_in_progress = 1;
- state->branch = read_and_strip_branch("rebase-merge/head-name");
- state->onto = read_and_strip_branch("rebase-merge/onto");
+ state->branch = get_branch(wt, "rebase-merge/head-name");
+ state->onto = get_branch(wt, "rebase-merge/onto");
  } else
  return 0;
  return 1;
@@ -1394,7 +1401,7 @@ void wt_status_get_state(struct wt_status_state *state,
 
  if (!stat(git_path_merge_head(), &st)) {
  state->merge_in_progress = 1;
- } else if (wt_status_check_rebase(state)) {
+ } else if (wt_status_check_rebase(NULL, state)) {
  /* all set */
  } else if (!stat(git_path_cherry_pick_head(), &st) &&
  !get_sha1("CHERRY_PICK_HEAD", sha1)) {
diff --git a/wt-status.h b/wt-status.h
index b398353..c4ddcad 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "pathspec.h"
 
+struct worktree;
+
 enum color_wt_status {
  WT_STATUS_HEADER = 0,
  WT_STATUS_UPDATED,
@@ -100,7 +102,8 @@ void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
 void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
-int wt_status_check_rebase(struct wt_status_state *state);
+int wt_status_check_rebase(const struct worktree *wt,
+   struct wt_status_state *state);
 
 void wt_shortstatus_print(struct wt_status *s);
 void wt_porcelain_print(struct wt_status *s);
--
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 v2 08/12] worktree.c: avoid referencing to worktrees[i] multiple times

Duy Nguyen
In reply to this post by Duy Nguyen
Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 worktree.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/worktree.c b/worktree.c
index 452f64a..b5ca78f 100644
--- a/worktree.c
+++ b/worktree.c
@@ -221,10 +221,12 @@ const struct worktree *find_shared_symref(const char *symref,
  worktrees = get_worktrees();
 
  for (i = 0; worktrees[i]; i++) {
+ struct worktree *wt = worktrees[i];
+
  strbuf_reset(&path);
  strbuf_reset(&sb);
  strbuf_addf(&path, "%s/%s",
-    get_worktree_git_dir(worktrees[i]),
+    get_worktree_git_dir(wt),
     symref);
 
  if (parse_ref(path.buf, &sb, NULL)) {
@@ -232,7 +234,7 @@ const struct worktree *find_shared_symref(const char *symref,
  }
 
  if (!strcmp(sb.buf, target)) {
- existing = worktrees[i];
+ existing = wt;
  break;
  }
  }
--
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 v2 09/12] worktree.c: test if branch being rebased in another worktree

Duy Nguyen
In reply to this post by Duy Nguyen
This function find_shared_symref() is used in a couple places:

1) in builtin/branch.c: it's used to detect if a branch is checked out
   elsewhere and refuse to delete the branch.

2) in builtin/notes.c: it's used to detect if a note is being merged in
   another worktree

3) in branch.c, the function die_if_checked_out() is actually used by
   "git checkout" and "git worktree add" to see if a branch is already
   checked out elsewhere and refuse the operation.

In cases 1 and 3, if a rebase is ongoing, "HEAD" will be in detached
mode, find_shared_symref() fails to detect it and declares "no branch is
checked out here", which is incorrect.

This patch tightens the test. If the given symref is "HEAD", we try to
detect if rebase is ongoing. If so return the branch being rebased. This
makes checkout and branch delete operations safer because you can't
checkout a branch being rebased in another place, or delete it.

Special case for checkout. If the current branch is being rebased,
git-rebase.sh may use "git checkout" to abort and return back to the
original branch. The updated test in find_shared_symref() will prevent
that and "git rebase --abort" will fail as a result.
find_shared_symref() and die_if_checked_out() have to learn a new
option ignore_current_worktree to loose the test a bit.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 branch.c                |  4 ++--
 branch.h                |  2 +-
 builtin/branch.c        |  2 +-
 builtin/checkout.c      |  2 +-
 builtin/notes.c         |  2 +-
 builtin/worktree.c      |  4 ++--
 t/t2025-worktree-add.sh | 38 ++++++++++++++++++++++++++++++++++++++
 worktree.c              | 32 +++++++++++++++++++++++++++++++-
 worktree.h              |  3 ++-
 9 files changed, 79 insertions(+), 10 deletions(-)

diff --git a/branch.c b/branch.c
index a84fb2c..8e323d3 100644
--- a/branch.c
+++ b/branch.c
@@ -334,11 +334,11 @@ void remove_branch_state(void)
  unlink(git_path_squash_msg());
 }
 
-void die_if_checked_out(const char *branch)
+void die_if_checked_out(const char *branch, int ignore_current_worktree)
 {
  const struct worktree *wt;
 
- wt = find_shared_symref("HEAD", branch);
+ wt = find_shared_symref("HEAD", branch, ignore_current_worktree);
  if (wt) {
  skip_prefix(branch, "refs/heads/", &branch);
  die(_("'%s' is already checked out at '%s'"),
diff --git a/branch.h b/branch.h
index d69163d..b2f9649 100644
--- a/branch.h
+++ b/branch.h
@@ -58,7 +58,7 @@ extern int read_branch_desc(struct strbuf *, const char *branch_name);
  * worktree and die (with a message describing its checkout location) if
  * it is.
  */
-extern void die_if_checked_out(const char *branch);
+extern void die_if_checked_out(const char *branch, int ignore_current_worktree);
 
 /*
  * Update all per-worktree HEADs pointing at the old ref to point the new ref.
diff --git a/builtin/branch.c b/builtin/branch.c
index bcde87d..bf91bbd 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -221,7 +221,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 
  if (kinds == FILTER_REFS_BRANCHES) {
  const struct worktree *wt =
- find_shared_symref("HEAD", name);
+ find_shared_symref("HEAD", name, 0);
  if (wt) {
  error(_("Cannot delete branch '%s' "
  "checked out at '%s'"),
diff --git a/builtin/checkout.c b/builtin/checkout.c
index efcbd8f..6041718 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1111,7 +1111,7 @@ static int checkout_branch(struct checkout_opts *opts,
  char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
  if (head_ref &&
     (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
- die_if_checked_out(new->path);
+ die_if_checked_out(new->path, 1);
  free(head_ref);
  }
 
diff --git a/builtin/notes.c b/builtin/notes.c
index c65b59a..f154a69 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -852,7 +852,7 @@ static int merge(int argc, const char **argv, const char *prefix)
  update_ref(msg.buf, "NOTES_MERGE_PARTIAL", result_sha1, NULL,
    0, UPDATE_REFS_DIE_ON_ERR);
  /* Store ref-to-be-updated into .git/NOTES_MERGE_REF */
- wt = find_shared_symref("NOTES_MERGE_REF", default_notes_ref());
+ wt = find_shared_symref("NOTES_MERGE_REF", default_notes_ref(), 0);
  if (wt)
  die(_("A notes merge into %s is already in-progress at %s"),
     default_notes_ref(), wt->path);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d8e3795..12c0af7 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -205,7 +205,7 @@ static int add_worktree(const char *path, const char *refname,
  if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&
  ref_exists(symref.buf)) { /* it's a branch */
  if (!opts->force)
- die_if_checked_out(symref.buf);
+ die_if_checked_out(symref.buf, 0);
  } else { /* must be a commit */
  commit = lookup_commit_reference_by_name(refname);
  if (!commit)
@@ -349,7 +349,7 @@ static int add(int ac, const char **av, const char *prefix)
  if (!opts.force &&
     !strbuf_check_branch_ref(&symref, opts.new_branch) &&
     ref_exists(symref.buf))
- die_if_checked_out(symref.buf);
+ die_if_checked_out(symref.buf, 0);
  strbuf_release(&symref);
  }
 
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 3acb992..da54327 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -4,6 +4,8 @@ test_description='test git worktree add'
 
 . ./test-lib.sh
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
 test_expect_success 'setup' '
  test_commit init
 '
@@ -225,4 +227,40 @@ test_expect_success '"add" worktree with --checkout' '
  test_cmp init.t swamp2/init.t
 '
 
+test_expect_success 'put a worktree under rebase' '
+ git worktree add under-rebase &&
+ (
+ cd under-rebase &&
+ set_fake_editor &&
+ FAKE_LINES="edit 1" git rebase -i HEAD^ &&
+ git worktree list | grep "under-rebase.*detached HEAD"
+ )
+'
+
+test_expect_success 'add a worktree, checking out a rebased branch' '
+ test_must_fail git worktree add new-rebase under-rebase &&
+ ! test -d new-rebase
+'
+
+test_expect_success 'checking out a rebased branch from another worktree' '
+ git worktree add new-place &&
+ test_must_fail git -C new-place checkout under-rebase
+'
+
+test_expect_success 'not allow to delete a branch under rebase' '
+ (
+ cd under-rebase &&
+ test_must_fail git branch -D under-rebase
+ )
+'
+
+test_expect_success 'check out from current worktree branch ok' '
+ (
+ cd under-rebase &&
+ git checkout under-rebase &&
+ git checkout - &&
+ git rebase --abort
+ )
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index b5ca78f..dc380a2 100644
--- a/worktree.c
+++ b/worktree.c
@@ -3,6 +3,7 @@
 #include "strbuf.h"
 #include "worktree.h"
 #include "dir.h"
+#include "wt-status.h"
 
 void free_worktrees(struct worktree **worktrees)
 {
@@ -207,8 +208,27 @@ const char *get_worktree_git_dir(const struct worktree *wt)
  return git_common_path("worktrees/%s", wt->id);
 }
 
+static int is_worktree_being_rebased(const struct worktree *wt,
+     const char *target)
+{
+ struct wt_status_state state;
+ int found_rebase;
+
+ memset(&state, 0, sizeof(state));
+ found_rebase = wt_status_check_rebase(wt, &state) &&
+ ((state.rebase_in_progress ||
+  state.rebase_interactive_in_progress) &&
+ state.branch &&
+ starts_with(target, "refs/heads/") &&
+ !strcmp(state.branch, target + strlen("refs/heads/")));
+ free(state.branch);
+ free(state.onto);
+ return found_rebase;
+}
+
 const struct worktree *find_shared_symref(const char *symref,
-  const char *target)
+  const char *target,
+  int ignore_current_worktree)
 {
  const struct worktree *existing = NULL;
  struct strbuf path = STRBUF_INIT;
@@ -223,6 +243,16 @@ const struct worktree *find_shared_symref(const char *symref,
  for (i = 0; worktrees[i]; i++) {
  struct worktree *wt = worktrees[i];
 
+ if (ignore_current_worktree && wt->is_current)
+ continue;
+
+ if (wt->is_detached && !strcmp(symref, "HEAD")) {
+ if (is_worktree_being_rebased(wt, target)) {
+ existing = wt;
+ break;
+ }
+ }
+
  strbuf_reset(&path);
  strbuf_reset(&sb);
  strbuf_addf(&path, "%s/%s",
diff --git a/worktree.h b/worktree.h
index 9d2463e..fb9f5cc 100644
--- a/worktree.h
+++ b/worktree.h
@@ -41,7 +41,8 @@ extern void free_worktrees(struct worktree **);
  * may be destroyed by the next call.
  */
 extern const struct worktree *find_shared_symref(const char *symref,
- const char *target);
+ const char *target,
+ int ignore_current_worktree);
 
 /*
  * Similar to git_path() and git_pathdup() but can produce paths for a
--
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 v2 10/12] wt-status.c: split bisect detection out of wt_status_get_state()

Duy Nguyen
In reply to this post by Duy Nguyen
And make it work with any given worktree, in preparation for (again)
find_shared_symref(). read_and_strip_branch() is deleted because it's
no longer used.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 wt-status.c | 23 ++++++++++++++---------
 wt-status.h |  2 ++
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 2295682..36c85f8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1296,11 +1296,6 @@ got_nothing:
  return NULL;
 }
 
-static char *read_and_strip_branch(const char *path)
-{
- return get_branch(NULL, path);
-}
-
 struct grab_1st_switch_cbdata {
  struct strbuf buf;
  unsigned char nsha1[20];
@@ -1393,6 +1388,19 @@ int wt_status_check_rebase(const struct worktree *wt,
  return 1;
 }
 
+int wt_status_check_bisect(const struct worktree *wt,
+   struct wt_status_state *state)
+{
+ struct stat st;
+
+ if (!stat(worktree_git_path(wt, "BISECT_LOG"), &st)) {
+ state->bisect_in_progress = 1;
+ state->branch = get_branch(wt, "BISECT_START");
+ return 1;
+ }
+ return 0;
+}
+
 void wt_status_get_state(struct wt_status_state *state,
  int get_detached_from)
 {
@@ -1408,10 +1416,7 @@ void wt_status_get_state(struct wt_status_state *state,
  state->cherry_pick_in_progress = 1;
  hashcpy(state->cherry_pick_head_sha1, sha1);
  }
- if (!stat(git_path("BISECT_LOG"), &st)) {
- state->bisect_in_progress = 1;
- state->branch = read_and_strip_branch("BISECT_START");
- }
+ wt_status_check_bisect(NULL, state);
  if (!stat(git_path_revert_head(), &st) &&
     !get_sha1("REVERT_HEAD", sha1)) {
  state->revert_in_progress = 1;
diff --git a/wt-status.h b/wt-status.h
index c4ddcad..2ca93f6 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -104,6 +104,8 @@ void wt_status_collect(struct wt_status *s);
 void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
 int wt_status_check_rebase(const struct worktree *wt,
    struct wt_status_state *state);
+int wt_status_check_bisect(const struct worktree *wt,
+   struct wt_status_state *state);
 
 void wt_shortstatus_print(struct wt_status *s);
 void wt_porcelain_print(struct wt_status *s);
--
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 v2 11/12] worktree.c: test if branch being bisected in another worktree

Duy Nguyen
In reply to this post by Duy Nguyen
Similar to the rebase case, we want to detect if "HEAD" in some worktree
is being bisected because

1) we do not want to checkout this branch in another worktree, after
   bisect is done it will want to go back to this branch

2) we do not want to delete the branch is either or git bisect will
   fail to return to the (long gone) branch

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 t/t2025-worktree-add.sh | 13 +++++++++++++
 worktree.c              | 19 +++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index da54327..8f53944 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -263,4 +263,17 @@ test_expect_success 'check out from current worktree branch ok' '
  )
 '
 
+test_expect_success 'checkout a branch under bisect' '
+ git worktree add under-bisect &&
+ (
+ cd under-bisect &&
+ git bisect start &&
+ git bisect bad &&
+ git bisect good HEAD~2 &&
+ git worktree list | grep "under-bisect.*detached HEAD" &&
+ test_must_fail git worktree add new-bisect under-bisect &&
+ ! test -d new-bisect
+ )
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index dc380a2..7b66071 100644
--- a/worktree.c
+++ b/worktree.c
@@ -226,6 +226,21 @@ static int is_worktree_being_rebased(const struct worktree *wt,
  return found_rebase;
 }
 
+static int is_worktree_being_bisected(const struct worktree *wt,
+      const char *target)
+{
+ struct wt_status_state state;
+ int found_rebase;
+
+ memset(&state, 0, sizeof(state));
+ found_rebase = wt_status_check_bisect(wt, &state) &&
+ state.branch &&
+ starts_with(target, "refs/heads/") &&
+ !strcmp(state.branch, target + strlen("refs/heads/"));
+ free(state.branch);
+ return found_rebase;
+}
+
 const struct worktree *find_shared_symref(const char *symref,
   const char *target,
   int ignore_current_worktree)
@@ -251,6 +266,10 @@ const struct worktree *find_shared_symref(const char *symref,
  existing = wt;
  break;
  }
+ if (is_worktree_being_bisected(wt, target)) {
+ existing = wt;
+ break;
+ }
  }
 
  strbuf_reset(&path);
--
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 v2 12/12] branch: do not rename a branch under bisect or rebase

Duy Nguyen
In reply to this post by Duy Nguyen
The branch name in that case could be saved in rebase's head_name or
bisect's BISECT_START files. Ideally we should try to update them as
well. But it's trickier (*). Let's play safe and see if the user
complains about inconveniences before doing that.

(*) If we do it, bisect and rebase need to provide an API to rename
branches. We can't do it in worktree.c or builtin/branch.c because
when other people change rebase/bisect code, they may not be aware of
this code and accidentally break it (e.g. rename the branch file, or
refer to the branch in new files). It's a lot more work.

Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
---
 builtin/branch.c        | 25 +++++++++++++++++++++++++
 t/t2025-worktree-add.sh |  8 ++++++++
 worktree.c              |  8 ++++----
 worktree.h              |  3 +++
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index bf91bbd..3a2eceb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -524,6 +524,29 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
  ref_array_clear(&array);
 }
 
+static void reject_rebase_or_bisect_branch(const char *target)
+{
+ struct worktree **worktrees = get_worktrees();
+ int i;
+
+ for (i = 0; worktrees[i]; i++) {
+ struct worktree *wt = worktrees[i];
+
+ if (!wt->is_detached)
+ continue;
+
+ if (is_worktree_being_rebased(wt, target))
+ die(_("Branch %s is being rebased at %s"),
+    target, wt->path);
+
+ if (is_worktree_being_bisected(wt, target))
+ die(_("Branch %s is being bisected at %s"),
+    target, wt->path);
+ }
+
+ free_worktrees(worktrees);
+}
+
 static void rename_branch(const char *oldname, const char *newname, int force)
 {
  struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
@@ -553,6 +576,8 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 
  validate_new_branchname(newname, &newref, force, clobber_head_ok);
 
+ reject_rebase_or_bisect_branch(oldref.buf);
+
  strbuf_addf(&logmsg, "Branch: renamed %s to %s",
  oldref.buf, newref.buf);
 
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 8f53944..3a22fc5 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -254,6 +254,10 @@ test_expect_success 'not allow to delete a branch under rebase' '
  )
 '
 
+test_expect_success 'rename a branch under rebase not allowed' '
+ test_must_fail git branch -M under-rebase rebase-with-new-name
+'
+
 test_expect_success 'check out from current worktree branch ok' '
  (
  cd under-rebase &&
@@ -276,4 +280,8 @@ test_expect_success 'checkout a branch under bisect' '
  )
 '
 
+test_expect_success 'rename a branch under bisect not allowed' '
+ test_must_fail git branch -M under-bisect bisect-with-new-name
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index 7b66071..8a3d394 100644
--- a/worktree.c
+++ b/worktree.c
@@ -208,8 +208,8 @@ const char *get_worktree_git_dir(const struct worktree *wt)
  return git_common_path("worktrees/%s", wt->id);
 }
 
-static int is_worktree_being_rebased(const struct worktree *wt,
-     const char *target)
+int is_worktree_being_rebased(const struct worktree *wt,
+      const char *target)
 {
  struct wt_status_state state;
  int found_rebase;
@@ -226,8 +226,8 @@ static int is_worktree_being_rebased(const struct worktree *wt,
  return found_rebase;
 }
 
-static int is_worktree_being_bisected(const struct worktree *wt,
-      const char *target)
+int is_worktree_being_bisected(const struct worktree *wt,
+       const char *target)
 {
  struct wt_status_state state;
  int found_rebase;
diff --git a/worktree.h b/worktree.h
index fb9f5cc..d4a3534 100644
--- a/worktree.h
+++ b/worktree.h
@@ -44,6 +44,9 @@ extern const struct worktree *find_shared_symref(const char *symref,
  const char *target,
  int ignore_current_worktree);
 
+int is_worktree_being_rebased(const struct worktree *wt, const char *target);
+int is_worktree_being_bisected(const struct worktree *wt, const char *target);
+
 /*
  * Similar to git_path() and git_pathdup() but can produce paths for a
  * specified worktree instead of current one
--
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 v2 06/12] wt-status.c: split rebase detection out of wt_status_get_state()

Ramsay Jones-2
In reply to this post by Duy Nguyen


On 20/04/16 14:24, Nguyễn Thái Ngọc Duy wrote:
> worktree.c:find_shared_symref() later needs to know if a branch is being
> rebased, and only rebased only. Split this code so it can be used
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Err ... what?

ATB,
Ramsay Jones
--
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 06/12] wt-status.c: split rebase detection out of wt_status_get_state()

Duy Nguyen
On Wed, Apr 20, 2016 at 8:48 PM, Ramsay Jones
<[hidden email]> wrote:
>
>
> On 20/04/16 14:24, Nguyễn Thái Ngọc Duy wrote:
>> worktree.c:find_shared_symref() later needs to know if a branch is being
>> rebased, and only rebased only. Split this code so it can be used
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Err ... what?

wt_status_get_state() detects more than rebase, it does bisect,
cherry-pick, detached head as well. I "only" too much there though.
--
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 03/12] worktree.c: make find_shared_symref() return struct worktree *

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

> This gives the caller more information and they can answer things like,
> "is it the main worktree" or "is it the current worktree". The latter
> question is needed for the "checkout a rebase branch" case later.

That makes good sense.

> diff --git a/worktree.h b/worktree.h
> index 3198c8d..d71d7ec 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -36,9 +36,10 @@ 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
> - * if it is.  Returns NULL if there is no existing ref.  The caller is
> - * responsible for freeing the returned path.
> + * if it is. Returns NULL if there is no existing ref. The result
> + * may be destroyed by the next call.
>   */

To return and keep alive one worktree[] instance (i.e. "existing"),
the code holds onto the entire return value from get_worktrees(), if
I am not misreading it.  Typically you would have only a handful of
worktrees so this may not be an issue, though.

--
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 09/12] worktree.c: test if branch being rebased in another worktree

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

> Subject: Re: [PATCH v2 09/12] worktree.c: test if branch being rebased in another worktree

Lacks the verb?  Perhaps s/being/is/ is sufficient.

> This function find_shared_symref() is used in a couple places:
>
> 1) in builtin/branch.c: it's used to detect if a branch is checked out
>    elsewhere and refuse to delete the branch.
>
> 2) in builtin/notes.c: it's used to detect if a note is being merged in
>    another worktree
>
> 3) in branch.c, the function die_if_checked_out() is actually used by
>    "git checkout" and "git worktree add" to see if a branch is already
>    checked out elsewhere and refuse the operation.
>
> In cases 1 and 3, if a rebase is ongoing, "HEAD" will be in detached
> mode, find_shared_symref() fails to detect it and declares "no branch is
> checked out here", which is incorrect.

which is technically correct but is not what we want to check.

> This patch tightens the test. If the given symref is "HEAD", we try to
> detect if rebase is ongoing. If so return the branch being rebased. This
> makes checkout and branch delete operations safer because you can't
> checkout a branch being rebased in another place, or delete it.

Is rebase the only thing that tentatively detach before working on
the real branch?  It may be currently the case, but I would imagine
that we want to makefind-shared-symref to be responsible for
detecting new cases other than rebase that we may introduce in the
future, in which case we may want to leave come comment near the
function to describe that expectation.

> Special case for checkout. If the current branch is being rebased,
> git-rebase.sh may use "git checkout" to abort and return back to the
> original branch. The updated test in find_shared_symref() will prevent
> that and "git rebase --abort" will fail as a result.
> find_shared_symref() and die_if_checked_out() have to learn a new
> option ignore_current_worktree to loose the test a bit.

s/loose/&n/

> +void die_if_checked_out(const char *branch, int ignore_current_worktree)
>  {
>   const struct worktree *wt;
>  
> - wt = find_shared_symref("HEAD", branch);
> + wt = find_shared_symref("HEAD", branch, ignore_current_worktree);
>   if (wt) {
>   skip_prefix(branch, "refs/heads/", &branch);
>   die(_("'%s' is already checked out at '%s'"),
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index efcbd8f..6041718 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1111,7 +1111,7 @@ static int checkout_branch(struct checkout_opts *opts,
>   char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
>   if (head_ref &&
>      (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
> - die_if_checked_out(new->path);
> + die_if_checked_out(new->path, 1);
>   free(head_ref);
>   }

So the idea is "if the branch is checked out (or "being worked on"
even if technically the HEAD is detached, like with 'rebase')
anywhere, callers of die-if-checked-out in general want to die; but
for this caller, it is OK if the place the branch is checked out or
being worked on is in this repository"?

I understand die_if_checked_out() taking that "ignore this one" bit
may be sensible, but I do not understand why find_shared_symref()
needs to be told about it.  The change makes the meaning of the
find_shared_symref() function unclear.  It used to mean "This
symbolic ref cannot point at the same ref in different worktrees, so
for a given pair of a symbolic ref and a concrete ref, there can be
at most one worktree in which the symbolic ref points at that ref".
That is already a mouthful.  As the worktree structure already have
"Am I the current worktree?" bit, "ignore" logic can easily be done
inside die_if_checked_out() and that would help find_shared_symref()
stay simpler and more focused function, no?
--
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 00/12] fix checking out a being-rebased branch

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

> Much happier with this version. This makes
>
>  - git checkout refuse if a branch is under rebase or bisect
>    elsewhere
>
>  - git worktree add refuse if a branch is under rebase or bisect
>
>  - git branch -D refuse if a branch is under rebase or bisect.
>    This applies for single worktree case as well.
>
>  - git branch -M refuse if a branch is under rebase or bisect
>    (single worktree case as well)

I agree that this reads much nicer than v1, especially with the
abstraction around find_shared_symref() that returns the actual
worktree.

>   [01/12] path.c: add git_common_path() and strbuf_git_common_path()
>   [02/12] worktree.c: store "id" instead of "git_dir"
>   [03/12] worktree.c: make find_shared_symref() return struct worktree *
>   [04/12] worktree.c: mark current worktree
>   [05/12] path.c: refactor and add worktree_git_path()
>   [06/12] wt-status.c: split rebase detection out of wt_status_get_state()
>   [07/12] wt-status.c: make wt_status_check_rebase() work on any worktree
>   [08/12] worktree.c: avoid referencing to worktrees[i] multiple times
>   [09/12] worktree.c: test if branch being rebased in another worktree
>   [10/12] wt-status.c: split bisect detection out of wt_status_get_state()
>   [11/12] worktree.c: test if branch being bisected in another worktree
>   [12/12] branch: do not rename a branch under bisect or rebase
>
> Total 13 files changed, 335 insertions(+), 67 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 v2 01/12] path.c: add git_common_path() and strbuf_git_common_path()

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

> diff --git a/path.c b/path.c
> @@ -503,6 +503,35 @@ void strbuf_git_path_submodule(struct strbuf *buf, const char *path,
> +const char *git_common_path(const char *fmt, ...)
> +{
> +       struct strbuf *pathname = get_pathname();
> +       va_list args;
> +       va_start(args, fmt);
> +       do_git_common_path(pathname, fmt, args);
> +       va_end(args);
> +       return pathname->buf;

Is the caller expected to free this value? If not, then shouldn't
'pathname' be static? If so, then perhaps strbuf_detach() would be
clearer (and return 'char *' rather than 'const char *').

> +}
--
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 01/12] path.c: add git_common_path() and strbuf_git_common_path()

Duy Nguyen
On Thu, Apr 21, 2016 at 1:11 AM, Eric Sunshine <[hidden email]> wrote:

> On Wed, Apr 20, 2016 at 9:24 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>> diff --git a/path.c b/path.c
>> @@ -503,6 +503,35 @@ void strbuf_git_path_submodule(struct strbuf *buf, const char *path,
>> +const char *git_common_path(const char *fmt, ...)
>> +{
>> +       struct strbuf *pathname = get_pathname();
>> +       va_list args;
>> +       va_start(args, fmt);
>> +       do_git_common_path(pathname, fmt, args);
>> +       va_end(args);
>> +       return pathname->buf;
>
> Is the caller expected to free this value? If not, then shouldn't
> 'pathname' be static? If so, then perhaps strbuf_detach() would be
> clearer (and return 'char *' rather than 'const char *').

get_pathname() actually holds a ring of static buffer. So no, we don't
need static pathname, it can be a new buffer next time, mostly to to
make printf("%s %s", git_common_path(..), git_common_path(..)) work.
And no the caller is not supposed to free it, a little bit more
convenient.
--
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 09/12] worktree.c: test if branch being rebased in another worktree

Duy Nguyen
In reply to this post by Junio C Hamano
On Thu, Apr 21, 2016 at 1:04 AM, Junio C Hamano <[hidden email]> wrote:

>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index efcbd8f..6041718 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -1111,7 +1111,7 @@ static int checkout_branch(struct checkout_opts *opts,
>>               char *head_ref = resolve_refdup("HEAD", 0, sha1, &flag);
>>               if (head_ref &&
>>                   (!(flag & REF_ISSYMREF) || strcmp(head_ref, new->path)))
>> -                     die_if_checked_out(new->path);
>> +                     die_if_checked_out(new->path, 1);
>>               free(head_ref);
>>       }
>
> So the idea is "if the branch is checked out (or "being worked on"
> even if technically the HEAD is detached, like with 'rebase')
> anywhere, callers of die-if-checked-out in general want to die; but
> for this caller, it is OK if the place the branch is checked out or
> being worked on is in this repository"?
>
> I understand die_if_checked_out() taking that "ignore this one" bit
> may be sensible, but I do not understand why find_shared_symref()
> needs to be told about it.  The change makes the meaning of the
> find_shared_symref() function unclear.  It used to mean "This
> symbolic ref cannot point at the same ref in different worktrees, so
> for a given pair of a symbolic ref and a concrete ref, there can be
> at most one worktree in which the symbolic ref points at that ref".
> That is already a mouthful.  As the worktree structure already have
> "Am I the current worktree?" bit, "ignore" logic can easily be done
> inside die_if_checked_out() and that would help find_shared_symref()
> stay simpler and more focused function, no?

That was the intention when I made find_shared_symref() return struct
worktree * instead of char *. I forget why I changed my mind and not
do so. The only case when find_shared_symref should do this is when a
ref is shared twice, then we need to ignore current worktree from
inside the loop. But that can't happen. Will move this
ignore-current-worktree test to die_if_checked_out().
--
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 03/12] worktree.c: make find_shared_symref() return struct worktree *

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

> This gives the caller more information and they can answer things like,
> "is it the main worktree" or "is it the current worktree". The latter
> question is needed for the "checkout a rebase branch" case later.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
> ---
> diff --git a/worktree.h b/worktree.h
> @@ -36,9 +36,10 @@ 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

Doesn't "return the path" become outdated with this patch?

Also (not a new problem): s/exising/existing/

> - * if it is.  Returns NULL if there is no existing ref.  The caller is
> - * responsible for freeing the returned path.
> + * if it is. Returns NULL if there is no existing ref. The result
> + * may be destroyed by the next call.
>   */
> -extern char *find_shared_symref(const char *symref, const char *target);
> +extern const struct worktree *find_shared_symref(const char *symref,
> +                                                const char *target);
--
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 04/12] worktree.c: mark current worktree

Eric Sunshine
In reply to this post by Duy Nguyen
On Wed, Apr 20, 2016 at 9:24 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/worktree.c b/worktree.c
> @@ -178,6 +182,18 @@ struct worktree **get_worktrees(void)
>         }
>         ALLOC_GROW(list, counter + 1, alloc);
>         list[counter] = NULL;
> +
> +       strbuf_addstr(&git_dir, absolute_path(get_git_dir()));
> +       for (i = 0; i < counter; i++) {
> +               struct worktree *wt = list[i];
> +               strbuf_addstr(&path, absolute_path(get_worktree_git_dir(wt)));
> +               wt->is_current = !strcmp_icase(git_dir.buf, path.buf);

Can you talk a bit about why this uses 'icase'? Should it be
respecting cache.h:ignore_case?

> +               strbuf_reset(&path);
> +               if (wt->is_current)
> +                       break;
> +       }
> +       strbuf_release(&git_dir);
> +       strbuf_release(&path);

Minor: Would it make sense to place this new code in its own function
-- say, mark_current_worktree() -- to keep get_worktrees() from
becoming overlong?

>         return list;
>  }
--
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 04/12] worktree.c: mark current worktree

Duy Nguyen
On Thu, Apr 21, 2016 at 2:20 PM, Eric Sunshine <[hidden email]> wrote:

> On Wed, Apr 20, 2016 at 9:24 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/worktree.c b/worktree.c
>> @@ -178,6 +182,18 @@ struct worktree **get_worktrees(void)
>>         }
>>         ALLOC_GROW(list, counter + 1, alloc);
>>         list[counter] = NULL;
>> +
>> +       strbuf_addstr(&git_dir, absolute_path(get_git_dir()));
>> +       for (i = 0; i < counter; i++) {
>> +               struct worktree *wt = list[i];
>> +               strbuf_addstr(&path, absolute_path(get_worktree_git_dir(wt)));
>> +               wt->is_current = !strcmp_icase(git_dir.buf, path.buf);
>
> Can you talk a bit about why this uses 'icase'? Should it be
> respecting cache.h:ignore_case?

It does.That function (in dir.c) is just one-liner

    return ignore_case ? strcasecmp(a, b) : strcmp(a, b);

I admit though, the naming does not make that clear.

>> +               strbuf_reset(&path);
>> +               if (wt->is_current)
>> +                       break;
>> +       }
>> +       strbuf_release(&git_dir);
>> +       strbuf_release(&path);
>
> Minor: Would it make sense to place this new code in its own function
> -- say, mark_current_worktree() -- to keep get_worktrees() from
> becoming overlong?

Good idea. Will do.
--
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 04/12] worktree.c: mark current worktree

Duy Nguyen
On Thu, Apr 21, 2016 at 3:19 PM, Duy Nguyen <[hidden email]> wrote:

> On Thu, Apr 21, 2016 at 2:20 PM, Eric Sunshine <[hidden email]> wrote:
>> On Wed, Apr 20, 2016 at 9:24 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/worktree.c b/worktree.c
>>> @@ -178,6 +182,18 @@ struct worktree **get_worktrees(void)
>>>         }
>>>         ALLOC_GROW(list, counter + 1, alloc);
>>>         list[counter] = NULL;
>>> +
>>> +       strbuf_addstr(&git_dir, absolute_path(get_git_dir()));
>>> +       for (i = 0; i < counter; i++) {
>>> +               struct worktree *wt = list[i];
>>> +               strbuf_addstr(&path, absolute_path(get_worktree_git_dir(wt)));
>>> +               wt->is_current = !strcmp_icase(git_dir.buf, path.buf);
>>
>> Can you talk a bit about why this uses 'icase'? Should it be
>> respecting cache.h:ignore_case?
>
> It does.That function (in dir.c) is just one-liner
>
>     return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
>
> I admit though, the naming does not make that clear.

While we're at it, how about renaming it to pathcmp (and its friend
strncmp_icase to pathncmp)?
--
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 04/12] worktree.c: mark current worktree

Eric Sunshine
On Thu, Apr 21, 2016 at 5:33 AM, Duy Nguyen <[hidden email]> wrote:

> On Thu, Apr 21, 2016 at 3:19 PM, Duy Nguyen <[hidden email]> wrote:
>> On Thu, Apr 21, 2016 at 2:20 PM, Eric Sunshine <[hidden email]> wrote:
>>> On Wed, Apr 20, 2016 at 9:24 AM, Nguyễn Thái Ngọc Duy <[hidden email]> wrote:
>>>> Signed-off-by: Nguyễn Thái Ngọc Duy <[hidden email]>
>>>> +       strbuf_addstr(&git_dir, absolute_path(get_git_dir()));
>>>> +       for (i = 0; i < counter; i++) {
>>>> +               struct worktree *wt = list[i];
>>>> +               strbuf_addstr(&path, absolute_path(get_worktree_git_dir(wt)));
>>>> +               wt->is_current = !strcmp_icase(git_dir.buf, path.buf);
>>>
>>> Can you talk a bit about why this uses 'icase'? Should it be
>>> respecting cache.h:ignore_case?
>>
>> It does.That function (in dir.c) is just one-liner
>>
>>     return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
>>
>> I admit though, the naming does not make that clear.

Ugh, this is only the fourth patch, yet the second stupid review
mistake I've made thus far in this series. For some reason, I kept
reading this as a call to strcasecmp() or stricmp() rather than
strcmp_icase(). Worse, I had even consulted path.c:strcmp_icase() to
see how the issue was handled there.

> While we're at it, how about renaming it to pathcmp (and its friend
> strncmp_icase to pathncmp)?

Yes, that seems like a good idea. For anyone familiar with
strcasecmp() or stricmp(), having "icase" in the name makes it seem as
though it's unconditionally case-insensitive, so dropping it from the
name would likely be beneficial.
--
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
123456