cg-diff still broken on some BSDs

cg-diff still broken on some BSDs

[Martin Langhoff]

Petr,

We still have a bug in cg-diff when using the BSD xargs. I posted a
fix, and Junio posted an alternative approach. The original patches
are here http://marc.theaimsgroup.com/?l=git&m=112541165904627&w=2

For some strange reason, it is not showing up on my OSX box, but it is
definitely reproduceable in a proper FreeBSD box.

I am happy to rebase my patch (or Junio's) against current cogito if
needed, but I rather do only one of them ;)

cheers,


martin

[PATCH] cg-diff fixed to work with BSD xargs

[Martin Langhoff]

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>

---

 cg-diff |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

7b1d159f557ee06a0358217cdc29c2a2b2ee52fc
diff --git a/cg-diff b/cg-diff
--- a/cg-diff
+++ b/cg-diff
@@ -155,7 +155,11 @@ 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
+	if [ -s $filter ]; then
+		cat $filter | xargs git-diff-cache -r -p $tree | colorize | pager  
+	else
+		git-diff-cache -r -p $tree | colorize | pager
+	fi
 
 	rm $filter
 

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Junio C Hamano]

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

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Petr Baudis]

Dear diary, on Tue, Aug 30, 2005 at 04:20:40PM CEST, I got a letter
where Junio C Hamano <junkio@cox.net> told me that...
> 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.

Thanks for the patch - it'd be actually very nice to get rid of the
complexity. But more than that, I want to be consistent - so do all the
GIT core commands work in subdirectories by now, or only a subset of
them? I'd hate to have half of the scripts stay in the subdirectory
and half use the Cogito's old relpath logic, so it's either everything
or anything from my POV.

Thanks,

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Petr Baudis]

Dear diary, on Tue, Aug 30, 2005 at 12:00:10PM CEST, I got a letter
where Martin Langhoff <martin@catalyst.net.nz> told me that...
> 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>

Thanks, I've applied it and also fixed the second git-diff-tree usage.

Dear diary, on Tue, Sep 20, 2005 at 06:15:28AM CEST, I got a letter
where Martin Langhoff <martin.langhoff@gmail.com> told me that...
> We still have a bug in cg-diff when using the BSD xargs. I posted a
> fix, and Junio posted an alternative approach. The original patches
> are here http://marc.theaimsgroup.com/?l=git&m=112541165904627&w=2

Looks like I've overlooked it in my fast Cogito patch scan. There are
still parts of mailing list history in late August and early September
that I didn't read.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Matthias Urlichs]

Hi, Martin Langhoff wrote:

> Calls to cg-diff without filename parameters were dependent on GNU xargs
> traits. 

I don't know what drugs your shell was on when you tested this (assuming
that you did ;-)  but this patch is wrong -- your test always succeeds,
due to the vagaries of test / [ ] semantics.

> -	cat $filter | xargs git-diff-cache -r -p $tree | colorize | pager
> +	if [ -s $filter ]; then
> +		cat $filter | xargs git-diff-cache -r -p $tree | colorize | pager  
> +	else
> +		git-diff-cache -r -p $tree | colorize | pager
> +	fi
>  
 $ foo=""
 $ [ -s $foo ]
 $ echo $?
0
 $ [ -s "$foo" ]
 $ echo $?
1
 $

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
God made pot. Man made beer. Who do you trust?

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Petr Baudis]

Dear diary, on Wed, Sep 21, 2005 at 09:33:15AM CEST, I got a letter
where Matthias Urlichs <smurf@smurf.noris.de> told me that...
> Hi, Martin Langhoff wrote:
> 
> > Calls to cg-diff without filename parameters were dependent on GNU xargs
> > traits. 
> 
> I don't know what drugs your shell was on when you tested this (assuming
> that you did ;-)  but this patch is wrong -- your test always succeeds,
> due to the vagaries of test / [ ] semantics.
> 
> > -	cat $filter | xargs git-diff-cache -r -p $tree | colorize | pager
> > +	if [ -s $filter ]; then
> > +		cat $filter | xargs git-diff-cache -r -p $tree | colorize | pager  
> > +	else
> > +		git-diff-cache -r -p $tree | colorize | pager
> > +	fi
> >  
>  $ foo=""
>  $ [ -s $foo ]
>  $ echo $?
> 0
>  $ [ -s "$foo" ]
>  $ echo $?
> 1
>  $

$filter is a file name. -s tests whether the file of that name exists
and is non-empty.

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Matthias Urlichs]

Hi,

Petr Baudis:
> 
> $filter is a file name. -s tests whether the file of that name exists
> and is non-empty.
> 
True -- but it's still a bug.

If the name itself is empty (or has spaces), you get wrong results.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
BOFH excuse #385:

Dyslexics retyping hosts file on servers

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Petr Baudis]

Dear diary, on Wed, Sep 21, 2005 at 12:06:31PM CEST, I got a letter
where Matthias Urlichs <smurf@smurf.noris.de> told me that...
> Hi,

Hi,

> Petr Baudis:
> > 
> > $filter is a file name. -s tests whether the file of that name exists
> > and is non-empty.
> > 
> True -- but it's still a bug.
> 
> If the name itself is empty (or has spaces), you get wrong results.

but the name is never empty. It can contain spaces, and this should be
fixed, but we have a lot of places to fix in Cogito if it is to work
right with $TMPDIR containing spaces (what a weird idea).

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
VI has two modes: the one in which it beeps and the one in which
it doesn't.

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Matthias Urlichs]

Hi,

Petr Baudis:
> but the name is never empty.

OK, OK, I'll shut up already... my fault, I guess, from not having scanned
the whole file.

> It can contain spaces, and this should be
> fixed, but we have a lot of places to fix in Cogito if it is to work
> right with $TMPDIR containing spaces (what a weird idea).

There are enough people out there whose tempdir is named "Temporary
Items" or similar, so I guess somebody (i.e. me ;-) should walk through
the shell scripts and try not to get brain lock-up as he scans for that
sort of problem.

Of course, I also need a few hours to complete the SVN interpreter, and
a few hours to finalize a draft implementation of a *real* git library,
and ... well, if somebody wants the stuff badly enough to actually pay
for it I'm listening.

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
The cruelest thing that has happened to Lincoln since he was shot by Booth
was to fall into the hands of Sandburg.
					-- Edmund Wilson

Re: [PATCH] cg-diff fixed to work with BSD xargs

[Martin Langhoff]

On 9/21/05, Matthias Urlichs <smurf@smurf.noris.de> wrote:
> I don't know what drugs your shell was on when you tested this (assuming
> that you did ;-)  but this patch is wrong -- your test always succeeds,
> due to the vagaries of test / [ ] semantics.

I've submitted test-time and commit-time urine tests to the relevant
authorities. Both my shell and me are clean of forbidden substances
;-)

As Petr notes, the code is testing for a variable that we generated,
not random user input. Yes, perhaps we should test $TMPDIR for sanity,
but I think it's a bit overparanoid.

cheers,


martin