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