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

Pass local and remote branch names as arguments / env variables to pre-push #320

Closed
JoyceBabu opened this issue Aug 21, 2022 · 26 comments · Fixed by #324
Closed

Pass local and remote branch names as arguments / env variables to pre-push #320

JoyceBabu opened this issue Aug 21, 2022 · 26 comments · Fixed by #324
Milestone

Comments

@JoyceBabu
Copy link

Is it possible to access the local / remote ref names in the pre-push hook?

Git passes the information as STDIN to the pre-push hook. But lefthook is not passing it to the actual hook.

@mrexox
Copy link
Member

mrexox commented Aug 21, 2022

Yes, lefthook doesn't pass STDIN now. Could you describe your case?

@JoyceBabu
Copy link
Author

I am using a pre-push hook to statically analyze the source code using PHPStan.

Since it is a legacy code base, the code is not fully typehinted, and there are several thousand existing issues.

We want to ensure that new changes does not introduce additional issues, while we are are slowly fixing the existing errors.
For this, we have a pre-push hook that statically analyzes the codebase twice. First for the merge base to create a baseline of existing issues, and then for the pushed commit. All issues that are missing in the baseline, are expected to be fixed, before the branch is pushed.

To determine the merge base, we need access to the local ref name.

@mrexox
Copy link
Member

mrexox commented Aug 24, 2022

Wow! This looks reasonable. Could you share your lefthook.yml? I think passing STDIN is possible but I need to find the most proper way to do so.

Does PHPStan support analyzing only the given files? I mean, you could run it only for files that were touched in commits that are being pushed. There is a {push_files} template for that. So, it would look like this:

pre-push:
  commands:
    phpstan:
      run: phpstan {push_files}

If it doesn't help, and using STDIN is the only option, I would ask you to provide some reproducible code+lefthook.yml set, so I could test STDIN passing while developing a feature.

@JoyceBabu
Copy link
Author

I am not sure whether static analysis is possible with just a subset of files. Changes in a file can affect code in unmodified files too.

Also, the hook is supposed to report only new issues. Because, sometimes the modifieds may have hundreds of issues and it may not be feasible to fix them all in a single go.

There is an existing issue about the missing STDIN.

I have created a test repo with the static analysis check
https://github.com/JoyceBabu/lefthook-test

I had to hard code the values for local_ref as HEAD and remote_ref as origin/master.

@mrexox
Copy link
Member

mrexox commented Aug 25, 2022

Thank you a lot! I will start working on it soon! (wow, you made a repo for this case! I appreciate it a lot!)

@mrexox mrexox added this to the 1.2.0 milestone Aug 25, 2022
@mrexox
Copy link
Member

mrexox commented Aug 25, 2022

I thought a little bit and came up with the following solution:

  1. Add a stdin option to hook
  2. Read if stdin: true, read from stdin once and pass it to all the commands/scripts defined for pre-push
  3. Otherwise just use blank stdin

So, it would look like this:

pre-push:
  stdin: true # default would be false
  scripts:
    "my-script.sh":
      runner: bash

@JoyceBabu , @ThisIsMissEm what do you think about it?

@JoyceBabu
Copy link
Author

The stdin option would definitely work for my case. But I think it would be better to enable it per script/command, rather than globally for all scripts / commands under a hook.

Users may use scripts/executables provided by third parties in their hook, that might behave differently depending on whether a stdin in available or not.

@Envek
Copy link
Member

Envek commented Aug 26, 2022

Btw, why not pass copies of stdin to every hooks always by default?

@mrexox
Copy link
Member

mrexox commented Aug 26, 2022

@Envek , there is a problem with LFS pre-push hook, which waits for input if stdin is provided. I guess lefthook users don't expect the tool to prompt for input by default, so it's better to have an option for those who need it. I am afraid we will have bugs otherwise, like, constant waiting for input when you are git-pushing from VScode/any other git wrapper.

@Envek
Copy link
Member

Envek commented Aug 29, 2022

I guess lefthook users don't expect the tool to prompt for input by default

Wait, where is input prompting came from? How it is related to the stdin contents that git itself produces?

@mrexox
Copy link
Member

mrexox commented Aug 29, 2022

I guess lefthook users don't expect the tool to prompt for input by default

Wait, where is input prompting came from? How it is related to the stdin contents that git itself produces?

Hmm, doesn't it prompt refs from user? Hell... seems like I misunderstood the issue.

I need some time to investigate why we can't pass stdin from lefthook to scripts/commands.

@mrexox mrexox mentioned this issue Aug 29, 2022
2 tasks
@mrexox
Copy link
Member

mrexox commented Sep 5, 2022

Hey! I have prepared and tested STDIN passing, but there is a limitation. You should not use while loop for reading data from STDIN in your hooks. In that case (for some reason) Lefthook hangs and doesn't receive EOF from STDIN.

Can you test the change? Does it work for you? #324

gh pr checkout 324
make # or `go build`
cp lefthook ${GOPATH}/bin

@JoyceBabu
Copy link
Author

The process is hanging for me, even without the while loop.

I have updated the test repo to read the values from stdin.

@JoyceBabu
Copy link
Author

JoyceBabu commented Sep 5, 2022

The script worked after commenting the following lines from .git/hooks/pre-commit.

if [ -t 1 ] ; then
  exec < /dev/tty ; # <- enables interactive shell
fi

It is now working with while read too.

Thank you for fixing this.

@mrexox
Copy link
Member

mrexox commented Sep 5, 2022

Oh, wow! I'll take a look and try to fix the script template. This makes sense.

@mrexox
Copy link
Member

mrexox commented Sep 6, 2022

Yes, I found the PR that introduced this exec < /dev/tty hack: #94. I guess this is not a very often-used feature, so I can add a special option interactive: true for the commands and scripts that enables it, and change the default behavior - without this hack. This is quite a big change, so it will go to 1.2.0 version. I'll add this in the same PR.

@mrexox
Copy link
Member

mrexox commented Sep 15, 2022

Hey @JoyceBabu ! I've updated the PR. Sorry for a long wait. But I can't make it work with wile read ....
I am going to merge this soon (just something to start with). Let me know if while read ... support is crucial for you, I'll spend more time investigating.

@JoyceBabu
Copy link
Author

JoyceBabu commented Sep 15, 2022

I never push to more than one target at a time, so while read is not important for me. I am not sure, but I think while read too was working for me after editing the hook script. I am on vacation and don’t have my laptop with me to verify this. I will confirm as soon as I am back.

Thanks a lot for fixing this.

@weilbith
Copy link

Unfortunately I still have the origin issue. Should mean I have a pre-push command that requires to get the originally passed arguments by Git. Is that somehow possible?

@mrexox
Copy link
Member

mrexox commented Jan 3, 2023

Yes, arguments passed by Git should be passed explicitly (see docs for details)

  • {0} - is the string with all Git args, space-separated
  • {1} - 1st arg
  • {2} - 2nd arg
  • ... and so on

Is it helpful? If not, could you provide more details about what you are trying to achieve?

@weilbith
Copy link

weilbith commented Jan 3, 2023

Oh. I'm sorry that I missed that part. Very nice.

Though, it is not fully working for me. I have this example configuration:

pre-push:
  commands:
    test:
      run: echo All arguments together are: {0}

    test-2:
      run: echo The arguments separated are: {1} {2} {3} {4}

The output of doing an actual git push then is:

Lefthook v1.2.6
SYNCING
SERVED HOOKS: pre-push
RUNNING HOOK: pre-push

  EXECUTE > test
refs/heads/main 734dadaba190aac9c4113243dc30a43e2ea000f1 refs/heads/main bae02095d38f5954a69c4f83b84a42f9ea2c4adb
The pre push arguments are: origin github.com:account/repository.git


  EXECUTE > test-2
The separate arguments are: origin git@github.com:account/repository.git {3} {4}


SUMMARY: (done in 0.01 seconds)
🥊  test
🥊  test-2
error: failed to push some refs to 'github.com:account/repository.git'

According to the man githooks documentation, the pre-push hook receives the following arguments: <local ref> SP <local object name> SP <remote ref> SP <remote object name> LF.

The very first line of the output that is not from any of my commands (refs/heads/main 734dadaba190aac9c4113243dc30a43e2ea000f1 refs/heads/main) looks pretty perfect. Where does it come from?
But the two echo commands look different. What am I missing or understanding wrong? 🙈

@mrexox
Copy link
Member

mrexox commented Jan 6, 2023

Hey @weilbith this is not all. The arguments are provided via STDIN:

Information about what is to be pushed is provided on the hook’s standard 
input with lines of the form:
  <local ref> SP <local object name> SP <remote ref> SP <remote object name> LF

So you need to read from STDIN to receive those lines. Use interactive option to enable STDIN in command or script. You still need to white your own script and parse that data. (or maybe there are existing solutions that can do that job). Please, write back about your experience 🙂

@weilbith
Copy link

weilbith commented Jan 6, 2023

Ah, interesting. I think the interactive was misinterpreted by me here.

But I'm afraid I'm too stupid to make it work. Having this simple bash command xargs -0 printf '%s' and test it in a normal Bash with an echo 'foo' | before, it works as expected. But using it within a Lefthook command, makes Lefthooks hang forever.

pre-push:
  commands:
    test:
      interactive: true
      run: xargs -0 printf '%s'; exit 1

The output is just:

SYNCING
SERVED HOOKS: pre-push

Using my "actual" command I want to make work, it is the same result. It waits for ever.

Btw: thank you very much for the great support! 🙏🏾

@weilbith
Copy link

I'm sorry, but may I ping you @mrexox 🙈

@mrexox
Copy link
Member

mrexox commented Jan 17, 2023

Hey @weilbith! Sorry for such a delay. I've tried to handle pre-push locally, and I found that I've made a mistake with the later comment. Here is my example:

.
├── lefthook.yml
└── .lefthook
    └── pre-push
        └── script.sh

The config

# lefthook.yml

pre-push:
  follow: true # need to use follow to get stdin from git push
  scripts:
    "script.sh":
      runner: bash

The hook .lefthook/pre-push/script.sh:

#!/usr/bin/env bash

remote="$1"
url="$2"

echo 'Hello there!'
echo remote $remote
echo url $url

read local_ref local_sha remote_ref remote_sha

echo local_ref $local_ref
echo local_sha $local_sha
echo remote_ref $remote_ref
echo remote_sha $remote_sha

exit 0

I forgot that there is a special option to follow the STDOUT of the command which also passes STDIN from git. interactive didn't work that way, and it shouldn't, since it was made to pass the terminal's STDIN (it connects to the tty).

Also I guess while read in the script still doesn't work now, and if you need it I can create an issue and take a look at it.

@hustcer
Copy link

hustcer commented Apr 26, 2023

I got a hang up while running git push when there is nothing to be pushed. It works as expected when there is something to push. It works fine for husky, Is this a bug? Or is there a workaround?

BTW: I'm using the latest version: 1.3.10

My pre-push script:

#!/bin/sh

while read local_ref local_oid remote_ref remote_oid
do
  echo $local_ref
  echo $local_oid
  echo $remote_ref
  echo $remote_oid
  echo '----------------------------------'

  # Break is important here, to stop another loop
  break;
done

exit 0

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 a pull request may close this issue.

5 participants