Re: [PATCH 6/8] worktree: rename copy-pasted variable

["Martin Ågren" <martin.agren@gmail.com>]

On Sat, 12 Sep 2020 at 16:01, Martin Ågren <martin.agren@gmail.com> wrote:
>
> 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