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

handle warnings if stash creation encounters permission issue #7351

Merged
merged 10 commits into from Apr 25, 2019

Conversation

shiftkey
Copy link
Member

@shiftkey shiftkey commented Apr 18, 2019

Overview

Related to #7344

Description

I'm not a huge fan of this current patch, but it's targeted to this specific scenario:

  • git stash push runs and creates the stash (including untracked files)
  • while doing this, Git encounters a path in the repository it does not have access to
  • operation continues, completing the stash but raises a non-zero exit code and adds messages to stderr about the problem path

This patch is supposed to:

  • look for the 1 exit code
  • inspect stderr to ensure no error: messages exist
  • if found, throw an error with the context (emulating previous behaviour)
  • otherwise just log whatever was found on stderr and continue

@say25 as you had a concrete reproduction of the permissions denied error, are you able to verify this fix at some stage by building this PR branch locally and trying it with that problem repository?

Release notes

Notes: [Fixed] stashing when untracked path is not accessible will not block checkout

@shiftkey shiftkey added this to the 1.7.0 milestone Apr 18, 2019
@say25
Copy link
Member

say25 commented Apr 18, 2019

@shiftkey Build and tested 5a34e36 locally. I can confirm unblocked the expected workflow.

What happens is:

  1. A stash is generated
  2. I get an error message saying there are merge conflicts (dismissable, confusing but not horrible)
  3. I my changes are carried to the new branch and that a patch file with the same change set is generated.

Log:

I had to redact some of the file names but for the most part you can see what is happening

2019-04-18.desktop.development.log

@shiftkey shiftkey marked this pull request as ready for review April 18, 2019 18:15
@shiftkey shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 18, 2019
@shiftkey
Copy link
Member Author

Moving this to ready-to-review because the new error uncovered is related to encountering conflicts after applying stashing, which is a known issue: #7278

2019-04-18T18:01:36.502Z - error: [ui] `git stash pop stash@{0}` exited with an unexpected code: 1.
The stash entry is kept in case you need it again.

error: Your local changes to the following files would be overwritten by merge:
    [files]
Please commit your changes or stash them before you merge.
Aborting

@iAmWillShepherd iAmWillShepherd self-assigned this Apr 22, 2019
@shiftkey
Copy link
Member Author

@iAmWillShepherd I re-requested a review from you to confirm whether you'd like to see this merged for the beta, or whether you want to investigate alternatives...

@outofambit outofambit added the epic:stashing Tracking label for work related to the stashing flow label Apr 23, 2019
outofambit
outofambit previously approved these changes Apr 23, 2019
@outofambit outofambit dismissed stale reviews from iAmWillShepherd and themself via 47dd7c1 April 25, 2019 14:54
Copy link
Contributor

@outofambit outofambit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you!

@outofambit outofambit self-assigned this Apr 25, 2019
@outofambit outofambit merged commit 1a668a3 into development Apr 25, 2019
@outofambit outofambit deleted the warn-but-continue-with-stash branch April 25, 2019 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
epic:stashing Tracking label for work related to the stashing flow ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants