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

[Christian Couder <christian.couder@xxxxxxxxx>]

On Thu, Feb 10, 2022 at 9:46 AM John Cai via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: John Cai <johncai86@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: John Cai <johncai86@xxxxxxxxx>

> @@ -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);