138373 2014-11-04 14:14:14 -0800 Add the ability to search for modifications that are staged for commit in git.py 2014-11-04 20:55:32 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 matthew_hanson webkit-unassigned commit-queue dbates ddkilzer glenn matthew_hanson rniwa oldest_to_newest 1046173 0 matthew_hanson 2014-11-04 14:14:14 -0800 Adding a modifications_staged_for_commit method to Git helps to support better merging workflows. 1046187 1 240951 matthew_hanson 2014-11-04 14:49:59 -0800 Created attachment 240951 Patch to add a method for finding modified files staged for commit 1046358 2 240951 dbates 2014-11-04 19:49:59 -0800 Comment on attachment 240951 Patch to add a method for finding modified files staged for commit View in context: https://bugs.webkit.org/attachment.cgi?id=240951&action=review > Tools/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). Nit: The Reviewed by line should come after the bug URL. An example of this can be seen in the change log entry for <http://trac.webkit.org/changeset/175601>. > Tools/Scripts/webkitpy/common/checkout/scm/git.py:213 > + def modifications_staged_for_commit(self): > + status_command = [self.executable_name, 'status'] > + modified_regexp = '^\s*modified:\s+(?P<filename>.+)\s*$' > + return self.run_status_and_extract_filenames(status_command, modified_regexp) Consider the following output from git status: [[ plasma:gitTest dbates$ git status On branch master Changes to be committed: (use "git reset HEAD <file>..." to unstage) modified: t1 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: t2 ]] Then this function will return the array ["t1", "t2"]. That is, this function will return the filenames of modified files that are staged for commit ("Changes to be committed") and not staged for commit. 1046363 3 240951 matthew_hanson 2014-11-04 20:18:13 -0800 Comment on attachment 240951 Patch to add a method for finding modified files staged for commit View in context: https://bugs.webkit.org/attachment.cgi?id=240951&action=review Thanks for the feedback. New patch coming shortly. >> Tools/ChangeLog:9 >> + Reviewed by NOBODY (OOPS!). > > Nit: The Reviewed by line should come after the bug URL. An example of this can be seen in the change log entry for <http://trac.webkit.org/changeset/175601>. Noted and fixed. >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:213 >> + return self.run_status_and_extract_filenames(status_command, modified_regexp) > > Consider the following output from git status: > > [[ > plasma:gitTest dbates$ git status > On branch master > Changes to be committed: > (use "git reset HEAD <file>..." to unstage) > > modified: t1 > > 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: t2 > ]] > > Then this function will return the array ["t1", "t2"]. That is, this function will return the filenames of modified files that are staged for commit ("Changes to be committed") and not staged for commit. Absolutely right. I am going to post another patch that uses the short-format defined in http://git-scm.com/docs/git-status 1046364 4 241000 matthew_hanson 2014-11-04 20:19:31 -0800 Created attachment 241000 Updated patch for getting modified files staged for commit. 1046370 5 241000 rniwa 2014-11-04 20:44:13 -0800 Comment on attachment 241000 Updated patch for getting modified files staged for commit. View in context: https://bugs.webkit.org/attachment.cgi?id=241000&action=review > Tools/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description but after the bug URL. 1046371 6 241000 matthew_hanson 2014-11-04 20:48:56 -0800 Comment on attachment 241000 Updated patch for getting modified files staged for commit. View in context: https://bugs.webkit.org/attachment.cgi?id=241000&action=review Thanks! >> Tools/ChangeLog:9 >> + Reviewed by NOBODY (OOPS!). > > This line should appear before the long description but after the bug URL. Indeed it should. 1046373 7 241000 dbates 2014-11-04 20:55:07 -0800 Comment on attachment 241000 Updated patch for getting modified files staged for commit. View in context: https://bugs.webkit.org/attachment.cgi?id=241000&action=review I know that this patch was already reviewed and landed. I had some questions about implementation, unit tests, and usage of this code. Currently modifications_staged_for_commit() is unused. I take it you plan to make use of this functionality in a follow up patch. Usually it's preferred to add new code together with a call site both to help the reviewer better understand the goal of the patch as well and to ensure that the added code doesn't inadvertently remain unused (say, if future plans change). > Tools/Scripts/webkitpy/common/checkout/scm/git.py:210 > + def modifications_staged_for_commit(self): Maybe a better name for this function would be modified_files_staged_for_commit? This would make the name of this function more closely match the name of other functions (e.g. changed_files). > Tools/Scripts/webkitpy/common/checkout/scm/git.py:215 > + # This will only return non-deleted files with the "updated in index" status > + # as defined by http://git-scm.com/docs/git-status. > + status_command = [self.executable_name, 'status', '--short'] > + updated_in_index_regexp = '^M[ M] (?P<filename>.+)$' > + return self.run_status_and_extract_filenames(status_command, updated_in_index_regexp) I didn't have a chance to read <http://git-scm.com/docs/git-status> yet. I take it that it doesn't make sense to write this function as: def modified_files_staged_for_commit(self): status_command = [self.executable_name, "diff", "--name-status", "--no-renames", "--cached"] return self.run_status_and_extract_filenames(status_command, self._status_regexp("M")) ? If it does not make sense to implement this function as mentioned above then I suggest we add a unit test for this function. See file <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/scm/scm_unittest.py>. 1046374 8 dbates 2014-11-04 20:55:32 -0800 Patch was committed in <http://trac.webkit.org/changeset/175604> 240951 2014-11-04 14:49:59 -0800 2014-11-04 20:19:31 -0800 Patch to add a method for finding modified files staged for commit 0002-Add-the-ability-to-search-for-modifications-that-are.patch text/plain 2437 matthew_hanson RnJvbSBkZDc0NzI5OTdmYWY2ZGQ1NmJmYzM0YTA3YzFhNWU2NTE2ZTk5NDYyIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXR0aGV3IEhhbnNvbiA8bWF0dGhld19oYW5zb25AYXBwbGUu Y29tPgpEYXRlOiBUdWUsIDQgTm92IDIwMTQgMTQ6MTY6MTkgLTA4MDAKU3ViamVjdDogW1BBVENI IDIvNF0gQWRkIHRoZSBhYmlsaXR5IHRvIHNlYXJjaCBmb3IgbW9kaWZpY2F0aW9ucyB0aGF0IGFy ZQogc3RhZ2VkIGZvciBjb21taXQuIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMzgzNzMKClRoaXMgbWFrZXMgaXMgZWFzeSB0byBkbyBpZGVudGlmeSBmaWxlcyB0aGF0 IHJlcXVpcmUgbWVyZ2UgcG9zdC1wcm9jZXNzaW5nLAppbiBhZGRpdGlvbiB0byBzdGFuZC1hbG9u ZSB1dGlsaXR5LgoKUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCgoqIFNjcmlwdHMvd2Via2l0 cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHk6CihHaXQubW9kaWZpY2F0aW9uc19zdGFnZWRf Zm9yX2NvbW1pdCk6IEFkZGVkLgotLS0KIFRvb2xzL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgfCAxMyArKysrKysrKysrKysrCiBUb29scy9TY3JpcHRzL3dlYmtp dHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0LnB5IHwgIDUgKysrKysKIDIgZmlsZXMgY2hhbmdl ZCwgMTggaW5zZXJ0aW9ucygrKQoKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xz L0NoYW5nZUxvZwppbmRleCA1YmE1MzRmLi45MDVkMTlkIDEwMDY0NAotLS0gYS9Ub29scy9DaGFu Z2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMTggQEAKIDIwMTQtMTEtMDQg IE1hdHRoZXcgSGFuc29uICA8bWF0dGhld19oYW5zb25AYXBwbGUuY29tPgogCisgICAgICAgIEFk ZCB0aGUgYWJpbGl0eSB0byBzZWFyY2ggZm9yIG1vZGlmaWNhdGlvbnMgdGhhdCBhcmUgc3RhZ2Vk IGZvciBjb21taXQuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMzgzNzMKKworICAgICAgICBUaGlzIG1ha2VzIGlzIGVhc3kgdG8gZG8gaWRlbnRpZnkg ZmlsZXMgdGhhdCByZXF1aXJlIG1lcmdlIHBvc3QtcHJvY2Vzc2luZywKKyAgICAgICAgaW4gYWRk aXRpb24gdG8gc3RhbmQtYWxvbmUgdXRpbGl0eS4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0 L3NjbS9naXQucHk6CisgICAgICAgIChHaXQubW9kaWZpY2F0aW9uc19zdGFnZWRfZm9yX2NvbW1p dCk6IEFkZGVkLgorCisyMDE0LTExLTA0ICBNYXR0aGV3IEhhbnNvbiAgPG1hdHRoZXdfaGFuc29u QGFwcGxlLmNvbT4KKwogICAgICAgICBBZGQgYSBicmFuY2gga2V5d29yZCBhcmd1bWVudCB0byBf cnVuX2dpdF9zdm5fZmluZF9yZXYuCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD0xMzgzNzIKIApkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRw eS9jb21tb24vY2hlY2tvdXQvc2NtL2dpdC5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29t bW9uL2NoZWNrb3V0L3NjbS9naXQucHkKaW5kZXggZGUxNjNkZi4uYWU1M2NkNyAxMDA2NDQKLS0t IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvc2NtL2dpdC5weQorKysg Yi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0LnB5CkBAIC0y MDcsNiArMjA3LDExIEBAIGNsYXNzIEdpdChTQ00sIFNWTlJlcG9zaXRvcnkpOgogCiAgICAgICAg IHJldHVybiBzZWxmLnJlbW90ZV9tZXJnZV9iYXNlKCkKIAorICAgIGRlZiBtb2RpZmljYXRpb25z X3N0YWdlZF9mb3JfY29tbWl0KHNlbGYpOgorICAgICAgICBzdGF0dXNfY29tbWFuZCA9IFtzZWxm LmV4ZWN1dGFibGVfbmFtZSwgJ3N0YXR1cyddCisgICAgICAgIG1vZGlmaWVkX3JlZ2V4cCA9ICde XHMqbW9kaWZpZWQ6XHMrKD9QPGZpbGVuYW1lPi4rKVxzKiQnCisgICAgICAgIHJldHVybiBzZWxm LnJ1bl9zdGF0dXNfYW5kX2V4dHJhY3RfZmlsZW5hbWVzKHN0YXR1c19jb21tYW5kLCBtb2RpZmll ZF9yZWdleHApCisKICAgICBkZWYgY2hhbmdlZF9maWxlcyhzZWxmLCBnaXRfY29tbWl0PU5vbmUp OgogICAgICAgICAjIEZJWE1FOiAtLWRpZmYtZmlsdGVyIGNvdWxkIGJlIHVzZWQgdG8gYXZvaWQg dGhlICJleHRyYWN0X2ZpbGVuYW1lcyIgc3RlcC4KICAgICAgICAgc3RhdHVzX2NvbW1hbmQgPSBb c2VsZi5leGVjdXRhYmxlX25hbWUsICdkaWZmJywgJy1yJywgJy0tbmFtZS1zdGF0dXMnLCAiLS1u by1yZW5hbWVzIiwgIi0tbm8tZXh0LWRpZmYiLCAiLS1mdWxsLWluZGV4Iiwgc2VsZi5tZXJnZV9i YXNlKGdpdF9jb21taXQpXQotLSAKMi4xLjEgKEFwcGxlIEdpdC01MSkKCg== 241000 2014-11-04 20:19:31 -0800 2014-11-04 20:44:13 -0800 Updated patch for getting modified files staged for commit. 0002-Add-the-ability-to-search-for-modifications-that-are.patch text/plain 2601 matthew_hanson RnJvbSAxNmNjNTQzNzVhMDdmNTI1ZTk1MzU4MzJlMzg4OWMyNDc1NDc1ZTk2IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXR0aGV3IEhhbnNvbiA8bWF0dGhld19oYW5zb25AYXBwbGUu Y29tPgpEYXRlOiBUdWUsIDQgTm92IDIwMTQgMTQ6MTY6MTkgLTA4MDAKU3ViamVjdDogW1BBVENI IDIvNF0gQWRkIHRoZSBhYmlsaXR5IHRvIHNlYXJjaCBmb3IgbW9kaWZpY2F0aW9ucyB0aGF0IGFy ZQogc3RhZ2VkIGZvciBjb21taXQuIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMzgzNzMKClRoaXMgbWFrZXMgaXMgZWFzeSB0byBkbyBpZGVudGlmeSBmaWxlcyB0aGF0 IHJlcXVpcmUgbWVyZ2UgcG9zdC1wcm9jZXNzaW5nLAppbiBhZGRpdGlvbiB0byBzdGFuZC1hbG9u ZSB1dGlsaXR5LgoKUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCgoqIFNjcmlwdHMvd2Via2l0 cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHk6CihHaXQubW9kaWZpY2F0aW9uc19zdGFnZWRf Zm9yX2NvbW1pdCk6IEFkZGVkLgotLS0KIFRvb2xzL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgfCAxMyArKysrKysrKysrKysrCiBUb29scy9TY3JpcHRzL3dlYmtp dHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0LnB5IHwgIDcgKysrKysrKwogMiBmaWxlcyBjaGFu Z2VkLCAyMCBpbnNlcnRpb25zKCspCgpkaWZmIC0tZ2l0IGEvVG9vbHMvQ2hhbmdlTG9nIGIvVG9v bHMvQ2hhbmdlTG9nCmluZGV4IDJjNGMyMDguLjhjMDNmZGQgMTAwNjQ0Ci0tLSBhL1Rvb2xzL0No YW5nZUxvZworKysgYi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEsNSArMSwxOCBAQAogMjAxNC0xMS0w NCAgTWF0dGhldyBIYW5zb24gIDxtYXR0aGV3X2hhbnNvbkBhcHBsZS5jb20+CiAKKyAgICAgICAg QWRkIHRoZSBhYmlsaXR5IHRvIHNlYXJjaCBmb3IgbW9kaWZpY2F0aW9ucyB0aGF0IGFyZSBzdGFn ZWQgZm9yIGNvbW1pdC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTEzODM3MworCisgICAgICAgIFRoaXMgbWFrZXMgaXMgZWFzeSB0byBkbyBpZGVudGlm eSBmaWxlcyB0aGF0IHJlcXVpcmUgbWVyZ2UgcG9zdC1wcm9jZXNzaW5nLAorICAgICAgICBpbiBh ZGRpdGlvbiB0byBzdGFuZC1hbG9uZSB1dGlsaXR5LgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tv dXQvc2NtL2dpdC5weToKKyAgICAgICAgKEdpdC5tb2RpZmljYXRpb25zX3N0YWdlZF9mb3JfY29t bWl0KTogQWRkZWQuCisKKzIwMTQtMTEtMDQgIE1hdHRoZXcgSGFuc29uICA8bWF0dGhld19oYW5z b25AYXBwbGUuY29tPgorCiAgICAgICAgIEFkZCBhIGJyYW5jaCBrZXl3b3JkIGFyZ3VtZW50IHRv IF9ydW5fZ2l0X3N2bl9maW5kX3Jldi4KICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEzODM3MgogCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtp dHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0LnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9j b21tb24vY2hlY2tvdXQvc2NtL2dpdC5weQppbmRleCBkZTE2M2RmLi4xNzc1ZmUzIDEwMDY0NAot LS0gYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0LnB5Cisr KyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHkKQEAg LTIwNyw2ICsyMDcsMTMgQEAgY2xhc3MgR2l0KFNDTSwgU1ZOUmVwb3NpdG9yeSk6CiAKICAgICAg ICAgcmV0dXJuIHNlbGYucmVtb3RlX21lcmdlX2Jhc2UoKQogCisgICAgZGVmIG1vZGlmaWNhdGlv bnNfc3RhZ2VkX2Zvcl9jb21taXQoc2VsZik6CisgICAgICAgICMgVGhpcyB3aWxsIG9ubHkgcmV0 dXJuIG5vbi1kZWxldGVkIGZpbGVzIHdpdGggdGhlICJ1cGRhdGVkIGluIGluZGV4IiBzdGF0dXMK KyAgICAgICAgIyBhcyBkZWZpbmVkIGJ5IGh0dHA6Ly9naXQtc2NtLmNvbS9kb2NzL2dpdC1zdGF0 dXMuCisgICAgICAgIHN0YXR1c19jb21tYW5kID0gW3NlbGYuZXhlY3V0YWJsZV9uYW1lLCAnc3Rh dHVzJywgJy0tc2hvcnQnXQorICAgICAgICB1cGRhdGVkX2luX2luZGV4X3JlZ2V4cCA9ICdeTVsg TV0gKD9QPGZpbGVuYW1lPi4rKSQnCisgICAgICAgIHJldHVybiBzZWxmLnJ1bl9zdGF0dXNfYW5k X2V4dHJhY3RfZmlsZW5hbWVzKHN0YXR1c19jb21tYW5kLCB1cGRhdGVkX2luX2luZGV4X3JlZ2V4 cCkKKwogICAgIGRlZiBjaGFuZ2VkX2ZpbGVzKHNlbGYsIGdpdF9jb21taXQ9Tm9uZSk6CiAgICAg ICAgICMgRklYTUU6IC0tZGlmZi1maWx0ZXIgY291bGQgYmUgdXNlZCB0byBhdm9pZCB0aGUgImV4 dHJhY3RfZmlsZW5hbWVzIiBzdGVwLgogICAgICAgICBzdGF0dXNfY29tbWFuZCA9IFtzZWxmLmV4 ZWN1dGFibGVfbmFtZSwgJ2RpZmYnLCAnLXInLCAnLS1uYW1lLXN0YXR1cycsICItLW5vLXJlbmFt ZXMiLCAiLS1uby1leHQtZGlmZiIsICItLWZ1bGwtaW5kZXgiLCBzZWxmLm1lcmdlX2Jhc2UoZ2l0 X2NvbW1pdCldCi0tIAoyLjEuMSAoQXBwbGUgR2l0LTUxKQoK