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

[Dave Reisner]

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.

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

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