bisect-helper: we do not bisect --objects

bisect-helper: we do not bisect --objects

[Junio C Hamano]

Ever since "bisect--helper" was introduced in 1bf072e366
("bisect--helper: implement "git bisect--helper"", 2009-03-26),
after setting up the "rev-list $bad --not $good_ones" machinery, the
code somehow prepared to mark the trees and blobs at the good boundary
as uninteresting, only when --objects option was given.  This was kept
across a bit of refactoring done by 2ace9727be ("bisect: move common
bisect functionality to "bisect_common"", 2009-04-19) and survives
to this day.

However, "git bisect" does not care about tree/blob object
reachability at all---it purely works at the commit DAG level and
nobody passes (and nobody should pass) "--objects" option to the
underlying rev-list machinery.  Remove the dead code.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Christian, do you recall what we were thinking when we added this
   mark_edges_uninteresting() call in this program?  If you don't,
   don't worry--this was done more than 8 years ago.  I am just
   being curious and also a bit being cautious in case I am missing
   something.

   Thanks.

 bisect.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/bisect.c b/bisect.c
index 30808cadf7..86c5929a23 100644
--- a/bisect.c
+++ b/bisect.c
@@ -634,8 +634,6 @@ static void bisect_common(struct rev_info *revs)
 {
 	if (prepare_revision_walk(revs))
 		die("revision walk setup failed");
-	if (revs->tree_objects)
-		mark_edges_uninteresting(revs, NULL);
 }
 
 static void exit_if_skipped_commits(struct commit_list *tried,

Re: bisect-helper: we do not bisect --objects

[Christian Couder]

On Fri, Mar 3, 2017 at 5:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ever since "bisect--helper" was introduced in 1bf072e366
> ("bisect--helper: implement "git bisect--helper"", 2009-03-26),
> after setting up the "rev-list $bad --not $good_ones" machinery, the
> code somehow prepared to mark the trees and blobs at the good boundary
> as uninteresting, only when --objects option was given.  This was kept
> across a bit of refactoring done by 2ace9727be ("bisect: move common
> bisect functionality to "bisect_common"", 2009-04-19) and survives
> to this day.
>
> However, "git bisect" does not care about tree/blob object
> reachability at all---it purely works at the commit DAG level and
> nobody passes (and nobody should pass) "--objects" option to the
> underlying rev-list machinery.  Remove the dead code.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * Christian, do you recall what we were thinking when we added this
>    mark_edges_uninteresting() call in this program?  If you don't,
>    don't worry--this was done more than 8 years ago.  I am just
>    being curious and also a bit being cautious in case I am missing
>    something.

I think I just copy pasted the code from cmd_rev_list() in
builtin-rev-list.c and probably didn't realize that revs->tree_objects
would always be false.

Thanks for spotting this and removing the dead code.

Re: bisect-helper: we do not bisect --objects

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> Ever since "bisect--helper" was introduced in 1bf072e366
> ("bisect--helper: implement "git bisect--helper"", 2009-03-26),
> after setting up the "rev-list $bad --not $good_ones" machinery, the
> code somehow prepared to mark the trees and blobs at the good boundary
> as uninteresting, only when --objects option was given.  This was kept
> across a bit of refactoring done by 2ace9727be ("bisect: move common
> bisect functionality to "bisect_common"", 2009-04-19) and survives
> to this day.
>
> However, "git bisect" does not care about tree/blob object
> reachability at all---it purely works at the commit DAG level and
> nobody passes (and nobody should pass) "--objects" option to the
> underlying rev-list machinery.  Remove the dead code.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * Christian, do you recall what we were thinking when we added this
>   mark_edges_uninteresting() call in this program?  If you don't,
>   don't worry--this was done more than 8 years ago.  I am just
>   being curious and also a bit being cautious in case I am missing
>   something.
>

Bikeshedding: If the given boundary is a tag, it could be tagging a blob or 
tree rather than a commit. Would that be a scenario that reaches this part 
of the code? I thought I read previous comments that there is a case in the 
Linux tree.
--
Philip

>   Thanks.
>
> bisect.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 30808cadf7..86c5929a23 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -634,8 +634,6 @@ static void bisect_common(struct rev_info *revs)
> {
>  if (prepare_revision_walk(revs))
>  die("revision walk setup failed");
> - if (revs->tree_objects)
> - mark_edges_uninteresting(revs, NULL);
> }
>
> static void exit_if_skipped_commits(struct commit_list *tried,
> 

Re: bisect-helper: we do not bisect --objects

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> Bikeshedding: If the given boundary is a tag, it could be tagging a
> blob or tree rather than a commit. Would that be a scenario that
> reaches this part of the code?

I do not think it is relevant.

Bisection is an operation over a "bisectable" commit DAG, where
commits can be partitioned into "new" (aka "bad") and "old" (aka
"good") camp, all descendants of a "new" commit are all "new", all
ancestors of an "old" commit are all "old".  Think of the "new"-ness
as a 100% inheritable disease that happens spontaneously and without
cure.  Once you are infected with "new"-ness, all your descendants
are forever "new".  If you know you are free of the "new"-ness, you
know that all your ancestors are not with the "new"-ness, either.

The goal of the operation is to find a "new" commit whose parents
are all "old".

The bisectability of the commit DAG is what allows you to say "this
commit is new" to a commit somewhere in the middle of the history
and avoid having to test any descendants, as they all inherit the
"new"-ness from it (similarly when you have one commit that is
"old", you do not have to test any ancestor), thereby reducing the
number of test from N (all commits in good..bad range) to log(N).

There is no room for a tree or a blob to participate in this graph
partitioning problem.  A "bad" tree that is "new" cannot infect its
children with the "new"-ness and a "good" tree cannot guarantee the
lack of "new"-ness of its parents, because a tree or a blob does not
have parent or child commits.

Re: bisect-helper: we do not bisect --objects

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> Bikeshedding: If the given boundary is a tag, it could be tagging a
>> blob or tree rather than a commit. Would that be a scenario that
>> reaches this part of the code?
>
> I do not think it is relevant.
>
> Bisection is an operation over a "bisectable" commit DAG, where
> commits can be partitioned into "new" (aka "bad") and "old" (aka
> "good") camp, all descendants of a "new" commit are all "new", all
> ancestors of an "old" commit are all "old".  Think of the "new"-ness
> as a 100% inheritable disease that happens spontaneously and without
> cure.  Once you are infected with "new"-ness, all your descendants
> are forever "new".  If you know you are free of the "new"-ness, you
> know that all your ancestors are not with the "new"-ness, either.
>
> The goal of the operation is to find a "new" commit whose parents
> are all "old".
>
> The bisectability of the commit DAG is what allows you to say "this
> commit is new" to a commit somewhere in the middle of the history
> and avoid having to test any descendants, as they all inherit the
> "new"-ness from it (similarly when you have one commit that is
> "old", you do not have to test any ancestor), thereby reducing the
> number of test from N (all commits in good..bad range) to log(N).
>
> There is no room for a tree or a blob to participate in this graph
> partitioning problem.  A "bad" tree that is "new" cannot infect its
> children with the "new"-ness and a "good" tree cannot guarantee the
> lack of "new"-ness of its parents, because a tree or a blob does not
> have parent or child commits.
>
Thanks.

I was happy with how the bisect actually works. I was more responding the 
the open question about how that piece of code may have come into existance, 
and the potential thought processes that can lead to such 'mistakes'.

My line of reasoning was that it is reasonable to pass both commits and 
tags, as revisions(7), to the cli as being the bad and good points in the 
graph. This could then lead to a expectation that the objects they point to 
should be suitably marked, which is quite reasonable for commits, and for 
most tags.

However there are tags that point to the commit tree, rather than the commit 
itself, so if that initial rule was followed in a simplistic manner, then 
(falsely) the peeled tree of the tag would be marked as good/bad, which as 
your patch identifies, would be the wrong thing to do.

The study of human error is quite interesting....

regards

Philip

Re: bisect-helper: we do not bisect --objects

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> I think I just copy pasted the code from cmd_rev_list() in
> builtin-rev-list.c and probably didn't realize that revs->tree_objects
> would always be false.
>
> Thanks for spotting this and removing the dead code.

Thanks for a quick confirmation. Just FYI I was looking at this code
because I was trying to assess how involved it would be to teach the
code to do a bisection only on the first-parent chain.

Re: bisect-helper: we do not bisect --objects

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> The study of human error is quite interesting....

Yes ;-)