Re: [PATCH v7 2/2] Verify index file before we opportunistically update it
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Apr 11, 2014, 10:36 UTC
- Message-ID
- <CACsJy8DGq8Q8i8pgYivcMhnk99gYhnKvSWUsetFL=+3L8YLPew@mail.gmail.com>
- In-Reply-To
- <xmqqppkpvv9m.fsf@gitster.dls.corp.google.com>
On Fri, Apr 11, 2014 at 2:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> +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[]).
Show 79 quoted lines
> +
> + 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html-- Duy