Re: [PATCH V2] git-apply: add --allow-empty flag
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 29, 2021, 00:32 UTC
- Message-ID
- <878s52ylko.fsf@evledraar.gmail.com>
- In-Reply-To
- <20210427194049.14399-1-jerry@skydio.com>
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 &&