[v13,12/27] stash: convert drop and clear to builtin
diff mbox series

Message ID 20190225231631.30507-13-t.gummerer@gmail.com
State New
Headers show
Series
  • Convert "git stash" to C builtin
Related show

Commit Message

Thomas Gummerer Feb. 25, 2019, 11:16 p.m. UTC
From: Joel Teichroeb <joel@teichroeb.net>

Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash--helper.c | 117 ++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |   4 +-
 2 files changed, 119 insertions(+), 2 deletions(-)

Comments

Jeff King March 7, 2019, 7:15 p.m. UTC | #1
On Mon, Feb 25, 2019 at 11:16:16PM +0000, Thomas Gummerer wrote:

> +static int do_drop_stash(const char *prefix, struct stash_info *info, int quiet)

This series hit next recently, so I started building it merged with my
-Wunused-parameters series. This "prefix" parameter is not ever used.
Skimming through the function, I don't see anything that _should_ be
using it, so I think it's just cruft, and not indicative of a bug.

The same is true of create_stash() elsewhere in the series. But there it
might be worth keeping for consistency with the other top-level action
functions. The other ones pass "prefix" to parse_options(), but
create_stash() doesn't actually parse any options (and intentionally so,
since even "--help" should be taken as part of the stash message).

-Peff
Thomas Gummerer March 9, 2019, 6:30 p.m. UTC | #2
On 03/07, Jeff King wrote:
> On Mon, Feb 25, 2019 at 11:16:16PM +0000, Thomas Gummerer wrote:
> 
> > +static int do_drop_stash(const char *prefix, struct stash_info *info, int quiet)
> 
> This series hit next recently, so I started building it merged with my
> -Wunused-parameters series. This "prefix" parameter is not ever used.
> Skimming through the function, I don't see anything that _should_ be
> using it, so I think it's just cruft, and not indicative of a bug.

Agreed, I think it's only cruft, and shouldn't be used anywhere in the
function.  Below is a patch to remove the parameter.

> The same is true of create_stash() elsewhere in the series. But there it
> might be worth keeping for consistency with the other top-level action
> functions. The other ones pass "prefix" to parse_options(), but
> create_stash() doesn't actually parse any options (and intentionally so,
> since even "--help" should be taken as part of the stash message).

Agreed, I'd be happy to keep the parameter there.  Looking at your
fork, you seem to have some WIP patches to introduce a UNUSED macro
for parameters like this, which I don't think I've seen on the list
yet (though I may have just missed them).

I guess it's probably best for you to mark this parameter as UNUSED as
part of your series, but if you have a different preference on how to
handle it, let me know.

--- >8 ---
Subject: [PATCH 2/2] stash: drop unused parameter

Drop the unused prefix parameter in do_drop_stash.

We also have an unused "prefix" parameter in the 'create_stash'
function, however we leave that in place for symmetry with the other
top-level functions.

Reported-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/stash.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 6eb67c75c3..069bf14846 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -527,7 +527,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
-static int do_drop_stash(const char *prefix, struct stash_info *info, int quiet)
+static int do_drop_stash(struct stash_info *info, int quiet)
 {
 	int ret;
 	struct child_process cp_reflog = CHILD_PROCESS_INIT;
@@ -597,7 +597,7 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
 
 	assert_stash_ref(&info);
 
-	ret = do_drop_stash(prefix, &info, quiet);
+	ret = do_drop_stash(&info, quiet);
 	free_stash_info(&info);
 	return ret;
 }
@@ -626,7 +626,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix)
 		printf_ln(_("The stash entry is kept in case "
 			    "you need it again."));
 	else
-		ret = do_drop_stash(prefix, &info, quiet);
+		ret = do_drop_stash(&info, quiet);
 
 	free_stash_info(&info);
 	return ret;
@@ -663,7 +663,7 @@ static int branch_stash(int argc, const char **argv, const char *prefix)
 	if (!ret)
 		ret = do_apply_stash(prefix, &info, 1, 0);
 	if (!ret && info.is_stash_ref)
-		ret = do_drop_stash(prefix, &info, 0);
+		ret = do_drop_stash(&info, 0);
 
 	free_stash_info(&info);
