Log git diff during build

[dogboydog]

• Issue git --no-pager diff if logDiffifDirty is specified for a build, whether or not you allow dirty builds.
• I know "DiffIf" looks a little hard to read. I can refactor it to a different name if you have a better idea.
• allowDirtyBuild was missing from action.yml. I don't know if this was intentional, but I added it with the information from README.md
• basic mocking tests added

Working screenshot:

Side note... any idea why ProjectSettings.asset is being modified during the build? I am using the same Unity version in the branch as I am specifying to the Unity Actions. It seems like the defaults used by unity-builder override the ProjectSettings. I wonder if you should ignore "dirtiness" if the only dirty file is ProjectSettings if that's the case.

Fix #117

[codecov]

Codecov Report

Merging #119 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   95.27%   95.37%   +0.10%     
==========================================
  Files          15       15              
  Lines         275      281       +6     
  Branches       58       58              
==========================================
+ Hits          262      268       +6     
  Misses         13       13              

Impacted Files	Coverage Δ
src/model/versioning.js	100.00% <100.00%> (ø)

[webbertakken]

Very nice first PR @dogboydog

I think this is a very nice first iteration and I would merge it if it didn't expose new parameters. As the goal for this project is to keep things as simple as possible for everyone and have as much functionality as possible work out of the box, we might need to make some small changes before merging it.

I think we should come to some common sense amount of files or lines of which we'd show the diff and that way prevent having to add another parameter to the workflow.

My suggestion would be to always show the first 20~100 lines of diff across files.

To answer your points:

• Issue git --no-pager diff if logDiffifDirty is specified for a build, whether or not you allow dirty builds.

I like it!

• I know "DiffIf" looks a little hard to read. I can refactor it to a different name if you have a better idea.

This would be prevented too by the suggestion, but otherwise i'd probably have suggested something like logDifference or logDiff (as the idea was not always log it either way).

• allowDirtyBuild was missing from action.yml. I don't know if this was intentional, but I added it with the information from README.md

This was not intentional. Thank you for coding inclusively.

[dogboydog]

Thanks.

I think we should come to some common sense amount of files or lines of which we'd show the diff and that way prevent having to add another parameter to the workflow.

My suggestion would be to always show the first 20~100 lines of diff across files.

Okay, sure. So when you say across files, do you mean 20-100 lines total? Not that many lines per file? Would this number be configurable (like via an environmental variable) or just a constant?

This would be prevented too by the suggestion, but otherwise i'd probably have suggested something like logDifference or logDiff (as the idea was not always log it either way).

Now that you mention it, the diff would just be blank if the repository were not dirty, so just logDiff would still seem to explain the concept to me and be much easier to read.

[webbertakken]

Okay, sure. So when you say across files, do you mean 20-100 lines total? Not that many lines per file? Would this number be configurable (like via an environmental variable) or just a constant?

Yes I meant in total, because we have to weight log pollution vs ease of use.

I wouldn't make this number configurable, at least not yet. For your use case this solves the problem and I think it will for most other cases where only 1 or 2 files change too. Making things variable would only make it harder to maintain, so I'd suggest we wait for a valid use case for it.

[dogboydog]

Cool. My plan is to refactor the option to be "logDiff", and limit it to whatever number of lines you want. 60?

[webbertakken]

Cool. My plan is to refactor the option to be "logDiff", and limit it to whatever number of lines you want. 60?

I still think we shouldn't expose an extra parameter but just enable this functionality by default.

[dogboydog]

Oh I see. I misread your first message, sorry. Okay I will do it unconditionally and remove the parameter.

[dogboydog]

Okay, I have your suggestions working. Diff lines are capped at 60 for now. Let me know what you think. Thanks for the feedback.

Beginning of diff:

End of diff:

I can move these questions to a different issue if you want, but I was curious about a few things.

• I have working code (pretty simple) to read m_EditorVersion from ProjectSettings\ProjectVersion.txt. I'm using this as the input to with: unityVersion. Something like this could be the default if unityVersion is not specified. Any thoughts on that? Seems to be in keeping with the goal of having things work out of the box, but maybe you want to avoid parsing this file? Maybe in production people want to use a constant version to build binaries, but you'd think they'd be developing with that version too, right? It looks like the ProjectVersion.txt file has been around for a while, and is automatically updated by Unity when you upgrade. Just one idea

• Should there be certain files excluded from consideration for "dirtiness"? I'm not sure how I can avoid ProjectSettings.asset being modified during the build since I am just using the default settings for the action. Although depending on the buildMethod specified, maybe other files like .meta files could change. I can just keep using allowDirtyBuild for the moment either way though.

[webbertakken]

Perfect. Thanks you for the nice cleanup!

[webbertakken]

I can move these questions to a different issue if you want

Yes I think that would be better

[webbertakken]

#95