Add support for "git revert"

koppor commented 3 months ago

Version

0.12.16

Operating System

Windows

Distribution Method

msi (Windows)

Describe the issue

I ran git revert {some-commit-id}. The revert was correctly added to the virtual branch. However, there was another branch created.

Now I cannot work with git butler any more, because it complains

The given reference name 'refs/gitbut1er/Revert-"Remove-
comments---don't-work-on-forks"-This-reverts-commit-
a3ab7d5c36e5b6d212a5de8b87Ø6b81d7ec9362e.' is not valid;
class-Reference (4); coae=Inva1idSpec (-12)

grafik

How to reproduce

Use git revert {some comit it}

Expected behavior

The virtual branch Revert "..." is not created at all.

Relevant log output

No response

mtsgrd commented 3 months ago

@Byron I think you've looked branch name normalization? There are sort of two problems here from a users perspective, first that a revert creates a new branch (but this is currently by design), but the invalid branch name is from what understand a new thing. The error message comes from libgit2: https://github.com/libgit2/libgit2/blob/503b66cf00ad7dca940148529f60b1a409ccc462/src/libgit2/refs.c#L1017

Byron commented 3 months ago

Indeed, this has been improved and it will now fail early, during normalization, as it detects invalid branch names before they are used (like is the case with the error message indicating git2 tried to create or update a branch of that name`).

I even was able to reproduce the issue, but could also recover from it with Version 0.12.16 (20240730.095735) doing the following:

run git revert @ on with gitbutler/integration checked out
refresh two times with Cmd+R
find the new virtual branch 'magically' created

When deleting it, there is an error message, but the deletion works. Trying to rename it fails as it can't do anything with the incorrect branch name. It's interesting that the branch-name normalization doesn't kick-in at all, it's not called it seems as it wouldn't be able to build a branch named refs/gitbutler/Revert-"GitButler-Integration-Commit"-This-reverts-commit-d6efa5fd96d36da445d5d1345b84163f05f5f229. with it.

And I put a test down and it's clear that the branch name it normalizes passes: Revert-"GitButler-Integration-Commit"-This-reverts-commit-d6efa5fd96d36da445d5d1345b84163f05f5f229.. And indeed, it's a valid branch name as far as git is concerned.

git also thinks it's invalid:

❯ git branch 'Revert-"GitButler-Integration-Commit"-This-reverts-commit-d6efa5fd96d36da445d5d1345b84163f05f5f229.'
fatal: 'Revert-"GitButler-Integration-Commit"-This-reverts-commit-d6efa5fd96d36da445d5d1345b84163f05f5f229.' is not a valid branch name

The culprit seems to be the trailing .! Let me fix that in gitoxide, which strangely let's it pass even though I thought I modelled this after the Git source code.

Byron commented 3 months ago

I made some improvements and now the trailing . is forbidden as well. The code I altered was from 2021, and I even had a passing test claiming that trailing . should work. Apparently, Git changed its stance over time.

As CI takes a moment, I will integrate the changes with GitButler tomorrow and see what will happen then when repeating this workflow.

Byron commented 3 months ago

With the PR merged, the overall issue is unchanged except that the error is clearer, with something like Could not turn "Revert-\"GitButler-Integration-Commit\"-This-reverts-commit-d6efa5fd96d36da445d5d1345b84163f05f5f229." into a valid reference name.

Since the virtual-branch creation is intended, I think this issue can be resolved specifically if the sanitization is improved. Having looked at the Git codebase, which has optional sanitization built-in to the validation, a proper and permanent fix could be achieved by providing this feature with gitoxide. This way, everything it would complain about it could optionally fix, making branch normalization work, always.

As implementing this will be a bit more effort, I will submit another PR to just fix the specific problem here, the tailing ., and see if this works better, overall.

Byron commented 3 months ago

Now that normalization succeeds, it looks better, here is how this looks like

run git revert @ in the workspace
observe that nothing seems to change in the UI
refresh the UI with Cmd + R and the new revert… virtual branch shows up with just the revert commit in it
it will also display changes that are in the index only
deleting the newly create virtual branch works, which nearly returns to the previous state

It's notable that the above would only work after I removed all the stray revert virtual branches directly in the .git/gitbutler/virtual_branches.toml file - before that it was in quite an invalid state for me and I even ended up with a virtual branch that showed the gitbutler/integration commit.

Please also note that all the improvements done so far are probably not satisfactory from a UX perspective.

nolith commented 1 month ago

I experienced this situation yesterday.

Is there a way we can make use of git revert COMMIT_SHA today? Even if it may require some workaround?

I had a very unexpected result in one of my testing, so I am wondering if I should avoid this in GB and spin up a git worktree add instead for working on reverts

Byron commented 1 month ago

Hiding the git revert operation by performing it in a separate worktree seems like a viable workaround for this, despite the effort it unfortunately takes to do so.

Maybe it would be easier to close GB while performing the git revert operation and place it in a separate branch, one that can then be applied into the GB workspace.

I hope one of these options works for you.

gitbutlerapp / gitbutler