Summary: | Add the ability to search for modifications that are staged for commit in git.py | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matthew Hanson <matthew_hanson> | ||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, dbates, ddkilzer, glenn, matthew_hanson, rniwa | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Matthew Hanson
2014-11-04 14:14:14 PST
Created attachment 240951 [details]
Patch to add a method for finding modified files staged for commit
Comment on attachment 240951 [details] 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. Comment on attachment 240951 [details] 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 Created attachment 241000 [details]
Updated patch for getting modified files staged for commit.
Comment on attachment 241000 [details] 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. Comment on attachment 241000 [details] 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. Comment on attachment 241000 [details] 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>. Patch was committed in <http://trac.webkit.org/changeset/175604> |