disambiguate git command #193
Conversation
Previously local files could collide with the passed target-branch. This would yield an error like 'fatal: ambiguous argument 'main': both revision and filename'.
thumbnail
requested review from
a team,
josefigueroa-nedap and
lennartbuit
and removed request for
a team
thumbnail
force-pushed
the
disambiguate-git-diff
branch
from
e99759c to
17f1aac
Compare
| (let [file (io/as-file "main")] | ||
| (try | ||
| (spit file "test file") | ||
| (is (sut/git-ref-exists? "main")) |
There was a problem hiding this comment.
🙌
If it's not an excessive burden I'd test git-diff-against-default-branch itself
There was a problem hiding this comment.
This test wouldn't fail on main, so i removed it.
I added a test for git-diff-against-default-branch, which was a pretty rough test to write, but it should be fine now :)
thumbnail
force-pushed
the
disambiguate-git-diff
branch
from
24bc89a to
6d5e122
Compare
| (try | ||
| ;; creating a file with a filename which collides with a sha. | ||
| (spit ambiguous-file creatable-contents) | ||
| (expect-sane-output! (sut/git-diff-against-default-branch :target-branch @root-commit)) |
There was a problem hiding this comment.
Because this assertion is programmatic/dynamic we can't be exactly sure of what is being asserted here? As the name hints, it only asserts some basic 'sanity' not correctness/completeness.
I wouldn't remove it, but additionally I would assert something related to the filename named after root-commit
There was a problem hiding this comment.
The expect-sane-output! asserts that sut/git-diff-against-default-branch returns at least 1 file. Without the patch the function will return nil. In this specific case the filename shouldn't be part of the diff but imo it doesn't matter (like the other cases in this test).
What would you suggest be asserted besides the existing assertions?
There was a problem hiding this comment.
In this specific case the filename shouldn't be part of the diff
When I see (spit ambiguous-file creatable-contents) I'd expect that file to be part of the diff
Maybe if we temporarily git add -A it should? (don't quote me on that, just quickly eyeing things on github)
In which case it would be good to exercise both cases (added , not added)
There was a problem hiding this comment.
The test exists to assert it runs without errors, exercising its specs. Testing the actual functionality of sut/git-diff-against-default-branch is out of scope for this test.
However, such a test is missing, so I added this issue to keep track: #194
| (let [ambiguous-file (io/file git-integration-dir @root-commit)] | ||
| (try | ||
| ;; creating a file with a filename which collides with a sha. | ||
| (spit ambiguous-file creatable-contents) |
There was a problem hiding this comment.
git-sha-named-file, maybe? I'm having trouble following this test
Brief
Previously local files could collide with the passed target-branch. This would yield an error like
'fatal: ambiguous argument 'main': both revision and filename'.QA plan
also executed in test as of 6d5e122
(strategies/git-diff-against-default-branch :target-branch "main"), assert seq of filenames$ touch master(strategies/git-diff-against-default-branch :target-branch "main")again, assert seq of filenamesStep 5 will fail on current
mainby returningnil.Author checklist
Reviewer checklist