* [PATCH v6 1/6] object-name tests: add tests for ambiguous object blind spots
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
@ 2021-12-28 14:34 ` Ævar Arnfjörð Bjarmason
2021-12-30 23:36 ` Junio C Hamano
2021-12-28 14:34 ` [PATCH v6 2/6] object-name: explicitly handle OBJ_BAD in show_ambiguous_object() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 14:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Extend the tests for ambiguous objects to check how we handle objects
where we return OBJ_BAD when trying to parse them. As noted in [1] we
have a blindspot when it comes to this behavior.
Since we need to add new test data here let's extend these tests to be
tested under SHA-256, in d7a2fc82491 (t1512: skip test if not using
SHA-1, 2018-05-13) all of the existing tests were skipped, as they
rely on specific SHA-1 object IDs.
For these tests it only matters that the first 4 characters of the OID
prefix are the same for both SHA-1 and SHA-256. This uses strings that
I mined, and have the same prefix when hashed with both.
We "test_cmp" the full output to guard against any future regressions,
and because a subsequent commit will tweak it. Showing a diff of how
the output changes is helpful to explain those subsequent commits.
1. https://lore.kernel.org/git/YZwbphPpfGk78w2f@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t1512-rev-parse-disambiguation.sh | 85 +++++++++++++++++++++++++++++
1 file changed, 85 insertions(+)
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 7891a6becf3..60d2a457067 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -25,6 +25,91 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+test_cmp_failed_rev_parse () {
+ dir=$1
+ rev=$2
+ shift
+
+ test_must_fail git -C "$dir" rev-parse "$rev" 2>actual.raw &&
+ sed "s/\($rev\)[0-9a-f]*/\1.../g" <actual.raw >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'ambiguous blob output' '
+ git init --bare blob.prefix &&
+ (
+ cd blob.prefix &&
+
+ # Both start with "dead..", under both SHA-1 and SHA-256
+ echo brocdnra | git hash-object -w --stdin &&
+ echo brigddsv | git hash-object -w --stdin &&
+
+ # Both start with "beef.."
+ echo 1agllotbh | git hash-object -w --stdin &&
+ echo 1bbfctrkc | git hash-object -w --stdin
+ ) &&
+
+ test_must_fail git -C blob.prefix rev-parse dead &&
+ cat >expect <<-\EOF &&
+ error: short object ID beef... is ambiguous
+ hint: The candidates are:
+ hint: beef... blob
+ hint: beef... blob
+ fatal: ambiguous argument '\''beef...'\'': unknown revision or path not in the working tree.
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
+ test_cmp_failed_rev_parse blob.prefix beef
+'
+
+test_expect_success 'ambiguous loose blob parsed as OBJ_BAD' '
+ git init --bare blob.bad &&
+ (
+ cd blob.bad &&
+
+ # Both have the prefix "bad0"
+ echo xyzfaowcoh | git hash-object -t bad -w --stdin --literally &&
+ echo xyzhjpyvwl | git hash-object -t bad -w --stdin --literally
+ ) &&
+
+ cat >expect <<-\EOF &&
+ error: short object ID bad0... is ambiguous
+ hint: The candidates are:
+ fatal: invalid object type
+ EOF
+ test_cmp_failed_rev_parse blob.bad bad0
+'
+
+test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
+ git init --bare blob.corrupt &&
+ (
+ cd blob.corrupt &&
+
+ # Both have the prefix "cafe"
+ echo bnkxmdwz | git hash-object -w --stdin &&
+ oid=$(echo bmwsjxzi | git hash-object -w --stdin) &&
+
+ oidf=objects/$(test_oid_to_path "$oid") &&
+ chmod 755 $oidf &&
+ echo broken >$oidf
+ ) &&
+
+ cat >expect <<-\EOF &&
+ error: short object ID cafe... is ambiguous
+ hint: The candidates are:
+ error: inflate: data stream error (incorrect header check)
+ error: unable to unpack cafe... header
+ error: inflate: data stream error (incorrect header check)
+ error: unable to unpack cafe... header
+ hint: cafe... unknown type
+ hint: cafe... blob
+ fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
+ test_cmp_failed_rev_parse blob.corrupt cafe
+'
+
if ! test_have_prereq SHA1
then
skip_all='not using SHA-1 for objects'
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v6 1/6] object-name tests: add tests for ambiguous object blind spots
2021-12-28 14:34 ` [PATCH v6 1/6] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
@ 2021-12-30 23:36 ` Junio C Hamano
0 siblings, 0 replies; 90+ messages in thread
From: Junio C Hamano @ 2021-12-30 23:36 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> +test_cmp_failed_rev_parse () {
> + dir=$1
> + rev=$2
> + shift
What are we shifting away?
> + test_must_fail git -C "$dir" rev-parse "$rev" 2>actual.raw &&
> + sed "s/\($rev\)[0-9a-f]*/\1.../g" <actual.raw >actual &&
I wonder if we need to ensure not to mistakenly produce second hit
in an object name that has $rev twice, e.g. "cafe123cafe..."?
> + test_cmp expect actual
> +}
It is a bit confusing to _depend_ on the caller to prepare a
fixed-name file, like this. We've avoided such confusion in
different ways in other tests, like (A) make the helper take
the expected output from its standard input, or (B) make the
helper take the name of the file that has expected output as
its argument.
> +test_expect_success 'ambiguous blob output' '
> + git init --bare blob.prefix &&
> + (
> + cd blob.prefix &&
> +
> + # Both start with "dead..", under both SHA-1 and SHA-256
> + echo brocdnra | git hash-object -w --stdin &&
> + echo brigddsv | git hash-object -w --stdin &&
> +
> + # Both start with "beef.."
> + echo 1agllotbh | git hash-object -w --stdin &&
> + echo 1bbfctrkc | git hash-object -w --stdin
> + ) &&
> +
> + test_must_fail git -C blob.prefix rev-parse dead &&
> + cat >expect <<-\EOF &&
> + error: short object ID beef... is ambiguous
> + hint: The candidates are:
> + hint: beef... blob
> + hint: beef... blob
> + fatal: ambiguous argument '\''beef...'\'': unknown revision or path not in the working tree.
> + Use '\''--'\'' to separate paths from revisions, like this:
> + '\''git <command> [<revision>...] -- [<file>...]'\''
> + EOF
> + test_cmp_failed_rev_parse blob.prefix beef
> +'
> +
> +test_expect_success 'ambiguous loose blob parsed as OBJ_BAD' '
"loose bad object", as they aren't even blobs, perhaps?
> + git init --bare blob.bad &&
> + (
> + cd blob.bad &&
> +
> + # Both have the prefix "bad0"
> + echo xyzfaowcoh | git hash-object -t bad -w --stdin --literally &&
> + echo xyzhjpyvwl | git hash-object -t bad -w --stdin --literally
> + ) &&
> +
> + cat >expect <<-\EOF &&
> + error: short object ID bad0... is ambiguous
> + hint: The candidates are:
> + fatal: invalid object type
That indeed is not very nice.
> + EOF
> + test_cmp_failed_rev_parse blob.bad bad0
> +'
> +
> +test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
> + git init --bare blob.corrupt &&
> + (
> + cd blob.corrupt &&
> +
> + # Both have the prefix "cafe"
> + echo bnkxmdwz | git hash-object -w --stdin &&
> + oid=$(echo bmwsjxzi | git hash-object -w --stdin) &&
> +
> + oidf=objects/$(test_oid_to_path "$oid") &&
> + chmod 755 $oidf &&
> + echo broken >$oidf
> + ) &&
> +
> + cat >expect <<-\EOF &&
> + error: short object ID cafe... is ambiguous
> + hint: The candidates are:
> + error: inflate: data stream error (incorrect header check)
> + error: unable to unpack cafe... header
> + error: inflate: data stream error (incorrect header check)
> + error: unable to unpack cafe... header
> + hint: cafe... unknown type
> + hint: cafe... blob
This is an interesting one. I _think_ it is clear enough for the
readers that the inflate errors are for the object that immediately
follows them, so as long as we show these hints one by one, the
above output is perfectly fine. But we'll see.
> + fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
> + Use '\''--'\'' to separate paths from revisions, like this:
> + '\''git <command> [<revision>...] -- [<file>...]'\''
> + EOF
> + test_cmp_failed_rev_parse blob.corrupt cafe
> +'
> +
> if ! test_have_prereq SHA1
> then
> skip_all='not using SHA-1 for objects'
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v6 2/6] object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
2021-12-28 14:34 ` [PATCH v6 1/6] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
@ 2021-12-28 14:34 ` Ævar Arnfjörð Bjarmason
2021-12-28 14:34 ` [PATCH v6 3/6] object-name: make ambiguous object output translatable Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 14:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Amend the "unknown type" handling in the code that displays the
ambiguous object list to assert() that we're either going to get the
"real" object types we can pass to type_name(), or a -1 (OBJ_BAD)
return value from oid_object_info().
See [1] for the current output, and [1] for the commit that added the
"unknown type" handling.
We are never going to get an "unknown type" in the sense of custom
types crafted with "hash-object --literally", since we're not using
the OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag.
If we manage to otherwise unpack such an object without errors we'll
die() in parse_loose_header_extended() called by sort_ambiguous()
before we get to show_ambiguous_object(), as is asserted by the test
added in the preceding commit.
So saying "unknown type" here was always misleading, we really meant
to say that we had a failure parsing the object at all, i.e. that we
had repository corruption. If the problem is only that it's type is
unknown we won't reach this code.
So let's emit a generic "[bad object]" instead. As our tests added in
the preceding commit show, we'll have emitted various "error" output
already in those cases.
We should do better in the truly "unknown type" cases, which we'd need
to handle if we were passing down the OBJECT_INFO_ALLOW_UNKNOWN_TYPE
flag. But let's leave that for some future improvement. In a
subsequent commit I'll improve the output we do show, and not having
to handle the "unknown type" (as in OBJECT_INFO_ALLOW_UNKNOWN_TYPE)
simplifies that change.
1. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
2. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 14 ++++++++++++--
t/t1512-rev-parse-disambiguation.sh | 2 +-
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..9750634ee76 100644
--- a/object-name.c
+++ b/object-name.c
@@ -361,6 +361,16 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
return 0;
type = oid_object_info(ds->repo, oid, NULL);
+
+ if (type < 0) {
+ strbuf_addstr(&desc, "[bad object]");
+ goto out;
+ }
+
+ assert(type == OBJ_TREE || type == OBJ_COMMIT ||
+ type == OBJ_BLOB || type == OBJ_TAG);
+ strbuf_addstr(&desc, type_name(type));
+
if (type == OBJ_COMMIT) {
struct commit *commit = lookup_commit(ds->repo, oid);
if (commit) {
@@ -374,9 +384,9 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
strbuf_addf(&desc, " %s", tag->tag);
}
- advise(" %s %s%s",
+out:
+ advise(" %s %s",
repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV),
- type_name(type) ? type_name(type) : "unknown type",
desc.buf);
strbuf_release(&desc);
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 60d2a457067..d68c411bfc7 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -101,7 +101,7 @@ test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
error: unable to unpack cafe... header
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
- hint: cafe... unknown type
+ hint: cafe... [bad object]
hint: cafe... blob
fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
Use '\''--'\'' to separate paths from revisions, like this:
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v6 3/6] object-name: make ambiguous object output translatable
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
2021-12-28 14:34 ` [PATCH v6 1/6] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
2021-12-28 14:34 ` [PATCH v6 2/6] object-name: explicitly handle OBJ_BAD in show_ambiguous_object() Ævar Arnfjörð Bjarmason
@ 2021-12-28 14:34 ` Ævar Arnfjörð Bjarmason
2021-12-30 23:46 ` Junio C Hamano
2021-12-28 14:35 ` [PATCH v6 4/6] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 14:34 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Change the output of show_ambiguous_object() added in [1] and last
tweaked in [2] and the preceding commit to be more friendly to
translators.
By being able to customize the "<SP><SP>%s\n" format we're even ready
for RTL languages, who'd presumably like to change that to
"%s<SP><SP>\n".
1. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
2. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
object-name.c | 65 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 58 insertions(+), 7 deletions(-)
diff --git a/object-name.c b/object-name.c
index 9750634ee76..dcf3ab99990 100644
--- a/object-name.c
+++ b/object-name.c
@@ -356,38 +356,89 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
const struct disambiguate_state *ds = data;
struct strbuf desc = STRBUF_INIT;
int type;
+ const char *hash;
if (ds->fn && !ds->fn(ds->repo, oid, ds->cb_data))
return 0;
+ hash = repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV);
type = oid_object_info(ds->repo, oid, NULL);
if (type < 0) {
- strbuf_addstr(&desc, "[bad object]");
+ /*
+ * TRANSLATORS: This is a line of ambiguous object
+ * output shown when we cannot look up or parse the
+ * object in question. E.g. "deadbeef [bad object]".
+ */
+ strbuf_addf(&desc, _("%s [bad object]"), hash);
goto out;
}
assert(type == OBJ_TREE || type == OBJ_COMMIT ||
type == OBJ_BLOB || type == OBJ_TAG);
- strbuf_addstr(&desc, type_name(type));
if (type == OBJ_COMMIT) {
+ struct strbuf date = STRBUF_INIT;
+ struct strbuf msg = STRBUF_INIT;
struct commit *commit = lookup_commit(ds->repo, oid);
+
if (commit) {
struct pretty_print_context pp = {0};
pp.date_mode.type = DATE_SHORT;
- format_commit_message(commit, " %ad - %s", &desc, &pp);
+ format_commit_message(commit, "%ad", &date, &pp);
+ format_commit_message(commit, "%s", &msg, &pp);
}
+
+ /*
+ * TRANSLATORS: This is a line of ambiguous commit
+ * object output. E.g.:
+ *
+ * "deadbeef commit 2021-01-01 - Some Commit Message"
+ */
+ strbuf_addf(&desc, _("%s commit %s - %s"),
+ hash, date.buf, msg.buf);
+
+ strbuf_release(&date);
+ strbuf_release(&msg);
} else if (type == OBJ_TAG) {
struct tag *tag = lookup_tag(ds->repo, oid);
+ const char *tag_tag = "";
+
if (!parse_tag(tag) && tag->tag)
- strbuf_addf(&desc, " %s", tag->tag);
+ tag_tag = tag->tag;
+
+ /*
+ * TRANSLATORS: This is a line of
+ * ambiguous tag object output. E.g.:
+ *
+ * "deadbeef tag Some Tag Message"
+ *
+ * The second argument is the "tag" string from
+ * object.c, it should (hopefully) already be
+ * translated.
+ */
+ strbuf_addf(&desc, _("%s tag %s"), hash, tag_tag);
+ } else if (type == OBJ_TREE) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous <type>
+ * object output. E.g. "deadbeef tree".
+ */
+ strbuf_addf(&desc, _("%s tree"), hash);
+ } else if (type == OBJ_BLOB) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous <type>
+ * object output. E.g. "deadbeef blob".
+ */
+ strbuf_addf(&desc, _("%s blob"), hash);
}
+
out:
- advise(" %s %s",
- repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV),
- desc.buf);
+ /*
+ * TRANSLATORS: This is line item of ambiguous object output
+ * from describe_ambiguous_object() above.
+ */
+ advise(_(" %s"), desc.buf);
strbuf_release(&desc);
return 0;
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v6 3/6] object-name: make ambiguous object output translatable
2021-12-28 14:34 ` [PATCH v6 3/6] object-name: make ambiguous object output translatable Ævar Arnfjörð Bjarmason
@ 2021-12-30 23:46 ` Junio C Hamano
0 siblings, 0 replies; 90+ messages in thread
From: Junio C Hamano @ 2021-12-30 23:46 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> + /*
> + * TRANSLATORS: This is a line of
> + * ambiguous tag object output. E.g.:
> + *
> + * "deadbeef tag Some Tag Message"
> + *
> + * The second argument is the "tag" string from
> + * object.c, it should (hopefully) already be
> + * translated.
> + */
> + strbuf_addf(&desc, _("%s tag %s"), hash, tag_tag);
It is better to lose ", it should (hopefully) already be translated"
near the end of the comment.
> + } else if (type == OBJ_TREE) {
> + /*
> + * TRANSLATORS: This is a line of ambiguous <type>
> + * object output. E.g. "deadbeef tree".
> + */
> + strbuf_addf(&desc, _("%s tree"), hash);
> + } else if (type == OBJ_BLOB) {
> + /*
> + * TRANSLATORS: This is a line of ambiguous <type>
> + * object output. E.g. "deadbeef blob".
> + */
> + strbuf_addf(&desc, _("%s blob"), hash);
> }
>
> +
> out:
> - advise(" %s %s",
> - repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV),
> - desc.buf);
> + /*
> + * TRANSLATORS: This is line item of ambiguous object output
> + * from describe_ambiguous_object() above.
> + */
> + advise(_(" %s"), desc.buf);
What do we expect the translators to do here? Swap order of the
leading space and the string around?
All the other sentence legos we see in the earlier part of this
patch (omitted) looked quite sensibly done. Especially the part
that shows a commit object, which lets the translators to take the
object name, the date string, and the message and combine them into
a single string in an order of their choice is nice.
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v6 4/6] object-name: show date for ambiguous tag objects
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-12-28 14:34 ` [PATCH v6 3/6] object-name: make ambiguous object output translatable Ævar Arnfjörð Bjarmason
@ 2021-12-28 14:35 ` Ævar Arnfjörð Bjarmason
2021-12-30 21:43 ` Junio C Hamano
2021-12-28 14:35 ` [PATCH v6 5/6] object-name: iterate ambiguous objects before showing header Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 14:35 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
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
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/object-name.c b/object-name.c
index dcf3ab99990..990f384129e 100644
--- a/object-name.c
+++ b/object-name.c
@@ -403,21 +403,26 @@ 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, it should (hopefully) already be
* translated.
*/
- 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>
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v6 4/6] object-name: show date for ambiguous tag objects
2021-12-28 14:35 ` [PATCH v6 4/6] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
@ 2021-12-30 21:43 ` Junio C Hamano
0 siblings, 0 replies; 90+ messages in thread
From: Junio C Hamano @ 2021-12-30 21:43 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> diff --git a/object-name.c b/object-name.c
> index dcf3ab99990..990f384129e 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -403,21 +403,26 @@ 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, it should (hopefully) already be
> * translated.
> */
> - 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);
So, when parse_tag() errors out, we show "" and epoch? We should be
able to do a better error reporting than that; tag_tag and tag_date
are both local and they do not have to be used to store sentinel values
like that. Instead perhaps remember that we failed to parse_tag(),
and _omit_ unavailable piece of information from the output? I dunno.
> } else if (type == OBJ_TREE) {
> /*
> * TRANSLATORS: This is a line of ambiguous <type>
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v6 5/6] object-name: iterate ambiguous objects before showing header
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-12-28 14:35 ` [PATCH v6 4/6] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
@ 2021-12-28 14:35 ` Ævar Arnfjörð Bjarmason
2021-12-28 14:35 ` [PATCH v6 6/6] object-name: re-use "struct strbuf" in show_ambiguous_object() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 14:35 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Change the "The candidates are" header that's shown for ambiguous
objects to be shown after we've iterated over all of the objects.
If we get any errors while doing so we don't want to split up the the
header and the list as a result. The two will now be printed together,
as shown in the updated testcase.
As we're accumulating the lines into as "struct strbuf" before
emitting them we need to add a trailing newline to the call in
show_ambiguous_object(). This and the change from "The candidates
are:" to "The candidates are:\n%s" helps to give translators more
context.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 27 +++++++++++++++++++++++----
t/t1512-rev-parse-disambiguation.sh | 3 +--
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/object-name.c b/object-name.c
index 990f384129e..743d272800d 100644
--- a/object-name.c
+++ b/object-name.c
@@ -351,9 +351,16 @@ static int init_object_disambiguation(struct repository *r,
return 0;
}
+struct ambiguous_output {
+ const struct disambiguate_state *ds;
+ struct strbuf advice;
+};
+
static int show_ambiguous_object(const struct object_id *oid, void *data)
{
- const struct disambiguate_state *ds = data;
+ struct ambiguous_output *state = data;
+ const struct disambiguate_state *ds = state->ds;
+ struct strbuf *advice = &state->advice;
struct strbuf desc = STRBUF_INIT;
int type;
const char *hash;
@@ -443,7 +450,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* TRANSLATORS: This is line item of ambiguous object output
* from describe_ambiguous_object() above.
*/
- advise(_(" %s"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), desc.buf);
strbuf_release(&desc);
return 0;
@@ -542,6 +549,10 @@ static enum get_oid_result get_short_oid(struct repository *r,
if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
struct oid_array collect = OID_ARRAY_INIT;
+ struct ambiguous_output out = {
+ .ds = &ds,
+ .advice = STRBUF_INIT,
+ };
error(_("short object ID %s is ambiguous"), ds.hex_pfx);
@@ -554,13 +565,21 @@ static enum get_oid_result get_short_oid(struct repository *r,
if (!ds.ambiguous)
ds.fn = NULL;
- advise(_("The candidates are:"));
repo_for_each_abbrev(r, ds.hex_pfx, collect_ambiguous, &collect);
sort_ambiguous_oid_array(r, &collect);
- if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
+ if (oid_array_for_each(&collect, show_ambiguous_object, &out))
BUG("show_ambiguous_object shouldn't return non-zero");
+
+ /*
+ * TRANSLATORS: The argument is the list of ambiguous
+ * objects composed in show_ambiguous_object(). See
+ * its "TRANSLATORS" comments for details.
+ */
+ advise(_("The candidates are:\n%s"), out.advice.buf);
+
oid_array_clear(&collect);
+ strbuf_release(&out.advice);
}
return status;
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index d68c411bfc7..cb8ee3d65ed 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -74,7 +74,6 @@ test_expect_success 'ambiguous loose blob parsed as OBJ_BAD' '
cat >expect <<-\EOF &&
error: short object ID bad0... is ambiguous
- hint: The candidates are:
fatal: invalid object type
EOF
test_cmp_failed_rev_parse blob.bad bad0
@@ -96,11 +95,11 @@ test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
cat >expect <<-\EOF &&
error: short object ID cafe... is ambiguous
- hint: The candidates are:
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
+ hint: The candidates are:
hint: cafe... [bad object]
hint: cafe... blob
fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v6 6/6] object-name: re-use "struct strbuf" in show_ambiguous_object()
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-12-28 14:35 ` [PATCH v6 5/6] object-name: iterate ambiguous objects before showing header Ævar Arnfjörð Bjarmason
@ 2021-12-28 14:35 ` Ævar Arnfjörð Bjarmason
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 14:35 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Reduce the allocations done by show_ambiguous_object() by moving the
"desc" strbuf into the "struct ambiguous_output" introduced in the
preceding commit.
This doesn't matter for optimization purposes, but since we're
accumulating a "struct strbuf advice" anyway let's follow that pattern
and add a "struct strbuf sb", we can then strbuf_reset() it rather
than calling strbuf_release() for each call to
show_ambiguous_object().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/object-name.c b/object-name.c
index 743d272800d..2d60e5177d3 100644
--- a/object-name.c
+++ b/object-name.c
@@ -354,6 +354,7 @@ static int init_object_disambiguation(struct repository *r,
struct ambiguous_output {
const struct disambiguate_state *ds;
struct strbuf advice;
+ struct strbuf sb;
};
static int show_ambiguous_object(const struct object_id *oid, void *data)
@@ -361,7 +362,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
struct ambiguous_output *state = data;
const struct disambiguate_state *ds = state->ds;
struct strbuf *advice = &state->advice;
- struct strbuf desc = STRBUF_INIT;
+ struct strbuf *sb = &state->sb;
int type;
const char *hash;
@@ -377,7 +378,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* output shown when we cannot look up or parse the
* object in question. E.g. "deadbeef [bad object]".
*/
- strbuf_addf(&desc, _("%s [bad object]"), hash);
+ strbuf_addf(sb, _("%s [bad object]"), hash);
goto out;
}
@@ -402,8 +403,8 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
*
* "deadbeef commit 2021-01-01 - Some Commit Message"
*/
- strbuf_addf(&desc, _("%s commit %s - %s"),
- hash, date.buf, msg.buf);
+ strbuf_addf(sb, _("%s commit %s - %s"), hash, date.buf,
+ msg.buf);
strbuf_release(&date);
strbuf_release(&msg);
@@ -427,7 +428,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* object.c, it should (hopefully) already be
* translated.
*/
- strbuf_addf(&desc, _("%s tag %s - %s"), hash,
+ strbuf_addf(sb, _("%s tag %s - %s"), hash,
show_date(tag_date, 0, DATE_MODE(SHORT)),
tag_tag);
} else if (type == OBJ_TREE) {
@@ -435,13 +436,13 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* TRANSLATORS: This is a line of ambiguous <type>
* object output. E.g. "deadbeef tree".
*/
- strbuf_addf(&desc, _("%s tree"), hash);
+ strbuf_addf(sb, _("%s tree"), hash);
} else if (type == OBJ_BLOB) {
/*
* TRANSLATORS: This is a line of ambiguous <type>
* object output. E.g. "deadbeef blob".
*/
- strbuf_addf(&desc, _("%s blob"), hash);
+ strbuf_addf(sb, _("%s blob"), hash);
}
@@ -450,9 +451,9 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* TRANSLATORS: This is line item of ambiguous object output
* from describe_ambiguous_object() above.
*/
- strbuf_addf(advice, _(" %s\n"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), sb->buf);
- strbuf_release(&desc);
+ strbuf_reset(sb);
return 0;
}
@@ -551,6 +552,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
struct oid_array collect = OID_ARRAY_INIT;
struct ambiguous_output out = {
.ds = &ds,
+ .sb = STRBUF_INIT,
.advice = STRBUF_INIT,
};
@@ -580,6 +582,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
oid_array_clear(&collect);
strbuf_release(&out.advice);
+ strbuf_release(&out.sb);
}
return status;
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 0/7] progress: test fixes / cleanup
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-12-28 14:35 ` [PATCH v6 6/6] object-name: re-use "struct strbuf" in show_ambiguous_object() Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:18 ` Ævar Arnfjörð Bjarmason
2021-12-28 15:18 ` [PATCH v8 1/7] leak tests: fix a memory leak in "test-progress" helper Ævar Arnfjörð Bjarmason
` (7 more replies)
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
7 siblings, 8 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:18 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
Various test, leak and other fixes for the progress.c code and its
tests. This v8 addresses feedback on v7[1] by Johannes
Altmanninger. For that round I accidentally broke the In-Reply-To
chain, so I'm replying to the v6 here to attach it to the original
thread again.
1. https://lore.kernel.org/git/cover-v7-0.7-00000000000-20211217T041945Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (7):
leak tests: fix a memory leak in "test-progress" helper
progress.c test helper: add missing braces
progress.c tests: make start/stop commands on stdin
progress.c tests: test some invalid usage
progress.c: add temporary variable from progress struct
pack-bitmap-write.c: don't return without stop_progress()
*.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
builtin/bisect--helper.c | 2 +-
builtin/bundle.c | 2 +-
compat/mingw.c | 2 +-
pack-bitmap-write.c | 6 +--
progress.c | 14 +++---
t/helper/test-progress.c | 52 +++++++++++++++-----
t/t0500-progress-display.sh | 94 ++++++++++++++++++++++++++++---------
7 files changed, 126 insertions(+), 46 deletions(-)
Range-diff against v7:
1: 5367293ee84 ! 1: aa08dab654d leak tests: fix a memory leaks in "test-progress" helper
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- leak tests: fix a memory leaks in "test-progress" helper
+ leak tests: fix a memory leak in "test-progress" helper
Fix a memory leak in the test-progress helper, and mark the
corresponding "t0500-progress-display.sh" test as being leak-free
2: 81788101763 = 2: 3ecdab074b6 progress.c test helper: add missing braces
3: d685c248686 ! 3: 271f6d7ec3b progress.c tests: make start/stop commands on stdin
@@ t/helper/test-progress.c
#include "progress.h"
#include "strbuf.h"
+#include "string-list.h"
-+
-+/*
-+ * We can't use "end + 1" as an argument to start_progress() below, it
-+ * doesn't xstrdup() its "title" argument. We need to hold onto a
-+ * valid "char *" for it until the end.
-+ */
-+static char *dup_title(struct string_list *titles, const char *title)
-+{
-+ return string_list_insert(titles, title)->string;
-+}
int cmd__progress(int argc, const char **argv)
{
@@ t/helper/test-progress.c
- if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
+ if (skip_prefix(line.buf, "start ", (const char **) &end)) {
+ uint64_t total = strtoull(end, &end, 10);
-+ if (*end == '\0')
-+ progress = start_progress(default_title, total);
++ const char *title;
++ const char *str;
++
++ /*
++ * We can't use "end + 1" as an argument to
++ * start_progress(), it doesn't xstrdup() its
++ * "title" argument. We need to hold onto a
++ * valid "char *" for it until the end.
++ */
++ if (!*end)
++ title = default_title;
+ else if (*end == ' ')
-+ progress = start_progress(dup_title(&titles,
-+ end + 1),
-+ total);
++ title = string_list_insert(&titles, end + 1)->string;
+ else
+ die("invalid input: '%s'\n", line.buf);
++
++ str = title ? title : default_title;
++ progress = start_progress(str, total);
+ } else if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
uint64_t item_count = strtoull(end, &end, 10);
if (*end != '\0')
4: 40e446da277 = 4: 7c1b8b287c5 progress.c tests: test some invalid usage
5: c2303bfd130 ! 5: 72a31bd7191 progress.c: add temporary variable from progress struct
@@ Metadata
## Commit message ##
progress.c: add temporary variable from progress struct
- Add a temporary "progress" variable for the dereferenced p_progress
- pointer to a "struct progress *". Before 98a13647408 (trace2: log
- progress time and throughput, 2020-05-12) we didn't dereference
- "p_progress" in this function, now that we do it's easier to read the
- code if we work with a "progress" struct pointer like everywhere else,
- instead of a pointer to a pointer.
+ Since 98a13647408 (trace2: log progress time and throughput,
+ 2020-05-12) stop_progress() dereferences a "struct progress **"
+ parameter in several places. Extract a dereferenced variable (like in
+ stop_progress_msg()) to reduce clutter and make it clearer who needs
+ to write to this parameter.
+
+ Now instead of using "*p_progress" several times in stop_progress() we
+ check it once for NULL and then use a dereferenced "progress" variable
+ thereafter. This continues the same pattern used in the above
+ stop_progress() function, see ac900fddb7f (progress: don't dereference
+ before checking for NULL, 2020-08-10).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## progress.c ##
-@@ progress.c: void stop_progress(struct progress **p_progress)
- finish_if_sparse(*p_progress);
+@@ progress.c: static void finish_if_sparse(struct progress *progress)
+
+ void stop_progress(struct progress **p_progress)
+ {
++ struct progress *progress;
+ if (!p_progress)
+ BUG("don't provide NULL to stop_progress");
++ progress = *p_progress;
+
+- finish_if_sparse(*p_progress);
++ finish_if_sparse(progress);
- if (*p_progress) {
-+ struct progress *progress = *p_progress;
+- if (*p_progress) {
++ if (progress) {
trace2_data_intmax("progress", the_repository, "total_objects",
- (*p_progress)->total);
+- (*p_progress)->total);
++ progress->total);
- if ((*p_progress)->throughput)
+- if ((*p_progress)->throughput)
++ if (progress->throughput)
trace2_data_intmax("progress", the_repository,
"total_bytes",
- (*p_progress)->throughput->curr_total);
6: 776362de897 ! 6: 0bd08e1b018 pack-bitmap-write.c: don't return without stop_progress()
@@ Commit message
reached the early exit in this function.
We could call stop_progress() before we return, but better yet is to
- defer calling start_progress() until we need it.
-
- This will matter in a subsequent commit where we BUG(...) out if this
- happens, and matters now e.g. because we don't have a corresponding
- "region_end" for the progress trace2 event.
+ defer calling start_progress() until we need it. For now this only
+ matters in practice because we'd previously omit the "region_leave"
+ for the progress trace2 event.
Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
7: 0670d1aa5f2 ! 7: 060483fb5ce various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- various *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
+ *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
- and around 10 "isatty(0)", but these used the
+ and around 10 "isatty(0)", but three callers used the
{STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
them.
- Let's change these for consistency, and because another commit that
- would like to be based on top of this one[1] has a recipe to change
- all of these for ad-hoc testing, not needing to match these with that
- ad-hoc regex will make things easier to explain. Only one of these is
- related to the "struct progress" code which it discusses, but let's
- change all of these while we're at it.
+ Let's change these for consistency. This makes it easier to change
+ all calls to isatty() at a whim, which is useful to test some
+ scenarios[1].
1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v8 1/7] leak tests: fix a memory leak in "test-progress" helper
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:18 ` Ævar Arnfjörð Bjarmason
2021-12-28 15:18 ` [PATCH v8 2/7] progress.c test helper: add missing braces Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:18 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
Fix a memory leak in the test-progress helper, and mark the
corresponding "t0500-progress-display.sh" test as being leak-free
under SANITIZE=leak. This fixes a leak added in 2bb74b53a4 (Test the
progress display, 2019-09-16).
My 48f68715b14 (tr2: stop leaking "thread_name" memory, 2021-08-27)
had fixed another memory leak in this test (as it did some trace2
testing).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/helper/test-progress.c | 1 +
t/t0500-progress-display.sh | 1 +
2 files changed, 2 insertions(+)
diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 5d05cbe7894..9265e6ab7cf 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -69,6 +69,7 @@ int cmd__progress(int argc, const char **argv)
die("invalid input: '%s'\n", line.buf);
}
stop_progress(&progress);
+ strbuf_release(&line);
return 0;
}
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 22058b503ac..f37cf2eb9c9 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -2,6 +2,7 @@
test_description='progress display'
+TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
show_cr () {
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 2/7] progress.c test helper: add missing braces
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
2021-12-28 15:18 ` [PATCH v8 1/7] leak tests: fix a memory leak in "test-progress" helper Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:18 ` Ævar Arnfjörð Bjarmason
2021-12-28 15:18 ` [PATCH v8 3/7] progress.c tests: make start/stop commands on stdin Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:18 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
If we have braces on one arm of an if/else all of them should have it,
per the CodingGuidelines's "When there are multiple arms to a
conditional[...]" advice. This formatting change makes a subsequent
commit smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/helper/test-progress.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 9265e6ab7cf..50fd3be3dad 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -63,10 +63,11 @@ int cmd__progress(int argc, const char **argv)
die("invalid input: '%s'\n", line.buf);
progress_test_ns = test_ms * 1000 * 1000;
display_throughput(progress, byte_count);
- } else if (!strcmp(line.buf, "update"))
+ } else if (!strcmp(line.buf, "update")) {
progress_test_force_update();
- else
+ } else {
die("invalid input: '%s'\n", line.buf);
+ }
}
stop_progress(&progress);
strbuf_release(&line);
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 3/7] progress.c tests: make start/stop commands on stdin
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
2021-12-28 15:18 ` [PATCH v8 1/7] leak tests: fix a memory leak in "test-progress" helper Ævar Arnfjörð Bjarmason
2021-12-28 15:18 ` [PATCH v8 2/7] progress.c test helper: add missing braces Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:18 ` Ævar Arnfjörð Bjarmason
2021-12-28 16:25 ` Johannes Altmanninger
2021-12-28 15:19 ` [PATCH v8 4/7] progress.c tests: test some invalid usage Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:18 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
Change the usage of the "test-tool progress" introduced in
2bb74b53a49 (Test the progress display, 2019-09-16) to take command
like "start" and "stop" on stdin, instead of running them implicitly.
This makes for tests that are easier to read, since the recipe will
mirror the API usage, and allows for easily testing invalid usage that
would yield (or should yield) a BUG(), e.g. providing two "start"
calls in a row. A subsequent commit will add such tests.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/helper/test-progress.c | 46 ++++++++++++++++++++++-------
t/t0500-progress-display.sh | 58 +++++++++++++++++++++++--------------
2 files changed, 72 insertions(+), 32 deletions(-)
diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
index 50fd3be3dad..becc163375f 100644
--- a/t/helper/test-progress.c
+++ b/t/helper/test-progress.c
@@ -3,6 +3,9 @@
*
* Reads instructions from standard input, one instruction per line:
*
+ * "start <total>[ <title>]" - Call start_progress(title, total),
+ * Uses the default title of "Working hard"
+ * if the " <title>" is omitted.
* "progress <items>" - Call display_progress() with the given item count
* as parameter.
* "throughput <bytes> <millis> - Call display_throughput() with the given
@@ -10,6 +13,7 @@
* specify the time elapsed since the
* start_progress() call.
* "update" - Set the 'progress_update' flag.
+ * "stop" - Call stop_progress().
*
* See 't0500-progress-display.sh' for examples.
*/
@@ -19,34 +23,52 @@
#include "parse-options.h"
#include "progress.h"
#include "strbuf.h"
+#include "string-list.h"
int cmd__progress(int argc, const char **argv)
{
- int total = 0;
- const char *title;
+ const char *const default_title = "Working hard";
+ struct string_list titles = STRING_LIST_INIT_DUP;
struct strbuf line = STRBUF_INIT;
- struct progress *progress;
+ struct progress *progress = NULL;
const char *usage[] = {
- "test-tool progress [--total=<n>] <progress-title>",
+ "test-tool progress <stdin",
NULL
};
struct option options[] = {
- OPT_INTEGER(0, "total", &total, "total number of items"),
OPT_END(),
};
argc = parse_options(argc, argv, NULL, options, usage, 0);
- if (argc != 1)
- die("need a title for the progress output");
- title = argv[0];
+ if (argc)
+ usage_with_options(usage, options);
progress_testing = 1;
- progress = start_progress(title, total);
while (strbuf_getline(&line, stdin) != EOF) {
char *end;
- if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
+ if (skip_prefix(line.buf, "start ", (const char **) &end)) {
+ uint64_t total = strtoull(end, &end, 10);
+ const char *title;
+ const char *str;
+
+ /*
+ * We can't use "end + 1" as an argument to
+ * start_progress(), it doesn't xstrdup() its
+ * "title" argument. We need to hold onto a
+ * valid "char *" for it until the end.
+ */
+ if (!*end)
+ title = default_title;
+ else if (*end == ' ')
+ title = string_list_insert(&titles, end + 1)->string;
+ else
+ die("invalid input: '%s'\n", line.buf);
+
+ str = title ? title : default_title;
+ progress = start_progress(str, total);
+ } else if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
uint64_t item_count = strtoull(end, &end, 10);
if (*end != '\0')
die("invalid input: '%s'\n", line.buf);
@@ -65,12 +87,14 @@ int cmd__progress(int argc, const char **argv)
display_throughput(progress, byte_count);
} else if (!strcmp(line.buf, "update")) {
progress_test_force_update();
+ } else if (!strcmp(line.buf, "stop")) {
+ stop_progress(&progress);
} else {
die("invalid input: '%s'\n", line.buf);
}
}
- stop_progress(&progress);
strbuf_release(&line);
+ string_list_clear(&titles, 0);
return 0;
}
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index f37cf2eb9c9..27ab4218b01 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -18,6 +18,7 @@ test_expect_success 'simple progress display' '
EOF
cat >in <<-\EOF &&
+ start 0
update
progress 1
update
@@ -26,8 +27,9 @@ test_expect_success 'simple progress display' '
progress 4
update
progress 5
+ stop
EOF
- test-tool progress "Working hard" <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -42,11 +44,13 @@ test_expect_success 'progress display with total' '
EOF
cat >in <<-\EOF &&
+ start 3
progress 1
progress 2
progress 3
+ stop
EOF
- test-tool progress --total=3 "Working hard" <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -63,14 +67,14 @@ Working hard.......2.........3.........4.........5.........6:
EOF
cat >in <<-\EOF &&
+ start 100000 Working hard.......2.........3.........4.........5.........6
progress 100
progress 1000
progress 10000
progress 100000
+ stop
EOF
- test-tool progress --total=100000 \
- "Working hard.......2.........3.........4.........5.........6" \
- <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -89,16 +93,16 @@ Working hard.......2.........3.........4.........5.........6:
EOF
cat >in <<-\EOF &&
+ start 100000 Working hard.......2.........3.........4.........5.........6
update
progress 1
update
progress 2
progress 10000
progress 100000
+ stop
EOF
- test-tool progress --total=100000 \
- "Working hard.......2.........3.........4.........5.........6" \
- <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -117,14 +121,14 @@ Working hard.......2.........3.........4.........5.........6:
EOF
cat >in <<-\EOF &&
+ start 100000 Working hard.......2.........3.........4.........5.........6
progress 25000
progress 50000
progress 75000
progress 100000
+ stop
EOF
- test-tool progress --total=100000 \
- "Working hard.......2.........3.........4.........5.........6" \
- <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -141,14 +145,14 @@ Working hard.......2.........3.........4.........5.........6.........7.........:
EOF
cat >in <<-\EOF &&
+ start 100000 Working hard.......2.........3.........4.........5.........6.........7.........
progress 25000
progress 50000
progress 75000
progress 100000
+ stop
EOF
- test-tool progress --total=100000 \
- "Working hard.......2.........3.........4.........5.........6.........7........." \
- <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -165,12 +169,14 @@ test_expect_success 'progress shortens - crazy caller' '
EOF
cat >in <<-\EOF &&
+ start 1000
progress 100
progress 200
progress 1
progress 1000
+ stop
EOF
- test-tool progress --total=1000 "Working hard" <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -186,6 +192,7 @@ test_expect_success 'progress display with throughput' '
EOF
cat >in <<-\EOF &&
+ start 0
throughput 102400 1000
update
progress 10
@@ -198,8 +205,9 @@ test_expect_success 'progress display with throughput' '
throughput 409600 4000
update
progress 40
+ stop
EOF
- test-tool progress "Working hard" <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -215,6 +223,7 @@ test_expect_success 'progress display with throughput and total' '
EOF
cat >in <<-\EOF &&
+ start 40
throughput 102400 1000
progress 10
throughput 204800 2000
@@ -223,8 +232,9 @@ test_expect_success 'progress display with throughput and total' '
progress 30
throughput 409600 4000
progress 40
+ stop
EOF
- test-tool progress --total=40 "Working hard" <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -240,6 +250,7 @@ test_expect_success 'cover up after throughput shortens' '
EOF
cat >in <<-\EOF &&
+ start 0
throughput 409600 1000
update
progress 1
@@ -252,8 +263,9 @@ test_expect_success 'cover up after throughput shortens' '
throughput 1638400 4000
update
progress 4
+ stop
EOF
- test-tool progress "Working hard" <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -268,6 +280,7 @@ test_expect_success 'cover up after throughput shortens a lot' '
EOF
cat >in <<-\EOF &&
+ start 0
throughput 1 1000
update
progress 1
@@ -277,8 +290,9 @@ test_expect_success 'cover up after throughput shortens a lot' '
throughput 3145728 3000
update
progress 3
+ stop
EOF
- test-tool progress "Working hard" <in 2>stderr &&
+ test-tool progress <in 2>stderr &&
show_cr <stderr >out &&
test_cmp expect out
@@ -286,6 +300,7 @@ test_expect_success 'cover up after throughput shortens a lot' '
test_expect_success 'progress generates traces' '
cat >in <<-\EOF &&
+ start 40
throughput 102400 1000
update
progress 10
@@ -298,10 +313,11 @@ test_expect_success 'progress generates traces' '
throughput 409600 4000
update
progress 40
+ stop
EOF
- GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool progress --total=40 \
- "Working hard" <in 2>stderr &&
+ GIT_TRACE2_EVENT="$(pwd)/trace.event" test-tool progress \
+ <in 2>stderr &&
# t0212/parse_events.perl intentionally omits regions and data.
test_region progress "Working hard" trace.event &&
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v8 3/7] progress.c tests: make start/stop commands on stdin
2021-12-28 15:18 ` [PATCH v8 3/7] progress.c tests: make start/stop commands on stdin Ævar Arnfjörð Bjarmason
@ 2021-12-28 16:25 ` Johannes Altmanninger
0 siblings, 0 replies; 90+ messages in thread
From: Johannes Altmanninger @ 2021-12-28 16:25 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, SZEDER Gábor, René Scharfe
On Tue, Dec 28, 2021 at 04:18:59PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Change the usage of the "test-tool progress" introduced in
> 2bb74b53a49 (Test the progress display, 2019-09-16) to take command
> like "start" and "stop" on stdin, instead of running them implicitly.
>
> This makes for tests that are easier to read, since the recipe will
> mirror the API usage, and allows for easily testing invalid usage that
> would yield (or should yield) a BUG(), e.g. providing two "start"
> calls in a row. A subsequent commit will add such tests.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> t/helper/test-progress.c | 46 ++++++++++++++++++++++-------
> t/t0500-progress-display.sh | 58 +++++++++++++++++++++++--------------
> 2 files changed, 72 insertions(+), 32 deletions(-)
>
> diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c
> index 50fd3be3dad..becc163375f 100644
> --- a/t/helper/test-progress.c
> +++ b/t/helper/test-progress.c
> @@ -3,6 +3,9 @@
> *
> * Reads instructions from standard input, one instruction per line:
> *
> + * "start <total>[ <title>]" - Call start_progress(title, total),
> + * Uses the default title of "Working hard"
> + * if the " <title>" is omitted.
> * "progress <items>" - Call display_progress() with the given item count
> * as parameter.
> * "throughput <bytes> <millis> - Call display_throughput() with the given
> @@ -10,6 +13,7 @@
> * specify the time elapsed since the
> * start_progress() call.
> * "update" - Set the 'progress_update' flag.
> + * "stop" - Call stop_progress().
> *
> * See 't0500-progress-display.sh' for examples.
> */
> @@ -19,34 +23,52 @@
> #include "parse-options.h"
> #include "progress.h"
> #include "strbuf.h"
> +#include "string-list.h"
>
> int cmd__progress(int argc, const char **argv)
> {
> - int total = 0;
> - const char *title;
> + const char *const default_title = "Working hard";
> + struct string_list titles = STRING_LIST_INIT_DUP;
> struct strbuf line = STRBUF_INIT;
> - struct progress *progress;
> + struct progress *progress = NULL;
>
> const char *usage[] = {
> - "test-tool progress [--total=<n>] <progress-title>",
> + "test-tool progress <stdin",
> NULL
> };
> struct option options[] = {
> - OPT_INTEGER(0, "total", &total, "total number of items"),
> OPT_END(),
> };
>
> argc = parse_options(argc, argv, NULL, options, usage, 0);
> - if (argc != 1)
> - die("need a title for the progress output");
> - title = argv[0];
> + if (argc)
> + usage_with_options(usage, options);
>
> progress_testing = 1;
> - progress = start_progress(title, total);
> while (strbuf_getline(&line, stdin) != EOF) {
> char *end;
>
> - if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
> + if (skip_prefix(line.buf, "start ", (const char **) &end)) {
> + uint64_t total = strtoull(end, &end, 10);
> + const char *title;
> + const char *str;
> +
> + /*
> + * We can't use "end + 1" as an argument to
> + * start_progress(), it doesn't xstrdup() its
> + * "title" argument. We need to hold onto a
> + * valid "char *" for it until the end.
> + */
> + if (!*end)
> + title = default_title;
> + else if (*end == ' ')
> + title = string_list_insert(&titles, end + 1)->string;
> + else
> + die("invalid input: '%s'\n", line.buf);
> +
> + str = title ? title : default_title;
I don't think title is ever NULL, so we should be able to elide this variable.
(Did you want to fall back to the default title when the input is "start "?)
> + progress = start_progress(str, total);
> + } else if (skip_prefix(line.buf, "progress ", (const char **) &end)) {
> uint64_t item_count = strtoull(end, &end, 10);
> if (*end != '\0')
> die("invalid input: '%s'\n", line.buf);
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v8 4/7] progress.c tests: test some invalid usage
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-12-28 15:18 ` [PATCH v8 3/7] progress.c tests: make start/stop commands on stdin Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:19 ` Ævar Arnfjörð Bjarmason
2021-12-28 16:33 ` Johannes Altmanninger
2021-12-28 15:19 ` [PATCH v8 5/7] progress.c: add temporary variable from progress struct Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
Test what happens when we "stop" without a "start", omit the "stop"
after a "start", or try to start two concurrent progress bars. This
extends the trace2 tests added in 98a13647408 (trace2: log progress
time and throughput, 2020-05-12).
These tests are not merely testing the helper, but invalid API usage
that can happen if the progress.c API is misused.
The "without stop" test will leak under SANITIZE=leak, since this
buggy use of the API will leak memory. But let's not skip it entirely,
or use the "!SANITIZE_LEAK" prerequisite check as we'd do with tests
that we're skipping due to leaks we haven't fixed yet. Instead
annotate the specific command that should skip leak checking with
custom $LSAN_OPTIONS[1].
1. https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t0500-progress-display.sh | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh
index 27ab4218b01..59e9f226ea4 100755
--- a/t/t0500-progress-display.sh
+++ b/t/t0500-progress-display.sh
@@ -325,4 +325,39 @@ test_expect_success 'progress generates traces' '
grep "\"key\":\"total_bytes\",\"value\":\"409600\"" trace.event
'
+test_expect_success 'progress generates traces: stop / start' '
+ cat >in <<-\EOF &&
+ start 0
+ stop
+ EOF
+
+ GIT_TRACE2_EVENT="$(pwd)/trace-startstop.event" test-tool progress \
+ <in 2>stderr &&
+ test_region progress "Working hard" trace-startstop.event
+'
+
+test_expect_success 'progress generates traces: start without stop' '
+ cat >in <<-\EOF &&
+ start 0
+ EOF
+
+ GIT_TRACE2_EVENT="$(pwd)/trace-start.event" \
+ LSAN_OPTIONS=detect_leaks=0 \
+ test-tool progress \
+ <in 2>stderr &&
+ grep region_enter.*progress trace-start.event &&
+ ! grep region_leave.*progress trace-start.event
+'
+
+test_expect_success 'progress generates traces: stop without start' '
+ cat >in <<-\EOF &&
+ stop
+ EOF
+
+ GIT_TRACE2_EVENT="$(pwd)/trace-stop.event" test-tool progress \
+ <in 2>stderr &&
+ ! grep region_enter.*progress trace-stop.event &&
+ ! grep region_leave.*progress trace-stop.event
+'
+
test_done
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v8 4/7] progress.c tests: test some invalid usage
2021-12-28 15:19 ` [PATCH v8 4/7] progress.c tests: test some invalid usage Ævar Arnfjörð Bjarmason
@ 2021-12-28 16:33 ` Johannes Altmanninger
0 siblings, 0 replies; 90+ messages in thread
From: Johannes Altmanninger @ 2021-12-28 16:33 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, SZEDER Gábor, René Scharfe
On Tue, Dec 28, 2021 at 04:19:00PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Test what happens when we "stop" without a "start", omit the "stop"
> after a "start", or try to start two concurrent progress bars. This
I think there is still no test for the two concurrent progress bars,
but you mention it here, and also in the previous patch's message,
which is misleading.
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v8 5/7] progress.c: add temporary variable from progress struct
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-12-28 15:19 ` [PATCH v8 4/7] progress.c tests: test some invalid usage Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:19 ` Ævar Arnfjörð Bjarmason
2021-12-28 16:05 ` René Scharfe
2021-12-28 16:13 ` Johannes Altmanninger
2021-12-28 15:19 ` [PATCH v8 6/7] pack-bitmap-write.c: don't return without stop_progress() Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 2 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
Since 98a13647408 (trace2: log progress time and throughput,
2020-05-12) stop_progress() dereferences a "struct progress **"
parameter in several places. Extract a dereferenced variable (like in
stop_progress_msg()) to reduce clutter and make it clearer who needs
to write to this parameter.
Now instead of using "*p_progress" several times in stop_progress() we
check it once for NULL and then use a dereferenced "progress" variable
thereafter. This continues the same pattern used in the above
stop_progress() function, see ac900fddb7f (progress: don't dereference
before checking for NULL, 2020-08-10).
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
progress.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/progress.c b/progress.c
index 680c6a8bf93..688749648be 100644
--- a/progress.c
+++ b/progress.c
@@ -319,21 +319,23 @@ static void finish_if_sparse(struct progress *progress)
void stop_progress(struct progress **p_progress)
{
+ struct progress *progress;
if (!p_progress)
BUG("don't provide NULL to stop_progress");
+ progress = *p_progress;
- finish_if_sparse(*p_progress);
+ finish_if_sparse(progress);
- if (*p_progress) {
+ if (progress) {
trace2_data_intmax("progress", the_repository, "total_objects",
- (*p_progress)->total);
+ progress->total);
- if ((*p_progress)->throughput)
+ if (progress->throughput)
trace2_data_intmax("progress", the_repository,
"total_bytes",
- (*p_progress)->throughput->curr_total);
+ progress->throughput->curr_total);
- trace2_region_leave("progress", (*p_progress)->title, the_repository);
+ trace2_region_leave("progress", progress->title, the_repository);
}
stop_progress_msg(p_progress, _("done"));
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v8 5/7] progress.c: add temporary variable from progress struct
2021-12-28 15:19 ` [PATCH v8 5/7] progress.c: add temporary variable from progress struct Ævar Arnfjörð Bjarmason
@ 2021-12-28 16:05 ` René Scharfe
2021-12-28 16:13 ` Johannes Altmanninger
1 sibling, 0 replies; 90+ messages in thread
From: René Scharfe @ 2021-12-28 16:05 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, SZEDER Gábor, Johannes Altmanninger
Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason:
> Since 98a13647408 (trace2: log progress time and throughput,
> 2020-05-12) stop_progress() dereferences a "struct progress **"
> parameter in several places. Extract a dereferenced variable (like in
> stop_progress_msg()) to reduce clutter and make it clearer who needs
> to write to this parameter.
>
> Now instead of using "*p_progress" several times in stop_progress() we
> check it once for NULL and then use a dereferenced "progress" variable
> thereafter. This continues the same pattern used in the above
> stop_progress() function, see ac900fddb7f (progress: don't dereference
> before checking for NULL, 2020-08-10).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> progress.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 680c6a8bf93..688749648be 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -319,21 +319,23 @@ static void finish_if_sparse(struct progress *progress)
>
> void stop_progress(struct progress **p_progress)
> {
> + struct progress *progress;
> if (!p_progress)
> BUG("don't provide NULL to stop_progress");
> + progress = *p_progress;
>
> - finish_if_sparse(*p_progress);
> + finish_if_sparse(progress);
>
> - if (*p_progress) {
> + if (progress) {
> trace2_data_intmax("progress", the_repository, "total_objects",
> - (*p_progress)->total);
> + progress->total);
>
> - if ((*p_progress)->throughput)
> + if (progress->throughput)
> trace2_data_intmax("progress", the_repository,
> "total_bytes",
> - (*p_progress)->throughput->curr_total);
> + progress->throughput->curr_total);
>
> - trace2_region_leave("progress", (*p_progress)->title, the_repository);
> + trace2_region_leave("progress", progress->title, the_repository);
> }
>
> stop_progress_msg(p_progress, _("done"));
This patch is trivially correct, but I wonder why all that code is here
instead of in stop_progress_msg(). I would expect stop_progress() to be
a thin wrapper that just provides a default message, but actually it
handles sparse progress and tracing. Isn't both necessary even with a
custom message?
In any case, moving the code there becomes easier after this patch.
René
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v8 5/7] progress.c: add temporary variable from progress struct
2021-12-28 15:19 ` [PATCH v8 5/7] progress.c: add temporary variable from progress struct Ævar Arnfjörð Bjarmason
2021-12-28 16:05 ` René Scharfe
@ 2021-12-28 16:13 ` Johannes Altmanninger
1 sibling, 0 replies; 90+ messages in thread
From: Johannes Altmanninger @ 2021-12-28 16:13 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, SZEDER Gábor, René Scharfe
On Tue, Dec 28, 2021 at 04:19:01PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Since 98a13647408 (trace2: log progress time and throughput,
> 2020-05-12) stop_progress() dereferences a "struct progress **"
> parameter in several places. Extract a dereferenced variable (like in
> stop_progress_msg()) to reduce clutter and make it clearer who needs
The "(like in stop_progress_msg())" can probably go because you explain the
added consistency in the next paragraph.
> to write to this parameter.
>
> Now instead of using "*p_progress" several times in stop_progress() we
> check it once for NULL and then use a dereferenced "progress" variable
> thereafter. This continues the same pattern used in the above
> stop_progress() function, see ac900fddb7f (progress: don't dereference
"above stop_progress" should be "below stop_progress_msg",
because stop_progress is the one you're modifying?
> before checking for NULL, 2020-08-10).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> progress.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/progress.c b/progress.c
> index 680c6a8bf93..688749648be 100644
> --- a/progress.c
> +++ b/progress.c
> @@ -319,21 +319,23 @@ static void finish_if_sparse(struct progress *progress)
>
> void stop_progress(struct progress **p_progress)
> {
> + struct progress *progress;
nit: in stop_progress_msg we have a blank line here, the inconsistency is
mildly surprising
> if (!p_progress)
> BUG("don't provide NULL to stop_progress");
> + progress = *p_progress;
>
> - finish_if_sparse(*p_progress);
> + finish_if_sparse(progress);
>
> - if (*p_progress) {
> + if (progress) {
> trace2_data_intmax("progress", the_repository, "total_objects",
> - (*p_progress)->total);
> + progress->total);
>
> - if ((*p_progress)->throughput)
> + if (progress->throughput)
> trace2_data_intmax("progress", the_repository,
> "total_bytes",
> - (*p_progress)->throughput->curr_total);
> + progress->throughput->curr_total);
>
> - trace2_region_leave("progress", (*p_progress)->title, the_repository);
> + trace2_region_leave("progress", progress->title, the_repository);
> }
>
> stop_progress_msg(p_progress, _("done"));
> --
> 2.34.1.1257.g2af47340c7b
>
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v8 6/7] pack-bitmap-write.c: don't return without stop_progress()
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-12-28 15:19 ` [PATCH v8 5/7] progress.c: add temporary variable from progress struct Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:19 ` Ævar Arnfjörð Bjarmason
2021-12-28 15:19 ` [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO) Ævar Arnfjörð Bjarmason
2022-01-08 0:45 ` [PATCH v8 0/7] progress: test fixes / cleanup Junio C Hamano
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
Fix a bug that's been here since 7cc8f971085 (pack-objects: implement
bitmap writing, 2013-12-21), we did not call stop_progress() if we
reached the early exit in this function.
We could call stop_progress() before we return, but better yet is to
defer calling start_progress() until we need it. For now this only
matters in practice because we'd previously omit the "region_leave"
for the progress trace2 event.
Suggested-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
pack-bitmap-write.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 9c55c1531e1..cab3eaa2acd 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -575,15 +575,15 @@ void bitmap_writer_select_commits(struct commit **indexed_commits,
QSORT(indexed_commits, indexed_commits_nr, date_compare);
- if (writer.show_progress)
- writer.progress = start_progress("Selecting bitmap commits", 0);
-
if (indexed_commits_nr < 100) {
for (i = 0; i < indexed_commits_nr; ++i)
push_bitmapped_commit(indexed_commits[i]);
return;
}
+ if (writer.show_progress)
+ writer.progress = start_progress("Selecting bitmap commits", 0);
+
for (;;) {
struct commit *chosen = NULL;
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-12-28 15:19 ` [PATCH v8 6/7] pack-bitmap-write.c: don't return without stop_progress() Ævar Arnfjörð Bjarmason
@ 2021-12-28 15:19 ` Ævar Arnfjörð Bjarmason
2021-12-28 16:47 ` René Scharfe
2022-01-08 0:45 ` [PATCH v8 0/7] progress: test fixes / cleanup Junio C Hamano
7 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 15:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, SZEDER Gábor, René Scharfe,
Johannes Altmanninger, Ævar Arnfjörð Bjarmason
We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
and around 10 "isatty(0)", but three callers used the
{STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
them.
Let's change these for consistency. This makes it easier to change
all calls to isatty() at a whim, which is useful to test some
scenarios[1].
1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/bisect--helper.c | 2 +-
builtin/bundle.c | 2 +-
compat/mingw.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28a2e6a5750..21360a4e70b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -830,7 +830,7 @@ static int bisect_autostart(struct bisect_terms *terms)
fprintf_ln(stderr, _("You need to start by \"git bisect "
"start\"\n"));
- if (!isatty(STDIN_FILENO))
+ if (!isatty(0))
return -1;
/*
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 5a85d7cd0fe..df69c651753 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -56,7 +56,7 @@ static int parse_options_cmd_bundle(int argc,
static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
int all_progress_implied = 0;
- int progress = isatty(STDERR_FILENO);
+ int progress = isatty(2);
struct strvec pack_opts;
int version = -1;
int ret;
diff --git a/compat/mingw.c b/compat/mingw.c
index e14f2d5f77c..7c55d0f0414 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2376,7 +2376,7 @@ int mingw_raise(int sig)
switch (sig) {
case SIGALRM:
if (timer_fn == SIG_DFL) {
- if (isatty(STDERR_FILENO))
+ if (isatty(2))
fputs("Alarm clock\n", stderr);
exit(128 + SIGALRM);
} else if (timer_fn != SIG_IGN)
--
2.34.1.1257.g2af47340c7b
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
2021-12-28 15:19 ` [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO) Ævar Arnfjörð Bjarmason
@ 2021-12-28 16:47 ` René Scharfe
2021-12-28 23:56 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 90+ messages in thread
From: René Scharfe @ 2021-12-28 16:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, SZEDER Gábor, Johannes Altmanninger
Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason:
> We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
> and around 10 "isatty(0)", but three callers used the
> {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
> them.
>
> Let's change these for consistency. This makes it easier to change
> all calls to isatty() at a whim, which is useful to test some
> scenarios[1].
Hmm. Matching e.g. "(0|STDIN_FILENO)" instead of "0" is harder, of
course, but not much.
Shouldn't we use these macros more to reduce the number of magic values?
The code is slightly easier to read before this patch because it doesn't
require the reader to know the meaning of these numbers.
Reducing the constants to their numerical values is easy to automate in
general; the opposite direction is harder. Coccinelle can help us take
such a step with a semantic patch like this:
@@
@@
isatty(
(
- 0
+ STDIN_FILENO
|
- 1
+ STDOUT_FILENO
|
- 2
+ STDERR_FILENO
)
)
>
> 1. https://lore.kernel.org/git/patch-v6-8.8-bff919994b5-20211102T122507Z-avarab@gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/bisect--helper.c | 2 +-
> builtin/bundle.c | 2 +-
> compat/mingw.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 28a2e6a5750..21360a4e70b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -830,7 +830,7 @@ static int bisect_autostart(struct bisect_terms *terms)
> fprintf_ln(stderr, _("You need to start by \"git bisect "
> "start\"\n"));
>
> - if (!isatty(STDIN_FILENO))
> + if (!isatty(0))
> return -1;
>
> /*
> diff --git a/builtin/bundle.c b/builtin/bundle.c
> index 5a85d7cd0fe..df69c651753 100644
> --- a/builtin/bundle.c
> +++ b/builtin/bundle.c
> @@ -56,7 +56,7 @@ static int parse_options_cmd_bundle(int argc,
>
> static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
> int all_progress_implied = 0;
> - int progress = isatty(STDERR_FILENO);
> + int progress = isatty(2);
> struct strvec pack_opts;
> int version = -1;
> int ret;
> diff --git a/compat/mingw.c b/compat/mingw.c
> index e14f2d5f77c..7c55d0f0414 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -2376,7 +2376,7 @@ int mingw_raise(int sig)
> switch (sig) {
> case SIGALRM:
> if (timer_fn == SIG_DFL) {
> - if (isatty(STDERR_FILENO))
> + if (isatty(2))
> fputs("Alarm clock\n", stderr);
> exit(128 + SIGALRM);
> } else if (timer_fn != SIG_IGN)
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO)
2021-12-28 16:47 ` René Scharfe
@ 2021-12-28 23:56 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-12-28 23:56 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, SZEDER Gábor, Johannes Altmanninger
On Tue, Dec 28 2021, René Scharfe wrote:
> Am 28.12.21 um 16:19 schrieb Ævar Arnfjörð Bjarmason:
>> We have over 50 uses of "isatty(1)" and "isatty(2)" in the codebase,
>> and around 10 "isatty(0)", but three callers used the
>> {STDIN_FILENO,STD{OUT,ERR}_FILENO} macros in "stdlib.h" to refer to
>> them.
>>
>> Let's change these for consistency. This makes it easier to change
>> all calls to isatty() at a whim, which is useful to test some
>> scenarios[1].
>
> Hmm. Matching e.g. "(0|STDIN_FILENO)" instead of "0" is harder, of
> course, but not much.
>
> Shouldn't we use these macros more to reduce the number of magic values?
> The code is slightly easier to read before this patch because it doesn't
> require the reader to know the meaning of these numbers.
>
> Reducing the constants to their numerical values is easy to automate in
> general; the opposite direction is harder. Coccinelle can help us take
> such a step with a semantic patch like this:
>
> @@
> @@
> isatty(
> (
> - 0
> + STDIN_FILENO
> |
> - 1
> + STDOUT_FILENO
> |
> - 2
> + STDERR_FILENO
> )
> )
We don't bother with EXIT_SUCCESS and EXIT_FAILURE, and for those (VMS)
there is a reason to not use the constants, as EXIT_FAILURE may differ.
But for these I personally think these symbolic names are rather
useless.
They never differ, and when working on POSIX systems you're going to
need to know that 1 is stdout, 2 is stderr. You're also going to have to
maintain shellscripts that use ">&2" or whatever. Those aren't using a
hypothetical ">&$STDERR_FILENO".
But in any case, this change isn't even trying to make the argument that
we *should* use one over the other, just that the constants are used
much more than *_FILENO, so changing them to make a subsequent (now
ejected out of this series) change easier to explain is worth it.
So I'd think we can just take this small change, and argue separately
whether it's worth it to apply that coccinelle rule.
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v8 0/7] progress: test fixes / cleanup
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-12-28 15:19 ` [PATCH v8 7/7] *.c: use isatty(0|2), not isatty(STDIN_FILENO|STDERR_FILENO) Ævar Arnfjörð Bjarmason
@ 2022-01-08 0:45 ` Junio C Hamano
7 siblings, 0 replies; 90+ messages in thread
From: Junio C Hamano @ 2022-01-08 0:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, SZEDER Gábor, René Scharfe, Johannes Altmanninger
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Various test, leak and other fixes for the progress.c code and its
> tests. This v8 addresses feedback on v7[1] by Johannes
> Altmanninger. For that round I accidentally broke the In-Reply-To
> chain, so I'm replying to the v6 here to attach it to the original
> thread again.
Is this replying to v6 of a totally unrelated topic that is about
ambiguous object name?
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date
2021-12-28 14:34 ` [PATCH v6 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-12-28 15:18 ` [PATCH v8 0/7] progress: test fixes / cleanup Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:39 ` Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
` (6 more replies)
7 siblings, 7 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
This topic improves the output we emit on ambiguous objects as noted
in 4/6, and makes it translatable, see 3/6. See [1] for v6.
This v7 addresses all the feedback on v7 from Junio. Note also that
there's an unrelated v8[2] in reply to the v6 from another topic,
because I mixed up the In-Reply-To for the two while submitting a
re-roll of it, sorry about that.
1. https://lore.kernel.org/git/cover-v6-0.6-00000000000-20211228T143223Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v8-0.7-00000000000-20211228T150728Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (6):
object-name tests: add tests for ambiguous object blind spots
object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
object-name: make ambiguous object output translatable
object-name: show date for ambiguous tag objects
object-name: iterate ambiguous objects before showing header
object-name: re-use "struct strbuf" in show_ambiguous_object()
object-name.c | 113 +++++++++++++++++++++++++---
t/t1512-rev-parse-disambiguation.sh | 78 +++++++++++++++++++
2 files changed, 179 insertions(+), 12 deletions(-)
Range-diff against v6:
1: 27f267ad555 ! 1: 28c01b7f8a5 object-name tests: add tests for ambiguous object blind spots
@@ t/t1512-rev-parse-disambiguation.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+test_cmp_failed_rev_parse () {
-+ dir=$1
-+ rev=$2
-+ shift
-+
-+ test_must_fail git -C "$dir" rev-parse "$rev" 2>actual.raw &&
-+ sed "s/\($rev\)[0-9a-f]*/\1.../g" <actual.raw >actual &&
++ cat >expect &&
++ test_must_fail git -C "$1" rev-parse "$2" 2>actual.raw &&
++ sed "s/\($2\)[0-9a-f]*/\1.../" <actual.raw >actual &&
+ test_cmp expect actual
+}
+
@@ t/t1512-rev-parse-disambiguation.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ ) &&
+
+ test_must_fail git -C blob.prefix rev-parse dead &&
-+ cat >expect <<-\EOF &&
++ test_cmp_failed_rev_parse blob.prefix beef <<-\EOF
+ error: short object ID beef... is ambiguous
+ hint: The candidates are:
+ hint: beef... blob
@@ t/t1512-rev-parse-disambiguation.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
-+ test_cmp_failed_rev_parse blob.prefix beef
+'
+
-+test_expect_success 'ambiguous loose blob parsed as OBJ_BAD' '
++test_expect_success 'ambiguous loose bad object parsed as OBJ_BAD' '
+ git init --bare blob.bad &&
+ (
+ cd blob.bad &&
@@ t/t1512-rev-parse-disambiguation.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ echo xyzhjpyvwl | git hash-object -t bad -w --stdin --literally
+ ) &&
+
-+ cat >expect <<-\EOF &&
++ test_cmp_failed_rev_parse blob.bad bad0 <<-\EOF
+ error: short object ID bad0... is ambiguous
+ hint: The candidates are:
+ fatal: invalid object type
+ EOF
-+ test_cmp_failed_rev_parse blob.bad bad0
+'
+
+test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
@@ t/t1512-rev-parse-disambiguation.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ echo broken >$oidf
+ ) &&
+
-+ cat >expect <<-\EOF &&
++ test_cmp_failed_rev_parse blob.corrupt cafe <<-\EOF
+ error: short object ID cafe... is ambiguous
+ hint: The candidates are:
+ error: inflate: data stream error (incorrect header check)
@@ t/t1512-rev-parse-disambiguation.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
-+ test_cmp_failed_rev_parse blob.corrupt cafe
+'
+
if ! test_have_prereq SHA1
2: c78243dc701 = 2: b7027dfc843 object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
3: daebc95542c ! 3: 65801f2c890 object-name: make ambiguous object output translatable
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi
+ * "deadbeef tag Some Tag Message"
+ *
+ * The second argument is the "tag" string from
-+ * object.c, it should (hopefully) already be
-+ * translated.
++ * object.c.
+ */
+ strbuf_addf(&desc, _("%s tag %s"), hash, tag_tag);
+ } else if (type == OBJ_TREE) {
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi
- desc.buf);
+ /*
+ * TRANSLATORS: This is line item of ambiguous object output
-+ * from describe_ambiguous_object() above.
++ * from describe_ambiguous_object() above. For RTL languages
++ * you'll probably want to swap the "%s" and leading " " space
++ * around.
+ */
+ advise(_(" %s"), desc.buf);
4: b5aa6e266f6 ! 4: 2e5511c9fa5 object-name: show date for ambiguous tag objects
@@ Commit message
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 ##
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi
+ * "deadbeef tag 2021-01-01 - Some Tag Message"
*
* The second argument is the "tag" string from
- * object.c, it should (hopefully) already be
- * translated.
+ * object.c.
*/
- strbuf_addf(&desc, _("%s tag %s"), hash, tag_tag);
+ strbuf_addf(&desc, _("%s tag %s - %s"), hash,
5: 644b076b2a6 ! 5: 2c03cdd3c1e object-name: iterate ambiguous objects before showing header
@@ object-name.c: static int init_object_disambiguation(struct repository *r,
int type;
const char *hash;
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data)
- * TRANSLATORS: This is line item of ambiguous object output
- * from describe_ambiguous_object() above.
+ * you'll probably want to swap the "%s" and leading " " space
+ * around.
*/
- advise(_(" %s"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), desc.buf);
@@ object-name.c: static enum get_oid_result get_short_oid(struct repository *r,
return status;
## t/t1512-rev-parse-disambiguation.sh ##
-@@ t/t1512-rev-parse-disambiguation.sh: test_expect_success 'ambiguous loose blob parsed as OBJ_BAD' '
+@@ t/t1512-rev-parse-disambiguation.sh: test_expect_success 'ambiguous loose bad object parsed as OBJ_BAD' '
- cat >expect <<-\EOF &&
+ test_cmp_failed_rev_parse blob.bad bad0 <<-\EOF
error: short object ID bad0... is ambiguous
- hint: The candidates are:
fatal: invalid object type
EOF
- test_cmp_failed_rev_parse blob.bad bad0
+ '
@@ t/t1512-rev-parse-disambiguation.sh: test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
- cat >expect <<-\EOF &&
+ test_cmp_failed_rev_parse blob.corrupt cafe <<-\EOF
error: short object ID cafe... is ambiguous
- hint: The candidates are:
error: inflate: data stream error (incorrect header check)
6: 6a31cfcfc29 ! 6: bf226f67099 object-name: re-use "struct strbuf" in show_ambiguous_object()
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi
strbuf_release(&date);
strbuf_release(&msg);
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data)
- * object.c, it should (hopefully) already be
- * translated.
+ * The second argument is the "tag" string from
+ * object.c.
*/
- strbuf_addf(&desc, _("%s tag %s - %s"), hash,
+ strbuf_addf(sb, _("%s tag %s - %s"), hash,
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data)
- * TRANSLATORS: This is line item of ambiguous object output
- * from describe_ambiguous_object() above.
+ * you'll probably want to swap the "%s" and leading " " space
+ * around.
*/
- strbuf_addf(advice, _(" %s\n"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), sb->buf);
--
2.34.1.1373.g062f5534af2
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:39 ` Ævar Arnfjörð Bjarmason
2022-01-13 22:39 ` Junio C Hamano
2022-01-12 12:39 ` [PATCH v7 2/6] object-name: explicitly handle OBJ_BAD in show_ambiguous_object() Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
6 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Extend the tests for ambiguous objects to check how we handle objects
where we return OBJ_BAD when trying to parse them. As noted in [1] we
have a blindspot when it comes to this behavior.
Since we need to add new test data here let's extend these tests to be
tested under SHA-256, in d7a2fc82491 (t1512: skip test if not using
SHA-1, 2018-05-13) all of the existing tests were skipped, as they
rely on specific SHA-1 object IDs.
For these tests it only matters that the first 4 characters of the OID
prefix are the same for both SHA-1 and SHA-256. This uses strings that
I mined, and have the same prefix when hashed with both.
We "test_cmp" the full output to guard against any future regressions,
and because a subsequent commit will tweak it. Showing a diff of how
the output changes is helpful to explain those subsequent commits.
1. https://lore.kernel.org/git/YZwbphPpfGk78w2f@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t1512-rev-parse-disambiguation.sh | 79 +++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index b0119bf8bc8..01feeeafb72 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -25,6 +25,85 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+test_cmp_failed_rev_parse () {
+ cat >expect &&
+ test_must_fail git -C "$1" rev-parse "$2" 2>actual.raw &&
+ sed "s/\($2\)[0-9a-f]*/\1.../" <actual.raw >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'ambiguous blob output' '
+ git init --bare blob.prefix &&
+ (
+ cd blob.prefix &&
+
+ # Both start with "dead..", under both SHA-1 and SHA-256
+ echo brocdnra | git hash-object -w --stdin &&
+ echo brigddsv | git hash-object -w --stdin &&
+
+ # Both start with "beef.."
+ echo 1agllotbh | git hash-object -w --stdin &&
+ echo 1bbfctrkc | git hash-object -w --stdin
+ ) &&
+
+ test_must_fail git -C blob.prefix rev-parse dead &&
+ test_cmp_failed_rev_parse blob.prefix beef <<-\EOF
+ error: short object ID beef... is ambiguous
+ hint: The candidates are:
+ hint: beef... blob
+ hint: beef... blob
+ fatal: ambiguous argument '\''beef...'\'': unknown revision or path not in the working tree.
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
+'
+
+test_expect_success 'ambiguous loose bad object parsed as OBJ_BAD' '
+ git init --bare blob.bad &&
+ (
+ cd blob.bad &&
+
+ # Both have the prefix "bad0"
+ echo xyzfaowcoh | git hash-object -t bad -w --stdin --literally &&
+ echo xyzhjpyvwl | git hash-object -t bad -w --stdin --literally
+ ) &&
+
+ test_cmp_failed_rev_parse blob.bad bad0 <<-\EOF
+ error: short object ID bad0... is ambiguous
+ hint: The candidates are:
+ fatal: invalid object type
+ EOF
+'
+
+test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
+ git init --bare blob.corrupt &&
+ (
+ cd blob.corrupt &&
+
+ # Both have the prefix "cafe"
+ echo bnkxmdwz | git hash-object -w --stdin &&
+ oid=$(echo bmwsjxzi | git hash-object -w --stdin) &&
+
+ oidf=objects/$(test_oid_to_path "$oid") &&
+ chmod 755 $oidf &&
+ echo broken >$oidf
+ ) &&
+
+ test_cmp_failed_rev_parse blob.corrupt cafe <<-\EOF
+ error: short object ID cafe... is ambiguous
+ hint: The candidates are:
+ error: inflate: data stream error (incorrect header check)
+ error: unable to unpack cafe... header
+ error: inflate: data stream error (incorrect header check)
+ error: unable to unpack cafe... header
+ hint: cafe... unknown type
+ hint: cafe... blob
+ fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
+'
+
if ! test_have_prereq SHA1
then
skip_all='not using SHA-1 for objects'
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots
2022-01-12 12:39 ` [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
@ 2022-01-13 22:39 ` Junio C Hamano
2022-01-14 12:07 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 90+ messages in thread
From: Junio C Hamano @ 2022-01-13 22:39 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> +test_cmp_failed_rev_parse () {
> + cat >expect &&
> + test_must_fail git -C "$1" rev-parse "$2" 2>actual.raw &&
> + sed "s/\($2\)[0-9a-f]*/\1.../" <actual.raw >actual &&
> + test_cmp expect actual
> +}
That's dense, especially without a comment (or named variable) that
hints readers what the arguments to this helper (and its standard
input) ought to be.
As long as messages from rev-parse on the error stream never has
more than one abbreviated object name on a single line, the above
should give us a copy of the message with expected object name
abbreviated to $2; otherwise we might be missing a /g in the sed
script.
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots
2022-01-13 22:39 ` Junio C Hamano
@ 2022-01-14 12:07 ` Ævar Arnfjörð Bjarmason
2022-01-14 18:45 ` Junio C Hamano
0 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-14 12:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
On Thu, Jan 13 2022, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> +test_cmp_failed_rev_parse () {
>> + cat >expect &&
>> + test_must_fail git -C "$1" rev-parse "$2" 2>actual.raw &&
>> + sed "s/\($2\)[0-9a-f]*/\1.../" <actual.raw >actual &&
>> + test_cmp expect actual
>> +}
>
> That's dense, especially without a comment (or named variable) that
> hints readers what the arguments to this helper (and its standard
> input) ought to be.
I got rid of the named variables from v6 in response to a "shift" that
shifted the wrong number, but perhaps I should have just removed the
"shift"?
> As long as messages from rev-parse on the error stream never has
> more than one abbreviated object name on a single line, the above
> should give us a copy of the message with expected object name
> abbreviated to $2; otherwise we might be missing a /g in the sed
> script.
In the v6 you rightly commented on the /g that was there previously not
being needed :)
So I dropped it, in this case we can rely on only getting the
abbreviated output.
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots
2022-01-14 12:07 ` Ævar Arnfjörð Bjarmason
@ 2022-01-14 18:45 ` Junio C Hamano
0 siblings, 0 replies; 90+ messages in thread
From: Junio C Hamano @ 2022-01-14 18:45 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Thu, Jan 13 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> +test_cmp_failed_rev_parse () {
>>> + cat >expect &&
>>> + test_must_fail git -C "$1" rev-parse "$2" 2>actual.raw &&
>>> + sed "s/\($2\)[0-9a-f]*/\1.../" <actual.raw >actual &&
>>> + test_cmp expect actual
>>> +}
>>
>> That's dense, especially without a comment (or named variable) that
>> hints readers what the arguments to this helper (and its standard
>> input) ought to be.
>
> I got rid of the named variables from v6 in response to a "shift" that
> shifted the wrong number, but perhaps I should have just removed the
> "shift"?
I agree that is a more sensible thing you could have done.
>> As long as messages from rev-parse on the error stream never has
>> more than one abbreviated object name on a single line, the above
>> should give us a copy of the message with expected object name
>> abbreviated to $2; otherwise we might be missing a /g in the sed
>> script.
>
> In the v6 you rightly commented on the /g that was there previously not
> being needed :)
>
> So I dropped it, in this case we can rely on only getting the
> abbreviated output.
I do not care either way, as long as it is clearly stated why /g is
there (or why /g is missing) for the future developers.
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v7 2/6] object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:39 ` Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 3/6] object-name: make ambiguous object output translatable Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
6 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Amend the "unknown type" handling in the code that displays the
ambiguous object list to assert() that we're either going to get the
"real" object types we can pass to type_name(), or a -1 (OBJ_BAD)
return value from oid_object_info().
See [1] for the current output, and [1] for the commit that added the
"unknown type" handling.
We are never going to get an "unknown type" in the sense of custom
types crafted with "hash-object --literally", since we're not using
the OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag.
If we manage to otherwise unpack such an object without errors we'll
die() in parse_loose_header_extended() called by sort_ambiguous()
before we get to show_ambiguous_object(), as is asserted by the test
added in the preceding commit.
So saying "unknown type" here was always misleading, we really meant
to say that we had a failure parsing the object at all, i.e. that we
had repository corruption. If the problem is only that it's type is
unknown we won't reach this code.
So let's emit a generic "[bad object]" instead. As our tests added in
the preceding commit show, we'll have emitted various "error" output
already in those cases.
We should do better in the truly "unknown type" cases, which we'd need
to handle if we were passing down the OBJECT_INFO_ALLOW_UNKNOWN_TYPE
flag. But let's leave that for some future improvement. In a
subsequent commit I'll improve the output we do show, and not having
to handle the "unknown type" (as in OBJECT_INFO_ALLOW_UNKNOWN_TYPE)
simplifies that change.
1. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
2. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 14 ++++++++++++--
t/t1512-rev-parse-disambiguation.sh | 2 +-
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..9750634ee76 100644
--- a/object-name.c
+++ b/object-name.c
@@ -361,6 +361,16 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
return 0;
type = oid_object_info(ds->repo, oid, NULL);
+
+ if (type < 0) {
+ strbuf_addstr(&desc, "[bad object]");
+ goto out;
+ }
+
+ assert(type == OBJ_TREE || type == OBJ_COMMIT ||
+ type == OBJ_BLOB || type == OBJ_TAG);
+ strbuf_addstr(&desc, type_name(type));
+
if (type == OBJ_COMMIT) {
struct commit *commit = lookup_commit(ds->repo, oid);
if (commit) {
@@ -374,9 +384,9 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
strbuf_addf(&desc, " %s", tag->tag);
}
- advise(" %s %s%s",
+out:
+ advise(" %s %s",
repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV),
- type_name(type) ? type_name(type) : "unknown type",
desc.buf);
strbuf_release(&desc);
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 01feeeafb72..5ed7e49edc7 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -96,7 +96,7 @@ test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
error: unable to unpack cafe... header
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
- hint: cafe... unknown type
+ hint: cafe... [bad object]
hint: cafe... blob
fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
Use '\''--'\'' to separate paths from revisions, like this:
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v7 3/6] object-name: make ambiguous object output translatable
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 1/6] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 2/6] object-name: explicitly handle OBJ_BAD in show_ambiguous_object() Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:39 ` Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 4/6] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
6 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Change the output of show_ambiguous_object() added in [1] and last
tweaked in [2] and the preceding commit to be more friendly to
translators.
By being able to customize the "<SP><SP>%s\n" format we're even ready
for RTL languages, who'd presumably like to change that to
"%s<SP><SP>\n".
1. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
2. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
object-name.c | 66 +++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 59 insertions(+), 7 deletions(-)
diff --git a/object-name.c b/object-name.c
index 9750634ee76..743f346842a 100644
--- a/object-name.c
+++ b/object-name.c
@@ -356,38 +356,90 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
const struct disambiguate_state *ds = data;
struct strbuf desc = STRBUF_INIT;
int type;
+ const char *hash;
if (ds->fn && !ds->fn(ds->repo, oid, ds->cb_data))
return 0;
+ hash = repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV);
type = oid_object_info(ds->repo, oid, NULL);
if (type < 0) {
- strbuf_addstr(&desc, "[bad object]");
+ /*
+ * TRANSLATORS: This is a line of ambiguous object
+ * output shown when we cannot look up or parse the
+ * object in question. E.g. "deadbeef [bad object]".
+ */
+ strbuf_addf(&desc, _("%s [bad object]"), hash);
goto out;
}
assert(type == OBJ_TREE || type == OBJ_COMMIT ||
type == OBJ_BLOB || type == OBJ_TAG);
- strbuf_addstr(&desc, type_name(type));
if (type == OBJ_COMMIT) {
+ struct strbuf date = STRBUF_INIT;
+ struct strbuf msg = STRBUF_INIT;
struct commit *commit = lookup_commit(ds->repo, oid);
+
if (commit) {
struct pretty_print_context pp = {0};
pp.date_mode.type = DATE_SHORT;
- format_commit_message(commit, " %ad - %s", &desc, &pp);
+ format_commit_message(commit, "%ad", &date, &pp);
+ format_commit_message(commit, "%s", &msg, &pp);
}
+
+ /*
+ * TRANSLATORS: This is a line of ambiguous commit
+ * object output. E.g.:
+ *
+ * "deadbeef commit 2021-01-01 - Some Commit Message"
+ */
+ strbuf_addf(&desc, _("%s commit %s - %s"),
+ hash, date.buf, msg.buf);
+
+ strbuf_release(&date);
+ strbuf_release(&msg);
} else if (type == OBJ_TAG) {
struct tag *tag = lookup_tag(ds->repo, oid);
+ const char *tag_tag = "";
+
if (!parse_tag(tag) && tag->tag)
- strbuf_addf(&desc, " %s", tag->tag);
+ tag_tag = tag->tag;
+
+ /*
+ * TRANSLATORS: This is a line of
+ * ambiguous tag object output. E.g.:
+ *
+ * "deadbeef tag Some Tag Message"
+ *
+ * The second argument is the "tag" string from
+ * object.c.
+ */
+ strbuf_addf(&desc, _("%s tag %s"), hash, tag_tag);
+ } else if (type == OBJ_TREE) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous <type>
+ * object output. E.g. "deadbeef tree".
+ */
+ strbuf_addf(&desc, _("%s tree"), hash);
+ } else if (type == OBJ_BLOB) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous <type>
+ * object output. E.g. "deadbeef blob".
+ */
+ strbuf_addf(&desc, _("%s blob"), hash);
}
+
out:
- advise(" %s %s",
- repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV),
- desc.buf);
+ /*
+ * TRANSLATORS: This is line item of ambiguous object output
+ * from describe_ambiguous_object() above. For RTL languages
+ * you'll probably want to swap the "%s" and leading " " space
+ * around.
+ */
+ advise(_(" %s"), desc.buf);
strbuf_release(&desc);
return 0;
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v7 4/6] object-name: show date for ambiguous tag objects
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-01-12 12:39 ` [PATCH v7 3/6] object-name: make ambiguous object output translatable Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:39 ` Ævar Arnfjörð Bjarmason
2022-01-13 22:46 ` Junio C Hamano
2022-01-12 12:39 ` [PATCH v7 5/6] object-name: iterate ambiguous objects before showing header Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
6 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
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(-)
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>
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v7 4/6] object-name: show date for ambiguous tag objects
2022-01-12 12:39 ` [PATCH v7 4/6] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
@ 2022-01-13 22:46 ` Junio C Hamano
2022-01-14 12:05 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 90+ messages in thread
From: Junio C Hamano @ 2022-01-13 22:46 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Æ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>
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v7 4/6] object-name: show date for ambiguous tag objects
2022-01-13 22:46 ` Junio C Hamano
@ 2022-01-14 12:05 ` Ævar Arnfjörð Bjarmason
2022-01-14 19:04 ` Junio C Hamano
0 siblings, 1 reply; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-14 12:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
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>
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v7 4/6] object-name: show date for ambiguous tag objects
2022-01-14 12:05 ` Ævar Arnfjörð Bjarmason
@ 2022-01-14 19:04 ` Junio C Hamano
2022-01-14 19:35 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 90+ messages in thread
From: Junio C Hamano @ 2022-01-14 19:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Æ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.
^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH v7 4/6] object-name: show date for ambiguous tag objects
2022-01-14 19:04 ` Junio C Hamano
@ 2022-01-14 19:35 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-14 19:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
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.
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v7 5/6] object-name: iterate ambiguous objects before showing header
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-01-12 12:39 ` [PATCH v7 4/6] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:39 ` Ævar Arnfjörð Bjarmason
2022-01-12 12:39 ` [PATCH v7 6/6] object-name: re-use "struct strbuf" in show_ambiguous_object() Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
6 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Change the "The candidates are" header that's shown for ambiguous
objects to be shown after we've iterated over all of the objects.
If we get any errors while doing so we don't want to split up the the
header and the list as a result. The two will now be printed together,
as shown in the updated testcase.
As we're accumulating the lines into as "struct strbuf" before
emitting them we need to add a trailing newline to the call in
show_ambiguous_object(). This and the change from "The candidates
are:" to "The candidates are:\n%s" helps to give translators more
context.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 27 +++++++++++++++++++++++----
t/t1512-rev-parse-disambiguation.sh | 3 +--
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/object-name.c b/object-name.c
index 7c6cb60ceff..71236ed1c16 100644
--- a/object-name.c
+++ b/object-name.c
@@ -351,9 +351,16 @@ static int init_object_disambiguation(struct repository *r,
return 0;
}
+struct ambiguous_output {
+ const struct disambiguate_state *ds;
+ struct strbuf advice;
+};
+
static int show_ambiguous_object(const struct object_id *oid, void *data)
{
- const struct disambiguate_state *ds = data;
+ struct ambiguous_output *state = data;
+ const struct disambiguate_state *ds = state->ds;
+ struct strbuf *advice = &state->advice;
struct strbuf desc = STRBUF_INIT;
int type;
const char *hash;
@@ -444,7 +451,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* you'll probably want to swap the "%s" and leading " " space
* around.
*/
- advise(_(" %s"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), desc.buf);
strbuf_release(&desc);
return 0;
@@ -543,6 +550,10 @@ static enum get_oid_result get_short_oid(struct repository *r,
if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
struct oid_array collect = OID_ARRAY_INIT;
+ struct ambiguous_output out = {
+ .ds = &ds,
+ .advice = STRBUF_INIT,
+ };
error(_("short object ID %s is ambiguous"), ds.hex_pfx);
@@ -555,13 +566,21 @@ static enum get_oid_result get_short_oid(struct repository *r,
if (!ds.ambiguous)
ds.fn = NULL;
- advise(_("The candidates are:"));
repo_for_each_abbrev(r, ds.hex_pfx, collect_ambiguous, &collect);
sort_ambiguous_oid_array(r, &collect);
- if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
+ if (oid_array_for_each(&collect, show_ambiguous_object, &out))
BUG("show_ambiguous_object shouldn't return non-zero");
+
+ /*
+ * TRANSLATORS: The argument is the list of ambiguous
+ * objects composed in show_ambiguous_object(). See
+ * its "TRANSLATORS" comments for details.
+ */
+ advise(_("The candidates are:\n%s"), out.advice.buf);
+
oid_array_clear(&collect);
+ strbuf_release(&out.advice);
}
return status;
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 5ed7e49edc7..9c43699d3ae 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -70,7 +70,6 @@ test_expect_success 'ambiguous loose bad object parsed as OBJ_BAD' '
test_cmp_failed_rev_parse blob.bad bad0 <<-\EOF
error: short object ID bad0... is ambiguous
- hint: The candidates are:
fatal: invalid object type
EOF
'
@@ -91,11 +90,11 @@ test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
test_cmp_failed_rev_parse blob.corrupt cafe <<-\EOF
error: short object ID cafe... is ambiguous
- hint: The candidates are:
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
+ hint: The candidates are:
hint: cafe... [bad object]
hint: cafe... blob
fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v7 6/6] object-name: re-use "struct strbuf" in show_ambiguous_object()
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-01-12 12:39 ` [PATCH v7 5/6] object-name: iterate ambiguous objects before showing header Ævar Arnfjörð Bjarmason
@ 2022-01-12 12:39 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
6 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-12 12:39 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Reduce the allocations done by show_ambiguous_object() by moving the
"desc" strbuf into the "struct ambiguous_output" introduced in the
preceding commit.
This doesn't matter for optimization purposes, but since we're
accumulating a "struct strbuf advice" anyway let's follow that pattern
and add a "struct strbuf sb", we can then strbuf_reset() it rather
than calling strbuf_release() for each call to
show_ambiguous_object().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/object-name.c b/object-name.c
index 71236ed1c16..bce3f42356a 100644
--- a/object-name.c
+++ b/object-name.c
@@ -354,6 +354,7 @@ static int init_object_disambiguation(struct repository *r,
struct ambiguous_output {
const struct disambiguate_state *ds;
struct strbuf advice;
+ struct strbuf sb;
};
static int show_ambiguous_object(const struct object_id *oid, void *data)
@@ -361,7 +362,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
struct ambiguous_output *state = data;
const struct disambiguate_state *ds = state->ds;
struct strbuf *advice = &state->advice;
- struct strbuf desc = STRBUF_INIT;
+ struct strbuf *sb = &state->sb;
int type;
const char *hash;
@@ -377,7 +378,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* output shown when we cannot look up or parse the
* object in question. E.g. "deadbeef [bad object]".
*/
- strbuf_addf(&desc, _("%s [bad object]"), hash);
+ strbuf_addf(sb, _("%s [bad object]"), hash);
goto out;
}
@@ -402,8 +403,8 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
*
* "deadbeef commit 2021-01-01 - Some Commit Message"
*/
- strbuf_addf(&desc, _("%s commit %s - %s"),
- hash, date.buf, msg.buf);
+ strbuf_addf(sb, _("%s commit %s - %s"), hash, date.buf,
+ msg.buf);
strbuf_release(&date);
strbuf_release(&msg);
@@ -426,7 +427,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* The second argument is the "tag" string from
* object.c.
*/
- strbuf_addf(&desc, _("%s tag %s - %s"), hash,
+ strbuf_addf(sb, _("%s tag %s - %s"), hash,
show_date(tag_date, 0, DATE_MODE(SHORT)),
tag_tag);
} else if (type == OBJ_TREE) {
@@ -434,13 +435,13 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* TRANSLATORS: This is a line of ambiguous <type>
* object output. E.g. "deadbeef tree".
*/
- strbuf_addf(&desc, _("%s tree"), hash);
+ strbuf_addf(sb, _("%s tree"), hash);
} else if (type == OBJ_BLOB) {
/*
* TRANSLATORS: This is a line of ambiguous <type>
* object output. E.g. "deadbeef blob".
*/
- strbuf_addf(&desc, _("%s blob"), hash);
+ strbuf_addf(sb, _("%s blob"), hash);
}
@@ -451,9 +452,9 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* you'll probably want to swap the "%s" and leading " " space
* around.
*/
- strbuf_addf(advice, _(" %s\n"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), sb->buf);
- strbuf_release(&desc);
+ strbuf_reset(sb);
return 0;
}
@@ -552,6 +553,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
struct oid_array collect = OID_ARRAY_INIT;
struct ambiguous_output out = {
.ds = &ds,
+ .sb = STRBUF_INIT,
.advice = STRBUF_INIT,
};
@@ -581,6 +583,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
oid_array_clear(&collect);
strbuf_release(&out.advice);
+ strbuf_release(&out.sb);
}
return status;
--
2.34.1.1373.g062f5534af2
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date
2022-01-12 12:39 ` [PATCH v7 0/6] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2022-01-12 12:39 ` [PATCH v7 6/6] object-name: re-use "struct strbuf" in show_ambiguous_object() Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 1/7] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
` (7 more replies)
6 siblings, 8 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
This topic improves the output we emit on ambiguous objects as noted
in 5/7, and makes it translatable, see 4/7. See [1] for v7.
This v8 addresses feedback from Junio. There's a small test change +
commit message change in 1/7, and a rather small change in adding an
explicit message in 5/7 for tags that we cannot parse.
The range-diff looks rather scary though because if we're going to add
such output it made sense to add it in a new 3/7, before we made the
output translatable, and then to carry that change forward.
1. https://lore.kernel.org/git/cover-v7-0.6-00000000000-20220111T130811Z-avarab@gmail.com/
Ævar Arnfjörð Bjarmason (7):
object-name tests: add tests for ambiguous object blind spots
object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
object-name: explicitly handle bad tags in show_ambiguous_object()
object-name: make ambiguous object output translatable
object-name: show date for ambiguous tag objects
object-name: iterate ambiguous objects before showing header
object-name: re-use "struct strbuf" in show_ambiguous_object()
object-name.c | 121 +++++++++++++++++++++++++---
t/t1512-rev-parse-disambiguation.sh | 81 +++++++++++++++++++
2 files changed, 190 insertions(+), 12 deletions(-)
Range-diff against v7:
1: 28c01b7f8a5 ! 1: 756c94bda7a object-name tests: add tests for ambiguous object blind spots
@@ Commit message
and because a subsequent commit will tweak it. Showing a diff of how
the output changes is helpful to explain those subsequent commits.
+ The "sed" invocation in test_cmp_failed_rev_parse() doesn't need a
+ "/g" because under both SHA-1 and SHA-256 we'll wildcard match any
+ trailing part of the OID after our known starting prefix. We'd like to
+ convert all of that to just "..." for the "test_cmp" which follows.
+
1. https://lore.kernel.org/git/YZwbphPpfGk78w2f@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
@@ t/t1512-rev-parse-disambiguation.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+test_cmp_failed_rev_parse () {
++ dir=$1
++ rev=$2
++
+ cat >expect &&
-+ test_must_fail git -C "$1" rev-parse "$2" 2>actual.raw &&
-+ sed "s/\($2\)[0-9a-f]*/\1.../" <actual.raw >actual &&
++ test_must_fail git -C "$dir" rev-parse "$rev" 2>actual.raw &&
++ sed "s/\($rev\)[0-9a-f]*/\1.../" <actual.raw >actual &&
+ test_cmp expect actual
+}
+
2: b7027dfc843 = 2: e60f100003a object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
4: 2e5511c9fa5 ! 3: eaede34fa4f object-name: show date for ambiguous tag objects
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- object-name: show date for ambiguous tag objects
+ object-name: explicitly handle bad tags in show_ambiguous_object()
- 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.:
+ Follow-up the handling of OBJ_BAD in the preceding commit and
+ explicitly handle those cases where parse_tag() fails, or we don't end
+ up with a non-NULL pointer in in tag->tag.
- $ 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
- [...]
+ If we run into such a tag we'd previously be silent about it. We
+ really should also be handling these batter in parse_tag_buffer() by
+ being more eager to emit an error(), instead of silently aborting with
+ "return -1;".
- Before this we'd emit a "tag" line of:
+ One example of such a tag is the one that's tested for in
+ "t3800-mktag.sh", where the code takes the "size <
+ the_hash_algo->hexsz + 24" branch.
- hint: b7e68c41d92 tag v2.32.0
+ But in lieu of earlier missing "error" output let's show the user
+ something to indicate why we're not showing a tag message in these
+ cases, now instead of showing:
- 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: deadbeef tag
- hint: 8d62cb0b06 tag 1970-01-01 -
+ We'll instead display:
- 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.
+ hint: deadbeef tag [tag could not be parsed]
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## object-name.c ##
@@ object-name.c: 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;
-+ }
+ if (!parse_tag(tag) && tag->tag)
+ strbuf_addf(&desc, " %s", tag->tag);
++ else
++ strbuf_addstr(&desc, " [tag could not be parsed]");
+ }
- /*
- * 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>
+ out:
3: 65801f2c890 ! 4: 6a26c917a94 object-name: make ambiguous object output translatable
@@ Commit message
for RTL languages, who'd presumably like to change that to
"%s<SP><SP>\n".
+ In the case of the existing "tag [tag could not be parsed]" output
+ we'll now instead emit "[bad tag, could not parse it]". This is
+ consistent with the "[bad object]" output. Rephrasing the message like
+ this is possible because we're not unconditionally adding the
+ type_name() at the beginning.
+
1. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
2. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi
+ strbuf_release(&msg);
} else if (type == OBJ_TAG) {
struct tag *tag = lookup_tag(ds->repo, oid);
-+ const char *tag_tag = "";
-+
- if (!parse_tag(tag) && tag->tag)
+- if (!parse_tag(tag) && tag->tag)
- strbuf_addf(&desc, " %s", tag->tag);
-+ tag_tag = tag->tag;
+- else
+- strbuf_addstr(&desc, " [tag could not be parsed]");
+
-+ /*
-+ * TRANSLATORS: This is a line of
-+ * ambiguous tag object output. E.g.:
-+ *
-+ * "deadbeef tag Some Tag Message"
-+ *
-+ * The second argument is the "tag" string from
-+ * object.c.
-+ */
-+ strbuf_addf(&desc, _("%s tag %s"), hash, tag_tag);
++ if (!parse_tag(tag) && tag->tag) {
++ /*
++ * TRANSLATORS: This is a line of ambiguous
++ * tag object output. E.g.:
++ *
++ * "deadbeef tag Some Tag Message"
++ *
++ * The second argument is the "tag" string
++ * from object.c.
++ */
++ strbuf_addf(&desc, _("%s tag %s"), hash, tag->tag);
++ } else {
++ /*
++ * TRANSLATORS: This is a line of ambiguous
++ * tag object output where we couldn't parse
++ * the tag itself. E.g.:
++ *
++ * "deadbeef tag [bad tag, could not parse it]"
++ */
++ strbuf_addf(&desc, _("%s [bad tag, could not parse it]"),
++ hash);
++ }
+ } else if (type == OBJ_TREE) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous <type>
-: ----------- > 5: 6237f07e3a9 object-name: show date for ambiguous tag objects
5: 2c03cdd3c1e = 6: 57336c67dd2 object-name: iterate ambiguous objects before showing header
6: bf226f67099 ! 7: 1f0e1053918 object-name: re-use "struct strbuf" in show_ambiguous_object()
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi
strbuf_release(&date);
strbuf_release(&msg);
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data)
- * The second argument is the "tag" string from
- * object.c.
- */
-- strbuf_addf(&desc, _("%s tag %s - %s"), hash,
-+ strbuf_addf(sb, _("%s tag %s - %s"), hash,
- show_date(tag_date, 0, DATE_MODE(SHORT)),
- tag_tag);
+ * The third argument is the "tag" string
+ * from object.c.
+ */
+- strbuf_addf(&desc, _("%s tag %s - %s"), hash,
++ strbuf_addf(sb, _("%s tag %s - %s"), hash,
+ show_date(tag->date, 0, DATE_MODE(SHORT)),
+ tag->tag);
+ } else {
+@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data)
+ *
+ * "deadbeef [bad tag, could not parse it]"
+ */
+- strbuf_addf(&desc, _("%s [bad tag, could not parse it]"),
++ strbuf_addf(sb, _("%s [bad tag, could not parse it]"),
+ hash);
+ }
} else if (type == OBJ_TREE) {
@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data)
* TRANSLATORS: This is a line of ambiguous <type>
--
2.35.0.890.gd7e422415d9
^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH v8 1/7] object-name tests: add tests for ambiguous object blind spots
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 2/7] object-name: explicitly handle OBJ_BAD in show_ambiguous_object() Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Extend the tests for ambiguous objects to check how we handle objects
where we return OBJ_BAD when trying to parse them. As noted in [1] we
have a blindspot when it comes to this behavior.
Since we need to add new test data here let's extend these tests to be
tested under SHA-256, in d7a2fc82491 (t1512: skip test if not using
SHA-1, 2018-05-13) all of the existing tests were skipped, as they
rely on specific SHA-1 object IDs.
For these tests it only matters that the first 4 characters of the OID
prefix are the same for both SHA-1 and SHA-256. This uses strings that
I mined, and have the same prefix when hashed with both.
We "test_cmp" the full output to guard against any future regressions,
and because a subsequent commit will tweak it. Showing a diff of how
the output changes is helpful to explain those subsequent commits.
The "sed" invocation in test_cmp_failed_rev_parse() doesn't need a
"/g" because under both SHA-1 and SHA-256 we'll wildcard match any
trailing part of the OID after our known starting prefix. We'd like to
convert all of that to just "..." for the "test_cmp" which follows.
1. https://lore.kernel.org/git/YZwbphPpfGk78w2f@coredump.intra.peff.net/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
t/t1512-rev-parse-disambiguation.sh | 82 +++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index b0119bf8bc8..c14d88eae20 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -25,6 +25,88 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+test_cmp_failed_rev_parse () {
+ dir=$1
+ rev=$2
+
+ cat >expect &&
+ test_must_fail git -C "$dir" rev-parse "$rev" 2>actual.raw &&
+ sed "s/\($rev\)[0-9a-f]*/\1.../" <actual.raw >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'ambiguous blob output' '
+ git init --bare blob.prefix &&
+ (
+ cd blob.prefix &&
+
+ # Both start with "dead..", under both SHA-1 and SHA-256
+ echo brocdnra | git hash-object -w --stdin &&
+ echo brigddsv | git hash-object -w --stdin &&
+
+ # Both start with "beef.."
+ echo 1agllotbh | git hash-object -w --stdin &&
+ echo 1bbfctrkc | git hash-object -w --stdin
+ ) &&
+
+ test_must_fail git -C blob.prefix rev-parse dead &&
+ test_cmp_failed_rev_parse blob.prefix beef <<-\EOF
+ error: short object ID beef... is ambiguous
+ hint: The candidates are:
+ hint: beef... blob
+ hint: beef... blob
+ fatal: ambiguous argument '\''beef...'\'': unknown revision or path not in the working tree.
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
+'
+
+test_expect_success 'ambiguous loose bad object parsed as OBJ_BAD' '
+ git init --bare blob.bad &&
+ (
+ cd blob.bad &&
+
+ # Both have the prefix "bad0"
+ echo xyzfaowcoh | git hash-object -t bad -w --stdin --literally &&
+ echo xyzhjpyvwl | git hash-object -t bad -w --stdin --literally
+ ) &&
+
+ test_cmp_failed_rev_parse blob.bad bad0 <<-\EOF
+ error: short object ID bad0... is ambiguous
+ hint: The candidates are:
+ fatal: invalid object type
+ EOF
+'
+
+test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
+ git init --bare blob.corrupt &&
+ (
+ cd blob.corrupt &&
+
+ # Both have the prefix "cafe"
+ echo bnkxmdwz | git hash-object -w --stdin &&
+ oid=$(echo bmwsjxzi | git hash-object -w --stdin) &&
+
+ oidf=objects/$(test_oid_to_path "$oid") &&
+ chmod 755 $oidf &&
+ echo broken >$oidf
+ ) &&
+
+ test_cmp_failed_rev_parse blob.corrupt cafe <<-\EOF
+ error: short object ID cafe... is ambiguous
+ hint: The candidates are:
+ error: inflate: data stream error (incorrect header check)
+ error: unable to unpack cafe... header
+ error: inflate: data stream error (incorrect header check)
+ error: unable to unpack cafe... header
+ hint: cafe... unknown type
+ hint: cafe... blob
+ fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
+ Use '\''--'\'' to separate paths from revisions, like this:
+ '\''git <command> [<revision>...] -- [<file>...]'\''
+ EOF
+'
+
if ! test_have_prereq SHA1
then
skip_all='not using SHA-1 for objects'
--
2.35.0.890.gd7e422415d9
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 2/7] object-name: explicitly handle OBJ_BAD in show_ambiguous_object()
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 1/7] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 3/7] object-name: explicitly handle bad tags " Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Amend the "unknown type" handling in the code that displays the
ambiguous object list to assert() that we're either going to get the
"real" object types we can pass to type_name(), or a -1 (OBJ_BAD)
return value from oid_object_info().
See [1] for the current output, and [1] for the commit that added the
"unknown type" handling.
We are never going to get an "unknown type" in the sense of custom
types crafted with "hash-object --literally", since we're not using
the OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag.
If we manage to otherwise unpack such an object without errors we'll
die() in parse_loose_header_extended() called by sort_ambiguous()
before we get to show_ambiguous_object(), as is asserted by the test
added in the preceding commit.
So saying "unknown type" here was always misleading, we really meant
to say that we had a failure parsing the object at all, i.e. that we
had repository corruption. If the problem is only that it's type is
unknown we won't reach this code.
So let's emit a generic "[bad object]" instead. As our tests added in
the preceding commit show, we'll have emitted various "error" output
already in those cases.
We should do better in the truly "unknown type" cases, which we'd need
to handle if we were passing down the OBJECT_INFO_ALLOW_UNKNOWN_TYPE
flag. But let's leave that for some future improvement. In a
subsequent commit I'll improve the output we do show, and not having
to handle the "unknown type" (as in OBJECT_INFO_ALLOW_UNKNOWN_TYPE)
simplifies that change.
1. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
2. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 14 ++++++++++++--
t/t1512-rev-parse-disambiguation.sh | 2 +-
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/object-name.c b/object-name.c
index fdff4601b2c..9750634ee76 100644
--- a/object-name.c
+++ b/object-name.c
@@ -361,6 +361,16 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
return 0;
type = oid_object_info(ds->repo, oid, NULL);
+
+ if (type < 0) {
+ strbuf_addstr(&desc, "[bad object]");
+ goto out;
+ }
+
+ assert(type == OBJ_TREE || type == OBJ_COMMIT ||
+ type == OBJ_BLOB || type == OBJ_TAG);
+ strbuf_addstr(&desc, type_name(type));
+
if (type == OBJ_COMMIT) {
struct commit *commit = lookup_commit(ds->repo, oid);
if (commit) {
@@ -374,9 +384,9 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
strbuf_addf(&desc, " %s", tag->tag);
}
- advise(" %s %s%s",
+out:
+ advise(" %s %s",
repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV),
- type_name(type) ? type_name(type) : "unknown type",
desc.buf);
strbuf_release(&desc);
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index c14d88eae20..80102cc43a3 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -99,7 +99,7 @@ test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
error: unable to unpack cafe... header
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
- hint: cafe... unknown type
+ hint: cafe... [bad object]
hint: cafe... blob
fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
Use '\''--'\'' to separate paths from revisions, like this:
--
2.35.0.890.gd7e422415d9
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 3/7] object-name: explicitly handle bad tags in show_ambiguous_object()
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 1/7] object-name tests: add tests for ambiguous object blind spots Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 2/7] object-name: explicitly handle OBJ_BAD in show_ambiguous_object() Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 4/7] object-name: make ambiguous object output translatable Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Follow-up the handling of OBJ_BAD in the preceding commit and
explicitly handle those cases where parse_tag() fails, or we don't end
up with a non-NULL pointer in in tag->tag.
If we run into such a tag we'd previously be silent about it. We
really should also be handling these batter in parse_tag_buffer() by
being more eager to emit an error(), instead of silently aborting with
"return -1;".
One example of such a tag is the one that's tested for in
"t3800-mktag.sh", where the code takes the "size <
the_hash_algo->hexsz + 24" branch.
But in lieu of earlier missing "error" output let's show the user
something to indicate why we're not showing a tag message in these
cases, now instead of showing:
hint: deadbeef tag
We'll instead display:
hint: deadbeef tag [tag could not be parsed]
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/object-name.c b/object-name.c
index 9750634ee76..298b742bac9 100644
--- a/object-name.c
+++ b/object-name.c
@@ -382,6 +382,8 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
struct tag *tag = lookup_tag(ds->repo, oid);
if (!parse_tag(tag) && tag->tag)
strbuf_addf(&desc, " %s", tag->tag);
+ else
+ strbuf_addstr(&desc, " [tag could not be parsed]");
}
out:
--
2.35.0.890.gd7e422415d9
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 4/7] object-name: make ambiguous object output translatable
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-01-27 5:26 ` [PATCH v8 3/7] object-name: explicitly handle bad tags " Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 5/7] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Change the output of show_ambiguous_object() added in [1] and last
tweaked in [2] and the preceding commit to be more friendly to
translators.
By being able to customize the "<SP><SP>%s\n" format we're even ready
for RTL languages, who'd presumably like to change that to
"%s<SP><SP>\n".
In the case of the existing "tag [tag could not be parsed]" output
we'll now instead emit "[bad tag, could not parse it]". This is
consistent with the "[bad object]" output. Rephrasing the message like
this is possible because we're not unconditionally adding the
type_name() at the beginning.
1. 1ffa26c461 (get_short_sha1: list ambiguous objects on error,
2016-09-26)
2. 5cc044e0257 (get_short_oid: sort ambiguous objects by type,
then SHA-1, 2018-05-10)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Josh Steadmon <steadmon@google.com>
---
object-name.c | 78 ++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 68 insertions(+), 10 deletions(-)
diff --git a/object-name.c b/object-name.c
index 298b742bac9..f31b50bc315 100644
--- a/object-name.c
+++ b/object-name.c
@@ -356,40 +356,98 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
const struct disambiguate_state *ds = data;
struct strbuf desc = STRBUF_INIT;
int type;
+ const char *hash;
if (ds->fn && !ds->fn(ds->repo, oid, ds->cb_data))
return 0;
+ hash = repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV);
type = oid_object_info(ds->repo, oid, NULL);
if (type < 0) {
- strbuf_addstr(&desc, "[bad object]");
+ /*
+ * TRANSLATORS: This is a line of ambiguous object
+ * output shown when we cannot look up or parse the
+ * object in question. E.g. "deadbeef [bad object]".
+ */
+ strbuf_addf(&desc, _("%s [bad object]"), hash);
goto out;
}
assert(type == OBJ_TREE || type == OBJ_COMMIT ||
type == OBJ_BLOB || type == OBJ_TAG);
- strbuf_addstr(&desc, type_name(type));
if (type == OBJ_COMMIT) {
+ struct strbuf date = STRBUF_INIT;
+ struct strbuf msg = STRBUF_INIT;
struct commit *commit = lookup_commit(ds->repo, oid);
+
if (commit) {
struct pretty_print_context pp = {0};
pp.date_mode.type = DATE_SHORT;
- format_commit_message(commit, " %ad - %s", &desc, &pp);
+ format_commit_message(commit, "%ad", &date, &pp);
+ format_commit_message(commit, "%s", &msg, &pp);
}
+
+ /*
+ * TRANSLATORS: This is a line of ambiguous commit
+ * object output. E.g.:
+ *
+ * "deadbeef commit 2021-01-01 - Some Commit Message"
+ */
+ strbuf_addf(&desc, _("%s commit %s - %s"),
+ hash, date.buf, msg.buf);
+
+ strbuf_release(&date);
+ strbuf_release(&msg);
} else if (type == OBJ_TAG) {
struct tag *tag = lookup_tag(ds->repo, oid);
- if (!parse_tag(tag) && tag->tag)
- strbuf_addf(&desc, " %s", tag->tag);
- else
- strbuf_addstr(&desc, " [tag could not be parsed]");
+
+ if (!parse_tag(tag) && tag->tag) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous
+ * tag object output. E.g.:
+ *
+ * "deadbeef tag Some Tag Message"
+ *
+ * The second argument is the "tag" string
+ * from object.c.
+ */
+ strbuf_addf(&desc, _("%s tag %s"), hash, tag->tag);
+ } else {
+ /*
+ * TRANSLATORS: This is a line of ambiguous
+ * tag object output where we couldn't parse
+ * the tag itself. E.g.:
+ *
+ * "deadbeef tag [bad tag, could not parse it]"
+ */
+ strbuf_addf(&desc, _("%s [bad tag, could not parse it]"),
+ hash);
+ }
+ } else if (type == OBJ_TREE) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous <type>
+ * object output. E.g. "deadbeef tree".
+ */
+ strbuf_addf(&desc, _("%s tree"), hash);
+ } else if (type == OBJ_BLOB) {
+ /*
+ * TRANSLATORS: This is a line of ambiguous <type>
+ * object output. E.g. "deadbeef blob".
+ */
+ strbuf_addf(&desc, _("%s blob"), hash);
}
+
out:
- advise(" %s %s",
- repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV),
- desc.buf);
+ /*
+ * TRANSLATORS: This is line item of ambiguous object output
+ * from describe_ambiguous_object() above. For RTL languages
+ * you'll probably want to swap the "%s" and leading " " space
+ * around.
+ */
+ advise(_(" %s"), desc.buf);
strbuf_release(&desc);
return 0;
--
2.35.0.890.gd7e422415d9
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 5/7] object-name: show date for ambiguous tag objects
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-01-27 5:26 ` [PATCH v8 4/7] object-name: make ambiguous object output translatable Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 6/7] object-name: iterate ambiguous objects before showing header Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
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 without a date, e.g.:
hint: b7e68c41d92 tag v2.32.0
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/object-name.c b/object-name.c
index f31b50bc315..cbf459f5664 100644
--- a/object-name.c
+++ b/object-name.c
@@ -408,19 +408,24 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* TRANSLATORS: This is a line of ambiguous
* tag object output. E.g.:
*
- * "deadbeef tag Some Tag Message"
+ * "deadbeef tag 2022-01-01 - Some Tag Message"
*
- * The second argument is the "tag" string
+ * The second argument is the YYYY-MM-DD found
+ * in the tag.
+ *
+ * The third 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 {
/*
* TRANSLATORS: This is a line of ambiguous
* tag object output where we couldn't parse
* the tag itself. E.g.:
*
- * "deadbeef tag [bad tag, could not parse it]"
+ * "deadbeef [bad tag, could not parse it]"
*/
strbuf_addf(&desc, _("%s [bad tag, could not parse it]"),
hash);
--
2.35.0.890.gd7e422415d9
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 6/7] object-name: iterate ambiguous objects before showing header
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-01-27 5:26 ` [PATCH v8 5/7] object-name: show date for ambiguous tag objects Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 5:26 ` [PATCH v8 7/7] object-name: re-use "struct strbuf" in show_ambiguous_object() Ævar Arnfjörð Bjarmason
2022-01-27 20:14 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Junio C Hamano
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Change the "The candidates are" header that's shown for ambiguous
objects to be shown after we've iterated over all of the objects.
If we get any errors while doing so we don't want to split up the the
header and the list as a result. The two will now be printed together,
as shown in the updated testcase.
As we're accumulating the lines into as "struct strbuf" before
emitting them we need to add a trailing newline to the call in
show_ambiguous_object(). This and the change from "The candidates
are:" to "The candidates are:\n%s" helps to give translators more
context.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 27 +++++++++++++++++++++++----
t/t1512-rev-parse-disambiguation.sh | 3 +--
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/object-name.c b/object-name.c
index cbf459f5664..6154e1ec6f8 100644
--- a/object-name.c
+++ b/object-name.c
@@ -351,9 +351,16 @@ static int init_object_disambiguation(struct repository *r,
return 0;
}
+struct ambiguous_output {
+ const struct disambiguate_state *ds;
+ struct strbuf advice;
+};
+
static int show_ambiguous_object(const struct object_id *oid, void *data)
{
- const struct disambiguate_state *ds = data;
+ struct ambiguous_output *state = data;
+ const struct disambiguate_state *ds = state->ds;
+ struct strbuf *advice = &state->advice;
struct strbuf desc = STRBUF_INIT;
int type;
const char *hash;
@@ -452,7 +459,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* you'll probably want to swap the "%s" and leading " " space
* around.
*/
- advise(_(" %s"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), desc.buf);
strbuf_release(&desc);
return 0;
@@ -551,6 +558,10 @@ static enum get_oid_result get_short_oid(struct repository *r,
if (!quietly && (status == SHORT_NAME_AMBIGUOUS)) {
struct oid_array collect = OID_ARRAY_INIT;
+ struct ambiguous_output out = {
+ .ds = &ds,
+ .advice = STRBUF_INIT,
+ };
error(_("short object ID %s is ambiguous"), ds.hex_pfx);
@@ -563,13 +574,21 @@ static enum get_oid_result get_short_oid(struct repository *r,
if (!ds.ambiguous)
ds.fn = NULL;
- advise(_("The candidates are:"));
repo_for_each_abbrev(r, ds.hex_pfx, collect_ambiguous, &collect);
sort_ambiguous_oid_array(r, &collect);
- if (oid_array_for_each(&collect, show_ambiguous_object, &ds))
+ if (oid_array_for_each(&collect, show_ambiguous_object, &out))
BUG("show_ambiguous_object shouldn't return non-zero");
+
+ /*
+ * TRANSLATORS: The argument is the list of ambiguous
+ * objects composed in show_ambiguous_object(). See
+ * its "TRANSLATORS" comments for details.
+ */
+ advise(_("The candidates are:\n%s"), out.advice.buf);
+
oid_array_clear(&collect);
+ strbuf_release(&out.advice);
}
return status;
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 80102cc43a3..98cefe3b703 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -73,7 +73,6 @@ test_expect_success 'ambiguous loose bad object parsed as OBJ_BAD' '
test_cmp_failed_rev_parse blob.bad bad0 <<-\EOF
error: short object ID bad0... is ambiguous
- hint: The candidates are:
fatal: invalid object type
EOF
'
@@ -94,11 +93,11 @@ test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' '
test_cmp_failed_rev_parse blob.corrupt cafe <<-\EOF
error: short object ID cafe... is ambiguous
- hint: The candidates are:
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
error: inflate: data stream error (incorrect header check)
error: unable to unpack cafe... header
+ hint: The candidates are:
hint: cafe... [bad object]
hint: cafe... blob
fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree.
--
2.35.0.890.gd7e422415d9
^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH v8 7/7] object-name: re-use "struct strbuf" in show_ambiguous_object()
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2022-01-27 5:26 ` [PATCH v8 6/7] object-name: iterate ambiguous objects before showing header Ævar Arnfjörð Bjarmason
@ 2022-01-27 5:26 ` Ævar Arnfjörð Bjarmason
2022-01-27 20:14 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Junio C Hamano
7 siblings, 0 replies; 90+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-01-27 5:26 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Bagas Sanjaya, Josh Steadmon,
Ævar Arnfjörð Bjarmason
Reduce the allocations done by show_ambiguous_object() by moving the
"desc" strbuf into the "struct ambiguous_output" introduced in the
preceding commit.
This doesn't matter for optimization purposes, but since we're
accumulating a "struct strbuf advice" anyway let's follow that pattern
and add a "struct strbuf sb", we can then strbuf_reset() it rather
than calling strbuf_release() for each call to
show_ambiguous_object().
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
object-name.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/object-name.c b/object-name.c
index 6154e1ec6f8..61b58a2f292 100644
--- a/object-name.c
+++ b/object-name.c
@@ -354,6 +354,7 @@ static int init_object_disambiguation(struct repository *r,
struct ambiguous_output {
const struct disambiguate_state *ds;
struct strbuf advice;
+ struct strbuf sb;
};
static int show_ambiguous_object(const struct object_id *oid, void *data)
@@ -361,7 +362,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
struct ambiguous_output *state = data;
const struct disambiguate_state *ds = state->ds;
struct strbuf *advice = &state->advice;
- struct strbuf desc = STRBUF_INIT;
+ struct strbuf *sb = &state->sb;
int type;
const char *hash;
@@ -377,7 +378,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* output shown when we cannot look up or parse the
* object in question. E.g. "deadbeef [bad object]".
*/
- strbuf_addf(&desc, _("%s [bad object]"), hash);
+ strbuf_addf(sb, _("%s [bad object]"), hash);
goto out;
}
@@ -402,8 +403,8 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
*
* "deadbeef commit 2021-01-01 - Some Commit Message"
*/
- strbuf_addf(&desc, _("%s commit %s - %s"),
- hash, date.buf, msg.buf);
+ strbuf_addf(sb, _("%s commit %s - %s"), hash, date.buf,
+ msg.buf);
strbuf_release(&date);
strbuf_release(&msg);
@@ -423,7 +424,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* The third argument is the "tag" string
* from object.c.
*/
- strbuf_addf(&desc, _("%s tag %s - %s"), hash,
+ strbuf_addf(sb, _("%s tag %s - %s"), hash,
show_date(tag->date, 0, DATE_MODE(SHORT)),
tag->tag);
} else {
@@ -434,7 +435,7 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
*
* "deadbeef [bad tag, could not parse it]"
*/
- strbuf_addf(&desc, _("%s [bad tag, could not parse it]"),
+ strbuf_addf(sb, _("%s [bad tag, could not parse it]"),
hash);
}
} else if (type == OBJ_TREE) {
@@ -442,13 +443,13 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* TRANSLATORS: This is a line of ambiguous <type>
* object output. E.g. "deadbeef tree".
*/
- strbuf_addf(&desc, _("%s tree"), hash);
+ strbuf_addf(sb, _("%s tree"), hash);
} else if (type == OBJ_BLOB) {
/*
* TRANSLATORS: This is a line of ambiguous <type>
* object output. E.g. "deadbeef blob".
*/
- strbuf_addf(&desc, _("%s blob"), hash);
+ strbuf_addf(sb, _("%s blob"), hash);
}
@@ -459,9 +460,9 @@ static int show_ambiguous_object(const struct object_id *oid, void *data)
* you'll probably want to swap the "%s" and leading " " space
* around.
*/
- strbuf_addf(advice, _(" %s\n"), desc.buf);
+ strbuf_addf(advice, _(" %s\n"), sb->buf);
- strbuf_release(&desc);
+ strbuf_reset(sb);
return 0;
}
@@ -560,6 +561,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
struct oid_array collect = OID_ARRAY_INIT;
struct ambiguous_output out = {
.ds = &ds,
+ .sb = STRBUF_INIT,
.advice = STRBUF_INIT,
};
@@ -589,6 +591,7 @@ static enum get_oid_result get_short_oid(struct repository *r,
oid_array_clear(&collect);
strbuf_release(&out.advice);
+ strbuf_release(&out.sb);
}
return status;
--
2.35.0.890.gd7e422415d9
^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date
2022-01-27 5:26 ` [PATCH v8 0/7] object-name: make ambiguous object output translatable + show tag date Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2022-01-27 5:26 ` [PATCH v8 7/7] object-name: re-use "struct strbuf" in show_ambiguous_object() Ævar Arnfjörð Bjarmason
@ 2022-01-27 20:14 ` Junio C Hamano
7 siblings, 0 replies; 90+ messages in thread
From: Junio C Hamano @ 2022-01-27 20:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jeff King, Bagas Sanjaya, Josh Steadmon
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> The range-diff looks rather scary though because if we're going to add
> such output it made sense to add it in a new 3/7, before we made the
> output translatable, and then to carry that change forward.
I agree with the direction. "translatable plus show date" are two
unrelated things, but it is easier to understand if the feature
enhancement is done first, so that marking and restructuring of the
messages for i18n needs to be done only once.
Will queue. Thanks.
^ permalink raw reply [flat|nested] 90+ messages in thread