Re: [PATCH 1/2 v4] worktree: add 'for_each_worktree' function
- From
David Turner <dturner@twopensource.com>
- Date
- Aug 13, 2015, 19:23 UTC
- Message-ID
- <1439493795.8855.16.camel@twopensource.com>
- In-Reply-To
- <1439490739-9361-2-git-send-email-rappazzo@gmail.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.
Show 33 quoted lines
> 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
Show 8 quoted lines
> + * 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
Show 5 quoted lines
> + struct stat st; > + struct strbuf worktree_path = STRBUF_INIT; > + struct strbuf worktree_git = STRBUF_INIT; > + const char *common_dir; > + DIR *dir;
ditto
Show 17 quoted lines
> + 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).
Show 13 quoted lines
> + }
> +
> + 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.
Show 6 quoted lines
> + 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)