Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2017, 18:49 UTC
Message-ID: <xmqqziio9paj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170118182843.iyico5jpii6a3z7i@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index f81273a85a..fbb85ba3dc 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -66,10 +66,10 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
>>  }
>>  
>>  typedef int (*each_tag_name_fn)(const char *name, const char *ref,
>> -				const unsigned char *sha1, void *cb_data);
>> +				const unsigned char *sha1, const void *cb_data);
>
> This would bite us later if one of the iterators really does need to
> pass something mutable. But as this iteration interface is confined to
> builtin/tag.c, I think it's a nice simple fix.
>
> A more general fix would be to pass a non-const pointer to const pointer
> (preferably inside a struct for readability). But I don't see any need
> for that complexity here.

My first trial was to loosen the constness of existing variable, which was OK, but made me feel dirty by turning what does not need to be mutable into mutable. The iterator being local made me try the other way and it turned out that currently there is no need for mutable callback data ;-)

I agree that this may have to be updated, and if this were more global thing, we'd better off doing so from the get-go, but for a calling convention that is limited within a single file, I am more comfortable saying we'll cross the bridge when we need to.

Thanks.