[Dnsmasq-discuss] patch review request, why

Geert Stappers stappers at stappers.nl
Sun Nov 10 23:19:39 GMT 2019


On Sun, Nov 10, 2019 at 01:01:35PM -0800, Kurt H Maier wrote:
> On Sun, Nov 10, 2019 at 11:33:44AM +0100, Patch Submitter wrote:
> > 
> > Imagine an open software project gets a patch submitted.
> 
> Substantive patches tend to get more attention.  At this point you've
> sent one email for each harmless whitespace character you've proposed to
> delete.  Why are you so exercised by this?  It's the worst form of
> bikeshedding.

Yes, bikeshedding is an odd way to express "I do care"


Following dnsmasq learnt me that patches get ignored,
at least some patch do need reminders.

Part of being exercised by this is finding the time
between reminders.  Other part is getting clear feedback
on submitted patches. Having a "patch seen and is rejected"
means there is no need for sending reminders.


The assumption of harmless white space is wrong. Tooling
such as `git` warns about trailing spaces. Do

 rm -f src/000*.patch
 git format-patch 04db148...04db148~1
 git checkout 6fe436a
 patch -p1 < 0001-Fix-crash-on-REFUSED-answers-to-DNSSEC-queries.patch 
 git diff --check

to get
|src/forward.c:949: trailing whitespace.
|+  if (forward->sentto->edns_pktsz > SAFE_PKTSZ && (forward->flags & FREC_TEST_PKTSZ) && 
|src/forward.c:976: trailing whitespace.
|+      if ((forward->sentto->flags & SERV_DO_DNSSEC) && 

Restore your git tree with
 patch -Rp1 < 0001-Fix-crash-on-REFUSED-answers-to-DNSSEC-queries.patch 
 git checkout master

Petr Mensik wrote wisely:
  When I am against forced reformatting like someone here suggested, I
  think some easy checks might be done before commiting changes. For
  example, git diff would show in red whitespaces on lines without
  anything else or after code before end of line. These are not nice and
  I would like them removed.
(http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2019q3/013368.html)


Maybe I should ask for
  sed --in-place -e 's/[ \t]*$//' src/*.c
but
  sed --in-place -e 's/^[ \t]*$//' src/*.c
is already a nice cleanup.


The larger plan is having
  Code style, ie layout, indent width, is not negotiable.
(http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2019q3/013318.html)
and
  I like that style, I chose that style and it's not going to change.
(http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2019q4/013414.html)
documented in something like "clang-format"
(http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2019q4/013417.html)

While working on that plan was "Remove empty tailing lines"
(http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2019q4/013418.html)
revealed.



Groeten
Geert Stappers
-- 
Leven en laten leven



More information about the Dnsmasq-discuss mailing list