[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 <redacted>
---
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

On Fri, Aug 3, 2018 at 4:52 PM Jeff King [off-list ref] wrote:

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"?

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

quoted

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

quoted

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

quoted

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

quoted

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

On Fri, Aug 3, 2018 at 5:38 PM Jeff King [off-list ref] 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.

quoted

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

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

quoted

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.

quoted

quoted

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

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.

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

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

quoted

+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

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

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 :)).

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

Jeff King wrote:

quoted

  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

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 <redacted>
---
 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