Re: [RFC] git-send-email: Cache generated message-ids, use them when prompting
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:58:27
Rasmus Villemoes [off-list ref] writes:
quoted hunk
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 <redacted> --- 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.
quoted hunk
@@ -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.
quoted hunk
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?
quoted hunk
); 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?
quoted hunk
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".
quoted hunk
@@ -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]"?
quoted hunk
@@ -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?
quoted hunk
@@ -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.