Bug 436200 - Directory with .git subdirectory incorrectly added to repository
Summary: Directory with .git subdirectory incorrectly added to repository
Status: REOPENED
Alias: None
Product: JGit
Classification: Technology
Component: JGit (show other bugs)
Version: 3.3.2   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 431257
  Show dependency tree
 
Reported: 2014-05-29 13:28 EDT by John Eblen CLA
Modified: 2016-04-05 05:43 EDT (History)
10 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description John Eblen CLA 2014-05-29 13:28:12 EDT
Consider the following sequence inside an empty directory, where "jgit" is command-line JGit:

> mkdir foo
> mkdir foo/.git
> jgit init
Initialized empty Git repository in /data/project/./.git
> jgit add foo
> jgit status
# On branch master
# Changes to be committed:
# 
# 	new file:   foo
> jgit commit -m "Initial commit"
[master 147ce49bb36ce1577cb4d42db0638e8f1a1b75fb] Initial commit

JGit commits an empty directory to the repository! Instead, "jgit add foo" should not add any files to be committed. This same behavior occurs if foo/.git is a real Git repository or if foo contains other files.
Comment 1 Roland Schulz CLA 2014-06-08 01:15:13 EDT
The problem is that treewalk.FileTreeIterator.FileEntry.FileEntry is setting mode = FileMode.GITLINK if a directory contains a ".git" folder. Not sure what the purpose of GITLINK is but this clearly causes different behavior than cgit.
Comment 2 Robin Rosenberg CLA 2014-06-09 03:38:11 EDT
GITLINK is for submodules, i.e. a reference to another repo that should be checked out that the location of the GITLINK.

C Git also adds foo if it contains a real repo with commits, so that's the same.

If foo is not a real repo, when JGit is wrong.

# mkdir x
# cd x
# git init
Initialized empty Git repository in /Users/me/tmp/x/.git/
# mkdir foo
# cd foo
# git init
Initialized empty Git repository in /Users/me/tmp/x/foo/.git/
# touch a
# git add a
# git commit -m "Added a"
[master (root-commit) 30bb68a] Added a
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 a
# cd ..
# git add .
# git status
On branch master

Initial commit

Changes to be committed:
  (use "git rm --cached <file>..." to unstage)

	new file:   foo
Comment 3 Roland Schulz CLA 2014-06-09 10:36:33 EDT
cgit calls resolve_gitlink_ref before deciding it is a submodule (thus the .git folder has to have a valid HEAD file). And cgit doesn't check for a submodule if any files inside a directory is already tracked. Also cgit has a flag DIR_NO_GITLINKS to avoid checking for submodules. If jgit added that flag, that would be the easiest solution to fix our depending bug.

The full sets of rules in cgit (from dir.c):
 * Case 1: If we *already* have entries in the index under that
 * directory name, we always recurse into the directory to see
 * all the files.
 *
 * Case 2: If we *already* have that directory name as a gitlink,
 * we always continue to see it as a gitlink, regardless of whether
 * there is an actual git directory there or not (it might not
 * be checked out as a subproject!)
 *
 * Case 3: if we didn't have it in the index previously, we
 * have a few sub-cases:
 *
 *  (a) if "show_other_directories" is true, we show it as
 *      just a directory, unless "hide_empty_directories" is
 *      also true, in which case we need to check if it contains any
 *      untracked and / or ignored files.
 *  (b) if it looks like a git directory, and we don't have
 *      'no_gitlinks' set we treat it as a gitlink, and show it
 *      as a directory.
 *  (c) otherwise, we recurse into it.
