Re: git-subtree Ready #2
- From
David A. Greene <greened@obbligato.org>
- Date
- Feb 21, 2012, 05:37 UTC
- Message-ID
- <87ehtowxu7.fsf@smith.obbligato.org>
- In-Reply-To
- <7vd399jdwc.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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.
Show 7 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. 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.
Show 32 quoted lines
> ----------------------------------------------------------------
> 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.
Show 12 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): > > 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