Revert should clean up repository state when there is nothing to revert by jamill · Pull Request #816 · libgit2/libgit2sharp

jamill · 2014-09-05T21:19:09Z

Currently - when calling Revert that results in no changes to revert, the repository is left in a reverting operation. From the command line - git will cleanup the operation state (unless you are reverting with the --no-commit option).

This change is to update Revert to follow that behavior. One thing I considered was adding a RevertStatus.NothingToCommit, as it might be unexpected to get a RevertSatus.Reverted result when nothing was reverted. But I think that is not needed as this can be inferred from the result (RevertStatus.Reverted + null commit indicates that there was nothing to revert). In this case RevertStatus.Reverted would mean that these changes were already reverted (even if no new revert commit was generated).

nulltoken · 2014-09-06T11:10:31Z

I think I'd prefer the RevertStatus.NothingToCommit option or maybe even RevertStatus.NothingToRevert. Although that looks like a corner case, exposing an explicit status may be more user friendly.

jamill · 2014-09-07T02:24:52Z

@nulltoken - I added the RevertStatus.NothingToRevert status. I also added an additional commit to check that the current branch is not orphaned to this PR. Although it is a different topic than the original PR, it is a small commit and touches some of the same files.

nulltoken · 2014-09-07T11:49:48Z

I also added an additional commit to check that the current branch is not orphaned to this PR.

❤️ it!

nulltoken · 2014-09-07T11:58:47Z

I'm not sure I follow the reasoning behind this (However, I'm not a hardcore Reverter, so there's that 😉).

Providing we're reverting once again the same commit, there's nothing to actually do, the workdir and the index would stay untouched. Thus, I don't understand why the RevertStatus would change depending on the commitOnSuccess value.

Similarly, why would the repo.Info.CurrentOperation be set to Revert if no change to the index or workdir has been applied?

/cc @carlosmn @ethomson @dahlbyk

So, I think that there are two concerns here:

The status returned when you ask to not commit and there's nothing to revert. I agree here with @nulltoken that it's odd that the status changes depending on whether you intend to commit or not. I would expect to get a RevertStatus.NothingToRevert whether I asked it to be committed or not.

Whether repo.Info.CurrentOperation be set to Revert if no change to the index or workdir has been applied? I think this makes sense, so that one could reasonably emulategit revert --no-commit` using LibGit2Sharp.

@jamill Do you foresee any issue returning RevertStatus.NothingToRevert in both cases?

I see - the way I thought about that status was that it would convey what the operation did. When asked to commit the revert, and there was nothing to revert, effectively nothing changed. In the case where you do not want the commit reverted, then we still do something for you - we have put you in the "revert" state... so, we did what you requested and the state of the repo is changed.

So, RevertStatus.NothingToRevert is an indication that the operation resulted in no changes, while the RevertStatus.Reverted indicates that we changed the state of the repository, rather than a statement of what changes were reverted.

@jamill Do you foresee any issue returning RevertStatus.NothingToRevert in both cases?

No... we could do it that way as well.

So, RevertStatus.NothingToRevert is an indication that the operation resulted in no changes, while the RevertStatus.Reverted indicates that we changed the state of the repository

Oh that's interesting. I was looking at it like "nothing to commit", just indicating that no work was performed. I had just inferred that the repository was still in the reverting state.

jamill · 2014-09-07T14:34:12Z

@nulltoken That is a great question 😄

Partly, this is because this is how the command line behaves. If you revert from the command line and specify the --no-commit option, then it will leave you in the reverting state, even if the revert resulted in no changes. If you revert without the --no-commit option, then it will clean up state for you. libgit2 also will leave you in the reverting state at the end of , even if there are no changes. I don't have a good scenario as to when you would want Revert with no changes to leave you in the revert state - but maybe if you wanted to revert a commit where the revert logic didn't properly revert the changes (so, it improperly left you with no changes)?

As to the part of RevertStatus value depending on CommitOnSuccess, this is partly why I originally decided to always return RevertStatus.Reverted. I could go either way - but as the caller will have to check for either a null value or the RevertStatus, I think this current approach is fine. The RevertStatus is not a boolean success or failure indication.

nulltoken · 2014-09-07T17:35:11Z

@peff I'd be very grateful if you could help us with this reverting use case. 🙏

jamill · 2014-09-09T13:48:26Z

@nulltoken Which alternate behaviors are you considering? Having revert always clean up itself if there are no changes (even when you asked it to not commit the result)?

ethomson · 2014-09-09T20:15:31Z

"the revert"

jamill · 2014-09-10T02:53:49Z

@nulltoken

Do you foresee any issue returning RevertStatus.NothingToRevert in both cases?

This has been updated to return RevertStatus.NothingToRevert in both cases 💥

nulltoken · 2014-09-10T05:41:40Z

Revert was run, but there were no changes to commit.

If reverting with the option to commit on success, and the revert completes successfully but there are no changes to commit, then revert will clean up the revert operation in progress state and the RevertStatus will indicate that there was nothing to revert.

nulltoken · 2014-09-10T06:02:08Z

🎱

jamill force-pushed the jamill/revert_with_nothing_to_revert branch 2 times, most recently from 4de6fc2 to 725d366 Compare September 7, 2014 02:20

jamill force-pushed the jamill/revert_with_nothing_to_revert branch from 725d366 to afc0fdb Compare September 7, 2014 02:32

nulltoken reviewed Sep 7, 2014
View reviewed changes

ethomson reviewed Sep 9, 2014
View reviewed changes

Comment thread LibGit2Sharp/RevertOptions.cs Outdated

ethomson Sep 9, 2014

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

"the revert"

jamill force-pushed the jamill/revert_with_nothing_to_revert branch from afc0fdb to 4dcb904 Compare September 10, 2014 02:50

nulltoken reviewed Sep 10, 2014
View reviewed changes

jamill added 2 commits September 10, 2014 07:45

Revert should throw if HEAD branch is orphaned

329c652

nulltoken force-pushed the jamill/revert_with_nothing_to_revert branch from 4dcb904 to 329c652 Compare September 10, 2014 05:45

nulltoken merged commit 329c652 into vNext Sep 10, 2014

nulltoken added this to the v0.20 milestone Sep 10, 2014

nulltoken deleted the jamill/revert_with_nothing_to_revert branch September 10, 2014 06:01

Uh oh!

Conversation

jamill commented Sep 5, 2014

Uh oh!

nulltoken commented Sep 6, 2014

Uh oh!

jamill commented Sep 7, 2014

Uh oh!

nulltoken commented Sep 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented Sep 7, 2014

Uh oh!

nulltoken commented Sep 7, 2014

Uh oh!

jamill commented Sep 9, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented Sep 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Sep 10, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants