| Message ID | patch-v2-5.7-be85a0565ef-20210912T001420Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | strvec: use size_t to store nr and alloc | expand |
On Sat, Sep 11, 2021 at 8:16 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > As in the preceding commit, prepare for the "nr" member of "struct > strvec" changing from an "int" to a "size_t". > > Let's change this code added in f57696802c3 (rebase: really just > passthru the `git am` options, 2018-11-14) so that it won't cause a > bug if the "i" variable here is updated to be a "size_t" instead of an > "int". > > The reason it would be buggy is because this for-loop relied on > "counting down" from nr-1 to 0, we'll then decrement the variable one > last time, so a value of -1 indicates that we've visited all > elements. Since we're looking for a needle in the haystack having > looked at all the hay means that the needle isn't there. s/haystack/&,/ > Let's replace this with simpler code that loops overthe "struct > strvec" and checks the current needle is "-q", if it isn't and we're > doing a merge we die, otherwise we do a "REBASE_APPLY". s/overthe/over the/ Nit: comma-splice[1] at `"-q",`; replace comma with semicolon or period. [1]: https://lore.kernel.org/git/CAPx1GvfFPWvJsj+uJV7RZrv1rgEpio=pk6rKF2UrjHebVY=LPA@mail.gmail.com/ > We've been able to simplify this code since 8295ed690bf (rebase: make > the backend configurable via config setting, 2020-02-15) when we > stopped using this for anything except this one check, so let's do > that and move the check into the loop itself. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
diff --git a/builtin/rebase.c b/builtin/rebase.c index 27669880918..dcd897fda9c 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1750,16 +1750,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.git_am_opts.nr || options.type == REBASE_APPLY) { /* all am options except -q are compatible only with --apply */ - for (i = options.git_am_opts.nr - 1; i >= 0; i--) - if (strcmp(options.git_am_opts.v[i], "-q")) - break; - - if (i >= 0) { - if (is_merge(&options)) - die(_("cannot combine apply options with " - "merge options")); - else + for (i = 0; i < options.git_am_opts.nr; i++) { + if (strcmp(options.git_am_opts.v[i], "-q")) { + if (is_merge(&options)) + die(_("cannot combine apply options with " + "merge options")); options.type = REBASE_APPLY; + break; + } } }
As in the preceding commit, prepare for the "nr" member of "struct strvec" changing from an "int" to a "size_t". Let's change this code added in f57696802c3 (rebase: really just passthru the `git am` options, 2018-11-14) so that it won't cause a bug if the "i" variable here is updated to be a "size_t" instead of an "int". The reason it would be buggy is because this for-loop relied on "counting down" from nr-1 to 0, we'll then decrement the variable one last time, so a value of -1 indicates that we've visited all elements. Since we're looking for a needle in the haystack having looked at all the hay means that the needle isn't there. Let's replace this with simpler code that loops overthe "struct strvec" and checks the current needle is "-q", if it isn't and we're doing a merge we die, otherwise we do a "REBASE_APPLY". We've been able to simplify this code since 8295ed690bf (rebase: make the backend configurable via config setting, 2020-02-15) when we stopped using this for anything except this one check, so let's do that and move the check into the loop itself. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/rebase.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)