Re: RFC: [PATCH] trans/fakeroot.c

[Samuel Thibault]

Svante Signell, le Tue 12 May 2015 10:00:26 +0200, a écrit :
> On Tue, 2015-05-12 at 09:42 +0200, Samuel Thibault wrote:
> > Svante Signell, le Tue 12 May 2015 09:15:46 +0200, a écrit :
> > > The idea is to limit the openmodes according to the values defined in
> > > struct netnode and to change the test for overlapping sets.
> > 
> > In which case is this needed? What happens in that case and how the
> > patch fixes this?
> 
> I already supplied a test case. What's up?

Explain what happens.

Only providing a testcase means the reviewer has to

- understand what the testcase is doing
- infer how the patched source code gets triggered
- find out how that source code does things wrong
- understand why the proposed changes fixes that behavior

Which takes a lot of time, while the submitter already know all this and
just has to explain it.

For instance here, just inventing dumb things, it's just to give an
example of what I need to see explained to avoid having to spend a few
hours to understand what's happening: the testcase creates a socket,
whose mode is not just read/write, and thus it test at line 1234 it
erroneously takes the "then" branch, while it shouldn't, since it's just
a socket. With the proposed changes, the function will just reject the
mode, and thus the caller will correctly revert back to code path "bar"
which does things properly: it calls foobaz(), which is what we want for
the testcase.

Really, doing this won't take you much time, and save the reviewer a lot
of time (which is just duplicate since it's exactly what you did in your
work).

Samuel