git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Siddharth Asthana <siddharthasthana31@gmail.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, johncai86@gmail.com
Subject: Re: [PATCH] doc/cat-file: allow --use-mailmap for --batch options
Date: Mon, 14 Nov 2022 02:56:52 +0530	[thread overview]
Message-ID: <a446840e-7a6d-984b-ac15-f6359a18595a@gmail.com> (raw)
In-Reply-To: <CAP8UFD2vLnE8Suf259RWQk2fcq+gDFTi+cDr9mrbidY49n3Lzw@mail.gmail.com>



On 31/10/22 16:24, Christian Couder wrote:
> On Sat, Oct 29, 2022 at 11:25 AM Siddharth Asthana
> <siddharthasthana31@gmail.com> wrote:
>>
>> The command git cat-file can now use the mailmap mechanism to replace
>> idents with their canonical versions for commit and tag objects. There
>> are several options like `--batch`, `--batch-check` and
>> `--batch-command` that can be combined with `--use-mailmap`. But, the
>> documentation for `--batch`, `--batch-check` and `--batch-command`
>> doesn't say so. This patch fixes that documentation.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: John Cai <johncai86@gmail.com>
>> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
>> ---
>>
>> This patch was previously sent as the first patch of a 3 patch series for
>> adding the mailmap support in git cat-file options:
>> https://lore.kernel.org/git/20220916205946.178925-2-siddharthasthana31@gmail.com/
> 
> I think Taylor is probably right that there should be a version number
> at the start of the subject line if the patch has already been sent
> many times with the same subject and mostly the same content to the
> list.
> 
>> Changes in v2:
>> - Fixed the names in trailers in the commit message
>> - Updated the documentation to clearly state that the `--batch-check`,
>>    `--batch-command` and `--batch` options can only be used with
>>    `--textconv`, `--filters` or `--use-mailmap`.
>> - Fixed formatting
>>
>> Changes in v3:
>> - Fixed formatting
>>
>> Changes in v4:
>> - Fixed some grammatical errors
> 
> The version number should logically be "v4" according to the above.
> 
>>   Documentation/git-cat-file.txt | 45 ++++++++++++++++++++++++----------
>>   1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
>> index ec30b5c574..db2bba9b65 100644
>> --- a/Documentation/git-cat-file.txt
>> +++ b/Documentation/git-cat-file.txt
>> @@ -89,26 +89,45 @@ OPTIONS
>>   --batch::
>>   --batch=<format>::
>>          Print object information and contents for each object provided
>> -       on stdin.  May not be combined with any other options or arguments
>> -       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.
>> +       on stdin. May not be 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
>> +         `BATCH OUTPUT` below for details.
>> +       +
>> +       * When used with `--use-mailmap`, the info command shows the size
>> +         of the object as if the identities recorded in it were replaced
>> +         by the mailmap mechanism.
> 
> Sorry to realize this only at this point but actually I think the
> `info` command is only available when the `--batch-command` option is
> used, not when the `--batch` option is used.
> 
> As the `--batch` option prints both object information (which includes
> the size) and contents (which includes the idents), I think something
> like the following should be used here:
> 
>         * When used with `--use-mailmap`, for commit and tag objects, the
>            contents part of the output shows the identities replaced using the
>            mailmap mechanism, while the information part of the output shows
>            the size of the object as if it actually recorded the replacement
>            identities.
> 
>>   --batch-check::
>>   --batch-check=<format>::
>> -       Print object information for each object provided on stdin.  May
>> -       not be combined with any other options or arguments 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.
>> +       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.
>> +       +
>> +       * When used with `--textconv` or `--filters`, the input lines must
>> +        specify the path, separated by whitespace. See the section
>> +        `BATCH OUTPUT` below for details.
>> +       +
>> +       * When used with `--use-mailmap`, the info command shows the size
>> +         of the object as if the identities recorded in it were replaced
>> +         by the mailmap mechanism.
> 
> Same remark about the `info` command as above.
> 
> As the `--batch-check` option prints only object information (which
> includes the size), I think something like the following should be
> used here:
> 
>         * When used with `--use-mailmap`, for commit and tag objects, the
>            printed object information shows the size of the object as if the
>            identities recorded in it were replaced by the mailmap mechanism.
> 
>>   --batch-command::
>>   --batch-command=<format>::
>>          Enter a command mode that reads commands and arguments from stdin. May
>> -       only be combined with `--buffer`, `--textconv` or `--filters`. In the
>> -       case of `--textconv` or `--filters`, the input lines also need to specify
>> -       the path, separated by whitespace. See the section `BATCH OUTPUT` below
>> -       for details.
>> +       only be combined with `--buffer`, `--textconv`, `--use-mailmap` or
>> +       `--filters`.
>> +       +
>> +       * When used with `--textconv` or `--filters`, the input lines must
>> +         specify the path, separated by whitespace. See the section
>> +         `BATCH OUTPUT` below for details.
>> +       +
>> +       * When used with `--use-mailmap`, the info command shows the size
>> +         of the object as if the identities recorded in it were replaced
>> +         by the mailmap mechanism.
> 
> s/info command/`info` command/
> 
> And I think it could be interesting to add:
> 
>    - that it happens only for commits and tags, and
>    - that when the `contents` command is used the idents are replaced.
> 
> So maybe something like:
> 
>         * When used with `--use-mailmap`, for commit and tag objects, the
>           `contents` command shows the identities replaced using the
>            mailmap mechanism, while the `info` command shows the size
>            of the object as if it actually recorded the replacement
>            identities.
> 
> Also this will become true only after your other patch series will be
> merged. Which means that you should say somewhere that either
> 
>    - this patch should be merged only after the other patch series is merged, or
>    - it's a bug that the `info` command doesn't show the size of the
> updated object and this is going to be fixed soon by the other patch
> series anyway.
> 
> Thanks for working on finishing this!
> Christian.

Thanks a ton for the review Christian :) Will make the suggested changes 
and send the v4 patch series.

      reply	other threads:[~2022-11-13 21:27 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-23 19:35 [PATCH] doc/cat-file: allow --use-mailmap for --batch options Siddharth Asthana
2022-09-24 14:49 ` Christian Couder
2022-09-26  9:14 ` Siddharth Asthana
2022-09-26 14:11   ` John Cai
2022-10-21 10:32     ` Siddharth Asthana
2022-10-21 10:34 ` Siddharth Asthana
2022-10-21 18:56   ` Junio C Hamano
2022-10-29  9:25 ` Siddharth Asthana
2022-10-29 18:06   ` Taylor Blau
2022-10-30  7:06     ` Christian Couder
2022-10-30 18:07       ` Taylor Blau
2022-10-31 10:54   ` Christian Couder
2022-11-13 21:26     ` Siddharth Asthana [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a446840e-7a6d-984b-ac15-f6359a18595a@gmail.com \
    --to=siddharthasthana31@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johncai86@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).