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 v3] Simplify handling of setup_git_directory_gently() failure cases.
Date: Thu, 3 Jan 2019 00:14:33 -0500	[thread overview]
Message-ID: <20190103051432.GE20047@sigill.intra.peff.net> (raw)
In-Reply-To: <1545953789-15040-1-git-send-email-eedahlgren@gmail.com>

On Thu, Dec 27, 2018 at 03:36:29PM -0800, Erin Dahlgren wrote:

> Before this change are two misleading additional behaviors:
> 
>   - GIT_DIR_HIT_CEILING: setup_nongit() changes to the cwd for no
> 	apparent reason. We never had the chance to change directories
> 	up to this point so chdir(current cwd) is pointless.
>   - GIT_DIR_HIT_MOUNT_POINT: strbuf_release() frees the buffer
> 	of a static struct strbuf (cwd). This is unnecessary because the
> 	struct is static so its buffer is always reachable. This is also
> 	misleading because nowhere else in the function is this buffer
> 	released.
> [..]

Overall this looks good to me, and I'd be fine to see it go in as-is.

A few minor nits/comments, though:

> During review, this change was amended to additionally include:
> [...]

When I find myself writing big bullet lists of changes in a commit
message, that is often a good time to split the commit into several
sub-parts.

This patch isn't _too_ big, so I don't think it's worth the effort at
this point for this particular case, but just something to think about
for the future. A series around this topic would probably be something
like:

  1: drop the useless chdir and fold setup_nongit() into the main
     function

  2: stop doing the early return from HIT_MOUNT_POINT, and treat it just
     like HIT_CEILING (and drop the useless strbuf release)

  3: treat GIT_DIR_NONE as a BUG

  4: rearrange the nongit logic at the end of the function for clarity

> +	/*
> +	 * At this point, nongit_ok is stable. If it is non-NULL and points
> +	 * to a non-zero value, then this means that we haven't found a
> +	 * repository and that the caller expects startup_info to reflect
> +	 * this.

Right, makes sense.

> +	 *
> +	 * Regardless of the state of nongit_ok, startup_info->prefix and
> +	 * the GIT_PREFIX environment variable must always match. For details
> +	 * see Documentation/config/alias.txt.
> +	 */

I think this "must match" makes sense to comment. I didn't find
config/alias.txt particularly enlightening in that regard, though. :)

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

We usually avoid "//" comments, even for single lines (though that is
perhaps superstition at this point, as we've started to embrace several
other C99-isms). IMHO this particular comment is not even really
necessary, as the whole conditional is suitably short and obvious after
your refactor.

> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
>  	 * the user has set GIT_DIR.  It may be beneficial to disallow bogus
>  	 * GIT_DIR values at some point in the future.
>  	 */
> -	if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
> +	if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
> +	    startup_info->have_repository ||
> +	    // GIT_DIR_EXPLICIT
> +	    getenv(GIT_DIR_ENVIRONMENT)) {

Same "//" style issue as above. I'm not sure how much value there is in
repeating the GIT_DIR_* cases here, as they're likely to grow out of
sync with the switch() statement above.

At first I thought this could all be folded into the "else" clause of
the conditional above (which would make the logic much easier to
follow), but that wouldn't cover the case of GIT_DIR=/bogus, which is
what that getenv() is trying to handle here.

So I think this is the best we can do for now.

-Peff

  reply	other threads:[~2019-01-03  5:14 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
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 [this message]
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=20190103051432.GE20047@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 v3] 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