Re: [RFC] git-send-email: Cache generated message-ids, use them when prompting

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 18, 2013, 21:08 UTC
Message-ID: <7vvc32n1vz.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1376701126-5759-1-git-send-email-rv@rasmusvillemoes.dk>

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

Show 30 quoted lines

> This is mostly a proof of concept/RFC patch. The idea is for
> git-send-email to store the message-ids it generates, along with the
> Subject and Date headers of the message. When prompting for which
> Message-ID should be used in In-Reply-To, display a list of recent
> emails (identifed using the Date/Subject pairs; the message-ids
> themselves are not for human consumption). Choosing from that list
> will then use the corresponding message-id; otherwise, the behaviour
> is as usual.
>
> When composing v2 or v3 of a patch or patch series, this avoids the
> need to get one's MUA to display the Message-ID of the earlier email
> (which is cumbersome in some MUAs) and then copy-paste that.
>
> If this idea is accepted, I'm certain I'll get to use the feature
> immediately, since the patch is not quite ready for inclusion :-)
>
> Signed-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>
> ---
>  git-send-email.perl | 101 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 96 insertions(+), 5 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index f608d9b..2e3685c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -26,6 +26,7 @@ use Data::Dumper;
>  use Term::ANSIColor;
>  use File::Temp qw/ tempdir tempfile /;
>  use File::Spec::Functions qw(catfile);
> +use Date::Parse;

Hmm, is this part of core that we do not have to worry about some people not having it? It appears that Git/SVN/Log.pm explicitly avoids using it.

Show 5 quoted lines

> @@ -203,6 +204,7 @@ my ($validate, $confirm);
>  my (@suppress_cc);
>  my ($auto_8bit_encoding);
>  my ($compose_encoding);
> +my ($msgid_cache_file, $msgid_maxprompt);

I do not see $msgid_maxprompt used anywhere in the new code.

Show 9 quoted lines

