{"thread":{"id":"11971","subject":"[PATCH] opening files in remote.c should ensure it is opening a file","startedAt":"2008-02-08T16:46:54Z","lastAt":"2008-02-18T11:31:57Z","messageCount":27,"participants":["H.Merijn Brand","Mike Ralphson","Junio C Hamano","Morten Welinder","Daniel Barkalow","Johannes Schindelin","Brandon Casey"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"67968","messageId":"20080208174654.2e9e679c@pc09.procura.nl","threadId":"11971","inReplyTo":null,"subject":"[PATCH] opening files in remote.c should ensure it is opening a file","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-08T16:46:54Z","receivedAt":"2008-02-08T16:46:54Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"HP-UX allows directories to be opened with fopen (path, \"r\"), which\nwill cause some translations that expect to read files, read dirs\ninstead. This patch makes sure the two fopen () calls in remote.c\nonly open the file if it is a file.\n\nSigned-off-by: H.Merijn Brand \n---\n\ndiff -pur git-1.5.4a/remote.c git-1.5.4b/remote.c\n--- git-1.5.4a/remote.c 2008-01-27 09:04:18 +0100\n+++ git-1.5.4/remote.c 2008-02-08 17:38:43 +0100\n@@ -121,9 +121,18 @@ static struct branch *make_branch(const\n return branches[empty];\n }\n\n+/* Helper function to ensure that we are opening a file and not a directory */\n+static FILE *open_file(char *full_path)\n+{\n+ struct stat st_buf;\n+ if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))\n+ return NULL;\n+ return (fopen(full_path, \"r\"));\n+}\n+\n static void read_remotes_file(struct remote *remote)\n {\n- FILE *f = fopen(git_path(\"remotes/%s\", remote->name), \"r\");\n+ FILE *f = open_file(git_path(\"remotes/%s\", remote->name));\n\n if (!f)\n return;\n@@ -173,7 +182,7 @@ static void read_branches_file(struct re\n char *frag;\n char *branch;\n int n = slash ? slash - remote->name : 1000;\n- FILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n+ FILE *f = open_file(git_path(\"branches/%.*s\", n, remote->name));\n char *s, *p;\n int len;\n\n--\ngit-1.5.4\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"67973","messageId":"e2b179460802080925s61270036q81896010c76236ae@mail.gmail.com","threadId":"11971","inReplyTo":"20080208174654.2e9e679c@pc09.procura.nl","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Mike Ralphson","fromEmail":"mike.ralphson@gmail.com","sentAt":"2008-02-08T17:25:52Z","receivedAt":"2008-02-08T17:25:52Z","isPatch":true,"sender":{"key":"mike.ralphson@gmail.com","avatar":"https://avatars.githubusercontent.com/u/21603?v=4"},"body":"On Feb 8, 2008 4:46 PM, H.Merijn Brand wrote:\n> HP-UX allows directories to be opened with fopen (path, \"r\"), which\n> will cause some translations that expect to read files, read dirs\n> instead. This patch makes sure the two fopen () calls in remote.c\n> only open the file if it is a file.\n>\n> Signed-off-by: H.Merijn Brand \n\nMany thanks, this is also required for AIX. I had got some way to\ntracking it down, but I thought it was an issue with strbuf. So...\n\nTested-by: Mike Ralphson \n\nYour other fix there [- if (!strbuf_avail(sb)) / + if\n(strbuf_avail(sb) < 64) ] is, guess what, also required on AIX.\n\nThanks again.\n"},{"id":"67984","messageId":"20080208210447.289022b6@pc09.procura.nl","threadId":"11971","inReplyTo":"e2b179460802080925s61270036q81896010c76236ae@mail.gmail.com","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-08T20:04:47Z","receivedAt":"2008-02-08T20:04:47Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Fri, 8 Feb 2008 17:25:52 +0000, \"Mike Ralphson\" \nwrote:\n\n> On Feb 8, 2008 4:46 PM, H.Merijn Brand wrote:\n> > HP-UX allows directories to be opened with fopen (path, \"r\"), which\n> > will cause some translations that expect to read files, read dirs\n> > instead. This patch makes sure the two fopen () calls in remote.c\n> > only open the file if it is a file.\n> >\n> > Signed-off-by: H.Merijn Brand \n> \n> Many thanks, this is also required for AIX. I had got some way to\n> tracking it down, but I thought it was an issue with strbuf. So...\n> \n> Tested-by: Mike Ralphson \n> \n> Your other fix there [- if (!strbuf_avail(sb)) / + if\n> (strbuf_avail(sb) < 64) ] is, guess what, also required on AIX.\n> \n> Thanks again.\n\nNot there yet ...\n\n$ cat do-tests\n#!/bin/sh\n\nexport TAR=ntar\nrm -f *.err\nfor t in t[0-9]*.sh ; do\n echo $t\n sh $t > test.err 2>&1 || mv test.err $t.err\n rm -f test.err\n done\n$\n\n197509 -rw-rw-rw- 1 merijn softwr 1633 Feb 8 18:03 t5302-pack-index.sh.err\n196846 -rw-rw-rw- 1 merijn softwr 943 Feb 8 18:04 t5500-fetch-pack.sh.err\n203431 -rw-rw-rw- 1 merijn softwr 344 Feb 8 18:05 t5600-clone-fail-cleanup.sh.err\n202602 -rw-rw-rw- 1 merijn softwr 458 Feb 8 18:05 t5701-clone-local.sh.err\n202761 -rw-rw-rw- 1 merijn softwr 3039 Feb 8 18:06 t6002-rev-list-bisect.sh.err\n202641 -rw-rw-rw- 1 merijn softwr 3980 Feb 8 18:06 t6003-rev-list-topo-order.sh.err\n202731 -rw-rw-rw- 1 merijn softwr 899 Feb 8 18:06 t6022-merge-rename.sh.err\n197510 -rw-rw-rw- 1 merijn softwr 1340 Feb 8 18:08 t7201-co.sh.err\n202705 -rw-rw-rw- 1 merijn softwr 149 Feb 8 18:09 t9300-fast-import.sh.err\n197051 -rw-rw-rw- 1 merijn softwr 1651 Feb 8 18:09 t9301-fast-export.sh.err\n\nhttp://www.xs4all.nl/~procura/git-1.5.3-1123ipf.tar\n\nTips welcome :)\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"67987","messageId":"7vhcgjjjlh.fsf@gitster.siamese.dyndns.org","threadId":"11971","inReplyTo":"20080208174654.2e9e679c@pc09.procura.nl","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-08T20:09:46Z","receivedAt":"2008-02-08T20:09:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H.Merijn Brand\" writes:\n\n> HP-UX allows directories to be opened with fopen (path, \"r\"), which\n> will cause some translations that expect to read files, read dirs\n> instead. This patch makes sure the two fopen () calls in remote.c\n> only open the file if it is a file.\n\n> +static FILE *open_file(char *full_path)\n> +{\n> + struct stat st_buf;\n> + if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))\n> + return NULL;\n> + return (fopen(full_path, \"r\"));\n> +}\n\nCan we make this a platform specific \"compat\" hack?\n\nIt is not fair to force stat() overhead to ports on platforms\nthat fails fopen() on directories, as I doubt we would ever want\nfrom directory using fopen() anyway.\n"},{"id":"67988","messageId":"118833cc0802081215t380587f6w7b5c0aba66a55799@mail.gmail.com","threadId":"11971","inReplyTo":"20080208174654.2e9e679c@pc09.procura.nl","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Morten Welinder","fromEmail":"mwelinder@gmail.com","sentAt":"2008-02-08T20:15:40Z","receivedAt":"2008-02-08T20:15:40Z","isPatch":true,"sender":{"key":"mwelinder@gmail.com","avatar":null},"body":"> +/* Helper function to ensure that we are opening a file and not a directory */\n> +static FILE *open_file(char *full_path)\n> +{\n> + struct stat st_buf;\n> + if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))\n> + return NULL;\n> + return (fopen(full_path, \"r\"));\n> +}\n\nThat looks wrong. stat+fopen has a pointless race condition that\nopen+fstat+fdopen would not have.\n\nMorten\n"},{"id":"67993","messageId":"7v8x1vjiic.fsf@gitster.siamese.dyndns.org","threadId":"11971","inReplyTo":"118833cc0802081215t380587f6w7b5c0aba66a55799@mail.gmail.com","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-08T20:33:15Z","receivedAt":"2008-02-08T20:33:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Morten Welinder\" writes:\n\n>> +/* Helper function to ensure that we are opening a file and not a directory */\n>> +static FILE *open_file(char *full_path)\n>> +{\n>> + struct stat st_buf;\n>> + if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))\n>> + return NULL;\n>> + return (fopen(full_path, \"r\"));\n>> +}\n>\n> That looks wrong. stat+fopen has a pointless race condition that\n> open+fstat+fdopen would not have.\n\nThat's true. How about doing something like this?\n\n (1) in a new file \"compat/gitfopen.c\" have this:\n\n\t#include \"../git-compat-util.h\"\n\t#undef fopen\n\tFILE *gitfopen(const char *path, const char *mode)\n {\n\t\tint fd, flags;\n struct stat st;\n \tif (mode[0] == 'w')\n \treturn fopen(path, mode);\n\t\tswitch (mode[0]) {\n case 'r': flags = O_RDONLY; break;\n case 'a': flags = O_APPEND; break;\n\t\tdefault:\n\t\t\terrno = EINVAL;\n \treturn NULL;\n\t\t}\n\t\tfd = open(path, flags);\n\t\tif (fd < 0 || fstat(fd, &st))\n \treturn NULL;\n\t\tif (S_ISDIR(st_buf.st_mode)) {\n \terrno = EISDIR;\n return NULL;\n\t\t}\n\t\treturn fdopen(fd, mode);\n\t}\n\n (2) in \"git-compat-util.h\" have this:\n\n\t#ifdef FOPEN_OPENS_DIRECTORIES\n #define fopen(a,b) gitfopen(a,b)\n\textern FILE *gitfopen(const char *, const char *);\n #endif\n\nAnd have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate\nplatforms.\n"},{"id":"67994","messageId":"alpine.LNX.1.00.0802081526350.13593@iabervon.org","threadId":"11971","inReplyTo":"e2b179460802080925s61270036q81896010c76236ae@mail.gmail.com","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-08T20:36:05Z","receivedAt":"2008-02-08T20:36:05Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 8 Feb 2008, Mike Ralphson wrote:\n\n> On Feb 8, 2008 4:46 PM, H.Merijn Brand wrote:\n> > HP-UX allows directories to be opened with fopen (path, \"r\"), which\n> > will cause some translations that expect to read files, read dirs\n> > instead. This patch makes sure the two fopen () calls in remote.c\n> > only open the file if it is a file.\n> >\n> > Signed-off-by: H.Merijn Brand \n> \n> Many thanks, this is also required for AIX. I had got some way to\n> tracking it down, but I thought it was an issue with strbuf. So...\n\nDoes the following help? We really ought to know that \"..\" must be a path \nliteral (and there obviously should be more limitations on nicknames for \nremotes, but I haven't figured out what they should be yet).\n\n\t-Daniel\n*This .sig left intentionally blank*\n\ndiff --git a/remote.c b/remote.c\nindex 0e00680..83a3d9d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)\n if (!name)\n name = default_remote_name;\n ret = make_remote(name, 0);\n- if (name[0] != '/') {\n+ if (name[0] != '/' && strcmp(name, \"..\")) {\n if (!ret->url)\n read_remotes_file(ret);\n if (!ret->url)\n"},{"id":"67996","messageId":"alpine.LSU.1.00.0802082035460.11591@racer.site","threadId":"11971","inReplyTo":"7vhcgjjjlh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-08T20:38:14Z","receivedAt":"2008-02-08T20:38:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Feb 2008, Junio C Hamano wrote:\n\n> \"H.Merijn Brand\" writes:\n> \n> > HP-UX allows directories to be opened with fopen (path, \"r\"), which\n> > will cause some translations that expect to read files, read dirs\n> > instead. This patch makes sure the two fopen () calls in remote.c\n> > only open the file if it is a file.\n> \n> > +static FILE *open_file(char *full_path)\n> > +{\n> > + struct stat st_buf;\n> > + if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))\n> > + return NULL;\n> > + return (fopen(full_path, \"r\"));\n> > +}\n> \n> Can we make this a platform specific \"compat\" hack?\n\nYou mean something like\n\n#ifdef FOPEN_OPENS_DIRECTORIES\ninline static FILE *fopen_compat(const char *path, const char *mode)\n{\n struct stat st_buf;\n if (stat(path, &st_buf) || !S_ISREG(st_buf.st_mode))\n return NULL;\n return (fopen(path, mode));\n}\n#define fopen fopen_compat\n#endif\n\nin git-compat-util.h, right?\n\nYeah, I can see that, even if I think the overhead would not be _that_ \ncrucial. But it is a nice way of fixing _all_ fopen() calls at the same \ntime.\n\nCiao,\nDscho\n"},{"id":"67997","messageId":"alpine.LSU.1.00.0802082040010.11591@racer.site","threadId":"11971","inReplyTo":"7v8x1vjiic.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-08T20:40:26Z","receivedAt":"2008-02-08T20:40:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Feb 2008, Junio C Hamano wrote:\n\n> \t#ifdef FOPEN_OPENS_DIRECTORIES\n\nFunny... our emails crossed, and you picked the same name ;-)\n\nCiao,\nDscho\n"},{"id":"68000","messageId":"alpine.LSU.1.00.0802082042440.11591@racer.site","threadId":"11971","inReplyTo":"alpine.LNX.1.00.0802081526350.13593@iabervon.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-08T20:44:10Z","receivedAt":"2008-02-08T20:44:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Feb 2008, Daniel Barkalow wrote:\n\n> On Fri, 8 Feb 2008, Mike Ralphson wrote:\n> \n> > On Feb 8, 2008 4:46 PM, H.Merijn Brand wrote:\n> > > HP-UX allows directories to be opened with fopen (path, \"r\"), which\n> > > will cause some translations that expect to read files, read dirs\n> > > instead. This patch makes sure the two fopen () calls in remote.c\n> > > only open the file if it is a file.\n> > >\n> > > Signed-off-by: H.Merijn Brand \n> > \n> > Many thanks, this is also required for AIX. I had got some way to\n> > tracking it down, but I thought it was an issue with strbuf. So...\n> \n> Does the following help? We really ought to know that \"..\" must be a path \n> literal (and there obviously should be more limitations on nicknames for \n> remotes, but I haven't figured out what they should be yet).\n> \n> \t-Daniel\n> *This .sig left intentionally blank*\n> \n> diff --git a/remote.c b/remote.c\n> index 0e00680..83a3d9d 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)\n> if (!name)\n> name = default_remote_name;\n> ret = make_remote(name, 0);\n> - if (name[0] != '/') {\n> + if (name[0] != '/' && strcmp(name, \"..\")) {\n> if (!ret->url)\n> read_remotes_file(ret);\n> if (!ret->url)\n\nYou'll need to check for \".\", too: \"git pull . \" was originally \nthe only way to merge a local branch, and it is still valid.\n\nCiao,\nDscho\n"},{"id":"68001","messageId":"47ACC261.6060404@nrlssc.navy.mil","threadId":"11971","inReplyTo":"7v8x1vjiic.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-02-08T20:58:09Z","receivedAt":"2008-02-08T20:58:09Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n\n> And have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate\n> platforms.\n\nWhich ones _don't_ open directories?\n\nShouldn't fopen(\"path_to_some_directory\", \"r\") work?\n\n-brandon\n\n#include \n\nint main(int argc, char* argv[])\n{\n if (!fopen(argv[1], \"r\")) {\n perror(\"File open failed\");\n return 1;\n }\n\n puts(\"File open succeeded.\");\n\n return 0;\n}\n"},{"id":"68004","messageId":"47ACC64E.7040102@nrlssc.navy.mil","threadId":"11971","inReplyTo":"47ACC261.6060404@nrlssc.navy.mil","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-02-08T21:14:54Z","receivedAt":"2008-02-08T21:14:54Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Brandon Casey wrote:\n> Junio C Hamano wrote:\n> \n>> And have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate\n>> platforms.\n> \n> Which ones _don't_ open directories?\n> \n> Shouldn't fopen(\"path_to_some_directory\", \"r\") work?\n\nOk. It's the FOPEN_OPENS_DIRECTORIES term that confused me.\n\nfopen is expected to succeed, even on directories. It's the read\nthat should fail but is not on HPUX. Obviously we don't want to\nchange the read.\n\n-brandon\n"},{"id":"68005","messageId":"7vwspfi1t4.fsf@gitster.siamese.dyndns.org","threadId":"11971","inReplyTo":"alpine.LSU.1.00.0802082040010.11591@racer.site","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-08T21:19:19Z","receivedAt":"2008-02-08T21:19:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 8 Feb 2008, Junio C Hamano wrote:\n>\n>> \t#ifdef FOPEN_OPENS_DIRECTORIES\n>\n> Funny... our emails crossed, and you picked the same name ;-)\n\nBad Dscho.\n\nIt has been a very well kept secret that Dscho and Junio are one\nand the same person, but you just spilled the beans.\n\n;-)\n"},{"id":"68012","messageId":"alpine.LSU.1.00.0802082146400.11591@racer.site","threadId":"11971","inReplyTo":"7vwspfi1t4.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-08T21:47:18Z","receivedAt":"2008-02-08T21:47:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Feb 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Fri, 8 Feb 2008, Junio C Hamano wrote:\n> >\n> >> \t#ifdef FOPEN_OPENS_DIRECTORIES\n> >\n> > Funny... our emails crossed, and you picked the same name ;-)\n> \n> Bad Dscho.\n> \n> It has been a very well kept secret that Dscho and Junio are one\n> and the same person, but you just spilled the beans.\n\nShush... left-hemisphere: shut up.\n\nCiao,\nDscho\n\nP.S.: double ;-)\n"},{"id":"68039","messageId":"47ACFFD9.2030705@nrlssc.navy.mil","threadId":"11971","inReplyTo":"7v8x1vjiic.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-02-09T01:20:25Z","receivedAt":"2008-02-09T01:20:25Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Junio C Hamano wrote:\n> \"Morten Welinder\" writes:\n> \n>>> +/* Helper function to ensure that we are opening a file and not a directory */\n>>> +static FILE *open_file(char *full_path)\n>>> +{\n>>> + struct stat st_buf;\n>>> + if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))\n>>> + return NULL;\n>>> + return (fopen(full_path, \"r\"));\n>>> +}\n>> That looks wrong. stat+fopen has a pointless race condition that\n>> open+fstat+fdopen would not have.\n> \n> That's true. How about doing something like this?\n> \n> (1) in a new file \"compat/gitfopen.c\" have this:\n> \n> \t#include \"../git-compat-util.h\"\n> \t#undef fopen\n> \tFILE *gitfopen(const char *path, const char *mode)\n> {\n> \t\tint fd, flags;\n> struct stat st;\n> \tif (mode[0] == 'w')\n> \treturn fopen(path, mode);\n> \t\tswitch (mode[0]) {\n> case 'r': flags = O_RDONLY; break;\n> case 'a': flags = O_APPEND; break;\n> \t\tdefault:\n> \t\t\terrno = EINVAL;\n> \treturn NULL;\n> \t\t}\n> \t\tfd = open(path, flags);\n> \t\tif (fd < 0 || fstat(fd, &st))\n> \treturn NULL;\n> \t\tif (S_ISDIR(st_buf.st_mode)) {\n> \terrno = EISDIR;\n> return NULL;\n> \t\t}\n> \t\treturn fdopen(fd, mode);\n> \t}\n\nCan we use fileno()? Something like:\n\nFILE *gitfopen(const char *path, const char *mode)\n{ \n FILE *fp;\n struct stat st;\n\n if (strpbrk(mode, \"wa\"))\n return fopen(path, mode);\n\n if (!(fp = fopen(path, mode)))\n return NULL;\n\n if (fstat(fileno(fp), &st)) {\n fclose(fp);\n return NULL;\n }\n\n if (S_ISDIR(st.st_mode)) {\n fclose(fp);\n errno = EISDIR;\n return NULL;\n }\n\n return fp;\n} \n\n-brandon\n"},{"id":"68041","messageId":"47AD10CF.1040207@nrlssc.navy.mil","threadId":"11971","inReplyTo":"47ACFFD9.2030705@nrlssc.navy.mil","subject":"[PATCH] Add compat/fopen.c which returns NULL on attempt to open directory","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-02-09T02:32:47Z","receivedAt":"2008-02-09T02:32:47Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Some systems do not fail as expected when fread et al. are called on\na directory stream. Replace fopen on such systems which will fail\nwhen the supplied path is a directory.\n\nSigned-off-by: Brandon Casey \n---\n Makefile | 7 +++++++\n compat/fopen.c | 26 ++++++++++++++++++++++++++\n git-compat-util.h | 5 +++++\n 3 files changed, 38 insertions(+), 0 deletions(-)\n create mode 100644 compat/fopen.c\n\ndiff --git a/Makefile b/Makefile\nindex 92341c4..debfc23 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3,6 +3,9 @@ all::\n \n # Define V=1 to have a more verbose compile.\n #\n+# Define FREAD_READS_DIRECTORIES if your are on a system which succeeds\n+# when attempting to read from an fopen'ed directory.\n+#\n # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n # This also implies MOZILLA_SHA1.\n #\n@@ -618,6 +621,10 @@ endif\n ifdef NO_C99_FORMAT\n \tBASIC_CFLAGS += -DNO_C99_FORMAT\n endif\n+ifdef FREAD_READS_DIRECTORIES\n+\tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n+\tCOMPAT_OBJS += compat/fopen.o\n+endif\n ifdef NO_SYMLINK_HEAD\n \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n endif\ndiff --git a/compat/fopen.c b/compat/fopen.c\nnew file mode 100644\nindex 0000000..ccb9e89\n--- /dev/null\n+++ b/compat/fopen.c\n@@ -0,0 +1,26 @@\n+#include \"../git-compat-util.h\"\n+#undef fopen\n+FILE *git_fopen(const char *path, const char *mode)\n+{\n+\tFILE *fp;\n+\tstruct stat st;\n+\n+\tif (mode[0] == 'w' || mode[0] == 'a')\n+\t\treturn fopen(path, mode);\n+\n+\tif (!(fp = fopen(path, mode)))\n+\t\treturn NULL;\n+\n+\tif (fstat(fileno(fp), &st)) {\n+\t\tfclose(fp);\n+\t\treturn NULL;\n+\t}\n+\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\tfclose(fp);\n+\t\terrno = EISDIR;\n+\t\treturn NULL;\n+\t}\n+\n+\treturn fp;\n+}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 4df90cb..46d5e93 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -204,6 +204,11 @@ void *gitmemmem(const void *haystack, size_t haystacklen,\n const void *needle, size_t needlelen);\n #endif\n \n+#ifdef FREAD_READS_DIRECTORIES\n+#define fopen(a,b) git_fopen(a,b)\n+extern FILE *git_fopen(const char*, const char*);\n+#endif\n+\n #ifdef __GLIBC_PREREQ\n #if __GLIBC_PREREQ(2, 1)\n #define HAVE_STRCHRNUL\n-- \n1.5.4.26.g5ef4da\n"},{"id":"68047","messageId":"7vzluag1bi.fsf@gitster.siamese.dyndns.org","threadId":"11971","inReplyTo":"47ACC261.6060404@nrlssc.navy.mil","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-09T05:12:49Z","receivedAt":"2008-02-09T05:12:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Junio C Hamano wrote:\n>\n>> And have Makefile set FOPEN_OPENS_DIRECTORIES on appropriate\n>> platforms.\n>\n> Which ones _don't_ open directories?\n\nAhh, sorry, of course you are right. We need to fix the\ncallers.\n"},{"id":"68048","messageId":"7vk5leg0nj.fsf@gitster.siamese.dyndns.org","threadId":"11971","inReplyTo":"alpine.LNX.1.00.0802081526350.13593@iabervon.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-09T05:27:12Z","receivedAt":"2008-02-09T05:27:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> diff --git a/remote.c b/remote.c\n> index 0e00680..83a3d9d 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)\n> if (!name)\n> name = default_remote_name;\n> ret = make_remote(name, 0);\n> - if (name[0] != '/') {\n> + if (name[0] != '/' && strcmp(name, \"..\")) {\n> if (!ret->url)\n> read_remotes_file(ret);\n> if (!ret->url)\n\nPerhaps \"static int valid_remote_nick(const char*)\" is needed?\nI'd say we can limit it to something like:\n\nstatic int valid_remote_nick(const char *name)\n{\n\tif (!name[0] || /* not empty */\n (name[0] == '.' && /* not \".\" */\n (!name[1] || /* not \"..\" */\n (name[1] == '.' && !name[2]))))\n\t\treturn 0;\n\treturn !!strchr(name, '/'); /* no slash */\n}\n"},{"id":"68053","messageId":"alpine.LNX.1.00.0802090053420.13593@iabervon.org","threadId":"11971","inReplyTo":"7vk5leg0nj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-02-09T05:54:08Z","receivedAt":"2008-02-09T05:54:08Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 8 Feb 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> > diff --git a/remote.c b/remote.c\n> > index 0e00680..83a3d9d 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -348,7 +348,7 @@ struct remote *remote_get(const char *name)\n> > if (!name)\n> > name = default_remote_name;\n> > ret = make_remote(name, 0);\n> > - if (name[0] != '/') {\n> > + if (name[0] != '/' && strcmp(name, \"..\")) {\n> > if (!ret->url)\n> > read_remotes_file(ret);\n> > if (!ret->url)\n> \n> Perhaps \"static int valid_remote_nick(const char*)\" is needed?\n> I'd say we can limit it to something like:\n> \n> static int valid_remote_nick(const char *name)\n> {\n> \tif (!name[0] || /* not empty */\n> (name[0] == '.' && /* not \".\" */\n> (!name[1] || /* not \"..\" */\n> (name[1] == '.' && !name[2]))))\n> \t\treturn 0;\n> \treturn !!strchr(name, '/'); /* no slash */\n> }\n\nYeah, that looks right to me.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"68061","messageId":"20080209110341.2337a19d@pc09.procura.nl","threadId":"11971","inReplyTo":"7vhcgjjjlh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-09T10:03:41Z","receivedAt":"2008-02-09T10:03:41Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Fri, 08 Feb 2008 12:09:46 -0800, Junio C Hamano wrote:\n\n> \"H.Merijn Brand\" writes:\n> \n> > HP-UX allows directories to be opened with fopen (path, \"r\"), which\n> > will cause some translations that expect to read files, read dirs\n> > instead. This patch makes sure the two fopen () calls in remote.c\n> > only open the file if it is a file.\n> \n> > +static FILE *open_file(char *full_path)\n> > +{\n> > + struct stat st_buf;\n> > + if (stat(full_path, &st_buf) || !S_ISREG(st_buf.st_mode))\n> > + return NULL;\n> > + return (fopen(full_path, \"r\"));\n> > +}\n> \n> Can we make this a platform specific \"compat\" hack?\n> \n> It is not fair to force stat() overhead to ports on platforms\n> that fails fopen() on directories,\n\nThe two I patched were in remote.c and do not happen on every file if I\nanalyzed it correctly, so overhead would be minimal. However, as I read\nthe rest of the discussion already, your approach to fix all fopen ()\ncalls at once seems very reasonable.\n\nCan I get the patch when it is submitted?\n\n> as I doubt we would ever want from directory using fopen() anyway.\n\nI didn't check\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"68349","messageId":"20080211102950.122ba93d@pc09.procura.nl","threadId":"11971","inReplyTo":"47AD10CF.1040207@nrlssc.navy.mil","subject":"Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-11T09:29:50Z","receivedAt":"2008-02-11T09:29:50Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Fri, 08 Feb 2008 20:32:47 -0600, Brandon Casey \nwrote:\n\n> Some systems do not fail as expected when fread et al. are called on\n> a directory stream. Replace fopen on such systems which will fail\n> when the supplied path is a directory.\n\nI applied this patch instead of mine, and added the Makefile define\nHarder to trace, as it is not issuing error messages, but could this\nsuccess^Wfailure be related?\n\n/pro/3gl/LINUX/git-1.5.4.rc5 103 > cat t/t5701-clone-local.sh.err\n* ok 1: preparing origin repository\n* ok 2: local clone without .git suffix\n* ok 3: local clone with .git suffix\n* ok 4: local clone from x\n* FAIL 5: local clone from x.git that does not exist\n\n cd \"$D\" &&\n if git clone -l -s x.git z\n then\n echo \"Oops, should have failed\"\n false\n else\n echo happy\n fi\n\n* ok 6: With -no-hardlinks, local will make a copy\n* ok 7: Even without -l, local will make a hardlink\n* failed 1 among 7 test(s)\n\nAny hints in where to start digging?\n\n> Signed-off-by: Brandon Casey \n> ---\n> Makefile | 7 +++++++\n> compat/fopen.c | 26 ++++++++++++++++++++++++++\n> git-compat-util.h | 5 +++++\n> 3 files changed, 38 insertions(+), 0 deletions(-)\n> create mode 100644 compat/fopen.c\n> \n> diff --git a/Makefile b/Makefile\n> index 92341c4..debfc23 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3,6 +3,9 @@ all::\n> \n> # Define V=1 to have a more verbose compile.\n> #\n> +# Define FREAD_READS_DIRECTORIES if your are on a system which succeeds\n> +# when attempting to read from an fopen'ed directory.\n> +#\n> # Define NO_OPENSSL environment variable if you do not have OpenSSL.\n> # This also implies MOZILLA_SHA1.\n> #\n> @@ -618,6 +621,10 @@ endif\n> ifdef NO_C99_FORMAT\n> \tBASIC_CFLAGS += -DNO_C99_FORMAT\n> endif\n> +ifdef FREAD_READS_DIRECTORIES\n> +\tCOMPAT_CFLAGS += -DFREAD_READS_DIRECTORIES\n> +\tCOMPAT_OBJS += compat/fopen.o\n> +endif\n> ifdef NO_SYMLINK_HEAD\n> \tBASIC_CFLAGS += -DNO_SYMLINK_HEAD\n> endif\n> diff --git a/compat/fopen.c b/compat/fopen.c\n> new file mode 100644\n> index 0000000..ccb9e89\n> --- /dev/null\n> +++ b/compat/fopen.c\n> @@ -0,0 +1,26 @@\n> +#include \"../git-compat-util.h\"\n> +#undef fopen\n> +FILE *git_fopen(const char *path, const char *mode)\n> +{\n> +\tFILE *fp;\n> +\tstruct stat st;\n> +\n> +\tif (mode[0] == 'w' || mode[0] == 'a')\n> +\t\treturn fopen(path, mode);\n> +\n> +\tif (!(fp = fopen(path, mode)))\n> +\t\treturn NULL;\n> +\n> +\tif (fstat(fileno(fp), &st)) {\n> +\t\tfclose(fp);\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\tif (S_ISDIR(st.st_mode)) {\n> +\t\tfclose(fp);\n> +\t\terrno = EISDIR;\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\treturn fp;\n> +}\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 4df90cb..46d5e93 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -204,6 +204,11 @@ void *gitmemmem(const void *haystack, size_t haystacklen,\n> const void *needle, size_t needlelen);\n> #endif\n> \n> +#ifdef FREAD_READS_DIRECTORIES\n> +#define fopen(a,b) git_fopen(a,b)\n> +extern FILE *git_fopen(const char*, const char*);\n> +#endif\n> +\n> #ifdef __GLIBC_PREREQ\n> #if __GLIBC_PREREQ(2, 1)\n> #define HAVE_STRCHRNUL\n\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"68360","messageId":"20080211111537.2bf47448@pc09.procura.nl","threadId":"11971","inReplyTo":"20080211102950.122ba93d@pc09.procura.nl","subject":"Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-11T10:15:37Z","receivedAt":"2008-02-11T10:15:37Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Mon, 11 Feb 2008 10:29:50 +0100, \"H.Merijn Brand\" \nwrote:\n\n> On Fri, 08 Feb 2008 20:32:47 -0600, Brandon Casey \n> wrote:\n> \n> > Some systems do not fail as expected when fread et al. are called on\n> > a directory stream. Replace fopen on such systems which will fail\n> > when the supplied path is a directory.\n> \n> I applied this patch instead of mine, and added the Makefile define\n> Harder to trace, as it is not issuing error messages, but could this\n> success^Wfailure be related?\n\nNo, it is not. Some shell weirdness. This fixes it. Don't know off-hand\nif it is portable enough\n\ndiff -pur a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\n--- a/t/t5701-clone-local.sh 2008-02-02 05:09:01 +0100\n+++ b/t/t5701-clone-local.sh 2008-02-11 11:13:26 +0100\n@@ -37,8 +37,8 @@ test_expect_success 'local clone from x'\n\n test_expect_success 'local clone from x.git that does not exist' '\n cd \"$D\" &&\n- if git clone -l -s x.git z\n- then\n+ git clone -l -s x.git z\n+ if $? ; then\n echo \"Oops, should have failed\"\n false\n else\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"68468","messageId":"7v8x1r6n62.fsf@gitster.siamese.dyndns.org","threadId":"11971","inReplyTo":"20080211111537.2bf47448@pc09.procura.nl","subject":"Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-12T00:20:05Z","receivedAt":"2008-02-12T00:20:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H.Merijn Brand\" writes:\n\n> No, it is not. Some shell weirdness. This fixes it. Don't know off-hand\n> if it is portable enough\n>\n> diff -pur a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\n> --- a/t/t5701-clone-local.sh 2008-02-02 05:09:01 +0100\n> +++ b/t/t5701-clone-local.sh 2008-02-11 11:13:26 +0100\n> @@ -37,8 +37,8 @@ test_expect_success 'local clone from x'\n>\n> test_expect_success 'local clone from x.git that does not exist' '\n> cd \"$D\" &&\n> - if git clone -l -s x.git z\n> - then\n> + git clone -l -s x.git z\n> + if $? ; then\n> echo \"Oops, should have failed\"\n> false\n> else\n\nI think your \"git clone\" is broken and I strongly suspect it is\nnot your shell (at least the \"if\" construct in the test).\n\nWhat's \n\n\tif $?; then\n\nIn sane shells, I think this tries to execute 0 or perhaps 124\nor whatever the error code from clone as if it was the name of a\ncommand, which would most likely fail and would not take \"then\"\npart (which reports the error). It did not fix, but just made\nit ignore the error from \"git clone\".\n\nIf it were\n\n\tif test $? != 0\n then\n\nit would have made a bit more sense.\n\nAnd if (this is a big \"if\" as I doubt any shell is so broken)\nthese two are equivalent to your shell, then I do not think it\nis portable at all.\n"},{"id":"68523","messageId":"20080212162722.1d98e05d@pc09.procura.nl","threadId":"11971","inReplyTo":"7v8x1r6n62.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Add compat/fopen.c which returns NULL on attempt to open directory","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-12T15:27:22Z","receivedAt":"2008-02-12T15:27:22Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Mon, 11 Feb 2008 16:20:05 -0800, Junio C Hamano wrote:\n\n> \"H.Merijn Brand\" writes:\n> \n> > No, it is not. Some shell weirdness. This fixes it. Don't know off-hand\n> > if it is portable enough\n> >\n> > diff -pur a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh\n> > --- a/t/t5701-clone-local.sh 2008-02-02 05:09:01 +0100\n> > +++ b/t/t5701-clone-local.sh 2008-02-11 11:13:26 +0100\n> > @@ -37,8 +37,8 @@ test_expect_success 'local clone from x'\n> >\n> > test_expect_success 'local clone from x.git that does not exist' '\n> > cd \"$D\" &&\n> > - if git clone -l -s x.git z\n> > - then\n> > + git clone -l -s x.git z\n> > + if $? ; then\n> > echo \"Oops, should have failed\"\n> > false\n> > else\n> \n> I think your \"git clone\" is broken and I strongly suspect it is\n> not your shell (at least the \"if\" construct in the test).\n\nof course it should have been 'if test $?' and as $? is erroneously\nequal to 0 in this case, this snippet doesn't matter\n\n'git clone' is calling 'cit-clone' which is a shell script, that does\nexit 1 in the function die:\n--8<---\ndie() {\n\techo >&2 \"$@\"\n\texit 1\n}\n-->8---\n\nbut somehow that exit code gets lost\n\n> What's \n> \n> \tif $?; then\n> \n> In sane shells, I think this tries to execute 0 or perhaps 124\n> or whatever the error code from clone as if it was the name of a\n> command, which would most likely fail and would not take \"then\"\n> part (which reports the error). It did not fix, but just made\n> it ignore the error from \"git clone\".\n> \n> If it were\n> \n> \tif test $? != 0\n> then\n> \n> it would have made a bit more sense.\n> \n> And if (this is a big \"if\" as I doubt any shell is so broken)\n> these two are equivalent to your shell, then I do not think it\n> is portable at all.\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"69108","messageId":"20080218101026.6098667f@pc09.procura.nl","threadId":"11971","inReplyTo":"20080208210447.289022b6@pc09.procura.nl","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-18T09:10:26Z","receivedAt":"2008-02-18T09:10:26Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Fri, 8 Feb 2008 21:04:47 +0100, \"H.Merijn Brand\" \nwrote:\n\n> $ cat do-tests\n> #!/bin/sh\n> \n> export TAR=ntar\n> rm -f *.err\n> for t in t[0-9]*.sh ; do\n> echo $t\n> sh $t > test.err 2>&1 || mv test.err $t.err\n> rm -f test.err\n> done\n> $\n> \n> 197509 -rw-rw-rw- 1 merijn softwr 1633 Feb 8 18:03 t5302-pack-index.sh.err\n> 196846 -rw-rw-rw- 1 merijn softwr 943 Feb 8 18:04 t5500-fetch-pack.sh.err\n> 203431 -rw-rw-rw- 1 merijn softwr 344 Feb 8 18:05 t5600-clone-fail-cleanup.sh.err\n> 202602 -rw-rw-rw- 1 merijn softwr 458 Feb 8 18:05 t5701-clone-local.sh.err\n> 202761 -rw-rw-rw- 1 merijn softwr 3039 Feb 8 18:06 t6002-rev-list-bisect.sh.err\n> 202641 -rw-rw-rw- 1 merijn softwr 3980 Feb 8 18:06 t6003-rev-list-topo-order.sh.err\n> 202731 -rw-rw-rw- 1 merijn softwr 899 Feb 8 18:06 t6022-merge-rename.sh.err\n> 197510 -rw-rw-rw- 1 merijn softwr 1340 Feb 8 18:08 t7201-co.sh.err\n> 202705 -rw-rw-rw- 1 merijn softwr 149 Feb 8 18:09 t9300-fast-import.sh.err\n> 197051 -rw-rw-rw- 1 merijn softwr 1651 Feb 8 18:09 t9301-fast-export.sh.err\n> \n> http://www.xs4all.nl/~procura/git-1.5.3-1123ipf.tar\n> \n> Tips welcome :)\n\nMost bizarre workaround found for clone (the first 4 failures):\n--8<---\ndiff -pur /a5/pro/3gl/LINUX/git-1.5.4/git-clone.sh git-clone.sh\n--- a/git-1.5.4/git-clone.sh 2008-02-02 05:09:01 +0100\n+++ b/git-1.5.4/git-clone.sh 2008-02-18 10:03:26 +0100\n@@ -368,7 +368,8 @@ yes)\n '') git-fetch-pack --all -k $quiet $depth $no_progress \"$repo\";;\n *) git-fetch-pack --all -k $quiet \"$upload_pack\" $depth $no_progress \"$repo\" ;;\n esac >\"$GIT_DIR/CLONE_HEAD\" ||\n- die \"fetch-pack from '$repo' failed.\"\n+ exit 1\n+ # die \"fetch-pack from '$repo' failed.\"\n ;;\n esac\n ;;\n-->8---\n\n4 down, 6 to go\n197225 -rw-rw-rw- 1 merijn softwr 7246 Feb 18 09:58 t6002-rev-list-bisect.sh.err\n197111 -rw-rw-rw- 1 merijn softwr 10763 Feb 18 09:58 t6003-rev-list-topo-order.sh.err\n197190 -rw-rw-rw- 1 merijn softwr 17903 Feb 18 09:58 t6022-merge-rename.sh.err\n196841 -rw-rw-rw- 1 merijn softwr 9299 Feb 18 10:00 t7201-co.sh.err\n196928 -rw-rw-rw- 1 merijn softwr 484 Feb 18 10:01 t9300-fast-import.sh.err\n196683 -rw-rw-rw- 1 merijn softwr 5035 Feb 18 10:01 t9301-fast-export.sh.err\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"},{"id":"69110","messageId":"7v3arqr4qo.fsf@gitster.siamese.dyndns.org","threadId":"11971","inReplyTo":"20080218101026.6098667f@pc09.procura.nl","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-18T09:30:39Z","receivedAt":"2008-02-18T09:30:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"H.Merijn Brand\" writes:\n\n> Most bizarre workaround found for clone (the first 4 failures):\n> --8<---\n> diff -pur /a5/pro/3gl/LINUX/git-1.5.4/git-clone.sh git-clone.sh\n> --- a/git-1.5.4/git-clone.sh 2008-02-02 05:09:01 +0100\n> +++ b/git-1.5.4/git-clone.sh 2008-02-18 10:03:26 +0100\n> @@ -368,7 +368,8 @@ yes)\n> '') git-fetch-pack --all -k $quiet $depth $no_progress \"$repo\";;\n> *) git-fetch-pack --all -k $quiet \"$upload_pack\" $depth $no_progress \"$repo\" ;;\n> esac >\"$GIT_DIR/CLONE_HEAD\" ||\n> - die \"fetch-pack from '$repo' failed.\"\n> + exit 1\n> + # die \"fetch-pack from '$repo' failed.\"\n> ;;\n> esac\n> ;;\n\nThat sounds *very* broken.\n\nIs your /bin/sh really a variant of Bourne?\n\nIf HP-UX is broken in a similar way as Solaris is, in that it\ninstalls a non-POSIX shell under /bin/sh and offers a Korn in\n/bin/ksh, \"make SHELL_PATH=/bin/ksh\" may help.\n"},{"id":"69122","messageId":"20080218123157.17c91f20@pc09.procura.nl","threadId":"11971","inReplyTo":"7v3arqr4qo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] opening files in remote.c should ensure it is opening a file","fromName":"H.Merijn Brand","fromEmail":"h.m.brand@xs4all.nl","sentAt":"2008-02-18T11:31:57Z","receivedAt":"2008-02-18T11:31:57Z","isPatch":true,"sender":{"key":"h.m.brand@xs4all.nl","avatar":"https://gravatar.com/avatar/5b8f83ee35c427a646cbea3b104346e00ab3663b99bbf435cddeb75cd4b3857b?d=mp&s=160"},"body":"On Mon, 18 Feb 2008 01:30:39 -0800, Junio C Hamano wrote:\n\n> \"H.Merijn Brand\" writes:\n> \n> > Most bizarre workaround found for clone (the first 4 failures):\n> > --8<---\n> > diff -pur /a5/pro/3gl/LINUX/git-1.5.4/git-clone.sh git-clone.sh\n> > --- a/git-1.5.4/git-clone.sh 2008-02-02 05:09:01 +0100\n> > +++ b/git-1.5.4/git-clone.sh 2008-02-18 10:03:26 +0100\n> > @@ -368,7 +368,8 @@ yes)\n> > '') git-fetch-pack --all -k $quiet $depth $no_progress \"$repo\";;\n> > *) git-fetch-pack --all -k $quiet \"$upload_pack\" $depth $no_progress \"$repo\" ;;\n> > esac >\"$GIT_DIR/CLONE_HEAD\" ||\n> > - die \"fetch-pack from '$repo' failed.\"\n> > + exit 1\n> > + # die \"fetch-pack from '$repo' failed.\"\n> > ;;\n> > esac\n> > ;;\n> \n> That sounds *very* broken.\n\nIndeed, and trying to see if eval or exiting from with a sub caused\nthis weird behaviour, I failed to come up with a simple test script\nto prove this.\n\n> Is your /bin/sh really a variant of Bourne?\n\nYes\n\n NAME\n sh - overview of various system shells\n\n SYNOPSIS\n POSIX Shell:\n sh [+-aefhikmnoprstuvx] [+-o option] ... [-c string] [arg ...]\n\n rsh [+-aefhikmnoprstuvx] [+-o option] ... [-c string] [arg ...]\n\n Korn Shell:\n ksh [+-aefhikmnoprstuvx] [+-o option] ... [-c string] [arg ...]\n\n rksh [+-aefhikmnoprstuvx] [+-o option] ... [-c string] [arg ...]\n\n C Shell:\n csh [-cefinstvxTVX] [command_file] [argument_list ...]\n\n Key Shell:\n keysh\n\n> If HP-UX is broken in a similar way as Solaris is, in that it\n> installs a non-POSIX shell under /bin/sh and offers a Korn in\n> /bin/ksh, \"make SHELL_PATH=/bin/ksh\" may help.\n\n$ path -al sh ksh\n 27231 100555 -r-x 2 bin 586136 27 Aug 2004 03:36 /usr/bin/sh\n 1744 100555 -r-x 1 bin 1219780 27 Aug 2004 03:36 /sbin/sh\n 3206 100555 -r-x 2 bin 446904 27 Aug 2004 03:20 /usr/bin/ksh\n\nAnd running all with ksh only makes things worse!\n\n$ cat t0000-basic.sh.err\nt0000-basic.sh[31]: !: not found\ntest_expect_success[31]: !: not found\ntest_expect_success[31]: !: not found\ntest_expect_failure[31]: !: not found\ntest_expect_success[31]: !: not found\ntest_expect_success[31]: !: not found\ntest_expect_success[31]: !: not found\ntest_expect_failure[31]: !: not found\n:\n:\n\n\n\n-- \nH.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/)\nusing & porting perl 5.6.2, 5.8.x, 5.10.x on HP-UX 10.20, 11.00, 11.11,\n& 11.23, SuSE 10.1 & 10.2, AIX 5.2, and Cygwin. http://qa.perl.org\nhttp://mirrors.develooper.com/hpux/ http://www.test-smoke.org\n http://www.goldmark.org/jeff/stupid-disclaimers/\n"}]}