Re: [PATCH 1/3] revision.c: introduce --min-parents and --max-parents

[Michael J Gruber <git@drmicha.warpmail.net>]

Jonathan Nieder venit, vidit, dixit 18.03.2011 21:48:
> Hi,
> 
> Michael J Gruber wrote:
> 
>> --max-parents=1: no merges
>> --min-parents=2: merges only
>> --max-parents=0: only roots
>> --min-parents=3: only octopusses
> 
> This is growing on me.  Thanks for inventing it.

Thanks for teaching me a new collocation (to grow on sb) ;)

> 
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1061,7 +1061,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	rev.commit_format = CMIT_FMT_EMAIL;
>>  	rev.verbose_header = 1;
>>  	rev.diff = 1;
>> -	rev.no_merges = 1;
>> +	rev.max_parents = MAX_PARENTS(1);
> 
> Is there a reason not to choose a convention for which
> 
> 	rev.max_parents = 1;
> 
> works?

Yes.

Oh, you also want to know what it is? I was somehow fixed on using a
limited number of bits (probably because of no_merges etc. and the
mentioning of tri-state), therefore using a bounded range.

Also, I was keen of having "8" be infinity. (My fingers are trained to
enter "`8" to get "\infty".) But I've come to realise I'm the only one.

> 
> What does --no-merges --merges do?  I would find it most intuitive to
> error out (since some people would want the last choice to win and
> others want --merges-only --nonmerges-only to select the empty set),
> but this patch does the backward-compatible thing, which is to show

Yes, a true case of being "backward(!)-compatible"...

> zero commits.  Maybe it deserves a test case?
> 
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1277,9 +1277,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>>  	} else if (!strcmp(arg, "--remove-empty")) {
>>  		revs->remove_empty_trees = 1;
>>  	} else if (!strcmp(arg, "--merges")) {
>> -		revs->merges_only = 1;
>> +		revs->min_parents = MIN_PARENTS(2);
> 
> Why not "revs->min_parents = 2;"?

For consistency.

> 
>>  	} else if (!strcmp(arg, "--no-merges")) {
>> -		revs->no_merges = 1;
>> +		revs->max_parents = MAX_PARENTS(1);
>> +	} else if (!prefixcmp(arg, "--min-parents=")) {
>> +		revs->min_parents = MIN_PARENTS(atoi(arg+14));
>> +	} else if (!prefixcmp(arg, "--max-parents=")) {
>> +		revs->max_parents = MAX_PARENTS(atoi(arg+14));
> 
> It would be nicer to error out for malformed numbers.  That's
> a separate topic, though --- you have plenty of company.

min_age etc., yes.

> 
>> @@ -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;
> 
> Sane.  If we feared enormous parent lists we could do
> 
> 	for (p = commit->parents; p && n <= 7 - revs->max_parents; p = p->next)
> 		n++;
> 
> but I suspect that's slower.
> 
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -20,6 +20,11 @@
>>  #define DECORATE_SHORT_REFS	1
>>  #define DECORATE_FULL_REFS	2
>>  
>> +/* 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;})
> 
> Statement expressions don't work in most non-gcc compilers (but
> inline functions do).
> 
> Hope that helps,
> Jonathan

Yes, that would have helped, although I'm going for simple "unbounded"
int's in v2 without the need for those range enforcing macros/inlines.

Am I allowed to do

unsigned max_parent = -1;

to get "infinity for practical purposes" on all compilers?

Michael