Re: [PATCH v4 1/7] bulk-checkin: extract abstract `bulk_checkin_source`

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2023, 16:55 UTC
Message-ID: <xmqqlebxs06n.fsf@gitster.g>
In-Reply-To: <20231020073500.GE1642714@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 16 quoted lines

> On Thu, Oct 19, 2023 at 01:28:42PM -0400, Taylor Blau wrote:
>
>> +struct bulk_checkin_source {
>> +	enum { SOURCE_FILE } type;
>> +
>> +	/* SOURCE_FILE fields */
>> +	int fd;
>> +
>> +	/* common fields */
>> +	size_t size;
>> +	const char *path;
>> +};
>
> This is a pretty minor nit, but we may find that "SOURCE_FILE" is not
> sufficiently name-spaced. Enum tags are in the global namespace, so
> the compiler will barf if there are any conflicts.

A very good point. This was one of the reasons why I suggested avoiding the switch() based on a new enum altogether and use a vtable based approach instead. But it may do while this one is private to the file and never exposed to other subsystems.

Show 7 quoted lines

> It might be OK here, since this is local to a single C file (so we at
> least are not hurting other code), but we may be in trouble if code in a
> header file is less careful. There is already a near-miss here with
> GREP_SOURCE_FILE, but fortunately grep.h is indeed careful. :)
>
> (I notice that ref-filter.c similarly uses SOURCE_* for an internal
> enum).

Thanks.