Re: git-subtree Ready #2
- From
Avery Pennarun <apenwarr@gmail.com>
- Date
- Feb 24, 2012, 01:19 UTC
- Message-ID
- <CAHqTa-2s1xbAfNvjD7cXBe2TBMs1985nag1NOYVfE+dATvfEWA@mail.gmail.com>
- In-Reply-To
- <7vd399jdwc.fsf@alter.siamese.dyndns.org>
On Mon, Feb 20, 2012 at 6:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> It sounds like the simplest and cleanest would be to treat it as if its > current version came as a patch submission, cook it just like any other > topic in 'pu' down to 'next' down to eventually 'master', with the usual > review cycle of pointing out what is wrong and needs fixing followed by a > series of re-rolls.
Yeah, my original intent with git-subtree was to one day submit it as basically a single patch against git. That's why I have some slightly suspicious commit messages in there (though in my defense, I think *most* of the commit messages are quite sensible :)).
Show 6 quoted lines
> After looking at the history of subtree branch there, however, I agree > that it would not help anybody to have its history in my tree with log > messages like these (excerpt from shortlog output): > [...] > Docs: when pushing to github, the repo path needs to end in .git > [...]
That commit message in particular I thought was perfectly fine; it's specifically a fix to the git-subtree docs to clarify a question from an actual user.
Overall I agree that there's little benefit in preserving the history, at least as far as I can see, *except* that some code changes were submitted by people other than me and squashing those changes might conceivably cause licensing confusion down the road. It's probably a fairly quick exercise with git-filter-branch to get rid of the more egregious commit message problems, if that's what we want to do. (In particular, just expurgating the entire 'todo' file from the history probably makes plenty of sense.)
There's no value I can see in being able to do future merges from outside the tree, so a filter-branch or rebase before merging should be pretty harmless.
Show 14 quoted lines
> The total amount of change does not look too bad, either: > > $ git diff --stat master...origin/subtree > contrib/subtree/.gitignore | 5 + > contrib/subtree/COPYING | 339 +++++++++++++++++ > contrib/subtree/Makefile | 45 +++ > contrib/subtree/README | 8 + > contrib/subtree/git-subtree.sh | 712 ++++++++++++++++++++++++++++++++++++ > contrib/subtree/git-subtree.txt | 366 ++++++++++++++++++ > contrib/subtree/t/Makefile | 71 ++++ > contrib/subtree/t/t7900-subtree.sh | 508 +++++++++++++++++++++++++ > contrib/subtree/todo | 50 +++ > t/test-lib.sh | 11 +- > 10 files changed, 2114 insertions(+), 1 deletion(-)
Note that COPYING, .gitignore, Makefile, t/Makefile, todo, and README should probably be ditched if it weren't going into contrib. The interesting files are git-subtree.{sh,txt} and t7900-subtree.sh.
Show 5 quoted lines
> I haven't looked at the script fully, but it has an issue > from its first line, which is marked with "#!/bin/bash". It is unclear if > it is infested by bash-isms beyond repair (in which case "#!/bin/bash" is > fine), or it was written portably but was marked with "#!/bin/bash" just > by inertia.
I'm generally pretty careful to avoid bashisms, but since my /bin/sh is bash, I usually mark scripts with /bin/bash just to be safe until someone has actually verified them with a non-bash shell. I expect few if any problems with that part.
Show 6 quoted lines
> A patch that corresponds to the above diffstat immediately > shows many style issues including trailing eye-sore whitespaces. > > It seems that it is even capable of installing from contrib/subtree, so > keeping it in contrib/ while many issues it may have gets fixed would not > hurt the original goal of giving the script more visibility.
Personally, I would prefer to just iterate the patch a few times to correct the coding style problems you see, rather than merging into contrib where it might be forgotten rather than fixed. As Peff alluded, people who want to install it separately from git already can; if we're going to merge it into git, let's do it right.
Have fun,
AVery