On Fri, 10 Feb 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@xxxxxxxxx> writes:
>
>>> The lack of any real use of @fill_only in this patch also makes it hard to
>>> judge if the new API gives a useful semantics. I would, without looking
>>> at the real usage in 2/5 patch, naïvely expect that such a lazy filling
>>> scheme would say "I am going to use A, B and C; I want to know if any of
>>> them is missing, because I need values for all of them and I am going to
>>> call a helper function to fill them if any of them is missing. Having A
>>> and B is not enough for the purpose of this query, because I still need to
>>> know C and I would call the helper function that computes all of them in
>>> such a case. Even though it might be wasteful to recompute A and B,
>>> computing all three at once is the only helper function available to me".
>>>
>>> So for a person who does not have access to the real usage of the new API,
>>> being able to give only a single $key *appears* make no sense at all, and
>>> also the meaning of the @fill_only parameter is unclear, especially the
>>> part that checks if that single $key appears in @fill_only.
>>
>> ...
>> information that is not already present. If @fill_only is nonempty, it
>> fills only selected information, again only if it is not already present.
>> @fill_only empty means no restrictions... which probably is not very obvious,
>> but is documented.
>>
>> project_info_needs_filling() returns true if $key is not filled and is
>> interesting.
>
> That still does not answer the fundamental issues I had with the presented
> API: why does it take only a single $key (please re-read my "A, B and C"
> example), and what does that single $key intersecting with @fill_only have
> anything to do with "needs-filling"?
project_info_needs_filling() in absence of @fill_only is just a thin
wrapper around "!defined $pr->{$key}", it checks for each key if it needs
to be filled.
It is used like this
if (project_info_needs_filled("A", "A, B, C")) {
fill A
}
if (project_info_needs_filled("B", "A, B, C")) {
fill B
}
...
> After all, that 'age' check actually wants to fill 'age' and 'age_string'
> in the project. Even if some other codepath starts filling 'age' in the
> project with a later change, the current callers of fill_project_list_info
> expects _both_ to be filled. So "I know the current implementation fills
> both at the same time, so checking 'age' alone is sufficient" is not an
> answer that shows good taste in the API design.
It is not as much matter of API, as the use of checks in loop in
fill_project_list_info().
What is now
my (@activity) = git_get_last_activity($pr->{'path'});
unless (@activity) {
next PROJECT;
}
($pr->{'age'}, $pr->{'age_string'}) = @activity;
should be
if (!defined $pr->{'age'} ||
!defined $pr->{'age_string'}) {
my (@activity) = git_get_last_activity($pr->{'path'});
unless (@activity) {
next PROJECT;
}
($pr->{'age'}, $pr->{'age_string'}) = @activity;
}
which would translate to
if (project_info_needs_filled($pr, 'age') ||
project_info_needs_filled($pr, 'age_string') {
my (@activity) = git_get_last_activity($pr->{'path'});
unless (@activity) {
next PROJECT;
}
($pr->{'age'}, $pr->{'age_string'}) = @activity;
}
and then with @fill_only
if (project_info_needs_filled($pr, 'age', @fill_only) ||
project_info_needs_filled($pr, 'age_string', @fill_only) {
my (@activity) = git_get_last_activity($pr->{'path'});
unless (@activity) {
next PROJECT;
}
($pr->{'age'}, $pr->{'age_string'}) = @activity;
}
The same should be done for 'descr_long' and 'descr' which are also
always filled together.
--
Jakub Narebski
Poland
--
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