Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.

From: Jeff King <peff@peff.net>
Date: Dec 19, 2018, 15:59 UTC
Message-ID: <20181219155928.GE14802@sigill.intra.peff.net>
In-Reply-To: <CAP_Smy14j4WK-mkqdKTKue=j7YoNjfaZVCBA-7S8xwNqX2rKhQ@mail.gmail.com>

On Tue, Dec 18, 2018 at 12:54:02PM -0800, Erin Dahlgren wrote:

Show 27 quoted lines

> > Might it make sense to make GIT_DIR_HIT_MOUNT_POINT more like
> > GIT_DIR_HIT_CEILING currently is, rather than the other way around?
> > I.e., something like:
> >
> >   case GIT_DIR_HIT_CEILING:
> >         if (!nongit_ok)
> >                 die(...);
> >         *nongit_ok = 1;
> >         prefix = NULL;
> >         break;
> >   case GIT_DIR_HIT_MOUNT_POINT:
> >         if (!nongit_ok)
> >                 die(...);
> >         *nongit_ok = 1;
> >         prefix = NULL;
> >         break;
> >
> > ?
> 
> After some digging, there seems to be a documented guarantee that
> "`GIT_PREFIX` is set as returned by running 'git rev-parse
> --show-prefix'". See Documentation/config/alias.txt. If the
> environment variable GIT_PREFIX is already set to something and we
> don't clear it when prefix is NULL, then the two can be out of sync.
> So that's a problem with my patch and the current handling of
> GIT_DIR_HIT_MOUNT_POINT. I'm not sure how important of a guarantee it
> is, but we should respect what's documented.

Yeah, agreed.

Another benefit of avoiding the early return is that we hit the cleanup code at the end of the function. That saves us having to release "dir" here. I assume we don't have to care about "gitdir" in these cases, but hitting the cleanup code means we don't even have to think about it.

Over in:

  https://public-inbox.org/git/20181219154853.GC14802@sigill.intra.peff.net/

I suggested adding more cleanup to that block, too (though I _think_ in these cases it would also be a noop, it's again nice to not have to worry about it).

Show 7 quoted lines

> Side note: One of the secondary goals of my patch was to make it
> really obvious that neither the GIT_DIR_HIT_CEILING nor the
> GIT_DIR_HIT_MOUNT_POINT case can get us into the block protected by
> (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)). With
> your suggestion (and it's a fair one) I don't think that's feasible
> without more significant refactoring. Should I just settle with a
> comment that explains this?

Yeah, I think a comment would probably be sufficient. Though we could also perhaps make the two cases (i.e., we found a repo or not) more clear. Something like:

  if (!*nongit_ok || !*nongit_ok) {
	startup_info->have_repository = 1;
	startup_info->prefix = prefix;
	if (prefix) ...etc...
  } else {
	start_info->have_repository = 0;
	startup_info->prefix = NULL;
	setenv(GIT_PREFIX_ENVIRONMENT, "", 1);
  }

That may introduce some slight repetition, but I think it's probably clearer to deal with the cases separately (and it saves earlier code from make-work like setting prefix=NULL when there's no repo).

The condition would also be a lot less confusing if nongit_ok were flipped, but I think that would be a big refactor due to the way we pass it around.

-Peff