On 3/13/2024 1:11 PM, Junio C Hamano wrote:
> Jacob Keller <jacob.e.keller@xxxxxxxxx> writes:
>
>> Its frustrating to download an mbox file and then have to manually re-sort
>> or re-apply patches because it failed to extract things nicely. I don't know
>> if this approach is the best solution, or whether there's something else we
>> could do instead. In theory we probably want something more robust for the
>> M/N extraction over using strchr, memrchr, and whatnot....
>>
>> builtin/mailsplit.c | 126 ++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 116 insertions(+), 10 deletions(-)
>>
>> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
>> index 3af9ddb8ae5c..5255f4056e91 100644
>> --- a/builtin/mailsplit.c
>> +++ b/builtin/mailsplit.c
>> @@ -10,7 +10,7 @@
>> #include "strbuf.h"
>>
>> static const char git_mailsplit_usage[] =
>> -"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] -o<directory> [(<mbox>|<Maildir>)...]";
>> +"git mailsplit [-d<prec>] [-f<n>] [-b] [--keep-cr] [--extract-patch-from-subject] -o<directory> [(<mbox>|<Maildir>)...]";
>
> "extract patch from subject" is quite a mouthful and more
> importantly, I suspect that it is different from what the option
> does (unless the sender is cramming the log message and the patch on
> a single title line).
>
Yea, I didn't really have a good suggestion for a name.
> This seems to try parsing the numbers out of the prefixes like
> "[PATCH v2 04/18]" but I wonder if it has to understand four and
> eighteen in such a header. Doesn't the sending side make sure that
> the patches will be in the right order if you sort the message in
> your MUA by the subject textually? After all, such a numbering
> convention is to aid humans to read them in order in a mailbox.
>
> I also wonder if it is enough to run the commit title (before
> stripping out the "[PATCH v4 04/18]" part) through
> format_sanitized_subject() to derive the patch file name. At the
> receiving end, they have to be prepared to take a non-number with
> your patch anyway, and when things go wrong, peeking the directory
> may become easier. I dunno.
>
I think this would be useful and a more desirable goal than my yucky
extraction of the patch number.
>> + if (found) {
>> + *nr = atoi(m_buf.buf);
>> + *total = atoi(n_buf.buf);
>> + } else {
>> + *nr = 0;
>> + *total = 0;
>> + }
>
> Having said all that, I also wonder if we want to simplify this to
> the other extreme. When we accept such a 18-patch series that is
> stored in a mailbox file in a wrong order, we do not even care about
> "/18" part of "[PATCH v2 04/18]". So instead of worrying about
> total at all, how about "--use-patchnum-from-subject" that only uses
> the "04" part of "[PATCH v2 04/18]" as the output filename? We
> would probably still want to count how many messges we processed and
> as long as we need to parse out "04" out of "04/18", we'd probably
> parse "18" as well, so we can maintain a bitmap to ensure we saw all
> the messages without duplicates, or something.
My main issue was "how to actually parse that". But maybe sanitizing the
subject is good enough.
>
> The reason why I care is because such a scheme would be easier for
> existing consumers that are prepared to take ...
>
>> - char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
>
> ... only numbers in their filenames, and not ...
>
>> ...
>> + name = xstrfmt("%s/%0*d_%0*d_%0*d", dir, nr_prec, total, nr_prec, nr, nr_prec, ++skip);
>
> ... a string like "04_18".
>
> Overall, I like the idea of massaging a mailbox that got patches in
> shuffled order to make it usable. In addition to the "sort the
> shuffled mess" benefit, it would allow us to drop the cover letter
> and pick only the patch messages. I just am not happy with the
> output "04_18", and the parsing code, as you said, is yucky indeed.
I agree, I didn't really like the output but was having problems
figuring out how to handle that.
>
> And of course totally outside the Git tools scope, I wonder if this
> is something "formail" should be able to do more naturally.
>
Well, in my case, I'm downloading the t.mbox.gz from a lore.kernel.org
server, and it just happens to get patches in seemingly random order.
The lore website itself manages this fine, but for some reason the
thread mbox files are not in the patch order. I don't have (direct)
control over those, and its a hassle especially as the mbox has not just
the patches from the original series, but also the entire set of
replies. It would be convenient to also strip out and skip those as well.
While I do get that this is somewhat outside the scope of git tooling,
its a relatively common problem at least for me. I could probably use a
different tool to parse and order the mailbox first... Hm.
FWIW, i couldn't figure out how to get formail to do what I want, but it
looks like I can write a little python program that can sort messages in
a mailbox using the mailbox module. That would be sufficient to resolve
my needs.
Let me look at the way the format_sanitize_subject thing works out. That
might still be worthwhile for when you want to peek into the folder with
git mailsplit.
