[bug report] git-am applying maildir patches in reverse

[bug report] git-am applying maildir patches in reverse

[William Giokas]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

All,

I've been using git for a while and this is the first time I've had to
use `git am` and I've got a 16 patch patchset that I'm looking to apply.
The files were copied to a separate maildir by mutt to keep things
clean, and then I ran `git am -i /path/to/maildir/` expecting things to
start from the patch with the subject 

    [PATCH 01/16] refactor common code in query_search/sync_search

But instead, it starts with the 16/16 patch and works backwards, which,
obviously, breaks the application process as the patches depend on each
other. I looked in the maildir directory just to see if the file names
were backwards, and that's not the issue. I talked to `gitster` on IRC
and he said to send in a bug report on this issue here. The patchset I'm
trying to apply can be found here[0]. 

Process to reproduce:
* find a multi-patch set with interdependent patches
* run `git am` on the maildir containing these patches

Expected result:
* Apply patches in [01..N] order

Actual result:
* Patches applied in [N N-1..01] order

[0]: https://mailman.archlinux.org/pipermail/pacman-dev/2013-March/016541.html

Thank you,
-- 
William Giokas | KaiSforza
GnuPG Key: 0x73CD09CF
Fingerprint: F73F 50EF BBE2 9846 8306  E6B8 6902 06D8 73CD 09CF

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [bug report] git-am applying maildir patches in reverse

[Junio C Hamano]

William Giokas <1007380@gmail.com> writes:

> All,
>
> I've been using git for a while and this is the first time I've had to
> use `git am` and I've got a 16 patch patchset that I'm looking to apply.
> The files were copied to a separate maildir by mutt to keep things
> clean, and then I ran `git am -i /path/to/maildir/` expecting things to
> start from the patch with the subject 
>
>     [PATCH 01/16] refactor common code in query_search/sync_search
>
> But instead, it starts with the 16/16 patch and works backwards, which,
> obviously, breaks the application process as the patches depend on each
> other. I looked in the maildir directory just to see if the file names
> were backwards, and that's not the issue. I talked to `gitster` on IRC
> and he said to send in a bug report on this issue here. The patchset I'm
> trying to apply can be found here[0]. 
>
> Process to reproduce:
> * find a multi-patch set with interdependent patches
> * run `git am` on the maildir containing these patches
>
> Expected result:
> * Apply patches in [01..N] order
>
> Actual result:
> * Patches applied in [N N-1..01] order

Note to bystanders.  This is coming from populate_maildir_list() in
builtin/mailsplit.c; the function claims to know what "maildir"
should look like, so it should be enforcing the ordering as
necessary by sorting the list, _if_ the implicit ordering given by
string_list_insert() is insufficient.

It also is likely that it is a user error to expect that patch
e-mails are received and stored in the maildir in the order they
were sent, or it could be "mutt" copying the mails in the order the
messages were originally received, or something silly like that.

>
> [0]: https://mailman.archlinux.org/pipermail/pacman-dev/2013-March/016541.html
>
> Thank you,

Re: [bug report] git-am applying maildir patches in reverse

[Jeff King]

On Fri, Mar 01, 2013 at 02:27:32PM -0800, Junio C Hamano wrote:

> > I've been using git for a while and this is the first time I've had to
> > use `git am` and I've got a 16 patch patchset that I'm looking to apply.
> > The files were copied to a separate maildir by mutt to keep things
> > clean, and then I ran `git am -i /path/to/maildir/` expecting things to
> > start from the patch with the subject 
> >
> >     [PATCH 01/16] refactor common code in query_search/sync_search
> >
> > But instead, it starts with the 16/16 patch and works backwards, which,
> > obviously, breaks the application process as the patches depend on each
>
> Note to bystanders.  This is coming from populate_maildir_list() in
> builtin/mailsplit.c; the function claims to know what "maildir"
> should look like, so it should be enforcing the ordering as
> necessary by sorting the list, _if_ the implicit ordering given by
> string_list_insert() is insufficient.
> 
> It also is likely that it is a user error to expect that patch
> e-mails are received and stored in the maildir in the order they
> were sent, or it could be "mutt" copying the mails in the order the
> messages were originally received, or something silly like that.

