From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Thu, 07 May 2009 08:43:55 GMT
Subject: Re: [PATCH] topgit tg push feature
Message-ID: <20090507084355.GA11680@pengutronix.de>
In-Reply-To: <36ca99e90905062245w1ec78828ice5ed18161b6ec43@mail.gmail.com>

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@gmx.de> 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@gmx.de>
> > 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@gmx.de>
> >
> > 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/  |