-
Notifications
You must be signed in to change notification settings - Fork 536
saving colons in stash messages from crashes #749
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
Conversation
Late night fix... the two stash-message-colon-related bugs fixed. |
If I run the updated test without the change in Please re-submit with a test that fails without your change. Also, I am not sure what the "MISNAMED" part is for. I think that a stash operation is always " |
@jcouball Thanks for your review The story behind this minor fix is me git-stashing from within emacs and this gem crashing my collab automation using it. You see, whereas At first, I misidentified the issue with the code, and also, forgot how Regexp greediness works in Ruby -- I myself never rely on particular greediness kind and make my regexes agnostic, so I will drop that part of the "fix" if you like. Now, for the second "MISNAMED" fix (the actual crash upon "unconventionally" named stashes), I honestly didn't know what to do to stay in line with the gem's logic because it seems stash objects here include branch information -- which is not always available. So, as I wouldn't want to make a big deal out of it, accommodating for these corner cases -- just to prevent crashes. I wasn't also sure if it's acceptable to create a "problem" stash for the test through running Thanks again, let me know what you think and I'll accommodate, of course. |
Ok, I see what the problem is when the stash entry is created with To maintain backward compatibility for 2.x version, I think we have to compromise with your case. Since it is a pretty rare case (I'd guess), it seems like the best option for 2.x. I propose that in your case where the stash is created with I opened #750 to address this issue for 3.x. Below are my proposed tests. The 1st test below replaces the existing test (before your changes). The 3rd test fails and is awaiting your implementation change. Note that in the 4th test shows the compromise described above.
|
Also, please squash your commits into one commit with a message that is suitable to show up in the release notes. |
- the entry point (in a Docker-enabled env) is `bin/tests` - fixed the (rather invasive outside of a container) `bin/test` test runner
- the tests are generously provided by James Couball <jcouball@yahoo.com> - more proper stash metadata parsing introduced - supporting both "branched" ("On <BRANCH>: ...") and "unbranched" messages - which might affect the future 3.x behaviour wrt "un/branched" stashes
792422f
to
2f467c6
Compare
@jcouball done and done I also hope that you don't mind I've "Dockerised" the tests; I do all my development containerised within a remote ("self server") dev env. To note, I usually call the testing entry point You're welcome to have a look at my "self server" "decentralized" git development demo at https://medium.com/@costa/git-decentralized-a25f00fd2955 |
@costa Thank you for your contribution of the bug fix! Running tests in docker is neat. Thanks! This is something that might be built upon for testing against different versions of git. I will follow up with a PR for the following:
Hopefully, I will finish these changes soon and do a new release sometime today. |
I addressed the feedback above in #751 and published 3.2.1 of the git gem to rubygems.org. See the change log in the Github release for 3.2.1. |
Thank you. If you may be interested in my container-based and git-based methodology tooling, e.g. a spec helper: |
A tiny fix, would appreciate a version ASAP. Thanks!