Stefan Beller <sbeller@xxxxxxxxxx> writes: > With traverse_trees_and_blobs factored out of the main traverse function, > the next patch can introduce an in-order revision walking with ease. > > The variable holding the base path is only used in the newly factored out > function `traverse_trees_and_blobs`, however we keep its scope to > `traverse_commit_list` to keep the number of invocations for memory > allocations and release to one per commit traversal. In this step, the caller is calling traverse_trees_and_blobs() only once per traversal anyway, so the paragraph does not quite justify this somewhat strange calling convention, unless the reader knows more calls will be made to the same function by the caller in later steps, potentially doubling the chance the buffer can be reused with this arrangement. Even after this function gains more callers, wouldn't the same invariant that "path" recorded for trees and blobs are relative to the root of tree recorded in the object in revs->pending.objects[]? IOW, should base_path->len be always 0 upon entry to this function? We may want to have an explicit _reset() or assert(->len == 0) at the beginning of the function to ensure that. > Rename the variable to `base_path` for clarity. Hmph. Inside traverse_commit_list(), which never looks at the contents of the buffer, it could just be called with a name that does not have any meaning (e.g. "scratch area for the callee to use"), and such a change would make it clear why it is defined one level above its use and the caller never looks at it. In the callee (i.e. inside traverse_trees_and_blobs()), however, I do not see a reason why base_path is any clearer than base. Other than that, this looks like a straight-forward code movement and looks good.
