t3903-stash: test without configured user name
PATCH 1/3 t3903-stash: test without configured user name
This is part of enhancement request that ask for git stash to work even if user.name is not configured. The issue is discussed here: https://public-inbox.org/git/[email protected]/T/#u.
--- t/t3903-stash.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
t/t3903-stash.sh +17 -0
@@ -1156,4 +1156,21 @@ test_expect_success 'stash -- <subdir> works with binary files' ' test_path_is_file subdir/untracked ' +test_expect_failure 'stash with HOME as non-existing directory' ' + test_commit 1 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && + ( + HOME=$(pwd)/none && + export HOME && + unset GIT_AUTHOR_NAME && + unset GIT_AUTHOR_EMAIL && + unset GIT_COMMITTER_NAME && + unset GIT_COMMITTER_EMAIL && + test_must_fail git config user.email && + echo changed >1.t && + git stash + ) +' + test_done
Signature · 1 line
2.19.1.windows.1
Quoted 3 lines · Slavica
This is part of enhancement request that ask for
git stashto work even ifuser.nameis not configured. The issue is discussed here: https://public-inbox.org/git/[email protected]/T/#u.
We prefer commit messages that contain as much as possible all the information necessary to understand the patch without links to other places.
It seems that only this email from you reached me. Did you send other emails for patches 2/3 and 3/3?
[...]
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1172
Quoted 10 lines
t/t3903-stash.sh +10
-0+ ( + HOME=$(pwd)/none && + export HOME && + unset GIT_AUTHOR_NAME && + unset GIT_AUTHOR_EMAIL && + unset GIT_COMMITTER_NAME && + unset GIT_COMMITTER_EMAIL && + test_must_fail git config user.email && + echo changed >1.t && + git stash
It seems that the above line is not indented like the previous ones.
Quoted 2 lines
+ )
+'
Thanks for contributing,
Christian.
Quoted 11 lines · Christian Couder
Quoted 2 lines · Slavica
This is part of enhancement request that ask for
git stashto work even ifuser.nameis not configured. The issue is discussed here: https://public-inbox.org/git/[email protected]/T/#u.We prefer commit messages that contain as much as possible all the information necessary to understand the patch without links to other places.
It seems that only this email from you reached me. Did you send other emails for patches 2/3 and 3/3?
[...]
Okay, I will change that. This is my first patch and I am still adapting.
Emails for patches 2/3 and 3/3 because aren't there because I am still preparing them.
(I didn't know if I had 3 patches in plan that they should be sent at almost the same time.)
Quoted 11 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1172
Quoted 11 lines
t/t3903-stash.sh +10
-0+ ( + HOME=$(pwd)/none && + export HOME && + unset GIT_AUTHOR_NAME && + unset GIT_AUTHOR_EMAIL && + unset GIT_COMMITTER_NAME && + unset GIT_COMMITTER_EMAIL && + test_must_fail git config user.email && + echo changed >1.t && + git stash
Quoted 1 line
It seems that the above line is not indented like the previous ones.
I don't know what is the reason, in my IDE everything seems fine, but I'll fix it.
Quoted 5 lines
Quoted 3 lines
+ )
+'Thanks for contributing,
Christian.
You are welcome,
Slavica
Quoted 2 lines
Quoted 13 lines · Slavica
Quoted 11 lines · Christian Couder
Quoted 2 lines · Slavica
This is part of enhancement request that ask for
git stashto work even ifuser.nameis not configured. The issue is discussed here: https://public-inbox.org/git/[email protected]/T/#u.We prefer commit messages that contain as much as possible all the information necessary to understand the patch without links to other places.
It seems that only this email from you reached me. Did you send other emails for patches 2/3 and 3/3?
[...]
Quoted 7 lines
Okay, I will change that. This is my first patch and I am still adapting.
Emails for patches 2/3 and 3/3 because aren't there because I am still preparing them.
(I didn't know if I had 3 patches in plan that they should be sent at almost the same time.)
It is more efficient for everybody involved.
- You may discover that 1/3 you just (thought) finished was not sufficient while working on 2/3 and 3/3, and by the time you are pretty close to finishing 2/3 and 3/3, you may want to update 1/3 in a big way. Sending a premature version and having others to review is wasting everbody's time.
- Your 1/3 might become perfect alone with help from others' reviews and your updates, but after that everybody may forget about it when you are ready to send out 2/3 and 3/3; if these three are truly related patches in a single topic, you would want to have what 1/3 did fresh in your reviewers' minds. You'd have to find the old message of 1/3 and make 2/3 and 3/3 responses to it to keep them properly threaded (which may take your time), and reviewers need to refresh their memory by going back to 1/3 before reviewing 2/3 and 3/3
One thing I learned twice while working in this project is that open source development is not a race to produce and show your product as quickly as possible.
When I was an individual contributor, the project was young and there were many people with good and competing ideas working to achieve more-or-less the same goal. It felt like a competition to get *MY* version of the vision, design and implementation over others' adopted and one way to stay in the competition was to send things as quickly as possible. I didn't know better, and I think I ended up wasting many people's time that way.
That changed when I became the maintainer, as (1) I no longer had to race with anybody ;-), and (2) I introduced the 'pu' (proposed update) system so that anything that was queued early can be discarded and replaced when a better thing come within a reasonable timeframe.
And then I re-learned the same "this is not a race" lesson a couple of years ago, when I started working in a timezone several hours away from the most active participants for a few months at a time. I do not have to respond to a message I see on the list immediately, as it is too late to catch the sender who is already in bed ;-)
So take your time and make sure what you are sending out can be reviewed the most efficiently. Completing 2/3 and 3/3 before sending 1/3 out to avoid having to redo 1/3 and avoid having reviewers to spend their time piecemeal is one thing. Making sure that the patch does not have style issues that distract reviewers' attention is another.
Sitting on what you think you have completed for a few days allows you to review your product with fresh eyes before sending them out, which is another benefit of trying not to rush.
Quoted 2 lines · Slavica
This is part of enhancement request that ask for
git stashto work even ifuser.nameis not configured. The issue is discussed here: https://public-inbox.org/git/[email protected]/T/#u.
As Christian mentioned already, it's best to try to describe the issue succinctly in the commit message so readers can understand it without chasing a link. For this simple case, it should be sufficient to explain that, due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1156
Quoted 5 lines
Signed-off-by Slavica <[email protected]>---
t/t3903-stash.sh +1
-0@@ -1156,4 +1156,21 @@ test_expect_success 'stash -- <subdir> works with binary files' ' +test_expect_failure 'stash with HOME as non-existing directory' '
The purpose of this test is to demonstrate that git-stash has an undesirable requirement that 'user.name' and 'user.email' be set. The test title should reflect that. So, instead of talking about non-existent HOME (which is just an implementation detail of the test), a better test title would be something like "stash works when user.name and user.email are not set".
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1166
Quoted 7 lines
t/t3903-stash.sh +7
-0+ test_commit 1 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && + ( + HOME=$(pwd)/none && + export HOME && + unset GIT_AUTHOR_NAME &&
Use sane_unset() for all of these rather than bare 'unset'.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1172
Quoted 6 lines
t/t3903-stash.sh +6
-0+ unset GIT_AUTHOR_EMAIL && + unset GIT_COMMITTER_NAME && + unset GIT_COMMITTER_EMAIL && + test_must_fail git config user.email && + echo changed >1.t && + git stash
Christian already mentioned the odd indentation.
Quoted 2 lines
t/t3903-stash.sh +2
-0+ ) +'
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1165
Quoted 7 lines · Slavica
t/t3903-stash.sh +7
-0+test_expect_failure 'stash with HOME as non-existing directory' ' + test_commit 1 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && + ( + HOME=$(pwd)/none && + export HOME &&
What is the reason why this test needs to move HOME away from TRASH_DIRECTORY (set in t/test-lib.sh)?
Quoted 11 lines
t/t3903-stash.sh +10
-0+ unset GIT_AUTHOR_NAME && + unset GIT_AUTHOR_EMAIL && + unset GIT_COMMITTER_NAME && + unset GIT_COMMITTER_EMAIL && + test_must_fail git config user.email && + echo changed >1.t && + git stash + ) +' + test_done
Hi Junio,
Quoted 10 lines · Junio C Hamano
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1165
Quoted 7 lines · Slavica
t/t3903-stash.sh +7
-0+test_expect_failure 'stash with HOME as non-existing directory' ' + test_commit 1 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && + ( + HOME=$(pwd)/none && + export HOME &&
Quoted 2 lines
What is the reason why this test needs to move HOME away from
TRASH_DIRECTORY(set int/test-lib.sh)?
This is to make sure that no user.name nor user.email is configured. That was my idea, hence I answer your question.
Ciao,
Dscho
Quoted 13 lines
Quoted 12 lines
t/t3903-stash.sh +10
-0+ unset GIT_AUTHOR_NAME && + unset GIT_AUTHOR_EMAIL && + unset GIT_COMMITTER_NAME && + unset GIT_COMMITTER_EMAIL && + test_must_fail git config user.email && + echo changed >1.t && + git stash + ) +' + test_done
Quoted 14 lines · Johannes Schindelin
Hi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Slavica
t/t3903-stash.sh +7
-0+test_expect_failure 'stash with HOME as non-existing directory' ' + test_commit 1 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && + ( + HOME=$(pwd)/none && + export HOME &&
Quoted 5 lines
Quoted 2 lines
What is the reason why this test needs to move HOME away from
TRASH_DIRECTORY(set int/test-lib.sh)?This is to make sure that no user.name nor user.email is configured. That was my idea, hence I answer your question.
HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do so to avoid getting affected by the real $HOME/.gitconfig of the user who happens to be running the test suite.
With that in place for ages, this test still moves HOME away from TRASH_DIRECTORY, and that is totally unnecessary if it is only done to avoid getting affected by the real $HOME/.gitconfig of the user. After all, this single test is not unique in its need to avoid reading from user's $HOME/.gitconfig---so I expected there may be other reasons.
That is why I asked, and your response is not quite answering that question.
Hi Junio,
Quoted 17 lines · Junio C Hamano
Quoted 15 lines · Johannes Schindelin
Quoted 12 lines · Junio C Hamano
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1165
Quoted 7 lines · Slavica
t/t3903-stash.sh +7
-0+test_expect_failure 'stash with HOME as non-existing directory' ' + test_commit 1 && + test_config user.useconfigonly true && + test_config stash.usebuiltin true && + ( + HOME=$(pwd)/none && + export HOME &&What is the reason why this test needs to move HOME away from
TRASH_DIRECTORY(set int/test-lib.sh)?
Quoted 6 lines
Quoted 2 lines
This is to make sure that no user.name nor user.email is configured. That was my idea, hence I answer your question.
HOME is set to
TRASH_DIRECTORYint/test-lib.shalready, and we do so to avoid getting affected by the real $HOME/.gitconfig of the user who happens to be running the test suite.
My bad. I should have checked. I was under the impression that we set HOME to the t/ directory and initialized it. But you are right, of course, and that subshell as well as the override of HOME are absolutely unnecessary.
Thanks,
Dscho
Quoted 11 lines
With that in place for ages, this test still moves HOME away from
TRASH_DIRECTORY, and that is totally unnecessary if it is only done to avoid getting affected by the real $HOME/.gitconfig of the user. After all, this single test is not unique in its need to avoid reading from user's $HOME/.gitconfig---so I expected there may be other reasons.That is why I asked, and your response is not quite answering that question.
Quoted 8 lines · Johannes Schindelin
Quoted 3 lines
HOME is set to
TRASH_DIRECTORYint/test-lib.shalready, and we do so to avoid getting affected by the real $HOME/.gitconfig of the user who happens to be running the test suite.My bad. I should have checked. I was under the impression that we set
HOMEto thet/directory and initialized it. But you are right, of course, and that subshell as well as the override ofHOMEare absolutely unnecessary.
I was afraid that I may be missing some future plans to update $TRASH_DIRECTORY/.gitconfig with "git config --global user.name Foo" etc. in an earlier part of the test script, which would have made the subshell and moving HOME elsewhere perfectly good ways to future proof the new test being added (in which case, in-code comment to say that near the assignment to HOME would have been a good improvement).
Not that having them breaks the logic, but they distract the readers by making them wonder what is going on, so I think we can do without the subshell and assignment to HOME.
Thanks.
PATCH v2 1/3 t3903-stash: test without configured user name
In reply to SlavicaChanges since v1:
*changed test title
*removed subshell and HOME override
*fixed weird identation
*unset() replaced with sane_unset()
Slavica (1):
[Outreachy] t3903-stash: test without configured user name
t/t3903-stash.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Signature · 1 line
2.19.1.windows.1
PATCH v2 1/3 t3903-stash: test without configured user name
From: Slavica <[email protected]>
This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here:
https://public-inbox.org/git/[email protected]/T/#u.
--- t/t3903-stash.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)
t/t3903-stash.sh +13 -0
@@ -1156,4 +1156,17 @@ 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_commit 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_must_fail git config user.email && + echo changed >1.t && + git stash +' + test_done
Signature · 1 line
2.19.1.windows.1
Quoted 4 lines · Slavica Djukic
This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't.
Thanks for re-rolling. This version looks better. One comment below...
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1164
Quoted 13 lines
Signed-off-by Slavica Djukic <[email protected]>---
t/t3903-stash.sh +9
-0@@ -1156,4 +1156,17 @@ test_expect_success 'stash -- <subdir> works with binary files' ' +test_expect_failure 'stash works when user.name and user.email are not set' ' + test_commit 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_must_fail git config user.email &&
Instead of simply asserting that 'user.email' is not set here, you could instead proactively ensure that it is not set. That is, instead of the test_must_fail(), do this:
test_unconfig user.email &&
test_unconfig user.name &&Quoted 8 lines
t/t3903-stash.sh +4
-1+ echo changed >1.t && + git stash +' + test_done --2.19.1.windows.1
Quoted 9 lines · Eric Sunshine
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1167
Quoted 8 lines
t/t3903-stash.sh +8
-0+ test_commit 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_must_fail git config user.email &&
Quoted 6 lines
Instead of simply asserting that 'user.email' is not set here, you could instead proactively ensure that it is not set. That is, instead of the
test_must_fail(), do this:test_unconfig user.email && test_unconfig user.name &&
Yes, it would be more in line with what is done to the environment variables and to other configuration variables in the same block.
Not that I think that this inconsistency is end of the world ;-)
Thanks.
Quoted 8 lines
Quoted 8 lines
t/t3903-stash.sh +4
-1+ echo changed >1.t && + git stash +' + test_done --2.19.1.windows.1
PATCH v3 1/3 t3903-stash: test without configured user name
In reply to Slavica DjukicChanges since v1:
*changed:
test_must_fail git config user.email
to:
test_unconfig user.email &&
test_unconfig user.name
This is done to make sure that user.email and user.name are not set, instead of asserting it with test_must_fail config user.email.
Slavica (1):
[Outreachy] t3903-stash: test without configured user name
t/t3903-stash.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Signature · 1 line
2.19.1.windows.1
PATCH v3 1/3 t3903-stash: test without configured user name
From: Slavica <[email protected]>
This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here:
https://public-inbox.org/git/[email protected]/T/#u.
--- t/t3903-stash.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
t/t3903-stash.sh +14 -0
@@ -1156,4 +1156,18 @@ 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_commit 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 && + echo changed >1.t && + git stash +' + test_done
Signature · 1 line
2.19.1.windows.1
Quoted 1 line · Slavica Djukic
From: Slavica <[email protected]>
Please make sure this matches your sign-off below.
Quoted 6 lines
This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here:
https://public-inbox.org/git/[email protected]/T/#u.
As the four lines above summarize the issue being highlighted by the expect-failure rather well, the last two lines are unnecessary. Please remove them. Alternatively, you can place them after the three-dash lines we see below.
Quoted 10 lines
Signed-off-by Slavica Djukic <[email protected]>--- t/t3903-stash.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)t/t3903-stash.sh +0
-0
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1160
Quoted 5 lines
t/t3903-stash.sh +2
-0@@ -1156,4 +1156,18 @@ 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_commit 1 &&
Just being curious, but do we need a fresh commit created at this point in the test? Many tests before this one begin with "git reset" and then run "git stash" without ever creating commit themselves, instead relying on the fact that there already is at least one commit created in the "setup" phase of the test that a "stash" created can be made relative to. I do not think this test is all that special in that regard to require its own commit.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1167
Quoted 7 lines
t/t3903-stash.sh +7
-0+ 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 &&
There are trailing whitespaces on the line above. Please remove.
Also, Don't be original in the form alone---all other tests in this file indent with a leading HT, not four SPs. Please match the style of surrounding code.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1173
Quoted 6 lines
t/t3903-stash.sh +5
-0+ test_unconfig user.name && + echo changed >1.t && + git stash +' + test_done
Thanks. Please do not reroll the next round at too rapid a pace.
Quoted 29 lines · Junio C Hamano
Quoted 1 line · Slavica Djukic
From: Slavica <[email protected]>
Please make sure this matches your sign-off below.
Quoted 6 lines
This is part of enhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured. Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't. The issue is discussed here:
https://public-inbox.org/git/[email protected]/T/#u.As the four lines above summarize the issue being highlighted by the expect-failure rather well, the last two lines are unnecessary. Please remove them. Alternatively, you can place them after the three-dash lines we see below.
Quoted 13 lines
Signed-off-by Slavica Djukic <[email protected]>--- t/t3903-stash.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)t/t3903-stash.sh +0
-0@@ -1156,4 +1156,18 @@ test_expect_success 'stash -- <subdir> works with binary files' ' test_path_is_file subdir/untracked '
Quoted 9 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1160
Quoted 2 lines
t/t3903-stash.sh +2
-0+test_expect_failure 'stash works when user.name and user.email are not set' ' + test_commit 1 &&Just being curious, but do we need a fresh commit created at this point in the test? Many tests before this one begin with "git reset" and then run "git stash" without ever creating commit themselves, instead relying on the fact that there already is at least one commit created in the "setup" phase of the test that a "stash" created can be made relative to. I do not think this test is all that special in that regard to require its own commit.
No, we don't need fresh commit here. Thank you for this and all other suggestions.
I've changed test according to them.
Quoted 14 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1167
Quoted 8 lines
t/t3903-stash.sh +7
-0+ 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 &&There are trailing whitespaces on the line above. Please remove.
Also, Don't be original in the form alone---all other tests in this file indent with a leading HT, not four SPs. Please match the style of surrounding code.
Quoted 7 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1173
Quoted 6 lines
t/t3903-stash.sh +5
-0+ test_unconfig user.name && + echo changed >1.t && + git stash +' + test_doneThanks. Please do not reroll the next round at too rapid a pace.
I've taken my time for next round, I am working on 2/3 and 3/3 parts as well. I wouldn't have sent this patch if I understood you well in previous reply.
Thank you,
Slavica.
Quoted 3 lines
cover make stash work if user.name and user.email are not configured
In reply to Slavica DjukicEnhancement request that ask for 'git stash' to work even if 'user.name' and 'user.email' are not configured.
Due to an implementation detail, git-stash undesirably requires 'user.name' and 'user.email' to be set, but shouldn't.
Slavica Djukic(3):
[Outreachy] t3903-stash: test without configured user.name and
user.email
[Outreachy] ident: introduce set_fallback_ident() function [Outreachy] stash: use set_fallback_ident() function
builtin/stash.c | 1 + cache.h | 1 + ident.c | 17 +++++++++++++++++ t/t3903-stash.sh | 15 +++++++++++++++ 4 files changed, 34 insertions(+)
Signature · 1 line
2.19.1.windows.1
PATCH 1/3 t3903-stash: test without configured user.name and user.email
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.
--- t/t3903-stash.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
t/t3903-stash.sh +15 -0
@@ -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
Signature · 1 line
2.19.1.windows.1
Re: [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email
Quoted 2 lines · Slavica Djukic
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".
Quoted 2 lines
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.
Quoted 1 line
Signed-off-by Slavica Djukic <[email protected]>
Thanks!
PATCH 1/1 stash: tolerate missing user identity
In reply to Slavica DjukicThe "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.
---
* 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(-)
git-stash.sh +17 -0
@@ -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
t/t3903-stash.sh +1 -1
@@ -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 &&
Signature · 1 line
2.19.1-801-gd582ea202b
PATCH 2/3 ident: introduce set_fallback_ident() function
In reply to Slavica DjukicUsually, 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.
--- cache.h | 1 + ident.c | 17 +++++++++++++++++ 2 files changed, 18 insertions(+)
cache.h +1 -0
@@ -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 {
ident.c +17 -0
@@ -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) {
Signature · 1 line
2.19.1.windows.1
Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
Quoted 11 lines · Slavica Djukic
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 <[email protected]>---
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.
Quoted 6 lines
ident.c +6
-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.c int git_ident_config(const char *var, const char *value, void *data) · line 523
Quoted 10 lines
ident.c +10
-0+ 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.
Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
Quoted 13 lines · Junio C Hamano
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.
Quoted 4 lines
- 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(+)
git-stash.sh +17 -0
@@ -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
Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function
Quoted 4 lines · Junio C Hamano
As we currently have no idea when
builtin/stash.cbecomes 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 3/3 stash: use set_fallback_ident() function
In reply to Slavica DjukicCall 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/[email protected]/T/#ma4fb50903a54cbcdecd4ef05856bf8094bc3c323
user points out that he would find it useful if stash had --author option.
--- builtin/stash.c | 1 + t/t3903-stash.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
builtin/stash.c +1 -0
@@ -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" ,"[email protected]" );git_config (git_default_config,NULL ); argc = parse_options(argc, argv, prefix, options, git_stash_usage,
t/t3903-stash.sh +1 -1
@@ -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 &&
Signature · 1 line
2.19.1.windows.1
v2 cover make stash work if user.name and user.email are not configured
In reply to Slavica DjukicChanges 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(+)
Signature · 1 line
2.19.1.1052.gd166e6afe
PATCH v2 1/2 t3903-stash: test without configured user.name and user.email
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.
--- t/t3903-stash.sh | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
t/t3903-stash.sh +23 -0
@@ -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
Signature · 1 line
2.19.1.1052.gd166e6afe
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Hi Slavica,
this looks very good to me. Just one grammar thing:
Quoted 3 lines · Slavica Djukic
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
Quoted 42 lines
Signed-off-by Slavica Djukic <[email protected]>--- t/t3903-stash.sh | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)t/t3903-stash.sh +23
-0@@ -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_doneSignature · 1 line
2.19.1.1052.gd166e6afe
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
In reply to Slavica Djukict/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1101
Quoted 3 lines · Slavica Djukic
t/t3903-stash.sh +3
-0+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.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1104
Quoted 3 lines
t/t3903-stash.sh +3
-0+ >1 && + git add 1 && + git stash &&
And we make sure we can create stash.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1106
Quoted 2 lines
t/t3903-stash.sh +2
-0+ 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?
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1110
Quoted 4 lines
t/t3903-stash.sh +4
-0+ >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.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1117
Quoted 7 lines
t/t3903-stash.sh +7
-0+ ( + 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.
Quoted 5 lines
t/t3903-stash.sh +4
-0+ git stash + ) +' + test_done
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Quoted 4 lines · Junio C Hamano
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.
Quoted 9 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1117
Quoted 7 lines
t/t3903-stash.sh +7
-0+ ( + 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.
Quoted 5 lines
Quoted 5 lines
t/t3903-stash.sh +4
-0+ git stash + ) +' + test_done
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
In reply to Junio C HamanoQuoted 15 lines · Junio C Hamano
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1101
Quoted 3 lines · Slavica Djukic
t/t3903-stash.sh +3
-0+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.t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1104
Quoted 3 lines
t/t3903-stash.sh +3
-0+ >1 && + git add 1 && + git stash &&And we make sure we can create stash.
Quoted 9 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1106
Quoted 2 lines
t/t3903-stash.sh +2
-0+ 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.
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
In reply to Junio C HamanoHi Junio,
Quoted 12 lines · Junio C Hamano
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1101
Quoted 3 lines · Slavica Djukic
t/t3903-stash.sh +3
-0+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.
Quoted 9 lines
Anyway, we grab the committer ident we use by default during the test with this command. OK.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1104
Quoted 3 lines
t/t3903-stash.sh +3
-0+ >1 && + git add 1 && + git stash &&And we make sure we can create stash.
Quoted 8 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1106
Quoted 2 lines
t/t3903-stash.sh +2
-0+ 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.
Quoted 36 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1110
Quoted 5 lines
t/t3903-stash.sh +4
-0+ >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.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1117
Quoted 7 lines
t/t3903-stash.sh +7
-0+ ( + 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.
Quoted 6 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1122
Quoted 5 lines
t/t3903-stash.sh +4
-0+ git stash + ) +' + test_done
Thank you,
Slavica
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Quoted 8 lines · Slavica Djukic
Quoted 8 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1106
Quoted 2 lines
t/t3903-stash.sh +2
-0+ 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?
Quoted 5 lines
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.
Quoted 3 lines
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.
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Hi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Slavica Djukic
Quoted 8 lines
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1106
Quoted 2 lines
t/t3903-stash.sh +2
-0+ 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?
Quoted 8 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 3 lines
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
Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email
Quoted 5 lines · Slavica Djukic
Quoted 3 lines
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:
Quoted 7 lines
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_cmpexpect 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.
PATCH v2 2/2 stash: tolerate missing user identity
In reply to Slavica DjukicThe "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.
--- git-stash.sh | 17 +++++++++++++++++ t/t3903-stash.sh | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-)
git-stash.sh +17 -0
@@ -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
t/t3903-stash.sh +1 -1
@@ -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 &&
Signature · 1 line
2.19.1.1052.gd166e6afe
Re: [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity
Quoted 30 lines · Slavica Djukic
git-stash.sh | 17 +++++++++++++++++ t/t3903-stash.sh | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-)git-stash.sh +14
-0@@ -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
git-stash.sh clear_stash () { · line 89
Quoted 9 lines
git-stash.sh +3
-0@@ -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.
t/t3903-stash.sh test_expect_success 'stash -- <subdir> works with binary files' ' · line 1099
Quoted 10 lines
t/t3903-stash.sh +1
-1@@ -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).
Quoted 3 lines
t/t3903-stash.sh +0
-0git reset && git var GIT_COMMITTER_IDENT >expected && >1 &&
v3 cover make stash work if user.name and user.email are not configured
In reply to Slavica DjukicChanges 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(+)
Signature · 1 line
2.19.1.1052.gd166e6afe
PATCH v3 1/1 stash: tolerate missing user identity
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.
--- git-stash.sh | 17 +++++++++++++++++ t/t3903-stash.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)
git-stash.sh +17 -0
@@ -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
t/t3903-stash.sh +28 -0
@@ -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
Signature · 1 line
2.19.1.1052.gd166e6afe