Re: [PATCH] upload-pack.c: use of parse-options API
Jeff King wrote: > On Thu, May 19, 2016 at 12:10:31PM +0200, Antoine Queru wrote: > > > > I'm not sure whether it is worth hiding the first two options. We > > > typically hide "internal" options like this for user-facing programs, so > > > as not to clutter the "-h" output. But upload-pack isn't a user-facing > > > program. Anybody who is calling it directly with "-h" may be interested > > > in even its more esoteric options. > > > > In fact, to do this, I looked at builtin/receive-pack.c, where the parser > > API > > was already implemented, and these first two options were hidden. There > > were > > also no description for any options, so I thought it was not needed. Maybe > > we > > could update this file too ? > > Yeah, I don't think it's that bad to hide them, and perhaps consistency > with receive-pack is better. IIRC, part of the reason receive-pack has hidden options is that it was a GSoC microproject, and writing an accurate description is much harder than what we expect from a microproject. IOW, I'm all for un-hiding these options, but that shouldn't be a requirement for a beginner's project. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] upload-pack.c: use of parse-options API
On Thu, May 19, 2016 at 12:10:31PM +0200, Antoine Queru wrote: > > I'm not sure whether it is worth hiding the first two options. We > > typically hide "internal" options like this for user-facing programs, so > > as not to clutter the "-h" output. But upload-pack isn't a user-facing > > program. Anybody who is calling it directly with "-h" may be interested > > in even its more esoteric options. > > In fact, to do this, I looked at builtin/receive-pack.c, where the parser API > was already implemented, and these first two options were hidden. There were > also no description for any options, so I thought it was not needed. Maybe we > could update this file too ? Yeah, I don't think it's that bad to hide them, and perhaps consistency with receive-pack is better. AFAICT, descriptions for hidden options are pointless; they are never shown (in fact, it seems like OPT_HIDDEN_BOOL probably shouldn't even take such an option?). But the non-hidden options _do_ need non-NULL descriptions. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] upload-pack.c: use of parse-options API
Thanks for your input.
> > -static const char upload_pack_usage[] = "git upload-pack [--strict]
> > [--timeout=] ";
> > +static const char * const upload_pack_usage[] = {
> > + N_("git upload-pack [--strict] [--timeout=] "),
> > + NULL
> > +};
>
> Do we need to enumerate the options here now? The usage message should
> list the options from "struct options", which make these redundant.
>
> Something like:
>
> git -upload-pack [options]
>
> probably makes more sense.
>
Yes, you are right.
> Of course, it's hard to read the usage message because...
>
> > + struct option options[] = {
> > + OPT_HIDDEN_BOOL(0, "stateless-rpc", _rpc, NULL),
> > + OPT_HIDDEN_BOOL(0, "advertise-refs", _refs, NULL),
> > + OPT_BOOL(0, "strict", , NULL),
> > + OPT_INTEGER(0, "timeout", , NULL),
> > + OPT_END()
> > + };
>
> You've left the description text for each of these options as NULL, so
> running "git-upload-pack -h" segfaults.
>
> I'm not sure whether it is worth hiding the first two options. We
> typically hide "internal" options like this for user-facing programs, so
> as not to clutter the "-h" output. But upload-pack isn't a user-facing
> program. Anybody who is calling it directly with "-h" may be interested
> in even its more esoteric options.
>
In fact, to do this, I looked at builtin/receive-pack.c, where the parser API
was already implemented, and these first two options were hidden. There were
also no description for any options, so I thought it was not needed. Maybe we
could update this file too ?
> As a style nit, we usually spell comparison-with-zero as just:
>
> if (timeout)
> daemon_mode = 1;
Because timeout is an int, I personnally think it is more understable to
treat it as it. But I'll update itfor consistency.
> > + argc = parse_options(argc, (const char **)argv, NULL, options,
> > upload_pack_usage, 0);
>
> Perhaps this is a good opportunity to use "const" in the declaration of
> main(), as most other git programs do. Then you can drop this cast.
>
Ok.
> > setup_path();
> >
> > - dir = argv[i];
> > + dir = argv[0];
> >
> > if (!enter_repo(dir, strict))
> > die("'%s' does not appear to be a git repository", dir);
>
> Prior to your patch, we used to check:
>
> - if (i != argc-1)
> - usage(upload_pack_usage);
>
> which ensured that "dir" was non-NULL. But with your patch, we may pass
> NULL to enter_repo. It fortunately catches this, but then we pass the
> NULL to die, which can segfault (though on glibc systems, stdio is kind
> enough to replace it with the "(null)").
>
> Related, we silently accept extra arguments after your patch (whereas
> before we showed the usage message). You probably want to check "argc ==
> 1", and otherwise show the usage message.
Ok.
-Antoine
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] upload-pack.c: use of parse-options API
On Wed, May 18, 2016 at 06:40:19PM +0200, Antoine Queru wrote:
> Option parsing now uses the parser API instead of a local parser.
> Code is now more compact.
Sounds like a good goal.
> -static const char upload_pack_usage[] = "git upload-pack [--strict]
> [--timeout=] ";
> +static const char * const upload_pack_usage[] = {
> + N_("git upload-pack [--strict] [--timeout=] "),
> + NULL
> +};
Do we need to enumerate the options here now? The usage message should
list the options from "struct options", which make these redundant.
Something like:
git -upload-pack [options]
probably makes more sense.
Of course, it's hard to read the usage message because...
> + struct option options[] = {
> + OPT_HIDDEN_BOOL(0, "stateless-rpc", _rpc, NULL),
> + OPT_HIDDEN_BOOL(0, "advertise-refs", _refs, NULL),
> + OPT_BOOL(0, "strict", , NULL),
> + OPT_INTEGER(0, "timeout", , NULL),
> + OPT_END()
> + };
You've left the description text for each of these options as NULL, so
running "git-upload-pack -h" segfaults.
I'm not sure whether it is worth hiding the first two options. We
typically hide "internal" options like this for user-facing programs, so
as not to clutter the "-h" output. But upload-pack isn't a user-facing
program. Anybody who is calling it directly with "-h" may be interested
in even its more esoteric options.
> - for (i = 1; i < argc; i++) {
> - char *arg = argv[i];
> -
> - if (arg[0] != '-')
> - break;
> - if (!strcmp(arg, "--advertise-refs")) {
> - advertise_refs = 1;
> - continue;
> - }
> - if (!strcmp(arg, "--stateless-rpc")) {
> - stateless_rpc = 1;
> - continue;
> - }
> - if (!strcmp(arg, "--strict")) {
> - strict = 1;
> - continue;
> - }
> - if (starts_with(arg, "--timeout=")) {
> - timeout = atoi(arg+10);
> - daemon_mode = 1;
> - continue;
> - }
> - if (!strcmp(arg, "--")) {
> - i++;
> - break;
> - }
> - }
A common pitfall in converting a for-loop to parse-options is when there
is anything stateful in the parsing loop. But I don't see anything here.
The trickiest thing is that --timeout implies daemon_mode. You've
handled that below as:
> + if (timeout != 0)
> + daemon_mode = 1;
I think that is OK. It would not be correct if, for example, some other
code set "timeout" to a non-zero value (e.g., a config option), but I
don't see that here.
As a style nit, we usually spell comparison-with-zero as just:
if (timeout)
daemon_mode = 1;
Looking at at daemon_mode, though, it appears that it cannot be set in
any other way except here. And it does nothing except to disable
progress in some cases. Weird. There may be some opportunities for
refactoring or simplification there, but I that is outside the scope of
your patch.
> + argc = parse_options(argc, (const char **)argv, NULL, options,
> upload_pack_usage, 0);
Perhaps this is a good opportunity to use "const" in the declaration of
main(), as most other git programs do. Then you can drop this cast.
> setup_path();
>
> - dir = argv[i];
> + dir = argv[0];
>
> if (!enter_repo(dir, strict))
> die("'%s' does not appear to be a git repository", dir);
Prior to your patch, we used to check:
- if (i != argc-1)
- usage(upload_pack_usage);
which ensured that "dir" was non-NULL. But with your patch, we may pass
NULL to enter_repo. It fortunately catches this, but then we pass the
NULL to die, which can segfault (though on glibc systems, stdio is kind
enough to replace it with the "(null)").
Related, we silently accept extra arguments after your patch (whereas
before we showed the usage message). You probably want to check "argc ==
1", and otherwise show the usage message.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