Comment 4 Roland Schulz CLA 2014-06-11 12:22:07 EDT
I'm not sure where the DIR_NO_GITLINKS option should be added. One could do it similar to recursive and put it into TreeWalk (if option is set then enter subtree also for GITLINK). Another option would be to add the option to FileTreeIterator/WorkingTreeIterator. The advantage of that would be that one could use it with e.g. AddCommand without any changes to AddCommand (one could pass in ones own TreeIterator with the option set).
Comment 5 Robin Rosenberg CLA 2014-06-13 10:20:53 EDT
Pushed a fix for the simple part of the issue: https://git.eclipse.org/r/28479
Comment 6 Shawn Pearce CLA 2014-11-24 19:20:12 EST
DHT support was removed in 130ad4ea4407316b1fd115db456c4aa950907196.
A JDBC/SQL type backend is probably infeasible.
Comment 7 Shawn Pearce CLA 2014-11-24 19:20:56 EST
(In reply to Shawn Pearce from comment #6)
> DHT support was removed in 130ad4ea4407316b1fd115db456c4aa950907196.
> A JDBC/SQL type backend is probably infeasible.

Ignore this, I updated the wrong bug. :(
Comment 8 Kevin Corcoran CLA 2015-06-29 17:35:02 EDT
Hi,

I'd like to voice my support for the solution proposed by Roland Schulz, in which he suggests implementing the ability to honor something like DIR_NO_GITLINKS by adding it to FileTreeIterator/WorkingTreeIterator.

Currently, I am accomplishing this by using my own implementation of WorkingTreeIterator - which is a direct copy of FileTreeIterator, except that it never uses FileMode.GITLINK.  However, it would highly preferable if FileTreeIterator simply had an option for this, and I didn't have to maintain my own implementation.

This change seems fairly straightforward, and I am willing to submit a patch for it.  Is this a direction that the project maintainers agree with?  Or, are there alternative solutions that need to be considered first?

Thanks!
Comment 9 Christian Halstrick CLA 2015-06-30 03:36:47 EDT
I fear that DIR_NO_GITLINKS in WorkingTreeIterator may become complicated when you clone repos which do have submodules. What is if dir/.git is a file and not a folder. Or what is if the index contains a submodule underneath at path 'dir' but the HEAD commit contains a file dir/foo.txt and the workingtree contains a file dir/.git/HEAD. I can imagine that in your use case you are sure that you don't have submodules ... but it's hard to tell that a certain repo will never contain a submodule. Even if you prevent it locally they may come through fetches.

Is there something wrong with better checking whether a added folder contains a valid git repo? As suggested in https://git.eclipse.org/r/28479. Kevin, wouldn't that solve your problem also? Then we should especially add some tests to https://git.eclipse.org/r/28479 to first make sure the behaviour is like you want it. Such a solution would not only be a benefit for the small amount of users who really instantiate own FileTreeIterators and are able to specify low-level options. But also EGit and other JGit-consumers may benefit
Comment 10 Roland Schulz CLA 2015-06-30 04:00:35 EDT
https://git.eclipse.org/r/28479 won't solve our depending bug 431257. In its case it doesn't matter whether it is a real git repo or not. The reason is that in that case a non-standard git folder (".ptp-sync") is used. And thus if a ".git" exists than this is unrelated.
Comment 11 Chris Price CLA 2015-06-30 04:53:31 EDT
(In reply to Christian Halstrick from comment #9)
> I fear that DIR_NO_GITLINKS in WorkingTreeIterator may become complicated
> when you clone repos which do have submodules. What is if dir/.git is a file
> and not a folder. Or what is if the index contains a submodule underneath at
> path 'dir' but the HEAD commit contains a file dir/foo.txt and the
> workingtree contains a file dir/.git/HEAD. I can imagine that in your use
> case you are sure that you don't have submodules ... but it's hard to tell
> that a certain repo will never contain a submodule. Even if you prevent it
> locally they may come through fetches.

Hello Christian, thanks for the reply.  (I'm a co-worker of Kevin's, working on the same issue w/him.)

I believe that in the solution we're proposing, DIR_NO_GITLINKS would only ever be set explicitly by a user; it wouldn't be the default behavior for the iterator, so it'd be up to the caller to determine whether or not the setting was appropriate for them?  (i.e. they'd have to consider exactly what you're describing where .git might be a file instead of a directory and make sure that the behavior was appropriate for their use case.)

 
> Is there something wrong with better checking whether a added folder
> contains a valid git repo? As suggested in https://git.eclipse.org/r/28479.
> Kevin, wouldn't that solve your problem also?

I think that would be a good idea in addition to the support for a DIR_NO_GITLINKS setting, but it wouldn't actually solve our particular use case.  In our use case we actually need to be able to ignore submodule trees entirely (even if they are entirely valid git submodules) and still be able to track changes to files that exist in those directories.  From reading the cgit source code, it seems like this is within the spirit of the capability provided by the DIR_NO_GITLINKS flag.

> Then we should especially add
> some tests to https://git.eclipse.org/r/28479 to first make sure the
> behaviour is like you want it.  Such a solution would not only be a benefit
> for the small amount of users who really instantiate own FileTreeIterators
> and are able to specify low-level options. But also EGit and other
> JGit-consumers may benefit

Agree that that patch probably hits a broader set of use cases than the one we are proposing.  We would be happy to see that merged and then we could add another patch on top of that that would add something like ".setNoGitLinks" on the WorkingTreeIterator class, if that sounds like something that could be accepted.  We would be happy to help polish off 28479 and do whatever you feel is necessary to get that merged first, if you think that it'd be OK for us to add on the additional support for DIR_NO_GITLINKS as well.  And we'd be happy to spend some time adding additional test coverage (I've already got a few new tests written that I put together in a branch for our DIR_NO_GITLINKS use case.)
Comment 12 Christian Halstrick CLA 2015-07-02 10:59:30 EDT
ok, I understood that https://git.eclipse.org/r/28479 won't help you. From my point of view it would be ok to support DIR_NO_GITLINKS in the JGit API. But I would prefer if we really offer that functionality like in native git's dir.c . When I read that code I see that they solved a lot of problems I have in mind because they for example double check that a certain path is NOT in the index before they evaluate DIR_NO_GITLINKS. Roland mentioned the rules in https://bugs.eclipse.org/bugs/show_bug.cgi?id=436200#c3 . 

If we implement that same rules and have tests for that I have no problem with this feature. I don't think JGITs porcellain API will publish this (native git also doesn't offer that option to porcellain users, or?). I guess that means that means that the implementation can't be in WorkingTreeIterator only. We need info from dircache and workingtree to support this feature.

Would you like to propose something in gerrit to implement this?
Comment 13 Christian Halstrick CLA 2015-07-03 04:26:03 EDT
Two question came up here while discussing with colleagues:

1) Can you describe the use case where you want to use that feature?

2) What should happen with the contents underneath the .git folder? Up to know we talked about the files in the working tree of a submodule. A setting of DIR_NO_GITLINKS should force a TreeWalk to visit also that workingtree files. But whats with the files in the .git folder itself?

01. /proj
02. /proj/.git/
03. /proj/.git/HEAD
04. /proj/.git/config
05. /proj/.git/...
06. /proj/.gitattributes
07. /proj/a.txt
08. /proj/src/
09. /proj/src/b.txt
10. /proj/src/subm/
11. /proj/src/subm/.git/
12. /proj/src/subm/.git/HEAD
13. /proj/src/subm/.git/config
14. /proj/src/subm/c.txt

Currently JGits TreeWalk/FileTreeIterator (if you set it to non-recursive and jump into subtrees explicitly) would return 06(file),07(file),08(tree),09(file),10(gitlink) . Which differences do you expect with DIR_NO_GITLINKS regarding 11), 12), 13). What prerequisites should be fullfilled so that 12),13) are also reported?
Comment 14 Chris Price CLA 2015-07-03 07:50:14 EDT
(In reply to Christian Halstrick from comment #13)
> Two question came up here while discussing with colleagues:
> 
> 1) Can you describe the use case where you want to use that feature?

