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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Apr 27 2021, Jerry Zhang wrote:

> 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".

> +	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 &&