diff mbox

[00/14] Stop using `the_repository` in some trivial cases

Message ID 20241217-pks-use-the-repository-conversion-v1-0-0dba48bcc239@pks.im (mailing list archive)
State New
Headers show

Commit Message

Patrick Steinhardt Dec. 17, 2024, 6:43 a.m. UTC
Hi,

this small patch series performs some refactorings to stop using
`the_repository` in several subsystems. There wasn't really any
criterium for which subsystems I picked, except that all of them have
been trivial to convert.

In this patch series I'm merely bubbling up `the_repository` one more
layer even though some calling contexts already have a repository
available. For the sake of triviality I decided not to handle these
cases though and instead let a future patch series worry about them.

This series is built on v2.48.0-rc0 with ps/build-sign-compare at
e03d2a9ccb (t/helper: don't depend on implicit wraparound, 2024-12-06)
merged into it. There's a single merge conflict with 'seen' that can be
solved like this:

+++ b/pack-bitmap.c
@@@ -2572,16 -2761,8 +2761,10 @@@ void test_bitmap_walk(struct rev_info *
        if (prepare_revision_walk(revs))
                die(_("revision walk setup failed"));

-       tdata.bitmap_git = bitmap_git;
-       tdata.base = bitmap_new();
-       tdata.commits = ewah_to_bitmap(bitmap_git->commits);
-       tdata.trees = ewah_to_bitmap(bitmap_git->trees);
-       tdata.blobs = ewah_to_bitmap(bitmap_git->blobs);
-       tdata.tags = ewah_to_bitmap(bitmap_git->tags);
+       prepare_bitmap_test_data(&tdata, bitmap_git);
 -      tdata.prg = start_progress("Verifying bitmap entries", result_popcnt);
 +      tdata.prg = start_progress(revs->repo,
 +                                 "Verifying bitmap entries",
 +                                 result_popcnt);
-       tdata.seen = 0;

        traverse_commit_list(revs, &test_show_commit, &test_show_object, &tdata);

Thanks!

Patrick

---
Patrick Steinhardt (14):
      progress: stop using `the_repository`
      pager: stop using `the_repository`
      trace: stop using `the_repository`
      serve: stop using `the_repository`
      send-pack: stop using `the_repository`
      server-info: stop using `the_repository`
      diagnose: stop using `the_repository`
      mailinfo: stop using `the_repository`
      credential: stop using `the_repository`
      resolve-undo: stop using `the_repository`
      tmp-objdir: stop using `the_repository`
      add-interactive: stop using `the_repository`
      graph: stop using `the_repository`
      match-trees: stop using `the_repository`

 add-interactive.c            | 19 ++++++-----
 add-patch.c                  |  2 +-
 builtin/am.c                 |  6 ++--
 builtin/blame.c              |  6 ++--
 builtin/bugreport.c          |  2 +-
 builtin/commit-graph.c       |  1 +
 builtin/credential.c         |  6 ++--
 builtin/diagnose.c           |  4 ++-
 builtin/fsck.c               | 12 ++++---
 builtin/grep.c               |  4 +--
 builtin/help.c               |  4 +--
 builtin/index-pack.c         |  7 ++--
 builtin/log.c                |  7 ++--
 builtin/mailinfo.c           |  2 +-
 builtin/pack-objects.c       | 21 ++++++++----
 builtin/prune.c              |  3 +-
 builtin/receive-pack.c       |  4 +--
 builtin/remote.c             |  3 +-
 builtin/repack.c             |  2 +-
 builtin/rev-list.c           |  3 +-
 builtin/send-pack.c          |  2 +-
 builtin/unpack-objects.c     |  3 +-
 builtin/update-server-info.c |  2 +-
 builtin/upload-pack.c        |  6 ++--
 builtin/var.c                |  2 +-
 bulk-checkin.c               |  2 +-
 commit-graph.c               | 20 ++++++++++--
 credential.c                 | 34 +++++++++----------
 credential.h                 | 11 ++++---
 delta-islands.c              |  3 +-
 diagnose.c                   | 15 +++++----
 diagnose.h                   |  5 ++-
 diff.c                       |  4 +--
 diffcore-rename.c            |  1 +
 entry.c                      |  4 ++-
 git.c                        | 10 +++---
 graph.c                      |  3 +-
 http.c                       | 24 +++++++-------
 imap-send.c                  | 10 +++---
 log-tree.c                   |  2 +-
 mailinfo.c                   |  5 ++-
 mailinfo.h                   |  4 ++-
 match-trees.c                | 50 +++++++++++++++-------------
 midx-write.c                 | 11 +++++--
 midx.c                       | 13 +++++---
 pack-bitmap-write.c          |  6 ++--
 pack-bitmap.c                |  4 ++-
 pager.c                      | 14 ++++----
 pager.h                      |  7 ++--
 preload-index.c              |  4 ++-
 progress.c                   | 34 +++++++++++--------
 progress.h                   | 13 +++++---
 prune-packed.c               |  3 +-
 pseudo-merge.c               |  3 +-
 read-cache.c                 |  7 ++--
 remote-curl.c                |  4 +--
 resolve-undo.c               | 14 ++++----
 resolve-undo.h               |  6 ++--
 send-pack.c                  | 77 +++++++++++++++++++++++---------------------
 send-pack.h                  |  3 +-
 serve.c                      | 36 ++++++++++-----------
 serve.h                      |  6 ++--
 server-info.c                | 40 ++++++++++++-----------
 server-info.h                |  4 ++-
 t/helper/test-progress.c     |  6 +++-
 t/helper/test-serve-v2.c     |  7 ++--
 tmp-objdir.c                 | 15 +++++----
 tmp-objdir.h                 |  5 +--
 trace.c                      |  9 +++---
 trace.h                      |  4 ++-
 transport.c                  |  2 +-
 unpack-trees.c               |  4 ++-
 walker.c                     |  3 +-
 73 files changed, 406 insertions(+), 298 deletions(-)


---
base-commit: 063bcebf0c917140ca0e705cbe0fdea127e90086
change-id: 20241128-pks-use-the-repository-conversion-c346af3388ee
prerequisite-message-id: <20241206-pks-meson-v11-0-525ed4792b88@pks.im>
prerequisite-patch-id: 39559d044bc4db5a61d8d428ba227596b6f9a572
prerequisite-patch-id: 76040d8ec266b78f698065cdc891c7a44a7deb59
prerequisite-patch-id: 6a99a5acb7d2a9d331bd2fa364c5f048c4807630
prerequisite-patch-id: f9bf8d3c7bd9ac8f1beeafc0d7bfa99809624ff5
prerequisite-patch-id: b3b09e0cc0b35176a63e768e3dae4f39ed2bbd96
prerequisite-patch-id: b2f0b893f4093ac7ac4466efe23472c381c207bf
prerequisite-patch-id: 942b24fe8f6b5a1d2fd892e4dd52b83238b2bf6a
prerequisite-patch-id: 95f28ecdf77807e7e6968c0bc75e29ea8fa5d687
prerequisite-patch-id: 58e4f31b532d41d4394654ea1de6a3d2a3c54bff
prerequisite-patch-id: 331073f51ccb93b8b02f14b6e52f8fb70651afc5
prerequisite-patch-id: c357c6e7040737739a8d5d76424348bcc9444a79
prerequisite-patch-id: d643b636c49ef0bca14fd198290ffbda331d2823
prerequisite-patch-id: aeee0e7f421acd0c5e6d8a9cd45eb22b8f52a322
prerequisite-patch-id: 88175b9418f0575a627ed6b592c61406bed0972c
prerequisite-patch-id: 95241bfd7e6798b39187c3dfc03d47ab37ca50c4
prerequisite-patch-id: 73dd2f88f2629f3a54ab01c2882962b9effd6055
prerequisite-patch-id: 291c5ec532ed3738a59ad7001ef3a84e2c43aa78
prerequisite-patch-id: 3b29840c001ec89f137b8e37796d710f60436a6a
prerequisite-patch-id: 78bd944cca8c7404feb1f54d9adfcbfea481fa96
prerequisite-patch-id: 9f34d87c5455cada6a68350daa86cb1069e7accc
prerequisite-patch-id: 798827dcb73a002d4d604472e1b1a3b64f9b159d
prerequisite-patch-id: a110e45c7f97287acbd8c69640d6f03d2e7d9bef
prerequisite-patch-id: 3ce7cd65e984c138658841e3a4802178cc9129b2
prerequisite-patch-id: 9cb10256bf2b131cf434ec078807f27be3a4d6cf
prerequisite-patch-id: a97085715ee32eaaad3bcde53e7d41582424f0f9
prerequisite-patch-id: 7a5e50bc7d49ec891a9679b1ea71575f41483187

Comments

shejialuo Dec. 17, 2024, 12:45 p.m. UTC | #1
On Tue, Dec 17, 2024 at 07:43:47AM +0100, Patrick Steinhardt wrote:
> Hi,
> 
> this small patch series performs some refactorings to stop using
> `the_repository` in several subsystems. There wasn't really any
> criterium for which subsystems I picked, except that all of them have
> been trivial to convert.
> 
> In this patch series I'm merely bubbling up `the_repository` one more
> layer even though some calling contexts already have a repository
> available. For the sake of triviality I decided not to handle these
> cases though and instead let a future patch series worry about them.
> 

Actually, I am excited to see that we remove the global variable
"the_repository" in some subsystems because I have seen every patch with
"<subsystem>: stop using `the_repository`".

By this, we make the problem smaller, which is good. I have read through
all the patches, which looks to me.

Thanks,
Jialuo
Patrick Steinhardt Dec. 27, 2024, 2:26 p.m. UTC | #2
On Tue, Dec 17, 2024 at 08:45:54PM +0800, shejialuo wrote:
> On Tue, Dec 17, 2024 at 07:43:47AM +0100, Patrick Steinhardt wrote:
> > Hi,
> > 
> > this small patch series performs some refactorings to stop using
> > `the_repository` in several subsystems. There wasn't really any
> > criterium for which subsystems I picked, except that all of them have
> > been trivial to convert.
> > 
> > In this patch series I'm merely bubbling up `the_repository` one more
> > layer even though some calling contexts already have a repository
> > available. For the sake of triviality I decided not to handle these
> > cases though and instead let a future patch series worry about them.
> > 
> 
> Actually, I am excited to see that we remove the global variable
> "the_repository" in some subsystems because I have seen every patch with
> "<subsystem>: stop using `the_repository`".
> 
> By this, we make the problem smaller, which is good. I have read through
> all the patches, which looks to me.

Thanks for your review!

Patrick
Karthik Nayak Jan. 7, 2025, 11:41 a.m. UTC | #3
Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this small patch series performs some refactorings to stop using
> `the_repository` in several subsystems. There wasn't really any
> criterium for which subsystems I picked, except that all of them have
> been trivial to convert.
>
> In this patch series I'm merely bubbling up `the_repository` one more
> layer even though some calling contexts already have a repository
> available. For the sake of triviality I decided not to handle these
> cases though and instead let a future patch series worry about them.
>
> This series is built on v2.48.0-rc0 with ps/build-sign-compare at
> e03d2a9ccb (t/helper: don't depend on implicit wraparound, 2024-12-06)
> merged into it. There's a single merge conflict with 'seen' that can be
> solved like this:

I went through half of the patches a week ago, but got back to reading
through the series today.

The approach here is to simply bubble up the usage of `the_repository`
to upper layers and use `the_repository` there. The alternative approach
would be to try and resolve the dependency on the upper layers and not
use `the_repository`. This approach seems much safer. The patches look
good to me.

Thanks!
Toon Claes Jan. 7, 2025, 9:12 p.m. UTC | #4
Karthik Nayak <karthik.188@gmail.com> writes:

> I went through half of the patches a week ago, but got back to reading
> through the series today.

I've also been reading through the whole series today, and the changes
are trivial and look good to me.

> The approach here is to simply bubble up the usage of `the_repository`
> to upper layers and use `the_repository` there. The alternative approach
> would be to try and resolve the dependency on the upper layers and not
> use `the_repository`. This approach seems much safer. The patches look
> good to me.

I took me a while to get into the mindset of taking this approach, but
after chatting with Patrick I've changed my mind and agree with this
approach. The goal of this series is to eliminate the use of
`the_repository` in the mentioned subsystems. Simply bubbling up the use
of that variable to the callers of those subsystems is very trivial and
safe to do.

--
Toon
Junio C Hamano Jan. 7, 2025, 9:15 p.m. UTC | #5
Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> I went through half of the patches a week ago, but got back to reading
>> through the series today.
>
> I've also been reading through the whole series today, and the changes
> are trivial and look good to me.
>
>> The approach here is to simply bubble up the usage of `the_repository`
>> to upper layers and use `the_repository` there. The alternative approach
>> would be to try and resolve the dependency on the upper layers and not
>> use `the_repository`. This approach seems much safer. The patches look
>> good to me.
>
> I took me a while to get into the mindset of taking this approach, but
> after chatting with Patrick I've changed my mind and agree with this
> approach. The goal of this series is to eliminate the use of
> `the_repository` in the mentioned subsystems. Simply bubbling up the use
> of that variable to the callers of those subsystems is very trivial and
> safe to do.

Yup.  That way, the conversion would be bug-to-bug compatible, which
is much better than a rewrite that improves some parts while by
mistake breaks the existing code.

Thanks, all.  Let's mark it for 'next'.
diff mbox

Patch

diff --cc pack-bitmap.c
index 48e3b99efb,7d8f910588..0000000000
--- a/pack-bitmap.c