On Wed, Feb 03, 2016 at 11:01:11PM -0500, Jeff King wrote:
>[..]
> > > * 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.
> 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.
> 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.
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html