threads / patch / 49024

patchadd a script to diff rendered documentation

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

## tl;dr

17 messages between Aug 3, 2018 and Aug 6, 2018. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

Jeff King· Aug 3, 2018, 20:52 UTC · lore

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
Show changes to 2 files +101 −0

Documentation/.gitignore, 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
Eric Sunshine· Aug 3, 2018, 21:33 UTC · re: Jeff King · lore

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

On Fri, Aug 3, 2018 at 4:52 PM Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
> [...]
> 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?
Show 11 quoted lines
> +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).

Show 5 quoted lines
> +       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"?
Jeff King· Aug 3, 2018, 21:38 UTC · re: Eric Sunshine · lore

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

On Fri, Aug 03, 2018 at 05:33:17PM -0400, Eric Sunshine wrote:
Show 6 quoted lines
> > +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.

Show 6 quoted lines
> > +--
> > +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".
Show 14 quoted lines
> > +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
Eric Sunshine· Aug 3, 2018, 21:44 UTC · re: Jeff King · lore

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

On Fri, Aug 3, 2018 at 5:38 PM Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> 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.
Show 6 quoted lines
> > > +# 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.

Junio C Hamano· Aug 3, 2018, 21:47 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 6 quoted lines
>> +               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?

Eric Sunshine· Aug 3, 2018, 22:41 UTC · re: Junio C Hamano · lore

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

On Fri, Aug 3, 2018 at 5:47 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
> 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.

brian m. carlson· Aug 5, 2018, 20:49 UTC · re: Jeff King · lore

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

On Fri, Aug 03, 2018 at 04:52:05PM -0400, Jeff King wrote:
Show 5 quoted lines
> 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.

Show 18 quoted lines
> 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
Jeff King· Aug 6, 2018, 13:39 UTC · re: brian m. carlson · lore

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

On Sun, Aug 05, 2018 at 08:49:31PM +0000, brian m. carlson wrote:
Show 5 quoted lines
> > +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
Jeff King· Aug 6, 2018, 13:42 UTC · re: Jeff King · lore

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

On Mon, Aug 06, 2018 at 09:39:55AM -0400, Jeff King wrote:
Show 5 quoted lines
>   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?
Show changes to builtin/rev-parse.c +5 −1
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
Junio C Hamano· Aug 6, 2018, 15:16 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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 ;-)

Show 18 quoted lines
> 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
Jeff King· Aug 6, 2018, 16:57 UTC · re: Junio C Hamano · lore

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

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:
Show changes to Makefile +6 −1
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
Jonathan Nieder· Aug 6, 2018, 15:01 UTC · re: Jeff King · lore

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

Jeff King wrote:
Show 5 quoted lines
>   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 :)).

Jeff King· Aug 6, 2018, 16:42 UTC · re: Jonathan Nieder · lore

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

On Mon, Aug 06, 2018 at 08:01:00AM -0700, Jonathan Nieder wrote:
Show 10 quoted lines
> 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
Jeff King· Aug 6, 2018, 17:37 UTC · re: Jeff King · lore

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

On Fri, Aug 03, 2018 at 04:52:05PM -0400, Jeff King wrote:
Show 7 quoted lines
> 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
Show changes to 2 files +110 −0

Documentation/.gitignore, 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
Junio C Hamano· Aug 6, 2018, 18:25 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> +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?
Show 12 quoted lines
> +	-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.

Show 6 quoted lines
> +	parallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null)
> +	if test $? != 0 || test -z "$parallel"
> +	then
> +		parallel=1
> +	fi
> +fi
Jeff King· Aug 6, 2018, 18:58 UTC · re: Junio C Hamano · lore

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

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:
Show changes to Documentation/doc-diff +2 −3
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
Junio C Hamano· Aug 6, 2018, 19:29 UTC · re: Jeff King · lore

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

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

← back to recent threads