Junio C Hamano <gitster@xxxxxxxxx> wrote: > Phil Hord <hordp@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html