"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
removing got rebase/histedit backups
To:
gameoftrees@openbsd.org
Date:
Thu, 15 Jul 2021 16:02:14 +0200

Download raw body.

Thread
  • Stefan Sperling:

    removing got rebase/histedit backups

Following up on the discussion about 'gotadmin cleanup' and how
references could be deleted:

My initial idea was to offer an interface via 'got ref', such as:
    got ref -d -R refs/got/backups

I am concerned about the possibility of somebody running commands
like 'got ref -d -R refs'. This would delete all references except
the symbolic HEAD reference and render the repository unusable.
Having a command which can break the repository is asking for trouble.
And trying to catch mistakes on the user's command line is not trivial.
So I would rather avoid adding recursive removal to 'got ref'.
If someone wants to break a repository in this way, they'll have to
delete all the references one-by-one ;)

My new plan is to add removal functionality to specific commands which
create a lot of references. The patch below starts off with 'got rebase'
and 'got histedit'.

Regarding the option name, I want an option letter which is not yet in
use by any command. This way, other commands can use the same option letter
when they add the same feature. One example would be 'got fetch'. We may want
to delete references under "refs/remotes/" which belong to a remote repository
that is no longer reachable, for example.
I ended up choosing the option -X, which is not used anywhere yet.

So we could now remove backups with:
	
	got histedit -X

And this also works for specific branches only:

	got histedit -X main

The patch is relatively small because my implementation re-uses the
logic which is already present for listing existing backups.

The commands print all references which were deleted and their values.
This makes it possible to re-create references which were deleted in
error, as long as the output is preserved.

I've made a small related change to check for cancellation while
looping over references. This catches the case where someone changes
their mind after starting a -X command and hits Ctrl-C.

ok?

diff 739c7757611e77d1f83847e58167b8df51d19cb8 85ae0b977286b125144bddd85552518464382e97
blob - 905badf9f77075dc5c72caa5308358990d308e9b
blob + 64eb89a0f0d09c013a42191b548de64e1ecc96c7
--- got/got.1
+++ got/got.1
@@ -1437,7 +1437,7 @@ conflicts must be resolved first.
 .It Cm bo
 Short alias for
 .Cm backout .
-.It Cm rebase Oo Fl a Oc Oo Fl c Oc Oo Fl l Oc Op Ar branch
+.It Cm rebase Oo Fl a Oc Oo Fl c Oc Oo Fl l Oc Oo Fl X Oc Op Ar branch
 Rebase commits on the specified
 .Ar branch
 onto the tip of the current branch of the work tree.
@@ -1585,11 +1585,34 @@ If this option is used,
 does not require a work tree.
 None of the other options can be used together with
 .Fl l .
+.It Fl X
+Delete backups created by past rebase operations, represented by references
+in the
+.Dq refs/got/backup/rebase
+reference namespace.
+.Pp
+If a
+.Ar branch
+is specified, only delete backups which at some point in time represented
+this branch.
+Otherwise, delete all references found within
+.Dq refs/got/backup/rebase .
+.Pp
+Any commit, tree, tag, and blob objects belonging to deleted backups
+remain in the repository and may be removed separately with
+Git's garbage collector or
+.Cm gotadmin cleanup .
+.Pp
+If this option is used,
+.Cm got rebase
+does not require a work tree.
+None of the other options can be used together with
+.Fl X .
 .El
 .It Cm rb
 Short alias for
 .Cm rebase .
-.It Cm histedit Oo Fl a Oc Oo Fl c Oc Oo Fl f Oc Oo Fl F Ar histedit-script Oc Oo Fl m Oc Oo Fl l Oc Op Ar branch
+.It Cm histedit Oo Fl a Oc Oo Fl c Oc Oo Fl f Oc Oo Fl F Ar histedit-script Oc Oo Fl m Oc Oo Fl l Oc Oo Fl X Oc Op Ar branch
 Edit commit history between the work tree's current base commit and
 the tip commit of the work tree's current branch.
 .Pp
