{"thread":{"id":"54586","subject":"[PATCH 7/8] Makefile: don't delete dist tarballs directly by name","startedAt":"2020-11-05T21:09:54Z","lastAt":"2020-11-06T20:23:54Z","messageCount":9,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"409228","messageId":"48fdd198-93ad-7282-27e6-9a0c6de93067@ramsayjones.plus.com","threadId":"54586","inReplyTo":null,"subject":"[PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-05T21:09:49Z","receivedAt":"2020-11-05T21:09:54Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nA future patch requires the 'clean' target not to depend, either\ndirectly or indirectly, on the $(GIT_VERSION) variable. However, the\ndistribution tarballs, $(GIT_TARNAME).tar.gz, $(htmldocs).tar.gz and\n$(manpages).tar.gz, all depend on $(GIT_VERSION). The 'clean' target\nattempts to remove these tarballs by name and so has the unwanted\ndependency.\n\nThe first attempt to remove this dependency involved creating the\ndistribution tarballs in a new top-level directory (e.g. 'dist-tars/'),\nthat didn't reference the version number, and could be simply removed\nin the 'clean' target. Unfortunately, this could lead to breaking an\nunknown number of scripts for an unknown number of developers. (This\nactually breaks one of my own scripts!). Despite this being my\npreferred solution, I had to abandon this approach, since I don't know\nwhat problems it may cause.\n\nThe second attempt involved using a wildcard pattern, in place of the\n$(GIT_VERSION) variable, in the distribution tarball filenames. This\nalso proved to be an inadequate solution, no matter how elaborate the\npattern became, because it was always possible that it could lead to\nthe removal of some '*.tar.gz' file that would, otherwise, not have\nbeen removed (eg. git-2.29.0-saved-build.tar.gz).\n\nAlso, note that the current 'clean' target requires that you do not move\nthe current branch away from the commit you were on, when creating the\ndistribution tarballs, before issuing the 'make clean'. If you do so,\nthen you will find that the tarballs are not removed:\n\n $ git checkout nclean\n ...\n $ make dist\n ...\n $ ls *.tar.gz\n git-2.29.0.6.g8255a76caf.tar.gz\n $\n\n $ git checkout master\n Switched to branch 'master'\n Your branch is up to date with 'origin/master'.\n $ make clean\n ...\n rm -f git-2.29.2.154.g8a58376a31.tar.gz\n rm -f git-htmldocs-2.29.2.154.g8a58376a31.tar.gz git-manpages-2.29.2.154.g8a58376a31.tar.gz\n ...\n $\n\n $ ls *.tar.gz\n git-2.29.0.6.g8255a76caf.tar.gz\n $\n\n[I always find the documentation tarballs of the last release intact\nwhen I am just about to create the new tarballs for this release. This\nmeans that I invariably remove them by hand.]\n\nIn order to remove the version dependency, append the name of each\ndistribution tarball created, in the 'dist' and 'doc-dist' targets, to\na file ('dist-tars'). Then in the 'clean' target, simply iterate through\nthe names in this file, if any, removing them as we go. (Not forgetting\nto clean up the 'dist-tars' file as well).\n\nSigned-off-by: Ramsay Jones \n---\n .gitignore | 1 +\n Makefile | 11 +++++++++--\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/.gitignore b/.gitignore\nindex 6232d33924..425b8cc2a4 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -191,6 +191,7 @@\n /gitweb/static/gitweb.min.*\n /config-list.h\n /command-list.h\n+/dist-tars\n *.tar.gz\n *.dsc\n *.deb\ndiff --git a/Makefile b/Makefile\nindex 90e91a2185..bc9ce28bc3 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure\n \t\t--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar\n \t@$(RM) -r .dist-tmp-dir\n \tgzip -f -9 $(GIT_TARNAME).tar\n+\t@echo $(GIT_TARNAME).tar.gz >>dist-tars\n \n rpm::\n \t@echo >&2 \"Use distro packaged sources to run rpmbuild\"\n@@ -3112,6 +3113,7 @@ dist-doc:\n \t$(MAKE) -C Documentation WEBDOC_DEST=../.doc-tmp-dir install-webdoc\n \tcd .doc-tmp-dir && $(TAR) cf ../$(htmldocs).tar $(TAR_DIST_EXTRA_OPTS) .\n \tgzip -n -9 -f $(htmldocs).tar\n+\t@echo $(htmldocs).tar.gz >>dist-tars\n \t:\n \t$(RM) -r .doc-tmp-dir\n \tmkdir -p .doc-tmp-dir/man1 .doc-tmp-dir/man5 .doc-tmp-dir/man7\n@@ -3122,6 +3124,7 @@ dist-doc:\n \t\tinstall\n \tcd .doc-tmp-dir && $(TAR) cf ../$(manpages).tar $(TAR_DIST_EXTRA_OPTS) .\n \tgzip -n -9 -f $(manpages).tar\n+\t@echo $(manpages).tar.gz >>dist-tars\n \t$(RM) -r .doc-tmp-dir\n \n ### Cleaning rules\n@@ -3151,8 +3154,12 @@ clean: profile-clean coverage-clean cocciclean\n \t$(RM) -r po/build/\n \t$(RM) *.pyc *.pyo */*.pyc */*.pyo $(GENERATED_H) $(ETAGS_TARGET) tags cscope*\n \t$(RM) -r .dist-tmp-dir .doc-tmp-dir\n-\t$(RM) $(GIT_TARNAME).tar.gz\n-\t$(RM) $(htmldocs).tar.gz $(manpages).tar.gz\n+\t@if test -s dist-tars; then \\\n+\t\tfor i in $$(cat dist-tars); do \\\n+\t\t\t$(RM) $$i; \\\n+\t\tdone \\\n+\tfi\n+\t$(RM) dist-tars\n \t$(MAKE) -C Documentation/ clean\n \t$(RM) Documentation/GIT-EXCLUDED-PROGRAMS\n ifndef NO_PERL\n-- \n2.29.0\n"},{"id":"409237","messageId":"xmqqr1p7h38t.fsf@gitster.c.googlers.com","threadId":"54586","inReplyTo":"48fdd198-93ad-7282-27e6-9a0c6de93067@ramsayjones.plus.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-05T22:01:54Z","receivedAt":"2020-11-05T22:02:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> diff --git a/.gitignore b/.gitignore\n> index 6232d33924..425b8cc2a4 100644\n> --- a/.gitignore\n> +++ b/.gitignore\n> @@ -191,6 +191,7 @@\n> /gitweb/static/gitweb.min.*\n> /config-list.h\n> /command-list.h\n> +/dist-tars\n> *.tar.gz\n> *.dsc\n> *.deb\n> diff --git a/Makefile b/Makefile\n> index 90e91a2185..bc9ce28bc3 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure\n> \t\t--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar\n> \t@$(RM) -r .dist-tmp-dir\n> \tgzip -f -9 $(GIT_TARNAME).tar\n> +\t@echo $(GIT_TARNAME).tar.gz >>dist-tars\n\nSorry, but I'd rather not to see such a longer-term \"list of files\nto be removed\" on the filesystem. This invites attackers to write a\nrogue test addition that writes into ../../dist-tars something like\n\"~/.gitconfig\" and wait for me to say \"make clean\".\n"},{"id":"409240","messageId":"xmqqeel7h0aj.fsf@gitster.c.googlers.com","threadId":"54586","inReplyTo":"xmqqr1p7h38t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-05T23:05:40Z","receivedAt":"2020-11-05T23:05:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ramsay Jones writes:\n>\n>> diff --git a/.gitignore b/.gitignore\n>> index 6232d33924..425b8cc2a4 100644\n>> --- a/.gitignore\n>> +++ b/.gitignore\n>> @@ -191,6 +191,7 @@\n>> /gitweb/static/gitweb.min.*\n>> /config-list.h\n>> /command-list.h\n>> +/dist-tars\n>> *.tar.gz\n>> *.dsc\n>> *.deb\n>> diff --git a/Makefile b/Makefile\n>> index 90e91a2185..bc9ce28bc3 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure\n>> \t\t--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar\n>> \t@$(RM) -r .dist-tmp-dir\n>> \tgzip -f -9 $(GIT_TARNAME).tar\n>> +\t@echo $(GIT_TARNAME).tar.gz >>dist-tars\n>\n> Sorry, but I'd rather not to see such a longer-term \"list of files\n> to be removed\" on the filesystem. This invites attackers to write a\n> rogue test addition that writes into ../../dist-tars something like\n> \"~/.gitconfig\" and wait for me to say \"make clean\".\n\nHaving said that, I also think that \"make clean\" target should help\nyour desire to keep tarballs that would not have been generated by\nthe current checkout (e.g. the last release), and I do not think\n\n\t$(RM) git-htmldocs-*.tar.gz git-manpages-*.tar.gz\n\nis an unconditionally better alternative to what you did in this\nstep.\n\nHow about moving removal of these \"distribution\" artifacts that are\ncreated by \"make dist$something\" targets from \"clean\" to \"distclean\"?\n"},{"id":"409260","messageId":"4b59622a-f2f3-6d86-2742-46a0524fa6d5@ramsayjones.plus.com","threadId":"54586","inReplyTo":"xmqqr1p7h38t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-06T01:36:45Z","receivedAt":"2020-11-06T01:37:14Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/11/2020 22:01, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> diff --git a/.gitignore b/.gitignore\n>> index 6232d33924..425b8cc2a4 100644\n>> --- a/.gitignore\n>> +++ b/.gitignore\n>> @@ -191,6 +191,7 @@\n>> /gitweb/static/gitweb.min.*\n>> /config-list.h\n>> /command-list.h\n>> +/dist-tars\n>> *.tar.gz\n>> *.dsc\n>> *.deb\n>> diff --git a/Makefile b/Makefile\n>> index 90e91a2185..bc9ce28bc3 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -3083,6 +3083,7 @@ dist: git-archive$(X) configure\n>> \t\t--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar\n>> \t@$(RM) -r .dist-tmp-dir\n>> \tgzip -f -9 $(GIT_TARNAME).tar\n>> +\t@echo $(GIT_TARNAME).tar.gz >>dist-tars\n> \n> Sorry, but I'd rather not to see such a longer-term \"list of files\n> to be removed\" on the filesystem. This invites attackers to write a\n> rogue test addition that writes into ../../dist-tars something like\n> \"~/.gitconfig\" and wait for me to say \"make clean\".\n\nYes, that is a reasonable concern. I suppose we could drop the last\ntwo patches then - most of the saving comes from the first four patches\n(as you can see from the table in the cover letter).\n\nAlso, I had an alternative patch for the last patch, which did away with\nthe '-include GIT-VERSION-FILE' entirely! (That had treewide implications\nthat I hadn't sorted through yet).\n\nATB,\nRamsay Jones\n\n"},{"id":"409261","messageId":"0cbe821b-092d-a5f9-e2ab-13a3c01e9a02@ramsayjones.plus.com","threadId":"54586","inReplyTo":"xmqqeel7h0aj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-06T01:44:18Z","receivedAt":"2020-11-06T01:44:23Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/11/2020 23:05, Junio C Hamano wrote:\n> Junio C Hamano writes:\n[snip]\n>> Sorry, but I'd rather not to see such a longer-term \"list of files\n>> to be removed\" on the filesystem. This invites attackers to write a\n>> rogue test addition that writes into ../../dist-tars something like\n>> \"~/.gitconfig\" and wait for me to say \"make clean\".\n> \n> Having said that, I also think that \"make clean\" target should help\n> your desire to keep tarballs that would not have been generated by\n> the current checkout (e.g. the last release), and I do not think\n> \n> \t$(RM) git-htmldocs-*.tar.gz git-manpages-*.tar.gz\n> \n> is an unconditionally better alternative to what you did in this\n> step.\n> \n> How about moving removal of these \"distribution\" artifacts that are\n> created by \"make dist$something\" targets from \"clean\" to \"distclean\"?\n\nI did have a patch #9 that moved the removal of the tarballs from\nthe 'clean' target to the 'distclean' target (that is move the new\n'iterate over the dist-tars file' code). However, I wasn't convinced\nthat it was a better place for it.\n\nAlso, just moving the new removal code would not alter the concerns\nabout it that you express above. So, we need to address that either\nway.\n\nThanks.\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"409263","messageId":"xmqqa6vvgs9y.fsf@gitster.c.googlers.com","threadId":"54586","inReplyTo":"0cbe821b-092d-a5f9-e2ab-13a3c01e9a02@ramsayjones.plus.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-06T01:58:49Z","receivedAt":"2020-11-06T01:58:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> I did have a patch #9 that moved the removal of the tarballs from\n> the 'clean' target to the 'distclean' target (that is move the new\n> 'iterate over the dist-tars file' code). However, I wasn't convinced\n> that it was a better place for it.\n\nI am not either, other than \"things created by 'make distX' cleaned\nby 'make distclean'---that sounds consistent\" ;-).\n\n> Also, just moving the new removal code would not alter the concerns\n> about it that you express above. So, we need to address that either\n> way.\n\n\"make distclean\" can still depend on GIT-VERSION-FILE and your daily\ncleaning needs can use \"make clean\" that cleans only the files that\ncan be cleaned without knowing what GIT_VERSION is, no? That way,\nwe do not have to use dist-tars file at all, no?\n\n"},{"id":"409265","messageId":"ec59a5d8-26fc-988e-4458-fd2cd421f9d5@ramsayjones.plus.com","threadId":"54586","inReplyTo":"xmqqa6vvgs9y.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-06T02:39:10Z","receivedAt":"2020-11-06T02:39:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/11/2020 01:58, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> I did have a patch #9 that moved the removal of the tarballs from\n>> the 'clean' target to the 'distclean' target (that is move the new\n>> 'iterate over the dist-tars file' code). However, I wasn't convinced\n>> that it was a better place for it.\n> \n> I am not either, other than \"things created by 'make distX' cleaned\n> by 'make distclean'---that sounds consistent\" ;-).\n\n:-D\n\n> \n>> Also, just moving the new removal code would not alter the concerns\n>> about it that you express above. So, we need to address that either\n>> way.\n> \n> \"make distclean\" can still depend on GIT-VERSION-FILE and your daily\n> cleaning needs can use \"make clean\" that cleans only the files that\n> can be cleaned without knowing what GIT_VERSION is, no? That way,\n> we do not have to use dist-tars file at all, no?\n\nHmm, so rather than dropping the last two patches, you are suggesting\nreplacing this patch with a patch that moves:\n\n\t$(RM) $(GIT_TARNAME).tar.gz\n\t$(RM) $(htmldocs).tar.gz $(manpages).tar.gz\n\nto the 'distclean' target?\n\nWell, that would address your concerns, but I was hoping to fix the\n'your current branch has to be the same as the one you created the\ntarballs on' for the '(dist)clean' target to actually remove those\nfiles issue.\n\nAs I said in the commit message, my preferred solution was to simply\nput them in a 'dist-tars/' directory. Then you would only delete the\nfiles actually generated by the 'dist' and 'dist-doc' targets, and\nyou could easily remove that directory (be it in 'clean' or 'distclean'\nor 'clean-dist-tars', ...).\n\nHmm, I wonder if my concerns about that solution are overblown? dunno.\n\nATB,\nRamsay Jones\n\n"},{"id":"409280","messageId":"xmqqsg9mfk26.fsf@gitster.c.googlers.com","threadId":"54586","inReplyTo":"ec59a5d8-26fc-988e-4458-fd2cd421f9d5@ramsayjones.plus.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-06T17:53:53Z","receivedAt":"2020-11-06T17:54:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n>> \"make distclean\" can still depend on GIT-VERSION-FILE and your daily\n>> cleaning needs can use \"make clean\" that cleans only the files that\n>> can be cleaned without knowing what GIT_VERSION is, no? That way,\n>> we do not have to use dist-tars file at all, no?\n>\n> Hmm, so rather than dropping the last two patches, you are suggesting\n> replacing this patch with a patch that moves:\n>\n> \t$(RM) $(GIT_TARNAME).tar.gz\n> \t$(RM) $(htmldocs).tar.gz $(manpages).tar.gz\n>\n> to the 'distclean' target?\n\nYup. FWIW, I consider it a feature that\n\n for m in maint-2.{27,28,29}\n do\n\tgit checkout \"$m\" &&\n make distclean &&\n\tmake dist || break\n do\n\ngives me three distribution tarballs of from-scratch builds. It\nmatters when I need to push out releases from multiple maintenance\ntracks at the same time (think: coordinated security releases).\n\nI could of course move them away before running distclean in each\nstep, but then I'd be following a different workflow from what I\nnormally use when cutting a single release---I would rather avoid\ndeviating from the routine when I have to be absolutely careful\n(again, think: coordinated security releases).\n\n> Well, that would address your concerns, but I was hoping to fix the\n> 'your current branch has to be the same as the one you created the\n> tarballs on' for the '(dist)clean' target to actually remove those\n> files issue.\n\nSo we on this part have directly opposing wish, unfortunately.\n"},{"id":"409295","messageId":"7ad94f1a-9cc7-5ac9-2859-1d1015ea468f@ramsayjones.plus.com","threadId":"54586","inReplyTo":"xmqqsg9mfk26.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 7/8] Makefile: don't delete dist tarballs directly by name","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-06T20:23:50Z","receivedAt":"2020-11-06T20:23:54Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/11/2020 17:53, Junio C Hamano wrote:\n> Ramsay Jones writes:\n>> Hmm, so rather than dropping the last two patches, you are suggesting\n>> replacing this patch with a patch that moves:\n>>\n>> \t$(RM) $(GIT_TARNAME).tar.gz\n>> \t$(RM) $(htmldocs).tar.gz $(manpages).tar.gz\n>>\n>> to the 'distclean' target?\n> \n> Yup. FWIW, I consider it a feature that\n> \n> for m in maint-2.{27,28,29}\n> do\n> \tgit checkout \"$m\" &&\n> make distclean &&\n> \tmake dist || break\n> do\n> \n> gives me three distribution tarballs of from-scratch builds. It\n> matters when I need to push out releases from multiple maintenance\n> tracks at the same time (think: coordinated security releases).\n\nAh, yes. I hadn't thought about that kind of usage. Hmm, but in that\ncase, wouldn't you rather get rid of the deletion of the tarballs in\nthe 'clean' target completely? Then you could either remove them by\nhand (which I actually always do in practice anyway) at a time that\nsuits you, or maybe add a new 'clean-tarballs' target.\n\n(That still leaves the issue of how you would identify the tarballs\nin such a target, of course).\n\nATB,\nRamsay Jones\n\n"}]}