From: Thomas Rast <tr@thomasrast.ch>
Date: Fri, 18 Jul 2014 18:17:01 GMT
Subject: Re: [PATCH v7 25/31] prune: strategies for linked checkouts
Message-ID: <87r41i352a.fsf@thomasrast.ch>
In-Reply-To: <1405227068-25506-26-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> (alias R=$GIT_COMMON_DIR/repos/<id>)
>
>  - linked checkouts are supposed to keep its location in $R/gitdir up
>    to date. The use case is auto fixup after a manual checkout move.
>
>  - linked checkouts are supposed to update mtime of $R/gitdir. If
>    $R/gitdir's mtime is older than a limit, and it points to nowhere,
>    repos/<id> is to be pruned.
>
>  - If $R/locked exists, repos/<id> is not supposed to be pruned. If
>    $R/locked exists and $R/gitdir's mtime is older than a really long
>    limit, warn about old unused repo.
>
>  - "git checkout --to" is supposed to make a hard link named $R/link
>    pointing to the .git file on supported file systems to help detect
>    the user manually deleting the checkout. If $R/link exists and its
>    link count is greated than 1, the repo is kept.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Documentation/git-prune.txt                |  3 +
>  Documentation/gitrepository-layout.txt     | 19 ++++++
>  builtin/checkout.c                         | 14 +++++
>  builtin/prune.c                            | 99 ++++++++++++++++++++++++++++++
>  setup.c                                    | 13 ++++
>  t/t2026-prune-linked-checkouts.sh (new +x) | 84 +++++++++++++++++++++++++

I get this from t2026.2 under valgrind:

  ==21334== Conditional jump or move depends on uninitialised value(s)
  ==21334==    at 0x46D49B: prune_repos_dir (prune.c:182)
  ==21334==    by 0x46D8C0: cmd_prune (prune.c:252)
  ==21334==    by 0x405C2F: run_builtin (git.c:351)
  ==21334==    by 0x405E47: handle_builtin (git.c:530)
  ==21334==    by 0x405F6B: run_argv (git.c:576)
  ==21334==    by 0x40610B: main (git.c:663)
  ==21334==  Uninitialised value was created by a stack allocation
  ==21334==    at 0x46D3BB: prune_repos_dir (prune.c:169)
  ==21334== 
  {
     <insert_a_suppression_name_here>
     Memcheck:Cond
     fun:prune_repos_dir
     fun:cmd_prune
     fun:run_builtin
     fun:handle_builtin
     fun:run_argv
     fun:main
  }
  not ok 2 - prune files inside $GIT_DIR/repos
  #
  #               mkdir .git/repos &&
  #               : >.git/repos/abc &&
  #               git prune --repos --verbose >actual &&
  #               cat >expect <<EOF &&
  #       Removing repos/abc: not a valid directory
  #       EOF
  #               test_i18ncmp expect actual &&
  #               ! test -f .git/repos/abc &&
  #               ! test -d .git/repos
  #

I think it's because of the early 'return 0' ...

> +static int prune_repo_dir(const char *id, struct stat *st, struct strbuf *reason)
> +{
> +	char *path;
> +	int fd, len;
> +
> +	if (!is_directory(git_path("repos/%s", id))) {
> +		strbuf_addf(reason, _("Removing repos/%s: not a valid directory"), id);
> +		return 1;
> +	}
> +	if (file_exists(git_path("repos/%s/locked", id)))
> +		return 0;

in this line, before the stat() actually runs, which then in the
condition ...

> +	if (stat(git_path("repos/%s/gitdir", id), st)) {
> +		st->st_mtime = expire;
> +		strbuf_addf(reason, _("Removing repos/%s: gitdir file does not exist"), id);
> +		return 1;
> +	}
[...]
> +}
> +
> +static void prune_repos_dir(void)
> +{
[...]
> +	struct stat st;
[...]
> +		if (!prune_repo_dir(d->d_name, &st, &reason) ||
> +		    st.st_mtime > expire)

causes the second arm to be evaluated when st.st_mtime is not
initialized yet.  Can you look into this?

-- 
Thomas Rast
tr@thomasrast.ch