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

[Thomas Gummerer <t.gummerer@gmail.com>]

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
>