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

[Junio C Hamano <gitster@pobox.com>]

Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:

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

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

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

>  );
>  
>  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?  

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

> @@ -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]"?

> @@ -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?

> @@ -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?

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

> +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?

> +# 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?

> +	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?

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