Re: [PATCH] git: fix paginate handling for commands with DELAY_PAGER_CONFIG

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 21, 2024, 21:50 UTC
Message-ID: <CAN0heSoryo-DO6WXKqqASJ5nDLqb3PNNiZv1TP=Pq11wsiSM6A@mail.gmail.com>
In-Reply-To: <20241120101741.8225-1-dev.mbstr@gmail.com>

Hi Matthew,

On Wed, 20 Nov 2024 at 11:41, Matthew Bystrin <dev.mbstr@gmail.com> wrote:

Show 9 quoted lines

>
> Calling commands using editor in terminal with `--paginate` option will
> break things. For example `git --paginate config --edit`. Add extra
> check to ignore paginate flag in case command have DELAY_PAGER_CONFIG
> set.
>
> Relates: cd878a206e8c (t7006: add tests for how git config paginates)
> Signed-off-by: Matthew Bystrin <dev.mbstr@gmail.com>
> ---

> Some time ago I've sent RFC patch [1], which was not quite ready, and I didn't
> receive any feedback. Now I'm sending a more complete version of it. Fixing
> mentioned behaviour of `note` command can be done in separate patch series.

Thanks for reposting, and welcome to the list.

> +       if (use_pager == 1 && (p->option & DELAY_PAGER_CONFIG))
> +               use_pager = 0;
>         if (run_setup && startup_info->have_repository)

This flag goes back to c409824cc2a ("git.c: let builtins opt for handling `pager.foo` themselves", 2017-08-02). The original observation was that setting `pager.tag` to true was more or less foolish -- it would help `git tag --list`, sure, but would also break `git tag -a` quite badly along the way since the latter wants to open an editor.

That's why c409824cc2a allowed builtins to basically say "delay pager config handling -- I'll respect it for some options/modes, but not for others". Now this proposed patch wants to extend that handling beyond "delay pager *config*" to even go "for some options/modes, I'm going to completely ignore `--paginate`." ("Delay pager *option*"?)

Actually, no, it's not so much ignoring as *forcing*. Since you force it to 0, doesn't that mean that `--paginate` ends up basically being `--no-pager`? So `git --paginate branch` is now `git --no-pager branch`? That doesn't seem right. An optionless `git branch` would have paginated, so adding `--paginate` shouldn't change anything.

But even if we force it to -1 instead (for "maybe"), I'm not sure I understand why such an undoing of user intention is wanted. If I run `git --paginate tag -a ...`, maybe that's just self-inflicted harm, but are we certain that for all the `git foo --bar` which won't respect `pager.foo`, that it's also completely crazy to provide `--paginate`?

Martin