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

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

[Jeff King]

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 <peff@peff.net>
---
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

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

[Eric Sunshine]

On Thu, Aug 30, 2018 at 4:12 AM Jeff King <peff@peff.net> 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 <peff@peff.net>

[PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"

[Jeff King]

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 <sunshine@sunshineco.com>
Signed-off-by: Jeff King <peff@peff.net>
---
> 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

Re: [PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"

[Eric Sunshine]

On Thu, Aug 30, 2018 at 3:55 PM Jeff King <peff@peff.net> wrote:
> 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 <sunshine@sunshineco.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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

Re: [PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"

[Eric Sunshine]

On Thu, Aug 30, 2018 at 4:34 PM Eric Sunshine <sunshine@sunshineco.com> 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

Re: [PATCH] doc/Makefile: remove tmp-doc-diff on "make clean"

[Jeff King]

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

> On Thu, Aug 30, 2018 at 3:55 PM Jeff King <peff@peff.net> wrote:
> > 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 <sunshine@sunshineco.com>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > 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

[PATCH 0/3] doc-diff: add "clean" mode & fix portability problem

[Eric Sunshine]

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

[PATCH 1/3] doc-diff: fix non-portable 'man' invocation

[Eric Sunshine]

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 <sunshine@sunshineco.com>
---
 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

Re: [PATCH 1/3] doc-diff: fix non-portable 'man' invocation

[Jeff King]

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

[PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk

[Eric Sunshine]

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 <sunshine@sunshineco.com>
---
 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

Re: [PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk

[Jeff King]

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.

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

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

Re: [PATCH 2/3] doc-diff: add --clean mode to remove temporary working gunk

[Eric Sunshine]

On Fri, Aug 31, 2018 at 4:01 PM Jeff King <peff@peff.net> wrote:
> On Fri, Aug 31, 2018 at 02:33:17AM -0400, Eric Sunshine wrote:
> >  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.

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

[PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Eric Sunshine]

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 <sunshine@sunshineco.com>
---
 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

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jeff King]

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.

> 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

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

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Eric Sunshine]

On Fri, Aug 31, 2018 at 4:07 PM Jeff King <peff@peff.net> wrote:
> On Fri, Aug 31, 2018 at 02:33:18AM -0400, Eric Sunshine wrote:
> > 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.

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jonathan Nieder]

Hi,

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".
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  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

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

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> +	'$(SHELL_PATH_SQ)' ./doc-diff --clean
>
> 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?

Gahh, you are absolutely right.  Also "doc-diff --clean", if I am
reading the code correctly, requires us to be in a Git repository,
not a tarball extract.

Having to have Git installed, or be in a repository, in order to be
able to run an optional "doc-diff" tool is fine.  Requiring either
in order to run "make clean" is a different story.

Thanks for spotting.  We can just prefix the line with '-'?  Or does
the script badly misbehave (due to lack of CEILING_DIRECTORY) when
run in a tarball extract inside somebody else's repository?

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Eric Sunshine wrote:

>>> +	'$(SHELL_PATH_SQ)' ./doc-diff --clean
>>
>> 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?
>
> Gahh, you are absolutely right.  Also "doc-diff --clean", if I am
> reading the code correctly, requires us to be in a Git repository,
> not a tarball extract.
>
> Having to have Git installed, or be in a repository, in order to be
> able to run an optional "doc-diff" tool is fine.  Requiring either
> in order to run "make clean" is a different story.
>
> Thanks for spotting.  We can just prefix the line with '-'?  Or does
> the script badly misbehave (due to lack of CEILING_DIRECTORY) when
> run in a tarball extract inside somebody else's repository?

I'd rather that we revert this change altogether.  I have nothing
against a convenient command to do this kind of non build related
cleanup, but it shouldn't be spelled as "make clean".

That said, for my particular use, prefixing with '-' would work okay.

Thanks,
Jonathan

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I'd rather that we revert this change altogether.  I have nothing
> against a convenient command to do this kind of non build related
> cleanup, but it shouldn't be spelled as "make clean".

OK, let's do this for now as I wanted to merge the remainder to
'master' today.

-- >8 --
Subject: Revert "doc/Makefile: drop doc-diff worktree and temporary files on "make clean""

This reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which
started to require that we have an executable git available in order
to say "make clean", which gives us a chicken-and-egg problem.

Having to have Git installed, or be in a repository, in order to be
able to run an optional "doc-diff" tool is fine.  Requiring either
in order to run "make clean" is a different story.

Reported by Jonathan Nieder <jrnieder@gmail.com>.
---
 Documentation/Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 623f1a866d..d079d7c73a 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -331,7 +331,6 @@ 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

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> I'd rather that we revert this change altogether.  I have nothing
>> against a convenient command to do this kind of non build related
>> cleanup, but it shouldn't be spelled as "make clean".
>
> OK, let's do this for now as I wanted to merge the remainder to
> 'master' today.
>
> -- >8 --
> Subject: Revert "doc/Makefile: drop doc-diff worktree and temporary files on "make clean""
>
> This reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which
> started to require that we have an executable git available in order
> to say "make clean", which gives us a chicken-and-egg problem.
>
> Having to have Git installed, or be in a repository, in order to be
> able to run an optional "doc-diff" tool is fine.  Requiring either
> in order to run "make clean" is a different story.
>
> Reported by Jonathan Nieder <jrnieder@gmail.com>.
> ---
>  Documentation/Makefile | 1 -
>  1 file changed, 1 deletion(-)

Does this want a sign-off?

In any event, this matches what I had applied in Debian[1] and is

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks,
Jonathan

[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
aka http://repo.or.cz/git/debian.git/blob/9872ab1d87634a9288266de290571928e5b9346f:/debian/patches/0002-Revert-doc-Makefile-drop-doc-diff-worktree-and-tempor.diff

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Jeff King]

On Mon, Sep 17, 2018 at 12:48:51PM -0700, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > I'd rather that we revert this change altogether.  I have nothing
> > against a convenient command to do this kind of non build related
> > cleanup, but it shouldn't be spelled as "make clean".
> 
> OK, let's do this for now as I wanted to merge the remainder to
> 'master' today.
> 
> -- >8 --
> Subject: Revert "doc/Makefile: drop doc-diff worktree and temporary files on "make clean""
> 
> This reverts commit 6f924265a0bf6efa677e9a684cebdde958e5ba06, which
> started to require that we have an executable git available in order
> to say "make clean", which gives us a chicken-and-egg problem.
> 
> Having to have Git installed, or be in a repository, in order to be
> able to run an optional "doc-diff" tool is fine.  Requiring either
> in order to run "make clean" is a different story.

Yeah, this seems like the best solution. We started with "can we just rm
-rf the temporary directory as part of 'make clean'", which is totally
sensible and matches the other bits there. But then it got more
complicated. :)

People who use doc-diff can still use "doc-diff --clean", so I don't
think much is lost.

Thanks.

-Peff

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Eric Sunshine]

On Mon, Sep 17, 2018 at 3:36 PM Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> >> +    '$(SHELL_PATH_SQ)' ./doc-diff --clean
> >
> > 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?
>
> Gahh, you are absolutely right.  Also "doc-diff --clean", if I am
> reading the code correctly, requires us to be in a Git repository,
> not a tarball extract.
>
> Having to have Git installed, or be in a repository, in order to be
> able to run an optional "doc-diff" tool is fine.  Requiring either
> in order to run "make clean" is a different story.
>
> Thanks for spotting.  We can just prefix the line with '-'?  Or does
> the script badly misbehave (due to lack of CEILING_DIRECTORY) when
> run in a tarball extract inside somebody else's repository?

I did consider this case and felt that it would be reasonable for it
to error out and ignore the error if git was missing or if the
directory was not a repository. And, I _thought_ I had prefixed the
line with "-" to handle just such a case, but apparently I botched it.
Oh well.

Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"

[Eric Sunshine]

On Mon, Sep 17, 2018 at 4:43 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> I did consider this case and felt that it would be reasonable for it
> to error out and ignore the error if git was missing or if the
> directory was not a repository. And, I _thought_ I had prefixed the
> line with "-" to handle just such a case, but apparently I botched it.
> Oh well.

Dredging up from memory, I think the omission of "-" from the Makefile
line was intentional since I specifically handled the case of missing
"git" command in the script itself by ignoring any error from it.
Specifically, this excerpt from 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

in which a problem invoking git is explicitly ignored and the script
exits cleanly, so no Makefile "-" is needed.

Unfortunately, I forgot about the:

    . "$(git --exec-path)/git-sh-setup"

which happens earlier in the script.

Re: [PATCH 0/3] doc-diff: add "clean" mode & fix portability problem

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

That sounds like a better approach.

> 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(-)

Re: [PATCH 0/3] doc-diff: add "clean" mode & fix portability problem

[Jeff King]

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