Why is there a --binary option needed for git-apply?

Why is there a --binary option needed for git-apply?

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

I was recently emailed a patch that introduced some new binary files
to my repository. The patch included a very pleasant-looking chunk
along the lines of:

	diff --git a/new/file.png b/new/file.png
	new file mode 100644
	index 0000000000000000000000000000000000000000..e7edd8141e15f5753ea94244e7315bd1341a8c05
	GIT binary patch

I tried applying the patch with git-apply (1.4.2.rc2.gef1d9) and
received an inscrutable error:

	fatal: patch with only garbage at line 90

Where line 90 happened to be the line after the first chunk.

I was disappointed that the operation had failed and started guessing
at problem causes (git version incompatibilities? MUA whitespace
munging?).

Shawn Pearce was kind enough to direct me to the --binary option for
git-apply which solved my problem. But that left me wondering why
git-apply requires this extra command-line option to do its
job. Shouldn't git-apply simply apply the patch it is given?

If there is some reason for git-apply to only apply binary patches
when under the duress of --binary, then at the very least it could use
a better error message explaining the situation.

-Carl
--
cworth@redhat.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Why is there a --binary option needed for git-apply?

[Shawn Pearce]

Carl Worth <cworth@cworth.org> wrote:
> Shawn Pearce was kind enough to direct me to the --binary option for
> git-apply which solved my problem. But that left me wondering why
> git-apply requires this extra command-line option to do its
> job. Shouldn't git-apply simply apply the patch it is given?
> 
> If there is some reason for git-apply to only apply binary patches
> when under the duress of --binary, then at the very least it could use
> a better error message explaining the situation.

I see no reason why git-apply shouldn't always have --binary enabled.
If the patch contains full pre-image/post-image blob IDs and we have
an exact match against the pre-image and we have the post-image
in our tree it should just apply even if the user didn't supply
--binary.  If the patch contains a binary delta and we have an
exact match against the pre-image it should also just apply.

But if there's a binary hunk and we lack the full pre/post image blob
IDs, we lack the post image and there's no detla, or the pre-image
doesn't exactly match then we should obviously still abort with a
reasonable error message as there's no sane course of action to take.

-- 
Shawn.

Re: Why is there a --binary option needed for git-apply?

[Junio C Hamano]

Carl Worth <cworth@cworth.org> writes:

> Shawn Pearce was kind enough to direct me to the --binary option for
> git-apply which solved my problem. But that left me wondering why
> git-apply requires this extra command-line option to do its
> job. Shouldn't git-apply simply apply the patch it is given?

There is no reason it shouldn't.

Initially it was done that way only because doing something
unusual should be done under explicit user permission.  Binary
patch is not "unusual" anymore, I guess ;-).

-- >8 --
[PATCH] Make apply --binary a no-op.

Historically we did not allow binary patch applied without an
explicit permission from the user, and this flag was the way to
do so.  This makes the flag a no-op by always allowing binary
patch application.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 Documentation/git-apply.txt |   13 ++++---------
 builtin-apply.c             |   19 +++++--------------
 t/t4103-apply-binary.sh     |    4 ++--
 3 files changed, 11 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index c76cfff..0a6f7b3 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -110,15 +110,10 @@ OPTIONS
 	deletion part but not addition part.
 
 --allow-binary-replacement, --binary::
-	When applying a patch, which is a git-enhanced patch
-	that was prepared to record the pre- and post-image object
-	name in full, and the path being patched exactly matches
-	the object the patch applies to (i.e. "index" line's
-	pre-image object name is what is in the working tree),
-	and the post-image object is available in the object
-	database, use the post-image object as the patch
-	result.  This allows binary files to be patched in a
-	very limited way.
+	Historically we did not allow binary patch applied
+	without an explicit permission from the user, and this
+	flag was the way to do so.  Currently we always allow binary
+	patch application, so this is a no-op.
 
 --exclude=<path-pattern>::
 	Don't apply changes to files matching the given path pattern. This can
diff --git a/builtin-apply.c b/builtin-apply.c
index 872c800..6e0864c 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -28,7 +28,6 @@ static int prefix_length = -1;
 static int newfd = -1;
 
 static int p_value = 1;
-static int allow_binary_replacement;
 static int check_index;
 static int write_index;
 static int cached;
@@ -1228,14 +1227,12 @@ static int parse_chunk(char *buffer, uns
 			}
 		}
 
-		/* Empty patch cannot be applied if:
-		 * - it is a binary patch and we do not do binary_replace, or
-		 * - text patch without metadata change
+		/* Empty patch cannot be applied if it is a text patch
+		 * without metadata change.  A binary patch appears
+		 * empty to us here.
 		 */
 		if ((apply || check) &&
-		    (patch->is_binary
-		     ? !allow_binary_replacement
-		     : !metadata_changes(patch)))
+		    (!patch->is_binary && !metadata_changes(patch)))
 			die("patch with only garbage at line %d", linenr);
 	}
 
@@ -1676,11 +1673,6 @@ static int apply_binary(struct buffer_de
 	unsigned char hdr[50];
 	int hdrlen;
 
-	if (!allow_binary_replacement)
-		return error("cannot apply binary patch to '%s' "
-			     "without --allow-binary-replacement",
-			     name);
-
 	/* For safety, we require patch index line to contain
 	 * full 40-byte textual SHA1 for old and new, at least for now.
 	 */
@@ -2497,8 +2489,7 @@ int cmd_apply(int argc, const char **arg
 		}
 		if (!strcmp(arg, "--allow-binary-replacement") ||
 		    !strcmp(arg, "--binary")) {
-			allow_binary_replacement = 1;
-			continue;
+			continue; /* now no-op */
 		}
 		if (!strcmp(arg, "--numstat")) {
 			apply = 0;
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index ff05269..e2b1124 100755
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -94,11 +94,11 @@ test_expect_failure 'apply binary diff (
 	'do_reset
 	 git-apply --index C.diff'
 
-test_expect_failure 'apply binary diff without replacement -- should fail.' \
+test_expect_success 'apply binary diff without replacement.' \
 	'do_reset
 	 git-apply BF.diff'
 
-test_expect_failure 'apply binary diff without replacement (copy) -- should fail.' \
+test_expect_success 'apply binary diff without replacement (copy).' \
 	'do_reset
 	 git-apply CF.diff'
 
-- 
1.4.2.gfce41