|
|
|
Created:
14 years, 2 months ago by techtonik Modified:
14 years, 2 months ago CC:
codereview-discuss_googlegroups.com Visibility:
Public. |
Currently, if you upload patch from Mercurial, diff is translated to SVN format, but filenames are still prepended with a/ and b/ prefixes.
See http://codereview.appspot.com/download/issue5338049_1.diff
Here is a fix for that.
Patch Set 1 #Patch Set 2 : Do not skip non-matched lines after header #Total messages: 11
|
techtonik
|
14 years, 2 months ago (2011年11月11日 20:13:27 UTC) #1 |
As a general note, I'd prefer this not to be done with git, since I use this information to determine what kind of patch it is (if it was generated with svn or git for a git-svn clone). I don't know about mercurial if it is a good idea. What's the rationale to do this?
On 2011年11月11日 20:26:37, M-A wrote: > As a general note, I'd prefer this not to be done with git, since I use this > information to determine what kind of patch it is (if it was generated with svn > or git for a git-svn clone). I'd call this a bad architecture. To me patch processing and transformation logic should be done on server side, so even after patch is split into chunks you still can get metadata about author, version control system, parent revision etc. See also these issues for use cases http://code.google.com/p/rietveld/issues/detail?id=235 and http://code.google.com/p/rietveld/issues/detail?id=220 > I don't know about mercurial if it is a good idea. What's the rationale to do > this? Well, some patches from Rietveld are applied without '--strip X' and some require it. This inconsistency is annoying and hard to automate in generic way (i.e. for integration with IDE, such as Spyder).
Le 11 novembre 2011 15:49, <techtonik@gmail.com> a écrit : > On 2011年11月11日 20:26:37, M-A wrote: > >> As a general note, I'd prefer this not to be done with git, since I >> > use this > >> information to determine what kind of patch it is (if it was generated >> > with svn > >> or git for a git-svn clone). >> > > I'd call this a bad architecture. To me patch processing and > transformation logic should be done on server side, so even after patch > is split into chunks you still can get metadata about author, version > control system, parent revision etc. > I assume you mean 'split into one patch per files'. Sounds mercurial - specific. > See also these issues for use cases > http://code.google.com/p/**rietveld/issues/detail?id=235<http://code.google.c... > http://code.google.com/p/**rietveld/issues/detail?id=220<http://code.google.c... > > > > I don't know about mercurial if it is a good idea. What's the >> > rationale to do > >> this? >> > > Well, some patches from Rietveld are applied without '--strip X' and > some require it. This inconsistency is annoying and hard to automate in > generic way (i.e. for integration with IDE, such as Spyder). > Well, if a patch was generated from mercurial, it should always require --strip 1. When doesn't it require it? M-A
On 2011年11月11日 21:01:24, M-A wrote: > Le 11 novembre 2011 15:49, <mailto:techtonik@gmail.com> a écrit : > > > On 2011年11月11日 20:26:37, M-A wrote: > > > >> As a general note, I'd prefer this not to be done with git, since I > >> > > use this > > > >> information to determine what kind of patch it is (if it was generated > >> > > with svn > > > >> or git for a git-svn clone). > >> > > > > I'd call this a bad architecture. To me patch processing and > > transformation logic should be done on server side, so even after patch > > is split into chunks you still can get metadata about author, version > > control system, parent revision etc. > > > > I assume you mean 'split into one patch per files'. Sounds mercurial - > specific. I mean that patchset is split into header with meta-info and patches, and each patch is split further into chunks for commenting. So, the idea here is that with server-side processing you don't need to rely on buggy transformation artifacts to detect the type of patch. You just save original one. > > > > I don't know about mercurial if it is a good idea. What's the > >> > > rationale to do > > > >> this? > >> > > > > Well, some patches from Rietveld are applied without '--strip X' and > > some require it. This inconsistency is annoying and hard to automate in > > generic way (i.e. for integration with IDE, such as Spyder). > > > > Well, if a patch was generated from mercurial, it should always require > --strip 1. When doesn't it require it? When it is generated by SVN, because you don't know beforehand if patch is generated by Mercurial or SVN. Usually SVN patches are detected by 'Index:' header, and you could safely assume --strip 0 for such patches, but not in Rietveld case.
On 2011年11月11日 22:17:16, techtonik wrote: > > Well, if a patch was generated from mercurial, it should always require > > --strip 1. When doesn't it require it? > > When it is generated by SVN, because you don't know beforehand if patch is > generated by Mercurial or SVN. Usually SVN patches are detected by 'Index:' > header, and you could safely assume --strip 0 for such patches, but not in > Rietveld case. When a patch can be generated by SVN if the source control is mercurial?
On 2011年11月11日 22:23:40, M-A wrote: > On 2011年11月11日 22:17:16, techtonik wrote: > > > Well, if a patch was generated from mercurial, it should always require > > > --strip 1. When doesn't it require it? > > > > When it is generated by SVN, because you don't know beforehand if patch is > > generated by Mercurial or SVN. Usually SVN patches are detected by 'Index:' > > header, and you could safely assume --strip 0 for such patches, but not in > > Rietveld case. > > When a patch can be generated by SVN if the source control is mercurial? If you integrate Rietveld support into IDE, you don't know what source control was used to generate the patch you're trying to apply from the issue. I often use hg-git instead of Git for GitHub project, so its not safe to assume that if project is handled by Mercurial then patch from Rietveld will be Mercurial also.
On Fri, Nov 11, 2011 at 11:17 PM, <techtonik@gmail.com> wrote: > On 2011年11月11日 21:01:24, M-A wrote: >> >> Le 11 novembre 2011 15:49, <mailto:techtonik@gmail.com> a écrit : > >> > On 2011年11月11日 20:26:37, M-A wrote: >> > >> >> As a general note, I'd prefer this not to be done with git, since I >> >> >> > use this >> > >> >> information to determine what kind of patch it is (if it was > > generated >> >> >> >> > with svn >> > >> >> or git for a git-svn clone). >> >> >> > >> > I'd call this a bad architecture. To me patch processing and >> > transformation logic should be done on server side, so even after > > patch >> >> > is split into chunks you still can get metadata about author, > > version >> >> > control system, parent revision etc. >> > > >> I assume you mean 'split into one patch per files'. Sounds mercurial - >> specific. > > I mean that patchset is split into header with meta-info and patches, > and each patch is split further into chunks for commenting. So, the idea > here is that with server-side processing you don't need to rely on buggy > transformation artifacts to detect the type of patch. You just save > original one. > >> > >> > I don't know about mercurial if it is a good idea. What's the >> >> >> > rationale to do >> > >> >> this? >> >> >> > >> > Well, some patches from Rietveld are applied without '--strip X' and >> > some require it. This inconsistency is annoying and hard to automate > > in >> >> > generic way (i.e. for integration with IDE, such as Spyder). >> > > >> Well, if a patch was generated from mercurial, it should always > > require >> >> --strip 1. When doesn't it require it? > > When it is generated by SVN, because you don't know beforehand if patch > is generated by Mercurial or SVN. Usually SVN patches are detected by > 'Index:' header, and you could safely assume --strip 0 for such patches, > but not in Rietveld case. You can't safely assume "--strip 0" for patches created by SVN since you can run svn (and upload.py) from a subdirectory in your working copy. In that case the parent directories are missing in the Index entry in the resulting diff. --Andi > > http://codereview.appspot.com/5369087/ >
On 2011年11月12日 06:48:00, Andi Albrecht wrote: > > > > When it is generated by SVN, because you don't know beforehand if patch > > is generated by Mercurial or SVN. Usually SVN patches are detected by > > 'Index:' header, and you could safely assume --strip 0 for such patches, > > but not in Rietveld case. > > You can't safely assume "--strip 0" for patches created by SVN since > you can run svn (and upload.py) from a subdirectory in your working > copy. In that case the parent directories are missing in the Index > entry in the resulting diff. When you integrate Rietveld with IDE, all patches are generated from the project root.
To summarize. I don't like that Rietveld generates invalid patches in SVN format. SVN patches shouldn't require any strip command to be applied - only correct directory. Another reason is that my python-patch library doesn't support strips, so before writing a wrapper implementing workaround for invalid paths, I decided to fix it in Rietveld first. So far I see only one objection against this change - it is because this bug is used to detect how patches were generated. Because there is no other way to detect VCS otherwise. So, instead of relying on this implicit behavior, I propose to implement the feature of detecting VCS properly, and use Repository ID in the meanwhile.