Re: [PATCH v1 16/19] read-cache: unlink old sharedindex files

From: Duy Nguyen <pclouds@gmail.com>
Date: Oct 25, 2016, 10:43 UTC
Message-ID: <CACsJy8C4Qd76LSYxk9BJtN3n3Knv_RCDgBOO-ybkNHFZK61ahQ@mail.gmail.com>
In-Reply-To: <20161023092648.12086-17-chriscool@tuxfamily.org>

On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder <christian.couder@gmail.com> wrote:

Show 13 quoted lines

> +static int can_delete_shared_index(const char *shared_sha1_hex)
> +{
> +       struct stat st;
> +       unsigned long expiration;
> +       const char *shared_index = git_path("sharedindex.%s", shared_sha1_hex);
> +
> +       /* Check timestamp */
> +       expiration = get_shared_index_expire_date();
> +       if (!expiration)
> +               return 0;
> +       if (stat(shared_index, &st))
> +               return error_errno("could not stat '%s", shared_index);
> +       if (st.st_mtime > expiration)

I wonder if we should check ctime too, in case mtime is not reliable (and ctime is less likely to be manipulated by user), just for extra safety. If (st.st_mtime > expiration || st.st_ctime > expiration).

Show 12 quoted lines

> +               return 0;
> +
> +       return 1;
> +}
> +
> +static void clean_shared_index_files(const char *current_hex)
> +{
> +       struct dirent *de;
> +       DIR *dir = opendir(get_git_dir());
> +
> +       if (!dir) {
> +               error_errno("unable to open git dir: %s", get_git_dir());

_()

> +               return;

Or just do "return error_errno(...)". The caller can ignore the return value for now.

Show 8 quoted lines

> +       }
> +
> +       while ((de = readdir(dir)) != NULL) {
> +               const char *sha1_hex;
> +               if (!skip_prefix(de->d_name, "sharedindex.", &sha1_hex))
> +                       continue;
> +               if (!strcmp(sha1_hex, current_hex))
> +                       continue;

Yeah.. make sure that the shared index linked to $GIT_DIR/index stay, even if mtime is screwed up. I wonder if we should have the same treatment for $GIT_DIR/index.lock though, as an extra safety measure. If you call this function in write_locked_index, then $GIT_DIR/index.lock is definitely there. Hmm.. maybe it _is_ current_hex, current_hex is not the value from $GIT_DIR/index...

> +               if (can_delete_shared_index(sha1_hex) > 0 &&
> +                   unlink(git_path("%s", de->d_name)))
> +                       error_errno("unable to unlink: %s", git_path("%s", de->d_name));

_()

Show 9 quoted lines

>  static int write_shared_index(struct index_state *istate,
> @@ -2211,8 +2269,11 @@ static int write_shared_index(struct index_state *istate,
>         }
>         ret = rename_tempfile(&temporary_sharedindex,
>                               git_path("sharedindex.%s", sha1_to_hex(si->base->sha1)));
> -       if (!ret)
> +       if (!ret) {
>                 hashcpy(si->base_sha1, si->base->sha1);
> +               clean_shared_index_files(sha1_to_hex(si->base->sha1));

This operation is technically garbage collection and should belong to "git gc --auto", which is already called automatically in a few places. Is it not called often enough that we need to do the cleaning up right after a new shared index is created?

-- 
Duy