[PATCH] doc-diff: always use oids inside worktree

The doc-diff script immediately resolves its two endpoints
to actual object ids, so that we can reuse cached results
even if they appear under a different name. But we still use
the original name the user fed us when running "git
checkout" in our temporary worktree. This can lead to
confusing results:

  - the namespace inside the worktree is different than the
    one outside. In particular, "./doc-diff origin HEAD"
    will resolve HEAD inside the worktree, whose detached
    HEAD will be pointing at origin! As a result, such a
    diff would always be empty.

  - worse, we will store this result under the oid we got by
    resolving HEAD in the main worktree, thus polluting our
    cache

  - we didn't pass --detach, which meant that using a branch
    name would cause us to actually check out that branch,
    making it unavailable to other worktrees.

We can solve this by feeding the already-resolved object id
to git-checkout. That naturally forces a detached HEAD, but
just to make clear our expectation, let's explicitly pass
--detach.

Signed-off-by: Jeff King <redacted>
---
Another fixup for jk/diff-rendered-docs, noticed when trying it out on
Eric's worktree series (which, btw, rendered as expected once I fixed
this bug ;) ).

 Documentation/doc-diff | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index 6e285e648c..c430fe7c99 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff

@@ -82,7 +82,7 @@ generate_render_makefile () {
 	done
 }
 
