[RFC] git-fsck-cache argument processing

[RFC] git-fsck-cache argument processing

[Sean]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]


Here is a first crack at using argp as suggested by Jeff Garzik to
implement argument processing as requested by Junio and Linus.  Each of
the long arguments have been given a single character equivalent as well.

This patch only converts fsck-cache to use argp in case anyone has
objections to the basic format or style.  The patch includes a version
number inside of fsck-cache.c; this should really be in a separate include
file so you can run any command with --version and get the same answer.

With this change you have:

$ git-fsck-cache -?
Usage: git-fsck-cache [OPTION...] [HEAD-SHA1...]
git-fsck-cache - repository consistency check

 -c, --cache            Mark all objects referenced by cache as reachable
 -d, --delta-depth      Show the maximum length of delta chains
 -r, --root             Show root objects, ie. those without parents
 -t, --tags             Show revision tags
 -u, --unreachable      Show missing objects or deltas
 -?, --help             Give this help list
     --usage            Give a short usage message
 -V, --version          Print program version

And the following should work as expected:

$ git-fsck-cache -crudt


fsck-cache.c |   64
+++++++++++++++++++++++++++++++++++------------------------
1 files changed, 39 insertions(+), 25 deletions(-)

Sean

[-- Attachment #2: fsck-cache-argp-v1.patch --]
[-- Type: application/octet-stream, Size: 2375 bytes --]

fsck-cache.c: needs update
Index: fsck-cache.c
===================================================================
--- 58741c69570705801db4b785681790d636475695/fsck-cache.c  (mode:100644)
+++ uncommitted/fsck-cache.c  (mode:100644)
@@ -1,5 +1,7 @@
 #include <sys/types.h>
 #include <dirent.h>
+#include <argp.h>
+const char *argp_program_version = "git 1.0";
 
 #include "cache.h"
 #include "commit.h"
@@ -407,36 +409,48 @@
 	find_file_objects(git_dir, "refs");
 }
 
+#define O_UNREACH 'u'
+#define O_TAGS 't'
+#define O_ROOT 'r'
+#define O_DELTA 'd'
+#define O_CACHE 'c'
+
+static const char doc[] = "Perform repository consistency check";
+
+static struct argp_option options[] = {
+ {"unreachable", O_UNREACH, 0, 0, "Show missing objects or deltas"},
+ {"tags", O_TAGS, 0, 0, "Show revision tags"},
+ {"root", O_ROOT, 0, 0, "Show root objects, ie. those without parents"},
+ {"delta-depth", O_DELTA, 0, 0, "Show the maximum length of delta chains"},
+ {"cache", O_CACHE, 0, 0, "Mark all objects referenced by cache as reachable"},
+ { }
+};
+
+static error_t parse_opt (int key, char *arg, struct argp_state *state)
+{
+	switch (key) {
+	case O_UNREACH:		show_unreachable = 1; break;
+	case O_TAGS:		show_tags = 1; break;
+	case O_ROOT:		show_root = 1; break;
+	case O_DELTA:		show_max_delta_depth = 1; break;
+	case O_CACHE:		keep_cache_objects = 1; break;
+	case ARGP_KEY_ARG:	state->next = state->argc; break;
+	default:		return ARGP_ERR_UNKNOWN;
+	}
+	return 0;
+}
+
+static const struct argp argp = { options, parse_opt, "[HEAD-SHA1...]", doc };
+
 int main(int argc, char **argv)
 {
 	int i, heads;
 	char *sha1_dir;
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--unreachable")) {
-			show_unreachable = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--tags")) {
-			show_tags = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--root")) {
-			show_root = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--delta-depth")) {
-			show_max_delta_depth = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--cache")) {
-			keep_cache_objects = 1;
-			continue;
-		}
-		if (*arg == '-')
-			usage("git-fsck-cache [--tags] [[--unreachable] [--cache] <head-sha1>*]");
+	error_t rc = argp_parse(&argp, argc, argv, 0, NULL, NULL);
+	if (rc) {
+		fprintf(stderr, "argument failed: %s\n", strerror(rc));
+		return 1;
 	}
 
 	sha1_dir = get_object_directory();

