{"thread":{"id":"49024","subject":"[PATCH] add a script to diff rendered documentation","startedAt":"2018-08-03T20:52:08Z","lastAt":"2018-08-06T19:29:32Z","messageCount":17,"participants":["Jeff King","Eric Sunshine","Junio C Hamano","brian m. carlson","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"354435","messageId":"20180803205204.GA3790@sigill.intra.peff.net","threadId":"49024","inReplyTo":null,"subject":"[PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-03T20:52:05Z","receivedAt":"2018-08-03T20:52:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After making a change to the documentation, it's easy to\nforget to check the rendered version to make sure it was\nformatted as you intended. And simply doing a diff between\nthe two built versions is less trivial than you might hope:\n\n - diffing the roff or html output isn't particularly\n readable; what we really care about is what the end user\n will see\n\n - you have to tweak a few build variables to avoid\n spurious differences (e.g., version numbers, build\n times)\n\nLet's provide a script that builds and installs the manpages\nfor two commits, renders the results using \"man\", and diffs\nthe result. Since this is time-consuming, we'll also do our\nbest to avoid repeated work, keeping intermediate results\nbetween runs.\n\nSome of this could probably be made a little less ugly if we\nbuilt support into Documentation/Makefile. But by relying\nonly on \"make install-man\" working, this script should work\nfor generating a diff between any two versions, whether they\ninclude this script or not.\n\nSigned-off-by: Jeff King \n---\nI wrote this up for my own use after our discussion in [1]. I'm not sure\nif it's too ugly for inclusion, or if it might be helpful to others.\nI've only just written it, but my plan is to try to run it on anything I\nsubmit to check the formatting. So it _seems_ useful and appears to\nwork, but only after a few minutes of playing with it. :)\n\n[1] https://public-inbox.org/git/20180720223608.GE18502@genre.crustytoothpaste.net/\n\n Documentation/.gitignore | 1 +\n Documentation/doc-diff | 100 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 101 insertions(+)\n create mode 100755 Documentation/doc-diff\n\ndiff --git a/Documentation/.gitignore b/Documentation/.gitignore\nindex c7096f11f1..3ef54e0adb 100644\n--- a/Documentation/.gitignore\n+++ b/Documentation/.gitignore\n@@ -12,3 +12,4 @@ cmds-*.txt\n mergetools-*.txt\n manpage-base-url.xsl\n SubmittingPatches.txt\n+tmp-doc-diff/\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nnew file mode 100755\nindex 0000000000..61deb2579e\n--- /dev/null\n+++ b/Documentation/doc-diff\n@@ -0,0 +1,100 @@\n+#!/bin/sh\n+\n+OPTIONS_SPEC=\"\\\n+doc-diff [-- diff options]\n+--\n+j\tparallel argument to pass to make\n+f\tforce rebuild; do not rely on cached results\n+\"\n+SUBDIRECTORY_OK=1\n+. \"$(git --exec-path)/git-sh-setup\"\n+\n+parallel=8\n+force=\n+while test $# -gt 0\n+do\n+\tcase \"$1\" in\n+\t-j)\n+\t\tparallel=${1#-j} ;;\n+\t-f)\n+\t\tforce=t ;;\n+\t--)\n+\t\tshift; break ;;\n+\t*)\n+\t\tusage ;;\n+\tesac\n+\tshift\n+done\n+\n+test $# -gt 1 || usage\n+from=$1; shift\n+to=$1; shift\n+\n+from_oid=$(git rev-parse --verify \"$from\") || exit 1\n+to_oid=$(git rev-parse --verify \"$to\") || exit 1\n+\n+cd_to_toplevel\n+tmp=Documentation/tmp-doc-diff\n+\n+if test -n \"$force\"\n+then\n+\trm -rf \"$tmp\"\n+fi\n+\n+# We'll do both builds in a single worktree, which lets make reuse\n+# results that don't differ between the two trees.\n+if ! test -d \"$tmp/worktree\"\n+then\n+\tgit worktree add --detach \"$tmp/worktree\" \"$from\" &&\n+\tdots=$(echo \"$tmp/worktree\" | sed 's#[^/]*#..#g') &&\n+\tln -s \"$dots/config.mak\" \"$tmp/worktree/config.mak\"\n+fi\n+\n+# generate_render_makefile \n+generate_render_makefile () {\n+\tfind \"$1\" -type f |\n+\twhile read src\n+\tdo\n+\t\tdst=$2/${src#$1/}\n+\t\tprintf 'all:: %s\\n' \"$dst\"\n+\t\tprintf '%s: %s\\n' \"$dst\" \"$src\"\n+\t\tprintf '\\t@echo >&2 \" RENDER $(notdir $@)\" && \\\\\\n'\n+\t\tprintf '\\tmkdir -p $(dir $@) && \\\\\\n'\n+\t\tprintf '\\tMANWIDTH=80 man -l $< >$@+ && \\\\\\n'\n+\t\tprintf '\\tmv $@+ $@\\n'\n+\tdone\n+}\n+\n+# render_tree \n+render_tree () {\n+\t# Skip install-man entirely if we already have an installed directory.\n+\t# We can't rely on make here, since \"install-man\" unconditionally\n+\t# copies the files (spending effort, but also updating timestamps that\n+\t# we then can't rely on during the render step). We use \"mv\" to make\n+\t# sure we don't get confused by a previous run that failed partway\n+\t# through.\n+\tif ! test -d \"$tmp/installed/$1\"\n+\tthen\n+\t\tgit -C \"$tmp/worktree\" checkout \"$2\" &&\n+\t\tmake -j$parallel -C \"$tmp/worktree\" \\\n+\t\t\tGIT_VERSION=omitted \\\n+\t\t\tSOURCE_DATE_EPOCH=0 \\\n+\t\t\tDESTDIR=\"$PWD/$tmp/installed/$1+\" \\\n+\t\t\tinstall-man &&\n+\t\tmv \"$tmp/installed/$1+\" \"$tmp/installed/$1\"\n+\tfi &&\n+\n+\t# As with \"installed\" above, we skip the render if it's already been\n+\t# done. So using make here is primarily just about running in\n+\t# parallel.\n+\tif ! test -d \"$tmp/rendered/$1\"\n+\tthen\n+\t\tgenerate_render_makefile \"$tmp/installed/$1\" \"$tmp/rendered/$1+\" |\n+\t\tmake -j$parallel -f - &&\n+\t\tmv \"$tmp/rendered/$1+\" \"$tmp/rendered/$1\"\n+\tfi\n+}\n+\n+render_tree $from_oid \"$from\" &&\n+render_tree $to_oid \"$to\" &&\n+git -C $tmp/rendered diff --no-index \"$@\" $from_oid $to_oid\n-- \n2.18.0.912.g3ccaa4d859\n"},{"id":"354436","messageId":"CAPig+cSDM4vjuhwfuapuPpjjOmXbrhz-i0eVhtShp+nOYReLnA@mail.gmail.com","threadId":"49024","inReplyTo":"20180803205204.GA3790@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T21:33:17Z","receivedAt":"2018-08-03T21:33:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 3, 2018 at 4:52 PM Jeff King wrote:\n> [...]\n> Let's provide a script that builds and installs the manpages\n> for two commits, renders the results using \"man\", and diffs\n> the result. Since this is time-consuming, we'll also do our\n> best to avoid repeated work, keeping intermediate results\n> between runs.\n> [...]\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/Documentation/doc-diff b/Documentation/doc-diff\n> @@ -0,0 +1,100 @@\n> +OPTIONS_SPEC=\"\\\n> +doc-diff [-- diff options]\n\nShould this be?\n\n doc-diff [] [-- ]\n\n> +--\n> +j parallel argument to pass to make\n> +f force rebuild; do not rely on cached results\n> +\"\n\nShould \"j\" and \"f\" be \"-j\" and \"-f\", respectively?\n\n> +while test $# -gt 0\n> +do\n> + case \"$1\" in\n> + -j)\n> + parallel=${1#-j} ;;\n> + -f)\n> + force=t ;;\n> + --)\n> + shift; break ;;\n> + *)\n> + usage ;;\n\nThere doesn't seem to a usage() function defined anywhere (and\nOPTIONS_SPEC doesn't seem to be used).\n\n> + esac\n> + shift\n> +done\n> +# We'll do both builds in a single worktree, which lets make reuse\n> +# results that don't differ between the two trees.\n\n\"which lets make reuse\"?\n"},{"id":"354437","messageId":"20180803213819.GA4650@sigill.intra.peff.net","threadId":"49024","inReplyTo":"CAPig+cSDM4vjuhwfuapuPpjjOmXbrhz-i0eVhtShp+nOYReLnA@mail.gmail.com","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-03T21:38:19Z","receivedAt":"2018-08-03T21:38:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2018 at 05:33:17PM -0400, Eric Sunshine wrote:\n\n> > +OPTIONS_SPEC=\"\\\n> > +doc-diff [-- diff options]\n> \n> Should this be?\n> \n> doc-diff [] [-- ]\n\nI suppose so. Frankly I only added that line to appease git-sh-options\nanyway.\n\n> > +--\n> > +j parallel argument to pass to make\n> > +f force rebuild; do not rely on cached results\n> > +\"\n> \n> Should \"j\" and \"f\" be \"-j\" and \"-f\", respectively?\n\nNo, they're input to \"rev-parse --parseopt\".\n\n> > +while test $# -gt 0\n> > +do\n> > + case \"$1\" in\n> > + -j)\n> > + parallel=${1#-j} ;;\n> > + -f)\n> > + force=t ;;\n> > + --)\n> > + shift; break ;;\n> > + *)\n> > + usage ;;\n> \n> There doesn't seem to a usage() function defined anywhere (and\n> OPTIONS_SPEC doesn't seem to be used).\n\nIt's git-sh-setup automagic. Try \"./doc-diff --foo\"\n\n> > +# We'll do both builds in a single worktree, which lets make reuse\n> > +# results that don't differ between the two trees.\n> \n> \"which lets make reuse\"?\n\nAs in, lets the tool \"make\" reuse results...\n\n-Peff\n"},{"id":"354438","messageId":"CAPig+cRKPdYa7-8YB=rV3jJDe47mgB4xwbeWu4a4JQhx9r2h3Q@mail.gmail.com","threadId":"49024","inReplyTo":"20180803213819.GA4650@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T21:44:37Z","receivedAt":"2018-08-03T21:44:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 3, 2018 at 5:38 PM Jeff King wrote:\n> On Fri, Aug 03, 2018 at 05:33:17PM -0400, Eric Sunshine wrote:\n> I suppose so. Frankly I only added that line to appease git-sh-options\n> anyway.\n> > Should \"j\" and \"f\" be \"-j\" and \"-f\", respectively?\n> No, they're input to \"rev-parse --parseopt\".\n> > There doesn't seem to a usage() function defined anywhere (and\n> > OPTIONS_SPEC doesn't seem to be used).\n> It's git-sh-setup automagic. Try \"./doc-diff --foo\"\n\nAh yes, I saw but then forgot that 'git-sh-setup' was sourced.\n\n> > > +# We'll do both builds in a single worktree, which lets make reuse\n> > > +# results that don't differ between the two trees.\n> >\n> > \"which lets make reuse\"?\n>\n> As in, lets the tool \"make\" reuse results...\n\nOkay, I was confused by \"make\" being a verb, and thought you had made\na last-minute edit, rewriting \"which makes it possible to reuse...\",\nand intending to say instead \"which lets us reuse...\". Had it been\nformatted \"which lets 'make' reuse...\", I'd probably have read it\ncorrectly. Not worth a re-roll.\n"},{"id":"354441","messageId":"xmqq1sbfw2ap.fsf@gitster-ct.c.googlers.com","threadId":"49024","inReplyTo":"CAPig+cSDM4vjuhwfuapuPpjjOmXbrhz-i0eVhtShp+nOYReLnA@mail.gmail.com","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-03T21:47:10Z","receivedAt":"2018-08-03T21:47:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + shift; break ;;\n>> + *)\n>> + usage ;;\n>\n> There doesn't seem to a usage() function defined anywhere (and\n> OPTIONS_SPEC doesn't seem to be used).\n\nIsn't this using the parse-options thing git-sh-setup gives us for\nfree?\n"},{"id":"354455","messageId":"CAPig+cTrYvvTwExoXq9-p62gJEa8MbHnOGTgdDuPN_LVLDcyeg@mail.gmail.com","threadId":"49024","inReplyTo":"xmqq1sbfw2ap.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-03T22:41:45Z","receivedAt":"2018-08-03T22:42:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 3, 2018 at 5:47 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > There doesn't seem to a usage() function defined anywhere (and\n> > OPTIONS_SPEC doesn't seem to be used).\n>\n> Isn't this using the parse-options thing git-sh-setup gives us for\n> free?\n\nYes. I saw that git-sh-setup was being dot-sourced and then promptly\nforgot about it. Peff corrected me.\n"},{"id":"354578","messageId":"20180805204930.GA202464@genre.crustytoothpaste.net","threadId":"49024","inReplyTo":"20180803205204.GA3790@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-08-05T20:49:31Z","receivedAt":"2018-08-05T20:49:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Aug 03, 2018 at 04:52:05PM -0400, Jeff King wrote:\n> I wrote this up for my own use after our discussion in [1]. I'm not sure\n> if it's too ugly for inclusion, or if it might be helpful to others.\n> I've only just written it, but my plan is to try to run it on anything I\n> submit to check the formatting. So it _seems_ useful and appears to\n> work, but only after a few minutes of playing with it. :)\n\nI think this would indeed be valuable. Junio seemed to indicate that he\nwould use it, and I would certainly use it as well.\n\n> diff --git a/Documentation/doc-diff b/Documentation/doc-diff\n> new file mode 100755\n> index 0000000000..61deb2579e\n> --- /dev/null\n> +++ b/Documentation/doc-diff\n> @@ -0,0 +1,100 @@\n> +#!/bin/sh\n> +\n> +OPTIONS_SPEC=\"\\\n> +doc-diff [-- diff options]\n> +--\n> +j\tparallel argument to pass to make\n> +f\tforce rebuild; do not rely on cached results\n> +\"\n> +SUBDIRECTORY_OK=1\n> +. \"$(git --exec-path)/git-sh-setup\"\n> +\n> +parallel=8\n\nI'm not sure -j8 is a great default. There are still a lot of\ntwo-core/four-thread machines out there, such as my laptop (from 2016).\nMaybe we should default this to 1 unless -j is provided, like make does.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"354601","messageId":"20180806133954.GA31282@sigill.intra.peff.net","threadId":"49024","inReplyTo":"20180805204930.GA202464@genre.crustytoothpaste.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-06T13:39:55Z","receivedAt":"2018-08-06T13:39:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 05, 2018 at 08:49:31PM +0000, brian m. carlson wrote:\n\n> > +parallel=8\n> \n> I'm not sure -j8 is a great default. There are still a lot of\n> two-core/four-thread machines out there, such as my laptop (from 2016).\n> Maybe we should default this to 1 unless -j is provided, like make does.\n\nI agree that \"8\" is arbitrary and probably not universally applicable. I\nwas just hoping to not have to say \"-j8\" every time I ran it (I already\n\"alias make='make -j8'\" in my shell, but obviously it doesn't kick in\ninside a script).\n\nI guess some other options are:\n\n 1. Respect a config variable. Which seems funny, since this isn't a\n git command.\n\n 2. Respect an environment variable (we already do a similar thing with\n GIT_PERF_MAKE_OPTS, though it feels pretty clumsy).\n\n I've also considered just setting MAKEFLAGS=-j8 in my environment,\n but it always seemed like an abuse of a variable intended for\n communicating between makes.\n\n 3. Default to number of CPUs, which is what a lot of other threading\n in Git does. Unfortunately getting that from the shell is\n non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but\n people on non-Linux platforms would have to fill in their own\n implementation.\n\n-Peff\n"},{"id":"354602","messageId":"20180806134237.GB31282@sigill.intra.peff.net","threadId":"49024","inReplyTo":"20180806133954.GA31282@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-06T13:42:38Z","receivedAt":"2018-08-06T13:42:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2018 at 09:39:55AM -0400, Jeff King wrote:\n\n> 3. Default to number of CPUs, which is what a lot of other threading\n> in Git does. Unfortunately getting that from the shell is\n> non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but\n> people on non-Linux platforms would have to fill in their own\n> implementation.\n\nIs this too horrible to contemplate?\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 0f09bbbf65..fa8caeec0c 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -635,6 +635,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (!strcmp(arg, \"--online-cpus\")) {\n+\t\t\tprintf(\"%d\", online_cpus());\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* The rest of the options require a git repository. */\n \t\tif (!did_repo_setup) {\n \t\t\tprefix = setup_git_directory();\n\n-Peff\n"},{"id":"354614","messageId":"20180806150100.GC97564@aiede.svl.corp.google.com","threadId":"49024","inReplyTo":"20180806133954.GA31282@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-06T15:01:00Z","receivedAt":"2018-08-06T15:01:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> 3. Default to number of CPUs, which is what a lot of other threading\n> in Git does. Unfortunately getting that from the shell is\n> non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but\n> people on non-Linux platforms would have to fill in their own\n> implementation.\n\nHow about $(getconf _NPROCESSORS_ONLN)? That's what Linux's\nscripts/coccicheck uses (apropos of a recent discussion :)).\n"},{"id":"354618","messageId":"xmqqd0uvr0dc.fsf@gitster-ct.c.googlers.com","threadId":"49024","inReplyTo":"20180806134237.GB31282@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T15:16:47Z","receivedAt":"2018-08-06T15:16:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 06, 2018 at 09:39:55AM -0400, Jeff King wrote:\n>\n>> 3. Default to number of CPUs, which is what a lot of other threading\n>> in Git does. Unfortunately getting that from the shell is\n>> non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but\n>> people on non-Linux platforms would have to fill in their own\n>> implementation.\n>\n> Is this too horrible to contemplate?\n\nChickens, eggs and a kitchen sink. Sounds like a title of a movie.\n\nIt's a bit dissapointing that we cannot express personal preference\nin config.mak ;-)\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 0f09bbbf65..fa8caeec0c 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -635,6 +635,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> \t\t\tcontinue;\n> \t\t}\n> \n> +\t\tif (!strcmp(arg, \"--online-cpus\")) {\n> +\t\t\tprintf(\"%d\", online_cpus());\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> \t\t/* The rest of the options require a git repository. */\n> \t\tif (!did_repo_setup) {\n> \t\t\tprefix = setup_git_directory();\n>\n> -Peff\n"},{"id":"354643","messageId":"20180806164208.GB19053@sigill.intra.peff.net","threadId":"49024","inReplyTo":"20180806150100.GC97564@aiede.svl.corp.google.com","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-06T16:42:08Z","receivedAt":"2018-08-06T16:42:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2018 at 08:01:00AM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > 3. Default to number of CPUs, which is what a lot of other threading\n> > in Git does. Unfortunately getting that from the shell is\n> > non-trivial. I'm OK with $(grep -c ^processor /proc/cpuinfo), but\n> > people on non-Linux platforms would have to fill in their own\n> > implementation.\n> \n> How about $(getconf _NPROCESSORS_ONLN)? That's what Linux's\n> scripts/coccicheck uses (apropos of a recent discussion :)).\n\nThanks, that's certainly less gross than grepping /proc/cpuinfo. getconf\nis POSIX, but _NPROCESSORS_ONLN is not. According to [1], it works on\nLinux and macOS, which is probably a reasonable start, though. This is,\nafter all, a script aimed at developers, and the worst case is that we\ndefault back to 1.\n\n-Peff\n\n[1] https://stackoverflow.com/questions/42862559/one-liner-for-n-1-cores/42863212#42863212\n"},{"id":"354644","messageId":"20180806165714.GC19053@sigill.intra.peff.net","threadId":"49024","inReplyTo":"xmqqd0uvr0dc.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-06T16:57:15Z","receivedAt":"2018-08-06T16:57:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2018 at 08:16:47AM -0700, Junio C Hamano wrote:\n\n> It's a bit dissapointing that we cannot express personal preference\n> in config.mak ;-)\n\nTry this:\n\ndiff --git a/Makefile b/Makefile\nindex e7994888e8..36bddff3be 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1119,6 +1119,11 @@ ifdef DEVELOPER\n include config.mak.dev\n endif\n \n+ifneq ($(or $(J_RECURSED), $(J), done), done)\n+%:\n+\t$(MAKE) -j$(J) J_RECURSED=done $@\n+else\n+\n comma := ,\n empty :=\n space := $(empty) $(empty)\n@@ -3034,3 +3039,4 @@ cover_db: coverage-report\n cover_db_html: cover_db\n \tcover -report html -outputdir cover_db_html cover_db\n \n+endif\n\n\nPretty nasty. ;)\n\n-Peff\n"},{"id":"354650","messageId":"20180806173720.GA5508@sigill.intra.peff.net","threadId":"49024","inReplyTo":"20180803205204.GA3790@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-06T17:37:20Z","receivedAt":"2018-08-06T17:37:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2018 at 04:52:05PM -0400, Jeff King wrote:\n\n> I wrote this up for my own use after our discussion in [1]. I'm not sure\n> if it's too ugly for inclusion, or if it might be helpful to others.\n> I've only just written it, but my plan is to try to run it on anything I\n> submit to check the formatting. So it _seems_ useful and appears to\n> work, but only after a few minutes of playing with it. :)\n> \n> [1] https://public-inbox.org/git/20180720223608.GE18502@genre.crustytoothpaste.net/\n\nOK, people seem to think this is possibly useful, so here it is with a\nlittle bit of polish based on earlier reviews:\n\n - we now default to $(getconf _NPROCESSORS_ONLN) parallelism, or 1 if\n that doesn't work (thanks for the getconf suggestion, Jonathan)\n\n - fixed formatting of usage message, per Eric's suggestion\n\n - put \"make\" as a noun in quotes ;)\n\nI suspect the rendering step could be done a little more efficiently. In\naddition to `man`, we run a shell, a `mkdir`, and a `mv` for each file.\nProbably the whole thing could be done via a single perl script,\nexec-ing man as appropriate. But we'd lose the parallelism, unless we do\nsomething clever with threads. So I've left it for now, but if anybody\nis interested in poking at it, go for it.\n\n-- >8 --\nSubject: [PATCH] add a script to diff rendered documentation\n\nAfter making a change to the documentation, it's easy to\nforget to check the rendered version to make sure it was\nformatted as you intended. And simply doing a diff between\nthe two built versions is less trivial than you might hope:\n\n - diffing the roff or html output isn't particularly\n readable; what we really care about is what the end user\n will see\n\n - you have to tweak a few build variables to avoid\n spurious differences (e.g., version numbers, build\n times)\n\nLet's provide a script that builds and installs the manpages\nfor two commits, renders the results using \"man\", and diffs\nthe result. Since this is time-consuming, we'll also do our\nbest to avoid repeated work, keeping intermediate results\nbetween runs.\n\nSome of this could probably be made a little less ugly if we\nbuilt support into Documentation/Makefile. But by relying\nonly on \"make install-man\" working, this script should work\nfor generating a diff between any two versions, whether they\ninclude this script or not.\n\nSigned-off-by: Jeff King \n---\n Documentation/.gitignore | 1 +\n Documentation/doc-diff | 109 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 110 insertions(+)\n create mode 100755 Documentation/doc-diff\n\ndiff --git a/Documentation/.gitignore b/Documentation/.gitignore\nindex c7096f11f1..3ef54e0adb 100644\n--- a/Documentation/.gitignore\n+++ b/Documentation/.gitignore\n@@ -12,3 +12,4 @@ cmds-*.txt\n mergetools-*.txt\n manpage-base-url.xsl\n SubmittingPatches.txt\n+tmp-doc-diff/\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nnew file mode 100755\nindex 0000000000..5d5b243384\n--- /dev/null\n+++ b/Documentation/doc-diff\n@@ -0,0 +1,109 @@\n+#!/bin/sh\n+\n+OPTIONS_SPEC=\"\\\n+doc-diff [options] [-- ]\n+--\n+j\tparallel argument to pass to make\n+f\tforce rebuild; do not rely on cached results\n+\"\n+SUBDIRECTORY_OK=1\n+. \"$(git --exec-path)/git-sh-setup\"\n+\n+parallel=\n+force=\n+while test $# -gt 0\n+do\n+\tcase \"$1\" in\n+\t-j)\n+\t\tparallel=${1#-j} ;;\n+\t-f)\n+\t\tforce=t ;;\n+\t--)\n+\t\tshift; break ;;\n+\t*)\n+\t\tusage ;;\n+\tesac\n+\tshift\n+done\n+\n+if test -z \"$parallel\"\n+then\n+\tparallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null)\n+\tif test $? != 0 || test -z \"$parallel\"\n+\tthen\n+\t\tparallel=1\n+\tfi\n+fi\n+\n+test $# -gt 1 || usage\n+from=$1; shift\n+to=$1; shift\n+\n+from_oid=$(git rev-parse --verify \"$from\") || exit 1\n+to_oid=$(git rev-parse --verify \"$to\") || exit 1\n+\n+cd_to_toplevel\n+tmp=Documentation/tmp-doc-diff\n+\n+if test -n \"$force\"\n+then\n+\trm -rf \"$tmp\"\n+fi\n+\n+# We'll do both builds in a single worktree, which lets \"make\" reuse\n+# results that don't differ between the two trees.\n+if ! test -d \"$tmp/worktree\"\n+then\n+\tgit worktree add --detach \"$tmp/worktree\" \"$from\" &&\n+\tdots=$(echo \"$tmp/worktree\" | sed 's#[^/]*#..#g') &&\n+\tln -s \"$dots/config.mak\" \"$tmp/worktree/config.mak\"\n+fi\n+\n+# generate_render_makefile \n+generate_render_makefile () {\n+\tfind \"$1\" -type f |\n+\twhile read src\n+\tdo\n+\t\tdst=$2/${src#$1/}\n+\t\tprintf 'all:: %s\\n' \"$dst\"\n+\t\tprintf '%s: %s\\n' \"$dst\" \"$src\"\n+\t\tprintf '\\t@echo >&2 \" RENDER $(notdir $@)\" && \\\\\\n'\n+\t\tprintf '\\tmkdir -p $(dir $@) && \\\\\\n'\n+\t\tprintf '\\tMANWIDTH=80 man -l $< >$@+ && \\\\\\n'\n+\t\tprintf '\\tmv $@+ $@\\n'\n+\tdone\n+}\n+\n+# render_tree \n+render_tree () {\n+\t# Skip install-man entirely if we already have an installed directory.\n+\t# We can't rely on make here, since \"install-man\" unconditionally\n+\t# copies the files (spending effort, but also updating timestamps that\n+\t# we then can't rely on during the render step). We use \"mv\" to make\n+\t# sure we don't get confused by a previous run that failed partway\n+\t# through.\n+\tif ! test -d \"$tmp/installed/$1\"\n+\tthen\n+\t\tgit -C \"$tmp/worktree\" checkout \"$2\" &&\n+\t\tmake -j$parallel -C \"$tmp/worktree\" \\\n+\t\t\tGIT_VERSION=omitted \\\n+\t\t\tSOURCE_DATE_EPOCH=0 \\\n+\t\t\tDESTDIR=\"$PWD/$tmp/installed/$1+\" \\\n+\t\t\tinstall-man &&\n+\t\tmv \"$tmp/installed/$1+\" \"$tmp/installed/$1\"\n+\tfi &&\n+\n+\t# As with \"installed\" above, we skip the render if it's already been\n+\t# done. So using make here is primarily just about running in\n+\t# parallel.\n+\tif ! test -d \"$tmp/rendered/$1\"\n+\tthen\n+\t\tgenerate_render_makefile \"$tmp/installed/$1\" \"$tmp/rendered/$1+\" |\n+\t\tmake -j$parallel -f - &&\n+\t\tmv \"$tmp/rendered/$1+\" \"$tmp/rendered/$1\"\n+\tfi\n+}\n+\n+render_tree $from_oid \"$from\" &&\n+render_tree $to_oid \"$to\" &&\n+git -C $tmp/rendered diff --no-index \"$@\" $from_oid $to_oid\n-- \n2.18.0.912.g3ccaa4d859\n\n"},{"id":"354663","messageId":"xmqqy3djnyik.fsf@gitster-ct.c.googlers.com","threadId":"49024","inReplyTo":"20180806173720.GA5508@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T18:25:07Z","receivedAt":"2018-08-06T18:25:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +while test $# -gt 0\n> +do\n> +\tcase \"$1\" in\n> +\t-j)\n> +\t\tparallel=${1#-j} ;;\n\nThis is curious. Did you mean \"-j*)\" on the line above this one?\n\n> +\t-f)\n> +\t\tforce=t ;;\n> +\t--)\n> +\t\tshift; break ;;\n> +\t*)\n> +\t\tusage ;;\n> +\tesac\n> +\tshift\n> +done\n> +\n> +if test -z \"$parallel\"\n> +then\n\nThen \"script -j\" (no explicit number) would get here and autodetect.\nRunning the script without any \"-j\" would also get an empty parallel\nand come here.\n\nSo \"script -j1\" would be how a user would say \"I want to use exactly\none process, not any parallelism\", which makes sense.\n\n> +\tparallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null)\n> +\tif test $? != 0 || test -z \"$parallel\"\n> +\tthen\n> +\t\tparallel=1\n> +\tfi\n> +fi\n"},{"id":"354669","messageId":"20180806185853.GA18119@sigill.intra.peff.net","threadId":"49024","inReplyTo":"xmqqy3djnyik.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-06T18:58:54Z","receivedAt":"2018-08-06T18:58:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 06, 2018 at 11:25:07AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +while test $# -gt 0\n> > +do\n> > +\tcase \"$1\" in\n> > +\t-j)\n> > +\t\tparallel=${1#-j} ;;\n> \n> This is curious. Did you mean \"-j*)\" on the line above this one?\n\nHmph, yes, I think this was broken even in the original. And after going\nthrough \"rev-parse --parseopt\", we should have a separate argument\nanyway, even for the \"stuck\" form. Worse, the OPTIONS_SPEC doesn't\nmention the argument, so it barfs on \"-j4\".\n\nI think we need to squash this in:\n\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nindex 5d5b243384..f483fe427c 100755\n--- a/Documentation/doc-diff\n+++ b/Documentation/doc-diff\n@@ -3,7 +3,7 @@\n OPTIONS_SPEC=\"\\\n doc-diff [options] [-- ]\n --\n-j\tparallel argument to pass to make\n+j=n\tparallel argument to pass to make\n f\tforce rebuild; do not rely on cached results\n \"\n SUBDIRECTORY_OK=1\n@@ -15,7 +15,7 @@ while test $# -gt 0\n do\n \tcase \"$1\" in\n \t-j)\n-\t\tparallel=${1#-j} ;;\n+\t\tparallel=$2; shift ;;\n \t-f)\n \t\tforce=t ;;\n \t--)\n\n> Then \"script -j\" (no explicit number) would get here and autodetect.\n> Running the script without any \"-j\" would also get an empty parallel\n> and come here.\n\nYeah, I think that is the wrong thing. If anything \"-j\" should behave\nlike \"make -j\". However, it looks like \"rev-parse --parseopt\" doesn't\nplay well with optional arguments for short-only options. You get \"-j\",\nbut then you have no idea whether the next argument is an optional value\nfor it, or another option entirely. Arguably it should give a blank\nstring or something (if you have long options, then it uses the\nlong-stock form, which is fine).\n\n> So \"script -j1\" would be how a user would say \"I want to use exactly\n> one process, not any parallelism\", which makes sense.\n\nRight, that was the thing I actually wanted to have happen. :)\n\n-Peff\n"},{"id":"354674","messageId":"xmqqmutznvjb.fsf@gitster-ct.c.googlers.com","threadId":"49024","inReplyTo":"20180806185853.GA18119@sigill.intra.peff.net","subject":"Re: [PATCH] add a script to diff rendered documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T19:29:28Z","receivedAt":"2018-08-06T19:29:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > +\tcase \"$1\" in\n>> > +\t-j)\n>> > +\t\tparallel=${1#-j} ;;\n>> \n>> This is curious. Did you mean \"-j*)\" on the line above this one?\n>\n> Hmph, yes, I think this was broken even in the original. And after going\n> through \"rev-parse --parseopt\", we should have a separate argument\n> anyway, even for the \"stuck\" form. Worse, the OPTIONS_SPEC doesn't\n> mention the argument, so it barfs on \"-j4\".\n\nAh, I forgot (just like somebody else, and even worse is that I\nreminded him of this fact that I am forgetting here) that we use the\nparseopt thing to normalize the option parsing, so with the correct\nspec \"-j)\" is the right thing to say (but yes then you'd look at $2\nand then shift both aawy).\n\n> I think we need to squash this in:\n\nYup. Thanks.\n\n> diff --git a/Documentation/doc-diff b/Documentation/doc-diff\n> index 5d5b243384..f483fe427c 100755\n> --- a/Documentation/doc-diff\n> +++ b/Documentation/doc-diff\n> @@ -3,7 +3,7 @@\n> OPTIONS_SPEC=\"\\\n> doc-diff [options] [-- ]\n> --\n> -j\tparallel argument to pass to make\n> +j=n\tparallel argument to pass to make\n> f\tforce rebuild; do not rely on cached results\n> \"\n> SUBDIRECTORY_OK=1\n> @@ -15,7 +15,7 @@ while test $# -gt 0\n> do\n> \tcase \"$1\" in\n> \t-j)\n> -\t\tparallel=${1#-j} ;;\n> +\t\tparallel=$2; shift ;;\n> \t-f)\n> \t\tforce=t ;;\n> \t--)\n>\n>> Then \"script -j\" (no explicit number) would get here and autodetect.\n>> Running the script without any \"-j\" would also get an empty parallel\n>> and come here.\n>\n> Yeah, I think that is the wrong thing. If anything \"-j\" should behave\n> like \"make -j\". However, it looks like \"rev-parse --parseopt\" doesn't\n> play well with optional arguments for short-only options. You get \"-j\",\n> but then you have no idea whether the next argument is an optional value\n> for it, or another option entirely. Arguably it should give a blank\n> string or something (if you have long options, then it uses the\n> long-stock form, which is fine).\n>\n>> So \"script -j1\" would be how a user would say \"I want to use exactly\n>> one process, not any parallelism\", which makes sense.\n>\n> Right, that was the thing I actually wanted to have happen. :)\n>\n> -Peff\n"}]}