-# render_tree <dirname> <committish>
+# render_tree <committish_oid>
 render_tree () {
 	# Skip install-man entirely if we already have an installed directory.
 	# We can't rely on make here, since "install-man" unconditionally

@@ -92,7 +92,7 @@ render_tree () {
 	# through.
 	if ! test -d "$tmp/installed/$1"
 	then
-		git -C "$tmp/worktree" checkout "$2" &&
+		git -C "$tmp/worktree" checkout --detach "$1" &&
 		make -j$parallel -C "$tmp/worktree" \
 			GIT_VERSION=omitted \
 			SOURCE_DATE_EPOCH=0 \

@@ -112,6 +112,6 @@ render_tree () {
 	fi
 }
 
-render_tree $from_oid "$from" &&
-render_tree $to_oid "$to" &&
+render_tree $from_oid &&
+render_tree $to_oid &&
 git -C $tmp/rendered diff --no-index "$@" $from_oid $to_oid

-- 
2.19.0.rc1.9.gf8a776330b

On Thu, Aug 30, 2018 at 4:12 AM Jeff King [off-list ref] wrote:

The doc-diff script immediately resolves its two endpoints
to actual object ids, so that we can reuse cached results
even if they appear under a different name. But we still use
the original name the user fed us when running "git
checkout" in our temporary worktree. This can lead to
confusing results:
[...]
  - we didn't pass --detach, which meant that using a branch
    name would cause us to actually check out that branch,
    making it unavailable to other worktrees.

Oof. The initial worktree creation correctly uses --detach, but indeed
the later git-checkout doesn't. I missed that too when reading over
this script.

We can solve this by feeding the already-resolved object id
to git-checkout. That naturally forces a detached HEAD, but
just to make clear our expectation, let's explicitly pass
--detach.

Specifying --detach explicitly makes a lot of sense, even if it is
implied in this case.

Signed-off-by: Jeff King <redacted>

The tmp-doc-diff directory isn't strictly a build product of
the Makefile, since it's only present if you manually run
the doc-diff script.  But anybody running "make clean" would
probably want it to go away.

Suggested-by: Eric Sunshine <redacted>
Signed-off-by: Jeff King <redacted>
---

Another fixup for jk/diff-rendered-docs,[...]

And here's one more. I don't have a strong opinion on this myself, but
it seems sensible. I doubt anybody cares overly much about the cost of
an extra $(RM) during "make clean" (and if they do, we really ought to
consider joining the existing ones into a single invocation).

 Documentation/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index a42dcfc745..99a349ce9a 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile

@@ -332,6 +332,7 @@ clean:
 	$(RM) SubmittingPatches.txt
 	$(RM) $(cmds_txt) $(mergetools_txt) *.made
 	$(RM) manpage-base-url.xsl
+	$(RM) -r tmp-doc-diff
 
 $(MAN_HTML): %.html : %.txt asciidoc.conf
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \

-- 
2.19.0.rc1.546.g3fcb3c0d7c

On Thu, Aug 30, 2018 at 3:55 PM Jeff King [off-list ref] wrote:

Taking into consideration that people might be surprised and alarmed
to find "git worktree list" showing a worktree they didn't explicitly
create, would it make sense to do something like this?

clean:
    ...
    -git worktree remove -f tmp-doc-diff 2>/dev/null
    $(RM) -r tmp-doc-diff

On Thu, Aug 30, 2018 at 4:34 PM Eric Sunshine [off-list ref] wrote:

Taking into consideration that people might be surprised and alarmed
to find "git worktree list" showing a worktree they didn't explicitly
create, would it make sense to do something like this?

clean:
    ...
    -git worktree remove -f tmp-doc-diff 2>/dev/null
    $(RM) -r tmp-doc-diff

More accurately:

    -git worktree remove -f Documentation/tmp-doc-diff/worktree 2>/dev/null
    $(RM) -r tmp-doc-diff

On Thu, Aug 30, 2018 at 04:34:43PM -0400, Eric Sunshine wrote:

On Thu, Aug 30, 2018 at 3:55 PM Jeff King [off-list ref] wrote:

quoted

The tmp-doc-diff directory isn't strictly a build product of
the Makefile, since it's only present if you manually run
the doc-diff script.  But anybody running "make clean" would
probably want it to go away.

Suggested-by: Eric Sunshine <redacted>
Signed-off-by: Jeff King <redacted>
---

diff --git a/Documentation/Makefile b/Documentation/Makefile

@@ -332,6 +332,7 @@ clean:
        $(RM) manpage-base-url.xsl
+       $(RM) -r tmp-doc-diff

Taking into consideration that people might be surprised and alarmed
to find "git worktree list" showing a worktree they didn't explicitly
create, would it make sense to do something like this?

clean:
    ...
    -git worktree remove -f tmp-doc-diff 2>/dev/null
    $(RM) -r tmp-doc-diff

Seems reasonable. Again, I don't have a strong feeling. It's a little
strange to me for the Makefile to be touching bits outside of the actual
working tree. But then, creating a separate worktree in the first place
is perhaps a little weird.

I dunno. Maybe you are right that worktrees are a bad fit here.

-Peff

This series replaces Peff's solo patch[1] which updates "make clean" to
remove doc-diff's temporary directory. Rather than imbuing the Makefile
with knowledge specific to doc-diff's internals, this series adds a
"clean" mode to doc-diff which removes its temporary worktree and
generated files, and has "make clean" invoke that instead. It also fixes
a portability problem which prevented doc-diff from working on MacOS and
FreeBSD.

[1]: https://public-inbox.org/git/20180830195546.GA22407@sigill.intra.peff.net/

Eric Sunshine (3):
  doc-diff: fix non-portable 'man' invocation
  doc-diff: add --clean mode to remove temporary working gunk
  doc/Makefile: drop doc-diff worktree and temporary files on "make
    clean"

 Documentation/Makefile |  1 +
 Documentation/doc-diff | 21 +++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.19.0.rc1.352.gb1634b371d

doc-diff invokes 'man' with the -l option to force "local" mode,
however, neither MacOS nor FreeBSD recognize this option. On those
platforms, if the argument to 'man' contains a slash, it is
automatically interpreted as a file specification, so a "local"-like
mode is not needed. And, it turns out, 'man' which does support -l
falls back to enabling -l automatically if it can't otherwise find a
manual entry corresponding to the argument. Since doc-diff always
passes an absolute path of the nroff source file to 'man', the -l
option kicks in anyhow, despite not being specified explicitly.
Therefore, make the invocation portable to the various platforms by
simply dropping -l.

Signed-off-by: Eric Sunshine <redacted>
---
 Documentation/doc-diff | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index f483fe427c..c2906eac5e 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff

@@ -69,7 +69,7 @@ generate_render_makefile () {
 		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 '\tMANWIDTH=80 man $< >$@+ && \\\n'
 		printf '\tmv $@+ $@\n'
 	done
 }

-- 
2.19.0.rc1.352.gb1634b371d

doc-diff creates a temporary working tree (git-worktree) and generates a
bunch of temporary files which it does not remove since they act as a
cache to speed up subsequent runs. Although doc-diff's working tree and
generated files are not strictly build products of the Makefile (which,
itself, never runs doc-diff), as a convenience, update "make clean" to
clean up doc-diff's working tree and generated files along with other
development detritus normally removed by "make clean".

Signed-off-by: Eric Sunshine <redacted>
---
 Documentation/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index a42dcfc745..26e268ae8d 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile

@@ -332,6 +332,7 @@ clean:
 	$(RM) SubmittingPatches.txt
 	$(RM) $(cmds_txt) $(mergetools_txt) *.made
 	$(RM) manpage-base-url.xsl
+	'$(SHELL_PATH_SQ)' ./doc-diff --clean
 
 $(MAN_HTML): %.html : %.txt asciidoc.conf
 	$(QUIET_ASCIIDOC)$(RM) $@+ $@ && \

-- 
2.19.0.rc1.352.gb1634b371d

As part of its operation, doc-diff creates a bunch of temporary
working files and holds onto them in order to speed up subsequent
invocations. These files are never deleted. Moreover, it creates a
temporary working tree (via git-wortkree) which likewise never gets
removed.

Without knowing the implementation details of the tool, a user may not
know how to clean up manually afterward. Worse, the user may find it
surprising and alarming to discover a working tree which s/he did not
create explicitly.

To address these issues, add a --clean mode which removes the
temporary working tree and deletes all generated files.

Signed-off-by: Eric Sunshine <redacted>
---
 Documentation/doc-diff | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index c2906eac5e..f397fd229b 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff

@@ -2,20 +2,25 @@
 
 OPTIONS_SPEC="\
 doc-diff [options] <from> <to> [-- <diff-options>]
+doc-diff (-c|--clean)
 --
 j=n	parallel argument to pass to make
 f	force rebuild; do not rely on cached results
+c,clean	cleanup temporary working files
 "
 SUBDIRECTORY_OK=1
 . "$(git --exec-path)/git-sh-setup"
 
 parallel=
 force=
+clean=
 while test $# -gt 0
 do
 	case "$1" in
 	-j)
 		parallel=$2; shift ;;
+	-c|--clean)
+		clean=t ;;
 	-f)
 		force=t ;;
 	--)