Jeff King March 10, 2019, 11:26 p.m. UTC | #3
On Sat, Mar 09, 2019 at 06:30:21PM +0000, Thomas Gummerer wrote:

> On 03/07, Jeff King wrote:
> > On Mon, Feb 25, 2019 at 11:16:16PM +0000, Thomas Gummerer wrote:
> > 
> > > +static int do_drop_stash(const char *prefix, struct stash_info *info, int quiet)
> > 
> > This series hit next recently, so I started building it merged with my
> > -Wunused-parameters series. This "prefix" parameter is not ever used.
> > Skimming through the function, I don't see anything that _should_ be
> > using it, so I think it's just cruft, and not indicative of a bug.
> 
> Agreed, I think it's only cruft, and shouldn't be used anywhere in the
> function.  Below is a patch to remove the parameter.

Thanks. I did something similar temporarily for my own tree, but I'll
assume yours will go on top of the stash topic upstream. The patch below
looks good (and matches what I did locally).

> > The same is true of create_stash() elsewhere in the series. But there it
> > might be worth keeping for consistency with the other top-level action
> > functions. The other ones pass "prefix" to parse_options(), but
> > create_stash() doesn't actually parse any options (and intentionally so,
> > since even "--help" should be taken as part of the stash message).
> 
> Agreed, I'd be happy to keep the parameter there.  Looking at your
> fork, you seem to have some WIP patches to introduce a UNUSED macro
> for parameters like this, which I don't think I've seen on the list
> yet (though I may have just missed them).
> 
> I guess it's probably best for you to mark this parameter as UNUSED as
> part of your series, but if you have a different preference on how to
> handle it, let me know.

Yep, that sounds good; I'll eventually mark this UNUSED when I send
those patches. The "mark unused" ones haven't hit the list yet. I've
been trickling the patches out, 10 or so at a time, but I'm still on the
"drop ones that we can" patches, and haven't even gotten to the "mark
ones we want to keep" patches. :)

-Peff
Junio C Hamano March 11, 2019, 1:40 a.m. UTC | #4
Thomas Gummerer <t.gummerer@gmail.com> writes:

> Agreed, I'd be happy to keep the parameter there.  Looking at your
> fork, you seem to have some WIP patches to introduce a UNUSED macro
> for parameters like this, which I don't think I've seen on the list
> yet (though I may have just missed them).
>
> I guess it's probably best for you to mark this parameter as UNUSED as
> part of your series, but if you have a different preference on how to
> handle it, let me know.

I agree that the uniformity among near-toplevel helpers like
create_stash() is a good thing to have.

In the meantime, you want the patch you sent (below) on top of the
stash-in-c topic to address do_drop_stash()?

Thanks for working well together.

> --- >8 ---
> Subject: [PATCH 2/2] stash: drop unused parameter
>
> Drop the unused prefix parameter in do_drop_stash.
>
> We also have an unused "prefix" parameter in the 'create_stash'
> function, however we leave that in place for symmetry with the other
> top-level functions.
>
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  builtin/stash.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 6eb67c75c3..069bf14846 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -527,7 +527,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
>  	return ret;
>  }
>  
> -static int do_drop_stash(const char *prefix, struct stash_info *info, int quiet)
> +static int do_drop_stash(struct stash_info *info, int quiet)
>  {
>  	int ret;
>  	struct child_process cp_reflog = CHILD_PROCESS_INIT;
> @@ -597,7 +597,7 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
>  
>  	assert_stash_ref(&info);
>  
> -	ret = do_drop_stash(prefix, &info, quiet);
> +	ret = do_drop_stash(&info, quiet);
>  	free_stash_info(&info);
>  	return ret;
>  }
> @@ -626,7 +626,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix)
>  		printf_ln(_("The stash entry is kept in case "
>  			    "you need it again."));
>  	else
> -		ret = do_drop_stash(prefix, &info, quiet);
> +		ret = do_drop_stash(&info, quiet);
>  
>  	free_stash_info(&info);
>  	return ret;
> @@ -663,7 +663,7 @@ static int branch_stash(int argc, const char **argv, const char *prefix)
>  	if (!ret)
>  		ret = do_apply_stash(prefix, &info, 1, 0);
>  	if (!ret && info.is_stash_ref)
> -		ret = do_drop_stash(prefix, &info, 0);
> +		ret = do_drop_stash(&info, 0);
>  
>  	free_stash_info(&info);
Thomas Gummerer March 11, 2019, 9:40 p.m. UTC | #5
On 03/11, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > Agreed, I'd be happy to keep the parameter there.  Looking at your
> > fork, you seem to have some WIP patches to introduce a UNUSED macro
> > for parameters like this, which I don't think I've seen on the list
> > yet (though I may have just missed them).
> >
> > I guess it's probably best for you to mark this parameter as UNUSED as
> > part of your series, but if you have a different preference on how to
> > handle it, let me know.
> 
> I agree that the uniformity among near-toplevel helpers like
> create_stash() is a good thing to have.
> 
> In the meantime, you want the patch you sent (below) on top of the
> stash-in-c topic to address do_drop_stash()?

