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

From:
Omar Polo <op@omarpolo.com>
Subject:
Re: got branch: support multiple -d options
To:
Stefan Sperling <stsp@stsp.name>
Cc:
Christian Weisgerber <naddy@mips.inka.de>, gameoftrees@openbsd.org
Date:
Fri, 22 Jul 2022 17:46:40 +0200

Download raw body.

Thread
resuming the thread.

Sorry Stefan for posting almost the same diff you sent back then, it
wasn't on purpose, i completely forgot about this thread and ended up
reinventing it to scratch an itch.

Omar Polo <op@omarpolo.com> wrote:
> Stefan Sperling <stsp@stsp.name> wrote:
> > On Sun, May 15, 2022 at 11:22:28PM +0200, Christian Weisgerber wrote:
> > > Stefan Sperling:
> > > 
> > > > If multiple -d options are passed to 'got branch' all arguments
> > > > except the last one end up being ignored.
> > > > 
> > > > It seems more useful to delete all specified branches instead.
> > > 
> > > Hmmm.  Should this behave like "ref -d", where the -d flag doesn't take
> > > an argument but switches to delete mode?
> > 
> > Or would it make sense to adjust 'got ref -d' to behave like
> > 'got send -d' and 'got branch -d' (after this patch)?
> 
> FWIW I'm fine either the way but after hearing naddy I think I agree
> with him.  Having -d as a "mode switch" allows to type something like
> 
> 	$ got branch -d foo bar baz ...
> 
> which is handier than spamming -d ... -d ... -d ...  but admittedly i
> don't remember how many times I wanted to delete more than one branch.
> 
> `got send -d' can't be tweaked to have -d as a "mode switch" because it
> needs already a positional argument for the remote to use, but that's
> probably fine as is.

even tho i just sent a diff that's almost identical to the one Stefan
sent, re-reading the thread convinced me that i agree with naddy and the
past myself.

There isn't much point in using -d in combo with other flags, and even
if it were it's currently disallowed.  on the other hand, a "mode
switch" -d is handy because one don't have to spam multiple flags when
cleaning up some temporary branches.

i'm attaching (yet another) diff that makes -d a switch.  the only
downside i see is that the description becomes a bit more weird since
branch allows only one positional argument unless -d is given.  I don't
see a way to make it cleaner in the usage string (i.e. `got branch [-d]
[name | branches...]' doesn't read well imho) so i just opted to add a
note that in case -d is provided, more than one branch name can be
given.  i'm still expecting that 90% of the usage will just be to delete
a single branch, no need to make the usage string overly complex.

note also that usage_branch already show [-d] and not [-d branch] as the
manpage did :)

> > Perhaps 'got ref' could remain as it is. Since it is not intended to
> > be used on a daily basis it doesn't need to provide much comfort.
> > Unlike the other tools it requires absolute refnames as input (which
> > start with "refs/"), and it does not have any safety checks. For instance,
> > 'got branch -d' will refuse to delete a branch that is used by the current
> > work tree. 'got ref -d' will delete the branch no matter what.

no opinions about got ref -d, but i agree that it doesn't need to
provide much comfort.

the churn in branch.sh is mostly because now that -d is a mode switch
(is it a real term or did i made that up?) the arguments needed to be
sorted.

diff /home/op/w/got
commit - c5b5bb9d8b7eb7e70c0ab60736b349128a267edc
path + /home/op/w/got
blob - 918d723a3556ec33343925c453566d54da948b5f
file + got/got.1
--- got/got.1
+++ got/got.1
@@ -1068,7 +1068,7 @@ Cannot be used together with any other options except
 .Fl r .
 .El
 .Tg br
-.It Cm branch Oo Fl c Ar commit Oc Oo Fl r Ar repository-path Oc Oo Fl l Oc Oo Fl t Oc Oo Fl d Ar name Oc Oo Fl n Oc Op Ar name
+.It Cm branch Oo Fl c Ar commit Oc Oo Fl r Ar repository-path Oc Oo Fl l Oc Oo Fl t Oc Oo Fl d Oc Oo Fl n Oc Op Ar name
 .Dl Pq alias: Cm br
 Create, list, or delete branches.
 .Pp
@@ -1155,7 +1155,7 @@ regardless.
 Use of this option requires the
 .Fl l
 option to be used as well.
-.It Fl d Ar name
+.It Fl d
 Delete the branch with the specified
 .Ar name
 from the
@@ -1163,6 +1163,9 @@ from the
 or
 .Dq refs/remotes
 reference namespace.
+When using this option, more than one branch
+.Ar name
+to delete can be provided.
 .Pp
 Only the branch reference is deleted.
 Any commit, tree, and blob objects belonging to the branch
blob - 009c0d06e2e5d8714a10c5c8a008771349f971b0
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -6675,7 +6675,8 @@ cmd_branch(int argc, char *argv[])
 	struct got_worktree *worktree = NULL;
 	char *cwd = NULL, *repo_path = NULL;
 	int ch, do_list = 0, do_show = 0, do_update = 1, sort_by_time = 0;
