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

Files that aren't in the commit are searched for commit comments #1

Open
SonOfLilit opened this issue Jan 15, 2016 · 9 comments
Open

Comments

@SonOfLilit
Copy link

$ git commit
.git/hooks/prepare-commit-msg: line 43: conditional binary operator expected
.git/hooks/prepare-commit-msg: line 43: syntax error near `=~'
.git/hooks/prepare-commit-msg: line 43: `  if [[ "${IGNORED[*]}" =~ "${filename}" || "${IGNORED[*]}" =~ "${extension}" ]]; then'

$ cd ../commit-comments/
$ git log | head -n 1
commit a916600e8cc2b0cb0f6da2533d616da3ddca5f68

$ /bin/bash --version
GNU bash, version 3.1.20(4)-release (i686-pc-msys)
Copyright (C) 2005 Free Software Foundation, Inc.

Running

#!/bin/bash
ADDED_FILES=( $(git ls-files) )
echo $ADDED_FILES
echo '************'
for i in ${!ADDED_FILES[@]}; do
  file_path=${ADDED_FILES["$i"]}
  filename=$( basename "$file_path" )
  extension="${filename##*.}"
  echo $file_path
  echo $filename
  echo $extension
  if [[ "${IGNORED[*]}" =~ "${filename}" || "${IGNORED[*]}" =~ "${extension}" ]]; then
    # File should not be read - remove
    unset ADDED_FILES["$i"]
  fi
done

gives

$ . test.sh
.gitignore
************
sh.exe": ./test.sh: line 12: conditional binary operator expected
sh.exe": ./test.sh: line 12: syntax error near `=~'
sh.exe": ./test.sh: line 12: `  if [[ "${IGNORED[*]}" =~ "${filename}" || "${IGNORED[*]}" =~ "${extension}" ]]; then'
@SonOfLilit
Copy link
Author

I tried to look into it.

Quickfix:

https://stackoverflow.com/questions/11206233/what-does-the-operator-do-in-shell-scripts

but .gitignore format is different from regex, and maybe it's better to use that?

https://stackoverflow.com/questions/2412450/git-pre-commit-hook-changed-added-files

but LarryH there points out that you'll pick up comments that are not up for being committed. That's bad because I love git add -i. He gives a solution, which is too big and architectural for me to PR without consulting with you first, and also I'm not sure I have time to do it, being as awful as I am in bash scripting.

In the meantime, I'll have to uninstall this :-(

@jryio
Copy link
Owner

jryio commented Jan 15, 2016

Sorry this isn't working and I appreciate the research into the issue!

I've looked into the compatibility of the Regular Expression operator for Bash version 3.1 - Documentation. It explains that the Regular Expression operator is indeed present for Bash (so you're not missing the operator).

However one change which was made after Bash version 3.2 is that the right hand side of the operator should not be quoted Ex. [[ "string quoted" =~ [a-zA-z0-9]* ]]"

Can you test the following commands (just enter them directly into Bash terminal session) and reply with the output?
1.

if [[ "hello world" =~ ".*" ]]; then echo "it worked"; else echo "it failed"; fi
if [[ "hello world" =~ .* ]]; then echo "it worked"; else echo "it failed"; fi

That would help determine if that is your issue


I am unable to reproduce this bug using Bash version 3.1. Here are some of the tests I've done to replicate this.

root@test-server:~# bash --version
GNU bash, version 3.1.0(1)-release (x86_64-unknown-linux-gnu)
Copyright (C) 2005 Free Software Foundation, Inc.
root@test-server:~#

cd commit-comments

root@hi-predictor-api:~/commit-comments# ./test.sh
- Added new link parsing function
- Stored Hello World in string
- Inline commit comment
- Another inline commit comment
- final inline commit comment
- Added a new function to basic.cpp
- Added forEach function for NodeList elements

Also, I made another script with your modified contents and executed it: son-of-lilit.sh

