doc: note the costs of fetching all pull requests #18382

vasild · 2020-03-19T13:29:03Z

Also mention that it is possible to fetch just one pull request.

doc/productivity.md

maflcko · 2020-03-19T13:54:13Z

I tend to fetch only a single pull by just copying the URL in blue on the top right (e.g. for this pull https://github.com/vasild/bitcoin/tree/note_fetch_all_cost)

Then, paste it into a terminal and replace the tree/ with a space:

$ git fetch https://github.com/vasild/bitcoin/ note_fetch_all_cost

remote: Enumerating objects: 14, done.
remote: Counting objects: 100% (14/14), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 17 (delta 11), reused 13 (delta 11), pack-reused 3
Unpacking objects: 100% (17/17), 10.37 KiB | 212.00 KiB/s, done.
From https://github.com/vasild/bitcoin
 * branch                  note_fetch_all_cost -> FETCH_HEAD

vasild · 2020-03-19T14:28:54Z

Actually, isn't it an overkill to download all pull requests - opened, closed and merged (~1 GB)? Usually one wants to review or test one PR.

@sipa you added this in 31444491f2#diff-e4de6d71249c78e229a61d6a03ac255aR422 almost 4 years ago when the volume of this was smaller. What about dropping this advice from the doc and replace it with instructions on fetching a single PR, as per the comment above and/or github help?

maflcko · 2020-03-19T14:45:03Z

Some people might have a really large disk and review a lot of pull requests or want to archive everything that happens for other reasons, so it might be good to mention both: the "archival solution" and the quick fetch solution.

fjahr

Concept ACK

I think this is good so people don't get surprised by the large amount of data. Agree with @kristapsk that there should not be any specific numbers though.

doc/productivity.md

vasild · 2020-03-20T07:51:35Z

Updated: https://github.com/bitcoin/bitcoin/blob/5ebd8f6b84eeff3bc3da219aa7ce978d861db65d/doc/productivity.md#reference-prs-easily-with-refspecs

theStack

ACK 5ebd8f6
Thanks for adding those valuable informations/instructions 👍

promag · 2020-03-23T00:35:20Z

FWIW I also tend to add a remote for each recurring contributor fork which also allows me to sneak peek what they are doing.

laanwj · 2020-03-26T17:44:51Z

This renders as:

I don't think the "For example:"s should be in the code box.

vasild · 2020-03-26T19:35:22Z

Right, updated. It looks better now.

sipa · 2020-03-26T23:16:49Z

Doing some numbers myself:

Cloning bitcoin/bitcoin downloads 138.8 MiB, resulting in a size on disk of .git of 146.8 MB.
Starting with a fresh clone:

Fetching refs/pull/* downloads an additional 801.37 MiB, resulting in a size on disk of .git of 1405.4 MB.
Fetching refs/pull/*/head instead downloads an additional 797.09 MiB, resulting in a size on disk of .git of 1233.0 MB.

I've generally never had a use for the refs/pull/*/merge branches (I prefer doing a merge with master myself if needed anyway, and that's rare), maybe it's reasonable to only suggest fetching the heads?

vasild · 2020-03-27T08:25:51Z

Those numbers more or less coincide with my observations.
I agree with refs/pull/* vs refs/pull/*/head.
Updated, render preview.

laanwj · 2020-03-27T13:55:04Z

Right, updated. It looks better now.

Thanks, yes, looks much better now.

ACK 61dd7e2

jonatack

As there are many ways to do this, and a wide range of user computing configurations and needs, it never really made sense to me why these precise instructions (and now examples) would be provided. (I use a somewhat different gitconfig for this and know other contributors who do as well). Just a counterpoint.

jonatack · 2020-03-27T22:32:13Z

doc/productivity.md

