[darcs-devel] patch review

[Ben Franksen]

Am 01.03.20 um 22:48 schrieb Ganesh Sittampalam:
> On 29/02/2020 22:34, Ben Franksen wrote:
> 
>> 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 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?
> 
> BTW don't the problems you describe apply anyway, even if we have "split
> bundles" (i.e. followups with dependencies sent in separate bundles)?

Um. You may have a point here.

> One approach is to pull the patches with that name from screened. There
> shouldn't be any bundles in state 'needs-review' that aren't already in
> screened, and then darcs will help you find the dependency. That's
> usually what I do when there are missing dependencies.

I'll try to do it like that next time. I didn't before because that
means I'll have these other patches in my "tentative reviewed" clone. So
when I review a different "bundle" (i.e. tracker entry) I have to do
that in a separate clone so I don't accidentally push these other
patches to reviewed. How do you manage that?

> I also aim to work through things in order of the fileXXXX numbers as
> the dependencies are usually roughly in the same sequence.

Good idea. I usually do them oldest first, too, unless the earliest one
is complicated and I'd like to postpone it.

> I actually have a little tool that I use to keep track of what needs to
> be done. It's here, but only builds against an old version of darcs at
> present:
> 
> https://hub.darcs.net/ganesh/darcs-roundup-watch
> 
> It produces output like the attached.

Wow, this is cool!

> It also downloads all the patch
> files so they're available on the local disk. It does show dependencies,
> though I don't find that matters that often. But it makes it easy to
> grab collections of work to do, like checking and pushing
> 'accepted-pending-tests' patches, or reviewing or screening. It also
> means I can review offline more easily.

No wonder you are more effective at reviewing than I am.

Your arguments have convinced me that attaching follow-ups to the same
ticket is cleaner, even if they have "foreign" dependencies. I will try
to adapt my work-flow to yours, perhaps give a little love to your tool
to make it work with current darcs.

Cheers
Ben
-------------- 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/20200302/9b9bf633/attachment.key>