Re: [gentoo-dev] git-2.eclass final review

[Mike Frysinger]

2011/3/22 Tomáš Chvátal:
> Dne 22.3.2011 22:26, Mike Frysinger napsal(a):
>>> # @BLURB: This eclass provides functions for fetch and unpack git repositories
>>
>> fetching/unpacking
>
> Yarp fixed.

well, the fix broke the blurb.  it has to be on one line.
# @BLURB: foo

> EGIT_BRANCH=${x:-${EGIT_BRANCH:=${EGIT_MASTER}}}
> EGIT_COMMIT=${x:-${EGIT_COMMIT:=${EGIT_BRANCH}}}

doesnt make much sense to use := ... 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 failed"
> 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}" != "${EGIT_BRANCH}" ]]; then

quoting doesnt matter here since you're using [[...]]

seems to come up in a bunch of places

> local save_sandbox_write=${SANDBOX_WRITE}
> if [[ ! -d "${EGIT_STORE_DIR}" ]] ; then
> ...
>     SANDBOX_WRITE=${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="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