Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver
Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> On Sat, Sep 08, 2012 at 09:40:39PM -0700, Junio C Hamano wrote:
>
>> The built-in "binary" attribute macro expands to "-diff -text", so
>> that textual diff is not produced, and the contents will not go
>> through any CR/LF conversion ever. During a merge, it should also
>> choose the "binary" low-level merge driver, but it didn't.
>>
>> Make it expand to "-diff -merge -text".
>
> Yeah, that seems like the obviously correct thing to do. In practice,
> most files would end up in the first few lines of ll_xdl_merge checking
> buffer_is_binary anyway, so I think this would really only make a
> difference when our "is it binary?" heuristic guesses wrong.
You made me look at that part again and then made me notice something unrelated.
if (buffer_is_binary(orig->ptr, orig->size) ||
buffer_is_binary(src1->ptr, src1->size) ||
buffer_is_binary(src2->ptr, src2->size)) {
warning("Cannot merge binary files: %s (%s vs. %s)",
path, name1, name2);
return ll_binary_merge(drv_unused, result,
path,
orig, orig_name,
src1, name1,
src2, name2,
opts, marker_size);
}Given that we now may know how to merge these things, the unconditional warning feels very wrong.
Perhaps something like this makes it better.
A path that is explicitly marked as binary did not get any such warning, but it will start to get warned just like a path that was auto-detected to be a binary.
It is a behaviour change, but I think it is a good one that makes two cases more consistent.
And we won't see the warning when -Xtheirs/-Xours large sledgehammer is in use, which tells us how to resolve these things "cleanly".
ll-merge.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git i/ll-merge.c w/ll-merge.c
index 8535e2d..307315b 100644
--- i/ll-merge.c
+++ w/ll-merge.c
@@ -35,7 +35,7 @@ struct ll_merge_driver {
*/
static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
mmbuffer_t *result,
- const char *path_unused,
+ const char *path,
mmfile_t *orig, const char *orig_name,
mmfile_t *src1, const char *name1,
mmfile_t *src2, const char *name2,
@@ -53,6 +53,9 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,
} else {
switch (opts->variant) {
default:
+ warning("Cannot merge binary files: %s (%s vs. %s)\n",
+ path, name1, name2);
+ /* fallthru */
case XDL_MERGE_FAVOR_OURS:
stolen = src1;
break;
@@ -88,8 +91,6 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,
if (buffer_is_binary(orig->ptr, orig->size) ||
buffer_is_binary(src1->ptr, src1->size) ||
buffer_is_binary(src2->ptr, src2->size)) {
- warning("Cannot merge binary files: %s (%s vs. %s)\n",
- path, name1, name2);
return ll_binary_merge(drv_unused, result,
path,
orig, orig_name,
