Re: git-subtree Ready #2

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2012, 23:14 UTC
Message-ID: <7vd399jdwc.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120220205346.GA6335@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

> On Wed, Feb 15, 2012 at 10:07:16PM -0600, David A. Greene wrote:
> ...
> Of course there's no real reason we can't take it slow by putting it in
> contrib, and then graduating from there. It just seems like an
> unnecessary and complicated interim step. Either way, I do think it's
> worth saving the commit history by doing a real merge.
>
> I dunno. It is really up to Junio, I guess.

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.

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

It does look like it needs to start its life in contrib/ if we were to put this in git.git. 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. 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.

The change to t/test-lib.sh should be made independent of this topic, I would think.

----------------------------------------------------------------

diff --git a/t/test-lib.sh b/t/test-lib.sh
index e28d5fd..c877a91 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -55,6 +55,7 @@ unset $(perl -e '
 		.*_TEST
 		PROVE
 		VALGRIND
+                BUILD_DIR
 	));
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
 	print join("\n", @vars);
@@ -924,7 +925,15 @@ then
 	# itself.
 	TEST_DIRECTORY=$(pwd)
 fi
-GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+
+if test -z "$GIT_BUILD_DIR"
+then
+    echo Here
+	# We allow tests to override this, in case they want to run tests
+	# outside of t/, e.g. for running tests on the test library
+	# itself.
+        GIT_BUILD_DIR="$TEST_DIRECTORY"/..
+fi
 
 if test -n "$valgrind"
 then
----------------------------------------------------------------
This change deserves its own justification.

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

      update todo
      Some todo items reported by pmccurdy
      todo
      Docs: when pushing to github, the repo path needs to end in .git
      todo
      todo^
      todo
      todo: idea for a 'git subtree grafts' command