[PATCH] add a script to diff rendered documentation

[PATCH] add a script to diff rendered documentation

[Jeff King]

After making a change to the documentation, it's easy to
forget to check the rendered version to make sure it was
formatted as you intended. And simply doing a diff between
the two built versions is less trivial than you might hope:

  - diffing the roff or html output isn't particularly
    readable; what we really care about is what the end user
    will see

  - you have to tweak a few build variables to avoid
    spurious differences (e.g., version numbers, build
    times)

Let's provide a script that builds and installs the manpages
for two commits, renders the results using "man", and diffs
the result. Since this is time-consuming, we'll also do our
best to avoid repeated work, keeping intermediate results
between runs.

Some of this could probably be made a little less ugly if we
built support into Documentation/Makefile. But by relying
only on "make install-man" working, this script should work
for generating a diff between any two versions, whether they
include this script or not.

Signed-off-by: Jeff King <peff@peff.net>
---
I wrote this up for my own use after our discussion in [1]. I'm not sure
if it's too ugly for inclusion, or if it might be helpful to others.
I've only just written it, but my plan is to try to run it on anything I
submit to check the formatting. So it _seems_ useful and appears to
work, but only after a few minutes of playing with it. :)

[1] https://public-inbox.org/git/20180720223608.GE18502@genre.crustytoothpaste.net/

 Documentation/.gitignore |   1 +
 Documentation/doc-diff   | 100 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)
 create mode 100755 Documentation/doc-diff

diff --git a/Documentation/.gitignore b/Documentation/.gitignore
index c7096f11f1..3ef54e0adb 100644
--- a/Documentation/.gitignore
+++ b/Documentation/.gitignore
@@ -12,3 +12,4 @@ cmds-*.txt
 mergetools-*.txt
 manpage-base-url.xsl
 SubmittingPatches.txt
