Re: [PATCH 2/5] prune --repos: fix uninitialized access
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 23, 2014, 19:59 UTC
- Message-ID
- <xmqq61inetib.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1406115795-24082-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 7 quoted lines
> There's a code path in prune_repo_dir() that does not initialize 'st' > buffer, which is checked by the caller, prune_repos_dir(). Instead > of leaking some prune logic out to prune_repos_dir(), move 'st' into > prune_repo_dir(). > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > ---
The return value from the function used to be "should .git/repos/$x be removed if it is stale enough?" and now it is just "should it be removed?", so the change makes sense.
It does not explain what the change to the test is about, though.
Thanks.
Show 86 quoted lines
> builtin/prune.c | 16 ++++++----------
> t/t2026-prune-linked-checkouts.sh | 2 +-
> 2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/prune.c b/builtin/prune.c
> index 28b7adf..e72c391 100644
> --- a/builtin/prune.c
> +++ b/builtin/prune.c
> @@ -112,8 +112,9 @@ static void prune_object_dir(const char *path)
> }
> }
>
> -static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason)
> +static int prune_repo_dir(const char *id, struct strbuf *reason)
> {
> + struct stat st;
> char *path;
> int fd, len;
>
> @@ -123,26 +124,23 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
> }
> if (file_exists(git_path("repos/%s/locked", id)))
> return 0;
> - if (stat(git_path("repos/%s/gitdir", id), st)) {
> - st->st_mtime = expire;
> + if (stat(git_path("repos/%s/gitdir", id), &st)) {
> strbuf_addf(reason, _("Removing repos/%s: gitdir file does not exist"), id);
> return 1;
> }
> fd = open(git_path("repos/%s/gitdir", id), O_RDONLY);
> if (fd < 0) {
> - st->st_mtime = expire;
> strbuf_addf(reason, _("Removing repos/%s: unable to read gitdir file (%s)"),
> id, strerror(errno));
> return 1;
> }
> - len = st->st_size;
> + len = st.st_size;
> path = xmalloc(len + 1);
> read_in_full(fd, path, len);
> close(fd);
> while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
> len--;
> if (!len) {
> - st->st_mtime = expire;
> strbuf_addf(reason, _("Removing repos/%s: invalid gitdir file"), id);
> free(path);
> return 1;
> @@ -162,7 +160,7 @@ static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason
> return 1;
> }
> free(path);
> - return 0;
> + return st.st_mtime <= expire;
> }
>
> static void prune_repos_dir(void)
> @@ -172,15 +170,13 @@ static void prune_repos_dir(void)
> DIR *dir = opendir(git_path("repos"));
> struct dirent *d;
> int ret;
> - struct stat st;
> if (!dir)
> return;
> while ((d = readdir(dir)) != NULL) {
> if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> continue;
> strbuf_reset(&reason);
> - if (!prune_repo_dir(d->d_name, &st, &reason) ||
> - st.st_mtime > expire)
> + if (!prune_repo_dir(d->d_name, &reason))
> continue;
> if (show_only || verbose)
> printf("%s\n", reason.buf);
> diff --git a/t/t2026-prune-linked-checkouts.sh b/t/t2026-prune-linked-checkouts.sh
> index 4ccfa4e..79d84cb 100755
> --- a/t/t2026-prune-linked-checkouts.sh
> +++ b/t/t2026-prune-linked-checkouts.sh
> @@ -77,7 +77,7 @@ test_expect_success 'not prune recent checkouts' '
> mkdir zz &&
> mkdir -p .git/repos/jlm &&
> echo "$TRASH_DIRECTORY"/zz >.git/repos/jlm/gitdir &&
> - git prune --repos --verbose &&
> + git prune --repos --verbose --expire=2.days.ago &&
> test -d .git/repos/jlm
> '