Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug when passing more than one file to changes #64

Conversation

msilvestre
Copy link

What does this implement/fix? Explain your changes.

When passing more than one file to changes it only assumes the first one.

Does this close any currently open issues?

No

Any relevant logs, error output, etc?

(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)

Any other comments?

No

Where has this been tested?

Operating system: Mac OS 10.11.6

@sobolevn
Copy link
Owner

sobolevn commented Feb 18, 2017

Thanks! That's a valuable contribution, I appreciate it.

Could you please consider some moments:

  1. I have tried to run your code, it works perfectly. But one thing caught my eye. Here's the log:

You need a passphrase to unlock the secret key for
user: "Nikita Sobolev (keybase) [email protected]"
2048-bit RSA key, ID 3F5E2521, created 2016-03-18 (main key ID 8AE3C73E)

changes in one.txt: 1c1,3
< Test

CONFIG = 1
DEBUG = 1
TEST = 0

You need a passphrase to unlock the secret key for
user: "Nikita Sobolev (keybase) [email protected]"
2048-bit RSA key, ID 3F5E2521, created 2016-03-18 (main key ID 8AE3C73E)

changes in two.txt: 1c1
< Different stuff

Different totelly stuff

These blocks of text produces by gpg is breaking diff blocks apart. This issue is not caused by your changes of course. But, maybe we could do something about it? I guess you have to look at _decrypt command. And redirect output from the gpg command.

  1. Add a test case for this functionality. It will be quite similar to @test "run 'changes' with multiple files changed"

  2. Fix linting:

In src/commands/git_secret_changes.sh line 21:
local filenames="$@"
^-- SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
Link: https://travis-ci.org/sobolevn/git-secret/jobs/202725065

It is fully described how to use shellcheck in https:/sobolevn/git-secret/blob/master/CONTRIBUTING.md

  1. Update docs in man folder.

Nice work!

@msilvestre
Copy link
Author

I am not familiar with this bash test tool. From my point of view the testa @test "run 'changes' with multiple files changed" is wrong, it should not be passing at all. Other proof that it is wrong is, with my change it now should be failing and it is not.

I don't have many time to work in this, but I will give it a try.

@msilvestre msilvestre closed this Feb 21, 2017
@msilvestre
Copy link
Author

Ups, I closed this by mistake.. Sorry

@msilvestre msilvestre reopened this Feb 21, 2017
@sobolevn
Copy link
Owner

This test seems good to me.

The logic behind it is simple: we scan for all changes in all files added to the git-secret.

Your logic should not break it.

@msilvestre
Copy link
Author

You are right, for some reason I though you were passing the files through the command line.

I have made a new push with shellcheck and the added test. I didn't change the man pages, I think it does not need any change, or what should be changed?

Thanks.

@sobolevn
Copy link
Owner

I will merge and fix it, since I want to make a release today.

Thanks again!

@sobolevn sobolevn merged commit acdcb45 into sobolevn:develop Feb 25, 2017
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