combined diff does not detect binary files and ignores -diff attribute

In a merge commit, I added some PNGs (so they were new to both parents). I was surprised when I did a "git show" on the commit and had the binary bits spewed to the terminal.

I thought it was just git not detecting the PNG as a binary file, but adding "*.png -diff" to .git/info/attributes made no difference.

Just reporting this for now, as I don't have time to investigate.

j.

Jay Soffian venit, vidit, dixit 22.05.2011 22:12:

We know, but we somehow got stuck, see:

http://permalink.gmane.org/gmane.comp.version-control.git/171613

I don't have the time to follow up on this currently, it got out of proportion.

Michael

On Mon, May 23, 2011 at 9:30 AM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Drat, seems like the perfect being the enemy of the good there. I wonder if Junio's patch in that thread isn't good enough for now.

Thanks,

j.

Here is a quick summary of what I have in the "talked about, sounded interesting, and never got completed" list.

gmane=http://thread.gmane.org/gmane.comp.version-control.git/

* Teach pack protocol to transfer estimated pack size and history
  depth to allow receiving end make more intelligent decision between
  unpack-objects and index-pack.

  $gmane/173610

* The "combined" diff always assumes it deals with text files. Teach it
  to punt on binary and also use the textconv.

  $gname/171613

* Audit use of symbolic-ref without -m in our scripts and for each
  case decide if leaving a reflog entry for the HEAD is desirable.
  If so, add them.

  $gmane/172516

* "git status" on intent-to-add index entries (say "I" in the first
  column instead of "A" for short status, add "(needs 'git add')" at the
  end of "new file: $path " in long status).

  $gmane/170658

* synopsys: use {} instead of () for grouping alternatives (Jari Aalto)
  $gmane/72243

* "[alias] st = status" and "cd .git && git st" (Jeff King)
  $gmane/72327

On Mon, May 23, 2011 at 11:17:14AM -0400, Jay Soffian wrote:

I'll have a patch series in a few minutes that at least handles the binary case. I'll see how painful the textconv bit is on top of that.

-Peff

On Mon, May 23, 2011 at 02:11:47PM -0400, Jeff King wrote:

It turned out not too bad:

  [1/5]: combine-diff: split header printing into its own function
  [2/5]: combine-diff: calculate mode_differs earlier
  [3/5]: combine-diff: handle binary files as binary
  [4/5]: refactor get_textconv to not require diff_filespec
  [5/5]: combine-diff: respect textconv attributes

-Peff

This is a pretty big logical chunk, so it makes the function a bit more readable to have it split out. In addition, it will make it easier to add an alternate code path for binary diffs in a future patch.

Signed-off-by: Jeff King <peff@peff.net>
---
 combine-diff.c |  135 ++++++++++++++++++++++++++++++-------------------------
 1 files changed, 74 insertions(+), 61 deletions(-)

One loop combined both the patch generation and checking whether there was any mode change to report. Let's factor that into two separate loops, as we may care about the mode change even if we are not generating patches (e.g., because we are showing a binary diff, which will come in a future patch).

Signed-off-by: Jeff King <peff@peff.net>
---
 combine-diff.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

The combined diff code path is totally different from the regular diff code path, and didn't handle binary files at all. The results of a combined diff on a binary file could range from annoying (since we spewed binary garbage, possibly upsetting the user's terminal), to wrong (embedded NULs caused us to show incorrect diffs, with lines truncated at the NUL character), to potential security problems (embedded NULs could interfere with "-z" output, possibly defeating policy hooks which parse diff output).

Instead, we consider a combined diff to be binary if any of the input blobs is binary. To show a binary combined diff, we indicate "Binary blobs differ"; the "index" meta line will show which parents had which blob.

Signed-off-by: Jeff King <peff@peff.net>
---
The big question here is what we want the output to look like. I chose
this:

  diff --combined foo
  index 7ea6ded,6197570..9563691
  Binary files differ

which contains all of the information we have: for file "foo" there were two parents (at 7ea6ded and 6197570 respectively), and the resolution was 9563691. If you want to see full sha1s, you can use --full-index. This format maps very closely to the non-combined case, which looks like:

  diff --git a/foo b/foo
  index 6197570..9563691 100644
  Binary files a/foo and b/foo differ

