Re: git-cl is down
On Fri, Jul 22, 2011 at 10:45:30PM +0200, Bertrand Bordage wrote: > What are we doing then? We only update git-cl's repository in the CG? I guess so? :( It would be nice to start a discussion with the rietveld people about allowing uploads of an actual git patch (regardless of whether numerous people here actually upload the final patch or keep their tree in un-squashed form). That would be a good capability to have on the server, and we wouldn't be able to change anything on our end until the server supports this anyway. We'll probably come back to this question in a few weeks or months anyway. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
What are we doing then? We only update git-cl's repository in the CG? Bertrand ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
2011/7/17 Carl Sorensen : > On 7/16/11 5:37 PM, "Graham Percival" wrote: > >> On Sat, Jul 16, 2011 at 05:13:29PM -0600, Carl Sorensen wrote: >>> >>> IMO, we should be aiming at one commit per Rietveld issue, rather than a >>> series of commits per Rietveld issue. >> >> That's beside the point, at least as far as I understand it. >> >> - Bertrand writes some code. >> - Bertrand makes a git commit. That commit has a nice message, it >> has his name, etc. >> - Bertrand gets this patch onto Rietveld using git-cl. > > More common, patch needs some changes. So Bertrand makes some changes and > then makes a git commit. This commit reflects the changes. Then Bertrand > pushes a new patch set. > > This happens a couple of times. > > Now Bertrand's repository doesn't have one commit on this branch, he has > three or four commits on his branch. And the first two or three are not > right -- they haven't passed code review. > > The final patch set passes code review. > > Graham wants to push this patch set. But he can't, unless he writes his own > commit message and sets the author. > > But if Bertrand has been uploading as he did with his test issue, there are > *3* patches, not *1*. And the first 2 are not accepted, so we don't want > them in our source tree. They might even break compiling; if so, they'd > mess up git bisect. > > Since we're only ready for committing after getting approval, we need to > combine into a single commit after approval. Either you can do it (creating > your own metadata, as you said) or he can do it (using git rebase -i). But > somebody needs to make the change to the commits in order get them ready for > pushing. Just for the record: i often make typos or style mistakes (in addition to regular fixes needed), so i often have more than 5 commits for a single issue, many of them with the message "blalah", "fix" or something like that. I usually do rebase -i when i feel the patch is finished and there are no unsolved comments; it's usually during countdown. cheers, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On Sat, Jul 16, 2011 at 07:35:50PM -0600, Carl Sorensen wrote: > Now Bertrand's repository doesn't have one commit on this branch, he has > three or four commits on his branch. And the first two or three are not > right -- they haven't passed code review. oh, I see. I personally always just modify the original commit -- in lily-git.tcl, it's option 2b instead of 2a. So I don't think in terms of multiple commits for each rietveld issue. > Since we're only ready for committing after getting approval, we need to > combine into a single commit after approval. Either you can do it (creating > your own metadata, as you said) or he can do it (using git rebase -i). But > somebody needs to make the change to the commits in order get them ready for > pushing. If one keeps separate commits like this, then yes, somebody needs to manually make those changes. I now understand why you were foucsing on the "multiple commits" aspect of this problem. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On 7/16/11 4:55 PM, "Graham Percival" wrote: > On Sat, Jul 16, 2011 at 08:34:56PM +0200, Jan Warchoł wrote: >> 2011/7/16 Bertrand Bordage : >>> Furthermore, there's something we should solve : upload.py is just uploading >>> diffs instead of full git commit patches. >>> By changing a line on upload.py, we can easily change this "git diff" for a >>> "git format-patch". >> >> Wouldn't this be best discussed in connection to GOP "Issue tracking >> with google code" & "Patch review tool" topics? > > If git-cl is nonexistent, then we need something to replace it > ASAP. Also, I think that Bertrand is just being a good "open > source citizen" by sending google rietveld a patch which allows > proper git commits as patches. > Here are other places where git-cl is available: https://github.com/martine/git-cl This is the original author's git repository, as far as I can tell. Here's a fork: https://github.com/jl/git-cl Either one of these could replace the current link in the CG. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On 7/16/11 5:37 PM, "Graham Percival" wrote: > On Sat, Jul 16, 2011 at 05:13:29PM -0600, Carl Sorensen wrote: >> >> IMO, we should be aiming at one commit per Rietveld issue, rather than a >> series of commits per Rietveld issue. > > That's beside the point, at least as far as I understand it. > > - Bertrand writes some code. > - Bertrand makes a git commit. That commit has a nice message, it > has his name, etc. > - Bertrand gets this patch onto Rietveld using git-cl. More common, patch needs some changes. So Bertrand makes some changes and then makes a git commit. This commit reflects the changes. Then Bertrand pushes a new patch set. This happens a couple of times. Now Bertrand's repository doesn't have one commit on this branch, he has three or four commits on his branch. And the first two or three are not right -- they haven't passed code review. The final patch set passes code review. Graham wants to push this patch set. But he can't, unless he writes his own commit message and sets the author. But if Bertrand has been uploading as he did with his test issue, there are *3* patches, not *1*. And the first 2 are not accepted, so we don't want them in our source tree. They might even break compiling; if so, they'd mess up git bisect. Since we're only ready for committing after getting approval, we need to combine into a single commit after approval. Either you can do it (creating your own metadata, as you said) or he can do it (using git rebase -i). But somebody needs to make the change to the commits in order get them ready for pushing. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
Am Sonntag, 17. Juli 2011, 01:37:42 schrieb Graham Percival: > But that's silly and stupid. Bertrand's git tree has a beautiful > commit. Why can't I get that commit when I want to push it? Why > does Rietveld and/or git-cl throw away that nice metadata? Because Rietveld is the problem: It was never intended for git, but rather for svn, where you only commit with a commit message, when you want to push to the server. During codereview in a Subversion workflow, you don't have the commit message written yet. Cheers, Reinhold -- -- Reinhold Kainhofer, reinh...@kainhofer.com, http://reinhold.kainhofer.com/ * Financial & Actuarial Math., Vienna Univ. of Technology, Austria * http://www.fam.tuwien.ac.at/, DVR: 0005886 * LilyPond, Music typesetting, http://www.lilypond.org ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On Sat, Jul 16, 2011 at 05:13:29PM -0600, Carl Sorensen wrote: > > IMO, we should be aiming at one commit per Rietveld issue, rather than a > series of commits per Rietveld issue. That's beside the point, at least as far as I understand it. - Bertrand writes some code. - Bertrand makes a git commit. That commit has a nice message, it has his name, etc. - Bertrand gets this patch onto Rietveld using git-cl. - review happens, it's accepted. - Graham wants to push this patch. ... - Whoops, there's no way to get the commit back from Rietveld! All I get is a raw diff. The author and commit message is lost! - suckiness and lack of profit. ok, I could manually patch -p1 < issue-1234.diff to my git tree, then do git commit --author "" and then type in a commit message. But that's silly and stupid. Bertrand's git tree has a beautiful commit. Why can't I get that commit when I want to push it? Why does Rietveld and/or git-cl throw away that nice metadata? Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On 7/16/11 5:00 PM, "Graham Percival" wrote: > On Sat, Jul 16, 2011 at 04:53:15PM -0600, Carl Sorensen wrote: >> Why are we trying to eliminate git-cl? What is the problem it causes? > > git-cl isn't the problem. > > The problem is that when I click on "download raw patch set" on a > codereview issue, I get a raw patch set. This loses the commit > message and author, so I can't (easily) push somebody's patch once > it's accepted. So I need to email the author(s), or more recently > Colin needs to track them down, ask them to send the patch to me, > forward me the patch when they send it to him instead, etc etc. > But if we use git format-patch instead of git diff, then we won't have a single patch; we'll have a collection of patches. Typically, I'll create a set of commits as I work through the comments on the issue. Then, before pushing, I'll do a rebase to get a clean commit. IMO, we should be aiming at one commit per Rietveld issue, rather than a series of commits per Rietveld issue. So I think there's more to it than what has been proposed so far. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On Sat, Jul 16, 2011 at 04:53:15PM -0600, Carl Sorensen wrote: > Why are we trying to eliminate git-cl? What is the problem it causes? git-cl isn't the problem. The problem is that when I click on "download raw patch set" on a codereview issue, I get a raw patch set. This loses the commit message and author, so I can't (easily) push somebody's patch once it's accepted. So I need to email the author(s), or more recently Colin needs to track them down, ask them to send the patch to me, forward me the patch when they send it to him instead, etc etc. For example, Bertrand's last patches were both delayed by a week due to bad timing between manually attaching the patch to an email and me building the latest devel release. That's just silly. However, it seems like fixing this requires some change to the rietveld source itself, so it depends on google programmers accepting the patch and adding that to their scheduled maintenance of the rietveld server(s). I certainly think it's worth raising it with them, though! Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On Sat, Jul 16, 2011 at 08:34:56PM +0200, Jan Warchoł wrote: > 2011/7/16 Bertrand Bordage : > > Furthermore, there's something we should solve : upload.py is just uploading > > diffs instead of full git commit patches. > > By changing a line on upload.py, we can easily change this "git diff" for a > > "git format-patch". > > Wouldn't this be best discussed in connection to GOP "Issue tracking > with google code" & "Patch review tool" topics? If git-cl is nonexistent, then we need something to replace it ASAP. Also, I think that Bertrand is just being a good "open source citizen" by sending google rietveld a patch which allows proper git commits as patches. This change could simplify your life by quite a bit, and is almost entirely non-invasive. I see no reason to delay. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On 7/16/11 10:38 AM, "Bertrand Bordage" wrote: >> Would it be possible to make upload.py sent multiple commits? For >> casual contributors, a single commit is fine, but serious >> developers like Mike require the ability to upload multiple >> commits for a single issue. > > Yes, this will be possible. > Do you mean something like this ? > http://codereview.appspot.com/4754046/ > This is achieved with a simple "python upload.py HEAD~3" > There is 3 commits and the three patches follow each other in the patch set. > As you can see on some side-by-side diffs, there is still a lot of bugs to > squash. Why are we trying to eliminate git-cl? What is the problem it causes? If it's just that it's not maintained, why not just add it as part of our source tree, along with lily-git? I guess there isn't a GPL license statement with git-cl, so maybe that won't work. Chromium uses git-cl in the depot_tools: http://dev.chromium.org/developers/how-tos/depottools It's a bit of a heavy download compared with git-cl, but one can also get git-cl directly from chromium: http://src.chromium.org/svn/trunk/tools/depot_tools/README.git-cl http://src.chromium.org/svn/trunk/tools/depot_tools/git_cl.py Personally, I think git-cl makes things much easier, so I wouldn't be in favor of eliminating it. Thanks, Carl ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On Sat, Jul 16, 2011 at 09:42:10PM +0200, Bertrand Bordage wrote: > > I finally made a good patch (bug-free), but we need to update codereview's > server. > The "temporary fork" option can't work. hmm, that makes things more complicated. Fortunately, Google code now supports git, so this could be an extra incentive for them to work on this? http://code.google.com/p/support/issues/detail?id=2454#c43 > I attached the patch for Rietveld. > It applies on this : > http://code.google.com/p/rietveld/source/checkout Please open an issue with Rietveld and include the patch with it. They may want to revise your patch, but definitely get in touch with them! Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
>
> I would like to rewrite the CG, but first I would like somebody
> (not me) to change upload.py
> - ideally the changes should be sent upstream
> - I'm willing to have a temporary "fork" of upload.py while we're
> waiting for a new official version of upload.py
>
I finally made a good patch (bug-free), but we need to update codereview's
server.
The "temporary fork" option can't work.
I attached the patch for Rietveld.
It applies on this :
http://code.google.com/p/rietveld/source/checkout
You can try it locally :
- Download Google App Engine :
http://code.google.com/appengine/downloads.html#Google_App_Engine_SDK_for_Python
- in Google App Engine's directory, run :
python dev_appserver.py [path_to_rietveld]
- send a patch set between commit1 and commit2 :
python [path_to_rietveld]/upload.py -slocalhost:8080 -y [commit2]..[commit1]
- see the result on localhost:8080
Regards,
Bertrand
Index: codereview/engine.py
===
--- codereview/engine.py (révision 775)
+++ codereview/engine.py (copie de travail)
@@ -53,11 +53,13 @@
patches = []
filename = None
diff = []
+ skip = False
for line in data.splitlines(True):
new_filename = None
if line.startswith('Index:'):
unused, new_filename = line.split(':', 1)
new_filename = new_filename.strip()
+ skip = False
elif line.startswith('Property changes on:'):
unused, temp_filename = line.split(':', 1)
# When a file is modified, paths use '/' between directories, however
@@ -67,6 +69,11 @@
if temp_filename != filename:
# File has property changes but no modifications, create a new diff.
new_filename = temp_filename
+# Skips metadatas in git patches
+if line == '-- \n':
+ skip = True
+if skip:
+ continue
if new_filename:
if filename and diff:
patches.append((filename, ''.join(diff)))
Index: upload.py
===
--- upload.py (révision 775)
+++ upload.py (copie de travail)
@@ -1239,7 +1239,7 @@
# git config key "diff.external" is used).
env = os.environ.copy()
if 'GIT_EXTERNAL_DIFF' in env: del env['GIT_EXTERNAL_DIFF']
-return RunShell(["git", "diff", "--no-ext-diff", "--full-index", "-M"]
+return RunShell(["git", "format-patch", "--stdout", "-M"]
+ extra_args, env=env)
def GetUnknownFiles(self):
@@ -1786,11 +1786,13 @@
patches = []
filename = None
diff = []
+ skip = False
for line in data.splitlines(True):
new_filename = None
if line.startswith('Index:'):
unused, new_filename = line.split(':', 1)
new_filename = new_filename.strip()
+ skip = False
elif line.startswith('Property changes on:'):
unused, temp_filename = line.split(':', 1)
# When a file is modified, paths use '/' between directories, however
@@ -1800,6 +1802,11 @@
if temp_filename != filename:
# File has property changes but no modifications, create a new diff.
new_filename = temp_filename
+# Skips metadatas in git patches
+if line == '-- \n':
+ skip = True
+if skip:
+ continue
if new_filename:
if filename and diff:
patches.append((filename, ''.join(diff)))
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
2011/7/16 Bertrand Bordage : > Hi all, > As mentioned in the title, git-cl's repository on neugierig.org is down. > It looks like this project isn't supported anymore. > I suggest we rewrite the last part of CG 3.3.4 "Uploading a patch for > review". > In fact, why were we using git-cl ? What is git-cl providing that can't be > done with upload.py from Rietveld codereview ? > The doc should be explaining how to use upload.py. > > Furthermore, there's something we should solve : upload.py is just uploading > diffs instead of full git commit patches. > By changing a line on upload.py, we can easily change this "git diff" for a > "git format-patch". > I created a fake issue to show the result : > http://codereview.appspot.com/4721041/ > When you click "download raw set", this gives you what is really expected. > Unfortunately, this changes the syntax of the input arguments of upload.py. > The result is that it is no longer possible to upload a commit other than > the latest. > But the idea should be further explored, since it would save Graham's time. > If you agree with this, the first thing to do would be to create an issue on > http://code.google.com/p/rietveld/issues/list Wouldn't this be best discussed in connection to GOP "Issue tracking with google code" & "Patch review tool" topics? cheers, Janek ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
> > git-cl stored the issue number inside each git branch, so that you can > easily update an issue with a new pathcset without having to look up the > issue number. > Hum, this is true. > Plus, you don't have to create a patch file on disk that you can then > upload. So, git-cl is basically only half the work of using upload.py > manually. > Maybe it was so, but it is now possible to upload directly with the same arguments as git diff. Bertrand ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
> > Would it be possible to make upload.py sent multiple commits? For > casual contributors, a single commit is fine, but serious > developers like Mike require the ability to upload multiple > commits for a single issue. Yes, this will be possible. Do you mean something like this ? http://codereview.appspot.com/4754046/ This is achieved with a simple "python upload.py HEAD~3" There is 3 commits and the three patches follow each other in the patch set. As you can see on some side-by-side diffs, there is still a lot of bugs to squash. Bertrand PS : Shall I send the issue on Rietveld's code.google ? ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On Sa., 16. Jul. 2011 17:42:36 CEST, Graham Percival wrote: > On Sat, Jul 16, 2011 at 10:56:28AM +0200, Bertrand Bordage wrote: > > In fact, why were we using git-cl ? What is git-cl providing that > > can't be done with upload.py from Rietveld codereview ? git-cl stored the issue number inside each git branch, so that you can easily update an issue with a new pathcset without having to look up the issue number. Plus, you don't have to create a patch file on disk that you can then upload. So, git-cl is basically only half the work of using upload.py manually. Cheers, Reinhold ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: git-cl is down
On Sat, Jul 16, 2011 at 10:56:28AM +0200, Bertrand Bordage wrote: > In fact, why were we using git-cl ? What is git-cl providing that can't be > done > with upload.py from Rietveld codereview ? I'm not certain that upload.py allowed us to use git patches in the past; I definitely did not think that such a thing was possible. > Furthermore, there's something we should solve : upload.py is just uploading > diffs instead of full git commit patches. > By changing a line on upload.py, we can easily change this "git diff" for a > "git format-patch". > I created a fake issue to show the result : http://codereview.appspot.com/ > 4721041/ > When you click "download raw set", this gives you what is really expected. > > Unfortunately, this changes the syntax of the input arguments of upload.py. > The > result is that it is no longer possible to upload a commit other than the > latest. Absolutely! > If you agree with this, the first thing to do would be to create an issue on > http://code.google.com/p/rietveld/issues/list > > If nobody else wants to rewrite the doc, I will. Same thing for patching > codereview. I would like to rewrite the CG, but first I would like somebody (not me) to change upload.py - ideally the changes should be sent upstream - I'm willing to have a temporary "fork" of upload.py while we're waiting for a new official version of upload.py Would it be possible to make upload.py sent multiple commits? For casual contributors, a single commit is fine, but serious developers like Mike require the ability to upload multiple commits for a single issue. Cheers, - Graham ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
git-cl is down
Hi all, As mentioned in the title, git-cl's repository on neugierig.org is down. It looks like this project isn't supported anymore. I suggest we rewrite the last part of CG 3.3.4 "Uploading a patch for review". In fact, why were we using git-cl ? What is git-cl providing that can't be done with upload.py from Rietveld codereview ? The doc should be explaining how to use upload.py. Furthermore, there's something we should solve : upload.py is just uploading diffs instead of full git commit patches. By changing a line on upload.py, we can easily change this "git diff" for a "git format-patch". I created a fake issue to show the result : http://codereview.appspot.com/4721041/ When you click "download raw set", this gives you what is really expected. Unfortunately, this changes the syntax of the input arguments of upload.py. The result is that it is no longer possible to upload a commit other than the latest. But the idea should be further explored, since it would save Graham's time. If you agree with this, the first thing to do would be to create an issue on http://code.google.com/p/rietveld/issues/list If nobody else wants to rewrite the doc, I will. Same thing for patching codereview. Regards, Bertrand ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
