[2/3,Outreachy] ident: introduce set_fallback_ident() function
diff mbox series

Message ID 20181101120029.13992-1-slawica92@hotmail.com
State New
Headers show
Series
  • make stash work if user.name and user.email are not configured
Related show

Commit Message

Slavica Djukic Nov. 1, 2018, noon UTC
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>
---
 cache.h |  1 +
 ident.c | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

Comments

Junio C Hamano Nov. 2, 2018, 3:01 a.m. UTC | #1
Slavica Djukic <slavicadj.ip2018@gmail.com> 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@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.

> +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.
Junio C Hamano Nov. 2, 2018, 4:41 a.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

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

As we currently have no idea when builtin/stash.c becomes ready for
'next', how about doing something like this instead, in order to
help end-users without waiting in the meantime?  The fix can be
picked up and ported when the C rewrite is updated, of course.

 git-stash.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/git-stash.sh b/git-stash.sh
index 94793c1a91..789ce2f41d 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -55,6 +55,20 @@ untracked_files () {
 	git ls-files -o $z $excl_opt -- "$@"
 }
 
+prepare_fallback_ident () {
+	if ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null 2>&1
+	then
+		GIT_AUTHOR_NAME="git stash"
+		GIT_AUTHOR_EMAIL=git@stash
+		GIT_COMMITTER_NAME="git stash"
+		GIT_COMMITTER_EMAIL=git@stash
+		export GIT_AUTHOR_NAME
+		export GIT_AUTHOR_EMAIL
+		export GIT_COMMITTER_NAME
+		export GIT_COMMITTER_EMAIL
+	fi
+}
+
 clear_stash () {
 	if test $# != 0
 	then
@@ -67,6 +81,9 @@ clear_stash () {
 }
 
 create_stash () {
+
+	prepare_fallback_ident
+
 	stash_msg=
 	untracked=
 	while test $# != 0
Junio C Hamano Nov. 2, 2018, 5:22 a.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

> As we currently have no idea when builtin/stash.c becomes ready for
> 'next', how about doing something like this instead, in order to
> help end-users without waiting in the meantime?  The fix can be
> picked up and ported when the C rewrite is updated, of course.

I think a better approach to fix the current C version, assuming
that a reroll won't change the structure of the code too much and
keeps using commit_tree() to synthesize the stash entries, would be
to teach commit_tree() -> commit_tree_extended() codepath to take
commiter identity just like it takes author identity.  Then inside
builtin/stash.c, we can choose what committer and author identity to
pass without having commit_tree_extended() ask for identity with the
STRICT option.  Right now, commit_tree() does not have a good way,
other than somehow lying to git_author/committer_info(), to record a
commit created under an arbitrary identity, which would be fixed
with such an approach and will help callers with similar needs in
the future.

Patch
diff mbox series

diff --git a/cache.h b/cache.h
index 681307f716..6b5b559a05 100644
--- a/cache.h
+++ b/cache.h
@@ -1470,6 +1470,7 @@  extern const char *git_sequence_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
+void set_fallback_ident(const char *name, const char *email);
 extern void reset_ident_date(void);
 
 struct ident_split {
diff --git a/ident.c b/ident.c
index 33bcf40644..410bd495e9 100644
--- a/ident.c
+++ b/ident.c
@@ -505,6 +505,23 @@  int git_ident_config(const char *var, const char *value, void *data)
 	return 0;
 }
 
+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;
+	}
+}
+
 static int buf_cmp(const char *a_begin, const char *a_end,
 		   const char *b_begin, const char *b_end)
 {