The readlink() function should be independent of core.symlinks

[rburgstaler]

This patch was motivated by a bug that was discovered: when calling git pack-refs --all in a working directory created via git-new-workdir, it will blow away the existing .git\packed-refs symlink that points to the source directory and replace packed-refs with a regular file with duplicate refs in it. This patch fixes this.

Fix: readlink() not returning target symlink when it is desired

Core api command readlink() was depending on a git configuration specific setting of has_symlinks to decide whether it should attempt to return the target of a symlink. It should be up to the caller to check whether symlink are enabled not up to the api command readlink(). If a caller wants to get a symlink target, then it should get it regardless of whether symlinks are configured in git.

This item is related to:
#220
and
#186
and
#225

[dscho]

Please format the commit message according to https://github.com/git-for-windows/git/blob/master/Documentation/SubmittingPatches (including the Sign-off).

However, I am not sure that this patch is a good idea. If we do not write symlinks, we should not read them, either. Otherwise a caller might be tempted to try to recreate a symlink, read it, and then fail to write it.

Also, git new-workdir is not officially supported in Git for Windows. Have you had a look at git worktree? That one is supported.

[rburgstaler]

I have updated the commit message. Hopefully it is now closer to the desired format.

So the way that I understand it the purpose behind mingw.c is to fill the missing gaps between what is supported in vanilla mingw and what the git code-base needs. mingw.c has a method symlink() which I am assuming is meant to implement http://pubs.opengroup.org/onlinepubs/9699919799/functions/symlink.html just like readlink() located in mingw.c is meant to implement http://pubs.opengroup.org/onlinepubs/9699919799/functions/readlink.html. The problem is that readlink() and symlink() both (before my patch) currently require a check on has_symlinks which is a git specific setting set via core.symlinks. My thoughts are that readlink() and symlink() should not care about a git specific configuration setting (has_symlinks and/or core.symlinks). They should do their job as instructed and return the result if possible (of course we all know about he admin rights requirements and the OS limitation of XP and greater to create/use symlinks).

So I guess I am suggesting also to revise this patch to also remove the has_symlinks check from the symlinks() method as well. I guess the million $$$$ question is, does removing has_symlinks form symlinks() and readlink() break some other expected behavior in git-for-windows. If so I would look into finding an approach to resolving that. I do know that having has_symlinks breaks my git-new-workdir use case (which I know you mentioned git-new-workdir is not officially supported so that argument might not have much merit)

I have looked at git worktree and would love to move forward with it, however, the large hold-back at this time is the incomplete support for submodules that we need.
https://git-scm.com/docs/git-worktree

Multiple checkout in general is still experimental, and the support for submodules is incomplete. It is NOT recommended to make multiple checkouts of a superproject.

Truthfully, I have not looked deep enough into what the shortcomings are with submodules in worktree yet so it might actually work for us/me. My intentions here were mainly to help identify a bug and possibly help to resolve it. After all this functionality worked before some commits from May of 2015 so I see it as a fix to a regression.

[dscho]

mingw.c has a method symlink() which I am assuming is meant to implement http://pubs.opengroup.org/onlinepubs/9699919799/functions/symlink.html just like readlink() located in mingw.c is meant to implement http://pubs.opengroup.org/onlinepubs/9699919799/functions/readlink.html.

The problem is: it cannot implement this. There is no precise equivalent in Windows for POSIX-style symbolic links. See https://github.com/git-for-windows/git/wiki/Symbolic-Links

That is the reason why we require users to turn on the core.symlinks flag, to indicate that they understand what thin ice they tread on, and that all problems they have with symlinks are their own fault. 😄

I have looked at git worktree and would love to move forward with it, however, the large hold-back at this time is the incomplete support for submodules that we need.

I am sure that Git would benefit if you started working on this.

[rburgstaler]

Just wanted to summarize quickly,
To generally categorize how symlink(), readlink(), and other symlink related routines located in mingw.c are being used in git would be the following:
1.) To operate on core git config files, ref files, etc. that may or may not be symlinks
Example: Before get goes to update the .git/packedrefs file, git will take into account whether .git/packedrefs is a symlink or while writing the file using the whole file-lock-api (which is kind of kewl by the way)
2.) To add support for symlink files and directories that can be committed as part of the repository.
Example: Gives the ability to have a repository and ability commit a symbolic link so that other files contained in the repository can use the symbolic link to refer to other files else where on the system even though they were not committed.

Looking at
https://www.kernel.org/pub/software/scm/git/docs/git-config.html

core.symlinks
If false, symbolic links are checked out as small plain files that contain the link text. git-update-index(1) and git-add(1) will not change the recorded type to regular file. Useful on filesystems like FAT that do not support symbolic links.

The default is true, except git-clone(1) or git-init(1) will probe and set core.symlinks false if appropriate when the repository is created.

From this it appears as though core.symlinks refers to case (2.) from above and does NOT really refer to case (1.) from above

In the case of git-for-windows, it has been determined by the git-for-windows community that setting core.symlinks to false will make case (1.) and (2.) not realize symlinks. Essentially no support for symlinks what so ever (even if needed by core git for non-repo operations).

With

That is the reason why we require users to turn on the core.symlinks flag, to indicate that they understand what thin ice they tread on, and that all problems they have with symlinks are their own fault.

You have convinced me that if I want git-new-workdir to work for now, I will tread on thin ice and set core.symlinks to true. So I am going to close this pull request.

As for git-new-workdir, I will start moving to the future with worktree, but I did/do have an immediate need with 50 repos that each member of a team of developers uses that need git-new-workdir to work properly and not blow away packed-refs. core.symlinks set to true will be the work around until I can get completely moved to worktree on the 50 repos and update these for everybody on the team.

Thanks for your time,

[dscho]

Thanks for understanding.

If you face any problems with worktree, I would like to encourage you to bring these problems up on the upstream Git mailing list. I, for one, will keep an eye out and try to assist with resolving those issues.