{"thread":{"id":"38813","subject":"[PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","startedAt":"2015-03-15T15:35:27Z","lastAt":"2015-03-16T19:20:24Z","messageCount":13,"participants":["Yurii Shevtsov","Matthieu Moy","t.gummerer@gmail.com","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"257723","messageId":"CAHLaBN+93mp6PQmtfjOHSvfW7iwDXwPitGQ5W1am9KBm9EZV2Q@mail.gmail.com","threadId":"38813","inReplyTo":null,"subject":"[PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Yurii Shevtsov","fromEmail":"ungetch@gmail.com","sentAt":"2015-03-15T15:35:27Z","receivedAt":"2015-03-15T15:35:27Z","isPatch":true,"sender":{"key":"ungetch@gmail.com","avatar":null},"body":"Changes 'git diff --no-index $directory $file' behaviour.\nNow it is transformed to 'git diff --no-index $directory/&file $file'\ninstead of throwing an error.\n\nSigned-off-by: Yurii Shevtsov <ungetch <at> gmail.com>\n---\n diff-no-index.c |   21 +++++++++++++++++++--\n 1 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 265709b..4e71b36 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n        if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n                return -1;\n\n-       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n-               return error(\"file/directory conflict: %s, %s\", name1, name2);\n+       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n+               struct strbuf dirnfile;\n+               const char *dir, *file;\n+               char *filename;\n+               int ret = 0;\n+\n+               dir = S_ISDIR(mode1) ? name1 : name2;\n+               file = (dir == name1) ? name2 : name1;\n+               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);\n+               strbuf_addstr(&dirnfile, dir);\n+               if (dirnfile.buf[dirnfile.len - 1] != '/')\n+                       strbuf_addch(&dirnfile, '/');\n+               filename = strrchr(file, '/');\n+               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);\n+               ret = queue_diff(o, dirnfile.buf, file);\n+               strbuf_release(&dirnfile);\n+\n+               return ret;\n+       }\n\n        if (S_ISDIR(mode1) || S_ISDIR(mode2)) {\n                struct strbuf buffer1 = STRBUF_INIT;\n--\n\nI hope I understood task correct. I think this patch requires writing\nadditional tests, so that's what I'm going to do now.\n"},{"id":"257725","messageId":"vpq1tkq5fsw.fsf@anie.imag.fr","threadId":"38813","inReplyTo":"CAHLaBN+93mp6PQmtfjOHSvfW7iwDXwPitGQ5W1am9KBm9EZV2Q@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-03-15T17:30:39Z","receivedAt":"2015-03-15T17:30:39Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Yurii Shevtsov <ungetch@gmail.com> writes:\n\n> Changes 'git diff --no-index $directory $file' behaviour.\n> Now it is transformed to 'git diff --no-index $directory/&file $file'\n\nI guess the & should be a $.\n\n> instead of throwing an error.\n\nTry to insist on _why_ you did this more than what it does in the commit\nmessage.\n\n> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>\n\nPlease, use a real email adress, not a mangled one.\n\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n>                 return -1;\n>\n> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n\nI'm surprised to see this error message totally go away. The idea of the\nmicroproject was to DWIM (do what I mean) better, but the dwim should\napply only when $directory/$file actually exists. Otherwise, the error\nmessage should actually be raised.\n\n> --\n>\n> I hope I understood task correct. I think this patch requires writing\n> additional tests, so that's what I'm going to do now.\n\nThis text should go between the --- and the diffstat, not at the end of\nthe message.\n\nAnd yes, this deserves tests ;-).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"257726","messageId":"20150315173451.GB17591@hank","threadId":"38813","inReplyTo":"CAHLaBN+93mp6PQmtfjOHSvfW7iwDXwPitGQ5W1am9KBm9EZV2Q@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-15T17:34:52Z","receivedAt":"2015-03-15T17:34:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Hi,\n\nOn 03/15, Yurii Shevtsov wrote:\n> Changes 'git diff --no-index $directory $file' behaviour.\n> Now it is transformed to 'git diff --no-index $directory/&file $file'\n> instead of throwing an error.\n\nThe commit message should describe why the change is made, see\nDocumentation/SubmittingPatches, section (2)\n\n> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>\n\nPlease use your full email here, without replacing @ with <at>\n\n> ---\n>  diff-no-index.c |   21 +++++++++++++++++++--\n\nYou should probably add a test for the new behaviour in\nt/t4053-diff-no-index.sh.\n\n>  1 files changed, 19 insertions(+), 2 deletions(-)\n> \n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 265709b..4e71b36 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n>                 return -1;\n> \n> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n> +               struct strbuf dirnfile;\n> +               const char *dir, *file;\n> +               char *filename;\n> +               int ret = 0;\n> +\n> +               dir = S_ISDIR(mode1) ? name1 : name2;\n> +               file = (dir == name1) ? name2 : name1;\n\nThis makes git diff --no-index $directory $file the same as\ngit diff --no-index $file $directory.  Shouldn't these commands give\ndifferent results?  (See the behaviour of diff in this case, and\ncompare it to the behaviour you introduced here)\n\n> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);\n> +               strbuf_addstr(&dirnfile, dir);\n> +               if (dirnfile.buf[dirnfile.len - 1] != '/')\n> +                       strbuf_addch(&dirnfile, '/');\n> +               filename = strrchr(file, '/');\n> +               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);\n> +               ret = queue_diff(o, dirnfile.buf, file);\n> +               strbuf_release(&dirnfile);\n> +\n> +               return ret;\n> +       }\n\nYour MUA seems to have replaced tabs with spaces in this email.  The\neasiest way to get the formatting correct is the git send-email tool.\nYou should also try to sending the mail to yourself first and see if\nit applies correctly with git am.\n\n> \n>         if (S_ISDIR(mode1) || S_ISDIR(mode2)) {\n>                 struct strbuf buffer1 = STRBUF_INIT;\n> --\n> \n> I hope I understood task correct. I think this patch requires writing\n> additional tests, so that's what I'm going to do now.\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"257733","messageId":"CAPig+cQgvwd=5hLGeOpY1r_476HbpUarumsu_QDU4HwB7ynXmw@mail.gmail.com","threadId":"38813","inReplyTo":"CAHLaBN+93mp6PQmtfjOHSvfW7iwDXwPitGQ5W1am9KBm9EZV2Q@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-15T18:04:02Z","receivedAt":"2015-03-15T18:04:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"In addition to the points raised by Matthieu and Thomas...\n\nOn Sun, Mar 15, 2015 at 11:35 AM, Yurii Shevtsov <ungetch@gmail.com> wrote:\n> make \"git diff --no-index $directory $file\" DWIM better.\n\nSpecify the area affected by the change, followed by a colon, followed\nby the change summary. Drop the period at the end. So, something like:\n\n    diff: improve '--no-index <directory> <file>' DWIMing\n\n> Changes 'git diff --no-index $directory $file' behaviour.\n> Now it is transformed to 'git diff --no-index $directory/&file $file'\n> instead of throwing an error.\n\nWrite in imperative mood, so \"Change\" rather than \"Changes\". By\nitself, the first sentence isn't saying much; it would read better if\nyou combined the two sentences into one.\n\n> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>\n> ---\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 265709b..4e71b36 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n>                 return -1;\n>\n> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n> +               struct strbuf dirnfile;\n\nIs this name supposed to stand for \"dir'n'file\", a shorthand for\n\"dir-and-file\"? Perhaps a simpler more idiomatic name such as \"path\"\nwould suffice. Also, you can initialize the strbuf here at point of\ndeclaration:\n\n    struct strbuf path = STRBUF_INIT;\n\n> +               const char *dir, *file;\n> +               char *filename;\n> +               int ret = 0;\n> +\n> +               dir = S_ISDIR(mode1) ? name1 : name2;\n> +               file = (dir == name1) ? name2 : name1;\n> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);\n\nUnless this is a performance critical loop where you want to avoid\nmultiple re-allocations, it's customary to pass 0 for the second\nargument. Doing so makes the code a bit easier to read and understand,\nand avoids questions like this one: Why are you adding 2 to the\nrequested length? I presume that you're taking the \"/\" and NUL into\naccount, however, strbuf takes NUL into account on its own as part of\nits contract, so you probably wanted to add only 1.\n\n> +               strbuf_addstr(&dirnfile, dir);\n> +               if (dirnfile.buf[dirnfile.len - 1] != '/')\n\nI don't know how others feel about it, but it makes me a bit\nuncomfortable to see potential access of array item -1. I suppose, in\nthis case, neither name will be zero-length, however, I'd still feel\nmore comfortable seeing that documented formally, either via assert():\n\n    assert(dirnfile.len > 0);\n    if (dirnfile.buf[dirnfile.len - 1] != '/')\n\nor:\n\n    if (dirnfile.len > 0 && dirnfile.buf[dirnfile.len - 1] != '/')\n\n> +                       strbuf_addch(&dirnfile, '/');\n> +               filename = strrchr(file, '/');\n> +               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);\n> +               ret = queue_diff(o, dirnfile.buf, file);\n> +               strbuf_release(&dirnfile);\n> +\n> +               return ret;\n> +       }\n>\n>         if (S_ISDIR(mode1) || S_ISDIR(mode2)) {\n>                 struct strbuf buffer1 = STRBUF_INIT;\n> --\n>\n> I hope I understood task correct. I think this patch requires writing\n> additional tests, so that's what I'm going to do now.\n> --\n"},{"id":"257735","messageId":"20150315181322.GC17591@hank","threadId":"38813","inReplyTo":"CAHLaBNLQ8-JzEBjypvJDDzhW8SwfzujuOknC_QWar+cL18cR3A@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"","fromEmail":"t.gummerer@gmail.com","sentAt":"2015-03-15T18:13:22Z","receivedAt":"2015-03-15T18:13:22Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"[re-added cc to the list]\n\nOn 03/15, Yurii Shevtsov wrote:\n> Hi, and thank for your reply\n> \n> >>  1 files changed, 19 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/diff-no-index.c b/diff-no-index.c\n> >> index 265709b..4e71b36 100644\n> >> --- a/diff-no-index.c\n> >> +++ b/diff-no-index.c\n> >> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n> >>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n> >>                 return -1;\n> >>\n> >> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n> >> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n> >> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n> >> +               struct strbuf dirnfile;\n> >> +               const char *dir, *file;\n> >> +               char *filename;\n> >> +               int ret = 0;\n> >> +\n> >> +               dir = S_ISDIR(mode1) ? name1 : name2;\n> >> +               file = (dir == name1) ? name2 : name1;\n> >\n> > This makes git diff --no-index $directory $file the same as\n> > git diff --no-index $file $directory.  Shouldn't these commands give\n> > different results?  (See the behaviour of diff in this case, and\n> > compare it to the behaviour you introduced here)\n> \n> I really checked behavior of usual diff. And swapping arguments\n> doesn't affect result. At first I had doubts about task formulation,\n> so I asked about it and got the answer:\n> gmane.comp.version-control.git/265479 Am I misunderstood it again?\n\nUsing the same example as in that thread, I get the following output:\n\ntommy at hank in work[1]  $ diff -u git junk/diff.h\n--- git/diff.h\t 2014-12-26 21:00:20.690774933 +0100\n+++ junk/diff.h\t 2015-03-15 18:02:03.441049918 +0100\n@@ -357,3 +357,4 @@\n extern void setup_diff_pager(struct diff_options *);\n\n #endif /* DIFF_H */\n +hello\ntommy at hank in work $ diff -u junk/diff.h git\n--- junk/diff.h\t 2015-03-15 18:02:03.441049918 +0100\n+++ git/diff.h\t 2014-12-26 21:00:20.690774933 +0100\n@@ -357,4 +357,3 @@\n extern void setup_diff_pager(struct diff_options *);\n\n #endif /* DIFF_H */\n -hello\n\nNotice the +hello vs. -hello in the last line off the diff.  Git with\nyour patch on the other hand gives me +hello in both cases.\n\ntommy at hank in work[1]  $ g diff --no-index git junk/diff.h\ndiff --git a/git/diff.h b/junk/diff.h\nindex b4a624d..81671dd 100644\n--- a/git/diff.h\n+++ b/junk/diff.h\n@@ -357,3 +357,4 @@ extern int print_stat_summary(FILE *fp, int files,\n extern void setup_diff_pager(struct diff_options *);\n\n #endif /* DIFF_H */\n +hello\ntommy at hank in work[1]  $ g diff --no-index junk/diff.h git\ndiff --git a/git/diff.h b/junk/diff.h\nindex b4a624d..81671dd 100644\n--- a/git/diff.h\n+++ b/junk/diff.h\n@@ -357,3 +357,4 @@ extern int print_stat_summary(FILE *fp, int files,\n extern void setup_diff_pager(struct diff_options *);\n\n #endif /* DIFF_H */\n +hello\n\nSo while I think the behaviour with git diff --no-index $directory $file\nis correct (minus the comments by Matthieu), git diff --no-index $file\n$directory is not, I think.\n"},{"id":"257745","messageId":"xmqqpp8at3qf.fsf@gitster.dls.corp.google.com","threadId":"38813","inReplyTo":"CAHLaBN+93mp6PQmtfjOHSvfW7iwDXwPitGQ5W1am9KBm9EZV2Q@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T20:17:28Z","receivedAt":"2015-03-15T20:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yurii Shevtsov <ungetch@gmail.com> writes:\n\n> Changes 'git diff --no-index $directory $file' behaviour.\n> Now it is transformed to 'git diff --no-index $directory/&file $file'\n> instead of throwing an error.\n\nIs this asymmetric?  Shouldn't \"git diff --no-index $file $directory\"\nbehave the same way, i.e. turned into \"$file $directory/$file\"?\n\nIf you intended the patch to do so, perhaps\n\n    \"git diff --no-index directory/ file\" used to error out, saying\n    that you cannot compare a directory and a file (with the\n    parameters swapped, \"git diff --no-index file directory/\" failed\n    the same way).\n\n    With normal \"diff\", \"diff D/ F\" acts as if it were told to\n    compare D/F and F (when D/F exists---if there isn't, then it\n    shows a creation of F), and behaving the same way would be more\n    natural to the users.\n\nor something?\n\n> Signed-off-by: Yurii Shevtsov <ungetch <at> gmail.com>\n> ---\n>  diff-no-index.c |   21 +++++++++++++++++++--\n>  1 files changed, 19 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 265709b..4e71b36 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n>                 return -1;\n>\n> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n> +               struct strbuf dirnfile;\n> +               const char *dir, *file;\n> +               char *filename;\n> +               int ret = 0;\n> +\n> +               dir = S_ISDIR(mode1) ? name1 : name2;\n> +               file = (dir == name1) ? name2 : name1;\n> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);\n> +               strbuf_addstr(&dirnfile, dir);\n> +               if (dirnfile.buf[dirnfile.len - 1] != '/')\n> +                       strbuf_addch(&dirnfile, '/');\n> +               filename = strrchr(file, '/');\n> +               strbuf_addstr(&dirnfile, filename ? (filename + 1) : file);\n> +               ret = queue_diff(o, dirnfile.buf, file);\n\nHmm, it appears that you are turning \"diff F D/\" into \"diff D/F F\",\nwhich is the other way around here, or am I mis-reading queue_diff().\n\nDoes queue_diff() do the right thing when D/F does not exist (not a\nrhetorical question; I just did not check it myself)?\n"},{"id":"257752","messageId":"CAPig+cRFCktpG2ksNnRZiFxDqmQnq38MafkA1E-LC6CHtcuk9g@mail.gmail.com","threadId":"38813","inReplyTo":"CAHLaBN+RVpDrG9OewUS7LCYaEOvVqsTY3znapgMj7VrMJWHaDw@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-15T21:28:43Z","receivedAt":"2015-03-15T21:28:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[re-adding cc:git]\n\nOn Sun, Mar 15, 2015 at 2:45 PM, Yurii Shevtsov <ungetch@gmail.com> wrote:\n>> On Sun, Mar 15, 2015 at 11:35 AM, Yurii Shevtsov <ungetch@gmail.com> wrote:\n>>> make \"git diff --no-index $directory $file\" DWIM better.\n>>\n>> Specify the area affected by the change, followed by a colon, followed\n>> by the change summary. Drop the period at the end. So, something like:\n>>\n>>     diff: improve '--no-index <directory> <file>' DWIMing\n>>\n>>> Changes 'git diff --no-index $directory $file' behaviour.\n>>> Now it is transformed to 'git diff --no-index $directory/&file $file'\n>>> instead of throwing an error.\n>>\n>> Write in imperative mood, so \"Change\" rather than \"Changes\". By\n>> itself, the first sentence isn't saying much; it would read better if\n>> you combined the two sentences into one.\n>\n> Got it! My commit message requires improvements\n>>> ---\n>>> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n>>> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n>>> +       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {\n>>> +               struct strbuf dirnfile;\n>>\n>> Is this name supposed to stand for \"dir'n'file\", a shorthand for\n>> \"dir-and-file\"? Perhaps a simpler more idiomatic name such as \"path\"\n>> would suffice. Also, you can initialize the strbuf here at point of\n>> declaration:\n>>\n>>     struct strbuf path = STRBUF_INIT;\n>\n> Yes it is supposed to be \"dir-and-file\" I thought \"path\" isn't\n> descriptive enough because it could be path to dir. But if you insist,\n> no problems\n\nThe reason I asked was because it is not uncommon for variable names\nwith an 'n' suffix to mean \"length\" of something, so the 'n' in 'dirn'\nwas a bit confusing. I personally find the idiomatic name 'path'\neasier to grok, however, Junio, of course, has final say-so. It's okay\nto argue for your choice in naming if you feel strongly that it is\nbetter.\n\n>>> +               const char *dir, *file;\n>>> +               char *filename;\n>>> +               int ret = 0;\n>>> +\n>>> +               dir = S_ISDIR(mode1) ? name1 : name2;\n>>> +               file = (dir == name1) ? name2 : name1;\n>>> +               strbuf_init(&dirnfile, strlen(name1) + strlen(name2) + 2);\n>>\n>> Unless this is a performance critical loop where you want to avoid\n>> multiple re-allocations, it's customary to pass 0 for the second\n>> argument. Doing so makes the code a bit easier to read and understand,\n>> and avoids questions like this one: Why are you adding 2 to the\n>> requested length? I presume that you're taking the \"/\" and NUL into\n>> account, however, strbuf takes NUL into account on its own as part of\n>> its contract, so you probably wanted to add only 1.\n>\n> Yes I thought about performance. I thought it is reasonable since I\n> always know size of resulting string. Checked strbuf.c one more time,\n> yoг are right I should really add only 1\n\nA few reasons I probably would just pass 0 in this case: (1) this\nstring construction is not performance critical; (2) a person reading\nthe code has to spend extra time double-checking the math; (3) the\nmath may become outdated if someone later alters the string\nconstruction in some way, thus it's a potential maintenance burden;\n(4) terser code tends to be easier to read and understand at a glance,\nso the less verbose the code, the better.\n\nHowever, as usual, it's entirely acceptable to argue for your version\nif you feel strongly about it.\n\n>>> +               strbuf_addstr(&dirnfile, dir);\n>>> +               if (dirnfile.buf[dirnfile.len - 1] != '/')\n>>\n>> I don't know how others feel about it, but it makes me a bit\n>> uncomfortable to see potential access of array item -1. I suppose, in\n>> this case, neither name will be zero-length, however, I'd still feel\n>> more comfortable seeing that documented formally, either via assert():\n>>\n>>     assert(dirnfile.len > 0);\n>>     if (dirnfile.buf[dirnfile.len - 1] != '/')\n>>\n>> or:\n>>\n>>     if (dirnfile.len > 0 && dirnfile.buf[dirnfile.len - 1] != '/')\n>\n> My fault again\n"},{"id":"257754","messageId":"xmqqzj7dsyyn.fsf@gitster.dls.corp.google.com","threadId":"38813","inReplyTo":"CAPig+cRFCktpG2ksNnRZiFxDqmQnq38MafkA1E-LC6CHtcuk9g@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-15T22:00:32Z","receivedAt":"2015-03-15T22:00:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>>> Is this name supposed to stand for \"dir'n'file\",...\n> ...I personally find the idiomatic name 'path'\n> easier to grok, however, Junio, of course, has final say-so.\n\nIf I were presented two identical patches, one calling it \"path\" and\nthe other calling it \"dirnfile\", I would definitely take the former,\nbut I agree that this is more of a preference than a taste, the\nlatter of which implies that you could make a value judgement,\ni.e. \"good taste\" vs \"bad taste\".\n\nIf for some reason we had a code that called a variable \"dirnfile\"\nalready in our official codebase and we saw a patch to \"correct\"\nthat to \"path\", I would likely say that it is not worth the churn to\napply such a \"correction\" patch.  If the new name were \"pathname\",\nhowever, I might be pursuaded to take it, simply because a\n\"pathname\" is a lot more familiar word than \"dirnfile\".\n"},{"id":"257767","messageId":"xmqqr3spsir2.fsf@gitster.dls.corp.google.com","threadId":"38813","inReplyTo":"vpq1tkq5fsw.fsf@anie.imag.fr","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-16T03:50:41Z","receivedAt":"2015-03-16T03:50:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n>> --- a/diff-no-index.c\n>> +++ b/diff-no-index.c\n>> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n>>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n>>                 return -1;\n>>\n>> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n>> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n>\n> I'm surprised to see this error message totally go away. The idea of the\n> microproject was to DWIM (do what I mean) better, but the dwim should\n> apply only when $directory/$file actually exists. Otherwise, the error\n> message should actually be raised.\n\nI actually think this check, when we really fixed \"diff --no-index\"\nto work sensibly, should go away and be replaced with something\nsensible.  As it stands now, even before we think about dwimming\n\"diff D/ F\" into \"diff D/F F\", a simple formulation like this will\nerror out.\n\n    $ mkdir -p a/sub b\n    $ touch a/file b/file b/sub a/sub/file\n    $ git diff --no-index a b\n    error: file/directory conflict: a/sub, b/sub\n\nAdmittedly, that is how ordinary \"diff -r\" works, but I am not sure\nif we want to emulate that aspect of GNU diff.  If the old tree a\nhas a directory 'sub' with 'file' under it (i.e. a/sub/file) where\nthe new tree has a file at 'sub', then the recursive diff can show\nthe removal of sub/file and creation of sub, no?  That is what we\nshow for normal \"git diff\".\n\nBut I _think_ fixing that is way outside the scope of GSoC Micro.\n\nAnd patching this function, because it is recursively called from\nwithin it, to \"dwim\" is simply wrong.  When we see a/sub that is a\ndirectory and b/sub that is a file, we do *NOT* want to rewrite the\ncomparison to comparison between a/sub/sub and b/sub.\n\nWhat needs to be fixed for the Micro is the top-level call to\nqueue_diff() that is made blindly between paths[0] and paths[1]\nwithout checking what kind of things these are.\n\n\n\n    \n"},{"id":"257797","messageId":"CAHLaBNJxRx9jkNHCM+djq7KEZBV2n5PFZN0-UUtzhO=ikR+Kuw@mail.gmail.com","threadId":"38813","inReplyTo":"xmqqr3spsir2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Yurii Shevtsov","fromEmail":"ungetch@gmail.com","sentAt":"2015-03-16T16:23:49Z","receivedAt":"2015-03-16T16:23:49Z","isPatch":true,"sender":{"key":"ungetch@gmail.com","avatar":null},"body":"> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n>>> --- a/diff-no-index.c\n>>> +++ b/diff-no-index.c\n>>> @@ -97,8 +97,25 @@ static int queue_diff(struct diff_options *o,\n>>>         if (get_mode(name1, &mode1) || get_mode(name2, &mode2))\n>>>                 return -1;\n>>>\n>>> -       if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2))\n>>> -               return error(\"file/directory conflict: %s, %s\", name1, name2);\n>>\n>> I'm surprised to see this error message totally go away. The idea of the\n>> microproject was to DWIM (do what I mean) better, but the dwim should\n>> apply only when $directory/$file actually exists. Otherwise, the error\n>> message should actually be raised.\n>\n> I actually think this check, when we really fixed \"diff --no-index\"\n> to work sensibly, should go away and be replaced with something\n> sensible.  As it stands now, even before we think about dwimming\n> \"diff D/ F\" into \"diff D/F F\", a simple formulation like this will\n> error out.\n>\n>     $ mkdir -p a/sub b\n>     $ touch a/file b/file b/sub a/sub/file\n>     $ git diff --no-index a b\n>     error: file/directory conflict: a/sub, b/sub\n>\n> Admittedly, that is how ordinary \"diff -r\" works, but I am not sure\n> if we want to emulate that aspect of GNU diff.  If the old tree a\n> has a directory 'sub' with 'file' under it (i.e. a/sub/file) where\n> the new tree has a file at 'sub', then the recursive diff can show\n> the removal of sub/file and creation of sub, no?  That is what we\n> show for normal \"git diff\".\n>\n> But I _think_ fixing that is way outside the scope of GSoC Micro.\n>\n> And patching this function, because it is recursively called from\n> within it, to \"dwim\" is simply wrong.  When we see a/sub that is a\n> directory and b/sub that is a file, we do *NOT* want to rewrite the\n> comparison to comparison between a/sub/sub and b/sub.\n>\n> What needs to be fixed for the Micro is the top-level call to\n> queue_diff() that is made blindly between paths[0] and paths[1]\n> without checking what kind of things these are.\n\nSo you want me to convert args (\"diff D/ F\" into \"diff D/F F\") before\ncalling queue_diff()? But why? queue_diff() already check args' types\nand decides which comparison to do. If I understood you right, it\nwould require calling get_mode() in diff_no_index(), while get_mode()\nwill be called again in queue_diff().\n"},{"id":"257801","messageId":"xmqq61a0sw48.fsf@gitster.dls.corp.google.com","threadId":"38813","inReplyTo":"CAHLaBNJxRx9jkNHCM+djq7KEZBV2n5PFZN0-UUtzhO=ikR+Kuw@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-16T17:14:15Z","receivedAt":"2015-03-16T17:14:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yurii Shevtsov <ungetch@gmail.com> writes:\n\n>> ...  As it stands now, even before we think about dwimming\n>> \"diff D/ F\" into \"diff D/F F\", a simple formulation like this will\n>> error out.\n>>\n>>     $ mkdir -p a/sub b\n>>     $ touch a/file b/file b/sub a/sub/file\n>>     $ git diff --no-index a b\n>>     error: file/directory conflict: a/sub, b/sub\n>>\n>> Admittedly, that is how ordinary \"diff -r\" works, but I am not sure\n>> if we want to emulate that aspect of GNU diff.  If the old tree a\n>> has a directory 'sub' with 'file' under it (i.e. a/sub/file) where\n>> the new tree has a file at 'sub', then the recursive diff can show\n>> the removal of sub/file and creation of sub, no?  That is what we\n>> show for normal \"git diff\".\n>>\n>> But I _think_ fixing that is way outside the scope of GSoC Micro.\n> \n> So you want me to convert args (\"diff D/ F\" into \"diff D/F F\") before\n> calling queue_diff()? But why?\n\nBecause it is wrong to do this inside queue_diff()?\n\nHave you actually read what I wrote, tried the above sample\nscenario, and thought what is happening in the codepath?\n\nWhen the user asks to compare directory a/ and b/, the top-level\ndiff_no_index() would have paths[0]==\"a\" and paths[1]==\"b\", and\nqueue_diff() is called with these in name1 and name2.  Once it\nlearns that both of these are directories, it _recurses_ into itself\nby appending the paths in these directories after these two names.\nIt finds that both of these directories have \"sub\" underneath, so it\nmakes a recursive call to itself to compare \"a/sub\" and \"b/sub\".\n\nThat call would notice that one is a directory and the other is\nnot.  That is where you are getting the \"f/d conflict\" error.\n\nAt that point, do you think it is sensible to rewrite that recursed\npart of the diff into a comparison between \"a/sub/sub\" (which does\nnot exist, and which the user did not mean to compare with b/sub)\nand \"b/sub\" (which is a file)?  I hope not.\n\n> queue_diff() already check args' types and decides which\n> comparison to do.\n\nYes, and I already hinted that that is an independent issue we may\nwant to fix, which I suspect is larger than GSoC Micro.  Also the\nfix would be different.  Right now, it checks the types of paths and\nthen refuses to compare a directory and a file.  If we wanted to\nchange it to closer to what the rest of Git does, we would want it\nto report that the directory and everything under it are removed and\nthen a new file is created (if the directory is on the left hand\nside of the comparision and the file is on the right hand side) or\nthe other way around.  That will not involve \"append the name of the\nfile at the end of the directory\".\n\nIn short, \"append the name of the file at the end of the directory\"\nlogic has no place to live inside queue_diff() which handles the\nrecursion part of the operation.\n"},{"id":"257805","messageId":"CAHLaBN+FZ5LLwU-6VnFoPva=omtPpCEPzmdvY_3H43dag8kqxg@mail.gmail.com","threadId":"38813","inReplyTo":"xmqq61a0sw48.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Yurii Shevtsov","fromEmail":"ungetch@gmail.com","sentAt":"2015-03-16T17:47:27Z","receivedAt":"2015-03-16T17:47:27Z","isPatch":true,"sender":{"key":"ungetch@gmail.com","avatar":null},"body":"Yes, I have red what you wrote several times and tried your example.\nI'm really sorry if I sound like I just ignored it. I just got a\nlittle bit lost about which procedure needs patching. You're\nabsolutely right, queue_diff() is wrong place for it. So do you agree\nthat \"append the name of the file at the end of the directory\" logic\nshould be put to diff_no_index() which will also include calling\nget_mode() for each path[] member? Sorry again for asking so much\nquestions\n\n2015-03-16 19:14 GMT+02:00 Junio C Hamano <gitster@pobox.com>:\n> Yurii Shevtsov <ungetch@gmail.com> writes:\n>\n>>> ...  As it stands now, even before we think about dwimming\n>>> \"diff D/ F\" into \"diff D/F F\", a simple formulation like this will\n>>> error out.\n>>>\n>>>     $ mkdir -p a/sub b\n>>>     $ touch a/file b/file b/sub a/sub/file\n>>>     $ git diff --no-index a b\n>>>     error: file/directory conflict: a/sub, b/sub\n>>>\n>>> Admittedly, that is how ordinary \"diff -r\" works, but I am not sure\n>>> if we want to emulate that aspect of GNU diff.  If the old tree a\n>>> has a directory 'sub' with 'file' under it (i.e. a/sub/file) where\n>>> the new tree has a file at 'sub', then the recursive diff can show\n>>> the removal of sub/file and creation of sub, no?  That is what we\n>>> show for normal \"git diff\".\n>>>\n>>> But I _think_ fixing that is way outside the scope of GSoC Micro.\n>>\n>> So you want me to convert args (\"diff D/ F\" into \"diff D/F F\") before\n>> calling queue_diff()? But why?\n>\n> Because it is wrong to do this inside queue_diff()?\n>\n> Have you actually read what I wrote, tried the above sample\n> scenario, and thought what is happening in the codepath?\n>\n> When the user asks to compare directory a/ and b/, the top-level\n> diff_no_index() would have paths[0]==\"a\" and paths[1]==\"b\", and\n> queue_diff() is called with these in name1 and name2.  Once it\n> learns that both of these are directories, it _recurses_ into itself\n> by appending the paths in these directories after these two names.\n> It finds that both of these directories have \"sub\" underneath, so it\n> makes a recursive call to itself to compare \"a/sub\" and \"b/sub\".\n>\n> That call would notice that one is a directory and the other is\n> not.  That is where you are getting the \"f/d conflict\" error.\n>\n> At that point, do you think it is sensible to rewrite that recursed\n> part of the diff into a comparison between \"a/sub/sub\" (which does\n> not exist, and which the user did not mean to compare with b/sub)\n> and \"b/sub\" (which is a file)?  I hope not.\n>\n>> queue_diff() already check args' types and decides which\n>> comparison to do.\n>\n> Yes, and I already hinted that that is an independent issue we may\n> want to fix, which I suspect is larger than GSoC Micro.  Also the\n> fix would be different.  Right now, it checks the types of paths and\n> then refuses to compare a directory and a file.  If we wanted to\n> change it to closer to what the rest of Git does, we would want it\n> to report that the directory and everything under it are removed and\n> then a new file is created (if the directory is on the left hand\n> side of the comparision and the file is on the right hand side) or\n> the other way around.  That will not involve \"append the name of the\n> file at the end of the directory\".\n>\n> In short, \"append the name of the file at the end of the directory\"\n> logic has no place to live inside queue_diff() which handles the\n> recursion part of the operation.\n"},{"id":"257814","messageId":"xmqq3854rbpj.fsf@gitster.dls.corp.google.com","threadId":"38813","inReplyTo":"CAHLaBN+FZ5LLwU-6VnFoPva=omtPpCEPzmdvY_3H43dag8kqxg@mail.gmail.com","subject":"Re: [PATCH/RFC][GSoC] make \"git diff --no-index $directory $file\" DWIM better.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-16T19:20:24Z","receivedAt":"2015-03-16T19:20:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yurii Shevtsov <ungetch@gmail.com> writes:\n\n> Yes, I have red what you wrote several times and tried your example.\n> I'm really sorry if I sound like I just ignored it. I just got a\n> little bit lost about which procedure needs patching. You're\n> absolutely right, queue_diff() is wrong place for it. So do you agree\n> that \"append the name of the file at the end of the directory\" logic\n> should be put to diff_no_index() which will also include calling\n> get_mode() for each path[] member? Sorry again for asking so much\n> questions\n\nQuestions are to be asked; no need to apologize.\n\nI think that the \"if asked to compare D and F, pretend as if asked\nto compare D/F and F\" and friends (meaning, \"if asked to compare F\nand D, compare F and D/F\" needs to be handled the same way, and also\nit needs to handle the case where \"D/F\" does *not* exist) logic can\nbe added to diff_no_index() just before it calls queue_diff().\n\n\"If one is directory but not the other, return an error\" code may\nneed to be fixed but I think that would be a larger change than a\nfew hours work (which I understand is the size GSoC Micro aims for).\n"}]}