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

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: May 25, 2009, 22:49 UTC
Message-ID: <cb7bb73a0905251549s49d7fc28ge63b12e52029dd0b@mail.gmail.com>
In-Reply-To: <7vk544u8hx.fsf@alter.siamese.dyndns.org>

On Tue, May 26, 2009 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> 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 ?

Show 11 quoted lines

>        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?

Show 21 quoted lines

>> +# 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.

Show 5 quoted lines

>> +     # (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.)

Show 12 quoted lines

>> +     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