Re: [PATCH v5] upload-pack.c: use parse-options API

Antoine Queru [off-list ref] writes:

From: Antoine Queru <redacted>

Don't you want to be known to the project as the email that matches
your Signed-off-by: line?

Use the parse-options API rather than a hand-rolled option parser.

Description for --stateless-rpc and --advertise-refs come from
42526b4 (Add stateless RPC options to upload-pack,
receive-pack, 2009-10-30).

Signed-off-by: Antoine Queru <redacted>
Signed-off-by: Matthieu Moy <redacted>
---
---strict::
+--[no-]strict::
 	Do not try <directory>/.git/ if <directory> is no Git directory.

Not a new problem, but "is no Git ..." may technically be correct,
but it would be easier to read if phrased "is not a Git ..." or
something like that.

	I am NOT asking _you_ to change/fix that in this patch.  It
	is just a note for "a low hanging fruit" for people to pick
	up with a separate patch.

+--stateless-rpc::
+	Perform only a single read-write cycle with stdin and stdout.
+	This fits with the HTTP POST request processing model where
+	a program may read the request, write a response, and must exit.
+
+--advertise-refs::
+	Only the initial ref advertisement is output, and the program exits
+    	immediately. This fits with the HTTP GET request model, where
+    	no request content is received but a response must be produced.
+

Good.

Output from "git grep -e '\[option' -e '\[<option' -- \*.c" tells me
that "[<options>]" would be more in line with the established
convention than "[options]".

	I personally wish if all these hits from the above grep
	consistently spelled it as "[options]", because there is not
	much gained by enclosing the word in <> to highlight it as a
	placeholder.  An argument that "it is done for uniformity
	with descriptions for other non option arguments, i.e. not
	to special case 'options'" would not hold water, e.g. in

        builtin/merge.c:        N_("git merge [<options>] [<commit>...]"),

	<options> is still special-cased in that it implies multiple
	things, unlike "<commit>..." notation that has to explicitly
	say that can have multiple.  [<options>...] would have been
	justifiable with the "make it uniform with non-option args",
	though.

	BUT this patch is not about "make usage string better"
	patch, so I do NOT want you to switch upload-pack's usage
	string to use the "options is special anyway, so let's not
	waste two display columns with enclosing <>" convention.

So, in conclusion, "git upload-pack [<options>] <dir>".

The remainder of the patch looked OK to me.

Thanks.

Hello Junio, thanks for your answer. The next version (and hopefully the last)
will come fast.

quoted

From: Antoine Queru <redacted>

Don't you want to be known to the project as the email that matches
your Signed-off-by: line?

Thanks for noticing it, it will be updated.
  

quoted

+--[no-]strict::
 	Do not try <directory>/.git/ if <directory> is no Git directory.

Not a new problem, but "is no Git ..." may technically be correct,
but it would be easier to read if phrased "is not a Git ..." or
something like that.

	I am NOT asking _you_ to change/fix that in this patch.  It
	is just a note for "a low hanging fruit" for people to pick
	up with a separate patch.

Ok, so this will not be changed.
 

quoted

diff --git a/upload-pack.c b/upload-pack.c
index dc802a0..083d068 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -14,8 +14,12 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "parse-options.h"
 
-static const char upload_pack_usage[] = "git upload-pack [--strict]

[--timeout=<n>] <dir>";
+static const char * const upload_pack_usage[] = {
+	N_("git upload-pack [options] <dir>"),
+	NULL
+};

Output from "git grep -e '\[option' -e '\[<option' -- \*.c" tells me
that "[<options>]" would be more in line with the established
convention than "[options]".

	I personally wish if all these hits from the above grep
	consistently spelled it as "[options]", because there is not
	much gained by enclosing the word in <> to highlight it as a
	placeholder.  An argument that "it is done for uniformity
	with descriptions for other non option arguments, i.e. not
	to special case 'options'" would not hold water, e.g. in

        builtin/merge.c:        N_("git merge [<options>] [<commit>...]"),

	<options> is still special-cased in that it implies multiple
	things, unlike "<commit>..." notation that has to explicitly
	say that can have multiple.  [<options>...] would have been
	justifiable with the "make it uniform with non-option args",
	though.

	BUT this patch is not about "make usage string better"
	patch, so I do NOT want you to switch upload-pack's usage
	string to use the "options is special anyway, so let's not
	waste two display columns with enclosing <>" convention.

So, in conclusion, "git upload-pack [<options>] <dir>".

Ok, so it will be fixed. I didn't notice this convention, my goal 
wasn't to change it, my bad.