threads / patch / 12823

patch, 2 partsbuiltin-prune.c: fix object parsing and use parse_options()

Subject: [PATCH 2/2] builtin-prune.c: fix object parsing and use parse_options()

## tl;dr

22 messages between Mar 23, 2008 and Mar 27, 2008. Diffs are folded; open one to read it.

replies: 21people: 4as markdown or json

Michele Ballabio· Mar 23, 2008, 20:50 UTC · lore

git-prune used to remove loose objects whether they were specified in the command line or not. This patch makes git-prune behave as stated in the manpage: it does not prune any listed head nor reachable objects; the parsing machinery now uses parse_options().

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-prune.c  |   50 ++++++++++++++++++++++++++++++--------------------
 t/t5304-prune.sh |    2 +-
 2 files changed, 31 insertions(+), 21 deletions(-)
Show changes to 2 files +31 −21

builtin-prune.c, t/t5304-prune.sh

diff --git a/builtin-prune.c b/builtin-prune.c
index bb8ead9..7b3e15d 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -4,8 +4,12 @@
 #include "revision.h"
 #include "builtin.h"
 #include "reachable.h"
+#include "parse-options.h"
 
-static const char prune_usage[] = "git-prune [-n]";
+static const char * const prune_usage[] = {
+	"git-prune [-n] [--expire <time>] [--] [<head>...]",
+	NULL
+};
 static int show_only;
 static unsigned long expire;
 
@@ -121,32 +125,38 @@ static void remove_temporary_files(void)
 	closedir(dir);
 }
 
+static int parse_opt_expire(const struct option *opt, const char *arg,
+		int unset)
+{
+	expire = approxidate(arg);
+	return 0;
+}
+
 int cmd_prune(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	struct rev_info revs;
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "-n")) {
-			show_only = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--expire")) {
-			if (++i < argc) {
-				expire = approxidate(argv[i]);
-				continue;
-			}
-		}
-		else if (!prefixcmp(arg, "--expire=")) {
-			expire = approxidate(arg + 9);
-			continue;
-		}
-		usage(prune_usage);
-	}
+	const struct option options[] = {
+		OPT_BOOLEAN('n', NULL, &show_only,
+				"do not remove, show only"),
+		OPT_CALLBACK(0, "expire", &expire, "time",
+				"expire objects older than <time>",
+				parse_opt_expire),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, options, prune_usage,
+			PARSE_OPT_STOP_AT_NON_OPTION |
+			PARSE_OPT_KEEP_DASHDASH);
 
 	save_commit_buffer = 0;
 	init_revisions(&revs, prefix);
+
+	if (argc > 1)
+		argc = setup_revisions(argc, argv, &revs, NULL);
+	if (argc > 1)
+		die ("unrecognized argument: %s", argv[1]);
+
 	mark_reachable_objects(&revs, 1);
 
 	prune_object_dir(get_object_directory());
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index d5f12f7..0638297 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -92,7 +92,7 @@ test_expect_success 'prune: prune unreachable heads' '
 
 '
 
-test_expect_failure 'prune: do not prune heads listed as an argument' '
+test_expect_success 'prune: do not prune heads listed as an argument' '
 
 	: > file2 &&
 	git add file2 &&

-- 
1.5.4.3
Johannes Schindelin· Mar 23, 2008, 22:21 UTC · re: Michele Ballabio · lore

Re: [PATCH 2/2] builtin-prune.c: fix object parsing and use parse_options()

Hi,
On Sun, 23 Mar 2008, Michele Ballabio wrote:
Show 5 quoted lines
> -static const char prune_usage[] = "git-prune [-n]";
> +static const char * const prune_usage[] = {
> +	"git-prune [-n] [--expire <time>] [--] [<head>...]",
> +	NULL
> +};
As you already use parse-options, should this not be rather
static const char * const prune_usage[] = {
	"git-prune [options] [--] [<commit>...]",
Hmm?
Show 6 quoted lines
> +static int parse_opt_expire(const struct option *opt, const char *arg,
> +		int unset)
> +{
> +	expire = approxidate(arg);
> +	return 0;
> +}
This would probably be a good candidate to live in parse-options.[ch], no?
But yes, the patch is good!

Ciao, Dscho

Michele Ballabio· Mar 24, 2008, 12:31 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/2] builtin-prune.c: fix object parsing and use parse_options()

