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

Allow diffs against the empty tree #53

Merged
merged 3 commits into from
Apr 21, 2020
Merged

Allow diffs against the empty tree #53

merged 3 commits into from
Apr 21, 2020

Conversation

olsen232
Copy link
Collaborator

@olsen232 olsen232 commented Apr 19, 2020

Right now sno diff only works between commits. This means there is no way to diff the first commit against the empty repository.

Fixes #44.

This change makes it so a RepositoryStructure can be a commit, or a tree - since the commit is mostly not used except as a way to get a particular tree.
This means getting a diff from the first commit to the empty repository can be done as follows:

sno diff 4b825dc642cb6eb9a060e54bf8d69288fbee4904...HEAD

where 4b825dc642cb6eb9a060e54bf8d69288fbee4904 is the hash of the empty repository.

This is not very easy to remember, but we could still build upon this change to make any of the following:

  • Make sno show show this diff if you sno show the first commit. This works in git.
  • Make an alias for the empty tree, eg sno diff EMPTY...HEAD
  • Make HEAD^ etc an alias for the empty tree if they would otherwise refer to the commit before the first commit.

Small change, but structural change: will wait for rcoup@ to review to make sure this makes sense to him.

@olsen232 olsen232 requested review from rcoup and craigds April 19, 2020 22:41
@craigds
Copy link
Member

craigds commented Apr 19, 2020

LGTM. I don't expect anyone to be using 4b825dc642cb6eb9a060e54bf8d69288fbee4904 explicitly, but the main thing is that this enables sno show <first-commit>, which currently doesn't work because it requires a diff from the empty tree.

@craigds
Copy link
Member

craigds commented Apr 20, 2020

Make sno show show this diff if you sno show the first commit. This works in git.

If you merge master into this branch that should work now (since #48 was merged)

Copy link
Member

@craigds craigds left a comment

Choose a reason for hiding this comment

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

Test feels like it needs splitting up into a few bits. Maybe parametrize the NOOP_SPECS and F_SPECS things too, so the test bodies become tiny.

e.g.

@pytest.mark.parametrize("noop_spec", (
    f"{POINTS_HEAD_TREE_SHA[:6]}..{POINTS_HEAD_TREE_SHA[:6]}",
    f"{POINTS_HEAD_TREE_SHA}..{POINTS_HEAD_TREE_SHA}",
    f"{POINTS_HEAD1_TREE_SHA}..{POINTS_HEAD1_TREE_SHA}",
    f"{POINTS_HEAD_TREE_SHA}..",
    f"..{POINTS_HEAD_TREE_SHA}",
))
def test_diff_rev_rev(data_archive, cli_runner, noop_spec):

@olsen232
Copy link
Collaborator Author

Re: parametrizing. I broke up the test and gave it more parametrization, so it's not such a mess. But, not to the extent you suggested - it seemed wasteful to set up a fresh repository six times, just to assert that the diff of a commit with itself is the empty diff six times.

Copy link
Member

@craigds craigds left a comment

Choose a reason for hiding this comment

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

Cool. Yeah. We could solve that with a session-scoped data_archive fixture for readonly tests, but probably its not necessary for this change. Might be worth doing separately to make the tests significantly faster though

@olsen232 olsen232 merged commit e12b56d into master Apr 21, 2020
@olsen232 olsen232 deleted the empty_tree branch April 21, 2020 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sno show/diff doesn't work for initial import commits
2 participants