From: Omar Polo <op@omarpolo.com>
Subject: Re: regress for got diff -d + minor fix
To: Mark Jamsek <mark@jamsek.com>
Cc: Game of Trees <gameoftrees@openbsd.org>
Date: Tue, 10 Jan 2023 10:47:14 +0100

On 2023/01/10 20:15:04 +1100, Mark Jamsek <mark@jamsek.com> wrote:
> The below diff adds a couple tests for 'got diff -d'. And they already
> picked up an edge case: if we specify paths to diff in a worktree for
> a commit in which one of the paths was deleted, diff_paths() passes
> "/dev/null" to got_diff_blob_output_unidiff() for the deleted path
> (i.e., label2). This is for the:
> 
>   --- file/path
>   +++ /dev/null
> 
> header in the diff. This means we can't unconditionally rely on label2
> for the path when collecting its diffstat. The fix, included in the
> diff, is to only use label2 for the path if it's not a GOT_STATUS_DELETE
> state or there is no label1; the latter of which shouldn't happen if the
> file has been deleted for there must have been a file path to be deleted
> in the first place, but we check just in case.
> 
> You can backout the change in the below diff to lib/diff.c and run the
> new tests to see what the diff.c change improves, but it basically turns
> the top output into the bottom:
> 
> -----8<-----------
> 
> diffstat 7f99867f74e9704bb31b4883ce21e64713a1d012 b178d0ebdaaa45141e130377c83cda1f46008361
>  D  /dev/null  |  0+  1-
>  A  new        |  1+  0-
> 
> diffstat 7f99867f74e9704bb31b4883ce21e64713a1d012 b178d0ebdaaa45141e130377c83cda1f46008361
>  D  beta  |  0+  1-
>  A  new   |  1+  0-
> 
> ------------>8----

yup, much better.  ok op@ but see comments inline for the regress.

the ones for lib/diff.c are not about this diff, but just some stuff I
spotted reading and haven't noticed before (sorry.)

> diffstat /home/mark/src/got
>  M  lib/diff.c               |   34+  26-
>  M  regress/cmdline/diff.sh  |  416+   0-
> 
> 2 files changed, 450 insertions(+), 26 deletions(-)
> 
> diff /home/mark/src/got
> commit - a76e88e58fb716d5dded83442b153b60687283cb
> path + /home/mark/src/got
> blob - b5900dcf76baf3e7af894d4817aa447491a287fc
> file + lib/diff.c
> --- lib/diff.c
> +++ lib/diff.c
> [...]
> @@ -254,25 +266,17 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
>  				goto done;
>  			}
>  		} else {
> -			path = strdup(label2 ? label2 : label1);
> +			if (label2 != NULL &&
> +			    (status != GOT_STATUS_DELETE || label1 == NULL))
> +				path = strdup(label2);
> +			else
> +				path = strdup(label1);
>  			if (path == NULL) {
>  				err = got_error_from_errno("malloc");
                                                           ^^^^^^^^

should be strdup.  Sorry, I haven't noticed when reading the original
diff that introduced it.  (same below too)

> [...]
> blob - 51fe8c9d8176eeaa086aa653071670afd2db3eeb
> file + regress/cmdline/diff.sh
> --- regress/cmdline/diff.sh
> +++ regress/cmdline/diff.sh
> @@ -1375,6 +1375,420 @@ test_parseargs "$@"
>  	test_done "$testroot" $ret
>  }
>  
> +test_diff_commit_diffstat() {
> +	local testroot=`test_init diff_commit_diffstat`
> +	local commit_id0=`git_show_head $testroot/repo`
> +	alpha_id0=`get_blob_id $testroot/repo "" alpha`
> +	beta_id0=`get_blob_id $testroot/repo "" beta`

alpha_id0, beta_id0 should be `local'

> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo "modified alpha" > $testroot/wt/alpha
> +	(cd $testroot/wt && got rm beta >/dev/null)
> +	echo "new file" > $testroot/wt/new
> +	(cd $testroot/wt && got add new >/dev/null)
> +	(cd $testroot/wt && got commit -m 'committing changes' >/dev/null)
> +	local commit_id1=`git_show_head $testroot/repo`
> +
> +	alpha_id1=`get_blob_id $testroot/repo "" alpha`
> +	new_id1=`get_blob_id $testroot/repo "" new`

ditto

> +	cat <<EOF >$testroot/stdout.expected
> +diffstat $commit_id0 refs/heads/master
> + M  alpha  |  1+  1-
> + D  beta   |  0+  1-
> + A  new    |  1+  0-
> +
> [...]