Skip to content

Update: (root) README #2010

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

Closed
wants to merge 21 commits into from
Closed

Conversation

RafaelJohn9
Copy link
Contributor

Changes

  • Updated README

Context

I’ve updated the README.md to improve the overall clarity and structure. The goal of these changes is to make it easier for new contributors and developers to understand how to set up the project locally, run tests, and begin contributing. I’ve tried to keep the original intent and flow intact, while rewording or reorganizing certain sections for better readability and guidance.

Thank you for your time and for reviewing the changes!

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While a quick glance at the resulting README looks pleasant, I am quite concerned about the form of this PR.

Take for example the commit message:

Update: (root) README

That's it. I would be much more convinced of these changes if the commit message followed the guidance in https://github.blog/2022-06-30-write-better-commits-build-better-projects/ to improve it, in particular with a strong focus on this part:

  What you’re doing Why you’re doing it
High-level (strategic) Intent (what does this accomplish?) Context (why does the code do what it does now?)
Low-level (tactical) Implementation (what did you do to accomplish your goal?) Justification (why is this change being made?)

And then there are many, many, many changes, all squashed into a single commit, making it very, very easy to miss anything undesired.

Simple example: the ```console construct is supposed to show a shell session, including prompts. Yet all of those $ prompts are removed. Hrmpf.

Other example, some of the more in-depth information is simply removed. Maybe an improvable idea?

Instead, a large set of changes should be structured into individual commits, each with a commit message that makes a good case for the introduced changes.

One such commit could be to add a table of contents. That should probably even be one of the first commits of this PR, if not the first.

Then, if you still insist on removing those $ prompts from the shell sessions, that would need to be its own commit (if only to make it easier to drop that part and not having to drop all other changes at the same time).

Don't get me wrong, I like a lot what I am seeing in the result. It's just that the current form of the PR makes it really, really hard to improve it. And improving it still needs, from my perspective at least.


> [!NOTE]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty concerned about removing an important visual marker like this.

I am not saying that you have to reinstate it if the new look gets too broken up by it, but I am saying that simply stating that the site was rewritten is a tad too little to convince me that all of these broad changes that are smooshed into a single commit are actually good.


Supported browsers include `firefox`, `chromium`, `webkit`, `chrome`. You can also simply download all of them using `npx playwright install` but please first note that they all weigh >100MB, so you might want to refrain from doing that. Side note: In GitHub Actions' hosted runners, Chrome comes pre-installed, and you might be able to use your own Chrome installation, too, if you have one.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'm not a fan of dropping this important information on the floor.

@dscho
Copy link
Member

dscho commented May 22, 2025

I'd just like to state that I stopped at only two comments because I anticipate that it will be much easier to review and improve this PR once the changes are broken up into a proper set of commits, not because there is nothing more to comment on.

@RafaelJohn9
Copy link
Contributor Author

Hey @dscho , I understand your concerns, I'll redo this PR in the manner you have suggested.

@dscho
Copy link
Member

dscho commented May 22, 2025

Hey @dscho , I understand your concerns, I'll redo this PR in the manner you have suggested.

@RafaelJohn9 thank you.

BTW if I were you, I'd use the technique described in https://git-scm.com/docs/git-rebase#_splitting_commits to avoid redoing the PR but instead simply splitting all the existing changes into appropriate commits. I find that git checkout -p <original-huge- commit> helps me with such tasks.

@RafaelJohn9
Copy link
Contributor Author

RafaelJohn9 commented May 22, 2025

thanks 👍 ,

everyday is a new learning day 😅

@dscho
Copy link
Member

dscho commented May 22, 2025

everyday is a new learning day 😅

Totally!

Move badges below the title to improve visual hierarchy. Reword the opening
paragraph to make the purpose of the site clearer for first-time visitors.
Replace initial local development note with a comprehensive Table of Contents
Organize sections for easy navigation: Getting Started, Local Development, Testing, Content Updates, Contributing, etc.
Introduce "Cloning the Repository" guide
Recommend using scalar for efficient sparse cloning
Make commands easier to copy and paste without errors
Add "Alternative: Manual Sparse Clone" section for users without scalar

Provide clean, prompt-free shell commands to improve copy-pasting

Enhance  usability of the cloning instructions
Reformat note for better readability and emphasis
Clarify instructions for users with a full clone to focus on subsets
Improve tone and consistency in documentation style
Provide brief explanations for each relevant directory
Improve clarity for developers navigating the project layout
…uctions

Add a new "Local Development" section with subsections for prerequisites and serving the site
Specify required tools: Hugo (extended v0.128.0+), Node.js, and Windows WSL recommendation
Improve instructions for verifying Hugo version and serving the site locally
Clarify note about pre-rendered directories and GitHub workflows
Reword instructions to use Hugo’s built-in server.
Explain the need to disable “ugly URLs” for compatibility with GitHub Pages
Add a clear note defining “ugly URLs” and why serve-public.js emulates GitHub Pages behavior
…atting

Introduce instructions to build the site with search enabled using Hugo and Pagefind
Remove $ prompt symbols from shell commands to improve copy-pasting experience
Improve clarity and usability of local testing documentation
Rephrase to highlight using Pagefind’s built-in server.
Remove $ from shell commands for easier copy-pasting
Add a clear note about slower performance and disabled live reload when using Pagefind
Rewrite introduction to describe Playwright test suite and location clearly
Add “Running Tests” subsection with step-by-step instructions
Remove $ prompt symbols from shell commands to simplify copy-pasting
Improve overall structure and readability of the testing setup documentation
Reorganize the Playwright testing section to improve clarity and usability.
Adds headings, formatting, and notes on browser download sizes.
Also explains how to run tests against local changes and filter tests with regex.
Add clear headings under 'Content Updates' for installing Ruby dependencies
and generating manual pages. Reorganize steps to separate local and remote
build instructions, and format code blocks consistently for better readability.
…teps

Structured manual page instructions into local and GitHub-based workflows.
Added steps for building localized manual pages using git-html-l10n.
Improved formatting and clarity for version-specific builds and prerequisites.
Renamed "Update the ProGit book" section to "ProGit Book" for clarity.
Added step-by-step formatting for prerequisites and script usage.
Clarified how to fetch and update book content from local repositories.
Updated download data instructions for improved readability.
Reorganized steps into clearly numbered list.
Added cloning instructions and clarified directory structure.
Documented alternative GitHub-based update method using API tokens.
Explained how to build the book for all available languages.
Encouraged contributions with a more welcoming tone.
Recommended using scalar and sparse-checkout for efficiency.
Added guidance on making pull requests and opening issues for features.
Added a new section with step-by-step guidance on contributing GUI client entries.
Clarified metadata fields required in the .md file.
Provided image naming and formatting requirements.
Ensured platform and ordering fields are clearly explained.
Promoted "Hugo", "Pagefind", "Lychee", and "Playwright" to top-level headings.
Reorganized and standardized the formatting of resource links.
Improved clarity and readability by aligning structure with other sections.
Promoted license section to a top-level heading.
Consolidated license descriptions for source code and graphical assets.
Enhanced readability and link presentation.
@RafaelJohn9
Copy link
Contributor Author

RafaelJohn9 commented May 29, 2025

Hey @dscho,

what's your opinion on this PR ?

Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While there is a lot of good stuff in this PR, there is at least an equal amount of unfortunate stuff that I have a hard time to bring myself to like. It was quite strenuous to try to make sense of those diffs in the commits, as it seems to me that the overall huge, honking, unreviewable diff was merely split at certain hunks, without regard whether the retained hunks actually reflect what their commit messages tried to convey. For example, removing a hint to run one script with a hint to write another script is wrong, and the underlying problem is that the removed hint should have been in a totally different commit (namely the one that rephrased that part of the README).

In its current form, I am not willing to merge this PR.

Do note, though, that I am not the maintainer of this site. There are other people who could merge this PR if you want to convince them (a strategy I would totally condone).


#### 1. Recommended Approach: Using Scalar

We recommend using [`scalar`](https://git-scm.com/docs/scalar) for an efficient and focused clone. This allows you to work only on the parts of the repository relevant to your interests.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still concerned about the removal of that > [!NOTE] marker. It is a rather important thing that this sentence talks about, which would likely be missed otherwise.

Comment on lines 39 to +42
```console
$ scalar clone https://github.com/git/git-scm.com
$ cd git-scm.com/src
$ git sparse-checkout set layouts content static assets hugo.yml data script
scalar clone https://github.com/git/git-scm.com
cd git-scm.com/src
git sparse-checkout set layouts content static assets hugo.yml data script
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you remove the $, then the console markup is wrong, it should be sh instead.

@@ -53,8 +53,8 @@ git sparse-checkout set layouts content static assets hugo.yml data script
git reset --hard
```

> [!NOTE]
> If you _already_ have a full clone and wish to accelerate development by focusing only on a small subset of the pages, you may want to run the `git sparse-checkout set [...]` command mentioned above.
> **Note:**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please compare the visual of > [!NOTE] vs **Note:**. I find the former vastly better and would highly recommend to keep it.

Comment on lines -67 to -68
- To add new GUIs:
- data/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you may find it clearer, the original intention of this section was to answer the question "Which directories do I need in my sparse checkout?". That's kind of gone, the reader now has to cobble together what they need from that admittedly neat-looking (but less helpful) bullet list.

- To work on the GitHub workflows that perform the automated, scheduled pre-rendering:
- .github/
## Local Development
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit (289edb6) is inconsistent. It removes some description of directories that should have been part of 5481388. This reduces the clarity of this PR and makes it quite hard to review.


Since Playwright uses headless versions of popular web browsers, you most likely need to install at least one of them, e.g. via:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another important piece of information that seems to be culled in the name of clarity. I do disagree that this is a good idea.

Comment on lines -165 to +173
This will populate the manual pages for all Git versions. You can also populate them only for a specific Git version (faster):
## Content Updates

```console
$ version=v2.23.0
$ REBUILD_DOC=$version ruby ./script/update-docs.rb /path/to/git/.git en
```
### Manual Pages

Or you can populate the man pages from GitHub (much slower) like this:
1. Install Ruby prerequisites:

```console
$ export GITHUB_API_TOKEN=github_personal_auth_token
$ REBUILD_DOC=$version ruby ./script/update-docs.rb remote en # specific version
```
```console
bundler install
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that you simply split the diff at the boundaries of the new section? If you carefully look at the removed lines and what was added in their place, the diff is non-sensical. I would have expected original text to be replaced by improved text, but basically talking about the exact same thing. That's distinctly not the case here.

