| Message ID | 20210427194049.14399-1-jerry@skydio.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] git-apply: add --allow-empty flag | expand |
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?
On Tue, Apr 27, 2021 at 10:08 PM Junio C Hamano <gitster@pobox.com> wrote: > > 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".
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 &&
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 &&
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(-)