Giuseppe Bilotta [off-list ref] 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?
On Sun, May 24, 2009 at 10:49 PM, Junio C Hamano [off-list ref] wrote:
Giuseppe Bilotta [off-list ref] writes:
quoted
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.
quoted
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
Heya,
On Sun, May 24, 2009 at 23:43, Giuseppe Bilotta
[off-list ref] 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
On Sun, May 24, 2009 at 11:55 PM, Sverre Rabbelier [off-list ref] wrote:
Heya,
On Sun, May 24, 2009 at 23:43, Giuseppe Bilotta
[off-list ref] wrote:
quoted
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