list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <>
To: Jeff King <>
Cc: Erin Dahlgren <>,,
	Johannes Schindelin <>
Subject: Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.
Date: Thu, 03 Jan 2019 10:09:18 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <> (Jeff King's message of "Thu, 3 Jan 2019 00:14:33 -0500")

>Subject: Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.


	Subject: setup: simplify setup_git_directory_gently() failure cases

to clarify which part of the entire world this patch is touching.

Jeff King <> writes:

> 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

Yeah, that organization does make sense.  And I agree with your rule
of thumb to use the length and complexity of the log message to
judge if a single step is getting too big.

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

FWIW "git grep" for // seems to show that we reserve use of //
inside commented out code samples.

I also agree the comments behind // in this patch are probably

>> @@ -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)) {
>> +	    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.

It is unclear to me if the original code is doing the right thing
under one condition, and this patch does not seem to change the

What happens if GIT_DIR environment is set to an invalid path and
nongit_ok is non-NULL?  setup_explicit_git_dir() would have assigned
1 to *nongit_ok, so have_repository is false at this point.

We enter the if() statement in such a case, and end up calling
setup_git_env(gitdir) using the bogus path that is not pointing at a
repository.  We leave have_repository to be false but the paths
recorded in the_repository by setup_git_env() would all point at
bogus places.

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

Yes, but should GIT_DIR=/bogus even be touching the_repository?  

It is a separate clean-up and does not affect the validity of this
simplification patchd, so I agreee with ...

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


  reply	other threads:[~2019-01-03 18:09 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-13 17:30 [PATCH] Simplify handling of setup_git_directory_gently() failure cases 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
2019-01-03 18:09       ` Junio C Hamano [this message]
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:

  List information:

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

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).