+tmp-doc-diff/
diff --git a/Documentation/doc-diff b/Documentation/doc-diff
new file mode 100755
index 0000000000..61deb2579e
--- /dev/null
+++ b/Documentation/doc-diff
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+OPTIONS_SPEC="\
+doc-diff <from> <to> [-- diff options]
+--
+j	parallel argument to pass to make
+f	force rebuild; do not rely on cached results
+"
+SUBDIRECTORY_OK=1
+. "$(git --exec-path)/git-sh-setup"
+
+parallel=8
+force=
+while test $# -gt 0
+do
+	case "$1" in
+	-j)
+		parallel=${1#-j} ;;
+	-f)
+		force=t ;;
+	--)
+		shift; break ;;
+	*)
+		usage ;;
+	esac
+	shift
+done
+
+test $# -gt 1 || usage
+from=$1; shift
+to=$1; shift
+
+from_oid=$(git rev-parse --verify "$from") || exit 1
+to_oid=$(git rev-parse --verify "$to") || exit 1
+
+cd_to_toplevel
+tmp=Documentation/tmp-doc-diff
+
+if test -n "$force"
+then
+	rm -rf "$tmp"
+fi
+
+# We'll do both builds in a single worktree, which lets make reuse
+# results that don't differ between the two trees.
+if ! test -d "$tmp/worktree"
+then
+	git worktree add --detach "$tmp/worktree" "$from" &&
+	dots=$(echo "$tmp/worktree" | sed 's#[^/]*#..#g') &&
+	ln -s "$dots/config.mak" "$tmp/worktree/config.mak"
+fi
+
+# generate_render_makefile <srcdir> <dstdir>
+generate_render_makefile () {
+	find "$1" -type f |
+	while read src
+	do
+		dst=$2/${src#$1/}
+		printf 'all:: %s\n' "$dst"
+		printf '%s: %s\n' "$dst" "$src"
+		printf '\t@echo >&2 "  RENDER $(notdir $@)" && \\\n'
+		printf '\tmkdir -p $(dir $@) && \\\n'
+		printf '\tMANWIDTH=80 man -l $< >$@+ && \\\n'
+		printf '\tmv $@+ $@\n'
+	done
+}
+
+# render_tree <dirname> <committish>
+render_tree () {
+	# Skip install-man entirely if we already have an installed directory.
+	# We can't rely on make here, since "install-man" unconditionally
+	# copies the files (spending effort, but also updating timestamps that
+	# we then can't rely on during the render step). We use "mv" to make
+	# sure we don't get confused by a previous run that failed partway
+	# through.
+	if ! test -d "$tmp/installed/$1"
+	then
+		git -C "$tmp/worktree" checkout "$2" &&
+		make -j$parallel -C "$tmp/worktree" \
+			GIT_VERSION=omitted \
+			SOURCE_DATE_EPOCH=0 \
+			DESTDIR="$PWD/$tmp/installed/$1+" \
+			install-man &&
+		mv "$tmp/installed/$1+" "$tmp/installed/$1"
+	fi &&
+
+	# As with "installed" above, we skip the render if it's already been
+	# done.  So using make here is primarily just about running in
+	# parallel.
+	if ! test -d "$tmp/rendered/$1"
+	then
+		generate_render_makefile "$tmp/installed/$1" "$tmp/rendered/$1+" |
+		make -j$parallel -f - &&
+		mv "$tmp/rendered/$1+" "$tmp/rendered/$1"
+	fi
+}
+
+render_tree $from_oid "$from" &&
+render_tree $to_oid "$to" &&
+git -C $tmp/rendered diff --no-index "$@" $from_oid $to_oid
-- 
2.18.0.912.g3ccaa4d859

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

[Eric Sunshine]

On Fri, Aug 3, 2018 at 4:52 PM Jeff King <peff@peff.net> wrote:
> [...]
> Let's provide a script that builds and installs the manpages
> for two commits, renders the results using "man", and diffs
> the result. Since this is time-consuming, we'll also do our
> best to avoid repeated work, keeping intermediate results
> between runs.
> [...]
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/Documentation/doc-diff b/Documentation/doc-diff
> @@ -0,0 +1,100 @@
> +OPTIONS_SPEC="\
> +doc-diff <from> <to> [-- diff options]

Should this be?

    doc-diff [<options>] <from> <to> [-- <diff-options>]

> +--
> +j      parallel argument to pass to make
> +f      force rebuild; do not rely on cached results
> +"

Should "j" and "f" be "-j" and "-f", respectively?

> +while test $# -gt 0
> +do
> +       case "$1" in
> +       -j)
> +               parallel=${1#-j} ;;
> +       -f)
> +               force=t ;;
> +       --)
> +               shift; break ;;
> +       *)
> +               usage ;;

There doesn't seem to a usage() function defined anywhere (and
OPTIONS_SPEC doesn't seem to be used).

> +       esac
> +       shift
> +done
> +# We'll do both builds in a single worktree, which lets make reuse
> +# results that don't differ between the two trees.

"which lets make reuse"?

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

[Jeff King]

On Fri, Aug 03, 2018 at 05:33:17PM -0400, Eric Sunshine wrote:

> > +OPTIONS_SPEC="\
> > +doc-diff <from> <to> [-- diff options]
> 
> Should this be?
> 
>     doc-diff [<options>] <from> <to> [-- <diff-options>]

I suppose so. Frankly I only added that line to appease git-sh-options
anyway.

> > +--
> > +j      parallel argument to pass to make
> > +f      force rebuild; do not rely on cached results
> > +"
> 
> Should "j" and "f" be "-j" and "-f", respectively?

No, they're input to "rev-parse --parseopt".

> > +while test $# -gt 0
> > +do
> > +       case "$1" in
> > +       -j)
> > +               parallel=${1#-j} ;;
> > +       -f)
> > +               force=t ;;
> > +       --)
> > +               shift; break ;;
> > +       *)
> > +               usage ;;
> 
> There doesn't seem to a usage() function defined anywhere (and
> OPTIONS_SPEC doesn't seem to be used).

