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

[Darren Kenny]

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 <address@hidden>
> > > ---
> > > 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