{"thread":{"id":"33051","subject":"[bug report] git-am applying maildir patches in reverse","startedAt":"2013-03-01T22:20:18Z","lastAt":"2013-03-03T06:32:44Z","messageCount":14,"participants":["William Giokas","Junio C Hamano","Jeff King","Andreas Schwab"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"210481","messageId":"20130301222018.GA839@WST420","threadId":"33051","inReplyTo":null,"subject":"[bug report] git-am applying maildir patches in reverse","fromName":"William Giokas","fromEmail":"1007380@gmail.com","sentAt":"2013-03-01T22:20:18Z","receivedAt":"2013-03-01T22:20:18Z","isPatch":false,"sender":{"key":"1007380@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1731430?v=4"},"body":"All,\n\nI've been using git for a while and this is the first time I've had to\nuse `git am` and I've got a 16 patch patchset that I'm looking to apply.\nThe files were copied to a separate maildir by mutt to keep things\nclean, and then I ran `git am -i /path/to/maildir/` expecting things to\nstart from the patch with the subject \n\n    [PATCH 01/16] refactor common code in query_search/sync_search\n\nBut instead, it starts with the 16/16 patch and works backwards, which,\nobviously, breaks the application process as the patches depend on each\nother. I looked in the maildir directory just to see if the file names\nwere backwards, and that's not the issue. I talked to `gitster` on IRC\nand he said to send in a bug report on this issue here. The patchset I'm\ntrying to apply can be found here[0]. \n\nProcess to reproduce:\n* find a multi-patch set with interdependent patches\n* run `git am` on the maildir containing these patches\n\nExpected result:\n* Apply patches in [01..N] order\n\nActual result:\n* Patches applied in [N N-1..01] order\n\n[0]: https://mailman.archlinux.org/pipermail/pacman-dev/2013-March/016541.html\n\nThank you,\n-- \nWilliam Giokas | KaiSforza\nGnuPG Key: 0x73CD09CF\nFingerprint: F73F 50EF BBE2 9846 8306  E6B8 6902 06D8 73CD 09CF\n"},{"id":"210482","messageId":"7vwqtqeox7.fsf@alter.siamese.dyndns.org","threadId":"33051","inReplyTo":"20130301222018.GA839@WST420","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-01T22:27:32Z","receivedAt":"2013-03-01T22:27:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"William Giokas <1007380@gmail.com> writes:\n\n> All,\n>\n> I've been using git for a while and this is the first time I've had to\n> use `git am` and I've got a 16 patch patchset that I'm looking to apply.\n> The files were copied to a separate maildir by mutt to keep things\n> clean, and then I ran `git am -i /path/to/maildir/` expecting things to\n> start from the patch with the subject \n>\n>     [PATCH 01/16] refactor common code in query_search/sync_search\n>\n> But instead, it starts with the 16/16 patch and works backwards, which,\n> obviously, breaks the application process as the patches depend on each\n> other. I looked in the maildir directory just to see if the file names\n> were backwards, and that's not the issue. I talked to `gitster` on IRC\n> and he said to send in a bug report on this issue here. The patchset I'm\n> trying to apply can be found here[0]. \n>\n> Process to reproduce:\n> * find a multi-patch set with interdependent patches\n> * run `git am` on the maildir containing these patches\n>\n> Expected result:\n> * Apply patches in [01..N] order\n>\n> Actual result:\n> * Patches applied in [N N-1..01] order\n\nNote to bystanders.  This is coming from populate_maildir_list() in\nbuiltin/mailsplit.c; the function claims to know what \"maildir\"\nshould look like, so it should be enforcing the ordering as\nnecessary by sorting the list, _if_ the implicit ordering given by\nstring_list_insert() is insufficient.\n\nIt also is likely that it is a user error to expect that patch\ne-mails are received and stored in the maildir in the order they\nwere sent, or it could be \"mutt\" copying the mails in the order the\nmessages were originally received, or something silly like that.\n\n>\n> [0]: https://mailman.archlinux.org/pipermail/pacman-dev/2013-March/016541.html\n>\n> Thank you,\n"},{"id":"210484","messageId":"20130301225231.GB862@sigill.intra.peff.net","threadId":"33051","inReplyTo":"7vwqtqeox7.fsf@alter.siamese.dyndns.org","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-01T22:52:31Z","receivedAt":"2013-03-01T22:52:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2013 at 02:27:32PM -0800, Junio C Hamano wrote:\n\n> > I've been using git for a while and this is the first time I've had to\n> > use `git am` and I've got a 16 patch patchset that I'm looking to apply.\n> > The files were copied to a separate maildir by mutt to keep things\n> > clean, and then I ran `git am -i /path/to/maildir/` expecting things to\n> > start from the patch with the subject \n> >\n> >     [PATCH 01/16] refactor common code in query_search/sync_search\n> >\n> > But instead, it starts with the 16/16 patch and works backwards, which,\n> > obviously, breaks the application process as the patches depend on each\n>\n> Note to bystanders.  This is coming from populate_maildir_list() in\n> builtin/mailsplit.c; the function claims to know what \"maildir\"\n> should look like, so it should be enforcing the ordering as\n> necessary by sorting the list, _if_ the implicit ordering given by\n> string_list_insert() is insufficient.\n> \n> It also is likely that it is a user error to expect that patch\n> e-mails are received and stored in the maildir in the order they\n> were sent, or it could be \"mutt\" copying the mails in the order the\n> messages were originally received, or something silly like that.\n\nI think it is a property of the maildir format that it does not\ntechnically define the message order. The order of items you get from\nreaddir is filesystem specific and not necessarily defined (and that is\nwhat we use now). On my ext4 system, I do not even get them in backwards\norder; it is jumbled.\n\nWe could sort based on the mtime of the file, but in some cases that\nwon't have sufficient resolution (e.g., with one second resolution,\nthey'll all probably have the same timestamp).\n\nThe maildir spec explicitly says that readers should not make\nassumptions about the content of the filenames. Mutt happens to write\nthem as:\n\n  ${epoch_seconds}.${pid}_${seq}.${host}\n\nso in practice, sorting them kind of works. Except that a series written\nout at once will all have the same timestamp and pid, and because ${seq}\nis not zero-padded, you have to actually parse up to there and do a\nnumeric instead of byte-wise comparison.  So we can add a mutt-specific\nhack, but that's the best we can do. Other maildir writers (including\nfuture versions of mutt) will not necessarily do the same thing.\n\nI think maildir's philosophy is that ordering is not important, and the\ncontents of the messages themselves should define the order in which a\nMUA presents them, anyway. It would make sense to me if we actually\nparsed the '[PATCH n/m]' bit from the subject of each and sorted based\non that. That is blurring the line between git-mailsplit and\ngit-mailinfo, in that mailsplit would have to start parsing the files.\n\nWe could also sort based on the rfc822 \"Date\" header, but I don't think\nthat is a good idea. It represents the author timestamp, so patches\nmoved via \"rebase -i\" will have out-of-sequence dates with respect to\nthe actual history graph.  You can encounter this if you \"format-patch\n--stdout\" a series into a single mbox and load it into a MUA that sorts\nby date (like mutt). The patches appear jumbled until you switch to\n\"unsorted\" or \"sort by subject\".\n\n-Peff\n"},{"id":"210485","messageId":"20130301230508.GC862@sigill.intra.peff.net","threadId":"33051","inReplyTo":"20130301225231.GB862@sigill.intra.peff.net","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-01T23:05:08Z","receivedAt":"2013-03-01T23:05:08Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:\n\n> > Note to bystanders.  This is coming from populate_maildir_list() in\n> > builtin/mailsplit.c; the function claims to know what \"maildir\"\n> > should look like, so it should be enforcing the ordering as\n> > necessary by sorting the list, _if_ the implicit ordering given by\n> > string_list_insert() is insufficient.\n> [...]\n> \n> I think it is a property of the maildir format that it does not\n> technically define the message order. The order of items you get from\n> readdir is filesystem specific and not necessarily defined (and that is\n> what we use now). On my ext4 system, I do not even get them in backwards\n> order; it is jumbled.\n\nHmph, sorry, I mistook string_list_insert for string_list_append. So we\ndo actually sort them by filename, not just random readdir order. But\ndue to this:\n\n> The maildir spec explicitly says that readers should not make\n> assumptions about the content of the filenames. Mutt happens to write\n> them as:\n> \n>   ${epoch_seconds}.${pid}_${seq}.${host}\n> \n> so in practice, sorting them kind of works. Except that a series written\n> out at once will all have the same timestamp and pid, and because ${seq}\n> is not zero-padded, you have to actually parse up to there and do a\n> numeric instead of byte-wise comparison.  So we can add a mutt-specific\n> hack, but that's the best we can do. Other maildir writers (including\n> future versions of mutt) will not necessarily do the same thing.\n\nThat ordering is not necessarily useful.\n\nSo one strategy could be to try harder to sort numeric elements\nnumerically. That is, to treat:\n\n  1234.5678_90.foo\n\nas the list\n\n  {1234, '.', 5678, '_', 90, \"foo\"}\n\nfor the purposes of sorting (even though we do not necessarily know what\neach piece means). That works for mutt and similar schemes (dovecot, for\nexample, does something like \"M${seq}P${pid}\"), but some systems may put\nrandom bytes in the middle (the point of it is to create a unique name).\n\nSo it is somewhat against the maildir spec, but I think in practice it\nwould help.\n\n-Peff\n"},{"id":"210488","messageId":"7vlia6em9x.fsf@alter.siamese.dyndns.org","threadId":"33051","inReplyTo":"20130301230508.GC862@sigill.intra.peff.net","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-01T23:24:42Z","receivedAt":"2013-03-01T23:24:42Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:\n> ...\n>> The maildir spec explicitly says that readers should not make\n>> assumptions about the content of the filenames. Mutt happens to write\n>> them as:\n>> \n>>   ${epoch_seconds}.${pid}_${seq}.${host}\n>> \n>> so in practice, sorting them kind of works. Except that ...\n>> << it does not work >> ...\n> That ordering is not necessarily useful.\n> ...\n> So it is somewhat against the maildir spec, but I think in practice it\n> would help.\n\nI do not think it would help, unless these epoch_seconds are the\nsender timestamps.  And the number of digits in epoch-seconds for\nrecent messages would be the same, so ordering textually would be\nsufficient if ordering by timestamp were.\n"},{"id":"210491","messageId":"20130301233548.GA13422@sigill.intra.peff.net","threadId":"33051","inReplyTo":"7vlia6em9x.fsf@alter.siamese.dyndns.org","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-01T23:35:48Z","receivedAt":"2013-03-01T23:35:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2013 at 03:24:42PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:\n> > ...\n> >> The maildir spec explicitly says that readers should not make\n> >> assumptions about the content of the filenames. Mutt happens to write\n> >> them as:\n> >> \n> >>   ${epoch_seconds}.${pid}_${seq}.${host}\n> >> \n> >> so in practice, sorting them kind of works. Except that ...\n> >> << it does not work >> ...\n> > That ordering is not necessarily useful.\n> > ...\n> > So it is somewhat against the maildir spec, but I think in practice it\n> > would help.\n> \n> I do not think it would help, unless these epoch_seconds are the\n> sender timestamps.  And the number of digits in epoch-seconds for\n> recent messages would be the same, so ordering textually would be\n> sufficient if ordering by timestamp were.\n\nThe epoch_seconds are the time of writing into the maildir. They will\ntypically all be the same, unless your system is very slow, or you are\nwriting a really long patch series. The PID likewise should be the same\nfor a given series. It's the sequence number that is the interesting bit\nto sort numerically (for mutt, anyway; ditto for dovecot).\n\nThe patch below seems to fix the issue for me with mutt. It's possible\nthat it regresses another case (which has numbers, but really wants them\nsorted as byte strings), but I find that unlikely. If you're\nzero-padding your numbers this will still work, and if you're not, then\nyou likely have no meaningful sort order at all.\n\n-- >8 --\nSubject: [PATCH] mailsplit: sort maildir filenames more cleverly\n\nA maildir does not technically record the order in which\nitems were placed into it. That means that when applying a\npatch series from a maildir, we may get the patches in the\nwrong order. We try to work around this by sorting the\nfilenames. Unfortunately, this may or may not work depending\non the naming scheme used by the writer of the maildir.\n\nFor instance, mutt will write:\n\n  ${epoch_seconds}.${pid}_${seq}.${host}\n\nwhere we have:\n\n  - epoch_seconds: timestamp at which entry was written\n  - pid: PID of writing process\n  - seq: a sequence number to ensure uniqueness of filenames\n  - host: hostname\n\nNone of the numbers are zero-padded. Therefore, when we sort\nthe names as byte strings, entries that cross a digit\nboundary (e.g., 10) will sort out of order.  In the case of\ntimestamps, it almost never matters (because we do not cross\na digit boundary in the epoch time very often these days).\nBut for the sequence number, a 10-patch series would be\nordered as 1, 10, 2, 3, etc.\n\nTo fix this, we can use a custom sort comparison function\nwhich traverses each string, comparing chunks of digits\nnumerically, and otherwise doing a byte-for-byte comparison.\nThat would sort:\n\n  123.456_1.bar\n  123.456_2.bar\n  ...\n  123.456_10.bar\n\naccording to the sequence number. Since maildir does not\ndefine a filename format, this is really just a heuristic.\nBut it happens to work for mutt, and there is a reasonable\nchance that it will work for other writers, too (at least as\nwell as a straight sort).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/mailsplit.c | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 2d43278..772c668 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -130,6 +130,26 @@ static int populate_maildir_list(struct string_list *list, const char *path)\n \treturn 0;\n }\n \n+static int maildir_filename_cmp(const char *a, const char *b)\n+{\n+\twhile (1) {\n+\t\tif (isdigit(*a) && isdigit(*b)) {\n+\t\t\tlong int na, nb;\n+\t\t\tna = strtol(a, (char **)&a, 10);\n+\t\t\tnb = strtol(b, (char **)&b, 10);\n+\t\t\tif (na != nb)\n+\t\t\t\treturn na - nb;\n+\t\t\t/* strtol advanced our pointers */\n+\t\t}\n+\t\telse {\n+\t\t\tif (*a != *b)\n+\t\t\t\treturn *a - *b;\n+\t\t\ta++;\n+\t\t\tb++;\n+\t\t}\n+\t}\n+}\n+\n static int split_maildir(const char *maildir, const char *dir,\n \tint nr_prec, int skip)\n {\n@@ -139,6 +159,8 @@ static int split_maildir(const char *maildir, const char *dir,\n \tint i;\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \n+\tlist.cmp = maildir_filename_cmp;\n+\n \tif (populate_maildir_list(&list, maildir) < 0)\n \t\tgoto out;\n \n-- \n1.8.1.39.gbb3bf60\n"},{"id":"210493","messageId":"7v621aekr0.fsf@alter.siamese.dyndns.org","threadId":"33051","inReplyTo":"20130301233548.GA13422@sigill.intra.peff.net","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-01T23:57:39Z","receivedAt":"2013-03-01T23:57:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Mar 01, 2013 at 03:24:42PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:\n>> > ...\n>> >> The maildir spec explicitly says that readers should not make\n>> >> assumptions about the content of the filenames. Mutt happens to write\n>> >> them as:\n>> >> \n>> >>   ${epoch_seconds}.${pid}_${seq}.${host}\n>> >> \n>> >> so in practice, sorting them kind of works. Except that ...\n>> >> << it does not work >> ...\n>> > That ordering is not necessarily useful.\n>> > ...\n>> > So it is somewhat against the maildir spec, but I think in practice it\n>> > would help.\n>> \n>> I do not think it would help, unless these epoch_seconds are the\n>> sender timestamps.  And the number of digits in epoch-seconds for\n>> recent messages would be the same, so ordering textually would be\n>> sufficient if ordering by timestamp were.\n>\n> The epoch_seconds are the time of writing into the maildir. They will\n> typically all be the same, unless your system is very slow, or you are\n> writing a really long patch series. The PID likewise should be the same\n> for a given series. It's the sequence number that is the interesting bit\n> to sort numerically (for mutt, anyway; ditto for dovecot).\n\nOK, so as long as the user tells mutt what order the messages need\nto be written in when choosing these 16 patches, tiebreaking with\nthe sequence number would be sufficient.\n\nIs it easy to tell that to mutt, though, given that the patches\narrive in random order, not in the order they were sent?  I would\nimagine that you sort the messages in your inbox, select a group of\nmessages and tell mutt to write them out to a (new) empty maildir,\nand at that point mutt writes them out in the order you used to sort\nthem---is that how it is supposed to work?\n\nOr are you assuming that the user chooses 1/16, tells mutt to write\nit out, chooses 2/16, tells mut to write that out, iterating it 16\ntimes to write out a 16-patch series?\n\nI can see why your patch would _not_ hurt, but I cannot tell how\nmuch it would help without knowing that part of the detail.\n\n> -- >8 --\n> Subject: [PATCH] mailsplit: sort maildir filenames more cleverly\n>\n> A maildir does not technically record the order in which\n> items were placed into it. That means that when applying a\n> patch series from a maildir, we may get the patches in the\n> wrong order. We try to work around this by sorting the\n> filenames. Unfortunately, this may or may not work depending\n> on the naming scheme used by the writer of the maildir.\n>\n> For instance, mutt will write:\n>\n>   ${epoch_seconds}.${pid}_${seq}.${host}\n>\n> where we have:\n>\n>   - epoch_seconds: timestamp at which entry was written\n>   - pid: PID of writing process\n>   - seq: a sequence number to ensure uniqueness of filenames\n>   - host: hostname\n>\n> None of the numbers are zero-padded. Therefore, when we sort\n> the names as byte strings, entries that cross a digit\n> boundary (e.g., 10) will sort out of order.  In the case of\n> timestamps, it almost never matters (because we do not cross\n> a digit boundary in the epoch time very often these days).\n> But for the sequence number, a 10-patch series would be\n> ordered as 1, 10, 2, 3, etc.\n>\n> To fix this, we can use a custom sort comparison function\n> which traverses each string, comparing chunks of digits\n> numerically, and otherwise doing a byte-for-byte comparison.\n> That would sort:\n>\n>   123.456_1.bar\n>   123.456_2.bar\n>   ...\n>   123.456_10.bar\n>\n> according to the sequence number. Since maildir does not\n> define a filename format, this is really just a heuristic.\n> But it happens to work for mutt, and there is a reasonable\n> chance that it will work for other writers, too (at least as\n> well as a straight sort).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/mailsplit.c | 22 ++++++++++++++++++++++\n>  1 file changed, 22 insertions(+)\n>\n> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\n> index 2d43278..772c668 100644\n> --- a/builtin/mailsplit.c\n> +++ b/builtin/mailsplit.c\n> @@ -130,6 +130,26 @@ static int populate_maildir_list(struct string_list *list, const char *path)\n>  \treturn 0;\n>  }\n>  \n> +static int maildir_filename_cmp(const char *a, const char *b)\n> +{\n> +\twhile (1) {\n> +\t\tif (isdigit(*a) && isdigit(*b)) {\n> +\t\t\tlong int na, nb;\n> +\t\t\tna = strtol(a, (char **)&a, 10);\n> +\t\t\tnb = strtol(b, (char **)&b, 10);\n> +\t\t\tif (na != nb)\n> +\t\t\t\treturn na - nb;\n> +\t\t\t/* strtol advanced our pointers */\n> +\t\t}\n> +\t\telse {\n> +\t\t\tif (*a != *b)\n> +\t\t\t\treturn *a - *b;\n> +\t\t\ta++;\n> +\t\t\tb++;\n> +\t\t}\n> +\t}\n> +}\n> +\n>  static int split_maildir(const char *maildir, const char *dir,\n>  \tint nr_prec, int skip)\n>  {\n> @@ -139,6 +159,8 @@ static int split_maildir(const char *maildir, const char *dir,\n>  \tint i;\n>  \tstruct string_list list = STRING_LIST_INIT_DUP;\n>  \n> +\tlist.cmp = maildir_filename_cmp;\n> +\n>  \tif (populate_maildir_list(&list, maildir) < 0)\n>  \t\tgoto out;\n"},{"id":"210494","messageId":"7v1ubyek9n.fsf@alter.siamese.dyndns.org","threadId":"33051","inReplyTo":"20130301233548.GA13422@sigill.intra.peff.net","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-02T00:08:04Z","receivedAt":"2013-03-02T00:08:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\n> index 2d43278..772c668 100644\n> --- a/builtin/mailsplit.c\n> +++ b/builtin/mailsplit.c\n> @@ -130,6 +130,26 @@ static int populate_maildir_list(struct string_list *list, const char *path)\n>  \treturn 0;\n>  }\n>  \n> +static int maildir_filename_cmp(const char *a, const char *b)\n> +{\n> +\twhile (1) {\n\nIt is somewhat funny that we do not need to check !*a or !*b in this\nloop.  As long as readdir() does not return duplicates, we won't be\ncomparing the same strings with this function, and we won't read\npast '\\0' at the end of both a and b at the same time.\n\n> +\t\tif (isdigit(*a) && isdigit(*b)) {\n> +\t\t\tlong int na, nb;\n> +\t\t\tna = strtol(a, (char **)&a, 10);\n> +\t\t\tnb = strtol(b, (char **)&b, 10);\n> +\t\t\tif (na != nb)\n> +\t\t\t\treturn na - nb;\n> +\t\t\t/* strtol advanced our pointers */\n> +\t\t}\n> +\t\telse {\n> +\t\t\tif (*a != *b)\n> +\t\t\t\treturn *a - *b;\n> +\t\t\ta++;\n> +\t\t\tb++;\n> +\t\t}\n> +\t}\n> +}\n"},{"id":"210495","messageId":"20130302003811.GA14936@sigill.intra.peff.net","threadId":"33051","inReplyTo":"7v621aekr0.fsf@alter.siamese.dyndns.org","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-02T00:38:12Z","receivedAt":"2013-03-02T00:38:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2013 at 03:57:39PM -0800, Junio C Hamano wrote:\n\n> > The epoch_seconds are the time of writing into the maildir. They will\n> > typically all be the same, unless your system is very slow, or you are\n> > writing a really long patch series. The PID likewise should be the same\n> > for a given series. It's the sequence number that is the interesting bit\n> > to sort numerically (for mutt, anyway; ditto for dovecot).\n> \n> OK, so as long as the user tells mutt what order the messages need\n> to be written in when choosing these 16 patches, tiebreaking with\n> the sequence number would be sufficient.\n> \n> Is it easy to tell that to mutt, though, given that the patches\n> arrive in random order, not in the order they were sent?\n\nYes. You can either write one at a time, or you can tag several and\nwrite them with a single command. In the latter case, it writes them out\naccording to the currently displayed sort order. The usual threaded\ndisplay gets it right for delivered messages (it shows them in date\norder within the thread), and you can use sort-by-subject to override it\nif you are working with munged timestamps.\n\n> I would imagine that you sort the messages in your inbox, select a\n> group of messages and tell mutt to write them out to a (new) empty\n> maildir, and at that point mutt writes them out in the order you used\n> to sort them---is that how it is supposed to work?\n> \n> Or are you assuming that the user chooses 1/16, tells mutt to write it\n> out, chooses 2/16, tells mut to write that out, iterating it 16 times\n> to write out a 16-patch series?\n\nRight. The latter probably already works (unless you are so fast that\nyou write out multiple messages in one second), but I would expect most\npeople would tag and then save all at once.\n\n-Peff\n"},{"id":"210496","messageId":"20130302004120.GB14936@sigill.intra.peff.net","threadId":"33051","inReplyTo":"7v1ubyek9n.fsf@alter.siamese.dyndns.org","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-02T00:41:20Z","receivedAt":"2013-03-02T00:41:20Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 01, 2013 at 04:08:04PM -0800, Junio C Hamano wrote:\n\n> > +static int maildir_filename_cmp(const char *a, const char *b)\n> > +{\n> > +\twhile (1) {\n> \n> It is somewhat funny that we do not need to check !*a or !*b in this\n> loop.  As long as readdir() does not return duplicates, we won't be\n> comparing the same strings with this function, and we won't read\n> past '\\0' at the end of both a and b at the same time.\n\nUgh, thanks for catching. I had structured this differently at first\nwith a while(1), and then after refactoring it forgot to update the loop\ncondition. Even if it is fine without duplicates, we should not rely on\nthat not to cause an infinite loop (in case the function is ever used\nagain). But moreover, you can trigger the problem even now, since we\nwould parse \"foo_0123\" and \"foo_123\" as equivalent, and hit the NUL.\n\nI think this needs squashed in:\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 772c668..1ddbff4 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -132,7 +132,7 @@ static int maildir_filename_cmp(const char *a, const char *b)\n \n static int maildir_filename_cmp(const char *a, const char *b)\n {\n-\twhile (1) {\n+\twhile (*a && *b) {\n \t\tif (isdigit(*a) && isdigit(*b)) {\n \t\t\tlong int na, nb;\n \t\t\tna = strtol(a, (char **)&a, 10);\n@@ -148,6 +148,7 @@ static int maildir_filename_cmp(const char *a, const char *b)\n \t\t\tb++;\n \t\t}\n \t}\n+\treturn *a - *b;\n }\n \n static int split_maildir(const char *maildir, const char *dir,\n\n-Peff\n"},{"id":"210504","messageId":"m28v66198o.fsf@igel.home","threadId":"33051","inReplyTo":"20130302004120.GB14936@sigill.intra.peff.net","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2013-03-02T08:44:39Z","receivedAt":"2013-03-02T08:44:39Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>  static int maildir_filename_cmp(const char *a, const char *b)\n>  {\n> -\twhile (1) {\n> +\twhile (*a && *b) {\n>  \t\tif (isdigit(*a) && isdigit(*b)) {\n>  \t\t\tlong int na, nb;\n>  \t\t\tna = strtol(a, (char **)&a, 10);\n> @@ -148,6 +148,7 @@ static int maildir_filename_cmp(const char *a, const char *b)\n>  \t\t\tb++;\n>  \t\t}\n>  \t}\n> +\treturn *a - *b;\n\nYou should always cast to unsigned char when determining the order of\ncharacters, to be consistent with strcmp/memcmp.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"210516","messageId":"7vtxotc895.fsf@alter.siamese.dyndns.org","threadId":"33051","inReplyTo":"m28v66198o.fsf@igel.home","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-03T06:22:46Z","receivedAt":"2013-03-03T06:22:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andreas Schwab <schwab@linux-m68k.org> writes:\n\n> You should always cast to unsigned char when determining the order of\n> characters, to be consistent with strcmp/memcmp.\n\nWe treat runs of digits as numbers, so it is not even similar to\nstrcmp.  As long as it is internally consistent (i.e. the return\nvalue inside the loop (*a - *b) must match the last return), it\nshould be OK, no?\n"},{"id":"210517","messageId":"20130303062647.GA23221@sigill.intra.peff.net","threadId":"33051","inReplyTo":"m28v66198o.fsf@igel.home","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-03T06:26:47Z","receivedAt":"2013-03-03T06:26:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 02, 2013 at 09:44:39AM +0100, Andreas Schwab wrote:\n\n> > +\treturn *a - *b;\n> \n> You should always cast to unsigned char when determining the order of\n> characters, to be consistent with strcmp/memcmp.\n\nThanks, I hadn't heard that advice before, but it makes obvious sense.\nJunio, do you want to squash this on top?\n\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 1ddbff4..06296d4 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -143,12 +143,12 @@ static int maildir_filename_cmp(const char *a, const char *b)\n \t\t}\n \t\telse {\n \t\t\tif (*a != *b)\n-\t\t\t\treturn *a - *b;\n+\t\t\t\treturn (unsigned char)*a - (unsigned char)*b;\n \t\t\ta++;\n \t\t\tb++;\n \t\t}\n \t}\n-\treturn *a - *b;\n+\treturn (unsigned char)*a - (unsigned char)*b;\n }\n \n static int split_maildir(const char *maildir, const char *dir,\n\n-Peff\n"},{"id":"210519","messageId":"20130303063244.GB23221@sigill.intra.peff.net","threadId":"33051","inReplyTo":"7vtxotc895.fsf@alter.siamese.dyndns.org","subject":"Re: [bug report] git-am applying maildir patches in reverse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-03T06:32:44Z","receivedAt":"2013-03-03T06:32:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 02, 2013 at 10:22:46PM -0800, Junio C Hamano wrote:\n\n> Andreas Schwab <schwab@linux-m68k.org> writes:\n> \n> > You should always cast to unsigned char when determining the order of\n> > characters, to be consistent with strcmp/memcmp.\n> \n> We treat runs of digits as numbers, so it is not even similar to\n> strcmp.  As long as it is internally consistent (i.e. the return\n> value inside the loop (*a - *b) must match the last return), it\n> should be OK, no?\n\nI almost responded and said something similar, but we also do byte-wise\ncomparisons for non-numeric elements, and we would want those to match\nwhat other programs may do (and what git used to do).\n\nI highly doubt that it matters in practice, as it would mean:\n\n  1. The sorting of a maildir's filenames are dependent on the sorting\n     of non-numeric bits. We can't rule out such a scheme, but I'd guess\n     implementations either use numbers, or their sort order is\n     meaningless (and that is what I found in the ones I looked at).\n\n  2. The importantly-sorted bits contain non-ascii characters (the\n     difference is only seen when we go outside the signed range).\n\nbut it doesn't hurt to be thorough (and to set a good example).\n\n-Peff\n"}]}