git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Hariom verma <hariom18599@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Hariom Verma via GitGitGadget <gitgitgadget@gmail.com>,
	Git List <git@vger.kernel.org>,
	Christian Couder <christian.couder@gmail.com>,
	Heba Waly <heba.waly@gmail.com>
Subject: Re: [PATCH v2 2/2] ref-filter: 'contents:trailers' show error if `:` is missing
Date: Tue, 25 Aug 2020 05:02:00 +0530	[thread overview]
Message-ID: <CA+CkUQ_eRqOB8Ushg-BcEmjRxEZSs7tmPnZcb8GUTwz3R55Xhg@mail.gmail.com> (raw)
In-Reply-To: <CAPig+cScdV1ORSbqDuUiOEvCd6TYgkR=3GK8OCUu4yuoKVy5Pg@mail.gmail.com>

Hi,

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:
> > On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@pobox.com> wrote:
> > > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > > ...an alternative would have been something like:
> > > >
> > > >   else if (!strcmp(arg, "trailers")) {
> > > >     if (trailers_atom_parser(format, atom, NULL, err))
> > > >       return -1;
> > > >   } else if (skip_prefix(arg, "trailers:", &arg)) {
> > > >     if (trailers_atom_parser(format, atom, arg, err))
> > > >       return -1;
> > > >   }
> > > >
> > > > which is quite simple to reason about (though has the cost of a tiny
> > > > bit of duplication).
> > >
> > > Yeah, that looks quite simple and straight-forward.
> >
> > 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.

> 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.

> 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.

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.

Thanks,
Hariom

  reply	other threads:[~2020-08-24 23:32 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 [this message]
2020-08-26  6:18               ` Christian Couder
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=CA+CkUQ_eRqOB8Ushg-BcEmjRxEZSs7tmPnZcb8GUTwz3R55Xhg@mail.gmail.com \
    --to=hariom18599@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.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).