{"thread":{"id":"28654","subject":"[PATCH] Make is_gitfile a non-static generic function","startedAt":"2011-10-11T22:52:27Z","lastAt":"2011-10-12T19:47:51Z","messageCount":7,"participants":["Phil Hord","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"177413","messageId":"4E94C8AB.3040807@cisco.com","threadId":"28654","inReplyTo":"4E94C70E.3080003@cisco.com","subject":"[PATCH] Make is_gitfile a non-static generic function","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2011-10-11T22:52:27Z","receivedAt":"2011-10-11T22:52:27Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"The new is_gitfile is an amalgam of similar functional checks\nfrom different places in the code.  All these places do\nslightly different checks on the suspect gitfile (for their\nown good reasons).  But the lack of common code leads to bugs\nand maintenance problems. Help move the code forward by making\nis_gitfile() a common function that everyone can use for this\npurpose.\n\nSigned-off-by: Phil Hord <hordp@cisco.com>\n---\n\nI'm not sure this function belongs in transport.c anymore, but\nI left it here to minimize conflicts.  I think a better home would\nbe path.c, but maybe not.  If someone has a preference,\nplease let me know.\n\n builtin/clone.c |    8 +-------\n cache.h         |    1 +\n transport.c     |    6 +++++-\n 3 files changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 488f48e..5110399 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -120,13 +120,7 @@ static char *get_repo_path(const char *repo, int\n*is_bundle)\n \t\t\treturn xstrdup(absolute_path(path));\n \t\t} else if (S_ISREG(st.st_mode) && st.st_size > 8) {\n \t\t\t/* Is it a \"gitfile\"? */\n-\t\t\tchar signature[8];\n-\t\t\tint len, fd = open(path, O_RDONLY);\n-\t\t\tif (fd < 0)\n-\t\t\t\tcontinue;\n-\t\t\tlen = read_in_full(fd, signature, 8);\n-\t\t\tclose(fd);\n-\t\t\tif (len != 8 || strncmp(signature, \"gitdir: \", 8))\n+\t\t\tif (!is_gitfile(path))\n \t\t\t\tcontinue;\n \t\t\tpath = read_gitfile(path);\n \t\t\tif (path) {\ndiff --git a/cache.h b/cache.h\nindex 601f6f6..7a8d9f9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -441,6 +441,7 @@ extern const char *get_git_work_tree(void);\n extern const char *read_gitfile(const char *path);\n extern const char *resolve_gitdir(const char *suspect);\n extern void set_git_work_tree(const char *tree);\n+extern int is_gitfile(const char *path);\n  #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n diff --git a/transport.c b/transport.c\nindex f3195c0..d08a826 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -859,7 +859,11 @@ static int is_local(const char *url)\n \t\thas_dos_drive_prefix(url);\n }\n -static int is_gitfile(const char *url)\n+/*\n+ * See if the referenced file looks like a 'gitfile'.\n+ * Does not try to determine if the referenced gitdir is actually valid.\n+ */\n+int is_gitfile(const char *url)\n {\n \tstruct stat st;\n \tchar buf[9];\n-- \n1.7.7.334.gfc143d\n"},{"id":"177418","messageId":"7vsjmzcdpf.fsf@alter.siamese.dyndns.org","threadId":"28654","inReplyTo":"4E94C8AB.3040807@cisco.com","subject":"Re: [PATCH] Make is_gitfile a non-static generic function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T23:26:20Z","receivedAt":"2011-10-11T23:26:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <hordp@cisco.com> writes:\n\n> I'm not sure this function belongs in transport.c anymore, but\n> I left it here to minimize conflicts.  I think a better home would\n> be path.c, but maybe not.  If someone has a preference,\n> please let me know.\n\nI would think either transport.c or setup.c is more appropriate than\npath.c; the last one is more of \"pathname manipulation utility bag of\nfunctions\" and does not have much to do with the \"Git\"-ness of the path\nthey deal with.\n\nI am not sure if is_gitfile() is a good name for a global function,\nthough.  Also I think the interface to the function should be updated so\nthat the caller can choose to receive the target path when the function\nreturns with positive answer, making \"read_gitfile()\" unnecessary for such\na caller.\n\nAs a matter of fact, couldn't we somehow unify these two slightly\ndifferent implementations around the same theme, making is_gitfile()\nfunction unnecessary? As far as I can tell, the only difference between\nthese functions is how they fail when given a non-gitfile, and many\ncallers just call read_gitfile() without first asking if it is a gitfile.\n"},{"id":"177419","messageId":"7vipnvccso.fsf@alter.siamese.dyndns.org","threadId":"28654","inReplyTo":"4E94C8AB.3040807@cisco.com","subject":"Re: [PATCH] Make is_gitfile a non-static generic function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T23:45:59Z","receivedAt":"2011-10-11T23:45:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <hordp@cisco.com> writes:\n\n> The new is_gitfile is an amalgam of similar functional checks\n> from different places in the code....\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 488f48e..5110399 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -120,13 +120,7 @@ static char *get_repo_path(const char *repo, int\n> *is_bundle)\n>  \t\t\treturn xstrdup(absolute_path(path));\n>  \t\t} else if (S_ISREG(st.st_mode) && st.st_size > 8) {\n>  \t\t\t/* Is it a \"gitfile\"? */\n> -\t\t\tchar signature[8];\n> -\t\t\tint len, fd = open(path, O_RDONLY);\n> -\t\t\tif (fd < 0)\n> -\t\t\t\tcontinue;\n> -\t\t\tlen = read_in_full(fd, signature, 8);\n> -\t\t\tclose(fd);\n> -\t\t\tif (len != 8 || strncmp(signature, \"gitdir: \", 8))\n> +\t\t\tif (!is_gitfile(path))\n>  \t\t\t\tcontinue;\n>  \t\t\tpath = read_gitfile(path);\n>  \t\t\tif (path) {\n> diff --git a/cache.h b/cache.h\n> index 601f6f6..7a8d9f9 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -441,6 +441,7 @@ extern const char *get_git_work_tree(void);\n>  extern const char *read_gitfile(const char *path);\n>  extern const char *resolve_gitdir(const char *suspect);\n>  extern void set_git_work_tree(const char *tree);\n> +extern int is_gitfile(const char *path);\n>   #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n>  diff --git a/transport.c b/transport.c\n> index f3195c0..d08a826 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -859,7 +859,11 @@ static int is_local(const char *url)\n>  \t\thas_dos_drive_prefix(url);\n>  }\n>  -static int is_gitfile(const char *url)\n> +/*\n> + * See if the referenced file looks like a 'gitfile'.\n> + * Does not try to determine if the referenced gitdir is actually valid.\n> + */\n> +int is_gitfile(const char *url)\n>  {\n>  \tstruct stat st;\n>  \tchar buf[9];\n\nAfter looking at this patch and the way the other caller in transport.c\nuses it, I am more and more convinced that \"is_gitfile()\" is a stupid and\nhorrible mistake.\n\nThe caller in transport.c says \"I am about to read from a regular file,\nand usually I would treat it as a bundle, but I want to avoid that\ncodepath if that regular file is not a bundle. So I use the codepath only\nwhen that file is not a gitfile\".\n\nIt should be saying \"Is it a bundle? Then I'd use the codepath to read\nfrom the bundle\" to begin with. Otherwise the code will break when we add\nyet another regular file we can fetch from that is not a bundle nor a\ngitfile.\n\nI think the hand-crafted check in builtin/clone.c you removed originated\nfrom laziness to avoid teaching read_gitfile() to read potential gitfile\nsilently (and signal errors by simply returning NULL). I also suspect the\ncodepath may become simpler if we had a way to ask \"Is this a bundle?\".\n\nI think read_bundle_header() in bundle.c can be refactored to a silent\ninterface that allows us to ask \"Is this a bundle?\" question properly.\n"},{"id":"177437","messageId":"CABURp0qDH6+S-m4nmPrfOECEVQUoXjsvz9ZQhA5PbVJEuwEL_g@mail.gmail.com","threadId":"28654","inReplyTo":"7vipnvccso.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make is_gitfile a non-static generic function","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2011-10-12T13:29:17Z","receivedAt":"2011-10-12T13:29:17Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Phil Hord <hordp@cisco.com> writes:\n>\n>> The new is_gitfile is an amalgam of similar functional checks\n>> from different places in the code....\n>\n>\n> After looking at this patch and the way the other caller in transport.c\n> uses it, I am more and more convinced that \"is_gitfile()\" is a stupid and\n> horrible mistake.\n\nI think it's a simple and low-impact change that fixes a bug with a\nminimum of disruption.  But I also think it is lazy.\n\n> The caller in transport.c says \"I am about to read from a regular file,\n> and usually I would treat it as a bundle, but I want to avoid that\n> codepath if that regular file is not a bundle. So I use the codepath only\n> when that file is not a gitfile\".\n>\n> It should be saying \"Is it a bundle? Then I'd use the codepath to read\n> from the bundle\" to begin with. Otherwise the code will break when we add\n> yet another regular file we can fetch from that is not a bundle nor a\n> gitfile.\n\nYes, and this is part of the kind of distraction that held back my\nupdate over the weekend.\n\nWhen we do add another file type we'll wind up with a half-dozen\nplaces that get affected in slightly different ways again.  Wouldn't\nit be nice to have a function to tell us what kind of thing it is\nwe've been asked to look at?  Something like git_type(url) that\nreturns GIT_BUNDLE, GIT_DIRECTORY or GIT_FILE, maybe.\n\nExcept I didn't see many examples in the code using this sort of\nenumerated decision function.\n\n> I think the hand-crafted check in builtin/clone.c you removed originated\n> from laziness to avoid teaching read_gitfile() to read potential gitfile\n> silently (and signal errors by simply returning NULL).\n\nI made a read_gitfile(... , gently) function, but I didn't like it\nmuch.  When !gently, I think it should be rather explicit about the\ntype of failure.  This makes the code look like 20% of it is repeated\n\"if (!gently) die... ;\\n return;\" sequences.  It's almost enough to\nlead me to macros.\n\nAnd what about when fopen() fails and we are running silently.  Do we\njust shrug and say \"Not a gitfile\"?  I don't think it's good enough.\nWe need to be able to say all of these:\n\n  It's a gitfile, here's the internal path.\n\n  It's not a gitfile, it is something else.\n\n  It looked like a gitfile until I ran into E_ACCES or some other error.\n\nMaking the one function run silently or not complicates the code further.\n\nI tried to find a similar style to mimic elsewhere in the code, but I\ndidn't find any consistency.  Pointers to clean examples would be\nwelcome.\n\nI started working on more of an API.  But it's still very ugly and not\nready for even a strawman discussion.\n\nBut I don't know how much time I have for a full writeup atm.  Without\nsomething, though, I cannot easily fetch from a submodule, because\nsubmodules all use gitfiles now, and git:master does not know how to\nfetch from them.\n\nAnd that's the itch I had to scratch.\n\n> I also suspect the\n> codepath may become simpler if we had a way to ask \"Is this a bundle?\".\n>\n> I think read_bundle_header() in bundle.c can be refactored to a silent\n> interface that allows us to ask \"Is this a bundle?\" question properly.\n\nI'll take a look at it.  But I won't have much time for it this week.\n\nThanks,\nPhil\n"},{"id":"177452","messageId":"CABURp0p4_oVMmkFDPue4kvhO_bEr_dBh-XFGArdSJFMz-Eboeg@mail.gmail.com","threadId":"28654","inReplyTo":"7vipnvccso.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make is_gitfile a non-static generic function","fromName":"Phil Hord","fromEmail":"phil.hord@gmail.com","sentAt":"2011-10-12T18:01:02Z","receivedAt":"2011-10-12T18:01:02Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> After looking at this patch and the way the other caller in transport.c\n> uses it, I am more and more convinced that \"is_gitfile()\" is a stupid and\n> horrible mistake.\n\nI think I misunderstood your objection before.  Now I think I\nunderstand.  Tell me if I am right.\n\n\nI think you mean that instead of this:\n        } else if (is_local(url) && is_file(url) && !is_gitfile(url)) {\n\nyou would like to see this:\n        } else if (is_local(url) && is_file(url) && is_bundle(url)) {\n\nOr maybe even this:\n        } else if (is_bundle(url)) {\n\nPhil\n"},{"id":"177455","messageId":"7vmxd69j72.fsf@alter.siamese.dyndns.org","threadId":"28654","inReplyTo":"CABURp0p4_oVMmkFDPue4kvhO_bEr_dBh-XFGArdSJFMz-Eboeg@mail.gmail.com","subject":"Re: [PATCH] Make is_gitfile a non-static generic function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T18:08:17Z","receivedAt":"2011-10-12T18:08:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Hord <phil.hord@gmail.com> writes:\n\n> On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> After looking at this patch and the way the other caller in transport.c\n>> uses it, I am more and more convinced that \"is_gitfile()\" is a stupid and\n>> horrible mistake.\n>\n> I think I misunderstood your objection before.  Now I think I\n> understand.  Tell me if I am right.\n>\n>\n> I think you mean that instead of this:\n>         } else if (is_local(url) && is_file(url) && !is_gitfile(url)) {\n>\n> you would like to see this:\n>         } else if (is_local(url) && is_file(url) && is_bundle(url)) {\n>\n> Or maybe even this:\n>         } else if (is_bundle(url)) {\n\nExactly.\n"},{"id":"177478","messageId":"4E95EEE7.9040208@cisco.com","threadId":"28654","inReplyTo":"7vmxd69j72.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Make is_gitfile a non-static generic function","fromName":"Phil Hord","fromEmail":"hordp@cisco.com","sentAt":"2011-10-12T19:47:51Z","receivedAt":"2011-10-12T19:47:51Z","isPatch":true,"sender":{"key":"phil.hord@gmail.com","avatar":"https://avatars.githubusercontent.com/u/123908?v=4"},"body":"Junio C Hamano <gitster@pobox.com> sez:\n> Phil Hord <phil.hord@gmail.com> writes:\n> \n>> On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> After looking at this patch and the way the other caller in transport.c\n>>> uses it, I am more and more convinced that \"is_gitfile()\" is a stupid and\n>>> horrible mistake.\n>>\n>> I think I misunderstood your objection before.  Now I think I\n>> understand.  Tell me if I am right.\n>>\n>>\n>> I think you mean that instead of this:\n>>         } else if (is_local(url) && is_file(url) && !is_gitfile(url)) {\n>>\n>> you would like to see this:\n>>         } else if (is_local(url) && is_file(url) && is_bundle(url)) {\n>>\n>> Or maybe even this:\n>>         } else if (is_bundle(url)) {\n> \n> Exactly.\n\nI tried to refactor read_bundle_header, but it was too ugly and I don't quite\nunderstand all the error paths.  Plus, the whole is_bundle part can be just \n10 lines of code.  Maybe read_bundle_header() should be shortened slightly to\nuse is_bundle() for code duplication avoidance.\n\n-- >8 --\nSubject: [PATCH] transport: Add/use is_bundle() to identify a url\n\nTransport currently decides that any local url which is a file\nmust be a bundle.  This is wrong now if the local url is a\ngitfile, and it will be wrong in the future when some other\nexception shows up.  Teach transport to verify the file is\nreally a bundle instead of just assuming it is so.\n---\n bundle.c    |   16 ++++++++++++++++\n bundle.h    |    1 +\n transport.c |   10 +---------\n 3 files changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex f82baae..3a64a43 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -23,6 +23,22 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name,\n \tlist->nr++;\n }\n \n+int is_bundle(const char *path)\n+{\n+\tchar buffer[100];\n+\tFILE *ffd = fopen(path, \"rb\");\n+\tint ret=1;\n+\n+\tif (!ffd)\n+\t\treturn 0;\n+\n+\tif (!fgets(buffer, sizeof(buffer), ffd) ||\n+\t\t\tstrcmp(buffer, bundle_signature))\n+\t\tret=0;\n+\tfclose(ffd);\n+\treturn ret;\n+}\n+\t\n /* returns an fd */\n int read_bundle_header(const char *path, struct bundle_header *header)\n {\ndiff --git a/bundle.h b/bundle.h\nindex c5a22c8..35aa0eb 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -14,6 +14,7 @@ struct bundle_header {\n \tstruct ref_list references;\n };\n \n+int is_bundle(const char *path);\n int read_bundle_header(const char *path, struct bundle_header *header);\n int create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv);\ndiff --git a/transport.c b/transport.c\nindex f3195c0..bcd9b74 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -881,14 +881,6 @@ static int is_gitfile(const char *url)\n \treturn !prefixcmp(buf, \"gitdir: \");\n }\n \n-static int is_file(const char *url)\n-{\n-\tstruct stat buf;\n-\tif (stat(url, &buf))\n-\t\treturn 0;\n-\treturn S_ISREG(buf.st_mode);\n-}\n-\n static int external_specification_len(const char *url)\n {\n \treturn strchr(url, ':') - url;\n@@ -929,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->fetch = fetch_objs_via_rsync;\n \t\tret->push = rsync_transport_push;\n \t\tret->smart_options = NULL;\n-\t} else if (is_local(url) && is_file(url) && !is_gitfile(url)) {\n+\t} else if (is_local(url) && is_bundle(url)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n \t\tret->get_refs_list = get_refs_from_bundle;\n-- \n1.7.7.334.g311c9.dirty\n"}]}