threads / patch / 55565

patchgit-apply: allow empty patch text

Subject: [PATCH] git-apply: allow empty patch text

## tl;dr

13 messages between Apr 27, 2021 and Dec 13, 2021. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Jerry Zhang· Apr 27, 2021, 01:12 UTC · lore

"git diff" produces no patch text if there is no diff, but "git apply" exits with code 128 if the patch text is empty.

Since every valid "git diff" should result in a successful patch application when applied to the same preimage as the diff, allow "git apply" to handle these empty diffs by doing nothing and returning 0.

Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
 apply.c                | 2 ++
 t/t4126-apply-empty.sh | 7 +++++++
 t/t7006-pager.sh       | 2 +-
 3 files changed, 10 insertions(+), 1 deletion(-)
Show changes to 3 files +10 −1

apply.c, t/t4126-apply-empty.sh, t/t7006-pager.sh

diff --git a/apply.c b/apply.c
index 5ea237232d..2abf2e8a61 100644
--- a/apply.c
+++ b/apply.c
@@ -4685,6 +4685,8 @@ static int apply_patch(struct apply_state *state,
 	state->patch_input_file = filename;
 	if (read_patch_file(&buf, fd) < 0)
 		return -128;
+	if (!buf.len)
+		goto end;
 	offset = 0;
 	while (offset < buf.len) {
 		struct patch *patch;
diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index ceb6a79fe0..37351be609 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -31,6 +31,13 @@ test_expect_success 'apply empty' '
 	test_cmp expect empty
 '
 
+test_expect_success 'apply empty diff' '
+	git reset --hard &&
+	git diff >empty.patch &&
+	git apply empty.patch &&
+	git diff | git apply -
+'
+
 test_expect_success 'apply --index empty' '
 	git reset --hard &&
 	rm -f missing &&
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435..2c0ada6bf6 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -567,7 +567,7 @@ test_default_pager        expect_success 'git -p shortlog'
 test_core_pager_subdir    expect_success 'git -p shortlog'
 
 test_core_pager_subdir    expect_success test_must_fail \
-					 'git -p apply </dev/null'
+					 'git -p log !'
 
 test_expect_success TTY 'command-specific pager' '
 	sane_unset PAGER GIT_PAGER &&
-- 
2.29.0
Eric Wong· Apr 27, 2021, 05:46 UTC · re: Jerry Zhang · lore

Re: [PATCH] git-apply: allow empty patch text

Jerry Zhang <jerry@skydio.com> wrote:
Show 8 quoted lines
> "git diff" produces no patch text if
> there is no diff, but "git apply" exits
> with code 128 if the patch text is empty.
> 
> Since every valid "git diff" should
> result in a successful patch application
> when applied to the same preimage as
> the diff,
Should it result in successful patch application? (Why?)

I fear this change can cause errors in pipelines to go undetected (since "set -o pipefail" is not POSIX). In my experience, zero-byte files is also a common failure mode for some filesystems, even after fsck marked them as clean.

Perhaps guarding this behavior with --allow-empty (as commit and cherry-pick do) is safer.

Show 14 quoted lines
> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
> index ceb6a79fe0..37351be609 100755
> --- a/t/t4126-apply-empty.sh
> +++ b/t/t4126-apply-empty.sh
> @@ -31,6 +31,13 @@ test_expect_success 'apply empty' '
>  	test_cmp expect empty
>  '
>  
> +test_expect_success 'apply empty diff' '
> +	git reset --hard &&
> +	git diff >empty.patch &&
> +	git apply empty.patch &&
> +	git diff | git apply -
> +'

It shouldn't be necessary to use "git diff" to generate empty output. Tests are too slow for me on Linux, even. Something like:

	>empty.patch &&
	git apply empty.patch &&
	git apply - <empty.patch

...ought to be realistic enough, but /dev/null in place of empty.patch is probably portable enough, too. Thanks.

Junio C Hamano· Apr 27, 2021, 06:28 UTC · re: Eric Wong · lore

Re: [PATCH] git-apply: allow empty patch text

Eric Wong <e@80x24.org> writes:
> I fear this change can cause errors in pipelines to go
> undetected (since "set -o pipefail" is not POSIX).
> In my experience, zero-byte files is also a common failure mode
> for some filesystems, even after fsck marked them as clean.
Thanks for saving me time to say it.

It would be a grave regression to any workflow automation to change the behaviour to silently succeed a non-patch application as a noop. And it does not take filesystem corruption.

Some people seem to gpg sign their patches sent to the list, which is not very useful at this point as I don't bother to collect their public keys anyway, but feeding such a piece of e-mail from GNUS to "git am -s" takes "\M-i r |" prefix instead of the regular "|" to "pipe the message to an external command" to properly get it processed. I was saved by the "empty input is wrong" behaviour a number of times.

Jerry Zhang· Apr 27, 2021, 19:40 UTC · re: Jerry Zhang · lore

[PATCH V2] git-apply: add --allow-empty flag

Some users or scripts will pipe "git diff" output to "git apply" when replaying diffs or commits. In these cases, they will rely on the return value of "git apply" to know whether the diff was applied successfully.

However, for empty commits, "git apply" will fail. This complicates scripts since they have to either buffer the diff and check its length, or run diff again with "exit-code", essentially doing the diff twice.

Add the "--allow-empty" flag to "git apply" which allows it to handle both empty diffs and empty commits created by "git format-patch --always" by doing nothing and returning 0.

Add tests for both with and without --allow-empty.
Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
This patch applies on top of "git-apply: add --quiet flag".
The conflict is in Documentation -> Synopsis and is
trivial to solve.
V1 -> V2:
- Moved behavior under a flag
- Added tests for both cases
 Documentation/git-apply.txt |  6 +++++-
 apply.c                     |  8 ++++++--
 apply.h                     |  1 +
 t/t4126-apply-empty.sh      | 16 ++++++++++++++++
 4 files changed, 28 insertions(+), 3 deletions(-)
Show changes to 4 files +28 −3

Documentation/git-apply.txt, apply.c, apply.h, t/t4126-apply-empty.sh

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index a32ad64718..b6d77f4206 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 	  [--ignore-space-change | --ignore-whitespace]
 	  [--whitespace=(nowarn|warn|fix|error|error-all)]
 	  [--exclude=<path>] [--include=<path>] [--directory=<root>]
-	  [--verbose | --quiet] [--unsafe-paths] [<patch>...]
+	  [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]
 
 DESCRIPTION
 -----------
@@ -256,6 +256,10 @@ When `git apply` is used as a "better GNU patch", the user can pass
 the `--unsafe-paths` option to override this safety check.  This option
 has no effect when `--index` or `--cached` is in use.
 
+--allow-empty::
+	Don't return error for patches containing no diff. This includes
+	empty patches and patches with commit text only.
+
 CONFIGURATION
 -------------
 
diff --git a/apply.c b/apply.c
index 918e0988bb..0ddde5e8a8 100644
--- a/apply.c
+++ b/apply.c
@@ -4732,8 +4732,10 @@ static int apply_patch(struct apply_state *state,
 	}
 
 	if (!list && !skipped_patch) {
-		error(_("unrecognized input"));
-		res = -128;
+		if (!state->allow_empty) {
+			error(_("unrecognized input"));
+			res = -128;
+		}
 		goto end;
 	}
 
@@ -5061,6 +5063,8 @@ int apply_parse_options(int argc, const char **argv,
 		OPT_CALLBACK(0, "directory", state, N_("root"),
 			N_("prepend <root> to all filenames"),
 			apply_option_parse_directory),
+		OPT_BOOL(0, "allow-empty", &state->allow_empty,
+			N_("don't return error for empty patches")),
 		OPT_END()
 	};
 
diff --git a/apply.h b/apply.h
index da3d95fa50..16202da160 100644
--- a/apply.h
+++ b/apply.h
@@ -66,6 +66,7 @@ struct apply_state {
 	int threeway;
 	int unidiff_zero;
 	int unsafe_paths;
+	int allow_empty;
 
 	/* Other non boolean parameters */
 	struct repository *repo;
diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index ceb6a79fe0..f89c53c5f6 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -31,6 +31,22 @@ test_expect_success 'apply empty' '
 	test_cmp expect empty
 '
 
+test_expect_success 'apply empty patch fails' '
+	git reset --hard &&
+	git commit --allow-empty -m "empty commit" &&
+	git format-patch --always HEAD~ >empty.patch &&
+	test_must_fail git apply empty.patch &&
+	test_must_fail git apply - </dev/null
+'
+
+test_expect_success 'apply with --allow-empty succeeds' '
+	git reset --hard &&
+	git commit --allow-empty -m "empty commit" &&
+	git format-patch --always HEAD~ >empty.patch &&
+	git apply --allow-empty empty.patch &&
+	git apply --allow-empty - </dev/null
+'
+
 test_expect_success 'apply --index empty' '
 	git reset --hard &&
 	rm -f missing &&
-- 
2.29.0
Junio C Hamano· Apr 28, 2021, 05:08 UTC · re: Jerry Zhang · lore

Re: [PATCH V2] git-apply: add --allow-empty flag

Jerry Zhang <jerry@skydio.com> writes:
Show 24 quoted lines
> Some users or scripts will pipe "git diff"
> output to "git apply" when replaying diffs
> or commits. In these cases, they will rely
> on the return value of "git apply" to know
> whether the diff was applied successfully.
>
> However, for empty commits, "git apply" will
> fail. This complicates scripts since they
> have to either buffer the diff and check
> its length, or run diff again with "exit-code",
> essentially doing the diff twice.
>
> Add the "--allow-empty" flag to "git apply"
> which allows it to handle both empty diffs
> and empty commits created by "git format-patch
> --always" by doing nothing and returning 0.
>
> Add tests for both with and without --allow-empty.
>
> Signed-off-by: Jerry Zhang <jerry@skydio.com>
> ---
> This patch applies on top of "git-apply: add --quiet flag".
> The conflict is in Documentation -> Synopsis and is
> trivial to solve.
> V1 -> V2:
> - Moved behavior under a flag
> - Added tests for both cases

When people add a flag (a boolean option), it becomes tempting to add a corresponding configuration variable.

I wonder it this step should start calling
	git apply --no-allow-empty

when "git am" drives it, so that a futre end-user configuration variable would not break it?

Jerry Zhang· Apr 28, 2021, 18:40 UTC · re: Junio C Hamano · lore

Re: [PATCH V2] git-apply: add --allow-empty flag

On Tue, Apr 27, 2021 at 10:08 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 42 quoted lines
>
> Jerry Zhang <jerry@skydio.com> writes:
>
> > Some users or scripts will pipe "git diff"
> > output to "git apply" when replaying diffs
> > or commits. In these cases, they will rely
> > on the return value of "git apply" to know
> > whether the diff was applied successfully.
> >
> > However, for empty commits, "git apply" will
> > fail. This complicates scripts since they
> > have to either buffer the diff and check
> > its length, or run diff again with "exit-code",
> > essentially doing the diff twice.
> >
> > Add the "--allow-empty" flag to "git apply"
> > which allows it to handle both empty diffs
> > and empty commits created by "git format-patch
> > --always" by doing nothing and returning 0.
> >
> > Add tests for both with and without --allow-empty.
> >
> > Signed-off-by: Jerry Zhang <jerry@skydio.com>
> > ---
> > This patch applies on top of "git-apply: add --quiet flag".
> > The conflict is in Documentation -> Synopsis and is
> > trivial to solve.
>
> > V1 -> V2:
> > - Moved behavior under a flag
> > - Added tests for both cases
>
> When people add a flag (a boolean option), it becomes tempting to
> add a corresponding configuration variable.
>
> I wonder it this step should start calling
>
>         git apply --no-allow-empty
>
> when "git am" drives it, so that a futre end-user configuration
> variable would not break it?
>

That's currently not necessary, since "git am" can catch empty patches before it ever calls into apply.c:

     if (is_empty_or_missing_file(am_path(state, "patch"))) {
         printf_ln(_("Patch is empty."));
         die_user_resolve(state);
     }

If someone does make a config variable I think it might be convenient to teach "git am" to use "allow-empty" as well, since applying empty commits can be useful for documenting the history. The config variable could then control the behavior of both "am" and "apply" since they are used for very similar things. Of course that's up to the decision of that hypothetical person since I don't currently have a need to change "am".

Ævar Arnfjörð Bjarmason· Apr 29, 2021, 00:32 UTC · re: Jerry Zhang · lore

Re: [PATCH V2] git-apply: add --allow-empty flag

On Tue, Apr 27 2021, Jerry Zhang wrote:
Show 116 quoted lines
> Some users or scripts will pipe "git diff"
> output to "git apply" when replaying diffs
> or commits. In these cases, they will rely
> on the return value of "git apply" to know
> whether the diff was applied successfully.
>
> However, for empty commits, "git apply" will
> fail. This complicates scripts since they
> have to either buffer the diff and check
> its length, or run diff again with "exit-code",
> essentially doing the diff twice.
>
> Add the "--allow-empty" flag to "git apply"
> which allows it to handle both empty diffs
> and empty commits created by "git format-patch
> --always" by doing nothing and returning 0.
>
> Add tests for both with and without --allow-empty.
>
> Signed-off-by: Jerry Zhang <jerry@skydio.com>
> ---
> This patch applies on top of "git-apply: add --quiet flag".
> The conflict is in Documentation -> Synopsis and is
> trivial to solve.
>
> V1 -> V2:
> - Moved behavior under a flag
> - Added tests for both cases
>
>  Documentation/git-apply.txt |  6 +++++-
>  apply.c                     |  8 ++++++--
>  apply.h                     |  1 +
>  t/t4126-apply-empty.sh      | 16 ++++++++++++++++
>  4 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index a32ad64718..b6d77f4206 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -16,7 +16,7 @@ SYNOPSIS
>  	  [--ignore-space-change | --ignore-whitespace]
>  	  [--whitespace=(nowarn|warn|fix|error|error-all)]
>  	  [--exclude=<path>] [--include=<path>] [--directory=<root>]
> -	  [--verbose | --quiet] [--unsafe-paths] [<patch>...]
> +	  [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]
>  
>  DESCRIPTION
>  -----------
> @@ -256,6 +256,10 @@ When `git apply` is used as a "better GNU patch", the user can pass
>  the `--unsafe-paths` option to override this safety check.  This option
>  has no effect when `--index` or `--cached` is in use.
>  
> +--allow-empty::
> +	Don't return error for patches containing no diff. This includes
> +	empty patches and patches with commit text only.
> +
>  CONFIGURATION
>  -------------
>  
> diff --git a/apply.c b/apply.c
> index 918e0988bb..0ddde5e8a8 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -4732,8 +4732,10 @@ static int apply_patch(struct apply_state *state,
>  	}
>  
>  	if (!list && !skipped_patch) {
> -		error(_("unrecognized input"));
> -		res = -128;
> +		if (!state->allow_empty) {
> +			error(_("unrecognized input"));
> +			res = -128;
> +		}
>  		goto end;
>  	}
>  
> @@ -5061,6 +5063,8 @@ int apply_parse_options(int argc, const char **argv,
>  		OPT_CALLBACK(0, "directory", state, N_("root"),
>  			N_("prepend <root> to all filenames"),
>  			apply_option_parse_directory),
> +		OPT_BOOL(0, "allow-empty", &state->allow_empty,
> +			N_("don't return error for empty patches")),
>  		OPT_END()
>  	};
>  
> diff --git a/apply.h b/apply.h
> index da3d95fa50..16202da160 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -66,6 +66,7 @@ struct apply_state {
>  	int threeway;
>  	int unidiff_zero;
>  	int unsafe_paths;
> +	int allow_empty;
>  
>  	/* Other non boolean parameters */
>  	struct repository *repo;
> diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
> index ceb6a79fe0..f89c53c5f6 100755
> --- a/t/t4126-apply-empty.sh
> +++ b/t/t4126-apply-empty.sh
> @@ -31,6 +31,22 @@ test_expect_success 'apply empty' '
>  	test_cmp expect empty
>  '
>  
> +test_expect_success 'apply empty patch fails' '
> +	git reset --hard &&
> +	git commit --allow-empty -m "empty commit" &&
> +	git format-patch --always HEAD~ >empty.patch &&
> +	test_must_fail git apply empty.patch &&
> +	test_must_fail git apply - </dev/null
> +'
> +
> +test_expect_success 'apply with --allow-empty succeeds' '
> +	git reset --hard &&
> +	git commit --allow-empty -m "empty commit" &&

Rather than making two --allow-empty commits, shouldn't this be added to the earlier "setup" routine, so both of these tests can use the same empty commit?

The setup routine sets up a patch0 and patch1, just add an empty-patch ?

And for the "git reset --hard" I see you're copy/pasting the existing convention in that file, but FWIW it's better to use 'test_when_finished "git reset --hard"' in the appropriate spot in the test (i.e. just before we'd need the reset if we fail), so a test cleans up after itself. As opposed to "I'm cleaning in case an earlier test didn't clean up".

Show 8 quoted lines
> +	git format-patch --always HEAD~ >empty.patch &&
> +	git apply --allow-empty empty.patch &&
> +	git apply --allow-empty - </dev/null
> +'
> +
>  test_expect_success 'apply --index empty' '
>  	git reset --hard &&
>  	rm -f missing &&
Jerry Zhang· Dec 11, 2021, 03:13 UTC · re: Jerry Zhang · lore

[PATCH V3] git-apply: add --allow-empty flag

Some users or scripts will pipe "git diff" output to "git apply" when replaying diffs or commits. In these cases, they will rely on the return value of "git apply" to know whether the diff was applied successfully.

However, for empty commits, "git apply" will fail. This complicates scripts since they have to either buffer the diff and check its length, or run diff again with "exit-code", essentially doing the diff twice.

Add the "--allow-empty" flag to "git apply" which allows it to handle both empty diffs and empty commits created by "git format-patch --always" by doing nothing and returning 0.

Add tests for both with and without --allow-empty.
Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
V2->V3:
- moved lines into setup routine
- switched all tests to using "test_when_finished" rather than resetting at the end
 Documentation/git-apply.txt |  6 +++++-
 apply.c                     |  8 ++++++--
 apply.h                     |  1 +
 t/t4126-apply-empty.sh      | 22 ++++++++++++++++++----
 4 files changed, 30 insertions(+), 7 deletions(-)
Show changes to 4 files +30 −7

Documentation/git-apply.txt, apply.c, apply.h, t/t4126-apply-empty.sh

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index a32ad64718..b6d77f4206 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -14,11 +14,11 @@ SYNOPSIS
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
 	  [--ignore-space-change | --ignore-whitespace]
 	  [--whitespace=(nowarn|warn|fix|error|error-all)]
 	  [--exclude=<path>] [--include=<path>] [--directory=<root>]
-	  [--verbose | --quiet] [--unsafe-paths] [<patch>...]
+	  [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]
 
 DESCRIPTION
 -----------
 Reads the supplied diff output (i.e. "a patch") and applies it to files.
 When running from a subdirectory in a repository, patched paths
@@ -254,10 +254,14 @@ running `git apply --directory=modules/git-gui`.
 +
 When `git apply` is used as a "better GNU patch", the user can pass
 the `--unsafe-paths` option to override this safety check.  This option
 has no effect when `--index` or `--cached` is in use.
 
+--allow-empty::
+	Don't return error for patches containing no diff. This includes
+	empty patches and patches with commit text only.
+
 CONFIGURATION
 -------------
 
 apply.ignoreWhitespace::
 	Set to 'change' if you want changes in whitespace to be ignored by default.
diff --git a/apply.c b/apply.c
index 9f00f882a2..f6029fd935 100644
--- a/apply.c
+++ b/apply.c
@@ -4752,12 +4752,14 @@ static int apply_patch(struct apply_state *state,
 		}
 		offset += nr;
 	}
 
 	if (!list && !skipped_patch) {
-		error(_("unrecognized input"));
-		res = -128;
+		if (!state->allow_empty) {
+			error(_("unrecognized input"));
+			res = -128;
+		}
 		goto end;
 	}
 
 	if (state->whitespace_error && (state->ws_error_action == die_on_ws_error))
 		state->apply = 0;
@@ -5081,10 +5083,12 @@ int apply_parse_options(int argc, const char **argv,
 			N_("do not trust the line counts in the hunk headers"),
 			APPLY_OPT_RECOUNT),
 		OPT_CALLBACK(0, "directory", state, N_("root"),
 			N_("prepend <root> to all filenames"),
 			apply_option_parse_directory),
+		OPT_BOOL(0, "allow-empty", &state->allow_empty,
+			N_("don't return error for empty patches")),
 		OPT_END()
 	};
 
 	return parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);
 }
diff --git a/apply.h b/apply.h
index da3d95fa50..16202da160 100644
--- a/apply.h
+++ b/apply.h
@@ -64,10 +64,11 @@ struct apply_state {
 	int apply_with_reject;
 	int no_add;
 	int threeway;
 	int unidiff_zero;
 	int unsafe_paths;
+	int allow_empty;
 
 	/* Other non boolean parameters */
 	struct repository *repo;
 	const char *index_file;
 	enum apply_verbosity apply_verbosity;
diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index ceb6a79fe0..949e284d14 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -7,10 +7,12 @@ test_description='apply empty'
 test_expect_success setup '
 	>empty &&
 	git add empty &&
 	test_tick &&
 	git commit -m initial &&
+	git commit --allow-empty -m "empty commit" &&
+	git format-patch --always HEAD~ >empty.patch &&
 	for i in a b c d e
 	do
 		echo $i
 	done >empty &&
 	cat empty >expect &&
@@ -23,34 +25,46 @@ test_expect_success setup '
 	>empty &&
 	git update-index --refresh
 '
 
 test_expect_success 'apply empty' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply patch0 &&
 	test_cmp expect empty
 '
 
+test_expect_success 'apply empty patch fails' '
+	test_when_finished "git reset --hard" &&
+	test_must_fail git apply empty.patch &&
+	test_must_fail git apply - </dev/null
+'
+
+test_expect_success 'apply with --allow-empty succeeds' '
+	test_when_finished "git reset --hard" &&
+	git apply --allow-empty empty.patch &&
+	git apply --allow-empty - </dev/null
+'
+
 test_expect_success 'apply --index empty' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply --index patch0 &&
 	test_cmp expect empty &&
 	git diff --exit-code
 '
 
 test_expect_success 'apply create' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply patch1 &&
 	test_cmp expect missing
 '
 
 test_expect_success 'apply --index create' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply --index patch1 &&
 	test_cmp expect missing &&
 	git diff --exit-code
 '
 
-- 
2.32.0.1314.g6ed4fcc4cc
Bagas Sanjaya· Dec 11, 2021, 07:36 UTC · re: Jerry Zhang · lore

Re: [PATCH V3] git-apply: add --allow-empty flag

On 11/12/21 10.13, Jerry Zhang wrote:
> +--allow-empty::
> +	Don't return error for patches containing no diff. This includes
> +	empty patches and patches with commit text only.
> +
Looks OK.
Show 9 quoted lines
>   	if (!list && !skipped_patch) {
> -		error(_("unrecognized input"));
> -		res = -128;
> +		if (!state->allow_empty) {
> +			error(_("unrecognized input"));
> +			res = -128;
> +		}
>   		goto end;
>   	}

When we apply empty patch but we forget --allow-empty, we got generic "unrecognized input". So for this case, we should disambiguate the former case and case when malformed patch input is provided.

-- 
An old man doll... just what I always wanted! - Clara
Jerry Zhang· Dec 13, 2021, 20:28 UTC · re: Jerry Zhang · lore

[PATCH V4] git-apply: add --allow-empty flag

Some users or scripts will pipe "git diff" output to "git apply" when replaying diffs or commits. In these cases, they will rely on the return value of "git apply" to know whether the diff was applied successfully.

However, for empty commits, "git apply" will fail. This complicates scripts since they have to either buffer the diff and check its length, or run diff again with "exit-code", essentially doing the diff twice.

Add the "--allow-empty" flag to "git apply" which allows it to handle both empty diffs and empty commits created by "git format-patch --always" by doing nothing and returning 0.

Add tests for both with and without --allow-empty.
Signed-off-by: Jerry Zhang <jerry@skydio.com>
---
V3->V4:
- Add detail to error text, and suggest --allow-empty
in case user runs into the empty patch error.
 Documentation/git-apply.txt |  6 +++++-
 apply.c                     |  8 ++++++--
 apply.h                     |  1 +
 t/t4126-apply-empty.sh      | 22 ++++++++++++++++++----
 4 files changed, 30 insertions(+), 7 deletions(-)
Show changes to 4 files +30 −7

Documentation/git-apply.txt, apply.c, apply.h, t/t4126-apply-empty.sh

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index a32ad64718..b6d77f4206 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -14,11 +14,11 @@ SYNOPSIS
 	  [--allow-binary-replacement | --binary] [--reject] [-z]
 	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
 	  [--ignore-space-change | --ignore-whitespace]
 	  [--whitespace=(nowarn|warn|fix|error|error-all)]
 	  [--exclude=<path>] [--include=<path>] [--directory=<root>]
-	  [--verbose | --quiet] [--unsafe-paths] [<patch>...]
+	  [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]
 
 DESCRIPTION
 -----------
 Reads the supplied diff output (i.e. "a patch") and applies it to files.
 When running from a subdirectory in a repository, patched paths
@@ -254,10 +254,14 @@ running `git apply --directory=modules/git-gui`.
 +
 When `git apply` is used as a "better GNU patch", the user can pass
 the `--unsafe-paths` option to override this safety check.  This option
 has no effect when `--index` or `--cached` is in use.
 
+--allow-empty::
+	Don't return error for patches containing no diff. This includes
+	empty patches and patches with commit text only.
+
 CONFIGURATION
 -------------
 
 apply.ignoreWhitespace::
 	Set to 'change' if you want changes in whitespace to be ignored by default.
diff --git a/apply.c b/apply.c
index 9f00f882a2..afc1c6510e 100644
--- a/apply.c
+++ b/apply.c
@@ -4752,12 +4752,14 @@ static int apply_patch(struct apply_state *state,
 		}
 		offset += nr;
 	}
 
 	if (!list && !skipped_patch) {
-		error(_("unrecognized input"));
-		res = -128;
+		if (!state->allow_empty) {
+			error(_("No valid patches in input (allow with \"--allow-empty\")"));
+			res = -128;
+		}
 		goto end;
 	}
 
 	if (state->whitespace_error && (state->ws_error_action == die_on_ws_error))
 		state->apply = 0;
@@ -5081,10 +5083,12 @@ int apply_parse_options(int argc, const char **argv,
 			N_("do not trust the line counts in the hunk headers"),
 			APPLY_OPT_RECOUNT),
 		OPT_CALLBACK(0, "directory", state, N_("root"),
 			N_("prepend <root> to all filenames"),
 			apply_option_parse_directory),
+		OPT_BOOL(0, "allow-empty", &state->allow_empty,
+			N_("don't return error for empty patches")),
 		OPT_END()
 	};
 
 	return parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);
 }
