Re: [Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 26/10/17 19:51, Darren Kenny wrote:
> On Thu, Oct 26, 2017 at 07:18:24PM +1100, Alexey Kardashevskiy wrote:
>> On 26/10/17 18:13, Darren Kenny wrote:
>>> Hi Alexey,
>>>
>>> On Thu, Oct 26, 2017 at 12:34:45PM +1100, Alexey Kardashevskiy wrote:
>>>> The new git-submodule.sh script writes .git-submodule-status to
>>>> the source directory every time no matter what. This makes it conditional.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v2:
>>>> * fixed "status" branch too
>>>> ---
>>>> scripts/git-submodule.sh | 15 ++++++++++-----
>>>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
>>>> index d8fbc7e47e..ae038d2e58 100755
>>>> --- a/scripts/git-submodule.sh
>>>> +++ b/scripts/git-submodule.sh
>>>> @@ -23,16 +23,21 @@ then
>>>>     exit 1
>>>> fi
>>>>
>>>> +substat_tmp=$(mktemp)
>>>> +
>>>> case "$command" in
>>>> status)
>>>>     test -f "$substat" || exit 1
>>>> -    trap "rm -f ${substat}.tmp" EXIT
>>>> -    git submodule status $modules > "${substat}.tmp"
>>>> -    diff "${substat}" "${substat}.tmp" >/dev/null
>>>> -    exit $?
>>>> +    git submodule status $modules > "$substat_tmp"
>>>> +    diff "${substat_tmp}" "${substat}" > /dev/null
>>>>     ;;
>>>> update)
>>>>     git submodule update --init $modules 1>/dev/null 2>&1
>>>> -    git submodule status $modules > "${substat}"
>>>> +    git submodule status $modules > "$substat_tmp"
>>>> +    diff "${substat_tmp}" "${substat}" || mv "${substat_tmp}"
>>>> "${substat}"
>>>
>>> Maybe you intended this, but the diff output here will be output to
>>> the screen - if you don't mean this to happen you might want to
>>> redirect the output.
>>
>>
>> Well, if I do:
>>
>> diff "${substat_tmp}" "${substat}" 1>/dev/null 2>&1 || mv "${substat_tmp}"
>> "${substat}"
>>
>> mv: inter-device move failed: '/tmp/tmp.gfcsXCSv4W' to
>> '.git-submodule-status'; unable to remove target: Read-only file system
>>
>>
>> and with my current variant it is:
>>
>>
>>  GIT     ui/keycodemapdb dtc
>> 1d0
>> <  558cd81bdd432769b59bff01240c44f82cfb1a9d dtc (v1.4.4)
>> 2a2
>>>  558cd81bdd432769b59bff01240c44f82cfb1a9d dtc (v1.4.4)
>> mv: inter-device move failed: '/tmp/tmp.4ol9mymsZj' to
>> '.git-submodule-status'; unable to remove target: Read-only file system
>>
>>
> 
> That's strange behaviour for adding a redirect... Maybe it's your
> use of 1>/dev/null instead of just >/dev/null.
> 
> TBH, /tmp should not be read-only in a normally running system.


It is not, this is why I am changing the script to write to /tmp instead of
source directory.


> 
> To avoid the redirect at all then maybe use 'diff -q' instead.


I do not want to make diff silent, I want it to scream actually.


>> which gives some clue about what is going on (I swapped 2 lines in
>> .git-submodule-status to trigger this).
>>
>>
>>>
>>> From other lines it looks like you would prefer it wasn't output.
>>>
>>>>     ;;
>>>> esac
>>>> +
>>>> +myret=$?
>>>
>>> Any small change is likely to break the value of myret here.
>>>
>>> You may want to put the above assignment as directly below the
>>> commands that you want to record as the exit status, and maybe
>>> initialize it before the case statement to the default value.
>>>
>>> For example, if somehow (not sure it's possible today) $command was
>>> not one of the values in the case statement, the value of $? here
>>> would be the return value of mktemp, which may not be your
>>> intention.
>>>
>>>
>>>> +rm "${substat_tmp}" 2>/dev/null
>>>
>>> Rather than doing this redirect, a simple rm -f will achieve what
>>> you want here - that is why makefiles tend to use it.
>>
>> I really do not like shell :) I gave up to using "trap", seems doing the
>> right thing and no messing is needed with "exit".
>>
>>
>> diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
>> index d8fbc7e47e..12f80c14a0 100755
>> --- a/scripts/git-submodule.sh
>> +++ b/scripts/git-submodule.sh
>> @@ -23,16 +23,18 @@ then
>>     exit 1
>> fi
>>
>> +substat_tmp=$(mktemp)
>> +trap "rm -f $substat_tmp" 0
>> +
>> case "$command" in
>> status)
>>     test -f "$substat" || exit 1
>> -    trap "rm -f ${substat}.tmp" EXIT
>> -    git submodule status $modules > "${substat}.tmp"
>> -    diff "${substat}" "${substat}.tmp" >/dev/null
>> -    exit $?
>> +    git submodule status $modules > "$substat_tmp"
>> +    diff "${substat_tmp}" "${substat}" > /dev/null
>>     ;;
>> update)
>>     git submodule update --init $modules 1>/dev/null 2>&1
>> -    git submodule status $modules > "${substat}"
>> +    git submodule status $modules > "$substat_tmp"
>> +    diff "${substat_tmp}" "${substat}" || mv "${substat_tmp}" "${substat}"
>>     ;;
>> esac
>>
>>
>> Is this good enough to repost?
> 
> If the exit code is not important here, then it should be OK,
> subject to my comments about using 'diff -q' instead.
> 
> If the exit code is important in this script I would still suggest
> being explicit about it, by setting myret=0 before the case, and
> then myret=$? after calls to diff, and finally an exit $myret.


The last command exit code goes to the caller - this is quite explicit imho.



>> ps. out of curiosity - your mail has "Mail-Followup-To" - is that
>> intentional?
> 
> I'm using the default in Neomutt, which suggests that is should be
> used to avoid duplicate e-mails when responding to lists.

A proper mailer will show a single copy, based on message-id (I know
nothing about neomutt) :)

>   https://www.neomutt.org/guide/advancedusage.html#using-lists
> 
> I've not changed from the default behaviour, but maybe it's not how
> people like to do it here... ;)





> 
> Thanks,
> 
> Darren.
>>
>>
>>>
>>> Thanks,
>>>
>>> Darren.
>>>
>>>> +exit $myret
>>>> -- 
>>>> 2.11.0
>>>>
>>>>
>>
>>
>> -- 
>> Alexey
>>


-- 
Alexey