{"thread":{"id":"49651","subject":"[PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","startedAt":"2018-10-23T16:31:18Z","lastAt":"2018-11-18T13:44:40Z","messageCount":41,"participants":["Slavica","Christian Couder","Eric Sunshine","Junio C Hamano","Johannes Schindelin","Slavica Djukic"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"361297","messageId":"20181023162941.3840-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":null,"subject":"[PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Slavica","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-10-23T16:29:41Z","receivedAt":"2018-10-23T16:31:18Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.\nThe issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n\nSigned-off-by: Slavica \n---\n t/t3903-stash.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 9e06494ba0..9ff34a65bc 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1156,4 +1156,21 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n+test_expect_failure 'stash with HOME as non-existing directory' '\n+ test_commit 1 &&\n+ test_config user.useconfigonly true &&\n+ test_config stash.usebuiltin true &&\n+ (\n+ HOME=$(pwd)/none &&\n+ export HOME &&\n+ unset GIT_AUTHOR_NAME &&\n+ unset GIT_AUTHOR_EMAIL &&\n+ unset GIT_COMMITTER_NAME &&\n+ unset GIT_COMMITTER_EMAIL &&\n+ test_must_fail git config user.email &&\n+ echo changed >1.t &&\n+\t\tgit stash\n+ )\n+'\n+\n test_done\n-- \n2.19.1.windows.1\n\n"},{"id":"361310","messageId":"CAP8UFD35aOb5weDcDVFth96e+H-as_Q9bLPuCpSDReKJERnM7Q@mail.gmail.com","threadId":"49651","inReplyTo":"20181023162941.3840-1-slawica92@hotmail.com","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-10-23T18:52:05Z","receivedAt":"2018-10-23T18:52:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Oct 23, 2018 at 6:35 PM Slavica wrote:\n>\n> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.\n> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n\nWe prefer commit messages that contain as much as possible all the\ninformation necessary to understand the patch without links to other\nplaces.\n\nIt seems that only this email from you reached me. Did you send other\nemails for patches 2/3 and 3/3?\n\n[...]\n\n> + (\n> + HOME=$(pwd)/none &&\n> + export HOME &&\n> + unset GIT_AUTHOR_NAME &&\n> + unset GIT_AUTHOR_EMAIL &&\n> + unset GIT_COMMITTER_NAME &&\n> + unset GIT_COMMITTER_EMAIL &&\n> + test_must_fail git config user.email &&\n> + echo changed >1.t &&\n> + git stash\n\nIt seems that the above line is not indented like the previous ones.\n\n> + )\n> +'\n\nThanks for contributing,\nChristian.\n"},{"id":"361317","messageId":"CAPig+cSeWhdKXBdwm6C5XTY0wjGsxBX9GUvPv5nWNZEQRirDUA@mail.gmail.com","threadId":"49651","inReplyTo":"20181023162941.3840-1-slawica92@hotmail.com","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-23T19:19:49Z","receivedAt":"2018-10-23T19:20:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Oct 23, 2018 at 12:31 PM Slavica wrote:\n> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.\n> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n\nAs Christian mentioned already, it's best to try to describe the issue\nsuccinctly in the commit message so readers can understand it without\nchasing a link. For this simple case, it should be sufficient to\nexplain that, due to an implementation detail, git-stash undesirably\nrequires 'user.name' and 'user.email' to be set, but shouldn't.\n\n> Signed-off-by: Slavica \n> ---\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> @@ -1156,4 +1156,21 @@ test_expect_success 'stash -- works with binary files' '\n> +test_expect_failure 'stash with HOME as non-existing directory' '\n\nThe purpose of this test is to demonstrate that git-stash has an\nundesirable requirement that 'user.name' and 'user.email' be set. The\ntest title should reflect that. So, instead of talking about\nnon-existent HOME (which is just an implementation detail of the\ntest), a better test title would be something like \"stash works when\nuser.name and user.email are not set\".\n\n> + test_commit 1 &&\n> + test_config user.useconfigonly true &&\n> + test_config stash.usebuiltin true &&\n> + (\n> + HOME=$(pwd)/none &&\n> + export HOME &&\n> + unset GIT_AUTHOR_NAME &&\n\nUse sane_unset() for all of these rather than bare 'unset'.\n\n> + unset GIT_AUTHOR_EMAIL &&\n> + unset GIT_COMMITTER_NAME &&\n> + unset GIT_COMMITTER_EMAIL &&\n> + test_must_fail git config user.email &&\n> + echo changed >1.t &&\n> + git stash\n\nChristian already mentioned the odd indentation.\n\n> + )\n> +'\n"},{"id":"361343","messageId":"xmqqd0s0qcuv.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"20181023162941.3840-1-slawica92@hotmail.com","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T02:48:08Z","receivedAt":"2018-10-24T02:48:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica writes:\n\n> +test_expect_failure 'stash with HOME as non-existing directory' '\n> + test_commit 1 &&\n> + test_config user.useconfigonly true &&\n> + test_config stash.usebuiltin true &&\n> + (\n> + HOME=$(pwd)/none &&\n> + export HOME &&\n\nWhat is the reason why this test needs to move HOME away from\nTRASH_DIRECTORY (set in t/test-lib.sh)?\n\n> + unset GIT_AUTHOR_NAME &&\n> + unset GIT_AUTHOR_EMAIL &&\n> + unset GIT_COMMITTER_NAME &&\n> + unset GIT_COMMITTER_EMAIL &&\n> + test_must_fail git config user.email &&\n> + echo changed >1.t &&\n> +\t\tgit stash\n> + )\n> +'\n> +\n> test_done\n"},{"id":"361380","messageId":"nycvar.QRO.7.76.6.1810240938310.4546@tvgsbejvaqbjf.bet","threadId":"49651","inReplyTo":"xmqqd0s0qcuv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T07:39:11Z","receivedAt":"2018-10-24T07:39:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 24 Oct 2018, Junio C Hamano wrote:\n\n> Slavica writes:\n> \n> > +test_expect_failure 'stash with HOME as non-existing directory' '\n> > + test_commit 1 &&\n> > + test_config user.useconfigonly true &&\n> > + test_config stash.usebuiltin true &&\n> > + (\n> > + HOME=$(pwd)/none &&\n> > + export HOME &&\n> \n> What is the reason why this test needs to move HOME away from\n> TRASH_DIRECTORY (set in t/test-lib.sh)?\n\nThis is to make sure that no user.name nor user.email is configured. That\nwas my idea, hence I answer your question.\n\nCiao,\nDscho\n\n> \n> > + unset GIT_AUTHOR_NAME &&\n> > + unset GIT_AUTHOR_EMAIL &&\n> > + unset GIT_COMMITTER_NAME &&\n> > + unset GIT_COMMITTER_EMAIL &&\n> > + test_must_fail git config user.email &&\n> > + echo changed >1.t &&\n> > +\t\tgit stash\n> > + )\n> > +'\n> > +\n> > test_done\n> \n"},{"id":"361402","messageId":"xmqqmur3mzsm.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"nycvar.QRO.7.76.6.1810240938310.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T09:58:33Z","receivedAt":"2018-10-24T09:58:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Wed, 24 Oct 2018, Junio C Hamano wrote:\n>\n>> Slavica writes:\n>> \n>> > +test_expect_failure 'stash with HOME as non-existing directory' '\n>> > + test_commit 1 &&\n>> > + test_config user.useconfigonly true &&\n>> > + test_config stash.usebuiltin true &&\n>> > + (\n>> > + HOME=$(pwd)/none &&\n>> > + export HOME &&\n>> \n>> What is the reason why this test needs to move HOME away from\n>> TRASH_DIRECTORY (set in t/test-lib.sh)?\n>\n> This is to make sure that no user.name nor user.email is configured. That\n> was my idea, hence I answer your question.\n\nHOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do\nso to avoid getting affected by the real $HOME/.gitconfig of the\nuser who happens to be running the test suite.\n\nWith that in place for ages, this test still moves HOME away from\nTRASH_DIRECTORY, and that is totally unnecessary if it is only done\nto avoid getting affected by the real $HOME/.gitconfig of the user.\nAfter all, this single test is not unique in its need to avoid\nreading from user's $HOME/.gitconfig---so I expected there may be\nother reasons.\n\nThat is why I asked, and your response is not quite answering that\nquestion.\n"},{"id":"361411","messageId":"45cf8bf9-adfa-655e-0ded-fdb71707f7ad@gmail.com","threadId":"49651","inReplyTo":"CAP8UFD35aOb5weDcDVFth96e+H-as_Q9bLPuCpSDReKJERnM7Q@mail.gmail.com","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Slavica","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-10-24T13:56:06Z","receivedAt":"2018-10-24T13:56:14Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"\nOn 23-Oct-18 8:52 PM, Christian Couder wrote:\n> On Tue, Oct 23, 2018 at 6:35 PM Slavica wrote:\n>> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.\n>> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n> We prefer commit messages that contain as much as possible all the\n> information necessary to understand the patch without links to other\n> places.\n>\n> It seems that only this email from you reached me. Did you send other\n> emails for patches 2/3 and 3/3?\n>\n> [...]\n\nOkay, I will change that. This is my first patch and I am still adapting.\n\nEmails for patches 2/3 and 3/3 because aren't there because I am still \npreparing them.\n\n(I didn't know if I had 3 patches in plan that they should be sent at \nalmost the same time.)\n\n>\n>> + (\n>> + HOME=$(pwd)/none &&\n>> + export HOME &&\n>> + unset GIT_AUTHOR_NAME &&\n>> + unset GIT_AUTHOR_EMAIL &&\n>> + unset GIT_COMMITTER_NAME &&\n>> + unset GIT_COMMITTER_EMAIL &&\n>> + test_must_fail git config user.email &&\n>> + echo changed >1.t &&\n>> + git stash\n> It seems that the above line is not indented like the previous ones.\nI don't know what is the reason, in my IDE everything seems fine, but \nI'll fix it.\n>\n>> + )\n>> +'\n> Thanks for contributing,\n> Christian.\n\nYou are welcome,\n\nSlavica\n\n>\n>\n"},{"id":"361418","messageId":"nycvar.QRO.7.76.6.1810241717240.4546@tvgsbejvaqbjf.bet","threadId":"49651","inReplyTo":"xmqqmur3mzsm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-10-24T15:18:39Z","receivedAt":"2018-10-24T15:18:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 24 Oct 2018, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Wed, 24 Oct 2018, Junio C Hamano wrote:\n> >\n> >> Slavica writes:\n> >> \n> >> > +test_expect_failure 'stash with HOME as non-existing directory' '\n> >> > + test_commit 1 &&\n> >> > + test_config user.useconfigonly true &&\n> >> > + test_config stash.usebuiltin true &&\n> >> > + (\n> >> > + HOME=$(pwd)/none &&\n> >> > + export HOME &&\n> >> \n> >> What is the reason why this test needs to move HOME away from\n> >> TRASH_DIRECTORY (set in t/test-lib.sh)?\n> >\n> > This is to make sure that no user.name nor user.email is configured. That\n> > was my idea, hence I answer your question.\n> \n> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do\n> so to avoid getting affected by the real $HOME/.gitconfig of the\n> user who happens to be running the test suite.\n\nMy bad. I should have checked. I was under the impression that we set\n`HOME` to the `t/` directory and initialized it. But you are right, of\ncourse, and that subshell as well as the override of `HOME` are absolutely\nunnecessary.\n\nThanks,\nDscho\n\n> \n> With that in place for ages, this test still moves HOME away from\n> TRASH_DIRECTORY, and that is totally unnecessary if it is only done\n> to avoid getting affected by the real $HOME/.gitconfig of the user.\n> After all, this single test is not unique in its need to avoid\n> reading from user's $HOME/.gitconfig---so I expected there may be\n> other reasons.\n> \n> That is why I asked, and your response is not quite answering that\n> question.\n> \n"},{"id":"361434","messageId":"cover.1540410925.git.slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181023162941.3840-1-slawica92@hotmail.com","subject":"[PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-10-24T20:01:55Z","receivedAt":"2018-10-24T20:02:50Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Changes since v1:\n\n *changed test title\n *removed subshell and HOME override\n *fixed weird identation\n *unset() replaced with sane_unset()\n\nSlavica (1):\n [Outreachy] t3903-stash: test without configured user name\n\n t/t3903-stash.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\n-- \n2.19.1.windows.1\n\n"},{"id":"361435","messageId":"a055296c2034a44f02c253ce3194018b21eb4e1f.1540410925.git.slawica92@hotmail.com","threadId":"49651","inReplyTo":"cover.1540410925.git.slawica92@hotmail.com","subject":"[PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-10-24T20:05:12Z","receivedAt":"2018-10-24T20:06:07Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"From: Slavica \n\nThis is part of enhancement request that ask for 'git stash' to work\neven if 'user.name' and 'user.email' are not configured.\nDue to an implementation detail, git-stash undesirably requires\n'user.name' and 'user.email' to be set, but shouldn't.\nThe issue is discussed here:\nhttps://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n\nSigned-off-by: Slavica Djukic \n---\n t/t3903-stash.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 9e06494ba0..048998d5ce 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1156,4 +1156,17 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n+test_expect_failure 'stash works when user.name and user.email are not set' '\n+ test_commit 1 &&\n+ test_config user.useconfigonly true &&\n+ test_config stash.usebuiltin true &&\n+ sane_unset GIT_AUTHOR_NAME &&\n+ sane_unset GIT_AUTHOR_EMAIL &&\n+ sane_unset GIT_COMMITTER_NAME &&\n+ sane_unset GIT_COMMITTER_EMAIL &&\n+ test_must_fail git config user.email &&\n+ echo changed >1.t &&\n+ git stash\n+'\n+\n test_done\n-- \n2.19.1.windows.1\n\n"},{"id":"361436","messageId":"CAPig+cRGn0Z7F7TpSwF=8cQJpN1LJkQb2VxHMDi6j6wsaqkORg@mail.gmail.com","threadId":"49651","inReplyTo":"a055296c2034a44f02c253ce3194018b21eb4e1f.1540410925.git.slawica92@hotmail.com","subject":"Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-24T20:25:55Z","receivedAt":"2018-10-24T20:26:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 24, 2018 at 4:06 PM Slavica Djukic\n wrote:\n> This is part of enhancement request that ask for 'git stash' to work\n> even if 'user.name' and 'user.email' are not configured.\n> Due to an implementation detail, git-stash undesirably requires\n> 'user.name' and 'user.email' to be set, but shouldn't.\n\nThanks for re-rolling. This version looks better. One comment below...\n\n> Signed-off-by: Slavica Djukic \n> ---\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> @@ -1156,4 +1156,17 @@ test_expect_success 'stash -- works with binary files' '\n> +test_expect_failure 'stash works when user.name and user.email are not set' '\n> + test_commit 1 &&\n> + test_config user.useconfigonly true &&\n> + test_config stash.usebuiltin true &&\n> + sane_unset GIT_AUTHOR_NAME &&\n> + sane_unset GIT_AUTHOR_EMAIL &&\n> + sane_unset GIT_COMMITTER_NAME &&\n> + sane_unset GIT_COMMITTER_EMAIL &&\n> + test_must_fail git config user.email &&\n\nInstead of simply asserting that 'user.email' is not set here, you\ncould instead proactively ensure that it is not set. That is, instead\nof the test_must_fail(), do this:\n\n test_unconfig user.email &&\n test_unconfig user.name &&\n\n> + echo changed >1.t &&\n> + git stash\n> +'\n> +\n> test_done\n> --\n> 2.19.1.windows.1\n>\n"},{"id":"361467","messageId":"xmqq4ldalkxj.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"nycvar.QRO.7.76.6.1810241717240.4546@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T04:17:12Z","receivedAt":"2018-10-25T04:17:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do\n>> so to avoid getting affected by the real $HOME/.gitconfig of the\n>> user who happens to be running the test suite.\n>\n> My bad. I should have checked. I was under the impression that we set\n> `HOME` to the `t/` directory and initialized it. But you are right, of\n> course, and that subshell as well as the override of `HOME` are absolutely\n> unnecessary.\n\nI was afraid that I may be missing some future plans to update\n$TRASH_DIRECTORY/.gitconfig with \"git config --global user.name Foo\"\netc. in an earlier part of the test script, which would have made\nthe subshell and moving HOME elsewhere perfectly good ways to future\nproof the new test being added (in which case, in-code comment to\nsay that near the assignment to HOME would have been a good\nimprovement).\n\nNot that having them breaks the logic, but they distract the\nreaders by making them wonder what is going on, so I think we can do\nwithout the subshell and assignment to HOME.\n\nThanks.\n"},{"id":"361468","messageId":"xmqqy3amk53s.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"45cf8bf9-adfa-655e-0ded-fdb71707f7ad@gmail.com","subject":"Re: [PATCH 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T04:44:23Z","receivedAt":"2018-10-25T04:44:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica writes:\n\n> On 23-Oct-18 8:52 PM, Christian Couder wrote:\n>> On Tue, Oct 23, 2018 at 6:35 PM Slavica wrote:\n>>> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.\n>>> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n>> We prefer commit messages that contain as much as possible all the\n>> information necessary to understand the patch without links to other\n>> places.\n>>\n>> It seems that only this email from you reached me. Did you send other\n>> emails for patches 2/3 and 3/3?\n>>\n>> [...]\n>\n> Okay, I will change that. This is my first patch and I am still adapting.\n>\n> Emails for patches 2/3 and 3/3 because aren't there because I am still\n> preparing them.\n>\n> (I didn't know if I had 3 patches in plan that they should be sent at\n> almost the same time.)\n\nIt is more efficient for everybody involved.\n\n - You may discover that 1/3 you just (thought) finished was not\n sufficient while working on 2/3 and 3/3, and by the time you are\n pretty close to finishing 2/3 and 3/3, you may want to update 1/3\n in a big way. Sending a premature version and having others to\n review is wasting everbody's time.\n\n - Your 1/3 might become perfect alone with help from others'\n reviews and your updates, but after that everybody may forget\n about it when you are ready to send out 2/3 and 3/3; if these\n three are truly related patches in a single topic, you would want\n to have what 1/3 did fresh in your reviewers' minds. You'd have\n to find the old message of 1/3 and make 2/3 and 3/3 responses to\n it to keep them properly threaded (which may take your time), and\n reviewers need to refresh their memory by going back to 1/3\n before reviewing 2/3 and 3/3\n\nOne thing I learned twice while working in this project is that open\nsource development is not a race to produce and show your product as\nquickly as possible.\n\nWhen I was an individual contributor, the project was young and\nthere were many people with good and competing ideas working to\nachieve more-or-less the same goal. It felt like a competition\nto get *MY* version of the vision, design and implementation over\nothers' adopted and one way to stay in the competition was to send\nthings as quickly as possible. I didn't know better, and I think I\nended up wasting many people's time that way.\n\nThat changed when I became the maintainer, as (1) I no longer had to\nrace with anybody ;-), and (2) I introduced the 'pu' (proposed\nupdate) system so that anything that was queued early can be\ndiscarded and replaced when a better thing come within a reasonable\ntimeframe.\n\nAnd then I re-learned the same \"this is not a race\" lesson a couple\nof years ago, when I started working in a timezone several hours\naway from the most active participants for a few months at a time.\nI do not have to respond to a message I see on the list immediately,\nas it is too late to catch the sender who is already in bed ;-)\n\n\nSo take your time and make sure what you are sending out can be\nreviewed the most efficiently. Completing 2/3 and 3/3 before\nsending 1/3 out to avoid having to redo 1/3 and avoid having\nreviewers to spend their time piecemeal is one thing. Making sure\nthat the patch does not have style issues that distract reviewers'\nattention is another.\n\nSitting on what you think you have completed for a few days allows\nyou to review your product with fresh eyes before sending them out,\nwhich is another benefit of trying not to rush.\n\n\n"},{"id":"361469","messageId":"xmqqtvlak4wu.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"CAPig+cRGn0Z7F7TpSwF=8cQJpN1LJkQb2VxHMDi6j6wsaqkORg@mail.gmail.com","subject":"Re: [PATCH v2 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T04:48:33Z","receivedAt":"2018-10-25T04:48:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + test_commit 1 &&\n>> + test_config user.useconfigonly true &&\n>> + test_config stash.usebuiltin true &&\n>> + sane_unset GIT_AUTHOR_NAME &&\n>> + sane_unset GIT_AUTHOR_EMAIL &&\n>> + sane_unset GIT_COMMITTER_NAME &&\n>> + sane_unset GIT_COMMITTER_EMAIL &&\n>> + test_must_fail git config user.email &&\n>\n> Instead of simply asserting that 'user.email' is not set here, you\n> could instead proactively ensure that it is not set. That is, instead\n> of the test_must_fail(), do this:\n>\n> test_unconfig user.email &&\n> test_unconfig user.name &&\n\nYes, it would be more in line with what is done to the environment\nvariables and to other configuration variables in the same block.\n\nNot that I think that this inconsistency is end of the world ;-)\n\nThanks.\n\n>> + echo changed >1.t &&\n>> + git stash\n>> +'\n>> +\n>> test_done\n>> --\n>> 2.19.1.windows.1\n>>\n"},{"id":"361551","messageId":"cover.1540494231.git.slawica92@hotmail.com","threadId":"49651","inReplyTo":"cover.1540410925.git.slawica92@hotmail.com","subject":"[PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-10-25T19:13:15Z","receivedAt":"2018-10-25T19:15:00Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Changes since v1:\n\n *changed:\n\t test_must_fail git config user.email \n\t to:\n\t test_unconfig user.email && \n\t test_unconfig user.name \n\n\tThis is done to make sure that user.email and user.name are not set,\n\tinstead of asserting it with test_must_fail config user.email.\n\n\n\nSlavica (1):\n [Outreachy] t3903-stash: test without configured user name\n\n t/t3903-stash.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\n-- \n2.19.1.windows.1\n\n"},{"id":"361552","messageId":"9ea38cd8e98890b8264696dfd647c1f9e709ae9e.1540494231.git.slawica92@hotmail.com","threadId":"49651","inReplyTo":"cover.1540494231.git.slawica92@hotmail.com","subject":"[PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-10-25T19:20:45Z","receivedAt":"2018-10-25T19:21:14Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"From: Slavica \n\nThis is part of enhancement request that ask for 'git stash' to work\neven if 'user.name' and 'user.email' are not configured.\nDue to an implementation detail, git-stash undesirably requires\n'user.name' and 'user.email' to be set, but shouldn't.\nThe issue is discussed here:\nhttps://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n\nSigned-off-by: Slavica Djukic \n---\n t/t3903-stash.sh | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 9e06494ba0..ae2c905343 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n+test_expect_failure 'stash works when user.name and user.email are not set' '\n+ test_commit 1 &&\n+ test_config user.useconfigonly true &&\n+ test_config stash.usebuiltin true &&\n+ sane_unset GIT_AUTHOR_NAME &&\n+ sane_unset GIT_AUTHOR_EMAIL &&\n+ sane_unset GIT_COMMITTER_NAME &&\n+ sane_unset GIT_COMMITTER_EMAIL &&\n+ test_unconfig user.email && \n+ test_unconfig user.name &&\n+ echo changed >1.t &&\n+ git stash\n+'\n+\n test_done\n-- \n2.19.1.windows.1\n\n"},{"id":"361584","messageId":"xmqqa7n1fr2c.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"9ea38cd8e98890b8264696dfd647c1f9e709ae9e.1540494231.git.slawica92@hotmail.com","subject":"Re: [PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-26T01:13:31Z","receivedAt":"2018-10-26T01:13:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica Djukic writes:\n\n> From: Slavica \n\nPlease make sure this matches your sign-off below.\n\n> This is part of enhancement request that ask for 'git stash' to work\n> even if 'user.name' and 'user.email' are not configured.\n> Due to an implementation detail, git-stash undesirably requires\n> 'user.name' and 'user.email' to be set, but shouldn't.\n> The issue is discussed here:\n> https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n\nAs the four lines above summarize the issue being highlighted by the\nexpect-failure rather well, the last two lines are unnecessary.\nPlease remove them. Alternatively, you can place them after the\nthree-dash lines we see below.\n\n> Signed-off-by: Slavica Djukic \n> ---\n> t/t3903-stash.sh | 14 ++++++++++++++\n> 1 file changed, 14 insertions(+)\n>\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 9e06494ba0..ae2c905343 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with binary files' '\n> \ttest_path_is_file subdir/untracked\n> '\n> \n> +test_expect_failure 'stash works when user.name and user.email are not set' '\n> + test_commit 1 &&\n\nJust being curious, but do we need a fresh commit created at this\npoint in the test? Many tests before this one begin with \"git reset\"\nand then run \"git stash\" without ever creating commit themselves,\ninstead relying on the fact that there already is at least one\ncommit created in the \"setup\" phase of the test that a \"stash\"\ncreated can be made relative to. I do not think this test is all\nthat special in that regard to require its own commit.\n\n> + test_config user.useconfigonly true &&\n> + test_config stash.usebuiltin true &&\n> + sane_unset GIT_AUTHOR_NAME &&\n> + sane_unset GIT_AUTHOR_EMAIL &&\n> + sane_unset GIT_COMMITTER_NAME &&\n> + sane_unset GIT_COMMITTER_EMAIL &&\n> + test_unconfig user.email && \n\nThere are trailing whitespaces on the line above. Please remove.\n\nAlso, Don't be original in the form alone---all other tests in this\nfile indent with a leading HT, not four SPs. Please match the style\nof surrounding code.\n\n> + test_unconfig user.name &&\n> + echo changed >1.t &&\n> + git stash\n> +'\n> +\n> test_done\n\nThanks. Please do not reroll the next round at too rapid a pace.\n\n"},{"id":"361986","messageId":"0e361938-29d7-6013-9632-a14d0cb02824@gmail.com","threadId":"49651","inReplyTo":"xmqqa7n1fr2c.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/3] [Outreachy] t3903-stash: test without configured user name","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-10-30T13:04:12Z","receivedAt":"2018-10-30T13:04:21Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"\nOn 26-Oct-18 3:13 AM, Junio C Hamano wrote:\n> Slavica Djukic writes:\n>\n>> From: Slavica \n> Please make sure this matches your sign-off below.\n>\n>> This is part of enhancement request that ask for 'git stash' to work\n>> even if 'user.name' and 'user.email' are not configured.\n>> Due to an implementation detail, git-stash undesirably requires\n>> 'user.name' and 'user.email' to be set, but shouldn't.\n>> The issue is discussed here:\n>> https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.\n> As the four lines above summarize the issue being highlighted by the\n> expect-failure rather well, the last two lines are unnecessary.\n> Please remove them. Alternatively, you can place them after the\n> three-dash lines we see below.\n>\n>> Signed-off-by: Slavica Djukic \n>> ---\n>> t/t3903-stash.sh | 14 ++++++++++++++\n>> 1 file changed, 14 insertions(+)\n>>\n>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n>> index 9e06494ba0..ae2c905343 100755\n>> --- a/t/t3903-stash.sh\n>> +++ b/t/t3903-stash.sh\n>> @@ -1156,4 +1156,18 @@ test_expect_success 'stash -- works with binary files' '\n>> \ttest_path_is_file subdir/untracked\n>> '\n>> \n>> +test_expect_failure 'stash works when user.name and user.email are not set' '\n>> + test_commit 1 &&\n> Just being curious, but do we need a fresh commit created at this\n> point in the test? Many tests before this one begin with \"git reset\"\n> and then run \"git stash\" without ever creating commit themselves,\n> instead relying on the fact that there already is at least one\n> commit created in the \"setup\" phase of the test that a \"stash\"\n> created can be made relative to. I do not think this test is all\n> that special in that regard to require its own commit.\n\nNo, we don't need fresh commit here. Thank you for this and all other \nsuggestions.\n\nI've changed test according to them.\n\n>\n>> + test_config user.useconfigonly true &&\n>> + test_config stash.usebuiltin true &&\n>> + sane_unset GIT_AUTHOR_NAME &&\n>> + sane_unset GIT_AUTHOR_EMAIL &&\n>> + sane_unset GIT_COMMITTER_NAME &&\n>> + sane_unset GIT_COMMITTER_EMAIL &&\n>> + test_unconfig user.email &&\n> There are trailing whitespaces on the line above. Please remove.\n>\n> Also, Don't be original in the form alone---all other tests in this\n> file indent with a leading HT, not four SPs. Please match the style\n> of surrounding code.\n>\n>> + test_unconfig user.name &&\n>> + echo changed >1.t &&\n>> + git stash\n>> +'\n>> +\n>> test_done\n> Thanks. Please do not reroll the next round at too rapid a pace.\n\nI've taken my time for next round, I am working on 2/3 and 3/3 parts as \nwell.\nI wouldn't have sent this patch if I understood you well in previous reply.\n\nThank you,\n\nSlavica.\n\n>\n>\n>\n"},{"id":"362165","messageId":"20181101115546.13516-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"9ea38cd8e98890b8264696dfd647c1f9e709ae9e.1540494231.git.slawica92@hotmail.com","subject":"[PATCH 0/3] [Outreachy] make stash work if user.name and user.email are not configured","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-01T11:55:46Z","receivedAt":"2018-11-01T11:56:38Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Enhancement request that ask for 'git stash' to work even if \n'user.name' and 'user.email' are not configured.\nDue to an implementation detail, git-stash undesirably requires \n'user.name' and 'user.email' to be set, but shouldn't.\n\nSlavica Djukic(3):\n [Outreachy] t3903-stash: test without configured user.name and\n user.email\n [Outreachy] ident: introduce set_fallback_ident() function\n [Outreachy] stash: use set_fallback_ident() function\n\n builtin/stash.c | 1 +\n cache.h | 1 +\n ident.c | 17 +++++++++++++++++\n t/t3903-stash.sh | 15 +++++++++++++++\n 4 files changed, 34 insertions(+)\n\n-- \n2.19.1.windows.1\n\n"},{"id":"362166","messageId":"20181101115834.19044-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181101115546.13516-1-slawica92@hotmail.com","subject":"[PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-01T11:58:34Z","receivedAt":"2018-11-01T11:59:06Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Add test to assert that stash fails if user.name and user.email\nare not configured.\nIn the final commit, test will be updated to expect success.\n\nSigned-off-by: Slavica Djukic \n---\n t/t3903-stash.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 9e06494ba0..aaff36978e 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1156,4 +1156,19 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n+test_expect_failure 'stash works when user.name and user.email are not set' '\n+\tgit reset &&\n+\t>1 &&\n+\tgit add 1 &&\n+\ttest_config user.useconfigonly true &&\n+\ttest_config stash.usebuiltin true &&\n+\tsane_unset GIT_AUTHOR_NAME &&\n+\tsane_unset GIT_AUTHOR_EMAIL &&\n+\tsane_unset GIT_COMMITTER_NAME &&\n+\tsane_unset GIT_COMMITTER_EMAIL &&\n+\ttest_unconfig user.email &&\n+\ttest_unconfig user.name &&\n+\tgit stash\n+'\n+\n test_done\n-- \n2.19.1.windows.1\n\n"},{"id":"362167","messageId":"20181101120029.13992-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181101115546.13516-1-slawica92@hotmail.com","subject":"[PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-01T12:00:29Z","receivedAt":"2018-11-01T12:00:54Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Usually, when creating a commit, ident is needed to record the author\nand commiter.\nBut, when there is commit not intended to published, e.g. when stashing\nchanges, valid ident is not necessary.\nTo allow creating commits in such scenario, let's introduce helper\nfunction \"set_fallback_ident(), which will pre-load the ident.\n\nIn following commit, set_fallback_ident() function will be called in stash.\n\nSigned-off-by: Slavica Djukic \n---\n cache.h | 1 +\n ident.c | 17 +++++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 681307f716..6b5b559a05 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1470,6 +1470,7 @@ extern const char *git_sequence_editor(void);\n extern const char *git_pager(int stdout_is_tty);\n extern int is_terminal_dumb(void);\n extern int git_ident_config(const char *, const char *, void *);\n+void set_fallback_ident(const char *name, const char *email);\n extern void reset_ident_date(void);\n \n struct ident_split {\ndiff --git a/ident.c b/ident.c\nindex 33bcf40644..410bd495e9 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -505,6 +505,23 @@ int git_ident_config(const char *var, const char *value, void *data)\n \treturn 0;\n }\n \n+void set_fallback_ident(const char *name, const char *email)\n+{\n+\tif (!git_default_name.len) {\n+\t\tstrbuf_addstr(&git_default_name, name);\n+\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n+\t\tident_config_given |= IDENT_NAME_GIVEN;\n+\t}\n+\n+\tif (!git_default_email.len) {\n+\t\tstrbuf_addstr(&git_default_email, email);\n+\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n+\t\tident_config_given |= IDENT_MAIL_GIVEN;\n+\t}\n+}\n+\n static int buf_cmp(const char *a_begin, const char *a_end,\n \t\t const char *b_begin, const char *b_end)\n {\n-- \n2.19.1.windows.1\n\n"},{"id":"362168","messageId":"20181101120239.15636-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181101115546.13516-1-slawica92@hotmail.com","subject":"[PATCH 3/3] [Outreachy] stash: use set_fallback_ident() function","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-01T12:02:39Z","receivedAt":"2018-11-01T12:03:05Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Call set_fallback_ident() in cmd_stash() and update test\nfrom the first commit to expect success.\n\nExecuting stash without user.name and user.email configured\ncan be useful when bots or similar users use stash, without anyone\nspecifing valid ident. Use case would be automated testing.\nThere are also users who find this convinient.\nFor example, in this thread:\nhttps://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#ma4fb50903a54cbcdecd4ef05856bf8094bc3c323\nuser points out that he would find it useful if stash had --author option.\n\nSigned-off-by: Slavica Djukic \n---\n builtin/stash.c | 1 +\n t/t3903-stash.sh | 2 +-\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 965e938ebd..add30aae64 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1523,6 +1523,7 @@ int cmd_stash(int argc, const char **argv, const char *prefix)\n \ttrace_repo_setup(prefix);\n \tsetup_work_tree();\n \n+\tset_fallback_ident(\"git stash\", \"stash@git.commands\");\n \tgit_config(git_default_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, options, git_stash_usage,\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex aaff36978e..06a2ffb398 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1156,7 +1156,7 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n-test_expect_failure 'stash works when user.name and user.email are not set' '\n+test_expect_success 'stash works when user.name and user.email are not set' '\n \tgit reset &&\n \t>1 &&\n \tgit add 1 &&\n-- \n2.19.1.windows.1\n\n"},{"id":"362192","messageId":"CAP8UFD2qUotva_f+GmawHB3hr3W=rBy5V36yUWj7Majf2K61fQ@mail.gmail.com","threadId":"49651","inReplyTo":"20181101115834.19044-1-slawica92@hotmail.com","subject":"Re: [PATCH 1/3][Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-11-01T14:53:09Z","receivedAt":"2018-11-01T14:53:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Nov 1, 2018 at 2:31 PM Slavica Djukic\n wrote:\n>\n> Add test to assert that stash fails if user.name and user.email\n\nNit: I am not sure that \"assert\" is the right word here.\ntest_expect_failure() is more for documenting an existing bug than for\nreally asserting a behavior (that users could rely upon). So I would\nreplace \"assert\" with \"document\" or maybe \"document the bug\".\n\n> are not configured.\n> In the final commit, test will be updated to expect success.\n\nOther nit: maybe use \"In a later commit\" instead of \"In the final\ncommit\" as you, or someone else, may add another commit in this patch\nseries after the current final one.\n\n> Signed-off-by: Slavica Djukic \n\nThanks!\n"},{"id":"362229","messageId":"xmqqwopwqj2g.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"20181101120029.13992-1-slawica92@hotmail.com","subject":"Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-02T03:01:11Z","receivedAt":"2018-11-02T03:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica Djukic writes:\n\n> Usually, when creating a commit, ident is needed to record the author\n> and commiter.\n> But, when there is commit not intended to published, e.g. when stashing\n> changes, valid ident is not necessary.\n> To allow creating commits in such scenario, let's introduce helper\n> function \"set_fallback_ident(), which will pre-load the ident.\n>\n> In following commit, set_fallback_ident() function will be called in stash.\n>\n> Signed-off-by: Slavica Djukic \n> ---\n\nThis is quite the other way around from what I expected to see.\n\nI would have expected that a patch would introduce a new flag to\ntell git_author/committer_info() functions that it is OK not to have\nname or email given, and pass that flag down the callchain.\n\nAnybody who knows that git_author/committer_info() will eventually\nget called can instead tell these helpers not to fail (and yield a\nsubstitute non-answer) beforehand with this function, instead of\npassing a flag down to affect _only_ one callflow without affecting\nothers, using this new function.\n\nI am not yet saying that being opposite from my intuition is\nnecessarily wrong, but the approach is like setting a global\nvariable that affects everybody and it will probably make it\nharder to later libify the functions involved. It certainly\nmakes this patch (and the next step) much simpler than passing\na flag IDENT_NO_NAME_OK|IDENT_NO_MAIL_OK thru the codepath.\n\n> +void set_fallback_ident(const char *name, const char *email)\n> +{\n> +\tif (!git_default_name.len) {\n> +\t\tstrbuf_addstr(&git_default_name, name);\n> +\t\tcommitter_ident_explicitly_given |= IDENT_NAME_GIVEN;\n> +\t\tauthor_ident_explicitly_given |= IDENT_NAME_GIVEN;\n> +\t\tident_config_given |= IDENT_NAME_GIVEN;\n> +\t}\n> +\n> +\tif (!git_default_email.len) {\n> +\t\tstrbuf_addstr(&git_default_email, email);\n> +\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> +\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> +\t\tident_config_given |= IDENT_MAIL_GIVEN;\n> +\t}\n> +}\n\nOne terrible thing about this approach is that the caller cannot\ntell if it used fallback name or a real name, because it lies in\nthese \"explicitly_given\" fields. The immediate caller (i.e. the one\nthat creates commit objects used to represent a stash entry) may not\ncare, but a helper function pretending to be reusable incredient to\nsolve a more general issue, this is far less than ideal.\n\nSo in short, I do agree that the series tackles an issue worth\naddressing, but I am not impressed with the approach at all.\n\nRather than adding this fallback trap, can't we do it more like\nthis?\n\n - At the beginning of \"git stash\", after parsing the command\n line, we know what subcommand of \"git stash\" we are going to\n run.\n\n - If it is a subcommand that could need the ident (i.e. the ones\n that create a stash entry), we check the ident (e.g. make a\n call to git_author/committer_info() ourselves) but without\n STRICT bit, so that we can probe without dying if we need to\n supply a fallback identy.\n\n - And if we do need it, then setenv() the necessary\n environment variables and arrange the next call by anybody\n to git_author/committer_info() will get the fallback values\n from there.\n"},{"id":"362232","messageId":"xmqqh8h0qefq.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"xmqqwopwqj2g.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-02T04:41:13Z","receivedAt":"2018-11-02T04:41:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Rather than adding this fallback trap, can't we do it more like\n> this?\n>\n> - At the beginning of \"git stash\", after parsing the command\n> line, we know what subcommand of \"git stash\" we are going to\n> run.\n>\n> - If it is a subcommand that could need the ident (i.e. the ones\n> that create a stash entry), we check the ident (e.g. make a\n> call to git_author/committer_info() ourselves) but without\n> STRICT bit, so that we can probe without dying if we need to\n> supply a fallback identy.\n>\n> - And if we do need it, then setenv() the necessary\n> environment variables and arrange the next call by anybody\n> to git_author/committer_info() will get the fallback values\n> from there.\n\nAs we currently have no idea when builtin/stash.c becomes ready for\n'next', how about doing something like this instead, in order to\nhelp end-users without waiting in the meantime? The fix can be\npicked up and ported when the C rewrite is updated, of course.\n\n git-stash.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 94793c1a91..789ce2f41d 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -55,6 +55,20 @@ untracked_files () {\n \tgit ls-files -o $z $excl_opt -- \"$@\"\n }\n \n+prepare_fallback_ident () {\n+\tif ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null 2>&1\n+\tthen\n+\t\tGIT_AUTHOR_NAME=\"git stash\"\n+\t\tGIT_AUTHOR_EMAIL=git@stash\n+\t\tGIT_COMMITTER_NAME=\"git stash\"\n+\t\tGIT_COMMITTER_EMAIL=git@stash\n+\t\texport GIT_AUTHOR_NAME\n+\t\texport GIT_AUTHOR_EMAIL\n+\t\texport GIT_COMMITTER_NAME\n+\t\texport GIT_COMMITTER_EMAIL\n+\tfi\n+}\n+\n clear_stash () {\n \tif test $# != 0\n \tthen\n@@ -67,6 +81,9 @@ clear_stash () {\n }\n \n create_stash () {\n+\n+\tprepare_fallback_ident\n+\n \tstash_msg=\n \tuntracked=\n \twhile test $# != 0\n"},{"id":"362233","messageId":"xmqq8t2cqdl9.fsf_-_@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"20181101115834.19044-1-slawica92@hotmail.com","subject":"[PATCH 2+3/3] stash: tolerate missing user identity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-02T04:59:30Z","receivedAt":"2018-11-02T04:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"git stash\" command insists on having a usable user identity to\nthe same degree as the \"git commit-tree\" and \"git commit\" commands\ndo, because it uses the same codepath that creates commit objects\nas these commands.\n\nIt is not strictly necesary to do so. Check if we will barf before\ncreating commit objects and then supply fake identity to please the\nmachinery that creates commits.\n\nThis is not that much of usability improvement, as the users who run\n\"git stash\" would eventually want to record their changes that are\ntemporarily stored in the stashes in a more permanent history by\ncommitting, and they must do \"git config user.{name,email}\" at that\npoint anyway, so arguably this change is only delaying a step that\nis necessary to work in the repository.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This time with a proposed commit log message and a flip to the\n test; this would be able to replce 2/3 and 3/3 without waiting\n for ps/stash-in-c to stabilize and become ready to be based on\n further work like this one.\n\n We need to extend the test so that when a reasonable identity is\n present, the stashes are created under that identity and not with\n the fallback one, which I do not think is tested with the previous\n step, so there still is a bit of room to improve [PATCH 1/3]\n\n git-stash.sh | 17 +++++++++++++++++\n t/t3903-stash.sh | 2 +-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 94793c1a91..789ce2f41d 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -55,6 +55,20 @@ untracked_files () {\n \tgit ls-files -o $z $excl_opt -- \"$@\"\n }\n \n+prepare_fallback_ident () {\n+\tif ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null 2>&1\n+\tthen\n+\t\tGIT_AUTHOR_NAME=\"git stash\"\n+\t\tGIT_AUTHOR_EMAIL=git@stash\n+\t\tGIT_COMMITTER_NAME=\"git stash\"\n+\t\tGIT_COMMITTER_EMAIL=git@stash\n+\t\texport GIT_AUTHOR_NAME\n+\t\texport GIT_AUTHOR_EMAIL\n+\t\texport GIT_COMMITTER_NAME\n+\t\texport GIT_COMMITTER_EMAIL\n+\tfi\n+}\n+\n clear_stash () {\n \tif test $# != 0\n \tthen\n@@ -67,6 +81,9 @@ clear_stash () {\n }\n \n create_stash () {\n+\n+\tprepare_fallback_ident\n+\n \tstash_msg=\n \tuntracked=\n \twhile test $# != 0\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex a9a573efa0..3dcf2f14d1 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1096,7 +1096,7 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n-test_expect_failure 'stash works when user.name and user.email are not set' '\n+test_expect_success 'stash works when user.name and user.email are not set' '\n \tgit reset &&\n \t>1 &&\n \tgit add 1 &&\n-- \n2.19.1-801-gd582ea202b\n\n"},{"id":"362236","messageId":"xmqqtvl0oxy2.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"xmqqh8h0qefq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] [Outreachy] ident: introduce set_fallback_ident() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-02T05:22:45Z","receivedAt":"2018-11-02T05:22:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> As we currently have no idea when builtin/stash.c becomes ready for\n> 'next', how about doing something like this instead, in order to\n> help end-users without waiting in the meantime? The fix can be\n> picked up and ported when the C rewrite is updated, of course.\n\nI think a better approach to fix the current C version, assuming\nthat a reroll won't change the structure of the code too much and\nkeeps using commit_tree() to synthesize the stash entries, would be\nto teach commit_tree() -> commit_tree_extended() codepath to take\ncommiter identity just like it takes author identity. Then inside\nbuiltin/stash.c, we can choose what committer and author identity to\npass without having commit_tree_extended() ask for identity with the\nSTRICT option. Right now, commit_tree() does not have a good way,\nother than somehow lying to git_author/committer_info(), to record a\ncommit created under an arbitrary identity, which would be fixed\nwith such an approach and will help callers with similar needs in\nthe future.\n\n\n\n\n"},{"id":"363399","messageId":"20181114221218.3112-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181101115546.13516-1-slawica92@hotmail.com","subject":"[PATCH v2 0/2] [Outreachy] make stash work if user.name and user.email are not configured","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-14T22:12:18Z","receivedAt":"2018-11-14T22:21:50Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Changes since v1:\n\t\n\t*extend test to check whether git stash executes under valid ident\n\t(and not under fallback one) when there is such present\n\t*add prepare_fallback_ident() function to git-stash.sh to \n\tprovide fallback identity\n\nSlavica Djukic (2):\n [Outreachy] t3903-stash: test without configured user.name and\n user.email\n [Outreachy] stash: tolerate missing user identity\n\n git-stash.sh | 17 +++++++++++++++++\n t/t3903-stash.sh | 23 +++++++++++++++++++++++\n 2 files changed, 40 insertions(+)\n\n-- \n2.19.1.1052.gd166e6afe\n\n"},{"id":"363400","messageId":"20181114222524.2624-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181114221218.3112-1-slawica92@hotmail.com","subject":"[PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-14T22:25:24Z","receivedAt":"2018-11-14T22:26:19Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Add test to document that stash fails if user.name and user.email\nare not configured.\nIn the later commit, test will be updated to expect success.\n\nSigned-off-by: Slavica Djukic \n---\n t/t3903-stash.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex cd216655b..bab8bec67 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1096,4 +1096,27 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n+test_expect_failure 'stash works when user.name and user.email are not set' '\n+\tgit reset &&\n+\tgit var GIT_COMMITTER_IDENT >expected &&\n+\t>1 &&\n+\tgit add 1 &&\n+\tgit stash &&\n+\tgit var GIT_COMMITTER_IDENT >actual &&\n+\ttest_cmp expected actual &&\n+\t>2 &&\n+\tgit add 2 &&\n+\ttest_config user.useconfigonly true &&\n+\ttest_config stash.usebuiltin true &&\n+\t(\n+\t\tsane_unset GIT_AUTHOR_NAME &&\n+\t\tsane_unset GIT_AUTHOR_EMAIL &&\n+\t\tsane_unset GIT_COMMITTER_NAME &&\n+\t\tsane_unset GIT_COMMITTER_EMAIL &&\n+\t\ttest_unconfig user.email &&\n+\t\ttest_unconfig user.name &&\n+\t\tgit stash\n+\t)\n+'\n+\n test_done\n-- \n2.19.1.1052.gd166e6afe\n\n"},{"id":"363401","messageId":"20181114222802.10928-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181114221218.3112-1-slawica92@hotmail.com","subject":"[PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-14T22:28:02Z","receivedAt":"2018-11-14T22:28:35Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"The \"git stash\" command insists on having a usable user identity to\nthe same degree as the \"git commit-tree\" and \"git commit\" commands\ndo, because it uses the same codepath that creates commit objects\nas these commands.\n\nIt is not strictly necesary to do so. Check if we will barf before\ncreating commit objects and then supply fake identity to please the\nmachinery that creates commits.\n\nThis is not that much of usability improvement, as the users who run\n\"git stash\" would eventually want to record their changes that are\ntemporarily stored in the stashes in a more permanent history by\ncommitting, and they must do \"git config user.{name,email}\" at that\npoint anyway, so arguably this change is only delaying a step that\nis necessary to work in the repository.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Slavica Djukic \n---\n git-stash.sh | 17 +++++++++++++++++\n t/t3903-stash.sh | 2 +-\n 2 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 94793c1a9..789ce2f41 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -55,6 +55,20 @@ untracked_files () {\n \tgit ls-files -o $z $excl_opt -- \"$@\"\n }\n \n+prepare_fallback_ident () {\n+\tif ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null 2>&1\n+\tthen\n+\t\tGIT_AUTHOR_NAME=\"git stash\"\n+\t\tGIT_AUTHOR_EMAIL=git@stash\n+\t\tGIT_COMMITTER_NAME=\"git stash\"\n+\t\tGIT_COMMITTER_EMAIL=git@stash\n+\t\texport GIT_AUTHOR_NAME\n+\t\texport GIT_AUTHOR_EMAIL\n+\t\texport GIT_COMMITTER_NAME\n+\t\texport GIT_COMMITTER_EMAIL\n+\tfi\n+}\n+\n clear_stash () {\n \tif test $# != 0\n \tthen\n@@ -67,6 +81,9 @@ clear_stash () {\n }\n \n create_stash () {\n+\n+\tprepare_fallback_ident\n+\n \tstash_msg=\n \tuntracked=\n \twhile test $# != 0\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex bab8bec67..0b0814421 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1096,7 +1096,7 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n-test_expect_failure 'stash works when user.name and user.email are not set' '\n+test_expect_success 'stash works when user.name and user.email are not set' '\n \tgit reset &&\n \tgit var GIT_COMMITTER_IDENT >expected &&\n \t>1 &&\n-- \n2.19.1.1052.gd166e6afe\n\n"},{"id":"363429","messageId":"nycvar.QRO.7.76.6.1811151336330.41@tvgsbejvaqbjf.bet","threadId":"49651","inReplyTo":"20181114222524.2624-1-slawica92@hotmail.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-15T12:37:06Z","receivedAt":"2018-11-15T12:37:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Slavica,\n\nthis looks very good to me. Just one grammar thing:\n\nOn Wed, 14 Nov 2018, Slavica Djukic wrote:\n\n> Add test to document that stash fails if user.name and user.email\n> are not configured.\n> In the later commit, test will be updated to expect success.\n\nIn a later commit [...]\n\nOtherwise, I would be totally fine with this version being merged.\n\nCiao,\nJohannes\n\n> \n> Signed-off-by: Slavica Djukic \n> ---\n> t/t3903-stash.sh | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n> \n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index cd216655b..bab8bec67 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1096,4 +1096,27 @@ test_expect_success 'stash -- works with binary files' '\n> \ttest_path_is_file subdir/untracked\n> '\n> \n> +test_expect_failure 'stash works when user.name and user.email are not set' '\n> +\tgit reset &&\n> +\tgit var GIT_COMMITTER_IDENT >expected &&\n> +\t>1 &&\n> +\tgit add 1 &&\n> +\tgit stash &&\n> +\tgit var GIT_COMMITTER_IDENT >actual &&\n> +\ttest_cmp expected actual &&\n> +\t>2 &&\n> +\tgit add 2 &&\n> +\ttest_config user.useconfigonly true &&\n> +\ttest_config stash.usebuiltin true &&\n> +\t(\n> +\t\tsane_unset GIT_AUTHOR_NAME &&\n> +\t\tsane_unset GIT_AUTHOR_EMAIL &&\n> +\t\tsane_unset GIT_COMMITTER_NAME &&\n> +\t\tsane_unset GIT_COMMITTER_EMAIL &&\n> +\t\ttest_unconfig user.email &&\n> +\t\ttest_unconfig user.name &&\n> +\t\tgit stash\n> +\t)\n> +'\n> +\n> test_done\n> -- \n> 2.19.1.1052.gd166e6afe\n> \n> \n"},{"id":"363466","messageId":"xmqqwopdk2jp.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"20181114222802.10928-1-slawica92@hotmail.com","subject":"Re: [PATCH v2 2/2] [Outreachy] stash: tolerate missing user identity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-16T05:35:22Z","receivedAt":"2018-11-16T05:35:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica Djukic writes:\n\n> git-stash.sh | 17 +++++++++++++++++\n> t/t3903-stash.sh | 2 +-\n> 2 files changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-stash.sh b/git-stash.sh\n> index 94793c1a9..789ce2f41 100755\n> --- a/git-stash.sh\n> +++ b/git-stash.sh\n> @@ -55,6 +55,20 @@ untracked_files () {\n> \tgit ls-files -o $z $excl_opt -- \"$@\"\n> }\n> \n> +prepare_fallback_ident () {\n> +\tif ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null 2>&1\n> +\tthen\n> +\t\tGIT_AUTHOR_NAME=\"git stash\"\n> +\t\tGIT_AUTHOR_EMAIL=git@stash\n> +\t\tGIT_COMMITTER_NAME=\"git stash\"\n> +\t\tGIT_COMMITTER_EMAIL=git@stash\n> +\t\texport GIT_AUTHOR_NAME\n> +\t\texport GIT_AUTHOR_EMAIL\n> +\t\texport GIT_COMMITTER_NAME\n> +\t\texport GIT_COMMITTER_EMAIL\n> +\tfi\n> +}\n> +\n> clear_stash () {\n> \tif test $# != 0\n> \tthen\n> @@ -67,6 +81,9 @@ clear_stash () {\n> }\n> \n> create_stash () {\n> +\n> +\tprepare_fallback_ident\n> +\n> \tstash_msg=\n> \tuntracked=\n> \twhile test $# != 0\n\nThat looks like a sensible implementation to me.\n\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index bab8bec67..0b0814421 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1096,7 +1096,7 @@ test_expect_success 'stash -- works with binary files' '\n> \ttest_path_is_file subdir/untracked\n> '\n> \n> -test_expect_failure 'stash works when user.name and user.email are not set' '\n> +test_expect_success 'stash works when user.name and user.email are not set' '\n\nThis line claims to the readers of patch that the known breakage\nthis known test piece demonstrated has been corrected, but they need\nto refresh their memory by going back to the previous patch to see\nif this \"failure-to-success\" flipping is done to the right test\npiece, and what exactly the test piece tested to see the existing\nbreakage, because all the interesting part of the test are chomped\noutside the post-context of this hunk.\n\nUnless the fix is fairly complex, adding ought-to-succeed tests that\nexpect success that break when the code change gets omitted from the\npatch in the same patch as the fix itself (i.e. squash patch 1/2 and\npatch 2/2 into a single patch) would be more helpful for the readers\n(it also helps cherry-picking the fix later to earlier maintenance\ntracks if it becomes necessary).\n\n> \tgit reset &&\n> \tgit var GIT_COMMITTER_IDENT >expected &&\n> \t>1 &&\n"},{"id":"363467","messageId":"xmqqsh01k1mr.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"20181114222524.2624-1-slawica92@hotmail.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-16T05:55:08Z","receivedAt":"2018-11-16T05:55:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica Djukic writes:\n\n> +test_expect_failure 'stash works when user.name and user.email are not set' '\n> +\tgit reset &&\n> +\tgit var GIT_COMMITTER_IDENT >expected &&\n\nAll the other existing test pieces in this file calls the expected\nresult \"expect\"; is there a reason why this patch needs to be\ndifferent (e.g. 'expect' file left by the earlier step needs to be\nkept unmodified for later use, or something like that)? If not,\nplease avoid making a difference in irrelevant details, as that\nwould waste time of readers by forcing them to guess if there is\nsuch a reason that readers cannot immediately see.\n\nAnyway, we grab the committer ident we use by default during the\ntest with this command. OK.\n\n> +\t>1 &&\n> +\tgit add 1 &&\n> +\tgit stash &&\n\nAnd we make sure we can create stash.\n\n> +\tgit var GIT_COMMITTER_IDENT >actual &&\n> +\ttest_cmp expected actual &&\n\nI am not sure what you are testing with this step. There is nothing\nthat changed environment variables or configuration since we ran\n\"git var\" above. Why does this test suspect that somebody in the\nfuture may break the expectation that after running 'git add' and/or\n'git stash', our committer identity may have been changed, and how\nwould such a breakage happen?\n\n> +\t>2 &&\n> +\tgit add 2 &&\n> +\ttest_config user.useconfigonly true &&\n> +\ttest_config stash.usebuiltin true &&\n\nNow we start using use-config-only, so unsetting environment\nvariables will cause trouble when Git insists on having an\nexplicitly configured identities. Makes sense.\n\n> +\t(\n> +\t\tsane_unset GIT_AUTHOR_NAME &&\n> +\t\tsane_unset GIT_AUTHOR_EMAIL &&\n> +\t\tsane_unset GIT_COMMITTER_NAME &&\n> +\t\tsane_unset GIT_COMMITTER_EMAIL &&\n> +\t\ttest_unconfig user.email &&\n> +\t\ttest_unconfig user.name &&\n\nAnd then we try the same test, but without environment or config.\nSince we are unsetting the environment, in order to be nice for\nfuture test writers, we do this in a subshell, so that we do not\nhave to restore the original values of environment variables.\n\nDon't we need to be nice the same way for configuration variables,\nthough? We _know_ that nobody sets user.{email,name} config up to\nthis point in the test sequence, so that is why we do not do a \"save\nbefore test and then restore to the original\" dance on them. Even\nthough we are relying on the fact that these two variables are left\nunset in the configuration file, we unconfig them here anyway, and I\ndo think it is a good idea for documentation purposes (i.e. we are\nnot documenting what we assume the config before running this test\nwould be; we are documenting what state we want these two variables\nare in when running this \"git stash\"---that is, they are both unset).\n\nSo these later part of this test piece makes sense. I still do not\nknow what you wanted to check in the earlier part of the test,\nthough.\n\n> +\t\tgit stash\n> +\t)\n> +'\n> +\n> test_done\n"},{"id":"363468","messageId":"xmqqa7m9k07b.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"xmqqsh01k1mr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-16T06:26:00Z","receivedAt":"2018-11-16T06:26:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Now we start using use-config-only, so unsetting environment\n> variables will cause trouble when Git insists on having an\n> explicitly configured identities. Makes sense.\n>\n>> +\t(\n>> +\t\tsane_unset GIT_AUTHOR_NAME &&\n>> +\t\tsane_unset GIT_AUTHOR_EMAIL &&\n>> +\t\tsane_unset GIT_COMMITTER_NAME &&\n>> +\t\tsane_unset GIT_COMMITTER_EMAIL &&\n>> +\t\ttest_unconfig user.email &&\n>> +\t\ttest_unconfig user.name &&\n>\n> And then we try the same test, but without environment or config.\n\nI suspect that it makes sense to replace the \"git stash\" we see\nbelow with something like this:\n\n\ttest_must_fail git commit -m should fail &&\n\techo \"git stash \" >expect &&\n\techo >2 &&\n\tgit stash &&\n\tgit show -s --format=\"%(authorname) <%(authoremail)>\" refs/stash >actual &&\n\ttest_cmp expect actual\n\nThat is\n\n - we make sure \"commit\" would not go through, to make sure our\n preparation to unset environment variables was sufficient;\n\n - we make sure \"stash\" does succeed (which is the primary thing you\n are interested in);\n\n - we make sure the resulting \"stash\" is not created under our\n default identity but under our fallback one.\n\n>> +\t\tgit stash\n>> +\t)\n>> +'\n>> +\n>> test_done\n"},{"id":"363469","messageId":"xmqq5zwxjzx6.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"xmqqsh01k1mr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-16T06:32:05Z","receivedAt":"2018-11-16T06:32:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Slavica Djukic writes:\n>\n>> +test_expect_failure 'stash works when user.name and user.email are not set' '\n>> +\tgit reset &&\n>> +\tgit var GIT_COMMITTER_IDENT >expected &&\n> ...\n> Anyway, we grab the committer ident we use by default during the\n> test with this command. OK.\n>\n>> +\t>1 &&\n>> +\tgit add 1 &&\n>> +\tgit stash &&\n>\n> And we make sure we can create stash.\n>\n>> +\tgit var GIT_COMMITTER_IDENT >actual &&\n>> +\ttest_cmp expected actual &&\n>\n> I am not sure what you are testing with this step. There is nothing\n> that changed environment variables or configuration since we ran\n> \"git var\" above. Why does this test suspect that somebody in the\n> future may break the expectation that after running 'git add' and/or\n> 'git stash', our committer identity may have been changed, and how\n> would such a breakage happen?\n\nJust a note.\n\n\"git var GIT_COMMITTER_IDENT\" has timestamp in it, so a naïve reader\nmight wonder what would happen if \"git add 1\" and \"git stash\" took\nmore than one second. But it won't be a problem in this case as the\ncommitter date comes from the environment GIT_COMMITTER_DATE, which\nis set to a fixed known value and is incremented only by calling\ntest_commit helper function, which does not happen between the two\n\"git var\" calls.\n\nIn any case, I am not sure I understand the point of comparing two\noutput from \"git var\" invocations we see ablve in this test.\n"},{"id":"363482","messageId":"2f3612b8-5f26-adae-9a7f-05d16040938e@gmail.com","threadId":"49651","inReplyTo":"xmqqsh01k1mr.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-16T08:28:07Z","receivedAt":"2018-11-16T08:28:15Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Hi Junio,\n\nOn 16-Nov-18 6:55 AM, Junio C Hamano wrote:\n> Slavica Djukic writes:\n>\n>> +test_expect_failure 'stash works when user.name and user.email are not set' '\n>> +\tgit reset &&\n>> +\tgit var GIT_COMMITTER_IDENT >expected &&\n> All the other existing test pieces in this file calls the expected\n> result \"expect\"; is there a reason why this patch needs to be\n> different (e.g. 'expect' file left by the earlier step needs to be\n> kept unmodified for later use, or something like that)? If not,\n> please avoid making a difference in irrelevant details, as that\n> would waste time of readers by forcing them to guess if there is\n> such a reason that readers cannot immediately see.\n\nThere is no specific reason for file to be \"expected\", I'll update that.\n\n>\n> Anyway, we grab the committer ident we use by default during the\n> test with this command. OK.\n>\n>> +\t>1 &&\n>> +\tgit add 1 &&\n>> +\tgit stash &&\n> And we make sure we can create stash.\n>\n>> +\tgit var GIT_COMMITTER_IDENT >actual &&\n>> +\ttest_cmp expected actual &&\n> I am not sure what you are testing with this step. There is nothing\n> that changed environment variables or configuration since we ran\n> \"git var\" above. Why does this test suspect that somebody in the\n> future may break the expectation that after running 'git add' and/or\n> 'git stash', our committer identity may have been changed, and how\n> would such a breakage happen?\nIn previous re-roll, you suggested that test should be improved so that \nwhen\nreasonable identity is present, git stash executes under that identity, \nand not\nunder the fallback one. Here I'm just making sure that after calling git \nstash,\nwe still have same reasonable identity present.\n>\n>> +\t>2 &&\n>> +\tgit add 2 &&\n>> +\ttest_config user.useconfigonly true &&\n>> +\ttest_config stash.usebuiltin true &&\n> Now we start using use-config-only, so unsetting environment\n> variables will cause trouble when Git insists on having an\n> explicitly configured identities. Makes sense.\n>\n>> +\t(\n>> +\t\tsane_unset GIT_AUTHOR_NAME &&\n>> +\t\tsane_unset GIT_AUTHOR_EMAIL &&\n>> +\t\tsane_unset GIT_COMMITTER_NAME &&\n>> +\t\tsane_unset GIT_COMMITTER_EMAIL &&\n>> +\t\ttest_unconfig user.email &&\n>> +\t\ttest_unconfig user.name &&\n> And then we try the same test, but without environment or config.\n> Since we are unsetting the environment, in order to be nice for\n> future test writers, we do this in a subshell, so that we do not\n> have to restore the original values of environment variables.\n>\n> Don't we need to be nice the same way for configuration variables,\n> though? We _know_ that nobody sets user.{email,name} config up to\n> this point in the test sequence, so that is why we do not do a \"save\n> before test and then restore to the original\" dance on them. Even\n> though we are relying on the fact that these two variables are left\n> unset in the configuration file, we unconfig them here anyway, and I\n> do think it is a good idea for documentation purposes (i.e. we are\n> not documenting what we assume the config before running this test\n> would be; we are documenting what state we want these two variables\n> are in when running this \"git stash\"---that is, they are both unset).\n>\n> So these later part of this test piece makes sense. I still do not\n> know what you wanted to check in the earlier part of the test,\n> though.\n>\n>> +\t\tgit stash\n>> +\t)\n>> +'\n>> +\n>> test_done\n>\nThank you,\nSlavica\n"},{"id":"363494","messageId":"xmqqsh01ib51.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"2f3612b8-5f26-adae-9a7f-05d16040938e@gmail.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-16T10:12:42Z","receivedAt":"2018-11-16T10:12:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica Djukic writes:\n\n>>> +\tgit var GIT_COMMITTER_IDENT >actual &&\n>>> +\ttest_cmp expected actual &&\n>> I am not sure what you are testing with this step. There is nothing\n>> that changed environment variables or configuration since we ran\n>> \"git var\" above. Why does this test suspect that somebody in the\n>> future may break the expectation that after running 'git add' and/or\n>> 'git stash', our committer identity may have been changed, and how\n>> would such a breakage happen?\n> In previous re-roll, you suggested that test should be improved so\n> that when\n> reasonable identity is present, git stash executes under that\n> identity, and not\n> under the fallback one. \n\nYes, but for that you'd need to be checking the resulting commit\nobject that represents the stash entry. It should be created under\nthe substitute identity.\n\n> Here I'm just making sure that after calling\n> git stash,\n> we still have same reasonable identity present.\n\nI do not think such a test would detect it, even when \"git stash\"\nincorrectly used the fallback identity to create the stash entry.\n"},{"id":"363559","messageId":"a75dcea8-797d-9c9b-3453-2de2a4d983dd@gmail.com","threadId":"49651","inReplyTo":"xmqqsh01ib51.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-17T18:47:53Z","receivedAt":"2018-11-17T18:48:01Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Hi Junio,\n\nOn 16-Nov-18 11:12 AM, Junio C Hamano wrote:\n> Slavica Djukic writes:\n>\n>>>> +\tgit var GIT_COMMITTER_IDENT >actual &&\n>>>> +\ttest_cmp expected actual &&\n>>> I am not sure what you are testing with this step. There is nothing\n>>> that changed environment variables or configuration since we ran\n>>> \"git var\" above. Why does this test suspect that somebody in the\n>>> future may break the expectation that after running 'git add' and/or\n>>> 'git stash', our committer identity may have been changed, and how\n>>> would such a breakage happen?\n>> In previous re-roll, you suggested that test should be improved so\n>> that when\n>> reasonable identity is present, git stash executes under that\n>> identity, and not\n>> under the fallback one.\n> Yes, but for that you'd need to be checking the resulting commit\n> object that represents the stash entry. It should be created under\n> the substitute identity.\nWould it be correct to check it like this:\n\n         git reset &&\n         >1 &&\n         git add 1 &&\n         echo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>\" >expect &&\n         git stash &&\n         git show -s --format=\"%an <%ae>\" refs/stash >actual &&\n         test_cmp expect actual\n\nIt is similar to your suggestion when there is no\nident present.\n>> Here I'm just making sure that after calling\n>> git stash,\n>> we still have same reasonable identity present.\n> I do not think such a test would detect it, even when \"git stash\"\n> incorrectly used the fallback identity to create the stash entry.\n>\n>\nThank you,\nSlavica\n"},{"id":"363565","messageId":"xmqq36ryhpbh.fsf@gitster-ct.c.googlers.com","threadId":"49651","inReplyTo":"a75dcea8-797d-9c9b-3453-2de2a4d983dd@gmail.com","subject":"Re: [PATCH v2 1/2] [Outreachy] t3903-stash: test without configured user.name and user.email","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-18T06:28:34Z","receivedAt":"2018-11-18T06:31:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Slavica Djukic writes:\n\n>> Yes, but for that you'd need to be checking the resulting commit\n>> object that represents the stash entry. It should be created under\n>> the substitute identity.\n> Would it be correct to check it like this:\n>\n>         git reset &&\n>         >1 &&\n>         git add 1 &&\n>         echo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>\" >expect &&\n>         git stash &&\n>         git show -s --format=\"%an <%ae>\" refs/stash >actual &&\n>         test_cmp expect actual\n\nSo, you create a stash, and grab %an and %ae out of the resulting\ncommit object and store them in actual, and then compare. Makes\nsense.\n"},{"id":"363575","messageId":"20181118132915.9336-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181114221218.3112-1-slawica92@hotmail.com","subject":"[PATCH 0/1 v3] make stash work if user.name and user.email are not configured","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-18T13:29:15Z","receivedAt":"2018-11-18T13:35:21Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"Changes since v2:\n\t* squash patch 1/2 and patch 2/2 into a single patch\n\t* modify first part of test when there is valid ident\n\t present: create a stash, grab %an and %ae out of the \n\t resulting commit object and compare to original ident\n\t \nSlavica Djukic (1):\n stash: tolerate missing user identity\n\n git-stash.sh | 17 +++++++++++++++++\n t/t3903-stash.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+)\n\n-- \n2.19.1.1052.gd166e6afe\n\n"},{"id":"363576","messageId":"20181118134407.11196-1-slawica92@hotmail.com","threadId":"49651","inReplyTo":"20181118132915.9336-1-slawica92@hotmail.com","subject":"[PATCH 1/1 v3] stash: tolerate missing user identity","fromName":"Slavica Djukic","fromEmail":"slavicadj.ip2018@gmail.com","sentAt":"2018-11-18T13:44:07Z","receivedAt":"2018-11-18T13:44:40Z","isPatch":true,"sender":{"key":"slavicadj.ip2018@gmail.com","avatar":null},"body":"The \"git stash\" command insists on having a usable user identity to\nthe same degree as the \"git commit-tree\" and \"git commit\" commands\ndo, because it uses the same codepath that creates commit objects\nas these commands.\n\nIt is not strictly necesary to do so. Check if we will barf before\ncreating commit objects and then supply fake identity to please the\nmachinery that creates commits.\nAdd test to document that stash executes correctly both with and\nwithout valid ident.\n\nThis is not that much of usability improvement, as the users who run\n\"git stash\" would eventually want to record their changes that are\ntemporarily stored in the stashes in a more permanent history by\ncommitting, and they must do \"git config user.{name,email}\" at that\npoint anyway, so arguably this change is only delaying a step that\nis necessary to work in the repository.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Slavica Djukic \n---\n git-stash.sh | 17 +++++++++++++++++\n t/t3903-stash.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 45 insertions(+)\n\ndiff --git a/git-stash.sh b/git-stash.sh\nindex 94793c1a9..789ce2f41 100755\n--- a/git-stash.sh\n+++ b/git-stash.sh\n@@ -55,6 +55,20 @@ untracked_files () {\n \tgit ls-files -o $z $excl_opt -- \"$@\"\n }\n \n+prepare_fallback_ident () {\n+\tif ! git -c user.useconfigonly=yes var GIT_COMMITTER_IDENT >/dev/null 2>&1\n+\tthen\n+\t\tGIT_AUTHOR_NAME=\"git stash\"\n+\t\tGIT_AUTHOR_EMAIL=git@stash\n+\t\tGIT_COMMITTER_NAME=\"git stash\"\n+\t\tGIT_COMMITTER_EMAIL=git@stash\n+\t\texport GIT_AUTHOR_NAME\n+\t\texport GIT_AUTHOR_EMAIL\n+\t\texport GIT_COMMITTER_NAME\n+\t\texport GIT_COMMITTER_EMAIL\n+\tfi\n+}\n+\n clear_stash () {\n \tif test $# != 0\n \tthen\n@@ -67,6 +81,9 @@ clear_stash () {\n }\n \n create_stash () {\n+\n+\tprepare_fallback_ident\n+\n \tstash_msg=\n \tuntracked=\n \twhile test $# != 0\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex cd216655b..5f8272b6f 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1096,4 +1096,32 @@ test_expect_success 'stash -- works with binary files' '\n \ttest_path_is_file subdir/untracked\n '\n \n+test_expect_success 'stash works when user.name and user.email are not set' '\n+\tgit reset &&\n+\t>1 &&\n+\tgit add 1 &&\n+\techo \"$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL>\" >expect &&\n+\tgit stash &&\n+\tgit show -s --format=\"%an <%ae>\" refs/stash >actual &&\n+\ttest_cmp expect actual &&\n+\t>2 &&\n+\tgit add 2 &&\n+\ttest_config user.useconfigonly true &&\n+\ttest_config stash.usebuiltin true &&\n+\t(\n+\t\tsane_unset GIT_AUTHOR_NAME &&\n+\t\tsane_unset GIT_AUTHOR_EMAIL &&\n+\t\tsane_unset GIT_COMMITTER_NAME &&\n+\t\tsane_unset GIT_COMMITTER_EMAIL &&\n+\t\ttest_unconfig user.email &&\n+\t\ttest_unconfig user.name &&\n+\t\ttest_must_fail git commit -m \"should fail\" &&\n+\t\techo \"git stash \" >expect &&\n+\t\t>2 &&\n+\t\tgit stash &&\n+\t\tgit show -s --format=\"%an <%ae>\" refs/stash >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n2.19.1.1052.gd166e6afe\n\n"}]}