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

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Mar 29, 2019, 20:15 UTC
Message-ID: <20190329201541.GP32487@hank.intra.tgummerer.com>
In-Reply-To: <87sgv69itl.fsf@evledraar.gmail.com>

On 03/29, Ævar Arnfjörð Bjarmason wrote:

Show 26 quoted lines

> 
> On Thu, Mar 28 2019, Thomas Gummerer wrote:
> > I notice that we are currently not using 'linkat()' anywhere else in
> > our codebase.  It looks like it has been introduced in POSIX.1-2008,
> > which sounds fairly recent by git's standards.  So I wonder if this is
> > really supported on all platforms that git is being built on.
> >
> > I also wonder what would need to be done on Windows if we were to
> > introduce this.  I see we define the 'link()' function in
> > 'compat/mingw.c' for that currently, so I guess something similar
> > would be needed for 'linkat()'.  I added Dscho to Cc for Windows
> > expertise.
> 
> For better of worse this particular quest started because I pointed out
> (with some WIP patches) that for understanding this change we should
> test whatever we did now, to ensure that the refactoring didn't have
> unintended side-effects.
> 
> But that's a separate question from whether or not we want to keep the
> current behavior.
> 
> I think the current behavior is clearly insane, so I think we should
> change it with some follow-up patches. In particular options like
> --dissociate should clearly (in my mind at least) have behavior similar
> to "cp -L", and --local should hardlink to the *target* of the symlink,
> if anything, at least for objects/{??,pack,info}

Right, I definitely agree with all of that. Adding tests for the current behaviour is definitely a good thing if we can do it in a sane way. And I also agree that the current behaviour is insane, and should be fixed, but that may not want to be part of this patch series.

> I think that changes the portability story with linkat(), since it's not
> something we should be planning to keep, just an intermediate step so we
> don't have a gigantic patch that both adds tests, refactors and changes
> the behavior.

Fair enough, but that also means that this patch series necessarily has to introduce the changes in behaviour as well as switching clone to use dir-iterator. Of course we could say that the switch-over to using dir-iterator could be done as a separate patch series, but that seems a bit too much of a change in scope of this series.

Now I think Matheus has actually found a nice solution to this issue using 'strbuf_readlink()', which gives us the same behaviour as using 'linkat()' in this patch would give us, so this might not be that big an issue in the end.