@@ -1785,6 +1808,29 @@ If this option is used,
 does not require a work tree.
 None of the other options can be used together with
 .Fl l .
+.It Fl X
+Delete backups created by past histedit operations, represented by references
+in the
+.Dq refs/got/backup/histedit
+reference namespace.
+.Pp
+If a
+.Ar branch
+is specified, only delete backups which at some point in time represented
+this branch.
+Otherwise, delete all references found within
+.Dq refs/got/backup/histedit .
+.Pp
+Any commit, tree, tag, and blob objects belonging to deleted backups
+remain in the repository and may be removed separately with
+Git's garbage collector or
+.Cm gotadmin cleanup .
+.Pp
+If this option is used,
+.Cm got histedit
+does not require a work tree.
+None of the other options can be used together with
+.Fl X .
 .El
 .It Cm he
 Short alias for
blob - 36887d987d05638e632528887359fa9921003127
blob + 8dc4d8f2f703bc7e275e8efeece9dd5690dc546f
--- got/got.c
+++ got/got.c
@@ -7518,7 +7518,7 @@ done:
 __dead static void
 usage_rebase(void)
 {
-	fprintf(stderr, "usage: %s rebase [-a] [-c] [-l] [branch]\n",
+	fprintf(stderr, "usage: %s rebase [-a] [-c] [-l] [-X] [branch]\n",
 	    getprogname());
 	exit(1);
 }
@@ -7834,6 +7834,27 @@ done:
 }
 
 static const struct got_error *
+delete_backup_ref(struct got_reference *ref, struct got_object_id *id,
+    struct got_repository *repo)
+{
+	const struct got_error *err;
+	char *id_str;
+
+	err = got_object_id_str(&id_str, id);
+	if (err)
+		return err;
+
+	err = got_ref_delete(ref, repo);
+	if (err)
+		goto done;
+
+	printf("Deleted %s: %s\n", got_ref_get_name(ref), id_str);
+done:
+	free(id_str);
+	return err;
+}
+
+static const struct got_error *
 print_backup_ref(const char *branch_name, const char *new_id_str,
     struct got_object_id *old_commit_id, struct got_commit_object *old_commit,
     struct got_reflist_object_id_map *refs_idmap,
@@ -7929,8 +7950,8 @@ done:
 }
 
 static const struct got_error *
