From: Omar Polo <op@omarpolo.com>
Subject: Re: support diffing dirs vs. files
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Fri, 10 Mar 2023 14:59:42 +0100

On 2023/03/10 13:09:58 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Mar 10, 2023 at 12:52:40PM +0100, Omar Polo wrote:
> > On 2023/03/10 12:29:09 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > > Make test_diff_file_to_dir pass, a test which was added by naddy recently.
> > > And add test_diff_dir_to_file, which covers the inverse case.
> > > 
> > > We display removal of the file (or all files in a directory) before displaying
> > > additions of files/dirs. I hope this will allow 'got patch' and other patch
> > > tools to apply such patches.
> > 
> > Yes, doing the removal first and the addition later is the way to go.
> > I haven't tested these kind of situations with 'got patch' or patch,
> > but if they fail now it's a got patch or patch(1) bug :-)
> > 
> > > ok?
> > 
> > ok op@
> 
> I missed that got log -P relies on the diff callback getting called
> if we are not diffing blob contents.
> 
> With that issue fixed, got log -P shows both 'D alpha' and 'A alpha/eta'.
> Previously only 'A alpha/eta' was shown. Add test coverage for this.
> 
> Still ok?

ooops, yes, of course!  forgot to think about it.  Good to add a test
case :)

> diff 9a298e5c10f6c68afbaca853454de2787a312c81 b1ac7627c5c45b470bedea8421ce5944293ffdab
> commit - 9a298e5c10f6c68afbaca853454de2787a312c81
> commit + b1ac7627c5c45b470bedea8421ce5944293ffdab
> blob - ab4e42cf1f2915c2bca5c735f86ed6ff502f258e
> blob + 5198b5ef5702bde4150b817019574f96e0b9f1f3
> --- lib/diff.c
> +++ lib/diff.c
> @@ -669,11 +669,50 @@ diff_kind_mismatch(struct got_object_id *id1, struct g
>  }
>  
>  static const struct got_error *
> -diff_kind_mismatch(struct got_object_id *id1, struct got_object_id *id2,
> +diff_kind_mismatch(struct got_tree_entry *te1, struct got_tree_entry *te2,
> +    FILE *f1, FILE *f2, int fd1, int fd2,
>      const char *label1, const char *label2, struct got_repository *repo,
> -    got_diff_blob_cb cb, void *cb_arg)
> +    got_diff_blob_cb cb, void *cb_arg, int diff_content)
>  {
> -	/* XXX TODO */
> +	const struct got_error *err = NULL;
> +
> +	/* 
> +	 * Handle files changing into directories and vice-versa.
> +	 * Disregard edge cases with FIFOs, device nodes, etc for now.
> +	 */
> +	if (!S_ISDIR(te1->mode) && S_ISDIR(te2->mode)) {
> +		if (S_ISREG(te1->mode)) {
> +			if (diff_content) {
> +				err = diff_deleted_blob(&te1->id, f1, fd1,
> +				    f2, label1, te1->mode, repo, cb, cb_arg);
> +			} else {
> +				err = cb(cb_arg, NULL, NULL, NULL, NULL,
> +				    &te1->id, NULL, label1, NULL,
> +				    te1->mode, 0, repo);
> +			}
> +			if (err)
> +				return err;
> +		}
> +		return diff_added_tree(&te2->id, f1, f2, fd2, label2,
> +		    repo, cb, cb_arg, diff_content);
> +	} else if (S_ISDIR(te1->mode) && !S_ISDIR(te2->mode)) {
> +		err = diff_deleted_tree(&te1->id, f1, fd1, f2,
> +		    label1, repo, cb, cb_arg, diff_content);
> +		if (err)
> +			return err;
> +		if (S_ISREG(te2->mode)) {
> +			if (diff_content) {
> +				err = diff_added_blob(&te2->id, f1, f2, fd2,
> +				    label2, te2->mode, repo, cb, cb_arg);
> +			} else {
> +				err = cb(cb_arg, NULL, NULL, NULL, NULL, NULL,
> +				    &te2->id, NULL, label2, 0, te2->mode, repo);
> +			}
> +			if (err)
> +				return err;
> +		}
> +	}
> +
>  	return NULL;
>  }
>  
> @@ -732,8 +771,8 @@ diff_entry_old_new(struct got_tree_entry *te1, struct 
>  	if (id_match)
>  		return NULL;
>  
> -	return diff_kind_mismatch(&te1->id, &te2->id, label1, label2, repo,
> -	    cb, cb_arg);
> +	return diff_kind_mismatch(te1, te2, f1, f2, fd1, fd2,
> +	    label1, label2, repo, cb, cb_arg, diff_content);
>  }
>  
>  static const struct got_error *
> blob - e8dc379f3766f3126a73441a2e308fd844afe31e
> blob + 12bca5996322c1822c24ced40493cd7acb0ca896
> --- regress/cmdline/common.sh
> +++ regress/cmdline/common.sh
> @@ -77,6 +77,13 @@ git_show_head()
>  	(cd $repo && git rm -q "$@")
>  }
>  
> +git_rmdir()
> +{
> +	local repo="$1"
> +	shift
> +	(cd $repo && git rm -q -r "$@")
> +}
> +
>  git_show_head()
>  {
>  	local repo="$1"
> blob - fe9c943bc19d1b0b6905788633041f88ae69282b
> blob + 74c5c3277048c41e7d957d274e2fd708fbd7e022
> --- regress/cmdline/diff.sh
> +++ regress/cmdline/diff.sh
> @@ -1796,6 +1796,7 @@ test_diff_file_to_dir() {
>  test_diff_file_to_dir() {
>  	local testroot=`test_init diff_file_to_dir`
>  	local commit_id0=`git_show_head $testroot/repo`
> +	local alpha_blobid=`get_blob_id $testroot/repo "" alpha`
>  
>  	got checkout $testroot/repo $testroot/wt > /dev/null
>  	ret=$?
> @@ -1810,20 +1811,155 @@ test_diff_file_to_dir() {
>  	(cd $testroot/repo && git add alpha/eta)
>  	git_commit $testroot/repo -m "changed alpha into directory"
>  	local commit_id1=`git_show_head $testroot/repo`
> +	local alpha_eta_blobid=`get_blob_id $testroot/repo alpha eta`
>  
> -	echo "diff $commit_id0 $commit_id1" > $testroot/stdout.expected
> -	echo "commit - $commit_id0" >> $testroot/stdout.expected
> -	echo "commit + $commit_id1" >> $testroot/stdout.expected
> +	cat <<EOF >$testroot/stdout.expected
> +diff $commit_id0 $commit_id1
> +commit - $commit_id0
> +commit + $commit_id1
> +blob - $alpha_blobid (mode 644)
> +blob + /dev/null
> +--- alpha
> ++++ /dev/null
> +@@ -1 +0,0 @@
> +-alpha
> +blob - /dev/null
> +blob + $alpha_eta_blobid (mode 644)
> +--- /dev/null
> ++++ alpha/eta
> +@@ -0,0 +1 @@
> ++eta
> +EOF
>  	got diff -r $testroot/repo $commit_id0 $commit_id1 > $testroot/stdout
> -	# Diff should not be empty
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "diff failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
>  	cmp -s $testroot/stdout.expected $testroot/stdout
>  	ret=$?
> -	if [ $ret -eq 0 ]; then
> -		ret="xfail file to directory"
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
>  	fi
> +
> +	local author_time=`git_show_author_time $testroot/repo`
> +	d=`date -u -r $author_time +"%a %b %e %X %Y UTC"`
> +	cat <<EOF >$testroot/stdout.expected
> +-----------------------------------------------
> +commit $commit_id1 (master)
> +from: $GOT_AUTHOR
> +date: $d
> + 
> + changed alpha into directory
> + 
> + D  alpha
> + A  alpha/eta
> +
> +EOF
> +
> +	got log -P -r $testroot/repo -l1 -c $commit_id1 > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "diff failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_diff_dir_to_file() {
> +	local testroot=`test_init diff_file_to_dir`
> +	local commit_id0=`git_show_head $testroot/repo`
> +	local epsilon_zeta_blobid=`get_blob_id $testroot/repo epsilon zeta`
> +
> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	git_rmdir $testroot/repo epsilon
> +	echo epsilon > $testroot/repo/epsilon
> +	(cd $testroot/repo && git add epsilon)
> +	git_commit $testroot/repo -m "changed epsilon into file"
> +	local commit_id1=`git_show_head $testroot/repo`
> +	local epsilon_blobid=`get_blob_id $testroot/repo "" epsilon`
> +
> +	cat <<EOF >$testroot/stdout.expected
> +diff $commit_id0 $commit_id1
> +commit - $commit_id0
> +commit + $commit_id1
> +blob - $epsilon_zeta_blobid (mode 644)
> +blob + /dev/null
> +--- epsilon/zeta
> ++++ /dev/null
> +@@ -1 +0,0 @@
> +-zeta
> +blob - /dev/null
> +blob + $epsilon_blobid (mode 644)
> +--- /dev/null
> ++++ epsilon
> +@@ -0,0 +1 @@
> ++epsilon
> +EOF
> +	got diff -r $testroot/repo $commit_id0 $commit_id1 > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "diff failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	local author_time=`git_show_author_time $testroot/repo`
> +	d=`date -u -r $author_time +"%a %b %e %X %Y UTC"`
> +	cat <<EOF >$testroot/stdout.expected
> +-----------------------------------------------
> +commit $commit_id1 (master)
> +from: $GOT_AUTHOR
> +date: $d
> + 
> + changed epsilon into file
> + 
> + D  epsilon/zeta
> + A  epsilon
> +
> +EOF
> +
> +	got log -P -r $testroot/repo -l1 -c $commit_id1 > $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "diff failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_parseargs "$@"
>  run_test test_diff_basic
>  run_test test_diff_shows_conflict
> @@ -1841,3 +1977,4 @@ run_test test_diff_file_to_dir
>  run_test test_diff_commit_diffstat
>  run_test test_diff_worktree_diffstat
>  run_test test_diff_file_to_dir
> +run_test test_diff_dir_to_file