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

[Tomáš Chvátal]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dne 23.3.2011 00:08, Mike Frysinger napsal(a):
> 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
Oh damn :P
>
>> EGIT_BRANCH=${x:-${EGIT_BRANCH:=${EGIT_MASTER}}}
>> EGIT_COMMIT=${x:-${EGIT_COMMIT:=${EGIT_BRANCH}}}
>
> doesnt make much sense to use := ... it should be :-
Yeah it should be but still it was just copy/paste and did no harm :)
Altered :)
>
>> [[ "$#" -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 ..."
Altered
>
>> 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
Yeah that does not matter that much, altered to make stderr visible again.
>
>> 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"
Well mostly nobody reads die messages but right here it is pointless.
>
>> 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.
Does not matter for me either. Since using both i will just stick
withoutdot for dies everywhere :)
>
>> if [[ "${EGIT_COMMIT}" != "${EGIT_BRANCH}" ]]; then
>
> quoting doesnt matter here since you're using [[...]]
>
> seems to come up in a bunch of places
Yeah overquoting is my favorite thing to do during the long and boring
evenings :) removed
>
>> 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
>
I actualy can't find any good reason why that var is defined at all.
Removed the code.

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

Good question, i actualy dunno why i picked rsync back then instead of cp.
I would love to use git bare clones (damn submodules) and just use git
to move this crap around. cp -pPR should be equal so lets try with that
and see how much people whine around :)
>
>> git-2_initial_clone()
>> git-2_update_repo()
>
> shouldnt EGIT_REPO_URI_SELECTED be set to "" at the start ?
Why not :)
And found issue in die string because it said wrong variable there :)
>
>> for x in $(git branch |grep -v "* ${EGIT_MASTER}" |tr '\n' ' '); do
>
> missing space after each pipe
Added
>
>> 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
Yerp i go with the later mostly but sometimes when i copy part statement
i get space into it. Unified.

Thanks for all the points :)

Again the diff is: http://tinyurl.com/62eb88b

Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2JQgsACgkQHB6c3gNBRYf79QCfV454YtzZD+2m7fOvFHvriDnf
0+4AnjnCA9oCwxPyzWsl8YzAI8ihXdm1
=qdu+
-----END PGP SIGNATURE-----