git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/2] Bisect: implement "bisect dunno" to mark untestable revisions.

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Oct 8, 2007, 05:36 UTC
Message-ID
<Pine.LNX.4.64.0710080632510.4174@racer.site>
In-Reply-To
<200710080734.23878.chriscool@tuxfamily.org>
Hi,
On Mon, 8 Oct 2007, Christian Couder wrote:
Show 27 quoted lines
> 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.
Show 26 quoted lines
> > > @@ -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

Previous: Christian Couder
Message 4 of 4 in “Bisect: implement "bisect dunno" to mark untestable revisions.”
  1. 2/2 Bisect: implement "bisect dunno" to mark untestable revisions.Christian Couder, Oct 8, 2007
  2. Johannes SchindelinOct 8, 2007
  3. Christian CouderOct 8, 2007
  4. Johannes SchindelinOct 8, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.