[PATCH] subtree/Makefile: Standardize (esp. for packagers)

[PATCH] subtree/Makefile: Standardize (esp. for packagers)

[nod.helm]

From: James Denholm <nod.helm@gmail.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.

Two major issues are present:

Firstly, calls to git itself (for $(gitdir) and $(gitver)), making
building difficult on systems that don't have git.

Secondly, the Makefile uses the variable $(libexecdir) for defining the
exec path.

To fix:

1: Scrap unused $(gitdir) assignment
    References were removed in 7ff8463dba0d74fc07a766bed457ae7afcc902b5,
    but the assignment itself wasn't. Ergo, $(gitdir) hasn't actually
    been using it since then.

2: Use GIT-VERSION-FILE for version info, from $(gitver)
    GVF is already being used in most/all other makefiles in the
    project, and has been for _quite_ a while.

3: :%s/libexecdir/gitexecdir/g
    $(libexecdir) isn't used anywhere else in the project, while
    $(gitexecdir) is the standard.

On minor fixes, also fiddled with clean rule and a few calls/variables
to improve congruency with other git makefiles.

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

Obligatory "first patch ever, yay, hello" exclamation goes here,
random misspelt words and/or life story optional.

I've left `rm -f -r subproj mainline` in the clean rule for now, however
I'd suggest those actually belong in contrib/subtree/t/Makefile:clean,
given that they are only ever generated by `make test`. But given that
there aren't any other comparable setups in contrib/, I'm somewhat
apprehensive to move them without opinion.

Anyway, hopefully this might make some distros more inclined to package
git-subtree in their canonical git packages. A very special thanks to
Dan, who proposed the initial patch back in 2012-or-so that this is
largely based off of.

 contrib/subtree/Makefile | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile
index 4030a16..e1956b8 100644
--- a/contrib/subtree/Makefile
+++ b/contrib/subtree/Makefile
@@ -3,17 +3,23 @@
 
 prefix ?= /usr/local
 mandir ?= $(prefix)/share/man
-libexecdir ?= $(prefix)/libexec/git-core
-gitdir ?= $(shell git --exec-path)
+gitexecdir ?= $(prefix)/libexec/git-core
 man1dir ?= $(mandir)/man1
 
-gitver ?= $(word 3,$(shell git --version))
+../../GIT-VERSION-FILE: FORCE
+	$(MAKE) -C ../../ GIT-VERSION-FILE
 
-# this should be set to a 'standard' bsd-type install program
-INSTALL ?= install
+-include ../../GIT-VERSION-FILE
 
-ASCIIDOC_CONF      = ../../Documentation/asciidoc.conf
-MANPAGE_NORMAL_XSL =  ../../Documentation/manpage-normal.xsl
+# These should be set to 'standard' bsd-type programs
+INSTALL  ?= install
+RM       ?= rm -f
+
+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
@@ -31,8 +37,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
 
@@ -41,19 +47,22 @@ 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) \
-		-agit_version=$(gitver) $^
+	$(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) \
-		-agit_version=$(gitver) $^
+	$(ASCIIDOC) -b xhtml11 -d manpage -f $(ASCIIDOC_CONF) \
+		-agit_version=$(GIT_VERSION) $^
 
 test:
 	$(MAKE) -C t/ test
 
 clean:
-	rm -f *~ *.xml *.html *.1
-	rm -rf subproj mainline
+	$(RM) $(GIT_SUBTREE)
+	$(RM) *.xml *.html *.1
+	$(RM) -r subproj mainline
+
+.PHONY: FORCE
-- 
1.9.2

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[nod.helm]

On 24 Apr 2014 11:52, <nod.helm@gmail.com> wrote:
>
> From: James Denholm <nod.helm@gmail.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.
>
> Two major issues are present:
>
> Firstly, calls to git itself (for $(gitdir) and $(gitver)), making
> building difficult on systems that don't have git.
>
> Secondly, the Makefile uses the variable $(libexecdir) for defining the
> exec path.
>
> (...)

I hate to be that guy, but could I get an opinion on the proposed patch? Is
git
interested in purely makefile patches,
or should I find further improvements
to make in subtree and purpose this again with those?

Regards,
James Denholm.

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[Jeff King]

On Sat, Apr 26, 2014 at 02:56:15PM +1000, nod.helm@gmail.com wrote:

