1. Patchew
  2. QEMU
  3. View series

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

Alexey Kardashevskiy posted 1 patch 6 years, 6 months ago
Failed in applying to current master (apply log)
scripts/git-submodule.sh | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
[Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary
Posted by Alexey Kardashevskiy 6 years, 6 months ago 
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}"
     ;;
 esac
+
+myret=$?
+rm "${substat_tmp}" 2>/dev/null
+exit $myret
-- 
2.11.0
Re: [Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary
Posted by Darren Kenny 6 years, 6 months ago 
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.

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.

Thanks,

Darren.

>+exit $myret
>-- 
>2.11.0
>
>
Re: [Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary
Posted by Alexey Kardashevskiy 6 years, 6 months ago 
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


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?



ps. out of curiosity - your mail has "Mail-Followup-To" - is that intentional?


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


-- 
Alexey
Re: [Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary
Posted by Darren Kenny 6 years, 6 months ago 
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.

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

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

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

   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
>
Re: [Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary
Posted by Alexey Kardashevskiy 6 years, 6 months ago 
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
Re: [Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary
Posted by Darren Kenny 6 years, 6 months ago 
On Thu, Oct 26, 2017 at 08:03:24PM +1100, Alexey Kardashevskiy wrote:
>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.

Ah, got it - didn't realise that you were referring to the old
behaviour here.
>
>
>>
>> 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.

Fair enough then, wasn't sure if was intended :)

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

It is, but the problem is that someone modifying this in the future
may not be aware of the expectation that you actually want the last
commands return value to be output.

So something as simple as putting in an echo at the end of the
script would mess that up.

Which is why I suggest that if it IS important, then be explicit
about it rather than implicit.

>
>
>>> 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) :)
>

A proper mailing app?  Is there such a thing? ;)

/me doesn't really want to start a mailer flame war, so bows out...

Thanks,

Darren.
Re: [Qemu-devel] [PATCH qemu v2] git-submodule.sh: Do not try writing to source directory if not necessary
Posted by Daniel P. Berrange 6 years, 6 months ago 
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

I don't see a need to change this to run a rm later - it is fine as is.

> -    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}"
>      ;;

This update command doesn't need changing - it is fine as it exists
already. Once the status command is fix, the update command will never
be run when the submodules are already up2date. 


Essentially all we need is this

diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
index 08932a35f0..67a33e5d79 100755
--- a/scripts/git-submodule.sh
+++ b/scripts/git-submodule.sh
@@ -26,9 +26,10 @@ fi
 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
+    substat_tmp=$(mktemp)
+    trap "rm -f ${substat_tmp}" EXIT
+    git submodule status $modules > "${substat_tmp}"
+    diff "${substat}" "${substat_tmp}" >/dev/null
     exit $?
     ;;
 update)


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.