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

2018-09-04 Thread Junio C Hamano
Stefan Beller  writes:

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

FWIW, I would also find it more natural to have the_repository as
the first parameter, for the same reason why the "index" functions
(as opposed to "cache" convenience wrappers) have the_index as the
first one.


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

2018-09-04 Thread Stefan Beller
On Mon, Sep 3, 2018 at 11:10 AM Nguyễn Thái Ngọc Duy  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 
> ---
>  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