git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Michael J Gruber <git@drmicha.warpmail.net>
Cc: Jeff King <peff@peff.net>, Jay Soffian <jaysoffian@gmail.com>,
	git <git@vger.kernel.org>
Subject: Re: combined diff does not detect binary files and ignores -diff attribute
Date: Tue, 24 May 2011 08:36:21 -0700	[thread overview]
Message-ID: <7vsjs48616.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <4DDB5C0F.1080102@drmicha.warpmail.net> (Michael J. Gruber's message of "Tue, 24 May 2011 09:19:43 +0200")

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

> 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.)

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".

>> 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?
>
> 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.

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.

> 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).

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.

  reply	other threads:[~2011-05-24 15:36 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-22 20:12 combined diff does not detect binary files and ignores -diff attribute Jay Soffian
2011-05-23 13:30 ` Michael J Gruber
2011-05-23 15:17   ` Jay Soffian
2011-05-23 17:07     ` Junio C Hamano
2011-05-23 18:11     ` Jeff King
2011-05-23 20:15       ` Jeff King
2011-05-23 20:16         ` [PATCH 1/5] combine-diff: split header printing into its own function Jeff King
2011-05-23 20:16         ` [PATCH 2/5] combine-diff: calculate mode_differs earlier Jeff King
2011-05-23 20:27         ` [PATCH 3/5] combine-diff: handle binary files as binary Jeff King
2011-05-23 23:02           ` Junio C Hamano
2011-05-23 23:50             ` Jeff King
2011-05-30  6:33           ` Junio C Hamano
2011-05-30 14:36             ` Jeff King
2011-05-30 16:19               ` Jeff King
2011-05-30 19:32                 ` Junio C Hamano
2011-05-31 22:42               ` Junio C Hamano
2011-05-23 20:30         ` [PATCH 4/5] refactor get_textconv to not require diff_filespec Jeff King
2011-05-23 20:31         ` [PATCH 5/5] combine-diff: respect textconv attributes Jeff King
2011-05-23 22:47           ` Junio C Hamano
2011-05-23 23:39             ` Jeff King
2011-05-24 16:20           ` Junio C Hamano
2011-05-24 18:52             ` Jeff King
2011-05-23 22:55         ` combined diff does not detect binary files and ignores -diff attribute Jay Soffian
2011-05-23 23:31           ` Jay Soffian
2011-05-23 23:49             ` Jeff King
2011-05-24  0:59               ` Jay Soffian
2011-05-23 23:41           ` Jeff King
2011-05-24  4:46             ` Junio C Hamano
2011-05-24  7:19               ` Michael J Gruber
2011-05-24 15:36                 ` Junio C Hamano [this message]
2011-05-24 16:38                   ` Michael J Gruber
2011-05-24 16:43                     ` Junio C Hamano
2011-05-24 16:52                     ` Jay Soffian
2011-05-24 19:13                 ` Jeff King
2011-05-25  7:38                   ` Michael J Gruber
2011-05-25 15:29                     ` Jeff King
2011-05-24 14:40               ` Jay Soffian

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vsjs48616.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@drmicha.warpmail.net \
    --cc=git@vger.kernel.org \
    --cc=jaysoffian@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).