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

[Darren Kenny]

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.

> 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