Re: [RFC] git-fsck-cache argument processing

[Jeff Garzik]

Sean wrote:
> Here is a first crack at using argp as suggested by Jeff Garzik to
> implement argument processing as requested by Junio and Linus.  Each of
> the long arguments have been given a single character equivalent as well.
> 
> This patch only converts fsck-cache to use argp in case anyone has
> objections to the basic format or style.  The patch includes a version
> number inside of fsck-cache.c; this should really be in a separate include
> file so you can run any command with --version and get the same answer.

Pretty good.  You'll probably want some additional changes:


1) eliminate
+       case ARGP_KEY_ARG:      state->next = state->argc; break;

This will cause option processing to stop at the first unknown argument.


2) Pass-by-reference a variable to argp_parse(), which will store the 
index of the argument where processing stopped.  This is the first 
hash/file/etc. non-option argument.

(example code from posixutils)

int parse_cmdline(struct cmdline_walker *cw)
{
       error_t rc_argp;
       int idx = 0;

       rc_argp = argp_parse(cw->argp, cw->argc, cw->argv, 0, &idx, NULL);
       if (rc_argp) {
               fprintf(stderr, "argp_parse: %s\n", strerror(rc_argp));
               return -rc_argp;
       }

       return idx;
}

'idx' in this case is the first non-option argument, which can be passed 
directly to argv[].  From there, you perform standard iteration over the 
arguments provided on the command line, starting at argv[idx].

If you have a fixed number of arguments following the options, then your 
parse_opt function can easily parse those args as well:

static error_t parse_opt (int key, char *arg, struct argp_state *state)
{
         switch (key) {
         case '1':
                 outmask |= OPT_FILE1;
                 break;
         case '2':
                 outmask |= OPT_FILE2;
                 break;
         case '3':
                 outmask |= OPT_DUP;
                 break;

         case ARGP_KEY_ARG:
           switch(state->arg_num) {
           case 0:         file1 = arg; break; /* 1st non-opt arg */
           case 1:         file2 = arg; break; /* 2nd non-opt arg */
           default:        argp_usage (state); break; /* too many args */
           }
           break;

         case ARGP_KEY_END:
                 if (state->arg_num < 2)         /* not enough args */
                         argp_usage (state);
                 break;

         default:
                 return ARGP_ERR_UNKNOWN;
         }

         return 0;
}

Re: [RFC] git-fsck-cache argument processing

[Sean]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Sat, May 21, 2005 12:08 am, Jeff Garzik said:

> Pretty good.  You'll probably want some additional changes:
>
> 1) eliminate
> +       case ARGP_KEY_ARG:      state->next = state->argc; break;
>
> This will cause option processing to stop at the first unknown argument.
>
> 2) Pass-by-reference a variable to argp_parse(), which will store the
> index of the argument where processing stopped.  This is the first
> hash/file/etc. non-option argument.
>

Thanks Jeff, that's pretty cool.  Here's an updated patch.

With this updated patch, options following or even intermingled with the
SHA1 list also are picked up, for example:

$ git-fsck-cache 804c64ea864d0a8ee13f3de0b74158a3e9c3166d -crudt


fsck-cache.c |   66
+++++++++++++++++++++++++++++++++++------------------------
1 files changed, 40 insertions(+), 26 deletions(-)

Sean

[-- Attachment #2: fsck-cache-argp-v2.patch --]
[-- Type: application/octet-stream, Size: 2565 bytes --]

fsck-cache.c: needs update
Index: fsck-cache.c
===================================================================
--- 58741c69570705801db4b785681790d636475695/fsck-cache.c  (mode:100644)
+++ uncommitted/fsck-cache.c  (mode:100644)
@@ -1,5 +1,7 @@
 #include <sys/types.h>
 #include <dirent.h>
+#include <argp.h>
+const char *argp_program_version = "git 1.0";
 
 #include "cache.h"
 #include "commit.h"
@@ -407,36 +409,48 @@
 	find_file_objects(git_dir, "refs");
 }
 
