{"thread":{"id":"61297","subject":"[PATCH] mailsplit add option to include sanitized subject in filename","startedAt":"2024-04-09T00:05:56Z","lastAt":"2024-04-13T00:07:35Z","messageCount":8,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492598","messageId":"20240409000546.3628898-1-jacob.e.keller@intel.com","threadId":"61297","inReplyTo":null,"subject":"[PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-04-09T00:05:46Z","receivedAt":"2024-04-09T00:05:56Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\ngit-am makes use of git-mailsplit to split an mbox into individual files\nbefore attempting to apply the patches. In most cases this works fine,\nbut it can fail to apply patches in cases where the mbox file is not\nproperly sorted. This can sometimes happen due to clock skew, or other\nissues with the software which saved the mbox.\n\nFor example, if you download a t.mbox.gz from a public inbox server such\nas lore.kernel.org it may sort the messages in the thread by arrival\ntime to the list. Due to clock skew or other issues this may not be the\ncorrect order of the patches to apply.\n\nA savvy user may then attempt to directly use git mailsplit to split the\nmailbox, only to find that the files are unhelpfully named \"0001\",\n\"0002\", etc. It requires further digging to figure out which message is\nwhich patch.\n\nGit has a format_sanitized_subject() function which is used by code to\ngenerate a suitable filename from a subject. Add a new --name-by-subject\noption to git mailsplit. If enabled, scan for lines beginning with the\n\"Subject:\" header when splitting mail. If found, extract the subject and\npass it to format_sanitized_subject(). Use this to create a new filename\nwhich appends the sanitized subject to the standard sequence number. A\nsavvy user can invoke git mailsplit with --name-by-subject to help\nanalyze why the mailbox was not split the intended way.\n\nI originally wanted to avoid the need for an option, but git-am\ncurrently depends on the strict sequence number filenames. It is unclear\nhow difficult it would be to refactor git-am to work with names that\ninclude the extra subject data.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jacob Keller \n---\n Documentation/git-mailsplit.txt | 5 +++++\n builtin/mailsplit.c | 25 ++++++++++++++++++++++++-\n t/t5100-mailinfo.sh | 25 +++++++++++++++++++++++++\n 3 files changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-mailsplit.txt b/Documentation/git-mailsplit.txt\nindex 3f0a6662c81e..2e5ba45e1988 100644\n--- a/Documentation/git-mailsplit.txt\n+++ b/Documentation/git-mailsplit.txt\n@@ -9,6 +9,7 @@ SYNOPSIS\n --------\n [verse]\n 'git mailsplit' [-b] [-f] [-d] [--keep-cr] [--mboxrd]\n+\t\t[--name-by-subject]\n \t\t-o [--] [(|)...]\n \n DESCRIPTION\n@@ -52,6 +53,10 @@ OPTIONS\n \tInput is of the \"mboxrd\" format and \"^>+From \" line escaping is\n \treversed.\n \n+--name-by-subject::\n+\tInclude the sanitized subject in the generated filenames, in\n+\taddition to the sequence number.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 3af9ddb8ae5c..df81782d05b3 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -8,9 +8,10 @@\n #include \"gettext.h\"\n #include \"string-list.h\"\n #include \"strbuf.h\"\n+#include \"pretty.h\"\n \n static const char git_mailsplit_usage[] =\n-\"git mailsplit [-d] [-f] [-b] [--keep-cr] -o [(|)...]\";\n+\"git mailsplit [-d] [-f] [-b] [--keep-cr] [--name-by-subject] -o [(|)...]\";\n \n static int is_from_line(const char *line, int len)\n {\n@@ -46,6 +47,7 @@ static int is_from_line(const char *line, int len)\n static struct strbuf buf = STRBUF_INIT;\n static int keep_cr;\n static int mboxrd;\n+static int name_by_subject;\n \n static int is_gtfrom(const struct strbuf *buf)\n {\n@@ -66,6 +68,9 @@ static int is_gtfrom(const struct strbuf *buf)\n */\n static int split_one(FILE *mbox, const char *name, int allow_bare)\n {\n+\tstruct strbuf sanitized_filename = STRBUF_INIT;\n+\tconst char *subject_start;\n+\tsize_t subject_len;\n \tFILE *output;\n \tint fd;\n \tint status = 0;\n@@ -101,10 +106,26 @@ static int split_one(FILE *mbox, const char *name, int allow_bare)\n \t\t\t}\n \t\t\tdie_errno(\"cannot read mbox\");\n \t\t}\n+\n+\t\t/* Get a sanitized filename from the subject */\n+\t\tif (name_by_subject && !sanitized_filename.len &&\n+\t\t skip_prefix_mem(buf.buf, buf.len, \"Subject:\",\n+\t\t\t\t &subject_start, &subject_len)) {\n+\t\t\tstrbuf_addf(&sanitized_filename, \"%s-\", name);\n+\t\t\tformat_sanitized_subject(&sanitized_filename,\n+\t\t\t\t\t\t subject_start,\n+\t\t\t\t\t\t subject_len);\n+\t\t}\n+\n \t\tif (!is_bare && is_from_line(buf.buf, buf.len))\n \t\t\tbreak; /* done with one message */\n \t}\n \tfclose(output);\n+\n+\tif (name_by_subject && sanitized_filename.len)\n+\t\trename(name, sanitized_filename.buf);\n+\tstrbuf_release(&sanitized_filename);\n+\n \treturn status;\n }\n \n@@ -296,6 +317,8 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)\n \t\t\tusage(git_mailsplit_usage);\n \t\t} else if ( arg[1] == 'b' && !arg[2] ) {\n \t\t\tallow_bare = 1;\n+\t\t} else if (!strcmp(arg, \"--name-by-subject\")) {\n+\t\t\tname_by_subject = 1;\n \t\t} else if (!strcmp(arg, \"--keep-cr\")) {\n \t\t\tkeep_cr = 1;\n \t\t} else if ( arg[1] == 'o' && arg[2] ) {\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex c8d06554541c..4826735c6033 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -44,6 +44,31 @@ do\n \t'\n done\n \n+test_expect_success 'split sample box with --name-by-subject' '\n+\tmkdir name-by-subject &&\n+\tgit mailsplit --name-by-subject -oname-by-subject \"$DATA/sample.mbox\" >last &&\n+\tlast=$(cat last) &&\n+\techo total is $last &&\n+\ttest $(cat last) = 18\n+'\n+\n+check_mailinfo_name_by_subject () {\n+\tmail=$1\n+\tmo=\"$(basename \"$mail\" | cut -c1-4)\"\n+\techo \"$(basename \"$mail\")\" >\"sanitized$mo\" &&\n+\tgit mailinfo -u \"msg$mo\" \"patch$mo\" <\"$mail\" >\"info$mo\" &&\n+\ttest_cmp \"$DATA/msg$mo\" \"msg$mo\" &&\n+\ttest_cmp \"$DATA/patch$mo\" \"patch$mo\" &&\n+\ttest_cmp \"$DATA/info$mo\" \"info$mo\" &&\n+\ttest_cmp \"$DATA/sanitized$mo\" \"sanitized$mo\"\n+}\n+\n+for mail in name-by-subject/00*\n+do\n+\ttest_expect_success \"check --name-by-subject $mail\" '\n+\t\tcheck_mailinfo_name_by_subject \"$mail\"\n+\t'\n+done\n \n test_expect_success 'split box with rfc2047 samples' \\\n \t'mkdir rfc2047 &&\n-- \n2.44.0.53.g0f9d4d28b7e6\n\n"},{"id":"492609","messageId":"xmqqpluz2tau.fsf@gitster.g","threadId":"61297","inReplyTo":"20240409000546.3628898-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-09T01:55:53Z","receivedAt":"2024-04-09T01:55:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> I originally wanted to avoid the need for an option, but git-am\n> currently depends on the strict sequence number filenames. It is\n> unclear how difficult it would be to refactor git-am to work with\n> names that include the extra subject data.\n\nI am not sure if I follow. Do you mean\n\n\t$ git am ./dir/0*.txt\n\nin a directory where I already have these files\n\n\t$ ls dir/0*.txt\n\tdir/0001-Documentation-CodingGuidelines.txt\n\tdir/0002-quote-assigned-value.txt\n\tdir/0003-t-local-var.txt\n\nthat have one patch per message does not work?\n\n"},{"id":"492751","messageId":"CA+P7+xooa08Y-D8CXDGK7_aZ5c2b9iXM6+rFS5qNLyZaG0Kh3A@mail.gmail.com","threadId":"61297","inReplyTo":"xmqqpluz2tau.fsf@gitster.g","subject":"Re: [PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2024-04-11T03:22:02Z","receivedAt":"2024-04-11T03:22:12Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Apr 8, 2024 at 6:55 PM Junio C Hamano wrote:\n>\n> Jacob Keller writes:\n>\n> > I originally wanted to avoid the need for an option, but git-am\n> > currently depends on the strict sequence number filenames. It is\n> > unclear how difficult it would be to refactor git-am to work with\n> > names that include the extra subject data.\n>\n> I am not sure if I follow. Do you mean\n>\n> $ git am ./dir/0*.txt\n>\n\nNo, I mean git am invokes git mailsplit to split a mailbox file into a\ntemporary directory, and then expects to find exactly \"0000\", \"0001\",\n\"0002\" etc, but not \"0001-fix-bug\" and \"0002-implement-feature\"\n"},{"id":"492752","messageId":"xmqq4jc8sbzg.fsf@gitster.g","threadId":"61297","inReplyTo":"CA+P7+xooa08Y-D8CXDGK7_aZ5c2b9iXM6+rFS5qNLyZaG0Kh3A@mail.gmail.com","subject":"Re: [PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-11T05:29:55Z","receivedAt":"2024-04-11T05:29:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> No, I mean git am invokes git mailsplit to split a mailbox file into a\n> temporary directory, and then expects to find exactly \"0000\", \"0001\",\n> \"0002\" etc, but not \"0001-fix-bug\" and \"0002-implement-feature\"\n\nAh, of course. \"am\" invokes mailsplit with the understanding that\nits external interface is that it will get the total number as\ndecimal number from its standard output, and the files are named as\njust numbers in the specified directory, with specified precision.\nIf you are mucking with mailsplit to update its output, of course\nyou must update the expected way \"am\" receives its input.\n\n\n\n"},{"id":"492754","messageId":"xmqqedbcqw84.fsf@gitster.g","threadId":"61297","inReplyTo":"CA+P7+xooa08Y-D8CXDGK7_aZ5c2b9iXM6+rFS5qNLyZaG0Kh3A@mail.gmail.com","subject":"Re: [PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-11T05:55:39Z","receivedAt":"2024-04-11T05:55:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n>> > I originally wanted to avoid the need for an option, but git-am\n>> > currently depends on the strict sequence number filenames. It is\n>> > unclear how difficult it would be to refactor git-am to work with\n>> > names that include the extra subject data.\n\nThe change may be a bit involved but depending on where you decide\nto stop, it may not be too bad.\n\nWhat is your design goal of this topic? IOW, what is the maximum\ncorrupted ordering of patches in a single mailbox do you want to\nrecover from?\n\nThe easiest and cleanest would be if you assume that the messages\nare in scrambled order, but are all from the same series, correctly\nnumbered, without anything missing. A mbox may have 8 patches from\na 8-patch series, with their subject lines having [1/8] to [8/8]\nwithout duplicates or droppages, without any other message that does\nnot belong to the series. If that is where you are willing to stop,\nthen you can still name the individual messages with just numbers\n(but taken out of the subject line, not the order the input was\nsplitted into). \"am\" does not have to even know or care what you\nare doing in mailsplit in this case.\n\nTHe next level would be to still assume that you stop at the same\nplace (i.e. you do not support patches from multiple series in the\nsame mailbox), but use the number-santized-subject format. This\nwould be a bit more involved, but I think all you need to update on\nthe \"am\" side is where the am_run() assigns the message file to the\nlocal variable \"mail\". You know the temporary directory where you\ntold \"mailsplit\" to create these individual messages, so you should\nbe able to \"opendir/readdir/closedir\" and create a list of numbered\nfiles in the directory very early in \"git am\". Knowing msgnum(state)\nat that point in the loop, it should be trivial to change the code\nthat currently assumes the 4-th file is named \"0004\" to check for\nthe file whose name begins with \"0004-\".\n\nI personally am not at all interested in doing that myself, because\nI do not see a reasonable way to lift the limitation of allowing a\nmailbox holding patches from only one series, and if we assume that\na tool (i.e. \"am\" driving \"mailsplit\" in the new mode) with such a\nlimitation is still useful, the source of such a scrambled mailbox\nmust be quite a narrow and common one. At that point, I suspect\nthat fixing the scrambling at that narrow and common source (e.g.\nyour \"t.mbox.gz from public inbox server that cannot be told to sort\nthe messages in any order other than the arrival timestamp\") would\nbe a much better use of our engineering resource.\n\n"},{"id":"492795","messageId":"5a25d75c-cc27-49d9-a49d-39f657fd17f4@intel.com","threadId":"61297","inReplyTo":"xmqqedbcqw84.fsf@gitster.g","subject":"Re: [PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-04-11T18:52:23Z","receivedAt":"2024-04-11T18:52:35Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 4/10/2024 10:55 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n> \n>>>> I originally wanted to avoid the need for an option, but git-am\n>>>> currently depends on the strict sequence number filenames. It is\n>>>> unclear how difficult it would be to refactor git-am to work with\n>>>> names that include the extra subject data.\n> \n> The change may be a bit involved but depending on where you decide\n> to stop, it may not be too bad.\n> \n> What is your design goal of this topic? IOW, what is the maximum\n> corrupted ordering of patches in a single mailbox do you want to\n> recover from?\n> \n> The easiest and cleanest would be if you assume that the messages\n> are in scrambled order, but are all from the same series, correctly\n> numbered, without anything missing. A mbox may have 8 patches from\n> a 8-patch series, with their subject lines having [1/8] to [8/8]\n> without duplicates or droppages, without any other message that does\n> not belong to the series. If that is where you are willing to stop,\n> then you can still name the individual messages with just numbers\n> (but taken out of the subject line, not the order the input was\n> splitted into). \"am\" does not have to even know or care what you\n> are doing in mailsplit in this case.\n\nThis is the main problem I'd like to solve. My original proposal was to\ntry and do just this, but the logic for extracting the number was bad.\nMaybe just directly using the subject-based name and sorting by that\nusing standard alpha-numeric sort would be sufficient?\n\n> \n> THe next level would be to still assume that you stop at the same\n> place (i.e. you do not support patches from multiple series in the\n> same mailbox), but use the number-santized-subject format. This\n> would be a bit more involved, but I think all you need to update on\n> the \"am\" side is where the am_run() assigns the message file to the\n> local variable \"mail\". You know the temporary directory where you\n> told \"mailsplit\" to create these individual messages, so you should\n> be able to \"opendir/readdir/closedir\" and create a list of numbered\n> files in the directory very early in \"git am\". Knowing msgnum(state)\n> at that point in the loop, it should be trivial to change the code\n> that currently assumes the 4-th file is named \"0004\" to check for\n> the file whose name begins with \"0004-\".\n\nYea, we pretty much just have to get the git-am process to work with the\nnew names. I can look at using opendir/readdir here instead.\n> \n> I personally am not at all interested in doing that myself, because\n> I do not see a reasonable way to lift the limitation of allowing a\n> mailbox holding patches from only one series, and if we assume that\n> a tool (i.e. \"am\" driving \"mailsplit\" in the new mode) with such a\n> limitation is still useful, the source of such a scrambled mailbox\n> must be quite a narrow and common one. At that point, I suspect\n> that fixing the scrambling at that narrow and common source (e.g.\n> your \"t.mbox.gz from public inbox server that cannot be told to sort\n> the messages in any order other than the arrival timestamp\") would\n> be a much better use of our engineering resource.\n> \n\nYa I don't care much about multiple series. I care more about making it\nhandle scrambled series better than it does now. I download series off\nof lore.kernel.org (public-inbox based) and those seem to routinely have\nseries out-of-order. I suspect this is because it bases them on arrival\ndate and sometimes certain mailers get it out of order when sending.\n"},{"id":"492806","messageId":"xmqqfrvrr3q7.fsf@gitster.g","threadId":"61297","inReplyTo":"5a25d75c-cc27-49d9-a49d-39f657fd17f4@intel.com","subject":"Re: [PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-11T21:25:52Z","receivedAt":"2024-04-11T21:25:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n>> THe next level would be to still assume that you stop at the same\n>> place (i.e. you do not support patches from multiple series in the\n>> same mailbox), but use the number-santized-subject format. This\n>> would be a bit more involved, but I think all you need to update on\n>> the \"am\" side is where the am_run() assigns the message file to the\n>> local variable \"mail\". You know the temporary directory where you\n>> told \"mailsplit\" to create these individual messages, so you should\n>> be able to \"opendir/readdir/closedir\" and create a list of numbered\n>> files in the directory very early in \"git am\". Knowing msgnum(state)\n>> at that point in the loop, it should be trivial to change the code\n>> that currently assumes the 4-th file is named \"0004\" to check for\n>> the file whose name begins with \"0004-\".\n>\n> Yea, we pretty much just have to get the git-am process to work with the\n> new names. I can look at using opendir/readdir here instead.\n\nNot \"here\", but probably just after you called \"mailsplit\" and saw\nit return. After that nobody should be adding more split mail\nmessages to the directory, so you do it once to grab all filenames.\n\n> Ya I don't care much about multiple series. I care more about making it\n> handle scrambled series better than it does now. I download series off\n> of lore.kernel.org (public-inbox based) and those seem to routinely have\n> series out-of-order. I suspect this is because it bases them on arrival\n> date and sometimes certain mailers get it out of order when sending.\n\nYeah, and that is why I said it would be a better use of the\nengineering resource to fix it at the source. Such a fix will\nbenefit folks with existing versions of \"git am\", not needing to\nwait for your improved version.\n\nThanks.\n"},{"id":"492898","messageId":"CA+P7+xqkTHrBy0adVC3Wmn6aqgGkdZyk7BdHPKsowBCyKWg11w@mail.gmail.com","threadId":"61297","inReplyTo":"xmqqfrvrr3q7.fsf@gitster.g","subject":"Re: [PATCH] mailsplit add option to include sanitized subject in filename","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2024-04-13T00:07:23Z","receivedAt":"2024-04-13T00:07:35Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Apr 11, 2024 at 2:25 PM Junio C Hamano wrote:\n>\n> Jacob Keller writes:\n>\n> >> THe next level would be to still assume that you stop at the same\n> >> place (i.e. you do not support patches from multiple series in the\n> >> same mailbox), but use the number-santized-subject format. This\n> >> would be a bit more involved, but I think all you need to update on\n> >> the \"am\" side is where the am_run() assigns the message file to the\n> >> local variable \"mail\". You know the temporary directory where you\n> >> told \"mailsplit\" to create these individual messages, so you should\n> >> be able to \"opendir/readdir/closedir\" and create a list of numbered\n> >> files in the directory very early in \"git am\". Knowing msgnum(state)\n> >> at that point in the loop, it should be trivial to change the code\n> >> that currently assumes the 4-th file is named \"0004\" to check for\n> >> the file whose name begins with \"0004-\".\n> >\n> > Yea, we pretty much just have to get the git-am process to work with the\n> > new names. I can look at using opendir/readdir here instead.\n>\n> Not \"here\", but probably just after you called \"mailsplit\" and saw\n> it return. After that nobody should be adding more split mail\n> messages to the directory, so you do it once to grab all filenames.\n>\n> > Ya I don't care much about multiple series. I care more about making it\n> > handle scrambled series better than it does now. I download series off\n> > of lore.kernel.org (public-inbox based) and those seem to routinely have\n> > series out-of-order. I suspect this is because it bases them on arrival\n> > date and sometimes certain mailers get it out of order when sending.\n>\n> Yeah, and that is why I said it would be a better use of the\n> engineering resource to fix it at the source. Such a fix will\n> benefit folks with existing versions of \"git am\", not needing to\n> wait for your improved version.\n>\n> Thanks.\n\nI went and talked to the public-inbox folks, and discovered that there\nis a known problem and solution, with a utility called b4 intended for\ndownloading mbox files from the public-inbox\n\nhttps://b4.docs.kernel.org/\n\nThought I'd mention that here if anyone else reading this thread was\ncurious about an ultimate solution.\n\nb4 will find patches in the series, sort them, remove the replies and\ncan do some other common cleanup operations including things like\napplying tags from other messages on the list.\n"}]}