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

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