Re: [PATCH v3] Add user.explicit boolean for when ident shouldn't be guessed

From: Dan Aloni <alonid@gmail.com>
Date: Feb 4, 2016, 05:36 UTC
Message-ID: <20160204053646.GA24453@gmail.com>
In-Reply-To: <20160204040111.GA27371@sigill.intra.peff.net>

On Wed, Feb 03, 2016 at 11:01:11PM -0500, Jeff King wrote:

Show 34 quoted lines

>[..]
> > > * Do these new tests really deserve a new test script, or would they
> > > fit into an existing script? (Genuine question.)
> > 
> > I am not sure. IMHO it makes sense to have a new test script for a new
> > feature.
> 
> I hope we never have more than 9999 features, then. :)
> 
> On to the patch itself...
> 
> > @@ -373,6 +374,21 @@ const char *fmt_ident(const char *name, const char *email,
> >  		die("unable to auto-detect email address (got '%s')", email);
> >  	}
> >  
> > +	if (ident_explicit) {
> > +		if (name == git_default_name.buf &&
> > +		    !(committer_ident_explicitly_given & IDENT_NAME_GIVEN) &&
> > +		    !(author_ident_explicitly_given & IDENT_NAME_GIVEN))
> > +			die("requested explicitly given ident in config, "
> > +			    "but user.name is not set, or environment is "
> > +			    "not set");
> > +		if (email == git_default_email.buf &&
> > +		    !(committer_ident_explicitly_given & IDENT_MAIL_GIVEN) &&
> > +		    !(author_ident_explicitly_given & IDENT_MAIL_GIVEN))
> > +			die("requested explicitly given ident in config, "
> > +			    "but user.email is not set, or environment is "
> > +			    "not set");
> > +	}
> > +
> 
> I'm not sure why this block is effectively repeated here, in
> git_author_info(), and in git_committer_info(). Don't the latter two
> just call fmt_ident?

Suspected that. Originally I started changing git_committer_info() and git_author_info(), and only changed fmt_ident() later when I saw it covers more cases. But the flow was confusing enough so I wasn't sure whether to keep it.

Show 13 quoted lines

> To be honest, I had expected something much simpler, like this (I
> omitted the config parsing for brevity):
>[..]
> In a sense, that encourages a nice workflow for your intended feature.
> You have to do:
> 
>   git clone -c user.name=... -c user.email=... clone ...
> 
> to set up your ident in the newly-cloned repository, or else clone will
> yell at you. But it's a little unfriendly. If you are just cloning to
> view and not make commits, you don't need your ident set up. And worse,
> if you forget to add your "-c" ident, clone will go through the trouble
> to copy all of the objects, and only then complain about your ident.

I think that forcing to give the configuration in 'git clone' could be problematic for automated tools (e.g. o build) that invoke 'git clone' just for building purposes (i.e. read-only) to a tool-managed directory. And what about sub-modules clones? It would be hard to distinguish manual clones and automatic clones anyway.

Show 20 quoted lines

> So I'd argue that this should only kick in for the strict case. Which
> means the check _has_ to go into fmt_ident, and we have to somehow
> inform fmt_ident of the four cases:
> 
>   1. this ident came from git config
> 
>   2. this ident came from the environment, but from explicit variables
> 
>   3. this ident came from the environment; we guessed but the results
>      look pretty good
> 
>   4. this ident came from the environment; it's probably bogus
> 
> Right now we can identify type 4, with the *_is_bogus flags. We can
> identify 3, because EXPLICITLY_GIVEN flags won't be set. But we can't
> tell the difference between types 1 and 2.
> 
> I suppose there is also a type 0: "this ident was from GIT_COMMITTER_*
> and we did not bother to look up ident at all". But I think we agree
> that strictness and explicitness don't even come into play there.

Looks like an enum type would be better here instead of a set of booleans.

Show 6 quoted lines

> So I think we can make this work by adding another variable to
> communicate the difference between types 1 and 2, and then act on it in
> fmt_ident only when "strict" is set. And I think "user.explicit" is not
> quite the right config name there. "user.guessIdent" is perhaps closer,
> but what we are really saying is "the ident must come from git config".
> So maybe "user.useConfigOnly" or something?

Yes, seems to me that useConfigOnly is better than both so far.

> I also wonder if we could simply expose the 4 levels of above in a
> variable, and default it to type-3. That would let people loosen or
> tighten as they see fit. But it would be a more complicated patch, so if
> nobody really cares about it beyond this use case, it may be overkill.

I get the impression from this and your later E-Mails that there are much more cases to cover when testing this feature (and I would not like to break stuff implementing this, obviously).

The code should be cleaned up anyway. I only delved into that code for the first time two days ago, so it would take me more time to come up with a new one (though reading your overview here of the cases is going to be helpful, thanks).

-- 
Dan Aloni