Re: [PATCH v3 09/12] revision.c: --all adds HEAD from all worktrees

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Apr 22, 2017, 05:48 UTC
Message-ID: <b0bd826e-8394-08f7-131e-8dfa44d0f647@alum.mit.edu>
In-Reply-To: <20170419110145.5086-10-pclouds@gmail.com>

On 04/19/2017 01:01 PM, Nguyễn Thái Ngọc Duy wrote:

Show 69 quoted lines

> Unless single_worktree is set, --all now adds HEAD from all worktrees.
> 
> Since reachable.c code does not use setup_revisions(), we need to call
> other_head_refs_submodule() explicitly there to have the same effect on
> "git prune", so that we won't accidentally delete objects needed by some
> other HEADs.
> 
> A new FIXME is added because we would need something like
> 
>     int refs_other_head_refs(struct ref_store *, each_ref_fn, cb_data);
> 
> in addition to other_head_refs() to handle it, which might require
> 
>     int get_submodule_worktrees(const char *submodule, int flags);
> 
> It could be a separate topic to reduce the scope of this one.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  reachable.c      |  1 +
>  refs.c           | 22 ++++++++++++++++++++++
>  refs.h           |  1 +
>  revision.c       | 13 +++++++++++++
>  submodule.c      |  2 ++
>  t/t5304-prune.sh | 12 ++++++++++++
>  6 files changed, 51 insertions(+)
> 
> diff --git a/reachable.c b/reachable.c
> index a8a979bd4f..a3b938b46c 100644
> --- a/reachable.c
> +++ b/reachable.c
> @@ -177,6 +177,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
>  
>  	/* detached HEAD is not included in the list above */
>  	head_ref(add_one_ref, revs);
> +	other_head_refs(add_one_ref, revs);
>  
>  	/* Add all reflog info */
>  	if (mark_reflog)
> diff --git a/refs.c b/refs.c
> index 537052f7ba..23e3607674 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1780,3 +1780,25 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
>  {
>  	return refs_rename_ref(get_main_ref_store(), oldref, newref, logmsg);
>  }
> +
> +int other_head_refs(each_ref_fn fn, void *cb_data)
> +{
> +	struct worktree **worktrees, **p;
> +	int ret = 0;
> +
> +	worktrees = get_worktrees(0);
> +	for (p = worktrees; *p; p++) {
> +		struct worktree *wt = *p;
> +		struct ref_store *refs;
> +
> +		if (wt->is_current)
> +			continue;
> +
> +		refs = get_worktree_ref_store(wt);
> +		ret = refs_head_ref(refs, fn, cb_data);
> +		if (ret)
> +			break;
> +	}
> +	free_worktrees(worktrees);
> +	return ret;
> +}

This function is mainly about iterating through all worktrees. Therefore it feels out of place in the refs module. But I don't have a strong feeling about it.

Show 27 quoted lines

> diff --git a/refs.h b/refs.h
> index e06db37118..cc71b6c7a0 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -247,6 +247,7 @@ int refs_for_each_remote_ref(struct ref_store *refs,
>  			     each_ref_fn fn, void *cb_data);
>  
>  int head_ref(each_ref_fn fn, void *cb_data);
> +int other_head_refs(each_ref_fn fn, void *cb_data);
>  int for_each_ref(each_ref_fn fn, void *cb_data);
>  int for_each_ref_in(const char *prefix, each_ref_fn fn, void *cb_data);
>  int for_each_fullref_in(const char *prefix, each_ref_fn fn, void *cb_data,
> diff --git a/revision.c b/revision.c
> index c329070c89..040a0064f6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2105,6 +2105,13 @@ static int handle_revision_pseudo_opt(const char *submodule,
>  	int argcount;
>  
>  	if (submodule) {
> +		/*
> +		 * We need some something like get_submodule_worktrees()
> +		 * before we can go through all worktrees of a submodule,
> +		 * .e.g with adding all HEADs from --all, which is not
> +		 * supported right now, so stick to single worktree.
> +		 */
> +		assert(revs->single_worktree != 0);

You don't need `!= 0` above.

We usually don't use `assert(foo)` but rather `if (!foo) die("BUG:...")`, which gives a better error message and isn't switched off if a distribution compiles with NDEBUG.

But here I'm confused about whether this check failing really indicates a bug within git, or whether it could indicate a situation that the user has set up that git just can't handle right now. For example, maybe the user will set up a submodule that itself uses worktrees (even if that's not possible now, maybe they will have done it with some future version of git or with another tool). If the problem is only that this version of git is too stupid to handle pruning safely in that situation, it would be more appropriate to use something more like

	if (!refs->single_worktree)
		die("error: git is currently unable to handle submodules that use
linked worktrees");

>  		refs = get_submodule_ref_store(submodule);
>  	} else
>  		refs = get_main_ref_store();
> [...]

Michael