On Sunday 23 March 2008, Johannes Schindelin wrote:
Show 14 quoted lines
> On Sun, 23 Mar 2008, Michele Ballabio wrote:
> 
> > -static const char prune_usage[] = "git-prune [-n]";
> > +static const char * const prune_usage[] = {
> > +	"git-prune [-n] [--expire <time>] [--] [<head>...]",
> > +	NULL
> > +};
> 
> As you already use parse-options, should this not be rather
> 
> static const char * const prune_usage[] = {
> 	"git-prune [options] [--] [<commit>...]",
> 
> Hmm?

Ok, but the usage string is quite short anyway... and other commands show a similar quite detailed usage string. Not that I care strongly about this, though.

Show 8 quoted lines
> > +static int parse_opt_expire(const struct option *opt, const char *arg,
> > +		int unset)
> > +{
> > +	expire = approxidate(arg);
> > +	return 0;
> > +}
> 
> This would probably be a good candidate to live in parse-options.[ch], no?
Uhm, probably, yes. See the patch below.
> But yes, the patch is good!
Thank you.

-- >8 -- parse-options.c: introduce callback function for approxidate()

There are quite a few places that will need to call approxidate(), when they'll adopt the parse-options system, so this patch adds the function parse_opt_approxidate_cb(), to be used within OPT_CALLBACK, and converts the only user so far.

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
 builtin-prune.c |    9 +--------
 parse-options.c |    7 +++++++
 parse-options.h |    2 ++
 3 files changed, 10 insertions(+), 8 deletions(-)
Show changes to 3 files +10 −8

builtin-prune.c, parse-options.c, parse-options.h

diff --git a/builtin-prune.c b/builtin-prune.c
index 7b3e15d..a5d6fe5 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -125,13 +125,6 @@ static void remove_temporary_files(void)
 	closedir(dir);
 }
 
-static int parse_opt_expire(const struct option *opt, const char *arg,
-		int unset)
-{
-	expire = approxidate(arg);
-	return 0;
-}
-
 int cmd_prune(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -141,7 +134,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 				"do not remove, show only"),
 		OPT_CALLBACK(0, "expire", &expire, "time",
 				"expire objects older than <time>",
-				parse_opt_expire),
+				parse_opt_approxidate_cb),
 		OPT_END()
 	};
 
diff --git a/parse-options.c b/parse-options.c
index 8e64316..6ec7fe8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -409,3 +409,10 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 	*(int *)(opt->value) = v;
 	return 0;
 }
+
+int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
+		int unset)
+{
+	*(unsigned int *)(opt->value) = approxidate(arg);
+	return 0;
+}
diff --git a/parse-options.h b/parse-options.h
index 1af62b0..e6976ed 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -110,6 +110,8 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
 
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
+extern int parse_opt_approxidate_cb(const struct option *, const char *,
+		int);
 
 #define OPT__VERBOSE(var)  OPT_BOOLEAN('v', "verbose", (var), "be verbose")
 #define OPT__QUIET(var)    OPT_BOOLEAN('q', "quiet",   (var), "be quiet")

-- 
1.5.4.3
Johannes Schindelin· Mar 24, 2008, 13:13 UTC · re: Michele Ballabio · lore

Re: [PATCH 2/2] builtin-prune.c: fix object parsing and use parse_options()

Hi,
On Mon, 24 Mar 2008, Michele Ballabio wrote:
Show 13 quoted lines
> diff --git a/parse-options.h b/parse-options.h
> index 1af62b0..e6976ed 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -110,6 +110,8 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
>  
>  /*----- some often used options -----*/
>  extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
> +extern int parse_opt_approxidate_cb(const struct option *, const char *,
> +		int);
>  
>  #define OPT__VERBOSE(var)  OPT_BOOLEAN('v', "verbose", (var), "be verbose")
>  #define OPT__QUIET(var)    OPT_BOOLEAN('q', "quiet",   (var), "be quiet")
And maybe a

