[PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Yurii Shevtsov]

Changes 'git diff --no-index $directory $file' behaviour.
Now it is transformed to 'git diff --no-index $directory/&file $file'
instead of throwing an error.

Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>
---
 diff-no-index.c |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 265709b..4e71b36 100644
--- 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);
+       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
+               struct strbuf dirnfile;
+               const char *dir, *file;
+               char *filename;
+               int ret = 0;
+
+               dir = S_ISDIR(mode1) ? name1 : name2;
+               file = (dir == name1) ? name2 : name1;
+               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);
+               strbuf_addstr(&dirnfile, dir);
+               if (dirnfile.buf[dirnfile.len - 1] != '/')
+                       strbuf_addch(&dirnfile, '/');
+               filename = strrchr(file, '/');
+               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);
+               ret = queue_diff(o, dirnfile.buf, file);
+               strbuf_release(&dirnfile);
+
+               return ret;
+       }

        if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
                struct strbuf buffer1 = STRBUF_INIT;
--

I hope I understood task correct. I think this patch requires writing
additional tests, so that's what I'm going to do now.

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Matthieu Moy]

Yurii Shevtsov <ungetch@gmail.com> writes:

> Changes 'git diff --no-index $directory $file' behaviour.
> Now it is transformed to 'git diff --no-index $directory/&file $file'

I guess the & should be a $.

> instead of throwing an error.

Try to insist on _why_ you did this more than what it does in the commit
message.

> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>

Please, use a real email adress, not a mangled one.

> --- 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 hope I understood task correct. I think this patch requires writing
> additional tests, so that's what I'm going to do now.

This text should go between the --- and the diffstat, not at the end of
the message.

And yes, this deserves tests ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Junio C Hamano]

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.



    

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Yurii Shevtsov]

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

So you want me to convert args ("diff D/ F" into "diff D/F F") before
calling queue_diff()? But why? queue_diff() already check args' types
and decides which comparison to do. If I understood you right, it
would require calling get_mode() in diff_no_index(), while get_mode()
will be called again in queue_diff().

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Junio C Hamano]

Yurii Shevtsov <ungetch@gmail.com> writes:

>> ...  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.
> 
> So you want me to convert args ("diff D/ F" into "diff D/F F") before
> calling queue_diff()? But why?

Because it is wrong to do this inside queue_diff()?

Have you actually read what I wrote, tried the above sample
scenario, and thought what is happening in the codepath?

When the user asks to compare directory a/ and b/, the top-level
diff_no_index() would have paths[0]=="a" and paths[1]=="b", and
queue_diff() is called with these in name1 and name2.  Once it
learns that both of these are directories, it _recurses_ into itself
by appending the paths in these directories after these two names.
It finds that both of these directories have "sub" underneath, so it
makes a recursive call to itself to compare "a/sub" and "b/sub".

That call would notice that one is a directory and the other is
not.  That is where you are getting the "f/d conflict" error.

At that point, do you think it is sensible to rewrite that recursed
part of the diff into a comparison between "a/sub/sub" (which does
not exist, and which the user did not mean to compare with b/sub)
and "b/sub" (which is a file)?  I hope not.

> queue_diff() already check args' types and decides which
> comparison to do.

Yes, and I already hinted that that is an independent issue we may
want to fix, which I suspect is larger than GSoC Micro.  Also the
fix would be different.  Right now, it checks the types of paths and
then refuses to compare a directory and a file.  If we wanted to
change it to closer to what the rest of Git does, we would want it
to report that the directory and everything under it are removed and
then a new file is created (if the directory is on the left hand
side of the comparision and the file is on the right hand side) or
the other way around.  That will not involve "append the name of the
file at the end of the directory".

In short, "append the name of the file at the end of the directory"
logic has no place to live inside queue_diff() which handles the
recursion part of the operation.

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Yurii Shevtsov]

Yes, I have red what you wrote several times and tried your example.
I'm really sorry if I sound like I just ignored it. I just got a
little bit lost about which procedure needs patching. You're
absolutely right, queue_diff() is wrong place for it. So do you agree
that "append the name of the file at the end of the directory" logic
should be put to diff_no_index() which will also include calling
get_mode() for each path[] member? Sorry again for asking so much
questions

