[pacman-dev] [PATCH 2/2] Accept all bzr URLs in the source array

[Maxime GAUDUIN]

On Tue, Apr 9, 2013 at 6:29 PM, Dave Reisner <d at falconindy.com> wrote:

> On Tue, Apr 9, 2013 at 9:25 AM, Maxime Gauduin <alucryd at gmail.com> wrote:
> >
> > On Tue, 2013-04-09 at 08:30 -0400, Dave Reisner wrote:
> > > On Tue, Apr 9, 2013 at 5:28 AM, Maxime Gauduin <alucryd at gmail.com>
> wrote:
> > >
> > > > From: Alucryd <alucryd at gmail.com>
> > > >
> > > > This check will run 'bzr info' on the distant URL provided in the
> source
> > > > array and compare the branch root URL with the output of
> > > > 'bzr config parent_location' run inside the local repo to make sure
> we
> > > > are building from the right sources. Previously, the check was only
> run
> > > > locally, as a result the local parent_location had to be used in the
> > > > source array. makepkg will fallback to this behavior for offline
> builds.
> > > >
> > > > Signed-off-by: Maxime Gauduin <alucryd at gmail.com>
> > > > ---
> > > >  scripts/makepkg.sh.in | 19 +++++++++++++++----
> > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> > > > index d5b9077..0ac4975 100644
> > > > --- a/scripts/makepkg.sh.in
> > > > +++ b/scripts/makepkg.sh.in
> > > > @@ -474,10 +474,21 @@ download_bzr() {
> > > >                 fi
> > > >         elif (( ! HOLDVER )); then
> > > >                 # Make sure we are fetching the right repo
> > > > -               if [[ "$url" != "$(bzr config parent_location -d
> $dir)"
> > > >  ]] ; then
> > > > -                       error "$(gettext "%s is not a branch of %s")"
> > > > "$dir" "$url"
> > > > -                       plain "$(gettext "Aborting...")"
> > > > -                       exit 1
> > > > +               local distant_url="$(bzr info "$url" 2> /dev/null |
> grep
> > > > 'branch root' | sed 's|  branch root: ||')"
> > > >
> > >
> > > grep|sed is pretty much always redundant -- sed can do this match on
> its
> > > own:
> > >
> > >   sed -n '/branch root/ { s/  branch root: //p; q; }'
> > >
> >
> > Thx for the suggestion, somebody suggested the following on the previous
> > thread, which seems to do the trick too: `bzr info lp:lightdm
> > 2> /dev/null | sed -n 's|  branch root: ||p'`. It's a bit shorter, if
> > you're okay with it.
>
> Well, its shorter, but its not necessarily equivalent. If "branch root"
> appears on two different lines, the shorter version prints twice. I'm not
> sure that this would ever happen, but better to be safe, imo.
>

It should not but I agree, I'll use the version you provided then.


>
> >
> > > And, just nitpicking, but quoting isn't needed for simple variable
> > > assignment.
> > >
> > > +               local local_url="$(bzr config parent_location -d
> "$dir")
> > >
> > >
> > > Same here.
> > >
> > >
> > > > +               if [[ -n "$distant_url" ]]; then
> > > >
> > >
> > > Quoting isn't needed here either.
> > >
> > >
> > > > +                       if [[ "$distant_url" != "$local_url" ]]; then
> > > >
> > >
> > > Or on the LHS here.
> > >
> >
> > I apologize if it's a dumb question, but why not the RHS too?
>
> glob metachars will be interpreted on the RHS, but the LHS is always taken
> as literal. It allows for matching:
>
>   [[ foobar = foo* ]] # this is true
>   [[ foo[a-z]bar = "foo[a-z]bar" ]] # this is true
>   [[ foo[a-z]bar = foo[a-z]bar ]] # this is false
>   [[ foobar = "foo*" ]] # this is false
>
> In single argument tests, there's never any interpretation and strings are
> always treated as literals.
>

I see, thank you for the kind explanation.


>
> >
> > >
> > > > +                               error "$(gettext "%s is not a branch
> of
> > > > %s")" "$dir" "$url"
> > > > +                               plain "$(gettext "Aborting...")"
> > > > +                               exit 1
> > > > +                       fi
> > > > +               else
> > > > +                       if [[ "$url" != "$local_url" ]] ; then
> > > >
> > >
> > > And same here.
> > >
> > >
> > > > +                               error "$(gettext "%s is not a branch
> of
> > > > %s")" "$dir" "$url"
> > > > +                               error "$(gettext "The local URL is
> %s")"
> > > > "$local_url"
> > > > +                               plain "$(gettext "Aborting...")"
> > > > +                               exit 1
> > > > +                       fi
> > > >                 fi
> > > >                 msg2 "$(gettext "Pulling %s ...")"
> "${displaylocation}"
> > > >                 cd_safe "$dir"
> > > > --
> > > > 1.8.2
> > > >
> > > >
> > > >
> > >
> >
> > Alright, to sum it up, I have now 3 patches named "Add support for bzr
> > lp URLs in makepkg", "Fix dest dir when using lp: bzr URLs in the source
> > array" (both should be good as is), and this one.
> > Now, would you prefer if I merged the 3 into something like "Full Bazaar
> > support for makepkg" with a full explanation to make it easier for you
> > to merge (and forget about the mess I've made on the mailing list,
> > sorry), or should I just send only this one again with Dave's
> > suggestion?
>
> No mess. Just send another version. Though, I suggest versioning your
> patches and threading:
>
> [PATCH] I added an awesome new feature
>
> (review happens)
>
> [PATCHv2] I added an awesome new feature
>
> (This patch is in-reply-to the message-id of the original patch. more
> review happens, perhaps)
>
> [PATCHv3] I added an awesome new feature
>
> (This patch is in-reply-to the message-id of patchv2)
>
>
Sure, I'll try to send a v2 patch tomorrow then, merging the 2 others into
this one. No need to fragment since they're tiny.

Thanks again.
-- 
Maxime