| Message ID | patch-v7-4.6-2e5511c9fa5-20220111T130811Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | object-name: make ambiguous object output translatable + show tag date | expand |
Æ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". > } else if (type == OBJ_TREE) { > /* > * TRANSLATORS: This is a line of ambiguous <type>
On Thu, Jan 13 2022, Junio C Hamano wrote: > Æ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>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > 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. Writing the above (and quoting it again to make me respond to it) have already wasted a lot more time than a better solution that does not lead to a misleading output, especially given that it was given for free to you already.
On Fri, Jan 14 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> 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. > > Writing the above (and quoting it again to make me respond to it) > have already wasted a lot more time than a better solution that does > not lead to a misleading output, especially given that it was given > for free to you already. I don't mind changing it, but the reason I re-quoted it is because your reply seemed to suggest that you had skimmed past that part before making your original comment, not to merely repeat myself. I.e. it's basically suggesting "how about?..." without addressing the "I intentionally didn't do this, because..." argument in the commit message. But sure, I'll add a translatable message for this edge case in a re-roll.
diff --git a/object-name.c b/object-name.c index 743f346842a..7c6cb60ceff 100644 --- a/object-name.c +++ b/object-name.c @@ -403,20 +403,25 @@ static int show_ambiguous_object(const struct object_id *oid, void *data) } else if (type == OBJ_TAG) { struct tag *tag = lookup_tag(ds->repo, oid); const char *tag_tag = ""; + timestamp_t tag_date = 0; - if (!parse_tag(tag) && tag->tag) + if (!parse_tag(tag) && tag->tag) { tag_tag = tag->tag; + tag_date = tag->date; + } /* * 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); } else if (type == OBJ_TREE) { /* * TRANSLATORS: This is a line of ambiguous <type>
Make the ambiguous tag object output nicer in the case of tag objects such as ebf3c04b262 (Git 2.32, 2021-06-06) by including the date in the "tagger" header. I.e.: $ git rev-parse b7e68 error: short object ID b7e68 is ambiguous hint: The candidates are: hint: b7e68c41d92 tag 2021-06-06 - v2.32.0 hint: b7e68ae18e0 commit 2019-12-23 - bisect: use the standard 'if (!var)' way to check for 0 hint: b7e68f6b413 tree hint: b7e68490b97 blob b7e68 [...] Before this we'd emit a "tag" line of: hint: b7e68c41d92 tag v2.32.0 As with OBJ_COMMIT we punt on the cases where the date in the object is nonsensical, and other cases where parse_tag() might fail. For those we'll use our default date of "0" and tag message of "". E.g. for some of the corrupt tags created by t3800-mktag.sh we'd emit a line like: hint: 8d62cb0b06 tag 1970-01-01 - 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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- object-name.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)