[PATCH 0/2] StGit patch series import

[PATCH 0/2] StGit patch series import

[Giuseppe Bilotta]

This small patch series implements support for Stacked Git patch
series import.

The first commit adds support for StGit patches to mailinfo, which is
required because StGit's default export template puts the From: line
between the subject and the body.

The second commit makes git-am autodetect an StGit patch series index
(when it's the only file passed to it) and proceeds to import the
patches indicated in the series.

Giuseppe Bilotta (2):
  mailinfo: handle StGit patches
  git-am: support StGit patch series

 builtin-mailinfo.c |   18 ++++++++++++++++++
 git-am.sh          |   22 ++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

Re: [PATCH 0/2] StGit patch series import

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> This small patch series implements support for Stacked Git patch
> series import.
>
> The first commit adds support for StGit patches to mailinfo, which is
> required because StGit's default export template puts the From: line
> between the subject and the body.

This problem description makes it sound as if we always expect From: to
come before Subject: in the mailbox, and reject the input if they come in
a different order, which would be a bug.  Fixing it would not be limited
to supporting StGIT generated patch email.

But a quick glance at the actual patch makes me suspect that is not what
you are doing.  You are feeding something that is not a mailbox at all to
the mailinfo and _unconditionally_ extract the information according to
StGIT rules.

That's a bad taste.

At least, add a "this is not a mailbox, but is a StGIT formatted file, so
please extract info according to the StGIT rule, not the mailbox rule"
option, and

  (1) have a parameter to mailinfo() to trigger your new codepath only
      when the option is given; or

  (2) have a separate function "stgitinfo()" not "mailinfo()" that perhaps
      largely share the logic with the original "mailinfo()" function, and
      call that when the option is given; or even

  (3) have a separate _program_ that knows how to extract information from
      such an input file;

so that normal mailinfo invocation does not mishandle input that is _not_
StGIT output.

> The second commit makes git-am autodetect an StGit patch series index
> (when it's the only file passed to it) and proceeds to import the
> patches indicated in the series.

And that change would be a good place to decide to pass that "This is not
a mailbox but is a StGIT output" option to the updated mailinfo program
(or the new "stgitinfo" program).

What is the larger picture workflow that this new feature is expected to
help?  A project takes patches not in e-mail form but in a directory full
of files uploaded via scp/sftp with the StGIT series file and individual
StGIT patches that are pointed by the series file contained within?

I do not use StGIT anymore, so I do not remember how flexible its export
template mechanism is, nor how widely people use non-default templates,
but I have wonder about two and half things.

 - I am assuming that your patch won't be able to read the StGIT output if
   the uploader used non-default export template, so such a project needs
   to ask the uploaders to use the default template.

   If that is the case, why not ask them to use a custom template that
   generates one single valid mailbox that stores the patches in the right
   order?  That can be processed with stock "git am"; in addition, the
   output can be fed not just to "git".  Any other SCM that can work with
   e-mail based patchflow can use it.

 - Such a project can allow users to use random export templates as long
   as the template used to export the series is indentifiable (perhaps by
   including that template itself in the upload).  Your mailinfo patch
   needs to be extended to reverse what the export template did, and it
   really shouldn't be in the normal mailinfo() codepath.  The right
   approach would become something like (3) above, i.e. separate
   "StGITinfo" program called from "git am" if that is what you shoot for.

 - If StGIT is used by the project to such an extent to allow series
   directory upload, shouldn't the receiving end be also using StGIT to
   import the series, instead of running "git am" anyway?

Re: [PATCH 0/2] StGit patch series import

[Giuseppe Bilotta]

On Sun, May 24, 2009 at 10:49 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> This small patch series implements support for Stacked Git patch
>> series import.
>>
>> The first commit adds support for StGit patches to mailinfo, which is
>> required because StGit's default export template puts the From: line
>> between the subject and the body.
>
> This problem description makes it sound as if we always expect From: to
> come before Subject: in the mailbox, and reject the input if they come in
> a different order, which would be a bug.  Fixing it would not be limited
> to supporting StGIT generated patch email.

I should probably have added the information that the subject in StGIT
patches is _not_ prefixed by 'Subject: ', it's just placed as-is. So
it's not a matter of ordering.

> But a quick glance at the actual patch makes me suspect that is not what
> you are doing.  You are feeding something that is not a mailbox at all to
> the mailinfo and _unconditionally_ extract the information according to
> StGIT rules.
>
> That's a bad taste.
>
> At least, add a "this is not a mailbox, but is a StGIT formatted file, so
> please extract info according to the StGIT rule, not the mailbox rule"
> option, and
>
>  (1) have a parameter to mailinfo() to trigger your new codepath only
>      when the option is given; or

[snip]

> so that normal mailinfo invocation does not mishandle input that is _not_
> StGIT output.

When I started coding this feature, I had some thoughts about this,
and my initial choice was to implement it following the suggestions
you mentioned. However, after thinking about it a while I realized the
following: the new code-path is taken only if the file does not start
with (what looks like) a mail header. If the file is not a StGIT patch
exported with the default template, the info extraction will fail
somewhere else (e.g. because no author or no diff is found).

So in the end I decided to go the much simpler way of the patches I sent.

>> The second commit makes git-am autodetect an StGit patch series index
>> (when it's the only file passed to it) and proceeds to import the
>> patches indicated in the series.
>
> And that change would be a good place to decide to pass that "This is not
> a mailbox but is a StGIT output" option to the updated mailinfo program
> (or the new "stgitinfo" program).

That was my initial thought too, but then I realized that having the
'heuristics' (although a very braindead one) in mailinfo makes more
sense because otherwise StGIT patch autodetection would only work when
applying a whole series, and not when applying a single (or a few)
patches.

> What is the larger picture workflow that this new feature is expected to
> help?  A project takes patches not in e-mail form but in a directory full
> of files uploaded via scp/sftp with the StGIT series file and individual
> StGIT patches that are pointed by the series file contained within?

Sort of.  The specifics is that there's a guy developing a DIB engine
for Wine and he's using StGIT to handle the patches on top of the
official Wine git tree. Periodically he zips the patch series
(index+patches) and attaches it to the relevant bugzilla entry.

> I do not use StGIT anymore, so I do not remember how flexible its export
> template mechanism is, nor how widely people use non-default templates,
> but I have wonder about two and half things.
>
>  - I am assuming that your patch won't be able to read the StGIT output if
>   the uploader used non-default export template, so such a project needs
>   to ask the uploaders to use the default template.

Well, in the use-case that triggered my need for the StGIT import,
that was the case already.

>   If that is the case, why not ask them to use a custom template that
>   generates one single valid mailbox that stores the patches in the right
>   order?  That can be processed with stock "git am"; in addition, the
>   output can be fed not just to "git".  Any other SCM that can work with
>   e-mail based patchflow can use it.

We have actually asked him to use plain git instead of StGIT, but he's
more comfortable with the latter. We could ask him to fix the header,
yes. I'm not too optimistic about a positive response.

>  - Such a project can allow users to use random export templates as long
>   as the template used to export the series is indentifiable (perhaps by
>   including that template itself in the upload).  Your mailinfo patch
>   needs to be extended to reverse what the export template did, and it
>   really shouldn't be in the normal mailinfo() codepath.  The right
>   approach would become something like (3) above, i.e. separate
>   "StGITinfo" program called from "git am" if that is what you shoot for.

Oh, in my grand plan of things git am should be able to handle all
kind of foreign patches (svn and the patches sent by Bram Moolenar
uses for vim being top candidates). (Of course it would be appropriate
to rename it to something else then.) Sadly, I quickly discovered that
my file and string manipulation-fu in C is not really what I would
call 'strong'.

(And since I needed it fast, I went the easy rather than the formally
correct way.)

>  - If StGIT is used by the project to such an extent to allow series
>   directory upload, shouldn't the receiving end be also using StGIT to
>   import the series, instead of running "git am" anyway?

As I mentioned, Wine uses plain git, and we've tried asking this
(non-core) developer to expose a standard git tree. But he finds StGIT
much more comfortable for the task.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/2] StGit patch series import

[Sverre Rabbelier]

Heya,

On Sun, May 24, 2009 at 23:43, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> As I mentioned, Wine uses plain git, and we've tried asking this
> (non-core) developer to expose a standard git tree. But he finds StGIT
> much more comfortable for the task.

Silly question, doesn't StGit have an export option? Methinks if all
the developer has to do is run 'stgit export' and then attach those
patches instead of the raw patches, it'd be really lame of him not to
do at least that?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 0/2] StGit patch series import

[Giuseppe Bilotta]

On Sun, May 24, 2009 at 11:55 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Sun, May 24, 2009 at 23:43, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>> As I mentioned, Wine uses plain git, and we've tried asking this
>> (non-core) developer to expose a standard git tree. But he finds StGIT
>> much more comfortable for the task.
>
> Silly question, doesn't StGit have an export option? Methinks if all
> the developer has to do is run 'stgit export' and then attach those
> patches instead of the raw patches, it'd be really lame of him not to
> do at least that?

That's what he does. The problem is that StGIT export exports a patch
series using a given template which (by default) is not in mailbox
format.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/2] StGit patch series import

