threads / rfc / 38875

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

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

## tl;dr

4 messages between Mar 21, 2015 and Mar 22, 2015. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Yurii Shevtsov· Mar 21, 2015, 12:50 UTC · lore

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

Show changes to diff-no-index.c +33 −3

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

Junio C Hamano· Mar 21, 2015, 18:18 UTC · re: Yurii Shevtsov · lore

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

Yurii Shevtsov <ungetch@gmail.com> writes:

Show 15 quoted lines

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

Yurii Shevtsov· Mar 21, 2015, 19:08 UTC · re: Junio C Hamano · lore

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

Show 35 quoted lines

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

Show 6 quoted lines

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

Eric Sunshine· Mar 22, 2015, 02:39 UTC · re: Yurii Shevtsov · lore

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

On Sat, Mar 21, 2015 at 8:50 AM, Yurii Shevtsov <ungetch@gmail.com> wrote:

Show 9 quoted lines

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

Show 25 quoted 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.

Show 14 quoted lines

> +
> +        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;
> --