On Tue, Dec 20 2022, Siddharth Asthana wrote:
> 2. Make one call to `oid_object_info_extended()` to get the type of the
> object. Then, if the object type is either of commit or tag, make a
> - call to `read_object_file()` to read the contents of the object.
> + call to `repo_read_object_file()` to read the contents of the object.
>
> I benchmarked the following command with both the above approaches and
> compared against the current implementation where `--use-mailmap`
> @@ Documentation/git-cat-file.txt: OPTIONS
> - also need to specify the path, separated by whitespace. See the
> - section `BATCH OUTPUT` below for details.
> + on stdin. May not be combined with any other options or arguments
> -+ except --textconv, --filters, or --use-mailmap.
> ++ except `--textconv`, `--filters`, or `--use-mailmap`.
> + +
> + * When used with `--textconv` or `--filters`, the input lines
> + must specify the path, separated by whitespace. See the section
> @@ Documentation/git-cat-file.txt: OPTIONS
> - need to specify the path, separated by whitespace. See the
> - section `BATCH OUTPUT` below for details.
> + Print object information for each object provided on stdin. May not be
> -+ combined with any other options or arguments except --textconv, --filters
> -+ or --use-mailmap.
> ++ combined with any other options or arguments except `--textconv`, `--filters`
> ++ or `--use-mailmap`.
> + +
> + * When used with `--textconv` or `--filters`, the input lines must
> + specify the path, separated by whitespace. See the section
> @@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
> + size_t s = data->size;
> + char *buf = NULL;
> +
> -+ buf = read_object_file(&data->oid, &data->type, &data->size);
> ++ buf = repo_read_object_file(the_repository, &data->oid, &data->type,
> ++ &data->size);
> + buf = replace_idents_using_mailmap(buf, &s);
> + data->size = cast_size_t_to_ulong(s);
> +
This series looks good to me at this point, thanks in particular for the
repo_*() change (will make something I plan to submit soon easier).
These[1][2] are nits I came up with while reviewing this, not worth a
re-roll, but tweaked some things I found a bit odd, namely:
* Let's not cast to void **, instead we can just declare a variable for
the 's' case
* If we don't need the "buf" return value we can skip the assignment
(although I guess technically this breaks the encapsulation, so maybe
we shouldn't skip it)
* We can skip the NULL assignment in 2/2, and instead just assign to
the variable as we declare it, and also do the replace/free on one
line (to make it clear that we immediately don't care about it, and
only want the size).
I don't think any of it's worth a re-roll, just notes to show you I've
looked at it carefully.
The only unresolved question I had while reading this is if we're sure
that a repo_read_object_file() following the a successful
oid_object_info_extended() is guaranteed to succeed? If not the code in
2/2 has a segfault we might trigger (as buf will be NULL), but maybe
we're guaranteed to always get the already-retrieved object from the
object cache.
1: fe3cc3715b2 ! 1: 31701b3e55d cat-file: add mailmap support to -s option
@@ Documentation/git-cat-file.txt: OPTIONS
## builtin/cat-file.c ##
@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
+ break;
case 's':
++ {
++ void *obuf = NULL;
++
oi.sizep = &size;
+
+ if (use_mailmap) {
+ oi.typep = &type;
-+ oi.contentp = (void**)&buf;
++ oi.contentp = &obuf;
+ }
+
if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const
+
+ if (use_mailmap && (type == OBJ_COMMIT || type == OBJ_TAG)) {
+ size_t s = size;
-+ buf = replace_idents_using_mailmap(buf, &s);
++
++ replace_idents_using_mailmap(obuf, &s);
+ size = cast_size_t_to_ulong(s);
+ }
++ free(obuf);
+
printf("%"PRIuMAX"\n", (uintmax_t)size);
ret = 0;
goto cleanup;
+-
++ }
+ case 'e':
+ return !has_object_file(&oid);
+
## t/t4203-mailmap.sh ##
@@ t/t4203-mailmap.sh: test_expect_success '--mailmap enables mailmap in cat-file for annotated tag obj
2: d6c621820d2 ! 2: 14d95db69e9 cat-file: add mailmap support to --batch-check option
@@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,
+
+ if (use_mailmap && (data->type == OBJ_COMMIT || data->type == OBJ_TAG)) {
+ size_t s = data->size;
-+ char *buf = NULL;
++ char *buf = repo_read_object_file(the_repository,
++ &data->oid,
++ &data->type,
++ &data->size);
+
-+ buf = repo_read_object_file(the_repository, &data->oid, &data->type,
-+ &data->size);
-+ buf = replace_idents_using_mailmap(buf, &s);
++ free(replace_idents_using_mailmap(buf, &s));
+ data->size = cast_size_t_to_ulong(s);
-+
-+ free(buf);
+ }
}
