Re: [PATCH 6/8] worktree: rename copy-pasted variable
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Sep 27, 2020, 13:29 UTC
- Message-ID
- <20200927132910.20515-1-martin.agren@gmail.com>
- In-Reply-To
- <CAN0heSrFQFRPnXzp-UT6_jrwvVh+4EQMdOvkJaQCQxeg8GUG4g@mail.gmail.com>
On Sat, 12 Sep 2020 at 16:01, Martin Ågren <martin.agren@gmail.com> wrote:
Show 17 quoted lines
>
> On Thu, 10 Sep 2020 at 22:29, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Martin Ågren <martin.agren@gmail.com> writes:
> >
> > > As the commit message of 04a3dfb8b5 ("worktree.c: check whether branch
> > > is bisected in another worktree", 2016-04-22) indicates, the function
> > > `is_worktree_being_bisected()` is based on the older function
> > > `is_worktree_being_rebased()`. This heritage can also be seen in the
> > > name of the variable where we store our return value: It was never
> > > adapted while copy-editing and remains as `found_rebase`.
> >
> > How bad is this copy and paste? Is it a possibility to make a
> > single helper function and these existing two a thin wrapper around
> > the helper that passes customization between bisect and rebase?
>
> That's a good point. I'll look into it.I did look into this, and here's what I came up with (this is on top of v2 that I just sent out, since that's how I tried it out). If there would be three or four such similar functions, I would feel a lot more confident going with this approach, since it'd be sort of obvious that they were all the same. But for "only" two it somehow feels a bit brittle. If any of those two functions need to do something differently, we might need to start shuffling logic between the helper and the callers. Or we'll end up with just a single caller, the other having broken away from the helper.
So in the end I didn't take the plunge for v2.
(BTW, the naming in this diff is clearly w-i-p grade...)
Martin
diff --git a/worktree.c b/worktree.c index a75230a950..4009856b54 100644 --- a/worktree.c +++ b/worktree.c @@ -356,36 +356,40 @@ void update_worktree_location(struct worktree *wt, const char *path_) strbuf_release(&path); } +static int is_worktree_being_x(int (*check_fn)(const struct worktree *wt, + struct wt_status_state *state), + const struct worktree *wt, + const char *target) +{ + struct wt_status_state state = { 0 }; + int found; + + found = check_fn(wt, &state) && + state.branch && + skip_prefix(target, "refs/heads/", &target) && + !strcmp(state.branch, target); + wt_status_state_free_buffers(&state); + return found; +} + +static int check_rebase_in_progress(const struct worktree *wt, + struct wt_status_state *state) +{ + return wt_status_check_rebase(wt, state) && + (state->rebase_in_progress || + state->rebase_interactive_in_progress); +} + int is_worktree_being_rebased(const struct worktree *wt, const char *target) { - struct wt_status_state state; - int found_rebase; - - memset(&state, 0, sizeof(state)); - found_rebase = wt_status_check_rebase(wt, &state) && - (state.rebase_in_progress || - state.rebase_interactive_in_progress) && - state.branch && - skip_prefix(target, "refs/heads/", &target) && - !strcmp(state.branch, target); - wt_status_state_free_buffers(&state); - return found_rebase; + return is_worktree_being_x(check_rebase_in_progress, wt, target); } int is_worktree_being_bisected(const struct worktree *wt, const char *target) { - struct wt_status_state state; - int found_bisect; - - memset(&state, 0, sizeof(state)); - found_bisect = wt_status_check_bisect(wt, &state) && - state.branch && - skip_prefix(target, "refs/heads/", &target) && - !strcmp(state.branch, target); - wt_status_state_free_buffers(&state); - return found_bisect; + return is_worktree_being_x(wt_status_check_bisect, wt, target); } /*
-- 2.28.0.277.g9b3c35fffd