[PATCH 2 of 2] patchbomb: display series summary and ask for confirmation

[Christian Ebert]

* Mads Kiilerich on Friday, January 29, 2010 at 18:41:36 +0100
> On 01/29/2010 05:55 PM, Christian Ebert wrote:
>> # HG changeset patch
>> # User sender
>> # Date 1264783904 0
>> # Node ID 0e75f6145d7d9571fc5578b4bcf5e756d6d1a70f
>> # Parent  765b92675e33139e0358c83202e7b10df9176904
>> patchbomb: display series summary and ask for confirmation
> 
> Nice, thanks!

Thanks for the feedback.

> Quick "i will rather ask a stupid question than research properly"
> review notes follows.
> 
>> The prompt can be avoided with -y/--noninteractive.
>> 
>> Confirmation check summary:
>> 
>> Subject: only with -v/--verbose
>> [ ... all subjects of series ... ]
> 
> An explicit statement (or example) of how the subject looks like
> could be nice.

Ok.

> I wonder if it includes [PATCH x of y] and flags?

Yes.

> Why hide it behind verbose? For all practical purposes it will be so
> few lines that they easily fit on the screen, and the lines contain
> information that isn't directly available from looking in the
> repository. Each of the lines represent one potential piece of spam
> (or waste of bandwidth and other peoples time), so the sender should
> always take the time to verify it an extra time.

Ok.

>> From: sender
>> To: recipient(s) or prompt for To
>> Cc: carbon copy recipients
>> Bcc: blind carbon copy recipients
>> 
>> test-patchbomb does not change as it runs in noninteractive mode.
> 
> FWIW:
> I don't like that it behaves differently interactively and from
> scripts. AFAIK Mercurial usually doesn't do that, and some other
> cases of such difference has been fixed.

I don't exactly get what you mean, but patchbomb.prompt() already
has that switch.

> Besides that: IIRC other tests fake interactive mode, and it would
> be very nice to have some tests for this. If for no other reason
> them to make it easier to comment on the exact output format ;-)

I'll include a test.

>> diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
>> --- a/hgext/patchbomb.py
>> +++ b/hgext/patchbomb.py
>> @@ -386,20 +386,35 @@
>> 
>>     def getaddrs(opt, prpt=None, default=None):
>>         if opts.get(opt):
>> +            if ui.interactive():
>> +                ui.write('%s: %s\n'
>> +                         % (opt.capitalize(), ', '.join(opts.get(opt))))
>>             return mail.addrlistencode(ui, opts.get(opt), _charsets,
>>                                        opts.get('test'))
>> 
>> -        addrs = (ui.config('email', opt) or
>> -                 ui.config('patchbomb', opt) or '')
>> +        addrs = ui.config('email', opt) or ui.config('patchbomb', opt) or ''
>>         if not addrs and prpt:
>>             addrs = prompt(ui, prpt, default)
>> +        elif addrs and ui.interactive():
>> +            ui.write('%s: %s\n' % (opt.capitalize(), addrs))
>> +        return mail.addrlistencode(ui, [addrs], _charsets, opts.get('test'))
>> 
>> -        return mail.addrlistencode(ui, [addrs], _charsets, opts.get('test'))
> 
> Attempts of editing and control characters sometimes mess the input
> prompt up. Perhaps it would be nice to postpone the echo to after
> both to, cc and bcc have been determined - and to show them all (if
> they are set), no matter if they were prompted for.

That looks really ugly and like a mistake imho, when you suddenly
have 2 To: or Subject: lines. That's why I made an effort to
avoid dupes. It makes the ui less clear.

> Depending on how the comment above is addressed it seems like this
> function gets more code duplication than before. Perhaps the
> function could be re-factored to have only one return statement?

>> +    if ui.interactive():
>> +        if ui.verbose:
> 
> Why not keep it simple and just use ui.note unconditionally?

Indeed, thanks. But I'll follow your suggestion to not special
case -v/--verbose.

>> +            displaymsgs = ((opts.get('subject') or not opts.get('desc')) and
>> +                           msgs or msgs[1:])
> 
> It is not obvious to me what is going on here and what the rationale is.
> 
> This is to skip the intro message? Why?
> 
> Why not use the "len(patches) > 1 or opts.get('intro')" used other places?

Because it's a different condition, --desc decides on whether the
editor is fired up: if yes, we need to display the subject of the
intro again, otherwise we don't.

>> +            for m, subj in displaymsgs:
>> +                ui.write('Subject: %s\n' % subj)
>> +        ui.write('From: %s\n' % sender)
> 
> It seems like "sender" changes "type" later when it is
> mail.addressencode'ed. Shouldn't we use that instead?
> 
> Perhaps the whole info-display and confirmation prompt could be
> postposted to the place where sender has been encoded, just before
> the loop where the mails are sent?

? I believe showing encoded headers would definitely confuse
non-geek users and make review harder. It should be a _user_
interface.

c
-- 
\black\trash movie               _S A M E_  _T I M E_  _S A M E_  _P L A C E_
                          --->> http://www.blacktrash.org/underdogma/stsp.php
\black\trash audio   _A N O T H E R_  _T I M E_  _A N O T H E R_  _P L A C E_
                         --->> http://www.blacktrash.org/underdogma/atap.html