Hi Marc, hi Bert,
On Thu, May 07, 2009 at 07:45:21AM +0200, Bert Wesarg wrote:
> On Thu, May 7, 2009 at 06:59, Marc Weber <marco-oweber@xxxxxx> wrote:
> > Uwe K,
> >
> > what do you think. Is there still much to change to include this patch into
> > upstream?
> I think its a usefull tool, but I haven't tested it yet.
Bert, thanks for your review.
> > new patch version. adding small comment saying that the two options
> > --ne-deps and --tgish-only will seldomly used..
> >
> > Sincerly
> > Marc Weber
> >
> > commit c1cff518e3f70e9bd6cb4f2119b86e506ab43776
> > Author: Marc Weber <marco-oweber@xxxxxx>
> > Date: Thu May 7 06:46:28 2009 +0200
> >
> > t/tg-push
> >
> > add tg-push pushing the branch, its deps and their bases
> >
> > Usage: tg push [(--no-deps | --tgish-only)] remote*
> >
> > Signed-off-by: Marc Weber <marco-oweber@xxxxxx>
> >
> > diff --git a/README b/README
> > index d2f095d..6f2b2bc 100644
> > --- a/README
> > +++ b/README
> > @@ -480,6 +480,19 @@ tg update
> >
> > TODO: tg update -a for updating all topic branches
> >
> > +tg push
> > + Usage: tg push [(--no-deps | --tgish-only)] remote*
> no need to repeat yourself. 'tg help push' will print the usage from
> tg-push first. And where is the --dry-run option?
>
> > + $git push remote branch # this doesn't push the base.
> > + $git push remote # pushes all branches (and bases)
> > + You use
> > +
> > + $tg push remote
> > + to push the current branch,
> > its deps and their both tgish and non-tgish deps.
> This phrase needs rewording, what about: "its deps, both their tgish
> and non-tgish ones."
I'd like to have a bit more prose. Something like:
tg push
~~~~~~~
pushes a TopGit-controlled topic branch to a remote
repository. By default the remote gets all dependencies
(both tgish and non-tgish) and bases pushed to.
> > --- /dev/null
> > +++ b/tg-push.sh
> > @@ -0,0 +1,69 @@
> > +#!/bin/sh
> > +# TopGit - A different patch queue manager
> > +# GPLv2
> > +
> > +remotes=
> > +
> > +## Parse options see README
> > +
> > +recurse_deps=1
> > +tgish_deps_only=
> > +
> > +while [ -n "$1" ]; do
> > + arg="$1"; shift
> > + case "$arg" in
> > + --no-deps)
> > + recurse_deps=;;
> > + --dry-run)
> > + dry_run=1;;
> > + --tgish-only)
> > + tgish_deps_only=1;;
> > + *)
> > + remotes="$remotes $arg";;
> > + esac
> > +done
> now i see why you have the Usage: in the README. common practice is to
> print the usage if an unknown option was given. see all the other
> tg-*.sh scripts.
ack
> > +
> > +if [ -z "$remotes" ]; then
> > + remotes="$(git config topgit.remote 2>/dev/null)"
> > +fi
> How effetcts the tg -r REMOTE option this command. Or more exactly why
> doesn't have this option an effect here?
ack
> > +
> > +if [ -z "$remotes" ]; then
> > + die "no remote location given. Either add a remote as additional argument or set topgit.remote"
> > +fi
> > +
> > +name="$(git symbolic-ref HEAD | sed 's#^refs/heads/##')"
> > +ref_exists "$name" || die "detached HEAD? Can't push that"
> the common error message is: "not a TopGit-controlled branch".
>
> > +
> > +push_branch(){
> > + # don't push remotes
> > + [ -z "${_dep##refs/remotes/*}" ] && return 0
maybe extend the comment to something like:
# if a remote is used ('topgit.remote' configuration variable or
# tg -r $remote, a tg-base implicitly depends on
# refs/remotes/$remote/top-bases/$_dep. Don't push these.
You could even test if this is a dependency as described in this
comment. Didn't test it, but something like
test "x$_dep" = "xrefs/remotes/$base_remote/top-bases/$_name"
could do the trick.
> > + # if so desired omit non tgish deps
> > + [ -z "$tgish_deps_only" ] || [ -n "$_dep_is_tgish" ] || return 0
I've always problems to understand these constructs. Are these any
better than
if test ...; then
return 0
fi
? And I think it's more readable to use
tgish_deps_only=false
...
--tgish-only)
tgish_deps_only=true;;
...
if $tgish_deps_only; then
...
fi
. (OK, I have to admit, that these constructs are used everywhere in
topgit, but I'm not happy with these either.)
> > + echo "$_dep"
> > + local base="top-bases/$_dep"
> > + if ref_exists "$base"; then
> > + echo "top-bases/$_dep"
> > + else
> > + echo "warning, no base found $base" 1>&2
> > + fi
mmh, I wonder why you need to do the top-bases explicitly. recurse_deps
doesn't that for you?
> > +}
> > +
> > +for remote in $remotes; do
> > + list="$(
> > + # deps
> > + if [ -n "$recurse_deps" ]; then
> > + recurse_deps push_branch "$name"
> > + fi
> > + # current branch
> > + _dep="$name"
> > + _dep_is_tgish=1
> > + push_branch "$name"
> > + )"
> > + echo "pushing:"; echo $list
> > + if [ -n "$dry_run" ]; then
> > + echo git push $remote $list
> > + else
> > + git push $remote $list
> > + fi
> why not pass the --dry-run option to git remote?
s/remote/push/
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html