| Message ID | 20221113212830.92609-1-siddharthasthana31@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add mailmap mechanism in cat-file options | expand |
On Sun, Nov 13, 2022 at 10:28 PM Siddharth Asthana <siddharthasthana31@gmail.com> wrote: > In this patch series we didn't want to change that '%(objectsize)' > always shows the size of the original object even when `--use-mailmap` > is set because first we have the long term plan to unify how the formats > for `git cat-file` and other commands works. And second existing formats > like the "pretty formats" used bt `git log` have different options for s/used bt/used by/ > fields respecting mailmap or not respecting it (%an is for author name > while %aN for author name respecting mailmap). [...] > = Patch Organization > > - The first patch makes `-s` option to return updated size of the > <commit/tag> object, when combined with `--use-mailmap` option, after > replacing the idents using the mailmap mechanism. > - The second patch makes `--batch-check` option to return updated size of > the <commit/tag> object, when combined with `--use-mailmap` option, > after replacing the idents using the mailmap mechanism. > - The third patch improves the documentation of `-s`, `--batch`, > `--batch-check` and `--batch-command` options by adding they can be > combined with `--use-mailmap` options. So the documentation patch is now part of this small series again. Even if this documentation patch is a bug fix, it might be better at this point to squash this patch into the patches 1/3 and 2/3. At least I think that would better follow Junio's last comments about this. If you go this way, you might want to squash the documentation parts about -s from patch 3/3 into patch 1/3 and the rest of patch 3/3 into patch 2/3. > = Changes in v4 > > - Improve the documentation patch to clearly state that the `-s`, > `--batch-check`, `--batch-command` and `--batch` options can be only > be used with `--textconv`, `--filters` or `--use-mailmap`. Here I think you should say that the documentation patch is part of this series again and explain a bit why. Anyway I took a look at the actual patches in this series and they look good to me now. So I would be Ok to merge it either as is or with patch 3/3 squashed into patches 1/3 and 2/3 as discussed above. Thanks!
On Mon, Nov 14, 2022 at 06:48:54PM +0100, Christian Couder wrote: > > = Patch Organization > > > > - The first patch makes `-s` option to return updated size of the > > <commit/tag> object, when combined with `--use-mailmap` option, after > > replacing the idents using the mailmap mechanism. > > - The second patch makes `--batch-check` option to return updated size of > > the <commit/tag> object, when combined with `--use-mailmap` option, > > after replacing the idents using the mailmap mechanism. > > - The third patch improves the documentation of `-s`, `--batch`, > > `--batch-check` and `--batch-command` options by adding they can be > > combined with `--use-mailmap` options. > > So the documentation patch is now part of this small series again. For what it's worth, I'm fine to include the documentation updates in this series. It makes it vaguely easier to queue, but I think if we're close to merging this one down, then there's no reason to separate the two. > Even if this documentation patch is a bug fix, it might be better at > this point to squash this patch into the patches 1/3 and 2/3. At least > I think that would better follow Junio's last comments about this. If > you go this way, you might want to squash the documentation parts > about -s from patch 3/3 into patch 1/3 and the rest of patch 3/3 into > patch 2/3. Thanks. I agree with you that following Junio's advice in this instance is good to do. Let's wait for one hopefully-final reroll and then start merging this down. Thanks, Taylor
On 14/11/22 23:18, Christian Couder wrote: > On Sun, Nov 13, 2022 at 10:28 PM Siddharth Asthana > <siddharthasthana31@gmail.com> wrote: > >> In this patch series we didn't want to change that '%(objectsize)' >> always shows the size of the original object even when `--use-mailmap` >> is set because first we have the long term plan to unify how the formats >> for `git cat-file` and other commands works. And second existing formats >> like the "pretty formats" used bt `git log` have different options for > > s/used bt/used by/ > >> fields respecting mailmap or not respecting it (%an is for author name >> while %aN for author name respecting mailmap). > > [...] > >> = Patch Organization >> >> - The first patch makes `-s` option to return updated size of the >> <commit/tag> object, when combined with `--use-mailmap` option, after >> replacing the idents using the mailmap mechanism. >> - The second patch makes `--batch-check` option to return updated size of >> the <commit/tag> object, when combined with `--use-mailmap` option, >> after replacing the idents using the mailmap mechanism. >> - The third patch improves the documentation of `-s`, `--batch`, >> `--batch-check` and `--batch-command` options by adding they can be >> combined with `--use-mailmap` options. > > So the documentation patch is now part of this small series again. > > Even if this documentation patch is a bug fix, it might be better at > this point to squash this patch into the patches 1/3 and 2/3. At least > I think that would better follow Junio's last comments about this. If > you go this way, you might want to squash the documentation parts > about -s from patch 3/3 into patch 1/3 and the rest of patch 3/3 into > patch 2/3. > >> = Changes in v4 >> >> - Improve the documentation patch to clearly state that the `-s`, >> `--batch-check`, `--batch-command` and `--batch` options can be only >> be used with `--textconv`, `--filters` or `--use-mailmap`. > > Here I think you should say that the documentation patch is part of > this series again and explain a bit why. > > Anyway I took a look at the actual patches in this series and they > look good to me now. So I would be Ok to merge it either as is or with > patch 3/3 squashed into patches 1/3 and 2/3 as discussed above. > > Thanks! Thanks a lot for the review Christian :) I have made the suggested changes in v5.
Thanks a lot Junio, Taylor and Christian for the review :) I have made the suggested changes. = Description At present, `git-cat-file` command with `--batch-check` and `-s` options does not complain when `--use-mailmap` option is given. The latter option is just ignored. Instead, for commit/tag objects, the command should compute the size of the object after replacing the idents and report it. So, this patch series makes `-s` and `--batch-check` options of `git-cat-file` honor mailmap when used with `--use-mailmap` option. In this patch series we didn't want to change that '%(objectsize)' always shows the size of the original object even when `--use-mailmap` is set because first we have the long term plan to unify how the formats for `git cat-file` and other commands works. And second existing formats like the "pretty formats" used bt `git log` have different options for fields respecting mailmap or not respecting it (%an is for author name while %aN for author name respecting mailmap). I would like to thank my mentors, Christian Couder and John Cai, for all of their help! Looking forward to the reviews! = Patch Organization - The first patch makes `-s` option to return updated size of the <commit/tag> object, when combined with `--use-mailmap` option, after replacing the idents using the mailmap mechanism. - The second patch makes `--batch-check` option to return updated size of the <commit/tag> object, when combined with `--use-mailmap` option, after replacing the idents using the mailmap mechanism. - The third patch improves the documentation of `-s`, `--batch`, `--batch-check` and `--batch-command` options by adding they can be combined with `--use-mailmap` options. = Changes in v4 - Improve the documentation patch to clearly state that the `-s`, `--batch-check`, `--batch-command` and `--batch` options can be only be used with `--textconv`, `--filters` or `--use-mailmap`. Siddharth Asthana (3): cat-file: add mailmap support to -s option cat-file: add mailmap support to --batch-check option doc/cat-file: allow --use-mailmap for --batch options Documentation/git-cat-file.txt | 53 ++++++++++++++++++++++-------- builtin/cat-file.c | 27 ++++++++++++++++ t/t4203-mailmap.sh | 59 ++++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 14 deletions(-) Range-diff against v3: 1: 38bb89d350 ! 1: 4ae3af37d2 cat-file: add mailmap support to -s option @@ Commit message Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: John Cai <johncai86@gmail.com> + Helped-by: Taylor Blau <me@ttaylorr.com> + Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com> - ## Documentation/git-cat-file.txt ## -@@ Documentation/git-cat-file.txt: 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 - ## builtin/cat-file.c ## @@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const char *obj_name, 2: 4d49cfde73 ! 2: a692646228 cat-file: add mailmap support to --batch-check option @@ Commit message Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: John Cai <johncai86@gmail.com> + Helped-by: Taylor Blau <me@ttaylorr.com> + Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com> - ## Documentation/git-cat-file.txt ## -@@ Documentation/git-cat-file.txt: OPTIONS - except `--textconv` or `--filters`, in which case the input lines - also need to specify the path, separated by whitespace. See the - section `BATCH OUTPUT` below for details. -+ If used with `--use-mailmap` option, will show the size of -+ updated object after replacing idents using the mailmap mechanism. - - --batch-check:: - --batch-check=<format>:: - ## builtin/cat-file.c ## @@ builtin/cat-file.c: static void batch_object_write(const char *obj_name, if (!data->skip_object_info) { -: ---------- > 3: 41b4650b24 doc/cat-file: allow --use-mailmap for --batch options