#define OPT_DATE(s, l, v, h) { OPTION_CALLBACK, (s), (l), (v), "time", (h), 0, parse_opt_approxidate_cb }

Hmm?

Ciao, Dscho

Michele Ballabio· Mar 24, 2008, 14:02 UTC · re: Johannes Schindelin · lore

[PATCH 3/2] parse-options.c: introduce OPT_DATE

There are quite a few places that will need to call approxidate(), when they'll adopt the parse-options system, so this patch adds the function parse_opt_approxidate_cb(), used by OPT_DATE, and converts the only user so far.

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
---
On Monday 24 March 2008, Johannes Schindelin wrote:
> And maybe a
> 
> #define OPT_DATE(s, l, v, h)    { OPTION_CALLBACK, (s), (l), (v), "time", 
> (h), 0, parse_opt_approxidate_cb }
Oh, right. Somehow I thought it was simpler not to do this.
Thank you for your review and suggestions.

This is on top of [PATCH 2/2] builtin-prune.c: fix object parsing and use parse_options()

 builtin-prune.c |   12 ++----------
 parse-options.c |    7 +++++++
 parse-options.h |    5 +++++
 3 files changed, 14 insertions(+), 10 deletions(-)
Show changes to 3 files +14 −10

builtin-prune.c, parse-options.c, parse-options.h

diff --git a/builtin-prune.c b/builtin-prune.c
index 7b3e15d..40581df 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -125,13 +125,6 @@ static void remove_temporary_files(void)
 	closedir(dir);
 }
 
-static int parse_opt_expire(const struct option *opt, const char *arg,
-		int unset)
-{
-	expire = approxidate(arg);
-	return 0;
-}
-
 int cmd_prune(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
@@ -139,9 +132,8 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	const struct option options[] = {
 		OPT_BOOLEAN('n', NULL, &show_only,
 				"do not remove, show only"),
-		OPT_CALLBACK(0, "expire", &expire, "time",
-				"expire objects older than <time>",
-				parse_opt_expire),
+		OPT_DATE(0, "expire", &expire,
+				"expire objects older than <time>"),
 		OPT_END()
 	};
 
diff --git a/parse-options.c b/parse-options.c
index 8e64316..6ec7fe8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -409,3 +409,10 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 	*(int *)(opt->value) = v;
 	return 0;
 }
+
+int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
+		int unset)
+{
+	*(unsigned int *)(opt->value) = approxidate(arg);
+	return 0;
+}
diff --git a/parse-options.h b/parse-options.h
index 1af62b0..c98f89e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -110,6 +110,8 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
 
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
+extern int parse_opt_approxidate_cb(const struct option *, const char *,
+		int);
 
 #define OPT__VERBOSE(var)  OPT_BOOLEAN('v', "verbose", (var), "be verbose")
 #define OPT__QUIET(var)    OPT_BOOLEAN('q', "quiet",   (var), "be quiet")
@@ -118,5 +120,8 @@ extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
 	{ OPTION_CALLBACK, 0, "abbrev", (var), "n", \
 	  "use <n> digits to display SHA-1s", \
 	  PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }
+#define OPT_DATE(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), "time",(h), 0, \
+	  parse_opt_approxidate_cb }
 
 #endif

-- 
1.5.4.3
Johannes Schindelin· Mar 24, 2008, 13:59 UTC · re: Michele Ballabio · lore

Re: [PATCH 3/2] parse-options.c: introduce OPT_DATE

Hi,
On Mon, 24 Mar 2008, Michele Ballabio wrote:
> There are quite a few places that will need to call approxidate(), when 
> they'll adopt the parse-options system, so this patch adds the function 
> parse_opt_approxidate_cb(), used by OPT_DATE, and converts the only user 
> so far.
Thanks!

Ciao, Dscho

Michele Ballabio· Mar 24, 2008, 16:25 UTC · re: Michele Ballabio · lore

Re: [PATCH 3/2] parse-options.c: introduce OPT_DATE

