Re: [PATCH] Display change history as a diff between two dirs

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 30, 2011, 06:09 UTC
Message-ID: <7vty6rrow8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4EAC6765.4030003@gmail.com>

Roland Kaufmann <rlndkfmn+git@gmail.com> writes:

Show 9 quoted lines

> diff --git a/git-dirdiff--helper.sh b/git-dirdiff--helper.sh
> new file mode 100755
> index 0000000..bc0b49d
> --- /dev/null
> +++ b/git-dirdiff--helper.sh
> @@ -0,0 +1,28 @@
> ...
> +# bail out if there is any problems copying
> +set -e

I do not think any of our scripted Porcelains use "set -e"; rather they try to catch errors and produce messages that are more appropriate for specific error sites.

Show 5 quoted lines

> +# check that we are called by git-dirdiff
> +if [ -z $__GIT_DIFF_DIR ]; then
> +  echo Error: Do not call $(basename $0) directly 1>&2
> +  exit 1
> +fi

(style)

	if test -z "$__GIT_DIFF_DIR"
        then
        	echo >&2 "..."
		exit 1
	fi

If this were to become part of the main Porcelain set, you would probably source in the git-sh-setup helper near the beginning with

	. git-sh-setup

and use "die" instead of "echo && exit 1".

Show 10 quoted lines

> diff --git a/git-dirdiff.sh b/git-dirdiff.sh
> new file mode 100755
> index 0000000..4e75eda
> --- /dev/null
> +++ b/git-dirdiff.sh
> @@ -0,0 +1,34 @@
> ...
> +# create a temporary directory to hold snapshots of changed files
> +__GIT_DIFF_DIR=$(mktemp --tmpdir -d git-dirdiff.XXXXXX)
> +export __GIT_DIFF_DIR

I do not think any of our scripted Porcelains use "mktemp" especially "mktemp -d". How portable is it?

Show 6 quoted lines

> +git diff --raw "$@" > $__GIT_DIFF_DIR/toc
> +
> +# let the helper script accumulate them into the temporary directory
> +cut -f 2- -s $__GIT_DIFF_DIR/toc | while read f; do
> +  GIT_EXTERNAL_DIFF=git-dirdiff--helper git --no-pager diff "$@" $f
> +done

(style)

	... upstream command ... |
        while read f
	do
		...
	done

It also is not clear what could be used in "$@". Obviously you would not want things like "-U20" and "--stat" fed to the first "list of paths" phase, but there may be some options you may want to give to the inner "external diff" thing.

It is not clear to me why you need to grab the list of paths in toc and iterate over them one by one. IOW, why isn't this sufficient?

    # dirdiff--helper will copy the files in the temporary directory
    GIT_EXTERNAL_DIFF=git-dirdiff--helper git --no-pager diff "$@"

    # Now the temporary old/ and new/ are populated, compare them
    git-difftool--helper - .....

Overall, I found it interesting, but I am not convinced yet that this should be in the set of main Porcelains. It seems to be a hack to work around the design of the current external diff interface that specifically targets tools that are about comparing single pair of paths at a time.

If "compare two sets of files, each extracted in its own temporary directory" turns out to be sufficiently useful thing to do (which I suspect is true), we would probably want to make it an option to "git diff" itself, and not a separate git subcommand like "dirdiff". I can see "git diff" (and possibly even things like "git log -p") populating two temporary directories (your old/ and new/) and running a custom program specified by GIT_EXTERNAL_TREEDIFF environment variable, instead of doing any textual diff generation internally.

I wouldn't mind carrying a polished version of this in contrib/ for a cycle or two in order to let people try it out and get kinks out of its design.

For example, how well does this approach work with -M/-C (I do not think it would do anything useful, but I haven't thought things through). It would be nice if we gave the hint to the external program that compares the populated temporary directories how paths in the preimage tree and postimage tree correspond to each other.