Re: [PATCH v5 0/6] fast-export, fast-import: add support for signed-commits

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 25, 2025, 14:53 UTC
Message-ID: <98b4c9e7-4034-4692-bc86-f6b905dcc5aa@gmail.com>
In-Reply-To: <20250224142744.279643-1-christian.couder@gmail.com>

Hi Christian

I've only glanced over this series, but I did notice a memory leak

On 24/02/2025 14:27, Christian Couder wrote:

> 
>       + * The returned string has had the ' ' line continuation markers
>      -+ * removed, and points to staticly allocated memory (not to memory
>      ++ * removed, and points to statically allocated memory (not to memory

This corrects the spelling but the changes below remove the static buffer so the user is now responsible for freeing the returned string. That means this comment is wrong and I don't see any corresponding changes to the callers to free the memory.

Show 24 quoted lines

>       + * within 'msg'), so it is only valid until the next call to
>       + * find_commit_multiline_header.
>       + *
>      @@ builtin/fast-export.c: static void anonymize_ident_line(const char **beg, const
>       +						const char *key,
>       +						const char **end)
>       +{
>      -+	static struct strbuf val = STRBUF_INIT;
>      ++	struct strbuf val = STRBUF_INIT;
>       +	const char *bol, *eol;
>       +	size_t len;
>       +
>      -+	strbuf_reset(&val);
>      -+
>       +	bol = find_commit_header(msg, key, &len);
>       +	if (!bol)
>       +		return NULL;
>      @@ builtin/fast-export.c: static void anonymize_ident_line(const char **beg, const
>       +	}
>       +
>       +	*end = eol;
>      -+	return val.buf;
>      ++	return strbuf_detach(&val, NULL);
>       +}

Best Wishes

Phillip

Show 249 quoted lines

>      - static char *reencode_message(const char *in_msg,
>      - 			      const char *in_encoding, size_t in_encoding_len)
>      + static void handle_commit(struct commit *commit, struct rev_info *rev,
>      + 			  struct string_list *paths_of_changed_objects)
>        {
>       @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct rev_info *rev,
>        	const char *author, *author_end, *committer, *committer_end;
>      - 	const char *encoding;
>      + 	const char *encoding = NULL;
>        	size_t encoding_len;
>      -+	const char *signature_alg = NULL, *signature;
>      ++	const char *signature_alg = NULL, *signature = NULL;
>        	const char *message;
>        	char *reencoded = NULL;
>        	struct commit_list *p;
>       @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct rev_info *rev,
>      - 	committer++;
>        	commit_buffer_cursor = committer_end = strchrnul(committer, '\n');
>        
>      --	/* find_commit_header() gets a `+ 1` because
>      + 	/*
>      +-	 * find_commit_header() gets a `+ 1` because
>       -	 * commit_buffer_cursor points at the trailing "\n" at the end
>       -	 * of the previous line, but find_commit_header() wants a
>      -+	/* find_commit_header() and find_commit_multiline_header() get
>      ++	 * find_commit_header() and find_commit_multiline_header() get
>       +	 * a `+ 1` because commit_buffer_cursor points at the trailing
>       +	 * "\n" at the end of the previous line, but they want a
>      - 	 * pointer to the beginning of the next line. */
>      + 	 * pointer to the beginning of the next line.
>      + 	 */
>       +
>      - 	encoding = find_commit_header(commit_buffer_cursor + 1, "encoding", &encoding_len);
>      - 	if (encoding)
>      - 		commit_buffer_cursor = encoding + encoding_len;
>      + 	if (*commit_buffer_cursor == '\n') {
>      + 		encoding = find_commit_header(commit_buffer_cursor + 1, "encoding", &encoding_len);
>      + 		if (encoding)
>      + 			commit_buffer_cursor = encoding + encoding_len;
>      + 	}
>        
>      -+	if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig", &commit_buffer_cursor)))
>      -+		signature_alg = "sha1";
>      -+	else if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig-sha256", &commit_buffer_cursor)))
>      -+		signature_alg = "sha256";
>      ++	if (*commit_buffer_cursor == '\n') {
>      ++		if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig", &commit_buffer_cursor)))
>      ++			signature_alg = "sha1";
>      ++		else if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig-sha256", &commit_buffer_cursor)))
>      ++			signature_alg = "sha256";
>      ++	}
>       +
>        	message = strstr(commit_buffer_cursor, "\n\n");
>        	if (message)
>      @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r
>        	printf("%.*s\n%.*s\n",
>        	       (int)(author_end - author), author,
>        	       (int)(committer_end - committer), committer);
>      -+	if (signature)
>      -+		switch(signed_commit_mode) {
>      ++	if (signature) {
>      ++		switch (signed_commit_mode) {
>       +		case SIGN_ABORT:
>       +			die("encountered signed commit %s; use "
>       +			    "--signed-commits=<mode> to handle it",
>      @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r
>       +		case SIGN_STRIP:
>       +			break;
>       +		}
>      ++		free((char *)signature);
>      ++	}
>        	if (!reencoded && encoding)
>        		printf("encoding %.*s\n", (int)encoding_len, encoding);
>        	printf("data %u\n%s",
>       @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag)
>        					       "\n-----BEGIN PGP SIGNATURE-----\n");
>        		if (signature)
>      - 			switch(signed_tag_mode) {
>      + 			switch (signed_tag_mode) {
>       -			case SIGNED_TAG_ABORT:
>       +			case SIGN_ABORT:
>        				die("encountered signed tag %s; use "
>      @@ builtin/fast-export.c: static void handle_tag(const char *name, struct tag *tag)
>        				message_size = signature + 1 - message;
>        				break;
>        			}
>      -@@ builtin/fast-export.c: static int parse_opt_anonymize_map(const struct option *opt,
>      -
>      - int cmd_fast_export(int argc, const char **argv, const char *prefix)
>      +@@ builtin/fast-export.c: int cmd_fast_export(int argc,
>      + 		    const char *prefix,
>      + 		    struct repository *repo UNUSED)
>        {
>       +	const char *env_signed_commits_noabort;
>        	struct rev_info revs;
>      - 	struct object_array commits = OBJECT_ARRAY_INIT;
>        	struct commit *commit;
>      -@@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const char *prefix)
>      + 	char *export_filename = NULL,
>      +@@ builtin/fast-export.c: int cmd_fast_export(int argc,
>        			    N_("show progress after <n> objects")),
>        		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, N_("mode"),
>        			     N_("select handling of signed tags"),
>      @@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const ch
>        		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
>        			     N_("select handling of tags that tag filtered objects"),
>        			     parse_opt_tag_of_filtered_mode),
>      -@@ builtin/fast-export.c: int cmd_fast_export(int argc, const char **argv, const char *prefix)
>      +@@ builtin/fast-export.c: int cmd_fast_export(int argc,
>        	if (argc == 1)
>        		usage_with_options (fast_export_usage, options);
>        
>      @@ builtin/fast-import.c: static void parse_new_commit(const char *arg)
>       +			strbuf_addstr(&new_data, "gpgsig-sha256 ");
>       +		else
>       +			die("Expected gpgsig algorithm sha1 or sha256, got %s", sig_alg);
>      -+		string_list_split_in_place(&siglines, sig.buf, '\n', -1);
>      ++		string_list_split_in_place(&siglines, sig.buf, "\n", -1);
>       +		strbuf_add_separated_string_list(&new_data, "\n ", &siglines);
>       +		strbuf_addch(&new_data, '\n');
>       +	}
>      @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=warn-strip' '
>       +	# between the two.
>       +	test_config i18n.commitEncoding ISO-8859-1 &&
>       +	git checkout -f -b commit-signing main &&
>      -+	echo Sign your name > file-sign &&
>      ++	echo Sign your name >file-sign &&
>       +	git add file-sign &&
>       +	git commit -S -m "signed commit" &&
>       +	COMMIT_SIGNING=$(git rev-parse --verify commit-signing)
>      @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=warn-strip' '
>       +
>       +test_expect_success GPG 'signed-commits default' '
>       +
>      -+	unset FAST_EXPORT_SIGNED_COMMITS_NOABORT &&
>      ++	sane_unset FAST_EXPORT_SIGNED_COMMITS_NOABORT &&
>       +	test_must_fail git fast-export --reencode=no commit-signing &&
>       +
>       +	FAST_EXPORT_SIGNED_COMMITS_NOABORT=1 git fast-export --reencode=no commit-signing >output 2>err &&
>       +	! grep ^gpgsig output &&
>       +	grep "^encoding ISO-8859-1" output &&
>       +	test -s err &&
>      -+	sed "s/commit-signing/commit-strip-signing/" output |
>      -+		(cd new &&
>      -+		 git fast-import &&
>      -+		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))
>      ++	sed "s/commit-signing/commit-strip-signing/" output | (
>      ++		cd new &&
>      ++		git fast-import &&
>      ++		STRIPPED=$(git rev-parse --verify refs/heads/commit-strip-signing) &&
>      ++		test $COMMIT_SIGNING != $STRIPPED
>      ++	)
>       +
>       +'
>       +
>      @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=warn-strip' '
>       +	git fast-export --signed-commits=verbatim --reencode=no commit-signing >output &&
>       +	grep "^gpgsig sha" output &&
>       +	grep "encoding ISO-8859-1" output &&
>      -+	(cd new &&
>      -+	 git fast-import &&
>      -+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
>      ++	(
>      ++		cd new &&
>      ++		git fast-import &&
>      ++		STRIPPED=$(git rev-parse --verify refs/heads/commit-signing) &&
>      ++		test $COMMIT_SIGNING = $STRIPPED
>      ++	) <output
>       +
>       +'
>       +
>      @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=warn-strip' '
>       +	grep "^gpgsig sha" output &&
>       +	grep "encoding ISO-8859-1" output &&
>       +	test -s err &&
>      -+	(cd new &&
>      -+	 git fast-import &&
>      -+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
>      ++	(
>      ++		cd new &&
>      ++		git fast-import &&
>      ++		STRIPPED=$(git rev-parse --verify refs/heads/commit-signing) &&
>      ++		test $COMMIT_SIGNING = $STRIPPED
>      ++	) <output
>       +
>       +'
>       +
>      @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=warn-strip' '
>       +	git fast-export --signed-commits=strip --reencode=no commit-signing >output &&
>       +	! grep ^gpgsig output &&
>       +	grep "^encoding ISO-8859-1" output &&
>      -+	sed "s/commit-signing/commit-strip-signing/" output |
>      -+		(cd new &&
>      -+		 git fast-import &&
>      -+		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))
>      ++	sed "s/commit-signing/commit-strip-signing/" output | (
>      ++		cd new &&
>      ++		git fast-import &&
>      ++		STRIPPED=$(git rev-parse --verify refs/heads/commit-strip-signing) &&
>      ++		test $COMMIT_SIGNING != $STRIPPED
>      ++	)
>       +
>       +'
>       +
>      @@ t/t9350-fast-export.sh: test_expect_success 'signed-tags=warn-strip' '
>       +	! grep ^gpgsig output &&
>       +	grep "^encoding ISO-8859-1" output &&
>       +	test -s err &&
>      -+	sed "s/commit-signing/commit-strip-signing/" output |
>      -+		(cd new &&
>      -+		 git fast-import &&
>      -+		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))
>      ++	sed "s/commit-signing/commit-strip-signing/" output | (
>      ++		cd new &&
>      ++		git fast-import &&
>      ++		STRIPPED=$(git rev-parse --verify refs/heads/commit-strip-signing) &&
>      ++		test $COMMIT_SIGNING != $STRIPPED
>      ++	)
>       +
>       +'
>       +
>        test_expect_success 'setup submodule' '
>        
>      + 	test_config_global protocol.file.allow always &&
>        	git checkout -f main &&
>      -+	{ git update-ref -d refs/heads/commit-signing || true; } &&
>      ++	test_might_fail git update-ref -d refs/heads/commit-signing &&
>        	mkdir sub &&
>        	(
>        		cd sub &&
> 
> 
> Christian Couder (1):
>    fast-export: fix missing whitespace after switch
> 
> Luke Shumaker (5):
>    git-fast-import.adoc: add missing LF in the BNF
>    fast-export: rename --signed-tags='warn' to 'warn-verbatim'
>    git-fast-export.txt: clarify why 'verbatim' may not be a good idea
>    fast-export: do not modify memory from get_commit_buffer
>    fast-export, fast-import: add support for signed-commits
> 
>   Documentation/git-fast-export.adoc |  25 +++-
>   Documentation/git-fast-import.adoc |  20 ++-
>   builtin/fast-export.c              | 189 +++++++++++++++++++++--------
>   builtin/fast-import.c              |  23 ++++
>   t/t9350-fast-export.sh             | 116 ++++++++++++++++++
>   5 files changed, 317 insertions(+), 56 deletions(-)
>