+for example:
+
+```
+git fetch https://github.com/glowang/bitcoin/ update_blocksonly_docs:docs_pr


Have you checked with this person about using their identity here?

Ping @glowang.

thank you for asking - this is fine with me!

RandyMcMillan · 2020-03-27T22:55:47Z

It may be useful to add a note about pulling from the most recent passing commit hash on Travis-ci.

laanwj · 2020-03-28T11:15:37Z

It may be useful to add a note about pulling from the most recent passing commit hash on Travis-ci.

I don't think github allows fetching by commit hash. You can only fetch labels or branches. It would be awesomely convenient sometimes to retrieve an old version of a PR that has been force-pushed over, to compare, but I've never managed to do this.

promag · 2020-03-28T11:24:00Z

I don't think github allows fetching by commit hash. You can only fetch labels or branches.

FWIW I also tend to add a remote for each recurring contributor fork which also allows me to sneak peek what they are doing.

@laanwj I do git remote update laanwj && git reset --hard laanwj/<sha>

theStack

re-ACK 61dd7e2
Just checked that since my last ACK:

formatting has improved (the for example: string is now outside of the monospaced code block)
example .gitconfig section has been changed to only retreiving the heads

theStack · 2020-03-28T12:47:44Z

As there are many ways to do this, and a wide range of user computing configurations and needs, it never really made sense to me why these precise instructions (and now examples) would be provided. (I use a somewhat different gitconfig for this and know other contributors who do as well). Just a counterpoint.

Personally speaking, the instructions on how to fetch all pull requests were very helpful when I started contributing/reviewing. It would be highly absurd to encourage new contributors to review and test code but then not including any instructions how to do so in terms of git(hub) workflow IMHO. I cannot follow the argumentation "there are many ways to do it, so better not even describe a single one of them".

laanwj · 2020-03-28T12:50:44Z

@laanwj I do git remote update laawnj && git reset --hard laawnj/

You don't need the laanwj/ before the hash. But yes, that will work, but only for commits that are currently (or have been at some point when you've fetched) connected to a tag or branch. If you've missed the point they were part of a branch, you cannot fetch them anymore. You can however view those commits through the web interface.

Personally speaking, the instructions on how to fetch all pull requests were very helpful when I started contributing/reviewing.

Personally I think it's useful to have some tips for things people frequently want to do, but otherwise refer to existing documentation. I also understand @jonatack's point that we don't want to end up maintaining an alternative git documentation here.

jonatack · 2020-03-28T13:07:40Z

It would be highly absurd to encourage new contributors to review and test code but then not including any instructions how to do so in terms of git(hub) workflow IMHO. I cannot follow the argumentation "there are many ways to do it, so better not even describe a single one of them".

I and others do describe these things more or less in detail in various articles to help onboard new people, a process I've been going through as well. Also, there are resources like https://bitcoincore.reviews to help onboard contributors -- and which I continue to learn from -- which everyone can contribute to and participate in.

theStack · 2020-03-29T15:32:17Z

It would be highly absurd to encourage new contributors to review and test code but then not including any instructions how to do so in terms of git(hub) workflow IMHO. I cannot follow the argumentation "there are many ways to do it, so better not even describe a single one of them".

I and others do describe these things more or less in detail in various articles to help onboard new people, a process I've been going through as well. Also, there are resources like https://bitcoincore.reviews to help onboard contributors -- and which I continue to learn from -- which everyone can contribute to and participate in.

Fair enough, there is indeed very good and deep introductory material out there (for me it was this article by Jimmy Song that got me started) and the PR Review Club is an absolutely awesome idea. I'd still without hesitation recommend to every newcomer to carefully read the included docs as early as possible (at the very least developer-notes.md and productivity.md), as common denominator of contributor-agreed best practices and tips, that are less likely to get outdated than articles maintained by single individuals. To sum up my view: as long as we have documents like productivity.md included in the repository, they should also include basics about how to fetch and checkout pull requests.

doc/productivity.md

Also, update the example to skip downloading the merge commits when downloading all PRs.

vasild · 2020-03-31T12:59:22Z

This PR caused more discussions than I anticipated plus some controversy.

The initial reason I opened it is that I expected the upstream-pull remote to download only the currently opened PRs and the download to be less than 100-ish MB, so I wanted to "warn" others that it is more expensive than that, given that git does not display the total download size upfront.

Updated the PR with just this basic info in one sentence (plus download only heads).

Render preview

maflcko · 2020-04-02T13:35:36Z

ACK d695eb4

fanquake

ACK d695eb4

d695eb4 doc: note the costs of fetching all pull requests (Vasil Dimov) Pull request description: Also mention that it is possible to fetch just one pull request. ACKs for top commit: MarcoFalke: ACK d695eb4 fanquake: ACK d695eb4 Tree-SHA512: afe080fd018b2e773fb974956937e819085831bf0c1c5623f7f12c728639906b80666b785234058ee39fd98115a53a2fad431c54ee0840667e60bb317e4a828d

fanquake added the Docs label Mar 19, 2020

kristapsk reviewed Mar 19, 2020

View reviewed changes

doc/productivity.md Outdated Show resolved Hide resolved

fjahr reviewed Mar 19, 2020

View reviewed changes

doc/productivity.md Outdated Show resolved Hide resolved

vasild force-pushed the note_fetch_all_cost branch from 5d7a817 to 5ebd8f6 Compare March 20, 2020 07:47

theStack approved these changes Mar 22, 2020

View reviewed changes

vasild force-pushed the note_fetch_all_cost branch from 5ebd8f6 to abf7ff1 Compare March 26, 2020 19:33

vasild force-pushed the note_fetch_all_cost branch from abf7ff1 to 61dd7e2 Compare March 27, 2020 08:23

laanwj added this to the 0.20.0 milestone Mar 27, 2020

jonatack reviewed Mar 27, 2020

View reviewed changes

theStack approved these changes Mar 28, 2020

View reviewed changes

jonatack reviewed Mar 29, 2020

View reviewed changes

doc/productivity.md Outdated Show resolved Hide resolved

doc: note the costs of fetching all pull requests

d695eb4

Also, update the example to skip downloading the merge commits when downloading all PRs.

vasild force-pushed the note_fetch_all_cost branch from 61dd7e2 to d695eb4 Compare March 31, 2020 12:44

laanwj removed this from the 0.20.0 milestone Apr 2, 2020

fanquake approved these changes Apr 3, 2020

View reviewed changes

fanquake merged commit d478595 into bitcoin:master Apr 3, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: note the costs of fetching all pull requests #18382

doc: note the costs of fetching all pull requests #18382

vasild commented Mar 19, 2020

maflcko commented Mar 19, 2020

vasild commented Mar 19, 2020

maflcko commented Mar 19, 2020

fjahr left a comment

vasild commented Mar 20, 2020

theStack left a comment

promag commented Mar 23, 2020 •

edited

Loading

laanwj commented Mar 26, 2020

vasild commented Mar 26, 2020

sipa commented Mar 26, 2020

vasild commented Mar 27, 2020

laanwj commented Mar 27, 2020 •

edited

Loading

jonatack left a comment

jonatack Mar 27, 2020

sipa Mar 27, 2020

glowang Mar 27, 2020

RandyMcMillan commented Mar 27, 2020

laanwj commented Mar 28, 2020

promag commented Mar 28, 2020 •

edited

Loading

theStack left a comment

theStack commented Mar 28, 2020

laanwj commented Mar 28, 2020

jonatack commented Mar 28, 2020 •

edited

Loading

theStack commented Mar 29, 2020

vasild commented Mar 31, 2020

maflcko commented Apr 2, 2020

fanquake left a comment

doc: note the costs of fetching all pull requests #18382

doc: note the costs of fetching all pull requests #18382

Conversation

vasild commented Mar 19, 2020

maflcko commented Mar 19, 2020

vasild commented Mar 19, 2020

maflcko commented Mar 19, 2020

fjahr left a comment

Choose a reason for hiding this comment

vasild commented Mar 20, 2020

theStack left a comment

Choose a reason for hiding this comment

promag commented Mar 23, 2020 • edited Loading

laanwj commented Mar 26, 2020

vasild commented Mar 26, 2020

sipa commented Mar 26, 2020

vasild commented Mar 27, 2020

laanwj commented Mar 27, 2020 • edited Loading

jonatack left a comment

Choose a reason for hiding this comment

jonatack Mar 27, 2020

Choose a reason for hiding this comment

sipa Mar 27, 2020

Choose a reason for hiding this comment

glowang Mar 27, 2020

Choose a reason for hiding this comment

RandyMcMillan commented Mar 27, 2020

laanwj commented Mar 28, 2020

promag commented Mar 28, 2020 • edited Loading

theStack left a comment

Choose a reason for hiding this comment

theStack commented Mar 28, 2020

laanwj commented Mar 28, 2020

jonatack commented Mar 28, 2020 • edited Loading

theStack commented Mar 29, 2020

vasild commented Mar 31, 2020

maflcko commented Apr 2, 2020

fanquake left a comment

Choose a reason for hiding this comment

promag commented Mar 23, 2020 •

edited

Loading

laanwj commented Mar 27, 2020 •

edited

Loading

promag commented Mar 28, 2020 •

edited

Loading

jonatack commented Mar 28, 2020 •

edited

Loading