Jeff King venit, vidit, dixit 18.03.2011 20:34:
> On Fri, Mar 18, 2011 at 03:50:23PM +0100, Michael J Gruber wrote:
>
>> Introduce --min-parents and --max-parents which take values 0,...,7 and
>> limit the revisions to those commits which have at least resp. at most
>> that many commits, where --max-parents=8 denotes --max-parents=infinity
>> (i.e. no upper limit). In fact, 7 (or any negative number) does, but 8
>> is infinity sideways 8-)
>
> In practice, I don't think anybody is all that interested in
> differentiating octopus merges by their numbers of parents. But the
> choice of "7" as a maximum seems kind of arbitrary. There are commits in
> linux-2.6 with up to 31 parents (once upon a time we had an arbitrary
> limit of 16 parents, but that was lifted in v1.6.0).
>
> You mention below that this fits in three 3 bits in rev_info. I don't
> think bits are precious in rev_info, though, as they are in other
> revision-related structs. We only have one such struct per walk.
>
> If it were just an implementation issue, I would say fine, we can tweak
> it later if somebody really cares. But we are cementing "7" as the value
> for infinity in the user-facing interface. Wouldn't a value like "-1" or
> "infinity" be more appropriate?
-1 works and "infinity" is difficult to parse.
We could advocate "-1" only, rather than the max which is infinity.
Regarding using an "unbounded" int: I think it is difficult to
distinguish between an unspecified max (value 0 from the initialisation)
and a specified "unbounded" if I don't do the range inversion, which
obviously needs a bounded range. But I'll recheck.
Regarding the 8: Besides the connotation of being infinity sideways,
there's also the undeniable fact that everything with fewer than 8 isn't
really an octo-puss, so the choice of 3 bits is indeed natural!
>> In particular:
>>
>> --max-parents=1: no merges
>> --min-parents=2: merges only
>> --max-parents=0: only roots
>> --min-parents=3: only octopusses
>>
>> --min-parents=n --max-parents=m with n>m gives you what you ask for
>> (nothing) just like --merges --no-merges does, but at least for an
>> obvious reason.
>
> I like this. It's much more natural than the list syntax I suggested
> earlier, and it handles ranges in an obvious way. It doesn't allow
> selecting, e.g., root commits and merges, but not regular commits. But I
> really don't see the point in supporting that.
>
>> @@ -2029,10 +2033,15 @@ enum commit_action get_commit_action(struct rev_info *revs, struct commit *commi
>> return commit_ignore;
>> if (revs->min_age != -1 && (commit->date > revs->min_age))
>> return commit_ignore;
>> - if (revs->no_merges && commit->parents && commit->parents->next)
>> - return commit_ignore;
>> - if (revs->merges_only && !(commit->parents && commit->parents->next))
>> - return commit_ignore;
>> + if (revs->min_parents || revs->max_parents) {
>> + int n = 0;
>> + struct commit_list *p;
>> + for (p = commit->parents; p; p = p->next)
>> + n++;
>> + if ((MIN_PARENTS(n) < revs->min_parents) ||
>> + (MAX_PARENTS(n) < revs->max_parents)) /* max is inv. */
>> + return commit_ignore;
>> + }
>
> You did the obvious optimization not to count parents if we don't care
> about min/max. You could also do something like:
>
> switch (revs->max_parents) {
> case 0:
> if (commit->parents)
> return commit_ignore;
> break;
> case 1:
> if (commit->parents && commit->parents->next)
> return commit_ignore;
> break;
> default:
> if (count_parents(commit) > commit->max_parents)
> return commit_ignore;
> break;
> }
>
> which more closely matches the original code (you would also need to do
> the same for min_parents, and obviously this code ignores the
> max-parents-is-inverted thing).
>
> I'm not sure it would buy much in practice, though. Commits with more
> than 2 parents are rare, so unnecessarily counting all of them just to
> find out something is a merge is probably not going to create a
> measurable slowdown.
That was exactly my thinking.
>
>> +/* limit to used range */
>> +#define MIN_PARENTS(m) ({ unsigned int __n = (m); (__n < 0) ? 0 : (__n > 7) ? 7 : __n; })
>> +/* invert fox MAX so that default = 0 -> infinity */
>> +#define MAX_PARENTS(m) ({ unsigned int __n = (m); (__n < 0) ? 7 : (__n > 7) ? 0 : 7 - __n;})
>
> Hmm. You assign the input to an unsigned int, and then compare it to 0.
> Won't that always be false?
Yes, sorry. I had an int there first, the unsigned gives the "negative =
infinity" automatically. I probably couldn't decide what
--min-parents=-1 should mean: should "-1" always denote "infinity", or
should it be equivalent to "0"? Switching from signed to unsigned
changed this.
> The inversion trick is clever to make a bzero'd rev_info work, but I
> think the code would be more obvious without it. And you really have to
> call init_revisions() anyway, so initializing it to a maximum there (or
> -1) would be acceptable (e.g., see how max_age and min_age work).
I think for all users of the code it doesn't make a difference since
they would call the macros anyways (for the range check), so it would be
only that single comparison in the walker (which has a comment) that
would become clearer.
Michael
--
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