[PATCH] send-email: error out when relogin delay is missing
When the batch size is neither configured nor given on the command line, but the relogin delay is given, then the current code ignores the relogin delay setting. This is unsafe as there was some intention when setting the batch size. One workaround would be to just assume a batch size of 1 as a default. This however may be bad UX, as then the user may wonder why it is sending slowly without apparent batching. Error out for now instead of potentially confusing the user. As 5453b83bdf (send-email: --batch-size to work around some SMTP server limit, 2017-05-21) lays out, we rather want to not have this interface anyway and would rather want to react on the server throttling dynamically. Helped-by: Eric SunshineSigned-off-by: Stefan Beller --- git-send-email.perl | 4 1 file changed, 4 insertions(+) diff --git a/git-send-email.perl b/git-send-email.perl index 340b5c8482..f7913f7c2c 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -379,6 +379,10 @@ unless ($rc) { die __("Cannot run git format-patch from outside a repository\n") if $format_patch and not $repo; +die __("`batch-size` and `relogin` must be specified together " . + "(via command-line or configuration option)\n") + if defined $relogin_delay and not defined $batch_size; + # Now, let's fill any that aren't set in with defaults: sub read_config { -- 2.15.1.433.g936d1b9894.dirty
Re: [PATCH] send-email: error out when relogin delay is missing
On Thu, Feb 8, 2018 at 1:21 PM, Stefan Bellerwrote: > On Thu, Feb 8, 2018 at 12:08 AM, Eric Sunshine > wrote: >> On Wed, Feb 7, 2018 at 6:43 PM, Stefan Beller wrote: >>> +die __("When a batch size is given, the relogin delay must be set\n") >>> + if defined $relogin_delay and not defined $batch_size; >> >> This only makes sense is 'batch-size' is specified but not 'relogin'. >> If the other way around, then the error is confusing. How about this >> instead? >> "--batch-size and --relogin must be specified together" >> ...or something. > > I like this for its expressiveness as it would have helped me a lot. > I dislike this because it is incorrect when you use the config options > instead of command line arguments. Perhaps: "`batch-size` and `relogin` must be specified together (via command-line or configuration option)"
Re: [PATCH] send-email: error out when relogin delay is missing
On Thu, Feb 8, 2018 at 12:08 AM, Eric Sunshinewrote: > On Wed, Feb 7, 2018 at 6:43 PM, Stefan Beller wrote: >> [...] >> Error out for now instead of potentially confusing the user. >> As 5453b83bdf (send-email: --batch-size to work around some SMTP >> server limit, 2017-05-21) lays out, we rather want to not have this >> interface anyway and would rather want to react on the server throttling >> dynamically. >> >> Signed-off-by: Stefan Beller >> --- >> diff --git a/git-send-email.perl b/git-send-email.perl >> @@ -379,6 +379,9 @@ unless ($rc) { >> +die __("When a batch size is given, the relogin delay must be set\n") >> + if defined $relogin_delay and not defined $batch_size; > > This only makes sense is 'batch-size' is specified but not 'relogin'. > If the other way around, then the error is confusing. How about this > instead? > > "--batch-size and --relogin must be specified together" > > ...or something. I like this for its expressiveness as it would have helped me a lot. I dislike this because it is incorrect when you use the config options instead of command line arguments. Stefan
Re: [PATCH] send-email: error out when relogin delay is missing
On Wed, Feb 7, 2018 at 6:43 PM, Stefan Bellerwrote: > [...] > Error out for now instead of potentially confusing the user. > As 5453b83bdf (send-email: --batch-size to work around some SMTP > server limit, 2017-05-21) lays out, we rather want to not have this > interface anyway and would rather want to react on the server throttling > dynamically. > > Signed-off-by: Stefan Beller > --- > diff --git a/git-send-email.perl b/git-send-email.perl > @@ -379,6 +379,9 @@ unless ($rc) { > +die __("When a batch size is given, the relogin delay must be set\n") > + if defined $relogin_delay and not defined $batch_size; This only makes sense is 'batch-size' is specified but not 'relogin'. If the other way around, then the error is confusing. How about this instead? "--batch-size and --relogin must be specified together" ...or something.
Re: [PATCH] send-email: error out when relogin delay is missing
> 在 2018年2月8日,上午7:43,Stefan Beller写道: > > +die __("When a batch size is given, the relogin delay must be set\n") > +if defined $relogin_delay and not defined $batch_size; > + According the code, maybe you want to say “When relogin delay is given, a batch size must be set “ ?
[PATCH] send-email: error out when relogin delay is missing
When the batch size is neither configured nor given on the command line, but the relogin delay is given, then the current code ignores the relogin delay setting. This is unsafe as there was some intention when setting the batch size. One workaround would be to just assume a batch size of 1 as a default. This however may be bad UX, as then the user may wonder why it is sending slowly without apparent batching. Error out for now instead of potentially confusing the user. As 5453b83bdf (send-email: --batch-size to work around some SMTP server limit, 2017-05-21) lays out, we rather want to not have this interface anyway and would rather want to react on the server throttling dynamically. Helped-by: Eric SunshineSigned-off-by: Stefan Beller --- git-send-email.perl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git-send-email.perl b/git-send-email.perl index 340b5c8482..bc0d3ade16 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -379,6 +379,9 @@ unless ($rc) { die __("Cannot run git format-patch from outside a repository\n") if $format_patch and not $repo; +die __("When a batch size is given, the relogin delay must be set\n") + if defined $relogin_delay and not defined $batch_size; + # Now, let's fill any that aren't set in with defaults: sub read_config { -- 2.16.0.rc1.238.g530d649a79-goog