diff --git a/apply.h b/apply.h
index da3d95fa50..16202da160 100644
--- a/apply.h
+++ b/apply.h
@@ -64,10 +64,11 @@ struct apply_state {
 	int apply_with_reject;
 	int no_add;
 	int threeway;
 	int unidiff_zero;
 	int unsafe_paths;
+	int allow_empty;
 
 	/* Other non boolean parameters */
 	struct repository *repo;
 	const char *index_file;
 	enum apply_verbosity apply_verbosity;
diff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh
index ceb6a79fe0..949e284d14 100755
--- a/t/t4126-apply-empty.sh
+++ b/t/t4126-apply-empty.sh
@@ -7,10 +7,12 @@ test_description='apply empty'
 test_expect_success setup '
 	>empty &&
 	git add empty &&
 	test_tick &&
 	git commit -m initial &&
+	git commit --allow-empty -m "empty commit" &&
+	git format-patch --always HEAD~ >empty.patch &&
 	for i in a b c d e
 	do
 		echo $i
 	done >empty &&
 	cat empty >expect &&
@@ -23,34 +25,46 @@ test_expect_success setup '
 	>empty &&
 	git update-index --refresh
 '
 
 test_expect_success 'apply empty' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply patch0 &&
 	test_cmp expect empty
 '
 
+test_expect_success 'apply empty patch fails' '
+	test_when_finished "git reset --hard" &&
+	test_must_fail git apply empty.patch &&
+	test_must_fail git apply - </dev/null
+'
+
+test_expect_success 'apply with --allow-empty succeeds' '
+	test_when_finished "git reset --hard" &&
+	git apply --allow-empty empty.patch &&
+	git apply --allow-empty - </dev/null
+'
+
 test_expect_success 'apply --index empty' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply --index patch0 &&
 	test_cmp expect empty &&
 	git diff --exit-code
 '
 
 test_expect_success 'apply create' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply patch1 &&
 	test_cmp expect missing
 '
 
 test_expect_success 'apply --index create' '
-	git reset --hard &&
 	rm -f missing &&
+	test_when_finished "git reset --hard" &&
 	git apply --index patch1 &&
 	test_cmp expect missing &&
 	git diff --exit-code
 '
 
-- 
2.32.0.1314.g6ed4fcc4cc
Junio C Hamano· Dec 13, 2021, 21:44 UTC · re: Jerry Zhang · lore

Re: [PATCH V4] git-apply: add --allow-empty flag

Jerry Zhang <jerry@skydio.com> writes:
Show 17 quoted lines
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index a32ad64718..b6d77f4206 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -14,11 +14,11 @@ SYNOPSIS
>  	  [--allow-binary-replacement | --binary] [--reject] [-z]
>  	  [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
>  	  [--ignore-space-change | --ignore-whitespace]
>  	  [--whitespace=(nowarn|warn|fix|error|error-all)]
>  	  [--exclude=<path>] [--include=<path>] [--directory=<root>]
> -	  [--verbose | --quiet] [--unsafe-paths] [<patch>...]
> +	  [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]
>  
>  DESCRIPTION
>  -----------
>  Reads the supplied diff output (i.e. "a patch") and applies it to files.
>  When running from a subdirectory in a repository, patched paths
Applying: git-apply: add --allow-empty flag
error: patch failed: Documentation/git-apply.txt:14
error: Documentation/git-apply.txt: patch does not apply
Patch failed at 0001 git-apply: add --allow-empty flag
Hmph....  Where did that "| --quiet" thing come from?
Jerry Zhang· Dec 13, 2021, 21:52 UTC · re: Junio C Hamano · lore

Re: [PATCH V4] git-apply: add --allow-empty flag

On Mon, Dec 13, 2021 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
>
> Jerry Zhang <jerry@skydio.com> writes:
>
> > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> > index a32ad64718..b6d77f4206 100644
> > --- a/Documentation/git-apply.txt
> > +++ b/Documentation/git-apply.txt
> > @@ -14,11 +14,11 @@ SYNOPSIS
> >         [--allow-binary-replacement | --binary] [--reject] [-z]
> >         [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
> >         [--ignore-space-change | --ignore-whitespace]
> >         [--whitespace=(nowarn|warn|fix|error|error-all)]
> >         [--exclude=<path>] [--include=<path>] [--directory=<root>]
> > -       [--verbose | --quiet] [--unsafe-paths] [<patch>...]
> > +       [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]
> >
> >  DESCRIPTION
> >  -----------
> >  Reads the supplied diff output (i.e. "a patch") and applies it to files.
> >  When running from a subdirectory in a repository, patched paths
>
> Applying: git-apply: add --allow-empty flag
> error: patch failed: Documentation/git-apply.txt:14
> error: Documentation/git-apply.txt: patch does not apply
> Patch failed at 0001 git-apply: add --allow-empty flag
>
> Hmph....  Where did that "| --quiet" thing come from?
>
>

Apologies, I overlooked a dependence on my other patch here: https://patchwork.kernel.org/project/git/patch/20210427194106.14500-1-jerry@skydio.com/

Let me know if you'd like a clean patch off of master. Please give the other patch a look though, as I've addressed outstanding comments.

Junio C Hamano· Dec 13, 2021, 21:57 UTC · re: Jerry Zhang · lore

Re: [PATCH V4] git-apply: add --allow-empty flag

Jerry Zhang <jerry@skydio.com> writes:
Show 36 quoted lines
> On Mon, Dec 13, 2021 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jerry Zhang <jerry@skydio.com> writes:
>>
>> > diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
>> > index a32ad64718..b6d77f4206 100644
>> > --- a/Documentation/git-apply.txt
>> > +++ b/Documentation/git-apply.txt
>> > @@ -14,11 +14,11 @@ SYNOPSIS
>> >         [--allow-binary-replacement | --binary] [--reject] [-z]
>> >         [-p<n>] [-C<n>] [--inaccurate-eof] [--recount] [--cached]
>> >         [--ignore-space-change | --ignore-whitespace]
>> >         [--whitespace=(nowarn|warn|fix|error|error-all)]
>> >         [--exclude=<path>] [--include=<path>] [--directory=<root>]
>> > -       [--verbose | --quiet] [--unsafe-paths] [<patch>...]
>> > +       [--verbose | --quiet] [--unsafe-paths] [--allow-empty] [<patch>...]
>> >
>> >  DESCRIPTION
>> >  -----------
>> >  Reads the supplied diff output (i.e. "a patch") and applies it to files.
>> >  When running from a subdirectory in a repository, patched paths
>>
>> Applying: git-apply: add --allow-empty flag
>> error: patch failed: Documentation/git-apply.txt:14
>> error: Documentation/git-apply.txt: patch does not apply
>> Patch failed at 0001 git-apply: add --allow-empty flag
>>
>> Hmph....  Where did that "| --quiet" thing come from?
>>
>>
> Apologies, I overlooked a dependence on my other patch here:
> https://patchwork.kernel.org/project/git/patch/20210427194106.14500-1-jerry@skydio.com/
>
> Let me know if you'd like a clean patch off of master. Please give the
> other patch
> a look though, as I've addressed outstanding comments.

I am OK with either a single patch without the other one at all, or N-patch series if they all have to go in together.

Thanks.

← back to recent threads