Re: [PATCH] Make is_gitfile a non-static generic function

[Phil Hord <hordp@cisco.com>]

Junio C Hamano <gitster@pobox.com> sez:
> Phil Hord <phil.hord@gmail.com> writes:
> 
>> On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 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