Bug 138373 – Add the ability to search for modifications that are staged for commit in git.py

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Matthew Hanson]

Adding a modifications_staged_for_commit method to Git helps to support better merging workflows.

[Matthew Hanson]

Created attachment 240951 [details]
Patch to add a method for finding modified files staged for commit

[Daniel Bates]

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.

[Matthew Hanson]

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

[Matthew Hanson]

Created attachment 241000 [details]
Updated patch for getting modified files staged for commit.

[Ryosuke Niwa]

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.

[Matthew Hanson]

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.

[Daniel Bates]

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>.

[Daniel Bates]

Patch was committed in <http://trac.webkit.org/changeset/175604>