Slavica Djukic <slavicadj.ip2018@xxxxxxxxx> writes:
> Usually, when creating a commit, ident is needed to record the author
> and commiter.
> But, when there is commit not intended to published, e.g. when stashing
> changes, valid ident is not necessary.
> To allow creating commits in such scenario, let's introduce helper
> function "set_fallback_ident(), which will pre-load the ident.
>
> In following commit, set_fallback_ident() function will be called in stash.
>
> Signed-off-by: Slavica Djukic <slawica92@xxxxxxxxxxx>
> ---
This is quite the other way around from what I expected to see.
I would have expected that a patch would introduce a new flag to
tell git_author/committer_info() functions that it is OK not to have
name or email given, and pass that flag down the callchain.
Anybody who knows that git_author/committer_info() will eventually
get called can instead tell these helpers not to fail (and yield a
substitute non-answer) beforehand with this function, instead of
passing a flag down to affect _only_ one callflow without affecting
others, using this new function.
I am not yet saying that being opposite from my intuition is
necessarily wrong, but the approach is like setting a global
variable that affects everybody and it will probably make it
harder to later libify the functions involved. It certainly
makes this patch (and the next step) much simpler than passing
a flag IDENT_NO_NAME_OK|IDENT_NO_MAIL_OK thru the codepath.
> +void set_fallback_ident(const char *name, const char *email)
> +{
> + if (!git_default_name.len) {
> + strbuf_addstr(&git_default_name, name);
> + committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
> + author_ident_explicitly_given |= IDENT_NAME_GIVEN;
> + ident_config_given |= IDENT_NAME_GIVEN;
> + }
> +
> + if (!git_default_email.len) {
> + strbuf_addstr(&git_default_email, email);
> + committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
> + author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
> + ident_config_given |= IDENT_MAIL_GIVEN;
> + }
> +}
One terrible thing about this approach is that the caller cannot
tell if it used fallback name or a real name, because it lies in
these "explicitly_given" fields. The immediate caller (i.e. the one
that creates commit objects used to represent a stash entry) may not
care, but a helper function pretending to be reusable incredient to
solve a more general issue, this is far less than ideal.
So in short, I do agree that the series tackles an issue worth
addressing, but I am not impressed with the approach at all.
Rather than adding this fallback trap, can't we do it more like
this?
- At the beginning of "git stash", after parsing the command
line, we know what subcommand of "git stash" we are going to
run.
- If it is a subcommand that could need the ident (i.e. the ones
that create a stash entry), we check the ident (e.g. make a
call to git_author/committer_info() ourselves) but without
STRICT bit, so that we can probe without dying if we need to
supply a fallback identy.
- And if we do need it, then setenv() the necessary
environment variables and arrange the next call by anybody
to git_author/committer_info() will get the fallback values
from there.