root@test-server:~/commit-comments# ./son-of-lilit.sh
.ccignore
************
.ccignore
.ccignore
ccignore
LICENSE
LICENSE
LICENSE
README.md
README.md
md
post-commit
post-commit
post-commit
prepare-commit-msg
prepare-commit-msg
prepare-commit-msg
test-files/basic.cpp
basic.cpp
cpp
test-files/thing.js
thing.js
js
test.sh
test.sh
sh

Could you please add your Operating System & version & any other shells you have running?

Thank you!

@SonOfLilit
Copy link
Author

$ if [[ "hello world" =~ ".*" ]]; then echo "it worked"; els
e echo "it failed"; fi
sh.exe": conditional binary operator expected
sh.exe": syntax error near `=~'
$ if [[ "hello world" =~ .* ]]; then echo "it worked"; else
echo "it failed"; fi
sh.exe": conditional binary operator expected
sh.exe": syntax error near `=~'

also:

[[ a =~ b ]]
sh.exe": conditional binary operator expected
sh.exe": syntax error near `=~'

I'm running Git Windows' bash on Windows 7. It has never shown deviance from standard before, but who knows.

$ git --version
git version 1.9.5.msysgit.0

I'll install 2.x and see if it helps.

However, I feel that the real issue I reported was this:

https://stackoverflow.com/questions/2412450/git-pre-commit-hook-changed-added-files

but LarryH there points out that you'll pick up comments that are not up for being committed. That's bad because I love git add -i. He gives a solution, which is too big and architectural for me to PR without consulting with you first, and also I'm not sure I have time to do it, being as awful as I am in bash scripting.

In the meantime, I'll have to uninstall this :-(

and it's far worse than some syntax error I have a slow workaround for, and even from .ccignore having different syntax than .gitignore's, don't you think?

@SonOfLilit SonOfLilit changed the title conditional binary operator expected Files that aren't in the commit are searched for commit comments Jan 15, 2016
@jryio
Copy link
Owner

jryio commented Jan 16, 2016

Ah sorry for glossing over the second issue. Let me tackle those in order:

  • Using git diff --cached --name-status --diff-filter=ACM is a great solution to cut down on the number of files to be searched. Additionally, as you've mentioned, if you're using git add -i and partially committing files, this again is a better method. Only looking at the staged files should have been my initial approach but I did not come across this command (so thank you for that 😄).
    @SonOfLilit If you'd like to open a PR to add the git diff implementation I would be really happy to help you (maybe practice your Bash scripting? 🚀). Regardless this is something I'll begin to work on right away.
  • Having a different syntax is somewhat troublesome. I am considering reverting the .ccignore to be .gitignore syntax.
    Part of the reason I allowed filetypes like .cpp and .sh to be in .ccignore is due to several projects I have with thousands of files which I don't directly contribute to or maintain. In hindsight this feature is almost entirely useless, and keeping with the .gitignore syntax is better for usability.
    And as mentioned above, using the --diff-filter option removes the need for .ccignore entirely
  • Just to double back for a second: If you're running Git Bash on Windows the =~ operator is actually unsupported!
    Perhaps this would solve the issue: http://stackoverflow.com/questions/15510083/syntax-error-operator-in-msysgit-bash

Edit: For portability I will no longer use the =~ operator and move to using echo $array | grep 'element' instead.

@SonOfLilit
Copy link
Author

First the small things:

=~ indeed seems to be unsupported by old Git for Windows' bash. I installed a new one, it should solve that, but again, I think this implementation is buggy in ways that will affect me so I won't use it until that is solved.

An extension is ignored in .gitignore syntax by *.ext. Just one key more, definitely worth it. Also, the right time to make a breaking change, since the current syntax is yet undocumented.

You were looking at araqnid's response to that SO question. That's the less interesting one (it gives a better way to do more or less the same thing, but that's the wrong thing to do.)

Read LarryH's response. If you want to adopt his method, I will consider trying my hands at it, but it's not that simple (especially removing the comments that were staged but not others... I think we'll need to do it in the temporary copy, then generate a patch, then apply it to the working dir... but this smells like "there will be horrible complications".)

jryio added a commit that referenced this issue Jan 16, 2016
This commit resolves an issue with a GNU grep command using the -P flag.
As a result, users without GNU Grep could not use the Perl Regular
Expressions flag.

This command has been removed and replaced with pcregrep which yields
the same results and also allows forward look-aheads.

Additionally, the test-files have been added to the .ccignore file so
cloning the repository does not cause users to have the test-files in
their first commit message.

Closes Issue #1
@jryio
Copy link
Owner

jryio commented Jan 16, 2016

Pardon the reference, wrong issue number.

=~ is a perfectly valid operator in Bash, however I would also agree that replacing it with an echo $var | grep would be more appropriate and less prone to portability issues. Working on that now.

I did read LarryH's response, and saw his solution for creating a temp directory, but I think a simpler approach exists.

If we're solving the issue of only reading & removing comments from staged files (either Added, Modified, or Copied) then storing the result of git diff --name-status --diff-filter=ACM in the prepare-commit-msg hook and passing to the post-commit hook through a temporary file would suffice. Then the post-commit hook would be able to remove the comments only from those files which were staged.

Additionally, @hauleth just submitted Pull Request #3 which attempts to leave a clean repository after removing comments (no longer requiring a second commit once files are removed).

Would you say that tying your suggestion with @hauleth's addition would fully cover the issues?

@SonOfLilit
Copy link
Author

The problem is that I can (and often do) "add" only part of a file. git add -i. Because of that, the temporary directory solution is really
essential.

On Sat, Jan 16, 2016 at 8:35 AM Jacob Young notifications@github.com
wrote:

Pardon the reference, wrong issue number.

=~ is a perfectly valid operator in Bash, however I would also agree that
replacing it with an echo $var | grep would be more appropriate and less
prone to portability issues. Working on that now.

I did read LarryH's response, and saw his solution for creating a temp
directory, but I think a simpler approach exists.

If we're solving the issue of only reading & removing comments from staged
files (either Added, Modified, or Copied) then storing the result of git
diff --name-status --diff-filter=ACM in the prepare-commit-msg hook and
passing to the post-commit hook through a temporary file would suffice.
Then the post-commit hook would be able to remove the comments only from
those files which were staged.

Additionally, @hauleth https://github.com/hauleth just submitted Pull
Request #3 #3 which
attempts to leave a clean repository after removing comments (no longer
requiring a second commit once files are removed).

Would you say that tying your suggestion with @hauleth
https://github.com/hauleth's addition would fully cover the issues?


Reply to this email directly or view it on GitHub
#1 (comment)
.

@jryio
Copy link
Owner

jryio commented Jan 16, 2016

Okay I clearly understand now.

Personally I do not use interactive Git adding using git add -i but I would assume you would be using the patch to select certain sections of a file diff to add, is that correct?

I'll try to incorporate this solution, however as you said, it might be a large architectural - but I will certainly attempt it

@SonOfLilit
Copy link
Author

Woops, I meant git add -p, it's easier to use cousin. (I have it aliased
to a for so long that I don't even remember the right syntax.)

Basically, with git you can commit an index that's completely unrelated to
what's in your working copy at the time of committing. And many people do,
quite often, me included.

On Sat, Jan 16, 2016 at 9:20 PM Jacob Young notifications@github.com
wrote:

Okay I clearly understand now.

Personally I do not use interactive Git adding using git add -i but I
would assume you would be using the patch to select certain sections of a
file diff to add, is that correct?

I'll try to incorporate this solution, however as you said, it might be a
large architectural - but I will certainly attempt it


Reply to this email directly or view it on GitHub
#1 (comment)
.

Repository owner deleted a comment Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants