Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Outreachy] New test in t3903-stash.sh : execute git stash without author #1891

Closed

Conversation

slavicaDj
Copy link

@slavicaDj slavicaDj commented Oct 22, 2018

This is test for git stash - it fails when author is not set, and represents known breakage.

@slavicaDj slavicaDj changed the title t3909-stash.sh added New test t3909-stash.sh : execute git stash without author Oct 22, 2018
@PhilipOakley
Copy link

The commit message doesn't explain why this is needed. Are these things (stash without author or HOME set) that you would expect to work anyway, or is it just that any error messages are given at an unhelpful time?

I'm guessing that you has a rush action that needed a stash, and you got an error message about failing to have an author email and in the rush don't have time to fix that. Maybe you were just testing stash and it told you you should have set the author variabvle. But it could be more complicated than that because of the HOME issue.

@slavicaDj
Copy link
Author

slavicaDj commented Oct 22, 2018

This test is part of enhancement discussed here: https://public-inbox.org/git/[email protected]/T/#u -- I am trying to contribute to said enhancement as Outreachy participant.

@slavicaDj slavicaDj force-pushed the adding_new_test_t3909 branch from 3d91285 to 4120032 Compare October 22, 2018 17:46
@slavicaDj slavicaDj changed the title New test t3909-stash.sh : execute git stash without author New test in t3903-stash.sh : execute git stash without author Oct 22, 2018
@slavicaDj slavicaDj force-pushed the adding_new_test_t3909 branch from 4120032 to fe5305c Compare October 22, 2018 18:18
@PhilipOakley
Copy link

Ahh, I though I'd seen something about this on the main Git List. Welcome!

Just to clarify, the Git list is for the upstream Git (the master one), which we (the Git for Windows devs, lead by dscho) follow. For our local (to GfW) commits we use the PR process. For the main Git List commits it is plain text patches submitted to the list.

Our bot will also do some rudimentary checks such as ensuring that you have 'signed off' the patches (see the Developers Certificate of Origin DCO in the SubmittingPatches file).

The commit message should have a short concise one-liner summary that is readable with the log/show commands, and a message that is in the imperative i.e. we tell the computer what to do and why. We also include references to discussions (as you did above).

You can always force push an update to this PR, which will run the checks and allow you to see how well it looks, or you can submit the patch directly upstream.

You should also mention that the submission is part of the Outreachy programme, as that will put the submission in context, and hopefully prompt helpful responses.

@dscho
Copy link
Member

dscho commented Oct 23, 2018

@PhilipOakley FWIW I was in contact (via private Gitter chat) with @slavicaDj and recommended to open this here PR so that I can help prepare this for contribution to the Git mailing list. Feel free to continue to chime in, this is great!

@slavicaDj
Copy link
Author

@PhilipOakley thank you for your welcome and constructive feedback! I am working on improving this PR.

@slavicaDj slavicaDj force-pushed the adding_new_test_t3909 branch from fe5305c to 8680a7e Compare October 23, 2018 12:49
@slavicaDj slavicaDj changed the title New test in t3903-stash.sh : execute git stash without author [Outreachy] New test in t3903-stash.sh : execute git stash without author Oct 23, 2018
@slavicaDj slavicaDj force-pushed the adding_new_test_t3909 branch 6 times, most recently from 21b046a to dcc0225 Compare October 24, 2018 16:31
@slavicaDj slavicaDj force-pushed the adding_new_test_t3909 branch 9 times, most recently from 278cd28 to 910ec8d Compare October 30, 2018 11:26
… and user.email

Add test to assert that stash fails if user.name and user.email
are not configured.
In the final commit, test will be updated to expect success.

Signed-off-by: Slavica Djukic <[email protected]>
@slavicaDj slavicaDj force-pushed the adding_new_test_t3909 branch 2 times, most recently from a0ad405 to 7ea3126 Compare November 7, 2018 12:28
@dscho
Copy link
Member

dscho commented Nov 29, 2018

For lurkers: this continues on the Git mailing list: https://public-inbox.org/git/[email protected]/#r

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants