Re: [PATCH] Make is_gitfile a non-static generic function
From: Phil Hord <hidden>
Date: 2016-06-15 22:52:14
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] Make is_gitfile a non-static generic function · Phil Hord <hidden>
- 2016-06-15 · Re: [PATCH] Make is_gitfile a non-static generic function · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] Make is_gitfile a non-static generic function · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] Make is_gitfile a non-static generic function · Phil Hord <hidden>
- 2016-06-15 · Re: [PATCH] Make is_gitfile a non-static generic function · Phil Hord <hidden>
Junio C Hamano [off-list ref] sez:
Phil Hord [off-list ref] writes:quoted
On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano [off-list ref] wrote:quoted
After looking at this patch and the way the other caller in transport.c uses it, I am more and more convinced that "is_gitfile()" is a stupid and horrible mistake.I think I misunderstood your objection before. Now I think I understand. Tell me if I am right. I think you mean that instead of this: } else if (is_local(url) && is_file(url) && !is_gitfile(url)) { you would like to see this: } else if (is_local(url) && is_file(url) && is_bundle(url)) { Or maybe even this: } else if (is_bundle(url)) {Exactly.
I tried to refactor read_bundle_header, but it was too ugly and I don't quite understand all the error paths. Plus, the whole is_bundle part can be just 10 lines of code. Maybe read_bundle_header() should be shortened slightly to use is_bundle() for code duplication avoidance. -- >8 -- Subject: [PATCH] transport: Add/use is_bundle() to identify a url Transport currently decides that any local url which is a file must be a bundle. This is wrong now if the local url is a gitfile, and it will be wrong in the future when some other exception shows up. Teach transport to verify the file is really a bundle instead of just assuming it is so. --- bundle.c | 16 ++++++++++++++++ bundle.h | 1 + transport.c | 10 +--------- 3 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/bundle.c b/bundle.c
index f82baae..3a64a43 100644
--- a/bundle.c
+++ b/bundle.c@@ -23,6 +23,22 @@ static void add_to_ref_list(const unsigned char *sha1, const char *name, list->nr++; } +int is_bundle(const char *path) +{ + char buffer[100]; + FILE *ffd = fopen(path, "rb"); + int ret=1; + + if (!ffd) + return 0; + + if (!fgets(buffer, sizeof(buffer), ffd) || + strcmp(buffer, bundle_signature)) + ret=0; + fclose(ffd); + return ret; +} + /* returns an fd */ int read_bundle_header(const char *path, struct bundle_header *header) {
diff --git a/bundle.h b/bundle.h
index c5a22c8..35aa0eb 100644
--- a/bundle.h
+++ b/bundle.h@@ -14,6 +14,7 @@ struct bundle_header { struct ref_list references; }; +int is_bundle(const char *path); int read_bundle_header(const char *path, struct bundle_header *header); int create_bundle(struct bundle_header *header, const char *path, int argc, const char **argv);
diff --git a/transport.c b/transport.c
index f3195c0..bcd9b74 100644
--- a/transport.c
+++ b/transport.c@@ -881,14 +881,6 @@ static int is_gitfile(const char *url) return !prefixcmp(buf, "gitdir: "); } -static int is_file(const char *url) -{ - struct stat buf; - if (stat(url, &buf)) - return 0; - return S_ISREG(buf.st_mode); -} - static int external_specification_len(const char *url) { return strchr(url, ':') - url;
@@ -929,7 +921,7 @@ struct transport *transport_get(struct remote *remote, const char *url) ret->fetch = fetch_objs_via_rsync; ret->push = rsync_transport_push; ret->smart_options = NULL; - } else if (is_local(url) && is_file(url) && !is_gitfile(url)) { + } else if (is_local(url) && is_bundle(url)) { struct bundle_transport_data *data = xcalloc(1, sizeof(*data)); ret->data = data; ret->get_refs_list = get_refs_from_bundle;
--
1.7.7.334.g311c9.dirty