Re: [PATCH 1/2] git-am foreign patch support: format autodetection

[Giuseppe Bilotta <giuseppe.bilotta@gmail.com>]

On Tue, May 26, 2009 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> This patch is the first step towards the introduction of a framework to
>> allow git-am to import patches not in mailbox format.
>>
>> Currently detected formats are
>>   * the mailbox format itself, which is assumed by default if input is
>>     form stdin
>>   * Mercurial's output from 'hg export'
>>   * Stacked Git's output from 'stg export' with the default export
>>     template; StGIT patch series are also detected and expanded.
>
> I personally do not trust "autodetection" (especially done by others ;-),
> and prefer to have an explicit override by the users, but that aside...

No problem. --patch-format or just --format ?

>        if test -z "$patch_format" && {
>           test $# = 0 || test "x$1" = x-
>        }
>        then
>                patch_format=mbox
>        else
>                patch_format=$(guess_patch_format)
>        fi
>
> Having this extra logic inside the main codeflow makes it extremely harder
> to read; have it in a separate shell function.

I assume you mean the patch format detection, yes?

>> +# a single non-stdin argument was passed, check if it's a StGit patch series
>> +# index by checking if the first line begins with '# This series'
>> +                     {
>> +                             read l1
>> +                             case "$l1" in
>> +                             '# This series '*)
>> +# replace the argument list with the files listed in the series index,
>> +# prefixing them with the series index dirname, skipping comment lines
>
> Can the "series-index-name" file begin with '-' (which would affect the
> way how 'set "@"' works in the loop below)?  A standard trick would be to
> do something like this.
>
>        series_index="$1"
>        shift ;# discard
>        set x
>        while ...
>        do
>                set "$@" another
>        done
>        shift ;# discard 'x' protection

Ah, good point. I'll do it that way.

>> +     # (which is not stdin) to try to understand the format.
>> +     if test $patch_format = none
>
> I do not understand this duplication and inconsistency.  Why have the
> detection in two places?

It's not in two places. The first part sets the patch format only if
we are either reading from stdin or have been passed a stgit patch
series. Otherwise, we still don't know what we're getting, so now we
inspect the first patch to see what format it's in. (Consider for
example the case of appication of a StGIT patch which is not part of a
series.)

>> +     case "$patch_format" in
>> +     mbox)
>> +             git mailsplit -d"$prec" -o"$dotest" -b -- "$@" > "$dotest/last" ||  {
>> +                     rm -fr "$dotest"
>> +                     exit 1
>> +             }
>> +             ;;
>> +     *)
>> +             echo "Patch format $patch_format is not currently handled, sorry"
>>               exit 1
>
> No fixing broken "Subject:" line for your format here?

No, I put that in the second patch, because it was a different thing
(patch processing as opposed to format detection).


-- 
Giuseppe "Oblomov" Bilotta