[PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Ramsay Jones]

A future patch requires the 'clean' target not to depend, either
directly or indirectly, on the $(GIT_VERSION) variable. However, the
distribution tarballs, $(GIT_TARNAME).tar.gz, $(htmldocs).tar.gz and
$(manpages).tar.gz, all depend on $(GIT_VERSION). The 'clean' target
attempts to remove these tarballs by name and so has the unwanted
dependency.

The first attempt to remove this dependency involved creating the
distribution tarballs in a new top-level directory (e.g. 'dist-tars/'),
that didn't reference the version number, and could be simply removed
in the 'clean' target. Unfortunately, this could lead to breaking an
unknown number of scripts for an unknown number of developers. (This
actually breaks one of my own scripts!). Despite this being my
preferred solution, I had to abandon this approach, since I don't know
what problems it may cause.

The second attempt involved using a wildcard pattern, in place of the
$(GIT_VERSION) variable, in the distribution tarball filenames. This
also proved to be an inadequate solution, no matter how elaborate the
pattern became, because it was always possible that it could lead to
the removal of some '*.tar.gz' file that would, otherwise, not have
been removed (eg. git-2.29.0-saved-build.tar.gz).

Also, note that the current 'clean' target requires that you do not move
the current branch away from the commit you were on, when creating the
distribution tarballs, before issuing the 'make clean'. If you do so,
then you will find that the tarballs are not removed:

  $ git checkout nclean
  ...
  $ make dist
  ...
  $ ls *.tar.gz
  git-2.29.0.6.g8255a76caf.tar.gz
  $

  $ git checkout master
  Switched to branch 'master'
  Your branch is up to date with 'origin/master'.
  $ make clean
  ...
  rm -f git-2.29.2.154.g8a58376a31.tar.gz
  rm -f git-htmldocs-2.29.2.154.g8a58376a31.tar.gz git-manpages-2.29.2.154.g8a58376a31.tar.gz
  ...
  $

  $ ls *.tar.gz
  git-2.29.0.6.g8255a76caf.tar.gz
  $

[I always find the documentation tarballs of the last release intact
when I am just about to create the new tarballs for this release. This
means that I invariably remove them by hand.]

In order to remove the version dependency, append the name of each
distribution tarball created, in the 'dist' and 'doc-dist' targets, to
a file ('dist-tars'). Then in the 'clean' target, simply iterate through
the names in this file, if any, removing them as we go. (Not forgetting
to clean up the 'dist-tars' file as well).

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
 .gitignore |  1 +
 Makefile   | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index 6232d33924..425b8cc2a4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -191,6 +191,7 @@
 /gitweb/static/gitweb.min.*
 /config-list.h
 /command-list.h
+/dist-tars
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index 90e91a2185..bc9ce28bc3 100644
--- a/Makefile
+++ b/Makefile
@@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure
 		--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
 	@$(RM) -r .dist-tmp-dir
 	gzip -f -9 $(GIT_TARNAME).tar
+	@echo $(GIT_TARNAME).tar.gz >>dist-tars
 
 rpm::
 	@echo >&2 "Use distro packaged sources to run rpmbuild"
@@ -3112,6 +3113,7 @@ dist-doc:
 	$(MAKE) -C Documentation WEBDOC_DEST=../.doc-tmp-dir install-webdoc
 	cd .doc-tmp-dir && $(TAR) cf ../$(htmldocs).tar $(TAR_DIST_EXTRA_OPTS) .
 	gzip -n -9 -f $(htmldocs).tar
+	@echo $(htmldocs).tar.gz >>dist-tars
 	:
 	$(RM) -r .doc-tmp-dir
 	mkdir -p .doc-tmp-dir/man1 .doc-tmp-dir/man5 .doc-tmp-dir/man7
@@ -3122,6 +3124,7 @@ dist-doc:
 		install
 	cd .doc-tmp-dir && $(TAR) cf ../$(manpages).tar $(TAR_DIST_EXTRA_OPTS) .
 	gzip -n -9 -f $(manpages).tar
+	@echo $(manpages).tar.gz >>dist-tars
 	$(RM) -r .doc-tmp-dir
 
 ### Cleaning rules
