From: Junio C Hamano <junkio@cox.net>
Date: Tue, 30 Aug 2005 14:20:40 GMT
Subject: Re: [PATCH] cg-diff fixed to work with BSD xargs
Message-ID: <7v7je3ld53.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <11253960093915-git-send-email-martin@catalyst.net.nz>

Martin Langhoff <martin@catalyst.net.nz> writes:

> Calls to cg-diff without filename parameters were dependent on GNU xargs
> traits. BSD xargs is hardcoded to do --no-run-if-empty -- so if the filter
> is effectively empty we avoid calling xargs.
>
> Signed-off-by: Martin Langhoff <martin@catalyst.net.nz>

The fix you did and the reason you stated why you did it in your
commit log message makes perfect sense (I think you missed the
other call to git-diff-tree at the end which is done the same
way with xargs, though); what I write below is not a complaint
to your patch.

But the code you are fixing looks to me like it is already
somewhat obsolete, even if it is still working.  The munging of
user-given paths into $filter temporary file was necessary only
because older git-diff-* family did not work from anywhere but
the top-level directory; they do, thanks to Linus' enhancements,
these days.

I think it is time to start updating Cogito to take advantage of
the modern core.  I do not do Porcelains, but here is my stab at
it.

------------
[PATCH] Redo cg-diff without its own "relative path" support.

It used to be that you had to do "relative path" by hand if you
wanted to work from a subdirectory, but some commands, notably
git-diff-* family, from the modern core knows how to do that
themselves, so take advantage of that.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

    jc: The upstream maintainer is welcome to take it, but this
    patch is not intended for immediate inclusion.  I am sure
    there are corner cases I overlooked, without knowing the
    subtleties of what tree-id is supposed to do, for example.
    Setting _git_repo_unneeded upfront to forcibly disable the
    relative path support is another thing I am not proud about;
    there would probably be a better way which I did not find
    only because I did not look closely enough.

cd /opt/packrat/playpen/public/in-place/git/git.pasky/
git diff HEAD
diff --git a/cg-diff b/cg-diff
--- a/cg-diff
+++ b/cg-diff
@@ -39,6 +39,7 @@
 
 USAGE="cg-diff [-c] [-m] [-p] [-r FROM_ID[:TO_ID]] [FILE]..."
 
+_git_repo_unneeded=t
 . ${COGITO_LIB}cg-Xlib || exit 1
 
 
@@ -136,17 +137,11 @@ if [ "$mergebase" ]; then
 fi
 
 
-filter=$(mktemp -t gitdiff.XXXXXX)
-[ "$_git_relpath" -a ! "$ARGS" ] && echo "$_git_relpath" >>$filter
-for file in "${ARGS[@]}"; do
-	echo "${_git_relpath}$file" >>$filter
-done
-
 if [ "$id2" = " " ]; then
 	if [ "$id1" != " " ]; then
-		tree=$(tree-id "$id1") || exit 1
+		tree=$(git-rev-parse --verify "$id1") || exit 1
 	else
-		tree=$(tree-id) || exit 1
+		tree=$(git-rev-parse --verify --default HEAD) || exit 1
 	fi
 
 	# Ensure to only diff modified files
@@ -155,21 +150,18 @@ if [ "$id2" = " " ]; then
 	# FIXME: Update ret based on what did we match. And take "$@"
 	# to account after all.
 	ret=
-	cat $filter | xargs git-diff-cache -r -p $tree | colorize | pager
-
-	rm $filter
+	git-diff-cache -r -p $tree "${ARGS[@]}" | colorize | pager
 
 	[ "$ret" ] && die "no files matched"
 	exit $ret
 fi
 
 
-id1=$(tree-id "$id1") || exit 1
-id2=$(tree-id "$id2") || exit 1
+id1=$(git-rev-parse --verify --default HEAD "$id1") || exit 1
+id2=$(git-rev-parse --verify --default HEAD "$id2") || exit 1
 
 [ "$id1" = "$id2" ] && die "trying to diff $id1 against itself"
 
-cat $filter | xargs git-diff-tree -r -p $id1 $id2 | colorize | pager
+git-diff-tree -r -p $id1 $id2 "${ARGS[@]}" | colorize | pager
 
-rm $filter
 exit 0