On Sat, Apr 26, 2014 at 02:56:15PM +1000, nod.helm@xxxxxxxxx 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html