It's git-sh-setup automagic. Try "./doc-diff --foo"

> > +# We'll do both builds in a single worktree, which lets make reuse
> > +# results that don't differ between the two trees.
> 
> "which lets make reuse"?

As in, lets the tool "make" reuse results...

-Peff

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

[Eric Sunshine]

On Fri, Aug 3, 2018 at 5:38 PM Jeff King <peff@peff.net> wrote:
> On Fri, Aug 03, 2018 at 05:33:17PM -0400, Eric Sunshine wrote:
> I suppose so. Frankly I only added that line to appease git-sh-options
> anyway.
> > Should "j" and "f" be "-j" and "-f", respectively?
> No, they're input to "rev-parse --parseopt".
> > There doesn't seem to a usage() function defined anywhere (and
> > OPTIONS_SPEC doesn't seem to be used).
> It's git-sh-setup automagic. Try "./doc-diff --foo"

Ah yes, I saw but then forgot that 'git-sh-setup' was sourced.

> > > +# We'll do both builds in a single worktree, which lets make reuse
> > > +# results that don't differ between the two trees.
> >
> > "which lets make reuse"?
>
> As in, lets the tool "make" reuse results...

Okay, I was confused by "make" being a verb, and thought you had made
a last-minute edit, rewriting "which makes it possible to reuse...",
and intending to say instead "which lets us reuse...". Had it been
formatted "which lets 'make' reuse...", I'd probably have read it
correctly. Not worth a re-roll.

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +               shift; break ;;
>> +       *)
>> +               usage ;;
>
> There doesn't seem to a usage() function defined anywhere (and
> OPTIONS_SPEC doesn't seem to be used).

Isn't this using the parse-options thing git-sh-setup gives us for
free?

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

[Eric Sunshine]

On Fri, Aug 3, 2018 at 5:47 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > There doesn't seem to a usage() function defined anywhere (and
> > OPTIONS_SPEC doesn't seem to be used).
>
> Isn't this using the parse-options thing git-sh-setup gives us for
> free?

Yes. I saw that git-sh-setup was being dot-sourced and then promptly
forgot about it. Peff corrected me.

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Fri, Aug 03, 2018 at 04:52:05PM -0400, Jeff King wrote:
> I wrote this up for my own use after our discussion in [1]. I'm not sure
> if it's too ugly for inclusion, or if it might be helpful to others.
> I've only just written it, but my plan is to try to run it on anything I
> submit to check the formatting. So it _seems_ useful and appears to
> work, but only after a few minutes of playing with it. :)

I think this would indeed be valuable.  Junio seemed to indicate that he
would use it, and I would certainly use it as well.

> diff --git a/Documentation/doc-diff b/Documentation/doc-diff
> new file mode 100755
> index 0000000000..61deb2579e
> --- /dev/null
> +++ b/Documentation/doc-diff
> @@ -0,0 +1,100 @@
> +#!/bin/sh
> +
> +OPTIONS_SPEC="\
> +doc-diff <from> <to> [-- diff options]
> +--
> +j	parallel argument to pass to make
> +f	force rebuild; do not rely on cached results
> +"
> +SUBDIRECTORY_OK=1
> +. "$(git --exec-path)/git-sh-setup"
> +
> +parallel=8

I'm not sure -j8 is a great default.  There are still a lot of
two-core/four-thread machines out there, such as my laptop (from 2016).
Maybe we should default this to 1 unless -j is provided, like make does.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

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

[Jeff King]

On Sun, Aug 05, 2018 at 08:49:31PM +0000, brian m. carlson wrote:

> > +parallel=8
> 
> I'm not sure -j8 is a great default.  There are still a lot of
> two-core/four-thread machines out there, such as my laptop (from 2016).
> Maybe we should default this to 1 unless -j is provided, like make does.