@@ -3151,8 +3154,12 @@ clean: profile-clean coverage-clean cocciclean
 	$(RM) -r po/build/
 	$(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope*
 	$(RM) -r .dist-tmp-dir .doc-tmp-dir
-	$(RM) $(GIT_TARNAME).tar.gz
-	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
+	@if test -s dist-tars; then \
+		for i in $$(cat dist-tars); do \
+			$(RM) $$i; \
+		done \
+	fi
+	$(RM) dist-tars
 	$(MAKE) -C Documentation/ clean
 	$(RM) Documentation/GIT-EXCLUDED-PROGRAMS
 ifndef NO_PERL
-- 
2.29.0

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> diff --git a/.gitignore b/.gitignore
> index 6232d33924..425b8cc2a4 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -191,6 +191,7 @@
>  /gitweb/static/gitweb.min.*
>  /config-list.h
>  /command-list.h
> +/dist-tars
>  *.tar.gz
>  *.dsc
>  *.deb
> diff --git a/Makefile b/Makefile
> index 90e91a2185..bc9ce28bc3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure
>  		--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
>  	@$(RM) -r .dist-tmp-dir
>  	gzip -f -9 $(GIT_TARNAME).tar
> +	@echo $(GIT_TARNAME).tar.gz >>dist-tars

Sorry, but I'd rather not to see such a longer-term "list of files
to be removed" on the filesystem.  This invites attackers to write a
rogue test addition that writes into ../../dist-tars something like
"~/.gitconfig" and wait for me to say "make clean".

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> diff --git a/.gitignore b/.gitignore
>> index 6232d33924..425b8cc2a4 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -191,6 +191,7 @@
>>  /gitweb/static/gitweb.min.*
>>  /config-list.h
>>  /command-list.h
>> +/dist-tars
>>  *.tar.gz
>>  *.dsc
>>  *.deb
>> diff --git a/Makefile b/Makefile
>> index 90e91a2185..bc9ce28bc3 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure
>>  		--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
>>  	@$(RM) -r .dist-tmp-dir
>>  	gzip -f -9 $(GIT_TARNAME).tar
>> +	@echo $(GIT_TARNAME).tar.gz >>dist-tars
>
> Sorry, but I'd rather not to see such a longer-term "list of files
> to be removed" on the filesystem.  This invites attackers to write a
> rogue test addition that writes into ../../dist-tars something like
> "~/.gitconfig" and wait for me to say "make clean".

Having said that, I also think that "make clean" target should help
your desire to keep tarballs that would not have been generated by
the current checkout (e.g. the last release), and I do not think

	$(RM) git-htmldocs-*.tar.gz git-manpages-*.tar.gz

is an unconditionally better alternative to what you did in this
step.

How about moving removal of these "distribution" artifacts that are
created by "make dist$something" targets from "clean" to "distclean"?

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Ramsay Jones]

On 05/11/2020 23:05, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
[snip]
>> Sorry, but I'd rather not to see such a longer-term "list of files
>> to be removed" on the filesystem.  This invites attackers to write a
>> rogue test addition that writes into ../../dist-tars something like
>> "~/.gitconfig" and wait for me to say "make clean".
> 
> Having said that, I also think that "make clean" target should help
> your desire to keep tarballs that would not have been generated by
> the current checkout (e.g. the last release), and I do not think
> 
> 	$(RM) git-htmldocs-*.tar.gz git-manpages-*.tar.gz
> 
> is an unconditionally better alternative to what you did in this
> step.
> 
> How about moving removal of these "distribution" artifacts that are
> created by "make dist$something" targets from "clean" to "distclean"?

I did have a patch #9 that moved the removal of the tarballs from
the 'clean' target to the 'distclean' target (that is move the new
'iterate over the dist-tars file' code). However, I wasn't convinced
that it was a better place for it.

Also, just moving the new removal code would not alter the concerns
about it that you express above. So, we need to address that either
way.

Thanks.

ATB,
Ramsay Jones

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> I did have a patch #9 that moved the removal of the tarballs from
> the 'clean' target to the 'distclean' target (that is move the new
> 'iterate over the dist-tars file' code). However, I wasn't convinced
> that it was a better place for it.

I am not either, other than "things created by 'make distX' cleaned
by 'make distclean'---that sounds consistent" ;-).

> Also, just moving the new removal code would not alter the concerns
> about it that you express above. So, we need to address that either
> way.

"make distclean" can still depend on GIT-VERSION-FILE and your daily
cleaning needs can use "make clean" that cleans only the files that
can be cleaned without knowing what GIT_VERSION is, no?  That way,
we do not have to use dist-tars file at all, no?

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Ramsay Jones]

On 06/11/2020 01:58, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> I did have a patch #9 that moved the removal of the tarballs from
>> the 'clean' target to the 'distclean' target (that is move the new
>> 'iterate over the dist-tars file' code). However, I wasn't convinced
>> that it was a better place for it.
> 
> I am not either, other than "things created by 'make distX' cleaned
> by 'make distclean'---that sounds consistent" ;-).

:-D

> 
>> Also, just moving the new removal code would not alter the concerns
>> about it that you express above. So, we need to address that either
>> way.
> 
> "make distclean" can still depend on GIT-VERSION-FILE and your daily
> cleaning needs can use "make clean" that cleans only the files that
> can be cleaned without knowing what GIT_VERSION is, no?  That way,
> we do not have to use dist-tars file at all, no?

