From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 16 Sep 2020 04:53:13 GMT
Subject: Re: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters
Message-ID: <xmqq8sdaxqyu.fsf@gitster.c.googlers.com>
In-Reply-To: <20200916020840.84892-1-alipman88@gmail.com>

Aaron Lipman <alipman88@gmail.com> writes:

>> 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.