Two other obvious options for format would be:

  1. A combined diff of fake text, like:

     - Binary blob 7ea6ded
      -Binary blob 6197570
     ++Binary blob 9563691

    But this contains no additional information over the index line, and
    I worry that it will be less obvious to readers what is going on.

  2. The --raw format, which looks like:

     ::100644 100644 100644 7ea6ded... 6197570... 9563691... MM binary

     but again, there's really no extra information there (the modes
     are there, but in the case of changed modes, we already produce a
     separate mode line like "mode 100644,100755 100755". And I
     personally find it way less readable (it's more compact, which is
     good for actual --raw mode, but in patch mode we are already
     spending quite a few lines per file).

So I think what I have is succint, complete, and matches the non-combined case as well as we can.

 combine-diff.c                  |   37 ++++++++++++-
 t/t4048-diff-combined-binary.sh |  113 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+), 2 deletions(-)
 create mode 100755 t/t4048-diff-combined-binary.sh

Jeff King <peff@peff.net> writes:

Yeah, I think the above is sane and matches more-or-less the conclusion of the previous discussion Michael quoted.

I've only read up to this patch (I wanted to conclude the first round of today's integration round before 17:00 local) and they all looked good; will queue the whole thing to 'pu' in the meantime.

On Mon, May 23, 2011 at 04:02:58PM -0700, Junio C Hamano wrote:

Thanks. I think there is a minor tweak to 5/5 that I posted elsewhere in the thread. I'll do more testing with it and post tomorrow. I'm done gitting for today.

-Peff

Jeff King <peff@peff.net> writes:

Two comments.

 - This loop will grab the blob from all parents just to peek and discard
   for most of commits. It feels somewhat wasteful, especially because
   "binary" that is not marked as binary is a rare minor case (most of the
   paths are text). Stopping immediately at the first binary blob does not
   optimize the loop even though it is better than not having one.

 - It may make sense to compare [i].sha1 with earlier [j].sha1 (j < i) and
   avoid grab_blob() altogether?  Cf. 3c39e9b (combine-diff: reuse diff
   from the same blob., 2006-02-01).

On Sun, May 29, 2011 at 11:33:38PM -0700, Junio C Hamano wrote:

Yeah, I am not too happy with that bit. Our choices are:

  1. Grab each blob, check binary-ness, and free. This double-loads in
     the common, non-binary case.

  2. Grab each blob, check binary-ness, and keep it in memory. This
     means using N times as much memory, where N is the number of
     parents. In practice, N generally equals 2, and the file sizes
     aren't gigantic, so it's not that bad. But there are some corner
     cases.

  3. Choose (1) or (2) based on file size, or based on the number of
     parents. The problem cases for (2) are going to be big files
     (bigger than bigFileThreshold?), and gigantic numbers of parents.

I'd also eventually like to do a "peek" binary-ness check on top of your streaming work, as I showed for the non-combined diff case (speaking of which, I need to polish that now that the streaming series seems to have settled). And we would probably want to peek only in the big file case.

I'll try to take a look at it this week and get some measurements on (1) versus (2) for both speed and peak memory usage. And then see if I can do better with (3), and implement the "peek" solution both here and in regular diff.

Yeah, that is probably worth doing, as it collapses the "N" above into "how many parents all modified the same file". I'll add it to my list to measure.

-Peff

On Mon, May 30, 2011 at 10:36:27AM -0400, Jeff King wrote:

I was curious about this, so I stole a few minutes to do some preliminary benchmarks this morning.

The first thing to look at is the performance of the original code, that does not check binary-ness at all. It's going to represent the best we can do with any strategy. So I tried:

  git log -p --cc --merges origin/master

on git.git using both v1.7.5.3 and the jk/combine-diff-binary-etc branch. And it turns out that the extra loads really don't make a difference in practice. My best-of-5 for the two cases were:

  $ time git.v1.7.5.3 log -p --cc --merges origin/master >/dev/null
  real    0m59.518s
  user    0m58.672s
  sys     0m0.688s

  $ time git.jk.binary-combined-diff log -p --cc \
      --merges origin/master >/dev/null
  real    0m58.949s
  user    0m58.220s
  sys     0m0.572s

The new code actually came out slightly faster. One reason may be that there are 3 combined diffs of git-gui/lib/git-gui.ico that we avoid doing (and just say "Binary files differ"). That's not a lot, but it gives us a very tiny edge (though that edge is very close to the amount of noise between runs). Still, I think it implies that the extra loads in the common non-binary case are not actually measurable.

The peak memory use between the two should be the same (since we free each blob immediately), but I didn't measure it.

So I think in practice it's not a big deal. I'll still take a look at the "peek" optimization later this week, since that can make a difference in some corner cases. And as part of that, it will probably make sense to keep the buffers around for small-ish files, so we'll get the optimization I mentioned more or less for free. I'll also do the check for duplicated sha1s that you mentioned.

-Peff

Jeff King <peff@peff.net> writes:

I am not surprised. git.git is small enough and developer machines these days are beefy enough so that most of the data from the disk are sitting in the buffer cache, and if you are well packed and Linus's law "a file always grows" holds true, it is likely that you are also benefiting from the delta-base cache, as the history traversal of the command goes newer to older.

Comparing that with the run time of the same log without patch generation would roughly give the cost of "all text combined" diff. I suspect more than 90% of the time goes to running the textual diff algorithm.

I guess you are hinting that we should optimize that part before worrying about the wastage of reading these blobs always twice.

Jeff King <peff@peff.net> writes:

I merely put it on the back burner, not necessarily declaring it as "settled".

I just created a pair of test repository with a single 800M binary junk file. One repository has it as a single loose object file, and the other repository has it in a packfile. The path is not explicitly marked with any attributes, but I have no CRLF funniness configured, so streaming would be used but there won't be any filtering involved.

Removing the file from the working tree and then checking it out of the index gave me a pleasant surprise. I originally did this only to help smaller machines that cannot comfortably fit inflated blob data in core, but it turns out that streaming write seems to perform better.

These are the typical /usr/bin/time output:

  $ /usr/bin/time git checkout a ;# w/o streaming
  1.39user 2.23system 0:03.62elapsed 99%CPU (0avgtext+0avgdata 6297056maxresident)k
  0inputs+1572872outputs (0major+399828minor)pagefaults 0swaps

  $ /usr/bin/time git checkout a ;# w/ streaming
  1.35user 1.22system 0:02.52elapsed 101%CPU (0avgtext+0avgdata 3151536maxresident)k
  0inputs+1572872outputs (0major+203226minor)pagefaults 0swaps

So now I can say I am reasonably happy with the series, but I haven't really exercised the code heavily, so there may still be bugs lurking ;-).

This function actually does two things:

  1. Load the userdiff driver for the filespec.

  2. Decide whether the driver has a textconv component, and
     initialize the textconv cache if applicable.

Only part (1) requires the filespec object, and some callers may not have a filespec at all. So let's split them it into two functions, and put part (2) with the userdiff code, which is a better fit.

Signed-off-by: Jeff King <peff@peff.net>
---
It's a little funny to have "userdiff_get_textconv(foo)" return either
NULL or "foo"; you are not really getting the textconv but rather seeing
if there is a textconv available, and if so, preparing it. But that is
not a new thing; get_textconv was already like that.

One possible refactoring would be that the textconv (including the cache) should be a sub-structure of the userdiff driver. I chose not to do that refactoring because of the amount of noise it would introduce in diff.c. But I can do it if we care.

 diff.c     |   14 +-------------
 userdiff.c |   17 +++++++++++++++++
 userdiff.h |    2 ++
 3 files changed, 20 insertions(+), 13 deletions(-)

When doing a combined diff, we did not respect textconv attributes at all. This generally lead to us printing "Binary files differ" when we could show a combined diff of the converted text.

This patch converts file contents according to textconv attributes. The implementation is slightly ugly; because the textconv code is tightly linked with the diff_filespec code, we temporarily create a diff_filespec during conversion. In practice, though, this should not create a performance problem.

Signed-off-by: Jeff King <peff@peff.net>
---
 combine-diff.c                  |   41 +++++++++++++----
 t/t4048-diff-combined-binary.sh |   99 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 131 insertions(+), 9 deletions(-)

Jeff King <peff@peff.net> writes:

I thought your 4/5 talked something about not requiring filespec for textconv handling...? Is it still yuck?

On Mon, May 23, 2011 at 03:47:40PM -0700, Junio C Hamano wrote:

Yeah, I removed the requirement for get_textconv to need a filespec. Which is nice, because it means the non-textconv codepath doesn't deal with this at all.

But removing the filespec from fill_textconv is much nastier, because it relies on diff.c's prepare_temp_file, which also uses the filespec.

So it's still yuck, but limited to this little conditional. It would be nice if we used filespecs all the way through the combine-diff code path, but that was a bit more refactoring than I was up for today.

-Peff

Jeff King <peff@peff.net> writes:

