Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make trailer_info struct private (plus sequencer cleanup) #1696

Closed

Conversation

listx
Copy link

@listx listx commented Mar 16, 2024

NOTE: This series is based on the la/format-trailer-info topic branch (see its discussion at [1]).

This series is based on the initial series [2], notably the v4 version of patches 17-20 as suggested by Christian [3]. This version addresses the review comments for those patches, namely the splitting up of Patch 19 there into 3 separate patches [4] (as Patches 05-07 here) .

The central idea is to make the trailer_info struct private (that is, move its definition from trailer.h to trailer.c) --- aka the "pimpl" idiom. See the detailed commit message for Patch 07 for the motivation behind the change.

Patch 04 makes sequencer.c a well-behaved trailer API consumer, by making use of the trailer iterator. Patch 03 prepares us for Patch 04. Patch 08 slightly reduces the weight of the API by removing (from the API surface) an unused function.

Notable changes in v4

  • Drop "25%" language in Patch 03
  • Rename some variables
  • Update patch emails to personal (linus@ucla.edu) email

Notable changes in v3

  • (NEW Patch 10) Expand test coverage to check the contents of each iteration (raw, key, val fields), not just the total number of iterations
  • (NEW Patch 09) Add documentation in <trailer.h> for using parse_trailers()
  • (unrelated) I will lose access to my linusa@google.com email address tomorrow (I'm switching jobs!) and so future emails from me will come from linus@ucla.edu [5]. I've added the latter email to the CC list here so things should just work. Cheers

Notable changes in v2

  • Add unit tests at the beginning of the series (Patches 01 and 02) and use it to verify that the other edge cases remain unchanged when we add the "raw" member (Patch 03)

[1] https://lore.kernel.org/git/pull.1694.git.1710485706.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@gmail.com/
[3] https://lore.kernel.org/git/CAP8UFD08F0V13X0+CJ1uhMPzPWVMs2okGVMJch0DkQg5M3BWLA@mail.gmail.com/
[4] https://lore.kernel.org/git/CAP8UFD1twELGKvvesxgCrZrypKZpgSt04ira3mvurG1UbpDfxQ@mail.gmail.com/
[5] https://lore.kernel.org/git/pull.1720.git.1713309711217.gitgitgadget@gmail.com/

Cc: "Christian Couder" chriscool@tuxfamily.org
Cc: "Junio C Hamano" gitster@pobox.com
Cc: "Emily Shaffer" nasamuffin@google.com
cc: "Josh Steadmon" steadmon@google.com
cc: "Randall S. Becker" rsbecker@nexbridge.com
cc: "Christian Couder" christian.couder@gmail.com
cc: "Kristoffer Haugsbakk" code@khaugsbakk.name
cc: "Linus Arver" linus@ucla.edu
cc: Linus Arver linusa@google.com
cc: Phillip Wood phillip.wood123@gmail.com

@listx
Copy link
Author

listx commented Mar 16, 2024

/preview

Copy link

gitgitgadget bot commented Mar 16, 2024

Preview email sent as pull.1696.git.1710568987.gitgitgadget@gmail.com

@listx listx changed the title Make trailer_info struct private, plus other cleanups Make trailer_info struct private (plus sequencer cleanup) Mar 16, 2024
@listx
Copy link
Author

listx commented Mar 16, 2024

/preview

Copy link

gitgitgadget bot commented Mar 16, 2024

Preview email sent as pull.1696.git.1710569540.gitgitgadget@gmail.com

@listx
Copy link
Author

listx commented Mar 16, 2024

/submit

Copy link

gitgitgadget bot commented Mar 16, 2024

Submitted as pull.1696.git.1710570428.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1696/listx/trailer-api-part-3-v1

To fetch this version to local tag pr-1696/listx/trailer-api-part-3-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1696/listx/trailer-api-part-3-v1

Copy link

gitgitgadget bot commented Mar 16, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> NOTE: This series is based on the la/format-trailer-info topic branch (see
> its discussion at [1]).

Folks, a quick review of the base topic is highly appreciated.  Not
having much review to talk about in [1] makes it a bit premature to
build another series on top of it.

Thanks.

Copy link

gitgitgadget bot commented Mar 16, 2024

This branch is now known as la/hide-trailer-info.

Copy link

gitgitgadget bot commented Mar 16, 2024

This patch series was integrated into seen via git@4d8c59c.

@gitgitgadget gitgitgadget bot added the seen label Mar 16, 2024
Copy link

gitgitgadget bot commented Mar 18, 2024

This patch series was integrated into seen via git@a4baca4.

Copy link

gitgitgadget bot commented Mar 19, 2024

There was a status update in the "New Topics" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.
source: <pull.1696.git.1710570428.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented Mar 20, 2024

This patch series was integrated into seen via git@89b91ff.

Copy link

gitgitgadget bot commented Mar 20, 2024

This patch series was integrated into seen via git@089ab3b.

Copy link

gitgitgadget bot commented Mar 21, 2024

This patch series was integrated into seen via git@c7fd30c.

Copy link

gitgitgadget bot commented Mar 22, 2024

This patch series was integrated into seen via git@edf20b9.

Copy link

gitgitgadget bot commented Mar 23, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.
source: <pull.1696.git.1710570428.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented Mar 26, 2024

This patch series was integrated into seen via git@30ff952.

Copy link

gitgitgadget bot commented Mar 26, 2024

This patch series was integrated into seen via git@fe650a4.

Copy link

gitgitgadget bot commented Mar 26, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.
source: <pull.1696.git.1710570428.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented Mar 26, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

> NOTE: This series is based on the la/format-trailer-info topic branch (see
> its discussion at [1]).

This unfortunately depends on another series, which has seen no
reviews after 10 days X-<.  It did not help that this was sent
almost immediately after that unreviewed series that it depends on.

Any takers?  There must be some folks who know the trailer code very
well, no?

Thanks.

Copy link

gitgitgadget bot commented Mar 27, 2024

This patch series was integrated into seen via git@6b3d731.

Copy link

gitgitgadget bot commented Mar 27, 2024

This patch series was integrated into seen via git@779dd5f.

Copy link

gitgitgadget bot commented Mar 27, 2024

This patch series was integrated into seen via git@57178f0.

Copy link

gitgitgadget bot commented Mar 28, 2024

This patch series was integrated into seen via git@b0af3a1.

Copy link

gitgitgadget bot commented Mar 28, 2024

This patch series was integrated into seen via git@468fa97.

Copy link

gitgitgadget bot commented Mar 29, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.

Needs review.
source: <pull.1696.git.1710570428.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented May 4, 2024

User Phillip Wood <phillip.wood123@gmail.com> has been added to the cc: list.

Copy link

gitgitgadget bot commented May 6, 2024

This patch series was integrated into seen via git@f40942e.

Copy link

gitgitgadget bot commented May 6, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.

Will merge to 'next'?
source: <pull.1696.v4.git.1714625667.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented May 7, 2024

This patch series was integrated into seen via git@042b1ba.

Copy link

gitgitgadget bot commented May 7, 2024

This patch series was integrated into seen via git@3b3b799.

Copy link

gitgitgadget bot commented May 8, 2024

This patch series was integrated into seen via git@267510e.

Copy link

gitgitgadget bot commented May 8, 2024

This patch series was integrated into seen via git@7738bed.

Copy link

gitgitgadget bot commented May 9, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.

Waiting for a review response.
cf. <a75133dc-a0bb-4f61-a616-988f2b4d5688@gmail.com>
source: <pull.1696.v4.git.1714625667.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented May 11, 2024

This patch series was integrated into seen via git@d8dcef7.

Copy link

gitgitgadget bot commented May 11, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.

Waiting for a review response.
cf. <a75133dc-a0bb-4f61-a616-988f2b4d5688@gmail.com>
source: <pull.1696.v4.git.1714625667.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented May 13, 2024

This patch series was integrated into seen via git@4828882.

Copy link

gitgitgadget bot commented May 14, 2024

This patch series was integrated into seen via git@f99e399.

Copy link

gitgitgadget bot commented May 14, 2024

This patch series was integrated into seen via git@8ef1776.

Copy link

gitgitgadget bot commented May 14, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.

Waiting for a review response.
cf. <a75133dc-a0bb-4f61-a616-988f2b4d5688@gmail.com>
source: <pull.1696.v4.git.1714625667.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented May 15, 2024

This patch series was integrated into seen via git@a96ece0.

Copy link

gitgitgadget bot commented May 15, 2024

This patch series was integrated into next via git@955ffe4.

Copy link

gitgitgadget bot commented May 16, 2024

This patch series was integrated into seen via git@294050a.

Copy link

gitgitgadget bot commented May 17, 2024

This patch series was integrated into seen via git@9b533b1.

Copy link

gitgitgadget bot commented May 17, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.

Will merge to 'master'.
source: <pull.1696.v4.git.1714625667.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented May 20, 2024

This patch series was integrated into seen via git@8abb6bf.

Copy link

gitgitgadget bot commented May 20, 2024

There was a status update in the "Cooking" section about the branch la/hide-trailer-info on the Git mailing list:

The trailer API has been reshuffled a bit.

Will merge to 'master'.
source: <pull.1696.v4.git.1714625667.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented May 22, 2024

This patch series was integrated into seen via git@c911567.

Copy link

gitgitgadget bot commented May 22, 2024

This patch series was integrated into seen via git@ef26a9f.

Copy link

gitgitgadget bot commented May 23, 2024

This patch series was integrated into seen via git@c51babf.

Copy link

gitgitgadget bot commented May 23, 2024

This patch series was integrated into seen via git@7593d66.

Copy link

gitgitgadget bot commented May 23, 2024

This patch series was integrated into master via git@7593d66.

Copy link

gitgitgadget bot commented May 23, 2024

This patch series was integrated into next via git@7593d66.

@gitgitgadget gitgitgadget bot added the master label May 23, 2024
@gitgitgadget gitgitgadget bot closed this May 23, 2024
Copy link

gitgitgadget bot commented May 23, 2024

Closed via 7593d66.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant