Rework clone

[erijo]

What does this PR do?

This PR changes the way clone works to use git clone instead of init+config+fetch+merge. I've also changed the default behavior to leave local changes in the working copy.

What issues does this PR fix or reference?

None

Previous Behavior

See above.

New Behavior

See above.

Have tests been written for this change?

No, but existing tests have been updated.

Have these commits been signed with GnuPG?

Yes

---

Please review yadm's Contributing Guide for best practices.

[TheLocehiliosan]

I'm not able to review this closely right now, but I can say I did look at making a similar change in the past few months, and there was some reasons that pushed me away from it. And until I get into it, I don't think I'm going to recall. I think it may have had something to do with previous concerns about exposed directories (ssh/gnupg). That is, if someone manages configs that are inside those directories, I need to be sure that those directories are not made readable at any point (even for a split second). Also, the data managed in them is part of the repo, so the repo itself also needs to be secured. Git (I believe) uses the same perms for everything based on the umask. One advantage of doing the init then merge is you can secure those directories (including the repo) before any of the repo's data arrives at the computer.

Again, I will watch your development with interest, but just so you know, I think that's background. There was a CVE logged against this project, and I had to scramble to to have it resolved (and it didn't matter how valid the CVE was). I will not make a change if I think it will open the project to something similar again.

[erijo]

I've looked into the permissions a bit. First, below is a part of the strace output when running git init --shared=0600 --bare repo.git:

[...]
150575 mkdir("repo.git", 0777)          = 0
[a lot of calls related to copying /usr/share/git-core/templates removed]
150575 chmod("/home/erik/tmp/yadm/repo.git", 02700) = 0
150575 mkdir("/home/erik/tmp/yadm/repo.git/refs", 0777) = 0
[...]
150575 mkdir("/home/erik/tmp/yadm/repo.git/objects", 0777) = 0
150575 chmod("/home/erik/tmp/yadm/repo.git/objects", 02700) = 0

I.e. the permissions for repo.git is set to the shared mode 02700 early on, before the objects dir is created. We can compare this with the strace output from git -c core.sharedrepository=0600 clone --separate-git-dir repo.git --no-checkout <url>:

[...]
149595 mkdir("/home/erik/tmp/yadm/repo.git", 0777) = 0
[a lot of calls related to copying /usr/share/git-core/templates removed]
149595 chmod("/home/erik/tmp/yadm/repo.git", 02700) = 0
149595 mkdir("/home/erik/tmp/yadm/repo.git/refs", 0777) = 0
[...]
149595 mkdir("/home/erik/tmp/yadm/repo.git/objects", 0777) = 0
149595 chmod("/home/erik/tmp/yadm/repo.git/objects", 02700) = 0
[...]
149596 socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 3

The calls related to the repo.git dir is very similar for both init and clone. In both cases the permissions are changed before anything is written to it. In the clone case we can e.g. see that opening the ssh socket is done after the permissions have been set.

If we look at the source code for git init it calls init_db at the end. This is also called from git clone. init_db calls create_default_files which does copy_templates() before doing adjust_shared_perm(get_git_dir()) followed by safe_create_dir(git_path("refs"), 1). So the calls seen in strace matches the code :)

Based on this I think the security should be as good as with the init variant.

[erijo]

This is no longer a WIP. I've done the things I wanted to do.

[erijo]

I'll rebase this later to fix the conflict and correct the zsh completion change.

[TheLocehiliosan]

@erijo I had time today to go through all of these changes in detail. This is a really solid change. 👍 I make only a few edits on top of this, and pushed them into branch pr/289. Please confirm this looks good to you as well. Only real change is to remove the mktemp dependency. I had problems in the past with mktemp portability.

You're using a variable named "wc" and I'm sure there must be something obvious that stands for, but it isn't apparent to me. What does "wc" mean?

[TheLocehiliosan]

The only other item that I was curious about was how far back the --separate-git-dir option was introduced. It looks like that's available since Git 1.7.5. So while this change will bump the minimum supported version of Git, but that's a really old version of Git. 👍

[TheLocehiliosan]

One last bit of praise; I really do prefer this strategy where conflicting changes are left as modified working files.

Stashing them was a convenient way to deal with the inability to merge in the past, but it was confusing to some users that are not familiar with all of Git's features. Your strategy leaves the work tree intact and changes as clear diffs. I think this will make sense to most everyone.

[erijo]

Your changes look fine to me 👍

wc is short for working copy.

[erijo]

The documentation needs an update as well after this. Will you do that @TheLocehiliosan or should I put together a PR?

[TheLocehiliosan]

I do make an effort to give the docs (both manage and website) an update when I do a release for all the changes in the release. But help is always nice.