Skip to content

Conversation

zen1t
Copy link
Contributor

@zen1t zen1t commented Jun 26, 2019

@olivertappin olivertappin self-requested a review July 24, 2019 13:47
// Get a list of changed files (not including deleted files)
$output = shell_exec(
'git diff ' . $this->baseBranch . ' ' . $this->currentBranch . ' --name-only --diff-filter=d'
'git diff ' . $this->baseBranch . ' ' . $this->currentBranch . ' --name-only --diff-filter=ACM'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! 🎉

For reference (mainly for others looking at this pull-request), here's the except from the manual which directly relate to these changes:

--diff-filter=[(A|C|D|M|R|T|U|X|B)...[*]]

Select only files that are Added (A), Copied (C), Deleted (D), Modified (M), Renamed (R), have their type (i.e. regular file, symlink, submodule, ...) changed (T), are Unmerged (U), are Unknown
(X), or have had their pairing Broken (B). Any combination of the filter characters (including none) can be used. When * (All-or-none) is added to the combination, all paths are selected if
there is any file that matches other criteria in the comparison; if there is no file that matches other criteria, nothing is selected.

Also, these upper-case letters can be downcased to exclude. E.g.  --diff-filter=ad excludes added and deleted paths.

Note that not all diffs can feature all types. For instance, diffs from the index to the working tree can never have Added entries (because the set of paths included in the diff is limited by
what is in the index). Similarly, copied and renamed entries cannot appear if detection for those types is disabled.

Tthe use of lowercase characters was introduced in v1.8.5 (although they didn't document it). You can see the change added here: git/git@7f2ea5f. So this change will support version v1.8.4 and below of git.

As well as supporting the lower versions, it will also slightly change the behaviour. Since we are currently removing deleted files, the new change would suggest we are now filtering out all other available options. More specifically, we would not be showing files covered by options: R, T, U, X and B:

R - Renamed
T - have their type (mode) changed
U - Unmerged
X - Unknown
B - have had their pairing Broken

I'm more than happy to have these removed as part of this change request, since they don't impact the check of code style within a diff.

This is more of a question of whether you're aware of the changes you've made, the impact it will have to this project and that you're happy with that. If so, I'm happy to approve and merge.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since two weeks has gone by with no reply, I'm going to merge this in anyway, as I'm happy with the proposed changes. If you have any questions about my comments above, please don't hesitate to ask.

Thank you again for contributing. ❤️

@olivertappin olivertappin merged commit 8b9130e into olivertappin:master Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants