Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 2, 2018, 03:01 UTC
Message-ID: <xmqqwopwqj2g.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181101120029.13992-1-slawica92@hotmail.com>

Slavica Djukic <slavicadj.ip2018@gmail.com> writes:

Show 11 quoted lines

> 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@hotmail.com>
> ---

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.

Show 16 quoted lines

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