After reading this patch again, I think this aversion to diff_filespec is probably unjustified. It is primarily a structure that records what path has which blob by recording its object name and what mode, and also in-core data when we make the contents available. There are small diff specific data associated with it, but that could be separated out if you really wanted to, perhaps like:

	struct diff_filespec {
        	struct filespec {
			... the generic and essential part ...
                } spec;
                /* diff specific part follows */
                const char *funcname_pattern_ident;
                int xfrm_flags;
                int rename_used;
                unsigned dirty_submodule :2;
                struct userdiff_driver *driver;
                int is_binary;
	};

and let most users use "struct filespec".

If anything else, we should be using the type in _more_ codepaths that are not diff related but want to represent a path with its contents in the git namespace (be it from working tree, index or a tree), not less, and in the longer term weaken functions like fill_textconv() that take diff_filespec to take filespec so that they can be made more reusable.

On Tue, May 24, 2011 at 09:20:53AM -0700, Junio C Hamano wrote:

It's not an aversion to diff_filespec; it's an aversion to only half buying-into diff_filespec. I think the best thing would be rewriting all of combine-diff in terms of diff_filespec; I just started to do it and it looked big and ugly for not much gain.

It's a little ugly to have to manually do the conversion into another data structure just to call a function like fill_textconv, but I can live with it. What I worry about more is that the ad-hoc use of a structure like diff_filespec means we are violating some assumption that other code has about the data structure (e.g., the bug Jay already uncovered that we must have a valid name in the "path" field).

I think what is there now is correct; it's just that as a general rule, switching data formats or abstractions in the middle of code makes me feel I'm doing something wrong (or that the code interfaces need to be refactored).

Yeah, I would agree with that.

-Peff

On Mon, May 23, 2011 at 4:15 PM, Jeff King <peff@peff.net> wrote:

Tested-by: Jay Soffian <jaysoffian@gmail.com>

In a real-world merge, png's were correctly shown as "Binary files differ". I also tested with "*.xib -diff" and that worked as expected.

However, custom diff drivers (still) don't work. :-)

Also read the patches and they LGTM.

Didn't test explicitly, but don't see anything obviously wrong.

Thanks Peff,

j.

On Mon, May 23, 2011 at 6:55 PM, Jay Soffian <jaysoffian@gmail.com> wrote:

Okay, tested these now. I noticed two tiny issues unrelated to this series, but rather since this is the first time I've used textconv.

1. The temporary path git generates loses the original file extension.
At least in the case of Mac OS X, ibtool gets angry when the ".xib"
isn' there. I worked around this with a wrapper, but maybe git could
preserve the extension.