2015-03-16 19:14 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> Yurii Shevtsov <ungetch@gmail.com> writes:
>
>>> ...  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.
>>
>> So you want me to convert args ("diff D/ F" into "diff D/F F") before
>> calling queue_diff()? But why?
>
> Because it is wrong to do this inside queue_diff()?
>
> Have you actually read what I wrote, tried the above sample
> scenario, and thought what is happening in the codepath?
>
> When the user asks to compare directory a/ and b/, the top-level
> diff_no_index() would have paths[0]=="a" and paths[1]=="b", and
> queue_diff() is called with these in name1 and name2.  Once it
> learns that both of these are directories, it _recurses_ into itself
> by appending the paths in these directories after these two names.
> It finds that both of these directories have "sub" underneath, so it
> makes a recursive call to itself to compare "a/sub" and "b/sub".
>
> That call would notice that one is a directory and the other is
> not.  That is where you are getting the "f/d conflict" error.
>
> At that point, do you think it is sensible to rewrite that recursed
> part of the diff into a comparison between "a/sub/sub" (which does
> not exist, and which the user did not mean to compare with b/sub)
> and "b/sub" (which is a file)?  I hope not.
>
>> queue_diff() already check args' types and decides which
>> comparison to do.
>
> Yes, and I already hinted that that is an independent issue we may
> want to fix, which I suspect is larger than GSoC Micro.  Also the
> fix would be different.  Right now, it checks the types of paths and
> then refuses to compare a directory and a file.  If we wanted to
> change it to closer to what the rest of Git does, we would want it
> to report that the directory and everything under it are removed and
> then a new file is created (if the directory is on the left hand
> side of the comparision and the file is on the right hand side) or
> the other way around.  That will not involve "append the name of the
> file at the end of the directory".
>
> In short, "append the name of the file at the end of the directory"
> logic has no place to live inside queue_diff() which handles the
> recursion part of the operation.

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Junio C Hamano]

Yurii Shevtsov <ungetch@gmail.com> writes:

> Yes, I have red what you wrote several times and tried your example.
> I'm really sorry if I sound like I just ignored it. I just got a
> little bit lost about which procedure needs patching. You're
> absolutely right, queue_diff() is wrong place for it. So do you agree
> that "append the name of the file at the end of the directory" logic
> should be put to diff_no_index() which will also include calling
> get_mode() for each path[] member? Sorry again for asking so much
> questions

Questions are to be asked; no need to apologize.

I think that the "if asked to compare D and F, pretend as if asked
to compare D/F and F" and friends (meaning, "if asked to compare F
and D, compare F and D/F" needs to be handled the same way, and also
it needs to handle the case where "D/F" does *not* exist) logic can
be added to diff_no_index() just before it calls queue_diff().

"If one is directory but not the other, return an error" code may
need to be fixed but I think that would be a larger change than a
few hours work (which I understand is the size GSoC Micro aims for).

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[t.gummerer]

Hi,

On 03/15, Yurii Shevtsov wrote:
> Changes 'git diff --no-index $directory $file' behaviour.
> Now it is transformed to 'git diff --no-index $directory/&file $file'
> instead of throwing an error.

The commit message should describe why the change is made, see
Documentation/SubmittingPatches, section (2)

> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>

Please use your full email here, without replacing @ with <at>

> ---
>  diff-no-index.c |   21 +++++++++++++++++++--

You should probably add a test for the new behaviour in
t/t4053-diff-no-index.sh.

>  1 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 265709b..4e71b36 100644
> --- 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);
> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
> +               struct strbuf dirnfile;
> +               const char *dir, *file;
> +               char *filename;
> +               int ret = 0;
> +
> +               dir = S_ISDIR(mode1) ? name1 : name2;
> +               file = (dir == name1) ? name2 : name1;

This makes git diff --no-index $directory $file the same as
git diff --no-index $file $directory.  Shouldn't these commands give
different results?  (See the behaviour of diff in this case, and
compare it to the behaviour you introduced here)

> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);
> +               strbuf_addstr(&dirnfile, dir);
> +               if (dirnfile.buf[dirnfile.len - 1] != '/')
> +                       strbuf_addch(&dirnfile, '/');
> +               filename = strrchr(file, '/');
> +               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);
> +               ret = queue_diff(o, dirnfile.buf, file);
> +               strbuf_release(&dirnfile);
> +
> +               return ret;
> +       }

