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

Topic commit deletion #25

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft

Topic commit deletion #25

wants to merge 22 commits into from

Conversation

forgottosave
Copy link
Collaborator

@forgottosave forgottosave commented Nov 15, 2024

Description

This is a follow-up PR of #21. It implements the possibility to delete commits from branches.

Features

  • Drag-and-drop commits onto a trash-bin to delete them
  • Keep keyboard-only control with new key-bindings:
    • arrow keys for commit selection
    • d for deletion
  • Ask for confirmation of actions before they are implemented

Work to do

  • implement a commit-drop in the cpplibostree
  • refactor the inPromotionMode to generally support different view modes (default, promotion, deletion)
  • add keyboard shortcut to activate commit deletion
  • add UI elements to support commit deletion
  • implement visual indicator for drag-and-drop to delete
  • code cleanup

@forgottosave forgottosave added state: 👷🏻 in development Currently worked on ✨ feature New feature or request labels Nov 15, 2024
@forgottosave forgottosave self-assigned this Nov 15, 2024
@forgottosave forgottosave linked an issue Nov 15, 2024 that may be closed by this pull request
2 tasks
@forgottosave forgottosave mentioned this pull request Nov 15, 2024
15 tasks
@forgottosave forgottosave changed the base branch from main to topic-commit_tree_rebuild November 15, 2024 21:21
@forgottosave forgottosave force-pushed the topic-commit_deletion branch 3 times, most recently from 0e00935 to e302d7d Compare November 18, 2024 16:33
@forgottosave forgottosave changed the base branch from topic-commit_tree_rebuild to main November 19, 2024 08:15
@forgottosave forgottosave added state: 👁️ review Ready for review and removed state: 👷🏻 in development Currently worked on labels Nov 25, 2024
src/core/CMakeLists.txt Show resolved Hide resolved
src/core/OSTreeTUI.hpp Outdated Show resolved Hide resolved
@forgottosave forgottosave removed the state: 👁️ review Ready for review label Nov 26, 2024
@forgottosave forgottosave added the state: 👷🏻 in development Currently worked on label Nov 26, 2024
@forgottosave forgottosave added state: 👁️ review Ready for review and removed state: 👷🏻 in development Currently worked on labels Nov 26, 2024
Copy link
Member

@COM8 COM8 left a comment

Choose a reason for hiding this comment

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

I think there is still a misunderstanding:
Screenshot from 2024-11-28 17-43-11

Based on the screenshoot if I would confirm, I would delete all commits and keep only the latest? This was not my intention.

I wanted to be able to delete any arbitrary single commit. Although it might be nice to delete commit ranges, I'd say this is out of scope for this PR.

Also there are a few warnings when build as release. A better approach instead of ignoring all warnings would be to convert all of them to errors (e.g. via -Werror) and then make exceptions for specific warnings like the deprecated one.

src/core/commit.cpp Outdated Show resolved Hide resolved
src/core/commit.cpp Outdated Show resolved Hide resolved
src/core/trashBin.cpp Outdated Show resolved Hide resolved
src/core/trashBin.cpp Outdated Show resolved Hide resolved
@COM8
Copy link
Member

COM8 commented Nov 28, 2024

Also clang tidy finds a lot of things that do not get classified as errors since -Werror is missing as compiler flag.
https://github.com/AP-Sensing/ostree-tui/actions/runs/12035951546/job/33556080420?pr=25

@forgottosave
Copy link
Collaborator Author

The problem with removing arbitrary single commits is that, by default, all commits preceding the deleted one do not belong to a branch anymore (as the recursive "parent-traverse" can't find those now orphaned commits anymore).

I guess what you would be looking for is something like a git stash to "delete" one commit, but keep the rest of the structure. I am, at the moment, not aware of how to do this in ostree.

@COM8
Copy link
Member

COM8 commented Nov 30, 2024

I also looked into this and you are right, ostree is not designed to drop arbitrary commits.
Would it be possible to:

  • Keep the current behaviour of dropping the current and all preceding commits.
  • Extend it to also add the possibility to drop the latest commit (like you had it before).

With that and the move commit between branches I'm able to do everything I need.
How much work would this be?

@COM8
Copy link
Member

COM8 commented Nov 30, 2024

An other question:
If I drop commits, does it also prune the commit data? Or does it only drop the commit ref?
e.g.:

ostree --repo=/path/to/repo reset <branch> <previous-commit-hash>
# Make sure we clean up unused refs
ostree --repo=/path/to/repo prune --refs-only

@forgottosave
Copy link
Collaborator Author

forgottosave commented Nov 30, 2024

I also looked into this and you are right, ostree is not designed to drop arbitrary commits. Would it be possible to:

* Keep the current behaviour of dropping the current and all preceding commits.

* Extend it to also add the possibility to drop the latest commit (like you had it before).

With that and the move commit between branches I'm able to do everything I need. How much work would this be?

This is already implemented. If you delete the latest commit, only that one will be deleted. Delete any other commit and all preceding will be dropped, too. This is also visually indicated in the deletion-window, when deleting the latest (or not latest) commit.

That's what I didn't tell you before to see, if the current implementation is intuitive or not. I guess they could be separate delete-options as well, but I thought I might keep it simple & with less buttons and options, when combining both into one deletion feature.

@forgottosave
Copy link
Collaborator Author

An other question: If I drop commits, does it also prune the commit data? Or does it only drop the commit ref? e.g.:

ostree --repo=/path/to/repo reset <branch> <previous-commit-hash>
# Make sure we clean up unused refs
ostree --repo=/path/to/repo prune --refs-only

It always also prunes the deleted commit (and in the case that it is not the latest commit: all preceding commits as well).

src/util/cpplibostree.hpp Outdated Show resolved Hide resolved
src/util/cpplibostree.hpp Outdated Show resolved Hide resolved
src/util/cpplibostree.hpp Outdated Show resolved Hide resolved
@COM8
Copy link
Member

COM8 commented Nov 30, 2024

I also looked into this and you are right, ostree is not designed to drop arbitrary commits. Would it be possible to:

* Keep the current behaviour of dropping the current and all preceding commits.

* Extend it to also add the possibility to drop the latest commit (like you had it before).

With that and the move commit between branches I'm able to do everything I need. How much work would this be?

This is already implemented. If you delete the latest commit, only that one will be deleted. Delete any other commit and all preceding will be dropped, too. This is also visually indicated in the deletion-window, when deleting the latest (or not latest) commit.

That's what I didn't tell you before to see, if the current implementation is intuitive or not. I guess they could be separate delete-options as well, but I thought I might keep it simple & with less buttons and options, when combining both into one deletion feature.

Ah, perfect! Then the best we can do now is make sure the tool tip reflects this (I did not check if it already does).

@forgottosave
Copy link
Collaborator Author

forgottosave commented Nov 30, 2024

[...]

Ah, perfect! Then the best we can do now is make sure the tool tip reflects this (I did not check if it already does).

It doesn't yet, as tool-tips kinda went out the window when I eliminated the commit-promotion tab on the right. I may re-add them to the new promotion- and deletion-windows.

Although I might push that to another issue / PR.

@forgottosave forgottosave mentioned this pull request Nov 30, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature New feature or request state: 👁️ review Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Commits "Removeable"
2 participants