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

From: Taylor Blau <me@ttaylorr.com>
Date: Feb 12, 2020, 20:18 UTC
Message-ID: <20200212201810.GB4364@syl.local>
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.

Show 6 quoted lines

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

Show 37 quoted lines

> 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