git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 6/8] worktree: rename copy-pasted variable
Date: Sun, 27 Sep 2020 15:29:10 +0200
Message-ID: <20200927132910.20515-1-martin.agren@gmail.com> (raw)
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:
>
> 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


  reply	other threads:[~2020-09-27 13:29 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-10 19:03 [PATCH 0/8] various wt-status/worktree cleanups Martin Ågren
2020-09-10 19:03 ` [PATCH 1/8] wt-status: replace sha1 mentions with oid Martin Ågren
2020-09-10 19:03 ` [PATCH 2/8] wt-status: print to s->fp, not stdout Martin Ågren
2020-09-10 19:03 ` [PATCH 3/8] wt-status: introduce wt_status_state_free_buffers() Martin Ågren
2020-09-10 19:03 ` [PATCH 4/8] worktree: drop useless call to strbuf_reset Martin Ågren
2020-09-10 19:15   ` Eric Sunshine
2020-09-10 19:39     ` Martin Ågren
2020-09-10 19:49       ` Eric Sunshine
2020-09-12 14:02         ` Martin Ågren
2020-09-10 19:03 ` [PATCH 5/8] worktree: update renamed variable in comment Martin Ågren
2020-09-10 19:03 ` [PATCH 6/8] worktree: rename copy-pasted variable Martin Ågren
2020-09-10 20:29   ` Junio C Hamano
2020-09-12 14:01     ` Martin Ågren
2020-09-27 13:29       ` Martin Ågren [this message]
2020-09-10 19:03 ` [PATCH 7/8] worktree: use skip_prefix to parse target Martin Ågren
2020-09-10 19:03 ` [PATCH 8/8] worktree: simplify search for unique worktree Martin Ågren
2020-09-10 19:28   ` Eric Sunshine
2020-09-10 19:48     ` Martin Ågren
2020-09-10 20:01       ` Eric Sunshine
2020-09-10 21:08         ` Junio C Hamano
2020-09-12  3:49 ` [PATCH 0/8] various wt-status/worktree cleanups Taylor Blau
2020-09-12 14:03   ` Martin Ågren
2020-09-27 13:15 ` [PATCH v2 0/7] " Martin Ågren
2020-09-27 13:15   ` [PATCH v2 1/7] wt-status: replace sha1 mentions with oid Martin Ågren
2020-09-27 13:15   ` [PATCH v2 2/7] wt-status: print to s->fp, not stdout Martin Ågren
2020-09-27 13:15   ` [PATCH v2 3/7] wt-status: introduce wt_status_state_free_buffers() Martin Ågren
2020-09-27 13:15   ` [PATCH v2 4/7] worktree: inline `worktree_ref()` into its only caller Martin Ågren
2020-09-28  5:30     ` Eric Sunshine
2020-09-28  6:57       ` Martin Ågren
2020-09-28  7:16         ` Eric Sunshine
2020-09-27 13:15   ` [PATCH v2 5/7] worktree: update renamed variable in comment Martin Ågren
2020-09-27 13:15   ` [PATCH v2 6/7] worktree: rename copy-pasted variable Martin Ågren
2020-09-27 13:15   ` [PATCH v2 7/7] worktree: use skip_prefix to parse target Martin Ågren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200927132910.20515-1-martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git