I think it is a property of the maildir format that it does not
technically define the message order. The order of items you get from
readdir is filesystem specific and not necessarily defined (and that is
what we use now). On my ext4 system, I do not even get them in backwards
order; it is jumbled.

We could sort based on the mtime of the file, but in some cases that
won't have sufficient resolution (e.g., with one second resolution,
they'll all probably have the same timestamp).

The maildir spec explicitly says that readers should not make
assumptions about the content of the filenames. Mutt happens to write
them as:

  ${epoch_seconds}.${pid}_${seq}.${host}

so in practice, sorting them kind of works. Except that a series written
out at once will all have the same timestamp and pid, and because ${seq}
is not zero-padded, you have to actually parse up to there and do a
numeric instead of byte-wise comparison.  So we can add a mutt-specific
hack, but that's the best we can do. Other maildir writers (including
future versions of mutt) will not necessarily do the same thing.

I think maildir's philosophy is that ordering is not important, and the
contents of the messages themselves should define the order in which a
MUA presents them, anyway. It would make sense to me if we actually
parsed the '[PATCH n/m]' bit from the subject of each and sorted based
on that. That is blurring the line between git-mailsplit and
git-mailinfo, in that mailsplit would have to start parsing the files.

We could also sort based on the rfc822 "Date" header, but I don't think
that is a good idea. It represents the author timestamp, so patches
moved via "rebase -i" will have out-of-sequence dates with respect to
the actual history graph.  You can encounter this if you "format-patch
--stdout" a series into a single mbox and load it into a MUA that sorts
by date (like mutt). The patches appear jumbled until you switch to
"unsorted" or "sort by subject".

-Peff

Re: [bug report] git-am applying maildir patches in reverse

[Jeff King]

On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:

> > Note to bystanders.  This is coming from populate_maildir_list() in
> > builtin/mailsplit.c; the function claims to know what "maildir"
> > should look like, so it should be enforcing the ordering as
> > necessary by sorting the list, _if_ the implicit ordering given by
> > string_list_insert() is insufficient.
> [...]
> 
> I think it is a property of the maildir format that it does not
> technically define the message order. The order of items you get from
> readdir is filesystem specific and not necessarily defined (and that is
> what we use now). On my ext4 system, I do not even get them in backwards
> order; it is jumbled.

Hmph, sorry, I mistook string_list_insert for string_list_append. So we
do actually sort them by filename, not just random readdir order. But
due to this:

> The maildir spec explicitly says that readers should not make
> assumptions about the content of the filenames. Mutt happens to write
> them as:
> 
>   ${epoch_seconds}.${pid}_${seq}.${host}
> 
> so in practice, sorting them kind of works. Except that a series written
> out at once will all have the same timestamp and pid, and because ${seq}
> is not zero-padded, you have to actually parse up to there and do a
> numeric instead of byte-wise comparison.  So we can add a mutt-specific
> hack, but that's the best we can do. Other maildir writers (including
> future versions of mutt) will not necessarily do the same thing.

That ordering is not necessarily useful.

So one strategy could be to try harder to sort numeric elements
numerically. That is, to treat:

  1234.5678_90.foo

as the list

  {1234, '.', 5678, '_', 90, "foo"}

for the purposes of sorting (even though we do not necessarily know what
each piece means). That works for mutt and similar schemes (dovecot, for
example, does something like "M${seq}P${pid}"), but some systems may put
random bytes in the middle (the point of it is to create a unique name).

So it is somewhat against the maildir spec, but I think in practice it
would help.

-Peff

Re: [bug report] git-am applying maildir patches in reverse

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:
> ...
>> The maildir spec explicitly says that readers should not make
>> assumptions about the content of the filenames. Mutt happens to write
>> them as:
>> 
>>   ${epoch_seconds}.${pid}_${seq}.${host}
>> 
>> so in practice, sorting them kind of works. Except that ...
>> << it does not work >> ...
> That ordering is not necessarily useful.
> ...
> So it is somewhat against the maildir spec, but I think in practice it
> would help.

I do not think it would help, unless these epoch_seconds are the
sender timestamps.  And the number of digits in epoch-seconds for
recent messages would be the same, so ordering textually would be
sufficient if ordering by timestamp were.

Re: [bug report] git-am applying maildir patches in reverse

[Jeff King]

On Fri, Mar 01, 2013 at 03:24:42PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:
> > ...
> >> The maildir spec explicitly says that readers should not make
> >> assumptions about the content of the filenames. Mutt happens to write
> >> them as:
> >> 
> >>   ${epoch_seconds}.${pid}_${seq}.${host}
> >> 
> >> so in practice, sorting them kind of works. Except that ...
> >> << it does not work >> ...
> > That ordering is not necessarily useful.
> > ...
> > So it is somewhat against the maildir spec, but I think in practice it
> > would help.
> 
> I do not think it would help, unless these epoch_seconds are the
> sender timestamps.  And the number of digits in epoch-seconds for
> recent messages would be the same, so ordering textually would be
> sufficient if ordering by timestamp were.

The epoch_seconds are the time of writing into the maildir. They will
typically all be the same, unless your system is very slow, or you are
writing a really long patch series. The PID likewise should be the same
for a given series. It's the sequence number that is the interesting bit
to sort numerically (for mutt, anyway; ditto for dovecot).

The patch below seems to fix the issue for me with mutt. It's possible
that it regresses another case (which has numbers, but really wants them
sorted as byte strings), but I find that unlikely. If you're
zero-padding your numbers this will still work, and if you're not, then
you likely have no meaningful sort order at all.

-- >8 --
Subject: [PATCH] mailsplit: sort maildir filenames more cleverly

A maildir does not technically record the order in which
items were placed into it. That means that when applying a
patch series from a maildir, we may get the patches in the
wrong order. We try to work around this by sorting the
filenames. Unfortunately, this may or may not work depending
on the naming scheme used by the writer of the maildir.

For instance, mutt will write:

  ${epoch_seconds}.${pid}_${seq}.${host}

where we have:

  - epoch_seconds: timestamp at which entry was written
  - pid: PID of writing process
  - seq: a sequence number to ensure uniqueness of filenames
  - host: hostname

None of the numbers are zero-padded. Therefore, when we sort
the names as byte strings, entries that cross a digit
boundary (e.g., 10) will sort out of order.  In the case of
timestamps, it almost never matters (because we do not cross
a digit boundary in the epoch time very often these days).
But for the sequence number, a 10-patch series would be
ordered as 1, 10, 2, 3, etc.

To fix this, we can use a custom sort comparison function
which traverses each string, comparing chunks of digits
numerically, and otherwise doing a byte-for-byte comparison.
That would sort:

  123.456_1.bar
  123.456_2.bar
  ...
  123.456_10.bar

according to the sequence number. Since maildir does not
define a filename format, this is really just a heuristic.
But it happens to work for mutt, and there is a reasonable
chance that it will work for other writers, too (at least as
well as a straight sort).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/mailsplit.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 2d43278..772c668 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -130,6 +130,26 @@ static int populate_maildir_list(struct string_list *list, const char *path)
 	return 0;
 }
 
+static int maildir_filename_cmp(const char *a, const char *b)
+{
+	while (1) {
+		if (isdigit(*a) && isdigit(*b)) {
+			long int na, nb;
+			na = strtol(a, (char **)&a, 10);
+			nb = strtol(b, (char **)&b, 10);
+			if (na != nb)
+				return na - nb;
+			/* strtol advanced our pointers */
+		}
+		else {
+			if (*a != *b)
+				return *a - *b;
+			a++;
+			b++;
+		}
+	}
+}
+
 static int split_maildir(const char *maildir, const char *dir,
 	int nr_prec, int skip)
 {
@@ -139,6 +159,8 @@ static int split_maildir(const char *maildir, const char *dir,
 	int i;
 	struct string_list list = STRING_LIST_INIT_DUP;
 
+	list.cmp = maildir_filename_cmp;
+
 	if (populate_maildir_list(&list, maildir) < 0)
 		goto out;
 
-- 
1.8.1.39.gbb3bf60

Re: [bug report] git-am applying maildir patches in reverse

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 01, 2013 at 03:24:42PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > On Fri, Mar 01, 2013 at 05:52:31PM -0500, Jeff King wrote:
>> > ...
>> >> The maildir spec explicitly says that readers should not make
>> >> assumptions about the content of the filenames. Mutt happens to write
>> >> them as:
>> >> 
>> >>   ${epoch_seconds}.${pid}_${seq}.${host}
>> >> 
>> >> so in practice, sorting them kind of works. Except that ...
>> >> << it does not work >> ...
>> > That ordering is not necessarily useful.
>> > ...
>> > So it is somewhat against the maildir spec, but I think in practice it
>> > would help.
>> 
>> I do not think it would help, unless these epoch_seconds are the
>> sender timestamps.  And the number of digits in epoch-seconds for
>> recent messages would be the same, so ordering textually would be
>> sufficient if ordering by timestamp were.
>
> The epoch_seconds are the time of writing into the maildir. They will
> typically all be the same, unless your system is very slow, or you are
> writing a really long patch series. The PID likewise should be the same
> for a given series. It's the sequence number that is the interesting bit
> to sort numerically (for mutt, anyway; ditto for dovecot).

OK, so as long as the user tells mutt what order the messages need
to be written in when choosing these 16 patches, tiebreaking with
the sequence number would be sufficient.

Is it easy to tell that to mutt, though, given that the patches
arrive in random order, not in the order they were sent?  I would
imagine that you sort the messages in your inbox, select a group of
messages and tell mutt to write them out to a (new) empty maildir,
and at that point mutt writes them out in the order you used to sort
them---is that how it is supposed to work?

Or are you assuming that the user chooses 1/16, tells mutt to write
it out, chooses 2/16, tells mut to write that out, iterating it 16
times to write out a 16-patch series?

I can see why your patch would _not_ hurt, but I cannot tell how
much it would help without knowing that part of the detail.

> -- >8 --
> Subject: [PATCH] mailsplit: sort maildir filenames more cleverly
>
> A maildir does not technically record the order in which
> items were placed into it. That means that when applying a
> patch series from a maildir, we may get the patches in the
> wrong order. We try to work around this by sorting the
> filenames. Unfortunately, this may or may not work depending
> on the naming scheme used by the writer of the maildir.
>
> For instance, mutt will write:
>
>   ${epoch_seconds}.${pid}_${seq}.${host}
>
> where we have:
>
>   - epoch_seconds: timestamp at which entry was written
>   - pid: PID of writing process
>   - seq: a sequence number to ensure uniqueness of filenames
>   - host: hostname
>
> None of the numbers are zero-padded. Therefore, when we sort
> the names as byte strings, entries that cross a digit
> boundary (e.g., 10) will sort out of order.  In the case of
> timestamps, it almost never matters (because we do not cross
> a digit boundary in the epoch time very often these days).
> But for the sequence number, a 10-patch series would be
> ordered as 1, 10, 2, 3, etc.
>
> To fix this, we can use a custom sort comparison function
> which traverses each string, comparing chunks of digits
> numerically, and otherwise doing a byte-for-byte comparison.
> That would sort:
>
>   123.456_1.bar
>   123.456_2.bar
>   ...
>   123.456_10.bar
>
> according to the sequence number. Since maildir does not
> define a filename format, this is really just a heuristic.
> But it happens to work for mutt, and there is a reasonable
> chance that it will work for other writers, too (at least as
> well as a straight sort).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/mailsplit.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 2d43278..772c668 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -130,6 +130,26 @@ static int populate_maildir_list(struct string_list *list, const char *path)
>  	return 0;
>  }
>  
> +static int maildir_filename_cmp(const char *a, const char *b)
> +{
> +	while (1) {
> +		if (isdigit(*a) && isdigit(*b)) {
> +			long int na, nb;
> +			na = strtol(a, (char **)&a, 10);
> +			nb = strtol(b, (char **)&b, 10);
> +			if (na != nb)
> +				return na - nb;
> +			/* strtol advanced our pointers */
> +		}
> +		else {
> +			if (*a != *b)
> +				return *a - *b;
> +			a++;
> +			b++;
> +		}
> +	}
> +}
> +
>  static int split_maildir(const char *maildir, const char *dir,
>  	int nr_prec, int skip)
>  {
> @@ -139,6 +159,8 @@ static int split_maildir(const char *maildir, const char *dir,
>  	int i;
>  	struct string_list list = STRING_LIST_INIT_DUP;
>  
> +	list.cmp = maildir_filename_cmp;
> +
>  	if (populate_maildir_list(&list, maildir) < 0)
>  		goto out;

Re: [bug report] git-am applying maildir patches in reverse

[Jeff King]

On Fri, Mar 01, 2013 at 03:57:39PM -0800, Junio C Hamano wrote:

> > The epoch_seconds are the time of writing into the maildir. They will
> > typically all be the same, unless your system is very slow, or you are
> > writing a really long patch series. The PID likewise should be the same
> > for a given series. It's the sequence number that is the interesting bit
> > to sort numerically (for mutt, anyway; ditto for dovecot).
> 
> OK, so as long as the user tells mutt what order the messages need
> to be written in when choosing these 16 patches, tiebreaking with
> the sequence number would be sufficient.
> 
> Is it easy to tell that to mutt, though, given that the patches
> arrive in random order, not in the order they were sent?

Yes. You can either write one at a time, or you can tag several and
write them with a single command. In the latter case, it writes them out
according to the currently displayed sort order. The usual threaded
display gets it right for delivered messages (it shows them in date
order within the thread), and you can use sort-by-subject to override it
if you are working with munged timestamps.

> I would imagine that you sort the messages in your inbox, select a
> group of messages and tell mutt to write them out to a (new) empty
> maildir, and at that point mutt writes them out in the order you used
> to sort them---is that how it is supposed to work?
> 
> Or are you assuming that the user chooses 1/16, tells mutt to write it
> out, chooses 2/16, tells mut to write that out, iterating it 16 times
> to write out a 16-patch series?

Right. The latter probably already works (unless you are so fast that
you write out multiple messages in one second), but I would expect most
people would tag and then save all at once.

-Peff

Re: [bug report] git-am applying maildir patches in reverse

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 2d43278..772c668 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -130,6 +130,26 @@ static int populate_maildir_list(struct string_list *list, const char *path)
>  	return 0;
>  }
>  
> +static int maildir_filename_cmp(const char *a, const char *b)
> +{
> +	while (1) {

It is somewhat funny that we do not need to check !*a or !*b in this
loop.  As long as readdir() does not return duplicates, we won't be
comparing the same strings with this function, and we won't read
past '\0' at the end of both a and b at the same time.

> +		if (isdigit(*a) && isdigit(*b)) {
> +			long int na, nb;
> +			na = strtol(a, (char **)&a, 10);
> +			nb = strtol(b, (char **)&b, 10);
> +			if (na != nb)
> +				return na - nb;
> +			/* strtol advanced our pointers */
> +		}
> +		else {
> +			if (*a != *b)
> +				return *a - *b;
> +			a++;
> +			b++;
> +		}
> +	}
> +}

Re: [bug report] git-am applying maildir patches in reverse

[Jeff King]

On Fri, Mar 01, 2013 at 04:08:04PM -0800, Junio C Hamano wrote:

> > +static int maildir_filename_cmp(const char *a, const char *b)
> > +{
> > +	while (1) {
> 
> It is somewhat funny that we do not need to check !*a or !*b in this
> loop.  As long as readdir() does not return duplicates, we won't be
> comparing the same strings with this function, and we won't read
> past '\0' at the end of both a and b at the same time.

Ugh, thanks for catching. I had structured this differently at first
with a while(1), and then after refactoring it forgot to update the loop
condition. Even if it is fine without duplicates, we should not rely on
that not to cause an infinite loop (in case the function is ever used
again). But moreover, you can trigger the problem even now, since we
would parse "foo_0123" and "foo_123" as equivalent, and hit the NUL.

I think this needs squashed in:

diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 772c668..1ddbff4 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -132,7 +132,7 @@ static int maildir_filename_cmp(const char *a, const char *b)
 
 static int maildir_filename_cmp(const char *a, const char *b)
 {
-	while (1) {
+	while (*a && *b) {
 		if (isdigit(*a) && isdigit(*b)) {
 			long int na, nb;
 			na = strtol(a, (char **)&a, 10);
@@ -148,6 +148,7 @@ static int maildir_filename_cmp(const char *a, const char *b)
 			b++;
 		}
 	}
+	return *a - *b;
 }
 
 static int split_maildir(const char *maildir, const char *dir,

-Peff

Re: [bug report] git-am applying maildir patches in reverse

[Andreas Schwab]

Jeff King <peff@peff.net> writes:

>  static int maildir_filename_cmp(const char *a, const char *b)
>  {
> -	while (1) {
> +	while (*a && *b) {
>  		if (isdigit(*a) && isdigit(*b)) {
>  			long int na, nb;
>  			na = strtol(a, (char **)&a, 10);
> @@ -148,6 +148,7 @@ static int maildir_filename_cmp(const char *a, const char *b)
>  			b++;
>  		}
>  	}
> +	return *a - *b;

You should always cast to unsigned char when determining the order of
characters, to be consistent with strcmp/memcmp.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [bug report] git-am applying maildir patches in reverse

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> You should always cast to unsigned char when determining the order of
> characters, to be consistent with strcmp/memcmp.

We treat runs of digits as numbers, so it is not even similar to
strcmp.  As long as it is internally consistent (i.e. the return
value inside the loop (*a - *b) must match the last return), it
should be OK, no?

Re: [bug report] git-am applying maildir patches in reverse

[Jeff King]

On Sat, Mar 02, 2013 at 10:22:46PM -0800, Junio C Hamano wrote:

> Andreas Schwab <schwab@linux-m68k.org> writes:
> 
> > You should always cast to unsigned char when determining the order of
> > characters, to be consistent with strcmp/memcmp.
> 
> We treat runs of digits as numbers, so it is not even similar to
> strcmp.  As long as it is internally consistent (i.e. the return
> value inside the loop (*a - *b) must match the last return), it
> should be OK, no?

I almost responded and said something similar, but we also do byte-wise
comparisons for non-numeric elements, and we would want those to match
what other programs may do (and what git used to do).

I highly doubt that it matters in practice, as it would mean:

  1. The sorting of a maildir's filenames are dependent on the sorting
     of non-numeric bits. We can't rule out such a scheme, but I'd guess
     implementations either use numbers, or their sort order is
     meaningless (and that is what I found in the ones I looked at).

  2. The importantly-sorted bits contain non-ascii characters (the
     difference is only seen when we go outside the signed range).

but it doesn't hurt to be thorough (and to set a good example).

-Peff

Re: [bug report] git-am applying maildir patches in reverse

[Jeff King]

On Sat, Mar 02, 2013 at 09:44:39AM +0100, Andreas Schwab wrote:

> > +	return *a - *b;
> 
> You should always cast to unsigned char when determining the order of
> characters, to be consistent with strcmp/memcmp.

Thanks, I hadn't heard that advice before, but it makes obvious sense.
Junio, do you want to squash this on top?

diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 1ddbff4..06296d4 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -143,12 +143,12 @@ static int maildir_filename_cmp(const char *a, const char *b)
 		}
 		else {
 			if (*a != *b)
-				return *a - *b;
+				return (unsigned char)*a - (unsigned char)*b;
 			a++;
 			b++;
 		}
 	}
-	return *a - *b;
+	return (unsigned char)*a - (unsigned char)*b;
 }
 
 static int split_maildir(const char *maildir, const char *dir,

-Peff