[Sverre Rabbelier]

Heya,

On Mon, May 25, 2009 at 00:04, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> That's what he does. The problem is that StGIT export exports a patch
> series using a given template which (by default) is not in mailbox
> format.

Ah, so you're patching the wrong command then :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 0/2] StGit patch series import

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> That was my initial thought too, but then I realized that having the
> 'heuristics' (although a very braindead one) in mailinfo makes more
> sense because otherwise StGIT patch autodetection would only work when
> applying a whole series, and not when applying a single (or a few)
> patches.

The above is very selfish---what if somebody else wanted to add a feature
to grok a non-mailbox input to the same codepath, and it is not a StGIT
patch?

That is what I called "bad taste".

The same comment may apply to the rest of your response.  "This hack is
good enough for _my_ use case; I do not care if my change makes life
harder for others to build on top of my patch" is not what I want to see.

Re: [PATCH 0/2] StGit patch series import

[Giuseppe Bilotta]

On Mon, May 25, 2009 at 12:02 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> That was my initial thought too, but then I realized that having the
>> 'heuristics' (although a very braindead one) in mailinfo makes more
>> sense because otherwise StGIT patch autodetection would only work when
>> applying a whole series, and not when applying a single (or a few)
>> patches.
>
> The above is very selfish---what if somebody else wanted to add a feature
> to grok a non-mailbox input to the same codepath, and it is not a StGIT
> patch?
>
> That is what I called "bad taste".
>
> The same comment may apply to the rest of your response.  "This hack is
> good enough for _my_ use case; I do not care if my change makes life
> harder for others to build on top of my patch" is not what I want to see.

