Add stage_fixed flag for hooks #109

rbUUbr · 2020-02-02T20:15:51Z

Hey, I like to do contributions in this repo, It helped me to practice a lot about Go with it. Thanks! So, I made this feature, it should work on Windows platform(I hope so, I don't have possibility to figure it out). Also, I've added processError function to output all errors and set pipe as broken but didn't make any refactoring, I hope it will be useful in the future.

Arkweid · 2020-02-03T11:14:19Z

Could you explain about a little more? What problem does this PR solve?

rbUUbr · 2020-02-03T11:46:01Z

sorry, I haven't provided issue :) It's closes #64

Arkweid · 2020-02-03T11:54:12Z

Awesome! I will check it soon.

lionskape · 2020-02-17T03:34:44Z

cmd/run_windows.go

 	// io.Copy(os.Stdout, ptyOut) // win specific
 	if command.Wait() == nil {
 		okList = append(okList, commandName)
+		context.ExecGitCommand(fmt.Sprintf("git add %v", strings.Replace(string(fileNames), "\r\n", " ", -1)))


Seems like output logging is inconsistent with bash command.

lionskape · 2020-02-17T03:55:21Z

Can you describe what is the difference between your approach and "git add {staged_files}", please?

How do you achieve the passing of partially staged content to the linter-formatter, and how do you grab the result of it?
How do you solve the merging problem for applying unstaged changes to the working tree?

I can explain some important cases for "stage_fixed" feature, as I understood it:

formatter can reorder lines, for example - key order, import order
formatter can split line (for example to solve "too long line" rule)
one line can be both reordered and split while the formatting process.
after the hook - you have to get a working tree with unstaged changes and fixes, but with committed fixes.

rbUUbr · 2020-02-22T19:21:24Z

Can you describe what is the difference between your approach and "git add {staged_files}", please?

How do you achieve the passing of partially staged content to the linter-formatter, and how do you grab the result of it?
How do you solve the merging problem for applying unstaged changes to the working tree?

I can explain some important cases for "stage_fixed" feature, as I understood it:

formatter can reorder lines, for example - key order, import order

formatter can split line (for example to solve "too long line" rule)

one line can be both reordered and split while the formatting process.

after the hook - you have to get a working tree with unstaged changes and fixes, but with committed fixes.

hey there, thanks for your review. There is no diff between this approach and git add {files}. Only one key difference: this feature automize staging of fixed files, that helps you to not write bash script with xargs for it. Also, this feature does not passes parts of files, because it can be the reason of chaos :)

rbUUbr · 2020-03-16T19:13:12Z

@Arkweid any updates?

Arkweid · 2020-03-17T09:51:55Z

Going to merge it at weekend. Sorry for so long!

Arkweid · 2020-03-22T12:05:04Z

@rbUUbr Again sorry for so long. I have tested it on discourse example:
https://github.com/discourse/discourse/blob/master/lefthook.yml
And everything goes down:

$ git commit -m "test"
Lefthook v0.7.0
RUNNING HOOKS GROUP: pre-commit

  EXECUTE > eslint-assets
read /dev/ptmx: input/output error

  EXECUTE > eslint-plugins-assets
read /dev/ptmx: input/output error

  EXECUTE > eslint-plugins-test
read /dev/ptmx: input/output error

  EXECUTE > prettier
read /dev/ptmx: input/output error

  EXECUTE > eslint-assets-tests
read /dev/ptmx: input/output error

  EXECUTE > eslint-test
read /dev/ptmx: input/output error

SUMMARY: (done in 10.00 seconds)
🥊  eslint-assets
🥊  eslint-plugins-assets
🥊  eslint-plugins-test
🥊  prettier
🥊  eslint-assets-tests
🥊  eslint-test

Could you recheck it?

rbUUbr · 2020-03-23T08:58:22Z

O_o will recheck it, thanks!

rbUUbr · 2020-04-12T14:17:12Z

Hi, sorry for long response time :( @Arkweid I can't reproduce it, but I found this issue creack/pty#21
I can't reproduce it on my version of left hook with next config:

pre-commit:
  parallel: true
  commands:
    govet:
      tags: backend style
      run: go vet ./...
      stage_fixed: true
    golint:
      tags: backend style
      run: golint ./...
      stage_fixed: true
    gofmt:
      tags: backend style
      run: go fmt ./...
      stage_fixed: true
    terraformfmt:
      tags: backend style
      glob: '**/*.tf'
      run: terraform fmt
      stage_fixed: true
pre-push:
  parallel: true
  scripts:
    "coverage.sh":
      runner: bash

shin-sforzando · 2022-03-03T06:26:50Z

I came across this PR because I was wondering how to include the results of automated documentation generation tools (like sphinx) and automated tests (like Coverage.py) in pre-commit.
There are times when newly generated files have not been staged in the past, but they are obviously harmless and we want to add them to the stage immediately.
In this case, I can't do git add {staged_files}, so I write the following every time.

pre-commit:
  commands:
    format:
      glob: "*.py"
      run: black {staged_files}
    test:
      glob: "*.py"
      run: pytest {files} && git add {files}
    doc:
      run: sphinx-build -a docs docs/_build/ && git add {files}

A little messy looking with && git add {files} all over the place.
And this way, files that may not be harmless, generated by other commands, will be mixed into the stage.
So I want to add only the results generated by each command to the stage.

I would be very happy if you could tell me how you all do it so well.

elliotwestlake · 2022-08-12T10:47:40Z

@Arkweid Is there any update on this? Would be good to get this merged in. Thanks!

elliotwestlake · 2022-08-14T14:06:54Z

@mrexox Maybe you'll be able to help out?

mrexox · 2022-08-15T08:19:38Z

This feature is a good idea, but I see some nuances:

Imagine we have staged a file partially:

$ git status
AM file1
A  file2
A  file3

If we just git add it, we would commit changes that weren't expected to be committed. Anyway, I like the idea of auto-stage files that were changed during the lefthook execution, but I want to add an extra step:

Hide changes that weren't staged before running lefthook (e.g. in stash).
Run lefthook command.
Some files changed? -> Stage those files. (q: should them be all files in a project or just those that were staged initially?)
"Unhide" the unstaged changes.
Proceed with git commit.

This feature is applicable only to pre-commit hook, isn't it? Do you agree that we should ignore it on other hooks?

elliotwestlake · 2022-08-15T10:09:42Z

This feature is a good idea, but I see some nuances:

Imagine we have staged a file partially:
$ git status
AM file1
A  file2
A  file3
If we just git add it, we would commit changes that weren't expected to be committed. Anyway, I like the idea of auto-stage files that were changed during the lefthook execution, but I want to add an extra step:

Hide changes that weren't staged before running lefthook (e.g. in stash).

Run lefthook command.

Some files changed? -> Stage those files. (q: should them be all files in a project or just those that were staged initially?)

"Unhide" the unstaged changes.

Proceed with git commit.

This feature is applicable only to pre-commit hook, isn't it? Do you agree that we should ignore it on other hooks?

Agreed, all makes sense to me. Yes this should only exist on pre-commit

@rbUUbr are you happy to work on this? If not I'm happy to pick up.

mrexox · 2023-03-15T08:08:07Z

Closing this in favor of #445. Please, feel free to open an issue if something doesn't work as expected.

rbUUbr added 2 commits February 2, 2020 23:07

Add feature for platforms

7a7edf3

Add fix for tabs

a509e07

rbUUbr requested a review from Arkweid February 3, 2020 09:23

lionskape reviewed Feb 17, 2020

View reviewed changes

ahimta mentioned this pull request Nov 16, 2021

Runs on contents which aren't staged for commit #60

Closed

This was referenced Mar 12, 2023

Lefthook doesn't block commits when using a --fix or --write option. #440

Closed

Feature Request: Automatic git add #64

Closed

mrexox mentioned this pull request Mar 14, 2023

feat: add stage_fixed option #445

Merged

3 tasks

mrexox closed this Mar 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stage_fixed flag for hooks #109

Add stage_fixed flag for hooks #109

rbUUbr commented Feb 2, 2020

Arkweid commented Feb 3, 2020

rbUUbr commented Feb 3, 2020

Arkweid commented Feb 3, 2020

lionskape Feb 17, 2020

lionskape commented Feb 17, 2020 •

edited

Loading

rbUUbr commented Feb 22, 2020

rbUUbr commented Mar 16, 2020

Arkweid commented Mar 17, 2020

Arkweid commented Mar 22, 2020 •

edited

Loading

rbUUbr commented Mar 23, 2020

rbUUbr commented Apr 12, 2020

shin-sforzando commented Mar 3, 2022 •

edited

Loading

elliotwestlake commented Aug 12, 2022

elliotwestlake commented Aug 14, 2022

mrexox commented Aug 15, 2022

elliotwestlake commented Aug 15, 2022

mrexox commented Mar 15, 2023

Add stage_fixed flag for hooks #109

Add stage_fixed flag for hooks #109

Conversation

rbUUbr commented Feb 2, 2020

Arkweid commented Feb 3, 2020

rbUUbr commented Feb 3, 2020

Arkweid commented Feb 3, 2020

lionskape Feb 17, 2020

Choose a reason for hiding this comment

lionskape commented Feb 17, 2020 • edited Loading

rbUUbr commented Feb 22, 2020

rbUUbr commented Mar 16, 2020

Arkweid commented Mar 17, 2020

Arkweid commented Mar 22, 2020 • edited Loading

rbUUbr commented Mar 23, 2020

rbUUbr commented Apr 12, 2020

shin-sforzando commented Mar 3, 2022 • edited Loading

elliotwestlake commented Aug 12, 2022

elliotwestlake commented Aug 14, 2022

mrexox commented Aug 15, 2022

elliotwestlake commented Aug 15, 2022

mrexox commented Mar 15, 2023

lionskape commented Feb 17, 2020 •

edited

Loading

Arkweid commented Mar 22, 2020 •

edited

Loading

shin-sforzando commented Mar 3, 2022 •

edited

Loading