Re: [PATCH 0/5] object.c: localize global the_repository variable into r

From: Taylor Blau <me@ttaylorr.com>
To: Parth Gala via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Parth Gala <parthpgala@gmail.com>
Subject: Re: [PATCH 0/5] object.c: localize global the_repository variable into r
Date: Wed, 12 Feb 2020 12:18:10 -0800	[thread overview]
Message-ID: <20200212201810.GB4364@syl.local> (raw)
In-Reply-To: <pull.545.git.1581535151.gitgitgadget@gmail.com>

Hi Parth,

On Wed, Feb 12, 2020 at 07:19:06PM +0000, Parth Gala via GitGitGadget wrote:
> I have created this commit in response to
> https://github.com/gitgitgadget/git/issues/379#issue-503866117.

Fantastic! Thank you for working on this.

> All the functions in object.c which relied on 'the_repository' have
> been modified.  The functions calling these functions in object.c
> consisted calls to other functions using 'the_repository' as well, and
> although I intended to use 'r' at all those instances, I thought it
> would make more sense when we would deal with their callee functions
> in another similar patch. What do you think ?

That makes sense, and follows the conventions that other similar
refactorings have done in the past.

Any reason why you decided to start with 'object.c'? Not that any such
reason is necessary, I'm just curious about how you came to this
decision.

> Signed-off-by: Parth Gala parthpgala@gmail.com [parthpgala@gmail.com]

Even though you *do* need a 'Signed-off-by' in each of your patches,
adding it in the cover letter is not necessary.

> Parth Gala (5):
>   object.c: get_max_object_index and get_indexed_object accept 'r'
>     parameter
>   object.c: lookup_unknown_object() accept 'r' as parameter
>   object.c: parse_object_or_die() accept 'r' as parameter
>   object.c: clear_object_flags() accept 'r' as parameter
>   object.c: clear_commit_marks_all() accept 'r' as parameter
>
>  builtin/checkout.c               |  3 ++-
>  builtin/fsck.c                   |  8 +++++---
>  builtin/grep.c                   |  6 ++++--
>  builtin/index-pack.c             |  5 +++--
>  builtin/log.c                    |  3 ++-
>  builtin/name-rev.c               |  5 +++--
>  builtin/pack-objects.c           |  3 ++-
>  builtin/prune.c                  |  3 ++-
>  bundle.c                         |  8 +++++---
>  http-push.c                      |  3 ++-
>  object.c                         | 32 ++++++++++++++++----------------
>  object.h                         | 12 ++++++------
>  pack-bitmap.c                    |  5 +++--
>  reachable.c                      |  6 ++++--
>  refs.c                           |  3 ++-
>  revision.c                       |  3 ++-
>  shallow.c                        | 13 ++++++++-----
>  t/helper/test-example-decorate.c |  7 ++++---
>  upload-pack.c                    | 19 ++++++++++++-------
>  walker.c                         |  3 ++-
>  20 files changed, 89 insertions(+), 61 deletions(-)
>
>
> base-commit: 0ad714499976290d9a0229230cbe4efae930b8dc
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-545%2FParthGala2k%2Flocalize-the_repository-variable-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-545/ParthGala2k/localize-the_repository-variable-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/545
> --
> gitgitgadget

Thanks,
Taylor