I agree that "8" is arbitrary and probably not universally applicable. I
was just hoping to not have to say "-j8" every time I ran it (I already
"alias make='make -j8'" in my shell, but obviously it doesn't kick in
inside a script).

I guess some other options are:

  1. Respect a config variable. Which seems funny, since this isn't a
     git command.

  2. Respect an environment variable (we already do a similar thing with
     GIT_PERF_MAKE_OPTS, though it feels pretty clumsy).

     I've also considered just setting MAKEFLAGS=-j8 in my environment,
     but it always seemed like an abuse of a variable intended for
     communicating between makes.

  3. Default to number of CPUs, which is what a lot of other threading
     in Git does. Unfortunately getting that from the shell is
     non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but
     people on non-Linux platforms would have to fill in their own
     implementation.

-Peff

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

[Jeff King]

On Mon, Aug 06, 2018 at 09:39:55AM -0400, Jeff King wrote:

>   3. Default to number of CPUs, which is what a lot of other threading
>      in Git does. Unfortunately getting that from the shell is
>      non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but
>      people on non-Linux platforms would have to fill in their own
>      implementation.

Is this too horrible to contemplate?

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 0f09bbbf65..fa8caeec0c 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -635,6 +635,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 
+		if (!strcmp(arg, "--online-cpus")) {
+			printf("%d", online_cpus());
+			continue;
+		}
+
 		/* The rest of the options require a git repository. */
 		if (!did_repo_setup) {
 			prefix = setup_git_directory();

-Peff

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

[Jonathan Nieder]

Jeff King wrote:

>   3. Default to number of CPUs, which is what a lot of other threading
>      in Git does. Unfortunately getting that from the shell is
>      non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but
>      people on non-Linux platforms would have to fill in their own
>      implementation.

How about $(getconf _NPROCESSORS_ONLN)?  That's what Linux's
scripts/coccicheck uses (apropos of a recent discussion :)).

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 06, 2018 at 09:39:55AM -0400, Jeff King wrote:
>
>>   3. Default to number of CPUs, which is what a lot of other threading
>>      in Git does. Unfortunately getting that from the shell is
>>      non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but
>>      people on non-Linux platforms would have to fill in their own
>>      implementation.
>
> Is this too horrible to contemplate?

Chickens, eggs and a kitchen sink.  Sounds like a title of a movie.

It's a bit dissapointing that we cannot express personal preference
in config.mak ;-)

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 0f09bbbf65..fa8caeec0c 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -635,6 +635,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  			continue;
>  		}
>  
> +		if (!strcmp(arg, "--online-cpus")) {
> +			printf("%d", online_cpus());
> +			continue;
> +		}
> +
>  		/* The rest of the options require a git repository. */
>  		if (!did_repo_setup) {
>  			prefix = setup_git_directory();
>
> -Peff

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

[Jeff King]

On Mon, Aug 06, 2018 at 08:01:00AM -0700, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> >   3. Default to number of CPUs, which is what a lot of other threading
> >      in Git does. Unfortunately getting that from the shell is
> >      non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but
> >      people on non-Linux platforms would have to fill in their own
> >      implementation.
> 
> How about $(getconf _NPROCESSORS_ONLN)?  That's what Linux's
> scripts/coccicheck uses (apropos of a recent discussion :)).

Thanks, that's certainly less gross than grepping /proc/cpuinfo. getconf
is POSIX, but _NPROCESSORS_ONLN is not. According to [1], it works on
Linux and macOS, which is probably a reasonable start, though. This is,
after all, a script aimed at developers, and the worst case is that we
default back to 1.

-Peff

[1] https://stackoverflow.com/questions/42862559/one-liner-for-n-1-cores/42863212#42863212

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

[Jeff King]

On Mon, Aug 06, 2018 at 08:16:47AM -0700, Junio C Hamano wrote:

> It's a bit dissapointing that we cannot express personal preference
> in config.mak ;-)

