git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Dmitry Safonov <dima@arista.com>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Dmitry Safonov" <0x7f454c46@gmail.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>,
	"Andrei Vagin" <avagin@openvz.org>
Subject: Re: [PATCHv3] send-email: Ask if a patch should be sent twice
Date: Tue, 30 Jul 2019 15:13:50 -0700	[thread overview]
Message-ID: <xmqq5znjrx0h.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190730203327.30958-1-dima@arista.com> (Dmitry Safonov's message of "Tue, 30 Jul 2019 21:33:27 +0100")

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?

> 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 ...


  parent reply	other threads:[~2019-07-30 22:13 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 [this message]
2019-07-30 22:56   ` Andrey Vagin
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=xmqq5znjrx0h.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=0x7f454c46@gmail.com \
    --cc=avagin@openvz.org \
    --cc=avarab@gmail.com \
    --cc=dima@arista.com \
    --cc=git@vger.kernel.org \
    --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).