* [PATCH v6 0/8] protect branches checked out in all worktrees
@ 2021-11-13 3:33 Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 1/8] fetch: lowercase error messages Anders Kaseorg
` (8 more replies)
0 siblings, 9 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
‘git fetch’ (without ‘--update-head-ok’), ‘git receive-pack’, and ‘git
branch -M’ protect the currently checked out branch from being
accidentally updated. However, the code for these checks predates
‘git worktree’. Improve it to protect branches checked out in all
worktrees, not just the current one.
Anders Kaseorg (8):
fetch: lowercase error messages
receive-pack: lowercase error messages
branch: lowercase error messages
worktree: simplify find_shared_symref() memory ownership model
fetch: protect branches checked out in all worktrees
receive-pack: clean dead code from update_worktree()
receive-pack: protect current branch for bare repository worktree
branch: protect branches checked out in all worktrees
branch.c | 41 +++++++-----
builtin/branch.c | 7 +-
builtin/fetch.c | 115 +++++++++++++++++---------------
builtin/notes.c | 6 +-
builtin/receive-pack.c | 88 +++++++++++++-----------
t/t2018-checkout-branch.sh | 2 +-
t/t3200-branch.sh | 9 ++-
t/t5504-fetch-receive-strict.sh | 2 +-
t/t5516-fetch-push.sh | 32 +++++++++
worktree.c | 8 +--
worktree.h | 5 +-
11 files changed, 191 insertions(+), 124 deletions(-)
--
2.33.1
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 1/8] fetch: lowercase error messages
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-16 5:19 ` Junio C Hamano
2021-11-22 1:14 ` Jiang Xin
2021-11-13 3:33 ` [PATCH v6 2/8] receive-pack: " Anders Kaseorg
` (7 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
Documentation/CodingGuidelines says “do not end error messages with a
full stop” and “do not capitalize the first word”. Reviewers requested
updating the existing messages to comply with these guidelines prior to
the following patches.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
builtin/fetch.c | 46 ++++++++++++++++++++++++----------------------
1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e064687dbd..e5971fa6e5 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -552,7 +552,7 @@ static struct ref *get_ref_map(struct remote *remote,
for (i = 0; i < fetch_refspec->nr; i++)
get_fetch_map(ref_map, &fetch_refspec->items[i], &oref_tail, 1);
} else if (refmap.nr) {
- die("--refmap option is only meaningful with command-line refspec(s).");
+ die("--refmap option is only meaningful with command-line refspec(s)");
} else {
/* Use the defaults */
struct branch *branch = branch_get(NULL);
@@ -583,7 +583,7 @@ static struct ref *get_ref_map(struct remote *remote,
} else if (!prefetch) {
ref_map = get_remote_ref(remote_refs, "HEAD");
if (!ref_map)
- die(_("Couldn't find remote ref HEAD"));
+ die(_("couldn't find remote ref HEAD"));
ref_map->fetch_head_status = FETCH_HEAD_MERGE;
tail = &ref_map->next;
}
@@ -1062,13 +1062,13 @@ static void close_fetch_head(struct fetch_head *fetch_head)
}
static const char warn_show_forced_updates[] =
-N_("Fetch normally indicates which branches had a forced update,\n"
- "but that check has been disabled. To re-enable, use '--show-forced-updates'\n"
- "flag or run 'git config fetch.showForcedUpdates true'.");
+N_("fetch normally indicates which branches had a forced update,\n"
+ "but that check has been disabled; to re-enable, use '--show-forced-updates'\n"
+ "flag or run 'git config fetch.showForcedUpdates true'");
static const char warn_time_show_forced_updates[] =
-N_("It took %.2f seconds to check forced updates. You can use\n"
+N_("it took %.2f seconds to check forced updates; you can use\n"
"'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n"
- " to avoid this check.\n");
+ " to avoid this check\n");
static int store_updated_refs(const char *raw_url, const char *remote_name,
int connectivity_checked, struct ref *ref_map)
@@ -1381,8 +1381,9 @@ static void check_not_current_branch(struct ref *ref_map)
for (; ref_map; ref_map = ref_map->next)
if (ref_map->peer_ref && !strcmp(current_branch->refname,
ref_map->peer_ref->name))
- die(_("Refusing to fetch into current branch %s "
- "of non-bare repository"), current_branch->refname);
+ die(_("refusing to fetch into current branch %s "
+ "of non-bare repository"),
+ current_branch->refname);
}
static int truncate_fetch_head(void)
@@ -1400,10 +1401,10 @@ static void set_option(struct transport *transport, const char *name, const char
{
int r = transport_set_option(transport, name, value);
if (r < 0)
- die(_("Option \"%s\" value \"%s\" is not valid for %s"),
+ die(_("option \"%s\" value \"%s\" is not valid for %s"),
name, value, transport->url);
if (r > 0)
- warning(_("Option \"%s\" is ignored for %s\n"),
+ warning(_("option \"%s\" is ignored for %s\n"),
name, transport->url);
}
@@ -1437,7 +1438,7 @@ static void add_negotiation_tips(struct git_transport_options *smart_options)
old_nr = oids->nr;
for_each_glob_ref(add_oid, s, oids);
if (old_nr == oids->nr)
- warning("Ignoring --negotiation-tip=%s because it does not match any refs",
+ warning("ignoring --negotiation-tip=%s because it does not match any refs",
s);
}
smart_options->negotiation_tips = oids;
@@ -1475,7 +1476,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
if (transport->smart_options)
add_negotiation_tips(transport->smart_options);
else
- warning("Ignoring --negotiation-tip because the protocol does not support it.");
+ warning("ignoring --negotiation-tip because the protocol does not support it");
}
return transport;
}
@@ -1638,8 +1639,8 @@ static int do_fetch(struct transport *transport,
else
warning(_("unknown branch type"));
} else {
- warning(_("no source branch found.\n"
- "you need to specify exactly one branch with the --set-upstream option."));
+ warning(_("no source branch found;\n"
+ "you need to specify exactly one branch with the --set-upstream option"));
}
}
skip:
@@ -1893,8 +1894,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
int remote_via_config = remote_is_configured(remote, 0);
if (!remote)
- die(_("No remote repository specified. Please, specify either a URL or a\n"
- "remote name from which new revisions should be fetched."));
+ die(_("no remote repository specified; please specify either a URL or a\n"
+ "remote name from which new revisions should be fetched"));
gtransport = prepare_transport(remote, 1);
@@ -1929,7 +1930,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
if (!strcmp(argv[i], "tag")) {
i++;
if (i >= argc)
- die(_("You need to specify a tag name."));
+ die(_("you need to specify a tag name"));
refspec_appendf(&rs, "refs/tags/%s:refs/tags/%s",
argv[i], argv[i]);
@@ -1997,7 +1998,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
if (deepen_relative) {
if (deepen_relative < 0)
- die(_("Negative depth in --deepen is not supported"));
+ die(_("negative depth in --deepen is not supported"));
if (depth)
die(_("--deepen and --depth are mutually exclusive"));
depth = xstrfmt("%d", deepen_relative);
@@ -2034,14 +2035,15 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
/* All arguments are assumed to be remotes or groups */
for (i = 0; i < argc; i++)
if (!add_remote_or_group(argv[i], &list))
- die(_("No such remote or remote group: %s"), argv[i]);
+ die(_("no such remote or remote group: %s"),
+ argv[i]);
} else {
/* Single remote or group */
(void) add_remote_or_group(argv[0], &list);
if (list.nr > 1) {
/* More than one remote */
if (argc > 1)
- die(_("Fetching a group and specifying refspecs does not make sense"));
+ die(_("fetching a group and specifying refspecs does not make sense"));
} else {
/* Zero or one remotes */
remote = remote_get(argv[0]);
@@ -2062,7 +2064,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
if (gtransport->smart_options) {
gtransport->smart_options->acked_commits = &acked_commits;
} else {
- warning(_("Protocol does not support --negotiate-only, exiting."));
+ warning(_("protocol does not support --negotiate-only, exiting"));
return 1;
}
if (server_options.nr)
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 2/8] receive-pack: lowercase error messages
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 1/8] fetch: lowercase error messages Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-18 4:53 ` Junio C Hamano
2021-11-13 3:33 ` [PATCH v6 3/8] branch: " Anders Kaseorg
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
Documentation/CodingGuidelines says “do not end error messages with a
full stop” and “do not capitalize the first word”. Reviewers requested
updating the existing messages to comply with these guidelines prior to
the following patches.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
builtin/receive-pack.c | 6 +++---
t/t5504-fetch-receive-strict.sh | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2d1f97e1ca..a82b60f387 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -170,7 +170,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
strbuf_addf(&fsck_msg_types, "%c%s=%s",
fsck_msg_types.len ? ',' : '=', var, value);
else
- warning("Skipping unknown msg id '%s'", var);
+ warning("skipping unknown msg id '%s'", var);
return 0;
}
@@ -1584,9 +1584,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
if (!parse_object(the_repository, old_oid)) {
old_oid = NULL;
if (ref_exists(name)) {
- rp_warning("Allowing deletion of corrupt ref.");
+ rp_warning("allowing deletion of corrupt ref");
} else {
- rp_warning("Deleting a non-existent ref.");
+ rp_warning("deleting a non-existent ref");
cmd->did_not_exist = 1;
}
}
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 6e5a9c20e7..b0b795aca9 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -292,7 +292,7 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
receive.fsck.missingEmail warn &&
git push --porcelain dst bogus >act 2>&1 &&
grep "missingEmail" act &&
- test_i18ngrep "Skipping unknown msg id.*whatever" act &&
+ test_i18ngrep "skipping unknown msg id.*whatever" act &&
git --git-dir=dst/.git branch -D bogus &&
git --git-dir=dst/.git config --add \
receive.fsck.missingEmail ignore &&
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 3/8] branch: lowercase error messages
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 1/8] fetch: lowercase error messages Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 2/8] receive-pack: " Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model Anders Kaseorg
` (5 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
Documentation/CodingGuidelines says “do not end error messages with a
full stop” and “do not capitalize the first word”. Reviewers requested
updating the existing messages to comply with these guidelines prior to
the following patches.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
branch.c | 16 ++++++++--------
t/t2018-checkout-branch.sh | 2 +-
t/t3200-branch.sh | 2 +-
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/branch.c b/branch.c
index 7a88a4861e..147827cf46 100644
--- a/branch.c
+++ b/branch.c
@@ -153,7 +153,7 @@ static void setup_tracking(const char *new_ref, const char *orig_ref,
}
if (tracking.matches > 1)
- die(_("Not tracking: ambiguous information for ref %s"),
+ die(_("not tracking: ambiguous information for ref %s"),
orig_ref);
if (install_branch_config(config_flags, new_ref, tracking.remote,
@@ -186,7 +186,7 @@ int read_branch_desc(struct strbuf *buf, const char *branch_name)
int validate_branchname(const char *name, struct strbuf *ref)
{
if (strbuf_check_branch_ref(ref, name))
- die(_("'%s' is not a valid branch name."), name);
+ die(_("'%s' is not a valid branch name"), name);
return ref_exists(ref->buf);
}
@@ -205,12 +205,12 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
return 0;
if (!force)
- die(_("A branch named '%s' already exists."),
+ die(_("a branch named '%s' already exists"),
ref->buf + strlen("refs/heads/"));
head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
- die(_("Cannot force update the current branch."));
+ die(_("cannot force update the current branch"));
return 1;
}
@@ -230,7 +230,7 @@ static int validate_remote_tracking_branch(char *ref)
}
static const char upstream_not_branch[] =
-N_("Cannot setup tracking information; starting point '%s' is not a branch.");
+N_("cannot set up tracking information; starting point '%s' is not a branch");
static const char upstream_missing[] =
N_("the requested upstream branch '%s' does not exist");
static const char upstream_advice[] =
@@ -278,7 +278,7 @@ void create_branch(struct repository *r,
}
die(_(upstream_missing), start_name);
}
- die(_("Not a valid object name: '%s'."), start_name);
+ die(_("not a valid object name: '%s'"), start_name);
}
switch (dwim_ref(start_name, strlen(start_name), &oid, &real_ref, 0)) {
@@ -298,12 +298,12 @@ void create_branch(struct repository *r,
}
break;
default:
- die(_("Ambiguous object name: '%s'."), start_name);
+ die(_("ambiguous object name: '%s'"), start_name);
break;
}
if ((commit = lookup_commit_reference(r, &oid)) == NULL)
- die(_("Not a valid branch point: '%s'."), start_name);
+ die(_("not a valid branch point: '%s'"), start_name);
oidcpy(&oid, &commit->object.oid);
if (reflog)
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 93be1c0eae..3e93506c04 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -148,7 +148,7 @@ test_expect_success 'checkout -b to an existing branch fails' '
test_expect_success 'checkout -b to @{-1} fails with the right branch name' '
git checkout branch1 &&
git checkout branch2 &&
- echo >expect "fatal: A branch named '\''branch1'\'' already exists." &&
+ echo >expect "fatal: a branch named '\''branch1'\'' already exists" &&
test_must_fail git checkout -b @{-1} 2>actual &&
test_cmp expect actual
'
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e575ffb4ff..6496e5dd38 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -866,7 +866,7 @@ test_expect_success '--set-upstream-to fails on a missing src branch' '
'
test_expect_success '--set-upstream-to fails on a non-ref' '
- echo "fatal: Cannot setup tracking information; starting point '"'"'HEAD^{}'"'"' is not a branch." >expect &&
+ echo "fatal: cannot set up tracking information; starting point '"'"'HEAD^{}'"'"' is not a branch" >expect &&
test_must_fail git branch --set-upstream-to HEAD^{} 2>err &&
test_cmp expect err
'
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
` (2 preceding siblings ...)
2021-11-13 3:33 ` [PATCH v6 3/8] branch: " Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-16 5:39 ` Junio C Hamano
2021-11-22 12:45 ` Johannes Schindelin
2021-11-13 3:33 ` [PATCH v6 5/8] fetch: protect branches checked out in all worktrees Anders Kaseorg
` (4 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
Storing the worktrees list in a static variable meant that
find_shared_symref() had to rebuild the list on each call (which is
inefficient when the call site is in a loop), and also that each call
invalidated the pointer returned by the previous call (which is
confusing).
Instead, make it the caller’s responsibility to pass in the worktrees
list and manage its lifetime.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
branch.c | 14 ++++++----
builtin/branch.c | 7 ++++-
builtin/notes.c | 6 +++-
builtin/receive-pack.c | 63 +++++++++++++++++++++++++++---------------
worktree.c | 8 ++----
worktree.h | 5 ++--
6 files changed, 65 insertions(+), 38 deletions(-)
diff --git a/branch.c b/branch.c
index 147827cf46..c7b9ba0e10 100644
--- a/branch.c
+++ b/branch.c
@@ -357,14 +357,16 @@ void remove_branch_state(struct repository *r, int verbose)
void die_if_checked_out(const char *branch, int ignore_current_worktree)
{
+ struct worktree **worktrees = get_worktrees();
const struct worktree *wt;
- wt = find_shared_symref("HEAD", branch);
- if (!wt || (ignore_current_worktree && wt->is_current))
- return;
- skip_prefix(branch, "refs/heads/", &branch);
- die(_("'%s' is already checked out at '%s'"),
- branch, wt->path);
+ wt = find_shared_symref(worktrees, "HEAD", branch);
+ if (wt && (!ignore_current_worktree || !wt->is_current)) {
+ skip_prefix(branch, "refs/heads/", &branch);
+ die(_("'%s' is already checked out at '%s'"), branch, wt->path);
+ }
+
+ free_worktrees(worktrees);
}
int replace_each_worktree_head_symref(const char *oldref, const char *newref,
diff --git a/builtin/branch.c b/builtin/branch.c
index 03c7b7253a..25785dc939 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -193,6 +193,7 @@ static void delete_branch_config(const char *branchname)
static int delete_branches(int argc, const char **argv, int force, int kinds,
int quiet)
{
+ struct worktree **worktrees;
struct commit *head_rev = NULL;
struct object_id oid;
char *name = NULL;
@@ -229,6 +230,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
if (!head_rev)
die(_("Couldn't look up commit object for HEAD"));
}
+
+ worktrees = get_worktrees();
+
for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
char *target = NULL;
int flags = 0;
@@ -239,7 +243,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(worktrees, "HEAD", name);
if (wt) {
error(_("Cannot delete branch '%s' "
"checked out at '%s'"),
@@ -300,6 +304,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
free(name);
strbuf_release(&bname);
+ free_worktrees(worktrees);
return ret;
}
diff --git a/builtin/notes.c b/builtin/notes.c
index 71c59583a1..7f60408dbb 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -861,15 +861,19 @@ static int merge(int argc, const char **argv, const char *prefix)
update_ref(msg.buf, default_notes_ref(), &result_oid, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
else { /* Merge has unresolved conflicts */
+ struct worktree **worktrees;
const struct worktree *wt;
/* Update .git/NOTES_MERGE_PARTIAL with partial merge result */
update_ref(msg.buf, "NOTES_MERGE_PARTIAL", &result_oid, 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());
+ worktrees = get_worktrees();
+ wt = find_shared_symref(worktrees, "NOTES_MERGE_REF",
+ default_notes_ref());
if (wt)
die(_("a notes merge into %s is already in-progress at %s"),
default_notes_ref(), wt->path);
+ free_worktrees(worktrees);
if (create_symref("NOTES_MERGE_REF", default_notes_ref(), NULL))
die(_("failed to store link to current notes ref (%s)"),
default_notes_ref());
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a82b60f387..5d7c4832c1 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1481,12 +1481,17 @@ static const char *update(struct command *cmd, struct shallow_info *si)
struct object_id *old_oid = &cmd->old_oid;
struct object_id *new_oid = &cmd->new_oid;
int do_update_worktree = 0;
- const struct worktree *worktree = is_bare_repository() ? NULL : find_shared_symref("HEAD", name);
+ struct worktree **worktrees = get_worktrees();
+ const struct worktree *worktree =
+ is_bare_repository() ?
+ NULL :
+ find_shared_symref(worktrees, "HEAD", name);
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
rp_error("refusing to create funny ref '%s' remotely", name);
- return "funny refname";
+ ret = "funny refname";
+ goto out;
}
strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
@@ -1505,7 +1510,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
rp_error("refusing to update checked out branch: %s", name);
if (deny_current_branch == DENY_UNCONFIGURED)
refuse_unconfigured_deny();
- return "branch is currently checked out";
+ ret = "branch is currently checked out";
+ goto out;
case DENY_UPDATE_INSTEAD:
/* pass -- let other checks intervene first */
do_update_worktree = 1;
@@ -1516,13 +1522,15 @@ static const char *update(struct command *cmd, struct shallow_info *si)
if (!is_null_oid(new_oid) && !has_object_file(new_oid)) {
error("unpack should have generated %s, "
"but I can't find it!", oid_to_hex(new_oid));
- return "bad pack";
+ ret = "bad pack";
+ goto out;
}
if (!is_null_oid(old_oid) && is_null_oid(new_oid)) {
if (deny_deletes && starts_with(name, "refs/heads/")) {
rp_error("denying ref deletion for %s", name);
- return "deletion prohibited";
+ ret = "deletion prohibited";
+ goto out;
}
if (worktree || (head_name && !strcmp(namespaced_name, head_name))) {
@@ -1538,9 +1546,11 @@ static const char *update(struct command *cmd, struct shallow_info *si)
if (deny_delete_current == DENY_UNCONFIGURED)
refuse_unconfigured_deny_delete_current();
rp_error("refusing to delete the current branch: %s", name);
- return "deletion of the current branch prohibited";
+ ret = "deletion of the current branch prohibited";
+ goto out;
default:
- return "Invalid denyDeleteCurrent setting";
+ ret = "Invalid denyDeleteCurrent setting";
+ goto out;
}
}
}
@@ -1558,25 +1568,30 @@ static const char *update(struct command *cmd, struct shallow_info *si)
old_object->type != OBJ_COMMIT ||
new_object->type != OBJ_COMMIT) {
error("bad sha1 objects for %s", name);
- return "bad ref";
+ ret = "bad ref";
+ goto out;
}
old_commit = (struct commit *)old_object;
new_commit = (struct commit *)new_object;
if (!in_merge_bases(old_commit, new_commit)) {
rp_error("denying non-fast-forward %s"
" (you should pull first)", name);
- return "non-fast-forward";
+ ret = "non-fast-forward";
+ goto out;
}
}
if (run_update_hook(cmd)) {
rp_error("hook declined to update %s", name);
- return "hook declined";
+ ret = "hook declined";
+ goto out;
}
if (do_update_worktree) {
- ret = update_worktree(new_oid->hash, find_shared_symref("HEAD", name));
+ ret = update_worktree(new_oid->hash,
+ find_shared_symref(worktrees, "HEAD",
+ name));
if (ret)
- return ret;
+ goto out;
}
if (is_null_oid(new_oid)) {
@@ -1595,17 +1610,19 @@ static const char *update(struct command *cmd, struct shallow_info *si)
old_oid,
0, "push", &err)) {
rp_error("%s", err.buf);
- strbuf_release(&err);
- return "failed to delete";
+ ret = "failed to delete";
+ } else {
+ ret = NULL; /* good */
}
strbuf_release(&err);
- return NULL; /* good */
}
else {
struct strbuf err = STRBUF_INIT;
if (shallow_update && si->shallow_ref[cmd->index] &&
- update_shallow_ref(cmd, si))
- return "shallow error";
+ update_shallow_ref(cmd, si)) {
+ ret = "shallow error";
+ goto out;
+ }
if (ref_transaction_update(transaction,
namespaced_name,
@@ -1613,14 +1630,16 @@ static const char *update(struct command *cmd, struct shallow_info *si)
0, "push",
&err)) {
rp_error("%s", err.buf);
- strbuf_release(&err);
-
- return "failed to update ref";
+ ret = "failed to update ref";
+ } else {
+ ret = NULL; /* good */
}
strbuf_release(&err);
-
- return NULL; /* good */
}
+
+out:
+ free_worktrees(worktrees);
+ return ret;
}
static void run_update_post_hook(struct command *commands)
diff --git a/worktree.c b/worktree.c
index 092a4f92ad..cf13d63845 100644
--- a/worktree.c
+++ b/worktree.c
@@ -402,17 +402,13 @@ int is_worktree_being_bisected(const struct worktree *wt,
* bisect). New commands that do similar things should update this
* function as well.
*/
-const struct worktree *find_shared_symref(const char *symref,
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+ const char *symref,
const char *target)
{
const struct worktree *existing = NULL;
- static struct worktree **worktrees;
int i = 0;
- if (worktrees)
- free_worktrees(worktrees);
- worktrees = get_worktrees();
-
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
const char *symref_target;
diff --git a/worktree.h b/worktree.h
index 8b7c408132..9e06fcbdf3 100644
--- a/worktree.h
+++ b/worktree.h
@@ -143,9 +143,10 @@ 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 worktree that holds the ref,
- * or NULL otherwise. The result may be destroyed by the next call.
+ * or NULL otherwise.
*/
-const struct worktree *find_shared_symref(const char *symref,
+const struct worktree *find_shared_symref(struct worktree **worktrees,
+ const char *symref,
const char *target);
/*
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 5/8] fetch: protect branches checked out in all worktrees
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
` (3 preceding siblings ...)
2021-11-13 3:33 ` [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-16 5:49 ` Junio C Hamano
2021-11-22 13:13 ` Johannes Schindelin
2021-11-13 3:33 ` [PATCH v6 6/8] receive-pack: clean dead code from update_worktree() Anders Kaseorg
` (3 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
Refuse to fetch into the currently checked out branch of any working
tree, not just the current one.
Fixes this previously reported bug:
https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de
As a side effect of using find_shared_symref, we’ll also refuse the
fetch when we’re on a detached HEAD because we’re rebasing or bisecting
on the branch in question. This seems like a sensible change.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
builtin/fetch.c | 75 +++++++++++++++++++++++--------------------
t/t5516-fetch-push.sh | 18 +++++++++++
2 files changed, 58 insertions(+), 35 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e5971fa6e5..f373252490 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -28,6 +28,7 @@
#include "promisor-remote.h"
#include "commit-graph.h"
#include "shallow.h"
+#include "worktree.h"
#define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
@@ -840,14 +841,13 @@ static void format_display(struct strbuf *display, char code,
static int update_local_ref(struct ref *ref,
struct ref_transaction *transaction,
- const char *remote,
- const struct ref *remote_ref,
- struct strbuf *display,
- int summary_width)
+ const char *remote, const struct ref *remote_ref,
+ struct strbuf *display, int summary_width,
+ struct worktree **worktrees)
{
struct commit *current = NULL, *updated;
enum object_type type;
- struct branch *current_branch = branch_get(NULL);
+ const struct worktree *wt;
const char *pretty_ref = prettify_refname(ref->name);
int fast_forward = 0;
@@ -862,16 +862,17 @@ static int update_local_ref(struct ref *ref,
return 0;
}
- if (current_branch &&
- !strcmp(ref->name, current_branch->name) &&
- !(update_head_ok || is_bare_repository()) &&
- !is_null_oid(&ref->old_oid)) {
+ if (!update_head_ok &&
+ (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
+ !wt->is_bare && !is_null_oid(&ref->old_oid)) {
/*
* If this is the head, and it's not okay to update
* the head, and the old value of the head isn't empty...
*/
format_display(display, '!', _("[rejected]"),
- _("can't fetch in current branch"),
+ wt->is_current ?
+ _("can't fetch in current branch") :
+ _("checked out in another worktree"),
remote, pretty_ref, summary_width);
return 1;
}
@@ -1071,7 +1072,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
" to avoid this check\n");
static int store_updated_refs(const char *raw_url, const char *remote_name,
- int connectivity_checked, struct ref *ref_map)
+ int connectivity_checked, struct ref *ref_map,
+ struct worktree **worktrees)
{
struct fetch_head fetch_head;
struct commit *commit;
@@ -1188,7 +1190,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
strbuf_reset(¬e);
if (ref) {
rc |= update_local_ref(ref, transaction, what,
- rm, ¬e, summary_width);
+ rm, ¬e, summary_width,
+ worktrees);
free(ref);
} else if (write_fetch_head || dry_run) {
/*
@@ -1301,16 +1304,15 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
}
/* Update local refs based on the ref values fetched from a remote */
-static int consume_refs(struct transport *transport, struct ref *ref_map)
+static int consume_refs(struct transport *transport, struct ref *ref_map,
+ struct worktree **worktrees)
{
int connectivity_checked = transport->smart_options
? transport->smart_options->connectivity_checked : 0;
int ret;
trace2_region_enter("fetch", "consume_refs", the_repository);
- ret = store_updated_refs(transport->url,
- transport->remote->name,
- connectivity_checked,
- ref_map);
+ ret = store_updated_refs(transport->url, transport->remote->name,
+ connectivity_checked, ref_map, worktrees);
transport_unlock_pack(transport);
trace2_region_leave("fetch", "consume_refs", the_repository);
return ret;
@@ -1371,19 +1373,18 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
return result;
}
-static void check_not_current_branch(struct ref *ref_map)
+static void check_not_current_branch(struct ref *ref_map,
+ struct worktree **worktrees)
{
- struct branch *current_branch = branch_get(NULL);
-
- if (is_bare_repository() || !current_branch)
- return;
-
+ const struct worktree *wt;
for (; ref_map; ref_map = ref_map->next)
- if (ref_map->peer_ref && !strcmp(current_branch->refname,
- ref_map->peer_ref->name))
- die(_("refusing to fetch into current branch %s "
- "of non-bare repository"),
- current_branch->refname);
+ if (ref_map->peer_ref &&
+ (wt = find_shared_symref(worktrees, "HEAD",
+ ref_map->peer_ref->name)) &&
+ !wt->is_bare)
+ die(_("refusing to fetch into branch '%s' "
+ "checked out at '%s'"),
+ ref_map->peer_ref->name, wt->path);
}
static int truncate_fetch_head(void)
@@ -1481,7 +1482,8 @@ static struct transport *prepare_transport(struct remote *remote, int deepen)
return transport;
}
-static void backfill_tags(struct transport *transport, struct ref *ref_map)
+static void backfill_tags(struct transport *transport, struct ref *ref_map,
+ struct worktree **worktrees)
{
int cannot_reuse;
@@ -1503,7 +1505,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
if (!fetch_refs(transport, ref_map))
- consume_refs(transport, ref_map);
+ consume_refs(transport, ref_map, worktrees);
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1521,6 +1523,7 @@ static int do_fetch(struct transport *transport,
struct transport_ls_refs_options transport_ls_refs_options =
TRANSPORT_LS_REFS_OPTIONS_INIT;
int must_list_refs = 1;
+ struct worktree **worktrees = get_worktrees();
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1576,7 +1579,7 @@ static int do_fetch(struct transport *transport,
ref_map = get_ref_map(transport->remote, remote_refs, rs,
tags, &autotags);
if (!update_head_ok)
- check_not_current_branch(ref_map);
+ check_not_current_branch(ref_map, worktrees);
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
@@ -1594,7 +1597,8 @@ static int do_fetch(struct transport *transport,
transport->url);
}
}
- if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {
+ if (fetch_refs(transport, ref_map) ||
+ consume_refs(transport, ref_map, worktrees)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
@@ -1643,7 +1647,7 @@ static int do_fetch(struct transport *transport,
"you need to specify exactly one branch with the --set-upstream option"));
}
}
- skip:
+skip:
free_refs(ref_map);
/* if neither --no-tags nor --tags was specified, do automated tag
@@ -1653,11 +1657,12 @@ static int do_fetch(struct transport *transport,
ref_map = NULL;
find_non_local_tags(remote_refs, &ref_map, &tail);
if (ref_map)
- backfill_tags(transport, ref_map);
+ backfill_tags(transport, ref_map, worktrees);
free_refs(ref_map);
}
- cleanup:
+cleanup:
+ free_worktrees(worktrees);
return retcode;
}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4db8edd9c8..36fb90f4b0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1770,4 +1770,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '
git -C cloned push origin HEAD:new-wt &&
test_must_fail git -C cloned push --delete origin new-wt
'
+
+test_expect_success 'refuse fetch to current branch of worktree' '
+ test_when_finished "git worktree remove --force wt && git branch -D wt" &&
+ git worktree add wt &&
+ test_commit apple &&
+ test_must_fail git fetch . HEAD:wt &&
+ git fetch -u . HEAD:wt
+'
+
+test_expect_success 'refuse fetch to current branch of bare repository worktree' '
+ test_when_finished "rm -fr bare.git" &&
+ git clone --bare . bare.git &&
+ git -C bare.git worktree add wt &&
+ test_commit banana &&
+ test_must_fail git -C bare.git fetch .. HEAD:wt &&
+ git -C bare.git fetch -u .. HEAD:wt
+'
+
test_done
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 6/8] receive-pack: clean dead code from update_worktree()
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
` (4 preceding siblings ...)
2021-11-13 3:33 ` [PATCH v6 5/8] fetch: protect branches checked out in all worktrees Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-16 5:49 ` Junio C Hamano
2021-11-13 3:33 ` [PATCH v6 7/8] receive-pack: protect current branch for bare repository worktree Anders Kaseorg
` (2 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
update_worktree() can only be called with a non-NULL worktree parameter,
because that’s the only case where we set do_update_worktree = 1.
worktree->path is always initialized to non-NULL.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
builtin/receive-pack.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5d7c4832c1..b04d4ad268 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1444,29 +1444,22 @@ static const char *push_to_checkout(unsigned char *hash,
static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
{
- const char *retval, *work_tree, *git_dir = NULL;
+ const char *retval, *git_dir;
struct strvec env = STRVEC_INIT;
- if (worktree && worktree->path)
- work_tree = worktree->path;
- else if (git_work_tree_cfg)
- work_tree = git_work_tree_cfg;
- else
- work_tree = "..";
+ if (!worktree || !worktree->path)
+ BUG("worktree->path must be non-NULL");
if (is_bare_repository())
return "denyCurrentBranch = updateInstead needs a worktree";
- if (worktree)
- git_dir = get_worktree_git_dir(worktree);
- if (!git_dir)
- git_dir = get_git_dir();
+ git_dir = get_worktree_git_dir(worktree);
strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
if (!find_hook(push_to_checkout_hook))
- retval = push_to_deploy(sha1, &env, work_tree);
+ retval = push_to_deploy(sha1, &env, worktree->path);
else
- retval = push_to_checkout(sha1, &env, work_tree);
+ retval = push_to_checkout(sha1, &env, worktree->path);
strvec_clear(&env);
return retval;
@@ -1587,9 +1580,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}
if (do_update_worktree) {
- ret = update_worktree(new_oid->hash,
- find_shared_symref(worktrees, "HEAD",
- name));
+ ret = update_worktree(new_oid->hash, worktree);
if (ret)
goto out;
}
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 7/8] receive-pack: protect current branch for bare repository worktree
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
` (5 preceding siblings ...)
2021-11-13 3:33 ` [PATCH v6 6/8] receive-pack: clean dead code from update_worktree() Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 8/8] branch: protect branches checked out in all worktrees Anders Kaseorg
2021-11-22 13:21 ` [PATCH v6 0/8] " Johannes Schindelin
8 siblings, 0 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
A bare repository won’t have a working tree at "..", but it may still
have separate working trees created with git worktree. We should protect
the current branch of such working trees from being updated or deleted,
according to receive.denyCurrentBranch.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
builtin/receive-pack.c | 8 +++-----
t/t5516-fetch-push.sh | 14 ++++++++++++++
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b04d4ad268..d72058543e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1450,7 +1450,7 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
if (!worktree || !worktree->path)
BUG("worktree->path must be non-NULL");
- if (is_bare_repository())
+ if (worktree->is_bare)
return "denyCurrentBranch = updateInstead needs a worktree";
git_dir = get_worktree_git_dir(worktree);
@@ -1476,9 +1476,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
int do_update_worktree = 0;
struct worktree **worktrees = get_worktrees();
const struct worktree *worktree =
- is_bare_repository() ?
- NULL :
- find_shared_symref(worktrees, "HEAD", name);
+ find_shared_symref(worktrees, "HEAD", name);
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1491,7 +1489,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
free(namespaced_name);
namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
- if (worktree) {
+ if (worktree && !worktree->is_bare) {
switch (deny_current_branch) {
case DENY_IGNORE:
break;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 36fb90f4b0..ecdda807d3 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1768,9 +1768,23 @@ test_expect_success 'denyCurrentBranch and worktrees' '
test_must_fail git -C cloned push origin HEAD:new-wt &&
test_config receive.denyCurrentBranch updateInstead &&
git -C cloned push origin HEAD:new-wt &&
+ test_path_exists new-wt/first.t &&
test_must_fail git -C cloned push --delete origin new-wt
'
+test_expect_success 'denyCurrentBranch and bare repository worktrees' '
+ test_when_finished "rm -fr bare.git" &&
+ git clone --bare . bare.git &&
+ git -C bare.git worktree add wt &&
+ test_commit grape &&
+ git -C bare.git config receive.denyCurrentBranch refuse &&
+ test_must_fail git push bare.git HEAD:wt &&
+ git -C bare.git config receive.denyCurrentBranch updateInstead &&
+ git push bare.git HEAD:wt &&
+ test_path_exists bare.git/wt/grape.t &&
+ test_must_fail git push --delete bare.git wt
+'
+
test_expect_success 'refuse fetch to current branch of worktree' '
test_when_finished "git worktree remove --force wt && git branch -D wt" &&
git worktree add wt &&
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v6 8/8] branch: protect branches checked out in all worktrees
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
` (6 preceding siblings ...)
2021-11-13 3:33 ` [PATCH v6 7/8] receive-pack: protect current branch for bare repository worktree Anders Kaseorg
@ 2021-11-13 3:33 ` Anders Kaseorg
2021-11-22 13:21 ` [PATCH v6 0/8] " Johannes Schindelin
8 siblings, 0 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-13 3:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason, Anders Kaseorg
Refuse to force-move a branch over the currently checked out branch of
any working tree, not just the current one.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
---
branch.c | 13 +++++++++----
t/t3200-branch.sh | 7 +++++++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/branch.c b/branch.c
index c7b9ba0e10..3a7d205fa4 100644
--- a/branch.c
+++ b/branch.c
@@ -199,7 +199,8 @@ int validate_branchname(const char *name, struct strbuf *ref)
*/
int validate_new_branchname(const char *name, struct strbuf *ref, int force)
{
- const char *head;
+ struct worktree **worktrees;
+ const struct worktree *wt;
if (!validate_branchname(name, ref))
return 0;
@@ -208,9 +209,13 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
die(_("a branch named '%s' already exists"),
ref->buf + strlen("refs/heads/"));
- head = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
- if (!is_bare_repository() && head && !strcmp(head, ref->buf))
- die(_("cannot force update the current branch"));
+ worktrees = get_worktrees();
+ wt = find_shared_symref(worktrees, "HEAD", ref->buf);
+ if (wt && !wt->is_bare)
+ die(_("cannot force update the branch '%s'"
+ "checked out at '%s'"),
+ ref->buf + strlen("refs/heads/"), wt->path);
+ free_worktrees(worktrees);
return 1;
}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6496e5dd38..797a2c752d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -168,6 +168,13 @@ test_expect_success 'git branch -M foo bar should fail when bar is checked out'
test_must_fail git branch -M bar foo
'
+test_expect_success 'git branch -M foo bar should fail when bar is checked out in worktree' '
+ git branch -f bar &&
+ test_when_finished "git worktree remove wt && git branch -D wt" &&
+ git worktree add wt &&
+ test_must_fail git branch -M bar wt
+'
+
test_expect_success 'git branch -M baz bam should succeed when baz is checked out' '
git checkout -b baz &&
git branch bam &&
--
2.33.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v6 1/8] fetch: lowercase error messages
2021-11-13 3:33 ` [PATCH v6 1/8] fetch: lowercase error messages Anders Kaseorg
@ 2021-11-16 5:19 ` Junio C Hamano
2021-11-16 7:10 ` Anders Kaseorg
2021-11-22 1:14 ` Jiang Xin
1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2021-11-16 5:19 UTC (permalink / raw)
To: Anders Kaseorg
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
Anders Kaseorg <andersk@mit.edu> writes:
> Documentation/CodingGuidelines says “do not end error messages with a
> full stop” and “do not capitalize the first word”. Reviewers requested
> updating the existing messages to comply with these guidelines prior to
> the following patches.
Thanks. Whether reviewers requested or you thought of it on your
own, separating such a preliminary clean-up into its own patch would
be a good idea, especially if the later patches need to update (some
of) them.
> @@ -1062,13 +1062,13 @@ static void close_fetch_head(struct fetch_head *fetch_head)
> }
>
> static const char warn_show_forced_updates[] =
> -N_("Fetch normally indicates which branches had a forced update,\n"
> - "but that check has been disabled. To re-enable, use '--show-forced-updates'\n"
> - "flag or run 'git config fetch.showForcedUpdates true'.");
> +N_("fetch normally indicates which branches had a forced update,\n"
> + "but that check has been disabled; to re-enable, use '--show-forced-updates'\n"
> + "flag or run 'git config fetch.showForcedUpdates true'");
> static const char warn_time_show_forced_updates[] =
> -N_("It took %.2f seconds to check forced updates. You can use\n"
> +N_("it took %.2f seconds to check forced updates; you can use\n"
> "'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n"
> - " to avoid this check.\n");
> + " to avoid this check\n");
The two guidelines cited in the proposed log message are primarily
to prefer
fatal: unrecognized argument: --no-such-option
over
fatal: Unrecognized argument: --no-such-option.
and does not say much what to do to a multi-sentence message. In
this part (and other parts) of the patch, I can see that you thought
this one through when preparing this patch. I very much appreciate
it.
The approach chosen (consistently) in this patch is to
(1) turn them into a (semi) single sentence, concatenated with ';'
(2) as a side effect of not being a free-standing sentence anymore,
the second and subsequent sentences in the original, that are
now just pieces in a single sentence separated with ';', do not
get capitalized, and
(3) the sentence as a whole lacks the full-stop, just like a single
sentence message.
I think we are fine with these rules, especially given that these
multi-sentence messages are not the main part of this topic touches
and are not the primary focus of this topic anyway.
I am highlighting this part of the change, just in case others think
of a better set of rules to follow. Existing multi-sentence messages
follow different ad-hoc patterns, it seems (e.g. "git show 00000000").
Thanks.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model
2021-11-13 3:33 ` [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model Anders Kaseorg
@ 2021-11-16 5:39 ` Junio C Hamano
2021-11-22 12:45 ` Johannes Schindelin
1 sibling, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-11-16 5:39 UTC (permalink / raw)
To: Anders Kaseorg
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
Anders Kaseorg <andersk@mit.edu> writes:
> Storing the worktrees list in a static variable meant that
> find_shared_symref() had to rebuild the list on each call (which is
> inefficient when the call site is in a loop), and also that each call
> invalidated the pointer returned by the previous call (which is
> confusing).
>
> Instead, make it the caller’s responsibility to pass in the worktrees
> list and manage its lifetime.
Very nice.
> + struct worktree **worktrees = get_worktrees();
> + const struct worktree *worktree =
> + is_bare_repository() ?
> + NULL :
> + find_shared_symref(worktrees, "HEAD", name);
>
> /* only refs/... are allowed */
> if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
> rp_error("refusing to create funny ref '%s' remotely", name);
> - return "funny refname";
> + ret = "funny refname";
> + goto out;
> }
Nice touch to make the code clean after itself in a single place.
Good.
> ...
> +out:
> + free_worktrees(worktrees);
> + return ret;
> }
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 5/8] fetch: protect branches checked out in all worktrees
2021-11-13 3:33 ` [PATCH v6 5/8] fetch: protect branches checked out in all worktrees Anders Kaseorg
@ 2021-11-16 5:49 ` Junio C Hamano
2021-11-16 6:44 ` Anders Kaseorg
2021-11-22 13:13 ` Johannes Schindelin
1 sibling, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2021-11-16 5:49 UTC (permalink / raw)
To: Anders Kaseorg
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
Anders Kaseorg <andersk@mit.edu> writes:
> Refuse to fetch into the currently checked out branch of any working
> tree, not just the current one.
>
> Fixes this previously reported bug:
>
> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de
>
> As a side effect of using find_shared_symref, we’ll also refuse the
> fetch when we’re on a detached HEAD because we’re rebasing or bisecting
> on the branch in question. This seems like a sensible change.
Indeed.
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
> builtin/fetch.c | 75 +++++++++++++++++++++++--------------------
> t/t5516-fetch-push.sh | 18 +++++++++++
> 2 files changed, 58 insertions(+), 35 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index e5971fa6e5..f373252490 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -28,6 +28,7 @@
> #include "promisor-remote.h"
> #include "commit-graph.h"
> #include "shallow.h"
> +#include "worktree.h"
>
> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>
> @@ -840,14 +841,13 @@ static void format_display(struct strbuf *display, char code,
>
> static int update_local_ref(struct ref *ref,
> struct ref_transaction *transaction,
> - const char *remote,
> - const struct ref *remote_ref,
> - struct strbuf *display,
> - int summary_width)
> + const char *remote, const struct ref *remote_ref,
> + struct strbuf *display, int summary_width,
> + struct worktree **worktrees)
> {
> struct commit *current = NULL, *updated;
> enum object_type type;
> - struct branch *current_branch = branch_get(NULL);
> + const struct worktree *wt;
> const char *pretty_ref = prettify_refname(ref->name);
> int fast_forward = 0;
Having to pass the parameter down to here through the
->do_fetch()
->backfill_tags() (or do_fetch() itself)
->consume_refs()
->store_updated_refs()
->update_local_ref()
callchain makes the "damage to the code" by the patch look larger
than it actually is. The real change is ...
> @@ -862,16 +862,17 @@ static int update_local_ref(struct ref *ref,
> return 0;
> }
>
> - if (current_branch &&
> - !strcmp(ref->name, current_branch->name) &&
> - !(update_head_ok || is_bare_repository()) &&
> - !is_null_oid(&ref->old_oid)) {
> + if (!update_head_ok &&
> + (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
> + !wt->is_bare && !is_null_oid(&ref->old_oid)) {
... this part, which looks very sensible.
> * If this is the head, and it's not okay to update
> * the head, and the old value of the head isn't empty...
> */
> format_display(display, '!', _("[rejected]"),
> - _("can't fetch in current branch"),
> + wt->is_current ?
> + _("can't fetch in current branch") :
> + _("checked out in another worktree"),
> remote, pretty_ref, summary_width);
> return 1;
> }
> @@ -1643,7 +1647,7 @@ static int do_fetch(struct transport *transport,
> "you need to specify exactly one branch with the --set-upstream option"));
> }
> }
> - skip:
> +skip:
> free_refs(ref_map);
;-)
I count 30 hits of "^ [a-z0-9]*:" and 255 hits of "^[a-z0-9]*:" in
our codebase. It must be some developers used to subscribe to
"don't place the label abut the left edge" school but no longer, or
something like that.
The code changes all look good to me.
Thanks.
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 4db8edd9c8..36fb90f4b0 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1770,4 +1770,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '
> git -C cloned push origin HEAD:new-wt &&
> test_must_fail git -C cloned push --delete origin new-wt
> '
> +
> +test_expect_success 'refuse fetch to current branch of worktree' '
> + test_when_finished "git worktree remove --force wt && git branch -D wt" &&
> + git worktree add wt &&
> + test_commit apple &&
> + test_must_fail git fetch . HEAD:wt &&
> + git fetch -u . HEAD:wt
> +'
> +
> +test_expect_success 'refuse fetch to current branch of bare repository worktree' '
> + test_when_finished "rm -fr bare.git" &&
> + git clone --bare . bare.git &&
> + git -C bare.git worktree add wt &&
> + test_commit banana &&
> + test_must_fail git -C bare.git fetch .. HEAD:wt &&
> + git -C bare.git fetch -u .. HEAD:wt
> +'
> +
> test_done
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 6/8] receive-pack: clean dead code from update_worktree()
2021-11-13 3:33 ` [PATCH v6 6/8] receive-pack: clean dead code from update_worktree() Anders Kaseorg
@ 2021-11-16 5:49 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-11-16 5:49 UTC (permalink / raw)
To: Anders Kaseorg
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
Anders Kaseorg <andersk@mit.edu> writes:
> update_worktree() can only be called with a non-NULL worktree parameter,
> because that’s the only case where we set do_update_worktree = 1.
> worktree->path is always initialized to non-NULL.
Yup. I remember seeing this analysed in earlier rounds' reviews.
Nice lossage of lines ;-)
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
> builtin/receive-pack.c | 23 +++++++----------------
> 1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 5d7c4832c1..b04d4ad268 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1444,29 +1444,22 @@ static const char *push_to_checkout(unsigned char *hash,
>
> static const char *update_worktree(unsigned char *sha1, const struct worktree *worktree)
> {
> - const char *retval, *work_tree, *git_dir = NULL;
> + const char *retval, *git_dir;
> struct strvec env = STRVEC_INIT;
>
> - if (worktree && worktree->path)
> - work_tree = worktree->path;
> - else if (git_work_tree_cfg)
> - work_tree = git_work_tree_cfg;
> - else
> - work_tree = "..";
> + if (!worktree || !worktree->path)
> + BUG("worktree->path must be non-NULL");
>
> if (is_bare_repository())
> return "denyCurrentBranch = updateInstead needs a worktree";
> - if (worktree)
> - git_dir = get_worktree_git_dir(worktree);
> - if (!git_dir)
> - git_dir = get_git_dir();
> + git_dir = get_worktree_git_dir(worktree);
>
> strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
>
> if (!find_hook(push_to_checkout_hook))
> - retval = push_to_deploy(sha1, &env, work_tree);
> + retval = push_to_deploy(sha1, &env, worktree->path);
> else
> - retval = push_to_checkout(sha1, &env, work_tree);
> + retval = push_to_checkout(sha1, &env, worktree->path);
>
> strvec_clear(&env);
> return retval;
> @@ -1587,9 +1580,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
> }
>
> if (do_update_worktree) {
> - ret = update_worktree(new_oid->hash,
> - find_shared_symref(worktrees, "HEAD",
> - name));
> + ret = update_worktree(new_oid->hash, worktree);
> if (ret)
> goto out;
> }
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 5/8] fetch: protect branches checked out in all worktrees
2021-11-16 5:49 ` Junio C Hamano
@ 2021-11-16 6:44 ` Anders Kaseorg
0 siblings, 0 replies; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-16 6:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
On 11/15/21 21:49, Junio C Hamano wrote:
>> - skip:
>> +skip:
>
> ;-)
(This was to satisfy ‘make style’. Otherwise it wanted the lines
following the 1-space indent to have a 9-space indent.)
Anders
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 1/8] fetch: lowercase error messages
2021-11-16 5:19 ` Junio C Hamano
@ 2021-11-16 7:10 ` Anders Kaseorg
2021-11-17 8:09 ` Junio C Hamano
0 siblings, 1 reply; 21+ messages in thread
From: Anders Kaseorg @ 2021-11-16 7:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
[-- Attachment #1: Type: text/plain, Size: 4131 bytes --]
On Tue, 16 Nov 2021, Junio C Hamano wrote:
> > Documentation/CodingGuidelines says “do not end error messages with a
> > full stop” and “do not capitalize the first word”. Reviewers requested
> > updating the existing messages to comply with these guidelines prior to
> > the following patches.
>
> Thanks. Whether reviewers requested or you thought of it on your
> own, separating such a preliminary clean-up into its own patch would
> be a good idea, especially if the later patches need to update (some
> of) them.
It was your request; I just mentioned it in case other reviewers wonder
why this belongs in this topic.
https://public-inbox.org/git/xmqq8rxvwp4b.fsf@gitster.g/
> The approach chosen (consistently) in this patch is to
>
> (1) turn them into a (semi) single sentence, concatenated with ';'
>
> (2) as a side effect of not being a free-standing sentence anymore,
> the second and subsequent sentences in the original, that are
> now just pieces in a single sentence separated with ';', do not
> get capitalized, and
>
> (3) the sentence as a whole lacks the full-stop, just like a single
> sentence message.
>
> I think we are fine with these rules, especially given that these
> multi-sentence messages are not the main part of this topic touches
> and are not the primary focus of this topic anyway.
I guess I should add that there are a few messages I left alone:
refuse_unconfigured_deny_msg and
refuse_unconfigured_deny_delete_current_msg in builtin/receive-pack.c
(patch 2/8). Not sure if they count as “error messages”, but these
multi-paragraph messages are too long to comfortably apply this approach.
Now I see that I missed a few others. This could be squashed into the
first three patches:
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f373252490..f3c7c057d0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1783,7 +1783,7 @@ static int fetch_failed_to_start(struct strbuf *out, void *cb, void *task_cb)
struct parallel_fetch_state *state = cb;
const char *remote = task_cb;
- state->result = error(_("Could not fetch %s"), remote);
+ state->result = error(_("could not fetch %s"), remote);
return 0;
}
@@ -1838,7 +1838,7 @@ static int fetch_multiple(struct string_list *list, int max_children)
if (verbosity >= 0)
printf(_("Fetching %s\n"), name);
if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
- error(_("Could not fetch %s"), name);
+ error(_("could not fetch %s"), name);
result = 1;
}
strvec_pop(&argv);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d72058543e..70b4e23a26 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2495,9 +2495,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);
if (argc > 1)
- usage_msg_opt(_("Too many arguments."), receive_pack_usage, options);
+ usage_msg_opt(_("too many arguments"), receive_pack_usage, options);
if (argc == 0)
- usage_msg_opt(_("You must specify a directory."), receive_pack_usage, options);
+ usage_msg_opt(_("you must specify a directory"), receive_pack_usage, options);
service_dir = argv[0];
diff --git a/branch.c b/branch.c
index 3a7d205fa4..0bea1335ae 100644
--- a/branch.c
+++ b/branch.c
@@ -64,7 +64,7 @@ int install_branch_config(int flag, const char *local, const char *origin, const
if (skip_prefix(remote, "refs/heads/", &shortname)
&& !strcmp(local, shortname)
&& !origin) {
- warning(_("Not setting branch %s as its own upstream."),
+ warning(_("not setting branch %s as its own upstream"),
local);
return 0;
}
@@ -116,7 +116,7 @@ int install_branch_config(int flag, const char *local, const char *origin, const
out_err:
strbuf_release(&key);
- error(_("Unable to write upstream branch configuration"));
+ error(_("unable to write upstream branch configuration"));
advise(_(tracking_advice),
origin ? origin : "",
Anders
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v6 1/8] fetch: lowercase error messages
2021-11-16 7:10 ` Anders Kaseorg
@ 2021-11-17 8:09 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-11-17 8:09 UTC (permalink / raw)
To: Anders Kaseorg
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
Anders Kaseorg <andersk@mit.edu> writes:
> On Tue, 16 Nov 2021, Junio C Hamano wrote:
>> > Documentation/CodingGuidelines says “do not end error messages with a
>> > full stop” and “do not capitalize the first word”. Reviewers requested
>> > updating the existing messages to comply with these guidelines prior to
>> > the following patches.
>>
>> Thanks. Whether reviewers requested or you thought of it on your
>> own, separating such a preliminary clean-up into its own patch would
>> be a good idea, especially if the later patches need to update (some
>> of) them.
>
> It was your request; I just mentioned it in case other reviewers wonder
> why this belongs in this topic.
Sorry, let me try again, as I wasn't clear enough.
Readers of "git log", for whom we write our log messages, are not
interested if reviewers suggested, or you came up on your own. The
more relevant thing for them to learn from our log messages is the
reason why that solution was chosen (and the fact that the author is
now committed to the chosen solution---not "this does not make much
sense but I am randomly updating as I was told"). E.g.
... first word". This file has many existing messages that
violate these guidelines. Clean them up in preparation for
subsequent patches that touch some of these messages.
or something like that.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 2/8] receive-pack: lowercase error messages
2021-11-13 3:33 ` [PATCH v6 2/8] receive-pack: " Anders Kaseorg
@ 2021-11-18 4:53 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2021-11-18 4:53 UTC (permalink / raw)
To: Anders Kaseorg
Cc: git, Johannes Schindelin, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
Anders Kaseorg <andersk@mit.edu> writes:
> Documentation/CodingGuidelines says “do not end error messages with a
> full stop” and “do not capitalize the first word”. Reviewers requested
> updating the existing messages to comply with these guidelines prior to
> the following patches.
The same comment applies to this part. Reviewers may make
suggestion to help polish the topic, but it ultimately is the
author's achievement. More importantly, we write our log messages
to help future developers to learn from. The fact somebody asked
some changes made is much less important than the reason why such
changes are desirable, so that they can craft their future topics
following the same pattern.
Documentation... says X and Y. Clean up existing messages, some
of which we will be touching in later steps in the series, that
deviate from these rules in this file, as a preparation for the
main part of the topic.
may convey the intention better, I would think.
The patch text looks good. Thanks.
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
> builtin/receive-pack.c | 6 +++---
> t/t5504-fetch-receive-strict.sh | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 2d1f97e1ca..a82b60f387 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -170,7 +170,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
> strbuf_addf(&fsck_msg_types, "%c%s=%s",
> fsck_msg_types.len ? ',' : '=', var, value);
> else
> - warning("Skipping unknown msg id '%s'", var);
> + warning("skipping unknown msg id '%s'", var);
> return 0;
> }
>
> @@ -1584,9 +1584,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)
> if (!parse_object(the_repository, old_oid)) {
> old_oid = NULL;
> if (ref_exists(name)) {
> - rp_warning("Allowing deletion of corrupt ref.");
> + rp_warning("allowing deletion of corrupt ref");
> } else {
> - rp_warning("Deleting a non-existent ref.");
> + rp_warning("deleting a non-existent ref");
> cmd->did_not_exist = 1;
> }
> }
> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> index 6e5a9c20e7..b0b795aca9 100755
> --- a/t/t5504-fetch-receive-strict.sh
> +++ b/t/t5504-fetch-receive-strict.sh
> @@ -292,7 +292,7 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
> receive.fsck.missingEmail warn &&
> git push --porcelain dst bogus >act 2>&1 &&
> grep "missingEmail" act &&
> - test_i18ngrep "Skipping unknown msg id.*whatever" act &&
> + test_i18ngrep "skipping unknown msg id.*whatever" act &&
> git --git-dir=dst/.git branch -D bogus &&
> git --git-dir=dst/.git config --add \
> receive.fsck.missingEmail ignore &&
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 1/8] fetch: lowercase error messages
2021-11-13 3:33 ` [PATCH v6 1/8] fetch: lowercase error messages Anders Kaseorg
2021-11-16 5:19 ` Junio C Hamano
@ 2021-11-22 1:14 ` Jiang Xin
1 sibling, 0 replies; 21+ messages in thread
From: Jiang Xin @ 2021-11-22 1:14 UTC (permalink / raw)
To: Anders Kaseorg, Jean-Noël Avila
Cc: Junio C Hamano, Git List, Johannes Schindelin, Jeff King,
Andreas Heiduk, Ævar Arnfjörð Bjarmason
On Sat, Nov 13, 2021 at 11:34 AM Anders Kaseorg <andersk@mit.edu> wrote:
> static const char warn_show_forced_updates[] =
> -N_("Fetch normally indicates which branches had a forced update,\n"
> - "but that check has been disabled. To re-enable, use '--show-forced-updates'\n"
> - "flag or run 'git config fetch.showForcedUpdates true'.");
> +N_("fetch normally indicates which branches had a forced update,\n"
> + "but that check has been disabled; to re-enable, use '--show-forced-updates'\n"
> + "flag or run 'git config fetch.showForcedUpdates true'");
> static const char warn_time_show_forced_updates[] =
> -N_("It took %.2f seconds to check forced updates. You can use\n"
> +N_("it took %.2f seconds to check forced updates; you can use\n"
> "'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n"
> - " to avoid this check.\n");
> + " to avoid this check\n");
The leading space character before "to avoid ..." is not necessary.
This will change "po/git.pot" like this:
https://github.com/git-l10n/git-po/blob/pot/seen/2021-11-19.diff#L374-L377
It was introduced in commit 182f59daf0 (l10n: reformat some localized
strings for v2.23.0, 2019-08-06).
--
Jiang Xin
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model
2021-11-13 3:33 ` [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model Anders Kaseorg
2021-11-16 5:39 ` Junio C Hamano
@ 2021-11-22 12:45 ` Johannes Schindelin
1 sibling, 0 replies; 21+ messages in thread
From: Johannes Schindelin @ 2021-11-22 12:45 UTC (permalink / raw)
To: Anders Kaseorg
Cc: Junio C Hamano, git, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
[-- Attachment #1: Type: text/plain, Size: 2893 bytes --]
Hi Anders,
tl;dr this looks great!
On Fri, 12 Nov 2021, Anders Kaseorg wrote:
> Storing the worktrees list in a static variable meant that
> find_shared_symref() had to rebuild the list on each call (which is
> inefficient when the call site is in a loop), and also that each call
> invalidated the pointer returned by the previous call (which is
> confusing).
>
> Instead, make it the caller’s responsibility to pass in the worktrees
> list and manage its lifetime.
Thank you for cleaning this up!
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
> branch.c | 14 ++++++----
> builtin/branch.c | 7 ++++-
> builtin/notes.c | 6 +++-
> builtin/receive-pack.c | 63 +++++++++++++++++++++++++++---------------
> worktree.c | 8 ++----
> worktree.h | 5 ++--
> 6 files changed, 65 insertions(+), 38 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 147827cf46..c7b9ba0e10 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -357,14 +357,16 @@ void remove_branch_state(struct repository *r, int verbose)
>
> void die_if_checked_out(const char *branch, int ignore_current_worktree)
> {
> + struct worktree **worktrees = get_worktrees();
> const struct worktree *wt;
>
> - wt = find_shared_symref("HEAD", branch);
> - if (!wt || (ignore_current_worktree && wt->is_current))
> - return;
> - skip_prefix(branch, "refs/heads/", &branch);
> - die(_("'%s' is already checked out at '%s'"),
> - branch, wt->path);
> + wt = find_shared_symref(worktrees, "HEAD", branch);
> + if (wt && (!ignore_current_worktree || !wt->is_current)) {
> + skip_prefix(branch, "refs/heads/", &branch);
> + die(_("'%s' is already checked out at '%s'"), branch, wt->path);
> + }
> +
> + free_worktrees(worktrees);
This is the only caller that is not in `builtin/`, i.e. it is not at once
clear how many times we would potentially re-generate the list.
I had a quick look:
$ git grep die_if_checked_out
branch.c:void die_if_checked_out(const char *branch, int ignore_current_worktree)
branch.h:void die_if_checked_out(const char *branch, int ignore_current_worktree);
builtin/checkout.c: die_if_checked_out(new_branch_info->path, 1);
builtin/rebase.c: die_if_checked_out(buf.buf, 1);
builtin/worktree.c: die_if_checked_out(symref.buf, 0);
builtin/worktree.c: die_if_checked_out(symref.buf, 0);
This suggests that all the callers of the `die_if_checked_out()` are in
the `builtin/` part, and only in the commands that were not touched
directly by your patch.
Which means that all is fine and dandy, we are unlikely to introduce a
code flow where the worktrees array is populated multiple times (when
before, it would only have been generated only once).
Very good.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 5/8] fetch: protect branches checked out in all worktrees
2021-11-13 3:33 ` [PATCH v6 5/8] fetch: protect branches checked out in all worktrees Anders Kaseorg
2021-11-16 5:49 ` Junio C Hamano
@ 2021-11-22 13:13 ` Johannes Schindelin
1 sibling, 0 replies; 21+ messages in thread
From: Johannes Schindelin @ 2021-11-22 13:13 UTC (permalink / raw)
To: Anders Kaseorg
Cc: Junio C Hamano, git, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
[-- Attachment #1: Type: text/plain, Size: 6900 bytes --]
Hi Anders,
tl;dr the patch looks nice! A few remarks below, to prove that I did not
merely glance over the patch.
On Fri, 12 Nov 2021, Anders Kaseorg wrote:
> Refuse to fetch into the currently checked out branch of any working
> tree, not just the current one.
>
> Fixes this previously reported bug:
>
> https://public-inbox.org/git/cb957174-5e9a-5603-ea9e-ac9b58a2eaad@mathema.de
These days, I think the lore.kernel.org/git/ links are slightly preferred.
>
> As a side effect of using find_shared_symref, we’ll also refuse the
> fetch when we’re on a detached HEAD because we’re rebasing or bisecting
> on the branch in question. This seems like a sensible change.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
> builtin/fetch.c | 75 +++++++++++++++++++++++--------------------
> t/t5516-fetch-push.sh | 18 +++++++++++
> 2 files changed, 58 insertions(+), 35 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index e5971fa6e5..f373252490 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -28,6 +28,7 @@
> #include "promisor-remote.h"
> #include "commit-graph.h"
> #include "shallow.h"
> +#include "worktree.h"
>
> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>
> @@ -840,14 +841,13 @@ static void format_display(struct strbuf *display, char code,
>
> static int update_local_ref(struct ref *ref,
> struct ref_transaction *transaction,
> - const char *remote,
> - const struct ref *remote_ref,
> - struct strbuf *display,
> - int summary_width)
> + const char *remote, const struct ref *remote_ref,
> + struct strbuf *display, int summary_width,
As a reviewer, I prefer to spend my time reviewing the actual change. In
this case, the re-wrapping is a bit distracting. Not really important
here, but maybe for future contributions. The easier a contributor makes
it on reviewers, the better (it definitely helps avoid the grumpy judge
bias[*1*]).
> + struct worktree **worktrees)
> {
> struct commit *current = NULL, *updated;
> enum object_type type;
> - struct branch *current_branch = branch_get(NULL);
> + const struct worktree *wt;
> const char *pretty_ref = prettify_refname(ref->name);
> int fast_forward = 0;
>
> @@ -862,16 +862,17 @@ static int update_local_ref(struct ref *ref,
> return 0;
> }
>
> - if (current_branch &&
> - !strcmp(ref->name, current_branch->name) &&
> - !(update_head_ok || is_bare_repository()) &&
> - !is_null_oid(&ref->old_oid)) {
> + if (!update_head_ok &&
> + (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
> + !wt->is_bare && !is_null_oid(&ref->old_oid)) {
> /*
> * If this is the head, and it's not okay to update
> * the head, and the old value of the head isn't empty...
> */
> format_display(display, '!', _("[rejected]"),
> - _("can't fetch in current branch"),
> + wt->is_current ?
> + _("can't fetch in current branch") :
> + _("checked out in another worktree"),
> remote, pretty_ref, summary_width);
> return 1;
> }
> @@ -1071,7 +1072,8 @@ N_("it took %.2f seconds to check forced updates; you can use\n"
> " to avoid this check\n");
>
> static int store_updated_refs(const char *raw_url, const char *remote_name,
> - int connectivity_checked, struct ref *ref_map)
> + int connectivity_checked, struct ref *ref_map,
> + struct worktree **worktrees)
> {
> struct fetch_head fetch_head;
> struct commit *commit;
> @@ -1188,7 +1190,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
> strbuf_reset(¬e);
> if (ref) {
> rc |= update_local_ref(ref, transaction, what,
> - rm, ¬e, summary_width);
> + rm, ¬e, summary_width,
> + worktrees);
> free(ref);
> } else if (write_fetch_head || dry_run) {
> /*
> @@ -1301,16 +1304,15 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
> }
>
> /* Update local refs based on the ref values fetched from a remote */
> -static int consume_refs(struct transport *transport, struct ref *ref_map)
> +static int consume_refs(struct transport *transport, struct ref *ref_map,
> + struct worktree **worktrees)
> {
> int connectivity_checked = transport->smart_options
> ? transport->smart_options->connectivity_checked : 0;
> int ret;
> trace2_region_enter("fetch", "consume_refs", the_repository);
> - ret = store_updated_refs(transport->url,
> - transport->remote->name,
> - connectivity_checked,
> - ref_map);
> + ret = store_updated_refs(transport->url, transport->remote->name,
> + connectivity_checked, ref_map, worktrees);
Again, this rewrapping is slightly distracting to my brain.
> transport_unlock_pack(transport);
> trace2_region_leave("fetch", "consume_refs", the_repository);
> return ret;
> @@ -1643,7 +1647,7 @@ static int do_fetch(struct transport *transport,
> "you need to specify exactly one branch with the --set-upstream option"));
> }
> }
> - skip:
> +skip:
Okay, this one is distracting _but also_ pleasing. I am only pointing this
out to prove that I did not go over your patch sloppily. :-)
> @@ -1653,11 +1657,12 @@ static int do_fetch(struct transport *transport,
> ref_map = NULL;
> find_non_local_tags(remote_refs, &ref_map, &tail);
> if (ref_map)
> - backfill_tags(transport, ref_map);
> + backfill_tags(transport, ref_map, worktrees);
> free_refs(ref_map);
> }
>
> - cleanup:
> +cleanup:
Same here.
> + free_worktrees(worktrees);
> return retcode;
> }
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 4db8edd9c8..36fb90f4b0 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1770,4 +1770,22 @@ test_expect_success 'denyCurrentBranch and worktrees' '
> git -C cloned push origin HEAD:new-wt &&
> test_must_fail git -C cloned push --delete origin new-wt
> '
> +
> +test_expect_success 'refuse fetch to current branch of worktree' '
> + test_when_finished "git worktree remove --force wt && git branch -D wt" &&
> + git worktree add wt &&
> + test_commit apple &&
> + test_must_fail git fetch . HEAD:wt &&
> + git fetch -u . HEAD:wt
> +'
> +
> +test_expect_success 'refuse fetch to current branch of bare repository worktree' '
> + test_when_finished "rm -fr bare.git" &&
> + git clone --bare . bare.git &&
> + git -C bare.git worktree add wt &&
> + test_commit banana &&
> + test_must_fail git -C bare.git fetch .. HEAD:wt &&
> + git -C bare.git fetch -u .. HEAD:wt
> +'
> +
> test_done
Reads nicely.
Thanks,
Dscho
Footnote: https://en.wikipedia.org/wiki/Hungry_judge_effect
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 0/8] protect branches checked out in all worktrees
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
` (7 preceding siblings ...)
2021-11-13 3:33 ` [PATCH v6 8/8] branch: protect branches checked out in all worktrees Anders Kaseorg
@ 2021-11-22 13:21 ` Johannes Schindelin
8 siblings, 0 replies; 21+ messages in thread
From: Johannes Schindelin @ 2021-11-22 13:21 UTC (permalink / raw)
To: Anders Kaseorg
Cc: Junio C Hamano, git, Jeff King, Andreas Heiduk,
Ævar Arnfjörð Bjarmason
[-- Attachment #1: Type: text/plain, Size: 650 bytes --]
Hi Anders,
On Fri, 12 Nov 2021, Anders Kaseorg wrote:
> ‘git fetch’ (without ‘--update-head-ok’), ‘git receive-pack’, and ‘git
> branch -M’ protect the currently checked out branch from being
> accidentally updated. However, the code for these checks predates
> ‘git worktree’. Improve it to protect branches checked out in all
> worktrees, not just the current one.
I read through these patches, and in particular the last three were a real
delight to read, a highlight of my catching-up with the mailing list
today.
I am very much in favor of these patches to advance to `next` as-are.
Thank you,
Dscho
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-11-22 13:21 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-13 3:33 [PATCH v6 0/8] protect branches checked out in all worktrees Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 1/8] fetch: lowercase error messages Anders Kaseorg
2021-11-16 5:19 ` Junio C Hamano
2021-11-16 7:10 ` Anders Kaseorg
2021-11-17 8:09 ` Junio C Hamano
2021-11-22 1:14 ` Jiang Xin
2021-11-13 3:33 ` [PATCH v6 2/8] receive-pack: " Anders Kaseorg
2021-11-18 4:53 ` Junio C Hamano
2021-11-13 3:33 ` [PATCH v6 3/8] branch: " Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 4/8] worktree: simplify find_shared_symref() memory ownership model Anders Kaseorg
2021-11-16 5:39 ` Junio C Hamano
2021-11-22 12:45 ` Johannes Schindelin
2021-11-13 3:33 ` [PATCH v6 5/8] fetch: protect branches checked out in all worktrees Anders Kaseorg
2021-11-16 5:49 ` Junio C Hamano
2021-11-16 6:44 ` Anders Kaseorg
2021-11-22 13:13 ` Johannes Schindelin
2021-11-13 3:33 ` [PATCH v6 6/8] receive-pack: clean dead code from update_worktree() Anders Kaseorg
2021-11-16 5:49 ` Junio C Hamano
2021-11-13 3:33 ` [PATCH v6 7/8] receive-pack: protect current branch for bare repository worktree Anders Kaseorg
2021-11-13 3:33 ` [PATCH v6 8/8] branch: protect branches checked out in all worktrees Anders Kaseorg
2021-11-22 13:21 ` [PATCH v6 0/8] " Johannes Schindelin
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).