Hi,
On Mon, 8 Oct 2007, Christian Couder wrote:
> Le lundi 8 octobre 2007, Johannes Schindelin a ?crit :
>
> > On Mon, 8 Oct 2007, Christian Couder wrote:
> > > diff --git a/git-bisect.sh b/git-bisect.sh
> > > index 388887a..c556318 100755
> > > --- a/git-bisect.sh
> > > +++ b/git-bisect.sh
> > > @@ -143,7 +145,7 @@ bisect_write_bad() {
> > >
> > > bisect_good() {
> > > bisect_autostart
> > > - case "$#" in
> > > + case "$#" in
> >
> > White space breakage.
>
> The patch tries to fix some white space breakages.
>
> > > @@ -153,7 +155,6 @@ bisect_good() {
> > > rev=$(git rev-parse --verify "$rev^{commit}") || exit
> > > bisect_write_good "$rev"
> > > echo "git-bisect good $rev" >>"$GIT_DIR/BISECT_LOG"
> > > -
> >
> > ?
>
> It also removes this unneeded blank line.
Both laudable changes; alas, they distracted me.
> > > @@ -164,6 +165,28 @@ bisect_write_good() {
> > > echo "# good: "$(git show-branch $rev) >>"$GIT_DIR/BISECT_LOG"
> > > }
> > >
> > > +bisect_dunno() {
> > > + bisect_autostart
> > > + case "$#" in
> > > + 0) revs=$(git rev-parse --verify HEAD) || exit ;;
> > > + *) revs=$(git rev-parse --revs-only --no-flags "$@") &&
> > > + test '' != "$revs" || die "Bad rev input: $@" ;;
> > > + esac
> > > + for rev in $revs
> > > + do
> > > + rev=$(git rev-parse --verify "$rev^{commit}") || exit
> > > + bisect_write_dunno "$rev"
> > > + echo "git-bisect dunno $rev" >>"$GIT_DIR/BISECT_LOG"
> >
> > Should the last line not be put into bisect_write_dunno? OTOH this is
> > the only call site of that function, so I strongly doubt that the
> > function (consisting of 3 lines, where the first is 'rev="$1"') is
> > necessary at all.
>
> Well, there are "bisect_write_bad" and "bisect_write_good" that already
> do the same thing as "bisect_write_dunno". In fact I thought that it was
> better to just copy "bisect_dunno" from "bisect_good" and
> "bisect_write_dunno" from "bisect_write_good".
If they also are called by just one site, and also do not do the complete
printing to the log in the function (but also in the caller), I think they
are not really worth it, either.
> If needed I can send another patch to factorise these functions.
That's not up to me to decide. I'm just saying what I dislike.
Please do not take my criticism as a sign of a personal attack; if I did
not find your patch worthwhile, I would not bother to respond. So in a
way, it is my way to show my appreciation for your work that I review and
criticize it; for efficiency, I do not mention what I like ;-)
Ciao,
Dscho
-
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
