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

From: Jeff King <peff@peff.net>
Date: Aug 6, 2018, 18:58 UTC
Message-ID: <20180806185853.GA18119@sigill.intra.peff.net>
In-Reply-To: <xmqqy3djnyik.fsf@gitster-ct.c.googlers.com>

On Mon, Aug 06, 2018 at 11:25:07AM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > +while test $# -gt 0
> > +do
> > +	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".

I think we need to squash this in:

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