On Mon, Feb 20, 2012 at 6:14 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> 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 :)).
> 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.
> 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.
> 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.
> 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
--
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