[Outreachy] Finish adding a 'os-version' capability to Git protocol v2 #1854
Conversation
The git_user_agent_sanitized() function performs some sanitizing to
avoid special characters being sent over the line and possibly messing
up with the protocol or with the parsing on the other side.
Let's extract this sanitizing into a new redact_non_printables() function,
as we will want to reuse it in a following patch.
For now the new redact_non_printables() function is still static as
it's only needed locally.
While at it, let's also make a few small improvements:
- use 'size_t' for 'i' instead of 'int',
- move the declaration of 'i' inside the 'for ( ... )',
- use strbuf_detach() to explicitly detach the string contained by
the 'buf' strbuf.
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
Some code from "builtin/bugreport.c" uses uname(2) to get system information. Let's refactor this code into a new get_uname_info() function, so that we can reuse it in a following commit. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
As some issues that can happen with a Git client can be operating system specific, it can be useful for a server to know which OS a client is using. In the same way it can be useful for a client to know which OS a server is using. This commit introduces a new feature allowing Git clients and servers to exchange operating system information. The feature is controlled by the new `transfer.advertiseOSVersion` config option. The `transfer.advertiseOSVersion` config option is added for privacy concern issue. It is default to `true` and can be changed to `false`. When enabled, this option makes clients and servers send each other the OS name (e.g., "Linux" or "Windows"). The information is retrieved using the 'sysname' field of the `uname(2)` system call. However, there are differences between `uname(1)` (command-line utility) and `uname(2)` (system call) outputs on Windows. These discrepancies complicate testing on Windows platforms. For example: - `uname(1)` output: MINGW64_NT-10.0-20348.3.4.10-87d57229.x86_64\ .2024-02-14.20:17.UTC.x86_64 - `uname(2)` output: Windows.10.0.20348 To address these inconsistencies, ongoing discussions are being held with my mentor to determine the best approach. Until resolved, the `transfer.advertiseOSVersion` is set to `false` on Windows during testing to avoid confusion. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
|
About "This commit introduces" in the commit message of the third patch, we try to use either a verb in the imperative form like "Introduce ..." or something like "Let's introduce ..." instead. Please take a look at 'Documentation/SubmittingPatches' which contains an "imperative-mood" section. |
|
Also in the same commit message:
|
|
About the following paragraph in the same commit message: "To address these inconsistencies, ongoing discussions are being held with my mentor to determine the best approach. Until resolved, the I don't think talking about discussions between us is a good thing. When you send this patch series, discussions between us should be over and what matters then are the discussions with the mailing list and with Junio. So I think it's better to just say something like: "Until a good way to test the feature on Windows is found, the |
|
Thanks for the review. I made the necessary changes in #1856 |
Please just force-push to the same branch. This whole "versioning in the branch name" business appears quite unlike Git. |
Hi @dscho, there is couples of reason why there is couple of branches. This is a quote from https://git.github.io/Mentoring-Program-Guide/ in the Draft patch series and branches section. "For example, if you are working on git bisect you could make some initial changes on a branch named bisect1, then, after receiving some comments on it, make further changes on bisect2, and so on. This way your mentors and others could check what you did using a diff, or a range-diff, between the branches." Having multiple branches also makes it easier for future interns to quickly review the progress and contributions of previous projects, especially when their role involves continuing or building upon the existing work. That is the initial reason why I was not opening any PR on the main git/git. |
|
Please use tags where tags are due, not branches. (And please only tag revisions that are worth having, as opposed to every iteration including known-incorrect ones.) And I fear that you over-index on the intention to teach future readers by diligently keeping a record of every step and mis-step along the way: In my experience, nobody looks at such records ever again, and I doubt that you will, either. It is vastly better use of your time and focus to aim for recording the relevant parts of the patches' history in their commit messages. For example, if you tried a variation of the patch and found out that it does not work, describe that approach and the reasoning in the commit message. This will make it substantially easier to learn from your iterations than throwing dozens of suffix-versioned branches at an interested future reader. See also https://github.blog/developer-skills/github/write-better-commits-build-better-projects/. Besides, having multiple revisions is pretty useless without the context provided by the conversations that led to the latter patch iterations. Force-pushing to the same PR keeps the context of those conversations, and also records the history of the iterations (and in a much less error-prone way than "suffix-versioned branches"). Using branch name suffixes for versioning, and opening new PRs left and right (so that inevitably, you start commenting on an outdated PR because you successfully confused yourself), does a much worse job at keeping the context. Sure, it's all there, just scattered and much less useful than if you used best practice (which is: keep iterating within the same PR, keep all conversations there, too, and coalesce important learnings in the final commit messages). tl;dr if there is an easy, paved path that also delivers superior results, why not go ahead and use it? |
Changes from previous version(transfer.advertise_5).