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
