Re: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 16, 2020, 04:53 UTC
Message-ID: <xmqq8sdaxqyu.fsf@gitster.c.googlers.com>
In-Reply-To: <20200916020840.84892-1-alipman88@gmail.com>

Aaron Lipman <alipman88@gmail.com> writes:

Show 8 quoted lines

>> I do not mind making the 0/1 a symbolic constant between
>> do_merge_filter() and filter_refs() for enhanced readability,
>> though.
>
> If I understand the convention, the constant names should be prefixed
> with DO_MERGE_FILTER. I suggest DO_MERGE_FILTER_REACHABLE and
> DO_MERGE_FILTER_UNREACHABLE. Happy to re-roll if others have a
> different preference - or feel free to edit.)

Even though I said "I do not mind", using DO_MERGE_FILTER prefix is way too much noise. The common prefix is appropriate for external API, but this is merely an internal contract between a private helper do_merge_filter() and its only caller.

If I were redesigning the code around this area, I probably would rename do_merge_filter() to something more descriptive, and tweak its parameters a bit more focused, e.g.

    #define EXCLUDE_REACHED 0
    #define INCLUDE_REACHED 1
    static void reach_filter(struct ref_array *array, 
			     struct commit_list *check_reachable,
			     int include_reached) {
		...
		for (i = 0; i < old_nr; i++) {
			struct commit *c = array->items[i];
			int is_reached = !!(c->object.flags & UNINTERESTING);

			if (is_reached == include_reached)
				array->items[array->nr++] = array->items[i];
			else
				free_array_item(array->items[i]);
		}
		...
    }

    /* the caller */
    ...
    reach_filter(array, filter->reachable_from, INCLUDE_REACHED);
    reach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);

to make it clear which part of the callback struct is really passed between the caller and the helper. Even if we are not renaming things that much, a locally defined preprocessor macro with shorter names, defined just before the callee, would be more appropriate for a case like this, with a single callee called by only one caller.

Thanks.