Re: git-subtree Ready #2

[greened@obbligato.org (David A. Greene)]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
> 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.

Ok, but we will preserve the history via the subtree merge, yes?

> The total amount of change does not look too bad, either:

Yes, it's a fairly small tool.

> It does look like it needs to start its life in contrib/ if we were to put
> this in git.git. 

That sounds good to me.  It should get a good shakedown before graduating.

> 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.

Ok.

> 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.

Right, I intentially designed it that way.

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

Ok, I'll propose those changes separately.  They are a prerequisite for
a git-subtree that is easily testable while in contrib.

> ----------------------------------------------------------------
> 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.

I'll put a patch together with a more extensive explanation.  Basically,
tests run outside of the top-level t/ directory don't work because there
are all sort of assumptions in test-lib.sh about where they live.  There
are comments in test-lib.sh indicating that it should support tests in
other directories but I could not make it work out of the box.

> 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

Ok, these are Avery's commits.  I don't know that I have enough context
to improve the logs but I will look throught revisions and try to figure
things out.  Avery, could you be of any help here?  It sounds like we
need more descriptive log messages.

                               -Dave