[darcs-devel] patch review

Ben Franksen ben.franksen at online.de
Sat Feb 29 22:34:12 UTC 2020


>>>   * tests: always use prim patches for generating/shrinking
>>
>> This one depends on patches in other bundles. It would have been better
>> if you had sent this follow-up as a separate bundle.
>>
>> I'll mark as accepted-pending-tests until I reviewed the other stuff.
> 
> I guess this is a question of workflow/logical grouping that it'd be
> helpful for us to agree on.
> 
> I like to keep a single logical group of patches in a single ticket
> (patchXXXX) on the patch tracker, which I think is what you mean by
> bundle here.

Yes.

> If there are followups that belong in the same review, I put them in the
> same ticket because then the review can be accepted as a single entity
> that takes into account both the initial patches and the followups.

Yes, this is a good reason to add them there.

> If this means we end up with patches/dependencies interleaved with other
> patches in screened, I don't see this as a problem. Once something is
> reviewed, I just move it to "accepted-pending-tests", but might not push
> it to reviewed immediately. Every so often, I sweep up any unblocked
> accepted-pending-tests patches, check that they build and the tests
> pass, and push them to reviewed. I might also do this with parts of a
> review bundle after checking that the intermediate state wouldn't be
> fundamentally broken in some subtle way.

This sweeping up thing is one of the things that don't work so well for
me. I mean I can see the accepted-pending-tests patches on the tracker.
But there is no easy way to find out which patches the ticket adds. I
have to open the ticket, download all the bundles, apply them one by
one, perhaps this fails because of missing deps etc etc. I find this
quite a hassle. I wonder how you do that without becoming completely
confused.

> I actually found it a bit confusing when you followed up on a review in
> another bundle recently because I then had to keep track of the
> relationship between the two, though it wasn't much of a big deal. I'm
> ok with doing it that way consistently if it works a lot better for you.

It may well be that I am just missing an obvious trick.

I usually review by downloading the bundle and using 'darcs apply -i' to
add the patches one by one, commenting on things, then apply the next
one etc. If the bundle cannot be applied I have to stop reviewing,
remember where I was, find the tickets that provide the missing patches,
review them, then finally come back to the one I interrupted.

This is clearly suboptimal, but how else can I make sure that what I
review are the patches that were sent?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pEpkey.asc
Type: application/pgp-keys
Size: 4211 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-devel/attachments/20200229/3880ffbd/attachment.key>


More information about the darcs-devel mailing list