It would be appreciated to receive some more feedback on this potential addition to the pipeline template ⁉️ I think, we should add additional pre-commit hooks to the pipeline template. These hooks would prevent: • Committing of files larger than 5MB • Committing of files in a pipeline

outdir

(a folder that contains a

pipeline_info

subfolder) • Committing of files that contain unresolved merge conflicts. Your feedback is wanted particularly on those issues: • Allowed size limit of the files (

docs

folder is excluded, so e.g. images in docs can be larger) • Pipeline

outdir

detection: Should we only

grep

for staged files in the

pipeline_info

folder itself or keep the current script that iterates through all paths? The later is more comprehensive, but also takes more compute and may take a moment or two.

I think all of those make sense to me!

Fully agreeing

Might also make sense to prevent commits of the work directory and

.nf-test

folder in this case?

For

.nextflow.log

,

work

and

.nf-test

directory, we are indeed relying on

.gitignore

. Since the names of those are predictable. Only the

--outdir

is easily customisable. Therefore, while we had

testing

and

results

in the

.gitignore

, it was still possible to commit a

result

and

result_test

folders. Because they were deleted with later commits, they didn’t show up in the combined pull request diff and the PR was merged.

I just checked my (non-nf-core) pipeline - I have a 22Mb HMM file included in my

assets/

directory. Maybe that should be somewhere else really but it’s pretty tightly integrated into the pipeline run, so I’m reluctant to make it an external resource - different HMM files are unlikely to work. I’m not too familiar with pre-commit - is it possible to make it so that it forces you to confirm whether something is what you meant to do, rather than just fail - “File $X is 5678kb - enter Y to confirm commit”?

I think you can exclude files in the pre commit config

@Jim Downie Maybe pre-commit has a

--force

flag or something the like, but I wouldn’t go there. I am already allowing bigger files in

docs

and we will just add

assets

to the list of excluded locations?

Is there a way of reporting (large) files that have been added and removed on a branch ? Regardless of the rules we come up with here, the scenario listed in the PR is still fairly common: someone adds a file by accident, finds their mistake and deletes the file, but the file will remain as a git blob somewhere, taking space. It’d be good if we could detected such files and maybe report them in the PR ? (that may mean making this a

nf-core

lint test rather than a

pre-commit

). Then, once we’re made aware of this, we have options such as rebasing, squashing, etc, before / when the PR is merged, but we first need to know the problem is there

I think

pre-commit

is the most straightforward, because it prevents the mistake from happening in the first place. Anything else will only limit the “damage” done and require cleaning operations with various degrees of complexity. The easiest is to go through each commit separately during a PR review. If people use commits semantically with a decent description, it also simplifies the review - although I am aware that there are huge differences between people. Some will commit tiny things with descriptions like

fix

,

ugh

or

lala

and others submit a whole pipeline with 5 commits.

I do both

Regardless of the rules we come up with here, the scenario listed in the PR is still fairly common: someone adds a file by accident, finds their mistake and deletes the file, but the file will remain as a git blob somewhere, taking space.

I think this could be partially solved by enforcing squash commits into

dev

Yes, squashing commits would have prevented the files from being merged to

dev

- but I occasionally like to step through single commits to retrace thinking when I try to understand what has been changed why. When the

download

command was refactored this summer in #CE5LG7WMB the resulting PR was touching about 9000 lines of code. Just imagine that all squashed into one commit - I think there is value in breaking this up semi-semantically so that the step-wise progress on this task can still be retraced in the future even after PR review is done.

Perhaps I’m missing something, but when I do a squash commit to merge a PR, GitHub by default adds a link in the commit message to the PR, where you can see and explore the original commit history unsquashed.

No you are right. But I believe that requires that the branch from the PR is retained in the fork. I have developed the bad habit of deleting merged branches in my forks - which could be reconsidered of course.

Hi. I found that the default pre-commit hooks have something to check for large files. Does anyone know if it’s a suitable alternative to our home-made script ? github.com/pre-commit/pre-commit-hooks#…

Our home-made script is a bit more precise, because it allows for exceptions and specifically looks for pipeline output folders. But that hook would probably cover 80% of the cases.

Yes we switched over to prek, but all those hooks are still valid

If you see a benefit of using the existing, non-custom hook, I am not opposed. I prosed this about a year ago and by the time it was merged to the template, I had almost forgotten that PR. I still think we should have a helper to prevent large files to be committed accidentally, but I am neutral with regard to the implementation. If you feel keen enough to replace it, go ahead please.