@@ -26,6 +31,17 @@ do
 	shift
 done
 
+cd_to_toplevel
+tmp=Documentation/tmp-doc-diff
+
+if test -n "$clean"
+then
+	test $# -eq 0 || usage
+	git worktree remove --force "$tmp/worktree" 2>/dev/null
+	rm -rf "$tmp"
+	exit 0
+fi
+
 if test -z "$parallel"
 then
 	parallel=$(getconf _NPROCESSORS_ONLN 2>/dev/null)

@@ -42,9 +58,6 @@ 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"

-- 
2.19.0.rc1.352.gb1634b371d

On Fri, Aug 31, 2018 at 02:33:16AM -0400, Eric Sunshine wrote:

doc-diff invokes 'man' with the -l option to force "local" mode,
however, neither MacOS nor FreeBSD recognize this option. On those
platforms, if the argument to 'man' contains a slash, it is
automatically interpreted as a file specification, so a "local"-like
mode is not needed. And, it turns out, 'man' which does support -l
falls back to enabling -l automatically if it can't otherwise find a
manual entry corresponding to the argument. Since doc-diff always
passes an absolute path of the nroff source file to 'man', the -l
option kicks in anyhow, despite not being specified explicitly.
Therefore, make the invocation portable to the various platforms by
simply dropping -l.

Neat. Today I learned.

Confirmed that this works just fine without "-l" on my system (and that
"./foo.1" is an easy alternative to "man -l" on other systems).

-Peff

On Fri, Aug 31, 2018 at 02:33:17AM -0400, Eric Sunshine wrote:

As part of its operation, doc-diff creates a bunch of temporary
working files and holds onto them in order to speed up subsequent
invocations. These files are never deleted. Moreover, it creates a
temporary working tree (via git-wortkree) which likewise never gets
removed.

Without knowing the implementation details of the tool, a user may not
know how to clean up manually afterward. Worse, the user may find it
surprising and alarming to discover a working tree which s/he did not
create explicitly.

To address these issues, add a --clean mode which removes the
temporary working tree and deletes all generated files.

That sounds like a good plan. I like keeping the complexity here in the
script.

This will cause parseopt to normalize "--clean" to "-c" (along with
"--cle", etc).

 parallel=
 force=
+clean=
 while test $# -gt 0
 do
 	case "$1" in
 	-j)
 		parallel=$2; shift ;;
+	-c|--clean)
+		clean=t ;;

So this part can just test for "-c". AFAICT this is how "rev-parse
--parseopt" has always worked, though the documentation is quite
unclear. Other scripts seem to also use these redundant long options.
I'm not opposed to including it as a defensive measure (or simply an
annotation for the reader).

+cd_to_toplevel
+tmp=Documentation/tmp-doc-diff
+
+if test -n "$clean"
+then
+	test $# -eq 0 || usage
+	git worktree remove --force "$tmp/worktree" 2>/dev/null
+	rm -rf "$tmp"
+	exit 0
+fi

