Re: [PATCH v7 4/6] object-name: show date for ambiguous tag objects

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 14, 2022, 12:05 UTC
Message-ID: <220114.865yqmtt9z.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq1r1bgso2.fsf@gitster.g>

On Thu, Jan 13 2022, Junio C Hamano wrote:

Show 47 quoted lines

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>>  	} else if (type == OBJ_TAG) {
>>  		struct tag *tag = lookup_tag(ds->repo, oid);
>>  		const char *tag_tag = "";
>> +		timestamp_t tag_date = 0;
>
> How about leaving these two uninitialized and introduce one extra
> bool,
> 		int tag_info_valid = 0;
>
> and then
>
>>  
>> -		if (!parse_tag(tag) && tag->tag)
>> +		if (!parse_tag(tag) && tag->tag) {
>>  			tag_tag = tag->tag;
>> +			tag_date = tag->date;
>
> 			tag_info_valid = 1;
>
>> +		}
>>  
>>  		/*
>>  		 * TRANSLATORS: This is a line of
>>  		 * ambiguous tag object output. E.g.:
>>  		 *
>> -		 *    "deadbeef tag Some Tag Message"
>> +		 *    "deadbeef tag 2021-01-01 - Some Tag Message"
>>  		 *
>>  		 * The second argument is the "tag" string from
>>  		 * object.c.
>>  		 */
>> -		strbuf_addf(&desc, _("%s tag %s"), hash, tag_tag);
>> +		strbuf_addf(&desc, _("%s tag %s - %s"), hash,
>> +			    show_date(tag_date, 0, DATE_MODE(SHORT)),
>> +			    tag_tag);
>
> Then this part can use tag_info_valid to conditionally use tag_date
> and tag_tag:
>
> 		if (tag_info_valid)
> 			strbuf_addf(&desc, ... <hash,date,tag>);
> 		else
> 			strbuf_addf(&desc, _("%s tag [bad]"), hash);
>
> without throwing a misleading "In 1970 this happened".

I still think the trade-off of not doing that discussed in the commit
message is better, i.e. (to quote upthread):
    
    We could detect that and emit a "%s [bad tag object]" message (to go
    with the existing generic "%s [bad object]"), but I don't think it's
    worth the effort. Users are unlikely to ever run into cases where
    they've got a broken object that's also ambiguous, and in case they do
    output that's a bit nonsensical beats wasting translator time on this
    obscure edge case.
    
    We should instead change parse_tag_buffer() to be more eager to emit
    an error() instead of silently aborting with "return -1;". In the case
    of "t3800-mktag.sh" it takes the "size < the_hash_algo->hexsz + 24"
    branch.

This really is so obscure that I don't think it warrants having N translators re-translate this message users are very likely never to see, ever.

And to the extent that they will see anything I've got some
planned/upcoming changes to make some of the underlying object machinery
emit better diagnostic messages on these bad objects, which would hint
in the general case about what's going wrong, instead of needing
ambiguous-object-display-specific messaging.

>>  	} else if (type == OBJ_TREE) {
>>  		/*
>>  		 * TRANSLATORS: This is a line of ambiguous <type>