* [PATCH v5 03/19] commit-graph: use free_commit_graph() instead of UNLEAK()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-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 v5 06/19] name-rev: don't xstrdup() an already dup'd string
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-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 v5 07/19] repack: fix leaks on error with "goto cleanup"
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-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 v5 01/19] tests: mark tests as passing with SANITIZE=leak
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-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 v5 02/19] bundle.c: don't leak the "args" in the "struct child_process"
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 UTC (permalink / raw)
To: git
Cc: René Scharfe, Junio C Hamano, Eric Sunshine,
Ævar Arnfjörð Bjarmason
In-Reply-To: <cover-v5-00.19-00000000000-20230118T120334Z-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 v5 00/19] leak fixes: various simple leak fixes
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 12:08 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>
See
https://lore.kernel.org/git/cover-v4-00.19-00000000000-20230117T151201Z-avarab@gmail.com/
for the v4. Change since then:
* Don't mark t/t3203-branch-output.sh as passing, which was new in
v4. As noted in
https://lore.kernel.org/git/xmqq1qns8gz4.fsf_-_@gitster.g/ it still
leaked.
As I noted in
https://lore.kernel.org/git/230118.86o7qwxg4e.gmgdl@evledraar.gmail.com/
this was a gcc v.s. clang difference.
I a fix for that "filter" leak as a follow-up, let's just fix it for
real at some point, rather than using the UNLEAK(), sorry about the
churn.
* The couple of things René pointed out on v4 I both took (and he
seems to think so) as suggestions for eventual follow-ups.
In general I agree that some of the fixes here have deeper
root-cause fixes that are worth doing at some point, but in the
meantime having some simple leak fixes (and CI regression testing
for them) is an improvement.
Æ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/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 +
72 files changed, 156 insertions(+), 50 deletions(-)
Range-diff against v4:
1: 2ed69e3cda3 = 1: c47fc0fb637 tests: mark tests as passing with SANITIZE=leak
2: 9993786ba0d = 2: 9eb758117dc bundle.c: don't leak the "args" in the "struct child_process"
3: 8e98d7c4ebf = 3: 01b6229f18a commit-graph: use free_commit_graph() instead of UNLEAK()
4: 966d7657d54 = 4: f4f3aef2861 clone: use free() instead of UNLEAK()
5: 93a8f8fa1b9 = 5: 8d10fbe0b8f various: add missing clear_pathspec(), fix leaks
6: bd15d991ac7 = 6: eb5dc3ac192 name-rev: don't xstrdup() an already dup'd string
7: fd890121ebe = 7: 1fac90c306a repack: fix leaks on error with "goto cleanup"
8: 1fe25bc6981 ! 8: 02248aca3eb 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: 6b3dd9b15f0 = 9: b39d6d29dd5 http-backend.c: fix "dir" and "cmd_arg" leaks in cmd_main()
10: 246f71bb447 = 10: 928dea2d4ee http-backend.c: fix cmd_main() memory leak, refactor reg{exec,free}()
11: ab31d8d10da = 11: 5770b9eb764 commit-graph: fix a parse_options_concat() leak
12: 9054b353220 = 12: 3ff86cb808c show-branch: free() allocated "head" before return
13: 05836b08e0f = 13: 1f3e3524580 builtin/merge.c: use fixed strings, not "strbuf", fix leak
14: e8ea18b08c2 = 14: 15e4b8db805 builtin/merge.c: free "&buf" on "Your local changes..." error
15: 66c24afb893 = 15: d36ad1f818a object-file.c: release the "tag" in check_tag()
16: 52744d9690f = 16: 10959760dfc grep.c: refactor free_grep_patterns()
17: 8ff63d9095c = 17: 6a8f4a567aa grep API: plug memory leaks by freeing "header_list"
18: 0ad7d59b881 = 18: 3c3d48df04b receive-pack: free() the "ref_name" in "struct command"
19: b3aee41d0b4 = 19: f29500a4abc push: free_refs() the "local_refs" in set_refspecs()
--
2.39.0.1225.g30a3d88132d
^ permalink raw reply
* Re: [PATCH v3] grep: correctly identify utf-8 characters with \{b,w} in -P
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 11:49 UTC (permalink / raw)
To: Carlo Arenas; +Cc: Junio C Hamano, git, Diomidis Spinellis
In-Reply-To: <CAPUEspgzrW63GgbjXhKuvjpKXjEhiKaC7jtupiB-3AhcKTba8A@mail.gmail.com>
On Tue, Jan 17 2023, Carlo Arenas wrote:
> On Tue, Jan 17, 2023 at 7:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Æ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 that is definitely the right thing to do for grep, because the
> current behaviour can only be described as a bug (and a bad one at
> it), but after all the push back and performance testing, I am also
> not convinced anymore it needs to be the default for git, because the
> negatives outweigh the positives.
>
> First there is the performance hit, which is inevitable because there
> are just a lot more characters to match when UCP tables are being
> used,[...]
I'm less concerned about the performance, we should aim for correctness
first. We can always provide an opt-out (and the locale setting is
already that opt-out).
> and second there is the fact that PCRE2_UCP itself might not be
> what you want when matching code, because for example numbers are
> never going to be using digits outside what ASCII provides, and
> identifiers have a narrow set of characters as valid than what you
> would expect from all written human languages in history.
[0-9] will be ASCII, but \d will use [^0-9] Unicode numbers.
I agree it might not be expected by some, but I can't really square that
view in my mind with the desire to match "\bÆvar" :). After all that "Æ"
is also arbitrary byte garbage in the ASCII-view of the world.
I can see how it might be more practical in some cases to have "\b" have
Unicode semantics, but to specifically make "\d" an exception. But the
ship has sailed on that in Perl & PCRE land years (or more than a decade
ago). I think us coming up with some exception to that would probably
suck more than going with their behavior.
> Lastly, even with PCRE2_UCP enabled, our current logic for word
> matches is still broken, because the current code still uses a
> definition of word that was done outside what the regex engines
> provide and that roughly matches what you would expect of identifiers
> from C in the ASCII times.
Yes, FWIW I have some WIP patches somewhere to get rid of that bit of
grep.c if we're using PCRE. I.e. the "-w" should be powered by just
adding "\b" to the start/end of the provided string.
That'll then be correct, and faster.
I can't remember if there were some subtle bugs in that, or why I didn't
finish that...
>> > 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 ;-)
>
> GNU grep -P has no knob and would likely never have one.
I think the general knob in not just GNU grep but GNU utils and the
wider *nix landscape is "tweak your LC_ALL and/or other locale
varibales".
Which works for it, and will work for us once we're using PCRE2_UCP too.
> So for now, I think we should acknowledge the bug, provide an option
> for people that might need the fix, and fix all other problems we
> have, which will include changes in PCRE2 as well to better fit our
> use case.
Hrm, what are those PCRE2 changes? The one I saw so far (or was it a
proposal) was to just make its "grep" utility use the PCRE2_UCP like GNU
grep is now doing in its unreleased version in its git repo...
^ permalink raw reply
* Re: [PATCH v6 11/12] http: read HTTP WWW-Authenticate response headers
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 11:42 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, M Hickford,
Jeff Hostetler, Glen Choo, Victoria Dye, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <bc1ac8d3eb3ac6e1161f6b6b67343874c10cd14d.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
> + strbuf_add(&buf, ptr, size);
> +
> + /* Strip the CRLF that should be present at the end of each field */
> + strbuf_trim_trailing_newline(&buf);
> +
> + /* Start of a new WWW-Authenticate header */
> + if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
> + while (isspace(*val))
> + val++;
> +
> + strvec_push(values, val);
> + http_auth.header_is_last_match = 1;
> + goto exit;
> [...]
> + if (http_auth.header_is_last_match && isspace(*buf.buf)) {
> + /* Trim leading whitespace from this continuation hdr line. */
> + strbuf_ltrim(&buf);
The mixture of this isspace() loop and then strbuf_ltrim() seems odd,
why not stick with the strbuf API?
I.e. after skip_iprefix() strbuf_splice() the start of the string away,
then use strbuf_ltrim() in the first "if" branch here?
Likewise this is open-coding the "isspace" in strbuf_ltrim() for the
second "if". Maybe run the strbuf_ltrim() unconditionally, save away the
length before, and then:
if (http_auth.header_is_last_match && prev_len != buf.len) { ...
?
^ permalink raw reply
* Re: [PATCH v6 10/12] http: replace unsafe size_t multiplication with st_mult
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 11:38 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, M Hickford,
Jeff Hostetler, Glen Choo, Victoria Dye, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <cc9a220ed1f12aef2f4df940e71adc1fad917a6b.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Replace direct multiplication of two size_t parameters in curl response
> stream handling callback functions with `st_mult` to guard against
> overflows.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
> http.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/http.c b/http.c
> index 8a5ba3f4776..a2a80318bb2 100644
> --- a/http.c
> +++ b/http.c
> @@ -146,7 +146,7 @@ static int http_schannel_use_ssl_cainfo;
>
> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> {
> - size_t size = eltsize * nmemb;
> + size_t size = st_mult(eltsize, nmemb);
> struct buffer *buffer = buffer_;
>
> if (size > buffer->buf.len - buffer->posn)
> @@ -176,7 +176,7 @@ curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp)
>
> size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> {
> - size_t size = eltsize * nmemb;
> + size_t size = st_mult(eltsize, nmemb);
> struct strbuf *buffer = buffer_;
>
> strbuf_add(buffer, ptr, size);
This is a really worthwhile fix, but shouldn't this be split into its
own stand-alone patch? It applies on "master", and seems like something
that's a good idea outside of this "test-http-server" topic.
^ permalink raw reply
* Re: [PATCH v6 08/12] test-http-server: add simple authentication
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 11:21 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, M Hickford,
Jeff Hostetler, Glen Choo, Victoria Dye, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <a1ff55dd6e25aa39f14b494f482720edf7d1eabd.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> +static struct auth_module *get_auth_module(const char *scheme, int create)
> +{
> + int i;
> + struct auth_module *mod;
> + for (i = 0; i < auth_modules_nr; i++) {
We can use "for (size_t i = 0" syntax now, let's do that here to not mix
"size_t" and "int" types needlessly.
> + if (create) {
> + struct auth_module *mod = xmalloc(sizeof(struct auth_module));
> + mod->scheme = xstrdup(scheme);
> + mod->challenge_params = NULL;
> + CALLOC_ARRAY(mod->tokens, 1);
> + string_list_init_dup(mod->tokens);
Don't use CALLOC_ARRAY() if you're then going to use
string_list_init_dup() (which is good!), use ALLOC_ARRAY() instead. We
don't need to set the memory to 0, only to overwrite it entirely again.
> + ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc);
> + auth_modules[auth_modules_nr++] = mod;
I have not looked at the whole context here, but instead of:
struct auth_module {
char *scheme;
char *challenge_params;
struct string_list *tokens;
};
Why not:
struct auth_module {
char *challenge_params;
struct string_list *tokens;
};
Then you could use a "struct string_list" for this, make the "scheme" be
the "string" member, and stick the remaining two fields in the "util",
and thus save yourself the manual memory management etc.
> +static int is_authed(struct req *req, const char **user, enum worker_result *wr)
> +{
> + enum auth_result result = AUTH_UNKNOWN;
> + struct string_list hdrs = STRING_LIST_INIT_NODUP;
> + struct auth_module *mod;
> +
> + struct string_list_item *hdr;
> + struct string_list_item *token;
> + const char *v;
> + struct strbuf **split = NULL;
> + int i;
> + char *challenge;
> +
> + /*
> + * Check all auth modules and try to validate the request.
> + * The first Authorization header that matches a known auth module
> + * scheme will be consulted to either approve or deny the request.
> + * If no module is found, or if there is no valid token, then 401 error.
> + * Otherwise, only permit the request if anonymous auth is enabled.
> + * It's atypical for user agents/clients to send multiple Authorization
> + * headers, but not explicitly forbidden or defined.
> + */
> + for_each_string_list_item(hdr, &req->header_list) {
> + if (skip_iprefix(hdr->string, "Authorization: ", &v)) {
> + split = strbuf_split_str(v, ' ', 2);
> + if (!split[0] || !split[1]) continue;
> +
> + /* trim trailing space ' ' */
> + strbuf_setlen(split[0], split[0]->len - 1);
> +
> + mod = get_auth_module(split[0]->buf, 0);
> + if (mod) {
> + result = AUTH_DENY;
> +
> + for_each_string_list_item(token, mod->tokens) {
> + if (!strcmp(split[1]->buf, token->string)) {
> + result = AUTH_ALLOW;
> + break;
> + }
> + }
> +
> + goto done;
Sometimes we need a strbuf_split_str, but in this case couldn't you use
the in-place "struct string_list" variant of that instead, and just
carry a "size_t len" here for it, which you'd then pass to
get_auth_module() (which this commit adds)?
Also, you "split" in the loop, but...
> + strbuf_list_free(split);
...only free() the last one here, isn't this leaking?
> +static int split_auth_param(const char *str, char **scheme, char **val)
> +{
> + struct strbuf **p = strbuf_split_str(str, ':', 2);
> +
> + if (!p[0])
> + return -1;
> +
> + /* trim trailing ':' */
> + if (p[0]->len > 0 && p[0]->buf[p[0]->len - 1] == ':')
Don't compare unsigned length fields to "> 0", just do "if (len &&
....)".
Also, maybe I'm just groggy today, but how do we have a trailing ":" if
we just split on ":", and with a limit such that...
> + if (p[1])
> + *val = strbuf_detach(p[1], NULL);
...we have an item after that?
> +static int read_auth_config(const char *name, const char *val, void *data)
> +{
> + int ret = 0;
> + char *scheme = NULL;
Don't init this to NULL, instead the split_auth_param() return value
should be trusted, the compiler will then help us catch errors, no?
> + char *token = NULL;
> + char *challenge = NULL;
In this case it *is* needed though, as the function will return
non-errors, but *maybe* give us the second out parameter.
For such a function though, isn't just assigning "*second_param = NULL"
at the start of it less of a "running with scissors" pattern?
> + struct auth_module *mod = NULL;
This NULL assignment can be dropped, we assign to it below
unconditionally before using it.
> +
> + if (!strcmp(name, "auth.challenge")) {
> + if (split_auth_param(val, &scheme, &challenge)) {
> + ret = error("invalid auth challenge '%s'", val);
> + goto cleanup;
> + }
> +
> + mod = get_auth_module(scheme, 1);
> +
> + /* Replace any existing challenge parameters */
> + free(mod->challenge_params);
> + mod->challenge_params = challenge ? xstrdup(challenge) : NULL;
> + } else if (!strcmp(name, "auth.token")) {
> + if (split_auth_param(val, &scheme, &token)) {
> + ret = error("invalid auth token '%s'", val);
> + goto cleanup;
> + }
> +
> + mod = get_auth_module(scheme, 1);
> +
> + /*
> + * Append to set of valid tokens unless an empty token value
> + * is provided, then clear the existing list.
> + */
> + if (token)
> + string_list_append(mod->tokens, token);
> + else
> + string_list_clear(mod->tokens, 1);
> + } else if (!strcmp(name, "auth.allowanonymous")) {
> + allow_anonymous = git_config_bool(name, val);
> + } else {
> + warning("unknown auth config '%s'", name);
> + }
> +
> +cleanup:
> + free(scheme);
> + free(token);
> + free(challenge);
> +
> + return ret;
> +}
> +
^ permalink raw reply
* Re: [PATCH v6 06/12] test-http-server: add HTTP request parsing
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 11:14 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, M Hickford,
Jeff Hostetler, Glen Choo, Victoria Dye, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <252098db219574527c587bc601565eab81b40c2c.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Add ability to parse HTTP requests to the test-http-server test helper.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
> t/helper/test-http-server.c | 175 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 173 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
> index 6cdac223a55..36f4a54fe6d 100644
> --- a/t/helper/test-http-server.c
> +++ b/t/helper/test-http-server.c
> @@ -83,6 +83,42 @@ enum worker_result {
> WR_HANGUP = 1<<1,
> };
>
> +/*
> + * Fields from a parsed HTTP request.
> + */
> +struct req {
> + struct strbuf start_line;
> +
> + const char *method;
> + const char *http_version;
> +
> + struct strbuf uri_path;
> + struct strbuf query_args;
> +
> + struct string_list header_list;
> + const char *content_type;
> + ssize_t content_length;
> +};
> +
> +#define REQ__INIT { \
> + .start_line = STRBUF_INIT, \
> + .uri_path = STRBUF_INIT, \
> + .query_args = STRBUF_INIT, \
> + .header_list = STRING_LIST_INIT_NODUP, \
> + .content_type = NULL, \
> + .content_length = -1 \
> + }
Style nit: Don't indent the trailing "}", and add a "," after the last
"content_length" item.
We omit the comma by convention when there really should not be another
item, such as when we have a "NULL" terminator, here though we might add
a struct element at the end, so...
> +static enum worker_result req__read(struct req *req, int fd)
> +{
> + struct strbuf h = STRBUF_INIT;
> + struct string_list start_line_fields = STRING_LIST_INIT_DUP;
> + int nr_start_line_fields;
> + const char *uri_target;
> + const char *query;
> + char *hp;
> + const char *hv;
> +
> + enum worker_result result = WR_OK;
> +
> + /*
> + * Read line 0 of the request and split it into component parts:
> + *
> + * <method> SP <uri-target> SP <HTTP-version> CRLF
> + *
> + */
> + if (strbuf_getwholeline_fd(&req->start_line, fd, '\n') == EOF) {
> + result = WR_OK | WR_HANGUP;
> + goto done;
> + }
> +
> + strbuf_trim_trailing_newline(&req->start_line);
> +
> + nr_start_line_fields = string_list_split(&start_line_fields,
> + req->start_line.buf,
> + ' ', -1);
> + if (nr_start_line_fields != 3) {
> + logerror("could not parse request start-line '%s'",
> + req->start_line.buf);
> + result = WR_IO_ERROR;
> + goto done;
> + }
> +
> + req->method = xstrdup(start_line_fields.items[0].string);
> + req->http_version = xstrdup(start_line_fields.items[2].string);
> +
> + uri_target = start_line_fields.items[1].string;
> +
> + if (strcmp(req->http_version, "HTTP/1.1")) {
> + logerror("unsupported version '%s' (expecting HTTP/1.1)",
> + req->http_version);
> + result = WR_IO_ERROR;
> + goto done;
> + }
> +
> + query = strchr(uri_target, '?');
> +
> + if (query) {
> + strbuf_add(&req->uri_path, uri_target, (query - uri_target));
> + strbuf_trim_trailing_dir_sep(&req->uri_path);
> + strbuf_addstr(&req->query_args, query + 1);
> + } else {
> + strbuf_addstr(&req->uri_path, uri_target);
> + strbuf_trim_trailing_dir_sep(&req->uri_path);
> + }
> +
> + /*
> + * Read the set of HTTP headers into a string-list.
> + */
> + while (1) {
> + if (strbuf_getwholeline_fd(&h, fd, '\n') == EOF)
> + goto done;
> + strbuf_trim_trailing_newline(&h);
> +
> + if (!h.len)
> + goto done; /* a blank line ends the header */
> +
> + hp = strbuf_detach(&h, NULL);
> + string_list_append(&req->header_list, hp);
> +
> + /* also store common request headers as struct req members */
> + if (skip_prefix(hp, "Content-Type: ", &hv)) {
> + req->content_type = hv;
> + } else if (skip_prefix(hp, "Content-Length: ", &hv)) {
> + req->content_length = strtol(hv, &hp, 10);
In POSIX the "ssize_t" is not a "this is the unsigned size_t", but can
be a much smaller integer type (although in practice it tends to be the
signed version of "size_t".
But this seems like a potential overflow trap as a result, but sometimes
we need to live with "ssize_t".
However, in this case it seems like we don't, as it seems the only
reason you init'd this to -1 and then...
> + if (trace2_is_enabled()) {
> + struct string_list_item *item;
> + trace2_printf("%s: %s", TR2_CAT, req->start_line.buf);
> + trace2_printf("%s: hver: %s", TR2_CAT, req->http_version);
> + trace2_printf("%s: hmth: %s", TR2_CAT, req->method);
> + trace2_printf("%s: path: %s", TR2_CAT, req->uri_path.buf);
> + trace2_printf("%s: qury: %s", TR2_CAT, req->query_args.buf);
> + if (req->content_length >= 0)
> + trace2_printf("%s: clen: %d", TR2_CAT, req->content_length);
...use that ">= 0" is to keep the state of "did I assign to this above?
So firstly, shouldn't we error or something on a "Content-Length: 0",
and aside from that wouldn't we just have a "int have_content_length =
0" in this function that we'd then flip to 1?
^ permalink raw reply
* Re: [PATCH v6 05/12] test-http-server: add HTTP error response function
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 11:07 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, M Hickford,
Jeff Hostetler, Glen Choo, Victoria Dye, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <79805f042b984bb8ca7c9aaf6a15f8101037c375.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Introduce a function to the test-http-server test helper to write more
> full and valid HTTP error responses, including all the standard response
> headers like `Server` and `Date`.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
> t/helper/test-http-server.c | 58 +++++++++++++++++++++++++++++++++----
> 1 file changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/t/helper/test-http-server.c b/t/helper/test-http-server.c
> index 11071b1dd89..6cdac223a55 100644
> --- a/t/helper/test-http-server.c
> +++ b/t/helper/test-http-server.c
> @@ -83,9 +83,59 @@ enum worker_result {
> WR_HANGUP = 1<<1,
> };
...okey, this is the commit that makes use of WR_HANGUP. Whatever else
we do, let's then squash that addition into this change.
> +static enum worker_result send_http_error(
> + int fd,
> + int http_code, const char *http_code_name,
> + int retry_after_seconds, struct string_list *response_headers,
> + enum worker_result wr_in)
In general in this series you are mis-indenting argument lists. Our
usual style is to wrap at 79 characters, then to align (with tabs and
spaces) with the "(".
So in this case:
static enum worker_result send_http_error(int fd, int http_code,
const char *http_code_name,
int retry_after_seconds,
struct string_list *response_headers,
enum worker_result wr_in)
> +{
> + struct strbuf response_header = STRBUF_INIT;
> + struct strbuf response_content = STRBUF_INIT;
> + struct string_list_item *h;
> + enum worker_result wr;
> +
> + strbuf_addf(&response_content, "Error: %d %s\r\n",
> + http_code, http_code_name);
Ditto here, where "http_code" should go on the preceding line...
> + if (retry_after_seconds > 0)
> + strbuf_addf(&response_content, "Retry-After: %d\r\n",
> + retry_after_seconds);
> +
> + strbuf_addf (&response_header, "HTTP/1.1 %d %s\r\n", http_code, http_code_name);
...and here there's a lack of such wrapping...
> + strbuf_addstr(&response_header, "Cache-Control: private\r\n");
> + strbuf_addstr(&response_header, "Content-Type: text/plain\r\n");
> + strbuf_addf (&response_header, "Content-Length: %d\r\n", (int)response_content.len);
> + if (retry_after_seconds > 0)
> + strbuf_addf(&response_header, "Retry-After: %d\r\n", retry_after_seconds);
> + strbuf_addf( &response_header, "Server: test-http-server/%s\r\n", git_version_string);
> + strbuf_addf( &response_header, "Date: %s\r\n", show_date(time(NULL), 0, DATE_MODE(RFC2822)));
...here you're adding strange whitespace at the start of an argument list...
> + if (response_headers)
> + for_each_string_list_item(h, response_headers)
> + strbuf_addf(&response_header, "%s\r\n", h->string);
> + strbuf_addstr(&response_header, "\r\n");
To comment on the code a bit, this whole thing would be more readable
IMO if your own headers were also a "struct string_list". Yes we'd waste
a bit more memory, but in this case that's fine..
I.e. don't add the "\r\n" every time, just:
string_list_append(&headers, "Cache-Control: private");
etc.
Then at the end you'd do e.g.:
add_headers(&buf, &headers);
if (response_headers)
add_headers(&buf, response_headers);
Where the add_headers() is a trivial "static" helper which does that
for_each_string_list_item() loop above.
> while (1) {
> - if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
> - logerror("unable to write response");
> - wr = WR_IO_ERROR;
> - }
> + wr = send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1,
> + NULL, WR_OK | WR_HANGUP);
This *does* use correct wrapping & indenation for a continuing argument
list.
^ permalink raw reply
* Re: [PATCH] ssh signing: better error message when key not in agent
From: Phillip Wood @ 2023-01-18 11:10 UTC (permalink / raw)
To: Adam Szkoda via GitGitGadget, git; +Cc: Adam Szkoda
In-Reply-To: <pull.1270.git.git.1674029874363.gitgitgadget@gmail.com>
Hi Adam
On 18/01/2023 08:17, Adam Szkoda via GitGitGadget wrote:
> From: Adam Szkoda <adaszko@gmail.com>
>
> When signing a commit with a SSH key, with the private key missing from
> ssh-agent, a confusing error message is produced:
>
> error: Load key
> "/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7":
> invalid format? fatal: failed to write commit object
>
> The temporary file .git_signing_key_tmpkArSj7 created by git contains a
> valid *public* key. The error message comes from `ssh-keygen -Y sign' and
> is caused by a fallback mechanism in ssh-keygen whereby it tries to
> interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in
> the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that
> needs to be done is to pass an additional backward-compatible option -U to
> 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file
> as public key and expects to find the private key in the agent.
The documentation for user.signingKey says
If gpg.format is set to ssh this can contain the path to either your
private ssh key or the public key when ssh-agent is used.
If I've understood correctly passing -U will prevent users from setting
this to a private key.
Best Wishes
Phillip
> As a result, when the private key is missing from the agent, a more accurate
> error message gets produced:
>
> error: Couldn't find key in agent
>
> [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429
>
> Signed-off-by: Adam Szkoda <adaszko@gmail.com>
> ---
> ssh signing: better error message when key not in agent
>
> When signing a commit with a SSH key, with the private key missing from
> ssh-agent, a confusing error message is produced:
>
> error: Load key "/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7": invalid format?
> fatal: failed to write commit object
>
>
> The temporary file .git_signing_key_tmpkArSj7 created by git contains a
> valid public key. The error message comes from `ssh-keygen -Y sign' and
> is caused by a fallback mechanism in ssh-keygen whereby it tries to
> interpret .git_signing_key_tmpkArSj7 as a private key if it can't find
> in the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All
> that needs to be done is to pass an additional backward-compatible
> option -U to 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always
> interprets the file as public key and expects to find the private key in
> the agent.
>
> As a result, when the private key is missing from the agent, a more
> accurate error message gets produced:
>
> error: Couldn't find key in agent
>
>
> [1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v1
> Pull-Request: https://github.com/git/git/pull/1270
>
> gpg-interface.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index 280f1fa1a58..4a5913ae942 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -1022,6 +1022,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
> strvec_pushl(&signer.args, use_format->program,
> "-Y", "sign",
> "-n", "git",
> + "-U",
> "-f", ssh_signing_key_file,
> buffer_file->filename.buf,
> NULL);
>
> base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
^ permalink raw reply
* Re: [PATCH v6 04/12] test-http-server: add stub HTTP server test helper
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 11:04 UTC (permalink / raw)
To: Matthew John Cheetham via GitGitGadget
Cc: git, Derrick Stolee, Lessley Dennington, M Hickford,
Jeff Hostetler, Glen Choo, Victoria Dye, Matthew John Cheetham,
Matthew John Cheetham
In-Reply-To: <d6e5e8825e8454242820738f0dfb03a9f1c01ced.1674012618.git.gitgitgadget@gmail.com>
On Wed, Jan 18 2023, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
> [...]
> +enum worker_result {
> + /*
> + * Operation successful.
> + * Caller *might* keep the socket open and allow keep-alive.
> + */
> + WR_OK = 0,
> [...]
> + enum worker_result wr = WR_OK;
> +
> + if (client_addr)
> + loginfo("Connection from %s:%s", client_addr, client_port);
> +
> + set_keep_alive(0, logerror);
> +
> + while (1) {
> + if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) {
> + logerror("unable to write response");
> + wr = WR_IO_ERROR;
> + }
> +
> + if (wr != WR_OK)
> + break;
> + }
> +
> + close(STDIN_FILENO);
> + close(STDOUT_FILENO);
> +
> + return !!(wr & WR_IO_ERROR);
> +}
We have cases where we assign "0" to a bitfield-looking structure like
this, but only in cases where we're planning to use it as a boolean too.
Or, in other cases where we want some to be explicitly <-1.
Here though we're adding a mixed "OK" and error use, which seems a bit
odd. Shouldn't we pick one or the other?
So far (maybe in later commits?) nothing uses WR_HANGUP, and oddly we
also use the bitfield-looking thing as a return value from main()....
^ permalink raw reply
* Re: [PATCH v4 08/19] worktree: fix a trivial leak in prune_worktrees()
From: Ævar Arnfjörð Bjarmason @ 2023-01-18 10:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, René Scharfe, Eric Sunshine
In-Reply-To: <xmqqedrs8igj.fsf@gitster.g>
On Tue, Jan 17 2023, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> We were leaking both the "struct strbuf" in prune_worktrees(), as well
>> as the "path" we got from should_prune_worktree(). Since these were
>> the only two uses of the "struct string_list" let's change it to a
>> "DUP" and push these to it with "string_list_append_nodup()".
>> ...
>> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
>> index d34d77f8934..ba8d929d189 100755
>> --- a/t/t3203-branch-output.sh
>> +++ b/t/t3203-branch-output.sh
>> @@ -1,6 +1,8 @@
>> #!/bin/sh
>>
>> test_description='git branch display tests'
>> +
>> +TEST_PASSES_SANITIZE_LEAK=true
>> . ./test-lib.sh
>> . "$TEST_DIRECTORY"/lib-terminal.sh
>
> This is wrong, isn't it?
>
> t3203 uses --points-at, which populates filter.points_at by calling
> parse_opt_object_name(). Various members of the ref-filter
> structure is never freed (and there is no API helper function in
> ref-filter subsystem).
>
> Other tests that use --points-at (e.g. t6302 and t7004) are not
> marked with "passes_sanitize_leak", and this one shouldn't be,
> either.
>
> With the following squashed in, the branch seems to pass, but I am
> not sure which is lessor of the two evils. From the point of view
> of the code maintenance, UNLEAK() to mark this singleton variable is
> far cleaner to deal with than selectively running the leak checks
> with the "passes_sanitize_leak" mechanism (which always feels like a
> losing whack-a-mole hack).
>
> builtin/branch.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git c/builtin/branch.c w/builtin/branch.c
> index f63fd45edb..4fe7757670 100644
> --- c/builtin/branch.c
> +++ w/builtin/branch.c
> @@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> if (filter.abbrev == -1)
> filter.abbrev = DEFAULT_ABBREV;
> filter.ignore_case = icase;
> + UNLEAK(filter);
>
> finalize_colopts(&colopts, -1);
> if (filter.verbose) {
I'll send a v5 re-roll without this change, sorry.
This is a case where the version of GCC I was testing with doesn't
report the leak, but clang does (and probably other versions of GCC),
sorry.
^ permalink raw reply
* Re: [PATCH v6 2/2] send-email: expose header information to git-send-email's sendemail-validate hook
From: Luben Tuikov @ 2023-01-18 8:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Strawbridge, Michael, git@vger.kernel.org
In-Reply-To: <xmqqbkmxbort.fsf@gitster.g>
On 2023-01-17 02:31, Junio C Hamano wrote:
> Luben Tuikov <luben.tuikov@amd.com> writes:
>
>>> +test_expect_success $PREREQ "--validate hook supports header argument" '
>>> + write_script my-hooks/sendemail-validate <<-\EOF &&
>>> + if test -s "$2"
>>> + then
>>> + cat "$2" >actual
>>> + exit 1
>>> + fi
>>> + EOF
>
> If "$2" is not given, or an empty "$2" is given, is that an error?
> I am wondering if the lack of "else" clause (and the hook exits with
> success when "$2" is an empty file) here is intentional.
I think we'll always have a $2, since it is the SMTP envelope and headers.
For the rest of the comments, I'll let Michael address them.
--
Regards,
Luben
^ permalink raw reply
* [PATCH] git-cat-file.txt: fix list continuations rendering literally
From: Martin Ågren @ 2023-01-18 8:27 UTC (permalink / raw)
To: git; +Cc: Siddharth Asthana
With Asciidoctor, all of the '+' introduced in a797c0ea04 ("cat-file:
add mailmap support to --batch-check option", 2022-12-20) render
literally rather than functioning as list continuations. With asciidoc,
this renders just fine. It's not too surprising that there is room for
ambiguity and surprises here, since we have lists within lists.
Simply replacing all of these '+' with empty lines makes this render
fine using both tools. Except, in the third hunk, where after this inner
'*' list ends, we want to continue with more contents of the outer list
item (`--batch-command=<format>`). We can solve any ambiguity here and
make this clear to both tools by wrapping the inner list in an open
block (using "--").
For consistency, let's wrap all three of these inner lists from
a797c0ea04 in open blocks. This also future-proofs us a little -- if we
ever gain more contents after any of those first two lists, as we did
already in a797c0ea04 for the third list, we're prepared and should
render fine with both asciidoc and Asciidoctor from the start.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Documentation/git-cat-file.txt | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 830f0a2eff..411de2e27d 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -93,47 +93,52 @@ OPTIONS
Print object information and contents for each object provided
on stdin. May not be combined with any other options or arguments
except `--textconv`, `--filters`, or `--use-mailmap`.
- +
++
+--
* When used with `--textconv` or `--filters`, the input lines
must specify the path, separated by whitespace. See the section
`BATCH OUTPUT` below for details.
- +
+
* When used with `--use-mailmap`, for commit and tag objects, the
contents part of the output shows the identities replaced using the
mailmap mechanism, while the information part of the output shows
the size of the object as if it actually recorded the replacement
identities.
+--
--batch-check::
--batch-check=<format>::
Print object information for each object provided on stdin. May not be
combined with any other options or arguments except `--textconv`, `--filters`
or `--use-mailmap`.
- +
++
+--
* When used with `--textconv` or `--filters`, the input lines must
specify the path, separated by whitespace. See the section
`BATCH OUTPUT` below for details.
- +
+
* When used with `--use-mailmap`, for commit and tag objects, the
printed object information shows the size of the object as if the
identities recorded in it were replaced by the mailmap mechanism.
+--
--batch-command::
--batch-command=<format>::
Enter a command mode that reads commands and arguments from stdin. May
only be combined with `--buffer`, `--textconv`, `--use-mailmap` or
`--filters`.
- +
++
+--
* When used with `--textconv` or `--filters`, the input lines must
specify the path, separated by whitespace. See the section
`BATCH OUTPUT` below for details.
- +
+
* When used with `--use-mailmap`, for commit and tag objects, the
`contents` command shows the identities replaced using the
mailmap mechanism, while the `info` command shows the size
of the object as if it actually recorded the replacement
identities.
-
+--
+
`--batch-command` recognizes the following commands:
+
--
2.39.0.348.g5efb778ab0
^ permalink raw reply related
* [PATCH] ssh signing: better error message when key not in agent
From: Adam Szkoda via GitGitGadget @ 2023-01-18 8:17 UTC (permalink / raw)
To: git; +Cc: Adam Szkoda, Adam Szkoda
From: Adam Szkoda <adaszko@gmail.com>
When signing a commit with a SSH key, with the private key missing from
ssh-agent, a confusing error message is produced:
error: Load key
"/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7":
invalid format? fatal: failed to write commit object
The temporary file .git_signing_key_tmpkArSj7 created by git contains a
valid *public* key. The error message comes from `ssh-keygen -Y sign' and
is caused by a fallback mechanism in ssh-keygen whereby it tries to
interpret .git_signing_key_tmpkArSj7 as a *private* key if it can't find in
the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All that
needs to be done is to pass an additional backward-compatible option -U to
'ssh-keygen -Y sign' call. With '-U', ssh-keygen always interprets the file
as public key and expects to find the private key in the agent.
As a result, when the private key is missing from the agent, a more accurate
error message gets produced:
error: Couldn't find key in agent
[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429
Signed-off-by: Adam Szkoda <adaszko@gmail.com>
---
ssh signing: better error message when key not in agent
When signing a commit with a SSH key, with the private key missing from
ssh-agent, a confusing error message is produced:
error: Load key "/var/folders/t5/cscwwl_n3n1_8_5j_00x_3t40000gn/T//.git_signing_key_tmpkArSj7": invalid format?
fatal: failed to write commit object
The temporary file .git_signing_key_tmpkArSj7 created by git contains a
valid public key. The error message comes from `ssh-keygen -Y sign' and
is caused by a fallback mechanism in ssh-keygen whereby it tries to
interpret .git_signing_key_tmpkArSj7 as a private key if it can't find
in the agent [1]. A fix is scheduled to be released in OpenSSH 9.1. All
that needs to be done is to pass an additional backward-compatible
option -U to 'ssh-keygen -Y sign' call. With '-U', ssh-keygen always
interprets the file as public key and expects to find the private key in
the agent.
As a result, when the private key is missing from the agent, a more
accurate error message gets produced:
error: Couldn't find key in agent
[1] https://bugzilla.mindrot.org/show_bug.cgi?id=3429
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1270%2Fradicle-dev%2Fmaint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1270/radicle-dev/maint-v1
Pull-Request: https://github.com/git/git/pull/1270
gpg-interface.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/gpg-interface.c b/gpg-interface.c
index 280f1fa1a58..4a5913ae942 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -1022,6 +1022,7 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,
strvec_pushl(&signer.args, use_format->program,
"-Y", "sign",
"-n", "git",
+ "-U",
"-f", ssh_signing_key_file,
buffer_file->filename.buf,
NULL);
base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees
From: Carlo Arenas @ 2023-01-18 7:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, pclouds, Jinwook Jeong, Rubén Justo, Eric Sunshine
In-Reply-To: <xmqq5yd48hcb.fsf@gitster.g>
On Tue, Jan 17, 2023 at 10:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
>
> > As reflected on the tests, this will change the behaviour of those
> > commands when they are invoked in a worktree that has that requested
> > branch checked out, as that matches the logic used by branch, is safer
> > (assuming both commands are user facing) and can be overriden with an
> > existing flag.
>
> ... meaning you can "--force", or something else? Allowing an
> existing option to be used as the safety valve does make sense,
> especially if the option is something users are already familiar
> with (like "--force") and naturally expected to work.
the following is the way to override:
$ git checkout --ignore-other-worktrees -B foo
> There might need an documentation update. Back when "checkout -b"
> and "branch" was written, there wasn't "multiple worktrees connected
> to a single repository" hence there was no need to provide safety
> against checking out the same branch in two different places. "git
> branch" might have learned to give that safety while "git checkout
> -b", which _ought_ to be equivalent to "git branch" followed by "git
> checkout", might have forgot to do so.
Not sure if it was originally forgotten, but it is definitely working now;
this change only fixes the uppercase (-B) version.
> After this change, it may
> still be correct to say that "checkout -b" is equivalent to "branch"
> followed by "checkout", but if the documentation to "branch" talks
> about this safety, it probably deserves to be mentioned in the
> documentation to "checkout -b", as well, if only to give an appropriate
> place to talk about how to override it "with an existing flag".
Interestingly, when the flag was added in 1d0fa898ea (checkout: add
--ignore-other-wortrees, 2015-01-03), it was only added to `checkout`.
`git branch` has no flag and will die even when `-f` is used
Carlo
^ permalink raw reply
* Re: [PATCH v3] grep: correctly identify utf-8 characters with \{b,w} in -P
From: Carlo Arenas @ 2023-01-18 7:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, git, Diomidis Spinellis
In-Reply-To: <xmqqr0vt9oj9.fsf@gitster.g>
On Tue, Jan 17, 2023 at 7:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Æ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 that is definitely the right thing to do for grep, because the
current behaviour can only be described as a bug (and a bad one at
it), but after all the push back and performance testing, I am also
not convinced anymore it needs to be the default for git, because the
negatives outweigh the positives.
First there is the performance hit, which is inevitable because there
are just a lot more characters to match when UCP tables are being
used, and second there is the fact that PCRE2_UCP itself might not be
what you want when matching code, because for example numbers are
never going to be using digits outside what ASCII provides, and
identifiers have a narrow set of characters as valid than what you
would expect from all written human languages in history.
Lastly, even with PCRE2_UCP enabled, our current logic for word
matches is still broken, because the current code still uses a
definition of word that was done outside what the regex engines
provide and that roughly matches what you would expect of identifiers
from C in the ASCII times.
> > 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 ;-)
GNU grep -P has no knob and would likely never have one.
So for now, I think we should acknowledge the bug, provide an option
for people that might need the fix, and fix all other problems we
have, which will include changes in PCRE2 as well to better fit our
use case.
Carlo
^ permalink raw reply
* Re: [PATCH v8] curl: resolve deprecated curl declarations
From: Junio C Hamano @ 2023-01-18 7:30 UTC (permalink / raw)
To: Rose via GitGitGadget; +Cc: git, Seija Kijin
In-Reply-To: <pull.1435.v8.git.git.1673991669894.gitgitgadget@gmail.com>
"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1435%2FAtariDreams%2Fcurl-v8
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1435/AtariDreams/curl-v8
> Pull-Request: https://github.com/git/git/pull/1435
>
> Range-diff vs v7:
>
> 1: 23094afb2e6 ! 1: 5e44592695e curl: resolve deprecated curl declarations
> ...
Please stop throwing in quick succession so many unsolicited
iterations of the same topic at the list, before waiting for reviews
from others.
I do not mean "after you sent an initial revision out, even if you
find problems in it, do not send updates in for a while". I mean
"do not send out that initial revision out, before you spend enough
time reading it to find these problems in it you found in, like you
did in these previous topics."
In other words, reviewing your own patches to carefully find
mistakes and correcting them is VERY GOOD, but rather than doing so
in public, you can do so in private before sending the patches out
to polish them sufficiently so that you won't find more trivial
problems [*]. Prepare patches in private and pretend to be a more
perfect human with fewer trivial mistakes ;-).
Note. And you shouldn't be offended by me saying "trivial"; they
were found by yourself in a few minutes to a few hours after
you sent out the previous iteration.
That way, you'd save yourself from public embarrassment and also
save reviewer time.
Thanks.
P.S. Doesn't Peff's 3-patch series already solve the same issue this
patch is trying to address?
^ permalink raw reply
* [PATCH v4 20/19] branch: the ref_filter is not cleaned
From: Junio C Hamano @ 2023-01-18 7:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, René Scharfe, Eric Sunshine
In-Reply-To: <xmqqedrs8igj.fsf@gitster.g>
Recently, a test that uses "branch --point-at" was marked
(incorrectly) as passing the leak tests, but it was premature.
As there is no API support to release the resource held by the
ref_filter structure when we are done, let's mark the singleton
instance that does not grow unbounded as such with UNLEAK() to
squelch pointless leak checker errors.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/branch.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/builtin/branch.c b/builtin/branch.c
index f63fd45edb..4fe7757670 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (filter.abbrev == -1)
filter.abbrev = DEFAULT_ABBREV;
filter.ignore_case = icase;
+ UNLEAK(filter);
finalize_colopts(&colopts, -1);
if (filter.verbose) {
--
2.39.1-231-ga7caae2729
^ permalink raw reply related
* Re: [PATCH v2] checkout/switch: disallow checking out same branch in multiple worktrees
From: Junio C Hamano @ 2023-01-18 6:52 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón
Cc: git, pclouds, Jinwook Jeong, Rubén Justo, Eric Sunshine
In-Reply-To: <20230118061527.76218-1-carenas@gmail.com>
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> As reflected on the tests, this will change the behaviour of those
> commands when they are invoked in a worktree that has that requested
> branch checked out, as that matches the logic used by branch, is safer
> (assuming both commands are user facing) and can be overriden with an
> existing flag.
... meaning you can "--force", or something else? Allowing an
existing option to be used as the safety valve does make sense,
especially if the option is something users are already familiar
with (like "--force") and naturally expected to work.
There might need an documentation update. Back when "checkout -b"
and "branch" was written, there wasn't "multiple worktrees connected
to a single repository" hence there was no need to provide safety
against checking out the same branch in two different places. "git
branch" might have learned th give that safety while "git checkout
-b", which _ought_ to be equivalent to "git branch" followed by "git
checkout", might have forgot to do so. After this change, it may
still be correct to say that "checkout -b" is equivalent to "branch"
followed by "checkout", but if the documentation to "branch" talks
about this safety, it probably deserves to be mentioned in the
documentation to "checkout -b", as well, if only to give an appropriate
place to talk about how to override it "with an existing flag".
Thanks.
^ permalink raw reply
* Re: [ANNOUNCE] Git 2.39.1 and others
From: Junio C Hamano @ 2023-01-18 6:34 UTC (permalink / raw)
To: Ramsay Jones; +Cc: git, linux-kernel, git-packagers, lwn
In-Reply-To: <3b79e8c2-ddb2-fa6f-db6f-1f3cae31a729@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> $ git tag -v v2.31.6
> object 82689d5e5d3f41da2ab1fbf9fbe7aacfd6da74c1
> type commit
> tag v2.31.6
> tagger Junio C Hamano <gitster@pobox.com> 1670933242 +0900
>
> Git 2.31.6
> error: no signature found
Oops. Corrected. Thanks.
^ permalink raw reply
* Re: [PATCH v4 08/19] worktree: fix a trivial leak in prune_worktrees()
From: Junio C Hamano @ 2023-01-18 6:28 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, René Scharfe, Eric Sunshine
In-Reply-To: <patch-v4-08.19-1fe25bc6981-20230117T151202Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> We were leaking both the "struct strbuf" in prune_worktrees(), as well
> as the "path" we got from should_prune_worktree(). Since these were
> the only two uses of the "struct string_list" let's change it to a
> "DUP" and push these to it with "string_list_append_nodup()".
> ...
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index d34d77f8934..ba8d929d189 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -1,6 +1,8 @@
> #!/bin/sh
>
> test_description='git branch display tests'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-terminal.sh
This is wrong, isn't it?
t3203 uses --points-at, which populates filter.points_at by calling
parse_opt_object_name(). Various members of the ref-filter
structure is never freed (and there is no API helper function in
ref-filter subsystem).
Other tests that use --points-at (e.g. t6302 and t7004) are not
marked with "passes_sanitize_leak", and this one shouldn't be,
either.
With the following squashed in, the branch seems to pass, but I am
not sure which is lessor of the two evils. From the point of view
of the code maintenance, UNLEAK() to mark this singleton variable is
far cleaner to deal with than selectively running the leak checks
with the "passes_sanitize_leak" mechanism (which always feels like a
losing whack-a-mole hack).
builtin/branch.c | 1 +
1 file changed, 1 insertion(+)
diff --git c/builtin/branch.c w/builtin/branch.c
index f63fd45edb..4fe7757670 100644
--- c/builtin/branch.c
+++ w/builtin/branch.c
@@ -742,6 +742,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (filter.abbrev == -1)
filter.abbrev = DEFAULT_ABBREV;
filter.ignore_case = icase;
+ UNLEAK(filter);
finalize_colopts(&colopts, -1);
if (filter.verbose) {
^ 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