You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by asutosh936 <gi...@git.apache.org> on 2018/03/12 01:08:16 UTC
[GitHub] zookeeper pull request #488: ZOOKEEPER-2993 - Removed 'generated' line from ...
GitHub user asutosh936 opened a pull request:
https://github.com/apache/zookeeper/pull/488
ZOOKEEPER-2993 - Removed 'generated' line from .gitignore
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/asutosh936/zookeeper ZOOKEEPER-2993
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/488.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #488
----
commit a02ef3db8d18cc738a1a3e24eafcaa0b5379d297
Author: asutosh936 <as...@...>
Date: 2018-02-25T23:40:14Z
ZOOKEEPER-2936 - Removed duplicate code
commit 6a17613be090835286f6fd15b3fb5c40e269a25f
Author: asutosh936 <as...@...>
Date: 2018-03-01T01:22:30Z
ZOOKEEPER-2936 - Deleted duplicate code
commit ad55832eafbe4e341e113a1bdeb0385674025b8e
Author: asutosh936 <as...@...>
Date: 2018-03-03T18:09:01Z
Merge branch 'master' of https://github.com/apache/zookeeper
commit 67d454b5ab91b72f20b878c90c7d9f7a45bcdf88
Author: asutosh936 <as...@...>
Date: 2018-03-07T01:39:40Z
Merge branch 'master' of https://github.com/apache/zookeeper
commit 13f8a235bad6f361120babf96f05aa2fc0cdcdb0
Author: asutosh936 <as...@...>
Date: 2018-03-12T01:06:57Z
ZOOKEEPER-2993 - Removed 'generated' line from .gitignore
----
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by hanm <gi...@git.apache.org>.
Github user hanm commented on the issue:
https://github.com/apache/zookeeper/pull/488
Committed to master, 3.5, and 3.4.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by tamaashu <gi...@git.apache.org>.
Github user tamaashu commented on the issue:
https://github.com/apache/zookeeper/pull/488
@asutosh936 Sorry but I don't think that the generated directory is ignored by a defect.
We should never add generated files to git, they must be ignored.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by asutosh936 <gi...@git.apache.org>.
Github user asutosh936 commented on the issue:
https://github.com/apache/zookeeper/pull/488
@afine - Currently there is a folder named 'generated' at below location.
https://github.com/apache/zookeeper/tree/master/src/java/main/org/apache/jute/compiler/generated
As per the defect any java file added to above location, it will be ignored by git to add to repository.
---
[GitHub] zookeeper pull request #488: ZOOKEEPER-2993 - Removed 'generated' line from ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/488
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/488
@tamaashu Generally speaking I agree with you, but that directory is somewhat different. There is code in it which we don't generate during our build process, but it's been in git since the beginning of ZooKeeper.
I have no idea why that directory named 'generated'. It's quite misleading.
@afine @tamaashu please take a closer look before making a judgement.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/488
@asutosh936 I agree with @anmolnar That directory exists, it is currently in version control, and I don't think any generated files actually end up there. My guess is that it is there purely for historical reasons and to contain the rcc.jj file.
In other words, I don't think your change to the gitignore will impact that directory. According to the gitignore documentation (https://git-scm.com/docs/gitignore):
> If the pattern does not contain a slash /, Git treats it as a shell glob pattern and checks for a match against the pathname relative to the location of the .gitignore file (relative to the toplevel of the work tree if not from a .gitignore file).
Since there are not any top level files called `generated` I don't think this line in the gitignore has any impact on the repository. Am I missing something here?
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/488
> The change is to remove 'generated' keyword from .gitignore because it prevents the dir and files under generated dir to be added to the repo. The compilation later fails due to the missing dir and files.
Which files are you referring to? I don't think we have any top level `generated` directory and I also don't think we would want to add such files to the repo.
Don't worry about squashing, I should be able to take care of that when it comes time to commit.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by asutosh936 <gi...@git.apache.org>.
Github user asutosh936 commented on the issue:
https://github.com/apache/zookeeper/pull/488
@anmolnar @afine - Thank You for the review comments.
The change is to remove 'generated' keyword from .gitignore because it prevents the dir and files under generated dir to be added to the repo. The compilation later fails due to the missing dir and files.
Also I am trying to squash the previous commits but somehow git is not allowing me to do so. Let me know if the changes looks good and to be merged or I need to close this PR and create new one.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by jason95 <gi...@git.apache.org>.
Github user jason95 commented on the issue:
https://github.com/apache/zookeeper/pull/488
Thanks everyone!! @afine @asutosh936 @anmolnar @tamaashu
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/488
@asutosh936 lgtm
It would be nice to squash your commits into one. Thanks.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/488
I agree with @anmolnar. This change should be fine from what I can tell but it would be nice to have a note as to why the change was made.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/488
@anmolnar It looks like you are right. removing this line does not impact which files git ignores. Thanks for persisting @jason95 @asutosh936
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/488
@afine In the other way around: if there's no '/' at the beginning of the keyword, git will check the entire path relative to the root for the string `generated` and if it finds a match, it'll ignore all files underneath.
My suggestion is to remove `generated` keyword from .gitignore - like in this PR -, because 'real' generated folders are already excluded explicitly. There could be other possible solutions to the problem, I'm open to them.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/488
I would like to have this change in 3.4 so I will merge after the 3.4.12 release.
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by asutosh936 <gi...@git.apache.org>.
Github user asutosh936 commented on the issue:
https://github.com/apache/zookeeper/pull/488
@anmolnar - Please review
---
[GitHub] zookeeper issue #488: ZOOKEEPER-2993 - Removed 'generated' line from .gitign...
Posted by jason95 <gi...@git.apache.org>.
Github user jason95 commented on the issue:
https://github.com/apache/zookeeper/pull/488
Hi folks,
Sorry for not keeping up with the jira.
@asutosh936 - thanks for the PR and others for reviewing.
Here are some info - hope it will shed some clarity.
- There exists in the repo/source code a dir named: src/java/main/org/apache/jute/compiler/generated
- In that dir it has Rcc.java and other files
- Files in that dir is required to for successful compilation
- However, the .gitignore has the keyword "generated"
- when I add the source code tree into out company repo, the src/java/main/org/apache/jute/compiler/generated was effectively ignored - and thus not added into our company repo
- I ran a jenkins job to compile the code from the repo - it failed - due to the missing Rcc and other required java sources under that excluded directory.
When I did a git status - everything was in sync even though my workspace has the Rcc.java - but my company repo actually is missing that dir. My local compile succeeds every time. But jenkins job compile using company repo always fails. After some comparison - then I noticed the missing dir on the company repo side.
Once I removed the generated keyword from the .gitignore - git immediately detected the generated dir in my workspace as new. I was able to git add/commit/push to make the jenkins compile successful.
Hope this help clarify the situation.
I prefer to have generated keyword removed. In the future, if the compile will generate artifacts that should be ignored, then modify the .gitignore and rename the src/java/main/org/apache/jute/compiler/generated dir - so the 2 does not conflict.
My 2 cents.
Thanks
---