Your MUA seems to have replaced tabs with spaces in this email.  The
easiest way to get the formatting correct is the git send-email tool.
You should also try to sending the mail to yourself first and see if
it applies correctly with git am.

> 
>         if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
>                 struct strbuf buffer1 = STRBUF_INIT;
> --
> 
> I hope I understood task correct. I think this patch requires writing
> additional tests, so that's what I'm going to do now.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[t.gummerer]

[re-added cc to the list]

On 03/15, Yurii Shevtsov wrote:
> Hi, and thank for your reply
> 
> >>  1 files changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/diff-no-index.c b/diff-no-index.c
> >> index 265709b..4e71b36 100644
> >> --- 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);
> >> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
> >> +               struct strbuf dirnfile;
> >> +               const char *dir, *file;
> >> +               char *filename;
> >> +               int ret = 0;
> >> +
> >> +               dir = S_ISDIR(mode1) ? name1 : name2;
> >> +               file = (dir == name1) ? name2 : name1;
> >
> > This makes git diff --no-index $directory $file the same as
> > git diff --no-index $file $directory.  Shouldn't these commands give
> > different results?  (See the behaviour of diff in this case, and
> > compare it to the behaviour you introduced here)
> 
> I really checked behavior of usual diff. And swapping arguments
> doesn't affect result. At first I had doubts about task formulation,
> so I asked about it and got the answer:
> gmane.comp.version-control.git/265479 Am I misunderstood it again?

Using the same example as in that thread, I get the following output:

tommy at hank in work[1]  $ diff -u git junk/diff.h
--- git/diff.h	 2014-12-26 21:00:20.690774933 +0100
+++ junk/diff.h	 2015-03-15 18:02:03.441049918 +0100
@@ -357,3 +357,4 @@
 extern void setup_diff_pager(struct diff_options *);

 #endif /* DIFF_H */
 +hello
tommy at hank in work $ diff -u junk/diff.h git
--- junk/diff.h	 2015-03-15 18:02:03.441049918 +0100
+++ git/diff.h	 2014-12-26 21:00:20.690774933 +0100
@@ -357,4 +357,3 @@
 extern void setup_diff_pager(struct diff_options *);

 #endif /* DIFF_H */
 -hello

Notice the +hello vs. -hello in the last line off the diff.  Git with
your patch on the other hand gives me +hello in both cases.

tommy at hank in work[1]  $ g diff --no-index git junk/diff.h
diff --git a/git/diff.h b/junk/diff.h
index b4a624d..81671dd 100644
--- a/git/diff.h
+++ b/junk/diff.h
@@ -357,3 +357,4 @@ extern int print_stat_summary(FILE *fp, int files,
 extern void setup_diff_pager(struct diff_options *);

 #endif /* DIFF_H */
 +hello
tommy at hank in work[1]  $ g diff --no-index junk/diff.h git
diff --git a/git/diff.h b/junk/diff.h
index b4a624d..81671dd 100644
--- a/git/diff.h
+++ b/junk/diff.h
@@ -357,3 +357,4 @@ extern int print_stat_summary(FILE *fp, int files,
 extern void setup_diff_pager(struct diff_options *);

 #endif /* DIFF_H */
 +hello

So while I think the behaviour with git diff --no-index $directory $file
is correct (minus the comments by Matthieu), git diff --no-index $file
$directory is not, I think.

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Eric Sunshine]

In addition to the points raised by Matthieu and Thomas...

On Sun, Mar 15, 2015 at 11:35 AM, Yurii Shevtsov <ungetch@gmail.com> wrote:
> make "git diff --no-index $directory $file" DWIM better.

Specify the area affected by the change, followed by a colon, followed
by the change summary. Drop the period at the end. So, something like:

    diff: improve '--no-index <directory> <file>' DWIMing

> Changes 'git diff --no-index $directory $file' behaviour.
> Now it is transformed to 'git diff --no-index $directory/&file $file'
> instead of throwing an error.

Write in imperative mood, so "Change" rather than "Changes". By
itself, the first sentence isn't saying much; it would read better if
you combined the two sentences into one.

> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>
> ---
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 265709b..4e71b36 100644
> --- 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);
> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
> +               struct strbuf dirnfile;

