{"thread":{"id":"48134","subject":"[RFC][PATCH] git-stash: convert git stash list to C builtin","startedAt":"2018-03-24T18:24:25Z","lastAt":"2018-04-07T08:58:44Z","messageCount":9,"participants":["Paul-Sebastian Ungureanu","Eric Sunshine","Thomas Gummerer","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"342834","messageId":"20180324182313.13705-1-ungureanupaulsebastian@gmail.com","threadId":"48134","inReplyTo":null,"subject":"[RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Paul-Sebastian Ungureanu","fromEmail":"ungureanupaulsebastian@gmail.com","sentAt":"2018-03-24T18:23:13Z","receivedAt":"2018-03-24T18:24:25Z","isPatch":true,"sender":{"key":"ungureanupaulsebastian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24317622?v=4"},"body":"Currently, because git stash is not fully converted to C, I\nintroduced a new helper that will hold the converted commands.\n---\n Makefile                |  1 +\n builtin.h               |  1 +\n builtin/stash--helper.c | 52 +++++++++++++++++++++++++++++++++++++++++\n git-stash.sh            |  7 +-----\n git.c                   |  1 +\n 5 files changed, 56 insertions(+), 6 deletions(-)\n create mode 100644 builtin/stash--helper.c\n\ndiff --git a/Makefile b/Makefile\nindex a1d8775ad..8ca361c57 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1020,6 +1020,7 @@ BUILTIN_OBJS += builtin/send-pack.o\n BUILTIN_OBJS += builtin/shortlog.o\n BUILTIN_OBJS += builtin/show-branch.o\n BUILTIN_OBJS += builtin/show-ref.o\n+BUILTIN_OBJS += builtin/stash--helper.o\n BUILTIN_OBJS += builtin/stripspace.o\n BUILTIN_OBJS += builtin/submodule--helper.o\n BUILTIN_OBJS += builtin/symbolic-ref.o\ndiff --git a/builtin.h b/builtin.h\nindex 42378f3aa..2ddb4bd5c 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -220,6 +220,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix);\n extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\n extern int cmd_status(int argc, const char **argv, const char *prefix);\n extern int cmd_stripspace(int argc, const char **argv, const char *prefix);\n+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n extern int cmd_tag(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\nnew file mode 100644\nindex 000000000..61fd5390d\n--- /dev/null\n+++ b/builtin/stash--helper.c\n@@ -0,0 +1,52 @@\n+#include \"builtin.h\"\n+#include \"cache.h\"\n+#include \"parse-options.h\"\n+#include \"argv-array.h\"\n+\n+enum {\n+\tLIST_STASH = 1\n+};\n+\n+static const char * ref_stash = \"refs/stash\";\n+\n+static const char * const git_stash__helper_usage[] = {\n+\tN_(\"git stash--helper --list [<options>]\"),\n+\tNULL\n+};\n+\n+static int list_stash(int argc, const char **argv, const char *prefix)\n+{\n+\tstruct object_id obj;\n+\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\n+\tif (get_oid(ref_stash, &obj))\n+\t\treturn 0;\n+\n+\targv_array_pushl(&args, \"log\", \"--format=%gd: %gs\", \"-g\", \"--first-parent\", \"-m\", NULL);\n+\targv_array_pushv(&args, argv);\n+\targv_array_push(&args, ref_stash);\n+\treturn !!cmd_log(args.argc, args.argv, prefix);\n+}\n+\n+int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n+{\n+\tint cmdmode = 0;\n+\n+\tstruct option options[] = {\n+\t\tOPT_CMDMODE(0, \"list\", &cmdmode,\n+\t\t\t N_(\"list stash entries\"), LIST_STASH),\n+\t\tOPT_END()\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options,\n+\t\t\t     git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n+\n+\tif (!cmdmode)\n+\t\tusage_with_options(git_stash__helper_usage, options);\n+\n+\tswitch (cmdmode) {\n+\t\tcase LIST_STASH:\n+\t\t\treturn list_stash(argc, argv, prefix);\n+\t}\n+\treturn 0;\n+}\ndiff --git a/git-stash.sh b/git-stash.sh\nindex fc8f8ae64..a5b9f5fb6 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -380,11 +380,6 @@ have_stash () {\n \tgit rev-parse --verify --quiet $ref_stash >/dev/null\n }\n \n-list_stash () {\n-\thave_stash || return 0\n-\tgit log --format=\"%gd: %gs\" -g --first-parent -m \"$@\" $ref_stash --\n-}\n-\n show_stash () {\n \tALLOW_UNKNOWN_FLAGS=t\n \tassert_stash_like \"$@\"\n@@ -695,7 +690,7 @@ test -n \"$seen_non_option\" || set \"push\" \"$@\"\n case \"$1\" in\n list)\n \tshift\n-\tlist_stash \"$@\"\n+\tgit stash--helper --list \"$@\"\n \t;;\n show)\n \tshift\ndiff --git a/git.c b/git.c\nindex 96cd734f1..6fd2ccd9a 100644\n--- a/git.c\n+++ b/git.c\n@@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {\n \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n \t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n+\t{ \"stash--helper\", cmd_stash__helper, RUN_SETUP },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n-- \n2.16.2.647.gb9d10dde1\n\n"},{"id":"342871","messageId":"CAPig+cS9QwCOG7BA7O5Nu_zsh-xTbDFy2vTWpAXxBuKTY-uzUw@mail.gmail.com","threadId":"48134","inReplyTo":"20180324182313.13705-1-ungureanupaulsebastian@gmail.com","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-03-25T07:08:41Z","receivedAt":"2018-03-25T07:08:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu\n<ungureanupaulsebastian@gmail.com> wrote:\n> Currently, because git stash is not fully converted to C, I\n> introduced a new helper that will hold the converted commands.\n> ---\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> @@ -0,0 +1,52 @@\n> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n> +{\n> +       int cmdmode = 0;\n> +\n> +       struct option options[] = {\n> +               OPT_CMDMODE(0, \"list\", &cmdmode,\n> +                        N_(\"list stash entries\"), LIST_STASH),\n> +               OPT_END()\n> +       };\n\nIs the intention that once git-stash--helper implements all 'stash'\nfunctionality, you will simply rename git-stash--helper to git-stash?\nIf so, then I'd think that you'd want it to accept subcommand\narguments as bare words (\"apply\", \"drop\") in order to be consistent\nwith the existing git-stash command set, not in dashed form\n(\"--apply\", \"--drop\"). In that case, OPT_CMDMODE doesn't seem\nappropriate. Instead, you should be consulting argv[] directly (as in\n[1]) after parse_options().\n\n[1]: https://public-inbox.org/git/20180324173707.17699-2-joel@teichroeb.net/\n\n> +       argc = parse_options(argc, argv, prefix, options,\n> +                            git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n> +\n> +       if (!cmdmode)\n> +               usage_with_options(git_stash__helper_usage, options);\n> +\n> +       switch (cmdmode) {\n> +               case LIST_STASH:\n> +                       return list_stash(argc, argv, prefix);\n> +       }\n> +       return 0;\n> +}\n> diff --git a/git.c b/git.c\n> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {\n>         { \"show-branch\", cmd_show_branch, RUN_SETUP },\n>         { \"show-ref\", cmd_show_ref, RUN_SETUP },\n>         { \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n> +       { \"stash--helper\", cmd_stash__helper, RUN_SETUP },\n\nYou don't require a working tree? Seems odd for git-stash.\n\n>         { \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n>         { \"stripspace\", cmd_stripspace },\n>         { \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n"},{"id":"342922","messageId":"20180325184303.GF10909@hank","threadId":"48134","inReplyTo":"20180324182313.13705-1-ungureanupaulsebastian@gmail.com","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-25T18:43:03Z","receivedAt":"2018-03-25T18:39:49Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/24, Paul-Sebastian Ungureanu wrote:\n> Currently, because git stash is not fully converted to C, I\n> introduced a new helper that will hold the converted commands.\n\nMissing sign-off?  I think it's a good idea to sign off your work even\nfor RFC patches that you don't expect to be applied.  If for nothing\nelse, it helps people not wonder whether you just forgot the sign-off\nor if you intentionally didn't add it.\n\n> ---\n>  Makefile                |  1 +\n>  builtin.h               |  1 +\n>  builtin/stash--helper.c | 52 +++++++++++++++++++++++++++++++++++++++++\n>  git-stash.sh            |  7 +-----\n>  git.c                   |  1 +\n>  5 files changed, 56 insertions(+), 6 deletions(-)\n>  create mode 100644 builtin/stash--helper.c\n> \n> diff --git a/Makefile b/Makefile\n> index a1d8775ad..8ca361c57 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1020,6 +1020,7 @@ BUILTIN_OBJS += builtin/send-pack.o\n>  BUILTIN_OBJS += builtin/shortlog.o\n>  BUILTIN_OBJS += builtin/show-branch.o\n>  BUILTIN_OBJS += builtin/show-ref.o\n> +BUILTIN_OBJS += builtin/stash--helper.o\n>  BUILTIN_OBJS += builtin/stripspace.o\n>  BUILTIN_OBJS += builtin/submodule--helper.o\n>  BUILTIN_OBJS += builtin/symbolic-ref.o\n> diff --git a/builtin.h b/builtin.h\n> index 42378f3aa..2ddb4bd5c 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -220,6 +220,7 @@ extern int cmd_show(int argc, const char **argv, const char *prefix);\n>  extern int cmd_show_branch(int argc, const char **argv, const char *prefix);\n>  extern int cmd_status(int argc, const char **argv, const char *prefix);\n>  extern int cmd_stripspace(int argc, const char **argv, const char *prefix);\n> +extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);\n>  extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);\n>  extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);\n>  extern int cmd_tag(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> new file mode 100644\n> index 000000000..61fd5390d\n> --- /dev/null\n> +++ b/builtin/stash--helper.c\n> @@ -0,0 +1,52 @@\n> +#include \"builtin.h\"\n> +#include \"cache.h\"\n> +#include \"parse-options.h\"\n> +#include \"argv-array.h\"\n> +\n> +enum {\n> +\tLIST_STASH = 1\n> +};\n> +\n> +static const char * ref_stash = \"refs/stash\";\n> +\n> +static const char * const git_stash__helper_usage[] = {\n> +\tN_(\"git stash--helper --list [<options>]\"),\n> +\tNULL\n> +};\n> +\n> +static int list_stash(int argc, const char **argv, const char *prefix)\n> +{\n> +\tstruct object_id obj;\n> +\tstruct argv_array args = ARGV_ARRAY_INIT;\n> +\n> +\tif (get_oid(ref_stash, &obj))\n> +\t\treturn 0;\n> +\n> +\targv_array_pushl(&args, \"log\", \"--format=%gd: %gs\", \"-g\", \"--first-parent\", \"-m\", NULL);\n> +\targv_array_pushv(&args, argv);\n> +\targv_array_push(&args, ref_stash);\n\nThis is missing the final '--' argument to 'git log'.  It's needed to\ndisambiguate the ref from paths.  If we don't have that, this git log\ncall will fail when a \"refs/stash\" directory exists.\n\n> +\treturn !!cmd_log(args.argc, args.argv, prefix);\n> +}\n> +\n> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint cmdmode = 0;\n> +\n> +\tstruct option options[] = {\n> +\t\tOPT_CMDMODE(0, \"list\", &cmdmode,\n> +\t\t\t N_(\"list stash entries\"), LIST_STASH),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\targc = parse_options(argc, argv, prefix, options,\n> +\t\t\t     git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n> +\n> +\tif (!cmdmode)\n> +\t\tusage_with_options(git_stash__helper_usage, options);\n> +\n> +\tswitch (cmdmode) {\n> +\t\tcase LIST_STASH:\n> +\t\t\treturn list_stash(argc, argv, prefix);\n> +\t}\n> +\treturn 0;\n> +}\n> diff --git a/git-stash.sh b/git-stash.sh\n> index fc8f8ae64..a5b9f5fb6 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -380,11 +380,6 @@ have_stash () {\n>  \tgit rev-parse --verify --quiet $ref_stash >/dev/null\n>  }\n>  \n> -list_stash () {\n> -\thave_stash || return 0\n> -\tgit log --format=\"%gd: %gs\" -g --first-parent -m \"$@\" $ref_stash --\n> -}\n> -\n>  show_stash () {\n>  \tALLOW_UNKNOWN_FLAGS=t\n>  \tassert_stash_like \"$@\"\n> @@ -695,7 +690,7 @@ test -n \"$seen_non_option\" || set \"push\" \"$@\"\n>  case \"$1\" in\n>  list)\n>  \tshift\n> -\tlist_stash \"$@\"\n> +\tgit stash--helper --list \"$@\"\n>  \t;;\n>  show)\n>  \tshift\n> diff --git a/git.c b/git.c\n> index 96cd734f1..6fd2ccd9a 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {\n>  \t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n>  \t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n>  \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n> +\t{ \"stash--helper\", cmd_stash__helper, RUN_SETUP },\n>  \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n>  \t{ \"stripspace\", cmd_stripspace },\n>  \t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n> -- \n> 2.16.2.647.gb9d10dde1\n> \n"},{"id":"343054","messageId":"nycvar.QRO.7.76.6.1803262058490.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"48134","inReplyTo":"CAPig+cS9QwCOG7BA7O5Nu_zsh-xTbDFy2vTWpAXxBuKTY-uzUw@mail.gmail.com","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-26T19:03:41Z","receivedAt":"2018-03-26T19:03:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Sun, 25 Mar 2018, Eric Sunshine wrote:\n\n> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu\n> <ungureanupaulsebastian@gmail.com> wrote:\n> > Currently, because git stash is not fully converted to C, I\n> > introduced a new helper that will hold the converted commands.\n> > ---\n> > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> > @@ -0,0 +1,52 @@\n> > +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n> > +{\n> > +       int cmdmode = 0;\n> > +\n> > +       struct option options[] = {\n> > +               OPT_CMDMODE(0, \"list\", &cmdmode,\n> > +                        N_(\"list stash entries\"), LIST_STASH),\n> > +               OPT_END()\n> > +       };\n> \n> Is the intention that once git-stash--helper implements all 'stash'\n> functionality, you will simply rename git-stash--helper to git-stash?\n> If so, then I'd think that you'd want it to accept subcommand\n> arguments as bare words (\"apply\", \"drop\") in order to be consistent\n> with the existing git-stash command set, not in dashed form\n\nWhy not start with cmdmode, and then add a single patch that *also*\naccepts argv[1] as a bare-word cmdmode?\n\nThis could even potentially be a patch to parse-options.[ch] that\nintroduces, say, PARSE_OPT_ALLOW_BARE_CMDMODE.\n\nCiao,\nDscho\n"},{"id":"343060","messageId":"20180326205831.GG10909@hank","threadId":"48134","inReplyTo":"nycvar.QRO.7.76.6.1803262058490.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2018-03-26T20:58:31Z","receivedAt":"2018-03-26T20:55:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/26, Johannes Schindelin wrote:\n> Hi Eric,\n> \n> On Sun, 25 Mar 2018, Eric Sunshine wrote:\n> \n> > On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu\n> > <ungureanupaulsebastian@gmail.com> wrote:\n> > > Currently, because git stash is not fully converted to C, I\n> > > introduced a new helper that will hold the converted commands.\n> > > ---\n> > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> > > @@ -0,0 +1,52 @@\n> > > +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n> > > +{\n> > > +       int cmdmode = 0;\n> > > +\n> > > +       struct option options[] = {\n> > > +               OPT_CMDMODE(0, \"list\", &cmdmode,\n> > > +                        N_(\"list stash entries\"), LIST_STASH),\n> > > +               OPT_END()\n> > > +       };\n> > \n> > Is the intention that once git-stash--helper implements all 'stash'\n> > functionality, you will simply rename git-stash--helper to git-stash?\n> > If so, then I'd think that you'd want it to accept subcommand\n> > arguments as bare words (\"apply\", \"drop\") in order to be consistent\n> > with the existing git-stash command set, not in dashed form\n> \n> Why not start with cmdmode, and then add a single patch that *also*\n> accepts argv[1] as a bare-word cmdmode?\n\nI don't think we should accept the dashed form of the commands for\n'git stash'.  The main reason being that we also have 'git stash'\nwithout any arguments, which acts as 'git stash push'.  So if we would\never come up with an argument to 'git stash push', that matches one of\nthe current verbs, or if we come up with a new verb that matches one\nof the options to 'git stash push', that would not work.\n\nIn that case we could obviously go for a different word, but I think\nthe rules when 'git stash' is going to be 'git stash push' and when it\nis not are already complicated enough, and allowing the verbs as\ndashed options would only make the interface more complicated.\n\n> This could even potentially be a patch to parse-options.[ch] that\n> introduces, say, PARSE_OPT_ALLOW_BARE_CMDMODE.\n\nNow if we'd take that one step further and make it\nPARSE_OPT_BARE_CMDMODE, which would only allow the non-dashed options,\nI think we could use that in other places in git as well (for example\nin 'git worktree').\n\n> Ciao,\n> Dscho\n"},{"id":"343398","messageId":"nycvar.QRO.7.76.6.1803301228230.5026@qfpub.tvgsbejvaqbjf.bet","threadId":"48134","inReplyTo":"20180326205831.GG10909@hank","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-30T10:29:40Z","receivedAt":"2018-03-30T10:29:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Thomas,\n\nOn Mon, 26 Mar 2018, Thomas Gummerer wrote:\n\n> On 03/26, Johannes Schindelin wrote:\n> > \n> > On Sun, 25 Mar 2018, Eric Sunshine wrote:\n> > \n> > > On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu\n> > > <ungureanupaulsebastian@gmail.com> wrote:\n> > > > Currently, because git stash is not fully converted to C, I\n> > > > introduced a new helper that will hold the converted commands.\n> > > > ---\n> > > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n> > > > @@ -0,0 +1,52 @@\n> > > > +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n> > > > +{\n> > > > +       int cmdmode = 0;\n> > > > +\n> > > > +       struct option options[] = {\n> > > > +               OPT_CMDMODE(0, \"list\", &cmdmode,\n> > > > +                        N_(\"list stash entries\"), LIST_STASH),\n> > > > +               OPT_END()\n> > > > +       };\n> > > \n> > > Is the intention that once git-stash--helper implements all 'stash'\n> > > functionality, you will simply rename git-stash--helper to git-stash?\n> > > If so, then I'd think that you'd want it to accept subcommand\n> > > arguments as bare words (\"apply\", \"drop\") in order to be consistent\n> > > with the existing git-stash command set, not in dashed form\n> > \n> > Why not start with cmdmode, and then add a single patch that *also*\n> > accepts argv[1] as a bare-word cmdmode?\n> \n> I don't think we should accept the dashed form of the commands for\n> 'git stash'.  The main reason being that we also have 'git stash'\n> without any arguments, which acts as 'git stash push'.  So if we would\n> ever come up with an argument to 'git stash push', that matches one of\n> the current verbs, or if we come up with a new verb that matches one\n> of the options to 'git stash push', that would not work.\n> \n> In that case we could obviously go for a different word, but I think\n> the rules when 'git stash' is going to be 'git stash push' and when it\n> is not are already complicated enough, and allowing the verbs as\n> dashed options would only make the interface more complicated.\n> \n> > This could even potentially be a patch to parse-options.[ch] that\n> > introduces, say, PARSE_OPT_ALLOW_BARE_CMDMODE.\n> \n> Now if we'd take that one step further and make it\n> PARSE_OPT_BARE_CMDMODE, which would only allow the non-dashed options,\n> I think we could use that in other places in git as well (for example\n> in 'git worktree').\n\nBut the valid options change with the subcommands, too. So my idea, even\nwith your adjustment to disallow dashed cmdmode, simply does not work.\n\nThanks for setting my head straight,\nDscho\n"},{"id":"343770","messageId":"63772b7d-7b1e-2a3a-b12c-2dae9e254b68@gmail.com","threadId":"48134","inReplyTo":"CAPig+cS9QwCOG7BA7O5Nu_zsh-xTbDFy2vTWpAXxBuKTY-uzUw@mail.gmail.com","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Paul-Sebastian Ungureanu","fromEmail":"ungureanupaulsebastian@gmail.com","sentAt":"2018-04-03T21:38:35Z","receivedAt":"2018-04-03T21:38:45Z","isPatch":true,"sender":{"key":"ungureanupaulsebastian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24317622?v=4"},"body":"\n\nOn 25.03.2018 10:08, Eric Sunshine wrote:\n> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu\n> <ungureanupaulsebastian@gmail.com> wrote:\n>> Currently, because git stash is not fully converted to C, I\n>> introduced a new helper that will hold the converted commands.\n>> ---\n>> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c\n>> @@ -0,0 +1,52 @@\n>> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)\n>> +{\n>> +       int cmdmode = 0;\n>> +\n>> +       struct option options[] = {\n>> +               OPT_CMDMODE(0, \"list\", &cmdmode,\n>> +                        N_(\"list stash entries\"), LIST_STASH),\n>> +               OPT_END()\n>> +       };\n> \n> Is the intention that once git-stash--helper implements all 'stash'\n> functionality, you will simply rename git-stash--helper to git-stash?\n> If so, then I'd think that you'd want it to accept subcommand\n> arguments as bare words (\"apply\", \"drop\") in order to be consistent\n> with the existing git-stash command set, not in dashed form\n> (\"--apply\", \"--drop\"). In that case, OPT_CMDMODE doesn't seem\n> appropriate. Instead, you should be consulting argv[] directly (as in\n> [1]) after parse_options().\n> \n> [1]: https://public-inbox.org/git/20180324173707.17699-2-joel@teichroeb.net/\n\nIt makes sense. In the end, when all stash is converted, it would just \nrequire an additional pointless effort to bring (back) from dashed form \nto bare word form.\n\n>> +       argc = parse_options(argc, argv, prefix, options,\n>> +                            git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);\n>> +\n>> +       if (!cmdmode)\n>> +               usage_with_options(git_stash__helper_usage, options);\n>> +\n>> +       switch (cmdmode) {\n>> +               case LIST_STASH:\n>> +                       return list_stash(argc, argv, prefix);\n>> +       }\n>> +       return 0;\n>> +}\n>> diff --git a/git.c b/git.c\n>> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {\n>>          { \"show-branch\", cmd_show_branch, RUN_SETUP },\n>>          { \"show-ref\", cmd_show_ref, RUN_SETUP },\n>>          { \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n>> +       { \"stash--helper\", cmd_stash__helper, RUN_SETUP },\n> \n> You don't require a working tree? Seems odd for git-stash.\n> \n>>          { \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n>>          { \"stripspace\", cmd_stripspace },\n>>          { \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n\nFor now, I do not think that it is necessary (for stash list), but I am \npretty sure that it will be required in the future when porting other \ncommands.\n\nThanks for advice!\n"},{"id":"344058","messageId":"CAPig+cSMv0p6m1kjNN4bGN8prX-9Ri_XbVzHah=MugHmZh46+w@mail.gmail.com","threadId":"48134","inReplyTo":"63772b7d-7b1e-2a3a-b12c-2dae9e254b68@gmail.com","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-07T08:56:34Z","receivedAt":"2018-04-07T08:56:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 3, 2018 at 5:38 PM, Paul-Sebastian Ungureanu\n<ungureanupaulsebastian@gmail.com> wrote:\n> On 25.03.2018 10:08, Eric Sunshine wrote:\n>> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu\n>> <ungureanupaulsebastian@gmail.com> wrote:\n>>> diff --git a/git.c b/git.c\n>>> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {\n>>>          { \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n>>> +       { \"stash--helper\", cmd_stash__helper, RUN_SETUP },\n>>\n>> You don't require a working tree? Seems odd for git-stash.\n>\n> For now, I do not think that it is necessary (for stash list), but I am\n> pretty sure that it will be required in the future when porting other\n> commands.\n\nThe existing git-stash requires a working directory:\n\n% git stash list\nfatal: not a git repository (or any of the parent directories): .git\n%\n\nso it would make sense for the C port to follow suit.\n\nMore generally, a stash is a temporary storage area for changes to the\n_work tree_ (and index). Without a work tree, you wouldn't be able to\ncreate any stashes in the first place, so \"git stash list\" without a\nwork tree would be meaningless.\n"},{"id":"344059","messageId":"CAPig+cRYeUxjVS-Wf-2L9da7TTNh9PAwbiEAEV0S3qSyR_8R1A@mail.gmail.com","threadId":"48134","inReplyTo":"CAPig+cSMv0p6m1kjNN4bGN8prX-9Ri_XbVzHah=MugHmZh46+w@mail.gmail.com","subject":"Re: [RFC][PATCH] git-stash: convert git stash list to C builtin","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-04-07T08:58:39Z","receivedAt":"2018-04-07T08:58:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Apr 7, 2018 at 4:56 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> The existing git-stash requires a working directory:\n>\n> % git stash list\n> fatal: not a git repository (or any of the parent directories): .git\n> %\n\nCorrection on the error message:\n\n% git stash list\nfatal: git-stash cannot be used without a working tree.\n%\n"}]}