| Message ID | 20220926105343.233296-2-siddharthasthana31@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add mailmap mechanism in cat-file options | expand |
On Mon, Sep 26 2022, Siddharth Asthana wrote: > Even though the cat-file command with `-s` option does not complain when > `--use-mailmap` option is given, the latter option is ignored. Compute > the size of the object after replacing the idents and report it instead. > > In order to make `-s` option honour the mailmap mechanism we have to > read the contents of the commit/tag object. Make use of the call to > `oid_object_info_extended()` to get the contents of the object and store > in `buf`. `buf` is later freed in the function. > > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by: John Cai <johncai86@gmail.com> > Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com> > --- > Documentation/git-cat-file.txt | 4 +++- > builtin/cat-file.c | 13 +++++++++++++ > t/t4203-mailmap.sh | 10 ++++++++++ > 3 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt > index ec30b5c574..594b6f2dfd 100644 > --- a/Documentation/git-cat-file.txt > +++ b/Documentation/git-cat-file.txt > @@ -45,7 +45,9 @@ OPTIONS > > -s:: > Instead of the content, show the object size identified by > - `<object>`. > + `<object>`. If used with `--use-mailmap` option, will show the > + size of updated object after replacing idents using the mailmap > + mechanism. > > -e:: > Exit with zero status if `<object>` exists and is a valid > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 989eee0bb4..9942b93867 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -132,8 +132,21 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, > > case 's': > oi.sizep = &size; > + > + if (use_mailmap) { > + oi.typep = &type; > + oi.contentp = (void**)&buf; > + } > + > if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0) > die("git cat-file: could not get object info"); > + > + if (use_mailmap && (type == OBJ_COMMIT || type == OBJ_TAG)) { > + size_t s = size; > + buf = replace_idents_using_mailmap(buf, &s); This is partially commentary on your already-landed series for cat-file --mailmap support. I wondered why we needed this temporary variable, and why we needed the cast_size_t_to_ulong() at all. On "master" we have a size, but e.g. for cat_one_file()'s *current* codpaths we just pass it to write_or_die(). So the net effect is that we refuse to use write_or_die() if the number in size_t doesn't fit an unsigned long, even though we never need an unsigned long in that case. We have *other* things in the object code that need unsigned long, so it probably amounts to no practical limitation, but it's confusing & I think per [1] below we could do away with it. There's also a subtle gotcha on "master", we replace_idents_using_mailmap() with a possibly NULL "contents", which is a misuse of the strbuf API (the "buf" member should never be NULL), but we're about to die anyway... > + size = cast_size_t_to_ulong(s); > + } > + > printf("%"PRIuMAX"\n", (uintmax_t)size); ...but expanding on "master", here we have seemingly the first use of cast_size_t_to_ulong() thaht's actually needed in this file. I.e. we are about to use PRIuMAX. But why not skip the cast(s) and make this more obvious by having the printf() argument be cast_size_t_to_ulong(size)? In your 2/2 you then have another use of cast_size_t_to_ulong() which *is* needed in that case (we're about to stick it in a "unsigned long" member, and the "size_t s" temporary variable is also needed in that case. 1. diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 989eee0bb4c..676c34cba4b 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -178,11 +178,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, if (!buf) die("Cannot read object %s", obj_name); - if (use_mailmap) { - size_t s = size; - buf = replace_idents_using_mailmap(buf, &s); - size = cast_size_t_to_ulong(s); - } + if (use_mailmap) + buf = replace_idents_using_mailmap(buf, &size); /* otherwise just spit out the data */ break; @@ -218,11 +215,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, buf = read_object_with_reference(the_repository, &oid, exp_type_id, &size, NULL); - if (use_mailmap) { - size_t s = size; - buf = replace_idents_using_mailmap(buf, &s); - size = cast_size_t_to_ulong(s); - } + if (use_mailmap) + buf = replace_idents_using_mailmap(buf, &size); break; } default: @@ -391,12 +385,8 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d contents = read_object_file(oid, &type, &size); - if (use_mailmap) { - size_t s = size; - contents = replace_idents_using_mailmap(contents, &s); - size = cast_size_t_to_ulong(s); - } - + if (use_mailmap) + contents = replace_idents_using_mailmap(contents, &size); if (!contents) die("object %s disappeared", oid_to_hex(oid)); if (type != data->type)
On Mon, Sep 26 2022, Siddharth Asthana wrote: > Even though the cat-file command with `-s` option does not complain when > `--use-mailmap` option is given, the latter option is ignored. Compute > the size of the object after replacing the idents and report it instead. > > In order to make `-s` option honour the mailmap mechanism we have to > read the contents of the commit/tag object. Make use of the call to > `oid_object_info_extended()` to get the contents of the object and store > in `buf`. `buf` is later freed in the function. > > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by: John Cai <johncai86@gmail.com> > Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com> > --- > Documentation/git-cat-file.txt | 4 +++- > builtin/cat-file.c | 13 +++++++++++++ > t/t4203-mailmap.sh | 10 ++++++++++ > 3 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt > index ec30b5c574..594b6f2dfd 100644 > --- a/Documentation/git-cat-file.txt > +++ b/Documentation/git-cat-file.txt > @@ -45,7 +45,9 @@ OPTIONS > > -s:: > Instead of the content, show the object size identified by > - `<object>`. > + `<object>`. If used with `--use-mailmap` option, will show the > + size of updated object after replacing idents using the mailmap > + mechanism. > > -e:: > Exit with zero status if `<object>` exists and is a valid > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 989eee0bb4..9942b93867 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -132,8 +132,21 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, > > case 's': > oi.sizep = &size; > + > + if (use_mailmap) { > + oi.typep = &type; > + oi.contentp = (void**)&buf; > + } > + > if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0) > die("git cat-file: could not get object info"); > + > + if (use_mailmap && (type == OBJ_COMMIT || type == OBJ_TAG)) { Just following along here: We want to handle both tag printing and size computations. I.e. we happily search-replace the author in tag objects: $ git -P diff -- .mailmap diff --git a/.mailmap b/.mailmap index 07db36a9bb9..cace49e462b 100644 --- a/.mailmap +++ b/.mailmap @@ -125,7 +125,7 @@ Jonathan del Strother <jon.delStrother@bestbefore.tv> <maillist@steelskies.com> Josh Triplett <josh@joshtriplett.org> <josh@freedesktop.org> Josh Triplett <josh@joshtriplett.org> <josht@us.ibm.com> Julian Phillips <julian@quantumfyre.co.uk> <jp3@quantumfyre.co.uk> -Junio C Hamano <gitster@pobox.com> <gitster@pobox.com> +Foo <bar@baz.blah> Junio C Hamano <gitster@pobox.com> Junio C Hamano <gitster@pobox.com> <junio@hera.kernel.org> Junio C Hamano <gitster@pobox.com> <junio@kernel.org> Junio C Hamano <gitster@pobox.com> <junio@pobox.com> $ ./git cat-file --use-mailmap tag v2.37.0 | head -n 4 object e4a4b31577c7419497ac30cebe30d755b97752c5 type commit tag v2.37.0 tagger Foo <bar@baz.blah> 1656346695 -0700 And we want the "-s" to match, okey, but... (continued below) > + size_t s = size; > + buf = replace_idents_using_mailmap(buf, &s); > + size = cast_size_t_to_ulong(s); > + } > + > printf("%"PRIuMAX"\n", (uintmax_t)size); > ret = 0; > goto cleanup; > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh > index cd1cab3e54..59513e7c57 100755 > --- a/t/t4203-mailmap.sh > +++ b/t/t4203-mailmap.sh > @@ -1022,4 +1022,14 @@ test_expect_success '--mailmap enables mailmap in cat-file for annotated tag obj > test_cmp expect actual > ' > > +test_expect_success 'git cat-file -s returns correct size with --use-mailmap' ' > + test_when_finished "rm .mailmap" && > + cat >.mailmap <<-EOF && nit: use \ before EOF, no variables here. > + C O Mitter <committer@example.com> Orig <orig@example.com> > + EOF > + echo "220" >expect && nit: no need for "" quotes. > + git cat-file --use-mailmap -s HEAD >actual && I'd find this a bit easier to follow if acter setting up .mailmap we did something like (I didn't look up what the actual "234" value is): >actual && git cat-file -s HEAD >actual && git cat-file -s --use-mailmap HEAD >>actual && cat >expect <<-\EOF 234 220 EOF We surely test that somewhere else, but it would be a bit more self-documenting, as the difference in sizes would correspond to the size of the address (or a multiple thereof, if it's used replaced N times). > + test_cmp expect actual > +' ...our test only checks the commit handling. Let's be a bit more defensive here & test both potential paths through that new "if".
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt index ec30b5c574..594b6f2dfd 100644 --- a/Documentation/git-cat-file.txt +++ b/Documentation/git-cat-file.txt @@ -45,7 +45,9 @@ OPTIONS -s:: Instead of the content, show the object size identified by - `<object>`. + `<object>`. If used with `--use-mailmap` option, will show the + size of updated object after replacing idents using the mailmap + mechanism. -e:: Exit with zero status if `<object>` exists and is a valid diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 989eee0bb4..9942b93867 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -132,8 +132,21 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name, case 's': oi.sizep = &size; + + if (use_mailmap) { + oi.typep = &type; + oi.contentp = (void**)&buf; + } + if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0) die("git cat-file: could not get object info"); + + if (use_mailmap && (type == OBJ_COMMIT || type == OBJ_TAG)) { + size_t s = size; + buf = replace_idents_using_mailmap(buf, &s); + size = cast_size_t_to_ulong(s); + } + printf("%"PRIuMAX"\n", (uintmax_t)size); ret = 0; goto cleanup; diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh index cd1cab3e54..59513e7c57 100755 --- a/t/t4203-mailmap.sh +++ b/t/t4203-mailmap.sh @@ -1022,4 +1022,14 @@ test_expect_success '--mailmap enables mailmap in cat-file for annotated tag obj test_cmp expect actual ' +test_expect_success 'git cat-file -s returns correct size with --use-mailmap' ' + test_when_finished "rm .mailmap" && + cat >.mailmap <<-EOF && + C O Mitter <committer@example.com> Orig <orig@example.com> + EOF + echo "220" >expect && + git cat-file --use-mailmap -s HEAD >actual && + test_cmp expect actual +' + test_done
Even though the cat-file command with `-s` option does not complain when `--use-mailmap` option is given, the latter option is ignored. Compute the size of the object after replacing the idents and report it instead. In order to make `-s` option honour the mailmap mechanism we have to read the contents of the commit/tag object. Make use of the call to `oid_object_info_extended()` to get the contents of the object and store in `buf`. `buf` is later freed in the function. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: John Cai <johncai86@gmail.com> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com> --- Documentation/git-cat-file.txt | 4 +++- builtin/cat-file.c | 13 +++++++++++++ t/t4203-mailmap.sh | 10 ++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-)