Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
268543013:
Minor fixes (Closed)
|
Created:
6 years, 7 months ago by pgervais Modified:
6 years, 6 months ago Reviewers:
iannucci CC:
chromium-reviews, Dirk Pranke, cmp-cc_chromium.org, iannucci+depot_tools_chromium.org, ilevy-cc_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Visibility:
Public. |
DescriptionMinor fixes
BUG=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=273851
Patch Set 1 #
Total comments: 9
Patch Set 2 : Fixes after review #
Total comments: 1
Patch Set 3 : man/html pages files generated #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... File man/src/depot_tools_tutorial.txt (right): https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:208: the local branch. maybe "... src checkout, including when you switch branches." https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:217: begin a new CL, just checkout the branch you want to track and run: don't need to do the checkout step. new-branch will make a new branch tracking origin/master, which is correct here. https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:225: the default upstream branch (usually `origin/master`). See no, the default upstream IS `origin/master`: https://storage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/g... https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:237: it's tracking), and will post it to the well, it could be tracking a local branch. I would just do: '... upstream branch (in this case `origin/master`)' https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:292: branch you want to track (usually master). Incorrect. You only need to do that with --upstream_current. https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:335: This tool just shows you which branches you have in your repo, and their :)
One question remaining. https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... File man/src/depot_tools_tutorial.txt (right): https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:217: begin a new CL, just checkout the branch you want to track and run: On 2014/05/05 21:48:48, iannucci wrote: > don't need to do the checkout step. new-branch will make a new branch tracking > origin/master, which is correct here. Does it also do the checkout? https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:225: the default upstream branch (usually `origin/master`). See On 2014/05/05 21:48:48, iannucci wrote: > no, the default upstream IS `origin/master`: > https://storage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/g... Alright, I didn't get it. https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... man/src/depot_tools_tutorial.txt:292: branch you want to track (usually master). On 2014/05/05 21:48:48, iannucci wrote: > Incorrect. You only need to do that with --upstream_current. It's the same mistake all the way...
On 2014/05/05 23:14:45, pgervais wrote: > One question remaining. > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > File man/src/depot_tools_tutorial.txt (right): > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > man/src/depot_tools_tutorial.txt:217: begin a new CL, just checkout the branch > you want to track and run: > On 2014/05/05 21:48:48, iannucci wrote: > > don't need to do the checkout step. new-branch will make a new branch tracking > > origin/master, which is correct here. > > Does it also do the checkout? It'll do the checkout of the new branch after it's created > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > man/src/depot_tools_tutorial.txt:225: the default upstream branch (usually > `origin/master`). See > On 2014/05/05 21:48:48, iannucci wrote: > > no, the default upstream IS `origin/master`: > > > https://storage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/g... > > Alright, I didn't get it. > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > man/src/depot_tools_tutorial.txt:292: branch you want to track (usually master). > On 2014/05/05 21:48:48, iannucci wrote: > > Incorrect. You only need to do that with --upstream_current. > > It's the same mistake all the way... I kinda reviewed from the bottom to top for some reason so I duped this comment :p
On 2014/05/06 00:11:17, iannucci wrote: > On 2014/05/05 23:14:45, pgervais wrote: > > One question remaining. > > > > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > > File man/src/depot_tools_tutorial.txt (right): > > > > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > > man/src/depot_tools_tutorial.txt:217: begin a new CL, just checkout the branch > > you want to track and run: > > On 2014/05/05 21:48:48, iannucci wrote: > > > don't need to do the checkout step. new-branch will make a new branch > tracking > > > origin/master, which is correct here. > > > > Does it also do the checkout? > > It'll do the checkout of the new branch after it's created > Fair enough, but where is the new branch pointing? To the same commit as master, or origin/master? > > > > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > > man/src/depot_tools_tutorial.txt:225: the default upstream branch (usually > > `origin/master`). See > > On 2014/05/05 21:48:48, iannucci wrote: > > > no, the default upstream IS `origin/master`: > > > > > > https://storage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/g... > > > > Alright, I didn't get it. > > > > > https://codereview.chromium.org/268543013/diff/1/man/src/depot_tools_tutorial... > > man/src/depot_tools_tutorial.txt:292: branch you want to track (usually > master). > > On 2014/05/05 21:48:48, iannucci wrote: > > > Incorrect. You only need to do that with --upstream_current. > > > > It's the same mistake all the way... > > I kinda reviewed from the bottom to top for some reason so I duped this comment > :p No worries. I duped the fix :-)
... > > > > It'll do the checkout of the new branch after it's created > > > Fair enough, but where is the new branch pointing? To the same commit as master, > or origin/master? > The new branch will point at the default upstream branch (origin/master). After the branch is created, new-branch checks out that branch (so, the same commit as origin/master). The user shouldn't have a master branch at all (well, they could do `git new-branch master` and commit stuff to it, but the story we're going for is that the name of the feature branch holds no special meaning with our tools. Thus 'master' is a feature branch just like 'cool-sprocket' or 'bug_fix' (assuming they're tracking origin/master)).
On 2014/05/06 00:43:13, iannucci wrote: > ... > > > > > > It'll do the checkout of the new branch after it's created > > > > > Fair enough, but where is the new branch pointing? To the same commit as > master, > > or origin/master? > > > > The new branch will point at the default upstream branch (origin/master). After > the branch is created, new-branch checks out that branch (so, the same commit as > origin/master). The user shouldn't have a master branch at all (well, they could > do `git new-branch master` and commit stuff to it, but the story we're going for > is that the name of the feature branch holds no special meaning with our tools. > Thus 'master' is a feature branch just like 'cool-sprocket' or 'bug_fix' > (assuming they're tracking origin/master)). Uploaded a new patchset. PTAL
lgtm, though you will need to regenerate the documentation and re-upload it. The make system for these docs is pretty awful at the moment, but basically: * run man/src/make_docs.sh once without the patch * reset all the changes (they're bogus timestamps, etc.) * apply this patch * run make_docs.sh again, and it should only update the depot_tools_tutorial pages. I'd also check the diff, since I have only run the doc generation stuff on my mac, and I'm not sure if it will change when run on linux. replacement of the doc making tool with something real would be greatly welcome. If not, I could download this, regenerate the docs and then upload a new patchset (I think... maybe rietveld doesn't let us do that any more). https://codereview.chromium.org/268543013/diff/20001/man/src/depot_tools_tuto... File man/src/depot_tools_tutorial.txt (right): https://codereview.chromium.org/268543013/diff/20001/man/src/depot_tools_tuto... man/src/depot_tools_tutorial.txt:236: This will take the diff of your branch against its upstream branch (in that I think this would read more naturally as "in this case"
On 2014/05/08 07:24:57, iannucci wrote: > lgtm, though you will need to regenerate the documentation and re-upload it. > > The make system for these docs is pretty awful at the moment, but basically: > * run man/src/make_docs.sh once without the patch > * reset all the changes (they're bogus timestamps, etc.) > * apply this patch > * run make_docs.sh again, and it should only update the depot_tools_tutorial > pages. > > > I'd also check the diff, since I have only run the doc generation stuff on my > mac, and I'm not sure if it will change when run on linux. > > replacement of the doc making tool with something real would be greatly welcome. > > If not, I could download this, regenerate the docs and then upload a new > patchset (I think... maybe rietveld doesn't let us do that any more). > > https://codereview.chromium.org/268543013/diff/20001/man/src/depot_tools_tuto... > File man/src/depot_tools_tutorial.txt (right): > > https://codereview.chromium.org/268543013/diff/20001/man/src/depot_tools_tuto... > man/src/depot_tools_tutorial.txt:236: This will take the diff of your branch > against its upstream branch (in that > I think this would read more naturally as "in this case" Done
belated lgtm
On 2014/05/30 03:33:46, iannucci wrote: > belated lgtm well, thanks for reminding me of this review. I had forgotten about it completely :-)
The CQ bit was checked by pgervais@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pgervais@chromium.org/268543013/40001
Message was sent while issue was closed.
Change committed as 273851 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
