Issue 4333051: Fix support for renamed files, aka git mv in upload.py. (Closed)
|
Created:
13 years, 11 months ago by M-A Modified:
12 years, 8 months ago CC:
codereview-discuss_googlegroups.com, cmp Base URL:
https://rietveld.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionFix support for renamed files, aka git mv in upload.py.
Adds usage of -C -C. With limited testing on the chromium tree of ~36k files, it adds a few tens of percent to the time required to generate the diff.
Prior this change, a renamed file would only have the new file listed on
rietveld and not the previous one. This is caused by the default behavior of
"git diff".
This change calls "git diff" two times to extract all the necessary data.
BUG=251
Patch Set 1 #Patch Set 2 : Reviving old CL #Patch Set 3 : Fix 80 cols #
Total comments: 5
Patch Set 4 : Address review comments #Patch Set 5 : Reinstate break #Patch Set 6 : fix grammar #MessagesTotal messages: 12
For a live example, these two uploads were made by the exact same git branch: current upload.py: http://codereview.appspot.com/4313063/ with this fix: http://codereview.appspot.com/4333050/
Sign in to reply to this message.
How do you call git then? I've tested it locally, but upload.py returned an error. Here's a transcript: $ git mv README README2 andi [git:master] rietveld$ ./upload.py -y -m "test" -s localhost:8080 Upload server: localhost:8080 (change with -s/--server) Loaded authentication cookies from /home/andi/.codereview_upload_cookies Issue created. URL: http://localhost:8080/5 Uploading base file for index.yaml Uploading base file for upload.py andi [git:master+] rietveld$ ./upload.py -y -m "test" -s localhost:8080 HEAD Got error status from ['git', 'show', 'HEAD:README2']: andi [git:master+] rietveld$ git status # On branch master # Changes to be committed: # (use "git reset HEAD <file>..." to unstage) # # renamed: README -> README2 # # Changes not staged for commit: # (use "git add <file>..." to update what will be committed) # (use "git checkout -- <file>..." to discard changes in working directory) # # modified: index.yaml # modified: upload.py # andi [git:master+] rietveld$
Sign in to reply to this message.
The problem is that we use it quite differently: You assume that you want to diff against HEAD all the time. The assumption is hidden in the fact that git show grabs always files from HEAD. Even more, it doesn't use self.renames[filename] but directly filename (!) We assume that you always want to diff against a branch/hash and that all the changes are committed and nothing is staged or modified. Only untracked files are ignored. I think the first workflow is untenable, otherwise how can you do parallel work or pipeline multiple reviews one against the other? You need to commit your changes to continue working so assuming that a diff is done against HEAD isn't useful. Having uncommitted changes removes all benefits from using git in the first place. The code somewhat assumes that extra_args can be a hash to diff against but is inconsistent. I'd rather enforce that it is always given and len(extra_args) == 1. M-A On 2011/04/01 12:26:31, Andi Albrecht wrote: > How do you call git then? I've tested it locally, but upload.py returned an > error. Here's a transcript: > > $ git mv README README2 > andi [git:master] rietveld$ ./upload.py -y -m "test" -s localhost:8080 > Upload server: localhost:8080 (change with -s/--server) > Loaded authentication cookies from /home/andi/.codereview_upload_cookies > Issue created. URL: http://localhost:8080/5 > Uploading base file for index.yaml > Uploading base file for upload.py > andi [git:master+] rietveld$ ./upload.py -y -m "test" -s localhost:8080 HEAD > Got error status from ['git', 'show', 'HEAD:README2']: > > andi [git:master+] rietveld$ git status > # On branch master > # Changes to be committed: > # (use "git reset HEAD <file>..." to unstage) > # > # renamed: README -> README2 > # > # Changes not staged for commit: > # (use "git add <file>..." to update what will be committed) > # (use "git checkout -- <file>..." to discard changes in working directory) > # > # modified: index.yaml > # modified: upload.py > # > andi [git:master+] rietveld$
Sign in to reply to this message.
maruel@chromium.org writes: > The problem is that we use it quite differently: > > You assume that you want to diff against HEAD all the time. The > assumption is hidden in the fact that git show grabs always files from > HEAD. Even more, it doesn't use self.renames[filename] but directly > filename (!) > > We assume that you always want to diff against a branch/hash and that > all the changes are committed and nothing is staged or modified. Only > untracked files are ignored. > > I think the first workflow is untenable, otherwise how can you do > parallel work or pipeline multiple reviews one against the other? You > need to commit your changes to continue working so assuming that a diff > is done against HEAD isn't useful. Having uncommitted changes removes > all benefits from using git in the first place. I totally agree. When I upload uncomitted changes I mostly commit them after the upload to continue working. Or in daily work, I break all rules and upload a trivial change, commit it afterwads and push it to the main repository - but it's *always* embarrasing to commit changes from the review then in a second commit :) > > The code somewhat assumes that extra_args can be a hash to diff against > but is inconsistent. I'd rather enforce that it is always given and > len(extra_args) == 1. IMO this restriction for the git backend would make sense. My use case turned out to be just laziness, where always comparing against a branch/hash everytime is much more accurate. So, if no one comes up here on the list with a better use case for not comparing against a branch/hash. Let's add this extra_args check. -Andi > > M-A > > On 2011/04/01 12:26:31, Andi Albrecht wrote: >> How do you call git then? I've tested it locally, but upload.py > returned an >> error. Here's a transcript: > >> $ git mv README README2 >> andi [git:master] rietveld$ ./upload.py -y -m "test" -s localhost:8080 >> Upload server: localhost:8080 (change with -s/--server) >> Loaded authentication cookies from > /home/andi/.codereview_upload_cookies >> Issue created. URL: http://localhost:8080/5 >> Uploading base file for index.yaml >> Uploading base file for upload.py >> andi [git:master+] rietveld$ ./upload.py -y -m "test" -s > localhost:8080 HEAD >> Got error status from ['git', 'show', 'HEAD:README2']: > >> andi [git:master+] rietveld$ git status >> # On branch master >> # Changes to be committed: >> # (use "git reset HEAD <file>..." to unstage) >> # >> # renamed: README -> README2 >> # >> # Changes not staged for commit: >> # (use "git add <file>..." to update what will be committed) >> # (use "git checkout -- <file>..." to discard changes in working > directory) >> # >> # modified: index.yaml >> # modified: upload.py >> # >> andi [git:master+] rietveld$ > > > > http://codereview.appspot.com/4333051/
Sign in to reply to this message.
On 2011/04/02 05:55:18, Andi Albrecht wrote: > > The code somewhat assumes that extra_args can be a hash to diff against > > but is inconsistent. I'd rather enforce that it is always given and > > len(extra_args) == 1. > > IMO this restriction for the git backend would make sense. My use case > turned out to be just laziness, where always comparing against a > branch/hash everytime is much more accurate. > > So, if no one comes up here on the list with a better use case for not > comparing against a branch/hash. Let's add this extra_args check. I'm not sure about this since there's --rev which is used by GitVCS.GenerateDiff() but not by GitVCS.GetBaseFile() so there's a huge inconsistency there. So while I've committed that change into depot_tools, I'm not sure it's a good idea for the general case. The best thing would be to include a "FindUpstream()" function similar to what "git cl upstream" does so options.revision could be set to this value by default. I'll try to rip the git cl upstream code into its own python script so this could be reused, but that's not a high priority for me atm.
Sign in to reply to this message.
On 2011/04/19 17:10:52, M-A wrote: > On 2011/04/02 05:55:18, Andi Albrecht wrote: > > > The code somewhat assumes that extra_args can be a hash to diff against > > > but is inconsistent. I'd rather enforce that it is always given and > > > len(extra_args) == 1. > > > > IMO this restriction for the git backend would make sense. My use case > > turned out to be just laziness, where always comparing against a > > branch/hash everytime is much more accurate. > > > > So, if no one comes up here on the list with a better use case for not > > comparing against a branch/hash. Let's add this extra_args check. > > I'm not sure about this since there's --rev which is used by > GitVCS.GenerateDiff() but not by GitVCS.GetBaseFile() so there's a huge > inconsistency there. So while I've committed that change into depot_tools, I'm > not sure it's a good idea for the general case. > > The best thing would be to include a "FindUpstream()" function similar to what > "git cl upstream" does so options.revision could be set to this value by > default. I'll try to rip the git cl upstream code into its own python script so > this could be reused, but that's not a high priority for me atm. I'll commit this change in chromium branch for posterity to simplify my life when updating depot_tools. Because of the current inconsistencies and the limited amount of time I can invest in that, I'll postpone this change.
Sign in to reply to this message.
On 2011/05/05 16:49:07, M-A wrote: > On 2011/04/19 17:10:52, M-A wrote: > > On 2011/04/02 05:55:18, Andi Albrecht wrote: > > > > The code somewhat assumes that extra_args can be a hash to diff against > > > > but is inconsistent. I'd rather enforce that it is always given and > > > > len(extra_args) == 1. > > > > > > IMO this restriction for the git backend would make sense. My use case > > > turned out to be just laziness, where always comparing against a > > > branch/hash everytime is much more accurate. > > > > > > So, if no one comes up here on the list with a better use case for not > > > comparing against a branch/hash. Let's add this extra_args check. > > > > I'm not sure about this since there's --rev which is used by > > GitVCS.GenerateDiff() but not by GitVCS.GetBaseFile() so there's a huge > > inconsistency there. So while I've committed that change into depot_tools, I'm > > not sure it's a good idea for the general case. > > > > The best thing would be to include a "FindUpstream()" function similar to what > > "git cl upstream" does so options.revision could be set to this value by > > default. I'll try to rip the git cl upstream code into its own python script > so > > this could be reused, but that's not a high priority for me atm. > > I'll commit this change in chromium branch for posterity to simplify my life > when updating depot_tools. Because of the current inconsistencies and the > limited amount of time I can invest in that, I'll postpone this change. Hi there, If I want to enjoy the benefit of this patch, shall I simply use the copy of upload.py on chromium branch [at http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/third_party/upl...
Sign in to reply to this message.
On Wed, May 25, 2011 at 12:07 PM, <mr.kschan@gmail.com> wrote: > On 2011/05/05 16:49:07, M-A wrote: >> >> On 2011/04/19 17:10:52, M-A wrote: >> > On 2011/04/02 05:55:18, Andi Albrecht wrote: >> > > > The code somewhat assumes that extra_args can be a hash to diff > > against >> >> > > > but is inconsistent. I'd rather enforce that it is always given > > and >> >> > > > len(extra_args) == 1. >> > > >> > > IMO this restriction for the git backend would make sense. My use > > case >> >> > > turned out to be just laziness, where always comparing against a >> > > branch/hash everytime is much more accurate. >> > > >> > > So, if no one comes up here on the list with a better use case for > > not >> >> > > comparing against a branch/hash. Let's add this extra_args check. >> > >> > I'm not sure about this since there's --rev which is used by >> > GitVCS.GenerateDiff() but not by GitVCS.GetBaseFile() so there's a > > huge >> >> > inconsistency there. So while I've committed that change into > > depot_tools, I'm >> >> > not sure it's a good idea for the general case. >> > >> > The best thing would be to include a "FindUpstream()" function > > similar to what >> >> > "git cl upstream" does so options.revision could be set to this > > value by >> >> > default. I'll try to rip the git cl upstream code into its own > > python script >> >> so >> > this could be reused, but that's not a high priority for me atm. > >> I'll commit this change in chromium branch for posterity to simplify > > my life >> >> when updating depot_tools. Because of the current inconsistencies and > > the >> >> limited amount of time I can invest in that, I'll postpone this > > change. > > Hi there, > > If I want to enjoy the benefit of this patch, shall I simply use the > copy of upload.py on chromium branch [at > http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/third_party/upl... I'm fine with this change going into trunk. -Andi > > > http://codereview.appspot.com/4333051/ >
Sign in to reply to this message.
Reviving a very old CL. I had committed it to the chromium branch but not in default, I don't recall why. It's almost the same as what is in the chromium branch, albeit with 80 cols fixed and better description and the addition of the use of -C -C.
Sign in to reply to this message.
http://codereview.appspot.com/4333051/diff/12002/upload.py File upload.py (right): http://codereview.appspot.com/4333051/diff/12002/upload.py#newcode338 upload.py:338: break Not by your change, but this break could be removed :) (just a reminder...) http://codereview.appspot.com/4333051/diff/12002/upload.py#newcode1310 upload.py:1310: diff += RunShell(cmd + ["-C", "-C", "--diff-filter=ACMRT"] + extra_args, What about "--find-copies-harder" instead of two "-C". That would make it easier to understand that it's not a typo - and to avoid "read the man page" responses :)
Sign in to reply to this message.
ptal http://codereview.appspot.com/4333051/diff/12002/upload.py File upload.py (right): http://codereview.appspot.com/4333051/diff/12002/upload.py#newcode338 upload.py:338: break On 2012/06/12 07:24:53, Andi Albrecht wrote: > Not by your change, but this break could be removed :) (just a reminder...) I did it initially but upon re-reading, the break is not a no-op, it's actually returning a valid credential, so the break is to stop from looping. I don't think this should be removed. In any case, I'd prefer not do it in this CL. http://codereview.appspot.com/4333051/diff/12002/upload.py#newcode1310 upload.py:1310: diff += RunShell(cmd + ["-C", "-C", "--diff-filter=ACMRT"] + extra_args, On 2012/06/12 07:24:53, Andi Albrecht wrote: > What about "--find-copies-harder" instead of two "-C". That would make it easier > to understand that it's not a typo - and to avoid "read the man page" responses > :) I preferred two -C but I don't mind. Chase used --find-copies-harder too so I'll comply. :)
Sign in to reply to this message.
LGTM http://codereview.appspot.com/4333051/diff/12002/upload.py File upload.py (right): http://codereview.appspot.com/4333051/diff/12002/upload.py#newcode338 upload.py:338: break On 2012/06/12 16:36:41, M-A wrote: > On 2012/06/12 07:24:53, Andi Albrecht wrote: > > Not by your change, but this break could be removed :) (just a reminder...) > > I did it initially but upon re-reading, the break is not a no-op, it's actually > returning a valid credential, so the break is to stop from looping. I don't > think this should be removed. In any case, I'd prefer not do it in this CL. oh, you're right. I've missed the outer loop!
Sign in to reply to this message.
|
