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

[Maxime Gauduin]

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.

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

> 
> > +                               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?

Cheers.
--
Maxime