Hi,
On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@xxxxxxxxx> wrote:
> > On Sat, Aug 22, 2020 at 12:47 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> > > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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
