Support --pathspec-from-file in rm, stash #530
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SyntevoAlex
commented
Jan 16, 2020
•
SyntevoAlex
commented
SyntevoAlex
force-pushed
the
#0207(git)_pathspec_from_file_3
branch
from
1a73d27
to
5707bf8
Compare
|
/preview |
|
Preview email sent as pull.530.git.1579185524.gitgitgadget@gmail.com |
SyntevoAlex
changed the title
SyntevoAlex
force-pushed
the
#0207(git)_pathspec_from_file_3
branch
from
5707bf8
to
7214102
Compare
|
/submit |
|
Submitted as pull.530.git.1579190965.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -8,16 +8,16 @@ git-rm - Remove files from the working tree and from the index | |||
| SYNOPSIS | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> This patch continues the effort that is already applied to
> `git commit`, `git reset`, `git checkout` etc.
>
> 1) Changed outdated descriptions to mention pathspec instead.
> 2) Added reference to 'linkgit:gitglossary[7]'.
> 3) Removed content that merely repeated gitglossary.
> 4) Merged the remainder of "discussion" into `<patchspec>`.
Thanks. Will queue.
| @@ -8,16 +8,18 @@ git-rm - Remove files from the working tree and from the index | |||
| SYNOPSIS | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> Decisions taken for simplicity:
> 1) It is not allowed to pass pathspec in both args and file.
>
> `if (!argc)` block was adapted to work with --pathspec-from-file. For
> that, I also had to parse pathspec earlier. Now it happens before
> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
> sounds fine to me.
That is not an explanation nor justification.
> In case of empty pathspec, there is now a clear error message instead
> of showing usage.
Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
specified, nothing removed" and it makes perfect sense, but I am not
sure "git rm" that gives the same message is better than the output
by usage_with_options(builtin_rm_usage, builtin_rm_options).
> -'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch] [--quiet] [--] <pathspec>...
> +'git rm' [-f | --force] [-n] [-r] [--cached] [--ignore-unmatch]
> + [--quiet] [--pathspec-from-file=<file> [--pathspec-file-nul]]
> + [--] [<pathspec>...]
OK.
> +--pathspec-from-file=<file>::
> + Pathspec is passed in `<file>` instead of commandline args. If
> + `<file>` is exactly `-` then standard input is used. Pathspec
> + elements are separated by LF or CR/LF. Pathspec elements can be
> + quoted as explained for the configuration variable `core.quotePath`
> + (see linkgit:git-config[1]). See also `--pathspec-file-nul` and
> + global `--literal-pathspecs`.
> +
> +--pathspec-file-nul::
> + Only meaningful with `--pathspec-from-file`. Pathspec elements are
> + separated with NUL character and all other characters are taken
> + literally (including newlines and quotes).
> +
OK.
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 19ce95a901..8e40795751 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -235,7 +235,8 @@ static int check_local_mod(struct object_id *head, int index_only)
> }
>
> static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
> -static int ignore_unmatch = 0;
> +static int ignore_unmatch = 0, pathspec_file_nul = 0;
> +static char *pathspec_from_file = NULL;
We may want to clean these "explicitly initialize to 0/NULL" up at
some point. The clean-up itself would not be in the scope of this
patch, of course, but not making it worse is something this patch
can do to help.
> @@ -259,8 +262,24 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>
> argc = parse_options(argc, argv, prefix, builtin_rm_options,
> builtin_rm_usage, 0);
> - if (!argc)
> - usage_with_options(builtin_rm_usage, builtin_rm_options);
> +
> + parse_pathspec(&pathspec, 0,
> + PATHSPEC_PREFER_CWD,
> + prefix, argv);
> +
> + if (pathspec_from_file) {
> + if (pathspec.nr)
> + die(_("--pathspec-from-file is incompatible with pathspec arguments"));
> +
> + parse_pathspec_file(&pathspec, 0,
> + PATHSPEC_PREFER_CWD,
> + prefix, pathspec_from_file, pathspec_file_nul);
> + } else if (pathspec_file_nul) {
> + die(_("--pathspec-file-nul requires --pathspec-from-file"));
> + }
> +
> + if (!pathspec.nr)
> + die(_("Nothing specified, nothing removed"));
I wonder if doing these in this order instead would make more sense
without making unnecessary behaviour change.
- parse the options (which would make pathspec_f_f available to
us)
- if pathspec_f_f is given, call parse_pathspec_file()
- otherwise complain if pathspec_file_nul is set
- otherwise check argc and give the usage_with_options()
I dunno.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):
Sorry for late reply, I was on vacation. Now I'm back and ready to
continue :)
Thanks for your review!
On 21.01.2020 20:36, Junio C Hamano wrote:
>> Decisions taken for simplicity:
>> 1) It is not allowed to pass pathspec in both args and file.
>>
>> `if (!argc)` block was adapted to work with --pathspec-from-file. For
>> that, I also had to parse pathspec earlier. Now it happens before
>> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
>> sounds fine to me.
>
> That is not an explanation nor justification.
I'm not exactly sure what are you suggesting. My best guess is that you
want to remove "`if (!argc)` block was adapted" paragraph from commit
message? I thought about it and it feels wrong to leave this change
unexplained. Or are you suggesting to reword it? If so, please give a hint.
>> In case of empty pathspec, there is now a clear error message instead
>> of showing usage.
>
> Hmph, "git rm --pathspec-from-file=/dev/null" would say "nothing
> specified, nothing removed" and it makes perfect sense, but I am not
> sure "git rm" that gives the same message is better than the output
> by usage_with_options(builtin_rm_usage, builtin_rm_options).
What feels wrong to me is when I make a mistake and git just slams me
with usage, and then it's up to me to figure what could be wrong. I
myself struggled to find a mistake a couple times (in similar cases, not
in this specific one) and didn't like the experience.
This could be a lot worse when there's no mistake, just the file was
empty - but you already agreed that showing a new error message is
reasonable with '--pathspec-from-file'.
Still, without '--pathspec-from-file', it should still be better to
point to a specific error rather then "here's usage and try to find a
difference". I have reworded the error message in V2 in hopes that it
will be less controversial.
If you still don't like it, I will change it to only show the new error
with '--pathspec-from-file'.
>> +static int ignore_unmatch = 0, pathspec_file_nul = 0;
>> +static char *pathspec_from_file = NULL;
>
> We may want to clean these "explicitly initialize to 0/NULL" up at
> some point. The clean-up itself would not be in the scope of this
> patch, of course, but not making it worse is something this patch
> can do to help.
Changed in V2.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes:
> Sorry for late reply, I was on vacation. Now I'm back and ready to
> continue :)
>
> Thanks for your review!
>
> On 21.01.2020 20:36, Junio C Hamano wrote:
>>> Decisions taken for simplicity:
>>> 1) It is not allowed to pass pathspec in both args and file.
>>>
>>> `if (!argc)` block was adapted to work with --pathspec-from-file. For
>>> that, I also had to parse pathspec earlier. Now it happens before
>>> `read_cache()` / `hold_locked_index()` / `setup_work_tree()`, which
>>> sounds fine to me.
>>
>> That is not an explanation nor justification.
>
> I'm not exactly sure what are you suggesting.
I expected that the proposed log message to explain and justify why
a change (in behaviour, in design, etc.) is made. "There is this
limitation" is not a justification---"because of such and such
reasons, there is this limitation, otherwise such and such bad
things happen" is.
There was a problem hiding this comment.
On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):
On 10.02.2020 19:48, Junio C Hamano wrote:
> I expected that the proposed log message to explain and justify why
> a change (in behaviour, in design, etc.) is made. "There is this
> limitation" is not a justification---"because of such and such
> reasons, there is this limitation, otherwise such and such bad
> things happen" is.
I have rewritten the commit message in V3. Is that better?
| @@ -43,9 +43,6 @@ created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` | |||
| is also possible). Stashes may also be referenced by specifying just the | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2dedc21997..f75b80a720 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -43,9 +43,6 @@ created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}`
> is also possible). Stashes may also be referenced by specifying just the
> stash index (e.g. the integer `n` is equivalent to `stash@{n}`).
>
> -OPTIONS
> --------
> -
> push [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [-m|--message <message>] [--] [<pathspec>...]::
>
> Save your local modifications to a new 'stash entry' and roll them
> @@ -152,40 +149,51 @@ store::
> reflog. This is intended to be useful for scripts. It is
> probably not the command you want to use; see "push" above.
>
> -If the `--all` option is used instead then the
> -ignored files are stashed and cleaned in addition to the untracked files.
> -
> -If the `--include-untracked` option is used, all untracked files are also
> -stashed and then cleaned up with `git clean`, leaving the working directory
> -in a very clean state.
> -
> -If the `--index` option is used, then tries to reinstate not only the working
> -tree's changes, but also the index's ones. However, this can fail, when you
> -have conflicts (which are stored in the index, where you therefore can no
> -longer apply the changes as they were originally).
> -
> -If the `--keep-index` option is used, all changes already added to the
> -index are left intact.
> -
> -With `--patch`, you can interactively select hunks from the diff
> -between HEAD and the working tree to be stashed. The stash entry is
> -constructed such that its index state is the same as the index state
> -of your repository, and its worktree contains only the changes you
> -selected interactively. The selected changes are then rolled back
> -from your worktree. See the ``Interactive Mode'' section of
> -linkgit:git-add[1] to learn how to operate the `--patch` mode.
> +OPTIONS
> +-------
> +-a::
> +--all::
> + All ignored and untracked files are also stashed and then cleaned
> + up with `git clean`.
> +
> +-u::
> +--include-untracked::
> + All untracked files are also stashed and then cleaned up with
> + `git clean`.
> +
> +--index::
> + Tries to reinstate not only the working tree's changes, but also
> + the index's ones. However, this can fail, when you have conflicts
> + (which are stored in the index, where you therefore can no longer
> + apply the changes as they were originally).
> +
> +-k::
> +--keep-index::
> +--no-keep-index::
> + All changes already added to the index are left intact.
> +
> +-p::
> +--patch::
> + Interactively select hunks from the diff between HEAD and the
> + working tree to be stashed. The stash entry is constructed such
> + that its index state is the same as the index state of your
> + repository, and its worktree contains only the changes you selected
> + interactively. The selected changes are then rolled back from your
> + worktree. See the ``Interactive Mode'' section of linkgit:git-add[1]
> + to learn how to operate the `--patch` mode.
I have a mixed feelings about this approach. While I am sympathetic
to the "have a single place to describe all" approach this patch
takes, the approach needs to be executed with care when subcommands
do not share much of the options at all. Those readers who jump to
the "OPTIONS" section and try to ignore anything outside the section
may not easily notice that --keep-index only applies to subcommands
that creates a new stash, and meaningless to subcommands that lets
you inspect existing stashes, or apply one to the working tree (and
optionally to the index), for example. If the orinal documentation
did not use "OPTIONS" as the section header and instead said perhaps
"SUBCOMMANDS", it would have been even better, but otherwise I would
suspect that the original "the options understood by 'push' are all
described under the part that begins with 'push [-p] [-k] ...'
command line" arrangement was much easier to understand when reading
them through for the first time to learn and also to find what the
user is looking for after learning the "concept" (e.g. "with
'stash', there is a way to stash-away the changes made to the
working tree") but before becoming familiar with exact set of
options for each subcommand (e.g. "and there was an option that let
me stash only partial changes piecemeal, but what was it spelled?").
If we were to make the result of "a single place to describe all"
approach anything useful, I think at least
(1) the list itself should make it clear that it does not talk
about options related to listing and showing at all,
before enumerating dashed options.
(2) each item in the enumeration should identify which
subcommand(s) accept(s) it.
So, I dunno.
There was a problem hiding this comment.
On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):
On 21.01.2020 21:21, Junio C Hamano wrote:
> I have a mixed feelings about this approach. While I am sympathetic
> to the "have a single place to describe all" approach this patch
> takes, the approach needs to be executed with care when subcommands
> do not share much of the options at all. Those readers who jump to
> the "OPTIONS" section and try to ignore anything outside the section
> may not easily notice that --keep-index only applies to subcommands
> that creates a new stash, and meaningless to subcommands that lets
> you inspect existing stashes, or apply one to the working tree (and
> optionally to the index), for example. If the orinal documentation
> did not use "OPTIONS" as the section header and instead said perhaps
> "SUBCOMMANDS", it would have been even better, but otherwise I would
> suspect that the original "the options understood by 'push' are all
> described under the part that begins with 'push [-p] [-k] ...'
> command line" arrangement was much easier to understand when reading
> them through for the first time to learn and also to find what the
> user is looking for after learning the "concept" (e.g. "with
> 'stash', there is a way to stash-away the changes made to the
> working tree") but before becoming familiar with exact set of
> options for each subcommand (e.g. "and there was an option that let
> me stash only partial changes piecemeal, but what was it spelled?").
>
> If we were to make the result of "a single place to describe all"
> approach anything useful, I think at least
>
> (1) the list itself should make it clear that it does not talk
> about options related to listing and showing at all,
> before enumerating dashed options.
>
> (2) each item in the enumeration should identify which
> subcommand(s) accept(s) it.
>
> So, I dunno.
I have updated the patch with (2). Sorry, I didn't understand what you
mean in (1).
I have included my reasoning in commit message. If you feel against this
change, I guess I'll just revert it. Afterall, my only goal was to
describe new options. Tried to change things because I didn't like how
this doc goes against the layout I have seen in all previous docs I edited.
| @@ -43,9 +43,6 @@ created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` | |||
| is also possible). Stashes may also be referenced by specifying just the | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> +-q::
> +--quiet::
> + Quiet, suppress feedback messages.
> +
> +\--::
> + Separates pathspec from options for disambiguation purposes.
> +
> <pathspec>...::
> The new stash entry records the modified states only for the files
> that match the pathspec. The index entries and working tree files
OK. Describing these in the documentation is a good thing. How
they should be added depends on in what shape the patch 4/8 should
settle, though.
Thanks.
| @@ -43,9 +43,6 @@ created stash, `stash@{1}` is the one before it, `stash@{2.hours.ago}` | |||
| is also possible). Stashes may also be referenced by specifying just the | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
>
> This patch continues the effort that is already applied to
> `git commit`, `git reset`, `git checkout` etc.
Makes sense.
|
This patch series was integrated into pu via git@4baab6c. |
|
This branch is now known as |
|
This patch series was integrated into pu via git@4736a41. |
|
This patch series was integrated into pu via git@7471511. |
|
This patch series was integrated into pu via git@85243ff. |
|
This patch series was integrated into pu via git@8b3a76e. |
|
This patch series was integrated into pu via git@b91a65c. |
|
This patch series was integrated into pu via git@d4bd845. |
SyntevoAlex
force-pushed
the
#0207(git)_pathspec_from_file_3
branch
from
7214102
to
0c6f28d
Compare
|
/submit |
|
Submitted as pull.530.v2.git.1581345948.gitgitgadget@gmail.com |
| @@ -8,16 +8,18 @@ git-rm - Remove files from the working tree and from the index | |||
| SYNOPSIS | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Alexandr Miloslavskiy via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/t/t3601-rm-pathspec-file.sh b/t/t3601-rm-pathspec-file.sh
> new file mode 100755
> index 0000000000..4542a0f02f
> --- /dev/null
> +++ b/t/t3601-rm-pathspec-file.sh
> @@ -0,0 +1,79 @@
> +#!/bin/sh
> +
> +test_description='rm --pathspec-from-file'
> +
> +. ./test-lib.sh
> +
> +test_tick
> +
> +test_expect_success setup '
> + echo A >fileA.t &&
> + echo B >fileB.t &&
> + echo C >fileC.t &&
> + echo D >fileD.t &&
> + git add fileA.t fileB.t fileC.t fileD.t &&
> + git commit -m "files" &&
> +
Trailing whitespace on this line.
> + git tag checkpoint
> +'
> + ...
> +test_expect_success 'error conditions' '
> + restore_checkpoint &&
> + echo fileA.t >list &&
> +
> + test_must_fail git rm --pathspec-from-file=list -- fileA.t 2>err &&
> + test_i18ngrep -e "--pathspec-from-file is incompatible with pathspec arguments" err &&
> +
> + test_must_fail git rm --pathspec-file-nul 2>err &&
> + test_i18ngrep -e "--pathspec-file-nul requires --pathspec-from-file" err &&
> +
And here too.
> + >empty_list &&
> + test_must_fail git rm --pathspec-from-file=empty_list 2>err &&
> + test_i18ngrep -e "No pathspec was given. Which files should I remove?" err
> +'
> +
> +test_done
There was a problem hiding this comment.
On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):
On 10.02.2020 21:39, Junio C Hamano wrote:
> Trailing whitespace on this line.
Whitespaces fixed in V3; I have also activated pre-commit hook. Sorry!
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com> writes:
> On 10.02.2020 21:39, Junio C Hamano wrote:
>> Trailing whitespace on this line.
>
> Whitespaces fixed in V3; I have also activated pre-commit hook. Sorry!
I may not have time to read it over at least in a few days, but lack
if v3 in the title will make it cumbersome to come back to the
thread later X-<. Thanks for a heads-up, anyway, thoguh.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> I may not have time to read it over at least in a few days, but lack
> if v3 in the title will make it cumbersome to come back ...
Oops, please disregard. I must have been looking at some wrong
thread.
|
This patch series was integrated into pu via git@b56f144. |
|
This patch series was integrated into pu via git@32ea6f7. |
|
This patch series was integrated into pu via git@8f78904. |
Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
This patch continues the effort that is already applied to `git commit`, `git reset`, `git checkout` etc. 1) Added reference to 'linkgit:gitglossary[7]'. 2) Fixed mentions of incorrectly plural "pathspecs". Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Eliminate crude option parsing and rely on real parsing instead, because 1) Crude parsing is crude, for example it's not capable of handling things like `git stash -m Message` 2) Adding options in two places is inconvenient and prone to bugs As a side result, the case of `git stash -m Message` gets fixed. Also give a good error message instead of just throwing usage at user. ---- Some review of what's been happening to this code: Before [1], `git-stash.sh` only verified that all args begin with `-` : # The default command is "push" if nothing but options are given seen_non_option= for opt do case "$opt" in --) break ;; -*) ;; *) seen_non_option=t; break ;; esac done Later, [1] introduced the duplicate code I'm now removing, also making the previous test more strict by white-listing options. ---- [1] Commit 40af146 ("stash: convert `stash--helper.c` into `stash.c`" 2019-02-26) Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Decisions taken for simplicity: 1) For now, `--pathspec-from-file` is declared incompatible with `--patch`, even when <file> is not `-`. Such use case is not really expected. 2) It is not allowed to pass pathspec in both args and file. Signed-off-by: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
SyntevoAlex
force-pushed
the
#0207(git)_pathspec_from_file_3
branch
from
0c6f28d
to
6465a29
Compare
|
/submit |
|
Submitted as pull.530.v3.git.1581960322.gitgitgadget@gmail.com |
| @@ -8,16 +8,18 @@ git-rm - Remove files from the working tree and from the index | |||
| SYNOPSIS | |||
There was a problem hiding this comment.
On the Git mailing list, Alexandr Miloslavskiy wrote (reply to this):
On 17.02.2020 18:59, Junio C Hamano wrote:
> I may not have time to read it over at least in a few days, but lack
> if v3 in the title will make it cumbersome to come back to the
> thread later X-<. Thanks for a heads-up, anyway, thoguh.
Here's the V3 in title :)
|
This patch series was integrated into pu via git@a874b9f. |
|
This patch series was integrated into pu via git@02a038d. |
|
This patch series was integrated into pu via git@355b8da. |
|
This patch series was integrated into pu via git@93e4555. |
|
This patch series was integrated into pu via git@44f0c85. |
|
This patch series was integrated into pu via git@99247a4. |
|
This patch series was integrated into pu via git@40705c0. |
|
This patch series was integrated into pu via git@f1532b6. |
|
This patch series was integrated into pu via git@4ad9f2e. |
|
This patch series was integrated into next via git@33ff7e2. |
|
This patch series was integrated into pu via git@4786a70. |
|
This patch series was integrated into pu via git@796da21. |
|
This patch series was integrated into pu via git@19b38f7. |
|
This patch series was integrated into pu via git@9b7f726. |
|
This patch series was integrated into next via git@9b7f726. |
|
This patch series was integrated into master via git@9b7f726. |
|
Closed via 9b7f726. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.