-	const char *delref = NULL, *commit_id_arg = NULL;
+	int do_delete = 0;
+	const char *commit_id_arg = NULL;
 	struct got_reference *ref = NULL;
 	struct got_pathlist_head paths;
 	struct got_pathlist_entry *pe;
@@ -6685,13 +6686,13 @@ cmd_branch(int argc, char *argv[])
 
 	TAILQ_INIT(&paths);
 
-	while ((ch = getopt(argc, argv, "c:d:r:lnt")) != -1) {
+	while ((ch = getopt(argc, argv, "c:dr:lnt")) != -1) {
 		switch (ch) {
 		case 'c':
 			commit_id_arg = optarg;
 			break;
 		case 'd':
-			delref = optarg;
+			do_delete = 1;
 			break;
 		case 'r':
 			repo_path = realpath(optarg, NULL);
@@ -6715,7 +6716,7 @@ cmd_branch(int argc, char *argv[])
 		}
 	}
 
-	if (do_list && delref)
+	if (do_list && do_delete)
 		option_conflict('l', 'd');
 	if (sort_by_time && !do_list)
 		errx(1, "-t option requires -l option");
@@ -6723,17 +6724,16 @@ cmd_branch(int argc, char *argv[])
 	argc -= optind;
 	argv += optind;
 
-	if (!do_list && !delref && argc == 0)
+	if (!do_list && !do_delete && argc == 0)
 		do_show = 1;
 
-	if ((do_list || delref || do_show) && commit_id_arg != NULL)
+	if ((do_list || do_delete || do_show) && commit_id_arg != NULL)
 		errx(1, "-c option can only be used when creating a branch");
 
-	if (do_list || delref) {
-		if (argc > 0)
-			usage_branch();
-	} else if (!do_show && argc != 1)
+	if ((do_list || do_show) && argc > 0)
 		usage_branch();
+	else if (do_delete && argc < 1)
+		usage_branch();
 
 #ifndef PROFILE
 	if (pledge("stdio rpath wpath cpath fattr flock proc exec "
@@ -6794,9 +6794,13 @@ cmd_branch(int argc, char *argv[])
 		error = show_current_branch(repo, worktree);
 	else if (do_list)
 		error = list_branches(repo, worktree, sort_by_time);
-	else if (delref)
-		error = delete_branch(repo, worktree, delref);
-	else {
+	else if (do_delete) {
+		for (; *argv; ++argv) {
+			error = delete_branch(repo, worktree, *argv);
+			if (error)
+				break;
+		}
+	} else {
 		struct got_reflist_head refs;
 		TAILQ_INIT(&refs);
 		error = got_ref_list(&refs, repo, NULL, got_ref_cmp_by_name,
blob - a025c5170abf06b7f707406ee2d9193c3fa6ff43
file + regress/cmdline/branch.sh
--- regress/cmdline/branch.sh
+++ regress/cmdline/branch.sh
@@ -248,7 +248,7 @@ test_branch_delete() {
 		fi
 	done
 
-	got branch -d branch2 -r $testroot/repo > $testroot/stdout
+	got branch -r $testroot/repo -d branch2 > $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "got branch command failed unexpectedly"
@@ -281,7 +281,7 @@ test_branch_delete() {
 		return 1
 	fi
 
-	got branch -d bogus_branch_name -r $testroot/repo \
+	got branch -r $testroot/repo -d bogus_branch_name \
 		> $testroot/stdout 2> $testroot/stderr
 	ret=$?
 	if [ $ret -eq 0 ]; then
@@ -323,7 +323,8 @@ test_branch_delete() {
 		return 1
 	fi
 
-	got branch -d origin/branch1 -r $testroot/repo > $testroot/stdout
+	got branch -r $testroot/repo -d origin/branch1 \
+		refs/remotes/origin/branch3 > $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "got branch command failed unexpectedly"
@@ -331,15 +332,6 @@ test_branch_delete() {
 		return 1
 	fi
 
-	got branch -d refs/remotes/origin/branch3 -r $testroot/repo \
-		> $testroot/stdout
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		echo "got branch command failed unexpectedly"
-		test_done "$testroot" "$ret"
-		return 1
-	fi
-
 	got ref -l -r $testroot/repo > $testroot/stdout
 	echo "HEAD: refs/heads/master" > $testroot/stdout.expected
 	echo "refs/heads/branch1: $commit_id" >> $testroot/stdout.expected
@@ -396,7 +388,7 @@ test_branch_delete_packed() {
 
 	(cd $testroot/repo && git pack-refs --all)
 
-	got branch -d refs/heads/branch2 -r $testroot/repo > $testroot/stdout
+	got branch -r $testroot/repo -d refs/heads/branch2 > $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		echo "got update command failed unexpectedly"
@@ -429,7 +421,7 @@ test_branch_delete_packed() {
 		return 1
 	fi
 
-	got branch -d bogus_branch_name -r $testroot/repo \
+	got branch -r $testroot/repo -d bogus_branch_name \
 		> $testroot/stdout 2> $testroot/stderr
 	ret=$?
 	if [ $ret -eq 0 ]; then