[PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[James Denholm]

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 <peff@peff.net>
Signed-off-by: James Denholm <nod.helm@gmail.com>
Based-on-patch-by: Dan McGee <dpmcgee@gmail.com>

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

[PATCH v2 1/5] contrib/subtree/Makefile: scrap unused $(gitdir)

[James Denholm]

In 7ff8463dba0d74fc07a766bed457ae7afcc902b5, the references to gitdir
were removed but the assignment itself wasn't. Hence, drop the gitdir
assignment.

Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: James Denholm <nod.helm@gmail.com>
---
 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

[PATCH v2 2/5] contrib/subtree/Makefile: Use GIT-VERSION-FILE

[James Denholm]

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 <peff@peff.net>
Signed-off-by: James Denholm <nod.helm@gmail.com>
---
 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

[PATCH v2 3/5] contrib/subtree/Makefile: s/libexecdir/gitexecdir

[James Denholm]

$(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 <peff@peff.net>
Signed-off-by: James Denholm <nod.helm@gmail.com>
---
 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

[PATCH v2 4/5] contrib/subtree/Makefile: Doc-gen rules cleanup

[James Denholm]

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 <peff@peff.net>
Signed-off-by: James Denholm <nod.helm@gmail.com>
---
 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

[PATCH v2 5/5] contrib/subtree/Makefile: clean rule cleanup

[James Denholm]

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 <peff@peff.net>
Signed-off-by: James Denholm <nod.helm@gmail.com>
---
 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

Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[Junio C Hamano]

James Denholm <nod.helm@gmail.com> 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 <peff@peff.net>
> Signed-off-by: James Denholm <nod.helm@gmail.com>
> Based-on-patch-by: Dan McGee <dpmcgee@gmail.com>

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

Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[James Denholm]

Junio C Hamano <gitster@pobox.com> 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.

[PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[James Denholm]

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.

Signed-off-by: James Denholm <nod.helm@gmail.com>
Based-on-patch-by: Dan McGee <dpmcgee@gmail.com>

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 | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

-- 
1.9.2

Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[Jeff King]

On Sat, May 03, 2014 at 10:49:30PM +1000, James Denholm wrote:

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

Thanks, these all look sane to me (I do not use subtree, but since it's
just about Makefiles, it was pretty easy to review).

> One problem is foreseen: 3/5 will necessitate that package maintainers
> who already have git-subtree included in their packages update their
> build-scripts.

I think that's probably OK. We strive for backwards compatibility in the
tool itself, but refactoring Makefiles in contrib/ affects a pretty
limited audience.

-Peff

Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[James Denholm]

On 5 May 2014 15:08:04 GMT+10:00, Jeff King <peff@peff.net> wrote:
>On Sat, May 03, 2014 at 10:49:30PM +1000, James Denholm wrote:
>
>> 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.
>
>Thanks, these all look sane to me (I do not use subtree, but since it's
>just about Makefiles, it was pretty easy to review).

Thanks for the review!

Given that subtree subtree doesn't really generate a lot of discussion,
would it be advisable to wrap this up (barring further discussion) and send
it off to Junio rather than waiting for further community consensus?

Regards,
James Denholm.

Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[Jeff King]

[fixed David's address in cc list]

On Tue, May 06, 2014 at 07:54:30AM +1000, James Denholm wrote:

> Given that subtree subtree doesn't really generate a lot of discussion,
> would it be advisable to wrap this up (barring further discussion) and send
> it off to Junio rather than waiting for further community consensus?

I do not know if "lack of discussion" is a good reason to consider
something in good shape; oftentimes it is a sign that nobody is
interested in the area. We usually rely on "area maintainers" to give an
OK to the patches if they are not something that the maintainer himself
has an interest in.

However, in this case, you did get review, and I think it is pretty easy
to see the patches are good even if one does not care about the
particular area. So I think they are fine to pass on and apply.

Note also that patches like this are a great place to get started, as
they help build trust in a contributor, who can later help out with
area maintenance.

-Peff

Re: [PATCH v2 0/5] contrib/subtree/Makefile: Standardisation pass

[James Denholm]

On 6 May 2014 08:01, Jeff King <peff@peff.net> wrote:
> [fixed David's address in cc list]

Ah, right. Wasn't sure what was going on there.

> On Tue, May 06, 2014 at 07:54:30AM +1000, James Denholm wrote:
>
>> Given that subtree subtree doesn't really generate a lot of discussion,
>> would it be advisable to wrap this up (barring further discussion) and send
>> it off to Junio rather than waiting for further community consensus?
>
> I do not know if "lack of discussion" is a good reason to consider
> something in good shape; oftentimes it is a sign that nobody is
> interested in the area. We usually rely on "area maintainers" to give an
> OK to the patches if they are not something that the maintainer himself
> has an interest in.

Yeah, I certainly only meant that in the context of this particular
patch, post-review.

> However, in this case, you did get review, and I think it is pretty easy
> to see the patches are good even if one does not care about the
> particular area. So I think they are fine to pass on and apply.

Sounds good, I'll send it on up now. Thanks again for the help.

> Note also that patches like this are a great place to get started, as
> they help build trust in a contributor, who can later help out with
> area maintenance.

Yeah, to be honest, beyond the immediate goal of getting subtree in more
distros, that is kinda the plan. A bit of a practical experience in
contributing to the project, learning the specific ropes and such
before proposing more substantial discussion and fixes/changes.

---
Regards,
James Denholm.