Jeff King <peff@xxxxxxxx> writes: > On Tue, Dec 18, 2018 at 12:54:02PM -0800, Erin Dahlgren wrote: > ... >> 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@xxxxxxxxxxxxxxxxxxxxx/ > > 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). > >> 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)? > 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). Sounds good. Thanks both.