2. It's not really clear to me when one should use textconv vs a
custom diff driver. (Also, --no-textconv isn't documented...).

Thanks,

j.

On Mon, May 23, 2011 at 07:31:02PM -0400, Jay Soffian wrote:

Hmm, I thought we did preserve the extension. I think it may actually be related to me making a fake filespec and not passing the path in.

Does the (totally untested) patch below fix it for you?

The motivation for textconv was originally a combination of "when you are too lazy to write a full diff driver" and "when you like how git formats the diff with pretty colors and word-diff". But now I think we can add to that:

  1. When you want something fancy and git-ish like combined diff.

  2. When you want free caching to speed up repeated log viewing (try
     setting diff.*.cachetextconv to true).

Maybe it is worth adding an "advantages of textconv" section to gitattributes(5) (the advantage of a full external diff command is that it can be more flexible, or even graphical in nature).

Anyway, here's the patch.

---

On Mon, May 23, 2011 at 7:49 PM, Jeff King <peff@peff.net> wrote:

Yes.

Gotcha.

Okay.

j.

On Mon, May 23, 2011 at 06:55:07PM -0400, Jay Soffian wrote:

Yeah, I didn't add any support for that. I'm not sure what it should do; custom diff drivers don't know how to handle combined diff, do they?

If you write me a test case that explains what _should_ happen, I'll see what I can do. :)

-Peff

Jeff King <peff@peff.net> writes:

I do not think it is sensible to expect anybody to come up with a sane semantics for combined diff to work with GIT_EXTERNAL_DIFF (and external diff driver that can be specified via the attributes mechanism) in any meaningful way.

The whole point of the external diff mechanism is that an external command can take _two_ files and represent the change between them in a way that is more suited for the need of the user than the patch form. The output from such an external command does not have any obligation to even follow the convention used by the patch output, namely:

  @@ from here to there things have changed @@
   this is common
  -this was the removed content
  +this is the new content

as the _whole_ point of the external diff mechanism is to give something that is _different_ from the patch form, in the hope that it is in a more appropriate form for whoever consumes the output.

On the other hand, combined diff is all about combining multiple patches show them side-by-side in a combined fashion. Without the above four kinds of cues, there is no way to even _align_ the change outputs from two parents, let alone _combining_ them.

Anybody interested can check "compare-cooking.perl" in the todo branch, which is used as an external diff driver to view the differences between "What's cooking" postings via these:

    [diff "whatscooking"]
            xfuncname = "^\\[(.*)\\]$"
            command = ./compare-cooking.perl

in the .git/config file, together with

    whats-cooking.txt diff=whatscooking

in the .gitattributes file. Running

    $ git log -p --ext-diff todo -- whats-cooking.txt

would give a sample output.

It is conceivable that we _could_ newly define a "combined external diff driver" that would take 3 or more files, and compute and show the combined result by itself, but that will certainly not go through the codepath you touched with the textconv patch. Calling out to such a new type of external diff driver would have to happen at the level where we have 1+N blob object names for a N-parent commit, namely, at the beginning of show_patch_diff(), bypassing the entire contents of that function and instead letting the new n-way external diff driver do everything.

I however highly doubt that such an interface would make sense. For example, what would be the desirable format to compare three versions of "What's cooking" postings, and how would the updated compare-cooking.perl script would look like?

First of all:

Jeff, thanks a bunch for taking this up again! That's a great improvement. (I'm not sure I can devote enough time to reviewing, but I'll see.)

Junio C Hamano venit, vidit, dixit 24.05.2011 06:46:

Yeah, currently --cc with external makes no sense, but there are several external tools which could present a 3-way diff in a useful way (or even n-way with n>3), e.g. vimdiff, kdiff3, meld.

When the --cc/textconv issue came up I looked into this, and maybe difftool is a place where one could plug this in first in the sense of refactoring that even more and providing a diff3tool or such to view a merge commit (or compare any 3 versions), or/and provide "git diff3 A B C" which creates a fake merge (A+B -> C). Now, imagine we also have INDEX and WORKTREE pseudorevs and can do

git diff3[tool] HEAD INDEX WORKTREE

:)

Michael

Michael J Gruber <git@drmicha.warpmail.net> writes:

Thanks. I've given a cursory look at the rest of the series (including the addendum to be squashed in) and they seemed Ok. Will give it another round of eyeballing before merging to "next".

With an external command that can make 3-way diffs of arbitrary three directories, you can trivially do something like:

	#!/bin/sh
	mkdir tmp-head tmp-index &&
        ( cd tmp-head &&
          GIT_DIR=../.git GIT_INDEX_FILE=$GIT_DIR/.index-head &&
          git checkout -f HEAD
	) &&
        ( cd tmp-index &&
          GIT_DIR=../.git &&
          git checkout -f .
	) &&
        $ext_diff3_cmd tmp-head/ tmp-index/ . &
        wait
        rm -fr tmp-index tmp-head

But that seems totally offtopic and has nothing to do with the "combined diff" discussion, no?

If you want to plug in an external command that can make n-way diff of n files when some paths are still shown using the usual --cc codepath, then you would need an interface totally different from the diff.<driver>.cmd interface for two-way diff to the external diff. I pointed at where to plug such a thing, but I do not think it would be of much use unless you are handing the whole n-trees to the external command (which essentially is what the above outline does). How would the user read the output that comes out mixed from different codepaths, some from our own --cc while others come from the external command, possibly opening separate windows and even worse grabbing control and getting the caller stuck until the user closes that window?

	Side note: about getting stuck, will we see an update to the
	diffstat count series by the end of this cycle? I do not mind
	carrying it over to the next cycle at all, but I'd rather see
	something already started gets finished.

You do not need "git diff3 A B C" for a fake merge.

	$ git diff 61d7503d 2d22086 5bf29b9

already is a way to show you how the commit 61d7503d was created by merging the other two (the merge result comes first and then its parents). You could put the index into the mix by doing something like:

	$ git diff next master $(git write-tree)

Trying to show combined diff to merge the index and the working tree into the current HEAD (which may be an example that does not make much sense) would look like this:

	$ git diff HEAD $(git write-tree) $(
		git read-tree --index-output=.tmp-index HEAD &&
		GIT_INDEX_FILE=.tmp-index git add -A :/ &&
                GIT_INDEX_FILE=.tmp-index git write-tree
	)

