Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Eric Sunshine]

On Sat, Apr 7, 2018 at 4:56 AM, Eric Sunshine  wrote:
> The existing git-stash requires a working directory:
>
> % git stash list
> fatal: not a git repository (or any of the parent directories): .git
> %

Correction on the error message:

% git stash list
fatal: git-stash cannot be used without a working tree.
%

Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Eric Sunshine]

On Tue, Apr 3, 2018 at 5:38 PM, Paul-Sebastian Ungureanu
 wrote:
> On 25.03.2018 10:08, Eric Sunshine wrote:
>> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
>>  wrote:
>>> diff --git a/git.c b/git.c
>>> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {
>>>  { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
>>> +   { "stash--helper", cmd_stash__helper, RUN_SETUP },
>>
>> You don't require a working tree? Seems odd for git-stash.
>
> For now, I do not think that it is necessary (for stash list), but I am
> pretty sure that it will be required in the future when porting other
> commands.

The existing git-stash requires a working directory:

% git stash list
fatal: not a git repository (or any of the parent directories): .git
%

so it would make sense for the C port to follow suit.

More generally, a stash is a temporary storage area for changes to the
_work tree_ (and index). Without a work tree, you wouldn't be able to
create any stashes in the first place, so "git stash list" without a
work tree would be meaningless.

Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Paul-Sebastian Ungureanu]

On 25.03.2018 10:08, Eric Sunshine wrote:

On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
 wrote:

Currently, because git stash is not fully converted to C, I
introduced a new helper that will hold the converted commands.
---
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
@@ -0,0 +1,52 @@
+int cmd_stash__helper(int argc, const char **argv, const char *prefix)
+{
+   int cmdmode = 0;
+
+   struct option options[] = {
+   OPT_CMDMODE(0, "list", ,
+N_("list stash entries"), LIST_STASH),
+   OPT_END()
+   };


Is the intention that once git-stash--helper implements all 'stash'
functionality, you will simply rename git-stash--helper to git-stash?
If so, then I'd think that you'd want it to accept subcommand
arguments as bare words ("apply", "drop") in order to be consistent
with the existing git-stash command set, not in dashed form
("--apply", "--drop"). In that case, OPT_CMDMODE doesn't seem
appropriate. Instead, you should be consulting argv[] directly (as in
[1]) after parse_options().

[1]: https://public-inbox.org/git/20180324173707.17699-2-j...@teichroeb.net/


It makes sense. In the end, when all stash is converted, it would just 
require an additional pointless effort to bring (back) from dashed form 
to bare word form.



+   argc = parse_options(argc, argv, prefix, options,
+git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+   if (!cmdmode)
+   usage_with_options(git_stash__helper_usage, options);
+
+   switch (cmdmode) {
+   case LIST_STASH:
+   return list_stash(argc, argv, prefix);
+   }
+   return 0;
+}
diff --git a/git.c b/git.c
@@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {
 { "show-branch", cmd_show_branch, RUN_SETUP },
 { "show-ref", cmd_show_ref, RUN_SETUP },
 { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+   { "stash--helper", cmd_stash__helper, RUN_SETUP },


You don't require a working tree? Seems odd for git-stash.


 { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 { "stripspace", cmd_stripspace },
 { "submodule--helper", cmd_submodule__helper, RUN_SETUP | 
SUPPORT_SUPER_PREFIX},


For now, I do not think that it is necessary (for stash list), but I am 
pretty sure that it will be required in the future when porting other 
commands.


Thanks for advice!

Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Johannes Schindelin]

Hi Thomas,

On Mon, 26 Mar 2018, Thomas Gummerer wrote:

> On 03/26, Johannes Schindelin wrote:
> > 
> > On Sun, 25 Mar 2018, Eric Sunshine wrote:
> > 
> > > On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
> > >  wrote:
> > > > Currently, because git stash is not fully converted to C, I
> > > > introduced a new helper that will hold the converted commands.
> > > > ---
> > > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> > > > @@ -0,0 +1,52 @@
> > > > +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> > > > +{
> > > > +   int cmdmode = 0;
> > > > +
> > > > +   struct option options[] = {
> > > > +   OPT_CMDMODE(0, "list", ,
> > > > +N_("list stash entries"), LIST_STASH),
> > > > +   OPT_END()
> > > > +   };
> > > 
> > > Is the intention that once git-stash--helper implements all 'stash'
> > > functionality, you will simply rename git-stash--helper to git-stash?
> > > If so, then I'd think that you'd want it to accept subcommand
> > > arguments as bare words ("apply", "drop") in order to be consistent
> > > with the existing git-stash command set, not in dashed form
> > 
> > Why not start with cmdmode, and then add a single patch that *also*
> > accepts argv[1] as a bare-word cmdmode?
> 
> I don't think we should accept the dashed form of the commands for
> 'git stash'.  The main reason being that we also have 'git stash'
> without any arguments, which acts as 'git stash push'.  So if we would
> ever come up with an argument to 'git stash push', that matches one of
> the current verbs, or if we come up with a new verb that matches one
> of the options to 'git stash push', that would not work.
> 
> In that case we could obviously go for a different word, but I think
> the rules when 'git stash' is going to be 'git stash push' and when it
> is not are already complicated enough, and allowing the verbs as
> dashed options would only make the interface more complicated.
> 
> > This could even potentially be a patch to parse-options.[ch] that
> > introduces, say, PARSE_OPT_ALLOW_BARE_CMDMODE.
> 
> Now if we'd take that one step further and make it
> PARSE_OPT_BARE_CMDMODE, which would only allow the non-dashed options,
> I think we could use that in other places in git as well (for example
> in 'git worktree').

But the valid options change with the subcommands, too. So my idea, even
with your adjustment to disallow dashed cmdmode, simply does not work.

Thanks for setting my head straight,
Dscho

Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Thomas Gummerer]

On 03/26, Johannes Schindelin wrote:
> Hi Eric,
> 
> On Sun, 25 Mar 2018, Eric Sunshine wrote:
> 
> > On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
> >  wrote:
> > > Currently, because git stash is not fully converted to C, I
> > > introduced a new helper that will hold the converted commands.
> > > ---
> > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> > > @@ -0,0 +1,52 @@
> > > +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> > > +{
> > > +   int cmdmode = 0;
> > > +
> > > +   struct option options[] = {
> > > +   OPT_CMDMODE(0, "list", ,
> > > +N_("list stash entries"), LIST_STASH),
> > > +   OPT_END()
> > > +   };
> > 
> > Is the intention that once git-stash--helper implements all 'stash'
> > functionality, you will simply rename git-stash--helper to git-stash?
> > If so, then I'd think that you'd want it to accept subcommand
> > arguments as bare words ("apply", "drop") in order to be consistent
> > with the existing git-stash command set, not in dashed form
> 
> Why not start with cmdmode, and then add a single patch that *also*
> accepts argv[1] as a bare-word cmdmode?

I don't think we should accept the dashed form of the commands for
'git stash'.  The main reason being that we also have 'git stash'
without any arguments, which acts as 'git stash push'.  So if we would
ever come up with an argument to 'git stash push', that matches one of
the current verbs, or if we come up with a new verb that matches one
of the options to 'git stash push', that would not work.

In that case we could obviously go for a different word, but I think
the rules when 'git stash' is going to be 'git stash push' and when it
is not are already complicated enough, and allowing the verbs as
dashed options would only make the interface more complicated.

> This could even potentially be a patch to parse-options.[ch] that
> introduces, say, PARSE_OPT_ALLOW_BARE_CMDMODE.

Now if we'd take that one step further and make it
PARSE_OPT_BARE_CMDMODE, which would only allow the non-dashed options,
I think we could use that in other places in git as well (for example
in 'git worktree').

> Ciao,
> Dscho

Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Johannes Schindelin]

Hi Eric,

On Sun, 25 Mar 2018, Eric Sunshine wrote:

> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
>  wrote:
> > Currently, because git stash is not fully converted to C, I
> > introduced a new helper that will hold the converted commands.
> > ---
> > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> > @@ -0,0 +1,52 @@
> > +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> > +{
> > +   int cmdmode = 0;
> > +
> > +   struct option options[] = {
> > +   OPT_CMDMODE(0, "list", ,
> > +N_("list stash entries"), LIST_STASH),
> > +   OPT_END()
> > +   };
> 
> Is the intention that once git-stash--helper implements all 'stash'
> functionality, you will simply rename git-stash--helper to git-stash?
> If so, then I'd think that you'd want it to accept subcommand
> arguments as bare words ("apply", "drop") in order to be consistent
> with the existing git-stash command set, not in dashed form

Why not start with cmdmode, and then add a single patch that *also*
accepts argv[1] as a bare-word cmdmode?

This could even potentially be a patch to parse-options.[ch] that
introduces, say, PARSE_OPT_ALLOW_BARE_CMDMODE.

Ciao,
Dscho

Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Thomas Gummerer]

On 03/24, Paul-Sebastian Ungureanu wrote:
> Currently, because git stash is not fully converted to C, I
> introduced a new helper that will hold the converted commands.

Missing sign-off?  I think it's a good idea to sign off your work even
for RFC patches that you don't expect to be applied.  If for nothing
else, it helps people not wonder whether you just forgot the sign-off
or if you intentionally didn't add it.

> ---
>  Makefile|  1 +
>  builtin.h   |  1 +
>  builtin/stash--helper.c | 52 +
>  git-stash.sh|  7 +-
>  git.c   |  1 +
>  5 files changed, 56 insertions(+), 6 deletions(-)
>  create mode 100644 builtin/stash--helper.c
> 
> diff --git a/Makefile b/Makefile
> index a1d8775ad..8ca361c57 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1020,6 +1020,7 @@ BUILTIN_OBJS += builtin/send-pack.o
>  BUILTIN_OBJS += builtin/shortlog.o
>  BUILTIN_OBJS += builtin/show-branch.o
>  BUILTIN_OBJS += builtin/show-ref.o
> +BUILTIN_OBJS += builtin/stash--helper.o
>  BUILTIN_OBJS += builtin/stripspace.o
>  BUILTIN_OBJS += builtin/submodule--helper.o
>  BUILTIN_OBJS += builtin/symbolic-ref.o
> diff --git a/builtin.h b/builtin.h
> index 42378f3aa..2ddb4bd5c 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -220,6 +220,7 @@ extern int cmd_show(int argc, const char **argv, const 
> char *prefix);
>  extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
>  extern int cmd_status(int argc, const char **argv, const char *prefix);
>  extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
> +extern int cmd_stash__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_submodule__helper(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
>  extern int cmd_tag(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> new file mode 100644
> index 0..61fd5390d
> --- /dev/null
> +++ b/builtin/stash--helper.c
> @@ -0,0 +1,52 @@
> +#include "builtin.h"
> +#include "cache.h"
> +#include "parse-options.h"
> +#include "argv-array.h"
> +
> +enum {
> + LIST_STASH = 1
> +};
> +
> +static const char * ref_stash = "refs/stash";
> +
> +static const char * const git_stash__helper_usage[] = {
> + N_("git stash--helper --list []"),
> + NULL
> +};
> +
> +static int list_stash(int argc, const char **argv, const char *prefix)
> +{
> + struct object_id obj;
> + struct argv_array args = ARGV_ARRAY_INIT;
> +
> + if (get_oid(ref_stash, ))
> + return 0;
> +
> + argv_array_pushl(, "log", "--format=%gd: %gs", "-g", 
> "--first-parent", "-m", NULL);
> + argv_array_pushv(, argv);
> + argv_array_push(, ref_stash);

This is missing the final '--' argument to 'git log'.  It's needed to
disambiguate the ref from paths.  If we don't have that, this git log
call will fail when a "refs/stash" directory exists.

> + return !!cmd_log(args.argc, args.argv, prefix);
> +}
> +
> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> +{
> + int cmdmode = 0;
> +
> + struct option options[] = {
> + OPT_CMDMODE(0, "list", ,
> +  N_("list stash entries"), LIST_STASH),
> + OPT_END()
> + };
> +
> + argc = parse_options(argc, argv, prefix, options,
> +  git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);
> +
> + if (!cmdmode)
> + usage_with_options(git_stash__helper_usage, options);
> +
> + switch (cmdmode) {
> + case LIST_STASH:
> + return list_stash(argc, argv, prefix);
> + }
> + return 0;
> +}
> diff --git a/git-stash.sh b/git-stash.sh
> index fc8f8ae64..a5b9f5fb6 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -380,11 +380,6 @@ have_stash () {
>   git rev-parse --verify --quiet $ref_stash >/dev/null
>  }
>  
> -list_stash () {
> - have_stash || return 0
> - git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
> -}
> -
>  show_stash () {
>   ALLOW_UNKNOWN_FLAGS=t
>   assert_stash_like "$@"
> @@ -695,7 +690,7 @@ test -n "$seen_non_option" || set "push" "$@"
>  case "$1" in
>  list)
>   shift
> - list_stash "$@"
> + git stash--helper --list "$@"
>   ;;
>  show)
>   shift
> diff --git a/git.c b/git.c
> index 96cd734f1..6fd2ccd9a 100644
> --- a/git.c
> +++ b/git.c
> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {
>   { "show-branch", cmd_show_branch, RUN_SETUP },
>   { "show-ref", cmd_show_ref, RUN_SETUP },
>   { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> + { "stash--helper", cmd_stash__helper, RUN_SETUP },
>   { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
>   { "stripspace", cmd_stripspace },
>   { "submodule--helper", cmd_submodule__helper, 

Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

[Eric Sunshine]

On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
 wrote:
> Currently, because git stash is not fully converted to C, I
> introduced a new helper that will hold the converted commands.
> ---
> diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> @@ -0,0 +1,52 @@
> +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> +{
> +   int cmdmode = 0;
> +
> +   struct option options[] = {
> +   OPT_CMDMODE(0, "list", ,
> +N_("list stash entries"), LIST_STASH),
> +   OPT_END()
> +   };

Is the intention that once git-stash--helper implements all 'stash'
functionality, you will simply rename git-stash--helper to git-stash?
If so, then I'd think that you'd want it to accept subcommand
arguments as bare words ("apply", "drop") in order to be consistent
with the existing git-stash command set, not in dashed form
("--apply", "--drop"). In that case, OPT_CMDMODE doesn't seem
appropriate. Instead, you should be consulting argv[] directly (as in
[1]) after parse_options().

[1]: https://public-inbox.org/git/20180324173707.17699-2-j...@teichroeb.net/

> +   argc = parse_options(argc, argv, prefix, options,
> +git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);
> +
> +   if (!cmdmode)
> +   usage_with_options(git_stash__helper_usage, options);
> +
> +   switch (cmdmode) {
> +   case LIST_STASH:
> +   return list_stash(argc, argv, prefix);
> +   }
> +   return 0;
> +}
> diff --git a/git.c b/git.c
> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {
> { "show-branch", cmd_show_branch, RUN_SETUP },
> { "show-ref", cmd_show_ref, RUN_SETUP },
> { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> +   { "stash--helper", cmd_stash__helper, RUN_SETUP },

You don't require a working tree? Seems odd for git-stash.

> { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
> { "stripspace", cmd_stripspace },
> { "submodule--helper", cmd_submodule__helper, RUN_SETUP | 
> SUPPORT_SUPER_PREFIX},

[RFC][PATCH] git-stash: convert git stash list to C builtin

[Paul-Sebastian Ungureanu]

Currently, because git stash is not fully converted to C, I
introduced a new helper that will hold the converted commands.
---
 Makefile|  1 +
 builtin.h   |  1 +
 builtin/stash--helper.c | 52 +
 git-stash.sh|  7 +-
 git.c   |  1 +
 5 files changed, 56 insertions(+), 6 deletions(-)
 create mode 100644 builtin/stash--helper.c

diff --git a/Makefile b/Makefile
index a1d8775ad..8ca361c57 100644
--- a/Makefile
+++ b/Makefile
@@ -1020,6 +1020,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa..2ddb4bd5c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -220,6 +220,7 @@ extern int cmd_show(int argc, const char **argv, const char 
*prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_tag(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 0..61fd5390d
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,52 @@
+#include "builtin.h"
+#include "cache.h"
+#include "parse-options.h"
+#include "argv-array.h"
+
+enum {
+   LIST_STASH = 1
+};
+
+static const char * ref_stash = "refs/stash";
+
+static const char * const git_stash__helper_usage[] = {
+   N_("git stash--helper --list []"),
+   NULL
+};
+
+static int list_stash(int argc, const char **argv, const char *prefix)
+{
+   struct object_id obj;
+   struct argv_array args = ARGV_ARRAY_INIT;
+
+   if (get_oid(ref_stash, ))
+   return 0;
+
+   argv_array_pushl(, "log", "--format=%gd: %gs", "-g", 
"--first-parent", "-m", NULL);
+   argv_array_pushv(, argv);
+   argv_array_push(, ref_stash);
+   return !!cmd_log(args.argc, args.argv, prefix);
+}
+
+int cmd_stash__helper(int argc, const char **argv, const char *prefix)
+{
+   int cmdmode = 0;
+
+   struct option options[] = {
+   OPT_CMDMODE(0, "list", ,
+N_("list stash entries"), LIST_STASH),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options,
+git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+   if (!cmdmode)
+   usage_with_options(git_stash__helper_usage, options);
+
+   switch (cmdmode) {
+   case LIST_STASH:
+   return list_stash(argc, argv, prefix);
+   }
+   return 0;
+}
diff --git a/git-stash.sh b/git-stash.sh
index fc8f8ae64..a5b9f5fb6 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -380,11 +380,6 @@ have_stash () {
git rev-parse --verify --quiet $ref_stash >/dev/null
 }
 
-list_stash () {
-   have_stash || return 0
-   git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash --
-}
-
 show_stash () {
ALLOW_UNKNOWN_FLAGS=t
assert_stash_like "$@"
@@ -695,7 +690,7 @@ test -n "$seen_non_option" || set "push" "$@"
 case "$1" in
 list)
shift
-   list_stash "$@"
+   git stash--helper --list "$@"
;;
 show)
shift
diff --git a/git.c b/git.c
index 96cd734f1..6fd2ccd9a 100644
--- a/git.c
+++ b/git.c
@@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {
{ "show-branch", cmd_show_branch, RUN_SETUP },
{ "show-ref", cmd_show_ref, RUN_SETUP },
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+   { "stash--helper", cmd_stash__helper, RUN_SETUP },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
{ "stripspace", cmd_stripspace },
{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | 
SUPPORT_SUPER_PREFIX},
-- 
2.16.2.647.gb9d10dde1