-
-
Notifications
You must be signed in to change notification settings - Fork 601
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
Support options
in git_status.fetch()
#2287
base: master
Are you sure you want to change the base?
Conversation
It looks like support was available/built-in, but possibly left out for some reason? Happy to rework, this is just to start conversations :)
I just realized I got it backwards! That is totally my bad 😅 |
thanks for looking into this. i wonder what blame tells us how this got overseen - I assume no one ever used it so it went unnoticed 🙈 |
It seems to be, it's been 3 years! To be completely fair, most people (myself included) don't know how Git works in most cases. All you usually need is the good ol' I remember seeing this was from Stephan Dilly (though I'm not sure who that is, he's in a lot of the old code. Is that your old handle / name perhaps? Or an old maintainer?) After blaming (using the recursive blame I just added! haHA!!), the project at that point was (as far as I can tell) fully able to support it (the options were there already). Maybe it was just planned to be done in a separate commit. |
I'm glad someone is working on this. Thank you guys. I use stashing a lot, so this would definitely help me. I suppose this solution doesn't really change anything in the functionality, when user swaps different values in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
I've noticed a minor snag with the --keep-index logic in your PR.
If you were to fix that and please add an entry to CHANGELOG.md, I'd say we're good to go, though.
let status_type = if self.options.keep_index { | ||
StatusType::Both | ||
} else { | ||
StatusType::WorkingDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My expectation here would be, that this file list shows all those files, which would be included in a stash.
Here's --keep-index
's description:
-k, --keep-index, --no-keep-index
This option is only valid for push and save commands.
All changes already added to the index are left intact.
I.e. "keep index" has no effect on whether a change in the index is stashed or not. It just affects whether that change is removed from the index and work tree afterwards.
Thus, I would have expected that whether "keep index" is active or not has no effect on whether that file a file in the index is listed here.
It looks like support was available/built-in, but possibly left out for some reason?
Happy to rework, this is just to start conversations :)
This Pull Request fixes/closes #2263.
I followed the checklist:
make check
without errorsIt would also be good to know your stance on the
?
idea, or any other additions you might want to this to make this mergeable.