Hi,
Le dimanche 13 juillet 2008, Johannes Schindelin a écrit :
> Hi,
>
> On Sun, 13 Jul 2008, Christian Couder wrote:
> > [PATCH] bisect: check all merge bases instead of only one
>
> It would have been so much nicer to squash the two patches into one, as
> we generally frown upon "let's submit one patch that we know is faulty,
> and then another that fixes it". That's so CVS/SVN.
I didn't think the first one was "faulty". It just didn't fix everything.
> > @@ -384,19 +383,21 @@ check_merge_bases() {
> > _skip="$3"
> > for _g in $_good; do
>
> I really wonder if we can be blessed with less ugly variable names.
> Maybe some that do not start with an underscore for no apparent reason,
There is a reason though. It's because in "bisect_next", variables with
those names ("good", "bad" and "skip") are already used, so reusing the
same name is not easily possible.
> and maybe some that are longer than one letter, so that you have a chance
> to understand later what it is supposed to contain. I.e. something like:
>
> for good in $good_revisions
> do
>
> (You see that I also broke up the "for" and "do" into two lines, as is
> common practice in the rest of Git's shell code.)
There are other places in git-bisect.sh where "for" and "do" are in the same
line. Perhaps one day I will submit a patch to fix these.
> > is_merge_base_ok "$_bad" "$_g" && continue
> > - _mb=$(git merge-base $_g $_bad)
> > - if test "$_mb" = "$_g" || is_among "$_mb" "$_good"; then
> > - mark_merge_base_ok "$_bad" "$_g"
> > - elif test "$_mb" = "$_bad"; then
> > - handle_bad_merge_base "$_bad" "$_g"
> > - elif is_among "$_mb" "$_skip"; then
> > - handle_skipped_merge_base "$_bad" "$_g" "_mb"
> > - else
> > - mark_testing_merge_base "$_mb"
> > - checkout "$_mb" "a merge base must be tested"
> > - checkout_done=1
> > - break
> > - fi
> > + for _mb in $(git merge-base --all $_g $_bad); do
> > + if test "$_mb" = "$_g" || is_among "$_mb" "$_good"; then
> > + continue
> > + elif test "$_mb" = "$_bad"; then
> > + handle_bad_merge_base "$_bad" "$_g"
> > + elif is_among "$_mb" "$_skip"; then
> > + handle_skipped_merge_base "$_bad" "$_g" "_mb"
> > + else
> > + mark_testing_merge_base "$_mb"
> > + checkout "$_mb" "a merge base must be tested"
> > + checkout_done=1
> > + return
> > + fi
> > + done
> > + mark_merge_base_ok "$_bad" "$_g"
> > done
> > }
>
> I really wonder if we cannot do better than that, in terms of code
> complexity.
>
> For example, I wonder if we should special-case the start, and not just
> check everytime if there are unchecked merge bases instead. If there
> are, check the first.
In fact, there was such a thing in my patch, search
for "$GIT_DIR/BISECT_ANCESTORS_OK". But it was a little bit broken if
people didn't test the commit that "git bisect" suggested.
In the next version I will post just after this mail, this is in the 2/2
patch (and hopefully fixed).
> But that can wait until you come back from your vacation...
>
> Have fun,
> Dscho
Thanks,
Christian.
--
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