Try this:

diff --git a/Makefile b/Makefile
index e7994888e8..36bddff3be 100644
--- a/Makefile
+++ b/Makefile
@@ -1119,6 +1119,11 @@ ifdef DEVELOPER
 include config.mak.dev
 endif
 
+ifneq ($(or $(J_RECURSED), $(J), done), done)
+%:
+	$(MAKE) -j$(J) J_RECURSED=done $@
+else
+
 comma := ,
 empty :=
 space := $(empty) $(empty)
@@ -3034,3 +3039,4 @@ cover_db: coverage-report
 cover_db_html: cover_db
 	cover -report html -outputdir cover_db_html cover_db
 
+endif


Pretty nasty. ;)

-Peff

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

[Jeff King]

On Fri, Aug 03, 2018 at 04:52:05PM -0400, Jeff King wrote:

> I wrote this up for my own use after our discussion in [1]. I'm not sure
> if it's too ugly for inclusion, or if it might be helpful to others.
> I've only just written it, but my plan is to try to run it on anything I
> submit to check the formatting. So it _seems_ useful and appears to
> work, but only after a few minutes of playing with it. :)
> 
> [1] https://public-inbox.org/git/20180720223608.GE18502@genre.crustytoothpaste.net/

OK, people seem to think this is possibly useful, so here it is with a
little bit of polish based on earlier reviews:

 - we now default to $(getconf _NPROCESSORS_ONLN) parallelism, or 1 if
   that doesn't work (thanks for the getconf suggestion, Jonathan)

 - fixed formatting of usage message, per Eric's suggestion

 - put "make" as a noun in quotes ;)

I suspect the rendering step could be done a little more efficiently. In
addition to `man`, we run a shell, a `mkdir`, and a `mv` for each file.
Probably the whole thing could be done via a single perl script,
exec-ing man as appropriate. But we'd lose the parallelism, unless we do
something clever with threads. So I've left it for now, but if anybody
is interested in poking at it, go for it.

-- >8 --
Subject: [PATCH] add a script to diff rendered documentation

After making a change to the documentation, it's easy to
forget to check the rendered version to make sure it was
formatted as you intended. And simply doing a diff between
the two built versions is less trivial than you might hope:

  - diffing the roff or html output isn't particularly
    readable; what we really care about is what the end user
    will see

  - you have to tweak a few build variables to avoid
    spurious differences (e.g., version numbers, build
    times)

Let's provide a script that builds and installs the manpages
for two commits, renders the results using "man", and diffs
the result. Since this is time-consuming, we'll also do our
best to avoid repeated work, keeping intermediate results
between runs.

Some of this could probably be made a little less ugly if we
built support into Documentation/Makefile. But by relying
only on "make install-man" working, this script should work
for generating a diff between any two versions, whether they
include this script or not.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/.gitignore |   1 +
 Documentation/doc-diff   | 109 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)
 create mode 100755 Documentation/doc-diff

diff --git a/Documentation/.gitignore b/Documentation/.gitignore
index c7096f11f1..3ef54e0adb 100644
--- a/Documentation/.gitignore
+++ b/Documentation/.gitignore
@@ -12,3 +12,4 @@ cmds-*.txt
 mergetools-*.txt
 manpage-base-url.xsl
 SubmittingPatches.txt
