More test coverage for Repository::state::InProgress #393
Conversation
There was a problem hiding this comment.
Thanks a million, it's great to see this fully tested (and of course to see that testing is truly worth it to catch actual issues).
I can do some refactoring on top (see my comment) to make it mergable, but would ask you to try to use git-lfs to not checkin actual binary files of fixture archives like is the state now.
It's surprising that git allows to add these in the first place as I would expect it to fail if a configured filter isn't found (it's in .gitattributes in the root).
Please let me know in case git-lfs isn't working for you for some reason and I will cleanup the history accordingly. I also realized that the test system definitely needs a switch to ignore archives which is something I'd use to validate the scripts actually work on MacOS and maybe even on Windows.
|
|
||
| git init -q | ||
|
|
||
| echo 1.main > 1 |
There was a problem hiding this comment.
The issue I am having with naming files with numbers while redirecting into them is that … >1 has a different meaning than > 1, and it's just a little too close for me to feel comfortable while reading the script.
I hope you can see the point and avoid such names in the future, and I am happy to rename it for you for now as well.
There was a problem hiding this comment.
Ah, the joys of sh. The only thing I'd add is that for this script I saved the path to the patch in ${patch_path} but the call to git am references the file directly.
The latter should be git am "${patch_path}".
There was a problem hiding this comment.
Thanks for the hint. It's adjusted in cfaf31f.
That way fixture scripts can forcefully be executed which adds some safety if the feature is actually used.
It seems less descriptive, but then again, people are coming from `git2` and might appreciate familiarity. Maybe this can be merged into something more descriptive in a future `status()` platform, like `status().in_progress()`.
|
Thanks a lot for your continued contribution, it's much appreciated! I took the liberty to do the refactors myself as it's faster to do that than to review again, thanks for your understanding. |
This also caught a flaw in the
CherryPickSequenceandRevertSequencelogic.