* [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-01 11:55 ` [PATCH 0/3] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
@ 2018-11-01 11:58 ` Slavica Djukic
2018-11-01 14:53 ` Christian Couder
2018-11-02 4:59 ` [PATCH 2+3/3] stash: tolerate missing user identity Junio C Hamano
2018-11-01 12:00 ` [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function Slavica Djukic
` (2 subsequent siblings)
3 siblings, 2 replies; 41+ messages in thread
From: Slavica Djukic @ 2018-11-01 11:58 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92
Add test to assert that stash fails if user.name and user.email
are not configured.
In the final commit, test will be updated to expect success.
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
t/t3903-stash.sh | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 9e06494ba0..aaff36978e 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1156,4 +1156,19 @@ test_expect_success 'stash -- <subdir> works with binary files' '
test_path_is_file subdir/untracked
'
+test_expect_failure 'stash works when user.name and user.email are not set' '
+ git reset &&
+ >1 &&
+ git add 1 &&
+ test_config user.useconfigonly true &&
+ test_config stash.usebuiltin true &&
+ sane_unset GIT_AUTHOR_NAME &&
+ sane_unset GIT_AUTHOR_EMAIL &&
+ sane_unset GIT_COMMITTER_NAME &&
+ sane_unset GIT_COMMITTER_EMAIL &&
+ test_unconfig user.email &&
+ test_unconfig user.name &&
+ git stash
+'
+
test_done
--
2.19.1.windows.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-01 11:58 ` [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
@ 2018-11-01 14:53 ` Christian Couder
2018-11-02 4:59 ` [PATCH 2+3/3] stash: tolerate missing user identity Junio C Hamano
1 sibling, 0 replies; 41+ messages in thread
From: Christian Couder @ 2018-11-01 14:53 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes Schindelin, git, Slavica
On Thu, Nov 1, 2018 at 2:31 PM Slavica Djukic
<slavicadj.ip2018@gmail.com> wrote:
>
> Add test to assert that stash fails if user.name and user.email
Nit: I am not sure that "assert" is the right word here.
test_expect_failure() is more for documenting an existing bug than for
really asserting a behavior (that users could rely upon). So I would
replace "assert" with "document" or maybe "document the bug".
> are not configured.
> In the final commit, test will be updated to expect success.
Other nit: maybe use "In a later commit" instead of "In the final
commit" as you, or someone else, may add another commit in this patch
series after the current final one.
> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
Thanks!
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 2+3/3] stash: tolerate missing user identity
2018-11-01 11:58 ` [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
2018-11-01 14:53 ` Christian Couder
@ 2018-11-02 4:59 ` Junio C Hamano
1 sibling, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2018-11-02 4:59 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, slawica92, Slavica Djukic
The "git stash" command insists on having a usable user identity to
the same degree as the "git commit-tree" and "git commit" commands
do, because it uses the same codepath that creates commit objects
as these commands.
It is not strictly necesary to do so. Check if we will barf before
creating commit objects and then supply fake identity to please the
machinery that creates commits.
This is not that much of usability improvement, as the users who run
"git stash" would eventually want to record their changes that are
temporarily stored in the stashes in a more permanent history by
committing, and they must do "git config user.{name,email}" at that
point anyway, so arguably this change is only delaying a step that
is necessary to work in the repository.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This time with a proposed commit log message and a flip to the
test; this would be able to replce 2/3 and 3/3 without waiting
for ps/stash-in-c to stabilize and become ready to be based on
further work like this one.
We need to extend the test so that when a reasonable identity is
present, the stashes are created under that identity and not with
the fallback one, which I do not think is tested with the previous
step, so there still is a bit of room to improve [PATCH 1/3]
git-stash.sh | 17 +++++++++++++++++
t/t3903-stash.sh | 2 +-
2 files changed, 18 insertions(+), 1 deletion(-)
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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index a9a573efa0..3dcf2f14d1 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1096,7 +1096,7 @@ test_expect_success 'stash -- <subdir> works with binary files' '
test_path_is_file subdir/untracked
'
-test_expect_failure 'stash works when user.name and user.email are not set' '
+test_expect_success 'stash works when user.name and user.email are not set' '
git reset &&
>1 &&
git add 1 &&
--
2.19.1-801-gd582ea202b
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
2018-11-01 11:55 ` [PATCH 0/3] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
2018-11-01 11:58 ` [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
@ 2018-11-01 12:00 ` Slavica Djukic
2018-11-02 3:01 ` Junio C Hamano
2018-11-01 12:02 ` [PATCH 3/3] [Outreachy] stash: use " Slavica Djukic
2018-11-14 22:12 ` [PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
3 siblings, 1 reply; 41+ messages in thread
From: Slavica Djukic @ 2018-11-01 12:00 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92
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(+)
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)
{
--
2.19.1.windows.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
2018-11-01 12:00 ` [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function Slavica Djukic
@ 2018-11-02 3:01 ` Junio C Hamano
2018-11-02 4:41 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2018-11-02 3:01 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
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.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
2018-11-02 3:01 ` Junio C Hamano
@ 2018-11-02 4:41 ` Junio C Hamano
2018-11-02 5:22 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2018-11-02 4:41 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
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
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
2018-11-02 4:41 ` Junio C Hamano
@ 2018-11-02 5:22 ` Junio C Hamano
0 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2018-11-02 5:22 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
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.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 3/3] [Outreachy] stash: use set_fallback_ident() function
2018-11-01 11:55 ` [PATCH 0/3] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
2018-11-01 11:58 ` [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
2018-11-01 12:00 ` [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function Slavica Djukic
@ 2018-11-01 12:02 ` Slavica Djukic
2018-11-14 22:12 ` [PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
3 siblings, 0 replies; 41+ messages in thread
From: Slavica Djukic @ 2018-11-01 12:02 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92
Call set_fallback_ident() in cmd_stash() and update test
from the first commit to expect success.
Executing stash without user.name and user.email configured
can be useful when bots or similar users use stash, without anyone
specifing valid ident. Use case would be automated testing.
There are also users who find this convinient.
For example, in this thread:
https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#ma4fb50903a54cbcdecd4ef05856bf8094bc3c323
user points out that he would find it useful if stash had --author option.
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
builtin/stash.c | 1 +
t/t3903-stash.sh | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 965e938ebd..add30aae64 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1523,6 +1523,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)
trace_repo_setup(prefix);
setup_work_tree();
+ set_fallback_ident("git stash", "stash@git.commands");
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, options, git_stash_usage,
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index aaff36978e..06a2ffb398 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1156,7 +1156,7 @@ test_expect_success 'stash -- <subdir> works with binary files' '
test_path_is_file subdir/untracked
'
-test_expect_failure 'stash works when user.name and user.email are not set' '
+test_expect_success 'stash works when user.name and user.email are not set' '
git reset &&
>1 &&
git add 1 &&
--
2.19.1.windows.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured
2018-11-01 11:55 ` [PATCH 0/3] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
` (2 preceding siblings ...)
2018-11-01 12:02 ` [PATCH 3/3] [Outreachy] stash: use " Slavica Djukic
@ 2018-11-14 22:12 ` Slavica Djukic
2018-11-14 22:25 ` [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
` (2 more replies)
3 siblings, 3 replies; 41+ messages in thread
From: Slavica Djukic @ 2018-11-14 22:12 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92
Changes since v1:
*extend test to check whether git stash executes under valid ident
(and not under fallback one) when there is such present
*add prepare_fallback_ident() function to git-stash.sh to
provide fallback identity
Slavica Djukic (2):
[Outreachy] t3903-stash: test without configured user.name and
user.email
[Outreachy] stash: tolerate missing user identity
git-stash.sh | 17 +++++++++++++++++
t/t3903-stash.sh | 23 +++++++++++++++++++++++
2 files changed, 40 insertions(+)
--
2.19.1.1052.gd166e6afe
^ permalink raw reply [flat|nested] 41+ messages in thread* [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-14 22:12 ` [PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
@ 2018-11-14 22:25 ` Slavica Djukic
2018-11-15 12:37 ` Johannes Schindelin
2018-11-16 5:55 ` Junio C Hamano
2018-11-14 22:28 ` [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity Slavica Djukic
2018-11-18 13:29 ` [PATCH 0/1 v3] make stash work if user.name and user.email are not configured Slavica Djukic
2 siblings, 2 replies; 41+ messages in thread
From: Slavica Djukic @ 2018-11-14 22:25 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92
Add test to document that stash fails if user.name and user.email
are not configured.
In the later commit, test will be updated to expect success.
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
t/t3903-stash.sh | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index cd216655b..bab8bec67 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1096,4 +1096,27 @@ test_expect_success 'stash -- <subdir> works with binary files' '
test_path_is_file subdir/untracked
'
+test_expect_failure 'stash works when user.name and user.email are not set' '
+ git reset &&
+ git var GIT_COMMITTER_IDENT >expected &&
+ >1 &&
+ git add 1 &&
+ git stash &&
+ git var GIT_COMMITTER_IDENT >actual &&
+ test_cmp expected actual &&
+ >2 &&
+ git add 2 &&
+ test_config user.useconfigonly true &&
+ test_config stash.usebuiltin true &&
+ (
+ sane_unset GIT_AUTHOR_NAME &&
+ sane_unset GIT_AUTHOR_EMAIL &&
+ sane_unset GIT_COMMITTER_NAME &&
+ sane_unset GIT_COMMITTER_EMAIL &&
+ test_unconfig user.email &&
+ test_unconfig user.name &&
+ git stash
+ )
+'
+
test_done
--
2.19.1.1052.gd166e6afe
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-14 22:25 ` [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
@ 2018-11-15 12:37 ` Johannes Schindelin
2018-11-16 5:55 ` Junio C Hamano
1 sibling, 0 replies; 41+ messages in thread
From: Johannes Schindelin @ 2018-11-15 12:37 UTC (permalink / raw)
To: Slavica Djukic; +Cc: git, slawica92
Hi Slavica,
this looks very good to me. Just one grammar thing:
On Wed, 14 Nov 2018, Slavica Djukic wrote:
> Add test to document that stash fails if user.name and user.email
> are not configured.
> In the later commit, test will be updated to expect success.
In a later commit [...]
Otherwise, I would be totally fine with this version being merged.
Ciao,
Johannes
>
> Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
> ---
> t/t3903-stash.sh | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index cd216655b..bab8bec67 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1096,4 +1096,27 @@ test_expect_success 'stash -- <subdir> works with binary files' '
> test_path_is_file subdir/untracked
> '
>
> +test_expect_failure 'stash works when user.name and user.email are not set' '
> + git reset &&
> + git var GIT_COMMITTER_IDENT >expected &&
> + >1 &&
> + git add 1 &&
> + git stash &&
> + git var GIT_COMMITTER_IDENT >actual &&
> + test_cmp expected actual &&
> + >2 &&
> + git add 2 &&
> + test_config user.useconfigonly true &&
> + test_config stash.usebuiltin true &&
> + (
> + sane_unset GIT_AUTHOR_NAME &&
> + sane_unset GIT_AUTHOR_EMAIL &&
> + sane_unset GIT_COMMITTER_NAME &&
> + sane_unset GIT_COMMITTER_EMAIL &&
> + test_unconfig user.email &&
> + test_unconfig user.name &&
> + git stash
> + )
> +'
> +
> test_done
> --
> 2.19.1.1052.gd166e6afe
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-14 22:25 ` [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
2018-11-15 12:37 ` Johannes Schindelin
@ 2018-11-16 5:55 ` Junio C Hamano
2018-11-16 6:26 ` Junio C Hamano
` (2 more replies)
1 sibling, 3 replies; 41+ messages in thread
From: Junio C Hamano @ 2018-11-16 5:55 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
Slavica Djukic <slavicadj.ip2018@gmail.com> writes:
> +test_expect_failure 'stash works when user.name and user.email are not set' '
> + git reset &&
> + git var GIT_COMMITTER_IDENT >expected &&
All the other existing test pieces in this file calls the expected
result "expect"; is there a reason why this patch needs to be
different (e.g. 'expect' file left by the earlier step needs to be
kept unmodified for later use, or something like that)? If not,
please avoid making a difference in irrelevant details, as that
would waste time of readers by forcing them to guess if there is
such a reason that readers cannot immediately see.
Anyway, we grab the committer ident we use by default during the
test with this command. OK.
> + >1 &&
> + git add 1 &&
> + git stash &&
And we make sure we can create stash.
> + git var GIT_COMMITTER_IDENT >actual &&
> + test_cmp expected actual &&
I am not sure what you are testing with this step. There is nothing
that changed environment variables or configuration since we ran
"git var" above. Why does this test suspect that somebody in the
future may break the expectation that after running 'git add' and/or
'git stash', our committer identity may have been changed, and how
would such a breakage happen?
> + >2 &&
> + git add 2 &&
> + test_config user.useconfigonly true &&
> + test_config stash.usebuiltin true &&
Now we start using use-config-only, so unsetting environment
variables will cause trouble when Git insists on having an
explicitly configured identities. Makes sense.
> + (
> + sane_unset GIT_AUTHOR_NAME &&
> + sane_unset GIT_AUTHOR_EMAIL &&
> + sane_unset GIT_COMMITTER_NAME &&
> + sane_unset GIT_COMMITTER_EMAIL &&
> + test_unconfig user.email &&
> + test_unconfig user.name &&
And then we try the same test, but without environment or config.
Since we are unsetting the environment, in order to be nice for
future test writers, we do this in a subshell, so that we do not
have to restore the original values of environment variables.
Don't we need to be nice the same way for configuration variables,
though? We _know_ that nobody sets user.{email,name} config up to
this point in the test sequence, so that is why we do not do a "save
before test and then restore to the original" dance on them. Even
though we are relying on the fact that these two variables are left
unset in the configuration file, we unconfig them here anyway, and I
do think it is a good idea for documentation purposes (i.e. we are
not documenting what we assume the config before running this test
would be; we are documenting what state we want these two variables
are in when running this "git stash"---that is, they are both unset).
So these later part of this test piece makes sense. I still do not
know what you wanted to check in the earlier part of the test,
though.
> + git stash
> + )
> +'
> +
> test_done
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-16 5:55 ` Junio C Hamano
@ 2018-11-16 6:26 ` Junio C Hamano
2018-11-16 6:32 ` Junio C Hamano
2018-11-16 8:28 ` Slavica Djukic
2 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2018-11-16 6:26 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
Junio C Hamano <gitster@pobox.com> writes:
> Now we start using use-config-only, so unsetting environment
> variables will cause trouble when Git insists on having an
> explicitly configured identities. Makes sense.
>
>> + (
>> + sane_unset GIT_AUTHOR_NAME &&
>> + sane_unset GIT_AUTHOR_EMAIL &&
>> + sane_unset GIT_COMMITTER_NAME &&
>> + sane_unset GIT_COMMITTER_EMAIL &&
>> + test_unconfig user.email &&
>> + test_unconfig user.name &&
>
> And then we try the same test, but without environment or config.
I suspect that it makes sense to replace the "git stash" we see
below with something like this:
test_must_fail git commit -m should fail &&
echo "git stash <git@stash>" >expect &&
echo >2 &&
git stash &&
git show -s --format="%(authorname) <%(authoremail)>" refs/stash >actual &&
test_cmp expect actual
That is
- we make sure "commit" would not go through, to make sure our
preparation to unset environment variables was sufficient;
- we make sure "stash" does succeed (which is the primary thing you
are interested in);
- we make sure the resulting "stash" is not created under our
default identity but under our fallback one.
>> + git stash
>> + )
>> +'
>> +
>> test_done
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-16 5:55 ` Junio C Hamano
2018-11-16 6:26 ` Junio C Hamano
@ 2018-11-16 6:32 ` Junio C Hamano
2018-11-16 8:28 ` Slavica Djukic
2 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2018-11-16 6:32 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
Junio C Hamano <gitster@pobox.com> writes:
> Slavica Djukic <slavicadj.ip2018@gmail.com> writes:
>
>> +test_expect_failure 'stash works when user.name and user.email are not set' '
>> + git reset &&
>> + git var GIT_COMMITTER_IDENT >expected &&
> ...
> Anyway, we grab the committer ident we use by default during the
> test with this command. OK.
>
>> + >1 &&
>> + git add 1 &&
>> + git stash &&
>
> And we make sure we can create stash.
>
>> + git var GIT_COMMITTER_IDENT >actual &&
>> + test_cmp expected actual &&
>
> I am not sure what you are testing with this step. There is nothing
> that changed environment variables or configuration since we ran
> "git var" above. Why does this test suspect that somebody in the
> future may break the expectation that after running 'git add' and/or
> 'git stash', our committer identity may have been changed, and how
> would such a breakage happen?
Just a note.
"git var GIT_COMMITTER_IDENT" has timestamp in it, so a naïve reader
might wonder what would happen if "git add 1" and "git stash" took
more than one second. But it won't be a problem in this case as the
committer date comes from the environment GIT_COMMITTER_DATE, which
is set to a fixed known value and is incremented only by calling
test_commit helper function, which does not happen between the two
"git var" calls.
In any case, I am not sure I understand the point of comparing two
output from "git var" invocations we see ablve in this test.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-16 5:55 ` Junio C Hamano
2018-11-16 6:26 ` Junio C Hamano
2018-11-16 6:32 ` Junio C Hamano
@ 2018-11-16 8:28 ` Slavica Djukic
2018-11-16 10:12 ` Junio C Hamano
2 siblings, 1 reply; 41+ messages in thread
From: Slavica Djukic @ 2018-11-16 8:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes.Schindelin, git, slawica92
Hi Junio,
On 16-Nov-18 6:55 AM, Junio C Hamano wrote:
> Slavica Djukic <slavicadj.ip2018@gmail.com> writes:
>
>> +test_expect_failure 'stash works when user.name and user.email are not set' '
>> + git reset &&
>> + git var GIT_COMMITTER_IDENT >expected &&
> All the other existing test pieces in this file calls the expected
> result "expect"; is there a reason why this patch needs to be
> different (e.g. 'expect' file left by the earlier step needs to be
> kept unmodified for later use, or something like that)? If not,
> please avoid making a difference in irrelevant details, as that
> would waste time of readers by forcing them to guess if there is
> such a reason that readers cannot immediately see.
There is no specific reason for file to be "expected", I'll update that.
>
> Anyway, we grab the committer ident we use by default during the
> test with this command. OK.
>
>> + >1 &&
>> + git add 1 &&
>> + git stash &&
> And we make sure we can create stash.
>
>> + git var GIT_COMMITTER_IDENT >actual &&
>> + test_cmp expected actual &&
> I am not sure what you are testing with this step. There is nothing
> that changed environment variables or configuration since we ran
> "git var" above. Why does this test suspect that somebody in the
> future may break the expectation that after running 'git add' and/or
> 'git stash', our committer identity may have been changed, and how
> would such a breakage happen?
In previous re-roll, you suggested that test should be improved so that
when
reasonable identity is present, git stash executes under that identity,
and not
under the fallback one. Here I'm just making sure that after calling git
stash,
we still have same reasonable identity present.
>
>> + >2 &&
>> + git add 2 &&
>> + test_config user.useconfigonly true &&
>> + test_config stash.usebuiltin true &&
> Now we start using use-config-only, so unsetting environment
> variables will cause trouble when Git insists on having an
> explicitly configured identities. Makes sense.
>
>> + (
>> + sane_unset GIT_AUTHOR_NAME &&
>> + sane_unset GIT_AUTHOR_EMAIL &&
>> + sane_unset GIT_COMMITTER_NAME &&
>> + sane_unset GIT_COMMITTER_EMAIL &&
>> + test_unconfig user.email &&
>> + test_unconfig user.name &&
> And then we try the same test, but without environment or config.
> Since we are unsetting the environment, in order to be nice for
> future test writers, we do this in a subshell, so that we do not
> have to restore the original values of environment variables.
>
> Don't we need to be nice the same way for configuration variables,
> though? We _know_ that nobody sets user.{email,name} config up to
> this point in the test sequence, so that is why we do not do a "save
> before test and then restore to the original" dance on them. Even
> though we are relying on the fact that these two variables are left
> unset in the configuration file, we unconfig them here anyway, and I
> do think it is a good idea for documentation purposes (i.e. we are
> not documenting what we assume the config before running this test
> would be; we are documenting what state we want these two variables
> are in when running this "git stash"---that is, they are both unset).
>
> So these later part of this test piece makes sense. I still do not
> know what you wanted to check in the earlier part of the test,
> though.
>
>> + git stash
>> + )
>> +'
>> +
>> test_done
>
Thank you,
Slavica
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-16 8:28 ` Slavica Djukic
@ 2018-11-16 10:12 ` Junio C Hamano
2018-11-17 18:47 ` Slavica Djukic
0 siblings, 1 reply; 41+ messages in thread
From: Junio C Hamano @ 2018-11-16 10:12 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
Slavica Djukic <slavicadj.ip2018@gmail.com> writes:
>>> + git var GIT_COMMITTER_IDENT >actual &&
>>> + test_cmp expected actual &&
>> I am not sure what you are testing with this step. There is nothing
>> that changed environment variables or configuration since we ran
>> "git var" above. Why does this test suspect that somebody in the
>> future may break the expectation that after running 'git add' and/or
>> 'git stash', our committer identity may have been changed, and how
>> would such a breakage happen?
> In previous re-roll, you suggested that test should be improved so
> that when
> reasonable identity is present, git stash executes under that
> identity, and not
> under the fallback one.
Yes, but for that you'd need to be checking the resulting commit
object that represents the stash entry. It should be created under
the substitute identity.
> Here I'm just making sure that after calling
> git stash,
> we still have same reasonable identity present.
I do not think such a test would detect it, even when "git stash"
incorrectly used the fallback identity to create the stash entry.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-16 10:12 ` Junio C Hamano
@ 2018-11-17 18:47 ` Slavica Djukic
2018-11-18 6:28 ` Junio C Hamano
0 siblings, 1 reply; 41+ messages in thread
From: Slavica Djukic @ 2018-11-17 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes.Schindelin, git, slawica92
Hi Junio,
On 16-Nov-18 11:12 AM, Junio C Hamano wrote:
> Slavica Djukic <slavicadj.ip2018@gmail.com> writes:
>
>>>> + git var GIT_COMMITTER_IDENT >actual &&
>>>> + test_cmp expected actual &&
>>> I am not sure what you are testing with this step. There is nothing
>>> that changed environment variables or configuration since we ran
>>> "git var" above. Why does this test suspect that somebody in the
>>> future may break the expectation that after running 'git add' and/or
>>> 'git stash', our committer identity may have been changed, and how
>>> would such a breakage happen?
>> In previous re-roll, you suggested that test should be improved so
>> that when
>> reasonable identity is present, git stash executes under that
>> identity, and not
>> under the fallback one.
> Yes, but for that you'd need to be checking the resulting commit
> object that represents the stash entry. It should be created under
> the substitute identity.
Would it be correct to check it like this:
git reset &&
>1 &&
git add 1 &&
echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" >expect &&
git stash &&
git show -s --format="%an <%ae>" refs/stash >actual &&
test_cmp expect actual
It is similar to your suggestion when there is no
ident present.
>> Here I'm just making sure that after calling
>> git stash,
>> we still have same reasonable identity present.
> I do not think such a test would detect it, even when "git stash"
> incorrectly used the fallback identity to create the stash entry.
>
>
Thank you,
Slavica
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
2018-11-17 18:47 ` Slavica Djukic
@ 2018-11-18 6:28 ` Junio C Hamano
0 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2018-11-18 6:28 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
Slavica Djukic <slavicadj.ip2018@gmail.com> writes:
>> Yes, but for that you'd need to be checking the resulting commit
>> object that represents the stash entry. It should be created under
>> the substitute identity.
> Would it be correct to check it like this:
>
> git reset &&
> >1 &&
> git add 1 &&
> echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" >expect &&
> git stash &&
> git show -s --format="%an <%ae>" refs/stash >actual &&
> test_cmp expect actual
So, you create a stash, and grab %an and %ae out of the resulting
commit object and store them in actual, and then compare. Makes
sense.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity
2018-11-14 22:12 ` [PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
2018-11-14 22:25 ` [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
@ 2018-11-14 22:28 ` Slavica Djukic
2018-11-16 5:35 ` Junio C Hamano
2018-11-18 13:29 ` [PATCH 0/1 v3] make stash work if user.name and user.email are not configured Slavica Djukic
2 siblings, 1 reply; 41+ messages in thread
From: Slavica Djukic @ 2018-11-14 22:28 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92, Junio C Hamano
The "git stash" command insists on having a usable user identity to
the same degree as the "git commit-tree" and "git commit" commands
do, because it uses the same codepath that creates commit objects
as these commands.
It is not strictly necesary to do so. Check if we will barf before
creating commit objects and then supply fake identity to please the
machinery that creates commits.
This is not that much of usability improvement, as the users who run
"git stash" would eventually want to record their changes that are
temporarily stored in the stashes in a more permanent history by
committing, and they must do "git config user.{name,email}" at that
point anyway, so arguably this change is only delaying a step that
is necessary to work in the repository.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
git-stash.sh | 17 +++++++++++++++++
t/t3903-stash.sh | 2 +-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 94793c1a9..789ce2f41 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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index bab8bec67..0b0814421 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1096,7 +1096,7 @@ test_expect_success 'stash -- <subdir> works with binary files' '
test_path_is_file subdir/untracked
'
-test_expect_failure 'stash works when user.name and user.email are not set' '
+test_expect_success 'stash works when user.name and user.email are not set' '
git reset &&
git var GIT_COMMITTER_IDENT >expected &&
>1 &&
--
2.19.1.1052.gd166e6afe
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity
2018-11-14 22:28 ` [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity Slavica Djukic
@ 2018-11-16 5:35 ` Junio C Hamano
0 siblings, 0 replies; 41+ messages in thread
From: Junio C Hamano @ 2018-11-16 5:35 UTC (permalink / raw)
To: Slavica Djukic; +Cc: Johannes.Schindelin, git, slawica92
Slavica Djukic <slavicadj.ip2018@gmail.com> writes:
> git-stash.sh | 17 +++++++++++++++++
> t/t3903-stash.sh | 2 +-
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 94793c1a9..789ce2f41 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
That looks like a sensible implementation to me.
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index bab8bec67..0b0814421 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1096,7 +1096,7 @@ test_expect_success 'stash -- <subdir> works with binary files' '
> test_path_is_file subdir/untracked
> '
>
> -test_expect_failure 'stash works when user.name and user.email are not set' '
> +test_expect_success 'stash works when user.name and user.email are not set' '
This line claims to the readers of patch that the known breakage
this known test piece demonstrated has been corrected, but they need
to refresh their memory by going back to the previous patch to see
if this "failure-to-success" flipping is done to the right test
piece, and what exactly the test piece tested to see the existing
breakage, because all the interesting part of the test are chomped
outside the post-context of this hunk.
Unless the fix is fairly complex, adding ought-to-succeed tests that
expect success that break when the code change gets omitted from the
patch in the same patch as the fix itself (i.e. squash patch 1/2 and
patch 2/2 into a single patch) would be more helpful for the readers
(it also helps cherry-picking the fix later to earlier maintenance
tracks if it becomes necessary).
> git reset &&
> git var GIT_COMMITTER_IDENT >expected &&
> >1 &&
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 0/1 v3] make stash work if user.name and user.email are not configured
2018-11-14 22:12 ` [PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured Slavica Djukic
2018-11-14 22:25 ` [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email Slavica Djukic
2018-11-14 22:28 ` [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity Slavica Djukic
@ 2018-11-18 13:29 ` Slavica Djukic
2018-11-18 13:44 ` [PATCH 1/1 v3] stash: tolerate missing user identity Slavica Djukic
2 siblings, 1 reply; 41+ messages in thread
From: Slavica Djukic @ 2018-11-18 13:29 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92
Changes since v2:
* squash patch 1/2 and patch 2/2 into a single patch
* modify first part of test when there is valid ident
present: create a stash, grab %an and %ae out of the
resulting commit object and compare to original ident
Slavica Djukic (1):
stash: tolerate missing user identity
git-stash.sh | 17 +++++++++++++++++
t/t3903-stash.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
--
2.19.1.1052.gd166e6afe
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/1 v3] stash: tolerate missing user identity
2018-11-18 13:29 ` [PATCH 0/1 v3] make stash work if user.name and user.email are not configured Slavica Djukic
@ 2018-11-18 13:44 ` Slavica Djukic
0 siblings, 0 replies; 41+ messages in thread
From: Slavica Djukic @ 2018-11-18 13:44 UTC (permalink / raw)
To: slavicadj.ip2018; +Cc: Johannes.Schindelin, git, slawica92, Junio C Hamano
The "git stash" command insists on having a usable user identity to
the same degree as the "git commit-tree" and "git commit" commands
do, because it uses the same codepath that creates commit objects
as these commands.
It is not strictly necesary to do so. Check if we will barf before
creating commit objects and then supply fake identity to please the
machinery that creates commits.
Add test to document that stash executes correctly both with and
without valid ident.
This is not that much of usability improvement, as the users who run
"git stash" would eventually want to record their changes that are
temporarily stored in the stashes in a more permanent history by
committing, and they must do "git config user.{name,email}" at that
point anyway, so arguably this change is only delaying a step that
is necessary to work in the repository.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Slavica Djukic <slawica92@hotmail.com>
---
git-stash.sh | 17 +++++++++++++++++
t/t3903-stash.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/git-stash.sh b/git-stash.sh
index 94793c1a9..789ce2f41 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
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index cd216655b..5f8272b6f 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1096,4 +1096,32 @@ test_expect_success 'stash -- <subdir> works with binary files' '
test_path_is_file subdir/untracked
'
+test_expect_success 'stash works when user.name and user.email are not set' '
+ git reset &&
+ >1 &&
+ git add 1 &&
+ echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>" >expect &&
+ git stash &&
+ git show -s --format="%an <%ae>" refs/stash >actual &&
+ test_cmp expect actual &&
+ >2 &&
+ git add 2 &&
+ test_config user.useconfigonly true &&
+ test_config stash.usebuiltin true &&
+ (
+ sane_unset GIT_AUTHOR_NAME &&
+ sane_unset GIT_AUTHOR_EMAIL &&
+ sane_unset GIT_COMMITTER_NAME &&
+ sane_unset GIT_COMMITTER_EMAIL &&
+ test_unconfig user.email &&
+ test_unconfig user.name &&
+ test_must_fail git commit -m "should fail" &&
+ echo "git stash <git@stash>" >expect &&
+ >2 &&
+ git stash &&
+ git show -s --format="%an <%ae>" refs/stash >actual &&
+ test_cmp expect actual
+ )
+'
+
test_done
--
2.19.1.1052.gd166e6afe
^ permalink raw reply related [flat|nested] 41+ messages in thread