Sure.  We are programmatically creating and managing a git repo to track some files that our users are creating.  We have our own bare repo / git dir that is outside of the working tree that the users are interacting with.  In the case where they happen to do a git clone of some repo of their own, somewhere inside the directory we're trying to monitor, we still want to be able to track the files inside of those nested directories.  We actually don't care at all that they happen to be using git to deploy some of their files.

So, in our case, what we basically want is to be able to recurse into directories regardless of whether they have a .git dir inside of them, and we basically want to ignore any .git directories that do exist anywhere in the tree.

We are currently achieving this by calling 'AddCommand.setWorkTreeIterator', and passing in a custom WorkTreeIterator.  Our custom WorkTreeIterator is currently a complete copy/paste of yours, with roughly a three-line change that allows us to recurse into all directories even if they contain a .git dir.  This works for us, but it's a lot of duplicate code that makes me nervous about staying in sync with your upstream code base as new releases come out, and we'd definitely be interested in contributing upstream if there was a mutually beneficial solution.

Failing the full-fledged "DIR_NO_GITLINK" support, if that change seems too invasive, maybe we could just propose a refactor to WorkTreeIterator that would allow users to subclass it and override the directory recursion strategy, without needing to maintain a full copy of the WorkTreeIterator class?  Just throwing that out as an idea; I'm willing to put some effort into whatever approach seems best to you.

