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

Add information about usage with nested subrepos #630

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

abeforgit
Copy link

@abeforgit abeforgit commented Aug 18, 2024

While testing git-subrepo, I found some gotchas with regards to nested subrepos. With a bit of thinking, the way git-subrepo acts is logical, but maybe not immediately intuitive.

This PR adds a section on working with nested subrepos to the readme. I hope it explains the logic in a clear manner.

Please let me know if I made some mistakes, I've only really just begun to try out git-subrepo, the chances I got something wrong are high.

rendered

closes #629

Copy link
Collaborator

@admorgan admorgan left a comment

Choose a reason for hiding this comment

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

I want to complement you on a very nice job, notably improving the documentation for this project. I really appreciate it. I do have some nit picks, but I couldn't have done this well myself.


There is *no* special handling for nested subrepos. When you clone a
subrepo, *all* `git-subrepo` does is download the code and set up the subrepo
file. It couldn't care less about the actual contents of your code.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"It couldn't care less about the actual contents of your code." Feels both a little harsh and a lot personified. Could this be removed and the following sentence reworked that I feel describes what is happening quite well?

|- test-tap-bash

However, it's important to understand how this works. Here's the key idea:
*`git-subrepo` is 100% agnostic about your dependencies!*
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel for some, especially those that aren't English speakers as a primary language may have issues understanding what 100% agnostic means and what is being referred to by dependency. I feel that the subsequent paragraph describes what is happening enough that the paragraph starting on line 620 is superfluous, and could be safely removed.

You're working on app-foo, and make some changes to bazlib. How should you
upstream these changes?

Well, first you must realize that from the perspective of app-foo, you've simply
Copy link
Collaborator

Choose a reason for hiding this comment

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

I try to avoid using phrases such as "first you must realize". Even if it is true the fact that they are reading the documentation means that the documentation will educate them to the level they will realized these facts when they are done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see being addressed in the commits on this PR.

doc/git-subrepo.swim Show resolved Hide resolved

`git subrepo push barlib/bazlib`

but you'll soon find out that doesn't work. Why not? Simple: `app-foo` doesn't
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer to remove the word Simple. If all of this was simple it wouldn't take so many lines of explanation :)

another contributor, aka:

* go to your local copy of `barlib`
* pull down the changes with a simple `git pull`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to remove the word simple. For all we know there will be merge conflicts and the like.

What you should do is treat the changes to `barlib` as if they would come from
another contributor, aka:

* go to your local copy of `barlib`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to propose changing "your local copy" to "a local copy", it is a minor difference, but I read your local copy as implying that you already have one, and "a local copy" as you should acquire one if you don't already have one.

doc/git-subrepo.swim Show resolved Hide resolved
@abeforgit
Copy link
Author

Oh this dropped off my radar, busy week
But thank you for the feedback, some good points, I'll update asap

@abeforgit abeforgit force-pushed the docs/add-info-about-nested-subrepos branch from 3a0c73a to 46dc432 Compare August 31, 2024 16:56
@abeforgit abeforgit requested a review from admorgan August 31, 2024 16:56
@abeforgit
Copy link
Author

I think I (finally 🙈) addressed all your remarks, let me know if you want me to change something about the "final step"

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.

Document intended usage with nested subrepos
2 participants