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

[Erin Dahlgren <eedahlgren@gmail.com>]

On Thu, Dec 27, 2018 at 8:24 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Dec 26, 2018 at 02:22:39PM -0800, Junio C Hamano wrote:
>
> > >> 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) {
> >
> > WTH is (A || A)?
>
> Heh, I should learn to cut and paste better. This should be:
>
>   if (!nongit_ok || !*nongit_ok)
>
> (which comes from the current code).

Yep, but I think we can benefit from De Morgan's law here, where:

  (!nongit_ok || !*nongit_ok) == (!(nongit_ok && *nongit_ok))

PATCH v3 (just sent) uses that transformation like this:

if (nongit_ok && *nongit_ok) {
  ... startup_info->has_repository = 0;
} else {
  // !nongit_ok || !*nongit_ok
  .. startup_info->has_repository = 1;
}

Because IMHO (nongit_ok && *nongit_ok) is easier to read and reason
about. Added brief comments as well.

Thanks.

- Erin

>
> -Peff