Re: [Qemu-devel] [PATCH v6 3/8] scripts: Submit changes while updating linux headers

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Mon, Aug 24, 2015 at 03:08:33PM +0100, Peter Maydell wrote:
>On 24 August 2015 at 13:03, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote:
>> This submits changes with formatted commit log while updating Linux
>> headers using scripts/update-linux-headers.sh.
>>
>> Signed-off-by: Gavin Shan <gwshan@linux.vent.ibm.com>
>
>Thanks for writing a patch for this.
>

Thanks for your review.

>> ---
>>  scripts/update-linux-headers.sh | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
>> index 18daabe..451b739 100755
>> --- a/scripts/update-linux-headers.sh
>> +++ b/scripts/update-linux-headers.sh
>> @@ -63,6 +63,25 @@ cp_virtio() {
>>      fi
>>  }
>>
>> +submit_change() {
>> +    from=$1
>> +    to=$2
>> +    if ! [ -e $to/include/qemu-common.h ]; then
>
>An error message about why we're bailing out might be nice.
>Also, it would be better to tell the user the output directory
>isn't valid before we spend all the time building the
>kernel headers, rather than afterwards...
>

Yes, I'll add one error message like below:

            echo "$to is not QEMU source directory, skip submitting changes"

Note that the output directory hasn't to be QEMU source directory in
original implementation, but I don't know the use cases to have invalid
QEMU source directory. If you're sure to change the behaviour, the
check here should be done at the beginning of this script as you
suggested and the above error message would be something like below
then:

            echo "Invalid QEMU source directory ($to) specified"

>> +        exit 3
>> +    fi
>> +
>> +    cd $from
>> +    version=$(make -s kernelversion)
>> +    subject="Sync Linux headers from kernel $version"
>> +    message=$(git log --oneline -1)
>> +    cd -
>
>I think 'cd -' is a bashism. Better to use
>   version=$(make -C $from -s kernelversion)
>   message = $(cd $from && git log ...)
>etc.
>

Right. I'll change accordingly.

>> +    cd $to
>> +    name=$(git config --get user.name)
>> +    email=$(git config --get user.email)
>> +    git commit -a -m "$subject" -m "$message" -m "Signed-off-by: $name <$email>"
>
>Is git commit's --signoff option not present on all the git
>versions we care about?
>
>
>The commit message could be made a bit more verbose. I'd suggest
>something like:
>
>   Synchronize Linux headers from kernel $version
>
>   Synchronize the Linux headers from kernel version $version
>   (commit $commithash).
>
>   This commit was created automatically by update-linux-headers.sh.
>

We needn't care about if user.name and user.email are existing or not.
If they're invalid, the commit log needs to be fixed manually. Or just
to give explicit message like below to remind users to fix it? Anyway,
the commit log isn't complete without correct name/email in SOB if
I'm correct.

    name=$(git config --get user.name)
    email=$(git config --get user.email)
    if ! [ "$name" ]; then
        name="FIXME"
    fi
    if ! [ '$(echo "$email" | grep -v -e '@' > /dev/null)' ]; then
        email="FIXME"
    fi


Thanks,
Gavin