From: Omar Polo <op@omarpolo.com>
Subject: Re: got: fix incorrect argc check when diffing blobs
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Thu, 25 Aug 2022 22:13:26 +0200

On 2022/08/26 01:44:05 +1000, Mark Jamsek <mark@jamsek.com> wrote:
> We should check for more than two args as we have two blob ids:
> 
>   $ got diff 66173f9f4759b015a8 dae7c1789fc137c1b6927dca
>   got: path arguments cannot be used when diffing blobs
> 
> With the below diff:
> 
>   $ got diff 66173f9f4759b015a8 dae7c1789fc137c1b6927dca
>   blob - 66173f9f4759b015a8e2f0f9acc21194830f066d
>   blob + dae7c1789fc137c1b6927dca65e7f44b457d1e34
>   --- 66173f9f4759b015a8e2f0f9acc21194830f066d
>   +++ dae7c1789fc137c1b6927dca65e7f44b457d1e34
>   @@ -21,6 +21,14 @@ got:
>      This must require an up-to-date and clean work tree to avoid unrelated
>      changes from getting mixed in. Perform an implicit work tree base-commit
>      bump after committing, like 'got rebase' and 'got histedit' do it.
>   +- Respect the current umask when creating or changing files and directories
>   +  in the work tree. This behaviour is already documented in got-worktree(5)
>   +  but not actually implemented.
>   +- When a clone fails the HEAD symref will always point to "refs/heads/main"
>   +  (ie. the internal default HEAD symref of Got). Resuming a failed clone with
>   +  'got fetch' is supposed to work. To make this easier, if the HEAD symref
>   +  points to a non-existent reference it should be updated by 'got fetch'
>   +  to match the HEAD symref sent by the server.
>   
>    network protocol:
>    - add http(s) transport with libtls, speaking the two Git HTTP protocols

agreed, and the similar codepath for diffing objects has the `argc > 2' check.

ok for me.

should we add something like this in regress?

diff /home/op/w/got
commit - f0680473a7db1e5941bffdc2ab5f80ddec209122
path + /home/op/w/got
blob - 136f659b08eaf6408f6c79e05f6440fe7c754523
file + regress/cmdline/diff.sh
--- regress/cmdline/diff.sh
+++ regress/cmdline/diff.sh
@@ -385,6 +385,43 @@ test_diff_basic() {
 		diff -u $testroot/stderr.expected $testroot/stderr
 		return 1
 	fi
+
+	# diff two blob ids, but first commit the changes to alpha
+	(cd $testroot/wt && got commit -m 'edit' alpha >/dev/null)
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got commit failed" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	(cd $testroot/wt && got log -l1 -p alpha) > $testroot/log
+	local bold="$(awk '/^blob \-/{print $3}' $testroot/log)"
+	local bnew="$(awk '/^blob \+/{print $3}' $testroot/log)"
+	sed -n	-e "s/^--- alpha/--- $bold/" \
+		-e "s/^\+\+\+ alpha/+++ $bnew/" \
+		-e '/^blob -/,$p' \
+		$testroot/log > $testroot/diff.expected
+
+	(cd $testroot/wt && got diff $bold $bnew) > $testroot/diff
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "diff failed unexpectedly" >&2
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	# got log adds an empty line at the bottom of a diff.
+	echo >> $testroot/diff
+
+	cmp -s $testroot/diff.expected $testroot/diff
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/diff.expected $testroot/diff
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
 	test_done "$testroot" "$ret"
 }