Re: [PATCH 08/15] ref-filter: make "%(symref)" atom work with the ':short' modifier

Karthik Nayak [off-list ref] writes:

On Tue, Mar 8, 2016 at 7:26 AM, Jacob Keller [off-list ref] wrote:

quoted

On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamano [off-list ref] wrote:

quoted

Karthik Nayak [off-list ref] writes:

quoted

The "%(symref)" atom doesn't work when used with the ':short' modifier
because we strictly match only 'symref' for setting the 'need_symref'
indicator. Fix this by using 'starts_with()' rather than 'strcmp()'.

Does that mean you also accept %(symrefgarbage) without complaining?

Looks like patch 9 fixes this by introducing symref_atom_parser.

There are two ways this kinda errors can occur:
1. %(symrefgarbage) : This is handled by parse_ref_filter_atom() which would
print a "fatal: unknown field name: symrefgarbage".
2. %(symref:garbage): This is handled by populate_value() which would print
a "fatal: unknown symref: format garbage".

Either ways we do not need to worry about this as existing code would handle
it. Also like Jacob mentioned Patch 09 would ensure this error checking would
happen within symref_atom_parser().

You forgot to mention that there is 3., which is that you just
closed the door for a new valid_atom[] that begins with substring
"symref" which does not need to flip need_symref on, I think.

You can check valid_atom[i].name with strcmp() to achieve what you
are trying to do here, instead of checking used_atom[at].name, and I
think that would be a cleaner way to avoid all three problems.

On Wed, Mar 9, 2016 at 1:49 AM, Junio C Hamano [off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

On Tue, Mar 8, 2016 at 7:26 AM, Jacob Keller [off-list ref] wrote:

quoted

On Mon, Mar 7, 2016 at 3:08 PM, Junio C Hamano [off-list ref] wrote:

quoted

Karthik Nayak [off-list ref] writes:

quoted

The "%(symref)" atom doesn't work when used with the ':short' modifier
because we strictly match only 'symref' for setting the 'need_symref'
indicator. Fix this by using 'starts_with()' rather than 'strcmp()'.

Does that mean you also accept %(symrefgarbage) without complaining?

Looks like patch 9 fixes this by introducing symref_atom_parser.

There are two ways this kinda errors can occur:
1. %(symrefgarbage) : This is handled by parse_ref_filter_atom() which would
print a "fatal: unknown field name: symrefgarbage".
2. %(symref:garbage): This is handled by populate_value() which would print
a "fatal: unknown symref: format garbage".

Either ways we do not need to worry about this as existing code would handle
it. Also like Jacob mentioned Patch 09 would ensure this error checking would
happen within symref_atom_parser().

You forgot to mention that there is 3., which is that you just
closed the door for a new valid_atom[] that begins with substring
"symref" which does not need to flip need_symref on, I think.

You can check valid_atom[i].name with strcmp() to achieve what you
are trying to do here, instead of checking used_atom[at].name, and I
think that would be a cleaner way to avoid all three problems.

That's actually a very good point, will incorporate that, thanks!

-- 
Regards,
Karthik Nayak