git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Erin Dahlgren <eedahlgren@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.
Date: Wed, 19 Dec 2018 10:59:28 -0500	[thread overview]
Message-ID: <20181219155928.GE14802@sigill.intra.peff.net> (raw)
In-Reply-To: <CAP_Smy14j4WK-mkqdKTKue=j7YoNjfaZVCBA-7S8xwNqX2rKhQ@mail.gmail.com>

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

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

> 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

  reply	other threads:[~2018-12-19 15:59 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-13 17:30 [PATCH] " Erin Dahlgren
2018-12-14 10:32 ` Johannes Schindelin
2018-12-16  1:05   ` Erin Dahlgren
2018-12-16  1:05 ` [PATCH v2] " Erin Dahlgren
2018-12-18 12:35   ` Johannes Schindelin
2018-12-18 19:50     ` Erin Dahlgren
2018-12-18 17:54   ` Jeff King
2018-12-18 20:54     ` Erin Dahlgren
2018-12-19 15:59       ` Jeff King [this message]
2018-12-26 22:22         ` Junio C Hamano
2018-12-27 16:24           ` Jeff King
2018-12-27 23:46             ` Erin Dahlgren
2019-01-03  4:54               ` Jeff King
2018-12-27 23:36   ` [PATCH v3] " Erin Dahlgren
2019-01-03  5:14     ` Jeff King
2019-01-03 18:09       ` Junio C Hamano
2019-01-04  8:25         ` Jeff King
2019-01-05 16:57           ` Erin Dahlgren
2019-01-06  6:22             ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181219155928.GE14802@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=eedahlgren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --subject='Re: [PATCH v2] Simplify handling of setup_git_directory_gently() failure cases.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git