{"thread":{"id":"61106","subject":"[RFC PATCH] mailsplit: add option to extract PATCH M/N numbers","startedAt":"2024-03-13T01:00:46Z","lastAt":"2024-03-14T00:03:26Z","messageCount":3,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"490522","messageId":"20240313010040.1828970-1-jacob.e.keller@intel.com","threadId":"61106","inReplyTo":null,"subject":"[RFC PATCH] mailsplit: add option to extract PATCH M/N numbers","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-03-13T00:58:48Z","receivedAt":"2024-03-13T01:00:46Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\ngit mailsplit splits a mailbox into files, which are then fed into git\nmailinfo to extract patch and subject data. This data is used by git am\nto apply patches from an email file, whether its a maildir or an mbox\nfile.\n\nIn some cases, the order of emails within an mbox does not reflect the\nactual patch order. I often find this the case when I download a\nt.mbox.gz file from a public inbox archive.\n\nWhen this occurs, git am will just try applying patches in whatever\norder they appear in the mbox, without any attempt to sort or order\nbased on the actual patch numbers.\n\nAs an initial attempt at enabling such functionality, implement\n--extract-patch-from-subject into git mailsplit.\n\nThis will cause the mailsplit command to store files in\n<total>_<nr>_<count> files instead of just using the mail count as\ncurrently.\n\nThis ordering should be generally useful as it sorts nicely when sorting\nthe files alphanumerically.\n\nThe extraction implementation is pretty rough, as I wasn't sure if we\nhad more robust string matching. I tried to use strchr and some\nassumptions about how patch formatting is done. It looks for a block\nwith \"[ .. PATCH .. (sp)M/N]\" and tries to copy the M and N out of the\nstring and then extract the numbers using atoi. If any of this fails, it\nfalls back to M=0 and N=0 when formatting the file names.\n\nThe end result is that files split from the mailbox or maildir have\nprefixed 000N_000M_000C rather than just using the count of mail files\nwithin the mailbox.\n\nTo allow this to work nicely, I used fgetpos and fsetpos to move through\nthe file stream while searching for the subject line. This doesn't work\nwith stdin so this support only works if you are processing a real file\nand not processing from stdin.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n\nI'm sending this as an RFC because its pretty ugly and I didn't completely\nfinish the work to combine this with git-am. The goal is to have all the\nmails extracted from mailsplit nicely sort along subject lines so that git\nam will apply a series in the order that the subject lines specify rather\nthan assuming the mails were received and stored in the correct order.\n\nIts frustrating to download an mbox file and then have to manually re-sort\nor re-apply patches because it failed to extract things nicely. I don't know\nif this approach is the best solution, or whether there's something else we\ncould do instead. In theory we probably want something more robust for the\nM/N extraction over using strchr, memrchr, and whatnot....\n\n builtin/mailsplit.c | 126 ++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 116 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 3af9ddb8ae5c..5255f4056e91 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -10,7 +10,7 @@\n #include \"strbuf.h\"\n \n static const char git_mailsplit_usage[] =\n-\"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]\";\n+\"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] [--extract-patch-from-subject] -o<directory> [(<mbox>|<Maildir>)...]\";\n \n static int is_from_line(const char *line, int len)\n {\n@@ -166,8 +166,83 @@ static int maildir_filename_cmp(const char *a, const char *b)\n \treturn (unsigned char)*a - (unsigned char)*b;\n }\n \n+static int extract_patch_subject(FILE *mbox, int *nr, int *total)\n+{\n+\tstruct strbuf tmp, n_buf, m_buf;\n+\tint found = 0;\n+\tfpos_t pos;\n+\tint err;\n+\n+\terr = fgetpos(mbox, &pos);\n+\tif (err) {\n+\t\terror_errno(\"cannot get file position for %s\", mbox);\n+\t\treturn err;\n+\t}\n+\n+\tstrbuf_init(&tmp, 100);\n+\tstrbuf_init(&n_buf, 10);\n+\tstrbuf_init(&m_buf, 10);\n+\n+\tfor (;;) {\n+\t\tchar *start, *patch, *last_space, *slash, *end;\n+\n+\t\tif (strbuf_getline(&tmp, mbox))\n+\t\t\tbreak;\n+\n+\t\tif (!starts_with(tmp.buf, \"Subject:\"))\n+\t\t\tcontinue;\n+\n+\t\tstart = strchr(tmp.buf, '[');\n+\n+\t\twhile (start) {\n+\t\t\tpatch = strstr(start, \"PATCH\");\n+\t\t\tslash = strchr(start, '/');\n+\t\t\tend = strchr(start, ']');\n+\n+\t\t\tif (patch && slash && end &&\n+\t\t\t    patch < end && slash < end) {\n+\n+\t\t\t\tlast_space = memrchr(patch, ' ', end - patch);\n+\t\t\t\tif (!last_space || last_space >= end)\n+\t\t\t\t\tbreak;\n+\n+\t\t\t\tfound = 1;\n+\n+\t\t\t\tstrbuf_reset(&n_buf);\n+\t\t\t\tstrbuf_reset(&m_buf);\n+\n+\t\t\t\tstrbuf_add(&m_buf, last_space + 1, slash - last_space - 1);\n+\t\t\t\tstrbuf_add(&n_buf, slash + 1, end - slash - 1);\n+\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\n+\t\tbreak;\n+\t}\n+\n+\tif (found) {\n+\t\t*nr = atoi(m_buf.buf);\n+\t\t*total = atoi(n_buf.buf);\n+\t} else {\n+\t\t*nr = 0;\n+\t\t*total = 0;\n+\t}\n+\n+\tstrbuf_release(&tmp);\n+\tstrbuf_release(&n_buf);\n+\tstrbuf_release(&m_buf);\n+\n+\terr = fsetpos(mbox, &pos);\n+\tif (err)\n+\t\terror_errno(\"cannot set file position for %s\", mbox);\n+\t\treturn err;\n+\n+\treturn 0;\n+}\n+\n static int split_maildir(const char *maildir, const char *dir,\n-\tint nr_prec, int skip)\n+\tint extract_patch, int nr_prec, int skip)\n {\n \tchar *file = NULL;\n \tFILE *f = NULL;\n@@ -197,7 +272,17 @@ static int split_maildir(const char *maildir, const char *dir,\n \t\t\tgoto out;\n \t\t}\n \n-\t\tname = xstrfmt(\"%s/%0*d\", dir, nr_prec, ++skip);\n+\t\tif (extract_patch) {\n+\t\t\tint nr, total;\n+\n+\t\t\tif (extract_patch_subject(f, &nr, &total))\n+\t\t\t\tgoto out;\n+\n+\t\t\tname = xstrfmt(\"%s/%0*d_%0*d_%0*d\", dir, nr_prec, total, nr_prec, nr, nr_prec, ++skip);\n+\t\t} else {\n+\t\t\tname = xstrfmt(\"%s/%0*d\", dir, nr_prec, ++skip);\n+\t\t}\n+\n \t\tsplit_one(f, name, 1);\n \t\tfree(name);\n \n@@ -214,7 +299,8 @@ static int split_maildir(const char *maildir, const char *dir,\n \treturn ret;\n }\n \n-static int split_mbox(const char *file, const char *dir, int allow_bare,\n+static int split_mbox(const char *file, const char *dir,\n+\t\t      int extract_patch, int allow_bare,\n \t\t      int nr_prec, int skip)\n {\n \tint ret = -1;\n@@ -223,8 +309,13 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,\n \tFILE *f = !strcmp(file, \"-\") ? stdin : fopen(file, \"r\");\n \tint file_done = 0;\n \n-\tif (isatty(fileno(f)))\n+\tif (isatty(fileno(f))) {\n+\t\tif (extract_patch) {\n+\t\t\terror(\"cannot use --extract-patch-from-subject with stdin\");\n+\t\t\treturn -1;\n+\t\t}\n \t\twarning(_(\"reading patches from stdin/tty...\"));\n+\t}\n \n \tif (!f) {\n \t\terror_errno(\"cannot open mbox %s\", file);\n@@ -256,7 +347,20 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,\n \t}\n \n \twhile (!file_done) {\n-\t\tchar *name = xstrfmt(\"%s/%0*d\", dir, nr_prec, ++skip);\n+\t\tchar *name;\n+\n+\t\tif (extract_patch) {\n+\t\t\tint nr, total;\n+\n+\t\t\tif (extract_patch_subject(f, &nr, &total)) {\n+\t\t\t\tnr = 0;\n+\t\t\t\ttotal = 0;\n+\t\t\t}\n+\n+\t\t\tname = xstrfmt(\"%s/%0*d_%0*d_%0*d\", dir, nr_prec, total, nr_prec, nr, nr_prec, ++skip);\n+\t\t} else {\n+\t\t\tname = xstrfmt(\"%s/%0*d\", dir, nr_prec, ++skip);\n+\t\t}\n \t\tfile_done = split_one(f, name, allow_bare);\n \t\tfree(name);\n \t}\n@@ -272,7 +376,7 @@ static int split_mbox(const char *file, const char *dir, int allow_bare,\n int cmd_mailsplit(int argc, const char **argv, const char *prefix)\n {\n \tint nr = 0, nr_prec = 4, num = 0;\n-\tint allow_bare = 0;\n+\tint allow_bare = 0, extract_patch = 0;\n \tconst char *dir = NULL;\n \tconst char **argp;\n \tstatic const char *stdin_only[] = { \"-\", NULL };\n@@ -302,6 +406,8 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)\n \t\t\tdir = arg+2;\n \t\t} else if (!strcmp(arg, \"--mboxrd\")) {\n \t\t\tmboxrd = 1;\n+\t\t} else if (!strcmp(arg, \"--extract-patch-from-subject\")) {\n+\t\t\textract_patch = 1;\n \t\t} else if ( arg[1] == '-' && !arg[2] ) {\n \t\t\targp++;\t/* -- marks end of options */\n \t\t\tbreak;\n@@ -338,7 +444,7 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)\n \t\tint ret = 0;\n \n \t\tif (arg[0] == '-' && arg[1] == 0) {\n-\t\t\tret = split_mbox(arg, dir, allow_bare, nr_prec, nr);\n+\t\t\tret = split_mbox(arg, dir, extract_patch, allow_bare, nr_prec, nr);\n \t\t\tif (ret < 0) {\n \t\t\t\terror(\"cannot split patches from stdin\");\n \t\t\t\treturn 1;\n@@ -354,9 +460,9 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tif (S_ISDIR(argstat.st_mode))\n-\t\t\tret = split_maildir(arg, dir, nr_prec, nr);\n+\t\t\tret = split_maildir(arg, dir, extract_patch, nr_prec, nr);\n \t\telse\n-\t\t\tret = split_mbox(arg, dir, allow_bare, nr_prec, nr);\n+\t\t\tret = split_mbox(arg, dir, extract_patch, allow_bare, nr_prec, nr);\n \n \t\tif (ret < 0) {\n \t\t\terror(\"cannot split patches from %s\", arg);\n-- \n2.44.0.53.g0f9d4d28b7e6\n\n"},{"id":"490577","messageId":"xmqqzfv1ao3y.fsf@gitster.g","threadId":"61106","inReplyTo":"20240313010040.1828970-1-jacob.e.keller@intel.com","subject":"Re: [RFC PATCH] mailsplit: add option to extract PATCH M/N numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-13T20:11:29Z","receivedAt":"2024-03-13T20:11:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> Its frustrating to download an mbox file and then have to manually re-sort\n> or re-apply patches because it failed to extract things nicely. I don't know\n> if this approach is the best solution, or whether there's something else we\n> could do instead. In theory we probably want something more robust for the\n> M/N extraction over using strchr, memrchr, and whatnot....\n>\n>  builtin/mailsplit.c | 126 ++++++++++++++++++++++++++++++++++++++++----\n>  1 file changed, 116 insertions(+), 10 deletions(-)\n>\n> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\n> index 3af9ddb8ae5c..5255f4056e91 100644\n> --- a/builtin/mailsplit.c\n> +++ b/builtin/mailsplit.c\n> @@ -10,7 +10,7 @@\n>  #include \"strbuf.h\"\n>  \n>  static const char git_mailsplit_usage[] =\n> -\"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]\";\n> +\"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] [--extract-patch-from-subject] -o<directory> [(<mbox>|<Maildir>)...]\";\n\n\"extract patch from subject\" is quite a mouthful and more\nimportantly, I suspect that it is different from what the option\ndoes (unless the sender is cramming the log message and the patch on\na single title line).\n\nThis seems to try parsing the numbers out of the prefixes like\n\"[PATCH v2 04/18]\" but I wonder if it has to understand four and\neighteen in such a header.  Doesn't the sending side make sure that\nthe patches will be in the right order if you sort the message in\nyour MUA by the subject textually?  After all, such a numbering\nconvention is to aid humans to read them in order in a mailbox.\n\nI also wonder if it is enough to run the commit title (before\nstripping out the \"[PATCH v4 04/18]\" part) through\nformat_sanitized_subject() to derive the patch file name.  At the\nreceiving end, they have to be prepared to take a non-number with\nyour patch anyway, and when things go wrong, peeking the directory\nmay become easier.  I dunno.\n\n> +\tif (found) {\n> +\t\t*nr = atoi(m_buf.buf);\n> +\t\t*total = atoi(n_buf.buf);\n> +\t} else {\n> +\t\t*nr = 0;\n> +\t\t*total = 0;\n> +\t}\n\nHaving said all that, I also wonder if we want to simplify this to\nthe other extreme.  When we accept such a 18-patch series that is\nstored in a mailbox file in a wrong order, we do not even care about\n\"/18\" part of \"[PATCH v2 04/18]\".  So instead of worrying about\ntotal at all, how about \"--use-patchnum-from-subject\" that only uses\nthe \"04\" part of \"[PATCH v2 04/18]\" as the output filename?  We\nwould probably still want to count how many messges we processed and\nas long as we need to parse out \"04\" out of \"04/18\", we'd probably\nparse \"18\" as well, so we can maintain a bitmap to ensure we saw all\nthe messages without duplicates, or something.\n\nThe reason why I care is because such a scheme would be easier for\nexisting consumers that are prepared to take ...\n\n> -\t\tchar *name = xstrfmt(\"%s/%0*d\", dir, nr_prec, ++skip);\n\n... only numbers in their filenames, and not ...\n\n> ...\n> +\t\t\tname = xstrfmt(\"%s/%0*d_%0*d_%0*d\", dir, nr_prec, total, nr_prec, nr, nr_prec, ++skip);\n\n... a string like \"04_18\".\n\nOverall, I like the idea of massaging a mailbox that got patches in\nshuffled order to make it usable.  In addition to the \"sort the\nshuffled mess\" benefit, it would allow us to drop the cover letter\nand pick only the patch messages.  I just am not happy with the\noutput \"04_18\", and the parsing code, as you said, is yucky indeed.\n\nAnd of course totally outside the Git tools scope, I wonder if this\nis something \"formail\" should be able to do more naturally.\n"},{"id":"490596","messageId":"1cfea792-3a7f-4f2a-b460-223a237de9a3@intel.com","threadId":"61106","inReplyTo":"xmqqzfv1ao3y.fsf@gitster.g","subject":"Re: [RFC PATCH] mailsplit: add option to extract PATCH M/N numbers","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2024-03-14T00:03:14Z","receivedAt":"2024-03-14T00:03:26Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"\n\nOn 3/13/2024 1:11 PM, Junio C Hamano wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n> \n>> Its frustrating to download an mbox file and then have to manually re-sort\n>> or re-apply patches because it failed to extract things nicely. I don't know\n>> if this approach is the best solution, or whether there's something else we\n>> could do instead. In theory we probably want something more robust for the\n>> M/N extraction over using strchr, memrchr, and whatnot....\n>>\n>>  builtin/mailsplit.c | 126 ++++++++++++++++++++++++++++++++++++++++----\n>>  1 file changed, 116 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\n>> index 3af9ddb8ae5c..5255f4056e91 100644\n>> --- a/builtin/mailsplit.c\n>> +++ b/builtin/mailsplit.c\n>> @@ -10,7 +10,7 @@\n>>  #include \"strbuf.h\"\n>>  \n>>  static const char git_mailsplit_usage[] =\n>> -\"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]\";\n>> +\"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] [--extract-patch-from-subject] -o<directory> [(<mbox>|<Maildir>)...]\";\n> \n> \"extract patch from subject\" is quite a mouthful and more\n> importantly, I suspect that it is different from what the option\n> does (unless the sender is cramming the log message and the patch on\n> a single title line).\n> \n\nYea, I didn't really have a good suggestion for a name.\n\n> This seems to try parsing the numbers out of the prefixes like\n> \"[PATCH v2 04/18]\" but I wonder if it has to understand four and\n> eighteen in such a header.  Doesn't the sending side make sure that\n> the patches will be in the right order if you sort the message in\n> your MUA by the subject textually?  After all, such a numbering\n> convention is to aid humans to read them in order in a mailbox.\n> \n> I also wonder if it is enough to run the commit title (before\n> stripping out the \"[PATCH v4 04/18]\" part) through\n> format_sanitized_subject() to derive the patch file name.  At the\n> receiving end, they have to be prepared to take a non-number with\n> your patch anyway, and when things go wrong, peeking the directory\n> may become easier.  I dunno.\n>\n\nI think this would be useful and a more desirable goal than my yucky\nextraction of the patch number.\n\n\n>> +\tif (found) {\n>> +\t\t*nr = atoi(m_buf.buf);\n>> +\t\t*total = atoi(n_buf.buf);\n>> +\t} else {\n>> +\t\t*nr = 0;\n>> +\t\t*total = 0;\n>> +\t}\n> \n> Having said all that, I also wonder if we want to simplify this to\n> the other extreme.  When we accept such a 18-patch series that is\n> stored in a mailbox file in a wrong order, we do not even care about\n> \"/18\" part of \"[PATCH v2 04/18]\".  So instead of worrying about\n> total at all, how about \"--use-patchnum-from-subject\" that only uses\n> the \"04\" part of \"[PATCH v2 04/18]\" as the output filename?  We\n> would probably still want to count how many messges we processed and\n> as long as we need to parse out \"04\" out of \"04/18\", we'd probably\n> parse \"18\" as well, so we can maintain a bitmap to ensure we saw all\n> the messages without duplicates, or something.\n\n\nMy main issue was \"how to actually parse that\". But maybe sanitizing the\nsubject is good enough.\n\n> \n> The reason why I care is because such a scheme would be easier for\n> existing consumers that are prepared to take ...\n> \n>> -\t\tchar *name = xstrfmt(\"%s/%0*d\", dir, nr_prec, ++skip);\n> \n> ... only numbers in their filenames, and not ...\n> \n>> ...\n>> +\t\t\tname = xstrfmt(\"%s/%0*d_%0*d_%0*d\", dir, nr_prec, total, nr_prec, nr, nr_prec, ++skip);\n> \n> ... a string like \"04_18\".\n> \n> Overall, I like the idea of massaging a mailbox that got patches in\n> shuffled order to make it usable.  In addition to the \"sort the\n> shuffled mess\" benefit, it would allow us to drop the cover letter\n> and pick only the patch messages.  I just am not happy with the\n> output \"04_18\", and the parsing code, as you said, is yucky indeed.\n\nI agree, I didn't really like the output but was having problems\nfiguring out how to handle that.\n\n> \n> And of course totally outside the Git tools scope, I wonder if this\n> is something \"formail\" should be able to do more naturally.\n> \n\nWell, in my case, I'm downloading the t.mbox.gz from a lore.kernel.org\nserver, and it just happens to get patches in seemingly random order.\nThe lore website itself manages this fine, but for some reason the\nthread mbox files are not in the patch order. I don't have (direct)\ncontrol over those, and its a hassle especially as the mbox has not just\nthe patches from the original series, but also the entire set of\nreplies. It would be convenient to also strip out and skip those as well.\n\nWhile I do get that this is somewhat outside the scope of git tooling,\nits a relatively common problem at least for me. I could probably use a\ndifferent tool to parse and order the mailbox first... Hm.\n\nFWIW, i couldn't figure out how to get formail to do what I want, but it\nlooks like I can write a little python program that can sort messages in\na mailbox using the mailbox module. That would be sufficient to resolve\nmy needs.\n\nLet me look at the way the format_sanitize_subject thing works out. That\nmight still be worthwhile for when you want to peek into the folder with\ngit mailsplit.\n"}]}