{"thread":{"id":"1701","subject":"[PATCH] cg-diff fixed to work with BSD xargs","startedAt":"2005-08-30T10:00:10Z","lastAt":"2005-09-21T10:56:12Z","messageCount":9,"participants":["Martin Langhoff","Junio C Hamano","Petr Baudis","Matthias Urlichs"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"7949","messageId":"11253960093915-git-send-email-martin@catalyst.net.nz","threadId":"1701","inReplyTo":null,"subject":"[PATCH] cg-diff fixed to work with BSD xargs","fromName":"Martin Langhoff","fromEmail":"martin@catalyst.net.nz","sentAt":"2005-08-30T10:00:10Z","receivedAt":"2005-08-30T10:00:10Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Calls to cg-diff without filename parameters were dependent on GNU xargs\ntraits. BSD xargs is hardcoded to do --no-run-if-empty -- so if the filter\nis effectively empty we avoid calling xargs.\n\nSigned-off-by: Martin Langhoff <martin@catalyst.net.nz>\n\n---\n\n cg-diff |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\n7b1d159f557ee06a0358217cdc29c2a2b2ee52fc\ndiff --git a/cg-diff b/cg-diff\n--- a/cg-diff\n+++ b/cg-diff\n@@ -155,7 +155,11 @@ if [ \"$id2\" = \" \" ]; then\n \t# FIXME: Update ret based on what did we match. And take \"$@\"\n \t# to account after all.\n \tret=\n-\tcat $filter | xargs git-diff-cache -r -p $tree | colorize | pager\n+\tif [ -s $filter ]; then\n+\t\tcat $filter | xargs git-diff-cache -r -p $tree | colorize | pager  \n+\telse\n+\t\tgit-diff-cache -r -p $tree | colorize | pager\n+\tfi\n \n \trm $filter\n \n"},{"id":"7951","messageId":"7v7je3ld53.fsf@assigned-by-dhcp.cox.net","threadId":"1701","inReplyTo":"11253960093915-git-send-email-martin@catalyst.net.nz","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-08-30T14:20:40Z","receivedAt":"2005-08-30T14:20:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Langhoff <martin@catalyst.net.nz> writes:\n\n> Calls to cg-diff without filename parameters were dependent on GNU xargs\n> traits. BSD xargs is hardcoded to do --no-run-if-empty -- so if the filter\n> is effectively empty we avoid calling xargs.\n>\n> Signed-off-by: Martin Langhoff <martin@catalyst.net.nz>\n\nThe fix you did and the reason you stated why you did it in your\ncommit log message makes perfect sense (I think you missed the\nother call to git-diff-tree at the end which is done the same\nway with xargs, though); what I write below is not a complaint\nto your patch.\n\nBut the code you are fixing looks to me like it is already\nsomewhat obsolete, even if it is still working.  The munging of\nuser-given paths into $filter temporary file was necessary only\nbecause older git-diff-* family did not work from anywhere but\nthe top-level directory; they do, thanks to Linus' enhancements,\nthese days.\n\nI think it is time to start updating Cogito to take advantage of\nthe modern core.  I do not do Porcelains, but here is my stab at\nit.\n\n------------\n[PATCH] Redo cg-diff without its own \"relative path\" support.\n\nIt used to be that you had to do \"relative path\" by hand if you\nwanted to work from a subdirectory, but some commands, notably\ngit-diff-* family, from the modern core knows how to do that\nthemselves, so take advantage of that.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n    jc: The upstream maintainer is welcome to take it, but this\n    patch is not intended for immediate inclusion.  I am sure\n    there are corner cases I overlooked, without knowing the\n    subtleties of what tree-id is supposed to do, for example.\n    Setting _git_repo_unneeded upfront to forcibly disable the\n    relative path support is another thing I am not proud about;\n    there would probably be a better way which I did not find\n    only because I did not look closely enough.\n\ncd /opt/packrat/playpen/public/in-place/git/git.pasky/\ngit diff HEAD\ndiff --git a/cg-diff b/cg-diff\n--- a/cg-diff\n+++ b/cg-diff\n@@ -39,6 +39,7 @@\n \n USAGE=\"cg-diff [-c] [-m] [-p] [-r FROM_ID[:TO_ID]] [FILE]...\"\n \n+_git_repo_unneeded=t\n . ${COGITO_LIB}cg-Xlib || exit 1\n \n \n@@ -136,17 +137,11 @@ if [ \"$mergebase\" ]; then\n fi\n \n \n-filter=$(mktemp -t gitdiff.XXXXXX)\n-[ \"$_git_relpath\" -a ! \"$ARGS\" ] && echo \"$_git_relpath\" >>$filter\n-for file in \"${ARGS[@]}\"; do\n-\techo \"${_git_relpath}$file\" >>$filter\n-done\n-\n if [ \"$id2\" = \" \" ]; then\n \tif [ \"$id1\" != \" \" ]; then\n-\t\ttree=$(tree-id \"$id1\") || exit 1\n+\t\ttree=$(git-rev-parse --verify \"$id1\") || exit 1\n \telse\n-\t\ttree=$(tree-id) || exit 1\n+\t\ttree=$(git-rev-parse --verify --default HEAD) || exit 1\n \tfi\n \n \t# Ensure to only diff modified files\n@@ -155,21 +150,18 @@ if [ \"$id2\" = \" \" ]; then\n \t# FIXME: Update ret based on what did we match. And take \"$@\"\n \t# to account after all.\n \tret=\n-\tcat $filter | xargs git-diff-cache -r -p $tree | colorize | pager\n-\n-\trm $filter\n+\tgit-diff-cache -r -p $tree \"${ARGS[@]}\" | colorize | pager\n \n \t[ \"$ret\" ] && die \"no files matched\"\n \texit $ret\n fi\n \n \n-id1=$(tree-id \"$id1\") || exit 1\n-id2=$(tree-id \"$id2\") || exit 1\n+id1=$(git-rev-parse --verify --default HEAD \"$id1\") || exit 1\n+id2=$(git-rev-parse --verify --default HEAD \"$id2\") || exit 1\n \n [ \"$id1\" = \"$id2\" ] && die \"trying to diff $id1 against itself\"\n \n-cat $filter | xargs git-diff-tree -r -p $id1 $id2 | colorize | pager\n+git-diff-tree -r -p $id1 $id2 \"${ARGS[@]}\" | colorize | pager\n \n-rm $filter\n exit 0\n"},{"id":"9012","messageId":"20050920193956.GE28621@pasky.or.cz","threadId":"1701","inReplyTo":"7v7je3ld53.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-09-20T19:39:56Z","receivedAt":"2005-09-20T19:39:56Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Tue, Aug 30, 2005 at 04:20:40PM CEST, I got a letter\nwhere Junio C Hamano <junkio@cox.net> told me that...\n> But the code you are fixing looks to me like it is already\n> somewhat obsolete, even if it is still working.  The munging of\n> user-given paths into $filter temporary file was necessary only\n> because older git-diff-* family did not work from anywhere but\n> the top-level directory; they do, thanks to Linus' enhancements,\n> these days.\n> \n> I think it is time to start updating Cogito to take advantage of\n> the modern core.  I do not do Porcelains, but here is my stab at\n> it.\n\nThanks for the patch - it'd be actually very nice to get rid of the\ncomplexity. But more than that, I want to be consistent - so do all the\nGIT core commands work in subdirectories by now, or only a subset of\nthem? I'd hate to have half of the scripts stay in the subdirectory\nand half use the Cogito's old relpath logic, so it's either everything\nor anything from my POV.\n\nThanks,\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nVI has two modes: the one in which it beeps and the one in which\nit doesn't.\n"},{"id":"9021","messageId":"pan.2005.09.21.07.33.14.533314@smurf.noris.de","threadId":"1701","inReplyTo":"11253960093915-git-send-email-martin@catalyst.net.nz","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-09-21T07:33:15Z","receivedAt":"2005-09-21T07:33:15Z","isPatch":true,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi, Martin Langhoff wrote:\n\n> Calls to cg-diff without filename parameters were dependent on GNU xargs\n> traits. \n\nI don't know what drugs your shell was on when you tested this (assuming\nthat you did ;-)  but this patch is wrong -- your test always succeeds,\ndue to the vagaries of test / [ ] semantics.\n\n> -\tcat $filter | xargs git-diff-cache -r -p $tree | colorize | pager\n> +\tif [ -s $filter ]; then\n> +\t\tcat $filter | xargs git-diff-cache -r -p $tree | colorize | pager  \n> +\telse\n> +\t\tgit-diff-cache -r -p $tree | colorize | pager\n> +\tfi\n>  \n $ foo=\"\"\n $ [ -s $foo ]\n $ echo $?\n0\n $ [ -s \"$foo\" ]\n $ echo $?\n1\n $\n\n-- \nMatthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nGod made pot. Man made beer. Who do you trust?\n"},{"id":"9023","messageId":"20050921081547.GA24902@pasky.or.cz","threadId":"1701","inReplyTo":"pan.2005.09.21.07.33.14.533314@smurf.noris.de","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-09-21T08:15:47Z","receivedAt":"2005-09-21T08:15:47Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Wed, Sep 21, 2005 at 09:33:15AM CEST, I got a letter\nwhere Matthias Urlichs <smurf@smurf.noris.de> told me that...\n> Hi, Martin Langhoff wrote:\n> \n> > Calls to cg-diff without filename parameters were dependent on GNU xargs\n> > traits. \n> \n> I don't know what drugs your shell was on when you tested this (assuming\n> that you did ;-)  but this patch is wrong -- your test always succeeds,\n> due to the vagaries of test / [ ] semantics.\n> \n> > -\tcat $filter | xargs git-diff-cache -r -p $tree | colorize | pager\n> > +\tif [ -s $filter ]; then\n> > +\t\tcat $filter | xargs git-diff-cache -r -p $tree | colorize | pager  \n> > +\telse\n> > +\t\tgit-diff-cache -r -p $tree | colorize | pager\n> > +\tfi\n> >  \n>  $ foo=\"\"\n>  $ [ -s $foo ]\n>  $ echo $?\n> 0\n>  $ [ -s \"$foo\" ]\n>  $ echo $?\n> 1\n>  $\n\n$filter is a file name. -s tests whether the file of that name exists\nand is non-empty.\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nVI has two modes: the one in which it beeps and the one in which\nit doesn't.\n"},{"id":"9031","messageId":"20050921100631.GQ14206@kiste.smurf.noris.de","threadId":"1701","inReplyTo":"20050921081547.GA24902@pasky.or.cz","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-09-21T10:06:31Z","receivedAt":"2005-09-21T10:06:31Z","isPatch":true,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi,\n\nPetr Baudis:\n> \n> $filter is a file name. -s tests whether the file of that name exists\n> and is non-empty.\n> \nTrue -- but it's still a bug.\n\nIf the name itself is empty (or has spaces), you get wrong results.\n\n-- \nMatthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nBOFH excuse #385:\n\nDyslexics retyping hosts file on servers\n"},{"id":"9032","messageId":"20050921101113.GF24902@pasky.or.cz","threadId":"1701","inReplyTo":"20050921100631.GQ14206@kiste.smurf.noris.de","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2005-09-21T10:11:13Z","receivedAt":"2005-09-21T10:11:13Z","isPatch":true,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"Dear diary, on Wed, Sep 21, 2005 at 12:06:31PM CEST, I got a letter\nwhere Matthias Urlichs <smurf@smurf.noris.de> told me that...\n> Hi,\n\nHi,\n\n> Petr Baudis:\n> > \n> > $filter is a file name. -s tests whether the file of that name exists\n> > and is non-empty.\n> > \n> True -- but it's still a bug.\n> \n> If the name itself is empty (or has spaces), you get wrong results.\n\nbut the name is never empty. It can contain spaces, and this should be\nfixed, but we have a lot of places to fix in Cogito if it is to work\nright with $TMPDIR containing spaces (what a weird idea).\n\n-- \n\t\t\t\tPetr \"Pasky\" Baudis\nStuff: http://pasky.or.cz/\nVI has two modes: the one in which it beeps and the one in which\nit doesn't.\n"},{"id":"9035","messageId":"20050921105451.GA24934@kiste.smurf.noris.de","threadId":"1701","inReplyTo":"20050921101113.GF24902@pasky.or.cz","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Matthias Urlichs","fromEmail":"smurf@smurf.noris.de","sentAt":"2005-09-21T10:54:51Z","receivedAt":"2005-09-21T10:54:51Z","isPatch":true,"sender":{"key":"matthias@urlichs.de","avatar":"https://gravatar.com/avatar/2708905af227313eba6f2b2ae0f7d0259b5ac5d71baef58fe5a13c699ce0bbf0?d=mp&s=160"},"body":"Hi,\n\nPetr Baudis:\n> but the name is never empty.\n\nOK, OK, I'll shut up already... my fault, I guess, from not having scanned\nthe whole file.\n\n> It can contain spaces, and this should be\n> fixed, but we have a lot of places to fix in Cogito if it is to work\n> right with $TMPDIR containing spaces (what a weird idea).\n\nThere are enough people out there whose tempdir is named \"Temporary\nItems\" or similar, so I guess somebody (i.e. me ;-) should walk through\nthe shell scripts and try not to get brain lock-up as he scans for that\nsort of problem.\n\nOf course, I also need a few hours to complete the SVN interpreter, and\na few hours to finalize a draft implementation of a *real* git library,\nand ... well, if somebody wants the stuff badly enough to actually pay\nfor it I'm listening.\n\n-- \nMatthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de\nDisclaimer: The quote was selected randomly. Really. | http://smurf.noris.de\n - -\nThe cruelest thing that has happened to Lincoln since he was shot by Booth\nwas to fall into the hands of Sandburg.\n\t\t\t\t\t-- Edmund Wilson\n"},{"id":"9034","messageId":"46a038f905092103561eac26a6@mail.gmail.com","threadId":"1701","inReplyTo":"pan.2005.09.21.07.33.14.533314@smurf.noris.de","subject":"Re: [PATCH] cg-diff fixed to work with BSD xargs","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2005-09-21T10:56:12Z","receivedAt":"2005-09-21T10:56:12Z","isPatch":true,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"On 9/21/05, Matthias Urlichs <smurf@smurf.noris.de> wrote:\n> I don't know what drugs your shell was on when you tested this (assuming\n> that you did ;-)  but this patch is wrong -- your test always succeeds,\n> due to the vagaries of test / [ ] semantics.\n\nI've submitted test-time and commit-time urine tests to the relevant\nauthorities. Both my shell and me are clean of forbidden substances\n;-)\n\nAs Petr notes, the code is testing for a variable that we generated,\nnot random user input. Yes, perhaps we should test $TMPDIR for sanity,\nbut I think it's a bit overparanoid.\n\ncheers,\n\n\nmartin\n"}]}