[PATCH] Make is_gitfile a non-static generic function

[PATCH] Make is_gitfile a non-static generic function

[Phil Hord]

The new is_gitfile is an amalgam of similar functional checks
from different places in the code.  All these places do
slightly different checks on the suspect gitfile (for their
own good reasons).  But the lack of common code leads to bugs
and maintenance problems. Help move the code forward by making
is_gitfile() a common function that everyone can use for this
purpose.

Signed-off-by: Phil Hord <hordp@cisco.com>
---

I'm not sure this function belongs in transport.c anymore, but
I left it here to minimize conflicts.  I think a better home would
be path.c, but maybe not.  If someone has a preference,
please let me know.

 builtin/clone.c |    8 +-------
 cache.h         |    1 +
 transport.c     |    6 +++++-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 488f48e..5110399 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -120,13 +120,7 @@ static char *get_repo_path(const char *repo, int
*is_bundle)
 			return xstrdup(absolute_path(path));
 		} else if (S_ISREG(st.st_mode) && st.st_size > 8) {
 			/* Is it a "gitfile"? */
-			char signature[8];
-			int len, fd = open(path, O_RDONLY);
-			if (fd < 0)
-				continue;
-			len = read_in_full(fd, signature, 8);
-			close(fd);
-			if (len != 8 || strncmp(signature, "gitdir: ", 8))
+			if (!is_gitfile(path))
 				continue;
 			path = read_gitfile(path);
 			if (path) {
diff --git a/cache.h b/cache.h
index 601f6f6..7a8d9f9 100644
--- a/cache.h
+++ b/cache.h
@@ -441,6 +441,7 @@ extern const char *get_git_work_tree(void);
 extern const char *read_gitfile(const char *path);
 extern const char *resolve_gitdir(const char *suspect);
 extern void set_git_work_tree(const char *tree);
+extern int is_gitfile(const char *path);
  #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 diff --git a/transport.c b/transport.c
index f3195c0..d08a826 100644
--- a/transport.c
+++ b/transport.c
@@ -859,7 +859,11 @@ static int is_local(const char *url)
 		has_dos_drive_prefix(url);
 }
 -static int is_gitfile(const char *url)
+/*
+ * See if the referenced file looks like a 'gitfile'.
+ * Does not try to determine if the referenced gitdir is actually valid.
+ */
+int is_gitfile(const char *url)
 {
 	struct stat st;
 	char buf[9];
-- 
1.7.7.334.gfc143d

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

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

> I'm not sure this function belongs in transport.c anymore, but
> I left it here to minimize conflicts.  I think a better home would
> be path.c, but maybe not.  If someone has a preference,
> please let me know.

I would think either transport.c or setup.c is more appropriate than
path.c; the last one is more of "pathname manipulation utility bag of
functions" and does not have much to do with the "Git"-ness of the path
they deal with.

I am not sure if is_gitfile() is a good name for a global function,
though.  Also I think the interface to the function should be updated so
that the caller can choose to receive the target path when the function
returns with positive answer, making "read_gitfile()" unnecessary for such
a caller.

As a matter of fact, couldn't we somehow unify these two slightly
different implementations around the same theme, making is_gitfile()
function unnecessary? As far as I can tell, the only difference between
these functions is how they fail when given a non-gitfile, and many
callers just call read_gitfile() without first asking if it is a gitfile.

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

[Junio C Hamano]

Phil Hord <hordp@cisco.com> writes:

> The new is_gitfile is an amalgam of similar functional checks
> from different places in the code....

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 488f48e..5110399 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -120,13 +120,7 @@ static char *get_repo_path(const char *repo, int
> *is_bundle)
>  			return xstrdup(absolute_path(path));
>  		} else if (S_ISREG(st.st_mode) && st.st_size > 8) {
>  			/* Is it a "gitfile"? */
> -			char signature[8];
> -			int len, fd = open(path, O_RDONLY);
> -			if (fd < 0)
> -				continue;
> -			len = read_in_full(fd, signature, 8);
> -			close(fd);
> -			if (len != 8 || strncmp(signature, "gitdir: ", 8))
> +			if (!is_gitfile(path))
>  				continue;
>  			path = read_gitfile(path);
>  			if (path) {
> diff --git a/cache.h b/cache.h
> index 601f6f6..7a8d9f9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -441,6 +441,7 @@ extern const char *get_git_work_tree(void);
>  extern const char *read_gitfile(const char *path);
>  extern const char *resolve_gitdir(const char *suspect);
>  extern void set_git_work_tree(const char *tree);
> +extern int is_gitfile(const char *path);
>   #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
>  diff --git a/transport.c b/transport.c
> index f3195c0..d08a826 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -859,7 +859,11 @@ static int is_local(const char *url)
>  		has_dos_drive_prefix(url);
>  }
>  -static int is_gitfile(const char *url)
> +/*
> + * See if the referenced file looks like a 'gitfile'.
> + * Does not try to determine if the referenced gitdir is actually valid.
> + */
> +int is_gitfile(const char *url)
>  {
>  	struct stat st;
>  	char buf[9];

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.

The caller in transport.c says "I am about to read from a regular file,
and usually I would treat it as a bundle, but I want to avoid that
codepath if that regular file is not a bundle. So I use the codepath only
when that file is not a gitfile".

It should be saying "Is it a bundle? Then I'd use the codepath to read
from the bundle" to begin with. Otherwise the code will break when we add
yet another regular file we can fetch from that is not a bundle nor a
gitfile.

I think the hand-crafted check in builtin/clone.c you removed originated
from laziness to avoid teaching read_gitfile() to read potential gitfile
silently (and signal errors by simply returning NULL). I also suspect the
codepath may become simpler if we had a way to ask "Is this a bundle?".

I think read_bundle_header() in bundle.c can be refactored to a silent
interface that allows us to ask "Is this a bundle?" question properly.

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

[Phil Hord]

Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <hordp@cisco.com> writes:
>
>> The new is_gitfile is an amalgam of similar functional checks
>> from different places in the code....
>
>
> 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 it's a simple and low-impact change that fixes a bug with a
minimum of disruption.  But I also think it is lazy.

> The caller in transport.c says "I am about to read from a regular file,
> and usually I would treat it as a bundle, but I want to avoid that
> codepath if that regular file is not a bundle. So I use the codepath only
> when that file is not a gitfile".
>
> It should be saying "Is it a bundle? Then I'd use the codepath to read
> from the bundle" to begin with. Otherwise the code will break when we add
> yet another regular file we can fetch from that is not a bundle nor a
> gitfile.

Yes, and this is part of the kind of distraction that held back my
update over the weekend.

When we do add another file type we'll wind up with a half-dozen
places that get affected in slightly different ways again.  Wouldn't
it be nice to have a function to tell us what kind of thing it is
we've been asked to look at?  Something like git_type(url) that
returns GIT_BUNDLE, GIT_DIRECTORY or GIT_FILE, maybe.

Except I didn't see many examples in the code using this sort of
enumerated decision function.

> I think the hand-crafted check in builtin/clone.c you removed originated
> from laziness to avoid teaching read_gitfile() to read potential gitfile
> silently (and signal errors by simply returning NULL).

I made a read_gitfile(... , gently) function, but I didn't like it
much.  When !gently, I think it should be rather explicit about the
type of failure.  This makes the code look like 20% of it is repeated
"if (!gently) die... ;\n return;" sequences.  It's almost enough to
lead me to macros.

And what about when fopen() fails and we are running silently.  Do we
just shrug and say "Not a gitfile"?  I don't think it's good enough.
We need to be able to say all of these:

  It's a gitfile, here's the internal path.

  It's not a gitfile, it is something else.

  It looked like a gitfile until I ran into E_ACCES or some other error.

Making the one function run silently or not complicates the code further.

I tried to find a similar style to mimic elsewhere in the code, but I
didn't find any consistency.  Pointers to clean examples would be
welcome.

I started working on more of an API.  But it's still very ugly and not
ready for even a strawman discussion.

But I don't know how much time I have for a full writeup atm.  Without
something, though, I cannot easily fetch from a submodule, because
submodules all use gitfiles now, and git:master does not know how to
fetch from them.

And that's the itch I had to scratch.

> I also suspect the
> codepath may become simpler if we had a way to ask "Is this a bundle?".
>
> I think read_bundle_header() in bundle.c can be refactored to a silent
> interface that allows us to ask "Is this a bundle?" question properly.

I'll take a look at it.  But I won't have much time for it this week.

Thanks,
Phil

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

[Phil Hord]

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)) {

Phil

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

[Junio C Hamano]

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.

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

[Phil Hord]

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