[PATCH 09/13] email: add support for X-Aiaiai-Cancel-Email-Test-Patchset

[Keller, Jacob E]

Hi,

I have thought more about this. Comments inlined.

On Mon, 2014-03-24 at 10:40 +0200, Artem Bityutskiy wrote:
> On Fri, 2014-03-21 at 20:57 +0000, Keller, Jacob E wrote:
> > > -----Original Message-----
> > > From: Artem Bityutskiy [mailto:dedekind1 at gmail.com]
> > > Sent: Friday, March 21, 2014 10:32 AM
> > > To: Keller, Jacob E
> > > Cc: aiaiai at lists.infradead.org
> > > Subject: Re: [PATCH 09/13] email: add support for X-Aiaiai-Cancel-Email-
> > > Test-Patchset
> > > 
> > > On Thu, 2014-03-20 at 16:37 -0700, Jacob Keller wrote:
> > > > +
> > > > +# Check whether a hook has indicated to cancel the testing this patch
> > > > set
> > > > +cancel="$(fetch_header "X-Aiaiai-Cancel-Email-Test-Patchset")"
> > > > +if [ -n "$cancel" ] && [ "$cancel" = "1" ]; then
> > > > +       verbose "Canceling $PROG without testing."
> > > > +       exit 0
> > > > +fi
> > > 
> > > So which e-mail will the sender receive if the patch-set testing was
> > > canceled?
> > 
> > In my example it's expected that the hook is the one that notifies the user, and this flag was the cleanest way to stop the email-test-patchset from running at all essentially...
> > 
> > That could be changed, but I expected the hook to have the most information about why the patch was cancelled.
> 
> OK. I do not know what is the better design, but would be nice to have
> less code duplication.

Definitely!

> 
> What do you think if the hooks are invoked by
> 'aiaia-email-testpatchset', rather than the dispatcher?
> 

This is a good change. Dispatcher has less information. Hook must be run
before project detection though, in order to support my idea.

> So, tespatchet would check the project config, see which hooks should be
> invoked, and run them.

Yes.

> 
> testpatches would probably know the specifics of the hooks too. E.g.,
> what they return.

I think we can standardize on format which is processable via
"fetch_header" but instead of passing $mbox we pass the stdout of the
hook.

> 
> I guess, generally, if hook returns failure exit code, then the stdout
> of the hook is considered to be the reply to the user, and testpatchet
> sends it out.

I suppose we pick a standard error code. A hook is expected to return
say 0 for success, in which case interpret the stdout of the hook as
headers processed via fetch_header? If it returns say 128, assume the
patch is bad, and send the stdout as an email.

If it returns anything but 128 or 0, assume some other internal error,
and send an appropriate email to the administrator, canceling aiaiai
completely.

> 
> If hook returns success, then testpatched proceeds. The stdout of the
> hook is interpeted differently, depending on the hook.

I propose that we interpret it like mailbox headers, but no need to
modify mbox directly. This makes hook implementation simpler, with fewer
dependency on aiaiai or knowing the tricks aiaiai does for this.

> 
> Testpatchset would also add various notes to the e-mail reply. For
> example, if the base commit was changed, testpatchset could tell about
> this in the e-mail reply.

Yes. I agree, we should have the email include information about this.

> 
> The e-mail tags would not be used, then, I guess.

I propose using similar tags, but not modify mbox directly. This way we
should be able to take advantage of fetch_header.

> 
> What do you think? I do not insist on any of the two, just asking. You
> gave this some thought, so you may see the pros and cons of both.
> 

I think this is a bit cleaner approach. I will draw something up.


How would you feel if I based this on master, and we did a force update
of the devel branch? I doubt anyone else has pulled it yet... and I
prefer clean history vs cluttered ideas that we later removed. If not, I
can make a single revert commit first, so that the other changes come in
logical order.

Thanks,
Jake

> Thanks!
>