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