| Message ID | pull.545.git.1581535151.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | object.c: localize global the_repository variable into r | expand |
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
On 13/02/20 1:48 am, Taylor Blau wrote: > 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. I am new to the git community and while searching for issues to solve I found the issue linked above. I figured solving it would give me a good experience navigating between functions and exploring the huge repository. Initially I grepped to find all functions using `the_repository` but randomly chose object.c since the refactoring had to start somewhere. Special thanks to Johannes Schindelin for this. >> 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. I probably did not check the preview I sent to myself carefully for this. Will make note of it. >> 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
I have created this commit in response to https://github.com/gitgitgadget/git/issues/379#issue-503866117. 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 ? Signed-off-by: Parth Gala parthpgala@gmail.com [parthpgala@gmail.com] 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