Re: Mailsplit
On Friday, September 7, 2018 8:15:43 AM MST Kevin Daudt wrote: > On Wed, Sep 05, 2018 at 09:17:29PM -0700, Stephen & Linda Smith wrote: > > This is the mailsplit command, and should be executed when running `git > mailsplit`. What does git --exec-dir return? > The other night when I ran "git mailsplit", I recieved an unknown command response. Since then I have upated to 2.19.0 plush my submitted patches for git commit. With the new build mailsplit is found. I don't know what was wrong before.
Re: Mailsplit
On Wed, Sep 05, 2018 at 09:17:29PM -0700, Stephen & Linda Smith wrote: > I thought I would use "git mailsplit" to split a mbox file (which downloaded > from public inbox) so that I could attemp to resurrect a patch series for > from > a year ago. > > The motivation is that I downloaded the series [1] and applied to a tag from > about the time period that the patch was sent out [2]. > > The "git am -3 patch.mbox quit 2/3 of the way though. I resolved the fix. > and ran "git am --continue" which didn't apply the rest of the patches in the > mbox. > > So two questions: > 1) why would git version 2.18.0 not appear to continue applying the patches. > > > 2) where do I find the command "git mailsplit". The onlything in my > installed tree is: > > $ find /usr/local/ -name '*mailsplit*' > /usr/local/share/doc/git-doc/git-mailsplit.txt > /usr/local/share/doc/git-doc/git-mailsplit.html > /usr/local/share/man/man1/git-mailsplit.1 > /usr/local/libexec/git-core/git-mailsplit This is the mailsplit command, and should be executed when running `git mailsplit`. What does git --exec-dir return? Kevin
Re: Mailsplit
On Wednesday, September 5, 2018 9:17:29 PM MST Stephen & Linda Smith wrote: > So two questions: > 1) why would git version 2.18.0 not appear to continue applying the > patches. > > 2) where do I find the command "git mailsplit". The onlything in my > installed tree is: > Never mind, I downloaded each patch separately.
Mailsplit
I thought I would use "git mailsplit" to split a mbox file (which downloaded
from public inbox) so that I could attemp to resurrect a patch series for from
a year ago.
The motivation is that I downloaded the series [1] and applied to a tag from
about the time period that the patch was sent out [2].
The "git am -3 patch.mbox quit 2/3 of the way though. I resolved the fix.
and ran "git am --continue" which didn't apply the rest of the patches in the
mbox.
So two questions:
1) why would git version 2.18.0 not appear to continue applying the patches.
2) where do I find the command "git mailsplit". The onlything in my
installed tree is:
$ find /usr/local/ -name '*mailsplit*'
/usr/local/share/doc/git-doc/git-mailsplit.txt
/usr/local/share/doc/git-doc/git-mailsplit.html
/usr/local/share/man/man1/git-mailsplit.1
/usr/local/libexec/git-core/git-mailsplit
sps
[1]
https://public-inbox.org/git/1502914591-26215-1-git-send-email-martin@mail.zuhause/
[2] I wanted to make sure that I could apply back then before attempting to
either apply to the latest git version or rebase to the latest git version.
[PATCH v4 09/25] mailinfo & mailsplit: check for EOF while parsing
While POSIX states that it is okay to pass EOF to isspace() (and it seems
to be implied that EOF should *not* be treated as whitespace), and also to
pass EOF to ungetc() (which seems to be intended to fail without buffering
the character), it is much better to handle these cases explicitly. Not
only does it reduce head-scratching (and helps static analysis avoid
reporting false positives), it also lets us handle files containing
nothing but whitespace by erroring out.
Reported via Coverity.
Signed-off-by: Johannes Schindelin
---
builtin/mailsplit.c | 10 ++
mailinfo.c | 9 -
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c13..664400b8169 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -232,6 +232,16 @@ static int split_mbox(const char *file, const char *dir,
int allow_bare,
do {
peek = fgetc(f);
+ if (peek == EOF) {
+ if (f == stdin)
+ /* empty stdin is OK */
+ ret = skip;
+ else {
+ fclose(f);
+ error(_("empty mbox: '%s'"), file);
+ }
+ goto out;
+ }
} while (isspace(peek));
ungetc(peek, f);
diff --git a/mailinfo.c b/mailinfo.c
index 68037758f2f..f92cb9f729c 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE
*in)
for (;;) {
int peek;
- peek = fgetc(in); ungetc(peek, in);
+ peek = fgetc(in);
+ if (peek == EOF)
+ break;
+ ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(&continuation, in))
@@ -1099,6 +1102,10 @@ int mailinfo(struct mailinfo *mi, const char *msg, const
char *patch)
do {
peek = fgetc(mi->input);
+ if (peek == EOF) {
+ fclose(cmitmsg);
+ return error("empty patch: '%s'", patch);
+ }
} while (isspace(peek));
ungetc(peek, mi->input);
--
2.12.2.windows.2.800.gede8f145e06
[PATCH v3 09/25] mailinfo & mailsplit: check for EOF while parsing
While POSIX states that it is okay to pass EOF to isspace() (and it seems
to be implied that EOF should *not* be treated as whitespace), and also to
pass EOF to ungetc() (which seems to be intended to fail without buffering
the character), it is much better to handle these cases explicitly. Not
only does it reduce head-scratching (and helps static analysis avoid
reporting false positives), it also lets us handle files containing
nothing but whitespace by erroring out.
Reported via Coverity.
Signed-off-by: Johannes Schindelin
---
builtin/mailsplit.c | 10 ++
mailinfo.c | 9 -
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c13..664400b8169 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -232,6 +232,16 @@ static int split_mbox(const char *file, const char *dir,
int allow_bare,
do {
peek = fgetc(f);
+ if (peek == EOF) {
+ if (f == stdin)
+ /* empty stdin is OK */
+ ret = skip;
+ else {
+ fclose(f);
+ error(_("empty mbox: '%s'"), file);
+ }
+ goto out;
+ }
} while (isspace(peek));
ungetc(peek, f);
diff --git a/mailinfo.c b/mailinfo.c
index 68037758f2f..f92cb9f729c 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE
*in)
for (;;) {
int peek;
- peek = fgetc(in); ungetc(peek, in);
+ peek = fgetc(in);
+ if (peek == EOF)
+ break;
+ ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(&continuation, in))
@@ -1099,6 +1102,10 @@ int mailinfo(struct mailinfo *mi, const char *msg, const
char *patch)
do {
peek = fgetc(mi->input);
+ if (peek == EOF) {
+ fclose(cmitmsg);
+ return error("empty patch: '%s'", patch);
+ }
} while (isspace(peek));
ungetc(peek, mi->input);
--
2.12.2.windows.2.800.gede8f145e06
Re: [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing
Hi Junio,
On Mon, 1 May 2017, Junio C Hamano wrote:
> Johannes Schindelin writes:
>
> > diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> > index 30681681c13..9b3efc8e987 100644
> > --- a/builtin/mailsplit.c
> > +++ b/builtin/mailsplit.c
> > @@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char
> > *dir, int allow_bare,
> > do {
> > peek = fgetc(f);
> > } while (isspace(peek));
> > - ungetc(peek, f);
> > + if (peek != EOF)
> > + ungetc(peek, f);
>
> I agree more with the first sentence in the proposed log message
> than what this code actually does. I.e. breaking upon seeing an EOF
> explicitly would be nice, just like the change to mailinfo.c in this
> patch we see below.
I changed it to error out with a (translated) "empty mbox: '%s'" as the
other hunks.
> > @@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg,
> > const char *patch)
> > return -1;
> > }
> >
> > - mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> > - mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> > -
> > do {
> > peek = fgetc(mi->input);
> > + if (peek == EOF) {
> > + fclose(cmitmsg);
> > + return error("empty patch: '%s'", patch);
> > + }
> > } while (isspace(peek));
> > ungetc(peek, mi->input);
>
> The handling of EOF is improved, but is it correct to move the
> allocation of p/s_hdr_data down?
Sorry, I *assumed* that the function was passed a zero-initialized
mailinfo struct. And as you pointed out, that assumption was wrong.
My thinking was that I did not want to introduce another leakage by my
patch, but as your careful analysis determined: the only caller that does
not die() afterwards releases the data (and would not even be able to
handle p_hdr_data == NULL...).
I reverted that move.
Ciao,
Dscho
Re: [PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing
Johannes Schindelin writes:
> While POSIX states that it is okay to pass EOF to isspace() (and it seems
> to be implied that EOF should *not* be treated as whitespace), and also to
> pass EOF to ungetc() (which seems to be intended to fail without buffering
> the character), it is much better to handle these cases explicitly. Not
> only does it reduce head-scratching (and helps static analysis avoid
> reporting false positives), it also lets us handle files containing
> nothing but whitespace by erroring out.
>
> Reported via Coverity.
>
> Signed-off-by: Johannes Schindelin
> ---
> builtin/mailsplit.c | 3 ++-
> mailinfo.c | 15 +++
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 30681681c13..9b3efc8e987 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char *dir,
> int allow_bare,
> do {
> peek = fgetc(f);
> } while (isspace(peek));
> - ungetc(peek, f);
> + if (peek != EOF)
> + ungetc(peek, f);
I agree more with the first sentence in the proposed log message
than what this code actually does. I.e. breaking upon seeing an EOF
explicitly would be nice, just like the change to mailinfo.c in this
patch we see below.
> @@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg,
> const char *patch)
> return -1;
> }
>
> - mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> - mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> -
> do {
> peek = fgetc(mi->input);
> + if (peek == EOF) {
> + fclose(cmitmsg);
> + return error("empty patch: '%s'", patch);
> + }
> } while (isspace(peek));
> ungetc(peek, mi->input);
The handling of EOF is improved, but is it correct to move the
allocation of p/s_hdr_data down?
Among the two callers, builtin/am.c just dies when it sees
mailinfo() returns an error, but builtin/mailinfo.c tries to be
nicer and calls clear_mailinfo(). Wouldn't this make that codepath
dereference a NULL pointer?
I think the moral of the story is that people tend to get sloppier
when doing "while we are at it" than the main task, and a reviewer
needs to be more careful while reviewing the "while we are at it"
part of the change than the primary thing a patch wants to do ;-)
> + mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
> + mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
> +
> /* process the email header */
> while (read_one_header_line(&line, mi->input))
> check_header(mi, &line, mi->p_hdr_data, 1);
[PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing
While POSIX states that it is okay to pass EOF to isspace() (and it seems
to be implied that EOF should *not* be treated as whitespace), and also to
pass EOF to ungetc() (which seems to be intended to fail without buffering
the character), it is much better to handle these cases explicitly. Not
only does it reduce head-scratching (and helps static analysis avoid
reporting false positives), it also lets us handle files containing
nothing but whitespace by erroring out.
Reported via Coverity.
Signed-off-by: Johannes Schindelin
---
builtin/mailsplit.c | 3 ++-
mailinfo.c | 15 +++
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c13..9b3efc8e987 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char *dir,
int allow_bare,
do {
peek = fgetc(f);
} while (isspace(peek));
- ungetc(peek, f);
+ if (peek != EOF)
+ ungetc(peek, f);
if (strbuf_getwholeline(&buf, f, '\n')) {
/* empty stdin is OK */
diff --git a/mailinfo.c b/mailinfo.c
index 68037758f2f..a319911b510 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE
*in)
for (;;) {
int peek;
- peek = fgetc(in); ungetc(peek, in);
+ peek = fgetc(in);
+ if (peek == EOF)
+ break;
+ ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(&continuation, in))
@@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg,
const char *patch)
return -1;
}
- mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
- mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
-
do {
peek = fgetc(mi->input);
+ if (peek == EOF) {
+ fclose(cmitmsg);
+ return error("empty patch: '%s'", patch);
+ }
} while (isspace(peek));
ungetc(peek, mi->input);
+ mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
+ mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
+
/* process the email header */
while (read_one_header_line(&line, mi->input))
check_header(mi, &line, mi->p_hdr_data, 1);
--
2.12.2.windows.2.800.gede8f145e06
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Junio C Hamano wrote: > Eric Wong writes: > > >> Also, doesn't it break "git rebase" (non-interactive), or anything > >> that internally runs format-patch to individual files and then runs > >> am on each of them, anything that knows that each output file from > >> format-patch corresponds to a single change and there is no need to > >> split, badly if we do this unconditionally? > > > > Yes, rebase should probably unescape is_from_line matches. > > This shouldn't matter for "git rebase", as it only reads from the > mbox "From " line to learn the > original commit and extract the log message directly from there. OK. > But a third-party script that wants to read format-patch output > would be forced to upgrade, which is a pain if we make this change > unconditionally. I suppose unconditionally having mailsplit unescape ">From ... $DATE" lines might be acceptable? It'll still propagate these mistakes to older versions of git, but those installations will eventually become fewer. > >> IOW, shouldn't this be an optional feature to format-patch that > >> is triggered by passing a new command line option that currently > >> nobody is passing? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit
Andreas Schwab writes:
>> +colon = line + len - 2;
>> +line += 5;
>> +for (;;) {
>> +if (colon < line)
>> +return 0;
>> +if (*--colon == ':')
>> +break;
>> +}
>> +
>> +if (!isdigit(colon[-4]) ||
>> +!isdigit(colon[-2]) ||
>> +!isdigit(colon[-1]) ||
>> +!isdigit(colon[ 1]) ||
>> +!isdigit(colon[ 2]))
>> +return 0;
>> +
>> +/* year */
>> +if (strtol(colon+3, NULL, 10) <= 90)
>> +return 0;
>> +
>> +/* Ok, close enough */
>> +return 1;
>> +}
>
> Should this be made more strict, like by checking for a space before the
> year?
The function seems to judge the line to be "close enough" by
checking these places (please view with fixed-width font ;-):
From me Mon Jul 25 01:23:45 2016
x x x
We only see if the year part is more than 90 but we do not even
ensure that it is at the end of the line without any further
garbage, which I think is probably OK for the purpose of "close
enough".
We also do not bother rejecting a single-digit hour, or there is a
colon between the hour and minute part.
I would say requiring SP between the year and the second, and
requiring colon between hour and minute, would probably be a good
change, i.e. something like this:
if (!isdigit(colon[-4]) ||
colon[-3] != ':' ||
!isdigit(colon[-2]) ||
!isdigit(colon[-1]) ||
!isdigit(colon[ 1]) ||
!isdigit(colon[ 2]) ||
colon[3] != ' ')
would be safe enough without increasing the false negatives too
much.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Eric Wong writes: >> Also, doesn't it break "git rebase" (non-interactive), or anything >> that internally runs format-patch to individual files and then runs >> am on each of them, anything that knows that each output file from >> format-patch corresponds to a single change and there is no need to >> split, badly if we do this unconditionally? > > Yes, rebase should probably unescape is_from_line matches. This shouldn't matter for "git rebase", as it only reads from the mbox "From " line to learn the original commit and extract the log message directly from there. But a third-party script that wants to read format-patch output would be forced to upgrade, which is a pain if we make this change unconditionally. >> IOW, shouldn't this be an optional feature to format-patch that >> is triggered by passing a new command line option that currently >> nobody is passing? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Junio C Hamano wrote: > Junio C Hamano writes: > > Eric Wong writes: > > > >> Users have mistakenly copied "From " lines into commit messages > >> in the past, and will certainly make the same mistakes in the > >> future. Since not everyone uses mboxrd, yet, we should at least > >> prevent miss-split mails by always escaping "From " lines based > >> on the check used by mailsplit. > >> > >> mailsplit will not perform unescaping by default, yet, as it > >> could cause further invocations of format-patch from old > >> versions of git to generate bad output. Propagating the mboxo > >> escaping is preferable to miss-split patches. Unescaping may > >> still be performed via "--mboxrd". > > > > As a tool to produce mbox file, quoting like this in format-patch > > output may make sense, I would think, but shouldn't send-email undo > > this when sending individual patches? > > Also, doesn't it break "git rebase" (non-interactive), or anything > that internally runs format-patch to individual files and then runs > am on each of them, anything that knows that each output file from > format-patch corresponds to a single change and there is no need to > split, badly if we do this unconditionally? Yes, rebase should probably unescape is_from_line matches. Anything which spawns an editor should probably warn/reprompt users on is_from_line() matches, too, to prevent user errors from sneaking in. > IOW, shouldn't this be an optional feature to format-patch that is > triggered by passing a new command line option that currently nobody > is passing? I added --pretty=mboxrd as the optional feature for this reason. It'll take a while for people to start using it (or perhaps make it the default in git 3.0). In the meantime, I would prefer extra ">" being injected rather than breaking mailsplit completely. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Junio C Hamano writes: > Eric Wong writes: > >> Users have mistakenly copied "From " lines into commit messages >> in the past, and will certainly make the same mistakes in the >> future. Since not everyone uses mboxrd, yet, we should at least >> prevent miss-split mails by always escaping "From " lines based >> on the check used by mailsplit. >> >> mailsplit will not perform unescaping by default, yet, as it >> could cause further invocations of format-patch from old >> versions of git to generate bad output. Propagating the mboxo >> escaping is preferable to miss-split patches. Unescaping may >> still be performed via "--mboxrd". > > As a tool to produce mbox file, quoting like this in format-patch > output may make sense, I would think, but shouldn't send-email undo > this when sending individual patches? Also, doesn't it break "git rebase" (non-interactive), or anything that internally runs format-patch to individual files and then runs am on each of them, anything that knows that each output file from format-patch corresponds to a single change and there is no need to split, badly if we do this unconditionally? IOW, shouldn't this be an optional feature to format-patch that is triggered by passing a new command line option that currently nobody is passing? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] format-patch: escape "From " lines recognized by mailsplit
On Sun, Jul 24, 2016 at 09:37:57AM +0200, Johannes Schindelin wrote:
> On Sun, 24 Jul 2016, Eric Wong wrote:
>
> > @@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,
> > strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
> > line, linelen);
> > else {
> > - if (pp->fmt == CMIT_FMT_MBOXRD &&
> > - is_mboxrd_from(line, linelen))
> > - strbuf_addch(sb, '>');
> > + switch (pp->fmt) {
> > + case CMIT_FMT_EMAIL:
> > + if (is_from_line(line, linelen))
> > + strbuf_addch(sb, '>');
> > + break;
> > + case CMIT_FMT_MBOXRD:
> > + if (is_mboxrd_from(line, linelen))
> > + strbuf_addch(sb, '>');
> > + break;
> > + default:
> > + break;
> > + }
>
> Sorry to be nitpicking once again; I think this would be conciser (and
> easier to read at least for me) as:
>
> - if (pp->fmt == CMIT_FMT_MBOXRD &&
> - is_mboxrd_from(line, linelen))
> + if ((pp->fmt == CMIT_FMT_MBOXRD &&
> + is_mboxrd_from(line, linelen)) ||
> + (pp->fmt == CMIT_FMT_EMAIL &&
> + is_from_line(line, linelen)))
> strbuf_addch(sb, '>');
Since we are nitpicking, I think:
static int needs_from_quoting(enum cmit_fmt fmt,
const char *line, size_t len)
{
if (fmt == CMIT_FMT_MBOXRD && is_mboxrd_from(line, linelen))
return 1;
if (fmt == CMIT_FMT_EMAIL && is_from_line(line, linelen))
return 1;
return 0;
}
...
if (needs_from_quoting(pp->fmt, line, linelen))
strbuf_addch(sb, '>');
is even nicer. There are lots of alternatives to write the helper
function, and I do not care much which one is chosen. But splitting it
out into a concise "do we need to do this?" query function makes the
flow in pp_remainder much easier to follow.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Junio C Hamano wrote: > As a tool to produce mbox file, quoting like this in format-patch > output may make sense, I would think, but shouldn't send-email undo > this when sending individual patches? Yes, that sounds like a good idea. Thanks. Will followup in a day or two. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit
Johannes Schindelin writes:
> Hi Andreas,
>
> On Sun, 24 Jul 2016, Andreas Schwab wrote:
>
>> Eric Wong writes:
>>
>> > diff --git a/mailinfo.c b/mailinfo.c
>> > index 9f19ca1..0ebd953 100644
>> > --- a/mailinfo.c
>> > +++ b/mailinfo.c
>> > @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
>> >
>> >strbuf_release(&mi->log_message);
>> > }
>> > +
>> > +int is_from_line(const char *line, int len)
>> > +{
>> > + const char *colon;
>> > +
>> > + if (len < 20 || memcmp("From ", line, 5))
>> > + return 0;
>> > +
>> > + colon = line + len - 2;
>> > + line += 5;
>> > + for (;;) {
>> > + if (colon < line)
>> > + return 0;
>> > + if (*--colon == ':')
>> > + break;
>> > + }
>> > +
>> > + if (!isdigit(colon[-4]) ||
>> > + !isdigit(colon[-2]) ||
>> > + !isdigit(colon[-1]) ||
>> > + !isdigit(colon[ 1]) ||
>> > + !isdigit(colon[ 2]))
>> > + return 0;
>> > +
>> > + /* year */
>> > + if (strtol(colon+3, NULL, 10) <= 90)
>> > + return 0;
>> > +
>> > + /* Ok, close enough */
>> > + return 1;
>> > +}
>>
>> Should this be made more strict, like by checking for a space before the
>> year?
>
> This patch only moves the function, so it would be inappropriate to change
> it.
>
> If you want to make it stricter, you will have to submit a separate patch.
You didn't answer my question.
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit
Hi Andreas,
On Sun, 24 Jul 2016, Andreas Schwab wrote:
> Eric Wong writes:
>
> > diff --git a/mailinfo.c b/mailinfo.c
> > index 9f19ca1..0ebd953 100644
> > --- a/mailinfo.c
> > +++ b/mailinfo.c
> > @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
> >
> > strbuf_release(&mi->log_message);
> > }
> > +
> > +int is_from_line(const char *line, int len)
> > +{
> > + const char *colon;
> > +
> > + if (len < 20 || memcmp("From ", line, 5))
> > + return 0;
> > +
> > + colon = line + len - 2;
> > + line += 5;
> > + for (;;) {
> > + if (colon < line)
> > + return 0;
> > + if (*--colon == ':')
> > + break;
> > + }
> > +
> > + if (!isdigit(colon[-4]) ||
> > + !isdigit(colon[-2]) ||
> > + !isdigit(colon[-1]) ||
> > + !isdigit(colon[ 1]) ||
> > + !isdigit(colon[ 2]))
> > + return 0;
> > +
> > + /* year */
> > + if (strtol(colon+3, NULL, 10) <= 90)
> > + return 0;
> > +
> > + /* Ok, close enough */
> > + return 1;
> > +}
>
> Should this be made more strict, like by checking for a space before the
> year?
This patch only moves the function, so it would be inappropriate to change
it.
If you want to make it stricter, you will have to submit a separate patch.
Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] format-patch: escape "From " lines recognized by mailsplit
Hi Eric,
On Sun, 24 Jul 2016, Eric Wong wrote:
> @@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,
> strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
>line, linelen);
> else {
> - if (pp->fmt == CMIT_FMT_MBOXRD &&
> - is_mboxrd_from(line, linelen))
> - strbuf_addch(sb, '>');
> + switch (pp->fmt) {
> + case CMIT_FMT_EMAIL:
> + if (is_from_line(line, linelen))
> + strbuf_addch(sb, '>');
> + break;
> + case CMIT_FMT_MBOXRD:
> + if (is_mboxrd_from(line, linelen))
> + strbuf_addch(sb, '>');
> + break;
> + default:
> + break;
> + }
Sorry to be nitpicking once again; I think this would be conciser (and
easier to read at least for me) as:
- if (pp->fmt == CMIT_FMT_MBOXRD &&
- is_mboxrd_from(line, linelen))
+ if ((pp->fmt == CMIT_FMT_MBOXRD &&
+is_mboxrd_from(line, linelen)) ||
+ (pp->fmt == CMIT_FMT_EMAIL &&
+is_from_line(line, linelen)))
strbuf_addch(sb, '>');
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 1206c48..8fa3982 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1606,4 +1606,18 @@ test_expect_success 'format-patch --pretty=mboxrd' '
> test_cmp expect actual
> '
>
> +test_expect_success 'format-patch From escaping' '
> + cat >msg <<-INPUT_END &&
> + somebody pasted format-patch output into a body
> +
> + From Mon Sep 17 00:00:00 2001
> + INPUT_END
> +
> + C=$(git commit-tree HEAD^^{tree} -p HEAD + git format-patch --stdout -1 $C~1..$C >patch &&
Either "-1 $C" or "$C~1..$C", not both...
> + git grep -h --no-index \
> + ">From " \
> + patch
> +'
> +
> test_done
> --
> EW
Heh, that's a nice Git version ;-)
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit
Eric Wong writes:
> diff --git a/mailinfo.c b/mailinfo.c
> index 9f19ca1..0ebd953 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
>
> strbuf_release(&mi->log_message);
> }
> +
> +int is_from_line(const char *line, int len)
> +{
> + const char *colon;
> +
> + if (len < 20 || memcmp("From ", line, 5))
> + return 0;
> +
> + colon = line + len - 2;
> + line += 5;
> + for (;;) {
> + if (colon < line)
> + return 0;
> + if (*--colon == ':')
> + break;
> + }
> +
> + if (!isdigit(colon[-4]) ||
> + !isdigit(colon[-2]) ||
> + !isdigit(colon[-1]) ||
> + !isdigit(colon[ 1]) ||
> + !isdigit(colon[ 2]))
> + return 0;
> +
> + /* year */
> + if (strtol(colon+3, NULL, 10) <= 90)
> + return 0;
> +
> + /* Ok, close enough */
> + return 1;
> +}
Should this be made more strict, like by checking for a space before the
year?
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Eric Wong writes: > Users have mistakenly copied "From " lines into commit messages > in the past, and will certainly make the same mistakes in the > future. Since not everyone uses mboxrd, yet, we should at least > prevent miss-split mails by always escaping "From " lines based > on the check used by mailsplit. > > mailsplit will not perform unescaping by default, yet, as it > could cause further invocations of format-patch from old > versions of git to generate bad output. Propagating the mboxo > escaping is preferable to miss-split patches. Unescaping may > still be performed via "--mboxrd". As a tool to produce mbox file, quoting like this in format-patch output may make sense, I would think, but shouldn't send-email undo this when sending individual patches? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] mailinfo: extract is_from_line from mailsplit
We will be reusing is_from_line for quoting format-patch output
in the next commit.
Suggested-by: Johannes Schindelin
Signed-off-by: Eric Wong
---
builtin/mailsplit.c | 32 +---
mailinfo.c | 31 +++
mailinfo.h | 1 +
3 files changed, 33 insertions(+), 31 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 3068168..bb8f9c9 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -8,41 +8,11 @@
#include "builtin.h"
#include "string-list.h"
#include "strbuf.h"
+#include "mailinfo.h"
static const char git_mailsplit_usage[] =
"git mailsplit [-d] [-f] [-b] [--keep-cr] -o
[(|)...]";
-static int is_from_line(const char *line, int len)
-{
- const char *colon;
-
- if (len < 20 || memcmp("From ", line, 5))
- return 0;
-
- colon = line + len - 2;
- line += 5;
- for (;;) {
- if (colon < line)
- return 0;
- if (*--colon == ':')
- break;
- }
-
- if (!isdigit(colon[-4]) ||
- !isdigit(colon[-2]) ||
- !isdigit(colon[-1]) ||
- !isdigit(colon[ 1]) ||
- !isdigit(colon[ 2]))
- return 0;
-
- /* year */
- if (strtol(colon+3, NULL, 10) <= 90)
- return 0;
-
- /* Ok, close enough */
- return 1;
-}
-
static struct strbuf buf = STRBUF_INIT;
static int keep_cr;
static int mboxrd;
diff --git a/mailinfo.c b/mailinfo.c
index 9f19ca1..0ebd953 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
strbuf_release(&mi->log_message);
}
+
+int is_from_line(const char *line, int len)
+{
+ const char *colon;
+
+ if (len < 20 || memcmp("From ", line, 5))
+ return 0;
+
+ colon = line + len - 2;
+ line += 5;
+ for (;;) {
+ if (colon < line)
+ return 0;
+ if (*--colon == ':')
+ break;
+ }
+
+ if (!isdigit(colon[-4]) ||
+ !isdigit(colon[-2]) ||
+ !isdigit(colon[-1]) ||
+ !isdigit(colon[ 1]) ||
+ !isdigit(colon[ 2]))
+ return 0;
+
+ /* year */
+ if (strtol(colon+3, NULL, 10) <= 90)
+ return 0;
+
+ /* Ok, close enough */
+ return 1;
+}
diff --git a/mailinfo.h b/mailinfo.h
index 93776a7..c1430a0 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -37,5 +37,6 @@ struct mailinfo {
extern void setup_mailinfo(struct mailinfo *);
extern int mailinfo(struct mailinfo *, const char *msg, const char *patch);
extern void clear_mailinfo(struct mailinfo *);
+int is_from_line(const char *line, int len);
#endif /* MAILINFO_H */
--
EW
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] format-patch: escape "From " lines recognized by mailsplit
Users have mistakenly copied "From " lines into commit messages
in the past, and will certainly make the same mistakes in the
future. Since not everyone uses mboxrd, yet, we should at least
prevent miss-split mails by always escaping "From " lines based
on the check used by mailsplit.
mailsplit will not perform unescaping by default, yet, as it
could cause further invocations of format-patch from old
versions of git to generate bad output. Propagating the mboxo
escaping is preferable to miss-split patches. Unescaping may
still be performed via "--mboxrd".
ref: https://public-inbox.org/git/20160606230248.ga15...@dcvr.yhbt.net/T/#u
Signed-off-by: Eric Wong
---
pretty.c| 16 +---
t/t4014-format-patch.sh | 14 ++
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/pretty.c b/pretty.c
index 9fa42c2..898c0a3 100644
--- a/pretty.c
+++ b/pretty.c
@@ -10,6 +10,7 @@
#include "color.h"
#include "reflog-walk.h"
#include "gpg-interface.h"
+#include "mailinfo.h"
static char *user_format;
static struct cmt_fmt_map {
@@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,
strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
line, linelen);
else {
- if (pp->fmt == CMIT_FMT_MBOXRD &&
- is_mboxrd_from(line, linelen))
- strbuf_addch(sb, '>');
+ switch (pp->fmt) {
+ case CMIT_FMT_EMAIL:
+ if (is_from_line(line, linelen))
+ strbuf_addch(sb, '>');
+ break;
+ case CMIT_FMT_MBOXRD:
+ if (is_mboxrd_from(line, linelen))
+ strbuf_addch(sb, '>');
+ break;
+ default:
+ break;
+ }
strbuf_add(sb, line, linelen);
}
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 1206c48..8fa3982 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1606,4 +1606,18 @@ test_expect_success 'format-patch --pretty=mboxrd' '
test_cmp expect actual
'
+test_expect_success 'format-patch From escaping' '
+ cat >msg <<-INPUT_END &&
+ somebody pasted format-patch output into a body
+
+ From Mon Sep 17 00:00:00 2001
+ INPUT_END
+
+ C=$(git commit-tree HEAD^^{tree} -p HEAD patch &&
+ git grep -h --no-index \
+ ">From " \
+ patch
+'
+
test_done
--
EW
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Johannes Schindelin wrote: > I think that this change: > deserves to live in a separate patch... It would make the real change > stick out better. Good point, I actually wrote a looser check based on is_mboxrd_from before realizing the stricter check from mailsplit would probably be more appropriate to use by default. The following changes since commit 08bb3500a2a718c3c78b0547c68601cafa7a8fd9: Sixth batch of topics for 2.10 (2016-07-19 13:26:16 -0700) are available in the git repository at: git://bogomips.org/git-svn.git mboxo for you to fetch changes up to 4f769f1799db11f135948bf517f2d8d864fa778f: format-patch: escape "From " lines recognized by mailsplit (2016-07-23 21:38:40 +) Eric Wong (2): mailinfo: extract is_from_line from mailsplit format-patch: escape "From " lines recognized by mailsplit builtin/mailsplit.c | 32 +--- mailinfo.c | 31 +++ mailinfo.h | 1 + pretty.c| 16 +--- t/t4014-format-patch.sh | 14 ++ 5 files changed, 60 insertions(+), 34 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit
Hi Eric,
I think that this change:
On Fri, 22 Jul 2016, Eric Wong wrote:
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 3068168..bb8f9c9 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -8,41 +8,11 @@
> #include "builtin.h"
> #include "string-list.h"
> #include "strbuf.h"
> +#include "mailinfo.h"
>
> static const char git_mailsplit_usage[] =
> "git mailsplit [-d] [-f] [-b] [--keep-cr] -o
> [(|)...]";
>
> -static int is_from_line(const char *line, int len)
> -{
> - const char *colon;
> -
> - if (len < 20 || memcmp("From ", line, 5))
> - return 0;
> -
> - colon = line + len - 2;
> - line += 5;
> - for (;;) {
> - if (colon < line)
> - return 0;
> - if (*--colon == ':')
> - break;
> - }
> -
> - if (!isdigit(colon[-4]) ||
> - !isdigit(colon[-2]) ||
> - !isdigit(colon[-1]) ||
> - !isdigit(colon[ 1]) ||
> - !isdigit(colon[ 2]))
> - return 0;
> -
> - /* year */
> - if (strtol(colon+3, NULL, 10) <= 90)
> - return 0;
> -
> - /* Ok, close enough */
> - return 1;
> -}
> -
> static struct strbuf buf = STRBUF_INIT;
> static int keep_cr;
> static int mboxrd;
> diff --git a/mailinfo.c b/mailinfo.c
> index 9f19ca1..0ebd953 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
>
> strbuf_release(&mi->log_message);
> }
> +
> +int is_from_line(const char *line, int len)
> +{
> + const char *colon;
> +
> + if (len < 20 || memcmp("From ", line, 5))
> + return 0;
> +
> + colon = line + len - 2;
> + line += 5;
> + for (;;) {
> + if (colon < line)
> + return 0;
> + if (*--colon == ':')
> + break;
> + }
> +
> + if (!isdigit(colon[-4]) ||
> + !isdigit(colon[-2]) ||
> + !isdigit(colon[-1]) ||
> + !isdigit(colon[ 1]) ||
> + !isdigit(colon[ 2]))
> + return 0;
> +
> + /* year */
> + if (strtol(colon+3, NULL, 10) <= 90)
> + return 0;
> +
> + /* Ok, close enough */
> + return 1;
> +}
> diff --git a/mailinfo.h b/mailinfo.h
> index 93776a7..c1430a0 100644
> --- a/mailinfo.h
> +++ b/mailinfo.h
> @@ -37,5 +37,6 @@ struct mailinfo {
> extern void setup_mailinfo(struct mailinfo *);
> extern int mailinfo(struct mailinfo *, const char *msg, const char *patch);
> extern void clear_mailinfo(struct mailinfo *);
> +int is_from_line(const char *line, int len);
>
> #endif /* MAILINFO_H */
deserves to live in a separate patch... It would make the real change
stick out better.
Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] format-patch: escape "From " lines recognized by mailsplit
Users have mistakenly copied "From " lines into commit messages
in the past, and will certainly make the same mistakes in the
future. Since not everyone uses mboxrd, yet, we should at least
prevent miss-split mails by always escaping "From " lines based
on the check used by mailsplit.
mailsplit will not perform unescaping by default, yet, as it
could cause further invocations of format-patch from old
versions of git to generate bad output. Propagating the mboxo
escaping is preferable to miss-split patches. Unescaping may
still be performed via "--mboxrd".
ref: https://public-inbox.org/git/20160606230248.ga15...@dcvr.yhbt.net/T/#u
Signed-off-by: Eric Wong
---
builtin/mailsplit.c | 32 +---
mailinfo.c | 31 +++
mailinfo.h | 1 +
pretty.c| 16 +---
t/t4014-format-patch.sh | 14 ++
5 files changed, 60 insertions(+), 34 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 3068168..bb8f9c9 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -8,41 +8,11 @@
#include "builtin.h"
#include "string-list.h"
#include "strbuf.h"
+#include "mailinfo.h"
static const char git_mailsplit_usage[] =
"git mailsplit [-d] [-f] [-b] [--keep-cr] -o
[(|)...]";
-static int is_from_line(const char *line, int len)
-{
- const char *colon;
-
- if (len < 20 || memcmp("From ", line, 5))
- return 0;
-
- colon = line + len - 2;
- line += 5;
- for (;;) {
- if (colon < line)
- return 0;
- if (*--colon == ':')
- break;
- }
-
- if (!isdigit(colon[-4]) ||
- !isdigit(colon[-2]) ||
- !isdigit(colon[-1]) ||
- !isdigit(colon[ 1]) ||
- !isdigit(colon[ 2]))
- return 0;
-
- /* year */
- if (strtol(colon+3, NULL, 10) <= 90)
- return 0;
-
- /* Ok, close enough */
- return 1;
-}
-
static struct strbuf buf = STRBUF_INIT;
static int keep_cr;
static int mboxrd;
diff --git a/mailinfo.c b/mailinfo.c
index 9f19ca1..0ebd953 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1035,3 +1035,34 @@ void clear_mailinfo(struct mailinfo *mi)
strbuf_release(&mi->log_message);
}
+
+int is_from_line(const char *line, int len)
+{
+ const char *colon;
+
+ if (len < 20 || memcmp("From ", line, 5))
+ return 0;
+
+ colon = line + len - 2;
+ line += 5;
+ for (;;) {
+ if (colon < line)
+ return 0;
+ if (*--colon == ':')
+ break;
+ }
+
+ if (!isdigit(colon[-4]) ||
+ !isdigit(colon[-2]) ||
+ !isdigit(colon[-1]) ||
+ !isdigit(colon[ 1]) ||
+ !isdigit(colon[ 2]))
+ return 0;
+
+ /* year */
+ if (strtol(colon+3, NULL, 10) <= 90)
+ return 0;
+
+ /* Ok, close enough */
+ return 1;
+}
diff --git a/mailinfo.h b/mailinfo.h
index 93776a7..c1430a0 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -37,5 +37,6 @@ struct mailinfo {
extern void setup_mailinfo(struct mailinfo *);
extern int mailinfo(struct mailinfo *, const char *msg, const char *patch);
extern void clear_mailinfo(struct mailinfo *);
+int is_from_line(const char *line, int len);
#endif /* MAILINFO_H */
diff --git a/pretty.c b/pretty.c
index 9fa42c2..898c0a3 100644
--- a/pretty.c
+++ b/pretty.c
@@ -10,6 +10,7 @@
#include "color.h"
#include "reflog-walk.h"
#include "gpg-interface.h"
+#include "mailinfo.h"
static char *user_format;
static struct cmt_fmt_map {
@@ -1745,9 +1746,18 @@ void pp_remainder(struct pretty_print_context *pp,
strbuf_add_tabexpand(sb, pp->expand_tabs_in_log,
line, linelen);
else {
- if (pp->fmt == CMIT_FMT_MBOXRD &&
- is_mboxrd_from(line, linelen))
- strbuf_addch(sb, '>');
+ switch (pp->fmt) {
+ case CMIT_FMT_EMAIL:
+ if (is_from_line(line, linelen))
+ strbuf_addch(sb, '>');
+ break;
+ case CMIT_FMT_MBOXRD:
+ if (is_mboxrd_from(line, linelen))
+ strbuf_addch(sb, '>');
+ break;
+ default:
+ break;
+ }
strbuf_add(sb, line, linel
Re: [PATCH v2 2/3] mailsplit: support unescaping mboxrd messages
Junio C Hamano wrote: > This just makes me wonder if there is a practical reason why people > would not want this always enabled. I just looked at output from > > $ git log --grep='>>*From ' Missing '^' ? Auto-unescaping in mailsplit might throw off people on older versions of git if reserialized as mail, though. Maybe in a few years, we can consider it. Auto-escaping on the other hand, I think we start doing in --pretty=email by default soon/today. At least for lines matching the stricter is_from_line() check from mailsplit. > in the kernel repository, and I saw no cases where the committer > really wanted to preserve the leading one or more '>' on that line. > No, I didn't go through all of 150+ such commits, but I did check > the couple dozen of them from the recent history. > > Our history also have 5 instances of them, none of which should have > had the leading '>' if the committer were careful. Right, but I'm actually more worried about unescaped /^From / breaking further attempts to split. git log --grep='^From .*:.*:.* .*' ...finds 45 commits in Linux 4.6.1 which might cause incorrect splits. We have 5 of those ourselves. Technically it is backwards-incompatible, but I would rather leave an extra '>' in the commit than break a split. > > +>From the beginning, mbox should have been mboxrd > > Indeed ;-) Yes, I'm really wishing we did this 11 years ago :) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] mailsplit: support unescaping mboxrd messages
Eric Wong writes: > This will allow us to parse the output of --pretty=mboxrd > and the output of other mboxrd generators. > > Signed-off-by: Eric Wong > --- > Documentation/git-mailsplit.txt | 7 ++- > builtin/mailsplit.c | 18 ++ > t/t5100-mailinfo.sh | 31 +++ > t/t5100/0001mboxrd | 4 > t/t5100/0002mboxrd | 5 + > t/t5100/sample.mboxrd | 19 +++ > 6 files changed, 83 insertions(+), 1 deletion(-) > create mode 100644 t/t5100/0001mboxrd > create mode 100644 t/t5100/0002mboxrd > create mode 100644 t/t5100/sample.mboxrd > > diff --git a/Documentation/git-mailsplit.txt b/Documentation/git-mailsplit.txt > index 4d1b871..e3b2a88 100644 > --- a/Documentation/git-mailsplit.txt > +++ b/Documentation/git-mailsplit.txt > @@ -8,7 +8,8 @@ git-mailsplit - Simple UNIX mbox splitter program > SYNOPSIS > > [verse] > -'git mailsplit' [-b] [-f] [-d] [--keep-cr] -o [--] > [(|)...] > +'git mailsplit' [-b] [-f] [-d] [--keep-cr] [--mboxrd] > + -o [--] [(|)...] > > DESCRIPTION > --- > @@ -47,6 +48,10 @@ OPTIONS > --keep-cr:: > Do not remove `\r` from lines ending with `\r\n`. > > +--mboxrd:: > + Input is of the "mboxrd" format and "^>+From " line escaping is > + reversed. This just makes me wonder if there is a practical reason why people would not want this always enabled. I just looked at output from $ git log --grep='>>*From ' in the kernel repository, and I saw no cases where the committer really wanted to preserve the leading one or more '>' on that line. No, I didn't go through all of 150+ such commits, but I did check the couple dozen of them from the recent history. Our history also have 5 instances of them, none of which should have had the leading '>' if the committer were careful. > diff --git a/t/t5100/sample.mboxrd b/t/t5100/sample.mboxrd > new file mode 100644 > index 000..79ad5ae > --- /dev/null > +++ b/t/t5100/sample.mboxrd > @@ -0,0 +1,19 @@ > +From mboxrd Mon Sep 17 00:00:00 2001 > +From: mboxrd writer > +Date: Fri, 9 Jun 2006 00:44:16 -0700 > +Subject: [PATCH] a commit with escaped From lines > + > +>From the beginning, mbox should have been mboxrd Indeed ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] mailsplit: support unescaping mboxrd messages
This will allow us to parse the output of --pretty=mboxrd
and the output of other mboxrd generators.
Signed-off-by: Eric Wong
---
Documentation/git-mailsplit.txt | 7 ++-
builtin/mailsplit.c | 18 ++
t/t5100-mailinfo.sh | 31 +++
t/t5100/0001mboxrd | 4
t/t5100/0002mboxrd | 5 +
t/t5100/sample.mboxrd | 19 +++
6 files changed, 83 insertions(+), 1 deletion(-)
create mode 100644 t/t5100/0001mboxrd
create mode 100644 t/t5100/0002mboxrd
create mode 100644 t/t5100/sample.mboxrd
diff --git a/Documentation/git-mailsplit.txt b/Documentation/git-mailsplit.txt
index 4d1b871..e3b2a88 100644
--- a/Documentation/git-mailsplit.txt
+++ b/Documentation/git-mailsplit.txt
@@ -8,7 +8,8 @@ git-mailsplit - Simple UNIX mbox splitter program
SYNOPSIS
[verse]
-'git mailsplit' [-b] [-f] [-d] [--keep-cr] -o [--]
[(|)...]
+'git mailsplit' [-b] [-f] [-d] [--keep-cr] [--mboxrd]
+ -o [--] [(|)...]
DESCRIPTION
---
@@ -47,6 +48,10 @@ OPTIONS
--keep-cr::
Do not remove `\r` from lines ending with `\r\n`.
+--mboxrd::
+ Input is of the "mboxrd" format and "^>+From " line escaping is
+ reversed.
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 4859ede..3068168 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -45,6 +45,19 @@ static int is_from_line(const char *line, int len)
static struct strbuf buf = STRBUF_INIT;
static int keep_cr;
+static int mboxrd;
+
+static int is_gtfrom(const struct strbuf *buf)
+{
+ size_t min = strlen(">From ");
+ size_t ngt;
+
+ if (buf->len < min)
+ return 0;
+
+ ngt = strspn(buf->buf, ">");
+ return ngt && starts_with(buf->buf + ngt, "From ");
+}
/* Called with the first line (potentially partial)
* already in buf[] -- normally that should begin with
@@ -77,6 +90,9 @@ static int split_one(FILE *mbox, const char *name, int
allow_bare)
strbuf_addch(&buf, '\n');
}
+ if (mboxrd && is_gtfrom(&buf))
+ strbuf_remove(&buf, 0, 1);
+
if (fwrite(buf.buf, 1, buf.len, output) != buf.len)
die_errno("cannot write output");
@@ -271,6 +287,8 @@ int cmd_mailsplit(int argc, const char **argv, const char
*prefix)
keep_cr = 1;
} else if ( arg[1] == 'o' && arg[2] ) {
dir = arg+2;
+ } else if (!strcmp(arg, "--mboxrd")) {
+ mboxrd = 1;
} else if ( arg[1] == '-' && !arg[2] ) {
argp++; /* -- marks end of options */
break;
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 85b3df5..1a5a546 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -111,4 +111,35 @@ test_expect_success 'mailinfo on message with quoted
>From' '
test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.expect quoted-from/msg
'
+test_expect_success 'mailinfo unescapes with --mboxrd' '
+ mkdir mboxrd &&
+ git mailsplit -omboxrd --mboxrd \
+ "$TEST_DIRECTORY"/t5100/sample.mboxrd >last &&
+ test x"$(cat last)" = x2 &&
+ for i in 0001 0002
+ do
+ git mailinfo mboxrd/msg mboxrd/patch \
+ mboxrd/out &&
+ test_cmp "$TEST_DIRECTORY"/t5100/${i}mboxrd mboxrd/msg
+ done &&
+ sp=" " &&
+ echo "From " >expect &&
+ echo "From " >>expect &&
+ echo >> expect &&
+ cat >sp <<-INPUT_END &&
+ From mboxrd Mon Sep 17 00:00:00 2001
+ From: trailing spacer
+ Subject: [PATCH] a commit with trailing space
+
+ From$sp
+ >From$sp
+
+ INPUT_END
+
+ git mailsplit -f2 -omboxrd --mboxrd last &&
+ test x"$(cat last)" = x1 &&
+ git mailinfo mboxrd/msg mboxrd/patch From escaped
+From not mangled but this line should have been escaped
+
diff --git a/t/t5100/0002mboxrd b/t/t5100/0002mboxrd
new file mode 100644
index 000..71343d4
--- /dev/null
+++ b/t/t5100/0002mboxrd
@@ -0,0 +1,5 @@
+ >From unchanged
+ From also unchanged
+no trailing space, no escaping necessary and '>' was intended:
+>From
+
diff --git a/t/t5100/sample.mboxrd b/t/t5100/sample.mboxrd
new file mode 100644
index 000..79ad5ae
--- /dev/null
+++ b/t/t5100/sample.mboxrd
@@ -0,0 +1,19 @@
+From mbo
[PATCH 2/3] mailsplit: support unescaping mboxrd messages
This will allow us to parse the output of --pretty=mboxrd
and the output of other mboxrd generators.
Signed-off-by: Eric Wong
---
Documentation/git-mailsplit.txt | 7 ++-
builtin/mailsplit.c | 23 +++
t/t5100-mailinfo.sh | 13 +
t/t5100/0001mboxrd | 4
t/t5100/0002mboxrd | 3 +++
t/t5100/sample.mboxrd | 17 +
6 files changed, 66 insertions(+), 1 deletion(-)
create mode 100644 t/t5100/0001mboxrd
create mode 100644 t/t5100/0002mboxrd
create mode 100644 t/t5100/sample.mboxrd
diff --git a/Documentation/git-mailsplit.txt b/Documentation/git-mailsplit.txt
index 4d1b871..e3b2a88 100644
--- a/Documentation/git-mailsplit.txt
+++ b/Documentation/git-mailsplit.txt
@@ -8,7 +8,8 @@ git-mailsplit - Simple UNIX mbox splitter program
SYNOPSIS
[verse]
-'git mailsplit' [-b] [-f] [-d] [--keep-cr] -o [--]
[(|)...]
+'git mailsplit' [-b] [-f] [-d] [--keep-cr] [--mboxrd]
+ -o [--] [(|)...]
DESCRIPTION
---
@@ -47,6 +48,10 @@ OPTIONS
--keep-cr::
Do not remove `\r` from lines ending with `\r\n`.
+--mboxrd::
+ Input is of the "mboxrd" format and "^>+From " line escaping is
+ reversed.
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 4859ede..fad871d 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -45,6 +45,7 @@ static int is_from_line(const char *line, int len)
static struct strbuf buf = STRBUF_INIT;
static int keep_cr;
+static regex_t *gtfrom;
/* Called with the first line (potentially partial)
* already in buf[] -- normally that should begin with
@@ -77,6 +78,10 @@ static int split_one(FILE *mbox, const char *name, int
allow_bare)
strbuf_addch(&buf, '\n');
}
+ if (gtfrom && buf.len > (sizeof(">From ") - 1) &&
+ !regexec(gtfrom, buf.buf, 0, 0, 0))
+ strbuf_remove(&buf, 0, 1);
+
if (fwrite(buf.buf, 1, buf.len, output) != buf.len)
die_errno("cannot write output");
@@ -242,6 +247,22 @@ out:
return ret;
}
+static regex_t *gtfrom_prepare(void)
+{
+ static regex_t preg;
+ const char re[] = "^>+From ";
+ int err = regcomp(&preg, re, REG_NOSUB | REG_EXTENDED);
+
+ if (err) {
+ char errbuf[1024];
+ regerror(err, &preg, errbuf, sizeof(errbuf));
+ regfree(&preg);
+ die("Cannot prepare regexp `%s': %s", re, errbuf);
+ }
+
+ return &preg;
+}
+
int cmd_mailsplit(int argc, const char **argv, const char *prefix)
{
int nr = 0, nr_prec = 4, num = 0;
@@ -271,6 +292,8 @@ int cmd_mailsplit(int argc, const char **argv, const char
*prefix)
keep_cr = 1;
} else if ( arg[1] == 'o' && arg[2] ) {
dir = arg+2;
+ } else if (!strcmp(arg, "--mboxrd")) {
+ gtfrom = gtfrom_prepare();
} else if ( arg[1] == '-' && !arg[2] ) {
argp++; /* -- marks end of options */
break;
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 85b3df5..62b442c 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -111,4 +111,17 @@ test_expect_success 'mailinfo on message with quoted
>From' '
test_cmp "$TEST_DIRECTORY"/t5100/quoted-from.expect quoted-from/msg
'
+test_expect_success 'mailinfo unescapes with --mboxrd' '
+ mkdir mboxrd &&
+ git mailsplit -omboxrd --mboxrd \
+ "$TEST_DIRECTORY"/t5100/sample.mboxrd >last &&
+ test x"$(cat last)" = x2 &&
+ for i in 0001 0002
+ do
+ git mailinfo mboxrd/msg mboxrd/patch \
+ mboxrd/out &&
+ test_cmp "$TEST_DIRECTORY"/t5100/${i}mboxrd mboxrd/msg
+ done
+'
+
test_done
diff --git a/t/t5100/0001mboxrd b/t/t5100/0001mboxrd
new file mode 100644
index 000..494ec55
--- /dev/null
+++ b/t/t5100/0001mboxrd
@@ -0,0 +1,4 @@
+From the beginning, mbox should have been mboxrd
+>From escaped
+From not mangled but this line should have been escaped
+
diff --git a/t/t5100/0002mboxrd b/t/t5100/0002mboxrd
new file mode 100644
index 000..3c30b0b
--- /dev/null
+++ b/t/t5100/0002mboxrd
@@ -0,0 +1,3 @@
+ >From unchanged
+ From also unchanged
+
diff --git a/t/t5100/sample.mboxrd b/t/t5100/sample.mboxrd
new file mode 100644
index 000..75de6dd
--- /dev/null
+++ b/t/t5100/sample.mboxrd
@@ -0,0 +1,17 @@
+From mboxrd Mon Sep 17 00:00:00 2001
+From:
[PATCH 10/68] mailsplit: make PATH_MAX buffers dynamic
There are several PATH_MAX-sized buffers in mailsplit, along
with some questionable uses of sprintf. These are not
really of security interest, as local mailsplit pathnames
are not typically under control of an attacker, and you
could generally only overflow a few numbers at the end of a
path that approaches PATH_MAX (a longer path would choke
mailsplit long before). But it does not hurt to be careful,
and as a bonus we lift some limits for systems with
too-small PATH_MAX varibles.
Signed-off-by: Jeff King
---
builtin/mailsplit.c | 34 +++---
1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..104277a 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -98,30 +98,37 @@ static int populate_maildir_list(struct string_list *list,
const char *path)
{
DIR *dir;
struct dirent *dent;
- char name[PATH_MAX];
+ char *name = NULL;
char *subs[] = { "cur", "new", NULL };
char **sub;
+ int ret = -1;
for (sub = subs; *sub; ++sub) {
- snprintf(name, sizeof(name), "%s/%s", path, *sub);
+ free(name);
+ name = xstrfmt("%s/%s", path, *sub);
if ((dir = opendir(name)) == NULL) {
if (errno == ENOENT)
continue;
error("cannot opendir %s (%s)", name, strerror(errno));
- return -1;
+ goto out;
}
while ((dent = readdir(dir)) != NULL) {
if (dent->d_name[0] == '.')
continue;
- snprintf(name, sizeof(name), "%s/%s", *sub,
dent->d_name);
+ free(name);
+ name = xstrfmt("%s/%s", *sub, dent->d_name);
string_list_insert(list, name);
}
closedir(dir);
}
- return 0;
+ ret = 0;
+
+out:
+ free(name);
+ return ret;
}
static int maildir_filename_cmp(const char *a, const char *b)
@@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char
*b)
static int split_maildir(const char *maildir, const char *dir,
int nr_prec, int skip)
{
- char file[PATH_MAX];
- char name[PATH_MAX];
+ char *file = NULL;
FILE *f = NULL;
int ret = -1;
int i;
@@ -161,7 +167,11 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
+ char *name;
+
+ free(file);
+ file = xstrfmt("%s/%s", maildir, list.items[i].string);
+
f = fopen(file, "r");
if (!f) {
error("cannot open mail %s (%s)", file,
strerror(errno));
@@ -173,8 +183,9 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
}
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
split_one(f, name, 1);
+ free(name);
fclose(f);
f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char
*dir,
out:
if (f)
fclose(f);
+ free(file);
string_list_clear(&list, 1);
return ret;
}
@@ -191,7 +203,6 @@ out:
static int split_mbox(const char *file, const char *dir, int allow_bare,
int nr_prec, int skip)
{
- char name[PATH_MAX];
int ret = -1;
int peek;
@@ -218,8 +229,9 @@ static int split_mbox(const char *file, const char *dir,
int allow_bare,
}
while (!file_done) {
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
file_done = split_one(f, name, allow_bare);
+ free(name);
}
if (f != stdin)
--
2.6.0.rc3.454.g204ad51
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/68] mailsplit: fix FILE* leak in split_maildir
If we encounter an error while splitting a maildir, we exit
the function early, leaking the open filehandle. This isn't
a big deal, since we exit the program soon after, but it's
easy enough to be careful.
Signed-off-by: Jeff King
---
builtin/mailsplit.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 8e02ea1..9de06e3 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -150,6 +150,7 @@ static int split_maildir(const char *maildir, const char
*dir,
{
char file[PATH_MAX];
char name[PATH_MAX];
+ FILE *f = NULL;
int ret = -1;
int i;
struct string_list list = STRING_LIST_INIT_DUP;
@@ -160,7 +161,6 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- FILE *f;
snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
f = fopen(file, "r");
if (!f) {
@@ -177,10 +177,13 @@ static int split_maildir(const char *maildir, const char
*dir,
split_one(f, name, 1);
fclose(f);
+ f = NULL;
}
ret = skip;
out:
+ if (f)
+ fclose(f);
string_list_clear(&list, 1);
return ret;
}
--
2.6.0.rc3.454.g204ad51
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic
On Wed, Sep 16, 2015 at 11:13:37AM -0700, Junio C Hamano wrote:
> Jeff King writes:
>
> > + free(file);
> > + file = xstrfmt("%s/%s", maildir, list.items[i].string);
>
> Repeated pattern makes one wonder if a thin wrapper
>
> xstrfmt_to(&file, "%s/%s", maildir, list.items[i].string);
>
> that first frees the existing value and then overwrites is an
> overall win. Perhaps not, as you would (1) initialize the variable
> to NULL before doing a series of xstrfmt_to(), and (2) free the final
> one yourself.
Yeah, exactly. If you want to wrap it up in something that understands
invariants, I think strbuf is the way to go. I dunno. Maybe I should
just have done this whole thing with strbufs.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic
Jeff King writes:
> + free(file);
> + file = xstrfmt("%s/%s", maildir, list.items[i].string);
Repeated pattern makes one wonder if a thin wrapper
xstrfmt_to(&file, "%s/%s", maildir, list.items[i].string);
that first frees the existing value and then overwrites is an
overall win. Perhaps not, as you would (1) initialize the variable
to NULL before doing a series of xstrfmt_to(), and (2) free the final
one yourself.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic
On Wed, Sep 16, 2015 at 06:14:18AM -0400, Jeff King wrote:
> I guess we could get away with always calling free() right before
> assigning (the equivalent of strbuf_reset()), and then rely on exiting
> the loop to "out" to do the final free. And then the result (versus the
> original code, not my patch) would look like:
And here is the whole patch converted to that style. I'm planning to go
with this, as the resulting diff is much smaller and much more clear
that we are touching only the allocations.
I _hope_ the result is pretty easy to understand. There is some subtlety
to the loop assumptions:
- on entering, the pointer is NULL or an allocated buffer to be
recycled; either way, free() is OK
- on leaving, the pointer is either NULL (if we never ran the loop) or
a buffer to be freed. The free() at the end is necessary to handle
this.
That is not too complicated, but it is not an idiom we use elsewhere
(whereas recycled strbufs are). I can switch the whole thing to strbufs
if that's the direction we want to go.
-- >8 --
Subject: [PATCH] mailsplit: make PATH_MAX buffers dynamic
There are several static PATH_MAX-sized buffers in
mailsplit, along with some questionable uses of sprintf.
These are not really of security interest, as local
mailsplit pathnames are not typically under control of an
attacker, and you could generally only overflow a few
numbers at the end of a path that approaches PATH_MAX (a
longer path would choke mailsplit long before). But it does
not hurt to be careful, and as a bonus we lift some limits
for systems with too-small PATH_MAX varibles.
Signed-off-by: Jeff King
---
builtin/mailsplit.c | 34 +++---
1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..104277a 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -98,30 +98,37 @@ static int populate_maildir_list(struct string_list *list,
const char *path)
{
DIR *dir;
struct dirent *dent;
- char name[PATH_MAX];
+ char *name = NULL;
char *subs[] = { "cur", "new", NULL };
char **sub;
+ int ret = -1;
for (sub = subs; *sub; ++sub) {
- snprintf(name, sizeof(name), "%s/%s", path, *sub);
+ free(name);
+ name = xstrfmt("%s/%s", path, *sub);
if ((dir = opendir(name)) == NULL) {
if (errno == ENOENT)
continue;
error("cannot opendir %s (%s)", name, strerror(errno));
- return -1;
+ goto out;
}
while ((dent = readdir(dir)) != NULL) {
if (dent->d_name[0] == '.')
continue;
- snprintf(name, sizeof(name), "%s/%s", *sub,
dent->d_name);
+ free(name);
+ name = xstrfmt("%s/%s", *sub, dent->d_name);
string_list_insert(list, name);
}
closedir(dir);
}
- return 0;
+ ret = 0;
+
+out:
+ free(name);
+ return ret;
}
static int maildir_filename_cmp(const char *a, const char *b)
@@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char
*b)
static int split_maildir(const char *maildir, const char *dir,
int nr_prec, int skip)
{
- char file[PATH_MAX];
- char name[PATH_MAX];
+ char *file = NULL;
FILE *f = NULL;
int ret = -1;
int i;
@@ -161,7 +167,11 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
+ char *name;
+
+ free(file);
+ file = xstrfmt("%s/%s", maildir, list.items[i].string);
+
f = fopen(file, "r");
if (!f) {
error("cannot open mail %s (%s)", file,
strerror(errno));
@@ -173,8 +183,9 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
}
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
split_one(f, name, 1);
+ free(name);
fclose(f);
f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char
*dir,
out:
if (f)
fclose(f);
+ free(file);
string_list_clear(&list, 1);
return ret;
}
@@ -191,7 +203,6 @@ out:
static int split_mbox(const char *file, co
Re: [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic
On Tue, Sep 15, 2015 at 08:51:26PM -0400, Eric Sunshine wrote:
> > if (strbuf_getwholeline(&buf, f, '\n')) {
> > - error("cannot read mail %s (%s)", file,
> > strerror(errno));
> > + error("cannot read mail %s (%s)", file.buf,
> > strerror(errno));
> > goto out;
> > }
> >
> > - sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
> > + name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
> > split_one(f, name, 1);
> > + free(name);
>
> Hmm, why does 'file' become a strbuf which is re-used each time
> through the loop, but 'name' is treated differently and gets
> re-allocated upon each iteration? Why doesn't 'name' deserve the same
> treatment as 'file'?
My thinking was rather the other way around: why doesn't "file" get the
same treatment as "name"?
I generally prefer xstrfmt to strbufs in these patches for two reasons:
1. The result has fewer lines.
2. The variable switches from an array to a pointer, so accessing it
doesn't change. Whereas with a strbuf, you have to s/foo/foo.buf/
wherever it is accessed.
We can do that easily with "name"; we allocate it, use it, and free it.
But the lifetime of "file" crosses the "goto out" boundaries, and so
it's simplest to clean it up in the "out" section. Doing that correctly
with a bare pointer is tricky (you have to re-NULL it every time you
free the old value), whereas the strbuf's invariants make it trivial.
I guess we could get away with always calling free() right before
assigning (the equivalent of strbuf_reset()), and then rely on exiting
the loop to "out" to do the final free. And then the result (versus the
original code, not my patch) would look like:
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..a82dd0d 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char
*b)
static int split_maildir(const char *maildir, const char *dir,
int nr_prec, int skip)
{
- char file[PATH_MAX];
- char name[PATH_MAX];
+ char *file = NULL;
FILE *f = NULL;
int ret = -1;
int i;
@@ -161,7 +167,11 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
+ char *name;
+
+ free(file);
+ file = xstrfmt("%s/%s", maildir, list.items[i].string);
+
f = fopen(file, "r");
if (!f) {
error("cannot open mail %s (%s)", file,
strerror(errno));
@@ -173,8 +183,9 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
}
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
split_one(f, name, 1);
+ free(name);
fclose(f);
f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char
*dir,
out:
if (f)
fclose(f);
+ free(file);
string_list_clear(&list, 1);
return ret;
}
which is not so bad.
Of course this is more allocations per loop than using a strbuf. I doubt
it matters in practice (we are about to fopen() and read into a strbuf,
after all!), but we could also follow the opposite direction and use
strbufs for both.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic
On Tue, Sep 15, 2015 at 11:28 AM, Jeff King wrote:
> There are several static PATH_MAX-sized buffers in
> mailsplit, along with some questionable uses of sprintf.
> These are not really of security interest, as local
> mailsplit pathnames are not typically under control of an
> attacker. But it does not hurt to be careful, and as a
> bonus we lift some limits for systems with too-small
> PATH_MAX varibles.
>
> Signed-off-by: Jeff King
> ---
> diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> index 9de06e3..fb0bc08 100644
> --- a/builtin/mailsplit.c
> +++ b/builtin/mailsplit.c
> @@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char
> *b)
> static int split_maildir(const char *maildir, const char *dir,
> int nr_prec, int skip)
> {
> - char file[PATH_MAX];
> - char name[PATH_MAX];
> + struct strbuf file = STRBUF_INIT;
> FILE *f = NULL;
> int ret = -1;
> int i;
> @@ -161,20 +167,25 @@ static int split_maildir(const char *maildir, const
> char *dir,
> goto out;
>
> for (i = 0; i < list.nr; i++) {
> - snprintf(file, sizeof(file), "%s/%s", maildir,
> list.items[i].string);
> - f = fopen(file, "r");
> + char *name;
> +
> + strbuf_reset(&file);
> + strbuf_addf(&file, "%s/%s", maildir, list.items[i].string);
> +
> + f = fopen(file.buf, "r");
> if (!f) {
> - error("cannot open mail %s (%s)", file,
> strerror(errno));
> + error("cannot open mail %s (%s)", file.buf,
> strerror(errno));
> goto out;
> }
>
> if (strbuf_getwholeline(&buf, f, '\n')) {
> - error("cannot read mail %s (%s)", file,
> strerror(errno));
> + error("cannot read mail %s (%s)", file.buf,
> strerror(errno));
> goto out;
> }
>
> - sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
> + name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
> split_one(f, name, 1);
> + free(name);
Hmm, why does 'file' become a strbuf which is re-used each time
through the loop, but 'name' is treated differently and gets
re-allocated upon each iteration? Why doesn't 'name' deserve the same
treatment as 'file'?
> fclose(f);
> f = NULL;
> @@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char
> *dir,
> out:
> if (f)
> fclose(f);
> + strbuf_release(&file);
> string_list_clear(&list, 1);
> return ret;
> }
> @@ -191,7 +203,6 @@ out:
> static int split_mbox(const char *file, const char *dir, int allow_bare,
> int nr_prec, int skip)
> {
> - char name[PATH_MAX];
> int ret = -1;
> int peek;
>
> @@ -218,8 +229,9 @@ static int split_mbox(const char *file, const char *dir,
> int allow_bare,
> }
>
> while (!file_done) {
> - sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
> + char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
> file_done = split_one(f, name, allow_bare);
> + free(name);
Same question, pretty much: Why not make 'name' a strbuf which is
re-used in the loop? (I don't have a strong preference; I'm just
trying to understand the apparent inconsistency of treatment.)
> }
>
> if (f != stdin)
> --
> 2.6.0.rc2.408.ga2926b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/67] mailsplit: make PATH_MAX buffers dynamic
There are several static PATH_MAX-sized buffers in
mailsplit, along with some questionable uses of sprintf.
These are not really of security interest, as local
mailsplit pathnames are not typically under control of an
attacker. But it does not hurt to be careful, and as a
bonus we lift some limits for systems with too-small
PATH_MAX varibles.
Signed-off-by: Jeff King
---
builtin/mailsplit.c | 46 +-
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 9de06e3..fb0bc08 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -98,30 +98,37 @@ static int populate_maildir_list(struct string_list *list,
const char *path)
{
DIR *dir;
struct dirent *dent;
- char name[PATH_MAX];
+ struct strbuf name = STRBUF_INIT;
char *subs[] = { "cur", "new", NULL };
char **sub;
+ int ret = -1;
for (sub = subs; *sub; ++sub) {
- snprintf(name, sizeof(name), "%s/%s", path, *sub);
- if ((dir = opendir(name)) == NULL) {
+ strbuf_reset(&name);
+ strbuf_addf(&name, "%s/%s", path, *sub);
+ if ((dir = opendir(name.buf)) == NULL) {
if (errno == ENOENT)
continue;
- error("cannot opendir %s (%s)", name, strerror(errno));
- return -1;
+ error("cannot opendir %s (%s)", name.buf,
strerror(errno));
+ goto out;
}
while ((dent = readdir(dir)) != NULL) {
if (dent->d_name[0] == '.')
continue;
- snprintf(name, sizeof(name), "%s/%s", *sub,
dent->d_name);
- string_list_insert(list, name);
+ strbuf_reset(&name);
+ strbuf_addf(&name, "%s/%s", *sub, dent->d_name);
+ string_list_insert(list, name.buf);
}
closedir(dir);
}
- return 0;
+ ret = 0;
+
+out:
+ strbuf_release(&name);
+ return ret;
}
static int maildir_filename_cmp(const char *a, const char *b)
@@ -148,8 +155,7 @@ static int maildir_filename_cmp(const char *a, const char
*b)
static int split_maildir(const char *maildir, const char *dir,
int nr_prec, int skip)
{
- char file[PATH_MAX];
- char name[PATH_MAX];
+ struct strbuf file = STRBUF_INIT;
FILE *f = NULL;
int ret = -1;
int i;
@@ -161,20 +167,25 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
- f = fopen(file, "r");
+ char *name;
+
+ strbuf_reset(&file);
+ strbuf_addf(&file, "%s/%s", maildir, list.items[i].string);
+
+ f = fopen(file.buf, "r");
if (!f) {
- error("cannot open mail %s (%s)", file,
strerror(errno));
+ error("cannot open mail %s (%s)", file.buf,
strerror(errno));
goto out;
}
if (strbuf_getwholeline(&buf, f, '\n')) {
- error("cannot read mail %s (%s)", file,
strerror(errno));
+ error("cannot read mail %s (%s)", file.buf,
strerror(errno));
goto out;
}
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
split_one(f, name, 1);
+ free(name);
fclose(f);
f = NULL;
@@ -184,6 +195,7 @@ static int split_maildir(const char *maildir, const char
*dir,
out:
if (f)
fclose(f);
+ strbuf_release(&file);
string_list_clear(&list, 1);
return ret;
}
@@ -191,7 +203,6 @@ out:
static int split_mbox(const char *file, const char *dir, int allow_bare,
int nr_prec, int skip)
{
- char name[PATH_MAX];
int ret = -1;
int peek;
@@ -218,8 +229,9 @@ static int split_mbox(const char *file, const char *dir,
int allow_bare,
}
while (!file_done) {
- sprintf(name, "%s/%0*d", dir, nr_prec, ++skip);
+ char *name = xstrfmt("%s/%0*d", dir, nr_prec, ++skip);
file_done = split_one(f, name, allow_bare);
+ free(name);
}
if (f != stdin)
--
2.6.0.rc2.408.ga2926b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/67] mailsplit: fix FILE* leak in split_maildir
If we encounter an error while splitting a maildir, we exit
the function early, leaking the open filehandle. This isn't
a big deal, since we exit the program soon after, but it's
easy enough to be careful.
Signed-off-by: Jeff King
---
builtin/mailsplit.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 8e02ea1..9de06e3 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -150,6 +150,7 @@ static int split_maildir(const char *maildir, const char
*dir,
{
char file[PATH_MAX];
char name[PATH_MAX];
+ FILE *f = NULL;
int ret = -1;
int i;
struct string_list list = STRING_LIST_INIT_DUP;
@@ -160,7 +161,6 @@ static int split_maildir(const char *maildir, const char
*dir,
goto out;
for (i = 0; i < list.nr; i++) {
- FILE *f;
snprintf(file, sizeof(file), "%s/%s", maildir,
list.items[i].string);
f = fopen(file, "r");
if (!f) {
@@ -177,10 +177,13 @@ static int split_maildir(const char *maildir, const char
*dir,
split_one(f, name, 1);
fclose(f);
+ f = NULL;
}
ret = skip;
out:
+ if (f)
+ fclose(f);
string_list_clear(&list, 1);
return ret;
}
--
2.6.0.rc2.408.ga2926b9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 05/45] builtin-am: split out mbox/maildir patches with git-mailsplit
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Helped-by: Junio C Hamano
Signed-off-by: Paul Tan
---
builtin/am.c | 107 +--
1 file changed, 104 insertions(+), 3 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index ac172c4..5f3c131 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -8,6 +8,12 @@
#include "exec_cmd.h"
#include "parse-options.h"
#include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+ PATCH_FORMAT_UNKNOWN = 0,
+ PATCH_FORMAT_MBOX
+};
struct am_state {
/* state directory path */
@@ -16,6 +22,9 @@ struct am_state {
/* current and last patch numbers, 1-indexed */
int cur;
int last;
+
+ /* number of digits in patch filename */
+ int prec;
};
/**
@@ -28,6 +37,8 @@ static void am_state_init(struct am_state *state, const char
*dir)
assert(dir);
state->dir = xstrdup(dir);
+
+ state->prec = 4;
}
/**
@@ -117,13 +128,71 @@ static void am_destroy(const struct am_state *state)
}
/**
+ * Splits out individual email patches from `paths`, where each path is either
+ * a mbox file or a Maildir. Returns 0 on success, -1 on failure.
+ */
+static int split_mail_mbox(struct am_state *state, const char **paths)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf last = STRBUF_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_push(&cp.args, "mailsplit");
+ argv_array_pushf(&cp.args, "-d%d", state->prec);
+ argv_array_pushf(&cp.args, "-o%s", state->dir);
+ argv_array_push(&cp.args, "-b");
+ argv_array_push(&cp.args, "--");
+ argv_array_pushv(&cp.args, paths);
+
+ if (capture_command(&cp, &last, 8))
+ return -1;
+
+ state->cur = 1;
+ state->last = strtol(last.buf, NULL, 10);
+
+ return 0;
+}
+
+/**
+ * Splits a list of files/directories into individual email patches. Each path
+ * in `paths` must be a file/directory that is formatted according to
+ * `patch_format`.
+ *
+ * Once split out, the individual email patches will be stored in the state
+ * directory, with each patch's filename being its index, padded to state->prec
+ * digits.
+ *
+ * state->cur will be set to the index of the first mail, and state->last will
+ * be set to the index of the last mail.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int split_mail(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
+{
+ switch (patch_format) {
+ case PATCH_FORMAT_MBOX:
+ return split_mail_mbox(state, paths);
+ default:
+ die("BUG: invalid patch_format");
+ }
+ return -1;
+}
+
+/**
* Setup a new am session for applying patches
*/
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
{
if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
die_errno(_("failed to create directory '%s'"), state->dir);
+ if (split_mail(state, patch_format, paths) < 0) {
+ am_destroy(state);
+ die(_("Failed to split patches."));
+ }
+
/*
* NOTE: Since the "next" and "last" files determine if an am_state
* session is in progress, they should be written last.
@@ -159,9 +228,25 @@ static void am_run(struct am_state *state)
am_destroy(state);
}
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg,
int unset)
+{
+ int *opt_value = opt->value;
+
+ if (!strcmp(arg, "mbox"))
+ *opt_value = PATCH_FORMAT_MBOX;
+ else
+ return error(_("Invalid value for --patch-format: %s"), arg);
+ return 0;
+}
+
int cmd_am(int argc, const char **argv, const char *prefix)
{
struct am_state state;
+ int patch_format = PATCH_FORMAT_UNKNOWN;
const char * const usage[] = {
N_("git am [options] [(|)...]"),
@@ -169,6 +254,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
};
struct option options[] = {
[PATCH v6 05/45] builtin-am: split out mbox/maildir patches with git-mailsplit
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Helped-by: Junio C Hamano
Signed-off-by: Paul Tan
---
builtin/am.c | 107 +--
1 file changed, 104 insertions(+), 3 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index ac172c4..5f3c131 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -8,6 +8,12 @@
#include "exec_cmd.h"
#include "parse-options.h"
#include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+ PATCH_FORMAT_UNKNOWN = 0,
+ PATCH_FORMAT_MBOX
+};
struct am_state {
/* state directory path */
@@ -16,6 +22,9 @@ struct am_state {
/* current and last patch numbers, 1-indexed */
int cur;
int last;
+
+ /* number of digits in patch filename */
+ int prec;
};
/**
@@ -28,6 +37,8 @@ static void am_state_init(struct am_state *state, const char
*dir)
assert(dir);
state->dir = xstrdup(dir);
+
+ state->prec = 4;
}
/**
@@ -117,13 +128,71 @@ static void am_destroy(const struct am_state *state)
}
/**
+ * Splits out individual email patches from `paths`, where each path is either
+ * a mbox file or a Maildir. Returns 0 on success, -1 on failure.
+ */
+static int split_mail_mbox(struct am_state *state, const char **paths)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf last = STRBUF_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_push(&cp.args, "mailsplit");
+ argv_array_pushf(&cp.args, "-d%d", state->prec);
+ argv_array_pushf(&cp.args, "-o%s", state->dir);
+ argv_array_push(&cp.args, "-b");
+ argv_array_push(&cp.args, "--");
+ argv_array_pushv(&cp.args, paths);
+
+ if (capture_command(&cp, &last, 8))
+ return -1;
+
+ state->cur = 1;
+ state->last = strtol(last.buf, NULL, 10);
+
+ return 0;
+}
+
+/**
+ * Splits a list of files/directories into individual email patches. Each path
+ * in `paths` must be a file/directory that is formatted according to
+ * `patch_format`.
+ *
+ * Once split out, the individual email patches will be stored in the state
+ * directory, with each patch's filename being its index, padded to state->prec
+ * digits.
+ *
+ * state->cur will be set to the index of the first mail, and state->last will
+ * be set to the index of the last mail.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int split_mail(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
+{
+ switch (patch_format) {
+ case PATCH_FORMAT_MBOX:
+ return split_mail_mbox(state, paths);
+ default:
+ die("BUG: invalid patch_format");
+ }
+ return -1;
+}
+
+/**
* Setup a new am session for applying patches
*/
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
{
if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
die_errno(_("failed to create directory '%s'"), state->dir);
+ if (split_mail(state, patch_format, paths) < 0) {
+ am_destroy(state);
+ die(_("Failed to split patches."));
+ }
+
/*
* NOTE: Since the "next" and "last" files determine if an am_state
* session is in progress, they should be written last.
@@ -159,9 +228,25 @@ static void am_run(struct am_state *state)
am_destroy(state);
}
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg,
int unset)
+{
+ int *opt_value = opt->value;
+
+ if (!strcmp(arg, "mbox"))
+ *opt_value = PATCH_FORMAT_MBOX;
+ else
+ return error(_("Invalid value for --patch-format: %s"), arg);
+ return 0;
+}
+
int cmd_am(int argc, const char **argv, const char *prefix)
{
struct am_state state;
+ int patch_format = PATCH_FORMAT_UNKNOWN;
const char * const usage[] = {
N_("git am [options] [(|)...]"),
@@ -169,6 +254,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
};
struct option options[] = {
[PATCH v5 05/44] builtin-am: split out mbox/maildir patches with git-mailsplit
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Helped-by: Junio C Hamano
Signed-off-by: Paul Tan
---
builtin/am.c | 107 +--
1 file changed, 104 insertions(+), 3 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 292f793..ae93e0e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -8,6 +8,12 @@
#include "exec_cmd.h"
#include "parse-options.h"
#include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+ PATCH_FORMAT_UNKNOWN = 0,
+ PATCH_FORMAT_MBOX
+};
struct am_state {
/* state directory path */
@@ -16,6 +22,9 @@ struct am_state {
/* current and last patch numbers, 1-indexed */
int cur;
int last;
+
+ /* number of digits in patch filename */
+ int prec;
};
/**
@@ -28,6 +37,8 @@ static void am_state_init(struct am_state *state, const char
*dir)
assert(dir);
state->dir = xstrdup(dir);
+
+ state->prec = 4;
}
/**
@@ -120,13 +131,71 @@ static void am_destroy(const struct am_state *state)
}
/**
+ * Splits out individual email patches from `paths`, where each path is either
+ * a mbox file or a Maildir. Returns 0 on success, -1 on failure.
+ */
+static int split_mail_mbox(struct am_state *state, const char **paths)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf last = STRBUF_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_push(&cp.args, "mailsplit");
+ argv_array_pushf(&cp.args, "-d%d", state->prec);
+ argv_array_pushf(&cp.args, "-o%s", state->dir);
+ argv_array_push(&cp.args, "-b");
+ argv_array_push(&cp.args, "--");
+ argv_array_pushv(&cp.args, paths);
+
+ if (capture_command(&cp, &last, 8))
+ return -1;
+
+ state->cur = 1;
+ state->last = strtol(last.buf, NULL, 10);
+
+ return 0;
+}
+
+/**
+ * Splits a list of files/directories into individual email patches. Each path
+ * in `paths` must be a file/directory that is formatted according to
+ * `patch_format`.
+ *
+ * Once split out, the individual email patches will be stored in the state
+ * directory, with each patch's filename being its index, padded to state->prec
+ * digits.
+ *
+ * state->cur will be set to the index of the first mail, and state->last will
+ * be set to the index of the last mail.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int split_mail(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
+{
+ switch (patch_format) {
+ case PATCH_FORMAT_MBOX:
+ return split_mail_mbox(state, paths);
+ default:
+ die("BUG: invalid patch_format");
+ }
+ return -1;
+}
+
+/**
* Setup a new am session for applying patches
*/
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
{
if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
die_errno(_("failed to create directory '%s'"), state->dir);
+ if (split_mail(state, patch_format, paths) < 0) {
+ am_destroy(state);
+ die(_("Failed to split patches."));
+ }
+
/*
* NOTE: Since the "next" and "last" files determine if an am_state
* session is in progress, they should be written last.
@@ -162,9 +231,25 @@ static void am_run(struct am_state *state)
am_destroy(state);
}
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg,
int unset)
+{
+ int *opt_value = opt->value;
+
+ if (!strcmp(arg, "mbox"))
+ *opt_value = PATCH_FORMAT_MBOX;
+ else
+ return error(_("Invalid value for --patch-format: %s"), arg);
+ return 0;
+}
+
int cmd_am(int argc, const char **argv, const char *prefix)
{
struct am_state state;
+ int patch_format = PATCH_FORMAT_UNKNOWN;
const char * const usage[] = {
N_("git am [options] [(|)...]"),
@@ -172,6 +257,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)
};
struct option options[] = {
[PATCH v4 05/44] builtin-am: split out mbox/maildir patches with git-mailsplit
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Helped-by: Junio C Hamano
Signed-off-by: Paul Tan
---
Notes:
v4
* The term "patch" was overloaded to mean "the RFC2822 mail" and the
"patch diff". To avoid confusion, use "mail" to refer to the RFC2822
mail, and thus split_patches() has been renamed to split_mail().
* An argv_array is used for `paths` instead of a string_list to avoid
clunkiness with string_list_item in later patches.
builtin/am.c | 107 +--
1 file changed, 104 insertions(+), 3 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 5b4e9af..136ccc6 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -8,6 +8,12 @@
#include "exec_cmd.h"
#include "parse-options.h"
#include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+ PATCH_FORMAT_UNKNOWN = 0,
+ PATCH_FORMAT_MBOX
+};
struct am_state {
/* state directory path */
@@ -16,6 +22,9 @@ struct am_state {
/* current and last patch numbers, 1-indexed */
int cur;
int last;
+
+ /* number of digits in patch filename */
+ int prec;
};
/**
@@ -28,6 +37,8 @@ static void am_state_init(struct am_state *state, const char
*dir)
assert(dir);
state->dir = xstrdup(dir);
+
+ state->prec = 4;
}
/**
@@ -120,13 +131,71 @@ static void am_destroy(const struct am_state *state)
}
/**
+ * Splits out individual email patches from `paths`, where each path is either
+ * a mbox file or a Maildir. Returns 0 on success, -1 on failure.
+ */
+static int split_mail_mbox(struct am_state *state, const char **paths)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf last = STRBUF_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_push(&cp.args, "mailsplit");
+ argv_array_pushf(&cp.args, "-d%d", state->prec);
+ argv_array_pushf(&cp.args, "-o%s", state->dir);
+ argv_array_push(&cp.args, "-b");
+ argv_array_push(&cp.args, "--");
+ argv_array_pushv(&cp.args, paths);
+
+ if (capture_command(&cp, &last, 8))
+ return -1;
+
+ state->cur = 1;
+ state->last = strtol(last.buf, NULL, 10);
+
+ return 0;
+}
+
+/**
+ * Splits a list of files/directories into individual email patches. Each path
+ * in `paths` must be a file/directory that is formatted according to
+ * `patch_format`.
+ *
+ * Once split out, the individual email patches will be stored in the state
+ * directory, with each patch's filename being its index, padded to state->prec
+ * digits.
+ *
+ * state->cur will be set to the index of the first mail, and state->last will
+ * be set to the index of the last mail.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int split_mail(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
+{
+ switch (patch_format) {
+ case PATCH_FORMAT_MBOX:
+ return split_mail_mbox(state, paths);
+ default:
+ die("BUG: invalid patch_format");
+ }
+ return -1;
+}
+
+/**
* Setup a new am session for applying patches
*/
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+ const char **paths)
{
if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
die_errno(_("failed to create directory '%s'"), state->dir);
+ if (split_mail(state, patch_format, paths) < 0) {
+ am_destroy(state);
+ die(_("Failed to split patches."));
+ }
+
/*
* NOTE: Since the "next" and "last" files determine if an am_state
* session is in progress, they should be written last.
@@ -162,9 +231,25 @@ static void am_run(struct am_state *state)
am_destroy(state);
}
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg,
int unset)
+{
+ int *opt_value = opt->value;
+
+ if (!strcmp(arg, "mbox"))
+ *opt_value = PATCH_FORMAT_MBOX;
+ else
+ return error(_("Invalid value for --patch-forma
Re: [PATCH/WIP v3 05/31] am: split out mbox/maildir patches with git-mailsplit
Paul Tan writes:
> @@ -111,13 +122,69 @@ static void am_destroy(const struct am_state *state)
> }
>
> /**
> + * Splits out individual patches from `paths`, where each path is either a
> mbox
> + * file or a Maildir. Return 0 on success, -1 on failure.
> + */
"Splits" and then "Return"? Be consistent.
> +static int split_patches_mbox(struct am_state *state, struct string_list
> *paths)
> +{
> ...
> +}
Looks straightforward ;-)
> +/**
> + * parse_options() callback that validates and sets opt->value to the
> + * PATCH_FORMAT_* enum value corresponding to `arg`.
> + */
> +static int parse_opt_patchformat(const struct option *opt, const char *arg,
> int unset)
> +{
> + int *opt_value = opt->value;
> +
> + if (!strcmp(arg, "mbox"))
> + *opt_value = PATCH_FORMAT_MBOX;
> + else
> + return -1;
> + return 0;
> +}
> +
> static struct am_state state;
> +static int opt_patch_format;
>
> static const char * const am_usage[] = {
> N_("git am [options] [(|)...]"),
> @@ -156,6 +239,8 @@ static const char * const am_usage[] = {
> };
>
> static struct option am_options[] = {
> + OPT_CALLBACK(0, "patch-format", &opt_patch_format, N_("format"),
> + N_("format the patch(es) are in"), parse_opt_patchformat),
> OPT_END()
> };
Looking good ;-).
Just FYI, you do not have to make am_options[], and the variables
that are referenced from there, e.g. opt_patch_format, etc., global
variables (instead you can have them all in the function scope of
cmd_am()).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/WIP v3 05/31] am: split out mbox/maildir patches with git-mailsplit
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Signed-off-by: Paul Tan
---
Notes:
v3
* Moved the TODO comment to the previous patch
builtin/am.c | 104 +--
1 file changed, 101 insertions(+), 3 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index af68c51..e9a3687 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -8,6 +8,12 @@
#include "exec_cmd.h"
#include "parse-options.h"
#include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+ PATCH_FORMAT_UNKNOWN = 0,
+ PATCH_FORMAT_MBOX
+};
struct am_state {
/* state directory path */
@@ -16,6 +22,9 @@ struct am_state {
/* current and last patch numbers, 1-indexed */
int cur;
int last;
+
+ /* number of digits in patch filename */
+ int prec;
};
/**
@@ -26,6 +35,8 @@ static void am_state_init(struct am_state *state)
memset(state, 0, sizeof(*state));
strbuf_init(&state->dir, 0);
+
+ state->prec = 4;
}
/**
@@ -111,13 +122,69 @@ static void am_destroy(const struct am_state *state)
}
/**
+ * Splits out individual patches from `paths`, where each path is either a mbox
+ * file or a Maildir. Return 0 on success, -1 on failure.
+ */
+static int split_patches_mbox(struct am_state *state, struct string_list
*paths)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct string_list_item *item;
+ struct strbuf last = STRBUF_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_push(&cp.args, "mailsplit");
+ argv_array_pushf(&cp.args, "-d%d", state->prec);
+ argv_array_pushf(&cp.args, "-o%s", state->dir.buf);
+ argv_array_push(&cp.args, "-b");
+ argv_array_push(&cp.args, "--");
+
+ for_each_string_list_item(item, paths)
+ argv_array_push(&cp.args, item->string);
+
+ if (capture_command(&cp, &last, 8))
+ return -1;
+
+ state->cur = 1;
+ state->last = strtol(last.buf, NULL, 10);
+
+ return 0;
+}
+
+/**
+ * Splits out individual patches, of patch_format, contained within paths.
+ * These patches will be stored in the state directory, with each patch's
+ * filename being its index, padded to state->prec digits. state->cur will be
+ * set to the index of the first patch, and state->last will be set to the
+ * index of the last patch.
+ *
+ * Returns 0 on success, -1 on failure.
+ */
+static int split_patches(struct am_state *state, enum patch_format
patch_format,
+ struct string_list *paths)
+{
+ switch (patch_format) {
+ case PATCH_FORMAT_MBOX:
+ return split_patches_mbox(state, paths);
+ default:
+ die("BUG: invalid patch_format");
+ }
+ return -1;
+}
+
+/**
* Setup a new am session for applying patches
*/
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+ struct string_list *paths)
{
if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
die_errno(_("failed to create directory '%s'"), state->dir.buf);
+ if (split_patches(state, patch_format, paths) < 0) {
+ am_destroy(state);
+ die(_("Failed to split patches."));
+ }
+
write_file(am_path(state, "next"), 1, "%d", state->cur);
write_file(am_path(state, "last"), 1, "%d", state->last);
@@ -148,7 +215,23 @@ static void am_run(struct am_state *state)
am_destroy(state);
}
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg,
int unset)
+{
+ int *opt_value = opt->value;
+
+ if (!strcmp(arg, "mbox"))
+ *opt_value = PATCH_FORMAT_MBOX;
+ else
+ return -1;
+ return 0;
+}
+
static struct am_state state;
+static int opt_patch_format;
static const char * const am_usage[] = {
N_("git am [options] [(|)...]"),
@@ -156,6 +239,8 @@ static const char * const am_usage[] = {
};
static struct option am_options[] = {
+ OPT_CALLBACK(0, "patch-format", &opt_patch_format, N_("format"),
+ N_
Re: [PATCH/WIP v2 05/19] am: split out mbox/maildir patches with git-mailsplit
On Fri, Jun 12, 2015 at 1:45 AM, Stefan Beller wrote:
> On Thu, Jun 11, 2015 at 3:21 AM, Paul Tan wrote:
>> @@ -138,13 +202,33 @@ static void am_next(struct am_state *state)
>> */
>> static void am_run(struct am_state *state)
>> {
>> - while (state->cur <= state->last)
>> + while (state->cur <= state->last) {
>> +
>> + /* TODO: Patch application not implemented yet */
>> +
>> am_next(state);
>> + }
>
> When reviewing the previous patch I did look at this loop for awhile confused,
> if you want to apply patches in am_next(state) and thought there might be
> a better approach.
>
> Maybe you want to move this chunk with the TODO into the previous patch,
> so it's clear after reading the documentation of am_run, that the actual am is
> missing there.
Ah right, this is a mistake. This comment should be in the previous patch.
Thanks,
Paul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP v2 05/19] am: split out mbox/maildir patches with git-mailsplit
On Thu, Jun 11, 2015 at 3:21 AM, Paul Tan wrote:
> git-am.sh supports mbox, stgit and mercurial patches. Re-implement
> support for splitting out mbox/maildirs using git-mailsplit, while also
> implementing the framework required to support other patch formats in
> the future.
>
> Re-implement support for the --patch-format option (since a5a6755
> (git-am foreign patch support: introduce patch_format, 2009-05-27)) to
> allow the user to choose between the different patch formats.
>
> Signed-off-by: Paul Tan
> ---
>
> Notes:
> v2
>
> * Declare int opt_patchformat as static.
>
> * Fix up indentation style for the switch()
>
> builtin/am.c | 107
> ---
> 1 file changed, 103 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index f061d21..5198a8e 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -8,6 +8,12 @@
> #include "exec_cmd.h"
> #include "parse-options.h"
> #include "dir.h"
> +#include "run-command.h"
> +
> +enum patch_format {
> + PATCH_FORMAT_UNKNOWN = 0,
> + PATCH_FORMAT_MBOX
> +};
>
> struct am_state {
> /* state directory path */
> @@ -16,6 +22,9 @@ struct am_state {
> /* current and last patch numbers, 1-indexed */
> int cur;
> int last;
> +
> + /* number of digits in patch filename */
> + int prec;
> };
>
> /**
> @@ -26,6 +35,7 @@ static void am_state_init(struct am_state *state)
> memset(state, 0, sizeof(*state));
>
> strbuf_init(&state->dir, 0);
> + state->prec = 4;
> }
>
> /**
> @@ -111,13 +121,67 @@ static void am_destroy(const struct am_state *state)
> }
>
> /**
> + * Splits out individual patches from `paths`, where each path is either a
> mbox
> + * file or a Maildir. Return 0 on success, -1 on failure.
> + */
> +static int split_patches_mbox(struct am_state *state, struct string_list
> *paths)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct string_list_item *item;
> + struct strbuf last = STRBUF_INIT;
> +
> + cp.git_cmd = 1;
> + argv_array_push(&cp.args, "mailsplit");
> + argv_array_pushf(&cp.args, "-d%d", state->prec);
> + argv_array_pushf(&cp.args, "-o%s", state->dir.buf);
> + argv_array_push(&cp.args, "-b");
> + argv_array_push(&cp.args, "--");
> +
> + for_each_string_list_item(item, paths)
> + argv_array_push(&cp.args, item->string);
> +
> + if (capture_command(&cp, &last, 8))
> + return -1;
> +
> + state->cur = 1;
> + state->last = strtol(last.buf, NULL, 10);
> +
> + return 0;
> +}
> +
> +/**
> + * Splits out individual patches, of patch_format, contained within paths.
> + * These patches will be stored in the state directory, with each patch's
> + * filename being its index, padded to state->prec digits. state->cur will be
> + * set to the index of the first patch, and state->last will be set to the
> + * index of the last patch. Returns 0 on success, -1 on failure.
> + */
> +static int split_patches(struct am_state *state, enum patch_format
> patch_format,
> + struct string_list *paths)
> +{
> + switch (patch_format) {
> + case PATCH_FORMAT_MBOX:
> + return split_patches_mbox(state, paths);
> + default:
> + die("BUG: invalid patch_format");
> + }
> + return -1;
> +}
> +
> +/**
> * Setup a new am session for applying patches
> */
> -static void am_setup(struct am_state *state)
> +static void am_setup(struct am_state *state, enum patch_format patch_format,
> + struct string_list *paths)
> {
> if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
> die_errno(_("failed to create directory '%s'"),
> state->dir.buf);
>
> + if (split_patches(state, patch_format, paths) < 0) {
> + am_destroy(state);
> + die(_("Failed to split patches."));
> + }
> +
> write_file(am_path(state, "next"), 1, "%d", state->cur);
>
> write_file(am_path(state, "last"), 1, "%d", state->last);
> @@ -138,13 +202,33 @@ static void am_next(struct am_state *state)
> */
> static void am_run(struct am_state *state)
> {
> - while (state->cur <= state->
[PATCH/WIP v2 05/19] am: split out mbox/maildir patches with git-mailsplit
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Signed-off-by: Paul Tan
---
Notes:
v2
* Declare int opt_patchformat as static.
* Fix up indentation style for the switch()
builtin/am.c | 107 ---
1 file changed, 103 insertions(+), 4 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index f061d21..5198a8e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -8,6 +8,12 @@
#include "exec_cmd.h"
#include "parse-options.h"
#include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+ PATCH_FORMAT_UNKNOWN = 0,
+ PATCH_FORMAT_MBOX
+};
struct am_state {
/* state directory path */
@@ -16,6 +22,9 @@ struct am_state {
/* current and last patch numbers, 1-indexed */
int cur;
int last;
+
+ /* number of digits in patch filename */
+ int prec;
};
/**
@@ -26,6 +35,7 @@ static void am_state_init(struct am_state *state)
memset(state, 0, sizeof(*state));
strbuf_init(&state->dir, 0);
+ state->prec = 4;
}
/**
@@ -111,13 +121,67 @@ static void am_destroy(const struct am_state *state)
}
/**
+ * Splits out individual patches from `paths`, where each path is either a mbox
+ * file or a Maildir. Return 0 on success, -1 on failure.
+ */
+static int split_patches_mbox(struct am_state *state, struct string_list
*paths)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct string_list_item *item;
+ struct strbuf last = STRBUF_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_push(&cp.args, "mailsplit");
+ argv_array_pushf(&cp.args, "-d%d", state->prec);
+ argv_array_pushf(&cp.args, "-o%s", state->dir.buf);
+ argv_array_push(&cp.args, "-b");
+ argv_array_push(&cp.args, "--");
+
+ for_each_string_list_item(item, paths)
+ argv_array_push(&cp.args, item->string);
+
+ if (capture_command(&cp, &last, 8))
+ return -1;
+
+ state->cur = 1;
+ state->last = strtol(last.buf, NULL, 10);
+
+ return 0;
+}
+
+/**
+ * Splits out individual patches, of patch_format, contained within paths.
+ * These patches will be stored in the state directory, with each patch's
+ * filename being its index, padded to state->prec digits. state->cur will be
+ * set to the index of the first patch, and state->last will be set to the
+ * index of the last patch. Returns 0 on success, -1 on failure.
+ */
+static int split_patches(struct am_state *state, enum patch_format
patch_format,
+ struct string_list *paths)
+{
+ switch (patch_format) {
+ case PATCH_FORMAT_MBOX:
+ return split_patches_mbox(state, paths);
+ default:
+ die("BUG: invalid patch_format");
+ }
+ return -1;
+}
+
+/**
* Setup a new am session for applying patches
*/
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+ struct string_list *paths)
{
if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
die_errno(_("failed to create directory '%s'"), state->dir.buf);
+ if (split_patches(state, patch_format, paths) < 0) {
+ am_destroy(state);
+ die(_("Failed to split patches."));
+ }
+
write_file(am_path(state, "next"), 1, "%d", state->cur);
write_file(am_path(state, "last"), 1, "%d", state->last);
@@ -138,13 +202,33 @@ static void am_next(struct am_state *state)
*/
static void am_run(struct am_state *state)
{
- while (state->cur <= state->last)
+ while (state->cur <= state->last) {
+
+ /* TODO: Patch application not implemented yet */
+
am_next(state);
+ }
am_destroy(state);
}
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg,
int unset)
+{
+ int *opt_value = opt->value;
+
+ if (!strcmp(arg, "mbox"))
+ *opt_value = PATCH_FORMAT_MBOX;
+ else
+ return -1;
+ return 0;
+}
+
static struct am_state state;
+static int opt_patch_format;
static
Re: [PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit
On Fri, May 29, 2015 at 7:05 AM, Eric Sunshine wrote:
> On Wed, May 27, 2015 at 9:33 AM, Paul Tan wrote:
>> @@ -128,13 +190,32 @@ static void am_next(struct am_state *state)
>> */
>> +/**
>> + * parse_options() callback that validates and sets opt->value to the
>> + * PATCH_FORMAT_* enum value corresponding to `arg`.
>> + */
>> +static int parse_opt_patchformat(const struct option *opt, const char *arg,
>> int unset)
>> +{
>> + int *opt_value = opt->value;
>> +
>> + if (!strcmp(arg, "mbox"))
>> + *opt_value = PATCH_FORMAT_MBOX;
>> + else
>> + return -1;
>> + return 0;
>> +}
>> +
>> struct am_state state;
>> +int opt_patch_format;
>
> Should these two variables be static?
Whoops. Yes, they should.
Thanks,
Paul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit
On Wed, May 27, 2015 at 9:33 AM, Paul Tan wrote:
> git-am.sh supports mbox, stgit and mercurial patches. Re-implement
> support for splitting out mbox/maildirs using git-mailsplit, while also
> implementing the framework required to support other patch formats in
> the future.
>
> Re-implement support for the --patch-format option (since a5a6755
> (git-am foreign patch support: introduce patch_format, 2009-05-27)) to
> allow the user to choose between the different patch formats.
>
> Signed-off-by: Paul Tan
> ---
> @@ -128,13 +190,32 @@ static void am_next(struct am_state *state)
> */
> +/**
> + * parse_options() callback that validates and sets opt->value to the
> + * PATCH_FORMAT_* enum value corresponding to `arg`.
> + */
> +static int parse_opt_patchformat(const struct option *opt, const char *arg,
> int unset)
> +{
> + int *opt_value = opt->value;
> +
> + if (!strcmp(arg, "mbox"))
> + *opt_value = PATCH_FORMAT_MBOX;
> + else
> + return -1;
> + return 0;
> +}
> +
> struct am_state state;
> +int opt_patch_format;
Should these two variables be static?
> static const char * const am_usage[] = {
> N_("git am [options] [(|)...]"),
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Signed-off-by: Paul Tan
---
builtin/am.c | 104 ---
1 file changed, 100 insertions(+), 4 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 6c9..9c7b058 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -6,11 +6,18 @@
#include "cache.h"
#include "parse-options.h"
#include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+ PATCH_FORMAT_UNKNOWN = 0,
+ PATCH_FORMAT_MBOX
+};
struct am_state {
struct strbuf dir;/* state directory path */
int cur; /* current patch number */
int last; /* last patch number */
+ int prec; /* number of digits in patch filename */
};
/**
@@ -21,6 +28,7 @@ static void am_state_init(struct am_state *state)
memset(state, 0, sizeof(*state));
strbuf_init(&state->dir, 0);
+ state->prec = 4;
}
/**
@@ -101,13 +109,67 @@ static void am_destroy(const struct am_state *state)
}
/**
+ * Splits out individual patches from `paths`, where each path is either a mbox
+ * file or a Maildir. Return 0 on success, -1 on failure.
+ */
+static int split_patches_mbox(struct am_state *state, struct string_list
*paths)
+{
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct string_list_item *item;
+ struct strbuf last = STRBUF_INIT;
+
+ cp.git_cmd = 1;
+ argv_array_push(&cp.args, "mailsplit");
+ argv_array_pushf(&cp.args, "-d%d", state->prec);
+ argv_array_pushf(&cp.args, "-o%s", state->dir.buf);
+ argv_array_push(&cp.args, "-b");
+ argv_array_push(&cp.args, "--");
+
+ for_each_string_list_item(item, paths)
+ argv_array_push(&cp.args, item->string);
+
+ if (capture_command(&cp, &last, 8))
+ return -1;
+
+ state->cur = 1;
+ state->last = strtol(last.buf, NULL, 10);
+
+ return 0;
+}
+
+/**
+ * Splits out individual patches, of patch_format, contained within paths.
+ * These patches will be stored in the state directory, with each patch's
+ * filename being its index, padded to state->prec digits. state->cur will be
+ * set to the index of the first patch, and state->last will be set to the
+ * index of the last patch. Returns 0 on success, -1 on failure.
+ */
+static int split_patches(struct am_state *state, enum patch_format
patch_format,
+ struct string_list *paths)
+{
+ switch (patch_format) {
+ case PATCH_FORMAT_MBOX:
+ return split_patches_mbox(state, paths);
+ default:
+ die("BUG: invalid patch_format");
+ }
+ return -1;
+}
+
+/**
* Setup a new am session for applying patches
*/
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+ struct string_list *paths)
{
if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
die_errno(_("failed to create directory '%s'"), state->dir.buf);
+ if (split_patches(state, patch_format, paths) < 0) {
+ am_destroy(state);
+ die(_("Failed to split patches."));
+ }
+
write_file(am_path(state, "next"), 1, "%d", state->cur);
write_file(am_path(state, "last"), 1, "%d", state->last);
@@ -128,13 +190,32 @@ static void am_next(struct am_state *state)
*/
static void am_run(struct am_state *state)
{
- while (state->cur <= state->last)
+ while (state->cur <= state->last) {
+ /* patch application not implemented yet */
+
am_next(state);
+ }
am_destroy(state);
}
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg,
int unset)
+{
+ int *opt_value = opt->value;
+
+ if (!strcmp(arg, "mbox"))
+ *opt_value = PATCH_FORMAT_MBOX;
+ else
+ return -1;
+ return 0;
+}
+
struct am_state state;
+int opt_patch_format;
static const char * const am_usage[] = {
N_("git am [options] [
[PATCH] mailsplit: remove unnecessary unlink(2) call
The output file hasn't been created at this point, yet, so there is no
need to delete it when exiting early.
Suggested-by: Jeff King
Signed-off-by: Rene Scharfe
---
Original thread: http://thread.gmane.org/gmane.comp.version-control.git/255140
builtin/mailsplit.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 763cda0..8e02ea1 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -59,7 +59,6 @@ static int split_one(FILE *mbox, const char *name, int
allow_bare)
int is_bare = !is_from_line(buf.buf, buf.len);
if (is_bare && !allow_bare) {
- unlink(name);
fprintf(stderr, "corrupt mailbox\n");
exit(1);
}
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
