From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got diff: add support for multiple path arguments
To: gameoftrees@openbsd.org
Date: Sat, 9 Oct 2021 01:02:29 +0200

On Thu, Oct 07, 2021 at 08:59:31PM +0000, Klemens Nanni wrote:
> get_worktree_paths_from_argv() should probably ensure a unique list,
> i.e. neither `got diff -P a a' nor `got diff -P a ./a' should diff "a"
> twice.  That's also how git behaves.  Not that important and something
> for a another diff.

This is easy to do if we want it. I cannot think of a case where
de-duplicating and sorting work tree path arguments would hurt.

Per-path progress output of some commands now appears in sorted order,
which is probably for the better.

Diffs produced with 'got diff path1 path2 ...' in a work tree now
remain sorted regardless of the given order of path arguments.

ok?

-----------------------------------------------
commit c57abc5cd9b18300108ada26a3ae52bab96974e8 (argvpaths)
from: Stefan Sperling <stsp@stsp.name>
date: Fri Oct  8 22:54:44 2021 UTC
 
 sort and de-duplicate work tree path command line arguments
 
diff 9d24e59e17ae2e7e0089f4e5b9d874dc236463fb a7f83dd0a3d8ca8bab8b158fa4296746d1dc2acb
blob - c246bc2ddfac03461f12f26e9b5773620c4e525b
blob + 401a93415d8e3b1b2383a4eb8d545a9cc803b683
--- got/got.c
+++ got/got.c
@@ -3240,6 +3240,7 @@ get_worktree_paths_from_argv(struct got_pathlist_head 
 {
 	const struct got_error *err = NULL;
 	char *path;
+	struct got_pathlist_entry *new;
 	int i;
 
 	if (argc == 0) {
@@ -3253,10 +3254,11 @@ get_worktree_paths_from_argv(struct got_pathlist_head 
 		err = got_worktree_resolve_path(&path, worktree, argv[i]);
 		if (err)
 			break;
-		err = got_pathlist_append(paths, path, NULL);
-		if (err) {
+		err = got_pathlist_insert(&new, paths, path, NULL);
+		if (err || new == NULL /* duplicate */) {
 			free(path);
-			break;
+			if (err)
+				break;
 		}
 	}
 
blob - 0b8274bf499788e7fa6712a4346be0ea1a745883
blob + e59e019d01dee6cf8c0d851c8d4b5ed6beb5c176
--- regress/cmdline/add.sh
+++ regress/cmdline/add.sh
@@ -90,9 +90,9 @@ test_add_multiple() {
 		return 1
 	fi
 
-	echo "A  foo" > $testroot/stdout.expected
-	echo "A  bar" >> $testroot/stdout.expected
+	echo "A  bar" > $testroot/stdout.expected
 	echo "A  baz" >> $testroot/stdout.expected
+	echo "A  foo" >> $testroot/stdout.expected
 
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
blob - c04e7e02b4abed6321e3ffb41a3e8510fff59ec0
blob + f2dc7235f799415c6057fc465ecf29316d93f810
--- regress/cmdline/diff.sh
+++ regress/cmdline/diff.sh
@@ -116,12 +116,6 @@ test_diff_basic() {
 
 	# diff several paths in a work tree
 	echo "diff $head_rev $testroot/wt" > $testroot/stdout.expected
-	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
-	echo '--- /dev/null' >> $testroot/stdout.expected
-	echo '+++ new' >> $testroot/stdout.expected
-	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
-	echo '+new file' >> $testroot/stdout.expected
 	echo -n 'blob - ' >> $testroot/stdout.expected
 	got tree -r $testroot/repo -i | grep 'alpha$' | cut -d' ' -f 1 \
 		>> $testroot/stdout.expected
@@ -132,15 +126,6 @@ test_diff_basic() {
 	echo '-alpha' >> $testroot/stdout.expected
 	echo '+modified alpha' >> $testroot/stdout.expected
 	echo -n 'blob - ' >> $testroot/stdout.expected
-	got tree -r $testroot/repo -i epsilon | grep 'zeta$' | cut -d' ' -f 1 \
-		>> $testroot/stdout.expected
-	echo 'file + epsilon/zeta' >> $testroot/stdout.expected
-	echo '--- epsilon/zeta' >> $testroot/stdout.expected
-	echo '+++ epsilon/zeta' >> $testroot/stdout.expected
-	echo '@@ -1 +1 @@' >> $testroot/stdout.expected
-	echo '-zeta' >> $testroot/stdout.expected
-	echo '+modified zeta' >> $testroot/stdout.expected
-	echo -n 'blob - ' >> $testroot/stdout.expected
 	got tree -r $testroot/repo -i | grep 'beta$' | cut -d' ' -f 1 \
 		>> $testroot/stdout.expected
 	echo 'file + /dev/null' >> $testroot/stdout.expected
@@ -148,6 +133,21 @@ test_diff_basic() {
 	echo '+++ /dev/null' >> $testroot/stdout.expected
 	echo '@@ -1 +0,0 @@' >> $testroot/stdout.expected
 	echo '-beta' >> $testroot/stdout.expected
+	echo -n 'blob - ' >> $testroot/stdout.expected
+	got tree -r $testroot/repo -i epsilon | grep 'zeta$' | cut -d' ' -f 1 \
+		>> $testroot/stdout.expected
+	echo 'file + epsilon/zeta' >> $testroot/stdout.expected
+	echo '--- epsilon/zeta' >> $testroot/stdout.expected
+	echo '+++ epsilon/zeta' >> $testroot/stdout.expected
+	echo '@@ -1 +1 @@' >> $testroot/stdout.expected
+	echo '-zeta' >> $testroot/stdout.expected
+	echo '+modified zeta' >> $testroot/stdout.expected
+	echo 'blob - /dev/null' >> $testroot/stdout.expected
+	echo 'file + new' >> $testroot/stdout.expected
+	echo '--- /dev/null' >> $testroot/stdout.expected
+	echo '+++ new' >> $testroot/stdout.expected
+	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
+	echo '+new file' >> $testroot/stdout.expected
 
 	(cd $testroot/wt && got diff new alpha epsilon beta > $testroot/stdout)
 	cmp -s $testroot/stdout.expected $testroot/stdout
@@ -158,6 +158,23 @@ test_diff_basic() {
 		return 1
 	fi
 
+	# different order of arguments results in same output order
+	(cd $testroot/wt && got diff alpha new epsilon beta \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		echo "diff failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	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
+
 	# a branch 'new' should not collide with path 'new' if more
 	# than two arguments are passed
 	got br -r $testroot/repo -c master new > /dev/null
@@ -177,56 +194,6 @@ test_diff_basic() {
 		return 1
 	fi
 
-	# different order of arguments results in different output order
-	echo "diff $head_rev $testroot/wt" > $testroot/stdout.expected
-	echo -n 'blob - ' >> $testroot/stdout.expected
-	got tree -r $testroot/repo -i | grep 'alpha$' | cut -d' ' -f 1 \
-		>> $testroot/stdout.expected
-	echo 'file + alpha' >> $testroot/stdout.expected
-	echo '--- alpha' >> $testroot/stdout.expected
-	echo '+++ alpha' >> $testroot/stdout.expected
-	echo '@@ -1 +1 @@' >> $testroot/stdout.expected
-	echo '-alpha' >> $testroot/stdout.expected
-	echo '+modified alpha' >> $testroot/stdout.expected
-	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
-	echo '--- /dev/null' >> $testroot/stdout.expected
-	echo '+++ new' >> $testroot/stdout.expected
-	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
-	echo '+new file' >> $testroot/stdout.expected
-	echo -n 'blob - ' >> $testroot/stdout.expected
-	got tree -r $testroot/repo -i epsilon | grep 'zeta$' | cut -d' ' -f 1 \
-		>> $testroot/stdout.expected
-	echo 'file + epsilon/zeta' >> $testroot/stdout.expected
-	echo '--- epsilon/zeta' >> $testroot/stdout.expected
-	echo '+++ epsilon/zeta' >> $testroot/stdout.expected
-	echo '@@ -1 +1 @@' >> $testroot/stdout.expected
-	echo '-zeta' >> $testroot/stdout.expected
-	echo '+modified zeta' >> $testroot/stdout.expected
-	echo -n 'blob - ' >> $testroot/stdout.expected
-	got tree -r $testroot/repo -i | grep 'beta$' | cut -d' ' -f 1 \
-		>> $testroot/stdout.expected
-	echo 'file + /dev/null' >> $testroot/stdout.expected
-	echo '--- beta' >> $testroot/stdout.expected
-	echo '+++ /dev/null' >> $testroot/stdout.expected
-	echo '@@ -1 +0,0 @@' >> $testroot/stdout.expected
-	echo '-beta' >> $testroot/stdout.expected
-	(cd $testroot/wt && got diff alpha new epsilon beta \
-		> $testroot/stdout 2> $testroot/stderr)
-	ret="$?"
-	if [ "$ret" != "0" ]; then
-		echo "diff failed unexpectedly" >&2
-		test_done "$testroot" "1"
-		return 1
-	fi
-	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
-
 	# Two arguments are interpreted as objects if a colliding path exists
 	echo master > $testroot/wt/master
 	(cd $testroot/wt && got add master > /dev/null)
@@ -289,17 +256,17 @@ test_diff_basic() {
 	fi
 	echo "diff $head_rev $testroot/wt" > $testroot/stdout.expected
 	echo 'blob - /dev/null' >> $testroot/stdout.expected
-	echo 'file + new' >> $testroot/stdout.expected
-	echo '--- /dev/null' >> $testroot/stdout.expected
-	echo '+++ new' >> $testroot/stdout.expected
-	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
-	echo '+new file' >> $testroot/stdout.expected
-	echo 'blob - /dev/null' >> $testroot/stdout.expected
 	echo 'file + master' >> $testroot/stdout.expected
 	echo '--- /dev/null' >> $testroot/stdout.expected
 	echo '+++ master' >> $testroot/stdout.expected
 	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
 	echo '+master' >> $testroot/stdout.expected
+	echo 'blob - /dev/null' >> $testroot/stdout.expected
+	echo 'file + new' >> $testroot/stdout.expected
+	echo '--- /dev/null' >> $testroot/stdout.expected
+	echo '+++ new' >> $testroot/stdout.expected
+	echo '@@ -0,0 +1 @@' >> $testroot/stdout.expected
+	echo '+new file' >> $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
 	if [ "$ret" != "0" ]; then
blob - 9766243def1b9ea970812bb138d4d8aeda056a96
blob + 608abc82cbefff93d4d24717552a9b9a1f703761
--- regress/cmdline/revert.sh
+++ regress/cmdline/revert.sh
@@ -1092,12 +1092,12 @@ test_revert_symlink() {
 
 	cat > $testroot/stdout.expected <<EOF
 R  alpha.link
-R  epsilon.link
-R  passwd.link
 R  epsilon/beta.link
-R  nonexistent.link
-R  zeta.link
+R  epsilon.link
 R  new.link
+R  nonexistent.link
+R  passwd.link
+R  zeta.link
 EOF
 	cmp -s $testroot/stdout.expected $testroot/stdout
 	ret="$?"
blob - 51dea1da6173dd6abe8666612459b839ee497013
blob + cab753f3139225f2518ae82f6ba928625c46e9e6
--- regress/cmdline/rm.sh
+++ regress/cmdline/rm.sh
@@ -423,10 +423,10 @@ test_rm_symlink() {
 	fi
 
 	echo 'D  alpha.link' > $testroot/stdout.expected
-	echo 'D  epsilon.link' >> $testroot/stdout.expected
-	echo 'D  passwd.link' >> $testroot/stdout.expected
 	echo 'D  epsilon/beta.link' >> $testroot/stdout.expected
+	echo 'D  epsilon.link' >> $testroot/stdout.expected
 	echo 'D  nonexistent.link' >> $testroot/stdout.expected
+	echo 'D  passwd.link' >> $testroot/stdout.expected
 	(cd $testroot/wt && got rm alpha.link epsilon.link passwd.link \
 		epsilon/beta.link nonexistent.link > $testroot/stdout)
 
blob - de94a6e129b914a487c98dde2b1cc0d59698c4cd
blob + 8b67476fd9377314c10033f7fe555651af4cda1a
--- regress/cmdline/status.sh
+++ regress/cmdline/status.sh
@@ -493,12 +493,12 @@ test_status_many_paths() {
 	mkdir $testroot/wt/newdir
 	(cd $testroot/wt && got add new >/dev/null)
 
-	(cd $testroot/wt && got status newdir > $testroot/stdout.expected)
-	(cd $testroot/wt && got status alpha >> $testroot/stdout.expected)
+	(cd $testroot/wt && got status alpha > $testroot/stdout.expected)
+	(cd $testroot/wt && got status beta >> $testroot/stdout.expected)
 	(cd $testroot/wt && got status epsilon >> $testroot/stdout.expected)
 	(cd $testroot/wt && got status foo >> $testroot/stdout.expected)
 	(cd $testroot/wt && got status new >> $testroot/stdout.expected)
-	(cd $testroot/wt && got status beta >> $testroot/stdout.expected)
+	(cd $testroot/wt && got status newdir >> $testroot/stdout.expected)
 	(cd $testroot/wt && got status . >> $testroot/stdout.expected)
 
 	(cd $testroot/wt && got status newdir alpha epsilon foo new beta . \
blob - 946422e07ecfbb970b7b9f9264e7ee67e4b62d55
blob + d064dee9ea07f4c1490ffd37ad9aed281f7f6cb0
--- regress/cmdline/update.sh
+++ regress/cmdline/update.sh
@@ -1181,8 +1181,8 @@ test_update_partial_add() {
 	(cd $testroot/repo && git add .)
 	git_commit $testroot/repo -m "added two files"
 
-	echo "A  new" > $testroot/stdout.expected
-	echo "A  epsilon/new2" >> $testroot/stdout.expected
+	echo "A  epsilon/new2" > $testroot/stdout.expected
+	echo "A  new" >> $testroot/stdout.expected
 	echo -n "Updated to refs/heads/master: " >> $testroot/stdout.expected
 	git_show_head $testroot/repo >> $testroot/stdout.expected
 	echo >> $testroot/stdout.expected