+#define O_UNREACH 'u'
+#define O_TAGS 't'
+#define O_ROOT 'r'
+#define O_DELTA 'd'
+#define O_CACHE 'c'
+
+static const char doc[] = "Perform repository consistency check";
+
+static struct argp_option options[] = {
+ {"unreachable", O_UNREACH, 0, 0, "Show missing objects or deltas"},
+ {"tags", O_TAGS, 0, 0, "Show revision tags"},
+ {"root", O_ROOT, 0, 0, "Show root objects, ie. those without parents"},
+ {"delta-depth", O_DELTA, 0, 0, "Show the maximum length of delta chains"},
+ {"cache", O_CACHE, 0, 0, "Mark all objects referenced by cache as reachable"},
+ { }
+};
+
+static error_t parse_opt (int key, char *arg, struct argp_state *state)
+{
+        switch (key) {
+        case O_UNREACH:		show_unreachable = 1; break;
+        case O_TAGS:		show_tags = 1; break;
+        case O_ROOT:		show_root = 1; break;
+        case O_DELTA:		show_max_delta_depth = 1; break;
+        case O_CACHE:		keep_cache_objects = 1; break;
+        default:		return ARGP_ERR_UNKNOWN;
+        }
+        return 0;
+}
+
+static const struct argp argp = { options, parse_opt, "[HEAD-SHA1...]", doc };
+
 int main(int argc, char **argv)
 {
 	int i, heads;
 	char *sha1_dir;
+	int idx;
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--unreachable")) {
-			show_unreachable = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--tags")) {
-			show_tags = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--root")) {
-			show_root = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--delta-depth")) {
-			show_max_delta_depth = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--cache")) {
-			keep_cache_objects = 1;
-			continue;
-		}
-		if (*arg == '-')
-			usage("git-fsck-cache [--tags] [[--unreachable] [--cache] <head-sha1>*]");
+	error_t rc = argp_parse(&argp, argc, argv, 0, &idx, NULL);
+	if (rc) {
+		fprintf(stderr, "argument failed: %s\n", strerror(rc));
+		return 1;
 	}
 
 	sha1_dir = get_object_directory();
@@ -450,7 +464,7 @@
 	expand_deltas();
 
 	heads = 0;