> 2) What should happen with the contents underneath the .git folder? Up to
> know we talked about the files in the working tree of a submodule. A setting
> of DIR_NO_GITLINKS should force a TreeWalk to visit also that workingtree
> files. But whats with the files in the .git folder itself?
> 
> 01. /proj
> 02. /proj/.git/
> 03. /proj/.git/HEAD
> 04. /proj/.git/config
> 05. /proj/.git/...
> 06. /proj/.gitattributes
> 07. /proj/a.txt
> 08. /proj/src/
> 09. /proj/src/b.txt
> 10. /proj/src/subm/
> 11. /proj/src/subm/.git/
> 12. /proj/src/subm/.git/HEAD
> 13. /proj/src/subm/.git/config
> 14. /proj/src/subm/c.txt
> 
> Currently JGits TreeWalk/FileTreeIterator (if you set it to non-recursive
> and jump into subtrees explicitly) would return
> 06(file),07(file),08(tree),09(file),10(gitlink) . Which differences do you
> expect with DIR_NO_GITLINKS regarding 11), 12), 13). What prerequisites
> should be fullfilled so that 12),13) are also reported?

For our use case, we'd want to totally ignore entries 11-13.  If that is not the behavior that cgit would display, though, it'd be fine if 11/12/13 were reported; I believe we could probably still achieve our use case by just adding an entry to our '.gitignore' file?
Comment 15 Chris Price CLA 2015-07-03 08:03:27 EDT
(In reply to Christian Halstrick from comment #12)
> ok, I understood that https://git.eclipse.org/r/28479 won't help you. From
> my point of view it would be ok to support DIR_NO_GITLINKS in the JGit API.
> But I would prefer if we really offer that functionality like in native
> git's dir.c . When I read that code I see that they solved a lot of problems
> I have in mind because they for example double check that a certain path is
> NOT in the index before they evaluate DIR_NO_GITLINKS. Roland mentioned the
> rules in https://bugs.eclipse.org/bugs/show_bug.cgi?id=436200#c3 . 
> 
> If we implement that same rules and have tests for that I have no problem
> with this feature. I don't think JGITs porcellain API will publish this
> (native git also doesn't offer that option to porcellain users, or?).

I think you are correct that native git does not expose this flag via porcelain.

I made a very crude attempt to put together a proof-of-concept before we posted on this bug report.  You can see it here:

https://github.com/cprice404/jgit/compare/v3.7.1...cprice404:bug/v3.7.1.201504261725-r/add-support-for-NO_GITLINKS

(Or, I can put it into gerritt if that's easier, I've just never done that before and am not quite sure how.)

That branch is definitely not in a mergeable state, it was more of an attempt to just figure out where would be a feasible place to expose the flag in the API, and figure out how hard it would be to get the flag passed all the way down through the code to where it was needed in the WorkTreeIterator.

I definitely understand your preference to implement it in a way that is compatible with how it is implemented in native git, and would be happy to work on a patch for that, but might need some additional guidance on how to achieve it.  It seemed like there would be at least a couple of challenges to deal with there:

1. Where to expose the setting in the JGit API?  What class would it be exposed on?  Repository?
2. How to check the index from inside the logic of the WorkTreeIterator, to see if the path is already in the index prior to evaluating DIR_NO_GITLINKS?

> I
> guess that means that means that the implementation can't be in
> WorkingTreeIterator only. We need info from dircache and workingtree to
> support this feature.
> 
> Would you like to propose something in gerrit to implement this?

I'd be happy to give it a shot if you like.  Any other pointers first?  And do you think it would be preferable, less risky, or less work to look into a way to refactor WorkTreeIterator to make it easier to subclass?

Thank you very much for your consideration.
Comment 16 Matthias Sohn CLA 2015-07-03 19:12:43 EDT
how to contribute patches via Gerrit is explained in our contributor guide
https://wiki.eclipse.org/EGit/Contributor_Guide#Contributing_Patches
Comment 17 Christian Halstrick CLA 2015-07-06 03:46:19 EDT
What you are proposing looks so good that we can start discussing on code (in gerrit) and not here anymore. At least in my experience that's faster way to get the feature in. So, please follow Matthias link and propose what you have. Don't worry if it is not yet mergable or finetuned. We'll fix that in the review.
Comment 18 Chris Price CLA 2015-07-06 04:56:00 EDT
(In reply to Christian Halstrick from comment #17)
> What you are proposing looks so good that we can start discussing on code
> (in gerrit) and not here anymore. At least in my experience that's faster
> way to get the feature in. So, please follow Matthias link and propose what
> you have. Don't worry if it is not yet mergable or finetuned. We'll fix that
> in the review.

I've uploaded it to gerritt:

https://git.eclipse.org/r/#/c/51402/

Is there something that I should do in order to link it to this bug report?

I also added a few clarifying comments to the code, not sure if they are visible to you or not (apologies, I've never used Gerritt before).

I'd be happy to work on that patch some more if it seems like the right direction.  Alternately, if it seems too complex, I could try to work up an alternate patch that would just make WorkTreeIterator easier to subclass and override, for comparison.
Comment 19 Roland Schulz CLA 2015-07-06 04:58:26 EDT
You have to click "Reply" and "Post" to make them visible.
Comment 20 Matthias Sohn CLA 2015-07-06 05:15:46 EDT
(In reply to Chris Price from comment #18)
> (In reply to Christian Halstrick from comment #17)
> > What you are proposing looks so good that we can start discussing on code
> > (in gerrit) and not here anymore. At least in my experience that's faster
> > way to get the feature in. So, please follow Matthias link and propose what
> > you have. Don't worry if it is not yet mergable or finetuned. We'll fix that
> > in the review.
> 
> I've uploaded it to gerritt:
> 
> https://git.eclipse.org/r/#/c/51402/
> 
> Is there something that I should do in order to link it to this bug report?

I added the bug number to the commit message footer which gerrit will linkify
Comment 21 Chris Price CLA 2015-07-06 05:18:07 EDT
(In reply to Roland Schulz from comment #19)
> You have to click "Reply" and "Post" to make them visible.

Great, thanks, I think I've done that.
Comment 22 Eclipse Genie CLA 2015-07-09 18:27:45 EDT
Gerrit change https://git.eclipse.org/r/51475 was merged to [master].
Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=88168c6d603833a7033af973af0282dc7056d7ef
Comment 23 Eclipse Genie CLA 2015-07-09 19:15:57 EDT
Gerrit change https://git.eclipse.org/r/51614 was merged to [master].
Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=ad16eaac6ebc6daa994a6dbdccb687094485c36f
Comment 24 Eclipse Genie CLA 2015-07-09 19:38:00 EDT
Gerrit change https://git.eclipse.org/r/51615 was merged to [master].
Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=37a1e4beaa9e31e642be3aef645f060eb96b1b86
Comment 25 Eclipse Genie CLA 2015-07-14 05:03:26 EDT
New Gerrit change created: https://git.eclipse.org/r/51906
Comment 26 Chris Price CLA 2015-07-16 01:40:17 EDT
I submitted a new Gerritt branch to start working on the improvements / changes we discussed related to how to expose the 'dir_no_gitlinks' setting:

https://git.eclipse.org/r/#/c/51906/1

But I'm currently stuck on figuring out what the API should look like for a caller to pass in that flag.  I think that once I get past that I should be able to figure out most of the rest of it fairly quickly, so any advice anyone has on how to handle that API would be greatly appreciated!
Comment 27 Chris Price CLA 2015-08-07 11:03:27 EDT
Hello,

I'm very eager to keep working on this; our company is building a feature that relies on this and we are using a really hacky workaround right now, so we're eager to devote some developer time to putting together a patch for this, but I'm stuck on figuring out what an API would look like that would be considered acceptable for merge.  As soon as I can get past that problem I'm confident we can crank something out and have a patch ready pretty quickly...

Can anyone help me sort out the API details?

Thanks in advance for any input anyone can give!
Comment 28 Andrey Loskutov CLA 2015-08-09 12:22:32 EDT
Just coming here from completely another, unrelated bug 474064 where the extra call to the file.exists() seem to be related to the performance drop: why do we need to check *each and every* directory for submodules at all, assuming we have no submodules in the main repo?

From http://git-scm.com/docs/git-submodule:

"A record in the .gitmodules (see gitmodules[5]) file at the root of the source tree assigns a logical name to the submodule and describes the default URL the submodule shall be cloned from."

So in case we have a simple repo with no submodules, we could just made one single file check if .gitmodules exists and set the flag "hasSubModules" on the Repository object/WorkingTreeOptions? The FileTreeIterator.FileEntry can then avoid calling file.exists() in that case, reducing file I/O. AFAIK full index diff reads file attributes 2 times for each directory: one time for the current one, and one time for the possible existing .git child - and this is one time to much.

Is that approach too naive, I'm missing something?
Comment 29 Eclipse Genie CLA 2015-08-09 18:08:37 EDT
New Gerrit change created: https://git.eclipse.org/r/53450
Comment 30 Andrey Loskutov CLA 2015-08-09 18:15:26 EDT
(In reply to Eclipse Genie from comment #29)
> New Gerrit change created: https://git.eclipse.org/r/53450

This above is the partial solution, the one I need for bug 474064 only.

I think the problem is that the FileEntry currently does not have any access to the git submodules information.

To properly identify if a directory containing .git file should be marked as a FileMode.GITLINK or FileMode.TREE, the FileTreeIterator should create a config object from .gitmodules at the beginning (if any .gitmodule exists) and pass it to each FileEntry it's creates. This later can decide if the current directory matches the git module path from the configuration file and so finally set the right mode.

But this is my naive view, based on the assumption that each "valid" submodule has a record in .gitmodules file.
Comment 31 Chris Price CLA 2015-08-14 11:25:58 EDT
(In reply to Andrey Loskutov from comment #30)
> (In reply to Eclipse Genie from comment #29)
> > New Gerrit change created: https://git.eclipse.org/r/53450
> 
> This above is the partial solution, the one I need for bug 474064 only.
> 
> I think the problem is that the FileEntry currently does not have any access
> to the git submodules information.
> 
> To properly identify if a directory containing .git file should be marked as
> a FileMode.GITLINK or FileMode.TREE, the FileTreeIterator should create a
> config object from .gitmodules at the beginning (if any .gitmodule exists)
> and pass it to each FileEntry it's creates. This later can decide if the
> current directory matches the git module path from the configuration file
> and so finally set the right mode.
> 
> But this is my naive view, based on the assumption that each "valid"
> submodule has a record in .gitmodules file.

For what I'm trying to do, I also need the ability to pass in more data into the FileEntry.  In my case it's the WorkingTreeOptions that I need access to.  So perhaps it would make sense to just pass in the entire Config object so that both things would be accessible?
Comment 32 Chris Price CLA 2015-08-14 11:31:32 EDT
Pasting in a comment from an old branch where I was proposing some tests to Christian, so it's easier to find this list in a few days when I start writing these tests :)

--------------------------------------

One other thing I've been thinking about in terms of building out the more complete implementation that looks at the index in addition to the DIR_NO_GITLINKS flag... I think it would probably be helpful to write out all the test cases that I need to cover first.  So, with a test directory structure like this:

??? foo
    ??? bar
        ??? .git
        ?   ??? ...
        ??? README1.md
        ??? README2.md

Off of the top of my head, here are the tests I can think of:

    new repo in foo, `git add bar`, verify that we get a GITLINK, verify that 'status' doesn't show any other files
    new repo in foo, `git add bar/README1.md`, verify that we get a regular file, no GITLINKs, verify that `status` shows README2.md as unstaged
    new repo in foo, set DIR_NO_GITLINKS on workTreeOptions, `git add bar`, verify that `status` both README1 and README2 are added, no GITLINKs? And maybe do another `git add bar` *without* setting DIR_NO_GITLINKS after that, and validate that it still doesn't create a GITLINK?

Other things we should make sure we cover?
Comment 33 Chris Price CLA 2015-08-25 05:40:50 EDT
Added some tests to https://git.eclipse.org/r/#/c/51483 , hopefully it's pretty much ready to go.
Comment 34 Preben Ingvaldsen CLA 2016-03-01 14:44:54 EST
I'm currently taking over implementation of the DIR_NO_GITLINKS functionality from Chris Price. I have an initial implementation of the tests he mentioned here:

https://github.com/fpringvaldsen/jgit/commit/1afb804c124f530fabbc506ca9a6e3b9c5dfaf2c

Christian Halstrick, could you take a look at those tests and let me know if they're approximately what you're thinking before I start working on an implementation of DIR_NO_GITLINKS? I have the tests on github, but if you'd prefer I can open a Gerrit change.
Comment 35 Matthias Sohn CLA 2016-03-01 16:44:27 EST
(In reply to Preben Ingvaldsen from comment #34)
> I'm currently taking over implementation of the DIR_NO_GITLINKS
> functionality from Chris Price. I have an initial implementation of the
> tests he mentioned here:
> 
> https://github.com/fpringvaldsen/jgit/commit/
> 1afb804c124f530fabbc506ca9a6e3b9c5dfaf2c
> 
> Christian Halstrick, could you take a look at those tests and let me know if
> they're approximately what you're thinking before I start working on an
> implementation of DIR_NO_GITLINKS? I have the tests on github, but if you'd
> prefer I can open a Gerrit change.

please push these tests to Gerrit for code review
Comment 36 Preben Ingvaldsen CLA 2016-03-01 17:20:09 EST
Matthias, I have currently run into an issue attempting to push my tests up for code review. I've based all my changes off of this patch:

https://git.eclipse.org/r/#/c/51483

That patch isn't strictly necessary for these tests, but it is going to become necessary when I actually implement DIR_NO_GITLINKS. Unfortunately, I did not write the above patch, and it looks like I can't push another person's commit up to Gerrit unless I am already a project committer.

Do you know what the best path forward is? Since that patch is not strictly necessary for these tests, I don't need it for those tests to be reviewed, so I could just push up that commit separately. However, I don't intend for this patch to be merged (the tests will currently fail since DIR_NO_GITLINKS is not implemented), so I'll run into this same problem when I eventually push up the final patch.
Comment 37 Preben Ingvaldsen CLA 2016-03-01 17:27:14 EST
Matthias, I just pushed up the tests for review on Gerrit. If they look valid I will start on an implementation of DIR_NO_GITLINKS using FileModeStrategy.
Comment 38 Matthias Sohn CLA 2016-03-01 17:49:28 EST
(In reply to Preben Ingvaldsen from comment #36)
> Matthias, I have currently run into an issue attempting to push my tests up
> for code review. I've based all my changes off of this patch:
> 
> https://git.eclipse.org/r/#/c/51483
> 
> That patch isn't strictly necessary for these tests, but it is going to
> become necessary when I actually implement DIR_NO_GITLINKS. Unfortunately, I
> did not write the above patch, and it looks like I can't push another
> person's commit up to Gerrit unless I am already a project committer.

Did you modify https://git.eclipse.org/r/#/c/51483 or why do you need to push it ?

Also see bug 447739

> Do you know what the best path forward is? Since that patch is not strictly
> necessary for these tests, I don't need it for those tests to be reviewed,
> so I could just push up that commit separately. However, I don't intend for
> this patch to be merged (the tests will currently fail since DIR_NO_GITLINKS
> is not implemented), so I'll run into this same problem when I eventually
> push up the final patch.

so you want the following chain of commits (latest on top)?

https://git.eclipse.org/r/#/c/67633/
change implementing DIR_NO_GITLINKS
https://git.eclipse.org/r/#/c/51483

Why do you need to push https://git.eclipse.org/r/#/c/51483 then ?
Comment 39 Preben Ingvaldsen CLA 2016-03-01 17:53:55 EST
(In reply to Matthias Sohn from comment #38)
> Why do you need to push https://git.eclipse.org/r/#/c/51483 then ?

So, the conclusion of the discussion on https://git.eclipse.org/r/#/c/51483 was that we wanted to exercise that FileModeStrategy change to implement the DIR_NO_GITLINKS setting. It was decided that we wanted to first come up with a patch implementing DIR_NO_GITLINKS using those changes before https://git.eclipse.org/r/#/c/51483 would be accepted, so I'm going to need to base my changes on top of that commit to get them to work.
Comment 40 Eclipse Genie CLA 2016-03-07 14:21:16 EST
New Gerrit change created: https://git.eclipse.org/r/67916
Comment 41 Eclipse Genie CLA 2016-03-07 14:21:29 EST
New Gerrit change created: https://git.eclipse.org/r/67917
Comment 42 Preben Ingvaldsen CLA 2016-03-07 14:23:14 EST
I've gone ahead and pushed up a patch that adds in a full implementation of DIR_NO_GITLINKS.
Comment 43 Christian Halstrick CLA 2016-03-08 04:35:37 EST
(In reply to Preben Ingvaldsen from comment #42)
> I've gone ahead and pushed up a patch that adds in a full implementation of
> DIR_NO_GITLINKS.

Great that you pick up the work. But did not fully understand the relation to the work from Chris Price in 51483. From your comments I thought that your work is based on the work in 51483. But when I look in gerrit your change 67916 is not based on it. Your change has the same commit message, touches the same files... How are  67916 and 51483 related?
Comment 44 Chris Price CLA 2016-03-08 09:08:37 EST
(In reply to Christian Halstrick from comment #43)
> (In reply to Preben Ingvaldsen from comment #42)
> > I've gone ahead and pushed up a patch that adds in a full implementation of
> > DIR_NO_GITLINKS.
> 
> Great that you pick up the work. But did not fully understand the relation
> to the work from Chris Price in 51483. From your comments I thought that
> your work is based on the work in 51483. But when I look in gerrit your
> change 67916 is not based on it. Your change has the same commit message,
> touches the same files... How are  67916 and 51483 related?

Preben is my co-worker at Puppet Labs.  He had bandwidth to pick this back up before I did.

My understanding is that Gerritt would not let him push a branch that contained a commit that was not authored by him, so I told him that it was fine with me if he just modified that commit to set the author to be himself.
Comment 45 Christian Halstrick CLA 2016-03-08 09:15:52 EST
Thanks for clarifying. Chris, please abandon your change 51483 and state that is is continued in 67916.
Comment 46 Chris Price CLA 2016-03-08 09:30:51 EST
(In reply to Christian Halstrick from comment #45)
> Thanks for clarifying. Chris, please abandon your change 51483 and state
> that is is continued in 67916.

Done, thanks.
Comment 47 Eclipse Genie CLA 2016-03-11 06:54:43 EST
Gerrit change https://git.eclipse.org/r/67916 was merged to [master].
Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=cff546b0cbcdbaaa8757c94f25e845e81bd633be
Comment 48 Preben Ingvaldsen CLA 2016-03-28 19:41:44 EDT
Christian and Matthias,

Are you still interested in accepting my remaining patch about adding DIR_NO_GITLINKS support (the one at https://git.eclipse.org/r/#/c/67917/)?
Comment 49 Christian Halstrick CLA 2016-04-01 09:14:55 EDT
yes, I am. I am preparing some comments currently
Comment 50 Eclipse Genie CLA 2016-04-05 04:51:26 EDT
New Gerrit change created: https://git.eclipse.org/r/69882
Comment 51 Eclipse Genie CLA 2016-04-05 05:13:03 EDT
Gerrit change https://git.eclipse.org/r/69882 was merged to [stable-4.3].
Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=403f04d8dd744a6cc1c2a61540c34483b94c06b5
Comment 52 Matthias Sohn CLA 2016-04-05 05:43:34 EDT
can we close this issue ?