>  
>  my ($debug_net_smtp) = 0;		# Net::SMTP, see send_message()
>  
> @@ -214,7 +216,7 @@ my %config_bool_settings = (
>      "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
>      "validate" => [\$validate, 1],
>      "multiedit" => [\$multiedit, undef],
> -    "annotate" => [\$annotate, undef]
> +    "annotate" => [\$annotate, undef],

Is this related to this patch in any way?

Show 17 quoted lines

>  );
>  
>  my %config_settings = (
> @@ -237,6 +239,7 @@ my %config_settings = (
>      "from" => \$sender,
>      "assume8bitencoding" => \$auto_8bit_encoding,
>      "composeencoding" => \$compose_encoding,
> +    "msgidcachefile" => \$msgid_cache_file,
>  );
>  
>  my %config_path_settings = (
> @@ -311,6 +314,7 @@ my $rc = GetOptions("h" => \$help,
>  		    "8bit-encoding=s" => \$auto_8bit_encoding,
>  		    "compose-encoding=s" => \$compose_encoding,
>  		    "force" => \$force,
> +		    "msgid-cache-file=s" => \$msgid_cache_file,
>  	 );

Is there a standard, recommended location we suggest users to store this?

Show 12 quoted lines

>  usage() if $help;
> @@ -784,10 +788,31 @@ sub expand_one_alias {
>  @bcclist = validate_address_list(sanitize_address_list(@bcclist));
>  
>  if ($thread && !defined $initial_reply_to) {
> -	$initial_reply_to = ask(
> -		"Message-ID to be used as In-Reply-To for the first email (if any)? ",
> -		default => "",
> -		valid_re => qr/\@.*\./, confirm_only => 1);
> +	my @choices;
> +	if ($msgid_cache_file) {
> +		@choices = msgid_cache_getmatches();

It is a bit strange that "filename is specified => we will find the latest 10" before seeing if we even check to see if that file exists. I would have expected that a two-step "filename is given => try to read it" and "if we did read something => give choices" process would be used.

Also "getmatches" that does not take any clue from what the caller knows (the title of the series, for example) seems much less useful than ideal. The callee is not getting anything to work with to get sensible "matches".

Show 5 quoted lines

> @@ -1282,6 +1307,8 @@ X-Mailer: git-send-email $gitversion
>  		}
>  	}
>  
> +	msgid_cache_this($message_id, $subject, $date) if ($msgid_cache_file && !$dry_run);

Is this caching each and every one, even for things like "[PATCH 23/47]"?

Show 5 quoted lines

> @@ -1508,6 +1535,8 @@ sub cleanup_compose_files {
>  
>  $smtp->quit if $smtp;
>  
> +msgid_cache_write() if $msgid_cache_file;

Is this done under --dry-run?

Show 16 quoted lines

> @@ -1556,3 +1585,65 @@ sub body_or_subject_has_nonascii {
>  	}
>  	return 0;
>  }
> +
> +my @msgid_new_entries;
> +
> +# For now, use a simple tab-separated format:
> +#
> +#    $id\t$date\t$subject\n
> +sub msgid_cache_read {
> +	my $fh;
> +	my $line;
> +	my @entries;
> +	if (not open ($fh, '<', $msgid_cache_file)) {
> +		# A non-existing cache file is ok, but should we warn if errno != ENOENT?

It should not be a warning but an informational message, "creating a new cachefile", when bootstrapping, no?

Show 8 quoted lines

> +	while ($line = <$fh>) {
> +		chomp($line);
> +		my ($id, $date, $subject) = split /\t/, $line;
> +		my $epoch = str2time($date);
> +		push @entries, {id=>$id, date=>$date, epoch=>$epoch, subject=>$subject};
> +	}
> +	close($fh);
> +	return @entries;

So all the old ones are read, without dropping ancient and possibly useless ones here...

Show 8 quoted lines

> +sub msgid_cache_write {
> +	my $fh;
> +	if (not open($fh, '>>', $msgid_cache_file)) {
> +	    warn "cannot open $msgid_cache_file for appending: $!";
> +	    return;
> +	}
> +	printf $fh "%s\t%s\t%s\n", $_->{id}, $_->{date}, $_->{subject} for (@msgid_new_entries);
> +	close($fh);

And the new ones are appended to the end without expiring anything.

When does the cache shrink, and who is responsible for doing so?

Show 7 quoted lines

> +# Return an array of cached message-ids, ordered by "relevance". It
> +# might make sense to take the Subject of the new mail as an extra
> +# argument and do some kind of fuzzy matching against the old
> +# subjects, but for now "more relevant" simply means "newer".
> +sub msgid_cache_getmatches {
> +	my ($maxentries) = @_;
> +	$maxentries //= 10;

The //= operator is mentioned in perl581delta.pod, it seems, and none of our Perl scripted Porcelains seems to use it yet. Is it safe to assume that everybody has it?

Show 18 quoted lines

> +	my @list = msgid_cache_read();
> +	@list = sort {$b->{epoch} <=> $a->{epoch}} @list;
> +	@list = @list[0 .. $maxentries-1] if (@list > $maxentries);
> +	return @list;
> +}
> +
> +sub msgid_cache_this {
> +	my $msgid = shift;
> +	my $subject = shift;
> +	my $date = shift;
> +	# Make sure there are no tabs which will confuse us, and save
> +	# some valuable horizontal real-estate by removing redundant
> +	# whitespace.
> +	if ($subject) {
> +		$subject =~ s/^\s+|\s+$//g;
> +		$subject =~ s/\s+/ /g;
> +	}
> +	# Replace undef or the empty string by an actual string. Nobody uses "0" as the subject...

That's a strange sloppiness for somebody who uses //=, no?

Show 5 quoted lines

> +	$subject ||= '(none)';
> +	$date //= format_2822_time(time());
> +	$date =~ s/\s+/ /g;
> +	push @msgid_new_entries, {id => $msgid, subject => $subject, date => $date};
> +}

I am personally not very interested, but that only means I will not be the one who will be enthusiastically rooting for inclusion; it does not mean I reject the general notion outright.

But the behaviour the posted patch seems to implement seems to fall far short of how useful the general notion "save message ID of what we sent, so that we can reply to them" could be.