-list_backup_refs(const char *backup_ref_prefix, const char *wanted_branch_name,
-    struct got_repository *repo)
+process_backup_refs(const char *backup_ref_prefix, const char *wanted_branch_name,
+    int delete, struct got_repository *repo)
 {
 	const struct got_error *err;
 	struct got_reflist_head refs, backup_refs;
@@ -7967,6 +7988,10 @@ list_backup_refs(const char *backup_ref_prefix, const 
 		const char *refname = got_ref_get_name(re->ref);
 		char *slash;
 
+		err = check_cancelled(NULL);
+		if (err)
+			break;
+
 		err = got_ref_resolve(&old_commit_id, repo, re->ref);
 		if (err)
 			break;
@@ -7997,8 +8022,14 @@ list_backup_refs(const char *backup_ref_prefix, const 
 		if (wanted_branch_name == NULL ||
 		    strcmp(wanted_branch_name, branch_name) == 0) {
 			wanted_branch_found = 1;
-			err = print_backup_ref(branch_name, refname,
-			   old_commit_id, old_commit, refs_idmap, repo);
+			if (delete) {
+				err = delete_backup_ref(re->ref,
+				    old_commit_id, repo);
+			} else {
+				err = print_backup_ref(branch_name, refname,
+				   old_commit_id, old_commit, refs_idmap,
+				   repo);
+			}
 			if (err)
 				break;
 		}
@@ -8043,6 +8074,7 @@ cmd_rebase(int argc, char *argv[])
 	struct got_commit_object *commit = NULL;
 	int ch, rebase_in_progress = 0, abort_rebase = 0, continue_rebase = 0;
 	int histedit_in_progress = 0, create_backup = 1, list_backups = 0;
+	int delete_backups = 0;
 	unsigned char rebase_status = GOT_STATUS_NO_CHANGE;
 	struct got_object_id_queue commits;
 	struct got_pathlist_head merged_paths;
@@ -8052,7 +8084,7 @@ cmd_rebase(int argc, char *argv[])
 	STAILQ_INIT(&commits);
 	TAILQ_INIT(&merged_paths);
 
-	while ((ch = getopt(argc, argv, "acl")) != -1) {
+	while ((ch = getopt(argc, argv, "aclX")) != -1) {
 		switch (ch) {
 		case 'a':
 			abort_rebase = 1;
@@ -8063,6 +8095,9 @@ cmd_rebase(int argc, char *argv[])
 		case 'l':
 			list_backups = 1;
 			break;
+		case 'X':
+			delete_backups = 1;
+			break;
 		default:
 			usage_rebase();
 			/* NOTREACHED */
@@ -8082,8 +8117,19 @@ cmd_rebase(int argc, char *argv[])
 			option_conflict('l', 'a');
 		if (continue_rebase)
 			option_conflict('l', 'c');
+		if (delete_backups)
+			option_conflict('l', 'X');
 		if (argc != 0 && argc != 1)
 			usage_rebase();
+	} else if (delete_backups) {
+		if (abort_rebase)
+			option_conflict('X', 'a');
+		if (continue_rebase)
+			option_conflict('X', 'c');
+		if (list_backups)
+			option_conflict('l', 'X');
+		if (argc != 0 && argc != 1)
+			usage_rebase();
 	} else {
 		if (abort_rebase && continue_rebase)
 			usage_rebase();
@@ -8101,7 +8147,7 @@ cmd_rebase(int argc, char *argv[])
 	}
 	error = got_worktree_open(&worktree, cwd);
 	if (error) {
-		if (list_backups) {
+		if (list_backups || delete_backups) {
 			if (error->code != GOT_ERR_NOT_WORKTREE)
 				goto done;
 		} else {
@@ -8122,9 +8168,10 @@ cmd_rebase(int argc, char *argv[])
 	if (error)
 		goto done;
 
-	if (list_backups) {
-		error = list_backup_refs(GOT_WORKTREE_REBASE_BACKUP_REF_PREFIX,
-		    argc == 1 ? argv[0] : NULL, repo);
+	if (list_backups || delete_backups) {
+		error = process_backup_refs(
+		    GOT_WORKTREE_REBASE_BACKUP_REF_PREFIX,
+		    argc == 1 ? argv[0] : NULL, delete_backups, repo);
 		goto done; /* nothing else to do */
 	}
 
@@ -8359,7 +8406,8 @@ __dead static void
 usage_histedit(void)
 {
 	fprintf(stderr, "usage: %s histedit [-a] [-c] [-f] "
-	    "[-F histedit-script] [-m] [-l] [branch]\n", getprogname());
+	    "[-F histedit-script] [-m] [-l] [-X] [branch]\n",
+	    getprogname());
 	exit(1);
 }
 
@@ -9193,7 +9241,7 @@ cmd_histedit(int argc, char *argv[])
 	struct got_update_progress_arg upa;
 	int edit_in_progress = 0, abort_edit = 0, continue_edit = 0;
 	int edit_logmsg_only = 0, fold_only = 0;
-	int list_backups = 0;
+	int list_backups = 0, delete_backups = 0;
 	const char *edit_script_path = NULL;
 	unsigned char rebase_status = GOT_STATUS_NO_CHANGE;
 	struct got_object_id_queue commits;
@@ -9208,7 +9256,7 @@ cmd_histedit(int argc, char *argv[])
 	TAILQ_INIT(&merged_paths);
 	memset(&upa, 0, sizeof(upa));
 
-	while ((ch = getopt(argc, argv, "acfF:ml")) != -1) {
+	while ((ch = getopt(argc, argv, "acfF:mlX")) != -1) {
 		switch (ch) {
 		case 'a':
 			abort_edit = 1;
@@ -9228,6 +9276,9 @@ cmd_histedit(int argc, char *argv[])
 		case 'l':
 			list_backups = 1;
 			break;
+		case 'X':
+			delete_backups = 1;
+			break;
 		default:
 			usage_histedit();
 			/* NOTREACHED */
@@ -9269,8 +9320,25 @@ cmd_histedit(int argc, char *argv[])
 			option_conflict('l', 'm');
 		if (fold_only)
 			option_conflict('l', 'f');
+		if (delete_backups)
+			option_conflict('l', 'X');
 		if (argc != 0 && argc != 1)
 			usage_histedit();
+	} else if (delete_backups) {
+		if (abort_edit)
+			option_conflict('X', 'a');
+		if (continue_edit)
+			option_conflict('X', 'c');
+		if (edit_script_path)
+			option_conflict('X', 'F');
+		if (edit_logmsg_only)
+			option_conflict('X', 'm');
+		if (fold_only)
+			option_conflict('X', 'f');
+		if (list_backups)
+			option_conflict('X', 'l');
+		if (argc != 0 && argc != 1)
+			usage_histedit();
 	} else if (argc != 0)
 		usage_histedit();
 
@@ -9290,7 +9358,7 @@ cmd_histedit(int argc, char *argv[])
 	}
 	error = got_worktree_open(&worktree, cwd);
 	if (error) {
-		if (list_backups) {
+		if (list_backups || delete_backups) {
 			if (error->code != GOT_ERR_NOT_WORKTREE)
 				goto done;
 		} else {
@@ -9301,7 +9369,7 @@ cmd_histedit(int argc, char *argv[])
 		}
 	}
 
-	if (list_backups) {
+	if (list_backups || delete_backups) {
 		error = got_repo_open(&repo,
 		    worktree ? got_worktree_get_repo_path(worktree) : cwd,
 		    NULL);
@@ -9311,9 +9379,9 @@ cmd_histedit(int argc, char *argv[])
 		    worktree ? got_worktree_get_root_path(worktree) : NULL);
 		if (error)
 			goto done;
-		error = list_backup_refs(
+		error = process_backup_refs(
 		    GOT_WORKTREE_HISTEDIT_BACKUP_REF_PREFIX,
-		    argc == 1 ? argv[0] : NULL, repo);
+		    argc == 1 ? argv[0] : NULL, delete_backups, repo);
 		goto done; /* nothing else to do */
 	}
 
blob - b6f5b85dd337c44468c9ff11b1cf6976705b46fb
blob + 15eedc6e738a474075643efa1fcd4692f2e2cec1
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -171,7 +171,38 @@ EOF
 	ret="$?"
 	if [ "$ret" != "0" ]; then
 		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
 	fi
+
+	(cd $testroot/repo && got histedit -X master \
+		> $testroot/stdout 2> $testroot/stderr)
+	echo -n "Deleted refs/got/backup/histedit/master/$new_commit2: " \
+		> $testroot/stdout.expected
+	echo "$old_commit2" >> $testroot/stdout.expected
+	echo -n > $testroot/stderr.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/repo && got histedit -l > $testroot/stdout)
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
 	test_done "$testroot" "$ret"
 }
 
blob - a1f81c4a5bbd0d9e60e07289ec4e53d119b78dd6
blob + d590abf4efb9a74b11e35c01ec82cfdfad3c7893
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -186,12 +186,45 @@ EOF
 	if [ "$ret" != "0" ]; then
 		diff -u $testroot/stdout.expected $testroot/stdout
 		test_done "$testroot" "$ret"
+		return 1
 	fi
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret="$?"
 	if [ "$ret" != "0" ]; then
 		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
 	fi
+
+	# Delete all backup refs
+	(cd $testroot/repo && got rebase -X \
+		> $testroot/stdout 2> $testroot/stderr)
+	echo -n "Deleted refs/got/backup/rebase/newbranch/$new_commit2: " \
+		> $testroot/stdout.expected
+	echo "$orig_commit2" >> $testroot/stdout.expected
+	echo -n > $testroot/stderr.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/repo && got rebase -l > $testroot/stdout)
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
 	test_done "$testroot" "$ret"
 }