From: Eric Sunshine <sunshine@sunshineco.com>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: Git List <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 8/8] worktree: simplify search for unique worktree
Date: Thu, 10 Sep 2020 16:01:57 -0400 [thread overview]
Message-ID: <CAPig+cQ871p8+oQdBBY=ebDdjDWfa5NvEMroitEmk4Db8DfLvA@mail.gmail.com> (raw)
In-Reply-To: <CAN0heSrUWiZ_xar3G5rZG-c=8OVp5-eByS6rMXOw9wfTA8kmbA@mail.gmail.com>
On Thu, Sep 10, 2020 at 3:48 PM Martin Ågren <martin.agren@gmail.com> wrote:
> On Thu, 10 Sep 2020 at 21:28, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Although this change appears to be correct and does simplify the code,
> > I think it also makes it a bit more opaque. With the explicit
> > `nr_found == 1`, it is quite obvious that the function considers
> > "success" to be when one and only one entry is found and any other
> > number is failure. But with the revised code, it is harder to work out
> > precisely what the conditions are.
>
> Thanks for commenting. I found the original trickier than it had to be.
> It spreads out the logic in several places and is careful to short-cut
> the loop. My first thought was "why doesn't this just use the standard
> form?". But I'm open to the idea that it might be a fairly personal
> standard form... If there's any risk that someone else comes along and
> simplifies this to use a `nr_found` variable, then maybe file this under
> code churning?
Maybe. Dunno. Even with the suggested function comment, I still find
that the revised code has a higher cognitive load because the reader
has to remember or figure out mentally what `found` contains by the
`return found;` at the end of the function. Compare that with the old
code, in which the reader doesn't have to remember or figure out
anything. The final `return nr_found == 1 ? found : NULL;` condition
spells out everything the reader needs to know -- even if the reader
didn't pay close attention to the meat of the function. So, we each
have a different take on the apparent complexity.
> > Having said that, I think a simple
> > comment before the function would suffice to clear up the opaqueness.
> > Perhaps something like:
> >
> > /* If exactly one worktree matches 'target', return it, else NULL. */
>
> That's a good suggestion regardless.
The function is so small that the increased cognitive load (for me) in
the rewrite probably doesn't matter at all, so I don't feel strongly
one way or the other. Keeping the patch (amended with the suggested
comment) or dropping it are both suitable options.
next prev parent reply other threads:[~2020-09-10 20:03 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
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 [this message]
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cQ871p8+oQdBBY=ebDdjDWfa5NvEMroitEmk4Db8DfLvA@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=martin.agren@gmail.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.