> > 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.
> >
> > Two major issues are present:
> >
> > Firstly, calls to git itself (for $(gitdir) and $(gitver)), making
> > building difficult on systems that don't have git.
> >
> > Secondly, the Makefile uses the variable $(libexecdir) for defining the
> > exec path.
> >
> > (...)
> 
> I hate to be that guy, but could I get an opinion on the proposed patch?

It's OK to be that guy; prompting or reposting when a patch has been
overlooked is normal here.

> Is git interested in purely makefile patches, or should I find further
> improvements to make in subtree and purpose this again with those?

Makefile improvements are fine on their own. I think the problem is that
contrib/subtree does not really have an active dedicated area
maintainer.

Your changes look fine to me from a cursory examination. It would
probably be more readable as four patches (the 3 "fix" points from your
list, plus the "minor fixes" mentioned at the end). Then each patch
stands on its own, can say what problem it's fixing, and how.

> I've left `rm -f -r subproj mainline` in the clean rule for now,
> however I'd suggest those actually belong in
> contrib/subtree/t/Makefile:clean, given that they are only ever
> generated by `make test`. But given that there aren't any other
> comparable setups in contrib/, I'm somewhat apprehensive to move them
> without opinion.

Do we even make those directories anymore? It looks like they are part
of the tests, but the whole test script runs inside its own trash
directory. I wonder if they are vestiges from the time when subtree was
its own repository outside of contrib/. If so, they can be dropped here
(and from .gitignore).

-Peff

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[James Denholm]

Jeff King <peff@peff.net> wrote:
>I think the problem is that
>contrib/subtree does not really have an active dedicated area
>maintainer.

Yeah, I can see how that might become a bit of a problem. I was
actually thinking of doing a bit of work on subtree beyond this
specific patch, so hopefully that won't be a show-stopper. We'll
see what happens, I guess.

>Your changes look fine to me from a cursory examination. It would
>probably be more readable as four patches (the 3 "fix" points from your
>list, plus the "minor fixes" mentioned at the end). Then each patch
>stands on its own, can say what problem it's fixing, and how.
>
> (...)
>
>Do we even make [subproject and mainline] anymore? It looks like they are part
>of the tests, but the whole test script runs inside its own trash
>directory.

subproject and mainline are actually made in  contrib/subtree,
but I'll look at perhaps "fixing" that when I split the proposal
into a series as you suggest.

Thanks for the advice!

Regards,
James Denholm.

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[Jeff King]

On Sun, Apr 27, 2014 at 12:35:13PM +1000, James Denholm wrote:

> >Do we even make [subproject and mainline] anymore? It looks like they are part
> >of the tests, but the whole test script runs inside its own trash
> >directory.
> 
> subproject and mainline are actually made in  contrib/subtree,
> but I'll look at perhaps "fixing" that when I split the proposal
> into a series as you suggest.

Are they? I couldn't find any reference to them as directories except in
the test script, and doing a "make" from contrib/subtree didn't create
them. I'll leave it to you to investigate further whether the "clean"
rules are cruft or not, but certainly if they are, cleaning up cruft is
a good thing.

-Peff

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[James Denholm]

Jeff King <peff@peff.net> wrote:
>On Sun, Apr 27, 2014 at 12:35:13PM +1000, James Denholm wrote:
>
>> >Do we even make [subproject and mainline] anymore? It looks like
>they are part
>> >of the tests, but the whole test script runs inside its own trash
>> >directory.
>> 
>> subproject and mainline are actually made in  contrib/subtree,
>> but I'll look at perhaps "fixing" that when I split the proposal
>> into a series as you suggest.
>
>Are they? I couldn't find any reference to them as directories except
>in
>the test script, and doing a "make" from contrib/subtree didn't create
>them.

Yeah, I could be wrong as I don't have the code on-hand at the moment,
but from memory they're made and populated by the test script as
well. Either way I'll move all that into a subdir of t/ and rejigger the thing, if
investigation reveals that as the Done Thing.

>I'll leave it to you to investigate further whether the "clean"
>rules are cruft or not, but certainly if they are, cleaning up cruft is
>a good thing.

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[Matthew Ogilvie]

On Sun, Apr 27, 2014 at 12:35:13PM +1000, James Denholm wrote:
> Jeff King <peff@peff.net> wrote:
> >I think the problem is that
> >contrib/subtree does not really have an active dedicated area
> >maintainer.
> 
> Yeah, I can see how that might become a bit of a problem. I was
> actually thinking of doing a bit of work on subtree beyond this
> specific patch, so hopefully that won't be a show-stopper. We'll
> see what happens, I guess.

