Hi Junio, El jue., 27 feb. 2020 a las 17:41, Junio C Hamano (<gitster@xxxxxxxxx>) escribió: > > "Miriam R." <mirucam@xxxxxxxxx> writes: > > >> It would be surprising if this code were correct. It may be that it > >> happens to (appear to) work because parents of the commit hasn't > >> been painted and calling the helper only clears the mark from the > >> commit (so we won't see repeated "painting down to the root commit") > >> in which case this might be an extremely expensive looking variant > >> of > >> > >> commit->object.flags &= ~ALL_REV_FLAGS; > >> > >> that only confuses the readers. > >> > >> Even then, I think by clearing bits like SEEN from commit, it breaks > >> the revision traversal machinery---for example, doesn't this mean > >> that the commit we just processed can be re-visited by > >> get_revision() without deduping in a history with forks and merges? > >> > >> Has this been shown to any of your mentors before sending it to the > >> list? > > > > Adding clear_commit_marks() was a suggestion of a previous review of this patch: > > https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@xxxxxxxxxxxxxxxxx/ > > > > And of course, my mentor always reviews my patch series before sending. > > OK, I just didn't know how you and your mentors work. Some leave > their door open for mentees and help them when asked but otherwise > act as an ordinary reviewer who somehow prioritises reviewing the > work by their mentees. So your mentors may be a better source of > information why this piece of code, which I still do not know how it > could be correct, is supposed to work. Good. > > After reading the above URL, I think you may have misread the > suggestion you were given. Resetting using clear_commit_marks() may > be necessary, but you do so when you finished walking so that you > can do unrelated revision walk later. For that, you clear the flag > bits after the while() loop that asks get_revision() to yield > commits are done, using the initial set of commits that you used to > start iteration. > > That is how bisect.c::check_ancestors() work, that is > > - it initializes a rev_info 'revs' from an array of commit rev[] > > - it lets bisect_common() use the 'revs', which is allowed to > smudge the flag bits of commit objects. > > - it uses clear_commit_marks_many() to clear the flags of the > commits whose flag bits may have been smudged and their > ancestors, recursively. In order to use as the starting points, > the original array of commit rev[] that started the revision > traversal is used. Thank you for your explanation. To my understanding, it looks like calling reset_revision_walk() after the while() loop should be enough. Am I right or am I missing something?
