git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] attr: "binary" attribute should choose built-in "binary" merge driver

From
Jeff King <peff@peff.net>
Date
Sep 12, 2012, 13:17 UTC
Message-ID
<20120912131702.GA13710@sigill.intra.peff.net>
In-Reply-To
<7vk3vzfwme.fsf@alter.siamese.dyndns.org>
On Wed, Sep 12, 2012 at 01:55:53AM -0700, Junio C Hamano wrote:
Show 35 quoted lines
> > 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".

Yeah, I think it is the right thing to do. I noticed that the warning would not trigger in the "-merge" case and wondered if it should, but figured it was not a big deal either way.

However, I agree it is very bad for it to trigger with -Xours/theirs, and that is worth fixing. That it triggers in the "-merge" case afterwards is a slight bonus.

-Peff
Previous: Stephen BashNext: Stephen Bash
Message 11 of 12 in “Binary file-friendly merge -Xours or -Xtheirs?”
  1. Stephen BashSep 7, 2012
  2. Junio C HamanoSep 7, 2012
  3. 0/2 Teaching -Xours/-Xtheirs to binary ll-merge driverJunio C Hamano, Sep 9, 2012
  4. 1/2 merge: teach -Xours/-Xtheirs to binary ll-merge driverJunio C Hamano, Sep 9, 2012
  5. 2/2 attr: "binary" attribute should choose built-in "binary" merge driverJunio C Hamano, Sep 9, 2012
  6. Jeff KingSep 10, 2012
  7. Junio C HamanoSep 12, 2012
  8. Stephen BashSep 12, 2012
  9. Junio C HamanoSep 12, 2012
  10. Stephen BashSep 12, 2012
  11. Jeff KingSep 12, 2012
  12. Stephen BashSep 9, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.