* [PATCH v4 07/19] repack: fix leaks on error with "goto cleanup"
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com>
Change cmd_repack() to "goto cleanup" rather than "return ret" on
error, when we returned we'd potentially skip cleaning up the
string_lists and other data we'd allocated in this function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/repack.c | 13 +++++++------
t/t6011-rev-list-with-bad-commit.sh | 1 +
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index c1402ad038f..f6493795318 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -948,7 +948,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
ret = start_command(&cmd);
if (ret)
- return ret;
+ goto cleanup;
if (geometry) {
FILE *in = xfdopen(cmd.in, "w");
@@ -977,7 +977,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
fclose(out);
ret = finish_command(&cmd);
if (ret)
- return ret;
+ goto cleanup;
if (!names.nr && !po_args.quiet)
printf_ln(_("Nothing new to pack."));
@@ -1007,7 +1007,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
&existing_nonkept_packs,
&existing_kept_packs);
if (ret)
- return ret;
+ goto cleanup;
if (delete_redundant && expire_to) {
/*
@@ -1039,7 +1039,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
&existing_nonkept_packs,
&existing_kept_packs);
if (ret)
- return ret;
+ goto cleanup;
}
}
@@ -1115,7 +1115,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
string_list_clear(&include, 0);
if (ret)
- return ret;
+ goto cleanup;
}
reprepare_packed_git(the_repository);
@@ -1172,10 +1172,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
write_midx_file(get_object_directory(), NULL, NULL, flags);
}
+cleanup:
string_list_clear(&names, 1);
string_list_clear(&existing_nonkept_packs, 0);
string_list_clear(&existing_kept_packs, 0);
clear_pack_geometry(geometry);
- return 0;
+ return ret;
}
diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh
index bad02cf5b83..b2e422cf0f7 100755
--- a/t/t6011-rev-list-with-bad-commit.sh
+++ b/t/t6011-rev-list-with-bad-commit.sh
@@ -2,6 +2,7 @@
test_description='git rev-list should notice bad commits'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# Note:
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v4 06/19] name-rev: don't xstrdup() an already dup'd string
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com>
When "add_to_tip_table()" is called with a non-zero
"shorten_unambiguous" we always return an xstrdup()'d string, which
we'd then xstrdup() again, leaking memory. See [1] and [2] for how
this leak came about.
We could xstrdup() only if "shorten_unambiguous" wasn't true, but
let's instead inline this code, so that information on whether we need
to xstrdup() is contained within add_to_tip_table().
1. 98c5c4ad015 (name-rev: allow to specify a subpath for --refs
option, 2013-06-18)
2. b23e0b9353e (name-rev: allow converting the exact object name at
the tip of a ref, 2013-07-07)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/name-rev.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 15535e914a6..49fae523694 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -273,17 +273,6 @@ static int subpath_matches(const char *path, const char *filter)
return -1;
}
-static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
-{
- if (shorten_unambiguous)
- refname = shorten_unambiguous_ref(refname, 0);
- else if (skip_prefix(refname, "refs/heads/", &refname))
- ; /* refname already advanced */
- else
- skip_prefix(refname, "refs/", &refname);
- return refname;
-}
-
struct name_ref_data {
int tags_only;
int name_only;
@@ -309,11 +298,19 @@ static void add_to_tip_table(const struct object_id *oid, const char *refname,
int shorten_unambiguous, struct commit *commit,
timestamp_t taggerdate, int from_tag, int deref)
{
- refname = name_ref_abbrev(refname, shorten_unambiguous);
+ char *short_refname = NULL;
+
+ if (shorten_unambiguous)
+ short_refname = shorten_unambiguous_ref(refname, 0);
+ else if (skip_prefix(refname, "refs/heads/", &refname))
+ ; /* refname already advanced */
+ else
+ skip_prefix(refname, "refs/", &refname);
ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc);
oidcpy(&tip_table.table[tip_table.nr].oid, oid);
- tip_table.table[tip_table.nr].refname = xstrdup(refname);
+ tip_table.table[tip_table.nr].refname = short_refname ?
+ short_refname : xstrdup(refname);
tip_table.table[tip_table.nr].commit = commit;
tip_table.table[tip_table.nr].taggerdate = taggerdate;
tip_table.table[tip_table.nr].from_tag = from_tag;
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v4 05/19] various: add missing clear_pathspec(), fix leaks
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com>
Fix memory leaks resulting from a missing clear_pathspec().
- archive.c: Plug a leak in the "struct archiver_args", and
clear_pathspec() the "pathspec" member that the "parse_pathspec_arg()"
call in this function populates.
- builtin/clean.c: Fix a memory leak that's been with us since
893d839970c (clean: convert to use parse_pathspec, 2013-07-14).
- builtin/reset.c: Add clear_pathspec() calls to cmd_reset(),
including to the codepaths where we'd return early.
- builtin/stash.c: Call clear_pathspec() on the pathspec initialized
in push_stash().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
archive.c | 1 +
builtin/clean.c | 1 +
builtin/reset.c | 11 ++++++++---
builtin/stash.c | 7 +++++--
t/t5001-archive-attr.sh | 1 +
t/t5004-archive-corner-cases.sh | 2 ++
t/t7105-reset-patch.sh | 2 ++
t/t7106-reset-unborn-branch.sh | 2 ++
t/t7107-reset-pathspec-file.sh | 1 +
t/t7301-clean-interactive.sh | 1 +
10 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/archive.c b/archive.c
index 941495f5d78..a2d813e50db 100644
--- a/archive.c
+++ b/archive.c
@@ -710,6 +710,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
string_list_clear_func(&args.extra_files, extra_file_info_clear);
free(args.refname);
+ clear_pathspec(&args.pathspec);
return rc;
}
diff --git a/builtin/clean.c b/builtin/clean.c
index b2701a28158..b15eab328b7 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -1092,5 +1092,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
strbuf_release(&buf);
string_list_clear(&del_list, 0);
string_list_clear(&exclude_list, 0);
+ clear_pathspec(&pathspec);
return (errors != 0);
}
diff --git a/builtin/reset.c b/builtin/reset.c
index fea20a9ba0b..e9c10618cd3 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -390,7 +390,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type != NONE)
die(_("options '%s' and '%s' cannot be used together"), "--patch", "--{hard,mixed,soft}");
trace2_cmd_mode("patch-interactive");
- return run_add_interactive(rev, "--patch=reset", &pathspec);
+ update_ref_status = run_add_interactive(rev, "--patch=reset", &pathspec);
+ goto cleanup;
}
/* git reset tree [--] paths... can be used to
@@ -439,8 +440,10 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
LOCK_DIE_ON_ERROR);
if (reset_type == MIXED) {
int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
- if (read_from_tree(&pathspec, &oid, intent_to_add))
- return 1;
+ if (read_from_tree(&pathspec, &oid, intent_to_add)) {
+ update_ref_status = 1;
+ goto cleanup;
+ }
the_index.updated_skipworktree = 1;
if (!no_refresh && get_git_work_tree()) {
uint64_t t_begin, t_delta_in_ms;
@@ -488,5 +491,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
discard_index(&the_index);
+cleanup:
+ clear_pathspec(&pathspec);
return update_ref_status;
}
diff --git a/builtin/stash.c b/builtin/stash.c
index bb0fd861434..45bffdf54bb 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1727,6 +1727,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul),
OPT_END()
};
+ int ret;
if (argc) {
force_assume = !strcmp(argv[0], "-p");
@@ -1766,8 +1767,10 @@ static int push_stash(int argc, const char **argv, const char *prefix,
die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file");
}
- return do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
- include_untracked, only_staged);
+ ret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
+ include_untracked, only_staged);
+ clear_pathspec(&ps);
+ return ret;
}
static int push_stash_unassumed(int argc, const char **argv, const char *prefix)
diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 2f6eef5e372..04d300eeda7 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh
@@ -3,6 +3,7 @@
test_description='git archive attribute tests'
TEST_CREATE_REPO_NO_TEMPLATE=1
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
SUBSTFORMAT='%H (%h)%n'
diff --git a/t/t5004-archive-corner-cases.sh b/t/t5004-archive-corner-cases.sh
index ae508e21623..9f2c6da80e8 100755
--- a/t/t5004-archive-corner-cases.sh
+++ b/t/t5004-archive-corner-cases.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='test corner cases of git-archive'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# the 10knuls.tar file is used to test for an empty git generated tar
diff --git a/t/t7105-reset-patch.sh b/t/t7105-reset-patch.sh
index fc2a6cf5c7a..9b46da7aaa7 100755
--- a/t/t7105-reset-patch.sh
+++ b/t/t7105-reset-patch.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='git reset --patch'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./lib-patch-mode.sh
test_expect_success PERL 'setup' '
diff --git a/t/t7106-reset-unborn-branch.sh b/t/t7106-reset-unborn-branch.sh
index ecb85c3b823..a0b67a0b843 100755
--- a/t/t7106-reset-unborn-branch.sh
+++ b/t/t7106-reset-unborn-branch.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='git reset should work on unborn branch'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t7107-reset-pathspec-file.sh b/t/t7107-reset-pathspec-file.sh
index 523efbecde1..af5ea406db3 100755
--- a/t/t7107-reset-pathspec-file.sh
+++ b/t/t7107-reset-pathspec-file.sh
@@ -2,6 +2,7 @@
test_description='reset --pathspec-from-file'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_tick
diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
index a07e8b86de2..d82a3210a1d 100755
--- a/t/t7301-clean-interactive.sh
+++ b/t/t7301-clean-interactive.sh
@@ -2,6 +2,7 @@
test_description='git clean -i basic tests'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
. "$TEST_DIRECTORY"/lib-terminal.sh
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v4 02/19] bundle.c: don't leak the "args" in the "struct child_process"
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com>
Fix a leak that's been here since 7366096de9d (bundle API: change
"flags" to be "extra_index_pack_args", 2021-09-05), if can't verify
the bundle we didn't call child_process_clear() to clear the "args".
But rather than doing that let's verify the bundle before we start
preparing the process we're going to spawn, if we get an error we
don't need to push anything to the "args".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
bundle.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/bundle.c b/bundle.c
index 4ef7256aa11..9ebb10a8f72 100644
--- a/bundle.c
+++ b/bundle.c
@@ -627,6 +627,10 @@ int unbundle(struct repository *r, struct bundle_header *header,
enum verify_bundle_flags flags)
{
struct child_process ip = CHILD_PROCESS_INIT;
+
+ if (verify_bundle(r, header, flags))
+ return -1;
+
strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL);
/* If there is a filter, then we need to create the promisor pack. */
@@ -638,8 +642,6 @@ int unbundle(struct repository *r, struct bundle_header *header,
strvec_clear(extra_index_pack_args);
}
- if (verify_bundle(r, header, flags))
- return -1;
ip.in = bundle_fd;
ip.no_stdout = 1;
ip.git_cmd = 1;
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v4 01/19] tests: mark tests as passing with SANITIZE=leak
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com>
When the "ab/various-leak-fixes" topic was merged in [1] only t6021
would fail if the tests were run in the
"GIT_TEST_PASSING_SANITIZE_LEAK=check" mode, i.e. to check whether we
marked all leak-free tests with "TEST_PASSES_SANITIZE_LEAK=true".
Since then we've had various tests starting to pass under
SANITIZE=leak. Let's mark those as passing, this is when they started
to pass, narrowed down with "git bisect":
- t5317-pack-objects-filter-objects.sh: In
faebba436e6 (list-objects-filter: plug pattern_list leak, 2022-12-01).
- t3210-pack-refs.sh, t5613-info-alternate.sh,
t7403-submodule-sync.sh: In 189e97bc4ba (diff: remove parseopts member
from struct diff_options, 2022-12-01).
- t1408-packed-refs.sh: In ab91f6b7c42 (Merge branch
'rs/diff-parseopts', 2022-12-19).
- t0023-crlf-am.sh, t4152-am-subjects.sh, t4254-am-corrupt.sh,
t4256-am-format-flowed.sh, t4257-am-interactive.sh,
t5403-post-checkout-hook.sh: In a658e881c13 (am: don't pass strvec to
apply_parse_options(), 2022-12-13)
- t1301-shared-repo.sh, t1302-repo-version.sh: In b07a819c05f (reflog:
clear leftovers in reflog_expiry_cleanup(), 2022-12-13).
- t1304-default-acl.sh, t1410-reflog.sh,
t5330-no-lazy-fetch-with-commit-graph.sh, t5502-quickfetch.sh,
t5604-clone-reference.sh, t6014-rev-list-all.sh,
t7701-repack-unpack-unreachable.sh: In b0c61be3209 (Merge branch
'rs/reflog-expiry-cleanup', 2022-12-26)
1. 9ea1378d046 (Merge branch 'ab/various-leak-fixes', 2022-12-14)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t0023-crlf-am.sh | 1 +
t/t1301-shared-repo.sh | 1 +
t/t1302-repo-version.sh | 1 +
t/t1304-default-acl.sh | 1 +
t/t1408-packed-refs.sh | 1 +
t/t1410-reflog.sh | 1 +
t/t3210-pack-refs.sh | 1 +
t/t4152-am-subjects.sh | 2 ++
t/t4254-am-corrupt.sh | 2 ++
t/t4256-am-format-flowed.sh | 1 +
t/t4257-am-interactive.sh | 2 ++
t/t5317-pack-objects-filter-objects.sh | 1 +
t/t5330-no-lazy-fetch-with-commit-graph.sh | 1 +
t/t5403-post-checkout-hook.sh | 1 +
t/t5502-quickfetch.sh | 1 +
t/t5604-clone-reference.sh | 1 +
t/t5613-info-alternate.sh | 2 ++
t/t6014-rev-list-all.sh | 1 +
t/t6021-rev-list-exclude-hidden.sh | 1 +
t/t7403-submodule-sync.sh | 1 +
t/t7701-repack-unpack-unreachable.sh | 1 +
21 files changed, 25 insertions(+)
diff --git a/t/t0023-crlf-am.sh b/t/t0023-crlf-am.sh
index f9bbb91f64e..575805513a3 100755
--- a/t/t0023-crlf-am.sh
+++ b/t/t0023-crlf-am.sh
@@ -2,6 +2,7 @@
test_description='Test am with auto.crlf'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
cat >patchfile <<\EOF
diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 93a2f91f8a5..a1251f65100 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -8,6 +8,7 @@ test_description='Test shared repository initialization'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# Remove a default ACL from the test dir if possible.
diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh
index 7cf80bf66a6..70389fa2ebb 100755
--- a/t/t1302-repo-version.sh
+++ b/t/t1302-repo-version.sh
@@ -5,6 +5,7 @@
test_description='Test repository version check'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t1304-default-acl.sh b/t/t1304-default-acl.sh
index c69ae41306c..31b89dd9693 100755
--- a/t/t1304-default-acl.sh
+++ b/t/t1304-default-acl.sh
@@ -9,6 +9,7 @@ test_description='Test repository with default ACL'
# => this must come before . ./test-lib.sh
umask 077
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# We need an arbitrary other user give permission to using ACLs. root
diff --git a/t/t1408-packed-refs.sh b/t/t1408-packed-refs.sh
index 41ba1f1d7fc..9469c79a585 100755
--- a/t/t1408-packed-refs.sh
+++ b/t/t1408-packed-refs.sh
@@ -5,6 +5,7 @@ test_description='packed-refs entries are covered by loose refs'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index aa59954f6c5..6c45965b1e4 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -7,6 +7,7 @@ test_description='Test prune and reflog expiration'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
check_have () {
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index 577f32dc71f..07a0ff93def 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -12,6 +12,7 @@ semantic is still the same.
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'enable reflogs' '
diff --git a/t/t4152-am-subjects.sh b/t/t4152-am-subjects.sh
index 4c68245acad..9f2edba1f83 100755
--- a/t/t4152-am-subjects.sh
+++ b/t/t4152-am-subjects.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='test subject preservation with format-patch | am'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
make_patches() {
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 54be7da1611..45f1d4f95e5 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='git am with corrupt input'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
make_mbox_with_nul () {
diff --git a/t/t4256-am-format-flowed.sh b/t/t4256-am-format-flowed.sh
index 2369c4e17ad..1015273bc82 100755
--- a/t/t4256-am-format-flowed.sh
+++ b/t/t4256-am-format-flowed.sh
@@ -2,6 +2,7 @@
test_description='test format=flowed support of git am'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t4257-am-interactive.sh b/t/t4257-am-interactive.sh
index aed8f4de3d6..f26d7fd2dbd 100755
--- a/t/t4257-am-interactive.sh
+++ b/t/t4257-am-interactive.sh
@@ -1,6 +1,8 @@
#!/bin/sh
test_description='am --interactive tests'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'set up patches to apply' '
diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
index 5b707d911b5..b26d476c646 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -5,6 +5,7 @@ test_description='git pack-objects using object filtering'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
# Test blob:none filter.
diff --git a/t/t5330-no-lazy-fetch-with-commit-graph.sh b/t/t5330-no-lazy-fetch-with-commit-graph.sh
index 2cc7fd7a476..5eb28f0512d 100755
--- a/t/t5330-no-lazy-fetch-with-commit-graph.sh
+++ b/t/t5330-no-lazy-fetch-with-commit-graph.sh
@@ -2,6 +2,7 @@
test_description='test for no lazy fetch with the commit-graph'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup: prepare a repository with a commit' '
diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh
index 978f240cdac..cfaae547398 100755
--- a/t/t5403-post-checkout-hook.sh
+++ b/t/t5403-post-checkout-hook.sh
@@ -7,6 +7,7 @@ test_description='Test the post-checkout hook.'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t5502-quickfetch.sh b/t/t5502-quickfetch.sh
index b160f8b7fb7..7b3ff21b984 100755
--- a/t/t5502-quickfetch.sh
+++ b/t/t5502-quickfetch.sh
@@ -5,6 +5,7 @@ test_description='test quickfetch from local'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index 2734e37e880..dc86dea1333 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -7,6 +7,7 @@ test_description='test clone --reference'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
base_dir=$(pwd)
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 895f46bb911..7708cbafa98 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -4,6 +4,8 @@
#
test_description='test transitive info/alternate entries'
+
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'preparing first repository' '
diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh
index c9bedd29cba..16b8bd1d090 100755
--- a/t/t6014-rev-list-all.sh
+++ b/t/t6014-rev-list-all.sh
@@ -2,6 +2,7 @@
test_description='--all includes detached HEADs'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
diff --git a/t/t6021-rev-list-exclude-hidden.sh b/t/t6021-rev-list-exclude-hidden.sh
index 32b2b094138..11c50b7c0dd 100755
--- a/t/t6021-rev-list-exclude-hidden.sh
+++ b/t/t6021-rev-list-exclude-hidden.sh
@@ -2,6 +2,7 @@
test_description='git rev-list --exclude-hidden test'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success 'setup' '
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index ea92ef52a5e..ff09443a0a4 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -11,6 +11,7 @@ These tests exercise the "git submodule sync" subcommand.
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
test_expect_success setup '
diff --git a/t/t7701-repack-unpack-unreachable.sh b/t/t7701-repack-unpack-unreachable.sh
index b7ac4f598a8..ebb267855fe 100755
--- a/t/t7701-repack-unpack-unreachable.sh
+++ b/t/t7701-repack-unpack-unreachable.sh
@@ -5,6 +5,7 @@ test_description='git repack works correctly'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
fsha1=
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v4 04/19] clone: use free() instead of UNLEAK()
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com>
Change an UNLEAK() added in 0c4542738e6 (clone: free or UNLEAK further
pointers when finished, 2021-03-14) to use a "to_free" pattern
instead. In this case the "repo" can be either this absolute_pathdup()
value, or in the "else if" branch seen in the context the the
"argv[0]" argument to "main()".
We can only free() the value in the former case, hence the "to_free"
pattern.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/clone.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 5453ba5277f..ba82f5e4108 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -892,6 +892,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
int is_bundle = 0, is_local;
int reject_shallow = 0;
const char *repo_name, *repo, *work_tree, *git_dir;
+ char *repo_to_free = NULL;
char *path = NULL, *dir, *display_repo = NULL;
int dest_exists, real_dest_exists = 0;
const struct ref *refs, *remote_head;
@@ -949,7 +950,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
path = get_repo_path(repo_name, &is_bundle);
if (path) {
FREE_AND_NULL(path);
- repo = absolute_pathdup(repo_name);
+ repo = repo_to_free = absolute_pathdup(repo_name);
} else if (strchr(repo_name, ':')) {
repo = repo_name;
display_repo = transport_anonymize_url(repo);
@@ -1413,7 +1414,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
free(unborn_head);
free(dir);
free(path);
- UNLEAK(repo);
+ free(repo_to_free);
junk_mode = JUNK_LEAVE_ALL;
transport_ls_refs_options_release(&transport_ls_refs_options);
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v4 03/19] commit-graph: use free_commit_graph() instead of UNLEAK()
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com>
In 0bfb48e6723 (builtin/commit-graph.c: UNLEAK variables, 2018-10-03)
this was made to UNLEAK(), but we can just as easily invoke the
free_commit_graph() function added in c3756d5b7fc (commit-graph: add
free_commit_graph, 2018-07-11) instead.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/commit-graph.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index e8f77f535f3..0102ac8540e 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -67,6 +67,7 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
int fd;
struct stat st;
int flags = 0;
+ int ret;
static struct option builtin_commit_graph_verify_options[] = {
OPT_BOOL(0, "shallow", &opts.shallow,
@@ -111,8 +112,9 @@ static int graph_verify(int argc, const char **argv, const char *prefix)
if (!graph)
return !!open_ok;
- UNLEAK(graph);
- return verify_commit_graph(the_repository, graph, flags);
+ ret = verify_commit_graph(the_repository, graph, flags);
+ free_commit_graph(graph);
+ return ret;
}
extern int read_replace_refs;
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* [PATCH v4 00/19] leak fixes: various simple leak fixes
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 17:11 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v3-00.19-00000000000-20230110T054138Z-avarab@gmail.com>
See
https://lore.kernel.org/git/cover-v3-00.19-00000000000-20230110T054138Z-avarab@gmail.com/
for the v3. Change since then:
* Reword the subject of a commit message that no longer matched its
content/body (thanks René).
* Re-ran "GIT_TEST_PASSING_SANITIZE_LEAK=check
GIT_TEST_SANITIZE_LEAK_LOG=true make SANITIZE=leak test" with "git
rebase -i -x", so and updated the leak markings for other fixes
that have landed on "master" since they were last updated. The tip
of this (and all intermediate commits) pass the "check" mode.
Ævar Arnfjörð Bjarmason (19):
tests: mark tests as passing with SANITIZE=leak
bundle.c: don't leak the "args" in the "struct child_process"
commit-graph: use free_commit_graph() instead of UNLEAK()
clone: use free() instead of UNLEAK()
various: add missing clear_pathspec(), fix leaks
name-rev: don't xstrdup() an already dup'd string
repack: fix leaks on error with "goto cleanup"
worktree: fix a trivial leak in prune_worktrees()
http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main()
http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()
commit-graph: fix a parse_options_concat() leak
show-branch: free() allocated "head" before return
builtin/merge.c: use fixed strings, not "strbuf", fix leak
builtin/merge.c: free "&buf" on "Your local changes..." error
object-file.c: release the "tag" in check_tag()
grep.c: refactor free_grep_patterns()
grep API: plug memory leaks by freeing "header_list"
receive-pack: free() the "ref_name" in "struct command"
push: free_refs() the "local_refs" in set_refspecs()
archive.c | 1 +
builtin/clean.c | 1 +
builtin/clone.c | 5 +++--
builtin/commit-graph.c | 10 ++++++----
builtin/merge.c | 14 ++++++-------
builtin/name-rev.c | 23 ++++++++++------------
builtin/push.c | 1 +
builtin/receive-pack.c | 10 ++++++++++
builtin/repack.c | 13 ++++++------
builtin/reset.c | 11 ++++++++---
builtin/show-branch.c | 1 +
builtin/stash.c | 7 +++++--
builtin/worktree.c | 6 +++---
bundle.c | 6 ++++--
grep.c | 15 +++++++++-----
http-backend.c | 9 +++++++--
object-file.c | 1 +
t/t0023-crlf-am.sh | 1 +
t/t1301-shared-repo.sh | 1 +
t/t1302-repo-version.sh | 1 +
t/t1304-default-acl.sh | 1 +
t/t1408-packed-refs.sh | 1 +
t/t1410-reflog.sh | 1 +
t/t1416-ref-transaction-hooks.sh | 1 +
t/t2401-worktree-prune.sh | 1 +
t/t2402-worktree-list.sh | 1 +
t/t2406-worktree-repair.sh | 1 +
t/t3203-branch-output.sh | 2 ++
t/t3210-pack-refs.sh | 1 +
t/t3800-mktag.sh | 1 +
t/t4152-am-subjects.sh | 2 ++
t/t4254-am-corrupt.sh | 2 ++
t/t4256-am-format-flowed.sh | 1 +
t/t4257-am-interactive.sh | 2 ++
t/t5001-archive-attr.sh | 1 +
t/t5004-archive-corner-cases.sh | 2 ++
t/t5302-pack-index.sh | 2 ++
t/t5317-pack-objects-filter-objects.sh | 1 +
t/t5330-no-lazy-fetch-with-commit-graph.sh | 1 +
t/t5403-post-checkout-hook.sh | 1 +
t/t5405-send-pack-rewind.sh | 1 +
t/t5406-remote-rejects.sh | 1 +
t/t5502-quickfetch.sh | 1 +
t/t5504-fetch-receive-strict.sh | 1 +
t/t5507-remote-environment.sh | 2 ++
t/t5522-pull-symlink.sh | 1 +
t/t5523-push-upstream.sh | 1 +
t/t5527-fetch-odd-refs.sh | 1 +
t/t5529-push-errors.sh | 2 ++
t/t5546-receive-limits.sh | 2 ++
t/t5547-push-quarantine.sh | 2 ++
t/t5560-http-backend-noserver.sh | 1 +
t/t5561-http-backend.sh | 1 +
t/t5562-http-backend-content-length.sh | 2 ++
t/t5604-clone-reference.sh | 1 +
t/t5606-clone-options.sh | 1 +
t/t5613-info-alternate.sh | 2 ++
t/t5705-session-id-in-capabilities.sh | 1 +
t/t5810-proto-disable-local.sh | 2 ++
t/t5813-proto-disable-ssh.sh | 2 ++
t/t6011-rev-list-with-bad-commit.sh | 1 +
t/t6014-rev-list-all.sh | 1 +
t/t6021-rev-list-exclude-hidden.sh | 1 +
t/t6439-merge-co-error-msgs.sh | 1 +
t/t7105-reset-patch.sh | 2 ++
t/t7106-reset-unborn-branch.sh | 2 ++
t/t7107-reset-pathspec-file.sh | 1 +
t/t7301-clean-interactive.sh | 1 +
t/t7403-submodule-sync.sh | 1 +
t/t7409-submodule-detached-work-tree.sh | 1 +
t/t7416-submodule-dash-url.sh | 2 ++
t/t7450-bad-git-dotfiles.sh | 2 ++
t/t7701-repack-unpack-unreachable.sh | 1 +
73 files changed, 158 insertions(+), 50 deletions(-)
Range-diff against v3:
1: f5b67f44e2d = 1: 2ed69e3cda3 tests: mark tests as passing with SANITIZE=leak
2: 88c6b66be3c = 2: 9993786ba0d bundle.c: don't leak the "args" in the "struct child_process"
3: 8cc8060cd92 = 3: 8e98d7c4ebf commit-graph: use free_commit_graph() instead of UNLEAK()
4: 765d5cbcf81 = 4: 966d7657d54 clone: use free() instead of UNLEAK()
5: 5087fb73286 = 5: 93a8f8fa1b9 various: add missing clear_pathspec(), fix leaks
6: 39cb8aefb58 = 6: bd15d991ac7 name-rev: don't xstrdup() an already dup'd string
7: a3f1e800127 = 7: fd890121ebe repack: fix leaks on error with "goto cleanup"
8: f918a6f2adc ! 8: 1fe25bc6981 worktree: fix a trivial leak in prune_worktrees()
@@ t/t2406-worktree-repair.sh
. ./test-lib.sh
test_expect_success setup '
+
+ ## t/t3203-branch-output.sh ##
+@@
+ #!/bin/sh
+
+ test_description='git branch display tests'
++
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+ . "$TEST_DIRECTORY"/lib-terminal.sh
+
9: 56204806dfd = 9: 6b3dd9b15f0 http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main()
10: 5355e0fc60b = 10: 246f71bb447 http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()
11: dfb52dbd1c4 = 11: ab31d8d10da commit-graph: fix a parse_options_concat() leak
12: e44e74dcc58 = 12: 9054b353220 show-branch: free() allocated "head" before return
13: 6d99fdcc44e ! 13: 05836b08e0f builtin/merge.c: always free "struct strbuf msg"
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- builtin/merge.c: always free "struct strbuf msg"
+ builtin/merge.c: use fixed strings, not "strbuf", fix leak
Follow-up 465028e0e25 (merge: add missing strbuf_release(),
2021-10-07) and address the "msg" memory leak in this block. We could
14: a3bf3045597 = 14: e8ea18b08c2 builtin/merge.c: free "&buf" on "Your local changes..." error
15: 7c70bbdebc8 = 15: 66c24afb893 object-file.c: release the "tag" in check_tag()
16: 17537e1393e = 16: 52744d9690f grep.c: refactor free_grep_patterns()
17: e4bd46a343e = 17: 8ff63d9095c grep API: plug memory leaks by freeing "header_list"
18: 3e4b12cb623 ! 18: 0ad7d59b881 receive-pack: free() the "ref_name" in "struct command"
@@ t/t5527-fetch-odd-refs.sh: test_description='test fetching of oddly-named refs'
# afterwards we will have:
+ ## t/t5560-http-backend-noserver.sh ##
+@@ t/t5560-http-backend-noserver.sh: test_description='test git-http-backend-noserver'
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ HTTPD_DOCUMENT_ROOT_PATH="$TRASH_DIRECTORY"
+
+ ## t/t5561-http-backend.sh ##
+@@ t/t5561-http-backend.sh: test_description='test git-http-backend'
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+ . "$TEST_DIRECTORY"/lib-httpd.sh
+
+
+ ## t/t5562-http-backend-content-length.sh ##
+@@
+ #!/bin/sh
+
+ test_description='test git-http-backend respects CONTENT_LENGTH'
++
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_lazy_prereq GZIP 'gzip --version'
+
## t/t5705-session-id-in-capabilities.sh ##
@@
19: d51ed239a8a ! 19: b3aee41d0b4 push: free_refs() the "local_refs" in set_refspecs()
@@ t/t1416-ref-transaction-hooks.sh: test_description='reference transaction hooks'
test_expect_success setup '
+ ## t/t2402-worktree-list.sh ##
+@@ t/t2402-worktree-list.sh: test_description='test git worktree list'
+ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
++TEST_PASSES_SANITIZE_LEAK=true
+ . ./test-lib.sh
+
+ test_expect_success 'setup' '
+
## t/t5504-fetch-receive-strict.sh ##
@@ t/t5504-fetch-receive-strict.sh: test_description='fetch/receive strict mode'
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply
* Re: [PATCH v7 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Junio C Hamano @ 2023-01-17 16:00 UTC (permalink / raw)
To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben
In-Reply-To: <20230117142706.230404-3-michael.strawbridge@amd.com>
"Strawbridge, Michael" <Michael.Strawbridge@amd.com> writes:
> +This hook is invoked by linkgit:git-send-email[1].
> +
> +It takes these command line arguments. They are,
> +1. the name of the file which holds the contents of the email to be sent.
> +2. The name of the file which holds the SMTP envelope and headers of the email.
The previous iteration said SMTP headers, but now this talks about
envelope. I however did not think we have direct access to any
envelope information [*] (i.e. such a feature is necessary if we
want to send to one set of recipients by specifying their addresses
in the envelope, while recording different set of recipients to the
e-mail headers' To: and Cc: list)?
Side note. We can specify different sender and it gets
passed as a command line argument "-f $sender" to the
sendmail program, which is used in "MAIL FROM" SMTP
envelope. But I do not think we muck with the list of
recipients that appear in the header when we formulate "RCPT
TO". And I do not see where you give "MAIL FROM" value in
the input to the hook in the patch.
If we say "we will give your hook information in the envelope and
headers" to those who know the distinction between the two, they
will inevitably say "that is great. Now how do I tell which in file
$2 are in the envelope and which are in the header, when some of
them are different?"
We just hand the message plus the header, and let $sendmail_cmd come
up with the envelope info using what is in the header, no? I am not
sure we want to mention envelope as that would give readers a false
impression that we may treat it separately from the headers.
Thanks.
^ permalink raw reply
* Re: [PATCH] treewide: always have a valid "index_state.repo" member
From: Derrick Stolee @ 2023-01-17 15:34 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Victoria Dye, Jeff Hostetler
In-Reply-To: <patch-1.1-b4998652822-20230117T135234Z-avarab@gmail.com>
On 1/17/2023 8:57 AM, Ævar Arnfjörð Bjarmason wrote:
> This an updated verison of the 6/6 of [A], which per Junio's [B]
> wasn't picked up with those patches, which are now in "next".
>
> Junio: Now that Derrick's ds/omit-trailing-hash-in-index has landed on
> "master" this can be applied on top a merge of "master" and what you
> have in "ab/cache-api-cleanup" (that topic itself being based on a
> too-old "master").
>
> Since the v2 I changed the "Complete the double-reference" logic in
> repo_read_index() so that we're not working around a state of a
> affairs that no longer exists with this change.
>
> A. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20230112T124842Z-avarab@gmail.com/
> B. https://lore.kernel.org/git/xmqqtu0u2q9u.fsf@gitster.g/
>
> Range-diff:
These changes look good to me. Thanks for dealing with the branch-
hopping.
Thanks,
-Stolee
^ permalink raw reply
* Re: [PATCH] rebase -i: allow a comment after a "break" command
From: Phillip Wood @ 2023-01-17 15:33 UTC (permalink / raw)
To: Junio C Hamano, Jeff King
Cc: Phillip Wood via GitGitGadget, git, Olliver Schinagl,
Johannes Schindelin, Elijah Newren,
Ævar Arnfjörð Bjarmason, Sergey Organov
In-Reply-To: <xmqqilha18yd.fsf@gitster.g>
On 13/01/2023 20:22, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> I had somewhat the opposite thought. The "break" command is special in
>> that it is not doing anything useful except returning control to the
>> user. And hence producing a message is a useful add-on. So I expected
>> the patch to just allow:
>>
>> break this is a message the user will see
>>
>> without any "#" at all.
>
> Ah, I am OK with that, too.
>
>> That does close the door for further arguments, but I have trouble
>> imagining what they would be.
>
> Making almost everything that the tool does not pay attention to
> (like the patch title of "pick") into comments, floated by Elijah in
> the thread, does sound another reasonable direction to go. If we
> are not doing "pick 0f2d4b69 # the title of the commit", adding the
> message without "#" to "break" might be a better way to go for
> consistency.
Indeed, it seems the question is whether we want to make the changes
Elijah suggested - if so then we should use a "#" with the "break"
command as well but if not then it I agree it would be better not have
have "#" for comments with "break".
My impression is that there is some support for Elijah's suggestion and
no one has spoken up to oppose it so maybe we should go for that.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v3 1/1] cat-file: quote-format name in error when using -z
From: Phillip Wood @ 2023-01-17 15:24 UTC (permalink / raw)
To: Toon Claes, git; +Cc: Phillip Wood
In-Reply-To: <20230116190749.4141516-2-toon@iotcl.com>
Hi Toon
On 16/01/2023 19:07, Toon Claes wrote:
> Since it's supported to have NUL-delimited input, introduced in
> db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,
> 2022-07-22), it's possible to pass paths that contain newlines. This
> works great when the object is found, but when it's not, the input path
> is returned in the error message. Because this can contain newlines, the
> error message might get spread over multiple lines, making it harder to
> machine-parse this error message.
>
> With this change, the input is quote-formatted in the error message, if
> needed. This ensures the error message is always on a single line and
> makes parsing the error more straightforward.
This looks good but it would be nice to have test coverage for
batch_one_object() as well as batch_object_write()
Best Wishes
Phillip
> Signed-off-by: Toon Claes <toon@iotcl.com>
> ---
> builtin/cat-file.c | 19 +++++++++++++++++++
> t/t1006-cat-file.sh | 8 ++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index cc17635e76..b678f69773 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -14,6 +14,7 @@
> #include "tree-walk.h"
> #include "oid-array.h"
> #include "packfile.h"
> +#include "quote.h"
> #include "object-store.h"
> #include "promisor-remote.h"
> #include "mailmap.h"
> @@ -455,8 +456,17 @@ static void batch_object_write(const char *obj_name,
> &data->oid, &data->info,
> OBJECT_INFO_LOOKUP_REPLACE);
> if (ret < 0) {
> + struct strbuf quoted = STRBUF_INIT;
> +
> + if (opt->nul_terminated &&
> + obj_name) {
> + quote_c_style(obj_name, "ed, NULL, 0);
> + obj_name = quoted.buf;
> + }
> +
> printf("%s missing\n",
> obj_name ? obj_name : oid_to_hex(&data->oid));
> + strbuf_release("ed);
> fflush(stdout);
> return;
> }
> @@ -503,6 +513,13 @@ static void batch_one_object(const char *obj_name,
> result = get_oid_with_context(the_repository, obj_name,
> flags, &data->oid, &ctx);
> if (result != FOUND) {
> + struct strbuf quoted = STRBUF_INIT;
> +
> + if (opt->nul_terminated) {
> + quote_c_style(obj_name, "ed, NULL, 0);
> + obj_name = quoted.buf;
> + }
> +
> switch (result) {
> case MISSING_OBJECT:
> printf("%s missing\n", obj_name);
> @@ -527,6 +544,8 @@ static void batch_one_object(const char *obj_name,
> result);
> break;
> }
> +
> + strbuf_release("ed);
> fflush(stdout);
> return;
> }
> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
> index 23b8942edb..e8ce20e739 100755
> --- a/t/t1006-cat-file.sh
> +++ b/t/t1006-cat-file.sh
> @@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '
> test_cmp expect actual
> '
>
> +test_expect_success '--batch-check, -z with newline in non-existent named object' '
> + printf "HEAD:newline${LF}missing" >in &&
> + git cat-file --batch-check -z <in >actual &&
> +
> + printf "\"HEAD:newline\\\\nmissing\" missing\n" >expect &&
> + test_cmp expect actual
> +'
> +
> batch_command_multiple_info="info $hello_sha1
> info $tree_sha1
> info $commit_sha1
> --
> 2.39.0.rc0.57.g2e71cbbddd.dirty
^ permalink raw reply
* Re: [PATCH v3] grep: correctly identify utf-8 characters with \{b,w} in -P
From: Junio C Hamano @ 2023-01-17 15:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Carlo Marcelo Arenas Belón, git, Diomidis Spinellis
In-Reply-To: <230117.865yd5z4ke.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> To argue with myself here, I'm not so sure that just making this the
> default isn't the right move, especially as the GNU grep maintainer
> seems to be convinced that that's the right thing for grep(1).
OK.
> I think calling this e.g.:
>
> grep.perl.Unicode=<bool>
> grep.patternTypePerl.Unicode=<bool>
>
> Or even:
>
> grep.patternTypePerl.Flags=u
>
> Would be better, i.e. PCRE's C API is really just mapping to the flags
> you can find in "perldoc perlre" (https://perldoc.perl.org/perlre). In
> this case the /u flag maps to the "PCRE2_UCP" API flag.
>
> That we happen to use PCRE to give ourselves "Perl" semantics is an
> implementation detail we should avoid exposing, so we could either give
> our config generic names, or literally map to the perl /flags/.
>
> For now we could just die on any "Flags" value that isn't "u".
>
> Of course all of this is predicated on us wanting to leave this as an
> opt-in, which I'm not so sure about. If it's opt-out we'll avoid this
> entire question,
Making it opt-out would also require a similar knob to turn the
"flag" off, be it a configuration variable or a command line option,
wouldn't it? I tend to agree with you that it makes sense to make
it a goal to take us closer to "grep -P" from GNU---do they have
such an opt-out knob? If not, let's make it simple by turning it
always on, which would be the simplest ;-)
Again, thanks for a careful review with concrete points.
^ permalink raw reply
* Re: [PATCH v5 1/2] send-email: refactor header generation functions
From: Junio C Hamano @ 2023-01-17 15:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Strawbridge, Michael, git@vger.kernel.org, Tuikov, Luben
In-Reply-To: <230117.86wn5lxpl0.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> +sub send_message {
>> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
>
> This makes the diff smaller, but if we're refactoring these functions to
> return arguments it's probably better to return a hash reference rather
> than remembering all the parameter names.
>
> But in this case it's probably fine...
> ...
>> +sub pre_process_file {
>> + my ($t, $quiet) = @_;
>
> This, I think, is an anti-pattern in this file. We can just read the
> "$quiet" and other things that we set up when we parse the parameters as
> globals, we don't need to pass them as named parameters.
>
> It doesn't help readability to shadow that variable with another lexical
> here below:
> ...
Thanks as usual for a careful review, with concrete suggestions for
improvements.
^ permalink raw reply
* Re: [PATCH v5 0/4] Optionally skip hashing index on write
From: Derrick Stolee @ 2023-01-17 14:49 UTC (permalink / raw)
To: Junio C Hamano, Derrick Stolee via GitGitGadget
Cc: git, vdye, avarab, newren, Jacob Keller
In-Reply-To: <xmqq358cgn3s.fsf@gitster.g>
On 1/15/2023 4:31 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> Writing the index is a critical action that takes place in multiple Git
>> commands. The recent performance improvements available with the sparse
>> index show how often the I/O costs around the index can affect different Git
>> commands, although reading the index takes place more often than a write.
>
> https://github.com/git/git/actions/runs/3922482355/jobs/6705454550#step:4:2006
>
> shows that all other platforms passed but osx-gcc failed one of the
> tests this series added. Could it be that there is something racy
> about the test (1006.6 if I am not mistaken)?
Yes, you are right. The patch below fixes the problem. Sorry about that!
-Stolee
-- >8 --
From 6827205f979ecfa66f4f9edabfb247cff05f0605 Mon Sep 17 00:00:00 2001
From: Derrick Stolee <derrickstolee@github.com>
Date: Tue, 17 Jan 2023 09:46:04 -0500
Subject: [PATCH] t1600: fix racy index.skipHash test
The test 1600.6 can fail under --stress due to mtime collisions. Most of
the tests include a removal of the index file to guarantee that the
index is updated. However, the submodule test addded in ee1f0c242ef
(read-cache: add index.skipHash config option, 2023-01-06) did not
include this removal. Thus, on rare occasions, the test can fail because
the index still has a non-null trailing hash, as detected by the helper
added in da9acde14ed (test-lib-functions: add helper for trailing hash,
2023-01-06).
By removing the submodule's index before the 'git -C sub add a' command,
we guarantee that the index is rewritten with the new index.skipHash
config option.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
t/t1600-index.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t1600-index.sh b/t/t1600-index.sh
index 0ebbae13058..9368d82f7d7 100755
--- a/t/t1600-index.sh
+++ b/t/t1600-index.sh
@@ -88,6 +88,7 @@ test_expect_success 'index.skipHash config option' '
git -c protocol.file.allow=always submodule add ./ sub &&
git config index.skipHash false &&
git -C sub config index.skipHash true &&
+ rm -f .git/modules/sub/index &&
>sub/file &&
git -C sub add a &&
test_trailing_hash .git/modules/sub/index >hash &&
--
2.38.0.vfs.0.0
^ permalink raw reply related
* [PATCH v7 0/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Strawbridge, Michael @ 2023-01-17 14:27 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Strawbridge, Michael
Thank you both for the input on v6. I believe that I have incorporated
all of your feedback. I decided to keep a few examples of headers in
the documentation as I believe if I were someone wanting to write a hook
I would find the example helpful. It points out unobvious things like
Cc being capitalized as it is and also how multiple lines work.
Michael Strawbridge (2):
send-email: refactor header generation functions
send-email: expose header information to git-send-email's
sendemail-validate hook
Documentation/githooks.txt | 28 +++++++++++--
git-send-email.perl | 80 +++++++++++++++++++++++++-------------
t/t9001-send-email.sh | 39 ++++++++++++++++++-
3 files changed, 113 insertions(+), 34 deletions(-)
--
2.34.1
^ permalink raw reply
* [PATCH v7 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Strawbridge, Michael @ 2023-01-17 14:27 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Strawbridge, Michael, Tuikov, Luben, Junio C Hamano
In-Reply-To: <20230117142706.230404-1-michael.strawbridge@amd.com>
To allow further flexibility in the Git hook, the SMTP header
information of the email which git-send-email intends to send, is now
passed as the 2nd argument to the sendemail-validate hook.
As an example, this can be useful for acting upon keywords in the
subject or specific email addresses.
Cc: Luben Tuikov <luben.tuikov@amd.com>
Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
Documentation/githooks.txt | 28 +++++++++++++++++++++++----
git-send-email.perl | 31 ++++++++++++++++++++----------
t/t9001-send-email.sh | 39 ++++++++++++++++++++++++++++++++++++--
3 files changed, 82 insertions(+), 16 deletions(-)
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index a16e62bc8c..978d599be5 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -583,10 +583,30 @@ processed by rebase.
sendemail-validate
~~~~~~~~~~~~~~~~~~
-This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,
-the name of the file that holds the e-mail to be sent. Exiting with a
-non-zero status causes `git send-email` to abort before sending any
-e-mails.
+This hook is invoked by linkgit:git-send-email[1].
+
+It takes these command line arguments. They are,
+1. the name of the file which holds the contents of the email to be sent.
+2. The name of the file which holds the SMTP envelope and headers of the email.
+
+The SMTP envelope and headers are passed in the exact same way as they
+are passed to the user's Mail Transport Agent (MTA). In effect, the email
+given to the user's MTA, is the contents of $2 followed by the contents
+of $1.
+
+Below is an example for a few common headers. Take notice of the
+capitalization and multi-line tab structure.
+
+ From: Example <from@example.com>
+ To: to@example.com
+ Cc: cc@example.com,
+ A <author@example.com>,
+ One <one@example.com>,
+ two@example.com
+ Subject: PATCH-STRING
+
+Exiting with a non-zero status causes `git send-email` to abort
+before sending any e-mails.
fsmonitor-watchman
~~~~~~~~~~~~~~~~~~
diff --git a/git-send-email.perl b/git-send-email.perl
index 42f135a266..4f2039284e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -787,14 +787,6 @@ sub is_format_patch_arg {
@files = handle_backup_files(@files);
-if ($validate) {
- foreach my $f (@files) {
- unless (-p $f) {
- validate_patch($f, $target_xfer_encoding);
- }
- }
-}
-
if (@files) {
unless ($quiet) {
print $_,"\n" for (@files);
@@ -1738,6 +1730,16 @@ sub send_message {
return 1;
}
+if ($validate) {
+ foreach my $f (@files) {
+ unless (-p $f) {
+ pre_process_file($f, 1);
+
+ validate_patch($f, $target_xfer_encoding);
+ }
+ }
+}
+
$in_reply_to = $initial_in_reply_to;
$references = $initial_in_reply_to || '';
$message_num = 0;
@@ -2101,11 +2103,20 @@ sub validate_patch {
chdir($repo->wc_path() or $repo->repo_path())
or die("chdir: $!");
local $ENV{"GIT_DIR"} = $repo->repo_path();
+
+ my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
+
+ require File::Temp;
+ my ($header_filehandle, $header_filename) = File::Temp::tempfile(
+ ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
+ print $header_filehandle $header;
+
my @cmd = ("git", "hook", "run", "--ignore-missing",
$hook_name, "--");
- my @cmd_msg = (@cmd, "<patch>");
- my @cmd_run = (@cmd, $target);
+ my @cmd_msg = (@cmd, "<patch>", "<header>");
+ my @cmd_run = (@cmd, $target, $header_filename);
$hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
+ unlink($header_filehandle);
chdir($cwd_save) or die("chdir: $!");
}
if ($hook_error) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 1130ef21b3..c0fcbacdaa 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -540,7 +540,7 @@ test_expect_success $PREREQ "--validate respects relative core.hooksPath path" '
test_path_is_file my-hooks.ran &&
cat >expect <<-EOF &&
fatal: longline.patch: rejected by sendemail-validate hook
- fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
+ fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
warning: no patches were sent
EOF
test_cmp expect actual
@@ -559,12 +559,47 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
test_path_is_file my-hooks.ran &&
cat >expect <<-EOF &&
fatal: longline.patch: rejected by sendemail-validate hook
- fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch>'"'"' died with exit code 1
+ fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
warning: no patches were sent
EOF
test_cmp expect actual
'
+test_expect_success $PREREQ 'setup expect' "
+cat >expected-headers <<\EOF
+From: Example <from@example.com>
+To: to@example.com
+Cc: cc@example.com,
+ A <author@example.com>,
+ One <one@example.com>,
+ two@example.com
+Subject: [PATCH 1/1] Second.
+Date: DATE-STRING
+Message-Id: MESSAGE-ID-STRING
+X-Mailer: X-MAILER-STRING
+Reply-To: Reply <reply@example.com>
+MIME-Version: 1.0
+Content-Transfer-Encoding: quoted-printable
+EOF
+"
+
+test_expect_success $PREREQ "--validate hook supports header argument" '
+ write_script my-hooks/sendemail-validate <<-\EOF &&
+ grep "X-test-header: v1.0" "$2"
+ EOF
+ test_config core.hooksPath "my-hooks" &&
+ rm -fr outdir &&
+ git format-patch \
+ --add-header="X-test-header: v1.0" \
+ -n HEAD^1 -o outdir &&
+ git send-email \
+ --dry-run \
+ --to=nobody@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ --validate \
+ outdir/000?-*.patch
+'
+
for enc in 7bit 8bit quoted-printable base64
do
test_expect_success $PREREQ "--transfer-encoding=$enc produces correct header" '
--
2.34.1
^ permalink raw reply related
* [PATCH v7 1/2] send-email: refactor header generation functions
From: Strawbridge, Michael @ 2023-01-17 14:27 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: Strawbridge, Michael, Tuikov, Luben, Junio C Hamano
In-Reply-To: <20230117142706.230404-1-michael.strawbridge@amd.com>
Split process_file and send_message into easier to use functions.
Making SMTP header information widely available.
Cc: Luben Tuikov <luben.tuikov@amd.com>
Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 49 ++++++++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 5861e99a6e..42f135a266 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1495,16 +1495,7 @@ sub file_name_is_absolute {
return File::Spec::Functions::file_name_is_absolute($path);
}
-# Prepares the email, then asks the user what to do.
-#
-# If the user chooses to send the email, it's sent and 1 is returned.
-# If the user chooses not to send the email, 0 is returned.
-# If the user decides they want to make further edits, -1 is returned and the
-# caller is expected to call send_message again after the edits are performed.
-#
-# If an error occurs sending the email, this just dies.
-
-sub send_message {
+sub gen_header {
my @recipients = unique_email_list(@to);
@cc = (grep { my $cc = extract_valid_address_or_die($_);
not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
@@ -1546,6 +1537,22 @@ sub send_message {
if (@xh) {
$header .= join("\n", @xh) . "\n";
}
+ my $recipients_ref = \@recipients;
+ return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header);
+}
+
+# Prepares the email, then asks the user what to do.
+#
+# If the user chooses to send the email, it's sent and 1 is returned.
+# If the user chooses not to send the email, 0 is returned.
+# If the user decides they want to make further edits, -1 is returned and the
+# caller is expected to call send_message again after the edits are performed.
+#
+# If an error occurs sending the email, this just dies.
+
+sub send_message {
+ my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
+ my @recipients = @$recipients_ref;
my @sendmail_parameters = ('-i', @recipients);
my $raw_from = $sender;
@@ -1735,11 +1742,8 @@ sub send_message {
$references = $initial_in_reply_to || '';
$message_num = 0;
-# Prepares the email, prompts the user, sends it out
-# Returns 0 if an edit was done and the function should be called again, or 1
-# otherwise.
-sub process_file {
- my ($t) = @_;
+sub pre_process_file {
+ my ($t, $quiet) = @_;
open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
@@ -1893,9 +1897,9 @@ sub process_file {
}
close $fh;
- push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t)
+ push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t, $quiet)
if defined $to_cmd;
- push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t)
+ push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet)
if defined $cc_cmd && !$suppress_cc{'cccmd'};
if ($broken_encoding{$t} && !$has_content_type) {
@@ -1954,6 +1958,15 @@ sub process_file {
@initial_to = @to;
}
}
+}
+
+# Prepares the email, prompts the user, and sends it out
+# Returns 0 if an edit was done and the function should be called again, or 1
+# on the email being successfully sent out.
+sub process_file {
+ my ($t) = @_;
+
+ pre_process_file($t, $quiet);
my $message_was_sent = send_message();
if ($message_was_sent == -1) {
@@ -2002,7 +2015,7 @@ sub process_file {
# Execute a command (e.g. $to_cmd) to get a list of email addresses
# and return a results array
sub recipients_cmd {
- my ($prefix, $what, $cmd, $file) = @_;
+ my ($prefix, $what, $cmd, $file, $quiet) = @_;
my @addresses = ();
open my $fh, "-|", "$cmd \Q$file\E"
--
2.34.1
^ permalink raw reply related
* [PATCH] treewide: always have a valid "index_state.repo" member
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 13:57 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Victoria Dye, Jeff Hostetler,
Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqqlem2coix.fsf@gitster.g>
When the "repo" member was added to "the_index" in [1] the
repo_read_index() was made to populate it, but the unpopulated
"the_index" variable didn't get the same treatment.
Let's do that in initialize_the_repository() when we set it up, and
likewise for all of the current callers initialized an empty "struct
index_state".
This simplifies code that needs to deal with "the_index" or a custom
"struct index_state", we no longer need to second-guess this part of
the "index_state" deep in the stack. A recent example of such
second-guessing is the "istate->repo ? istate->repo : the_repository"
code in [2]. We can now simply use "istate->repo".
We're doing this by making use of the INDEX_STATE_INIT() macro (and
corresponding function) added in [3], which now have mandatory "repo"
arguments.
Because we now call index_state_init() in repository.c's
initialize_the_repository() we don't need to handle the case where we
have a "repo->index" whose "repo" member doesn't match the "repo"
we're setting up, i.e. the "Complete the double-reference" code in
repo_read_index() being altered here. That logic was originally added
in [1], and was working around the lack of what we now have in
initialize_the_repository().
For "fsmonitor-settings.c" we can remove the initialization of a NULL
"r" argument to "the_repository". This was added back in [4], and was
needed at the time for callers that would pass us the "r" from an
"istate->repo". Before this change such a change to
"fsmonitor-settings.c" would segfault all over the test suite (e.g. in
t0002-gitfile.sh).
This change has wider eventual implications for
"fsmonitor-settings.c". The reason the other lazy loading behavior in
it is required (starting with "if (!r->settings.fsmonitor) ..." is
because of the previously passed "r" being "NULL".
I have other local changes on top of this which move its configuration
reading to "prepare_repo_settings()" in "repo-settings.c", as we could
now start to rely on it being called for our "r". But let's leave all
of that for now, and narrowly remove this particular part of the
lazy-loading.
1. 1fd9ae517c4 (repository: add repo reference to index_state,
2021-01-23)
2. ee1f0c242ef (read-cache: add index.skipHash config option,
2023-01-06)
3. 2f6b1eb794e (cache API: add a "INDEX_STATE_INIT" macro/function,
add release_index(), 2023-01-12)
4. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
2022-03-25)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
This an updated verison of the 6/6 of [A], which per Junio's [B]
wasn't picked up with those patches, which are now in "next".
Junio: Now that Derrick's ds/omit-trailing-hash-in-index has landed on
"master" this can be applied on top a merge of "master" and what you
have in "ab/cache-api-cleanup" (that topic itself being based on a
too-old "master").
Since the v2 I changed the "Complete the double-reference" logic in
repo_read_index() so that we're not working around a state of a
affairs that no longer exists with this change.
A. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20230112T124842Z-avarab@gmail.com/
B. https://lore.kernel.org/git/xmqqtu0u2q9u.fsf@gitster.g/
Range-diff:
1: 291233b0420 ! 1: b4998652822 treewide: always have a valid "index_state.repo" member
@@ Commit message
code in [2]. We can now simply use "istate->repo".
We're doing this by making use of the INDEX_STATE_INIT() macro (and
- corresponding function) added in the preceding commit, which now have
- mandatory "repo" arguments. As seen here there are cases where we set
- it to NULL, but only if we're about to fill in the correct non-NULL
- "repo" right afterwards.
+ corresponding function) added in [3], which now have mandatory "repo"
+ arguments.
+
+ Because we now call index_state_init() in repository.c's
+ initialize_the_repository() we don't need to handle the case where we
+ have a "repo->index" whose "repo" member doesn't match the "repo"
+ we're setting up, i.e. the "Complete the double-reference" code in
+ repo_read_index() being altered here. That logic was originally added
+ in [1], and was working around the lack of what we now have in
+ initialize_the_repository().
For "fsmonitor-settings.c" we can remove the initialization of a NULL
- "r" argument to "the_repository". This was added back in [3], and was
+ "r" argument to "the_repository". This was added back in [4], and was
needed at the time for callers that would pass us the "r" from an
"istate->repo". Before this change such a change to
"fsmonitor-settings.c" would segfault all over the test suite (e.g. in
@@ Commit message
2021-01-23)
2. ee1f0c242ef (read-cache: add index.skipHash config option,
2023-01-06)
- 3. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
+ 3. 2f6b1eb794e (cache API: add a "INDEX_STATE_INIT" macro/function,
+ add release_index(), 2023-01-12)
+ 4. 1e0ea5c4316 (fsmonitor: config settings are repository-specific,
2022-03-25)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
@@ repository.c: void initialize_the_repository(void)
}
@@ repository.c: int repo_read_index(struct repository *repo)
+ {
+ int res;
++ /* Complete the double-reference */
if (!repo->index) {
ALLOC_ARRAY(repo->index, 1);
- index_state_init(repo->index);
-+ index_state_init(repo->index, NULL /* set below */);
- }
+- }
+-
+- /* Complete the double-reference */
+- if (!repo->index->repo)
+- repo->index->repo = repo;
+- else if (repo->index->repo != repo)
++ index_state_init(repo->index, repo);
++ } else if (repo->index->repo != repo) {
+ BUG("repo's index should point back at itself");
++ }
+
+ res = read_index_from(repo->index, repo->index_file, repo->gitdir);
- /* Complete the double-reference */
## revision.c ##
@@ revision.c: void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
apply.c | 2 +-
builtin/difftool.c | 2 +-
builtin/sparse-checkout.c | 2 +-
builtin/stash.c | 8 ++++----
builtin/worktree.c | 2 +-
cache.h | 9 ++++++---
fsmonitor-settings.c | 14 --------------
fsmonitor.c | 2 +-
merge-recursive.c | 2 +-
read-cache.c | 17 +++++------------
repository.c | 13 ++++++-------
revision.c | 2 +-
sparse-index.c | 9 ---------
split-index.c | 2 +-
unpack-trees.c | 2 +-
15 files changed, 30 insertions(+), 58 deletions(-)
diff --git a/apply.c b/apply.c
index 821fe411ec8..5eec433583e 100644
--- a/apply.c
+++ b/apply.c
@@ -4105,7 +4105,7 @@ static int preimage_oid_in_gitlink_patch(struct patch *p, struct object_id *oid)
static int build_fake_ancestor(struct apply_state *state, struct patch *list)
{
struct patch *patch;
- struct index_state result = INDEX_STATE_INIT;
+ struct index_state result = INDEX_STATE_INIT(state->repo);
struct lock_file lock = LOCK_INIT;
int res;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 758e7bd3b6a..dbbfb19f192 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
struct hashmap_iter iter;
struct pair_entry *entry;
- struct index_state wtindex = INDEX_STATE_INIT;
+ struct index_state wtindex = INDEX_STATE_INIT(the_repository);
struct checkout lstate, rstate;
int err = 0;
struct child_process cmd = CHILD_PROCESS_INIT;
diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index dc332c6d05f..c3738154918 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -217,7 +217,7 @@ static int update_working_directory(struct pattern_list *pl)
o.head_idx = -1;
o.src_index = r->index;
o.dst_index = r->index;
- index_state_init(&o.result);
+ index_state_init(&o.result, r);
o.skip_sparse_checkout = 0;
o.pl = pl;
diff --git a/builtin/stash.c b/builtin/stash.c
index a5f13fb1e9f..839569a9803 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1137,7 +1137,7 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
int ret = 0;
struct strbuf untracked_msg = STRBUF_INIT;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
- struct index_state istate = INDEX_STATE_INIT;
+ struct index_state istate = INDEX_STATE_INIT(the_repository);
cp_upd_index.git_cmd = 1;
strvec_pushl(&cp_upd_index.args, "update-index", "-z", "--add",
@@ -1176,7 +1176,7 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
{
int ret = 0;
struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
- struct index_state istate = INDEX_STATE_INIT;
+ struct index_state istate = INDEX_STATE_INIT(the_repository);
if (write_index_as_tree(&info->w_tree, &istate, the_repository->index_file,
0, NULL)) {
@@ -1209,7 +1209,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
int ret = 0;
struct child_process cp_read_tree = CHILD_PROCESS_INIT;
struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
- struct index_state istate = INDEX_STATE_INIT;
+ struct index_state istate = INDEX_STATE_INIT(the_repository);
char *old_index_env = NULL, *old_repo_index_file;
remove_path(stash_index_path.buf);
@@ -1271,7 +1271,7 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
struct rev_info rev;
struct child_process cp_upd_index = CHILD_PROCESS_INIT;
struct strbuf diff_output = STRBUF_INIT;
- struct index_state istate = INDEX_STATE_INIT;
+ struct index_state istate = INDEX_STATE_INIT(the_repository);
init_revisions(&rev, NULL);
copy_pathspec(&rev.prune_data, ps);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 311d6e90750..f51c40f1e1e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -923,7 +923,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
static void validate_no_submodules(const struct worktree *wt)
{
- struct index_state istate = INDEX_STATE_INIT;
+ struct index_state istate = INDEX_STATE_INIT(the_repository);
struct strbuf path = STRBUF_INIT;
int i, found_submodules = 0;
diff --git a/cache.h b/cache.h
index be28fce12ac..4bf14e0bd94 100644
--- a/cache.h
+++ b/cache.h
@@ -367,10 +367,13 @@ struct index_state {
* If the variable won't be used again, use release_index() to free()
* its resources. If it needs to be used again use discard_index(),
* which does the same thing, but will use use index_state_init() at
- * the end.
+ * the end. The discard_index() will use its own "istate->repo" as the
+ * "r" argument to index_state_init() in that case.
*/
-#define INDEX_STATE_INIT { 0 }
-void index_state_init(struct index_state *istate);
+#define INDEX_STATE_INIT(r) { \
+ .repo = (r), \
+}
+void index_state_init(struct index_state *istate, struct repository *r);
void release_index(struct index_state *istate);
/* Name hashing */
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index ee63a97dc51..899bfe9c813 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -143,8 +143,6 @@ static void lookup_fsmonitor_settings(struct repository *r)
enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
{
- if (!r)
- r = the_repository;
if (!r->settings.fsmonitor)
lookup_fsmonitor_settings(r);
@@ -153,8 +151,6 @@ enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
const char *fsm_settings__get_hook_path(struct repository *r)
{
- if (!r)
- r = the_repository;
if (!r->settings.fsmonitor)
lookup_fsmonitor_settings(r);
@@ -174,8 +170,6 @@ void fsm_settings__set_ipc(struct repository *r)
* Caller requested IPC explicitly, so avoid (possibly
* recursive) config lookup.
*/
- if (!r)
- r = the_repository;
if (!r->settings.fsmonitor)
r->settings.fsmonitor = alloc_settings();
@@ -197,8 +191,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
* Caller requested hook explicitly, so avoid (possibly
* recursive) config lookup.
*/
- if (!r)
- r = the_repository;
if (!r->settings.fsmonitor)
r->settings.fsmonitor = alloc_settings();
@@ -210,8 +202,6 @@ void fsm_settings__set_hook(struct repository *r, const char *path)
void fsm_settings__set_disabled(struct repository *r)
{
- if (!r)
- r = the_repository;
if (!r->settings.fsmonitor)
r->settings.fsmonitor = alloc_settings();
@@ -223,8 +213,6 @@ void fsm_settings__set_disabled(struct repository *r)
void fsm_settings__set_incompatible(struct repository *r,
enum fsmonitor_reason reason)
{
- if (!r)
- r = the_repository;
if (!r->settings.fsmonitor)
r->settings.fsmonitor = alloc_settings();
@@ -235,8 +223,6 @@ void fsm_settings__set_incompatible(struct repository *r,
enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
{
- if (!r)
- r = the_repository;
if (!r->settings.fsmonitor)
lookup_fsmonitor_settings(r);
diff --git a/fsmonitor.c b/fsmonitor.c
index 08af00c7387..a5b9e75437b 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -304,7 +304,7 @@ void refresh_fsmonitor(struct index_state *istate)
char *buf;
unsigned int i;
int is_trivial = 0;
- struct repository *r = istate->repo ? istate->repo : the_repository;
+ struct repository *r = istate->repo;
enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
enum fsmonitor_reason reason = fsm_settings__get_reason(r);
diff --git a/merge-recursive.c b/merge-recursive.c
index 0948b3ea961..ae469f8cc81 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -412,7 +412,7 @@ static int unpack_trees_start(struct merge_options *opt,
{
int rc;
struct tree_desc t[3];
- struct index_state tmp_index = INDEX_STATE_INIT;
+ struct index_state tmp_index = INDEX_STATE_INIT(opt->repo);
memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts));
if (opt->priv->call_depth)
diff --git a/read-cache.c b/read-cache.c
index d191741f600..7bd12afb38c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2300,8 +2300,6 @@ static void set_new_index_sparsity(struct index_state *istate)
* If the index's repo exists, mark it sparse according to
* repo settings.
*/
- if (!istate->repo)
- return;
prepare_repo_settings(istate->repo);
if (!istate->repo->settings.command_requires_full_index &&
is_sparse_index_allowed(istate, 0))
@@ -2330,8 +2328,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
fd = open(path, O_RDONLY);
if (fd < 0) {
if (!must_exist && errno == ENOENT) {
- if (!istate->repo)
- istate->repo = the_repository;
set_new_index_sparsity(istate);
return 0;
}
@@ -2433,9 +2429,6 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
trace2_data_intmax("index", the_repository, "read/cache_nr",
istate->cache_nr);
- if (!istate->repo)
- istate->repo = the_repository;
-
/*
* If the command explicitly requires a full index, force it
* to be full. Otherwise, correct the sparsity based on repository
@@ -2501,7 +2494,7 @@ int read_index_from(struct index_state *istate, const char *path,
release_index(split_index->base);
else
ALLOC_ARRAY(split_index->base, 1);
- index_state_init(split_index->base);
+ index_state_init(split_index->base, istate->repo);
base_oid_hex = oid_to_hex(&split_index->base_oid);
base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex);
@@ -2540,9 +2533,9 @@ int is_index_unborn(struct index_state *istate)
return (!istate->cache_nr && !istate->timestamp.sec);
}
-void index_state_init(struct index_state *istate)
+void index_state_init(struct index_state *istate, struct repository *r)
{
- struct index_state blank = INDEX_STATE_INIT;
+ struct index_state blank = INDEX_STATE_INIT(r);
memcpy(istate, &blank, sizeof(*istate));
}
@@ -2579,7 +2572,7 @@ void release_index(struct index_state *istate)
void discard_index(struct index_state *istate)
{
release_index(istate);
- index_state_init(istate);
+ index_state_init(istate, istate->repo);
}
/*
@@ -2933,7 +2926,7 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
int ieot_entries = 1;
struct index_entry_offset_table *ieot = NULL;
int nr, nr_threads;
- struct repository *r = istate->repo ? istate->repo : the_repository;
+ struct repository *r = istate->repo;
f = hashfd(tempfile->fd, tempfile->filename.buf);
diff --git a/repository.c b/repository.c
index a5805fa33b1..937fa974b38 100644
--- a/repository.c
+++ b/repository.c
@@ -28,6 +28,8 @@ void initialize_the_repository(void)
the_repo.remote_state = remote_state_new();
the_repo.parsed_objects = parsed_object_pool_new();
+ index_state_init(&the_index, the_repository);
+
repo_set_hash_algo(&the_repo, GIT_HASH_SHA1);
}
@@ -302,16 +304,13 @@ int repo_read_index(struct repository *repo)
{
int res;
+ /* Complete the double-reference */
if (!repo->index) {
ALLOC_ARRAY(repo->index, 1);
- index_state_init(repo->index);
- }
-
- /* Complete the double-reference */
- if (!repo->index->repo)
- repo->index->repo = repo;
- else if (repo->index->repo != repo)
+ index_state_init(repo->index, repo);
+ } else if (repo->index->repo != repo) {
BUG("repo's index should point back at itself");
+ }
res = read_index_from(repo->index, repo->index_file, repo->gitdir);
diff --git a/revision.c b/revision.c
index fb090886f5b..21f5f572c22 100644
--- a/revision.c
+++ b/revision.c
@@ -1813,7 +1813,7 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
worktrees = get_worktrees();
for (p = worktrees; *p; p++) {
struct worktree *wt = *p;
- struct index_state istate = INDEX_STATE_INIT;
+ struct index_state istate = INDEX_STATE_INIT(revs->repo);
if (wt->is_current)
continue; /* current index already taken care of */
diff --git a/sparse-index.c b/sparse-index.c
index 86e3b99870b..147a13386a4 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -128,9 +128,6 @@ int is_sparse_index_allowed(struct index_state *istate, int flags)
if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
return 0;
- if (!istate->repo)
- istate->repo = the_repository;
-
if (!(flags & SPARSE_INDEX_MEMORY_ONLY)) {
int test_env;
@@ -327,9 +324,6 @@ void expand_index(struct index_state *istate, struct pattern_list *pl)
pl = NULL;
}
- if (!istate->repo)
- istate->repo = the_repository;
-
/*
* A NULL pattern set indicates we are expanding a full index, so
* we use a special region name that indicates the full expansion.
@@ -552,9 +546,6 @@ void expand_to_path(struct index_state *istate,
if (!istate->sparse_index)
return;
- if (!istate->repo)
- istate->repo = the_repository;
-
in_expand_to_path = 1;
/*
diff --git a/split-index.c b/split-index.c
index a5b56c0c37f..5d0f04763ea 100644
--- a/split-index.c
+++ b/split-index.c
@@ -91,7 +91,7 @@ void move_cache_to_base_index(struct index_state *istate)
}
ALLOC_ARRAY(si->base, 1);
- index_state_init(si->base);
+ index_state_init(si->base, istate->repo);
si->base->version = istate->version;
/* zero timestamp disables racy test in ce_write_index() */
si->base->timestamp = istate->timestamp;
diff --git a/unpack-trees.c b/unpack-trees.c
index d1d1b0f319e..3d05e45a279 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1905,7 +1905,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
populate_from_existing_patterns(o, &pl);
}
- index_state_init(&o->result);
+ index_state_init(&o->result, o->src_index->repo);
o->result.initialized = 1;
o->result.timestamp.sec = o->src_index->timestamp.sec;
o->result.timestamp.nsec = o->src_index->timestamp.nsec;
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply related
* Re: [PATCH v5 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 13:23 UTC (permalink / raw)
To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben, Junio C Hamano
In-Reply-To: <20230110211452.2568535-3-michael.strawbridge@amd.com>
On Tue, Jan 10 2023, Strawbridge, Michael wrote:
> To allow further flexibility in the git hook, the SMTP header
> information of the email that git-send-email intends to send, is now
> passed as a 2nd argument to the sendemail-validate hook.
>
> As an example, this can be useful for acting upon keywords in the
> subject or specific email addresses.
Okey, but...
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index a16e62bc8c..2b5c6640cc 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -583,10 +583,19 @@ processed by rebase.
> sendemail-validate
> ~~~~~~~~~~~~~~~~~~
>
> -This hook is invoked by linkgit:git-send-email[1]. It takes a single parameter,
> -the name of the file that holds the e-mail to be sent. Exiting with a
> -non-zero status causes `git send-email` to abort before sending any
> -e-mails.
> +This hook is invoked by linkgit:git-send-email[1].
> +
> +It takes these command line arguments:
> +1. the name of the file that holds the e-mail to be sent.
> +2. the name of the file that holds the SMTP headers to be used.
> +
> +The hook doesn't need to support multiple header names (for example only Cc
> +is passed). However, it does need to understand that lines beginning with
> +whitespace belong to the previous header. The header information follows
> +the same format as the confirmation given at the end of send-email.
> +
> +Exiting with a non-zero status causes `git send-email` to abort
> +before sending any e-mails.
>
> fsmonitor-watchman
> ~~~~~~~~~~~~~~~~~~
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 810dd1f1ce..b2adca515e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -787,14 +787,6 @@ sub is_format_patch_arg {
>
> @files = handle_backup_files(@files);
>
> -if ($validate) {
> - foreach my $f (@files) {
> - unless (-p $f) {
> - validate_patch($f, $target_xfer_encoding);
> - }
> - }
> -}
> -
> if (@files) {
> unless ($quiet) {
> print $_,"\n" for (@files);
> @@ -1738,6 +1730,16 @@ sub send_message {
> return 1;
> }
>
> +if ($validate) {
> + foreach my $f (@files) {
> + unless (-p $f) {
> + pre_process_file($f, 1);
> +
> + validate_patch($f, $target_xfer_encoding);
> + }
> + }
> +}
...here we have the seemingly unrelated change of first doing the
validation before this, and if we pass it we'll print the names of the
files we're sending unless --quiet.
Now we'll do it the other way around, maybe that's good, or maybe not,
but your updated docs don't say.
Also (and I didn't look at this all that carefully), why are you moving
the control logic to between the later function declarations?
Perl isn't a language where you need to arrange your source in that way
(unless a bareword is involved, or if this happens at BEGIN time or
whatever). The current structure is:
<use & imports>
<basic setup (getopts etc)>
<main logic>
<helper function>
Here you're moving part of the main logic to in-between two helper
function, why?
> $in_reply_to = $initial_in_reply_to;
> $references = $initial_in_reply_to || '';
> $message_num = 0;
> @@ -2101,11 +2103,20 @@ sub validate_patch {
> chdir($repo->wc_path() or $repo->repo_path())
> or die("chdir: $!");
> local $ENV{"GIT_DIR"} = $repo->repo_path();
> +
> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
> +
> + require File::Temp;
> + my ($header_filehandle, $header_filename) = File::Temp::tempfile(
> + ".gitsendemail.header.XXXXXX", DIR => $repo->repo_path());
> + print $header_filehandle $header;
> +
> my @cmd = ("git", "hook", "run", "--ignore-missing",
> $hook_name, "--");
> - my @cmd_msg = (@cmd, "<patch>");
> - my @cmd_run = (@cmd, $target);
> + my @cmd_msg = (@cmd, "<patch>", "<header>");
> + my @cmd_run = (@cmd, $target, $header_filename);
> $hook_error = system_or_msg(\@cmd_run, undef, "@cmd_msg");
> + unlink($header_filehandle);
> chdir($cwd_save) or die("chdir: $!");
I know "git hook run" doesn't support input on stdin yet, but isn't this
just working around it not supporting that? That seems like a much
better & natural interface than what we're doing here.
I have out-of-tree patches for that (or rather, a re-roll of Emily's
patches to do that), if that landed in-tree could this use that
interface, do you think?
I'd rather that we didn't forever codify a strange interface here due to
a temporary limitation in "git hook" and the hook API...
^ permalink raw reply
* Re: [PATCH v5 1/2] send-email: refactor header generation functions
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 13:20 UTC (permalink / raw)
To: Strawbridge, Michael; +Cc: git@vger.kernel.org, Tuikov, Luben, Junio C Hamano
In-Reply-To: <20230110211452.2568535-2-michael.strawbridge@amd.com>
On Tue, Jan 10 2023, Strawbridge, Michael wrote:
> Split process_file and send_message into easier to use functions.
> Making SMTP header information more widely available.
>
> Cc: Luben Tuikov <luben.tuikov@amd.com>
> Cc: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
> ---
> git-send-email.perl | 49 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5861e99a6e..810dd1f1ce 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1495,16 +1495,7 @@ sub file_name_is_absolute {
> return File::Spec::Functions::file_name_is_absolute($path);
> }
>
> -# Prepares the email, then asks the user what to do.
> -#
> -# If the user chooses to send the email, it's sent and 1 is returned.
> -# If the user chooses not to send the email, 0 is returned.
> -# If the user decides they want to make further edits, -1 is returned and the
> -# caller is expected to call send_message again after the edits are performed.
> -#
> -# If an error occurs sending the email, this just dies.
> -
> -sub send_message {
> +sub gen_header {
> my @recipients = unique_email_list(@to);
> @cc = (grep { my $cc = extract_valid_address_or_die($_);
> not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
> @@ -1546,6 +1537,22 @@ sub send_message {
> if (@xh) {
> $header .= join("\n", @xh) . "\n";
> }
> + my $recipients_ref = \@recipients;
> + return ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header);
> +}
> +
> +# Prepares the email, then asks the user what to do.
> +#
> +# If the user chooses to send the email, it's sent and 1 is returned.
> +# If the user chooses not to send the email, 0 is returned.
> +# If the user decides they want to make further edits, -1 is returned and the
> +# caller is expected to call send_message again after the edits are performed.
> +#
> +# If an error occurs sending the email, this just dies.
> +
> +sub send_message {
> + my ($recipients_ref, $to, $date, $gitversion, $cc, $ccline, $header) = gen_header();
This makes the diff smaller, but if we're refactoring these functions to
return arguments it's probably better to return a hash reference rather
than remembering all the parameter names.
But in this case it's probably fine...
> + my @recipients = @$recipients_ref;
>
> my @sendmail_parameters = ('-i', @recipients);
> my $raw_from = $sender;
> @@ -1735,11 +1742,8 @@ sub send_message {
> $references = $initial_in_reply_to || '';
> $message_num = 0;
>
> -# Prepares the email, prompts the user, sends it out
> -# Returns 0 if an edit was done and the function should be called again, or 1
> -# otherwise.
> -sub process_file {
> - my ($t) = @_;
> +sub pre_process_file {
> + my ($t, $quiet) = @_;
This, I think, is an anti-pattern in this file. We can just read the
"$quiet" and other things that we set up when we parse the parameters as
globals, we don't need to pass them as named parameters.
It doesn't help readability to shadow that variable with another lexical
here below:
> [...]
> +}
> +
> +# Prepares the email, prompts the user, sends it out
> +# Returns 0 if an edit was done and the function should be called again, or 1
> +# otherwise.
> +sub process_file {
> + my ($t) = @_;
> +
> + pre_process_file($t, $quiet);
>
> my $message_was_sent = send_message();
> if ($message_was_sent == -1) {
> @@ -2002,7 +2015,7 @@ sub process_file {
> # Execute a command (e.g. $to_cmd) to get a list of email addresses
> # and return a results array
> sub recipients_cmd {
> - my ($prefix, $what, $cmd, $file) = @_;
> + my ($prefix, $what, $cmd, $file, $quiet) = @_;
>
> my @addresses = ();
> open my $fh, "-|", "$cmd \Q$file\E"
^ permalink raw reply
* Re: [PATCH 2/2] [RFC] push: allow delete one level ref
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 13:14 UTC (permalink / raw)
To: ZheNing Hu via GitGitGadget
Cc: git, Junio C Hamano, Derrick Stolee, Elijah Newren,
Michael Haggerty, Christian Couder, Jeff King, ZheNing Hu
In-Reply-To: <605b95bf8ab6f1fb5b1ec5b75cd4dcaefbb7f3b6.1673951562.git.gitgitgadget@gmail.com>
On Tue, Jan 17 2023, ZheNing Hu via GitGitGadget wrote:
> From: ZheNing Hu <adlternative@gmail.com>
>
> Git will reject the deletion of one level refs e,g. "refs/foo"
> through "git push -d", however, some users want to be able to
> clean up these branches that were created unexpectedly on the
> remote.
>
> Therefore, when updating branches on the server with
> "git receive-pack", by checking whether it is a branch deletion
> operation, it will determine whether to allow the update of
> one level refs. This avoids creating/updating such one level
> branches, but allows them to be deleted.
>
> On the client side, "git push" also does not properly fill in
> the old-oid of one level refs, which causes the server-side
> "git receive-pack" to think that the ref's old-oid has changed
> when deleting one level refs, this causes the push to be rejected.
>
> So the solution is to fix the client to be able to delete
> one level refs by properly filling old-oid.
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
> builtin/receive-pack.c | 5 ++++-
> connect.c | 2 +-
> t/t5516-fetch-push.sh | 13 +++++++++++++
> 3 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 13ff9fae3ba..ad21877ea1b 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1463,7 +1463,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)
> find_shared_symref(worktrees, "HEAD", name);
>
> /* only refs/... are allowed */
> - if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
> + if (!starts_with(name, "refs/") ||
> + check_refname_format(name + 5,
> + is_null_oid(new_oid) ?
> + REFNAME_ALLOW_ONELEVEL : 0)) {
Style nit: We tend to wrap at 79 characters, adn with argument lists you
"keep going" until you hit that limit.
In this case strictly following that rule will lead to funny
indentation, as we'll have to wrap at "is_null_oid(...)" etc.
But even when avoiding that (which seems good in this case) this should
be:
if (!starts_with(name, "refs/") ||
check_refname_format(name + 5, is_null_oid(new_oid) ?
REFNAME_ALLOW_ONELEVEL : 0)) {
> rp_error("refusing to update funny ref '%s' remotely", name);
> ret = "funny refname";
> goto out;
> diff --git a/connect.c b/connect.c
> index 63e59641c0d..b841ae58e03 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -30,7 +30,7 @@ static int check_ref(const char *name, unsigned int flags)
> return 0;
>
> /* REF_NORMAL means that we don't want the magic fake tag refs */
> - if ((flags & REF_NORMAL) && check_refname_format(name, 0))
> + if ((flags & REF_NORMAL) && check_refname_format(name, REFNAME_ALLOW_ONELEVEL))
Here we should wrap after "name,", we end up with a too-long line.
> return 0;
>
> /* REF_HEADS means that we want regular branch heads */
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index f37861efc40..dec8950a392 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -903,6 +903,19 @@ test_expect_success 'push --delete refuses empty string' '
> test_must_fail git push testrepo --delete ""
> '
>
> +test_expect_success 'push --delete onelevel refspecs' '
> + mk_test testrepo heads/main &&
> + (
> + cd testrepo &&
> + git update-ref refs/onelevel refs/heads/main
> + ) &&
Avoid the subshell here with:
git -C update-ref ....
> + git push testrepo --delete refs/onelevel &&
> + (
> + cd testrepo &&
> + test_must_fail git rev-parse --verify refs/onelevel
Ditto.
^ permalink raw reply
* Re: [PATCH v3] grep: correctly identify utf-8 characters with \{b,w} in -P
From: Ævar Arnfjörð Bjarmason @ 2023-01-17 12:38 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, gitster, Diomidis Spinellis
In-Reply-To: <20230117105123.58328-1-carenas@gmail.com>
On Tue, Jan 17 2023, Carlo Marcelo Arenas Belón wrote:
> When UTF is enabled for a PCRE match, the PCRE2_UTF flag is used
> by the pcre2_compile() call, but that would only allow for the
> use of Unicode character properties when caseless is required
> but not to include the additional UTF characters for all other
> class matches.
>
> This would result in failed matches for expressions that rely
> on those properties, for ex:
>
> $ git grep -P '\bÆvar'
>
> Add a configuration that could be used to enable the PCRE2_UCP
> flag to correctly match those cases, when required.
>
> The use of this has an impact on performance that has been estimated
> to be significant.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> Changes since v2:
> * make setting UCP and opt-in as suggested by Ævar
To argue with myself here, I'm not so sure that just making this the
default isn't the right move, especially as the GNU grep maintainer
seems to be convinced that that's the right thing for grep(1).
We've usually just followed GNU grep semantics, so if it's doing X and
we're doing Y after this it's probably better to unify our behavior with
theirs.
I was mainly concerned with the behavior change sneaking in as a mere
bugfix, I think it's OK if we change the behavior, as long as we're
going into it with our eyes open...
> * remove performance test and instead add a test
...I didn't follow the thread(s) where this may have been discussed, but
I for one would like to see a perf test with this, but maybe it was
removed for a good reason that I'm not aware of...
> Documentation/config/grep.txt | 6 ++++++
> grep.c | 11 ++++++++++-
> grep.h | 1 +
> t/t7810-grep.sh | 13 +++++++++++++
> 4 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
> index e521f20390..8848db7311 100644
> --- a/Documentation/config/grep.txt
> +++ b/Documentation/config/grep.txt
> @@ -26,3 +26,9 @@ grep.fullName::
> grep.fallbackToNoIndex::
> If set to true, fall back to git grep --no-index if git grep
> is executed outside of a git repository. Defaults to false.
> +
> +pcre.ucp::
> + If set to true, will use all Unicode Character Properties when matching
> + `\w`, `\b`, `\d` or the POSIX classes (ex: `[:alnum:]`) and PCRE is used
> + as the underlying engine. If PCRE is not being used it is ignored.
> + Defaults to false
There's a couple of exceptions to this, but we tend to stick config docs
in their corresponding Documentation/config/<namespace>.txt, so this
should be in a new Documentation/config/pcre.txt if we're adding this
name.
But I'd rather that we don't expose the implementation detail that we're
using PCRE, which we haven't done so far. We just have a
"grep.patternType=perl", which (and that's another issue, in any case)
we should explicitly mention here, i.e. that this is for use with that
config (and corresponding option(s)).
I think calling this e.g.:
grep.perl.Unicode=<bool>
grep.patternTypePerl.Unicode=<bool>
Or even:
grep.patternTypePerl.Flags=u
Would be better, i.e. PCRE's C API is really just mapping to the flags
you can find in "perldoc perlre" (https://perldoc.perl.org/perlre). In
this case the /u flag maps to the "PCRE2_UCP" API flag.
That we happen to use PCRE to give ourselves "Perl" semantics is an
implementation detail we should avoid exposing, so we could either give
our config generic names, or literally map to the perl /flags/.
For now we could just die on any "Flags" value that isn't "u".
Of course all of this is predicated on us wanting to leave this as an
opt-in, which I'm not so sure about. If it's opt-out we'll avoid this
entire question,
> diff --git a/grep.c b/grep.c
> index 06eed69493..ceafb8937d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -102,6 +102,12 @@ int grep_config(const char *var, const char *value, void *cb)
> return config_error_nonbool(var);
> return color_parse(value, color);
> }
> +
> + if (!strcmp(var, "pcre.ucp")) {
> + opt->pcre_ucp = git_config_bool(var, value);
> + return 0;
> + }
> +
> return 0;
> }
>
> @@ -292,8 +298,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
> }
> options |= PCRE2_CASELESS;
> }
> - if (!opt->ignore_locale && is_utf8_locale() && !literal)
> + if (!opt->ignore_locale && is_utf8_locale() && !literal) {
> options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
> + if (opt->pcre_ucp)
> + options |= PCRE2_UCP;
> + }
This interaction with locale settings etc. is probably correct, but if
we're keeping the config etc. we should really document how this
interacts with those.
I.e. you might expect "-c grep.patternType=perl -c
<whatever_the_setting_is>=true" to give you UCP semantics, but we'll
ignore it based on these other criteria.
> #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER
> /* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */
> diff --git a/grep.h b/grep.h
> index 6075f997e6..082bd3a0c7 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -171,6 +171,7 @@ struct grep_opt {
> int file_break;
> int heading;
> int max_count;
> + int pcre_ucp;
> void *priv;
The reason for why we have some "bool"-like settings (like "int
ignore_case") as an "int" as opposed to an "unsigned int <name>:1"
bitfield is because we need to take their address via the
parse_options() API.
But in this case it's a purely internal field, so (and again, if we're
keeping the option) let's use "unsigned int ...:1" here instead?
Unless that is, we're expecting a corresponding command-line option.
>
> void (*output)(struct grep_opt *opt, const void *data, size_t size);
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 8eded6ab27..a99a967060 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh
> @@ -95,6 +95,7 @@ test_expect_success setup '
> then
> echo "¿" >reverse-question-mark
> fi &&
> + echo "émotion" >ucp &&
Here we carry this file through the entirety ouf our tests...
> git add . &&
> test_tick &&
> git commit -m initial
> @@ -1474,6 +1475,18 @@ test_expect_success PCRE 'grep -P backreferences work (the PCRE NO_AUTO_CAPTURE
> test_cmp hello_world actual
> '
>
> +test_expect_success PCRE 'grep -c pcre.ucp -P fixes \b' '
> + cat >expected <<-\EOF &&
> + ucp:émotion
...only to use it in this one test, it clearly didn't harm anything (or
rather, I didn't run this, but I expect you did and it passed), but how
about avoiding more global state here doing:
test_when_finished "rm -rf repo" &&
git init repo &&
test_commit -C repo msg ucp émotion &&
[...]
> + EOF
> + cat >pattern <<-\EOF &&
> + \bémotion
> + EOF
> + LC_ALL=en_US.UTF-8 git -c pcre.ucp=true grep -P -f pattern >actual &&
> + test_cmp expected actual &&
> + LC_ALL=en_US.UTF-8 test_must_fail git grep -P -f pattern
This will break on platforms that don't have en_US.UTF-8 (and that's not
hypothetical, some systems will skip installing locales for various
reasons).
I see we have some almost recent breakage 1819ad327b7 (grep: fix
multibyte regex handling under macOS, 2022-08-26), but that one adds a
new MB_REGEX prereq, which presumably fails if we don't have this
locale.
You can just use the existing "GETTEXT_LOCALE", which will piggy-back on
our existing locale tests, or we could add a corresponding one for
en_US.UTF-8 and refactor the existing MB_REGEX to be in lib-gettext.sh
where it arguably belongs...
> +'
> +
> test_expect_success 'grep -G invalidpattern properly dies ' '
> test_must_fail git grep -G "a["
> '
^ permalink raw reply
* [PATCH v3] grep: correctly identify utf-8 characters with \{b,w} in -P
From: Carlo Marcelo Arenas Belón @ 2023-01-17 10:51 UTC (permalink / raw)
To: git; +Cc: avarab, gitster, Carlo Marcelo Arenas Belón
In-Reply-To: <20230108155217.2817-1-carenas@gmail.com>
When UTF is enabled for a PCRE match, the PCRE2_UTF flag is used
by the pcre2_compile() call, but that would only allow for the
use of Unicode character properties when caseless is required
but not to include the additional UTF characters for all other
class matches.
This would result in failed matches for expressions that rely
on those properties, for ex:
$ git grep -P '\bÆvar'
Add a configuration that could be used to enable the PCRE2_UCP
flag to correctly match those cases, when required.
The use of this has an impact on performance that has been estimated
to be significant.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Changes since v2:
* make setting UCP and opt-in as suggested by Ævar
* remove performance test and instead add a test
Documentation/config/grep.txt | 6 ++++++
grep.c | 11 ++++++++++-
grep.h | 1 +
t/t7810-grep.sh | 13 +++++++++++++
4 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt
index e521f20390..8848db7311 100644
--- a/Documentation/config/grep.txt
+++ b/Documentation/config/grep.txt
@@ -26,3 +26,9 @@ grep.fullName::
grep.fallbackToNoIndex::
If set to true, fall back to git grep --no-index if git grep
is executed outside of a git repository. Defaults to false.
+
+pcre.ucp::
+ If set to true, will use all Unicode Character Properties when matching
+ `\w`, `\b`, `\d` or the POSIX classes (ex: `[:alnum:]`) and PCRE is used
+ as the underlying engine. If PCRE is not being used it is ignored.
+ Defaults to false
diff --git a/grep.c b/grep.c
index 06eed69493..ceafb8937d 100644
--- a/grep.c
+++ b/grep.c
@@ -102,6 +102,12 @@ int grep_config(const char *var, const char *value, void *cb)
return config_error_nonbool(var);
return color_parse(value, color);
}
+
+ if (!strcmp(var, "pcre.ucp")) {
+ opt->pcre_ucp = git_config_bool(var, value);
+ return 0;
+ }
+
return 0;
}
@@ -292,8 +298,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
}
options |= PCRE2_CASELESS;
}
- if (!opt->ignore_locale && is_utf8_locale() && !literal)
+ if (!opt->ignore_locale && is_utf8_locale() && !literal) {
options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
+ if (opt->pcre_ucp)
+ options |= PCRE2_UCP;
+ }
#ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER
/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */
diff --git a/grep.h b/grep.h
index 6075f997e6..082bd3a0c7 100644
--- a/grep.h
+++ b/grep.h
@@ -171,6 +171,7 @@ struct grep_opt {
int file_break;
int heading;
int max_count;
+ int pcre_ucp;
void *priv;
void (*output)(struct grep_opt *opt, const void *data, size_t size);
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 8eded6ab27..a99a967060 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -95,6 +95,7 @@ test_expect_success setup '
then
echo "¿" >reverse-question-mark
fi &&
+ echo "émotion" >ucp &&
git add . &&
test_tick &&
git commit -m initial
@@ -1474,6 +1475,18 @@ test_expect_success PCRE 'grep -P backreferences work (the PCRE NO_AUTO_CAPTURE
test_cmp hello_world actual
'
+test_expect_success PCRE 'grep -c pcre.ucp -P fixes \b' '
+ cat >expected <<-\EOF &&
+ ucp:émotion
+ EOF
+ cat >pattern <<-\EOF &&
+ \bémotion
+ EOF
+ LC_ALL=en_US.UTF-8 git -c pcre.ucp=true grep -P -f pattern >actual &&
+ test_cmp expected actual &&
+ LC_ALL=en_US.UTF-8 test_must_fail git grep -P -f pattern
+'
+
test_expect_success 'grep -G invalidpattern properly dies ' '
test_must_fail git grep -G "a["
'
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply related
* [PATCH 2/2] [RFC] push: allow delete one level ref
From: ZheNing Hu via GitGitGadget @ 2023-01-17 10:32 UTC (permalink / raw)
To: git
Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano,
Derrick Stolee, Elijah Newren, Michael Haggerty, Christian Couder,
Jeff King, ZheNing Hu, ZheNing Hu
In-Reply-To: <pull.1465.git.1673951562.gitgitgadget@gmail.com>
From: ZheNing Hu <adlternative@gmail.com>
Git will reject the deletion of one level refs e,g. "refs/foo"
through "git push -d", however, some users want to be able to
clean up these branches that were created unexpectedly on the
remote.
Therefore, when updating branches on the server with
"git receive-pack", by checking whether it is a branch deletion
operation, it will determine whether to allow the update of
one level refs. This avoids creating/updating such one level
branches, but allows them to be deleted.
On the client side, "git push" also does not properly fill in
the old-oid of one level refs, which causes the server-side
"git receive-pack" to think that the ref's old-oid has changed
when deleting one level refs, this causes the push to be rejected.
So the solution is to fix the client to be able to delete
one level refs by properly filling old-oid.
Signed-off-by: ZheNing Hu <adlternative@gmail.com>
---
builtin/receive-pack.c | 5 ++++-
connect.c | 2 +-
t/t5516-fetch-push.sh | 13 +++++++++++++
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 13ff9fae3ba..ad21877ea1b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1463,7 +1463,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)
find_shared_symref(worktrees, "HEAD", name);
/* only refs/... are allowed */
- if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
+ if (!starts_with(name, "refs/") ||
+ check_refname_format(name + 5,
+ is_null_oid(new_oid) ?
+ REFNAME_ALLOW_ONELEVEL : 0)) {
rp_error("refusing to update funny ref '%s' remotely", name);
ret = "funny refname";
goto out;
diff --git a/connect.c b/connect.c
index 63e59641c0d..b841ae58e03 100644
--- a/connect.c
+++ b/connect.c
@@ -30,7 +30,7 @@ static int check_ref(const char *name, unsigned int flags)
return 0;
/* REF_NORMAL means that we don't want the magic fake tag refs */
- if ((flags & REF_NORMAL) && check_refname_format(name, 0))
+ if ((flags & REF_NORMAL) && check_refname_format(name, REFNAME_ALLOW_ONELEVEL))
return 0;
/* REF_HEADS means that we want regular branch heads */
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index f37861efc40..dec8950a392 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -903,6 +903,19 @@ test_expect_success 'push --delete refuses empty string' '
test_must_fail git push testrepo --delete ""
'
+test_expect_success 'push --delete onelevel refspecs' '
+ mk_test testrepo heads/main &&
+ (
+ cd testrepo &&
+ git update-ref refs/onelevel refs/heads/main
+ ) &&
+ git push testrepo --delete refs/onelevel &&
+ (
+ cd testrepo &&
+ test_must_fail git rev-parse --verify refs/onelevel
+ )
+'
+
test_expect_success 'warn on push to HEAD of non-bare repository' '
mk_test testrepo heads/main &&
(
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox