Team members cannot run installation

[edisonywh]

A little oversight, but right now the installation script will check if commit.exs exists, and if it does, stops the execution of installation.

However, this means that let's say in a team of 3, if one person in the team installed, commit.exs will be git-tracked, now if it's pushed to repo and then the other two people pull it down, and try to run mix committee.install again, they won't run the git hooks installation part, because commit.exs exists.

Not quite sure what will be fix, maybe always run the git hooks installation? (but then this will override the backup). So ideally other team members should be able to run it.

Another idea I can think of is to create sort of like a "sentinel file". commit.exs should be that, but that's not enough because it's git-tracked, which means I need a gitignored version of sentinel file. Maybe could create a file inside .git/hooks/committee.checksum or something that acts as a sentinel, and run install if that file doesn't exist?

[thiamsantos]

First of, really great initiative with this library @edisonywh. It is sad that we still don't have a mature and good solution for git hooks yet.

A little oversight, but right now the installation script will check if commit.exs exists, and if it does, stops the execution of installation.

Nice that you opened a issue, because that is something necessary for me to be able to introduce this library in my projects.

I think here we have two problems, one is about properly checking if the githook is installed with committee and another how to prevent from override the backup values.

For the first, how to properly check if the githook was installed. I think instead of checking the existance of the commit.exs file, we could use the same approach that husky uses. It adds a # husky at the top of the git hook file. And whenever someone tries to install, it reads the git hook file and check if it has this flag, if does not it proceeds with the installing, otherwise it just stops. That approach could be also useful, in the future, in case we want to upgrade the format of the git hook, we can just check the version in this "flag" and have a backward compatible update.

https://github.com/typicode/husky/blob/master/src/installer/index.ts#L57 https://github.com/typicode/husky/blob/master/src/installer/is.ts#L3 https://github.com/typicode/husky/blob/master/src/installer/getScript.ts#L34

For the second, I would propose for instead of just adding the sufix .old to the backup file, we add some data that is unique, like a uuid. So we prevent us from rewriting existing backup files.

WDYT? I would be happy to send some PRs for these changes.

[edisonywh]

👋 @thiamsantos!

Thanks for the kind words! I think in general the git hooks game just isn't very popular in general, the biggest community I've found is in JS's Husky, but in Ruby, for example, it's not as common from what I've seen. Maybe with you and the community's help, Committee can get there :D

Yeah apologies for that, I've been using it on my side project so it wasn't very noticeable until I tried to introduce it in a team project and I immediately recognized what's happening, haha. Way to shoot myself in the foot :p

---

Thanks for sharing the links from Husky! I didn't actually know that's how they did it. Speaking of, I don't know why I didn't refer to Husky's implementation more, these are great stuff and I think there's a lot to learn from them! I really like their debug ENV implementation too!.

For the first, how to properly check if the githook was installed.

I agree with this problem statement, but I'm not sure if I understand your last point where you mentioned:

That approach could be also useful, in the future, in case we want to upgrade the format of the git hook, we can just check the version in this "flag" and have a backward compatible update.

Can you elaborate on this/give an example?

If we end up using the # husky way, wouldn't that mean that we have to read all hooks file to check the first line of text? I'm not sure how Husky does it, whether they install all hooks upfront, or only when defined (because Committee installs everything upfront), so maybe that's why they had to check for each file - they don't know which ones have been installed.

In Committee case however, because we installed everything upfront, the state is clear for us - Committee is either installed, or not installed. It's a binary boolean. For that the ideal solution in my mind is a sentinel file inside .git/hooks directory called .committee perhaps, and because like I mentioned earlier the user has either installed or not, this .committee file will tell us whether we should proceed with installation or not. What do you think of this?

---

For the second, I would propose for instead of just adding the sufix .old to the backup file, we add some data that is unique, like a uuid. So we prevent us from rewriting existing backup files.

I don't really understand what you mean in your second point, so feel free to elaborate a little more. The idea for a .old is just a single backup before Committee overrides your git hooks, having a single file also makes it easier to revert/restore the backup (rename .old). I reckon this might be a little trickier with a UUID? Also I fear that this might open up a can of worm where if a user accidentally run installation multiple times (perhaps if they have it as a mix task), we generate a whole lot of unused backups in their .git/hooks

From your point I think the underlying issue is that we should not be able to run committee.install twice and have it rewrite the existing backup, for instance:

• User has hooks from Husky
• Runs committee.install
• Husky hooks backed up to .old
• User re-run committee.install
• Skips installation, because config file (commit.exs) exists
• User delete config file
• Rerun committee.install
• Previous Committee hooks backed up to .old, overwriting Husky, and now Husky backup is lost forever.

Therefore I think the problem is that, we should not be able to backup hooks generated by Committee, because. This problem is also kinda solved if we implemented the first solution with a sentinel file, that way users won't even be able to proceed with the installation of Committee if they have previously done so. What do you think?

Thanks again for the feedbacks, I really appreciate you taking the time to do that 👍

[thiamsantos]

Yeah apologies for that, I've been using it on my side project so it wasn't very noticeable until I tried to introduce it in a team project and I immediately recognized what's happening, haha. Way to shoot myself in the foot :p

No worries about that, I think you did an incredible job. I already tested most of the existing solutions with elixir, and also tried my own (using mix scripts), but neither of them had a clear and simple approach. So is a really awesome idea of using a simple elixir script file.

If we end up using the # husky way, wouldn't that mean that we have to read all hooks file to check the first line of text? I'm not sure how Husky does it, whether they install all hooks upfront, or only when defined (because Committee installs everything upfront), so maybe that's why they had to check for each file - they don't know which ones have been installed.

Yeah, that's exactly what they do. https://github.com/typicode/husky/blob/master/src/installer/index.ts#L42 They read every single git hook, if the file is generated by husky, they just skip it.

That's the flow that I imagine for commitee, mainly stealing from husky:

1 - If the commit.exs does not exist, create it. 2 - Reads all git hooks. 3 - For each git hook file. 3.1 - If it was generated by the current version of commitee, just skip the installation. 3.2 - If it was generated by a previous version of commitee, update the script with the new version. 3.4 - If it was not generated by commitee. 3.4.1 - Backup the file, adding the .old prefix. 3.4.2 - Generate the hook using committee.

With 3.1 we prevent the creation of multiple backup files and overwriting the hook already setted up. Also we handle the case of future support for other hooks, today we only support pre_commit and post_commit. If we add suport for another hook how we are going to install those ones? The sentinel file would already exists and the installation would be skipped. So we would need to instruct the user to remove the sentinel file to perform another install, with that we would be leaking the internals for the users, that could be specially hard for newcomers to understand what is going on. In my opinion, the perfect scenario would be just rerunning the install script and everything be update automatically without the user knowing that.

With 3.2 we account for a possible v2 of commitee. Let's say that in the future the format file change drastically or we rename committee.runner to committee.foo.bar. How we are going to handle that? With a single sentinel file, if it already exists the script wouldn't be updated, and again we would require the user to explicit remove the sentinel file, but I think that the script should handle all that by itself.

I think having a way to track the current version of commit for each hook script separated is better than a single sentinel because:

1. We would be accounting for the future support of new hooks. A single sentinel file would prevent the user from adding the hooks supported by the updated version of committee.
2. We would be accounting for the possible future change on the commitee hook format. A single sentinel file would prevent the user from updating the already existing script to the new version, requiring some additional steps for migrating to the new version.

Also I fear that this might open up a can of worm where if a user accidentally run installation multiple times (perhaps if they have it as a mix task), we generate a whole lot of unused backups in their .git/hooks

I think that will not be a problem if we just create the backup file for hooks not generating by us. Adding a unique part to it is just a failsafe for not overwriting existing backup files that a user might have, generated by other things.

@edisonywh What do you think?

[edisonywh]

I think you raised great points that I didn't spot earlier, very good arguments about versioning and hooks support! Love the idea about fixing the unlimited backups with versioned Committee files too, solves the problem rather elegantly.

I am pretty convinced so let's go with that! 🎉

[thiamsantos]

@edisonywh Nice :rocket:

WDYT of focus on the #2 first, so when we make this changes we have more trust that we are not breaking anything? :)