Yes, I think that would be good, thanks! (And if I read Peff's reply
in this thread correctly, I think that's his preference as well).

I also just realized that this is patch 2/2 as I created it on the
same branch as the pathspec fixups.  But they are really independent,
even though they should both end up on top of ps/stash-in-c.

> Thanks for working well together.
> 
> > --- >8 ---
> > Subject: [PATCH 2/2] stash: drop unused parameter
> >
> > Drop the unused prefix parameter in do_drop_stash.
> >
> > We also have an unused "prefix" parameter in the 'create_stash'
> > function, however we leave that in place for symmetry with the other
> > top-level functions.
> >
> > Reported-by: Jeff King <peff@peff.net>
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >  builtin/stash.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index 6eb67c75c3..069bf14846 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -527,7 +527,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix)
> >  	return ret;
> >  }
> >  
> > -static int do_drop_stash(const char *prefix, struct stash_info *info, int quiet)
> > +static int do_drop_stash(struct stash_info *info, int quiet)
> >  {
> >  	int ret;
> >  	struct child_process cp_reflog = CHILD_PROCESS_INIT;
> > @@ -597,7 +597,7 @@ static int drop_stash(int argc, const char **argv, const char *prefix)
> >  
> >  	assert_stash_ref(&info);
> >  
> > -	ret = do_drop_stash(prefix, &info, quiet);
> > +	ret = do_drop_stash(&info, quiet);
> >  	free_stash_info(&info);
> >  	return ret;
> >  }
> > @@ -626,7 +626,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix)
> >  		printf_ln(_("The stash entry is kept in case "
> >  			    "you need it again."));
> >  	else
> > -		ret = do_drop_stash(prefix, &info, quiet);
> > +		ret = do_drop_stash(&info, quiet);
> >  
> >  	free_stash_info(&info);
> >  	return ret;
> > @@ -663,7 +663,7 @@ static int branch_stash(int argc, const char **argv, const char *prefix)
> >  	if (!ret)
> >  		ret = do_apply_stash(prefix, &info, 1, 0);
> >  	if (!ret && info.is_stash_ref)
> > -		ret = do_drop_stash(prefix, &info, 0);
> > +		ret = do_drop_stash(&info, 0);
> >  
> >  	free_stash_info(&info);

Patch
diff mbox series

diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
index 03511f466b..c515f0b358 100644
--- a/builtin/stash--helper.c
+++ b/builtin/stash--helper.c
@@ -12,7 +12,14 @@ 
 #include "rerere.h"
 
 static const char * const git_stash_helper_usage[] = {
+	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
 	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
+	N_("git stash--helper clear"),
+	NULL
+};
+
+static const char * const git_stash_helper_drop_usage[] = {
+	N_("git stash--helper drop [-q|--quiet] [<stash>]"),
 	NULL
 };
 
@@ -21,6 +28,11 @@  static const char * const git_stash_helper_apply_usage[] = {
 	NULL
 };
 
