Re: [PATCH 2/5] Add platform-independent .git "symlink"

From: Lars Hjemli <hjemli@gmail.com>
Date: Feb 18, 2008, 13:18 UTC
Message-ID: <8c5c35580802180518x5c392bddya21c48ea4db9a7cd@mail.gmail.com>
In-Reply-To: <alpine.LSU.1.00.0802181217580.30505@racer.site>

On Feb 18, 2008 1:34 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 24 quoted lines

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

Show 6 quoted lines

> [* 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?

Show 10 quoted lines

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