All of lore.kernel.org
 help / color / mirror / Atom feed
From: Michael J Gruber <git@drmicha.warpmail.net>
To: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>
Cc: 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 09:19:43 +0200	[thread overview]
Message-ID: <4DDB5C0F.1080102@drmicha.warpmail.net> (raw)
In-Reply-To: <7v39k4aeos.fsf@alter.siamese.dyndns.org>

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:
> Jeff King <peff@peff.net> writes:
> 
>>> However, custom diff drivers (still) don't work. :-)
>>
>> 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. :)
> 
> 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?

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

  reply	other threads:[~2011-05-24  7:19 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 [this message]
2011-05-24 15:36                 ` Junio C Hamano
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=4DDB5C0F.1080102@drmicha.warpmail.net \
    --to=git@drmicha.warpmail.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.