Comment on lines -193 to +200
```console
$ ruby ./script/update-download-data.rb
```
For localized man pages, use a local clone of [git-html-l10n](https://github.com/jnavila/git-html-l10n):

```console
ruby ./script/update-docs.rb /path/to/git-html-l10n/.git l10n
REBUILD_DOC=$version ruby ./script/update-docs.rb /path/to/git-html-l10n/.git l10n
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is update-download-data replaced with update-docs? This diff makes no sense, it is highly unclear what was improved, and in what way, and whether it is an improvement altogether. For a PR that wants to improve the clarity of the document, the PR itself lacks quite a bit of clarity.

Comment on lines -202 to +207
## Update the ProGit book
### Downloads Data

First, you will have to get the necessary prerequisites:
Update the downloads data for the site:

```console
$ bundler install
ruby ./script/update-download-data.rb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff makes no sense.

All graphical assets are licensed under the
[Creative Commons Attribution 3.0 Unported License](https://creativecommons.org/licenses/by/3.0/).
The source code for this site is licensed under the MIT license (see `MIT-LICENSE.txt`).
All graphical assets are licensed under the [Creative Commons Attribution 3.0 Unported License](https://creativecommons.org/licenses/by/3.0/).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing that line break makes the diff less clear.

@RafaelJohn9
Copy link
Contributor Author

Hey @dscho , Thank you for your detailed review ,

I apologize if this PR seems like a mess and does not satisfy your standards. if its okay, I would like to close this for now and create issues while submitting PRs. That would make it easier to review and make our discussion not so over the place and rather be concentrated into specific points.

@dscho
Copy link
Member

dscho commented Jul 2, 2025

if its okay, I would like to close this for now and create issues while submitting PRs.

Totally okay with me. But as I said, I am not the maintainer of this site (even if it seems that the onus of reviewing and merging PRs somehow falls mostly on me, I guess I wasn't up in the tree at the count of three).

@RafaelJohn9
Copy link
Contributor Author

hey @dscho ,

you are who we work with :) 👍

@RafaelJohn9 RafaelJohn9 closed this Jul 2, 2025
@RafaelJohn9 RafaelJohn9 deleted the update/README branch July 2, 2025 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants