Fix bug where dirty submodules broke hash generation

[mattkelly]

This is a proposal for a fix for #685. Please let me know what you think of the approach! I'm new to the Skaffold code, so hopefully I'm not misunderstanding how this should work.

Regarding splitting the status lines: ?? isn't the only special case (two characters). MM is also a valid status, for example. If my understanding of the purpose of that code was correct, then my new implementation should be more robust. Ideally I would have split this out as a separate commit, but it's pretty tangled in there.

---

Instead of appending the entire contents of modified files to the sha256
input, we should only append the diff as computed by git diff in order
to handle all modifications properly, including submodules.

This also fixes a bug where the processing of the git status output was
too fragile. It makes it more generic by splitting the status string on
fields instead.

Signed-off-by: Matt Kelly matt.kelly@containership.io

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[mattkelly]

Whoops, totally missed that there are tests that need fixing now.

Along those lines, this approach needs some tweaking if it's going to have reproducible results. This is because the default git diff output includes some index-dependent headers. For example:

diff --git a/pkg/skaffold/build/tag/git_commit.go b/pkg/skaffold/build/tag/git_commit.go
index ad5d08c..4bbed3f 100644 // this is a problem
--- a/pkg/skaffold/build/tag/git_commit.go
+++ b/pkg/skaffold/build/tag/git_commit.go
<actual diff contents follow>

We should really only hash the diff content itself. That still might be a problem for git diff on submodules, though, because the diff content looks like this:

-Subproject commit e82a80da031152a55fed6fec2697c6ab2961f27b
+Subproject commit b75d42f81c22009fb9194592c87e6c606fb7a7c6

Before I go off and change the diff command and fix the tests (for non-submodules at least), what do people think of this approach in general given these considerations?

[dlorenc]

This makes sense overall to me. Will you be able to sign the CLA?

[mattkelly]

@dlorenc yup, sorry - I started getting that sorted out but now I'm traveling for a few days. I'll have the CLA signed and the tests fixed by Monday. Thanks!

[googlebot]

CLAs look good, thanks!

[mattkelly]

Fixed tests and rebased / fixed conflicts to incorporate b8cfc37#diff-7b47f8007b648de24c6b5c948a869dd6R171 properly. Two notes:

1. I decided to leave the git command git diff as it is. Now that I understand how the tests work (and commit hash generation) at a deeper level, I realized that having specific hashes in the diff is fine.

2. I removed the change to use strings.Fields() to separate the status code from the file path. The reason is that the XY form of git status codes can actually start with a blank (for example M). strings.Fields() would miss the blank in this case. That said, I do still think that the existing implementation is fragile/broken because there are many possible two-character status codes - none of which are checked for except for ??. A bigger problem is that if we change the code to properly support XY codes, then we break a lot of unit tests because go-git (AFAICT) only has support for single character status codes. So in general I think that this should be solved under a new issue because it's a nontrivial change.

[dgageot]

Thanks @mattkelly I'm going to merge your fix. However, in #714, I stop computing a sha256 of file changes because of the reasons you mention. Instead I use the imageID when the repo is dirty.