Junio C Hamano wrote:
> I like the part that gets rid of that "get-mode-bits" but at the
> same time, I find this part wanting a reasonable in-code comment.
Indeed. (As I said, a bit rough around the edges ;-)
> At least, with the earlier get-mode-bits, it was clear why we are
> doing something special in that codepath, both from the name of the
> helper function/macro and the comment attached to it describing how
> the "regular" one is cheating.
>
> We must say why this "fast" is not used everywhere and what criteria
> you should apply when deciding to use it (or not use it). And the
> "fast" name is much less descriptive.
>
> I suspect (without thinking it through) that the rule would be
> something like:
>
> The "fast" variant is to be used to read from the filesystem the
> "stat" bits that are stuffed into the index for quick "touch
> detection" (aka "cached stat info") and/or that are compared
> with the cached stat info, and should not be used anywhere else.
Sounds good to me.
> But then we always use lstat(2) and not stat(2) for that, so...
Indeed. Although there may be need of an "fast_fstat" (see below). :(
>> +#ifndef GIT_FAST_STAT
>> +static inline int fast_stat(const char *path, struct stat *st)
>> +{
>> + return stat(path, st);
>> +}
>> +static inline int fast_lstat(const char *path, struct stat *st)
>> +{
>> + return lstat(path, st);
>> +}
>> +#endif
Yes, I'm not very good at naming things, so suggestions welcome!
Note that I missed at least one lstat() call which needed to be renamed
to fast_lstat() (builtin/apply.c, line 3094 in checkout_target()).
This is my main concern with this patch (i.e. that I have missed some
more lstat calls that need to be renamed). I was a little surprised
at the size of the patch; direct index manipulation is more widespread
than I had expected.
Also, since cygwin has UNRELIABLE_FSTAT defined, on the first pass of
the patch, I ignored the use of fstat() in write_entry(). However, *if*
we allow for some other platform, which has a reliable fstat, but wants
to provide "fast" stat variants, then these fstat calls should logically
be "fast". Alternatively, we could drop the use of fstat(), like so:
diff --git a/entry.c b/entry.c
index 4d2ac73..d04d7a1 100644
--- a/entry.c
+++ b/entry.c
@@ -104,21 +104,9 @@ static int open_output_fd(char *path, struct cache_entry *ce, int to_tempfile)
}
}
-static int fstat_output(int fd, const struct checkout *state, struct stat *st)
-{
- /* use fstat() only when path == ce->name */
- if (fstat_is_reliable() &&
- state->refresh_cache && !state->base_dir_len) {
- fstat(fd, st);
- return 1;
- }
- return 0;
-}
-
static int streaming_write_entry(struct cache_entry *ce, char *path,
struct stream_filter *filter,
- const struct checkout *state, int to_tempfile,
- int *fstat_done, struct stat *statbuf)
+ const struct checkout *state, int to_tempfile)
{
int result = 0;
int fd;
@@ -128,7 +116,6 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,
return -1;
result |= stream_blob_to_fd(fd, ce->sha1, filter, 1);
- *fstat_done = fstat_output(fd, state, statbuf);
result |= close(fd);
if (result)
@@ -139,7 +126,7 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,
static int write_entry(struct cache_entry *ce, char *path, const struct checkout *state, int to_tempfile)
{
unsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;
- int fd, ret, fstat_done = 0;
+ int fd, ret;
char *new;
struct strbuf buf = STRBUF_INIT;
unsigned long size;
@@ -150,8 +137,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
struct stream_filter *filter = get_stream_filter(ce->name, ce->sha1);
if (filter &&
!streaming_write_entry(ce, path, filter,
- state, to_tempfile,
- &fstat_done, &st))
+ state, to_tempfile))
goto finish;
}
@@ -190,8 +176,6 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
}
wrote = write_in_full(fd, new, size);
- if (!to_tempfile)
- fstat_done = fstat_output(fd, state, &st);
close(fd);
free(new);
if (wrote != size)
@@ -209,8 +193,7 @@ static int write_entry(struct cache_entry *ce, char *path, const struct checkout
finish:
if (state->refresh_cache) {
- if (!fstat_done)
- fast_lstat(ce->name, &st);
+ fast_lstat(ce->name, &st);
fill_stat_cache_info(ce, &st);
}
return 0;
--
I would need to do some timing tests (on Linux) to see what effect this
would have on performance. (I noticed that the test suite ran about 2%
slower with the above applied).
A simple pass-though fast_fstat(), including on cygwin, is probably the
way to go.
Sorry for being a bit slow on this - I'm very busy at the moment. :(
ATB,
Ramsay Jones
--
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