From: Christian Couder <christian.couder@gmail.com>
To: Hariom verma <hariom18599@gmail.com>
Cc: Eric Sunshine <sunshine@sunshineco.com>,
Junio C Hamano <gitster@pobox.com>,
Hariom Verma via GitGitGadget <gitgitgadget@gmail.com>,
Git List <git@vger.kernel.org>, Heba Waly <heba.waly@gmail.com>
Subject: Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing
Date: Wed, 26 Aug 2020 08:18:29 +0200 [thread overview]
Message-ID: <CAP8UFD03Am94_84FvRPxEdt_AG74864eQ4TimggKtUYWjJYqCg@mail.gmail.com> (raw)
In-Reply-To: <CA+CkUQ_eRqOB8Ushg-BcEmjRxEZSs7tmPnZcb8GUTwz3R55Xhg@mail.gmail.com>
Hi,
On Tue, Aug 25, 2020 at 1:32 AM Hariom verma <hariom18599@gmail.com> wrote:
> On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@gmail.com> wrote:
> > > Recently, I sent a patch series "Improvements to ref-filter"[1]. A
> > > patch in this patch series introduced "sanitize" modifier to "subject"
> > > atom. i.e "%(subject:sanitize)".
> > >
> > > What if in the future we also want "%(contents:subject:sanitize)" to work?
> > > We can use this helper any number of times, whenever there is a need.
> > >
> > > Sorry, I missed saying this earlier. But I don't prefer duplicating
> > > the code here.
> >
> > Pushing back on a reviewer suggestion is fine. Explaining the reason
> > for your position -- as you do here -- helps reviewers understand why
> > you feel the way you do. My review suggestion about making it easier
> > to reason about the code while avoiding a brand new function, at the
> > cost of a minor amount of duplication, was made in the context of this
> > one-off case in which the function increased cognitive load and was
> > used just once (not knowing that you envisioned future callers). If
> > you expect the new function to be re-used by upcoming changes, then
> > that may be a good reason to keep it. Stating so in the commit message
> > will help reviewers see beyond the immediate patch or patch series.
>
> Yeah. I should have mentioned this in the commit message.
I agree.
> > Aside from a couple minor style violations[1,2], I don't particularly
> > oppose the helper function, though I have a quibble with the name
> > check_format_field(), which I don't find helpful, and which (at least
> > for me) increases the cognitive load. The increased cognitive load, I
> > think, comes not only from the function name not spelling out what the
> > function actually does, but also because the function is dual-purpose:
> > it's both checking that the argument matches a particular token
> > ("trailers", in this case) and extracting the sub-argument. Perhaps
> > naming it match_and_extract_subarg() or something similar would help,
> > though that's a mouthful.
>
> I will fix those violations.
> Also, "match_and_extract_subarg()" looks good to me.
I am not sure about the "subarg" part of the name. In the for-each-ref
doc, names inside %(...) are called "field names", and parts after ":"
are called "options". So it might be better to have "field_option"
instead of "subarg" in the name.
I think we could also get rid of the "match_and_" part of the
suggestion, in the same way as skip_prefix() is not called
match_and_skip_prefix(). Readers can just expect that if there is no
match the function will return 0.
So maybe "extract_field_option()".
> > But the observation about the function being dual-purpose (thus
> > potentially confusing) brings up other questions. For instance, is it
> > too special-purpose? If you foresee more callers in the future with
> > multiple-token arguments such as `%(content:subject:sanitize)`, should
> > the function provide more assistance by splitting out each of the
> > sub-arguments rather than stopping at the first? Taking that even
> > further, a generalized helper for "splitting" arguments like that
> > might be useful at the top-level of contents_atom_parser() too, rather
> > than only for specific arguments, such as "trailers". Of course, this
> > may all be way too ambitious for this little bug fix series or even
> > for whatever upcoming changes you're planning, thus not worth
> > pursuing.
>
> Splitting sub-arguments is done at "<atomname>_atom_parser()".
> If you mean pre-splitting every argument...
> something like: ['contents', 'subject', 'sanitize'] for
> `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not
> able to see how it can be useful.
Yeah, it seems to me that such a splitting would require a complete
rewrite of the current code, so I am not sure it's an interesting way
forward for now. And anyway adding extract_field_option() goes in the
right direction of abstracting the parsing and making the code
simpler, more efficient and likely more correct.
> Sorry, If I got your concerned wrong.
>
> > As for the helper's implementation, I might have written it like this:
> >
> > static int check_format_field(...)
> > {
> > const char *opt
> > if (!strcmp(arg, field))
> > *option = NULL;
> > else if (skip_prefix(arg, field, opt) && *opt == ':')
> > *option = opt + 1;
> > else
> > return 0;
> > return 1;
> > }
> >
> > which is more compact and closer to what I suggested earlier for
> > avoiding the helper function in the first place. But, of course,
> > programming is quite subjective, and you may find your implementation
> > easier to reason about. Plus, your version has the benefit of being
> > slightly more optimal since it avoids an extra string scan, although
> > that probably is mostly immaterial considering that
> > contents_atom_parser() itself contains a long chain of potentially
> > sub-optimal strcmp() and skip_prefix() calls.
>
> "programming is quite subjective"
> Yeah, I couldn't agree more.
>
> The change you suggested looks good too. But I'm little inclined to my
> keeping my changes. I'm curious, what others have to say on this.
I also prefer a slightly more optimal one even if it's a bit less compact.
Thanks,
Christian.
next prev parent reply other threads:[~2020-08-26 6:18 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-19 12:52 [PATCH 0/2] Fix trailers atom bug and improved tests Hariom Verma via GitGitGadget
2020-08-19 12:52 ` [PATCH 1/2] t6300: unify %(trailers) and %(contents:trailers) tests Hariom Verma via GitGitGadget
2020-08-19 17:31 ` Junio C Hamano
2020-08-21 10:03 ` Hariom verma
2020-08-19 12:52 ` [PATCH 2/2] ref-filter: 'contents:trailers' show error if `:` is missing Hariom Verma via GitGitGadget
2020-08-19 17:55 ` Junio C Hamano
2020-08-19 19:07 ` Junio C Hamano
2020-08-19 19:39 ` Eric Sunshine
2020-08-19 22:08 ` Junio C Hamano
2020-08-20 17:19 ` Hariom verma
2020-08-21 10:11 ` [PATCH v2 0/2] Fix trailers atom bug and improved tests Hariom Verma via GitGitGadget
2020-08-21 10:11 ` [PATCH v2 1/2] t6300: unify %(trailers) and %(contents:trailers) tests Hariom Verma via GitGitGadget
2020-08-21 10:11 ` [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing Hariom Verma via GitGitGadget
2020-08-21 16:56 ` Eric Sunshine
2020-08-21 19:17 ` Junio C Hamano
2020-08-23 19:25 ` Hariom verma
2020-08-24 3:49 ` Eric Sunshine
2020-08-24 23:32 ` Hariom verma
2020-08-26 6:18 ` Christian Couder [this message]
2020-08-26 6:22 ` Christian Couder
2020-08-26 15:18 ` Hariom verma
2020-08-21 21:06 ` [PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests Hariom Verma via GitGitGadget
2020-08-21 21:06 ` [PATCH v3 1/4] t6300: unify %(trailers) and %(contents:trailers) tests Hariom Verma via GitGitGadget
2020-08-21 21:06 ` [PATCH v3 2/4] ref-filter: 'contents:trailers' show error if `:` is missing Hariom Verma via GitGitGadget
2020-08-21 21:13 ` Eric Sunshine
2020-08-21 16:19 ` Hariom verma
2020-08-21 21:54 ` Junio C Hamano
2020-08-21 21:06 ` [PATCH v3 3/4] pretty.c: refactor trailer logic to `format_set_trailers_options()` Hariom Verma via GitGitGadget
2020-08-21 21:06 ` [PATCH v3 4/4] ref-filter: using pretty.c logic for trailers Hariom Verma via GitGitGadget
2020-08-21 21:56 ` [PATCH v3 0/4] [GSoC] Fix trailers atom bug and improved tests Junio C Hamano
2020-08-22 14:03 ` Hariom verma
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=CAP8UFD03Am94_84FvRPxEdt_AG74864eQ4TimggKtUYWjJYqCg@mail.gmail.com \
--to=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=hariom18599@gmail.com \
--cc=heba.waly@gmail.com \
--cc=sunshine@sunshineco.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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).