threads / patch / 8431

patch, 3 partsgit-merge-file: refuse to merge binary files

Subject: [PATCH 3/3] git-merge-file: refuse to merge binary files

## tl;dr

5 messages between Jun 5, 2007 and Jun 5, 2007. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Johannes Schindelin· Jun 5, 2007, 02:37 UTC · lore

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-merge-file.c  |    6 +++++-
 t/t6023-merge-file.sh |    5 +++++
 2 files changed, 10 insertions(+), 1 deletions(-)

Show changes to 2 files +10 −1

builtin-merge-file.c, t/t6023-merge-file.sh

diff --git a/builtin-merge-file.c b/builtin-merge-file.c
index 9135773..10ec63b 100644
--- a/builtin-merge-file.c
+++ b/builtin-merge-file.c
@@ -36,9 +36,13 @@ int cmd_merge_file(int argc, char **argv, char **envp)
 	for (; i < 3; i++)
 		names[i] = argv[i + 1];
 
-	for (i = 0; i < 3; i++)
+	for (i = 0; i < 3; i++) {
 		if (read_mmfile(mmfs + i, argv[i + 1]))
 			return -1;
+		if (buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
+			return error("Cannot merge binary files: %s\n",
+					argv[i + 1]);
+	}
 
 	ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
 			&xpp, XDL_MERGE_ZEALOUS, &result);
diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index c76fccf..1decbfb 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -134,5 +134,10 @@ EOF
 
 test_expect_success "expected conflict markers" "git diff expect out"
 
+test_expect_success 'binary files cannot be merged' '
+	! git merge-file -p orig.txt ../test4012.png new1.txt 2> merge.err &&
+	grep "Cannot merge binary files" merge.err
+'
+
 test_done

-- 
1.5.2.1.2626.ge1044-dirty

Linus Torvalds· Jun 5, 2007, 04:00 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] git-merge-file: refuse to merge binary files

I really think this would be better off using the crlf-like heuristics.

It's entirely possible that the "NUL character in the first X bytes" heuristic is wrong for some cases, so I find it a bit nasty to hardcoding it as the only rule for "it must be binary"

		Linus

Johannes Schindelin· Jun 5, 2007, 05:11 UTC · re: Linus Torvalds · lore

Re: [PATCH 3/3] git-merge-file: refuse to merge binary files

Hi,

On Mon, 4 Jun 2007, Linus Torvalds wrote:

> I really think this would be better off using the crlf-like heuristics.

I don't know. Given that it took quite a while to actually hit this bug...

> It's entirely possible that the "NUL character in the first X bytes" 
> heuristic is wrong for some cases, so I find it a bit nasty to 
> hardcoding it as the only rule for "it must be binary"

Yes, you're right.

However, if you call git-merge-file, you have to be aware of what it does. It is a _standalone_ program, meant to make it easier to write scripts. Maybe it is not worth detecting binary files _at all_ in merge-file.

Ciao, Dscho

Junio C Hamano· Jun 5, 2007, 05:16 UTC · re: Linus Torvalds · lore

Re: [PATCH 3/3] git-merge-file: refuse to merge binary files

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 7 quoted lines

> I really think this would be better off using the crlf-like heuristics.
>
> It's entirely possible that the "NUL character in the first X bytes" 
> heuristic is wrong for some cases, so I find it a bit nasty to hardcoding 
> it as the only rule for "it must be binary"
>
> 		Linus

On the other hand that hardcoded fallback is consistent with diff, which is closely related to xdl_merge(). Attributes can be specified to override this anyway, so...

Junio C Hamano· Jun 5, 2007, 05:16 UTC · re: Johannes Schindelin · lore

Re: [PATCH 3/3] git-merge-file: refuse to merge binary files

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 25 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin-merge-file.c  |    6 +++++-
>  t/t6023-merge-file.sh |    5 +++++
>  2 files changed, 10 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-merge-file.c b/builtin-merge-file.c
> index 9135773..10ec63b 100644
> --- a/builtin-merge-file.c
> +++ b/builtin-merge-file.c
> @@ -36,9 +36,13 @@ int cmd_merge_file(int argc, char **argv, char **envp)
>  	for (; i < 3; i++)
>  		names[i] = argv[i + 1];
>  
> -	for (i = 0; i < 3; i++)
> +	for (i = 0; i < 3; i++) {
>  		if (read_mmfile(mmfs + i, argv[i + 1]))
>  			return -1;
> +		if (buffer_is_binary(mmfs[i].ptr, mmfs[i].size))
> +			return error("Cannot merge binary files: %s\n",
> +					argv[i + 1]);
> +	}
>  
>  	ret = xdl_merge(mmfs + 1, mmfs + 0, names[0], mmfs + 2, names[2],
>  			&xpp, XDL_MERGE_ZEALOUS, &result);

Oh, my.

I wonder if we can expose ll_merge() from merge-recursive to this part, instead of raw xdl_merge(). That would give you the custom merge driver the user may have specified in .gitattributes.