elasticdog / transcrypt

transparently encrypt files within a git repository
MIT License
1.44k stars 102 forks source link

transcrypt ignores the `git config core.hooksPath` setting #104

Closed ljm42 closed 2 years ago

ljm42 commented 3 years ago

By default, git runs hooks from the .git/hooks directory, but it allows for the directory to be adjusted through the core.hooksPath setting, like this: git config core.hooksPath "/some/other/path"

The PR below updates transcrypt to use the core.hooksPath setting if it exists, otherwise it defaults to .git/hooks

This change should be transparent, except that I did add the GIT_HOOKS directory to the --display output. If this is unwanted, the additional output could be suppressed when it contains the default value.

In my case I need to go a step further and change the location of the crypt/[clean|smudge|textconv|merge] files as well. The reason is that my git repo is on a drive that is mounted without execute perms, so git can't execute files stored in .git/[hooks|crypt]

If you are open to it, I will submit another PR that checks whether core.hooksPath is set. If so, it would create the crypt folder in core.hooksPath. Otherwise, keep it where it is (.git/crypt). Does that sound reasonable?

jmurty commented 3 years ago

Hi @ljm42 thanks for your submission. I have created a new pull request #110 which respects the core.hooksPath setting, without reporting on it under --display. I feel it's probably excessive to report on the GIT_HOOKS path even if it has a custom setting, since it isn't really specific to Transcript and the user is probably already aware of that customisation.

Regarding the custom location for the crypt/ directory, I think that storing it under a hooks directory would be surprising and confusing to people even if this would only happen for those with custom core.hooksPath settings. Could this be done differently, perhaps with a new --crypt-path script option?

ljm42 commented 3 years ago

Thanks @jmurty, this looks great! A command line param for --crypt-path sounds like a good solution too

ljm42 commented 3 years ago

I like the changes you are working on over in https://github.com/elasticdog/transcrypt/pull/112

Would you be open to defining something like this: CRYPT_PATH="${GIT_DIR}/crypt" and then using that variable everywhere? It would make it much easier for me to tweak my local install, even without having the --crypt-path option

jmurty commented 3 years ago

Hi @ljm42 thanks for the reminder, this is on my radar. I'm doing some further clean-up work based on the now-merge #112 PR which should make it easier still to customise the crypt/ directory path.

jmurty commented 3 years ago

Hi @ljm42 I think the latest commit in PR #114 (on the further-clean-up-helper-scripts branch) will give you a reasonable way to control where transcrypt puts its crypt/ directory.

If you manually set the transcrypt.crypt-dir setting before init-ing transcrypt in a repository, it should respect that path when storing and using the (now single) scripts it stores in the crypt/ directory.

It has been quite fiddly to get this working, so it would be great if you can test it.

ljm42 commented 3 years ago

Nice! I like that the filters call transcript clean so we don't even need a separate clean file now :)

All of the unit tests pass on my system, in case that was a potential question.

I can confirm that both of these settings are respected and files are put in the right place, woot!

I have done basic commits and everything seems to work well, although I did need to make the changes I mentioned in #116. This is really nice, thanks a ton for your work on transcrypt!

ljm42 commented 3 years ago

Actually, not going quite as well as I thought. I am not able to commit a txz file once adding it to a repo. Pretty sure every txz/zip will have the problem, but here is a test file that definitely has it: http://slackware.cs.utah.edu/pub/slackware/slackware64-14.2/slackware64/d/git-2.9.0-x86_64-1.txz

ljm42 commented 3 years ago

Here is a test script to show the problem:

cd <some path>
mkdir repo git-crypt git-hooks
cd repo

git init
git config user.email "John Doe"
git config user.name "John Doe"
git config core.hooksPath /path/to/git-hooks
git config transcrypt.crypt-dir /path/to/git-crypt

/path/to/transcrypt

export GIT_TRACE=1

# this works fine
echo test > sensitive_file
echo 'sensitive_file  filter=crypt diff=crypt merge=crypt' >> .gitattributes
git add .gitattributes sensitive_file
git commit -m 'Add encrypted version of a sensitive file'

# this commit fails silently
wget http://slackware.cs.utah.edu/pub/slackware/slackware64-14.2/slackware64/d/git-2.9.0-x86_64-1.txz
echo 'git-2.9.0-x86_64-1.txz  filter=crypt diff=crypt merge=crypt' >> .gitattributes
git add .gitattributes git-2.9.0-x86_64-1.txz
git commit -m 'Add encrypted version of git-2.9.0-x86_64-1.txz'
ljm42 commented 3 years ago

Would it make sense to commit all this and then have a separate issue for txz files?

jmurty commented 3 years ago

Hi @ljm42 thanks for following up, I tested the example you provided and it showed genuine bugs in PR #114 so that needs more work before it can be merged.

I have applied fixes to the PR's further-clean-up-helper-scripts branch which fixed the txz file problems for me, can you please re-test to confirm?

jmurty commented 2 years ago

The last remaining fixes for problems identified in this issue are now merged to main in fdf81c53f0ad27651e03a67ea732b164d209e948 (via #114)

In particular I have confirmed that, post-merge, this test script works and results in two commits.

ljm42 commented 2 years ago

This is great, thank you very much!