| Message ID | 20230303191708.77894-2-toon@iotcl.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cat-file: quote-format name in error when using -z | expand |
Toon Claes <toon@iotcl.com> writes: > There are two callsites that were formatting an error message in batch > mode if an object could not be found. We're about to make changes to > that and to avoid doing that twice, we extract this into a separate > function. > > Signed-off-by: Toon Claes <toon@iotcl.com> > ... > +static void batch_print_error(const char *obj_name, > + enum get_oid_result result, > + struct batch_options* opt) > +{ > + switch (result) { The body of this function is indented way too deep. You should lose one HT at the beginning from all its lines. > + case MISSING_OBJECT: > + printf("%s missing\n", obj_name); > + break; This one expects that obj_name is always usable as a string. > @@ -455,9 +486,7 @@ static void batch_object_write(const char *obj_name, > &data->oid, &data->info, > OBJECT_INFO_LOOKUP_REPLACE); > if (ret < 0) { > - printf("%s missing\n", > - obj_name ? obj_name : oid_to_hex(&data->oid)); > - fflush(stdout); This caller used to be prepared for the case where obj_name is NULL and used the hexadecimal object name stored in data->oid in such a case, but now ... > + batch_print_error(obj_name, MISSING_OBJECT, opt); ... the updated caller assumes that obj_name is always safe to feed to printf("%s") above. Maybe obj_name is always not-NULL when the control reaches this point in which case the new code would be safe, but if that is the case, the proposed log message should explain how that is true to justify this change. As batch_object_cb() makes a call to batch_object_write() with obj_name set to NULL, I do not think this change is defensible, though.
Junio C Hamano <gitster@pobox.com> writes: > As batch_object_cb() makes a call to batch_object_write() with > obj_name set to NULL, I do not think this change is defensible, > though. This indeed seems to break t5313 when queued on top of 'master'; it tries to run "git cat-file --batch-all-objects --batch-check" and hits the exact codepath where a missing object is sent to the error codepath without obj_name set to anything. I guess we now have an existing test that you can mimic that exhibits "missing" error? I do not know offhand if this test already qualifies as the test coverage Phillip wanted to make sure exists. ..... >8 .......... >8 .......... >8 .......... >8 .......... >8 ..... expecting success of 5313.3 'pack/index object count mismatch': do_pack $object && munge $pack 8 "\377\0\0\0" && clear_base && # We enumerate the objects from the completely-fine # .idx, but notice later that the .pack is bogus # and fail to show any data. echo "$object missing" >expect && git cat-file --batch-all-objects --batch-check >actual && test_cmp expect actual && # ...and here fail to load the object (without segfaulting), # but fallback to a good copy if available. test_must_fail git cat-file blob $object && restore_base && git cat-file blob $object >actual && test_cmp file actual && # ...and make sure that index-pack --verify, which has its # own reading routines, does not segfault. test_must_fail git index-pack --verify $pack 4+0 records in 4+0 records out 4 bytes copied, 0.000119221 s, 33.6 kB/s error: packfile .git/objects/pack/pack-67be769e2843d598c78218852612520795998892.pack claims to have 4278190080 objects while index indicates 1 objects error: packfile .git/objects/pack/pack-67be769e2843d598c78218852612520795998892.pack claims to have 4278190080 objects while index indicates 1 objects --- expect 2023-03-03 23:11:37.504011940 +0000 +++ actual 2023-03-03 23:11:37.508012250 +0000 @@ -1 +1 @@ -fff0a2476aa5c8e60a3ef21cfc66e0cc670920be missing +(null) missing not ok 3 - pack/index object count mismatch
diff --git a/builtin/cat-file.c b/builtin/cat-file.c index cc17635e76..0c47190f17 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -429,6 +429,37 @@ static void print_default_format(struct strbuf *scratch, struct expand_data *dat (uintmax_t)data->size); } +static void batch_print_error(const char *obj_name, + enum get_oid_result result, + struct batch_options* opt) +{ + switch (result) { + case MISSING_OBJECT: + printf("%s missing\n", obj_name); + break; + case SHORT_NAME_AMBIGUOUS: + printf("%s ambiguous\n", obj_name); + break; + case DANGLING_SYMLINK: + printf("dangling %"PRIuMAX"\n%s\n", + (uintmax_t)strlen(obj_name), obj_name); + break; + case SYMLINK_LOOP: + printf("loop %"PRIuMAX"\n%s\n", + (uintmax_t)strlen(obj_name), obj_name); + break; + case NOT_DIR: + printf("notdir %"PRIuMAX"\n%s\n", + (uintmax_t)strlen(obj_name), obj_name); + break; + default: + BUG("unknown get_sha1_with_context result %d\n", + result); + break; + } + fflush(stdout); +} + /* * If "pack" is non-NULL, then "offset" is the byte offset within the pack from * which the object may be accessed (though note that we may also rely on @@ -455,9 +486,7 @@ static void batch_object_write(const char *obj_name, &data->oid, &data->info, OBJECT_INFO_LOOKUP_REPLACE); if (ret < 0) { - printf("%s missing\n", - obj_name ? obj_name : oid_to_hex(&data->oid)); - fflush(stdout); + batch_print_error(obj_name, MISSING_OBJECT, opt); return; } @@ -503,31 +532,7 @@ static void batch_one_object(const char *obj_name, result = get_oid_with_context(the_repository, obj_name, flags, &data->oid, &ctx); if (result != FOUND) { - switch (result) { - case MISSING_OBJECT: - printf("%s missing\n", obj_name); - break; - case SHORT_NAME_AMBIGUOUS: - printf("%s ambiguous\n", obj_name); - break; - case DANGLING_SYMLINK: - printf("dangling %"PRIuMAX"\n%s\n", - (uintmax_t)strlen(obj_name), obj_name); - break; - case SYMLINK_LOOP: - printf("loop %"PRIuMAX"\n%s\n", - (uintmax_t)strlen(obj_name), obj_name); - break; - case NOT_DIR: - printf("notdir %"PRIuMAX"\n%s\n", - (uintmax_t)strlen(obj_name), obj_name); - break; - default: - BUG("unknown get_sha1_with_context result %d\n", - result); - break; - } - fflush(stdout); + batch_print_error(obj_name, result, opt); return; }
There are two callsites that were formatting an error message in batch mode if an object could not be found. We're about to make changes to that and to avoid doing that twice, we extract this into a separate function. Signed-off-by: Toon Claes <toon@iotcl.com> --- builtin/cat-file.c | 61 +++++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 28 deletions(-)