Re: [GSoC][PATCH v5 2/7] clone: better handle symlinked files at .git/objects/

[Thomas Gummerer <t.gummerer@gmail.com>]

On 03/30, Matheus Tavares wrote:
> There is currently an odd behaviour when locally cloning a repository
> with symlinks at .git/objects: using --no-hardlinks all symlinks are
> dereferenced but without it, Git will try to hardlink the files with the
> link() function, which has an OS-specific behaviour on symlinks. On OSX
> and NetBSD, it creates a hardlink to the file pointed by the symlink
> whilst on GNU/Linux, it creates a hardlink to the symlink itself.
> 
> On Manjaro GNU/Linux:
>     $ touch a
>     $ ln -s a b
>     $ link b c
>     $ ls -li a b c
>     155 [...] a
>     156 [...] b -> a
>     156 [...] c -> a
> 
> But on NetBSD:
>     $ ls -li a b c
>     2609160 [...] a
>     2609164 [...] b -> a
>     2609160 [...] c
> 
> It's not good to have the result of a local clone to be OS-dependent and
> besides that, the current behaviour on GNU/Linux may result in broken
> symlinks. So let's standardize this by making the hardlinks always point
> to dereferenced paths, instead of the symlinks themselves. Also, add
> tests for symlinked files at .git/objects/.
> 
> Note: Git won't create symlinks at .git/objects itself, but it's better
> to handle this case and be friendly with users who manually create them.
> 
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Co-authored-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/clone.c            |  5 ++++-
>  t/t5604-clone-reference.sh | 27 ++++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 50bde99618..f975b509f1 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -443,7 +443,10 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest,
>  		if (unlink(dest->buf) && errno != ENOENT)
>  			die_errno(_("failed to unlink '%s'"), dest->buf);
>  		if (!option_no_hardlinks) {
> -			if (!link(src->buf, dest->buf))
> +			char *resolved_path = real_pathdup(src->buf, 1);
> +			int status = link(resolved_path, dest->buf);
> +			free(resolved_path);
> +			if (!status)

Is there any reason why we can't use 'real_path()' here?  As I
mentioned in [*1*], 'real_path()' doesn't require the callers to free
any memory, so the above could become much simpler, and could just be

+			if (!link(real_path(src->buf), dest->buf))

*1*: <20190330192738.GQ32487@hank.intra.tgummerer.com>

>  				continue;
>  			if (option_local > 0)
>  				die_errno(_("failed to create link '%s'"), dest->buf);