Is this name supposed to stand for "dir'n'file", a shorthand for
"dir-and-file"? Perhaps a simpler more idiomatic name such as "path"
would suffice. Also, you can initialize the strbuf here at point of
declaration:

    struct strbuf path = STRBUF_INIT;

> +               const char *dir, *file;
> +               char *filename;
> +               int ret = 0;
> +
> +               dir = S_ISDIR(mode1) ? name1 : name2;
> +               file = (dir == name1) ? name2 : name1;
> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);

Unless this is a performance critical loop where you want to avoid
multiple re-allocations, it's customary to pass 0 for the second
argument. Doing so makes the code a bit easier to read and understand,
and avoids questions like this one: Why are you adding 2 to the
requested length? I presume that you're taking the "/" and NUL into
account, however, strbuf takes NUL into account on its own as part of
its contract, so you probably wanted to add only 1.

> +               strbuf_addstr(&dirnfile, dir);
> +               if (dirnfile.buf[dirnfile.len - 1] != '/')

I don't know how others feel about it, but it makes me a bit
uncomfortable to see potential access of array item -1. I suppose, in
this case, neither name will be zero-length, however, I'd still feel
more comfortable seeing that documented formally, either via assert():

    assert(dirnfile.len > 0);
    if (dirnfile.buf[dirnfile.len - 1] != '/')

or:

    if (dirnfile.len > 0 && dirnfile.buf[dirnfile.len - 1] != '/')

> +                       strbuf_addch(&dirnfile, '/');
> +               filename = strrchr(file, '/');
> +               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);
> +               ret = queue_diff(o, dirnfile.buf, file);
> +               strbuf_release(&dirnfile);
> +
> +               return ret;
> +       }
>
>         if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
>                 struct strbuf buffer1 = STRBUF_INIT;
> --
>
> I hope I understood task correct. I think this patch requires writing
> additional tests, so that's what I'm going to do now.
> --

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Eric Sunshine]

[re-adding cc:git]

On Sun, Mar 15, 2015 at 2:45 PM, Yurii Shevtsov <ungetch@gmail.com> wrote:
>> On Sun, Mar 15, 2015 at 11:35 AM, Yurii Shevtsov <ungetch@gmail.com> wrote:
>>> make "git diff --no-index $directory $file" DWIM better.
>>
>> Specify the area affected by the change, followed by a colon, followed
>> by the change summary. Drop the period at the end. So, something like:
>>
>>     diff: improve '--no-index <directory> <file>' DWIMing
>>
>>> Changes 'git diff --no-index $directory $file' behaviour.
>>> Now it is transformed to 'git diff --no-index $directory/&file $file'
>>> instead of throwing an error.
>>
>> Write in imperative mood, so "Change" rather than "Changes". By
>> itself, the first sentence isn't saying much; it would read better if
>> you combined the two sentences into one.
>
> Got it! My commit message requires improvements
>>> ---
>>> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))
>>> -               return error("file/directory conflict: %s, %s", name1, name2);
>>> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
>>> +               struct strbuf dirnfile;
>>
>> Is this name supposed to stand for "dir'n'file", a shorthand for
>> "dir-and-file"? Perhaps a simpler more idiomatic name such as "path"
>> would suffice. Also, you can initialize the strbuf here at point of
>> declaration:
>>
>>     struct strbuf path = STRBUF_INIT;
>
> Yes it is supposed to be "dir-and-file" I thought "path" isn't
> descriptive enough because it could be path to dir. But if you insist,
> no problems

The reason I asked was because it is not uncommon for variable names
with an 'n' suffix to mean "length" of something, so the 'n' in 'dirn'
was a bit confusing. I personally find the idiomatic name 'path'
easier to grok, however, Junio, of course, has final say-so. It's okay
to argue for your choice in naming if you feel strongly that it is
better.

