{"thread":{"id":"34901","subject":"[PATCH 0/2] reset: handle submodule with trailing slash","startedAt":"2013-09-10T19:13:16Z","lastAt":"2013-09-13T08:48:34Z","messageCount":27,"participants":["John Keeping","Jens Lehmann","Johannes Sixt","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"227355","messageId":"cover.1378840318.git.john@keeping.me.uk","threadId":"34901","inReplyTo":null,"subject":"[PATCH 0/2] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-10T19:13:16Z","receivedAt":"2013-09-10T19:13:16Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The first patch is the important one here, the second one I noticed\nwhile checking if any other commands fail to handle submodule paths with\na trailing slash and is just a simplification.\n\nJohn Keeping (2):\n  reset: handle submodule with trailing slash\n  rm: re-use parse_pathspec's trailing-slash removal\n\n builtin/reset.c            |  5 +++++\n builtin/rm.c               | 20 ++++----------------\n t/t7400-submodule-basic.sh |  6 ++++--\n 3 files changed, 13 insertions(+), 18 deletions(-)\n\n-- \n1.8.2\n"},{"id":"227357","messageId":"c7e026f44f9ccbf5736b72e728a360b31887a50f.1378840318.git.john@keeping.me.uk","threadId":"34901","inReplyTo":"cover.1378840318.git.john@keeping.me.uk","subject":"[PATCH 1/2] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-10T19:13:17Z","receivedAt":"2013-09-10T19:13:17Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When using tab-completion, a directory path will often end with a\ntrailing slash which currently confuses \"git rm\" when dealing with\nsubmodules.  Now that we have parse_pathspec we can easily handle this\nby simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/reset.c            | 5 +++++\n t/t7400-submodule-basic.sh | 6 ++++--\n 2 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 5e4c551..9efac0f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,\n \t\t}\n \t}\n \t*rev_ret = rev;\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n \tparse_pathspec(pathspec, 0,\n \t\t       PATHSPEC_PREFER_FULL |\n+\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n \t\t       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),\n \t\t       prefix, argv);\n }\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 4192fe0..c268d3c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '\n \n '\n \n-test_expect_success 'gracefully add submodule with a trailing slash' '\n+test_expect_success 'gracefully add/reset submodule with a trailing slash' '\n \n \tgit reset --hard &&\n \tgit commit -m \"commit subproject\" init &&\n@@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '\n \tgit add init/ &&\n \ttest_must_fail git diff --exit-code --cached init &&\n \ttest $commit = $(git ls-files --stage |\n-\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\")\n+\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\") &&\n+\tgit reset init/ &&\n+\tgit diff --exit-code --cached init\n \n '\n \n-- \n1.8.2\n"},{"id":"227356","messageId":"b16901cdc3d433a8e0f7078475cb06f90b4590dd.1378840318.git.john@keeping.me.uk","threadId":"34901","inReplyTo":"cover.1378840318.git.john@keeping.me.uk","subject":"[PATCH 2/2] rm: re-use parse_pathspec's trailing-slash removal","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-10T19:13:18Z","receivedAt":"2013-09-10T19:13:18Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Instead of re-implementing the \"remove trailing slashes\" loop in\nbuiltin/rm.c just pass PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to\nparse_pathspec.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/rm.c | 20 ++++----------------\n 1 file changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 9b59ab3..3a0e0ea 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -298,22 +298,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \n-\t/*\n-\t * Drop trailing directory separators from directories so we'll find\n-\t * submodules in the index.\n-\t */\n-\tfor (i = 0; i < argc; i++) {\n-\t\tsize_t pathlen = strlen(argv[i]);\n-\t\tif (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&\n-\t\t    is_directory(argv[i])) {\n-\t\t\tdo {\n-\t\t\t\tpathlen--;\n-\t\t\t} while (pathlen && is_dir_sep(argv[i][pathlen - 1]));\n-\t\t\targv[i] = xmemdupz(argv[i], pathlen);\n-\t\t}\n-\t}\n-\n-\tparse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);\n+\tparse_pathspec(&pathspec, 0,\n+\t\t       PATHSPEC_PREFER_CWD |\n+\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n+\t\t       prefix, argv);\n \trefresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n \n \tseen = xcalloc(pathspec.nr, 1);\n-- \n1.8.2\n"},{"id":"227358","messageId":"522F7509.1050100@web.de","threadId":"34901","inReplyTo":"c7e026f44f9ccbf5736b72e728a360b31887a50f.1378840318.git.john@keeping.me.uk","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-09-10T19:37:45Z","receivedAt":"2013-09-10T19:37:45Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 10.09.2013 21:13, schrieb John Keeping:\n> When using tab-completion, a directory path will often end with a\n> trailing slash which currently confuses \"git rm\" when dealing with\n\nI think you meant to say \"git reset\" in the line above. Apart from\nthat I'm all for it.\n\n> submodules.  Now that we have parse_pathspec we can easily handle this\n> by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n> \n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  builtin/reset.c            | 5 +++++\n>  t/t7400-submodule-basic.sh | 6 ++++--\n>  2 files changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 5e4c551..9efac0f 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,\n>  \t\t}\n>  \t}\n>  \t*rev_ret = rev;\n> +\n> +\tif (read_cache() < 0)\n> +\t\tdie(_(\"index file corrupt\"));\n> +\n>  \tparse_pathspec(pathspec, 0,\n>  \t\t       PATHSPEC_PREFER_FULL |\n> +\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n>  \t\t       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),\n>  \t\t       prefix, argv);\n>  }\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 4192fe0..c268d3c 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '\n>  \n>  '\n>  \n> -test_expect_success 'gracefully add submodule with a trailing slash' '\n> +test_expect_success 'gracefully add/reset submodule with a trailing slash' '\n>  \n>  \tgit reset --hard &&\n>  \tgit commit -m \"commit subproject\" init &&\n> @@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '\n>  \tgit add init/ &&\n>  \ttest_must_fail git diff --exit-code --cached init &&\n>  \ttest $commit = $(git ls-files --stage |\n> -\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\")\n> +\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\") &&\n> +\tgit reset init/ &&\n> +\tgit diff --exit-code --cached init\n>  \n>  '\n>  \n> \n"},{"id":"227360","messageId":"20130910194652.GP2582@serenity.lan","threadId":"34901","inReplyTo":"522F7509.1050100@web.de","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-10T19:46:52Z","receivedAt":"2013-09-10T19:46:52Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Sep 10, 2013 at 09:37:45PM +0200, Jens Lehmann wrote:\n> Am 10.09.2013 21:13, schrieb John Keeping:\n> > When using tab-completion, a directory path will often end with a\n> > trailing slash which currently confuses \"git rm\" when dealing with\n> \n> I think you meant to say \"git reset\" in the line above. Apart from\n> that I'm all for it.\n\nYeah, you're right - I obviously got confused between the two patches :-(.\nI'll wait for more feedback before submitting a re-roll.\n\n> > submodules.  Now that we have parse_pathspec we can easily handle this\n> > by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n> > \n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> >  builtin/reset.c            | 5 +++++\n> >  t/t7400-submodule-basic.sh | 6 ++++--\n> >  2 files changed, 9 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/builtin/reset.c b/builtin/reset.c\n> > index 5e4c551..9efac0f 100644\n> > --- a/builtin/reset.c\n> > +++ b/builtin/reset.c\n> > @@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,\n> >  \t\t}\n> >  \t}\n> >  \t*rev_ret = rev;\n> > +\n> > +\tif (read_cache() < 0)\n> > +\t\tdie(_(\"index file corrupt\"));\n> > +\n> >  \tparse_pathspec(pathspec, 0,\n> >  \t\t       PATHSPEC_PREFER_FULL |\n> > +\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n> >  \t\t       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),\n> >  \t\t       prefix, argv);\n> >  }\n> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> > index 4192fe0..c268d3c 100755\n> > --- a/t/t7400-submodule-basic.sh\n> > +++ b/t/t7400-submodule-basic.sh\n> > @@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '\n> >  \n> >  '\n> >  \n> > -test_expect_success 'gracefully add submodule with a trailing slash' '\n> > +test_expect_success 'gracefully add/reset submodule with a trailing slash' '\n> >  \n> >  \tgit reset --hard &&\n> >  \tgit commit -m \"commit subproject\" init &&\n> > @@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '\n> >  \tgit add init/ &&\n> >  \ttest_must_fail git diff --exit-code --cached init &&\n> >  \ttest $commit = $(git ls-files --stage |\n> > -\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\")\n> > +\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\") &&\n> > +\tgit reset init/ &&\n> > +\tgit diff --exit-code --cached init\n> >  \n> >  '\n> >  \n> > \n"},{"id":"227407","messageId":"52300838.5040703@kdbg.org","threadId":"34901","inReplyTo":"c7e026f44f9ccbf5736b72e728a360b31887a50f.1378840318.git.john@keeping.me.uk","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-09-11T06:05:44Z","receivedAt":"2013-09-11T06:05:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.09.2013 21:13, schrieb John Keeping:\n> When using tab-completion, a directory path will often end with a\n> trailing slash which currently confuses \"git rm\" when dealing with\n> submodules.  Now that we have parse_pathspec we can easily handle this\n> by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n> \n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  builtin/reset.c            | 5 +++++\n>  t/t7400-submodule-basic.sh | 6 ++++--\n>  2 files changed, 9 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 5e4c551..9efac0f 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,\n>  \t\t}\n>  \t}\n>  \t*rev_ret = rev;\n> +\n> +\tif (read_cache() < 0)\n> +\t\tdie(_(\"index file corrupt\"));\n\nWhen the index is now read here, I would have expected hunk in this\npatch that removes a read_cache() invocation.\n\n> +\n>  \tparse_pathspec(pathspec, 0,\n>  \t\t       PATHSPEC_PREFER_FULL |\n> +\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n>  \t\t       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),\n>  \t\t       prefix, argv);\n>  }\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 4192fe0..c268d3c 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '\n>  \n>  '\n>  \n> -test_expect_success 'gracefully add submodule with a trailing slash' '\n> +test_expect_success 'gracefully add/reset submodule with a trailing slash' '\n>  \n>  \tgit reset --hard &&\n>  \tgit commit -m \"commit subproject\" init &&\n> @@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '\n>  \tgit add init/ &&\n>  \ttest_must_fail git diff --exit-code --cached init &&\n>  \ttest $commit = $(git ls-files --stage |\n> -\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\")\n> +\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\") &&\n> +\tgit reset init/ &&\n> +\tgit diff --exit-code --cached init\n>  \n>  '\n>  \n> \n"},{"id":"227432","messageId":"CACsJy8Dw+RJor-XfjFQC5U5Pt39TZ656fEnkFxpDnx=kTqvADQ@mail.gmail.com","threadId":"34901","inReplyTo":"b16901cdc3d433a8e0f7078475cb06f90b4590dd.1378840318.git.john@keeping.me.uk","subject":"Re: [PATCH 2/2] rm: re-use parse_pathspec's trailing-slash removal","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-11T07:48:51Z","receivedAt":"2013-09-11T07:48:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 11, 2013 at 2:13 AM, John Keeping <john@keeping.me.uk> wrote:\n> Instead of re-implementing the \"remove trailing slashes\" loop in\n> builtin/rm.c just pass PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to\n> parse_pathspec.\n>\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  builtin/rm.c | 20 ++++----------------\n>  1 file changed, 4 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/rm.c b/builtin/rm.c\n> index 9b59ab3..3a0e0ea 100644\n> --- a/builtin/rm.c\n> +++ b/builtin/rm.c\n> @@ -298,22 +298,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n>         if (read_cache() < 0)\n>                 die(_(\"index file corrupt\"));\n>\n> -       /*\n> -        * Drop trailing directory separators from directories so we'll find\n> -        * submodules in the index.\n> -        */\n> -       for (i = 0; i < argc; i++) {\n> -               size_t pathlen = strlen(argv[i]);\n> -               if (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&\n> -                   is_directory(argv[i])) {\n> -                       do {\n> -                               pathlen--;\n> -                       } while (pathlen && is_dir_sep(argv[i][pathlen - 1]));\n> -                       argv[i] = xmemdupz(argv[i], pathlen);\n> -               }\n> -       }\n> -\n> -       parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);\n> +       parse_pathspec(&pathspec, 0,\n> +                      PATHSPEC_PREFER_CWD |\n> +                      PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n\nI notice that _CHEAP implementation and the removed code are not\nexactly the same. But I think they have the same purpose so it's\nprobably ok even there are some subtle behavioral changes.\n\nYou may want to improve _CHEAP to remove consecutive trailing slashes\n(i.e. foo//// -> foo) too. And maybe is is_dir_sep() instead of\nexplicit == '/' comparison in there.\n\n> +                      prefix, argv);\n>         refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n>\n>         seen = xcalloc(pathspec.nr, 1);\n-- \nDuy\n"},{"id":"227434","messageId":"20130911082042.GR2582@serenity.lan","threadId":"34901","inReplyTo":"52300838.5040703@kdbg.org","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-11T08:20:43Z","receivedAt":"2013-09-11T08:20:43Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Sep 11, 2013 at 08:05:44AM +0200, Johannes Sixt wrote:\n> Am 10.09.2013 21:13, schrieb John Keeping:\n> > When using tab-completion, a directory path will often end with a\n> > trailing slash which currently confuses \"git rm\" when dealing with\n> > submodules.  Now that we have parse_pathspec we can easily handle this\n> > by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n> > \n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> >  builtin/reset.c            | 5 +++++\n> >  t/t7400-submodule-basic.sh | 6 ++++--\n> >  2 files changed, 9 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/builtin/reset.c b/builtin/reset.c\n> > index 5e4c551..9efac0f 100644\n> > --- a/builtin/reset.c\n> > +++ b/builtin/reset.c\n> > @@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,\n> >  \t\t}\n> >  \t}\n> >  \t*rev_ret = rev;\n> > +\n> > +\tif (read_cache() < 0)\n> > +\t\tdie(_(\"index file corrupt\"));\n> \n> When the index is now read here, I would have expected hunk in this\n> patch that removes a read_cache() invocation.\n\nI think that needs to look like this on top - there's also a\nread_cache_unmerged() around line 68 but I don't think we can remove\nthat one.\n\n-- >8 --\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 9efac0f..800117f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.output_format = DIFF_FORMAT_CALLBACK;\n \topt.format_callback = update_index_from_diff;\n \n-\tread_cache();\n \tif (do_diff_cache(tree_sha1, &opt))\n \t\treturn 1;\n \tdiffcore_std(&opt);\n@@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,\n \n static void die_if_unmerged_cache(int reset_type)\n {\n-\tif (is_merge() || read_cache() < 0 || unmerged_cache())\n+\tif (is_merge() || unmerged_cache())\n \t\tdie(_(\"Cannot do a %s reset in the middle of a merge.\"),\n \t\t    _(reset_type_names[reset_type]));\n"},{"id":"227435","messageId":"20130911082441.GS2582@serenity.lan","threadId":"34901","inReplyTo":"CACsJy8Dw+RJor-XfjFQC5U5Pt39TZ656fEnkFxpDnx=kTqvADQ@mail.gmail.com","subject":"Re: [PATCH 2/2] rm: re-use parse_pathspec's trailing-slash removal","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-11T08:24:41Z","receivedAt":"2013-09-11T08:24:41Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Sep 11, 2013 at 02:48:51PM +0700, Duy Nguyen wrote:\n> On Wed, Sep 11, 2013 at 2:13 AM, John Keeping <john@keeping.me.uk> wrote:\n> > Instead of re-implementing the \"remove trailing slashes\" loop in\n> > builtin/rm.c just pass PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to\n> > parse_pathspec.\n> >\n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> >  builtin/rm.c | 20 ++++----------------\n> >  1 file changed, 4 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/builtin/rm.c b/builtin/rm.c\n> > index 9b59ab3..3a0e0ea 100644\n> > --- a/builtin/rm.c\n> > +++ b/builtin/rm.c\n> > @@ -298,22 +298,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n> >         if (read_cache() < 0)\n> >                 die(_(\"index file corrupt\"));\n> >\n> > -       /*\n> > -        * Drop trailing directory separators from directories so we'll find\n> > -        * submodules in the index.\n> > -        */\n> > -       for (i = 0; i < argc; i++) {\n> > -               size_t pathlen = strlen(argv[i]);\n> > -               if (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&\n> > -                   is_directory(argv[i])) {\n> > -                       do {\n> > -                               pathlen--;\n> > -                       } while (pathlen && is_dir_sep(argv[i][pathlen - 1]));\n> > -                       argv[i] = xmemdupz(argv[i], pathlen);\n> > -               }\n> > -       }\n> > -\n> > -       parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);\n> > +       parse_pathspec(&pathspec, 0,\n> > +                      PATHSPEC_PREFER_CWD |\n> > +                      PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n> \n> I notice that _CHEAP implementation and the removed code are not\n> exactly the same. But I think they have the same purpose so it's\n> probably ok even there are some subtle behavioral changes.\n\nProviding that there's only one trailing slash, the user-visible effect\nshould be the same since the only case affected by that is submodules.\nIn fact _CHEAP does better in the case where the submodule does not\nexist in the working tree.\n\n> You may want to improve _CHEAP to remove consecutive trailing slashes\n> (i.e. foo//// -> foo) too. And maybe is is_dir_sep() instead of\n> explicit == '/' comparison in there.\n\nSounds good, I'll try to look at that tonight.\n\n> > +                      prefix, argv);\n> >         refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n> >\n> >         seen = xcalloc(pathspec.nr, 1);\n"},{"id":"227441","messageId":"CACsJy8BgEM3eEDo8wOgkqYTL1fkh9azZNqbogxBubp9g5KRNbQ@mail.gmail.com","threadId":"34901","inReplyTo":"20130911082042.GR2582@serenity.lan","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-11T10:54:48Z","receivedAt":"2013-09-11T10:54:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 11, 2013 at 3:20 PM, John Keeping <john@keeping.me.uk> wrote:\n> On Wed, Sep 11, 2013 at 08:05:44AM +0200, Johannes Sixt wrote:\n>> Am 10.09.2013 21:13, schrieb John Keeping:\n>> > When using tab-completion, a directory path will often end with a\n>> > trailing slash which currently confuses \"git rm\" when dealing with\n>> > submodules.  Now that we have parse_pathspec we can easily handle this\n>> > by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n>> >\n>> > Signed-off-by: John Keeping <john@keeping.me.uk>\n>> > ---\n>> >  builtin/reset.c            | 5 +++++\n>> >  t/t7400-submodule-basic.sh | 6 ++++--\n>> >  2 files changed, 9 insertions(+), 2 deletions(-)\n>> >\n>> > diff --git a/builtin/reset.c b/builtin/reset.c\n>> > index 5e4c551..9efac0f 100644\n>> > --- a/builtin/reset.c\n>> > +++ b/builtin/reset.c\n>> > @@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,\n>> >             }\n>> >     }\n>> >     *rev_ret = rev;\n>> > +\n>> > +   if (read_cache() < 0)\n>> > +           die(_(\"index file corrupt\"));\n>>\n>> When the index is now read here, I would have expected hunk in this\n>> patch that removes a read_cache() invocation.\n>\n> I think that needs to look like this on top - there's also a\n> read_cache_unmerged() around line 68 but I don't think we can remove\n> that one.\n>\n> -- >8 --\n> diff --git a/builtin/reset.c b/builtin/reset.c\n> index 9efac0f..800117f 100644\n> --- a/builtin/reset.c\n> +++ b/builtin/reset.c\n> @@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n>         opt.output_format = DIFF_FORMAT_CALLBACK;\n>         opt.format_callback = update_index_from_diff;\n>\n> -       read_cache();\n>         if (do_diff_cache(tree_sha1, &opt))\n>                 return 1;\n>         diffcore_std(&opt);\n> @@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,\n>\n>  static void die_if_unmerged_cache(int reset_type)\n>  {\n> -       if (is_merge() || read_cache() < 0 || unmerged_cache())\n> +       if (is_merge() || unmerged_cache())\n>                 die(_(\"Cannot do a %s reset in the middle of a merge.\"),\n>                     _(reset_type_names[reset_type]));\n\nreset --soft does not go through these code paths (i.e. it does not\nneed index at all). If we fail to load index index in \"reset --soft\" I\nthink it's ok to die(). Corrupt index is fatal anyway. But \"reset\n--soft\" now has to pay the cost to load index, which could be slow\nwhen the index is big. Assuming nobody does \"reset --soft\" that often\nI think this is OK.\n\nAlternatively we could load index lazily in _CHEAP code only when we\nsee trailing slashes, then replace these read_cache() with\nread_cache_unless_its_already_loaded_earlier() or something.\n-- \nDuy\n"},{"id":"227442","messageId":"20130911110806.GT2582@serenity.lan","threadId":"34901","inReplyTo":"CACsJy8BgEM3eEDo8wOgkqYTL1fkh9azZNqbogxBubp9g5KRNbQ@mail.gmail.com","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-11T11:08:06Z","receivedAt":"2013-09-11T11:08:06Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Sep 11, 2013 at 05:54:48PM +0700, Duy Nguyen wrote:\n> On Wed, Sep 11, 2013 at 3:20 PM, John Keeping <john@keeping.me.uk> wrote:\n> > On Wed, Sep 11, 2013 at 08:05:44AM +0200, Johannes Sixt wrote:\n> >> Am 10.09.2013 21:13, schrieb John Keeping:\n> >> > When using tab-completion, a directory path will often end with a\n> >> > trailing slash which currently confuses \"git rm\" when dealing with\n> >> > submodules.  Now that we have parse_pathspec we can easily handle this\n> >> > by simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n> >> >\n> >> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> >> > ---\n> >> >  builtin/reset.c            | 5 +++++\n> >> >  t/t7400-submodule-basic.sh | 6 ++++--\n> >> >  2 files changed, 9 insertions(+), 2 deletions(-)\n> >> >\n> >> > diff --git a/builtin/reset.c b/builtin/reset.c\n> >> > index 5e4c551..9efac0f 100644\n> >> > --- a/builtin/reset.c\n> >> > +++ b/builtin/reset.c\n> >> > @@ -220,8 +220,13 @@ static void parse_args(struct pathspec *pathspec,\n> >> >             }\n> >> >     }\n> >> >     *rev_ret = rev;\n> >> > +\n> >> > +   if (read_cache() < 0)\n> >> > +           die(_(\"index file corrupt\"));\n> >>\n> >> When the index is now read here, I would have expected hunk in this\n> >> patch that removes a read_cache() invocation.\n> >\n> > I think that needs to look like this on top - there's also a\n> > read_cache_unmerged() around line 68 but I don't think we can remove\n> > that one.\n> >\n> > -- >8 --\n> > diff --git a/builtin/reset.c b/builtin/reset.c\n> > index 9efac0f..800117f 100644\n> > --- a/builtin/reset.c\n> > +++ b/builtin/reset.c\n> > @@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n> >         opt.output_format = DIFF_FORMAT_CALLBACK;\n> >         opt.format_callback = update_index_from_diff;\n> >\n> > -       read_cache();\n> >         if (do_diff_cache(tree_sha1, &opt))\n> >                 return 1;\n> >         diffcore_std(&opt);\n> > @@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,\n> >\n> >  static void die_if_unmerged_cache(int reset_type)\n> >  {\n> > -       if (is_merge() || read_cache() < 0 || unmerged_cache())\n> > +       if (is_merge() || unmerged_cache())\n> >                 die(_(\"Cannot do a %s reset in the middle of a merge.\"),\n> >                     _(reset_type_names[reset_type]));\n> \n> reset --soft does not go through these code paths (i.e. it does not\n> need index at all). If we fail to load index index in \"reset --soft\" I\n> think it's ok to die(). Corrupt index is fatal anyway. But \"reset\n> --soft\" now has to pay the cost to load index, which could be slow\n> when the index is big. Assuming nobody does \"reset --soft\" that often\n> I think this is OK.\n> \n> Alternatively we could load index lazily in _CHEAP code only when we\n> see trailing slashes, then replace these read_cache() with\n> read_cache_unless_its_already_loaded_earlier() or something.\n\nread_cache() already has an early return if the index is already loaded\nso I don't think we need to worry about a special function for that.\n\nI'm not sure it's worth optimizing this case too heavily, but it might\nbe a nice change to make parse_pathspec() not rely on the index being\nloaded before it is called with certain flags.\n"},{"id":"227443","messageId":"CACsJy8BQWSthAKePsBDyc5nAofLiTpXO5oc+jbSGx33YBioTeg@mail.gmail.com","threadId":"34901","inReplyTo":"20130911110806.GT2582@serenity.lan","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-11T11:20:44Z","receivedAt":"2013-09-11T11:20:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 11, 2013 at 6:08 PM, John Keeping <john@keeping.me.uk> wrote:\n>> > -- >8 --\n>> > diff --git a/builtin/reset.c b/builtin/reset.c\n>> > index 9efac0f..800117f 100644\n>> > --- a/builtin/reset.c\n>> > +++ b/builtin/reset.c\n>> > @@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n>> >         opt.output_format = DIFF_FORMAT_CALLBACK;\n>> >         opt.format_callback = update_index_from_diff;\n>> >\n>> > -       read_cache();\n>> >         if (do_diff_cache(tree_sha1, &opt))\n>> >                 return 1;\n>> >         diffcore_std(&opt);\n>> > @@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,\n>> >\n>> >  static void die_if_unmerged_cache(int reset_type)\n>> >  {\n>> > -       if (is_merge() || read_cache() < 0 || unmerged_cache())\n>> > +       if (is_merge() || unmerged_cache())\n>> >                 die(_(\"Cannot do a %s reset in the middle of a merge.\"),\n>> >                     _(reset_type_names[reset_type]));\n>>\n>> reset --soft does not go through these code paths (i.e. it does not\n>> need index at all). If we fail to load index index in \"reset --soft\" I\n>> think it's ok to die(). Corrupt index is fatal anyway. But \"reset\n>> --soft\" now has to pay the cost to load index, which could be slow\n>> when the index is big. Assuming nobody does \"reset --soft\" that often\n>> I think this is OK.\n>>\n>> Alternatively we could load index lazily in _CHEAP code only when we\n>> see trailing slashes, then replace these read_cache() with\n>> read_cache_unless_its_already_loaded_earlier() or something.\n>\n> read_cache() already has an early return if the index is already loaded\n> so I don't think we need to worry about a special function for that.\n>\n> I'm not sure it's worth optimizing this case too heavily, but it might\n> be a nice change to make parse_pathspec() not rely on the index being\n> loaded before it is called with certain flags.\n\nYeah I ddin't check. I agree putting read_cache() in _CHEAP code\nsounds nice. We won't need to worry about forgotten read_cache()\nelsewhere.\n-- \nDuy\n"},{"id":"227465","messageId":"xmqqwqmnthfh.fsf@gitster.dls.corp.google.com","threadId":"34901","inReplyTo":"CACsJy8BgEM3eEDo8wOgkqYTL1fkh9azZNqbogxBubp9g5KRNbQ@mail.gmail.com","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-11T17:08:18Z","receivedAt":"2013-09-11T17:08:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> reset --soft does not go through these code paths (i.e. it does not\n> need index at all). If we fail to load index index in \"reset --soft\" I\n> think it's ok to die(). Corrupt index is fatal anyway.\n\nDo I smell a breakage here?  Isn't \"reset --soft HEAD\" (or some\nknown good commit) a way to recover from a corrupt index?\n\nIf that is the case, I do not think it is OK at all.  What do we\nsuggest as an alternative?  \"rm .git/index && read-tree\"?\n\n> But \"reset\n> --soft\" now has to pay the cost to load index, which could be slow\n> when the index is big. Assuming nobody does \"reset --soft\" that often\n> I think this is OK.\n>\n> Alternatively we could load index lazily in _CHEAP code only when we\n> see trailing slashes, then replace these read_cache() with\n> read_cache_unless_its_already_loaded_earlier() or something.\n"},{"id":"227468","messageId":"20130911172705.GV2582@serenity.lan","threadId":"34901","inReplyTo":"xmqqwqmnthfh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-11T17:27:05Z","receivedAt":"2013-09-11T17:27:05Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Sep 11, 2013 at 10:08:18AM -0700, Junio C Hamano wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n> \n> > reset --soft does not go through these code paths (i.e. it does not\n> > need index at all). If we fail to load index index in \"reset --soft\" I\n> > think it's ok to die(). Corrupt index is fatal anyway.\n> \n> Do I smell a breakage here?  Isn't \"reset --soft HEAD\" (or some\n> known good commit) a way to recover from a corrupt index?\n> \n> If that is the case, I do not think it is OK at all.  What do we\n> suggest as an alternative?  \"rm .git/index && read-tree\"?\n\nDuy's suggestion below is necessary to avoid this then I think - we'll\ndie if the user has a corrupt index and gives a path with a trailing\nslash, but without that path we won't try to load the index.\n\n> > But \"reset\n> > --soft\" now has to pay the cost to load index, which could be slow\n> > when the index is big. Assuming nobody does \"reset --soft\" that often\n> > I think this is OK.\n> >\n> > Alternatively we could load index lazily in _CHEAP code only when we\n> > see trailing slashes, then replace these read_cache() with\n> > read_cache_unless_its_already_loaded_earlier() or something.\n"},{"id":"227474","messageId":"xmqq8uz3tece.fsf@gitster.dls.corp.google.com","threadId":"34901","inReplyTo":"20130911172705.GV2582@serenity.lan","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-11T18:14:57Z","receivedAt":"2013-09-11T18:14:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Wed, Sep 11, 2013 at 10:08:18AM -0700, Junio C Hamano wrote:\n>> Duy Nguyen <pclouds@gmail.com> writes:\n>> \n>> > reset --soft does not go through these code paths (i.e. it does not\n>> > need index at all). If we fail to load index index in \"reset --soft\" I\n>> > think it's ok to die(). Corrupt index is fatal anyway.\n>> \n>> Do I smell a breakage here?  Isn't \"reset --soft HEAD\" (or some\n>> known good commit) a way to recover from a corrupt index?\n>> \n>> If that is the case, I do not think it is OK at all.  What do we\n>> suggest as an alternative?  \"rm .git/index && read-tree\"?\n>\n> Duy's suggestion below is necessary to avoid this then I think - we'll\n> die if the user has a corrupt index and gives a path with a trailing\n> slash, but without that path we won't try to load the index.\n\nSorry, but I don't quite follow.  Isn't \"git reset --soft <path>\" a\nnonsense, with or without a trailing slash at the end of <path>?\n\n\n>> > But \"reset\n>> > --soft\" now has to pay the cost to load index, which could be slow\n>> > when the index is big. Assuming nobody does \"reset --soft\" that often\n>> > I think this is OK.\n>> >\n>> > Alternatively we could load index lazily in _CHEAP code only when we\n>> > see trailing slashes, then replace these read_cache() with\n>> > read_cache_unless_its_already_loaded_earlier() or something.\n"},{"id":"227475","messageId":"20130911182233.GW2582@serenity.lan","threadId":"34901","inReplyTo":"xmqq8uz3tece.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-11T18:22:33Z","receivedAt":"2013-09-11T18:22:33Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Sep 11, 2013 at 11:14:57AM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > On Wed, Sep 11, 2013 at 10:08:18AM -0700, Junio C Hamano wrote:\n> >> Duy Nguyen <pclouds@gmail.com> writes:\n> >> \n> >> > reset --soft does not go through these code paths (i.e. it does not\n> >> > need index at all). If we fail to load index index in \"reset --soft\" I\n> >> > think it's ok to die(). Corrupt index is fatal anyway.\n> >> \n> >> Do I smell a breakage here?  Isn't \"reset --soft HEAD\" (or some\n> >> known good commit) a way to recover from a corrupt index?\n> >> \n> >> If that is the case, I do not think it is OK at all.  What do we\n> >> suggest as an alternative?  \"rm .git/index && read-tree\"?\n> >\n> > Duy's suggestion below is necessary to avoid this then I think - we'll\n> > die if the user has a corrupt index and gives a path with a trailing\n> > slash, but without that path we won't try to load the index.\n> \n> Sorry, but I don't quite follow.  Isn't \"git reset --soft <path>\" a\n> nonsense, with or without a trailing slash at the end of <path>?\n\nYes, but my point was that providing the user doesn't give a path with a\ntrailing slash we will get past the option parser if we take the\napproach below.\n\nHowever, I think we do do a read_cache when using \"reset --soft\" since\nwe go through builtin/reset.c::die_if_unmerged_cache() which dies if\nread_cache fails.  So I don't think we are losing anything by moving\nthis check earlier.\n\n> >> > But \"reset\n> >> > --soft\" now has to pay the cost to load index, which could be slow\n> >> > when the index is big. Assuming nobody does \"reset --soft\" that often\n> >> > I think this is OK.\n> >> >\n> >> > Alternatively we could load index lazily in _CHEAP code only when we\n> >> > see trailing slashes, then replace these read_cache() with\n> >> > read_cache_unless_its_already_loaded_earlier() or something.\n"},{"id":"227481","messageId":"xmqqvc27rydp.fsf@gitster.dls.corp.google.com","threadId":"34901","inReplyTo":"20130911182233.GW2582@serenity.lan","subject":"Re: [PATCH 1/2] reset: handle submodule with trailing slash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-11T18:45:06Z","receivedAt":"2013-09-11T18:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> However, I think we do do a read_cache when using \"reset --soft\" since\n> we go through builtin/reset.c::die_if_unmerged_cache() which dies if\n> read_cache fails.  So I don't think we are losing anything by moving\n> this check earlier.\n\nThanks.\n"},{"id":"227557","messageId":"cover.1379013786.git.john@keeping.me.uk","threadId":"34901","inReplyTo":"cover.1378840318.git.john@keeping.me.uk","subject":"[PATCH v2 0/4] submodule trailing slash improvements","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-12T19:24:57Z","receivedAt":"2013-09-12T19:24:57Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Changes since v1:\n\n* Improvements to existing pathspec code to use is_dir_sep instead of\n  comparing against '/' and handle multiple trailing slashes\n* Remove calls to read_cache() made redundant by a new call in\n  builtin/reset.c::parse_args()\n\nJohn Keeping (4):\n  pathspec: use is_dir_sep() to check for trailing slashes\n  pathspec: strip multiple trailing slashes from submodules\n  rm: re-use parse_pathspec's trailing-slash removal\n  reset: handle submodule with trailing slash\n\n builtin/reset.c            |  8 ++++++--\n builtin/rm.c               | 20 ++++----------------\n pathspec.c                 | 30 +++++++++++++++++++-----------\n t/t7400-submodule-basic.sh |  6 ++++--\n 4 files changed, 33 insertions(+), 31 deletions(-)\n\n-- \n1.8.4.277.gfbd6843.dirty\n"},{"id":"227558","messageId":"88455dac2dce36135c070cff01215e9ae0259635.1379013786.git.john@keeping.me.uk","threadId":"34901","inReplyTo":"cover.1379013786.git.john@keeping.me.uk","subject":"[PATCH v2 1/4] pathspec: use is_dir_sep() to check for trailing slashes","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-12T19:24:58Z","receivedAt":"2013-09-12T19:24:58Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This allows us to correctly removing trailing backslashes on Windows\nwhen checking for submodules.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n pathspec.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex ad1a9f5..7c6963b 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -252,7 +252,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \titem->prefix = prefixlen;\n \n \tif ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&\n-\t    (item->len >= 1 && item->match[item->len - 1] == '/') &&\n+\t    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&\n \t    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&\n \t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n \t\titem->len--;\n@@ -267,7 +267,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t\t\tif (!S_ISGITLINK(ce->ce_mode))\n \t\t\t\tcontinue;\n \n-\t\t\tif (item->len <= ce_len || match[ce_len] != '/' ||\n+\t\t\tif (item->len <= ce_len ||\n+\t\t\t    !is_dir_sep(match[ce_len]) ||\n \t\t\t    memcmp(ce->name, match, ce_len))\n \t\t\t\tcontinue;\n \t\t\tif (item->len == ce_len + 1) {\n-- \n1.8.4.277.gfbd6843.dirty\n"},{"id":"227559","messageId":"7c478c19da6ee3322ca87e77a90358a30178c286.1379013786.git.john@keeping.me.uk","threadId":"34901","inReplyTo":"cover.1379013786.git.john@keeping.me.uk","subject":"[PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-12T19:24:59Z","receivedAt":"2013-09-12T19:24:59Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This allows us to replace the submodule path trailing slash removal in\nbuiltin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to\nparse_pathspec() without changing the behaviour with respect to multiple\ntrailing slashes.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n pathspec.c | 27 +++++++++++++++++----------\n 1 file changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 7c6963b..11b031a 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -251,12 +251,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \titem->len = strlen(item->match);\n \titem->prefix = prefixlen;\n \n-\tif ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&\n-\t    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&\n-\t    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&\n-\t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n-\t\titem->len--;\n-\t\tmatch[item->len] = '\\0';\n+\tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) {\n+\t\tsize_t pathlen = item->len;\n+\t\twhile (pathlen > 0 && is_dir_sep(item->match[pathlen - 1]))\n+\t\t\tpathlen--;\n+\n+\t\tif ((i = cache_name_pos(item->match, pathlen)) >= 0 &&\n+\t\t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n+\t\t\titem->len = pathlen;\n+\t\t\tmatch[item->len] = '\\0';\n+\t\t}\n \t}\n \n \tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)\n@@ -271,11 +275,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t\t\t    !is_dir_sep(match[ce_len]) ||\n \t\t\t    memcmp(ce->name, match, ce_len))\n \t\t\t\tcontinue;\n-\t\t\tif (item->len == ce_len + 1) {\n-\t\t\t\t/* strip trailing slash */\n+\n+\t\t\twhile (item->len > 0 && is_dir_sep(match[item->len - 1]))\n \t\t\t\titem->len--;\n-\t\t\t\tmatch[item->len] = '\\0';\n-\t\t\t} else\n+\n+\t\t\t/* strip trailing slash */\n+\t\t\tmatch[item->len] = '\\0';\n+\n+\t\t\tif (item->len != ce_len)\n \t\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n \t\t\t\t     elt, ce_len, ce->name);\n \t\t}\n-- \n1.8.4.277.gfbd6843.dirty\n"},{"id":"227560","messageId":"238adf049ede4f54baad1b78fceff4de8313c4a6.1379013786.git.john@keeping.me.uk","threadId":"34901","inReplyTo":"cover.1379013786.git.john@keeping.me.uk","subject":"[PATCH v2 3/4] rm: re-use parse_pathspec's trailing-slash removal","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-12T19:25:00Z","receivedAt":"2013-09-12T19:25:00Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Instead of re-implementing the \"remove trailing slashes\" loop in\nbuiltin/rm.c just pass PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP to\nparse_pathspec.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/rm.c | 20 ++++----------------\n 1 file changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 9b59ab3..3a0e0ea 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -298,22 +298,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \n-\t/*\n-\t * Drop trailing directory separators from directories so we'll find\n-\t * submodules in the index.\n-\t */\n-\tfor (i = 0; i < argc; i++) {\n-\t\tsize_t pathlen = strlen(argv[i]);\n-\t\tif (pathlen && is_dir_sep(argv[i][pathlen - 1]) &&\n-\t\t    is_directory(argv[i])) {\n-\t\t\tdo {\n-\t\t\t\tpathlen--;\n-\t\t\t} while (pathlen && is_dir_sep(argv[i][pathlen - 1]));\n-\t\t\targv[i] = xmemdupz(argv[i], pathlen);\n-\t\t}\n-\t}\n-\n-\tparse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD, prefix, argv);\n+\tparse_pathspec(&pathspec, 0,\n+\t\t       PATHSPEC_PREFER_CWD |\n+\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,\n+\t\t       prefix, argv);\n \trefresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n \n \tseen = xcalloc(pathspec.nr, 1);\n-- \n1.8.4.277.gfbd6843.dirty\n"},{"id":"227561","messageId":"46340e653ef24dec3dcb51e4d88c5ec46f197d82.1379013786.git.john@keeping.me.uk","threadId":"34901","inReplyTo":"cover.1379013786.git.john@keeping.me.uk","subject":"[PATCH v2 4/4] reset: handle submodule with trailing slash","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-12T19:25:01Z","receivedAt":"2013-09-12T19:25:01Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When using tab-completion, a directory path will often end with a\ntrailing slash which currently confuses \"git reset\" when dealing with\nsubmodules.  Now that we have parse_pathspec we can easily handle this\nby simply adding the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag.\n\nTo do this, we need to move the read_cache() call before the\nparse_pathspec() call.  All of the existing paths through cmd_reset()\nthat do not die early already call read_cache() at some point, so there\nis no performance impact to doing this in the common case.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/reset.c            | 8 ++++++--\n t/t7400-submodule-basic.sh | 6 ++++--\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 5e4c551..800117f 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -143,7 +143,6 @@ static int read_from_tree(const struct pathspec *pathspec,\n \topt.output_format = DIFF_FORMAT_CALLBACK;\n \topt.format_callback = update_index_from_diff;\n \n-\tread_cache();\n \tif (do_diff_cache(tree_sha1, &opt))\n \t\treturn 1;\n \tdiffcore_std(&opt);\n@@ -169,7 +168,7 @@ static void set_reflog_message(struct strbuf *sb, const char *action,\n \n static void die_if_unmerged_cache(int reset_type)\n {\n-\tif (is_merge() || read_cache() < 0 || unmerged_cache())\n+\tif (is_merge() || unmerged_cache())\n \t\tdie(_(\"Cannot do a %s reset in the middle of a merge.\"),\n \t\t    _(reset_type_names[reset_type]));\n \n@@ -220,8 +219,13 @@ static void parse_args(struct pathspec *pathspec,\n \t\t}\n \t}\n \t*rev_ret = rev;\n+\n+\tif (read_cache() < 0)\n+\t\tdie(_(\"index file corrupt\"));\n+\n \tparse_pathspec(pathspec, 0,\n \t\t       PATHSPEC_PREFER_FULL |\n+\t\t       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |\n \t\t       (patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0),\n \t\t       prefix, argv);\n }\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 4192fe0..c268d3c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -481,7 +481,7 @@ test_expect_success 'do not add files from a submodule' '\n \n '\n \n-test_expect_success 'gracefully add submodule with a trailing slash' '\n+test_expect_success 'gracefully add/reset submodule with a trailing slash' '\n \n \tgit reset --hard &&\n \tgit commit -m \"commit subproject\" init &&\n@@ -495,7 +495,9 @@ test_expect_success 'gracefully add submodule with a trailing slash' '\n \tgit add init/ &&\n \ttest_must_fail git diff --exit-code --cached init &&\n \ttest $commit = $(git ls-files --stage |\n-\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\")\n+\t\tsed -n \"s/^160000 \\([^ ]*\\).*/\\1/p\") &&\n+\tgit reset init/ &&\n+\tgit diff --exit-code --cached init\n \n '\n \n-- \n1.8.4.277.gfbd6843.dirty\n"},{"id":"227564","messageId":"xmqqzjrhom85.fsf@gitster.dls.corp.google.com","threadId":"34901","inReplyTo":"7c478c19da6ee3322ca87e77a90358a30178c286.1379013786.git.john@keeping.me.uk","subject":"Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-12T19:48:10Z","receivedAt":"2013-09-12T19:48:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> This allows us to replace the submodule path trailing slash removal in\n> builtin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to\n> parse_pathspec() without changing the behaviour with respect to multiple\n> trailing slashes.\n\nWhere does prefix_pathspec()'s input, which could have an unwanted\ntrailing slash, come from?\n\nIf it is read from some of our internal data structure and known to\nhave at most one, then this change makes me feel very uneasy to cope\nwith potentially sloppy end-user input and data generated by ourselves\nwith the same logic.  It will allow our internal to be sloppy without\nforcing us notice and fix that sloppiness.\n\nIf it is coming from an end-user input, then I would not object to\nthe change, though.\n\nThanks.\n\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  pathspec.c | 27 +++++++++++++++++----------\n>  1 file changed, 17 insertions(+), 10 deletions(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index 7c6963b..11b031a 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -251,12 +251,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>  \titem->len = strlen(item->match);\n>  \titem->prefix = prefixlen;\n>  \n> -\tif ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&\n> -\t    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&\n> -\t    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&\n> -\t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n> -\t\titem->len--;\n> -\t\tmatch[item->len] = '\\0';\n> +\tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) {\n> +\t\tsize_t pathlen = item->len;\n> +\t\twhile (pathlen > 0 && is_dir_sep(item->match[pathlen - 1]))\n> +\t\t\tpathlen--;\n> +\n> +\t\tif ((i = cache_name_pos(item->match, pathlen)) >= 0 &&\n> +\t\t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n> +\t\t\titem->len = pathlen;\n> +\t\t\tmatch[item->len] = '\\0';\n> +\t\t}\n>  \t}\n>  \n>  \tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)\n> @@ -271,11 +275,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>  \t\t\t    !is_dir_sep(match[ce_len]) ||\n>  \t\t\t    memcmp(ce->name, match, ce_len))\n>  \t\t\t\tcontinue;\n> -\t\t\tif (item->len == ce_len + 1) {\n> -\t\t\t\t/* strip trailing slash */\n> +\n> +\t\t\twhile (item->len > 0 && is_dir_sep(match[item->len - 1]))\n>  \t\t\t\titem->len--;\n> -\t\t\t\tmatch[item->len] = '\\0';\n> -\t\t\t} else\n> +\n> +\t\t\t/* strip trailing slash */\n> +\t\t\tmatch[item->len] = '\\0';\n> +\n> +\t\t\tif (item->len != ce_len)\n>  \t\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n>  \t\t\t\t     elt, ce_len, ce->name);\n>  \t\t}\n"},{"id":"227570","messageId":"52322149.7050201@kdbg.org","threadId":"34901","inReplyTo":"88455dac2dce36135c070cff01215e9ae0259635.1379013786.git.john@keeping.me.uk","subject":"Re: [PATCH v2 1/4] pathspec: use is_dir_sep() to check for trailing slashes","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-09-12T20:17:13Z","receivedAt":"2013-09-12T20:17:13Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.09.2013 21:24, schrieb John Keeping:\n> This allows us to correctly removing trailing backslashes on Windows\n> when checking for submodules.\n> \n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  pathspec.c | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index ad1a9f5..7c6963b 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -252,7 +252,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>  \titem->prefix = prefixlen;\n>  \n>  \tif ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&\n> -\t    (item->len >= 1 && item->match[item->len - 1] == '/') &&\n> +\t    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&\n>  \t    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&\n>  \t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n>  \t\titem->len--;\n> @@ -267,7 +267,8 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>  \t\t\tif (!S_ISGITLINK(ce->ce_mode))\n>  \t\t\t\tcontinue;\n>  \n> -\t\t\tif (item->len <= ce_len || match[ce_len] != '/' ||\n> +\t\t\tif (item->len <= ce_len ||\n> +\t\t\t    !is_dir_sep(match[ce_len]) ||\n>  \t\t\t    memcmp(ce->name, match, ce_len))\n>  \t\t\t\tcontinue;\n>  \t\t\tif (item->len == ce_len + 1) {\n\nA design decisions to keep in mind:\n\nPaths in the index *ALWAYS* use the slash, even on Windows. On Windows,\npathspec that are user input must undergo backslash-to-slash\ntransformation at a very early stage so that later processing that\ncompares the user input to index contents need not do it on the fly. The\nbackslash-to-slash transformation used to happen in get_pathspec() via\nprefix_path() and normalize_path_copy().\n\nIf, at this point, the contents of 'match' is still being parsed for\npathspec magic, then it is likely correct to use is_dir_sep().\n\nOn the other hand, if at this point the contents of 'match' are used to\nexecute pathspec magic, then it is not correct to use is_dir_sep(); the\nconversion of backslash to slash should have happened earlier, and no\nbackslashes should be present anymore.\n\n(Yes, this means that on Windows we cannot escape glob characters\nbecause, e.g., 'a\\*.c' was turned into 'a/*.c'.)\n\n-- Hannes\n"},{"id":"227571","messageId":"20130912202128.GB2582@serenity.lan","threadId":"34901","inReplyTo":"xmqqzjrhom85.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-12T20:21:28Z","receivedAt":"2013-09-12T20:21:28Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Sep 12, 2013 at 12:48:10PM -0700, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > This allows us to replace the submodule path trailing slash removal in\n> > builtin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to\n> > parse_pathspec() without changing the behaviour with respect to multiple\n> > trailing slashes.\n> \n> Where does prefix_pathspec()'s input, which could have an unwanted\n> trailing slash, come from?\n> \n> If it is read from some of our internal data structure and known to\n> have at most one, then this change makes me feel very uneasy to cope\n> with potentially sloppy end-user input and data generated by ourselves\n> with the same logic.  It will allow our internal to be sloppy without\n> forcing us notice and fix that sloppiness.\n> \n> If it is coming from an end-user input, then I would not object to\n> the change, though.\n\nI added this in response to Duy's comment on v1 [1].\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/234548\n\nLooking more closely, this does come from user input (via the argv\npassed into parse_pathspec) but does (some of the time) go through\nprefix_path_gently which calls normalize_path_copy_len.\n\nIt's not immediately clear to me when prefix_pathspec goes through this\nparticular code path, but I think we may be able to drop this (and the\nprevious patch) without affecting the user.\n\n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> >  pathspec.c | 27 +++++++++++++++++----------\n> >  1 file changed, 17 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/pathspec.c b/pathspec.c\n> > index 7c6963b..11b031a 100644\n> > --- a/pathspec.c\n> > +++ b/pathspec.c\n> > @@ -251,12 +251,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n> >  \titem->len = strlen(item->match);\n> >  \titem->prefix = prefixlen;\n> >  \n> > -\tif ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&\n> > -\t    (item->len >= 1 && is_dir_sep(item->match[item->len - 1])) &&\n> > -\t    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&\n> > -\t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n> > -\t\titem->len--;\n> > -\t\tmatch[item->len] = '\\0';\n> > +\tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) {\n> > +\t\tsize_t pathlen = item->len;\n> > +\t\twhile (pathlen > 0 && is_dir_sep(item->match[pathlen - 1]))\n> > +\t\t\tpathlen--;\n> > +\n> > +\t\tif ((i = cache_name_pos(item->match, pathlen)) >= 0 &&\n> > +\t\t    S_ISGITLINK(active_cache[i]->ce_mode)) {\n> > +\t\t\titem->len = pathlen;\n> > +\t\t\tmatch[item->len] = '\\0';\n> > +\t\t}\n> >  \t}\n> >  \n> >  \tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)\n> > @@ -271,11 +275,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n> >  \t\t\t    !is_dir_sep(match[ce_len]) ||\n> >  \t\t\t    memcmp(ce->name, match, ce_len))\n> >  \t\t\t\tcontinue;\n> > -\t\t\tif (item->len == ce_len + 1) {\n> > -\t\t\t\t/* strip trailing slash */\n> > +\n> > +\t\t\twhile (item->len > 0 && is_dir_sep(match[item->len - 1]))\n> >  \t\t\t\titem->len--;\n> > -\t\t\t\tmatch[item->len] = '\\0';\n> > -\t\t\t} else\n> > +\n> > +\t\t\t/* strip trailing slash */\n> > +\t\t\tmatch[item->len] = '\\0';\n> > +\n> > +\t\t\tif (item->len != ce_len)\n> >  \t\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n> >  \t\t\t\t     elt, ce_len, ce->name);\n> >  \t\t}\n"},{"id":"227603","messageId":"CACsJy8DX_v9r0QYQMH=697LGeGsCaE80oCo0ppd2LFZk-Os+TA@mail.gmail.com","threadId":"34901","inReplyTo":"20130912202128.GB2582@serenity.lan","subject":"Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-13T01:28:24Z","receivedAt":"2013-09-13T01:28:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Sep 13, 2013 at 3:21 AM, John Keeping <john@keeping.me.uk> wrote:\n> On Thu, Sep 12, 2013 at 12:48:10PM -0700, Junio C Hamano wrote:\n>> John Keeping <john@keeping.me.uk> writes:\n>>\n>> > This allows us to replace the submodule path trailing slash removal in\n>> > builtin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to\n>> > parse_pathspec() without changing the behaviour with respect to multiple\n>> > trailing slashes.\n>>\n>> Where does prefix_pathspec()'s input, which could have an unwanted\n>> trailing slash, come from?\n>>\n>> If it is read from some of our internal data structure and known to\n>> have at most one, then this change makes me feel very uneasy to cope\n>> with potentially sloppy end-user input and data generated by ourselves\n>> with the same logic.  It will allow our internal to be sloppy without\n>> forcing us notice and fix that sloppiness.\n>>\n>> If it is coming from an end-user input, then I would not object to\n>> the change, though.\n>\n> I added this in response to Duy's comment on v1 [1].\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/234548\n\nLooks like I add more noise to this thread than anything valuable.\nYes, once argv goes through parse_pathspec it's normalized so we do\nnot need to strip consecutive slashes any more. I'm not entirely sure\nif it also converts Windows '\\\\' to '/'..\n\n> Looking more closely, this does come from user input (via the argv\n> passed into parse_pathspec) but does (some of the time) go through\n> prefix_path_gently which calls normalize_path_copy_len.\n>\n> It's not immediately clear to me when prefix_pathspec goes through this\n> particular code path, but I think we may be able to drop this (and the\n> previous patch) without affecting the user.\n-- \nDuy\n"},{"id":"227617","messageId":"20130913084833.GD2582@serenity.lan","threadId":"34901","inReplyTo":"CACsJy8DX_v9r0QYQMH=697LGeGsCaE80oCo0ppd2LFZk-Os+TA@mail.gmail.com","subject":"Re: [PATCH v2 2/4] pathspec: strip multiple trailing slashes from submodules","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-13T08:48:34Z","receivedAt":"2013-09-13T08:48:34Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Sep 13, 2013 at 08:28:24AM +0700, Duy Nguyen wrote:\n> On Fri, Sep 13, 2013 at 3:21 AM, John Keeping <john@keeping.me.uk> wrote:\n> > On Thu, Sep 12, 2013 at 12:48:10PM -0700, Junio C Hamano wrote:\n> >> John Keeping <john@keeping.me.uk> writes:\n> >>\n> >> > This allows us to replace the submodule path trailing slash removal in\n> >> > builtin/rm.c with the PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to\n> >> > parse_pathspec() without changing the behaviour with respect to multiple\n> >> > trailing slashes.\n> >>\n> >> Where does prefix_pathspec()'s input, which could have an unwanted\n> >> trailing slash, come from?\n> >>\n> >> If it is read from some of our internal data structure and known to\n> >> have at most one, then this change makes me feel very uneasy to cope\n> >> with potentially sloppy end-user input and data generated by ourselves\n> >> with the same logic.  It will allow our internal to be sloppy without\n> >> forcing us notice and fix that sloppiness.\n> >>\n> >> If it is coming from an end-user input, then I would not object to\n> >> the change, though.\n> >\n> > I added this in response to Duy's comment on v1 [1].\n> >\n> > [1] http://article.gmane.org/gmane.comp.version-control.git/234548\n> \n> Looks like I add more noise to this thread than anything valuable.\n> Yes, once argv goes through parse_pathspec it's normalized so we do\n> not need to strip consecutive slashes any more. I'm not entirely sure\n> if it also converts Windows '\\\\' to '/'..\n\nIf it goes through normalize_path_copy_len then it does.\n\nJunio, can you please drop the first two patches in this series?  I can\nresend the final two unmodified if necessary, but I suspect it's easier\nfor you to just drop the commits.\n\n> > Looking more closely, this does come from user input (via the argv\n> > passed into parse_pathspec) but does (some of the time) go through\n> > prefix_path_gently which calls normalize_path_copy_len.\n> >\n> > It's not immediately clear to me when prefix_pathspec goes through this\n> > particular code path, but I think we may be able to drop this (and the\n> > previous patch) without affecting the user.\n"}]}