On Monday 24 March 2008, Michele Ballabio wrote:
> +               OPT_DATE(0, "expire", &expire,
[...]
> +#define OPT_DATE(s, l, v, h) \

Ooops. To be consistent, these should be OPT__DATE (with two underscores) instead (and in the commit message, too).

Johannes Schindelin· Mar 24, 2008, 20:03 UTC · re: Michele Ballabio · lore

Re: [PATCH 3/2] parse-options.c: introduce OPT_DATE

Hi,
On Mon, 24 Mar 2008, Michele Ballabio wrote:
Show 9 quoted lines
> On Monday 24 March 2008, Michele Ballabio wrote:
> > +               OPT_DATE(0, "expire", &expire,
> 
> [...]
> 
> > +#define OPT_DATE(s, l, v, h) \
> 
> Ooops. To be consistent, these should be OPT__DATE (with two 
> underscores) instead (and in the commit message, too).

I thought OPT__BLA was reserved for --bla options? IOW OPT__DATE would not be usable to implement --expire, but only --date.

I might be wrong, though.

Ciao, Dscho

Junio C Hamano· Mar 24, 2008, 20:10 UTC · re: Michele Ballabio · lore

Re: [PATCH 3/2] parse-options.c: introduce OPT_DATE

Michele Ballabio <barra_cuda@katamail.com> writes:
> +int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
> +		int unset)
> +{
> +	*(unsigned int *)(opt->value) = approxidate(arg);
Doesn't approxidate return ulong, not uint?
Michele Ballabio· Mar 24, 2008, 21:18 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/2] parse-options.c: introduce OPT_DATE

On Monday 24 March 2008, Junio C Hamano wrote:
Show 8 quoted lines
> Michele Ballabio <barra_cuda@katamail.com> writes:
> 
> > +int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
> > +             int unset)
> > +{
> > +     *(unsigned int *)(opt->value) = approxidate(arg);
> 
> Doesn't approxidate return ulong, not uint?
Yes, you're right.
Junio C Hamano· Mar 25, 2008, 06:58 UTC · re: Michele Ballabio · lore

Re: [PATCH 3/2] parse-options.c: introduce OPT_DATE

Michele Ballabio <barra_cuda@katamail.com> writes:
Show 11 quoted lines
> On Monday 24 March 2008, Junio C Hamano wrote:
>> Michele Ballabio <barra_cuda@katamail.com> writes:
>> 
>> > +int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
>> > +             int unset)
>> > +{
>> > +     *(unsigned int *)(opt->value) = approxidate(arg);
>> 
>> Doesn't approxidate return ulong, not uint?
>
> Yes, you're right.

Now, it is getting somewhat tiring to keep track of "oops, that was wrong, and here is a fix-up on top".

So here is my attempt to summarize by presenting them in an order that I think is sensible.

Junio C Hamano· Mar 25, 2008, 06:59 UTC · re: Michele Ballabio · lore

[PATCH 1/5] parse-options.c: introduce OPT_DATE

From: Michele Ballabio <barra_cuda@katamail.com>
Date: Mon, 24 Mar 2008 15:02:21 +0100

There are quite a few places that will need to call approxidate(), when they'll adopt the parse-options system, so this patch adds the function parse_opt_approxidate_cb(), used by OPT_DATE.

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c |    7 +++++++
 parse-options.h |    4 ++++
 2 files changed, 11 insertions(+), 0 deletions(-)
Show changes to 2 files +11 −0

parse-options.c, parse-options.h

diff --git a/parse-options.c b/parse-options.c
index 8e64316..e87cafb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -409,3 +409,10 @@ int parse_opt_abbrev_cb(const struct option *opt, const char *arg, int unset)
 	*(int *)(opt->value) = v;
 	return 0;
 }
+
+int parse_opt_approxidate_cb(const struct option *opt, const char *arg,
+			     int unset)
+{
+	*(unsigned long *)(opt->value) = approxidate(arg);
+	return 0;
+}
diff --git a/parse-options.h b/parse-options.h
index 1af62b0..4ee443d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -94,6 +94,9 @@ struct option {
 #define OPT_SET_PTR(s, l, v, h, p)  { OPTION_SET_PTR, (s), (l), (v), NULL, (h), 0, NULL, (p) }
 #define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), NULL, (h) }
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
+#define OPT_DATE(s, l, v, h) \
+	{ OPTION_CALLBACK, (s), (l), (v), "time",(h), 0, \
+	  parse_opt_approxidate_cb }
 #define OPT_CALLBACK(s, l, v, a, h, f) \
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), 0, (f) }
 
