On Tue, Apr 9, 2013 at 9:25 AM, Maxime Gauduin <alucryd@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@gmail.com>
From: Alucryd <alucryd@gmail.com>
This check will run 'bzr info' on the distant URL provided in the
wrote: 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@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)