public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed
From: greened@obbligato•org (David A. Greene)
To: Junio C Hamano <gitster@pobox•com>
Cc: Jeff King <peff@peff•net>,
	git@vger•kernel.org, Avery Pennarun <apenwarr@gmail•com>
Subject: Re: git-subtree Ready #2
Date: Mon, 20 Feb 2012 23:37:52 -0600	[thread overview]
Message-ID: <87ehtowxu7.fsf@smith.obbligato.org> (raw)
In-Reply-To: <7vd399jdwc.fsf@alter.siamese.dyndns.org> (Junio C. Hamano's message of "Mon, 20 Feb 2012 15:14:43 -0800")

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

  reply	other threads:[~2012-02-21  5:40 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-11 17:35 git-subtree Ready #2 David A. Greene
2012-02-11 18:03 ` Junio C Hamano
2012-02-11 19:22   ` David A. Greene
2012-02-15  4:30 ` David A. Greene
2012-02-15  5:08   ` Jeff King
2012-02-15  5:31     ` David A. Greene
2012-02-16  4:07       ` David A. Greene
2012-02-20 19:34         ` David A. Greene
2012-02-20 20:53         ` Jeff King
2012-02-20 23:14           ` Junio C Hamano
2012-02-21  5:37             ` David A. Greene [this message]
2012-02-21  6:34               ` Junio C Hamano
2012-02-21  7:10                 ` Junio C Hamano
2012-02-21  8:44                 ` Junio C Hamano
2012-02-21  9:07               ` Thomas Rast
2012-02-24  1:19             ` Avery Pennarun
2012-02-24 20:56               ` Junio C Hamano
2012-02-24 23:57                 ` Avery Pennarun
2012-02-25  5:00                   ` David A. Greene
2012-02-25  9:00                     ` Junio C Hamano
2012-02-25 15:00                       ` David A. Greene
2012-02-27 21:06                         ` Junio C Hamano
2012-02-27 21:21                           ` Jeff King
2012-02-27 21:23                             ` Jeff King
2012-02-28  2:04                             ` Jakub Narebski
2012-02-28 22:42                               ` Avery Pennarun
2012-03-02  3:42                           ` David A. Greene
2012-02-21  5:31           ` David A. Greene

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87ehtowxu7.fsf@smith.obbligato.org \
    --to=greened@obbligato$(echo .)org \
    --cc=apenwarr@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=gitster@pobox$(echo .)com \
    --cc=peff@peff$(echo .)net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox