Code Review?

[jbenet]

Hey @chriscool, could you please take a look at this script?

thanks

[chriscool]

@jbenet ok:

usage="USAGE
    $0 [--clean] <remote> <branch-prefix>

As <remote> and <branch-prefix> are optional, perhaps use the following:

usage="USAGE
    $0 [--clean] [<remote> [<branch-prefix>]]

[chriscool]

Also the help/usage message could say what the default for <remote> and <branch-prefix> are.

[chriscool]

It looks like it would not error out and maybe do funny things if passed more than 2 non option arguments, like with git-push-each foo bar baz.

[chriscool]

I would also add a die() function like:

die() {
    echo >&2 "$@"
    exit 1
}

and use it to check that the push worked, like for example:

git push "$remote" "$spec" || die "Could not git push '$remote' '$spec'"

[chriscool]

About [0-9a-f]{40}, I think it doesn't work when using uppercase letters from A to F in the hash, so you might want to add the -i, ignore case, switch when calling egrep, or use [0-9a-fA-F]{40} instead.

[chriscool]

Otherwise it looks good to me. If you want I can have a look at providing patches for the above suggestions in the next days.

[chriscool]

Also I am worried about what happens if git branch -r fails in this line:

 for line in $(git branch -r | egrep -wo "$remote/$spec" | egrep -o "$spec")

I think it would be better if the script would "die" if it fails.

[jbenet]

As and are optional, perhaps use the following:

SGTM 👍

Also the help/usage message could say what the default for and are.

SGTM 👍

It looks like it would not error out and maybe do funny things if passed more than 2 non option arguments, like with git-push-each foo bar baz.

Ah yes-- any good ideas there?

About [0-9a-f]{40}, I think it doesn't work when using uppercase letters from A to F in the hash, so you might want to add the -i, ignore case, switch when calling egrep, or use [0-9a-fA-F]{40} instead.

Ah yes, 👍

If you want I can have a look at providing patches for the above suggestions in the next days.

yes please!

Also I am worried about what happens if git branch -r fails in this line:
for line in $(git branch -r | egrep -wo "$remote/$spec" | egrep -o "$spec")
I think it would be better if the script would "die" if it fails.

Yeah entirely agreed.

---

@chriscool thanks so much-- this was a quick hack, and clearly not robust yet.

do you think it makes sense to test it with sharness?

[jbenet]

Oh and:

would also add a die() function like:
....
and use it to check that the push worked, like for example:

yep 👍

[chriscool]

@jbenet I don't think it's worth adding sharness tests for this script for now. It is still quite small and not very sensitive.

[jbenet]

@jbenet I don't think it's worth adding sharness tests for this script for now. It is still quite small and not very sensitive.

yeah, my thoughts too. sounds good.

[chriscool]

@jbenet can you close this now as PR #6 is merged?

[jbenet]

Yep! Thank you!