From: Eric Sunshine <sunshine@sunshineco.com>
To: git@vger.kernel.org
Cc: "Duy Nguyen" <pclouds@gmail.com>,
"Jonathan Müller" <jonathanmueller.dev@gmail.com>,
"Shourya Shukla" <shouryashukla.oo@gmail.com>,
"Junio C Hamano" <gitster@pobox.com>,
"Ramsay Jones" <ramsay@ramsayjones.plus.com>,
"Eric Sunshine" <sunshine@sunshineco.com>
Subject: [PATCH v2 0/7] worktree: tighten duplicate path detection
Date: Wed, 10 Jun 2020 02:30:42 -0400 [thread overview]
Message-ID: <20200610063049.74666-1-sunshine@sunshineco.com> (raw)
This is a re-roll of [1] which teaches "git worktree prune" to detect
multiple worktrees referencing the same path, and makes "git worktree
move" disallow moving a worktree atop another registered worktree when
the destination worktree is missing (for instance, if it resides on
removable media).
Changes in v2:
* drop patch 2/8, which made 'prune' remove corrupt locked worktree
entries, since it was difficult to justify the change
* fix a couple typos and a style violation
* fix a Sparse warning reported by Ramsay[2]
* fix a -Werror=main complaint noticed by Junio
[1]: https://lore.kernel.org/git/20200608062356.40264-1-sunshine@sunshineco.com/T/
[2]: https://lore.kernel.org/git/CAPig+cTF+pwBasVCzmucXmMZcm1K0ctkGOavj7bMcGsw2MvoKw@mail.gmail.com/T/
Eric Sunshine (7):
worktree: factor out repeated string literal
worktree: give "should be pruned?" function more meaningful name
worktree: make high-level pruning re-usable
worktree: prune duplicate entries referencing same worktree path
worktree: prune linked worktree referencing main worktree path
worktree: generalize candidate worktree path validation
worktree: make "move" refuse to move atop missing registered worktree
Documentation/git-worktree.txt | 4 +-
builtin/worktree.c | 128 ++++++++++++++++++++++++---------
t/t2401-worktree-prune.sh | 24 +++++++
t/t2403-worktree-move.sh | 21 ++++++
4 files changed, 141 insertions(+), 36 deletions(-)
Interdiff against v1:
diff --git a/builtin/worktree.c b/builtin/worktree.c
index dda7da497c..1238b6bab1 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -68,11 +68,11 @@ static void delete_worktrees_dir_if_empty(void)
}
/*
- * Return NULL if worktree entry should be pruned (along with reason for
- * pruning), otherwise return the path of the worktree itself. Caller is
- * responsible for freeing return value.
+ * Return true if worktree entry should be pruned, along with the reason for
+ * pruning. Otherwise, return false and the worktree's path, or NULL if it
+ * cannot be determined. Caller is responsible for freeing returned path.
*/
-static char *worktree_disposition(const char *id, struct strbuf *reason)
+static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
{
struct stat st;
char *path;
@@ -80,19 +80,22 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)
size_t len;
ssize_t read_result;
+ *wtpath = NULL;
if (!is_directory(git_path("worktrees/%s", id))) {
strbuf_addstr(reason, _("not a valid directory"));
- return NULL;
+ return 1;
}
+ if (file_exists(git_path("worktrees/%s/locked", id)))
+ return 0;
if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
strbuf_addstr(reason, _("gitdir file does not exist"));
- return NULL;
+ return 1;
}
fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
if (fd < 0) {
strbuf_addf(reason, _("unable to read gitdir file (%s)"),
strerror(errno));
- return NULL;
+ return 1;
}
len = xsize_t(st.st_size);
path = xmallocz(len);
@@ -103,7 +106,7 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)
strerror(errno));
close(fd);
free(path);
- return NULL;
+ return 1;
}
close(fd);
@@ -112,29 +115,29 @@ static char *worktree_disposition(const char *id, struct strbuf *reason)
_("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
(uintmax_t)len, (uintmax_t)read_result);
free(path);
- return NULL;
+ return 1;
}
while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
len--;
if (!len) {
strbuf_addstr(reason, _("invalid gitdir file"));
free(path);
- return NULL;
+ return 1;
}
path[len] = '\0';
if (!file_exists(path)) {
- if (file_exists(git_path("worktrees/%s/locked", id)))
- return path;
if (stat(git_path("worktrees/%s/index", id), &st) ||
st.st_mtime <= expire) {
strbuf_addstr(reason, _("gitdir file points to non-existent location"));
free(path);
- return NULL;
+ return 1;
} else {
- return path;
+ *wtpath = path;
+ return 0;
}
}
- return path;
+ *wtpath = path;
+ return 0;
}
static void prune_worktree(const char *id, const char *reason)
@@ -153,7 +156,12 @@ static int prune_cmp(const void *a, const void *b)
if ((c = fspathcmp(x->string, y->string)))
return c;
- /* paths same; main worktee (util==0) sorts above all others */
+ /*
+ * paths same; prune_dupes() removes all but the first worktree entry
+ * having the same path, so sort main worktree ('util' is NULL) above
+ * linked worktrees ('util' not NULL) since main worktree can't be
+ * removed
+ */
if (!x->util)
return -1;
if (!y->util)
@@ -176,7 +184,7 @@ static void prune_dups(struct string_list *l)
static void prune_worktrees(void)
{
struct strbuf reason = STRBUF_INIT;
- struct strbuf main = STRBUF_INIT;
+ struct strbuf main_path = STRBUF_INIT;
struct string_list kept = STRING_LIST_INIT_NODUP;
DIR *dir = opendir(git_path("worktrees"));
struct dirent *d;
@@ -187,19 +195,17 @@ static void prune_worktrees(void)
if (is_dot_or_dotdot(d->d_name))
continue;
strbuf_reset(&reason);
- path = worktree_disposition(d->d_name, &reason);
- if (path) {
+ if (should_prune_worktree(d->d_name, &reason, &path))
+ prune_worktree(d->d_name, reason.buf);
+ else if (path)
string_list_append(&kept, path)->util = xstrdup(d->d_name);
- continue;
- }
- prune_worktree(d->d_name, reason.buf);
}
closedir(dir);
- strbuf_add_absolute_path(&main, get_git_common_dir());
+ strbuf_add_absolute_path(&main_path, get_git_common_dir());
/* massage main worktree absolute path to match 'gitdir' content */
- strbuf_strip_suffix(&main, "/.");
- string_list_append(&kept, strbuf_detach(&main, 0));
+ strbuf_strip_suffix(&main_path, "/.");
+ string_list_append(&kept, strbuf_detach(&main_path, NULL));
prune_dups(&kept);
string_list_clear(&kept, 1);
diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
index 5f3db93b31..a6ce7f590b 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -69,21 +69,11 @@ test_expect_success 'prune directories with gitdir pointing to nowhere' '
'
test_expect_success 'not prune locked checkout' '
- test_when_finished rm -fr .git/worktrees ghi &&
- git worktree add ghi &&
- : >.git/worktrees/ghi/locked &&
- rm -r ghi &&
- git worktree prune &&
- test -d .git/worktrees/ghi
-'
-
-test_expect_success 'prune corrupt despite lock' '
- test_when_finished rm -fr .git/worktrees ghi &&
+ test_when_finished rm -r .git/worktrees &&
mkdir -p .git/worktrees/ghi &&
- : >.git/worktrees/ghi/gitdir &&
: >.git/worktrees/ghi/locked &&
git worktree prune &&
- ! test -d .git/worktrees/ghi
+ test -d .git/worktrees/ghi
'
test_expect_success 'not prune recent checkouts' '
diff --git a/t/t2403-worktree-move.sh b/t/t2403-worktree-move.sh
index 7035c9d72e..a4e1a178e0 100755
--- a/t/t2403-worktree-move.sh
+++ b/t/t2403-worktree-move.sh
@@ -113,7 +113,7 @@ test_expect_success 'move locked worktree (force)' '
'
test_expect_success 'refuse to move worktree atop existing path' '
- > bobble &&
+ >bobble &&
git worktree add --detach beeble &&
test_must_fail git worktree move beeble bobble
'
Range-diff against v1:
1: 75e2f832bf = 1: 75e2f832bf worktree: factor out repeated string literal
2: 24662000d2 < -: ---------- worktree: prune corrupted worktree even if locked
3: 4fb95b3eea = 2: 0e458b3da5 worktree: give "should be pruned?" function more meaningful name
4: d16d993aa2 = 3: 7cf5b6ca40 worktree: make high-level pruning re-usable
5: f6bf2f0e72 ! 4: e28790761f worktree: prune duplicate entries referencing same worktree path
@@ builtin/worktree.c: static void delete_worktrees_dir_if_empty(void)
-static int should_prune_worktree(const char *id, struct strbuf *reason)
+/*
-+ * Return NULL if worktree entry should be pruned (along with reason for
-+ * pruning), otherwise return the path of the worktree itself. Caller is
-+ * responsible for freeing return value.
++ * Return true if worktree entry should be pruned, along with the reason for
++ * pruning. Otherwise, return false and the worktree's path, or NULL if it
++ * cannot be determined. Caller is responsible for freeing returned path.
+ */
-+static char *worktree_disposition(const char *id, struct strbuf *reason)
++static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
{
struct stat st;
char *path;
@@ builtin/worktree.c: static int should_prune_worktree(const char *id, struct strbuf *reason)
+ size_t len;
+ ssize_t read_result;
++ *wtpath = NULL;
if (!is_directory(git_path("worktrees/%s", id))) {
strbuf_addstr(reason, _("not a valid directory"));
-- return 1;
-+ return NULL;
- }
- if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
- strbuf_addstr(reason, _("gitdir file does not exist"));
-- return 1;
-+ return NULL;
- }
- fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
- if (fd < 0) {
- strbuf_addf(reason, _("unable to read gitdir file (%s)"),
- strerror(errno));
-- return 1;
-+ return NULL;
- }
- len = xsize_t(st.st_size);
- path = xmallocz(len);
-@@ builtin/worktree.c: static int should_prune_worktree(const char *id, struct strbuf *reason)
- strerror(errno));
- close(fd);
- free(path);
-- return 1;
-+ return NULL;
- }
- close(fd);
-
+ return 1;
@@ builtin/worktree.c: static int should_prune_worktree(const char *id, struct strbuf *reason)
- _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
- (uintmax_t)len, (uintmax_t)read_result);
- free(path);
-- return 1;
-+ return NULL;
- }
- while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
- len--;
- if (!len) {
- strbuf_addstr(reason, _("invalid gitdir file"));
- free(path);
-- return 1;
-+ return NULL;
}
path[len] = '\0';
if (!file_exists(path)) {
- free(path);
- if (file_exists(git_path("worktrees/%s/locked", id)))
-- return 0;
-+ return path;
if (stat(git_path("worktrees/%s/index", id), &st) ||
st.st_mtime <= expire) {
strbuf_addstr(reason, _("gitdir file points to non-existent location"));
-- return 1;
+ free(path);
-+ return NULL;
+ return 1;
} else {
-- return 0;
-+ return path;
++ *wtpath = path;
+ return 0;
}
}
- free(path);
-- return 0;
-+ return path;
++ *wtpath = path;
+ return 0;
}
- static void prune_worktree(const char *id, const char *reason)
@@ builtin/worktree.c: static void prune_worktree(const char *id, const char *reason)
delete_git_dir(id);
}
@@ builtin/worktree.c: static void prune_worktree(const char *id, const char *reaso
continue;
strbuf_reset(&reason);
- if (!should_prune_worktree(d->d_name, &reason))
-+ path = worktree_disposition(d->d_name, &reason);
-+ if (path) {
+- continue;
+- prune_worktree(d->d_name, reason.buf);
++ if (should_prune_worktree(d->d_name, &reason, &path))
++ prune_worktree(d->d_name, reason.buf);
++ else if (path)
+ string_list_append(&kept, path)->util = xstrdup(d->d_name);
- continue;
-+ }
- prune_worktree(d->d_name, reason.buf);
}
closedir(dir);
+
6: 6244cbb689 ! 5: ded0632001 worktree: prune linked worktree referencing main worktree path
@@ builtin/worktree.c: static int prune_cmp(const void *a, const void *b)
if ((c = fspathcmp(x->string, y->string)))
return c;
-+ /* paths same; main worktee (util==0) sorts above all others */
++ /*
++ * paths same; prune_dupes() removes all but the first worktree entry
++ * having the same path, so sort main worktree ('util' is NULL) above
++ * linked worktrees ('util' not NULL) since main worktree can't be
++ * removed
++ */
+ if (!x->util)
+ return -1;
+ if (!y->util)
@@ builtin/worktree.c: static void prune_dups(struct string_list *l)
static void prune_worktrees(void)
{
struct strbuf reason = STRBUF_INIT;
-+ struct strbuf main = STRBUF_INIT;
++ struct strbuf main_path = STRBUF_INIT;
struct string_list kept = STRING_LIST_INIT_NODUP;
DIR *dir = opendir(git_path("worktrees"));
struct dirent *d;
@@ builtin/worktree.c: static void prune_worktrees(void)
}
closedir(dir);
-+ strbuf_add_absolute_path(&main, get_git_common_dir());
++ strbuf_add_absolute_path(&main_path, get_git_common_dir());
+ /* massage main worktree absolute path to match 'gitdir' content */
-+ strbuf_strip_suffix(&main, "/.");
-+ string_list_append(&kept, strbuf_detach(&main, 0));
++ strbuf_strip_suffix(&main_path, "/.");
++ string_list_append(&kept, strbuf_detach(&main_path, NULL));
prune_dups(&kept);
string_list_clear(&kept, 1);
7: 1355b373d3 = 6: 2ca210fa73 worktree: generalize candidate worktree path validation
8: 8bfe91bfd2 ! 7: 74dd7d1ac0 worktree: make "move" refuse to move atop missing registered worktree
@@ Commit message
careful when validating the destination location and will happily move
the source worktree atop the location of a missing worktree. This leads
to the anomalous situation of multiple worktrees being associated with
- the same path, which is expressively forbidden by design. For example:
+ the same path, which is expressly forbidden by design. For example:
$ git clone foo.git
$ cd foo
@@ t/t2403-worktree-move.sh: test_expect_success 'move locked worktree (force)' '
'
+test_expect_success 'refuse to move worktree atop existing path' '
-+ > bobble &&
++ >bobble &&
+ git worktree add --detach beeble &&
+ test_must_fail git worktree move beeble bobble
+'
--
2.27.0.90.gabb59f83a2
next reply other threads:[~2020-06-10 6:32 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-10 6:30 Eric Sunshine [this message]
2020-06-10 6:30 ` [PATCH v2 1/7] worktree: factor out repeated string literal Eric Sunshine
2020-06-10 6:30 ` [PATCH v2 2/7] worktree: give "should be pruned?" function more meaningful name Eric Sunshine
2020-06-10 6:30 ` [PATCH v2 3/7] worktree: make high-level pruning re-usable Eric Sunshine
2020-06-10 6:30 ` [PATCH v2 4/7] worktree: prune duplicate entries referencing same worktree path Eric Sunshine
2020-06-10 11:50 ` Shourya Shukla
2020-06-10 15:21 ` Eric Sunshine
2020-06-10 17:34 ` Junio C Hamano
2020-06-10 6:30 ` [PATCH v2 5/7] worktree: prune linked worktree referencing main " Eric Sunshine
2020-06-10 6:30 ` [PATCH v2 6/7] worktree: generalize candidate worktree path validation Eric Sunshine
2020-06-10 17:11 ` Shourya Shukla
2020-06-10 17:18 ` Eric Sunshine
2020-06-10 6:30 ` [PATCH v2 7/7] worktree: make "move" refuse to move atop missing registered worktree Eric Sunshine
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200610063049.74666-1-sunshine@sunshineco.com \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathanmueller.dev@gmail.com \
--cc=pclouds@gmail.com \
--cc=ramsay@ramsayjones.plus.com \
--cc=shouryashukla.oo@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.