Wireshark-dev: Re: [Wireshark-dev] Why are authors never Cc'ed before their code is changed?
Hi Pascal,
> For me not all changes have the same potential of breakage: a change in an
> API used by an external tool can easily lead to a complete incompatibility
> (GSMTAP format unapproved changes for example) while a filter rename or a
> wrong subtree used to display a field (GMR-1 breakages seen lately) are
> "just" an annoyance (unless you have an external tool configuring filters by
> command line?).
To me, anything that impacts input / output should be treated with
special attention.
This breaks documentation for example, because all info becomes outdated ...
> That said, applying
> general rules (like filter names) to the whole code base once defined should
> have precedence over a single developer feelings.
Sure, I'm all for consistency but sometimes rules need to be revised
for changing environment.
Now I will leave this particular discussion for the other thread were
I explained my arguments.
> I can see different type of commits:
> [...snip...]
> - general code cleaning spreading across several files. Usually it should
> not impact the dissector usability while complying with the coding style.
> Getting in touch with any contributor simply because a given Wireshark
> internal API gets deprecated and replaced by another one is not doable from
> my point of vue. When the changes are related to warnings removal (if I
> remember correctly it was the root cause of one of the two regressions seen
> with GMR-1 dissector) author could be contacted also. To avoid this kind of
> problems, an author can also check by himself the Clang / MSVC Code Analysis
> warnings for his own dissectors as they are freely available on the buildbot
> pages ;)
My opinion is that I'd like to be CC'ed for every change. Obviously
that needs to be automatic in someway and I guess I can setup a cron
myself for that to check the tree periodically ... I mean even API
changes I'm interested, even just because I always have "newer /
experimental" version of the dissector in my tree and they may need
updating as well.
I agree that for API changes there is not much point in asking the
author his opinion. But if I take the last 3 changes to the gmr
dissector as example:
- PD tree warning fixes
- Rename of RR to CCCH (granted you fixed it yourself, but still I
think it shouldn't have happened in the first place)
- Filter changes
Those 3 changed the way the dissector worked. The code before and
after was not equivalent (not like replacing a 0 by a ENC_BIG_ENDIAN
for example).
And each time I think the problem happened because the person writing
the patch had no understanding of GMR-1 itself. For instance, part of
the code you can't possibly understand why it's been done a given way
just by the code ... simply because the dissector is not complete and
it takes good knowledge of GMR-1 to see the logic in it, to see how it
will expand in the future).
And I've seen this kind of thing happen to other dissectors in the
past. For a while the display of the GSM dissected packet was broken
(IE name repeated twice), seems to be fixed now.
For those kind of changes I think consulting the author would be nice.
Of course you're right that this is not a trivial process to
implement.
Personally I think that having a standardized way to signal (beyond
just the copyright) the "maintainer" of a module would be a start, so
that you have a clear point of contact (and if the original author
doesn't want to maintain, just don't put his name ...). As for the
timeout I think it can be pretty short. All the guy has to do if he
doesn't have the time to do a full review is respond quickly with
"Hold on, I'll review this during the week".
This would also help with reporting bugs. I mean right now if someone
post a decoding bug for the GMR proto on the wireshark and doesn't cc
me explicitely I'm not sure I'll ever see the question ... (I'm
subscribed to the ML ... but I'm subscribed to so many ML ...)
Just my 2ct.
Cheers,
Sylvain