+tmp-doc-diff/
diff --git a/Documentation/doc-diff b/Documentation/doc-diff
new file mode 100755
index 0000000000..5d5b243384
--- /dev/null
+++ b/Documentation/doc-diff
@@ -0,0 +1,109 @@
+#!/bin/sh
+
+OPTIONS_SPEC="\
+doc-diff [options] <from> <to> [-- <diff-options>]
+--
+j	parallel argument to pass to make
+f	force rebuild; do not rely on cached results
+"
+SUBDIRECTORY_OK=1
+. "$(git --exec-path)/git-sh-setup"
+
+parallel=
+force=
+while test $# -gt 0
+do
+	case "$1" in
+	-j)
+		parallel=${1#-j} ;;
+	-f)
+		force=t ;;
+	--)
+		shift; break ;;
+	*)
+		usage ;;
+	esac
+	shift
+done
+
+if test -z "$parallel"
+then
+	parallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null)
+	if test $? != 0 || test -z "$parallel"
+	then
+		parallel=1
+	fi
+fi
+
+test $# -gt 1 || usage
+from=$1; shift
+to=$1; shift
+
+from_oid=$(git rev-parse --verify "$from") || exit 1
+to_oid=$(git rev-parse --verify "$to") || exit 1
+
+cd_to_toplevel
+tmp=Documentation/tmp-doc-diff
+
+if test -n "$force"
+then
+	rm -rf "$tmp"
+fi
+
+# We'll do both builds in a single worktree, which lets "make" reuse
+# results that don't differ between the two trees.
+if ! test -d "$tmp/worktree"
+then
+	git worktree add --detach "$tmp/worktree" "$from" &&
+	dots=$(echo "$tmp/worktree" | sed 's#[^/]*#..#g') &&
+	ln -s "$dots/config.mak" "$tmp/worktree/config.mak"
+fi
+
+# generate_render_makefile <srcdir> <dstdir>
+generate_render_makefile () {
+	find "$1" -type f |
+	while read src
+	do
+		dst=$2/${src#$1/}
+		printf 'all:: %s\n' "$dst"
+		printf '%s: %s\n' "$dst" "$src"
+		printf '\t@echo >&2 "  RENDER $(notdir $@)" && \\\n'
+		printf '\tmkdir -p $(dir $@) && \\\n'
+		printf '\tMANWIDTH=80 man -l $< >$@+ && \\\n'
+		printf '\tmv $@+ $@\n'
+	done
+}
+
+# render_tree <dirname> <committish>
+render_tree () {
+	# Skip install-man entirely if we already have an installed directory.
+	# We can't rely on make here, since "install-man" unconditionally
+	# copies the files (spending effort, but also updating timestamps that
+	# we then can't rely on during the render step). We use "mv" to make
+	# sure we don't get confused by a previous run that failed partway
+	# through.
+	if ! test -d "$tmp/installed/$1"
+	then
+		git -C "$tmp/worktree" checkout "$2" &&
+		make -j$parallel -C "$tmp/worktree" \
+			GIT_VERSION=omitted \
+			SOURCE_DATE_EPOCH=0 \
+			DESTDIR="$PWD/$tmp/installed/$1+" \
+			install-man &&
+		mv "$tmp/installed/$1+" "$tmp/installed/$1"
+	fi &&
+
+	# As with "installed" above, we skip the render if it's already been
+	# done.  So using make here is primarily just about running in
+	# parallel.
+	if ! test -d "$tmp/rendered/$1"
+	then
+		generate_render_makefile "$tmp/installed/$1" "$tmp/rendered/$1+" |
+		make -j$parallel -f - &&
+		mv "$tmp/rendered/$1+" "$tmp/rendered/$1"
+	fi
+}
+
+render_tree $from_oid "$from" &&
+render_tree $to_oid "$to" &&
+git -C $tmp/rendered diff --no-index "$@" $from_oid $to_oid
-- 
2.18.0.912.g3ccaa4d859

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

[Junio C Hamano]

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?

> +	-f)
> +		force=t ;;
> +	--)
> +		shift; break ;;
> +	*)
> +		usage ;;
> +	esac
> +	shift
> +done
> +
> +if test -z "$parallel"
> +then

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.

So "script -j1" would be how a user would say "I want to use exactly
one process, not any parallelism", which makes sense.

> +	parallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null)
> +	if test $? != 0 || test -z "$parallel"
> +	then
> +		parallel=1
> +	fi
> +fi

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

[Jeff King]

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

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

> 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