Sorry, that's totally not the idea was trying to convey. In fact, just
after sending the email I went back to the code to look for a better
solution (I should have probably also made the first try a RFC).

I did feel it was necessary to explain the reasons for the simplicity
of the patches, especially since it gave me the chance to raises some
points that I thought were worth discussing, such as the matter of
where to place patch type autodetection.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/2] StGit patch series import

[Sverre Rabbelier]

Heya,

On Mon, May 25, 2009 at 00:18, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> Sorry, that's totally not the idea was trying to convey. In fact, just
> after sending the email I went back to the code to look for a better
> solution (I should have probably also made the first try a RFC).

Perhaps a more elegant solution is a script that munges a hg/stgit/svn
patch into a mailbox format before feeding it to git-am? 'git munge'
is free :D

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 0/2] StGit patch series import

[Giuseppe Bilotta]

On Mon, May 25, 2009 at 12:28 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Mon, May 25, 2009 at 00:18, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>> Sorry, that's totally not the idea was trying to convey. In fact, just
>> after sending the email I went back to the code to look for a better
>> solution (I should have probably also made the first try a RFC).
>
> Perhaps a more elegant solution is a script that munges a hg/stgit/svn
> patch into a mailbox format before feeding it to git-am? 'git munge'
> is free :D

This is an interesting solution. And git-am could call git-munge
automatically before feeding the patches to mailinfo, replacing
  git mailinfo $keep $utf8 "$dotest/msg" "$dotest/patch" \
      <"$dotest/$msgnum" >"$dotest/info"
with
  git munge "$dotest/$msgnum" | git mailinfo etc

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/2] StGit patch series import

[Sverre Rabbelier]

Heya,

On Mon, May 25, 2009 at 00:53, Giuseppe Bilotta
<giuseppe.bilotta@gmail.com> wrote:
> This is an interesting solution. And git-am could call git-munge
> automatically before feeding the patches to mailinfo, replacing
>  git mailinfo $keep $utf8 "$dotest/msg" "$dotest/patch" \
>      <"$dotest/$msgnum" >"$dotest/info"
> with
>  git munge "$dotest/$msgnum" | git mailinfo etc

I was thinking the other way around, so that 'git am' doesn't need to
be modified.

Behind the scenes, git munge "$dotest/$msgnum" does something like:
1. munge the file so that it's in the appropriate format
2. dispatch to 'git am' with the munged file
3. profit

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 0/2] StGit patch series import

[Giuseppe Bilotta]

