Re: [PATCH] add a script to diff rendered documentation

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 6, 2018, 19:29 UTC
Message-ID: <xmqqmutznvjb.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180806185853.GA18119@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

>> > +	case "$1" in
>> > +	-j)
>> > +		parallel=${1#-j} ;;
>> 
>> This is curious.  Did you mean "-j*)" on the line above this one?
>
> Hmph, yes, I think this was broken even in the original. And after going
> through "rev-parse --parseopt", we should have a separate argument
> anyway, even for the "stuck" form. Worse, the OPTIONS_SPEC doesn't
> mention the argument, so it barfs on "-j4".

Ah, I forgot (just like somebody else, and even worse is that I reminded him of this fact that I am forgetting here) that we use the parseopt thing to normalize the option parsing, so with the correct spec "-j)" is the right thing to say (but yes then you'd look at $2 and then shift both aawy).

> I think we need to squash this in:

Yup.  Thanks.

Show 41 quoted lines

> diff --git a/Documentation/doc-diff b/Documentation/doc-diff
> index 5d5b243384..f483fe427c 100755
> --- a/Documentation/doc-diff
> +++ b/Documentation/doc-diff
> @@ -3,7 +3,7 @@
>  OPTIONS_SPEC="\
>  doc-diff [options] <from> <to> [-- <diff-options>]
>  --
> -j	parallel argument to pass to make
> +j=n	parallel argument to pass to make
>  f	force rebuild; do not rely on cached results
>  "
>  SUBDIRECTORY_OK=1
> @@ -15,7 +15,7 @@ while test $# -gt 0
>  do
>  	case "$1" in
>  	-j)
> -		parallel=${1#-j} ;;
> +		parallel=$2; shift ;;
>  	-f)
>  		force=t ;;
>  	--)
>
>> Then "script -j" (no explicit number) would get here and autodetect.
>> Running the script without any "-j" would also get an empty parallel
>> and come here.
>
> Yeah, I think that is the wrong thing. If anything "-j" should behave
> like "make -j". However, it looks like "rev-parse --parseopt" doesn't
> play well with optional arguments for short-only options. You get "-j",
> but then you have no idea whether the next argument is an optional value
> for it, or another option entirely. Arguably it should give a blank
> string or something (if you have long options, then it uses the
> long-stock form, which is fine).
>
>> So "script -j1" would be how a user would say "I want to use exactly
>> one process, not any parallelism", which makes sense.
>
> Right, that was the thing I actually wanted to have happen. :)
>
> -Peff