Christian Couder <christian.couder@xxxxxxxxx> writes: > On Wed, Feb 7, 2024 at 9:48 PM Linus Arver <linusa@xxxxxxxxxx> wrote: >> Christian Couder <christian.couder@xxxxxxxxx> writes: > >> > It actually kind of "works" when you pass blobs or trees. It looks >> > like the command just doesn't use them (except for checking that they >> > actually exist) unless options like --objects, --missing=print and >> > perhaps others are used. So yeah, the doc might need an update, but I >> > think it could be a separate issue, as it's not new and doesn't depend >> > on this small series. >> >> SG. But also, if there's a way to put invisible (developer-facing, not >> user facing) comments inside the relevant doc file it might be easy >> enough to add a to this series. > > It might seem easy to add/improve some doc, but there is sometimes > bikeshedding. So I don't think making this series dependent on such a > dco update is a good thing for both that doc update and this series. I > will try to work on such a doc update soon though. Oh, I did not mean to say you should add doc updates in this series at all. I was just asking if you could add a comment with "NEEDSWORK" or similar that only developers will see. Looks like asciidoc does support such comments that are left out of the published text: https://docs.asciidoctor.org/asciidoc/latest/comments/. I just tested with "//" style comments (asciidoc has other styles also, not sure which one is preferred in our codebase) and running "make" to generate the docs confirm that we can indeed put in such developer-facing comments. Back to this topic, you could add something like "// NEEDSWORK: rev-list can take blob and tree objects also as arguments, not just commits". This is why I thought it would be easy. >> > "quarantined objects" refers to the following doc: >> > >> > https://www.git-scm.com/docs/git-receive-pack#_quarantine_environment >> > >> > So to clarify things, the above paragraph looks like the following now: >> > >> > "When such a command is used to find the dependencies of some objects, >> > for example the dependencies of quarantined objects (see the >> > "QUARANTINE ENVIRONMENT" section in the git-receive-pack(1) >> > documentation), it would be better if the command would instead >> > consider such missing objects, especially commits, in the same way as >> > other missing objects." >> >> This reads much better, and is a good motivation to keep in the log >> message. > > Ok, I have kept it in the V2 I just sent: > > https://lore.kernel.org/git/20240208135055.2705260-1-christian.couder@xxxxxxxxx/ > > By the way, sorry for forgetting to use the --in-reply-to=... option > when I sent it. NP, thanks for the heads up. See you in the new thread. >> Aside: it's a bit unfortunate that the meaning of "missing" becomes >> overloaded slightly here because one could say '$tip == ""' is a >> "missing" tip becauuse the name is not provided. Subtle. Plus there's >> some interplay with the "if (get_oid_with_context(...))" case above >> because we will (?) hit that path if we do pass in "" (empty string arg) >> as a tip to rev-list. It might be worth saying in the comments in the >> implementation, something like >> >> The term "missing" here means the case where we could not find the object >> given the object_id. For example, given HEAD~1, its object id (oid) >> could be 82335b23aa7234320d6f8055243c852e4b6a5ca3. If no real object >> with this oid exists, it is considered missing. Providing an empty >> string to rev-list does not touch the "missing tip" codepath. >> >> I wrote the above hastily so it may need further edits to make it >> succinct. But the point is that this definition isn't spelled out in the >> test case, which makes the "" argument for $tip that much more subtle. > > I think this is related to the --missing option in general (which has > been with us for around 6 years already), not specifically to this > series, so I think it can be done separately. Ah, thanks for the added context. I agree. >> >> OK so you are using this empty string to clear the expect.raw file. If >> >> that's true then what stops us from doing this at the start of every >> >> iteration? >> > >> > I am not sure what you mean. Both: >> > >> > git rev-list --objects --no-object-names HEAD ^$obj >expect.raw >> > >> > and: >> > >> > >expect.raw #2 >> > >> > are clearing "expect.raw" as ">expect.raw" is used in both cases. >> > >> > If we did the latter at the start of every iteration, then when we do >> > the former afterwards, we would clear "expect.raw" raw again, while >> > there is no point in clearing it twice. >> >> Yes but doing it that way would separate "set up a clean environment for >> this test case" from "find the oid of the non-missing tip" from each >> other in the same if/else stanza, which I believe helps readability. > > On one hand it can help readability, but on other hand some people > interested in test performance might see it as some waste. So I prefer > not to do it for now. In most situations I would agree with "let's hurt test performance by 1%, in exchange for better readability". I think this is one such situation. And also, it's not even clear if we can measure the performance hit from an extra ">expect.raw" call. But as this is a small nit, I won't pursue this suggestion further. >> Now that I have your attention (was this my plan all along? ;) ), I will >> take this opportunity to ping you directly for a review of my own patch >> series for the trailers subsystem: >> https://lore.kernel.org/git/pull.1632.v4.git.1707196348.gitgitgadget@xxxxxxxxx/ >> which is in its 4th iteration after many helpful comments from Junio. >> Please don't let the patch count (28) raise any alarms --- they used to >> be 10 but I broke them down into smaller steps to ease the review >> process. > > I will try to take a look soon. Thanks for working on improving the > trailers subsystem! Thanks!
