Re: [PATCH 1/2 v4] worktree: add 'for_each_worktree' function

[David Turner <dturner@twopensource.com>]

On Thu, 2015-08-13 at 14:32 -0400, Michael Rappazzo wrote:
> for_each_worktree iterates through each worktree and invokes a callback
> function.  The main worktree (if not bare) is always encountered first,
> followed by worktrees created by `git worktree add`.

Thanks!  This will be super-useful!  I particularly appreciate the
detailed function documentation.

More comments below.

> If the callback function returns a non-zero value, iteration stops, and
> the callback's return value is returned from the for_each_worktree
> function.
> 
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  builtin/worktree.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 430b51e..a43e360 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -26,6 +26,14 @@ static int show_only;
>  static int verbose;
>  static unsigned long expire;
>  
> +/*
> + * The signature for the callback function for the for_each_worktree()
> + * function below.  The memory pointed to by the callback arguments
> + * is only guaranteed to be valid for the duration of a single
> + * callback invocation.
> + */
> +typedef int each_worktree_fn(const char *path, const char *git_dir, void *cb_data);
> +
>  static int prune_worktree(const char *id, struct strbuf *reason)
>  {
>  	struct stat st;
> @@ -358,6 +366,82 @@ static int add(int ac, const char **av, const char *prefix)
>  
>  	return add_worktree(path, branch, &opts);
>  }
> +/*

nit: newline above this line

> + * Iterate through each worktree and invoke the callback function.  If
> + * the callback function returns non-zero, the iteration stops, and
> + * this function returns that return value
> + */
> +static int for_each_worktree(each_worktree_fn fn, void *cb_data)
> +{
> +	struct strbuf main_path = STRBUF_INIT;
> +	struct dirent *d;


The scope of d can be smaller; move it down to the place I've marked
below

> +	struct stat st;
> +	struct strbuf worktree_path = STRBUF_INIT;
> +	struct strbuf worktree_git = STRBUF_INIT;
> +	const char *common_dir;
> +	DIR *dir;

ditto

> +	int main_is_bare = 0;
> +	int ret = 0;
> +
> +	common_dir = get_git_common_dir();
> +	if (!strcmp(common_dir, get_git_dir())) {
> +		/* simple case - this is the main repo */
> +		main_is_bare = is_bare_repository();
> +		if (!main_is_bare) {
> +			strbuf_addstr(&main_path, get_git_work_tree());
> +		} else {
> +			strbuf_addstr(&main_path, common_dir);
> +		}
> +	} else {
> +		strbuf_addstr(&main_path, common_dir);
> +		strbuf_strip_suffix(&main_path, "/.git");
> +		/* If the common_dir DOES NOT end with '/.git', then it is bare */
> +		main_is_bare = !strcmp(common_dir, main_path.buf);

strbuf_strip_suffix returns 1 if the suffix was stripped and 0
otherwise, so there is no need for this strcmp.

I'm a little worried about this path manipulation; it looks like the
config setting core.bare is the actual thing to check?  (Along with
maybe the GIT_WORK_TREE environment var; not sure how that interacts
with worktrees).

> +	}
> +
> +	if (!main_is_bare) {
> +		strbuf_addstr(&worktree_path, main_path.buf);
> +
> +		strbuf_addstr(&main_path, "/.git");
> +		strbuf_addstr(&worktree_git, main_path.buf);
> +
> +		ret = fn(worktree_path.buf, worktree_git.buf, cb_data);
> +		if (ret)
> +			goto done;
> +	}
> +	strbuf_addstr(&main_path, "/worktrees");

Earlier, you said:
		strbuf_addstr(&main_path, common_dir);
		strbuf_strip_suffix(&main_path, "/.git");

Now you are adding /worktrees.  Doesn't this mean, in the non-bare case,
that you'll be looking in $common_dir/worktrees instead of
$common_dir/.git/worktrees ?

> +	if (is_directory(main_path.buf)) {

declare dir here...

> +		dir = opendir(main_path.buf);
> +		if (dir) {

... and d here.

> +			while ((d = readdir(dir)) != NULL) {
> +				if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
> +					continue;
> +
> +				strbuf_reset(&worktree_git);
> +				strbuf_addf(&worktree_git, "%s/%s/gitdir", main_path.buf, d->d_name);

tioli: main_path never changes, so no need to keep chopping it off and
adding it back on; just truncate worktree_git to main_path.len + 1 here
and then add d->name.

> +				if (stat(worktree_git.buf, &st))
> +					continue;

Do we want to report errors other than ENOENT? (genuine question)