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@

> -----------------------------------------------
>  make diffing files which changed into dirs and vice-versa possible
>  
>  Problem marked with a TODO comment by me ages ago and rediscovered by naddy@
>  
> diff 9a298e5c10f6c68afbaca853454de2787a312c81 a785c78c6fc391bad27b79a12aa7ee92cfa022b3
> commit - 9a298e5c10f6c68afbaca853454de2787a312c81
> commit + a785c78c6fc391bad27b79a12aa7ee92cfa022b3
> blob - ab4e42cf1f2915c2bca5c735f86ed6ff502f258e
> blob + b0a37c8444b90290194224a490365b6613892921
> --- lib/diff.c
> +++ lib/diff.c
> @@ -669,11 +669,39 @@ 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) && diff_content) {
> +			err = diff_deleted_blob(&te1->id, f1, fd1,
> +			    f2, label1, te1->mode, repo, cb, cb_arg);
> +			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) && diff_content) {
> +			err = diff_added_blob(&te2->id, f1, f2, fd2,
> +			    label2, te2->mode, repo, cb, cb_arg);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
>  	return NULL;
>  }
>  
> @@ -732,8 +760,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 + ff2e9aad578c06ff14d22b4646afab0a5c07bf96
> --- 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,93 @@ 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
>  	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
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_parseargs "$@"
>  run_test test_diff_basic
>  run_test test_diff_shows_conflict
> @@ -1841,3 +1915,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