Re: [Qemu-devel] [PULL v2 10/37] scripts: Add archive-source.sh

[Fam Zheng <famz@redhat.com>]

On Sat, 09/09 13:07, Peter Maydell wrote:
> On 9 September 2017 at 06:45, Fam Zheng <famz@redhat.com> wrote:
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Message-Id: <20170905021201.25684-4-famz@redhat.com>
> > ---
> >  scripts/archive-source.sh | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100755 scripts/archive-source.sh
> >
> > diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
> > new file mode 100755
> > index 0000000000..1de369532e
> > --- /dev/null
> > +++ b/scripts/archive-source.sh
> > @@ -0,0 +1,31 @@
> > +#!/bin/sh
> > +#
> > +# Author: Fam Zheng <famz@redhat.com>
> > +#
> > +# Create archive of source tree, including submodules
> > +#
> > +# This code is licensed under the GPL version 2 or later.  See
> > +# the COPYING file in the top-level directory.
> 
> So is this the script that for instance Mike Roth would use
> to create the release tarballs? If it isn't, should it be?

I don't know the workflow of release tarballs, there is ./scripts/make-release
for that. So this one is not.

> Is it intended for end users to create tarballs with, or
> is it really just a helper script for the docker stuff?
> If the latter, it would be helpful to say so. If the former,
> it could really use more usage information/documentation...

I'm not sure what end users would use this for, but it should do its work just
the same.. What kind of usage information do you suggest? More explaination in
the header, or in the "usage" output when no target is specified, or a "-h"
option for help?

> 
> > +
> > +set -e
> > +
> > +if test $# -lt 1; then
> > +    echo "Usage: $0 <output>"
> > +    exit 1
> > +fi
> > +
> > +submodules=$(git submodule foreach --recursive --quiet 'echo $name')
> > +
> > +if test -n "$submodules"; then
> > +    {
> > +        git ls-files
> > +        for sm in $submodules; do
> > +            (cd $sm; git ls-files) | sed "s:^:$sm/:"
> > +        done
> > +    } | grep -x -v $(for sm in $submodules; do echo "-e $sm"; done) > $1.list
> 
> Supporting '-e something' in a tar -T listfile seems to
> be GNU tar specific. Do we care?

The "-e $sm" will go to the grep command line, so it isn't GNU specific, is it?

> 
> > +else
> > +    git ls-files > $1.list
> > +fi
> 
> This will blow up if we ever have a file in the repo
> that starts with a '-'. Do we care?

I'm sure such a file will be a trouble, but I'd rather we deal with it when
there is one: I don't think anyone will think about adding, or welcome such a
file.

> 
> > +
> > +tar -cf $1 -T $1.list
> > +rm $1.list
> 
> This is missing a lot of quoting for $1, so it will go wrong
> if there's a space in that filename argument.

Yes, good point. I will fix it before sending another pull request along with
other comments that are just raised.

Fam