On Mon, May 25, 2009 at 12:57 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Heya,
>
> On Mon, May 25, 2009 at 00:53, Giuseppe Bilotta
> <giuseppe.bilotta@gmail.com> wrote:
>> This is an interesting solution. And git-am could call git-munge
>> automatically before feeding the patches to mailinfo, replacing
>>  git mailinfo $keep $utf8 "$dotest/msg" "$dotest/patch" \
>>      <"$dotest/$msgnum" >"$dotest/info"
>> with
>>  git munge "$dotest/$msgnum" | git mailinfo etc
>
> I was thinking the other way around, so that 'git am' doesn't need to
> be modified.
>
> Behind the scenes, git munge "$dotest/$msgnum" does something like:
> 1. munge the file so that it's in the appropriate format
> 2. dispatch to 'git am' with the munged file
> 3. profit

But then the name munge would not be descriptive of what the command
does. If git am is to be left untouched, I would rather then have
something like git import-patches that accepts patches in 'any'
format, expanding StGIT series into the respective patches and
transforming everything into mbox format, invoking am on the final
result. But I see no particular reason why git-am shouldn't be able to
handle all this by itself


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH 0/2] StGit patch series import

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> But then the name munge would not be descriptive of what the command
> does. If git am is to be left untouched, I would rather then have
> something like git import-patches that accepts patches in 'any'
> format,...

There is no need to introduce any new program, I think.

Nobody is against your teaching the "git am" callchain to accept things
other than Berkeley mbox.  "git am" is "import-patches" in that sense
already.

You can add a conditional (perhaps triggered by a "the patches are in this
format" command line option) to "git am" to alter the way it splits the
input into individual pieces of e-mail, named 0001, 0002, 0003, ..., and
leaves the total number of message in $dotest/last.

Currently we call "git-mailsplit" to do all of that; your new conditional
will take StGIT export, and instead of calling "git-mailsplit", read the
series file and process individual patch files into 0001, 0002,... that
are in proper mbox format (i.e. you would need to fix the "Subject: " less
title line when you do this), and leave the total number of patches in
$dotest/last file.

With that change, the main loop that iterates 'while test "$this" -le
"$last"' does not have to change, right?

We could for example add RMAIL format support in the same way.

Re: [PATCH 0/2] StGit patch series import

[Giuseppe Bilotta]

On Mon, May 25, 2009 at 8:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> But then the name munge would not be descriptive of what the command
>> does. If git am is to be left untouched, I would rather then have
>> something like git import-patches that accepts patches in 'any'
>> format,...
>
> There is no need to introduce any new program, I think.

I'm not so sure about this, see below.

> Nobody is against your teaching the "git am" callchain to accept things
> other than Berkeley mbox.  "git am" is "import-patches" in that sense
> already.

I thought as much 8-)

> You can add a conditional (perhaps triggered by a "the patches are in this
> format" command line option) to "git am"

I'd rather have autodetection than command line options, at least as
long as it can be done reliably enough.

> to alter the way it splits the
> input into individual pieces of e-mail, named 0001, 0002, 0003, ..., and
> leaves the total number of message in $dotest/last.
> Currently we call "git-mailsplit" to do all of that; your new conditional
> will take StGIT export, and instead of calling "git-mailsplit", read the
> series file and process individual patch files into 0001, 0002,... that
> are in proper mbox format (i.e. you would need to fix the "Subject: " less
> title line when you do this), and leave the total number of patches in
> $dotest/last file.

Hm. It does make sense to have mail conversion at 'mailsplit' time.
The preprocessing would (1) explode any series, if present (2) convert
the patch from whatever format they are in into mbox format (3) put
them in place. (1) is what my git am patch does, so what we need is
essentially (2). For efficiency we could assume that the user does not
pass patches of different formats to us, so we peek at the first patch
and treat all of them as that kind of patch.

I do suspect that patch format conversion _does_ require an additional
program, in the sense that not doing so would make the git-am script
grow, and I'm not sure it would be very efficient. Is core git C+sh
only or can we rely on some Perl stuff? It'd probably be more
appropriate for this kind of job.

> With that change, the main loop that iterates 'while test "$this" -le
> "$last"' does not have to change, right?

Well, my proposed change git-munge | git-mailinfo would not be that
big of a change, but it does make sense to make the conversion early
on, as I mentioned above. It also makes it easier to debug the
conversion, as the result is kept in the dotest directory.

> We could for example add RMAIL format support in the same way.

No idea what that is.

-- 
Giuseppe "Oblomov" Bilotta