Fossy-Cats / Git-Buch_EN

English translation of "Das Git-Buch" (The Git Book)
https://git.io/gitbook
Other
1 stars 0 forks source link

Add HTML creator Git hook installer #5

Closed SicroAtGit closed 4 years ago

SicroAtGit commented 4 years ago

Currently, we have to create the HTML file manually. With the hook this installer installs, Git automatically takes care of creating the HTML file for each commit we make.

tajmone commented 4 years ago

This sounds cool.

I just have a few questions, because I haven't used hooks before.

  1. The docs_src/build.sh might be a temporary build script, because in the future we might have separate build scripts for the HML version, the PDF version, and for the online GHPages website preview. Surely, the build.sh might still be kept as an invoker of all the other scripts, I'm just not sure that the script won't be renamed. How is this going to affect the hook? i.e. could updating the install-git-hooks.sh solve this?
  2. How does the hook script work? It will automatically build the document before a commit and include it in the commit (i.e. add the HTML doc to it)?
  3. What happens if, for any reason, we want to skip updating the HTML build in specific commits (e.g. because working on separate features and wanting to keep them separate from doc updates)?

Other than these questions, it seems a good idea to automate that part.

SicroAtGit commented 4 years ago

Git hooks are shell scripts located in the .git/hooks directory of the local repository. Git automatically creates sample scripts for all kinds of events in this directory. Each sample script describes well on which event the script is executed and also contains sample code. As soon as you remove the .sample extension from a script or make a copy of a sample script without the .sample extension, it will be activated. The filename of the scripts is important, otherwise Git won't be able to associate them with events.

To 1: If build.sh is renamed, Git will display an error message that the script can no longer be found, but will then create the commits completely without aborting (at least for my current script. It's also possible in the script to make commits abort). So in that case we also have to adjust the install-git-hooks.sh with the new name of the build.sh-file and everyone has to run the script install-git-hooks.sh again locally so that the script adjusts the hook-script. I can also program the hook script to delete itself if it can't find the build.sh script anymore. Many things are possible.

To 2: Exactly. I use the pre-commit hook script from Git (.git/hooks/pre-commit), which, as the filename already suggests, is executed before the commit. So if you run git commit ..., Git will first run the pre-commit hook script, and then finish the commit. The pre-commit hook script that is created by the install-git-hooks.sh script runs build.sh and adds the created HTML file to the stage with git add Git-Buch_EN.html before Git commits the stage content.

To 3: I could program the hook script to determine the current branch and if it is not alpha-dev, skip the creation of the HTML file and continue without it. Or we could have the hook script search for a keyword in the commit message and if it exists, skip the HTML creation or vice versa.

tajmone commented 4 years ago

Regarding (2) I was wondering if the script exit status would affect the commit — i.e. if the script returns non NUL (i.e. an error), or if the script invocation fails, does the commit also abort?

The problem right now is that the script is only present in the dev branches, so like you mentioned in (3) it should only work on the dev branch right now. My worry is about what would happen if one commits to master, where the script is not found.

To 1: If build.sh is renamed, Git will display an error message that the script can no longer be found, but will then create the commits completely without aborting (at least for my current script.

So execution of the script is not conditional for the commit to happen.

It's also possible in the script to make commits abort).

How? By setting an exit status error?

I was also thinking of the EditorConfig validation for code styles consistency. That is something useful to check on every commit, but of course it would also mean that end users would have to install the EClint tool for validating the code — which is a Node.js tool, which is no longer actively developed and, therefore, possibly unsafe (well, Node,js is by itself a huge security hole anyhow). So, on the one hand it's tempting to enforce that, but on the other hand I don' like to impose an unsafe app on end users.

But, if I understood correctly, it's up to the end users to decide whether or not to install those hooks, so it's not a mandatory thing anyhow.

What I suggest is to edit the current PR to ensure that the hook would work only on the current alpha-dev branch — or, if you can specify globbing/RegEx like patterns, something like *-dev, so it will work also on the upcoming Beta.

And, possibly, if you can also add a hook-uninstaller script it would be good, so if an end user wants to remove it he/she can do so via a dedicated script, without having to manually do so.

As long as it's going to be possible to update the hook via an update script (i.e. changing its contents by re-running the hook installing script) then it's fine, because we can therefore handle changes in scripts names, branches behavior, etc., without the need to manually fiddle with the hooks inside the ./git/ folder.

SicroAtGit commented 4 years ago

Regarding (2) I was wondering if the script exit status would affect the commit — i.e. if the script returns non NUL (i.e. an error), or if the script invocation fails, does the commit also abort?

An exit status other than zero (i.e. error) will cause Git to abort the commit without an error message.

My worry is about what would happen if one commits to master, where the script is not found.

Then the missing script is reported in the terminal (command prompt), but because I don't currently change the exit status in the script (exit status remains zero), the commit is still completed by Git, i.e. without the HTML creation.

So execution of the script is not conditional for the commit to happen.

In all cases the commit is created.

How? By setting an exit status error?

Yes, if the exit status is other than zero, Git will abort the commit with no error message.

I was also thinking of the EditorConfig validation for code styles consistency.

This is a different topic and we should create an issue to discuss whether we use EditorConfig or alternatively program our own tool. This PR should only be about the HTML creation script. I did not give the installation script install-git-hooks a specific name so that it is open for further hook scripts.

But, if I understood correctly, it's up to the end users to decide whether or not to install those hooks, so it's not a mandatory thing anyhow.

Yes, everyone can decide whether to install the hook script or not.

What I suggest is to edit the current PR to ensure that the hook would work only on the current alpha-dev branch — or, if you can specify globbing/RegEx like patterns, something like *-dev, so it will work also on the upcoming Beta. And, possibly, if you can also add a hook-uninstaller script it would be good, so if an end user wants to remove it he/she can do so via a dedicated script, without having to manually do so.

Ok, I will adapt the script.

As long as it's going to be possible to update the hook via an update script (i.e. changing its contents by re-running the hook installing script) then it's fine, because we can therefore handle changes in scripts names, branches behavior, etc., without the need to manually fiddle with the hooks inside the ./git/ folder.

That is my goal.

tajmone commented 4 years ago

Ok, I will adapt the script.

Feel free to merge the PR when you're satisfied with your changes then!

SicroAtGit commented 4 years ago

The installer and uninstaller preserve content in the pre-commit file that does not belong to the HTML creation hook script.

It should be ok now, but I leave the PR still open for today and merge it tomorrow.