git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Andrey Vagin <avagin@openvz.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: "Dmitry Safonov" <dima@arista.com>,
	git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Dmitry Safonov" <0x7f454c46@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCHv3] send-email: Ask if a patch should be sent twice
Date: Tue, 30 Jul 2019 15:56:43 -0700	[thread overview]
Message-ID: <CANaxB-yygbxt=-sP+cyhR9WhcyEck+Wy=453huwfVNC9+So0BA@mail.gmail.com> (raw)
In-Reply-To: <xmqq5znjrx0h.fsf@gitster-ct.c.googlers.com>

On Tue, Jul 30, 2019 at 3:13 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Dmitry Safonov <dima@arista.com> writes:
>
> > I was almost certain that git won't let me send the same patch twice,
>
> Why?  And more importantly, does it matter to readers of this
> message what you thought?

Sounds rude. What matter to readers except author's thoughts? I guess you want
to say that the comment should be in more neutral technical form without
personal pronouns.

>
> > but today I've managed to double-send a directory by a mistake:
> >       git send-email --to linux-kernel@vger.kernel.org /tmp/timens/
> >           --cc 'Dmitry Safonov <0x7f454c46@gmail.com>' /tmp/timens/`
> >
> > [I haven't noticed that I put the directory twice ^^]
> >
> > Prevent this shipwreck from happening again by asking if a patch
> > is sent multiple times on purpose.
> >
> > link: https://lkml.kernel.org/r/4d53ebc7-d5b2-346e-c383-606401d19d3a@gmail.com
>
> What does "link:" mean?
>
> > Cc: Andrei Vagin <avagin@openvz.org>
>
> What's the significance for this project to record that this patch
> was CCed to Andrei?
>
> > Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> I think "Helped-by:" is a lot more appropriate, viewing the exchange
> on v2 from the sideline.
>
> > Signed-off-by: Dmitry Safonov <dima@arista.com>
> > ---
>
> > diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> > index d93e5d0f58f0..0441bb1b5d3b 100644
> > --- a/Documentation/git-send-email.txt
> > +++ b/Documentation/git-send-email.txt
> > @@ -421,6 +421,8 @@ have been specified, in which case default to 'compose'.
> >                       ('auto', 'base64', or 'quoted-printable') is used;
> >                       this is due to SMTP limits as described by
> >                       http://www.ietf.org/rfc/rfc5322.txt.
> > +             *       Ask confirmation before sending patches multiple times
> > +                     if the supplied patches set overlaps.
> >  --
> >  +
> >  Default is the value of `sendemail.validate`; if this is not set,
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index 5f92c89c1c1b..c1638d06f81d 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -688,6 +688,9 @@ sub is_format_patch_arg {
> >  @files = handle_backup_files(@files);
> >
> >  if ($validate) {
> > +     my %seen;
> > +     my @dupes = grep { $seen{$_}++ } @files;
> > +
> >       foreach my $f (@files) {
> >               unless (-p $f) {
> >                       my $error = validate_patch($f, $target_xfer_encoding);
> > @@ -695,6 +698,17 @@ sub is_format_patch_arg {
> >                                                 $f, $error);
> >               }
>
> This is not a fault of your patch at all, but the two hunks are
> curious.  If "git format-patch" chose to coalesce these two hunks
> into one, the second hunk header can be replaced by
>
>                 $error and die sprintf(__("fatal: ..."),
>
> The end result would be that we will spend the same number of lines
> and we will see more useful information.
>
> >       }
> > +     if (@dupes) {
> > +             printf(__("Patches specified several times: \n"));
> > +             printf(__("%s \n" x @dupes), @dupes);
> > +             $_ = ask(__("Do you want to send those patches several times? Y/n "),
> > +                     default => "y",
> > +                     valid_re => qr/^(?:yes|y|no|n)/i);
> > +             if (/^n/i) {
> > +                     cleanup_compose_files();
> > +                     exit(0);
> > +             }
> > +     }
>
> Perhaps this should be inserted _before_ the "let's examine each
> patchfile and complain" loop.  Otherwise, you'd see this warning
> only after seeing the same "the file has too long a line" error
> on the same patch.
>
> While you are counting with %seen how many times the contents of
> @files appear, perhaps you can also create a list of unique files,
> so that you do not have to call validate_patch() more than once
> for each of them.  It would also allow you to offer another choice
> in the above question "do you want to send them more than once?",
> which may be much more useful than yes/no: "drop duplicates".  If
> you did so, you do not need to swap the order of the checks.  You
> would first count the occurences of each element in @files, then
> call validate_patch() on each of them just once, and after you are
> done, check if the user wants to send duplicates, abort, or dedup.
>
> Perhaps like this:
>
>  if ($validate) {
> +       my (@dupes, %seen, @uniq);
> +
>         foreach my $f (@files) {
> +               if ($seen{$f}) {
> +                       if ($seen{$f} == 1) { push @dupes, $f; }
> +                       next;
> +               }
> +               $seen{$f}++;
> +               push @uniq, $f;
> +       }
> +       foreach my $f (@uniq) {
>                 unless (-p $f) {
>                         my $error = validate_patch(...);
>                 ... the existing loop ...
>         }
> +
> +       if (@dupes) {
> +               ... the new code in your patch ...
>

  reply	other threads:[~2019-07-30 22:56 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-30 20:33 Dmitry Safonov
2019-07-30 21:10 ` SZEDER Gábor
2019-07-30 21:19   ` Dmitry Safonov
2019-07-30 22:13 ` Junio C Hamano
2019-07-30 22:56   ` Andrey Vagin [this message]
2019-07-31  1:06     ` Jeff King
2019-07-30 23:05   ` Dmitry Safonov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANaxB-yygbxt=-sP+cyhR9WhcyEck+Wy=453huwfVNC9+So0BA@mail.gmail.com' \
    --to=avagin@openvz.org \
    --cc=0x7f454c46@gmail.com \
    --cc=avarab@gmail.com \
    --cc=dima@arista.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCHv3] send-email: Ask if a patch should be sent twice' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).