Re: Should reset_revision_walk clear more flags?

From: Jeff King <peff@peff.net>
Date: Dec 5, 2016, 05:40 UTC
Message-ID: <20161205054013.taosbwjamxiwzocn@sigill.intra.peff.net>
In-Reply-To: <20161204230958.h3ilhueqqptv253u@glandium.org>

On Mon, Dec 05, 2016 at 08:09:58AM +0900, Mike Hommey wrote:

Show 8 quoted lines

> While trying to use the revision walking API twice in a row, I noticed
> that the second time for the same setup would not yield the same result.
> In my case, it turns out I was requesting boundaries, and
> reset_revision_walk() is not resetting CHILD_SHOWN and BOUNDARY, both
> required to be reset for the second revision walk to work.
> 
> So the question is, are consumers supposed to reset those flags on their
> own, or should reset_revision_walk clear them?

I would think that reset_revision_walk() should reset any flags set by the revision-walking code (so anything set by calling init_revisions(), and then either a call into list_objects() or repeated calls of get_revision()).

Which I think would include both of the flags you mentioned, along with others like SYMMETRIC_LEFT, PATCHSAME, etc. Probably really everything mentioned in revision.h, which should be summed up as ALL_REV_FLAGS. Some callsites already seem to feed that to clear_commit_marks().

I doubt you can go too wrong by clearing more flags. It's possible that some caller is relying on a flag _not_ being cleared between two traversals, but in that case it should probably be using clear_commit_marks() or clear_object_flags() explicitly to make it clear what it expects to be saved.

-Peff