Phil Hord [off-list ref] writes:
The new is_gitfile is an amalgam of similar functional checks
from different places in the code....
quoted hunk
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.
Junio C Hamano [off-list ref] wrote:
Phil Hord [off-list ref] writes:
quoted
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
On Tue, Oct 11, 2011 at 7:45 PM, Junio C Hamano [off-list ref] 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