Re: PATCH 6/6] archive: convert,to parse_options() [was: [PATCH 5/6] archive: allow --exec and --remote without equal sign]

From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Jul 26, 2008, 07:09 UTC
Message-ID: <488ACDA4.4010404@lsrfire.ath.cx>
In-Reply-To: <7vmyk5pjsy.fsf@gitster.siamese.dyndns.org>

Junio C Hamano schrieb:

Show 8 quoted lines

>>  archive.c |  110 +++++++++++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 68 insertions(+), 42 deletions(-)
> 
> Hmph, somewhat dubious.
> 
> The real point of parse-options was to make the code smaller, easier to
> maintain and command line handling more consistent.  At least this patch
> seems to fail on the two out of three counts.

Well, if we hid away the compression level handling in a macro defined in parse-options.h, we could save sixteen lines of code. The patch makes the four modes of running archive more explicit, adding three usage lines. Three empty lines are added -- they don't really increase the code's size.

Handling --exec and --remote takes six lines; we didn't do that before at this place, but have to now, since we want them to show up in the usage. We have to handle --no-format and --no-prefix, which adds four lines.

So I don't think the bigger size make this patch dubious, but of course I'm biased. Disallowing --no-format (using a new OPT_STRING_NONEG?) and adding an OPT__COMPRESSION helper might be a good idea (reducing line count in archive.c by seventeen).

Having parse-options provide a way to make --exec and --remote appear in the usage but to reject them (OPT_UNKNOWN?) is a bit too strange, though.

> All of the other patches made obvious sense to me and are queued for -rc1
> but I'd like to backburner this one.

Fair enough.

René