On Thu, Dec 27, 2018 at 8:24 AM Jeff King <peff@xxxxxxxx> 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
