| Message ID | 20250318115912.2978992-1-usmanakinyemi202@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | remove unnecessary if statement | expand |
Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > In an earlier patch[1] which has been merged to the master, > We checked `repo` is not NULL before making call to `repo_config()`. > Later, in another patch series[2] which has been merged to next, > `repo_config()` was taught to allow `repo` to be NULL. > > So there is not need for checking if the `repo` is NULL before calling > repo_config() in the earlier patch. OK, that sounds good. Are we confident that our half-hearted choice of "there is no repo, so just do a very-early-config thing" is appropriate for any code paths? At least we should be perfectly happy with that choice applied to all of these code paths touched by this series. > Note, I have already sent the first 8 patches in [2] but, the 9th > patch depends on the first patch of that series. So, is this [v2 0/9] of ua/some-builtins-wo-the-repository? I think that topic has long been merged to 'next', and it is way too late to do a wholesale replacement like this. > [1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/ > [2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/ > > Usman Akinyemi (9): > config: teach repo_config to allow `repo` to be NULL > builtin/verify-tag: stop using `the_repository` > builtin/verify-commit: stop using `the_repository` > builtin/send-pack: stop using `the_repository` > builtin/pack-refs: stop using `the_repository` > builtin/ls-files: stop using `the_repository` > builtin/for-each-ref: stop using `the_repository` > builtin/checkout-index: stop using `the_repository` > builtin/update-server-info: remove unnecessary if statement > > builtin/checkout-index.c | 43 ++++++++++++++++----------------- > builtin/for-each-ref.c | 5 ++-- > builtin/ls-files.c | 32 ++++++++++++------------ > builtin/pack-refs.c | 8 +++--- > builtin/send-pack.c | 7 +++--- > builtin/update-server-info.c | 4 +-- > builtin/verify-commit.c | 13 +++++----- > builtin/verify-tag.c | 7 +++--- > config.c | 4 +++ > config.h | 9 +++++++ > t/t0610-reftable-basics.sh | 7 ++++++ > t/t2006-checkout-index-basic.sh | 7 ++++++ > t/t3004-ls-files-basic.sh | 7 ++++++ > t/t5400-send-pack.sh | 7 ++++++ > t/t6300-for-each-ref.sh | 7 ++++++ > t/t7030-verify-tag.sh | 7 ++++++ > t/t7510-signed-commit.sh | 7 ++++++ > 17 files changed, 118 insertions(+), 63 deletions(-)
On Wed, Mar 19, 2025 at 1:51 AM Junio C Hamano <gitster@pobox.com> wrote: > > Usman Akinyemi <usmanakinyemi202@gmail.com> writes: > > > In an earlier patch[1] which has been merged to the master, > > We checked `repo` is not NULL before making call to `repo_config()`. > > Later, in another patch series[2] which has been merged to next, > > `repo_config()` was taught to allow `repo` to be NULL. > > > > So there is not need for checking if the `repo` is NULL before calling > > repo_config() in the earlier patch. > > OK, that sounds good. > > Are we confident that our half-hearted choice of "there is no repo, > so just do a very-early-config thing" is appropriate for any code > paths? > > At least we should be perfectly happy with that choice applied to > all of these code paths touched by this series. This is fine for the commands which use only RUN_SETUP. I also picked and checked those code paths touched by this series to ensure they do what we intend what we intend them to do. I mean, if there is any better option, that would be cool. > > > Note, I have already sent the first 8 patches in [2] but, the 9th > > patch depends on the first patch of that series. > > So, is this [v2 0/9] of ua/some-builtins-wo-the-repository? > > I think that topic has long been merged to 'next', and it is way too > late to do a wholesale replacement like this. It is not a replacement for that patch series but, the last patch here 9/9 depends on the first patch in that series. That is why I sent everything together. Thank you. > > > [1] https://public-inbox.org/git/20250210181103.3609495-1-usmanakinyemi202@gmail.com/ > > [2] https://public-inbox.org/git/20250307233543.1721552-1-usmanakinyemi202@gmail.com/ > > > > Usman Akinyemi (9): > > config: teach repo_config to allow `repo` to be NULL > > builtin/verify-tag: stop using `the_repository` > > builtin/verify-commit: stop using `the_repository` > > builtin/send-pack: stop using `the_repository` > > builtin/pack-refs: stop using `the_repository` > > builtin/ls-files: stop using `the_repository` > > builtin/for-each-ref: stop using `the_repository` > > builtin/checkout-index: stop using `the_repository` > > builtin/update-server-info: remove unnecessary if statement > > > > builtin/checkout-index.c | 43 ++++++++++++++++----------------- > > builtin/for-each-ref.c | 5 ++-- > > builtin/ls-files.c | 32 ++++++++++++------------ > > builtin/pack-refs.c | 8 +++--- > > builtin/send-pack.c | 7 +++--- > > builtin/update-server-info.c | 4 +-- > > builtin/verify-commit.c | 13 +++++----- > > builtin/verify-tag.c | 7 +++--- > > config.c | 4 +++ > > config.h | 9 +++++++ > > t/t0610-reftable-basics.sh | 7 ++++++ > > t/t2006-checkout-index-basic.sh | 7 ++++++ > > t/t3004-ls-files-basic.sh | 7 ++++++ > > t/t5400-send-pack.sh | 7 ++++++ > > t/t6300-for-each-ref.sh | 7 ++++++ > > t/t7030-verify-tag.sh | 7 ++++++ > > t/t7510-signed-commit.sh | 7 ++++++ > > 17 files changed, 118 insertions(+), 63 deletions(-)