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

From: Antoine Queru <antoine.queru@ensimag.grenoble-inp.fr>
Date: May 31, 2016, 09:53 UTC
Message-ID: <766819198.192241.1464688434974.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To: <xmqqd1o38i0m.fsf@gitster.mtv.corp.google.com>

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

> > From: Antoine Queru <Antoine.Queru@ensimag.grenoble-inp.fr>
>
> 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.

Show 11 quoted lines

> > +--[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.

Show 43 quoted lines

> > 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.