Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Date: Jul 4, 2013, 18:18 UTC
Message-ID: <51D5BC83.20706@ramsay1.demon.co.uk>
In-Reply-To: <7v4ncfjs32.fsf@alter.siamese.dyndns.org>

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 ;-)

Show 16 quoted lines

> 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). :(

Show 10 quoted lines

>> +#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