{"thread":{"id":"34851","subject":"[PATCH 1/3] pathspec: catch prepending :(prefix) on pathspec with short magic","startedAt":"2013-09-05T03:40:38Z","lastAt":"2013-09-05T18:39:56Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"226776","messageId":"1378352440-25410-1-git-send-email-pclouds@gmail.com","threadId":"34851","inReplyTo":null,"subject":"[PATCH 1/3] pathspec: catch prepending :(prefix) on pathspec with short magic","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-05T03:40:38Z","receivedAt":"2013-09-05T03:40:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":":(prefix) is in the long form. Suppose people pass :!foo with '!'\nbeing the short form of magic 'bar', the code will happily turn it to\n:(prefix..)!foo, which makes '!' part of the path and no longer a magic.\n\nThe correct form must be ':(prefix..,bar)foo', but as so far we\nhaven't had any magic in short form yet (*), the code to convert from\nshort form to long one will be inactive anyway. Let's postpone it\nuntil a real short form magic appears.\n\n(*) The short form magic '/' is a special case and won't be caught by\nthis die(), which is correct. When '/' magic is detected, prefixlen is\nset back to 0 and the whole \"if (prefixlen..)\" block is skipped.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n fixes on top of nd/magic-pathspec.\n\n pathspec.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex d9f4143..62fde50 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -231,7 +231,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t\tconst char *start = elt;\n \t\tif (prefixlen && !literal_global) {\n \t\t\t/* Preserve the actual prefix length of each pattern */\n-\t\t\tif (long_magic_end) {\n+\t\t\tif (short_magic)\n+\t\t\t\tdie(\"BUG: prefixing on short magic is not supported\");\n+\t\t\telse if (long_magic_end) {\n \t\t\t\tstrbuf_add(&sb, start, long_magic_end - start);\n \t\t\t\tstrbuf_addf(&sb, \",prefix:%d\", prefixlen);\n \t\t\t\tstart = long_magic_end;\n-- \n1.8.2.83.gc99314b\n"},{"id":"226777","messageId":"1378352440-25410-2-git-send-email-pclouds@gmail.com","threadId":"34851","inReplyTo":"1378352440-25410-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] add: lift the pathspec magic restriction on \"add -p\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-05T03:40:39Z","receivedAt":"2013-09-05T03:40:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Since 480ca64 (convert run_add_interactive to use struct pathspec -\n2013-07-14), we have unconditionally passed :(prefix)xxx to\nadd-interactive.perl. It implies that all commands\nadd-interactive.perl calls must be aware of pathspec magic, or\n:(prefix) is barfed. The restriction to :/ only becomes unnecessary.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/add.c | 8 +-------\n builtin/checkout.c | 7 -------\n 2 files changed, 1 insertion(+), 14 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 9d52fc7..b4035f6 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -270,13 +270,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)\n {\n \tstruct pathspec pathspec;\n \n-\t/*\n-\t * git-add--interactive itself does not parse pathspec. It\n-\t * simply passes the pathspec to other builtin commands. Let's\n-\t * hope all of them support all magic, or we'll need to limit\n-\t * the magic here.\n-\t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC & ~PATHSPEC_FROMTOP,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH |\n \t\t PATHSPEC_PREFIX_ORIGIN,\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 7ea1100..b235e04 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1149,13 +1149,6 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (argc) {\n-\t\t/*\n-\t\t * In patch mode (opts.patch_mode != 0), we pass the\n-\t\t * pathspec to an external program, git-add--interactive.\n-\t\t * Do not accept any kind of magic that that program\n-\t\t * cannot handle. Magic mask is pretty safe to be\n-\t\t * lifted for new magic when opts.patch_mode == 0.\n-\t\t */\n \t\tparse_pathspec(&opts.pathspec, 0,\n \t\t\t opts.patch_mode ? PATHSPEC_PREFIX_ORIGIN : 0,\n \t\t\t prefix, argv);\n-- \n1.8.2.83.gc99314b\n"},{"id":"226778","messageId":"1378352440-25410-3-git-send-email-pclouds@gmail.com","threadId":"34851","inReplyTo":"1378352440-25410-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] add--interactive: fix external command invocation on Windows","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-05T03:40:40Z","receivedAt":"2013-09-05T03:40:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"From: Johannes Sixt \n\nBack in 21e9757e (Hack git-add--interactive to make it work with\nActiveState Perl, 2007-08-01), the invocation of external commands was\nchanged to use qx{} on Windows. The rationale was that the command\ninterpreter on Windows is not a POSIX shell, but rather Windows's CMD.\nThat patch was wrong to include 'msys' in the check whether to use qx{}\nor not: 'msys' identifies MSYS perl as shipped with Git for Windows,\nwhich does not need the special treatment; qx{} should be used only with\nActiveState perl, which is identified by 'MSWin32'.\n\nSigned-off-by: Johannes Sixt \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n git-add--interactive.perl | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 75a991f..5156384 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -169,7 +169,7 @@ my %patch_modes = (\n my %patch_mode_flavour = %{$patch_modes{stage}};\n \n sub run_cmd_pipe {\n-\tif ($^O eq 'MSWin32' || $^O eq 'msys') {\n+\tif ($^O eq 'MSWin32') {\n \t\tmy @invalid = grep {m/[\":*]/} @_;\n \t\tdie \"$^O does not support: @invalid\\n\" if @invalid;\n \t\tmy @args = map { m/ /o ? \"\\\"$_\\\"\": $_ } @_;\n-- \n1.8.2.83.gc99314b\n"},{"id":"226868","messageId":"xmqqppsni03n.fsf@gitster.dls.corp.google.com","threadId":"34851","inReplyTo":"1378352440-25410-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/3] pathspec: catch prepending :(prefix) on pathspec with short magic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-05T18:39:56Z","receivedAt":"2013-09-05T18:39:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> :(prefix) is in the long form. Suppose people pass :!foo with '!'\n> being the short form of magic 'bar', the code will happily turn it to\n> :(prefix..)!foo, which makes '!' part of the path and no longer a magic.\n>\n> The correct form must be ':(prefix..,bar)foo', but as so far we\n> haven't had any magic in short form yet (*), the code to convert from\n> short form to long one will be inactive anyway. Let's postpone it\n> until a real short form magic appears.\n>\n> (*) The short form magic '/' is a special case and won't be caught by\n> this die(), which is correct. When '/' magic is detected, prefixlen is\n> set back to 0 and the whole \"if (prefixlen..)\" block is skipped.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> fixes on top of nd/magic-pathspec.\n>\n> pathspec.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index d9f4143..62fde50 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -231,7 +231,9 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n> \t\tconst char *start = elt;\n> \t\tif (prefixlen && !literal_global) {\n> \t\t\t/* Preserve the actual prefix length of each pattern */\n> -\t\t\tif (long_magic_end) {\n> +\t\t\tif (short_magic)\n> +\t\t\t\tdie(\"BUG: prefixing on short magic is not supported\");\n> +\t\t\telse if (long_magic_end) {\n> \t\t\t\tstrbuf_add(&sb, start, long_magic_end - start);\n> \t\t\t\tstrbuf_addf(&sb, \",prefix:%d\", prefixlen);\n> \t\t\t\tstart = long_magic_end;\n\nGood.\n\nI wonder if we should start thinking about removing the big\n\"NEEDSWORK\" comment in front of this function.\n\nAlso the pathspec_magic[] array was the table-driven way that was\nmeant to be enhanced to fit future needs to parse all supported\ntypes of pathspec magic, but it seems that \"prefix:12\" magic is\nimplemented using a custom/special case code. We may want to see if\nwe want to enrich the parser to fold this to match the table-driven\napproach better someday---this is not urgent as we are not adding\nany new pathspec magic now.\n\nWill queue. Thanks.\n"}]}