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 
  -     /* 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, 
                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);
                if (wrote != size)
  @@ -209,8 +193,7 @@ static int write_entry(struct cache_entry *ce, char 
*path, const struct checkout
        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. :(

Ramsay Jones

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to