And this matches what I'd expect "--clean" to do. Makes sense.

-Peff

On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote:

doc-diff creates a temporary working tree (git-worktree) and generates a
bunch of temporary files which it does not remove since they act as a
cache to speed up subsequent runs. Although doc-diff's working tree and
generated files are not strictly build products of the Makefile (which,
itself, never runs doc-diff), as a convenience, update "make clean" to
clean up doc-diff's working tree and generated files along with other
development detritus normally removed by "make clean".

Makes sense.

This spelling took me by surprise. The doc-diff script itself specifies
/bin/sh, and we do not build it, so the #! line is never replaced. I
guess we are leaving it to people on exotic shells to run "$their_sh
doc-diff" in the first place. That's probably OK, since it should work
out of the box on most /bin/sh instances, and people on other platforms
aren't that likely to even run it.

I don't think the script does anything complicated that would choke a
lesser /bin/sh. But it doesn't hurt to be defensive, since this bit of
the Makefile will be run for everyone, whether they care about doc-diff
or not.

So that all makes sense. I initially wrote this to suggest that we call
out this subtlety in the commit message. But I see this is based on
existing instances from ee7ec2f9de (documentation: Makefile accounts for
SHELL_PATH setting, 2009-03-22). So maybe I am just showing my
ignorance. ;)

-Peff

On Fri, Aug 31, 2018 at 02:33:15AM -0400, Eric Sunshine wrote:

This series replaces Peff's solo patch[1] which updates "make clean" to
remove doc-diff's temporary directory. Rather than imbuing the Makefile
with knowledge specific to doc-diff's internals, this series adds a
"clean" mode to doc-diff which removes its temporary worktree and
generated files, and has "make clean" invoke that instead. It also fixes
a portability problem which prevented doc-diff from working on MacOS and
FreeBSD.

Thanks. I left a few comments, but this looks good to me as-is.

-Peff

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

On Fri, Aug 31, 2018 at 02:33:17AM -0400, Eric Sunshine wrote:

quoted

 OPTIONS_SPEC="\
 doc-diff [options] <from> <to> [-- <diff-options>]
+doc-diff (-c|--clean)
 --
 j=n  parallel argument to pass to make
 f    force rebuild; do not rely on cached results
+c,clean      cleanup temporary working files
 "

This will cause parseopt to normalize "--clean" to "-c" (along with
"--cle", etc).

Good to know. The documentation for git-sh-setup didn't talk about
that at all, and while git-rev-parse documentation says that it
"normalizes" options, that word didn't really convey this specific
meaning to me, so I missed it.

quoted

 while test $# -gt 0
 do
      case "$1" in
      -j)
              parallel=$2; shift ;;
+     -c|--clean)
+             clean=t ;;

So this part can just test for "-c". AFAICT this is how "rev-parse
--parseopt" has always worked, though the documentation is quite
unclear. Other scripts seem to also use these redundant long options.
I'm not opposed to including it as a defensive measure (or simply an
annotation for the reader).

I'm fine leaving it as-is too since it seems that every other client
of git-sh-setup does the same (and to save a re-roll).

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

On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote:

quoted

diff --git a/Documentation/Makefile b/Documentation/Makefile

@@ -332,6 +332,7 @@ clean:
      $(RM) manpage-base-url.xsl
+     '$(SHELL_PATH_SQ)' ./doc-diff --clean

This spelling took me by surprise. The doc-diff script itself specifies
/bin/sh, and we do not build it, so the #! line is never replaced. [...]

I don't think the script does anything complicated that would choke a
lesser /bin/sh. But it doesn't hurt to be defensive, since this bit of
the Makefile will be run for everyone, whether they care about doc-diff
or not.

So that all makes sense. I initially wrote this to suggest that we call
out this subtlety in the commit message. But I see this is based on
existing instances from ee7ec2f9de (documentation: Makefile accounts for
SHELL_PATH setting, 2009-03-22). So maybe I am just showing my
ignorance. ;)

Correct. I was concerned that invoking it simply as "./doc-diff
--clean" could be problematic, so, knowing that the Makefile invoked
other scripts in Documentation/, I mirrored their invocation. If it
didn't follow existing practice of invoking the command with
$(SHELL_PATH_SQ), then that would merit mention in the commit message,
but as it is, the commit message is probably fine.

Thanks for the review.

Hi,

Eric Sunshine wrote:

This means I need a copy of git in order to run "make clean".  That
was never required before.  It makes bootstrapping difficult --- do we
really need it?

Thanks,
Jonathan