# [PATCH v2 2/5] contrib/subtree/Makefile: Use GIT-VERSION-FILE 8 messages from 2014-05-06 to 2014-05-06. Participants: James Denholm, Junio C Hamano. Thread: https://gitlist.dev/t/36588 ## James Denholm, 2014-05-06 12:41 Subject: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass Message-ID: <1399380109-3805-1-git-send-email-nod.helm@gmail.com> URL: https://gitlist.dev/e/1399380109-3805-1-git-send-email-nod.helm%40gmail.com ``` contrib/subtree/Makefile is a shambles in regards to it's consistency with other makefiles, which makes subtree overly painful to include in build scripts. The main issues are that calls are made to git itself in the build process, and that a subtree-exclusive variable is used for specifying the exec path. Patches 1/5 through 3/5 resolve these. The "cleanup" fixes (4/5 and 5/5) are based on precedents set by other makefiles across the project. One problem is foreseen: 3/5 will necessitate that package maintainers who already have git-subtree included in their packages update their build-scripts. Reviewed-by: Jeff King Signed-off-by: James Denholm Based-on-patch-by: Dan McGee James Denholm (5): contrib/subtree/Makefile: scrap unused $(gitdir) contrib/subtree/Makefile: Use GIT-VERSION-FILE contrib/subtree/Makefile: s/libexecdir/gitexecdir contrib/subtree/Makefile: Doc-gen rules cleanup contrib/subtree/Makefile: clean rule cleanup contrib/subtree/Makefile | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) -- 1.9.2 ``` ## James Denholm, 2014-05-06 12:41 Subject: [PATCH v2 1/5] contrib/subtree/Makefile: scrap unused $(gitdir) Message-ID: <1399380109-3805-2-git-send-email-nod.helm@gmail.com> URL: https://gitlist.dev/e/1399380109-3805-2-git-send-email-nod.helm%40gmail.com In-Reply-To: <1399380109-3805-1-git-send-email-nod.helm@gmail.com> ``` In 7ff8463dba0d74fc07a766bed457ae7afcc902b5, the references to gitdir were removed but the assignment itself wasn't. Hence, drop the gitdir assignment. Reviewed-by: Jeff King Signed-off-by: James Denholm --- contrib/subtree/Makefile | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index 4030a16..87797ed 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -4,7 +4,6 @@ prefix ?= /usr/local mandir ?= $(prefix)/share/man libexecdir ?= $(prefix)/libexec/git-core -gitdir ?= $(shell git --exec-path) man1dir ?= $(mandir)/man1 gitver ?= $(word 3,$(shell git --version)) -- 1.9.2 ``` ## James Denholm, 2014-05-06 12:41 Subject: [PATCH v2 2/5] contrib/subtree/Makefile: Use GIT-VERSION-FILE Message-ID: <1399380109-3805-3-git-send-email-nod.helm@gmail.com> URL: https://gitlist.dev/e/1399380109-3805-3-git-send-email-nod.helm%40gmail.com In-Reply-To: <1399380109-3805-1-git-send-email-nod.helm@gmail.com> ``` GVF is already being used in most/all other makefiles in the project, and has been for _quite_ a while. Hence, drop file-unique gitver and replace with GIT_VERSION. Reviewed-by: Jeff King Signed-off-by: James Denholm --- contrib/subtree/Makefile | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index 87797ed..f63334b 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -6,7 +6,10 @@ mandir ?= $(prefix)/share/man libexecdir ?= $(prefix)/libexec/git-core man1dir ?= $(mandir)/man1 -gitver ?= $(word 3,$(shell git --version)) +../../GIT-VERSION-FILE: FORCE + $(MAKE) -C ../../ GIT-VERSION-FILE + +-include ../../GIT-VERSION-FILE # this should be set to a 'standard' bsd-type install program INSTALL ?= install @@ -44,11 +47,11 @@ $(GIT_SUBTREE_DOC): $(GIT_SUBTREE_XML) $(GIT_SUBTREE_XML): $(GIT_SUBTREE_TXT) asciidoc -b docbook -d manpage -f $(ASCIIDOC_CONF) \ - -agit_version=$(gitver) $^ + -agit_version=$(GIT_VERSION) $^ $(GIT_SUBTREE_HTML): $(GIT_SUBTREE_TXT) asciidoc -b xhtml11 -d manpage -f $(ASCIIDOC_CONF) \ - -agit_version=$(gitver) $^ + -agit_version=$(GIT_VERSION) $^ test: $(MAKE) -C t/ test @@ -56,3 +59,5 @@ test: clean: rm -f *~ *.xml *.html *.1 rm -rf subproj mainline + +.PHONY: FORCE -- 1.9.2 ``` ## James Denholm, 2014-05-06 12:41 Subject: [PATCH v2 3/5] contrib/subtree/Makefile: s/libexecdir/gitexecdir Message-ID: <1399380109-3805-4-git-send-email-nod.helm@gmail.com> URL: https://gitlist.dev/e/1399380109-3805-4-git-send-email-nod.helm%40gmail.com In-Reply-To: <1399380109-3805-1-git-send-email-nod.helm@gmail.com> ``` $(libexecdir) isn't used anywhere else in the project, while $(gitexecdir) is the standard in the other appropriate makefiles. Hence, replace the former with the latter. Reviewed-by: Jeff King Signed-off-by: James Denholm --- contrib/subtree/Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index f63334b..579bb51 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -3,7 +3,7 @@ prefix ?= /usr/local mandir ?= $(prefix)/share/man -libexecdir ?= $(prefix)/libexec/git-core +gitexecdir ?= $(prefix)/libexec/git-core man1dir ?= $(mandir)/man1 ../../GIT-VERSION-FILE: FORCE @@ -33,8 +33,8 @@ $(GIT_SUBTREE): $(GIT_SUBTREE_SH) doc: $(GIT_SUBTREE_DOC) $(GIT_SUBTREE_HTML) install: $(GIT_SUBTREE) - $(INSTALL) -d -m 755 $(DESTDIR)$(libexecdir) - $(INSTALL) -m 755 $(GIT_SUBTREE) $(DESTDIR)$(libexecdir) + $(INSTALL) -d -m 755 $(DESTDIR)$(gitexecdir) + $(INSTALL) -m 755 $(GIT_SUBTREE) $(DESTDIR)$(gitexecdir) install-doc: install-man -- 1.9.2 ``` ## James Denholm, 2014-05-06 12:41 Subject: [PATCH v2 4/5] contrib/subtree/Makefile: Doc-gen rules cleanup Message-ID: <1399380109-3805-5-git-send-email-nod.helm@gmail.com> URL: https://gitlist.dev/e/1399380109-3805-5-git-send-email-nod.helm%40gmail.com In-Reply-To: <1399380109-3805-1-git-send-email-nod.helm@gmail.com> ``` git:Documentation/Makefile establishes asciidoc/xmlto calls as being handled through their appropriate variables, Hence, change to bring into congruency with. Similarly, MANPAGE_XSL exists in git:Documentation/Makefile, while MANPAGE_NORMAL_XSL does not outside contrib/subtree. Hence, replace MANPAGE_NORMAL_XSL with MANPAGE_XSL. Reviewed-by: Jeff King Signed-off-by: James Denholm --- contrib/subtree/Makefile | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index 579bb51..f3834b5 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -14,8 +14,11 @@ man1dir ?= $(mandir)/man1 # this should be set to a 'standard' bsd-type install program INSTALL ?= install -ASCIIDOC_CONF = ../../Documentation/asciidoc.conf -MANPAGE_NORMAL_XSL = ../../Documentation/manpage-normal.xsl +ASCIIDOC = asciidoc +XMLTO = xmlto + +ASCIIDOC_CONF = ../../Documentation/asciidoc.conf +MANPAGE_XSL = ../../Documentation/manpage-normal.xsl GIT_SUBTREE_SH := git-subtree.sh GIT_SUBTREE := git-subtree @@ -43,14 +46,14 @@ install-man: $(GIT_SUBTREE_DOC) $(INSTALL) -m 644 $^ $(DESTDIR)$(man1dir) $(GIT_SUBTREE_DOC): $(GIT_SUBTREE_XML) - xmlto -m $(MANPAGE_NORMAL_XSL) man $^ + $(XMLTO) -m $(MANPAGE_XSL) man $^ $(GIT_SUBTREE_XML): $(GIT_SUBTREE_TXT) - asciidoc -b docbook -d manpage -f $(ASCIIDOC_CONF) \ + $(ASCIIDOC) -b docbook -d manpage -f $(ASCIIDOC_CONF) \ -agit_version=$(GIT_VERSION) $^ $(GIT_SUBTREE_HTML): $(GIT_SUBTREE_TXT) - asciidoc -b xhtml11 -d manpage -f $(ASCIIDOC_CONF) \ + $(ASCIIDOC) -b xhtml11 -d manpage -f $(ASCIIDOC_CONF) \ -agit_version=$(GIT_VERSION) $^ test: -- 1.9.2 ``` ## James Denholm, 2014-05-06 12:41 Subject: [PATCH v2 5/5] contrib/subtree/Makefile: clean rule cleanup Message-ID: <1399380109-3805-6-git-send-email-nod.helm@gmail.com> URL: https://gitlist.dev/e/1399380109-3805-6-git-send-email-nod.helm%40gmail.com In-Reply-To: <1399380109-3805-1-git-send-email-nod.helm@gmail.com> ``` git:Documentation/Makefile and others establish "RM ?= rm -f" as a convention for rm calls in clean rules, hence follow this convention instead of simply forcing clean to use rm. subproj and mainline no longer need to be removed in clean, as they are no longer created in git:contrib/subtree by "make test". Hence, remove the rm call for those folders. Other makefiles don't remove "*~" files, remove the rm call to prevent unexpected behaviour in the future. Similarly, clean doesn't remove the installable file, so rectify this. Reviewed-by: Jeff King Signed-off-by: James Denholm --- contrib/subtree/Makefile | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile index f3834b5..d888d45 100644 --- a/contrib/subtree/Makefile +++ b/contrib/subtree/Makefile @@ -12,7 +12,8 @@ man1dir ?= $(mandir)/man1 -include ../../GIT-VERSION-FILE # this should be set to a 'standard' bsd-type install program -INSTALL ?= install +INSTALL ?= install +RM ?= rm -f ASCIIDOC = asciidoc XMLTO = xmlto @@ -60,7 +61,7 @@ test: $(MAKE) -C t/ test clean: - rm -f *~ *.xml *.html *.1 - rm -rf subproj mainline + $(RM) $(GIT_SUBTREE) + $(RM) *.xml *.html *.1 .PHONY: FORCE -- 1.9.2 ``` ## Junio C Hamano, 2014-05-06 21:17 Subject: Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass Message-ID: URL: https://gitlist.dev/e/xmqq7g5yehf4.fsf%40gitster.dls.corp.google.com In-Reply-To: <1399380109-3805-1-git-send-email-nod.helm@gmail.com> ``` James Denholm writes: > contrib/subtree/Makefile is a shambles in regards to it's consistency > with other makefiles, which makes subtree overly painful to include in > build scripts. > > The main issues are that calls are made to git itself in the build > process, and that a subtree-exclusive variable is used for specifying > the exec path. Patches 1/5 through 3/5 resolve these. > > The "cleanup" fixes (4/5 and 5/5) are based on precedents set by other > makefiles across the project. > > One problem is foreseen: 3/5 will necessitate that package maintainers > who already have git-subtree included in their packages update their > build-scripts. > > Reviewed-by: Jeff King > Signed-off-by: James Denholm > Based-on-patch-by: Dan McGee It is funny to see sign-off on 0/5 ;-) By the way, this is v3, not v2, no? It was somewhat confusing to see Peff saying "filfre to add my reviewed-by" on v2, noticing you posted something new, and not finding v3. Will queue. Thanks. > > James Denholm (5): > contrib/subtree/Makefile: scrap unused $(gitdir) > contrib/subtree/Makefile: Use GIT-VERSION-FILE > contrib/subtree/Makefile: s/libexecdir/gitexecdir > contrib/subtree/Makefile: Doc-gen rules cleanup > contrib/subtree/Makefile: clean rule cleanup > > contrib/subtree/Makefile | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) ``` ## James Denholm, 2014-05-06 21:46 Subject: Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass Message-ID: URL: https://gitlist.dev/e/fa80985d-6420-4eb5-8623-00e2d8e8e798%40email.android.com In-Reply-To: ``` Junio C Hamano wrote: >It is funny to see sign-off on 0/5 ;-) Yeah, I wasn't quite sure of exact protocol, and sort-of defaulted to sign-all-the-things mode. >By the way, this is v3, not v2, no? It was somewhat confusing to >see Peff saying "filfre to add my reviewed-by" on v2, noticing you >posted something new, and not finding v3. Ah, right. I thought that resending a post-discussion patch was the done thing, given Documentation/SubmittingPatches, but that a comment line might not have been worth a version bump. >Will queue. Thanks. Awesome, thanks. Regards, James Denholm. ```