Re: [PATCH v2 22/24] revision.c: remove implicit dependency on the_index

[Stefan Beller <sbeller@google.com>]

On Mon, Sep 3, 2018 at 11:10 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> "remove" is probably a strong word because the dependency is still
> there, hidden behind the_repository. This patch is almost mechanical,
> all call sites are updated to take the_repository, no exception.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  bisect.c                         |  4 ++--
>  builtin/add.c                    |  4 ++--
>  builtin/am.c                     |  6 +++---
>  builtin/blame.c                  |  2 +-
>  builtin/checkout.c               |  4 ++--
>  builtin/commit.c                 |  2 +-
>  builtin/describe.c               |  4 ++--
>  builtin/diff-files.c             |  2 +-
>  builtin/diff-index.c             |  2 +-
>  builtin/diff-tree.c              |  2 +-
>  builtin/diff.c                   |  2 +-
>  builtin/fast-export.c            |  2 +-
>  builtin/fmt-merge-msg.c          |  2 +-
>  builtin/log.c                    | 16 ++++++++--------
>  builtin/merge.c                  |  4 ++--
>  builtin/pack-objects.c           |  2 +-
>  builtin/prune.c                  |  2 +-
>  builtin/reflog.c                 |  2 +-
>  builtin/rev-list.c               |  2 +-
>  builtin/revert.c                 |  2 +-
>  builtin/shortlog.c               |  2 +-
>  builtin/submodule--helper.c      |  2 +-
>  bundle.c                         |  4 ++--
>  diff-lib.c                       |  4 ++--
>  http-push.c                      |  2 +-
>  merge-recursive.c                |  2 +-
>  pack-bitmap-write.c              |  2 +-
>  ref-filter.c                     |  2 +-
>  remote.c                         |  2 +-
>  revision.c                       | 32 ++++++++++++++++++--------------
>  revision.h                       | 10 +++++++---
>  sequencer.c                      |  8 ++++----
>  shallow.c                        |  2 +-
>  submodule.c                      |  6 +++---
>  t/helper/test-revision-walking.c |  2 +-
>  wt-status.c                      | 10 +++++-----
>  36 files changed, 84 insertions(+), 76 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index e1275ba79e..e19c60829c 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -632,7 +632,7 @@ static void bisect_rev_setup(struct rev_info *revs, const char *prefix,
>         struct argv_array rev_argv = ARGV_ARRAY_INIT;
>         int i;
>
> -       init_revisions(revs, prefix);
> +       init_revisions(revs, the_repository, prefix);

Thanks for this patch!

At first I wondered why we put the repository as the second argument,
but that comes down to personal preference, so I wanted to keep it silent.
(FWIW: Thinking about it, I'd either go with this order or with (repo,
prefix, revs)
as it puts the target struct to initialize either first or last, and
then the repo
struct first in the number of parameters. I think the order is fine
after thinking
about it.)

However we have Documentation/technical/api-revision-walking.txt
which would break with this change:

    `init_revisions`::
        Initialize a rev_info structure with default values. The second
        parameter may be NULL or can be prefix path, [...]

The second parameter changes, so we'd want to update the docs
eventually, too. for now I'd think a cheap s/second/prefix/ would do
in that part of the doc.

That said, we can do that on top depending on quickly this merges down
(it touches a lot of code, so keeping the churn down would be nice).

Thanks,
Stefan