From: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Michael Haggerty" <mhagger@alum.mit.edu>,
"Jeff King" <peff@peff.net>,
"Torsten Bögershausen" <tboegi@web.de>,
"Johannes Sixt" <j6t@kdbg.org>,
"Shawn O. Pearce" <spearce@spearce.org>,
mlevedahl@gmail.com, dpotapov@gmail.com,
"GIT Mailing-list" <git@vger.kernel.org>
Subject: Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions
Date: Thu, 04 Jul 2013 19:18:43 +0100 [thread overview]
Message-ID: <51D5BC83.20706@ramsay1.demon.co.uk> (raw)
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 ;-)
> 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
next prev parent reply other threads:[~2013-07-04 18:29 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <51C5FD28.1070004@ramsay1.demon.co.uk>
[not found] ` <51C7A875.6020205@gmail.com>
[not found] ` <7va9mf6txq.fsf@alter.siamese.dyndns.org>
2013-06-25 19:23 ` [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions Johannes Sixt
2013-06-25 20:23 ` Torsten Bögershausen
2013-06-25 21:18 ` Junio C Hamano
2013-06-26 14:19 ` Torsten Bögershausen
2013-06-26 21:54 ` Ramsay Jones
2013-06-27 15:19 ` Torsten Bögershausen
2013-06-27 23:18 ` Ramsay Jones
2013-06-27 2:37 ` Mark Levedahl
[not found] ` <51C6BC4B.9030905@web.de>
[not found] ` <51C8BF2C.2050203@ramsay1.demon.co.uk>
[not found] ` <7vy59y4w3r.fsf@alter.siamese.dyndns.org>
2013-06-26 21:39 ` Ramsay Jones
[not found] ` <51C94425.7050006@alum.mit.edu>
2013-06-26 21:45 ` Ramsay Jones
2013-06-26 22:35 ` Jeff King
2013-06-26 22:43 ` Jeff King
2013-06-27 22:58 ` Ramsay Jones
2013-06-28 2:31 ` Mark Levedahl
2013-06-27 5:51 ` Michael Haggerty
2013-06-27 19:58 ` Jeff King
2013-06-27 21:04 ` Junio C Hamano
2013-06-27 23:09 ` Ramsay Jones
2013-06-30 17:28 ` Ramsay Jones
2013-06-30 19:50 ` Junio C Hamano
2013-07-04 18:18 ` Ramsay Jones [this message]
2013-07-09 11:02 ` Torsten Bögershausen
2013-07-11 17:31 ` Ramsay Jones
2013-06-27 22:17 ` Ramsay Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=51D5BC83.20706@ramsay1.demon.co.uk \
--to=ramsay@ramsay1.demon.co.uk \
--cc=dpotapov@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=mhagger@alum.mit.edu \
--cc=mlevedahl@gmail.com \
--cc=peff@peff.net \
--cc=spearce@spearce.org \
--cc=tboegi@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).