diff mbox series

[1/3,Outreachy] t3903-stash: test without configured user name

Message ID 20181023162941.3840-1-slawica92@hotmail.com (mailing list archive)
State New, archived
Headers show
Series [1/3,Outreachy] t3903-stash: test without configured user name | expand

Commit Message

Slavica Djukic Oct. 23, 2018, 4:29 p.m. UTC
This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.
The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.

Signed-off-by: Slavica <slawica92@hotmail.com>
---
 t/t3903-stash.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Christian Couder Oct. 23, 2018, 6:52 p.m. UTC | #1
On Tue, Oct 23, 2018 at 6:35 PM Slavica <slavicadj.ip2018@gmail.com> wrote:
>
> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.
> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.

We prefer commit messages that contain as much as possible all the
information necessary to understand the patch without links to other
places.

It seems that only this email from you reached me. Did you send other
emails for patches 2/3 and 3/3?

[...]

> +    (
> +        HOME=$(pwd)/none &&
> +        export HOME &&
> +        unset GIT_AUTHOR_NAME &&
> +        unset GIT_AUTHOR_EMAIL &&
> +        unset GIT_COMMITTER_NAME &&
> +        unset GIT_COMMITTER_EMAIL &&
> +        test_must_fail git config user.email &&
> +        echo changed >1.t &&
> +               git stash

It seems that the above line is not indented like the previous ones.

> +    )
> +'

Thanks for contributing,
Christian.
Eric Sunshine Oct. 23, 2018, 7:19 p.m. UTC | #2
On Tue, Oct 23, 2018 at 12:31 PM Slavica <slavicadj.ip2018@gmail.com> wrote:
> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.
> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.

As Christian mentioned already, it's best to try to describe the issue
succinctly in the commit message so readers can understand it without
chasing a link. For this simple case, it should be sufficient to
explain that, due to an implementation detail, git-stash undesirably
requires 'user.name' and 'user.email' to be set, but shouldn't.

> Signed-off-by: Slavica <slawica92@hotmail.com>
> ---
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> @@ -1156,4 +1156,21 @@ test_expect_success 'stash -- <subdir> works with binary files' '
> +test_expect_failure 'stash with HOME as non-existing directory' '

The purpose of this test is to demonstrate that git-stash has an
undesirable requirement that 'user.name' and 'user.email' be set. The
test title should reflect that. So, instead of talking about
non-existent HOME (which is just an implementation detail of the
test), a better test title would be something like "stash works when
user.name and user.email are not set".

> +    test_commit 1 &&
> +    test_config user.useconfigonly true &&
> +    test_config stash.usebuiltin true &&
> +    (
> +        HOME=$(pwd)/none &&
> +        export HOME &&
> +        unset GIT_AUTHOR_NAME &&

Use sane_unset() for all of these rather than bare 'unset'.

> +        unset GIT_AUTHOR_EMAIL &&
> +        unset GIT_COMMITTER_NAME &&
> +        unset GIT_COMMITTER_EMAIL &&
> +        test_must_fail git config user.email &&
> +        echo changed >1.t &&
> +               git stash

Christian already mentioned the odd indentation.

> +    )
> +'
Junio C Hamano Oct. 24, 2018, 2:48 a.m. UTC | #3
Slavica <slavicadj.ip2018@gmail.com> writes:

> +test_expect_failure 'stash with HOME as non-existing directory' '
> +    test_commit 1 &&
> +    test_config user.useconfigonly true &&
> +    test_config stash.usebuiltin true &&
> +    (
> +        HOME=$(pwd)/none &&
> +        export HOME &&

What is the reason why this test needs to move HOME away from
TRASH_DIRECTORY (set in t/test-lib.sh)?

> +        unset GIT_AUTHOR_NAME &&
> +        unset GIT_AUTHOR_EMAIL &&
> +        unset GIT_COMMITTER_NAME &&
> +        unset GIT_COMMITTER_EMAIL &&
> +        test_must_fail git config user.email &&
> +        echo changed >1.t &&
> +		git stash
> +    )
> +'
> +
>  test_done
Johannes Schindelin Oct. 24, 2018, 7:39 a.m. UTC | #4
Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> Slavica <slavicadj.ip2018@gmail.com> writes:
> 
> > +test_expect_failure 'stash with HOME as non-existing directory' '
> > +    test_commit 1 &&
> > +    test_config user.useconfigonly true &&
> > +    test_config stash.usebuiltin true &&
> > +    (
> > +        HOME=$(pwd)/none &&
> > +        export HOME &&
> 
> What is the reason why this test needs to move HOME away from
> TRASH_DIRECTORY (set in t/test-lib.sh)?

This is to make sure that no user.name nor user.email is configured. That
was my idea, hence I answer your question.

Ciao,
Dscho

> 
> > +        unset GIT_AUTHOR_NAME &&
> > +        unset GIT_AUTHOR_EMAIL &&
> > +        unset GIT_COMMITTER_NAME &&
> > +        unset GIT_COMMITTER_EMAIL &&
> > +        test_must_fail git config user.email &&
> > +        echo changed >1.t &&
> > +		git stash
> > +    )
> > +'
> > +
> >  test_done
>
Junio C Hamano Oct. 24, 2018, 9:58 a.m. UTC | #5
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Wed, 24 Oct 2018, Junio C Hamano wrote:
>
>> Slavica <slavicadj.ip2018@gmail.com> writes:
>> 
>> > +test_expect_failure 'stash with HOME as non-existing directory' '
>> > +    test_commit 1 &&
>> > +    test_config user.useconfigonly true &&
>> > +    test_config stash.usebuiltin true &&
>> > +    (
>> > +        HOME=$(pwd)/none &&
>> > +        export HOME &&
>> 
>> What is the reason why this test needs to move HOME away from
>> TRASH_DIRECTORY (set in t/test-lib.sh)?
>
> This is to make sure that no user.name nor user.email is configured. That
> was my idea, hence I answer your question.

HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do
so to avoid getting affected by the real $HOME/.gitconfig of the
user who happens to be running the test suite.

With that in place for ages, this test still moves HOME away from
TRASH_DIRECTORY, and that is totally unnecessary if it is only done
to avoid getting affected by the real $HOME/.gitconfig of the user.
After all, this single test is not unique in its need to avoid
reading from user's $HOME/.gitconfig---so I expected there may be
other reasons.

That is why I asked, and your response is not quite answering that
question.
Slavica Djukic Oct. 24, 2018, 1:56 p.m. UTC | #6
On 23-Oct-18 8:52 PM, Christian Couder wrote:
> On Tue, Oct 23, 2018 at 6:35 PM Slavica <slavicadj.ip2018@gmail.com> wrote:
>> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.
>> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.
> We prefer commit messages that contain as much as possible all the
> information necessary to understand the patch without links to other
> places.
>
> It seems that only this email from you reached me. Did you send other
> emails for patches 2/3 and 3/3?
>
> [...]

Okay, I will change that. This is my first patch and I am still adapting.

Emails for patches 2/3 and 3/3 because aren't there because I am still 
preparing them.

(I didn't know if I had 3 patches in plan that they should be sent at 
almost the same time.)

>
>> +    (
>> +        HOME=$(pwd)/none &&
>> +        export HOME &&
>> +        unset GIT_AUTHOR_NAME &&
>> +        unset GIT_AUTHOR_EMAIL &&
>> +        unset GIT_COMMITTER_NAME &&
>> +        unset GIT_COMMITTER_EMAIL &&
>> +        test_must_fail git config user.email &&
>> +        echo changed >1.t &&
>> +               git stash
> It seems that the above line is not indented like the previous ones.
I don't know what is the reason, in my IDE everything seems fine, but 
I'll fix it.
>
>> +    )
>> +'
> Thanks for contributing,
> Christian.

You are welcome,

Slavica

>
>
Johannes Schindelin Oct. 24, 2018, 3:18 p.m. UTC | #7
Hi Junio,

On Wed, 24 Oct 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 24 Oct 2018, Junio C Hamano wrote:
> >
> >> Slavica <slavicadj.ip2018@gmail.com> writes:
> >> 
> >> > +test_expect_failure 'stash with HOME as non-existing directory' '
> >> > +    test_commit 1 &&
> >> > +    test_config user.useconfigonly true &&
> >> > +    test_config stash.usebuiltin true &&
> >> > +    (
> >> > +        HOME=$(pwd)/none &&
> >> > +        export HOME &&
> >> 
> >> What is the reason why this test needs to move HOME away from
> >> TRASH_DIRECTORY (set in t/test-lib.sh)?
> >
> > This is to make sure that no user.name nor user.email is configured. That
> > was my idea, hence I answer your question.
> 
> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do
> so to avoid getting affected by the real $HOME/.gitconfig of the
> user who happens to be running the test suite.

My bad. I should have checked. I was under the impression that we set
`HOME` to the `t/` directory and initialized it. But you are right, of
course, and that subshell as well as the override of `HOME` are absolutely
unnecessary.

Thanks,
Dscho

> 
> With that in place for ages, this test still moves HOME away from
> TRASH_DIRECTORY, and that is totally unnecessary if it is only done
> to avoid getting affected by the real $HOME/.gitconfig of the user.
> After all, this single test is not unique in its need to avoid
> reading from user's $HOME/.gitconfig---so I expected there may be
> other reasons.
> 
> That is why I asked, and your response is not quite answering that
> question.
>
Slavica Djukic Oct. 24, 2018, 8:01 p.m. UTC | #8
Changes since v1:

    *changed test title
    *removed subshell and HOME override
    *fixed weird identation
    *unset() replaced with sane_unset()

Slavica (1):
  [Outreachy] t3903-stash: test without configured user name

 t/t3903-stash.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Junio C Hamano Oct. 25, 2018, 4:17 a.m. UTC | #9
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> HOME is set to TRASH_DIRECTORY in t/test-lib.sh already, and we do
>> so to avoid getting affected by the real $HOME/.gitconfig of the
>> user who happens to be running the test suite.
>
> My bad. I should have checked. I was under the impression that we set
> `HOME` to the `t/` directory and initialized it. But you are right, of
> course, and that subshell as well as the override of `HOME` are absolutely
> unnecessary.

I was afraid that I may be missing some future plans to update
$TRASH_DIRECTORY/.gitconfig with "git config --global user.name Foo"
etc. in an earlier part of the test script, which would have made
the subshell and moving HOME elsewhere perfectly good ways to future
proof the new test being added (in which case, in-code comment to
say that near the assignment to HOME would have been a good
improvement).

Not that having them breaks the logic, but they distract the
readers by making them wonder what is going on, so I think we can do
without the subshell and assignment to HOME.

Thanks.
Junio C Hamano Oct. 25, 2018, 4:44 a.m. UTC | #10
Slavica <slavicadj.ip2018@gmail.com> writes:

> On 23-Oct-18 8:52 PM, Christian Couder wrote:
>> On Tue, Oct 23, 2018 at 6:35 PM Slavica <slavicadj.ip2018@gmail.com> wrote:
>>> This is part of enhancement request that ask for `git stash` to work even if `user.name` is not configured.
>>> The issue is discussed here: https://public-inbox.org/git/87o9debty4.fsf@evledraar.gmail.com/T/#u.
>> We prefer commit messages that contain as much as possible all the
>> information necessary to understand the patch without links to other
>> places.
>>
>> It seems that only this email from you reached me. Did you send other
>> emails for patches 2/3 and 3/3?
>>
>> [...]
>
> Okay, I will change that. This is my first patch and I am still adapting.
>
> Emails for patches 2/3 and 3/3 because aren't there because I am still
> preparing them.
>
> (I didn't know if I had 3 patches in plan that they should be sent at
> almost the same time.)

It is more efficient for everybody involved.

 - You may discover that 1/3 you just (thought) finished was not
   sufficient while working on 2/3 and 3/3, and by the time you are
   pretty close to finishing 2/3 and 3/3, you may want to update 1/3
   in a big way.  Sending a premature version and having others to
   review is wasting everbody's time.

 - Your 1/3 might become perfect alone with help from others'
   reviews and your updates, but after that everybody may forget
   about it when you are ready to send out 2/3 and 3/3; if these
   three are truly related patches in a single topic, you would want
   to have what 1/3 did fresh in your reviewers' minds.  You'd have
   to find the old message of 1/3 and make 2/3 and 3/3 responses to
   it to keep them properly threaded (which may take your time), and
   reviewers need to refresh their memory by going back to 1/3
   before reviewing 2/3 and 3/3

One thing I learned twice while working in this project is that open
source development is not a race to produce and show your product as
quickly as possible.

When I was an individual contributor, the project was young and
there were many people with good and competing ideas working to
achieve more-or-less the same goal.  It felt like a competition
to get *MY* version of the vision, design and implementation over
others' adopted and one way to stay in the competition was to send
things as quickly as possible.  I didn't know better, and I think I
ended up wasting many people's time that way.

That changed when I became the maintainer, as (1) I no longer had to
race with anybody ;-), and (2) I introduced the 'pu' (proposed
update) system so that anything that was queued early can be
discarded and replaced when a better thing come within a reasonable
timeframe.

And then I re-learned the same "this is not a race" lesson a couple
of years ago, when I started working in a timezone several hours
away from the most active participants for a few months at a time.
I do not have to respond to a message I see on the list immediately,
as it is too late to catch the sender who is already in bed ;-)


