diff mbox series

git-apply: allow empty patch text

Message ID 20210427011246.28054-1-jerry@skydio.com (mailing list archive)
State New
Headers show
Series git-apply: allow empty patch text | expand

Commit Message

Jerry Zhang April 27, 2021, 1:12 a.m. UTC
"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(-)

Comments

Eric Wong April 27, 2021, 5:46 a.m. UTC | #1
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.
Junio C Hamano April 27, 2021, 6:28 a.m. UTC | #2
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 mbox series

Patch

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