But for the "working tree" set, which paths should be included? The same set as what is in the index? Or would we use the set that is the union of other tree-like things that are being compared, including the ones that are not in the index? Or everything in the working tree, as we are interested in what the user _could_ add? That is one of the reasons why I do not think it makes much sense trying to throw the working tree into the picture, as it would have to open a large can of worms.

Junio C Hamano venit, vidit, dixit 24.05.2011 17:36:

and the fancy version of that is "difftool" in a sense, yes.

Well, it seemed that Jay wanted an external tool for merge diffs. "-m" does a series of simple 2way diffs, but "--cc" is special in that it looks at all diffs at once, so a corresponding external tool would have to do the same.

I don't quite follow, I think I/we/? am/are mixing external diff drivers and difftools. I agree that is a side track/off-topic.

Yes, on my list. End of month you said, right?

Yes. (I came across this when I investigated a bit back then but failed to mention it.)

I've simply been drooling too much over vim-fugitive and was wondering which aspects would fit into our ui. I was thinking of a 3-way-diff version of git status, so to say. Next would be a 3-way-merge interface which does all your add/reset/checkout -p from 3 vim bufs in diff mode, which even vim-fugitive doesn't do. We can all dream, can't we?

Michael

Michael J Gruber <git@drmicha.warpmail.net> writes:

Yes, but if you post anything on May 31st, it will never have enough time to land in 'master' before the feature freeze ;-).

Heh.

On Tue, May 24, 2011 at 12:38 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Only because I wasn't using textconv, but I should have been.

j.

On Tue, May 24, 2011 at 09:19:43AM +0200, Michael J Gruber wrote:

I agree with Junio that we would need a new config option and external interface for "n-way combined diff". However, isn't what things like vimdiff and meld do the reverse of our combined diff? That is, don't they assume the 3 trees are: ours, theirs, and ancestor (i.e., merge base)? Whereas in a combined diff, it is actually: merge parent 1 (ours), merge parent 2 (theirs), and merge _result_.

Also, do those tools generally handle n-way comparisons as opposed to 3-way?

-Peff

Jeff King venit, vidit, dixit 24.05.2011 21:13:

"Those" tools do "that" which I mean :)

It depends on the tool, of course. vimdiff does not have any assumptions that I know of, it marks those hunks which are not common to all buffers, and marks them differently depending on what subset of buffers shares them. That aspect is the same for kdiff3 and meld (n<=3 for meld) in diff mode.

There are more differences in merge mode:

In vimdiff, you can "put" a hunk (i.e. apply the change) to another buffer (or "obtain" one), i.e. you can easily move hunks between the buffers (to do a merge). In kdiff3's auto merge mode there is the assumption you mention (because it actively does some merging).

So, in vimdiff, you could in principle change and write any buffer but our tools don't support it so far because it is not needed for a merge which has one "target" only. In the ui of my dreams, I would have 3 buffers HEAD INDEX WORKTREE (H I W) and moving hunks between them would do all of add -p, reset -p and checkout -p (the HEAD buf would be read-only).

With "differently abled" tools it could still be useful to have, say a tool invoked for the merge HEAD+WORKTREE -> INDEX (with the current state of the INDEX as the automatic resolution to start off from) so that you can do add+reset -p with your mergetool, and maybe similarly for HEAD+INDEX -> WORKTREE. I.e. addtool and checkouttool in addition to the difftool and mergetool which we have. Just not for the current release cycle any more.

Michael

On Wed, May 25, 2011 at 09:38:43AM +0200, Michael J Gruber wrote:

OK, they are more capable than I realized, then. :)

This should be relatively easy to implement on the git side, as you are doing the hard parts of the "-p" operation in vim already. You just need to write vim's buffer into the worktree or into the index via hash-object / update-index.

Also, wouldn't you potentially want more than 3 buffers, if you were cherry-picking changes from another commit (as in "git checkout -p $commit" or even putting and taking things from a stash? I think that would be a simple generalization of what you are proposing though.

Yeah, getting back to the original discussion. How badly do people actually want an external diff driver that can do fancy things with multiple parents? It seems that for non-text diff viewers, the preferred solution is to use difftool these days (but that is just my impression; I don't use either difftool or external diff drivers).

-Peff

On Tue, May 24, 2011 at 12:46 AM, Junio C Hamano <gitster@pobox.com> wrote:

Indeed, it was a thinko that I even considered it. It turns out I had been using an external diff command where I should have been using textconv all along.

j.