[3/6] pkg-infra: git helper creates an empty archive if PKG_VERSION is a missing hash

From: "Yann E. MORIN" <yann.morin.1998@free.fr>

If the PKG_VERSION is a sha1 that does not exist in the repository, the git
helper just creates an empty archive instead of failing and erroring out.

This is because of the way we create the archive:
    git archive ${cset} |gzip -c >"${archive}"

In this construct, git does exit with a non-zero exit-code, but gzip does not,
as it was successful in creating the archive (from an empty output, but gzip
does not mind that).

Since a pipe exits with the exit-code of the right-most simple command (see
POSIX.1-2008), the pipe above exits with gzip's exit-code. And we miss the
error altogether.

This is most harmful, in that the error may not occur until much later, when
we try to configure the package, with the error message buried deep down
ipotentially thousands of lines above... :-(

Fix that by checking that the hash (or whatever treeish we've been passed, for
that matters) does indeed exist in the repository, before we even attempt to
create the archive.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
---
 support/download/git | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Hi Yann,

Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> If the PKG_VERSION is a sha1 that does not exist in the repository, the git
> helper just creates an empty archive instead of failing and erroring out.
>
> This is because of the way we create the archive:
>      git archive ${cset} |gzip -c >"${archive}"
>
> In this construct, git does exit with a non-zero exit-code, but gzip does not,
> as it was successful in creating the archive (from an empty output, but gzip
> does not mind that).
>
> Since a pipe exits with the exit-code of the right-most simple command (see
> POSIX.1-2008), the pipe above exits with gzip's exit-code. And we miss the
> error altogether.
>
> This is most harmful, in that the error may not occur until much later, when
> we try to configure the package, with the error message buried deep down
> ipotentially thousands of lines above... :-(

s/ipotentially/potentially/

>
> Fix that by checking that the hash (or whatever treeish we've been passed, for
> that matters) does indeed exist in the repository, before we even attempt to
> create the archive.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Acked-by: Luca Ceresoli <luca@lucaceresoli.net>

Luca, All,

On 2014-01-13 15:22 +0100, Luca Ceresoli spake thusly:
> Yann E. MORIN wrote:
> >From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >
> >If the PKG_VERSION is a sha1 that does not exist in the repository, the git
> >helper just creates an empty archive instead of failing and erroring out.
> >
> >This is because of the way we create the archive:
> >     git archive ${cset} |gzip -c >"${archive}"
> >
> >In this construct, git does exit with a non-zero exit-code, but gzip does not,
> >as it was successful in creating the archive (from an empty output, but gzip
> >does not mind that).
> >
> >Since a pipe exits with the exit-code of the right-most simple command (see
> >POSIX.1-2008), the pipe above exits with gzip's exit-code. And we miss the
> >error altogether.
> >
> >This is most harmful, in that the error may not occur until much later, when
> >we try to configure the package, with the error message buried deep down
> >ipotentially thousands of lines above... :-(
> 
> s/ipotentially/potentially/

Fixed.

This is where you see that I use vi as my editor! ;-)

> >Fix that by checking that the hash (or whatever treeish we've been passed, for
> >that matters) does indeed exist in the repository, before we even attempt to
> >create the archive.
> >
> >Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> 
> Acked-by: Luca Ceresoli <luca@lucaceresoli.net>

Thank you!

Regards,
Yann E. MORIN.

On 13/01/14 00:44, Yann E. MORIN wrote:
> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
>
> If the PKG_VERSION is a sha1 that does not exist in the repository, the git
> helper just creates an empty archive instead of failing and erroring out.
>
> This is because of the way we create the archive:
>      git archive ${cset} |gzip -c >"${archive}"
>
> In this construct, git does exit with a non-zero exit-code, but gzip does not,
> as it was successful in creating the archive (from an empty output, but gzip
> does not mind that).
>
> Since a pipe exits with the exit-code of the right-most simple command (see
> POSIX.1-2008), the pipe above exits with gzip's exit-code. And we miss the
> error altogether.
>
> This is most harmful, in that the error may not occur until much later, when
> we try to configure the package, with the error message buried deep down
> ipotentially thousands of lines above... :-(
>
> Fix that by checking that the hash (or whatever treeish we've been passed, for
> that matters) does indeed exist in the repository, before we even attempt to
> create the archive.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> ---
>   support/download/git | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/support/download/git b/support/download/git
> index 99ea1b2..508f8a1 100755
> --- a/support/download/git
> +++ b/support/download/git
> @@ -28,9 +28,15 @@ else
>   fi
>
>   pushd .git-tmp-repo >/dev/null
> -${GIT} archive --prefix="${prefix}" --format=tar "${cset}"  \
> -|gzip -c >"${output}"
> +res=1
> +if ${GIT} rev-list -n 1 "${cset}" >/dev/null 2>&1; then

  When we've done a shallow clone, this check is redundant because we 
have cloned exactly this ref.

  So instead, I'd do the check in the full clone condition, with an 
explicit exit.

  Regards,
  Arnout

> +    ${GIT} archive --prefix="${prefix}" --format=tar "${cset}"  \
> +    |gzip -c >"${output}"
> +    res=0
> +fi
>   popd >/dev/null
>
>   rm -rf .git-tmp-repo
>   popd >/dev/null
> +
> +exit ${res}
>

Arnout, All,

On 2014-01-14 21:43 +0100, Arnout Vandecappelle spake thusly:
> On 13/01/14 00:44, Yann E. MORIN wrote:
> >From: "Yann E. MORIN" <yann.morin.1998@free.fr>
[--SNIP--]
> >-${GIT} archive --prefix="${prefix}" --format=tar "${cset}"  \
> >-|gzip -c >"${output}"
> >+res=1
> >+if ${GIT} rev-list -n 1 "${cset}" >/dev/null 2>&1; then
> 
>  When we've done a shallow clone, this check is redundant because we have
> cloned exactly this ref.
> 
>  So instead, I'd do the check in the full clone condition, with an explicit
> exit.

I see. But it complexifies the script.

And doing the check a second time ensures we did not download garbage

Heck, there's even a race condition, since we check the remote, then
clone it, and the ref may have vanished (granted, very unlikely, but
yet...). :-p

Regards,
Yann E. MORIN.

On 15/01/14 00:21, Yann E. MORIN wrote:
> Arnout, All,
>
> On 2014-01-14 21:43 +0100, Arnout Vandecappelle spake thusly:
>> On 13/01/14 00:44, Yann E. MORIN wrote:
>>> From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> [--SNIP--]
>>> -${GIT} archive --prefix="${prefix}" --format=tar "${cset}"  \
>>> -|gzip -c >"${output}"
>>> +res=1
>>> +if ${GIT} rev-list -n 1 "${cset}" >/dev/null 2>&1; then
>>
>>   When we've done a shallow clone, this check is redundant because we have
>> cloned exactly this ref.
>>
>>   So instead, I'd do the check in the full clone condition, with an explicit
>> exit.
>
> I see. But it complexifies the script.
>
> And doing the check a second time ensures we did not download garbage
>
> Heck, there's even a race condition, since we check the remote, then
> clone it, and the ref may have vanished (granted, very unlikely, but
> yet...). :-p

  True. I yield to your wisdom.

  Regards,
  Arnout

Arnout, All,

On 2014-01-15 09:17 +0100, Arnout Vandecappelle spake thusly:
> On 15/01/14 00:21, Yann E. MORIN wrote:
> >Arnout, All,
> >
> >On 2014-01-14 21:43 +0100, Arnout Vandecappelle spake thusly:
> >>On 13/01/14 00:44, Yann E. MORIN wrote:
> >>>From: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >[--SNIP--]
> >>>-${GIT} archive --prefix="${prefix}" --format=tar "${cset}"  \
> >>>-|gzip -c >"${output}"
> >>>+res=1
> >>>+if ${GIT} rev-list -n 1 "${cset}" >/dev/null 2>&1; then
> >>
> >>  When we've done a shallow clone, this check is redundant because we have
> >>cloned exactly this ref.
> >>
> >>  So instead, I'd do the check in the full clone condition, with an explicit
> >>exit.
> >
> >I see. But it complexifies the script.
> >
> >And doing the check a second time ensures we did not download garbage
> >
> >Heck, there's even a race condition, since we check the remote, then
> >clone it, and the ref may have vanished (granted, very unlikely, but
> >yet...). :-p
> 
>  True. I yield to your wisdom.

Sorry, that race-condition was not meant to be taken in the first degree.
Of course, the chances it happens are so thin we can easily ignore it.

However, I tried massaging the script so that we do the check only once,
and I can't find an easy way, except duplicating the check before
calling git archive.

I've added a comment saying so in the script.

Thank you for the review! :-)

Regards,
Yann E. MORIN.