On Fri, Apr 11, 2014 at 2:28 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> +static int verify_index_from(const struct index_state *istate, const char *path)
> +{
> + int fd;
> + ssize_t n;
> + struct stat st;
> + unsigned char sha1[20];
> +
> + if (!istate->initialized)
> + return 0;
> +
> + fd = open(path, O_RDONLY);
> + if (fd < 0)
> + return 0;
Suppose another process "git rm --cached :/" is racing with us and
this imaginary git is so smart that it figures if nothing valueable is
left in the index, there's no need to write the index down at all. So
it removes $GIT_DIR/index, then $GIT_DIR/index.lock. When we come
here, we see ENOENT, but we should return 1 instead because the file
removal in this case is a form of change.
That opens a question about writing a new index. I think we could use
all-zero sha-1 as the indicator that this is a fresh new index. If
istate->sha1[] is all-zero and no index file exists, then we do not
need to verify (i.e. return 0). However if istate->sha1[] is all-zero,
but $GIT_DIR/index exists, then return 1.
I'm still not sure if elsewhere in the code base we read
$GIT_DIR/index to active_index, create a new index_state, copy entries
over (but not active_index.sha1[]), then write the _new_ index_state
down. That would hit the "however" statement above and incorrectly
return 1.
I suppose that all other errors except ENOENT could be safely ignored
(i.e. return 0 regardless of istate->sha1[]).
> +
> + if (fstat(fd, &st))
> + goto out;
> +
> + if (st.st_size < sizeof(struct cache_header) + 20)
> + goto out;
> +
> + n = pread_in_full(fd, sha1, 20, st.st_size - 20);
> + if (n != 20)
> + goto out;
> +
> + if (hashcmp(istate->sha1, sha1))
> + goto out;
> +
> + close(fd);
> + return 1;
> +
> +out:
> + close(fd);
> + return 0;
> +}
> +
> +static int verify_index(const struct index_state *istate)
> +{
> + return verify_index_from(istate, get_index_file());
> +}
> +
> static int has_racy_timestamp(struct index_state *istate)
> {
> int entries = istate->cache_nr;
> @@ -1766,7 +1811,7 @@ static int has_racy_timestamp(struct index_state *istate)
> void update_index_if_able(struct index_state *istate, struct lock_file *lockfile)
> {
> if ((istate->cache_changed || has_racy_timestamp(istate)) &&
> - !write_index(istate, lockfile->fd))
> + verify_index(istate) && !write_index(istate, lockfile->fd))
> commit_locked_index(lockfile);
> else
> rollback_lock_file(lockfile);
> diff --git a/wrapper.c b/wrapper.c
> index 5b3c7fc..bc1bfb8 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -232,6 +232,26 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
> return total;
> }
>
> +ssize_t pread_in_full(int fd, void *buf, size_t count, off_t offset)
> +{
> + char *p = buf;
> + ssize_t total = 0;
> +
> + while (count > 0) {
> + ssize_t loaded = xpread(fd, p, count, offset);
> + if (loaded < 0)
> + return -1;
> + if (loaded == 0)
> + return total;
> + count -= loaded;
> + p += loaded;
> + total += loaded;
> + offset += loaded;
> + }
> +
> + return total;
> +}
> +
> int xdup(int fd)
> {
> int ret = dup(fd);
> --
> 1.9.2-590-g468068b
>
>
>
> --
> 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
--
Duy
--
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