diff mbox series

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

Message ID 6d913ea3e0c30cc1dbcff05974b5d990797e8dc2.1599762679.git.martin.agren@gmail.com
State Superseded
Headers show
Series various wt-status/worktree cleanups | expand

Commit Message

Martin Ågren Sept. 10, 2020, 7:03 p.m. UTC
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`.

Rename the variable to make clear that we're looking for a bisect(ion),
nothing else.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 worktree.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Junio C Hamano Sept. 10, 2020, 8:29 p.m. UTC | #1
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`.
>
> Rename the variable to make clear that we're looking for a bisect(ion),
> nothing else.

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?
Martin Ågren Sept. 12, 2020, 2:01 p.m. UTC | #2
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`.
> >
> > Rename the variable to make clear that we're looking for a bisect(ion),
> > nothing else.
>
> 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.

Thanks
Martin
Martin Ågren Sept. 27, 2020, 1:29 p.m. UTC | #3
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);
 }
 
 /*
diff mbox series

Patch

diff --git a/worktree.c b/worktree.c
index 050f22dd65..a2d0d20564 100644
--- a/worktree.c
+++ b/worktree.c
@@ -377,15 +377,15 @@  int is_worktree_being_bisected(const struct worktree *wt,
 			       const char *target)
 {
 	struct wt_status_state state;
-	int found_rebase;
+	int found_bisect;
 
 	memset(&state, 0, sizeof(state));
-	found_rebase = wt_status_check_bisect(wt, &state) &&
-		state.branch &&
-		starts_with(target, "refs/heads/") &&
-		!strcmp(state.branch, target + strlen("refs/heads/"));
+	found_bisect = wt_status_check_bisect(wt, &state) &&
+		       state.branch &&
+		       starts_with(target, "refs/heads/") &&
+		       !strcmp(state.branch, target + strlen("refs/heads/"));
 	wt_status_state_free_buffers(&state);
-	return found_rebase;
+	return found_bisect;
 }
 
 /*