Skip to content

Conversation

@ethomson
Copy link
Member

Currently, if you try Repository.Reset() when you have renamed entries, it chokes as Index.Reset expects only certain types of ChangeKinds. It's easiest to treat renamed entries exactly like git core does, which is to say to treat them as an add and a delete in git-reset and allow you to reset each side independently (which then breaks the facade of a rename).

Copy link
Member

Choose a reason for hiding this comment

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

small request: could you include a comment about what commit "32eab9c" is? Is this the Tip commit of the standard test repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is at present; but I think that future commits to that repo would make that comment outdated quickly. (The other Reset tests use an older commit which I suspect was tip when they were written.)

Copy link
Member

Choose a reason for hiding this comment

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

If that were to happen, I imagine this test would need to be updated anyways, as you would be reseting the index to a commit other than HEAD, and it would probably have other staged changes at that point?

Anyway, not a big deal. I do think a comment describing what the intent of why you are using that specific commit might help when looking at this code in the future.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW there are many tests that depend on the current repo.Head.Tip, including some Reset() tests. No harm in depending on it here too, IMO.

@jamill
Copy link
Member

jamill commented Jun 20, 2014

👍 Thanks @ethomson!

@ethomson
Copy link
Member Author

One thing to point out is that you can reset to prior commits, so adding another commit in that repository, or moving HEAD forward, will not result in these tests failing. (I'm sure that you could create some HEAD that did something evil to one of the files in question. But repo.Head.Tip wouldn't really help in this scenario.) This is why all the tests reset to the explicit commit that they're interested in, instead of just throwing out the Head commit. When the test repo is updated, as happens occasionally, this makes it easier to reason about, I think. This is obviously up for debate in the explicit commit vs repo.Head.Tip camp.

Anyway. I rebased this.

/cc @nulltoken

@nulltoken nulltoken merged commit 2378a56 into libgit2:vNext Jun 23, 2014
@nulltoken
Copy link
Member

Anyway. I rebased this.

And I merged it. A thousand thanks! ✨

@nulltoken nulltoken added this to the v0.19.0 milestone Aug 24, 2014
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.

4 participants