| Message ID | 20181101120029.13992-1-slawica92@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | make stash work if user.name and user.email are not configured | expand |
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 <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 <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.
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) {
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(+)