-	for (i = 1; i < argc; i++) {
+	for (i = idx; i < argc; i++) {
 		const char *arg = argv[i]; 
 
 		if (*arg == '-')

Re: [RFC] git-fsck-cache argument processing

[Junio C Hamano]

The patch looks good.  Before you proceed to convert the rest,
could I ask you to first let us see the list of new set of
options and semantics changes, if any ("checkout-cache -f -a" vs
"checkout-cache -a -f" immediately comes to mind)?

Presumably you would be doing the Documentation updates as well,
so starting from the documentaiton updates before writing the
actual code may be a good way for us to understand and ack on
what is going to happen.

Re: [RFC] git-fsck-cache argument processing

[Jeff Garzik]

Patch looks good to me...

Re: [RFC] git-fsck-cache argument processing

[Jeff Garzik]

On Fri, May 20, 2005 at 10:08:19PM -0700, Junio C Hamano wrote:
> The patch looks good.  Before you proceed to convert the rest,
> could I ask you to first let us see the list of new set of
> options and semantics changes, if any ("checkout-cache -f -a" vs
> "checkout-cache -a -f" immediately comes to mind)?

Typical implementation is agnostic on the ordering of options,
but with a few lines of code in parse_opt() that need not always be the
case.

FWIW most users (including me!) would expect that order of options is
-not- significant.

	Jeff

Re: [RFC] git-fsck-cache argument processing

[Junio C Hamano]

>>>>> "JG" == Jeff Garzik <jgarzik@pobox.com> writes:

JG> On Fri, May 20, 2005 at 10:08:19PM -0700, Junio C Hamano wrote:

>> The patch looks good.  Before you proceed to convert the rest,
>> could I ask you to first let us see the list of new set of
>> options and semantics changes, if any ("checkout-cache -f -a" vs
>> "checkout-cache -a -f" immediately comes to mind)?

JG> FWIW most users (including me!) would expect that order of
JG> options is -not- significant.

That set of users includes me.  I was hoping that this round
would not just change things to use argp, but at the same time
attempt to "fix" the problems around argument parsing.  

JG> Typical implementation is agnostic on the ordering of
JG> options, but with a few lines of code in parse_opt() that
JG> need not always be the case.

I think you are responding to my "semantic changes" question,
but I did not mean to say that exactly emulating the current
behaviour is the requirement, nor I meant to ask if doing so is
impossible using argp().  I just wanted to see "the set of
planned fixes" while we are at it.

Re: [RFC] git-fsck-cache argument processing

[Olivier Galibert]

On Fri, May 20, 2005 at 11:38:52PM -0400, Sean wrote:
>  -?, --help             Give this help list

Could you make that '-h' please ?


>  -V, --version          Print program version

And that '-v'.  -V traditionally means verbose, -v version.  Yes, I
know there are counter-examples, but statistically...

  OG.

Re: [RFC] git-fsck-cache argument processing

[Jeff Garzik]

Olivier Galibert wrote:
> And that '-v'.  -V traditionally means verbose, -v version.  Yes, I
> know there are counter-examples, but statistically...

In my experience, lowercase 'v' means verbose :)

	Jeff

Re: [RFC] git-fsck-cache argument processing

[Sean]

On Sat, May 21, 2005 11:09 am, Olivier Galibert said:
> On Fri, May 20, 2005 at 11:38:52PM -0400, Sean wrote:
>>  -?, --help             Give this help list
>
> Could you make that '-h' please ?
>
>>  -V, --version          Print program version
>
> And that '-v'.  -V traditionally means verbose, -v version.  Yes, I
> know there are counter-examples, but statistically...
>

Hey Olivier,

Both of these options are generated automatically by argp.  I'm sure there
is a way to override them, but i'd rather just leave them as given by
argp.  For the first case, if you try '-h' on the command line you get:

$ git-fsck-cache -h
git-fsck-cache: invalid option -- h
Try `git-fsck-cache --help' or `git-fsck-cache --usage' for more information.

So it leads to the proper help message.

Sean

Re: [RFC] git-fsck-cache argument processing

[Olivier Galibert]

On Sat, May 21, 2005 at 01:22:30PM -0400, Sean wrote:
> Both of these options are generated automatically by argp.

I see.  Then I'll guess I'll have to put argp in the "crap" pile next
to libtool and automake.


> I'm sure there
> is a way to override them, but i'd rather just leave them as given by
> argp.  For the first case, if you try '-h' on the command line you get:
> 
> $ git-fsck-cache -h
> git-fsck-cache: invalid option -- h
> Try `git-fsck-cache --help' or `git-fsck-cache --usage' for more information.
> 
> So it leads to the proper help message.

How neat.  Of course using the right option under (t)csh gives:

galibert@m62:~ #201 >git-fsck-cache -?
git-fsck-cache: No match.

Importing this windowism is beyond stupid.

  OG.

Re: [RFC] git-fsck-cache argument processing

[Sean]

On Sat, May 21, 2005 2:49 pm, Olivier Galibert said:
> On Sat, May 21, 2005 at 01:22:30PM -0400, Sean wrote:
>> Both of these options are generated automatically by argp.
>
> I see.  Then I'll guess I'll have to put argp in the "crap" pile next
> to libtool and automake.

Okay now don't go saying things you can't take back <g>.

> How neat.  Of course using the right option under (t)csh gives:
>
> galibert@m62:~ #201 >git-fsck-cache -?
> git-fsck-cache: No match.
>
> Importing this windowism is beyond stupid.

Hmmm, that kinda sucks, but the --help should still work fine.  I suppose
we could figure out how to use -h instead.

Sean

Re: [RFC] git-fsck-cache argument processing

[Linus Torvalds]

On Fri, 20 May 2005, Sean wrote:
>
>  -u, --unreachable      Show missing objects or deltas

That's the wrong description.

fsck always shows missing objects, but "--unreachable" makes it also show
objects that cannot be reached from any of the references (either passed
in on the command line, or the implicit references we take if no explicit
reference is given).

So in many ways, "--unreachable" is about the _reverse_ of showing missing 
objects: it's about showing _extraneous_ objects that aren't needed by the 
ref that was passed in.

		Linus

Re: [RFC] git-fsck-cache argument processing

[Linus Torvalds]

Oh, and please fix your mailer. Your attachments show up for me as 
unreadable:

    [ Part 2, Application/OCTET-STREAM (Name: "fsck-cache-argp-v1.patch")  3.2KB. ]
    [ Cannot display this part. Press "V" then "S" to save in a file. ]

ie I'd have to save them to a file to look at them, which is just silly. 
So either just add patches in-line (and make sure your mailer doesn't crap 
all over whitespace), or add them as text attachments so that people see 
the things in their mail readers.

		Linus

Re: [RFC] git-fsck-cache argument processing

[Sean]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Sat, May 21, 2005 3:47 pm, Linus Torvalds said:
> On Fri, 20 May 2005, Sean wrote:
>>
>>  -u, --unreachable      Show missing objects or deltas
>
> That's the wrong description.
>
> fsck always shows missing objects, but "--unreachable" makes it also show
> objects that cannot be reached from any of the references (either passed
> in on the command line, or the implicit references we take if no explicit
> reference is given).
>
> So in many ways, "--unreachable" is about the _reverse_ of showing missing
> objects: it's about showing _extraneous_ objects that aren't needed by the
> ref that was passed in.

Right, fixed up in attached patch which basically punts with a description
of   "Show unreachable objects", and leaves it to the documentation to do
better.

Sean

[-- Attachment #2: argp-fsck-cache-v4.patch --]
[-- Type: plain/text, Size: 3256 bytes --]

Re: [RFC] git-fsck-cache argument processing

[Sean]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

On Sat, May 21, 2005 4:46 pm, Sean said:

> Right, fixed up in attached patch which basically punts with a
> description of   "Show unreachable objects", and leaves it to the
> documentation to do better.
Linus,

Try 2 at that attachment thingy, hopefully this one will make your mailer
happy.

Sean


[-- Attachment #2: argp-fsck-cache-v4.patch --]
[-- Type: text/plain, Size: 3255 bytes --]

Index: Documentation/git-fsck-cache.txt
===================================================================
--- 85ec256656a7839ca7a2067792b828c9b63c8711/Documentation/git-fsck-cache.txt  (mode:100644)
+++ uncommitted/Documentation/git-fsck-cache.txt  (mode:100644)
@@ -9,7 +9,8 @@
 
 SYNOPSIS
 --------
-'git-fsck-cache' [--tags] [--root] [[--unreachable] [--cache] <object>\*]
+'git-fsck-cache' [--tags] [--root] [--delta-depth] 
+			[[--unreachable] [--cache] <object>\*]
 
 DESCRIPTION
 -----------
@@ -34,6 +35,9 @@
 	Consider any object recorded in the cache also as a head node for
 	an unreachability trace.
 
+--delta-depth::
+	Show length of longest delta chain.
+
 It tests SHA1 and general object sanity, and it does full tracking of
 the resulting reachability and everything else. It prints out any
 corruption it finds (missing or bad objects), and if you use the
Index: fsck-cache.c
===================================================================
--- 85ec256656a7839ca7a2067792b828c9b63c8711/fsck-cache.c  (mode:100644)
+++ uncommitted/fsck-cache.c  (mode:100644)
@@ -1,5 +1,7 @@
 #include <sys/types.h>
 #include <dirent.h>
+#include <argp.h>
+const char *argp_program_version = VERSION;
 
 #include "cache.h"
 #include "commit.h"
@@ -407,36 +409,42 @@
 	find_file_objects(git_dir, "refs");
 }
 
+static const char doc[] = "Perform repository consistency check";
+
+static struct argp_option options[] = {
+ {"unreachable", 'u', 0, 0, "Show unreachable objects"},
+ {"tags", 't', 0, 0, "Show revision tags"},
+ {"root", 'r', 0, 0, "Show root objects, ie. those without parents"},
+ {"delta-depth", 'd', 0, 0, "Show length of longest delta chain"},
+ {"cache", 'c', 0, 0, "Mark all objects referenced by cache as reachable"},
+ { }
+};
+
+static error_t parse_opt (int key, char *arg, struct argp_state *state)
+{
+        switch (key) {
+        case 'u':		show_unreachable = 1; break;
+        case 't':		show_tags = 1; break;
+        case 'r':		show_root = 1; break;
+        case 'd':		show_max_delta_depth = 1; break;
+        case 'c':		keep_cache_objects = 1; break;
+        default:		return ARGP_ERR_UNKNOWN;
+        }
+        return 0;
+}
+
+static const struct argp argp = { options, parse_opt, "[HEAD-SHA1...]", doc };
+
 int main(int argc, char **argv)
 {
 	int i, heads;
 	char *sha1_dir;
+	int idx;
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--unreachable")) {
-			show_unreachable = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--tags")) {
-			show_tags = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--root")) {
-			show_root = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--delta-depth")) {
-			show_max_delta_depth = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--cache")) {
-			keep_cache_objects = 1;
-			continue;
-		}
-		if (*arg == '-')
-			usage("git-fsck-cache [--tags] [[--unreachable] [--cache] <head-sha1>*]");
+	error_t rc = argp_parse(&argp, argc, argv, 0, &idx, NULL);
+	if (rc) {
+		fprintf(stderr, "argument failed: %s\n", strerror(rc));
+		return 1;
 	}
 
 	sha1_dir = get_object_directory();
@@ -450,7 +458,7 @@
 	expand_deltas();
 
 	heads = 0;
-	for (i = 1; i < argc; i++) {
+	for (i = idx; i < argc; i++) {
 		const char *arg = argv[i]; 
 
 		if (*arg == '-')

Re: [RFC] git-fsck-cache argument processing

[Joel Becker]

On Sat, May 21, 2005 at 05:09:26PM +0200, Olivier Galibert wrote:
> On Fri, May 20, 2005 at 11:38:52PM -0400, Sean wrote:
> >  -?, --help             Give this help list
> 
> Could you make that '-h' please ?

	I generally think supporting -? and -h is the usual thing.  SysV
loves -?, most real folks like -h.

> >  -V, --version          Print program version
> 
> And that '-v'.  -V traditionally means verbose, -v version.  Yes, I
> know there are counter-examples, but statistically...

	I've never seen -V mean verbose.  I just queried all of
coreutils, and as most folks expect, -v means verbose.  Statistically,
I'd be interested in seeing statistics.  I've never used a program where
-V meant verbose.  I've never used a program that had '--verbose' where
'-v' didn't mean verbose.  tar(1), rsync(1), chgrp(1), chown(1), cp(1),
mkdir(1), all use '-v' for verbose.

Joel

-- 

"The nearest approach to immortality on Earth is a government
 bureau."
	- James F. Byrnes

Joel Becker
Senior Member of Technical Staff
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [RFC] git-fsck-cache argument processing

[Jeff Garzik]

Olivier Galibert wrote:
> On Sat, May 21, 2005 at 01:22:30PM -0400, Sean wrote:
> 
>>Both of these options are generated automatically by argp.
> 
> 
> I see.  Then I'll guess I'll have to put argp in the "crap" pile next
> to libtool and automake.

It's called "consistency with most other Linux apps."

Consistency and predictability are good things.

	Jeff