Re: [PATCH v4 2/3] cat-file: introduce batch_mode enum to replace print_contents

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 10, 2022, 10:10 UTC
Message-ID: <CAP8UFD1axq6x624pLFMZNT5H5nUdwWFZ0Eb8MQy8eYd3+6qthA@mail.gmail.com>
In-Reply-To: <81bc5ae1fc1415b9bd751d93ba1ad305e31af4d6.1644465706.git.gitgitgadget@gmail.com>

On Thu, Feb 10, 2022 at 9:46 AM John Cai via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 6 quoted lines

>
> From: John Cai <johncai86@gmail.com>
>
> The next patch introduces a new --batch-command flag. Including --batch
> and --batch-check, we will have a total of three batch modes. Currently,
> from the batch_options struct's perspective, print_options is the only

Here you talk about "print_options"...

> member used to distinguish between the different modes. This makes the
> code harder to read.
>
> To reduce potential confusion, replace print_contents with an enum to

...but here it's "print_contents".

Also it would perhaps be a bit clearer if you introduced it saying something like "the print_contents flag (or boolean?) is the only member..."

> help readability and clarity.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: John Cai <johncai86@gmail.com>

Show 13 quoted lines

> @@ -635,7 +640,15 @@ static int batch_option_callback(const struct option *opt,
>         }
>
>         bo->enabled = 1;
> -       bo->print_contents = !strcmp(opt->long_name, "batch");
> +
> +       if (!strcmp(opt->long_name, "batch")) {
> +               bo->batch_mode = BATCH_MODE_CONTENTS;
> +       } else if (!strcmp(opt->long_name, "batch-check")) {
> +               bo->batch_mode = BATCH_MODE_INFO;
> +       } else {
> +               BUG("%s given to batch-option-callback", opt->long_name);
> +       }

I think we prefer to remove braces when there is only one instruction. So the above could be just:

       if (!strcmp(opt->long_name, "batch"))
               bo->batch_mode = BATCH_MODE_CONTENTS;
       else if (!strcmp(opt->long_name, "batch-check"))
               bo->batch_mode = BATCH_MODE_INFO;
       else
               BUG("%s given to batch-option-callback", opt->long_name);