>>> +               const char *dir, *file;
>>> +               char *filename;
>>> +               int ret = 0;
>>> +
>>> +               dir = S_ISDIR(mode1) ? name1 : name2;
>>> +               file = (dir == name1) ? name2 : name1;
>>> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);
>>
>> Unless this is a performance critical loop where you want to avoid
>> multiple re-allocations, it's customary to pass 0 for the second
>> argument. Doing so makes the code a bit easier to read and understand,
>> and avoids questions like this one: Why are you adding 2 to the
>> requested length? I presume that you're taking the "/" and NUL into
>> account, however, strbuf takes NUL into account on its own as part of
>> its contract, so you probably wanted to add only 1.
>
> Yes I thought about performance. I thought it is reasonable since I
> always know size of resulting string. Checked strbuf.c one more time,
> yoг are right I should really add only 1

A few reasons I probably would just pass 0 in this case: (1) this
string construction is not performance critical; (2) a person reading
the code has to spend extra time double-checking the math; (3) the
math may become outdated if someone later alters the string
construction in some way, thus it's a potential maintenance burden;
(4) terser code tends to be easier to read and understand at a glance,
so the less verbose the code, the better.

However, as usual, it's entirely acceptable to argue for your version
if you feel strongly about it.

>>> +               strbuf_addstr(&dirnfile, dir);
>>> +               if (dirnfile.buf[dirnfile.len - 1] != '/')
>>
>> I don't know how others feel about it, but it makes me a bit
>> uncomfortable to see potential access of array item -1. I suppose, in
>> this case, neither name will be zero-length, however, I'd still feel
>> more comfortable seeing that documented formally, either via assert():
>>
>>     assert(dirnfile.len > 0);
>>     if (dirnfile.buf[dirnfile.len - 1] != '/')
>>
>> or:
>>
>>     if (dirnfile.len > 0 && dirnfile.buf[dirnfile.len - 1] != '/')
>
> My fault again

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>> Is this name supposed to stand for "dir'n'file",...
> ...I personally find the idiomatic name 'path'
> easier to grok, however, Junio, of course, has final say-so.

If I were presented two identical patches, one calling it "path" and
the other calling it "dirnfile", I would definitely take the former,
but I agree that this is more of a preference than a taste, the
latter of which implies that you could make a value judgement,
i.e. "good taste" vs "bad taste".

If for some reason we had a code that called a variable "dirnfile"
already in our official codebase and we saw a patch to "correct"
that to "path", I would likely say that it is not worth the churn to
apply such a "correction" patch.  If the new name were "pathname",
however, I might be pursuaded to take it, simply because a
"pathname" is a lot more familiar word than "dirnfile".

Re: [PATCH/RFC][GSoC] make "git diff --no-index $directory $file" DWIM better.

[Junio C Hamano]

Yurii Shevtsov <ungetch@gmail.com> writes:

> Changes 'git diff --no-index $directory $file' behaviour.
> Now it is transformed to 'git diff --no-index $directory/&file $file'
> instead of throwing an error.

Is this asymmetric?  Shouldn't "git diff --no-index $file $directory"
behave the same way, i.e. turned into "$file $directory/$file"?

If you intended the patch to do so, perhaps

    "git diff --no-index directory/ file" used to error out, saying
    that you cannot compare a directory and a file (with the
    parameters swapped, "git diff --no-index file directory/" failed
    the same way).

    With normal "diff", "diff D/ F" acts as if it were told to
    compare D/F and F (when D/F exists---if there isn't, then it
    shows a creation of F), and behaving the same way would be more
    natural to the users.

or something?

> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>
> ---
>  diff-no-index.c |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 265709b..4e71b36 100644
> --- 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);
> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
> +               struct strbuf dirnfile;
> +               const char *dir, *file;
> +               char *filename;
> +               int ret = 0;
> +
> +               dir = S_ISDIR(mode1) ? name1 : name2;
> +               file = (dir == name1) ? name2 : name1;
> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);
> +               strbuf_addstr(&dirnfile, dir);
> +               if (dirnfile.buf[dirnfile.len - 1] != '/')
> +                       strbuf_addch(&dirnfile, '/');
> +               filename = strrchr(file, '/');
> +               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);
> +               ret = queue_diff(o, dirnfile.buf, file);

Hmm, it appears that you are turning "diff F D/" into "diff D/F F",
which is the other way around here, or am I mis-reading queue_diff().

Does queue_diff() do the right thing when D/F does not exist (not a
rhetorical question; I just did not check it myself)?