On Thu, Jan 7, 2016 at 2:44 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Tue, Jan 5, 2016 at 3:02 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote: >> Eric suggested that I make match_atom_name() not return a value [0]. I >> haven't done that as we use match_atom_name() in [14/15] for matching >> 'subject' and 'body' in contents_atom_parser() and although Eric >> suggested I use strcmp() instead, this would not work as we need to >> check for derefernced 'subject' and 'body' atoms. >> [0]: http://article.gmane.org/gmane.comp.version-control.git/282701 > > I don't understand the difficulty. It should be easy to manually skip > the 'deref' for this one particular case: > > const char *name = atom->name; > if (*name == '*') > name++; > > Which would allow this unnecessarily complicated code from patch 14/15: > > if (match_atom_name(atom->name, "subject", &buf) && !buf) { > ... > return; > } else if (match_atom_name(atom->name, "body", &buf) && !buf) { > ... > return; > } if (!match_atom_name(atom->name, "contents", &buf)) > die("BUG: parsing non-'contents'"); > > to be simplified to the more easily understood form suggested during > review[1] of v2: > > if (!strcmp(name, "subject")) { > ... > return; > } else if (!strcmp(name, "body")) { > ... > return; > } else if (!match_atom_name(name,"contents", &buf)) > die("BUG: expected 'contents' or 'contents:'"); > > You could also just use (!strcmp("body") || !strcmp("*body")) rather > than skipping "*" manually, but the repetition makes that a bit > noisier and uglier. > > [1]: http://article.gmane.org/gmane.comp.version-control.git/282645 Definitely not a difficulty per se. Just that it seems like something match_atom_name() seems to be fit for. As the function name suggests that we're matching the atom name and the check for '!buf' indicates that no options are to be included for that particular atom. Also after Junio's suggestion[1], I think It looks better now[2]. But either ways, I'm not strongly against what you're saying, so my opinion on this matter is quite flexible. [1]: http://article.gmane.org/gmane.comp.version-control.git/283404 [2]: http://article.gmane.org/gmane.comp.version-control.git/283449 -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html