Agreed.  It also doesn't help that when subtree patches are proposed
(especially new features instead of obvious bugs), there often seems
to be little or no feedback from anyone.

--------
Depending on how much time you have:

This may be outside the scope of work you were planning on, but
it might be worth grepping through old mailing list archives for
"subtree" patches that haven't been merged, and see if there is
anything worth revisiting/resubmitting.  I believe most of the
following (at least) kind of languished and died, often with little
or no real review and feedback:

http://marc.info/?l=git&m=138644067726844&w=2
http://marc.info/?l=git&m=138523794407181&w=2
  - My own series, plus another patch that has roughly the
    same description, but different semantics.

http://marc.info/?l=git&m=136321400525507&w=2
http://marc.info/?l=git&m=136321400525507&w=2
  - Some series from Paul Campbell.

http://marc.info/?l=git&m=136122107605036&w=2
http://marc.info/?l=git&m=135813589922554&w=2
http://marc.info/?l=git&m=136415434127550&w=2
http://marc.info/?l=git&m=136127692217856&w=2
  - Other series.

http://marc.info/?l=git&m=138557714926045&w=2
http://marc.info/?l=git&m=138129106613560&w=2
http://marc.info/?l=git&m=136415882128742&w=2
http://marc.info/?l=git&m=136415654228062&w=2
  - Miscellaneous

And probably others...

(I don't know if these are the latest or "best" versions of these, nor
have I really looked at them closely to decide if they are worth
including at all.  Be sure to exameine not just the discussion around
the specific patches, but also the other patches in each series...)

                     - Matthew Ogilvie

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[James Denholm]

Matthew Ogilvie <mmogilvi_git@miniinfo.net> wrote:
> On Sun, Apr 27, 2014 at 12:35:13PM +1000, James Denholm wrote:
>> Jeff King <peff@peff.net> wrote:
> Agreed.  It also doesn't help that when subtree patches are proposed
> (especially new features instead of obvious bugs), there often seems
> to be little or no feedback from anyone.
>
> --------
> Depending on how much time you have:
>
> This may be outside the scope of work you were planning on,

While current, immediate focus is really just getting the makefile fixed
up and hopefully then have more people package subtree by default,
overall I'll very likely extend that to general work on subtree and such.

>                                                                                          but
> it might be worth grepping through old mailing list archives for
> "subtree" patches that haven't been merged, and see if there is
> anything worth revisiting/resubmitting.  I believe most of the
> following (at least) kind of languished and died, often with little
> or no real review and feedback:
>
> (...)
>
> (I don't know if these are the latest or "best" versions of these, nor
> have I really looked at them closely to decide if they are worth
> including at all.  Be sure to exameine not just the discussion around
> the specific patches, but also the other patches in each series...)

Yeah, certainly, I'll be sure to have a sticky-beak. Thanks for pointing
those out!

Regards,
James Denholm.

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[Felipe Contreras]

James Denholm wrote:
> Matthew Ogilvie <mmogilvi_git@miniinfo.net> wrote:
> > On Sun, Apr 27, 2014 at 12:35:13PM +1000, James Denholm wrote:
> >> Jeff King <peff@peff.net> wrote:
> > Agreed.  It also doesn't help that when subtree patches are proposed
> > (especially new features instead of obvious bugs), there often seems
> > to be little or no feedback from anyone.
> >
> > --------
> > Depending on how much time you have:
> >
> > This may be outside the scope of work you were planning on,
> 
> While current, immediate focus is really just getting the makefile fixed
> up and hopefully then have more people package subtree by default,
> overall I'll very likely extend that to general work on subtree and such.

I think you should take a look at the Makefile of
contrib/remote-helpers. I bet something simple like that would work just
fine for subtree.

-- 
Felipe Contreras

Re: [PATCH] subtree/Makefile: Standardize (esp. for packagers)

[James Denholm]

On 4 May 2014 05:22:48 GMT+10:00, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>I think you should take a look at the Makefile of
>contrib/remote-helpers. I bet something simple like that would work
>just
>fine for subtree.

The current makefile is simple enough, just quirky and likes
to be a special snowflake. 'sall good, the v2 addresses most
of my immediate concerns with it.

Regards,
James Denholm.