From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from pigeon.gentoo.org ([208.92.234.80] helo=lists.gentoo.org) by finch.gentoo.org with esmtp (Exim 4.60) (envelope-from ) id 1Q2Ahz-0001ez-Eq for garchives@archives.gentoo.org; Tue, 22 Mar 2011 23:09:53 +0000 Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id B390F1C051; Tue, 22 Mar 2011 23:09:42 +0000 (UTC) Received: from mail-fx0-f53.google.com (mail-fx0-f53.google.com [209.85.161.53]) by pigeon.gentoo.org (Postfix) with ESMTP id 76BBF1C001 for ; Tue, 22 Mar 2011 23:09:14 +0000 (UTC) Received: by fxm11 with SMTP id 11so9187167fxm.40 for ; Tue, 22 Mar 2011 16:09:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:in-reply-to:references:from :date:x-google-sender-auth:message-id:subject:to:cc:content-type :content-transfer-encoding; bh=h9eImniBP7iJDoRTDQoerpdutxcSqFQKOSNe/q4eQ64=; b=NjVoNqrYQIT+QIEylSOt2k/MHv4Mq67aaz/tGwUWoxhkmtAMnJTrEuYlTdZAlZnYNm pmCkMGB6nsroKvTjyQdiuk0HRj/FTZbB0Am4KTF2iEpRrA3hsGy03yZZF5Klam5FOAWA /37IGrQVu7DsSDuPYb/5JMxF3DJUQEJtAk1vs= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:from:date :x-google-sender-auth:message-id:subject:to:cc:content-type :content-transfer-encoding; b=nYJc7BbszJWjuzilZM0ytxQiG02uh8ZWmSlTLsSZvnoHW8HJkPsnjzPu2tlEvhIn93 cFLnB7jtI9LbDND/jk1waz0+27mpgRY9PQb+vfbw2TA3Ao2fq1p9Ca5kcGYEbQG6qFBf /bEqwYxD+wljY7hrKvzimNcFiKyYPLlv5KDgE= Received: by 10.223.15.141 with SMTP id k13mr2229648faa.30.1300835353180; Tue, 22 Mar 2011 16:09:13 -0700 (PDT) Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-dev@lists.gentoo.org Reply-to: gentoo-dev@lists.gentoo.org MIME-Version: 1.0 Sender: vapierfilter@gmail.com Received: by 10.223.5.15 with HTTP; Tue, 22 Mar 2011 16:08:53 -0700 (PDT) In-Reply-To: <4D8924CD.8000404@gentoo.org> References: <4D890F8D.4090706@gentoo.org> <4D8924CD.8000404@gentoo.org> From: Mike Frysinger Date: Tue, 22 Mar 2011 19:08:53 -0400 X-Google-Sender-Auth: tVOUSRnW2z0sYiKS7VXBw_B_zzU Message-ID: Subject: Re: [gentoo-dev] git-2.eclass final review To: gentoo-dev@lists.gentoo.org Cc: =?ISO-8859-2?Q?Tom=E1=B9_Chv=E1tal?= Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Archives-Salt: X-Archives-Hash: b7b8e17c18915e0489d09bc8afb58761 2011/3/22 Tom=C3=A1=C5=A1 Chv=C3=A1tal: > Dne 22.3.2011 22:26, Mike Frysinger napsal(a): >>> # @BLURB: This eclass provides functions for fetch and unpack git repos= itories >> >> fetching/unpacking > > Yarp fixed. well, the fix broke the blurb. it has to be on one line. # @BLURB: foo > EGIT_BRANCH=3D${x:-${EGIT_BRANCH:=3D${EGIT_MASTER}}} > EGIT_COMMIT=3D${x:-${EGIT_COMMIT:=3D${EGIT_BRANCH}}} doesnt make much sense to use :=3D ... it should be :- > [[ "$#" -ne 1 ]] && die "${FUNCNAME}: requires 1 argument (path)" quoting doesnt make much sense ... -ne compares an int, not a string also, the error msg is a bit vague. it should say "... requires exactly 1 = ..." > pushd "${1}" &> /dev/null > popd &> /dev/null do you really want to silence errors ? normally people only send stdout to /dev/null because of the echoed dirlist. seems to come up in every func > git submodule init || die "${FUNCNAME}: git submodule initialisation fail= ed" > git submodule sync "" die "${FUNCNAME}: git submodule sync failed" > git submodule update || die "${FUNCNAME}: git submodule update failed" the die strings are abit redundant ... when it fails, the output will show "git submodule init || die", so people can easily figure out "the git submodule init cmd failed" > die "\"${EGIT_BOOTSTRAP}\" is not executable." i find periods in die messages which are a single sentence to be noise. but maybe that's just me. > if [[ "${EGIT_COMMIT}" !=3D "${EGIT_BRANCH}" ]]; then quoting doesnt matter here since you're using [[...]] seems to come up in a bunch of places > local save_sandbox_write=3D${SANDBOX_WRITE} > if [[ ! -d "${EGIT_STORE_DIR}" ]] ; then > ... > SANDBOX_WRITE=3D${save_sandbox_write} > fi might as well have the save done inside the if statement since that's the only place it's used > rsync -rlpgo . "${EGIT_SOURCEDIR}" \ this means you need to have DEPEND=3D"net-misc/rsync". why not just use `cp -pPR` instead ? i vaguely recall rsync being slower than a straight cp too ... not much point of doing a rsync when the vast majority of the time (all the time?) the destination is empty. > git-2_initial_clone() > git-2_update_repo() shouldnt EGIT_REPO_URI_SELECTED be set to "" at the start ? > for x in $(git branch |grep -v "* ${EGIT_MASTER}" |tr '\n' ' '); do missing space after each pipe > if [[ -n ${EGIT_BOOTSTRAP} ]] ; then > if [[ -f ${EGIT_BOOTSTRAP} ]]; then seems inconsistent in the whole file ... personally, i prefer the space before the semicolon in if statements -mike