Allow git-try to try rietveld changes directly.

Allow git-try to try rietveld changes directly. 

Some external contributors need their CL's try'd quickly, but currently its not easy unless we patch locally. This will allow you to submit the rietveld issue number and will do the rest automatically. As well, renable the dry_run option. It was removed. 

BUG=None 
TEST=git try --issue 12345

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

[M-A Ruel, 2009-12-09 20:27:20]

http://codereview.chromium.org/481006/diff/1/2
File git-try (right):

http://codereview.chromium.org/481006/diff/1/2#newcode229
git-try:229: file_list=None
This will make the presubmit scripts fail but I think it's fixes already.

http://codereview.chromium.org/481006/diff/1/2#newcode284
git-try:284: print args
are you sure?

[Evan Martin, 2009-12-09 20:30:52]

http://codereview.chromium.org/481006/diff/1/2
File git-try (right):

http://codereview.chromium.org/481006/diff/1/2#newcode210
git-try:210: if (not options.diff):
parens not necessary

http://codereview.chromium.org/481006/diff/1/2#newcode229
git-try:229: file_list=None
missing spaces around =

http://codereview.chromium.org/481006/diff/1/2#newcode284
git-try:284: print args
Is this change intentional?

[Mohamed Mansour, 2009-12-09 20:52:31]

Please re-review guys, now, we can finally do without patching it locally:
git try --issue=1234 --bot=linux

http://codereview.chromium.org/481006/diff/1/2
File git-try (right):

http://codereview.chromium.org/481006/diff/1/2#newcode229
git-try:229: file_list=None
On 2009/12/09 20:27:20, Marc-Antoine Ruel wrote:
> This will make the presubmit scripts fail but I think it's fixes already.

Done. Presubmits do fail, but it successfully uploads it correctly. That is why
I asked on irc if we have unit test, it seems members have been not added to
that list.
FAIL: testMembersChanged (__main__.TryChangeUnittest)

http://codereview.chromium.org/481006/diff/1/2#newcode284
git-try:284: print args
On 2009/12/09 20:27:20, Marc-Antoine Ruel wrote:
> are you sure?

Done. typo from testing :)

[Mohamed Mansour, 2009-12-09 23:44:18]

I tested this, and it works with svn styled diffs, and git styled diffs.

I munged the diff before sending, now file_lists and diffs are correct. 

To make it work, there are three ways now:
- To send a try on your local branch: git try 
- To send a try on a local diff file: git try --diff=hi.diff
- To send a try on a issue from riet: git try --issue=12345

[M-A Ruel, 2009-12-10 01:21:07]

http://codereview.chromium.org/481006/diff/9/2008
File git-try (right):

http://codereview.chromium.org/481006/diff/9/2008#newcode185
git-try:185: fetch = "curl --silent %s" % url
Na, use urllib.urlopen().read()

[Mohamed Mansour, 2009-12-10 03:39:30]

Thanks maruel, please take another look and criticize. This is  like my second
patch working with python, so I am still a noobie.

[M-A Ruel, 2009-12-10 15:30:28]

http://codereview.chromium.org/481006/diff/9001/9002
File git-try (right):

http://codereview.chromium.org/481006/diff/9001/9002#newcode93
git-try:93: output = []
remove this line

http://codereview.chromium.org/481006/diff/9001/9002#newcode99
git-try:99: new_cwd = None
Remove this line

http://codereview.chromium.org/481006/diff/9001/9002#newcode237
git-try:237: url = "http://%s/%s" % (GetRietveldServer(), options.issue)
Err, well, hum, that works but it's not what I'd say is great. Especially that
there is no way to make sure you grab the latest patch. I know by default this
will work but still. We should look to see if rietveld has a programmatic way to
get the latest patchset number for a random issue. Can you take a look at 
http://code.google.com/p/rietveld/source/browse/#svn/trunk/codereview ?

http://codereview.chromium.org/481006/diff/9001/9002#newcode243
git-try:243: # Dump it to a temporary file.
Remove this comment, it's misleading.

[Mohamed Mansour, 2009-12-10 17:11:46]

http://codereview.chromium.org/481006/diff/9001/9002
File git-try (right):

http://codereview.chromium.org/481006/diff/9001/9002#newcode93
git-try:93: output = []
On 2009/12/10 15:30:28, Marc-Antoine Ruel wrote:
> remove this line

Done.

http://codereview.chromium.org/481006/diff/9001/9002#newcode99
git-try:99: new_cwd = None
On 2009/12/10 15:30:28, Marc-Antoine Ruel wrote:
> Remove this line

Done.

http://codereview.chromium.org/481006/diff/9001/9002#newcode237
git-try:237: url = "http://%s/%s" % (GetRietveldServer(), options.issue)
On 2009/12/10 15:30:28, Marc-Antoine Ruel wrote:
> Err, well, hum, that works but it's not what I'd say is great. Especially that
> there is no way to make sure you grab the latest patch. I know by default this
> will work but still. We should look to see if rietveld has a programmatic way
to
> get the latest patchset number for a random issue. Can you take a look at 
> http://code.google.com/p/rietveld/source/browse/#svn/trunk/codereview ?

I was looking into rietveld before to see if we can get the patchset logically,
but we can't unless we query it from the database and grab its
PatchSet.key().id(). There was two choices:

1) Either implement a restful service similar on how its done when we fetch the
"description", such as we can do one that fetches "patchset".

2) Do what Evan Martin did in his, 'git cl patch <issue>' where he parsed for
the issue number. At most there could be  just one in the initial request.

I would rather prefer us to go with #1, but it was simpler to go with #2 since
its already implemented for git-cl. Another reason to go with #2 is that I don't
have the environment to setup python/django/appengine application. I never
worked with appengine/django/python before, all I did in python was two patches
(this is my second one).

If you want, I can go with #1, because I'd like to learn django/python. Good
experience.

http://codereview.chromium.org/481006/diff/9001/9002#newcode243
git-try:243: # Dump it to a temporary file.
On 2009/12/10 15:30:28, Marc-Antoine Ruel wrote:
> Remove this comment, it's misleading.

Done.

[Mohamed Mansour, 2009-12-12 05:39:09]

Since we are in the talks with Guide Van Rossum by implementing an API for
Rietveld, can we push this in until the other stuff is going to be pushed. 

I would like to get this in, and submit this patch as it is since it works as
intended and uses the same technique as "git cl" to fetch the patch set number. 

I don't thing Guide will like the other patch since its just adding another
submethod, like the following:
http://mohamedmansour.com:8080/1/json

I believe he wants a full fledged API and for the likes of it, it seems
reviewing would take a long time :) And once that gets submitted, some rework of
"git cl" and "gcl" would be needed.

[Mohamed Mansour, 2010-01-07 23:20:29]

I should close this patch right? Since git-try has been completely redone since
trychange is super awesome :) (as described in the log)

I assume people could now do:
git try --issue=1234 --email=foo@bar --url=http://path/to/diff

Since maruel committed the small api change I made, maybe we could still do:
git try --patch=1234

But I don't think that would be needed I think since it can already be done with
what Marc-Antoine did via --url.

[M-A Ruel, 2010-01-07 23:52:35]

No, don't drop the patch, it's useful but a part will need to be ported to
trychange.py. If you don't have time it's fine.

[Mohamed Mansour, 2010-01-09 23:45:28]

Alright I updated this. I am having a problem that I cannot access tryserver.
Using the normal git-try. 

$ git try
Results will be emailed to: mhm@chromium.org
svn commit
/cygdrive/c/Users/Mohamed/AppData/Local/Temp/tmprR4PK4/Mohamed.print-baaa58.2010-01-09
18.43.40.923691.diff --file /cygdrive/c/Users/Mohame
d/AppData/Local/Temp/tmpeGE7Qh
Ouput:

Sorry, Tryserver is not available.

[M-A Ruel, 2010-01-10 00:32:34]

In short, lgtm with a few changes.

http://codereview.chromium.org/481006/diff/16005/15007
File git-try (right):

http://codereview.chromium.org/481006/diff/16005/15007#newcode33
git-try:33: ['config', 'rietveld.server'], error_ok=True).rstrip('\n')
you can also just use .strip()

http://codereview.chromium.org/481006/diff/16005/15007#newcode44
git-try:44: else:
rietveld_url = GetRietveldServerUrl()
if rietveld_url:

http://codereview.chromium.org/481006/diff/16005/15008
File trychange.py (right):

http://codereview.chromium.org/481006/diff/16005/15008#newcode439
trychange.py:439: group.add_option("--rietveld_url",
I'm not sure about "rietveld_url" if we could find something better. Bof.

http://codereview.chromium.org/481006/diff/16005/15008#newcode578
trychange.py:578: api_url = 'http://%s/api/%d' % (options.rietveld_url,
options.issue)
Add the http:// prefix in git-try, in case we'd support a https:// code review
site.
So in short, use:
  api_url = posixpath.join(options.rietveld_url, 'api', str(options.issue))

http://codereview.chromium.org/481006/diff/16005/15008#newcode625
trychange.py:625: if options.dry_run:
Remove this. Search for "if options.dry_run:" in the file, you'll see where it's
done now.

[Nico, 2010-01-10 04:55:49]

This breaks `git try` for me. Here's an attempted explanation:

http://codereview.chromium.org/481006/diff/15011/15012
File git-try (right):

http://codereview.chromium.org/481006/diff/15011/15012#newcode39
git-try:39: if patchset:
if patchset evaluates to true here…

http://codereview.chromium.org/481006/diff/15011/15013
File trychange.py (right):

http://codereview.chromium.org/481006/diff/15011/15013#newcode580
trychange.py:580: diff_url = ('http://%s/download/issue%d_%d.diff' %
…then this branch is taken and api_url is http://None/api... 'cause rietveld_url
is set only in the other branch in git-try

[Mohamed Mansour, 2010-01-10 05:00:32]

I will try to figure out which case this caused that, since git-try should never
go into that conditional since its only meant if we supply git-try with --issue.

I will take a look at it now, thanks thakis. Sorry for the troubles.

[Nico, 2010-01-10 05:02:44]

http://codereview.chromium.org/481006/diff/15011/15013
File trychange.py (right):

http://codereview.chromium.org/481006/diff/15011/15013#newcode582
trychange.py:582: diff = GetMungedDiff('', urllib.urlopen(diff_url).readlines())
wait, do i read this right? does this mean that this (once rietveld_urls is set)
will make `git try` try the latest version that i uploaded to rietveld instead
of the latest version in my local branch if i uploaded my branch at some time in
the past?

if so, that would be bad.

[Nico, 2010-01-10 05:05:33]

...and finally, it looks like the diffs generated in the new branch don't work
with `patch`, see
http://build.chromium.org/buildbot/try-server/builders/linux/builds/13625/ste...

[Nico, 2010-01-10 05:17:52]

Looks like the core problem is that the new branch should only be executed if
the user passed --issue explicitly to git-try, but --issue is also passed from
git-try to trychange if rietveldpatchset is set for a branch, i.e. it has been
uploaded at least once, which is wrong.

[Mohamed Mansour, 2010-01-10 05:45:58]

Thanks thakis, the problem should be committed and resolved. Thanks for your
help!

-Mohamed Mansour


On Sun, Jan 10, 2010 at 12:17 AM, <thakis@chromium.org> wrote:

> Looks like the core problem is that the new branch should only be executed
> if
> the user passed --issue explicitly to git-try, but --issue is also passed
> from
> git-try to trychange if rietveldpatchset is set for a branch, i.e. it has
> been
> uploaded at least once, which is wrong.
>
>
> http://codereview.chromium.org/481006
>