[PATCH/RFC][GSoC] diff-no-index: transform "$directory $file" args to "$directory/$file $file"

[PATCH/RFC][GSoC] diff-no-index: transform "$directory $file" args to "$directory/$file $file"

[Yurii Shevtsov]

git diff --no-index refuses to compare if args are directory and file,
instead of usual diff.

Now git diff --no-index compares args' target names and, if they are
equal, refuses to compare. If args differ, it transforms args and
diffs files, as usual diff does.
This is the least expensive way in terms of amount of new code and
also doesn't affect usual behaviour.

Changes are done in queue_diff(). Adding args transformation logic to
top-level is irrational,
because requires writing additional code to define args' types, which
is already done in
queue_diff(). The error message about file\directory conflict left as
is until further guidence.

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

diff --git a/diff-no-index.c b/diff-no-index.c
index 265709b..9a3439a 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -97,8 +97,39 @@ 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 path;
+        const char *dir, *file;
+        char *filename, *dirname = 0;
+        int i, ret = 0;
+
+        dir = S_ISDIR(mode1) ? name1 : name2;
+        file = (dir == name1) ? name2 : name1;
+        strbuf_init(&path, strlen(name1) + strlen(name2) + 1);
+        strbuf_addstr(&path, dir);
+        filename = strrchr(file, '/');
+        if (path.len && path.buf[path.len - 1] != '/')
+            strbuf_addch(&path, '/');
+        for (i = path.len - 2; i >= 0; i--)
+            if (path.buf[i] == '/') {
+                dirname = &path.buf[i];
+                break;
+            }
+        if (dirname == 0)
+            dirname = path.buf;
+
+        if (!strncmp(dirname, filename, strlen(filename)))
+            return error("file/directory conflict: %s, %s", name1, name2);
+
+        strbuf_addstr(&path, filename ? (filename + 1) : file);
+        if (file == name1)
+            ret = queue_diff(o, file, path.buf);
+        else
+            ret = queue_diff(o, path.buf, file);
+        strbuf_release(&path);
+
+        return ret;
+    }

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

Re: [PATCH/RFC][GSoC] diff-no-index: transform "$directory $file" args to "$directory/$file $file"

[Junio C Hamano]

Yurii Shevtsov <ungetch@gmail.com> writes:

> diff --git a/diff-no-index.c b/diff-no-index.c
> index 265709b..9a3439a 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -97,8 +97,39 @@ 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 path;
> +        const char *dir, *file;
> +        char *filename, *dirname = 0;
> +        int i, ret = 0;

If you have two directories, a and b, under which there are two
files a/sub/file and b/sub (i.e. 'sub' in a/ is a directory and b/
is a file), and if you say "git diff --no-index a b", what happens?

 - the caller of this function gives a and b in name1 and name2;

 - we do not come in this codepath as both are directories;

 - we read from a/ and b/ and fill p1 and p2 with names of paths in
   the directories -- p1 and p2 will both have 'sub';

 - queue_diff() is recursively called to compare a/sub and b/sub;

   - now we have name1 = a/sub and name2 = b/sub;

   - we come in this codepath, and they are turned into comparison
     between a/sub/sub and b/sub.

The last step is simply crazy.

Hmmmm, is vger reinjecting an old message, or you sent an older and
wrong version of a patch by mistake?  We discussed why doing this in
queue_diff() is wrong in the thread that has $gmane/265543 in it,
and I was expecting to see a logic like this in the caller.

Puzzled...

Re: [PATCH/RFC][GSoC] diff-no-index: transform "$directory $file" args to "$directory/$file $file"

[Yurii Shevtsov]

>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index 265709b..9a3439a 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -97,8 +97,39 @@ 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 path;
>> +        const char *dir, *file;
>> +        char *filename, *dirname = 0;
>> +        int i, ret = 0;
>
> If you have two directories, a and b, under which there are two
> files a/sub/file and b/sub (i.e. 'sub' in a/ is a directory and b/
> is a file), and if you say "git diff --no-index a b", what happens?
>
>  - the caller of this function gives a and b in name1 and name2;
>
>  - we do not come in this codepath as both are directories;
>
>  - we read from a/ and b/ and fill p1 and p2 with names of paths in
>    the directories -- p1 and p2 will both have 'sub';
>
>  - queue_diff() is recursively called to compare a/sub and b/sub;
>
>    - now we have name1 = a/sub and name2 = b/sub;
>
>    - we come in this codepath, and they are turned into comparison
>      between a/sub/sub and b/sub.
>
> The last step is simply crazy.

It won't try to compare a/sub/sub and b/sub, since git diff with this
patch falls before with error message, as I planned (if it did compare
I would get 'Couldn't access a/sub/sub'). I tested my code! Original
git diff also stops comparing when hits folder and file with same
names. So my patch allows to pass 'D F' args, and doesn't affect other
logic.

> Hmmmm, is vger reinjecting an old message, or you sent an older and
> wrong version of a patch by mistake?  We discussed why doing this in
> queue_diff() is wrong in the thread that has $gmane/265543 in it,
> and I was expecting to see a logic like this in the caller.
>
> Puzzled...

Yes, we discussed it, but, as I mentioned in commit message, I think
my solution much better fits in queue_diff()

Re: [PATCH/RFC][GSoC] diff-no-index: transform "$directory $file" args to "$directory/$file $file"

[Eric Sunshine]

On Sat, Mar 21, 2015 at 8:50 AM, Yurii Shevtsov <ungetch@gmail.com> wrote:
> Signed-off-by: Yurii Shevtsov <ungetch@gmail.com>
> ---
> diff --git a/diff-no-index.c b/diff-no-index.c
> index 265709b..9a3439a 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -97,8 +97,39 @@ static int queue_diff(struct diff_options *o,
>      if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
>          return -1;

Somehow, you lost all the tabs in the patch, and everything is instead
indented with spaces (including context lines).

> -    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 path;
> +        const char *dir, *file;
> +        char *filename, *dirname = 0;
> +        int i, ret = 0;
> +
> +        dir = S_ISDIR(mode1) ? name1 : name2;
> +        file = (dir == name1) ? name2 : name1;
> +        strbuf_init(&path, strlen(name1) + strlen(name2) + 1);
> +        strbuf_addstr(&path, dir);
> +        filename = strrchr(file, '/');
> +        if (path.len && path.buf[path.len - 1] != '/')
> +            strbuf_addch(&path, '/');
> +        for (i = path.len - 2; i >= 0; i--)
> +            if (path.buf[i] == '/') {
> +                dirname = &path.buf[i];
> +                break;
> +            }
> +        if (dirname == 0)
> +            dirname = path.buf;
> +
> +        if (!strncmp(dirname, filename, strlen(filename)))
> +            return error("file/directory conflict: %s, %s", name1, name2);

Leaking 'path' strbuf.

> +
> +        strbuf_addstr(&path, filename ? (filename + 1) : file);
> +        if (file == name1)
> +            ret = queue_diff(o, file, path.buf);
> +        else
> +            ret = queue_diff(o, path.buf, file);
> +        strbuf_release(&path);
> +
> +        return ret;
> +    }
>
>      if (S_ISDIR(mode1) || S_ISDIR(mode2)) {
>          struct strbuf buffer1 = STRBUF_INIT;
> --