GNOME Bugzilla – Bug 705979
Add 'ostree reset' command to undo a bad commit
Last modified: 2013-08-16 16:33:35 UTC
Need a simple way to undo a bad commit. I think that one can use write-refs for this, but that's very awkward, and doesn't do much sanity checking.
Created attachment 251604 [details] [review] Add 'ostree reset' command to undo a bad commit Accepts the following arguments: ref checksum Checks that the checksum is a parent of the ref before rewriting the ref.
Review of attachment 251604 [details] [review]: There's also the "refs" builtin; maybe we could take inspriation from git's "tag" command which does listing, creation, and deletion. Unlike git, ostree should make it easy to delete commits, since it's intended for binaries derived from source code, and you should always be able to rebuild. # List refs $ ostree ref gnome-ostree/buildmaster/x86_64-runtime gnome-ostree/buildmaster/x86_64-devel-debug ... # Create stable tags manually $ ostree ref gnome-ostree/stable/x86_64-runtime gnome-ostree/buildmaster/x86_64-runtime # Revert $ ostree ref gnome-ostree/stable/x86_64-runtime gnome-ostree/stable/x86_64-runtime^ # Delete $ ostree ref -d gnome-ostree/stable/x86_64-runtime Ok, the revert case could be nicer...maybe something like: # Create $ ostree ref -c gnome-ostree/stable/x86_64-runtime gnome-ostree/buildmaster/x86_64-runtime # Revert to parent $ ostree ref --revert gnome-ostree/stable/x86_64-runtime ? Now "refs" at the moment supports prefix matching; maybe that could be: # List all refs matching this prefix $ ostree ref --prefix=gnome-ostree/stable/x86_64-runtime # Delete all refs matching this prefix $ ostree ref --prefix=gnome-ostree/ -d
There are two things I don't like about "reset": 1) ostree already has two commands for manipulating refs (refs and write-refs), this would be a third special case. Admittedly the command line is not super well thought out, but I'd like to try to improve that. 2) It doesn't do anything like what git's "reset" command does.
(In reply to comment #2) > Review of attachment 251604 [details] [review]: > > There's also the "refs" builtin; maybe we could take inspriation from git's > "tag" command which does listing, creation, and deletion. Unlike git, ostree > should make it easy to delete commits, since it's intended for binaries derived > from source code, and you should always be able to rebuild. > > # List refs > $ ostree ref > gnome-ostree/buildmaster/x86_64-runtime > gnome-ostree/buildmaster/x86_64-devel-debug > ... > # Create stable tags manually > $ ostree ref gnome-ostree/stable/x86_64-runtime > gnome-ostree/buildmaster/x86_64-runtime > # Revert > $ ostree ref gnome-ostree/stable/x86_64-runtime > gnome-ostree/stable/x86_64-runtime^ > # Delete > $ ostree ref -d gnome-ostree/stable/x86_64-runtime > > Ok, the revert case could be nicer...maybe something like: > > # Create > $ ostree ref -c gnome-ostree/stable/x86_64-runtime > gnome-ostree/buildmaster/x86_64-runtime I have no idea what this is doing. I guess I just don't understand ostree enough. I'm trying to revert an 'ostree commit' > # Revert to parent > $ ostree ref --revert gnome-ostree/stable/x86_64-runtime But since you understand this problem space better, I don't mind stepping aside and waiting for a more complete solution from you.
(In reply to comment #3) > 2) It doesn't do anything like what git's "reset" command does. Hmmm. It does something pretty much exactly like the 'git reset' I use daily. 'git reset' moves the HEAD ref back to a specific ancestor commit. eg: git reset HEAD~1 If you don't specify --hard, then *in addition* it takes the changes and puts them in your working tree. Obviously since ostree doesn't have the concept of a working tree, this aspect doesn't apply.
Created attachment 251653 [details] [review] Add 'ostree reset' command to undo a bad commit Adding a rebased patch for use in the mean time.
Review of attachment 251604 [details] [review]: ::: src/ostree/ot-builtin-reset.c @@ +35,3 @@ + const gchar *ref, + GCancellable *cancellable, + GError **error) Why can't we just use ostree_repo_resolve_rev()? @@ +43,3 @@ + return NULL; + ret = g_strdup (g_hash_table_lookup (refs, ref)); + g_hash_table_unref (refs); There is gs_unref_hashtable btw. @@ +108,3 @@ + + if (!ostree_validate_checksum_string (checksum, error)) + goto out; Why limit the target to a checksum? ostree does support the ^ character at least, which is equivalent to ~1. (And I'd happily add support for the ~ specifier). You should be able to just call ostree_repo_resolve_rev() here. @@ +112,3 @@ + current = find_current_ref (repo, ref, cancellable, error); + if (current == NULL) + goto out; (and here)
Created attachment 251686 [details] [review] Add 'ostree reset' command to undo a bad commit Accepts the following arguments: ref checksum Checks that the checksum is a parent of the ref before rewriting the ref.
By the way, I'm not trying to push this through at all costs. If you want to take the different approach, I'm fine with that. In the meantime, though I just thought I'd make the patch useful so I can use it in my development. (In reply to comment #7) > Review of attachment 251604 [details] [review]: > > ::: src/ostree/ot-builtin-reset.c > @@ +35,3 @@ > + const gchar *ref, > + GCancellable *cancellable, > + GError **error) > > Why can't we just use ostree_repo_resolve_rev()? Because we really want the first 'ostree reset' argument to be an existing ref, not a checksum, and not a ^ ref. > @@ +108,3 @@ > + > + if (!ostree_validate_checksum_string (checksum, error)) > + goto out; > > Why limit the target to a checksum? ostree does support the ^ character at > least, which is equivalent to ~1. (And I'd happily add support for the ~ > specifier). > > You should be able to just call ostree_repo_resolve_rev() here. True. > @@ +112,3 @@ > + current = find_current_ref (repo, ref, cancellable, error); > + if (current == NULL) > + goto out; > > (and here) See above.
Comment on attachment 251686 [details] [review] Add 'ostree reset' command to undo a bad commit (In reply to comment #9) > By the way, I'm not trying to push this through at all costs. If you want to > take the different approach, I'm fine with that. In the meantime, though I just > thought I'd make the patch useful so I can use it in my development. I think I'll be happy for now if I just kill "write-refs" which was a performance hack for gnome-ostree; but now that recently ostree became a public introspectable library, I can just have gnome-ostree use the library directly. After that I'll think about what to do with "refs". > (In reply to comment #7) > > Review of attachment 251604 [details] [review] [details]: > > > > ::: src/ostree/ot-builtin-reset.c > > @@ +35,3 @@ > > + const gchar *ref, > > + GCancellable *cancellable, > > + GError **error) > > > > Why can't we just use ostree_repo_resolve_rev()? > > Because we really want the first 'ostree reset' argument to be an existing ref, > not a checksum, and not a ^ ref. Ok, please commit then with just the other change. Thanks!
Attachment 251686 [details] pushed as f9b2c45 - Add 'ostree reset' command to undo a bad commit