All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Cc: Yurii Shevtsov <ungetch@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.
Date: Sun, 15 Mar 2015 20:50:41 -0700	[thread overview]
Message-ID: <xmqqr3spsir2.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <vpq1tkq5fsw.fsf@anie.imag.fr> (Matthieu Moy's message of "Sun, 15 Mar 2015 18:30:39 +0100")

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,
>>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
>>                 return -1;
>>
>> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
>> -               return error("file/directory conflict: %s, %s", name1, name2);
>
> I'm surprised to see this error message totally go away. The idea of the
> microproject was to DWIM (do what I mean) better, but the dwim should
> apply only when $directory/$file actually exists. Otherwise, the error
> message should actually be raised.

I actually think this check, when we really fixed "diff --no-index"
to work sensibly, should go away and be replaced with something
sensible.  As it stands now, even before we think about dwimming
"diff D/ F" into "diff D/F F", a simple formulation like this will
error out.

    $ mkdir -p a/sub b
    $ touch a/file b/file b/sub a/sub/file
    $ git diff --no-index a b
    error: file/directory conflict: a/sub, b/sub

Admittedly, that is how ordinary "diff -r" works, but I am not sure
if we want to emulate that aspect of GNU diff.  If the old tree a
has a directory 'sub' with 'file' under it (i.e. a/sub/file) where
the new tree has a file at 'sub', then the recursive diff can show
the removal of sub/file and creation of sub, no?  That is what we
show for normal "git diff".

But I _think_ fixing that is way outside the scope of GSoC Micro.

And patching this function, because it is recursively called from
within it, to "dwim" is simply wrong.  When we see a/sub that is a
directory and b/sub that is a file, we do *NOT* want to rewrite the
comparison to comparison between a/sub/sub and b/sub.

What needs to be fixed for the Micro is the top-level call to
queue_diff() that is made blindly between paths[0] and paths[1]
without checking what kind of things these are.



    

  reply	other threads:[~2015-03-16  3:52 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-15 15:35 [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better Yurii Shevtsov
2015-03-15 17:30 ` Matthieu Moy
2015-03-16  3:50   ` Junio C Hamano [this message]
2015-03-16 16:23     ` Yurii Shevtsov
2015-03-16 17:14       ` Junio C Hamano
2015-03-16 17:47         ` Yurii Shevtsov
2015-03-16 19:20           ` Junio C Hamano
2015-03-15 17:34 ` t.gummerer
     [not found]   ` <CAHLaBNLQ8-JzEBjypvJDDzhW8SwfzujuOknC_QWar+cL18cR3A@mail.gmail.com>
2015-03-15 18:13     ` t.gummerer
2015-03-15 18:04 ` Eric Sunshine
     [not found]   ` <CAHLaBN+RVpDrG9OewUS7LCYaEOvVqsTY3znapgMj7VrMJWHaDw@mail.gmail.com>
2015-03-15 21:28     ` Eric Sunshine
2015-03-15 22:00       ` Junio C Hamano
2015-03-15 20:17 ` Junio C Hamano

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=xmqqr3spsir2.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=git@vger.kernel.org \
    --cc=ungetch@gmail.com \
    /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.