git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [RFC][PATCH] git-stash: convert git stash list to C builtin
@ 2018-03-24 18:23 Paul-Sebastian Ungureanu
  2018-03-25  7:08 ` Eric Sunshine
  2018-03-25 18:43 ` Thomas Gummerer
  0 siblings, 2 replies; 9+ messages in thread
From: Paul-Sebastian Ungureanu @ 2018-03-24 18:23 UTC (permalink / raw)
  To: git; +Cc: 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 000000000..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 [<options>]"),
+	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, &obj))
+		return 0;
+
+	argv_array_pushl(&args, "log", "--format=%gd: %gs", "-g", "--first-parent", "-m", NULL);
+	argv_array_pushv(&args, argv);
+	argv_array_push(&args, 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", &cmdmode,
+			 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-03-24 18:23 [RFC][PATCH] git-stash: convert git stash list to C builtin Paul-Sebastian Ungureanu
@ 2018-03-25  7:08 ` Eric Sunshine
  2018-03-26 19:03   ` Johannes Schindelin
  2018-04-03 21:38   ` Paul-Sebastian Ungureanu
  2018-03-25 18:43 ` Thomas Gummerer
  1 sibling, 2 replies; 9+ messages in thread
From: Eric Sunshine @ 2018-03-25  7:08 UTC (permalink / raw)
  To: Paul-Sebastian Ungureanu; +Cc: Git List

On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
<ungureanupaulsebastian@gmail.com> 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", &cmdmode,
> +                        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-joel@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},

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-03-24 18:23 [RFC][PATCH] git-stash: convert git stash list to C builtin Paul-Sebastian Ungureanu
  2018-03-25  7:08 ` Eric Sunshine
@ 2018-03-25 18:43 ` Thomas Gummerer
  1 sibling, 0 replies; 9+ messages in thread
From: Thomas Gummerer @ 2018-03-25 18:43 UTC (permalink / raw)
  To: Paul-Sebastian Ungureanu; +Cc: git

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 000000000..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 [<options>]"),
> +	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, &obj))
> +		return 0;
> +
> +	argv_array_pushl(&args, "log", "--format=%gd: %gs", "-g", "--first-parent", "-m", NULL);
> +	argv_array_pushv(&args, argv);
> +	argv_array_push(&args, 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", &cmdmode,
> +			 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
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-03-25  7:08 ` Eric Sunshine
@ 2018-03-26 19:03   ` Johannes Schindelin
  2018-03-26 20:58     ` Thomas Gummerer
  2018-04-03 21:38   ` Paul-Sebastian Ungureanu
  1 sibling, 1 reply; 9+ messages in thread
From: Johannes Schindelin @ 2018-03-26 19:03 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Paul-Sebastian Ungureanu, Git List

Hi Eric,

On Sun, 25 Mar 2018, Eric Sunshine wrote:

> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
> <ungureanupaulsebastian@gmail.com> 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", &cmdmode,
> > +                        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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-03-26 19:03   ` Johannes Schindelin
@ 2018-03-26 20:58     ` Thomas Gummerer
  2018-03-30 10:29       ` Johannes Schindelin
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Gummerer @ 2018-03-26 20:58 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Eric Sunshine, Paul-Sebastian Ungureanu, Git List

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
> > <ungureanupaulsebastian@gmail.com> 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", &cmdmode,
> > > +                        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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-03-26 20:58     ` Thomas Gummerer
@ 2018-03-30 10:29       ` Johannes Schindelin
  0 siblings, 0 replies; 9+ messages in thread
From: Johannes Schindelin @ 2018-03-30 10:29 UTC (permalink / raw)
  To: Thomas Gummerer; +Cc: Eric Sunshine, Paul-Sebastian Ungureanu, Git List

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
> > > <ungureanupaulsebastian@gmail.com> 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", &cmdmode,
> > > > +                        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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-03-25  7:08 ` Eric Sunshine
  2018-03-26 19:03   ` Johannes Schindelin
@ 2018-04-03 21:38   ` Paul-Sebastian Ungureanu
  2018-04-07  8:56     ` Eric Sunshine
  1 sibling, 1 reply; 9+ messages in thread
From: Paul-Sebastian Ungureanu @ 2018-04-03 21:38 UTC (permalink / raw)
  To: Eric Sunshine; +Cc: Git List



On 25.03.2018 10:08, Eric Sunshine wrote:
> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
> <ungureanupaulsebastian@gmail.com> 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", &cmdmode,
>> +                        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-joel@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!

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-04-03 21:38   ` Paul-Sebastian Ungureanu
@ 2018-04-07  8:56     ` Eric Sunshine
  2018-04-07  8:58       ` Eric Sunshine
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Sunshine @ 2018-04-07  8:56 UTC (permalink / raw)
  To: Paul-Sebastian Ungureanu; +Cc: Git List

On Tue, Apr 3, 2018 at 5:38 PM, Paul-Sebastian Ungureanu
<ungureanupaulsebastian@gmail.com> wrote:
> On 25.03.2018 10:08, Eric Sunshine wrote:
>> On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
>> <ungureanupaulsebastian@gmail.com> 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
  2018-04-07  8:56     ` Eric Sunshine
@ 2018-04-07  8:58       ` Eric Sunshine
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Sunshine @ 2018-04-07  8:58 UTC (permalink / raw)
  To: Paul-Sebastian Ungureanu; +Cc: Git List

On Sat, Apr 7, 2018 at 4:56 AM, Eric Sunshine <sunshine@sunshineco.com> 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.
%

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-04-07  8:58 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-24 18:23 [RFC][PATCH] git-stash: convert git stash list to C builtin Paul-Sebastian Ungureanu
2018-03-25  7:08 ` Eric Sunshine
2018-03-26 19:03   ` Johannes Schindelin
2018-03-26 20:58     ` Thomas Gummerer
2018-03-30 10:29       ` Johannes Schindelin
2018-04-03 21:38   ` Paul-Sebastian Ungureanu
2018-04-07  8:56     ` Eric Sunshine
2018-04-07  8:58       ` Eric Sunshine
2018-03-25 18:43 ` Thomas Gummerer

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).