| Message ID | 20210427011246.28054-1-jerry@skydio.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-apply: allow empty patch text | expand |
Jerry Zhang <jerry@skydio.com> wrote: > "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. > 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.
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.
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 &&
"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(-)