Issue 10875056: Properly handle Git URLs with trailing slashes.

Keyboard Shortcuts

	File
u :	up to issue
j / k :	jump to file after / before current file
J / K :	jump to next file with a comment after / before current file
	Side-by-side diff
i :	toggle intra-line diffs
e :	expand all comments
c :	collapse all comments
s :	toggle showing all comments
n / p :	next / previous diff chunk or comment
N / P :	next / previous comment
<Up> / <Down> :	next / previous line

	Issue
u :	up to list of issues
j / k :	jump to patch after / before current patch
o / <Enter> :	open current patch in side-by-side view
i :	open current patch in unified diff view

	Issue List
j / k :	jump to issue after / before current issue
o / <Enter> :	open current issue

Issue 10875056: Properly handle Git URLs with trailing slashes. (Closed)

Created:
8 years, 4 months ago by nweiz

Modified:
8 years, 4 months ago

Reviewers:
Bob Nystrom

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Properly handle Git URLs with trailing slashes. BUG=3493 Committed: https://code.google.com/p/dart/source/detail?r=11343

Patch Set 1 #

Total comments: 2

Created: 8 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+67 lines, -2 lines)			Patch
M	utils/pub/git_source.dart	View	1 chunk	+4 lines, -2 lines	0 comments	Download
M	utils/pub/pubspec.dart	View	1 chunk	+7 lines, -0 lines	0 comments	Download
M	utils/tests/pub/pub_install_git_test.dart	View	2 chunks	+56 lines, -0 lines	2 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Bob Nystrom

Nit, but LGTM. https://chromiumcodereview.appspot.com/10875056/diff/1/utils/tests/pub/pub_install_git_test.dart File utils/tests/pub/pub_install_git_test.dart (right): https://chromiumcodereview.appspot.com/10875056/diff/1/utils/tests/pub/pub_install_git_test.dart#newcode340 utils/tests/pub/pub_install_git_test.dart:340: group("(regression)", () { I don't think ...

8 years, 4 months ago (2012-08-24 21:03:16 UTC) #2

nweiz

8 years, 4 months ago (2012-08-24 21:09:20 UTC) #3

https://chromiumcodereview.appspot.com/10875056/diff/1/utils/tests/pub/pub_in...
File utils/tests/pub/pub_install_git_test.dart (right):

https://chromiumcodereview.appspot.com/10875056/diff/1/utils/tests/pub/pub_in...
utils/tests/pub/pub_install_git_test.dart:340: group("(regression)", () {
On 2012/08/24 21:03:16, Bob Nystrom wrote:
> I don't think this group() adds much. Every test is eventually a regression
> test. :)
> 
> Remove it?

I think there's a useful distinction between tests that specify a core piece of
the behavior of Pub and tests that check for an edge case that was buggy at one
point. This is the sort of test that we'd be unlikely to write when initially
verifying the behavior of the code, but that exists because we've failed to
support this edge case in the past and want to make sure we don't again.

Expand Messages | Collapse Messages