Re: [PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"
On Mon, Sep 17, 2018 at 12:48:51PM -0700, Junio C Hamano wrote: > Jonathan Nieder 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"
On Mon, Sep 17, 2018 at 4:43 PM Eric Sunshine 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 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"
On Mon, Sep 17, 2018 at 3:36 PM Junio C Hamano wrote: > Jonathan Nieder 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"
Junio C Hamano wrote: > Jonathan Nieder 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 . > --- > 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 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"
Jonathan Nieder 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 . --- 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"
Junio C Hamano wrote: > Jonathan Nieder 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"
Jonathan Nieder 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"
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 > --- > 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"
On Fri, Aug 31, 2018 at 4:07 PM Jeff King 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"
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
[PATCH 3/3] doc/Makefile: drop doc-diff worktree and temporary files on "make clean"
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 --- 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
