Add period below review URL when adding review URL footer to committed CLs

Add period below review URL when adding review URL footer to committed CLs

Because some interfaces aren't linkifying the URL when the commit message ends with it.

BUG=480922
TESTED=tests/git_cl_test.py; presubmit ok

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295566

[smut, 2015-06-08 20:47:17]

smut@google.com changed reviewers:
+ jrobbins@chromium.org, maruel@chromium.org

[smut, 2015-06-08 20:47:17]

Honestly I hate the way < > looks, but I expect that adding non-breaking spaces,
zero-width spaces, and other "invisible" Unicode characters (as suggested on the
bug) is just asking for trouble.

[M-A Ruel, 2015-06-08 22:14:11]

On 2015/06/08 20:47:17, smut wrote:
> Honestly I hate the way < > looks, but I expect that adding non-breaking
spaces,
> zero-width spaces, and other "invisible" Unicode characters (as suggested on
the
> bug) is just asking for trouble.

We're talking about XCode here. I don't like mangling the url. If you want junk,
add a dot on a line after.

[M-A Ruel, 2015-06-08 22:21:01]

https://codereview.chromium.org/1165293002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/1165293002/diff/20001/git_cl.py#newcode2397
git_cl.py:2397: commit_desc.append_footer('Review URL: %s' % cl.GetIssueURL())
What you want is this:
commit_desc.append_footer('Review URL: %s\n.' % cl.GetIssueURL())

otherwise it will add a blank line in between.

Please add a comment as to why, be clear it's XCode that causes this.

[smut, 2015-06-08 22:22:57]

On 2015/06/08 22:21:01, M-A Ruel wrote:
> https://codereview.chromium.org/1165293002/diff/20001/git_cl.py
> File git_cl.py (right):
> 
> https://codereview.chromium.org/1165293002/diff/20001/git_cl.py#newcode2397
> git_cl.py:2397: commit_desc.append_footer('Review URL: %s' % cl.GetIssueURL())
> What you want is this:
> commit_desc.append_footer('Review URL: %s\n.' % cl.GetIssueURL())
> 
> otherwise it will add a blank line in between.
> 
> Please add a comment as to why, be clear it's XCode that causes this.

Couldn't we just add the period on the same line? Review URL:
https://codereview.chromium.org/1165293002.

[M-A Ruel, 2015-06-08 22:23:27]

On 2015/06/08 22:22:57, smut wrote:
> On 2015/06/08 22:21:01, M-A Ruel wrote:
> > https://codereview.chromium.org/1165293002/diff/20001/git_cl.py
> > File git_cl.py (right):
> > 
> > https://codereview.chromium.org/1165293002/diff/20001/git_cl.py#newcode2397
> > git_cl.py:2397: commit_desc.append_footer('Review URL: %s' %
cl.GetIssueURL())
> > What you want is this:
> > commit_desc.append_footer('Review URL: %s\n.' % cl.GetIssueURL())
> > 
> > otherwise it will add a blank line in between.
> > 
> > Please add a comment as to why, be clear it's XCode that causes this.
> 
> Couldn't we just add the period on the same line? Review URL:
> https://codereview.chromium.org/1165293002.

Ok, I prefer that.

[smut, 2015-06-08 22:25:24]

On 2015/06/08 22:23:27, M-A Ruel wrote:
> On 2015/06/08 22:22:57, smut wrote:
> > On 2015/06/08 22:21:01, M-A Ruel wrote:
> > > https://codereview.chromium.org/1165293002/diff/20001/git_cl.py
> > > File git_cl.py (right):
> > > 
> > >
https://codereview.chromium.org/1165293002/diff/20001/git_cl.py#newcode2397
> > > git_cl.py:2397: commit_desc.append_footer('Review URL: %s' %
> cl.GetIssueURL())
> > > What you want is this:
> > > commit_desc.append_footer('Review URL: %s\n.' % cl.GetIssueURL())
> > > 
> > > otherwise it will add a blank line in between.
> > > 
> > > Please add a comment as to why, be clear it's XCode that causes this.
> > 
> > Couldn't we just add the period on the same line? Review URL:
> > https://codereview.chromium.org/1165293002.
> 
> Ok, I prefer that.

Comment is perhaps unnecessary now that this doesn't look odd or out of place
(completely reasonable to end a line with a period I think). But I left it in
for the benefit of people who rewrite the message.

[M-A Ruel, 2015-06-08 22:26:48]

lgtm

and yes I prefer with the comment.

[smut, 2015-06-08 22:28:15]

The CQ bit was checked by smut@google.com

[commit-bot: I haz the power, 2015-06-08 22:28:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1165293002/40001

[commit-bot: I haz the power, 2015-06-08 22:31:31]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=295566

[Sergiy Byelozyorov, 2015-07-06 08:54:49]

sergiyb@chromium.org changed reviewers:
+ sergiyb@chromium.org

[Sergiy Byelozyorov, 2015-07-06 08:54:50]

This actually broke links on Gitiles, e.g.
https://chrome-internal.googlesource.com/infra/puppet.git/+/400fe0b559f2f1e76....
Now '.' becomes part of the URL and resolves to 404, which requires manually
editing the URL in the omnibar to remove the dot. This is correct behavior since
'.' is a valid character in the URL.

Is there any other way to fix Xcode linkification? Perhaps we can add a space
and then a '.'? It's ugly, but will probably work for both Xcode and Gitiles:
https://codereview.chromium.org/1225713002.

[Sergiy Byelozyorov, 2015-07-06 09:23:57]

On 2015/07/06 08:54:50, Sergiy Byelozyorov wrote:
> This actually broke links on Gitiles, e.g.
>
https://chrome-internal.googlesource.com/infra/puppet.git/+/400fe0b559f2f1e76....
> Now '.' becomes part of the URL and resolves to 404, which requires manually
> editing the URL in the omnibar to remove the dot. This is correct behavior
since
> '.' is a valid character in the URL.
> 
> Is there any other way to fix Xcode linkification? Perhaps we can add a space
> and then a '.'? It's ugly, but will probably work for both Xcode and Gitiles:
> https://codereview.chromium.org/1225713002.

Just noticed that your first patchset was with <> brackets. This looks much
nicer than a space before period. Why did you change it?

[smut, 2015-07-06 20:09:11]

On 2015/07/06 09:23:57, Sergiy Byelozyorov wrote:
> On 2015/07/06 08:54:50, Sergiy Byelozyorov wrote:
> > This actually broke links on Gitiles, e.g.
> >
>
https://chrome-internal.googlesource.com/infra/puppet.git/+/400fe0b559f2f1e76....
> > Now '.' becomes part of the URL and resolves to 404, which requires manually
> > editing the URL in the omnibar to remove the dot. This is correct behavior
> since
> > '.' is a valid character in the URL.
> > 
> > Is there any other way to fix Xcode linkification? Perhaps we can add a
space
> > and then a '.'? It's ugly, but will probably work for both Xcode and
Gitiles:
> > https://codereview.chromium.org/1225713002.
> 
> Just noticed that your first patchset was with <> brackets. This looks much
> nicer than a space before period. Why did you change it?

It was inconsistent with the entire rest of the commit message, which never has
angle brackets around any of the URLs. maruel@ suggested adding the dot.