@@ -110,6 +113,7 @@ extern NORETURN void usage_with_options(const char * const *usagestr,
 
 /*----- some often used options -----*/
 extern int parse_opt_abbrev_cb(const struct option *, const char *, int);
+extern int parse_opt_approxidate_cb(const struct option *, const char *, int);
 
 #define OPT__VERBOSE(var)  OPT_BOOLEAN('v', "verbose", (var), "be verbose")
 #define OPT__QUIET(var)    OPT_BOOLEAN('q', "quiet",   (var), "be quiet")

-- 
1.5.5.rc1.121.g1594
Junio C Hamano· Mar 25, 2008, 06:59 UTC · re: Michele Ballabio · lore

[PATCH 2/5] test_must_fail: 129 is a valid error code from usage()

From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 24 Mar 2008 23:07:08 -0700

When a git command is run under test_must_fail to make sure that the argument parser catches bogus command line, it exits with 129. We need to catch it as a valid "graceful error exit".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to t/test-lib.sh +1 −1
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 870b255..7c2a8ba 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -300,7 +300,7 @@ test_expect_code () {
 
 test_must_fail () {
 	"$@"
-	test $? -gt 0 -a $? -le 128
+	test $? -gt 0 -a $? -le 129
 }
 
 # test_cmp is a helper function to compare actual and expected output.

-- 
1.5.5.rc1.121.g1594
Johannes Schindelin· Mar 25, 2008, 10:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/5] test_must_fail: 129 is a valid error code from usage()

Hi,
On Mon, 24 Mar 2008, Junio C Hamano wrote:
Show 10 quoted lines
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 870b255..7c2a8ba 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -300,7 +300,7 @@ test_expect_code () {
>  
>  test_must_fail () {
>  	"$@"
> -	test $? -gt 0 -a $? -le 128
> +	test $? -gt 0 -a $? -le 129

IIRC exit status is a signed byte on Win32. Can somebody check? I'll be in a hurry in 1.5 hours and travelling for two days.

Ciao, Dscho

Johannes Sixt· Mar 25, 2008, 11:21 UTC · re: Johannes Schindelin · lore

Re: [PATCH 2/5] test_must_fail: 129 is a valid error code from usage()

Johannes Schindelin schrieb:
Show 16 quoted lines
> Hi,
> 
> On Mon, 24 Mar 2008, Junio C Hamano wrote:
> 
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 870b255..7c2a8ba 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -300,7 +300,7 @@ test_expect_code () {
>>  
>>  test_must_fail () {
>>  	"$@"
>> -	test $? -gt 0 -a $? -le 128
>> +	test $? -gt 0 -a $? -le 129
> 
> IIRC exit status is a signed byte on Win32.  Can somebody check?
Not at the shell level. This command:
   git branch foo bar baz
exits with 129 both when invoked by bash ($?) and CMD (%ERRORLEVEL%).
-- Hannes
Johannes Schindelin· Mar 25, 2008, 19:27 UTC · re: Johannes Sixt · lore

Re: [PATCH 2/5] test_must_fail: 129 is a valid error code from usage()

Hi,
On Tue, 25 Mar 2008, Johannes Sixt wrote:
Show 5 quoted lines
> This command:
> 
>    git branch foo bar baz
> 
> exits with 129 both when invoked by bash ($?) and CMD (%ERRORLEVEL%).

Thanks, Dscho

Junio C Hamano· Mar 25, 2008, 06:59 UTC · re: Michele Ballabio · lore

[PATCH 3/5] Add tests for git-prune

From: Michele Ballabio <barra_cuda@katamail.com>
Date: Sun, 23 Mar 2008 22:34:34 +0100

It seems that git prune changed behaviour with respect to revisions added from command line, probably when it became a builtin. Currently, it prints a short usage and exits: instead, it should take those revisions into account and not prune them. So add a couple of test to point this out.

We'll be fixing this by switching to parse_options(), so add tests to detect bogus command line parameters as well, to keep ourselves from introducing regressions.

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5304-prune.sh |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)
Show changes to t/t5304-prune.sh +34 −0
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 47090c4..3d81e1f 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -78,4 +78,38 @@ test_expect_success 'gc: start with ok gc.pruneExpire' '
 
 '
 
+test_expect_success 'prune: prune nonsense parameters' '
+
+	test_must_fail git prune garbage &&
+	test_must_fail git prune --- &&
+	test_must_fail git prune --no-such-option
+
+'
+
+test_expect_success 'prune: prune unreachable heads' '
+
+	git config core.logAllRefUpdates false &&
+	mv .git/logs .git/logs.old &&
+	: > file2 &&
+	git add file2 &&
+	git commit -m temporary &&
+	tmp_head=$(git rev-list -1 HEAD) &&
+	git reset HEAD^ &&
+	git prune &&
+	test_must_fail git reset $tmp_head --
+
+'
+
+test_expect_failure 'prune: do not prune heads listed as an argument' '
+
+	: > file2 &&
+	git add file2 &&
+	git commit -m temporary &&
+	tmp_head=$(git rev-list -1 HEAD) &&
+	git reset HEAD^ &&
+	git prune -- $tmp_head &&
+	git reset $tmp_head --
+
+'
+
 test_done

-- 
1.5.5.rc1.121.g1594
Junio C Hamano· Mar 25, 2008, 06:59 UTC · re: Michele Ballabio · lore

[PATCH 4/5] builtin-prune.c: use parse_options()

From: Michele Ballabio <barra_cuda@katamail.com>
Date: Sun, 23 Mar 2008 21:50:29 +0100

Using the OPT_DATE() introduced earlier, this updates builtin-prune to use parse_options().

Signed-off-by: Michele Ballabio <barra_cuda@katamail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-prune.c |   38 ++++++++++++++++----------------------
 1 files changed, 16 insertions(+), 22 deletions(-)
Show changes to builtin-prune.c +16 −22
diff --git a/builtin-prune.c b/builtin-prune.c
index bb8ead9..71caac5 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -4,8 +4,12 @@
 #include "revision.h"
 #include "builtin.h"
 #include "reachable.h"
+#include "parse-options.h"
 
-static const char prune_usage[] = "git-prune [-n]";
+static const char * const prune_usage[] = {
+	"git-prune [-n] [--expire <time>] [--] [<head>...]",
+	NULL
+};
 static int show_only;
 static unsigned long expire;
 
@@ -123,32 +127,22 @@ static void remove_temporary_files(void)
 
 int cmd_prune(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	struct rev_info revs;
-
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "-n")) {
-			show_only = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--expire")) {
-			if (++i < argc) {
-				expire = approxidate(argv[i]);
-				continue;
-			}
-		}
-		else if (!prefixcmp(arg, "--expire=")) {
-			expire = approxidate(arg + 9);
-			continue;
-		}
-		usage(prune_usage);
-	}
+	const struct option options[] = {
+		OPT_BOOLEAN('n', NULL, &show_only,
+			    "do not remove, show only"),
+		OPT_DATE(0, "expire", &expire,
+			 "expire objects older than <time>"),
+		OPT_END()
+	};
 
 	save_commit_buffer = 0;
 	init_revisions(&revs, prefix);
-	mark_reachable_objects(&revs, 1);
 
+	argc = parse_options(argc, argv, options, prune_usage, 0);
+	if (argc)
+		die ("unrecognized argument: %s", name);
+	mark_reachable_objects(&revs, 1);
 	prune_object_dir(get_object_directory());
 
 	sync();

-- 
1.5.5.rc1.121.g1594
Junio C Hamano· Mar 25, 2008, 06:59 UTC · re: Michele Ballabio · lore

[PATCH 5/5] builtin-prune: protect objects listed on the command line

From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 24 Mar 2008 23:20:51 -0700

Finally, this resurrects the documented behaviour to protect other objects listed on the command line from getting pruned.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This is done deliberately differently from what you did.  Because we do
   not want to accept "we allow losing what's reachable from master" with
   "git prune master..next", setup_revisions() is not the right thing to
   use for this command.
 builtin-prune.c  |   12 ++++++++++--
 t/t5304-prune.sh |    2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)
Show changes to 2 files +11 −3

builtin-prune.c, t/t5304-prune.sh

diff --git a/builtin-prune.c b/builtin-prune.c
index 71caac5..ca50cca 100644
--- a/builtin-prune.c
+++ b/builtin-prune.c
@@ -140,8 +140,16 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	init_revisions(&revs, prefix);
 
 	argc = parse_options(argc, argv, options, prune_usage, 0);
-	if (argc)
-		die ("unrecognized argument: %s", name);
+	while (argc--) {
+		struct object *object;
+		unsigned char sha1[20];
+		const char *name = *argv++;
+
+		if (!get_sha1(name, sha1) && (object = parse_object(sha1)))
+			add_pending_object(&revs, object, "");
+		else
+			die ("unrecognized argument: %s", name);
+	}
 	mark_reachable_objects(&revs, 1);
 	prune_object_dir(get_object_directory());
 
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 3d81e1f..9fd9d07 100644
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -100,7 +100,7 @@ test_expect_success 'prune: prune unreachable heads' '
 
 '
 
-test_expect_failure 'prune: do not prune heads listed as an argument' '
+test_expect_success 'prune: do not prune heads listed as an argument' '
 
 	: > file2 &&
 	git add file2 &&

-- 
1.5.5.rc1.121.g1594
Junio C Hamano· Mar 27, 2008, 16:32 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/5] builtin-prune: protect objects listed on the command line