Hmm, so rather than dropping the last two patches, you are suggesting
replacing this patch with a patch that moves:

	$(RM) $(GIT_TARNAME).tar.gz
	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz

to the 'distclean' target?

Well, that would address your concerns, but I was hoping to fix the
'your current branch has to be the same as the one you created the
tarballs on' for the '(dist)clean' target to actually remove those
files issue.

As I said in the commit message, my preferred solution was to simply
put them in a 'dist-tars/' directory. Then you would only delete the
files actually generated by the 'dist' and 'dist-doc' targets, and
you could easily remove that directory (be it in 'clean' or 'distclean'
or 'clean-dist-tars', ...).

Hmm, I wonder if my concerns about that solution are overblown? dunno.

ATB,
Ramsay Jones

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>> "make distclean" can still depend on GIT-VERSION-FILE and your daily
>> cleaning needs can use "make clean" that cleans only the files that
>> can be cleaned without knowing what GIT_VERSION is, no?  That way,
>> we do not have to use dist-tars file at all, no?
>
> Hmm, so rather than dropping the last two patches, you are suggesting
> replacing this patch with a patch that moves:
>
> 	$(RM) $(GIT_TARNAME).tar.gz
> 	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
>
> to the 'distclean' target?

Yup.  FWIW, I consider it a feature that

    for m in maint-2.{27,28,29}
    do
	git checkout "$m" &&
        make distclean &&
	make dist || break
    do

gives me three distribution tarballs of from-scratch builds.  It
matters when I need to push out releases from multiple maintenance
tracks at the same time (think: coordinated security releases).

I could of course move them away before running distclean in each
step, but then I'd be following a different workflow from what I
normally use when cutting a single release---I would rather avoid
deviating from the routine when I have to be absolutely careful
(again, think: coordinated security releases).

> Well, that would address your concerns, but I was hoping to fix the
> 'your current branch has to be the same as the one you created the
> tarballs on' for the '(dist)clean' target to actually remove those
> files issue.

So we on this part have directly opposing wish, unfortunately.

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Ramsay Jones]

On 06/11/2020 17:53, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>> Hmm, so rather than dropping the last two patches, you are suggesting
>> replacing this patch with a patch that moves:
>>
>> 	$(RM) $(GIT_TARNAME).tar.gz
>> 	$(RM) $(htmldocs).tar.gz $(manpages).tar.gz
>>
>> to the 'distclean' target?
> 
> Yup.  FWIW, I consider it a feature that
> 
>     for m in maint-2.{27,28,29}
>     do
> 	git checkout "$m" &&
>         make distclean &&
> 	make dist || break
>     do
> 
> gives me three distribution tarballs of from-scratch builds.  It
> matters when I need to push out releases from multiple maintenance
> tracks at the same time (think: coordinated security releases).

Ah, yes. I hadn't thought about that kind of usage. Hmm, but in that
case, wouldn't you rather get rid of the deletion of the tarballs in
the 'clean' target completely? Then you could either remove them by
hand (which I actually always do in practice anyway) at a time that
suits you, or maybe add a new 'clean-tarballs' target.

(That still leaves the issue of how you would identify the tarballs
in such a target, of course).

ATB,
Ramsay Jones

Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name

[Ramsay Jones]

On 05/11/2020 22:01, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> diff --git a/.gitignore b/.gitignore
>> index 6232d33924..425b8cc2a4 100644
>> --- a/.gitignore
>> +++ b/.gitignore
>> @@ -191,6 +191,7 @@
>>  /gitweb/static/gitweb.min.*
>>  /config-list.h
>>  /command-list.h
>> +/dist-tars
>>  *.tar.gz
>>  *.dsc
>>  *.deb
>> diff --git a/Makefile b/Makefile
>> index 90e91a2185..bc9ce28bc3 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure
>>  		--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
>>  	@$(RM) -r .dist-tmp-dir
>>  	gzip -f -9 $(GIT_TARNAME).tar
>> +	@echo $(GIT_TARNAME).tar.gz >>dist-tars
> 
> Sorry, but I'd rather not to see such a longer-term "list of files
> to be removed" on the filesystem.  This invites attackers to write a
> rogue test addition that writes into ../../dist-tars something like
> "~/.gitconfig" and wait for me to say "make clean".

Yes, that is a reasonable concern. I suppose we could drop the last
two patches then - most of the saving comes from the first four patches
(as you can see from the table in the cover letter).

Also, I had an alternative patch for the last patch, which did away with
the '-include GIT-VERSION-FILE' entirely! (That had treewide implications
that I hadn't sorted through yet).

ATB,
Ramsay Jones