{"thread":{"id":"49259","subject":"[PATCH] doc-diff: always use oids inside worktree","startedAt":"2018-08-30T08:12:06Z","lastAt":"2018-09-18T17:38:14Z","messageCount":25,"participants":["Jeff King","Eric Sunshine","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"356950","messageId":"20180830081202.GA10224@sigill.intra.peff.net","threadId":"49259","inReplyTo":null,"subject":"[PATCH] doc-diff: always use oids inside worktree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T08:12:03Z","receivedAt":"2018-08-30T08:12:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The doc-diff script immediately resolves its two endpoints\nto actual object ids, so that we can reuse cached results\neven if they appear under a different name. But we still use\nthe original name the user fed us when running \"git\ncheckout\" in our temporary worktree. This can lead to\nconfusing results:\n\n - the namespace inside the worktree is different than the\n one outside. In particular, \"./doc-diff origin HEAD\"\n will resolve HEAD inside the worktree, whose detached\n HEAD will be pointing at origin! As a result, such a\n diff would always be empty.\n\n - worse, we will store this result under the oid we got by\n resolving HEAD in the main worktree, thus polluting our\n cache\n\n - we didn't pass --detach, which meant that using a branch\n name would cause us to actually check out that branch,\n making it unavailable to other worktrees.\n\nWe can solve this by feeding the already-resolved object id\nto git-checkout. That naturally forces a detached HEAD, but\njust to make clear our expectation, let's explicitly pass\n--detach.\n\nSigned-off-by: Jeff King \n---\nAnother fixup for jk/diff-rendered-docs, noticed when trying it out on\nEric's worktree series (which, btw, rendered as expected once I fixed\nthis bug ;) ).\n\n Documentation/doc-diff | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nindex 6e285e648c..c430fe7c99 100755\n--- a/Documentation/doc-diff\n+++ b/Documentation/doc-diff\n@@ -82,7 +82,7 @@ generate_render_makefile () {\n \tdone\n }\n \n-# render_tree \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@@ -92,7 +92,7 @@ render_tree () {\n \t# through.\n \tif ! test -d \"$tmp/installed/$1\"\n \tthen\n-\t\tgit -C \"$tmp/worktree\" checkout \"$2\" &&\n+\t\tgit -C \"$tmp/worktree\" checkout --detach \"$1\" &&\n \t\tmake -j$parallel -C \"$tmp/worktree\" \\\n \t\t\tGIT_VERSION=omitted \\\n \t\t\tSOURCE_DATE_EPOCH=0 \\\n@@ -112,6 +112,6 @@ render_tree () {\n \tfi\n }\n \n-render_tree $from_oid \"$from\" &&\n-render_tree $to_oid \"$to\" &&\n+render_tree $from_oid &&\n+render_tree $to_oid &&\n git -C $tmp/rendered diff --no-index \"$@\" $from_oid $to_oid\n-- \n2.19.0.rc1.9.gf8a776330b\n"},{"id":"356960","messageId":"CAPig+cRm80qBUXiykTtrHMxrMbzMLjYb3TvJB1UwoHNPFNxkkg@mail.gmail.com","threadId":"49259","inReplyTo":"20180830081202.GA10224@sigill.intra.peff.net","subject":"Re: [PATCH] doc-diff: always use oids inside worktree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-30T09:12:01Z","receivedAt":"2018-08-30T09:12:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 30, 2018 at 4:12 AM Jeff King wrote:\n> The doc-diff script immediately resolves its two endpoints\n> to actual object ids, so that we can reuse cached results\n> even if they appear under a different name. But we still use\n> the original name the user fed us when running \"git\n> checkout\" in our temporary worktree. This can lead to\n> confusing results:\n> [...]\n> - we didn't pass --detach, which meant that using a branch\n> name would cause us to actually check out that branch,\n> making it unavailable to other worktrees.\n\nOof. The initial worktree creation correctly uses --detach, but indeed\nthe later git-checkout doesn't. I missed that too when reading over\nthis script.\n\n> We can solve this by feeding the already-resolved object id\n> to git-checkout. That naturally forces a detached HEAD, but\n> just to make clear our expectation, let's explicitly pass\n> --detach.\n\nSpecifying --detach explicitly makes a lot of sense, even if it is\nimplied in this case.\n\n> Signed-off-by: Jeff King \n"},{"id":"357011","messageId":"20180830195546.GA22407@sigill.intra.peff.net","threadId":"49259","inReplyTo":"20180830081202.GA10224@sigill.intra.peff.net","subject":"[PATCH] doc/Makefile: remove tmp-doc-diff on \"make clean\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T19:55:46Z","receivedAt":"2018-08-30T19:55:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The tmp-doc-diff directory isn't strictly a build product of\nthe Makefile, since it's only present if you manually run\nthe doc-diff script. But anybody running \"make clean\" would\nprobably want it to go away.\n\nSuggested-by: Eric Sunshine \nSigned-off-by: Jeff King \n---\n> Another fixup for jk/diff-rendered-docs,[...]\n\nAnd here's one more. I don't have a strong opinion on this myself, but\nit seems sensible. I doubt anybody cares overly much about the cost of\nan extra $(RM) during \"make clean\" (and if they do, we really ought to\nconsider joining the existing ones into a single invocation).\n\n Documentation/Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex a42dcfc745..99a349ce9a 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -332,6 +332,7 @@ clean:\n \t$(RM) SubmittingPatches.txt\n \t$(RM) $(cmds_txt) $(mergetools_txt) *.made\n \t$(RM) manpage-base-url.xsl\n+\t$(RM) -r tmp-doc-diff\n \n $(MAN_HTML): %.html : %.txt asciidoc.conf\n \t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-- \n2.19.0.rc1.546.g3fcb3c0d7c\n\n\n"},{"id":"357023","messageId":"CAPig+cRXEegGwm3QiyEZqkVN1NYv5xAAYRuAKHUE8n-g9m6jqw@mail.gmail.com","threadId":"49259","inReplyTo":"20180830195546.GA22407@sigill.intra.peff.net","subject":"Re: [PATCH] doc/Makefile: remove tmp-doc-diff on \"make clean\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-30T20:34:43Z","receivedAt":"2018-08-30T20:34:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 30, 2018 at 3:55 PM Jeff King wrote:\n> The tmp-doc-diff directory isn't strictly a build product of\n> the Makefile, since it's only present if you manually run\n> the doc-diff script. But anybody running \"make clean\" would\n> probably want it to go away.\n>\n> Suggested-by: Eric Sunshine \n> Signed-off-by: Jeff King \n> ---\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> @@ -332,6 +332,7 @@ clean:\n> $(RM) manpage-base-url.xsl\n> + $(RM) -r tmp-doc-diff\n\nTaking into consideration that people might be surprised and alarmed\nto find \"git worktree list\" showing a worktree they didn't explicitly\ncreate, would it make sense to do something like this?\n\nclean:\n ...\n -git worktree remove -f tmp-doc-diff 2>/dev/null\n $(RM) -r tmp-doc-diff\n"},{"id":"357024","messageId":"CAPig+cSq=AikZU9X8hC9QM29vUA4bNKXLvf60dweJWyvFWY0EA@mail.gmail.com","threadId":"49259","inReplyTo":"CAPig+cRXEegGwm3QiyEZqkVN1NYv5xAAYRuAKHUE8n-g9m6jqw@mail.gmail.com","subject":"Re: [PATCH] doc/Makefile: remove tmp-doc-diff on \"make clean\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-30T20:36:40Z","receivedAt":"2018-08-30T20:36:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 30, 2018 at 4:34 PM Eric Sunshine wrote:\n> Taking into consideration that people might be surprised and alarmed\n> to find \"git worktree list\" showing a worktree they didn't explicitly\n> create, would it make sense to do something like this?\n>\n> clean:\n> ...\n> -git worktree remove -f tmp-doc-diff 2>/dev/null\n> $(RM) -r tmp-doc-diff\n\nMore accurately:\n\n -git worktree remove -f Documentation/tmp-doc-diff/worktree 2>/dev/null\n $(RM) -r tmp-doc-diff\n"},{"id":"357026","messageId":"20180830205208.GB27399@sigill.intra.peff.net","threadId":"49259","inReplyTo":"CAPig+cRXEegGwm3QiyEZqkVN1NYv5xAAYRuAKHUE8n-g9m6jqw@mail.gmail.com","subject":"Re: [PATCH] doc/Makefile: remove tmp-doc-diff on \"make clean\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-30T20:52:08Z","receivedAt":"2018-08-30T20:52:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 30, 2018 at 04:34:43PM -0400, Eric Sunshine wrote:\n\n> On Thu, Aug 30, 2018 at 3:55 PM Jeff King wrote:\n> > The tmp-doc-diff directory isn't strictly a build product of\n> > the Makefile, since it's only present if you manually run\n> > the doc-diff script. But anybody running \"make clean\" would\n> > probably want it to go away.\n> >\n> > Suggested-by: Eric Sunshine \n> > Signed-off-by: Jeff King \n> > ---\n> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n> > @@ -332,6 +332,7 @@ clean:\n> > $(RM) manpage-base-url.xsl\n> > + $(RM) -r tmp-doc-diff\n> \n> Taking into consideration that people might be surprised and alarmed\n> to find \"git worktree list\" showing a worktree they didn't explicitly\n> create, would it make sense to do something like this?\n> \n> clean:\n> ...\n> -git worktree remove -f tmp-doc-diff 2>/dev/null\n> $(RM) -r tmp-doc-diff\n\nSeems reasonable. Again, I don't have a strong feeling. It's a little\nstrange to me for the Makefile to be touching bits outside of the actual\nworking tree. But then, creating a separate worktree in the first place\nis perhaps a little weird.\n\nI dunno. Maybe you are right that worktrees are a bad fit here.\n\n-Peff\n"},{"id":"357066","messageId":"20180831063318.33373-1-sunshine@sunshineco.com","threadId":"49259","inReplyTo":"20180830195546.GA22407@sigill.intra.peff.net","subject":"[PATCH 0/3] doc-diff: add \"clean\" mode & fix portability problem","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-31T06:33:15Z","receivedAt":"2018-08-31T06:33:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This series replaces Peff's solo patch[1] which updates \"make clean\" to\nremove doc-diff's temporary directory. Rather than imbuing the Makefile\nwith knowledge specific to doc-diff's internals, this series adds a\n\"clean\" mode to doc-diff which removes its temporary worktree and\ngenerated files, and has \"make clean\" invoke that instead. It also fixes\na portability problem which prevented doc-diff from working on MacOS and\nFreeBSD.\n\n[1]: https://public-inbox.org/git/20180830195546.GA22407@sigill.intra.peff.net/\n\nEric Sunshine (3):\n doc-diff: fix non-portable 'man' invocation\n doc-diff: add --clean mode to remove temporary working gunk\n doc/Makefile: drop doc-diff worktree and temporary files on \"make\n clean\"\n\n Documentation/Makefile | 1 +\n Documentation/doc-diff | 21 +++++++++++++++++----\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\n-- \n2.19.0.rc1.352.gb1634b371d\n\n"},{"id":"357067","messageId":"20180831063318.33373-2-sunshine@sunshineco.com","threadId":"49259","inReplyTo":"20180831063318.33373-1-sunshine@sunshineco.com","subject":"[PATCH 1/3] doc-diff: fix non-portable 'man' invocation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-31T06:33:16Z","receivedAt":"2018-08-31T06:33:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"doc-diff invokes 'man' with the -l option to force \"local\" mode,\nhowever, neither MacOS nor FreeBSD recognize this option. On those\nplatforms, if the argument to 'man' contains a slash, it is\nautomatically interpreted as a file specification, so a \"local\"-like\nmode is not needed. And, it turns out, 'man' which does support -l\nfalls back to enabling -l automatically if it can't otherwise find a\nmanual entry corresponding to the argument. Since doc-diff always\npasses an absolute path of the nroff source file to 'man', the -l\noption kicks in anyhow, despite not being specified explicitly.\nTherefore, make the invocation portable to the various platforms by\nsimply dropping -l.\n\nSigned-off-by: Eric Sunshine \n---\n Documentation/doc-diff | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nindex f483fe427c..c2906eac5e 100755\n--- a/Documentation/doc-diff\n+++ b/Documentation/doc-diff\n@@ -69,7 +69,7 @@ generate_render_makefile () {\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 '\\tMANWIDTH=80 man $< >$@+ && \\\\\\n'\n \t\tprintf '\\tmv $@+ $@\\n'\n \tdone\n }\n-- \n2.19.0.rc1.352.gb1634b371d\n\n"},{"id":"357068","messageId":"20180831063318.33373-4-sunshine@sunshineco.com","threadId":"49259","inReplyTo":"20180831063318.33373-1-sunshine@sunshineco.com","subject":"[PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-31T06:33:18Z","receivedAt":"2018-08-31T06:33:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"doc-diff creates a temporary working tree (git-worktree) and generates a\nbunch of temporary files which it does not remove since they act as a\ncache to speed up subsequent runs. Although doc-diff's working tree and\ngenerated files are not strictly build products of the Makefile (which,\nitself, never runs doc-diff), as a convenience, update \"make clean\" to\nclean up doc-diff's working tree and generated files along with other\ndevelopment detritus normally removed by \"make clean\".\n\nSigned-off-by: Eric Sunshine \n---\n Documentation/Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex a42dcfc745..26e268ae8d 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -332,6 +332,7 @@ clean:\n \t$(RM) SubmittingPatches.txt\n \t$(RM) $(cmds_txt) $(mergetools_txt) *.made\n \t$(RM) manpage-base-url.xsl\n+\t'$(SHELL_PATH_SQ)' ./doc-diff --clean\n \n $(MAN_HTML): %.html : %.txt asciidoc.conf\n \t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-- \n2.19.0.rc1.352.gb1634b371d\n\n"},{"id":"357069","messageId":"20180831063318.33373-3-sunshine@sunshineco.com","threadId":"49259","inReplyTo":"20180831063318.33373-1-sunshine@sunshineco.com","subject":"[PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-31T06:33:17Z","receivedAt":"2018-08-31T06:33:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"As part of its operation, doc-diff creates a bunch of temporary\nworking files and holds onto them in order to speed up subsequent\ninvocations. These files are never deleted. Moreover, it creates a\ntemporary working tree (via git-wortkree) which likewise never gets\nremoved.\n\nWithout knowing the implementation details of the tool, a user may not\nknow how to clean up manually afterward. Worse, the user may find it\nsurprising and alarming to discover a working tree which s/he did not\ncreate explicitly.\n\nTo address these issues, add a --clean mode which removes the\ntemporary working tree and deletes all generated files.\n\nSigned-off-by: Eric Sunshine \n---\n Documentation/doc-diff | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/doc-diff b/Documentation/doc-diff\nindex c2906eac5e..f397fd229b 100755\n--- a/Documentation/doc-diff\n+++ b/Documentation/doc-diff\n@@ -2,20 +2,25 @@\n \n OPTIONS_SPEC=\"\\\n doc-diff [options] [-- ]\n+doc-diff (-c|--clean)\n --\n j=n\tparallel argument to pass to make\n f\tforce rebuild; do not rely on cached results\n+c,clean\tcleanup temporary working files\n \"\n SUBDIRECTORY_OK=1\n . \"$(git --exec-path)/git-sh-setup\"\n \n parallel=\n force=\n+clean=\n while test $# -gt 0\n do\n \tcase \"$1\" in\n \t-j)\n \t\tparallel=$2; shift ;;\n+\t-c|--clean)\n+\t\tclean=t ;;\n \t-f)\n \t\tforce=t ;;\n \t--)\n@@ -26,6 +31,17 @@ do\n \tshift\n done\n \n+cd_to_toplevel\n+tmp=Documentation/tmp-doc-diff\n+\n+if test -n \"$clean\"\n+then\n+\ttest $# -eq 0 || usage\n+\tgit worktree remove --force \"$tmp/worktree\" 2>/dev/null\n+\trm -rf \"$tmp\"\n+\texit 0\n+fi\n+\n if test -z \"$parallel\"\n then\n \tparallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null)\n@@ -42,9 +58,6 @@ to=$1; shift\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-- \n2.19.0.rc1.352.gb1634b371d\n\n"},{"id":"357110","messageId":"xmqqva7q1id3.fsf@gitster-ct.c.googlers.com","threadId":"49259","inReplyTo":"20180831063318.33373-1-sunshine@sunshineco.com","subject":"Re: [PATCH 0/3] doc-diff: add \"clean\" mode & fix portability problem","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-31T18:49:44Z","receivedAt":"2018-08-31T18:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> This series replaces Peff's solo patch[1] which updates \"make clean\" to\n> remove doc-diff's temporary directory. Rather than imbuing the Makefile\n> with knowledge specific to doc-diff's internals, this series adds a\n> \"clean\" mode to doc-diff which removes its temporary worktree and\n> generated files, and has \"make clean\" invoke that instead.\n\nThat sounds like a better approach.\n\n> It also fixes\n> a portability problem which prevented doc-diff from working on MacOS and\n> FreeBSD.\n>\n> [1]: https://public-inbox.org/git/20180830195546.GA22407@sigill.intra.peff.net/\n>\n> Eric Sunshine (3):\n> doc-diff: fix non-portable 'man' invocation\n> doc-diff: add --clean mode to remove temporary working gunk\n> doc/Makefile: drop doc-diff worktree and temporary files on \"make\n> clean\"\n>\n> Documentation/Makefile | 1 +\n> Documentation/doc-diff | 21 +++++++++++++++++----\n> 2 files changed, 18 insertions(+), 4 deletions(-)\n"},{"id":"357114","messageId":"20180831194927.GB5120@sigill.intra.peff.net","threadId":"49259","inReplyTo":"20180831063318.33373-2-sunshine@sunshineco.com","subject":"Re: [PATCH 1/3] doc-diff: fix non-portable 'man' invocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-31T19:49:27Z","receivedAt":"2018-08-31T19:49:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 02:33:16AM -0400, Eric Sunshine wrote:\n\n> doc-diff invokes 'man' with the -l option to force \"local\" mode,\n> however, neither MacOS nor FreeBSD recognize this option. On those\n> platforms, if the argument to 'man' contains a slash, it is\n> automatically interpreted as a file specification, so a \"local\"-like\n> mode is not needed. And, it turns out, 'man' which does support -l\n> falls back to enabling -l automatically if it can't otherwise find a\n> manual entry corresponding to the argument. Since doc-diff always\n> passes an absolute path of the nroff source file to 'man', the -l\n> option kicks in anyhow, despite not being specified explicitly.\n> Therefore, make the invocation portable to the various platforms by\n> simply dropping -l.\n\nNeat. Today I learned.\n\nConfirmed that this works just fine without \"-l\" on my system (and that\n\"./foo.1\" is an easy alternative to \"man -l\" on other systems).\n\n-Peff\n"},{"id":"357116","messageId":"20180831200135.GC5120@sigill.intra.peff.net","threadId":"49259","inReplyTo":"20180831063318.33373-3-sunshine@sunshineco.com","subject":"Re: [PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-31T20:01:35Z","receivedAt":"2018-08-31T20:01:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 02:33:17AM -0400, Eric Sunshine wrote:\n\n> As part of its operation, doc-diff creates a bunch of temporary\n> working files and holds onto them in order to speed up subsequent\n> invocations. These files are never deleted. Moreover, it creates a\n> temporary working tree (via git-wortkree) which likewise never gets\n> removed.\n> \n> Without knowing the implementation details of the tool, a user may not\n> know how to clean up manually afterward. Worse, the user may find it\n> surprising and alarming to discover a working tree which s/he did not\n> create explicitly.\n> \n> To address these issues, add a --clean mode which removes the\n> temporary working tree and deletes all generated files.\n\nThat sounds like a good plan. I like keeping the complexity here in the\nscript.\n\n> diff --git a/Documentation/doc-diff b/Documentation/doc-diff\n> index c2906eac5e..f397fd229b 100755\n> --- a/Documentation/doc-diff\n> +++ b/Documentation/doc-diff\n> @@ -2,20 +2,25 @@\n> \n> OPTIONS_SPEC=\"\\\n> doc-diff [options] [-- ]\n> +doc-diff (-c|--clean)\n> --\n> j=n\tparallel argument to pass to make\n> f\tforce rebuild; do not rely on cached results\n> +c,clean\tcleanup temporary working files\n> \"\n\nThis will cause parseopt to normalize \"--clean\" to \"-c\" (along with\n\"--cle\", etc).\n\n> parallel=\n> force=\n> +clean=\n> while test $# -gt 0\n> do\n> \tcase \"$1\" in\n> \t-j)\n> \t\tparallel=$2; shift ;;\n> +\t-c|--clean)\n> +\t\tclean=t ;;\n\nSo this part can just test for \"-c\". AFAICT this is how \"rev-parse\n--parseopt\" has always worked, though the documentation is quite\nunclear. Other scripts seem to also use these redundant long options.\nI'm not opposed to including it as a defensive measure (or simply an\nannotation for the reader).\n\n> +cd_to_toplevel\n> +tmp=Documentation/tmp-doc-diff\n> +\n> +if test -n \"$clean\"\n> +then\n> +\ttest $# -eq 0 || usage\n> +\tgit worktree remove --force \"$tmp/worktree\" 2>/dev/null\n> +\trm -rf \"$tmp\"\n> +\texit 0\n> +fi\n\nAnd this matches what I'd expect \"--clean\" to do. Makes sense.\n\n-Peff\n"},{"id":"357117","messageId":"20180831200748.GD5120@sigill.intra.peff.net","threadId":"49259","inReplyTo":"20180831063318.33373-4-sunshine@sunshineco.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-31T20:07:48Z","receivedAt":"2018-08-31T20:07:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote:\n\n> doc-diff creates a temporary working tree (git-worktree) and generates a\n> bunch of temporary files which it does not remove since they act as a\n> cache to speed up subsequent runs. Although doc-diff's working tree and\n> generated files are not strictly build products of the Makefile (which,\n> itself, never runs doc-diff), as a convenience, update \"make clean\" to\n> clean up doc-diff's working tree and generated files along with other\n> development detritus normally removed by \"make clean\".\n\nMakes sense.\n\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index a42dcfc745..26e268ae8d 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -332,6 +332,7 @@ clean:\n> \t$(RM) SubmittingPatches.txt\n> \t$(RM) $(cmds_txt) $(mergetools_txt) *.made\n> \t$(RM) manpage-base-url.xsl\n> +\t'$(SHELL_PATH_SQ)' ./doc-diff --clean\n\nThis spelling took me by surprise. The doc-diff script itself specifies\n/bin/sh, and we do not build it, so the #! line is never replaced. I\nguess we are leaving it to people on exotic shells to run \"$their_sh\ndoc-diff\" in the first place. That's probably OK, since it should work\nout of the box on most /bin/sh instances, and people on other platforms\naren't that likely to even run it.\n\nI don't think the script does anything complicated that would choke a\nlesser /bin/sh. But it doesn't hurt to be defensive, since this bit of\nthe Makefile will be run for everyone, whether they care about doc-diff\nor not.\n\nSo that all makes sense. I initially wrote this to suggest that we call\nout this subtlety in the commit message. But I see this is based on\nexisting instances from ee7ec2f9de (documentation: Makefile accounts for\nSHELL_PATH setting, 2009-03-22). So maybe I am just showing my\nignorance. ;)\n\n-Peff\n"},{"id":"357118","messageId":"20180831200819.GE5120@sigill.intra.peff.net","threadId":"49259","inReplyTo":"20180831063318.33373-1-sunshine@sunshineco.com","subject":"Re: [PATCH 0/3] doc-diff: add \"clean\" mode & fix portability problem","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-31T20:08:20Z","receivedAt":"2018-08-31T20:08:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 31, 2018 at 02:33:15AM -0400, Eric Sunshine wrote:\n\n> This series replaces Peff's solo patch[1] which updates \"make clean\" to\n> remove doc-diff's temporary directory. Rather than imbuing the Makefile\n> with knowledge specific to doc-diff's internals, this series adds a\n> \"clean\" mode to doc-diff which removes its temporary worktree and\n> generated files, and has \"make clean\" invoke that instead. It also fixes\n> a portability problem which prevented doc-diff from working on MacOS and\n> FreeBSD.\n\nThanks. I left a few comments, but this looks good to me as-is.\n\n-Peff\n"},{"id":"357134","messageId":"CAPig+cRoOYZaDU-bPeCpabLVY3MiZwyE-Afc2AMZk7ksyj3tTQ@mail.gmail.com","threadId":"49259","inReplyTo":"20180831200135.GC5120@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-31T21:24:21Z","receivedAt":"2018-08-31T21:24:36Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 31, 2018 at 4:01 PM Jeff King wrote:\n> On Fri, Aug 31, 2018 at 02:33:17AM -0400, Eric Sunshine wrote:\n> > OPTIONS_SPEC=\"\\\n> > doc-diff [options] [-- ]\n> > +doc-diff (-c|--clean)\n> > --\n> > j=n parallel argument to pass to make\n> > f force rebuild; do not rely on cached results\n> > +c,clean cleanup temporary working files\n> > \"\n>\n> This will cause parseopt to normalize \"--clean\" to \"-c\" (along with\n> \"--cle\", etc).\n\nGood to know. The documentation for git-sh-setup didn't talk about\nthat at all, and while git-rev-parse documentation says that it\n\"normalizes\" options, that word didn't really convey this specific\nmeaning to me, so I missed it.\n\n> > while test $# -gt 0\n> > do\n> > case \"$1\" in\n> > -j)\n> > parallel=$2; shift ;;\n> > + -c|--clean)\n> > + clean=t ;;\n>\n> So this part can just test for \"-c\". AFAICT this is how \"rev-parse\n> --parseopt\" has always worked, though the documentation is quite\n> unclear. Other scripts seem to also use these redundant long options.\n> I'm not opposed to including it as a defensive measure (or simply an\n> annotation for the reader).\n\nI'm fine leaving it as-is too since it seems that every other client\nof git-sh-setup does the same (and to save a re-roll).\n"},{"id":"357135","messageId":"CAPig+cSktNnaWBew02Di5BaCQAqP6kXtMyoC3=g1A9hZcT-BEw@mail.gmail.com","threadId":"49259","inReplyTo":"20180831200748.GD5120@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-08-31T21:37:33Z","receivedAt":"2018-08-31T21:37:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 31, 2018 at 4:07 PM Jeff King wrote:\n> On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote:\n> > diff --git a/Documentation/Makefile b/Documentation/Makefile\n> > @@ -332,6 +332,7 @@ clean:\n> > $(RM) manpage-base-url.xsl\n> > + '$(SHELL_PATH_SQ)' ./doc-diff --clean\n>\n> This spelling took me by surprise. The doc-diff script itself specifies\n> /bin/sh, and we do not build it, so the #! line is never replaced. [...]\n>\n> I don't think the script does anything complicated that would choke a\n> lesser /bin/sh. But it doesn't hurt to be defensive, since this bit of\n> the Makefile will be run for everyone, whether they care about doc-diff\n> or not.\n>\n> So that all makes sense. I initially wrote this to suggest that we call\n> out this subtlety in the commit message. But I see this is based on\n> existing instances from ee7ec2f9de (documentation: Makefile accounts for\n> SHELL_PATH setting, 2009-03-22). So maybe I am just showing my\n> ignorance. ;)\n\nCorrect. I was concerned that invoking it simply as \"./doc-diff\n--clean\" could be problematic, so, knowing that the Makefile invoked\nother scripts in Documentation/, I mirrored their invocation. If it\ndidn't follow existing practice of invoking the command with\n$(SHELL_PATH_SQ), then that would merit mention in the commit message,\nbut as it is, the commit message is probably fine.\n\nThanks for the review.\n"},{"id":"358310","messageId":"20180917183258.GC140909@aiede.svl.corp.google.com","threadId":"49259","inReplyTo":"20180831063318.33373-4-sunshine@sunshineco.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-17T18:32:58Z","receivedAt":"2018-09-17T18:33:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nEric Sunshine wrote:\n\n> doc-diff creates a temporary working tree (git-worktree) and generates a\n> bunch of temporary files which it does not remove since they act as a\n> cache to speed up subsequent runs. Although doc-diff's working tree and\n> generated files are not strictly build products of the Makefile (which,\n> itself, never runs doc-diff), as a convenience, update \"make clean\" to\n> clean up doc-diff's working tree and generated files along with other\n> development detritus normally removed by \"make clean\".\n>\n> Signed-off-by: Eric Sunshine \n> ---\n> Documentation/Makefile | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index a42dcfc745..26e268ae8d 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -332,6 +332,7 @@ clean:\n> \t$(RM) SubmittingPatches.txt\n> \t$(RM) $(cmds_txt) $(mergetools_txt) *.made\n> \t$(RM) manpage-base-url.xsl\n> +\t'$(SHELL_PATH_SQ)' ./doc-diff --clean\n\nThis means I need a copy of git in order to run \"make clean\". That\nwas never required before. It makes bootstrapping difficult --- do we\nreally need it?\n\nThanks,\nJonathan\n"},{"id":"358324","messageId":"xmqq36u77w7y.fsf@gitster-ct.c.googlers.com","threadId":"49259","inReplyTo":"20180917183258.GC140909@aiede.svl.corp.google.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-17T19:36:01Z","receivedAt":"2018-09-17T19:36:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> +\t'$(SHELL_PATH_SQ)' ./doc-diff --clean\n>\n> This means I need a copy of git in order to run \"make clean\". That\n> was never required before. It makes bootstrapping difficult --- do we\n> really need it?\n\nGahh, you are absolutely right. Also \"doc-diff --clean\", if I am\nreading the code correctly, requires us to be in a Git repository,\nnot a tarball extract.\n\nHaving to have Git installed, or be in a repository, in order to be\nable to run an optional \"doc-diff\" tool is fine. Requiring either\nin order to run \"make clean\" is a different story.\n\nThanks for spotting. We can just prefix the line with '-'? Or does\nthe script badly misbehave (due to lack of CEILING_DIRECTORY) when\nrun in a tarball extract inside somebody else's repository?\n"},{"id":"358325","messageId":"20180917194214.GG140909@aiede.svl.corp.google.com","threadId":"49259","inReplyTo":"xmqq36u77w7y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-17T19:42:14Z","receivedAt":"2018-09-17T19:42:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>> Eric Sunshine wrote:\n\n>>> +\t'$(SHELL_PATH_SQ)' ./doc-diff --clean\n>>\n>> This means I need a copy of git in order to run \"make clean\". That\n>> was never required before. It makes bootstrapping difficult --- do we\n>> really need it?\n>\n> Gahh, you are absolutely right. Also \"doc-diff --clean\", if I am\n> reading the code correctly, requires us to be in a Git repository,\n> not a tarball extract.\n>\n> Having to have Git installed, or be in a repository, in order to be\n> able to run an optional \"doc-diff\" tool is fine. Requiring either\n> in order to run \"make clean\" is a different story.\n>\n> Thanks for spotting. We can just prefix the line with '-'? Or does\n> the script badly misbehave (due to lack of CEILING_DIRECTORY) when\n> run in a tarball extract inside somebody else's repository?\n\nI'd rather that we revert this change altogether. I have nothing\nagainst a convenient command to do this kind of non build related\ncleanup, but it shouldn't be spelled as \"make clean\".\n\nThat said, for my particular use, prefixing with '-' would work okay.\n\nThanks,\nJonathan\n"},{"id":"358326","messageId":"xmqqy3bz6h24.fsf@gitster-ct.c.googlers.com","threadId":"49259","inReplyTo":"20180917194214.GG140909@aiede.svl.corp.google.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-17T19:48:51Z","receivedAt":"2018-09-17T19:48:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I'd rather that we revert this change altogether. I have nothing\n> against a convenient command to do this kind of non build related\n> cleanup, but it shouldn't be spelled as \"make clean\".\n\nOK, let's do this for now as I wanted to merge the remainder to\n'master' today.\n\n-- >8 --\nSubject: Revert \"doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"\"\n\nThis reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which\nstarted to require that we have an executable git available in order\nto say \"make clean\", which gives us a chicken-and-egg problem.\n\nHaving to have Git installed, or be in a repository, in order to be\nable to run an optional \"doc-diff\" tool is fine. Requiring either\nin order to run \"make clean\" is a different story.\n\nReported by Jonathan Nieder .\n---\n Documentation/Makefile | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 623f1a866d..d079d7c73a 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -331,7 +331,6 @@ clean:\n \t$(RM) SubmittingPatches.txt\n \t$(RM) $(cmds_txt) $(mergetools_txt) *.made\n \t$(RM) manpage-base-url.xsl\n-\t'$(SHELL_PATH_SQ)' ./doc-diff --clean\n \n $(MAN_HTML): %.html : %.txt asciidoc.conf\n \t$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \\\n-- \n2.19.0\n\n"},{"id":"358328","messageId":"20180917195526.GH140909@aiede.svl.corp.google.com","threadId":"49259","inReplyTo":"xmqqy3bz6h24.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-17T19:55:26Z","receivedAt":"2018-09-17T19:55:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> I'd rather that we revert this change altogether. I have nothing\n>> against a convenient command to do this kind of non build related\n>> cleanup, but it shouldn't be spelled as \"make clean\".\n>\n> OK, let's do this for now as I wanted to merge the remainder to\n> 'master' today.\n>\n> -- >8 --\n> Subject: Revert \"doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"\"\n>\n> This reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which\n> started to require that we have an executable git available in order\n> to say \"make clean\", which gives us a chicken-and-egg problem.\n>\n> Having to have Git installed, or be in a repository, in order to be\n> able to run an optional \"doc-diff\" tool is fine. Requiring either\n> in order to run \"make clean\" is a different story.\n>\n> Reported by Jonathan Nieder .\n> ---\n> Documentation/Makefile | 1 -\n> 1 file changed, 1 deletion(-)\n\nDoes this want a sign-off?\n\nIn any event, this matches what I had applied in Debian[1] and is\n\nReviewed-by: Jonathan Nieder \n\nThanks,\nJonathan\n\n[1] http://repo.or.cz/git/debian.git/blob/refs/heads/debian-experimental:/debian/patches/0002-Revert-doc-Makefile-drop-doc-diff-worktree-and-tempor.diff\naka http://repo.or.cz/git/debian.git/blob/9872ab1d87634a9288266de290571928e5b9346f:/debian/patches/0002-Revert-doc-Makefile-drop-doc-diff-worktree-and-tempor.diff\n"},{"id":"358331","messageId":"CAPig+cSrvvWn1rd6TcG8H6tGvGDvfNJZE3KnxYazwDTfxAkVeg@mail.gmail.com","threadId":"49259","inReplyTo":"xmqq36u77w7y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-17T20:43:13Z","receivedAt":"2018-09-17T20:43:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 17, 2018 at 3:36 PM Junio C Hamano wrote:\n> Jonathan Nieder writes:\n> >> + '$(SHELL_PATH_SQ)' ./doc-diff --clean\n> >\n> > This means I need a copy of git in order to run \"make clean\". That\n> > was never required before. It makes bootstrapping difficult --- do we\n> > really need it?\n>\n> Gahh, you are absolutely right. Also \"doc-diff --clean\", if I am\n> reading the code correctly, requires us to be in a Git repository,\n> not a tarball extract.\n>\n> Having to have Git installed, or be in a repository, in order to be\n> able to run an optional \"doc-diff\" tool is fine. Requiring either\n> in order to run \"make clean\" is a different story.\n>\n> Thanks for spotting. We can just prefix the line with '-'? Or does\n> the script badly misbehave (due to lack of CEILING_DIRECTORY) when\n> run in a tarball extract inside somebody else's repository?\n\nI did consider this case and felt that it would be reasonable for it\nto error out and ignore the error if git was missing or if the\ndirectory was not a repository. And, I _thought_ I had prefixed the\nline with \"-\" to handle just such a case, but apparently I botched it.\nOh well.\n"},{"id":"358334","messageId":"CAPig+cTmfnXPX-5Ffy1TYfhVvvGsNe0j6XR3a4N2+z0_L-4aKA@mail.gmail.com","threadId":"49259","inReplyTo":"CAPig+cSrvvWn1rd6TcG8H6tGvGDvfNJZE3KnxYazwDTfxAkVeg@mail.gmail.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-09-17T20:52:02Z","receivedAt":"2018-09-17T20:52:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Sep 17, 2018 at 4:43 PM Eric Sunshine wrote:\n> I did consider this case and felt that it would be reasonable for it\n> to error out and ignore the error if git was missing or if the\n> directory was not a repository. And, I _thought_ I had prefixed the\n> line with \"-\" to handle just such a case, but apparently I botched it.\n> Oh well.\n\nDredging up from memory, I think the omission of \"-\" from the Makefile\nline was intentional since I specifically handled the case of missing\n\"git\" command in the script itself by ignoring any error from it.\nSpecifically, this excerpt from doc-diff:\n\n if test -n \"$clean\"\n then\n test $# -eq 0 || usage\n git worktree remove --force \"$tmp/worktree\" 2>/dev/null\n rm -rf \"$tmp\"\n exit 0\n fi\n\nin which a problem invoking git is explicitly ignored and the script\nexits cleanly, so no Makefile \"-\" is needed.\n\nUnfortunately, I forgot about the:\n\n . \"$(git --exec-path)/git-sh-setup\"\n\nwhich happens earlier in the script.\n"},{"id":"358406","messageId":"20180918173810.GF15470@sigill.intra.peff.net","threadId":"49259","inReplyTo":"xmqqy3bz6h24.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-18T17:38:10Z","receivedAt":"2018-09-18T17:38:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 17, 2018 at 12:48:51PM -0700, Junio C Hamano wrote:\n\n> Jonathan Nieder writes:\n> \n> > I'd rather that we revert this change altogether. I have nothing\n> > against a convenient command to do this kind of non build related\n> > cleanup, but it shouldn't be spelled as \"make clean\".\n> \n> OK, let's do this for now as I wanted to merge the remainder to\n> 'master' today.\n> \n> -- >8 --\n> Subject: Revert \"doc/Makefile: drop doc-diff worktree and temporary files on \"make clean\"\"\n> \n> This reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which\n> started to require that we have an executable git available in order\n> to say \"make clean\", which gives us a chicken-and-egg problem.\n> \n> Having to have Git installed, or be in a repository, in order to be\n> able to run an optional \"doc-diff\" tool is fine. Requiring either\n> in order to run \"make clean\" is a different story.\n\nYeah, this seems like the best solution. We started with \"can we just rm\n-rf the temporary directory as part of 'make clean'\", which is totally\nsensible and matches the other bits there. But then it got more\ncomplicated. :)\n\nPeople who use doc-diff can still use \"doc-diff --clean\", so I don't\nthink much is lost.\n\nThanks.\n\n-Peff\n"}]}