On Feb 18, 2008 1:34 PM, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote:
> Hi,
>
> On Mon, 18 Feb 2008, Lars Hjemli wrote:
>
> > diff --git a/environment.c b/environment.c
> > index 3527f16..8058e7b 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -49,6 +49,8 @@ static void setup_git_env(void)
> > {
> > git_dir = getenv(GIT_DIR_ENVIRONMENT);
> > if (!git_dir)
> > + git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> > + if (!git_dir)
> > git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
>
> I still maintain that the code (maybe not the diff) is easier to read like
> this:
>
> if (!git_dir) {
> git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> if (!git_dir)
> git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> }
IMHO such constructs are butt ugly, but if there's consensus for your
way, I'll abide...
> [* snip *]
>
> So maybe we can just scratch all that, and I agree that an invalid .git
> file means "no repository" (as opposed to "no valid repository").
>
> In that case, you might want to test for that, too...
I think I do, did you find a loophole in the testing?
>
> Speaking about tests:
>
> > +test_expect_success 'setup' '
> > + REAL="$(pwd)/.real" &&
> > + mv .git "$REAL" &&
> > + echo "gitdir: $REAL" >.git
> > +'
>
> Let's not do this. It would clutter the t/ directory unnecessarily.
What do you mean? The test just moves t/trash/.git to t/trash/.real...
--
larsh
-
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