Junio C Hamano <junio@pobox.com> writes:
Show 12 quoted lines
> From: Junio C Hamano <gitster@pobox.com>
> Date: Mon, 24 Mar 2008 23:20:51 -0700
>
> Finally, this resurrects the documented behaviour to protect other
> objects listed on the command line from getting pruned.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * This is done deliberately differently from what you did.  Because we do
>    not want to accept "we allow losing what's reachable from master" with
>    "git prune master..next", setup_revisions() is not the right thing to
>    use for this command.
Ping?
Johannes Schindelin· Mar 27, 2008, 16:35 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/5] builtin-prune: protect objects listed on the command line

Hi,
On Thu, 27 Mar 2008, Junio C Hamano wrote:
Show 15 quoted lines
> Junio C Hamano <junio@pobox.com> writes:
> 
> > From: Junio C Hamano <gitster@pobox.com>
> > Date: Mon, 24 Mar 2008 23:20:51 -0700
> >
> > Finally, this resurrects the documented behaviour to protect other 
> > objects listed on the command line from getting pruned.
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com> ---
> >  * This is done deliberately differently from what you did.  Because 
> >    we do not want to accept "we allow losing what's reachable from 
> >    master" with "git prune master..next", setup_revisions() is not the 
> >    right thing to use for this command.
> 
> Ping?

I did not see any problem with your implementation, but I thought Michele would look more deeply, as he obviously cares about the to-be-fixed behaviour.

Ciao, Dscho

Michele Ballabio· Mar 27, 2008, 21:11 UTC · re: Johannes Schindelin · lore

Re: [PATCH 5/5] builtin-prune: protect objects listed on the command line

On Thursday 27 March 2008, Johannes Schindelin wrote:
Show 19 quoted lines
> On Thu, 27 Mar 2008, Junio C Hamano wrote:
> 
> > Junio C Hamano <junio@pobox.com> writes:
> > 
> > > From: Junio C Hamano <gitster@pobox.com>
> > > Date: Mon, 24 Mar 2008 23:20:51 -0700
> > >
> > > Finally, this resurrects the documented behaviour to protect other 
> > > objects listed on the command line from getting pruned.
> > >
> > > Signed-off-by: Junio C Hamano <gitster@pobox.com> ---
> > >  * This is done deliberately differently from what you did.  Because 
> > >    we do not want to accept "we allow losing what's reachable from 
> > >    master" with "git prune master..next", setup_revisions() is not the 
> > >    right thing to use for this command.
> > 
> > Ping?
> 
> I did not see any problem with your implementation
Me neither, but, as a nitpick, wouldn't something like
		if (!get_sha1(name, sha1)) {
			object = parse_object(sha1);
			if (!object)
				die("bad object %s", name);
		} else
			die("unrecognized argument: %s", name);
be a bit more useful?

← back to recent threads