+static const char * const git_stash_helper_clear_usage[] = {
+	N_("git stash--helper clear"),
+	NULL
+};
+
 static const char *ref_stash = "refs/stash";
 static struct strbuf stash_index_path = STRBUF_INIT;
 
@@ -136,6 +148,32 @@  static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	return !(ret == 0 || ret == 1);
 }
 
+static int do_clear_stash(void)
+{
+	struct object_id obj;
+	if (get_oid(ref_stash, &obj))
+		return 0;
+
+	return delete_ref(NULL, ref_stash, &obj, 0);
+}
+
+static int clear_stash(int argc, const char **argv, const char *prefix)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_helper_clear_usage,
+			     PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (argc)
+		return error(_("git stash clear with parameters is "
+			       "unimplemented"));
+
+	return do_clear_stash();
+}
+
 static int reset_tree(struct object_id *i_tree, int update, int reset)
 {
 	int nr_trees = 1;
@@ -423,6 +461,81 @@  static int apply_stash(int argc, const char **argv, const char *prefix)
 	return ret;
 }
 
+static int do_drop_stash(const char *prefix, struct stash_info *info, int quiet)
+{
+	int ret;
+	struct child_process cp_reflog = CHILD_PROCESS_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	/*
+	 * reflog does not provide a simple function for deleting refs. One will
+	 * need to be added to avoid implementing too much reflog code here
+	 */
+
+	cp_reflog.git_cmd = 1;
+	argv_array_pushl(&cp_reflog.args, "reflog", "delete", "--updateref",
+			 "--rewrite", NULL);
+	argv_array_push(&cp_reflog.args, info->revision.buf);
+	ret = run_command(&cp_reflog);
+	if (!ret) {
+		if (!quiet)
+			printf_ln(_("Dropped %s (%s)"), info->revision.buf,
+				  oid_to_hex(&info->w_commit));
+	} else {
+		return error(_("%s: Could not drop stash entry"),
+			     info->revision.buf);
+	}
+
+	/*
+	 * This could easily be replaced by get_oid, but currently it will throw
+	 * a fatal error when a reflog is empty, which we can not recover from.
+	 */
+	cp.git_cmd = 1;
+	/* Even though --quiet is specified, rev-parse still outputs the hash */
+	cp.no_stdout = 1;
+	argv_array_pushl(&cp.args, "rev-parse", "--verify", "--quiet", NULL);
+	argv_array_pushf(&cp.args, "%s@{0}", ref_stash);
+	ret = run_command(&cp);
+
+	/* do_clear_stash if we just dropped the last stash entry */
+	if (ret)
+		do_clear_stash();
+
+	return 0;
+}
+
+static void assert_stash_ref(struct stash_info *info)
+{
+	if (!info->is_stash_ref) {
+		error(_("'%s' is not a stash reference"), info->revision.buf);
+		free_stash_info(info);
+		exit(1);
+	}
+}
+
+static int drop_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret;
+	int quiet = 0;
+	struct stash_info info;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_helper_drop_usage, 0);
+
+	if (get_stash_info(&info, argc, argv))
+		return -1;
+
+	assert_stash_ref(&info);
+
+	ret = do_drop_stash(prefix, &info, quiet);
+	free_stash_info(&info);
+	return ret;
+}
+
 int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 {
 	pid_t pid = getpid();
@@ -445,6 +558,10 @@  int cmd_stash__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_stash_helper_usage, options);
 	if (!strcmp(argv[0], "apply"))
 		return !!apply_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "clear"))
+		return !!clear_stash(argc, argv, prefix);
+	else if (!strcmp(argv[0], "drop"))
+		return !!drop_stash(argc, argv, prefix);
 
 	usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
 		      git_stash_helper_usage, options);
diff --git a/git-stash.sh b/git-stash.sh
index 366a082853..b8f70230f9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -670,7 +670,7 @@  apply)
 	;;
 clear)
 	shift
-	clear_stash "$@"
+	git stash--helper clear "$@"
 	;;
 create)
 	shift
@@ -682,7 +682,7 @@  store)
 	;;
 drop)
 	shift
-	drop_stash "$@"
+	git stash--helper drop "$@"
 	;;
 pop)
 	shift