So take your time and make sure what you are sending out can be
reviewed the most efficiently.  Completing 2/3 and 3/3 before
sending 1/3 out to avoid having to redo 1/3 and avoid having
reviewers to spend their time piecemeal is one thing.  Making sure
that the patch does not have style issues that distract reviewers'
attention is another.

Sitting on what you think you have completed for a few days allows
you to review your product with fresh eyes before sending them out,
which is another benefit of trying not to rush.
Slavica Djukic Oct. 25, 2018, 7:13 p.m. UTC | #11
Changes since v1:

    *changed:
	 test_must_fail git config user.email 
	 to:
	 test_unconfig user.email &&      
	 test_unconfig user.name 

	This is done to make sure that user.email and user.name are not set,
	instead of asserting it with test_must_fail config user.email.



Slavica (1):
  [Outreachy] t3903-stash: test without configured user name

 t/t3903-stash.sh | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
diff mbox series

Patch

diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 9e06494ba0..9ff34a65bc 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1156,4 +1156,21 @@  test_expect_success 'stash -- <subdir> works with binary files' '
 	test_path_is_file subdir/untracked
 '
 
+test_expect_failure 'stash with HOME as non-existing directory' '
+    test_commit 1 &&
+    test_config user.useconfigonly true &&
+    test_config stash.usebuiltin true &&
+    (
+        HOME=$(pwd)/none &&
+        export HOME &&
+        unset GIT_AUTHOR_NAME &&
+        unset GIT_AUTHOR_EMAIL &&
+        unset GIT_COMMITTER_NAME &&
+        unset GIT_COMMITTER_EMAIL &&
+        test_must_fail git config user.email &&
+        echo changed >1.t &&
+		git stash
+    )
+'
+
 test_done