On Mon, Feb 25 2019, Matheus Tavares Bernardino wrote: > Hi, Christian and Ævar > > First of all, thanks for the fast and attentive reviews. > > I am a little confused about what I should do next. How should I > proceed with this series? > > By what was said, I understood that the series: > 1) Is indeed an improvement under --local, because it won't deference > symlinks in this case. > 2) Don't make --dissociate any better but as it is already buggy that > would be some work for another patchset. > 3) Makes git-clone copy hidden paths which is a good behaviour. > 4) Breaks --no-hardlinks when there are symlinks at the repo's objects > directory. > > I understood that even though git itself does not create symlinks in > .git/objects, we should take care of the case where the user manually > creates them, right? But what would be the appropriate behaviour: to > follow (i.e. deference) symlinks (the way it is done now) or just copy > the link file itself (the way my series currently do)? And shouldn't > we document this decision somewhere? > > About the failure with --no-hardlinks having symlinks at .dir/objects, > it's probably because copy_or_link_directory() is trying to copy a > file which is a symlink to a dir and the copy function used is trying > to copy the dir not the link itself. A possible fix is to change > copy.c to copy the link file, but I haven't studied yet how that could > be accomplished. > > Another possible fix is to make copy_or_link_directory() deference > symlink structures when --no-hardlinks is given. But because the > function falls back to no-hardlinks when failing to hardlink, I don't > think it would be easy to accomplish this without making the function > *always* deference symlinks. And that would make the series lose the > item 1), which I understand you liked. I don't really have formed opinions one way or the other about what these specific flags should do in combination with such a repository, e.g. should --dissociate copy data rather than point to the same symlinks? I'm inclined to think so, but I've only thought about it for a couple of minutes. Maybe if someone starts digging they'll rightly come to a different conclusion. Rather, my comment is on the process. Clone behavior is too important to leave to prose in a commit message. I already found a case where we hard error not explicitly called out, are there other edge cases I didn't think of? So having this e.g. be a 4-part series where 3/4 is introducing tests in the direction I posted upthread (but needs more work), with 4/4 going through/justifying each one. > On Sun, Feb 24, 2019 at 6:41 AM Christian Couder > <christian.couder@xxxxxxxxx> wrote: >> >> On Sat, Feb 23, 2019 at 11:48 PM Ævar Arnfjörð Bjarmason >> <avarab@xxxxxxxxx> wrote: >> > >> > >> > On Sat, Feb 23 2019, Matheus Tavares wrote: >> > >> > > Replace usage of opendir/readdir/closedir API to traverse directories >> > > recursively, at copy_or_link_directory function, by the dir-iterator >> > > API. This simplifies the code and avoid recursive calls to >> > > copy_or_link_directory. >> > >> > Sounds good in principle. >> > >> > > This process also brings some safe behaviour changes to >> > > copy_or_link_directory: >> > >> > I ad-hoc tested some of these, and could spot behavior changes. We >> > should have tests for these. >> >> I agree that ideally we should have a few tests for these, but this is >> a grey area (see below) and there are areas that are not grey for >> which we don't have any test... >> >> And then adding tests would make this series become larger than a >> typical GSoC micro-project... >> >> > > - It will no longer follows symbolic links. This is not a problem, >> > > since the function is only used to copy .git/objects directory, and >> > > symbolic links are not expected there. >> > >> > I don't think we should make that assumption, and I don't know of >> > anything else in git that does. >> >> I think Git itself doesn't create symlinks in ".git/objects/" and we >> don't recommend people manually tweaking what's inside ".git/". That's >> why I think it's a grey area. >> >> > I've certainly symlinked individual objects or packs into a repo for >> > debugging / recovery, and it would be unexpected to clone that and miss >> > something. >> >> If people tweak what's inside ".git" by hand, they are expected to >> know what they doing and be able to debug it. >> >> > So in the general case we should be strict in what we generate, but >> > permissive in what we accept. We don't want a "clone" of an existing >> > repo to fail, or "fsck" to fail after clone... >> >> Yeah, but realistically I don't think we are going to foolproof Git >> from everything that someone could do by tweaking random things >> manually in ".git/". >> >> I am not saying that it should be ok to make things much worse than >> they are now in case some things have been tweaked in ".git/", but if >> things in general don't look worse in this grey area, and a patch >> otherwise improves things, then I think the patch should be ok. >> >> > When trying to test this I made e.g. objects/c4 a symlink to /tmp/c4, >> > and a specific object in objects/4d/ a symlink to /tmp too. >> > >> > Without this patch the individual object is still a symlink, but the >> > object under the directory gets resolved, and "un-symlinked", also with >> > --dissociate, which seems like an existing bug. >> > >> > With your patch that symlink structure is copied as-is. That's more >> > faithful under --local, but a regression for --dissociate (which didn't >> > work fully to begin with...). >> >> I think that I use --local (which is the default if the repository is >> specified as a local path) much more often than --dissociate, so for >> me the patch would be very positive, especially since --dissociate is >> already buggy anyway in this case. >> >> > I was paranoid that "no longer follows symbolic links" could also mean >> > "will ignore those objects", but it seems to more faithfully copy things >> > as-is for *that* case. >> >> Nice! >> >> > But then I try with --no-hardlinks and stock git dereferences my symlink >> > structure, but with your patches fails completely: >> > >> > Cloning into bare repository 'repo2'... >> > error: copy-fd: read returned: Is a directory >> > fatal: failed to copy file to 'repo2/objects/c4': Is a directory >> > fatal: the remote end hung up unexpectedly >> > fatal: cannot change to 'repo2': No such file or directory >> >> Maybe this could be fixed. Anyway I don't use --no-hardlinks very >> often, so I still think the patch is a positive even with this >> failure. >> >> > So there's at least one case in a few minutes of prodding this where we >> > can't clone a working repo now, however obscure the setup. >> > >> > > - Hidden directories won't be skipped anymore. In fact, it is odd that >> > > the function currently skip hidden directories but not hidden files. >> > > The reason for that could be unintentional: probably the intention >> > > was to skip '.' and '..' only, but it ended up accidentally skipping >> > > all directories starting with '.'. Again, it must not be a problem >> > > not to skip hidden dirs since hidden dirs/files are not expected at >> > > .git/objects. >> > >> > I reproduce this with --local. A ".foo" isn't copied before, now it >> > is. Good, I guess. We'd have already copied a "foo". >> > >> > > - Now, copy_or_link_directory will call die() in case of an error on >> > > openddir, readdir or lstat, inside dir_iterator_advance. That means >> > > it will abort in case of an error trying to fetch any iteration >> > > entry. >> >> It would be nice if the above paragraph in the commit message would >> say what was the previous behavior and why it's better to die() . >> >> > Good, but really IMNSHO this series is tweaking some critical core code >> > and desperately needs tests. >> >> It's critical that this code works well in the usual case, yes. (And >> there are already a lot of tests that test that.) But when people have >> manually tweaked things in their ".git/objects/", it's not critical >> what happens. Many systems have "undefined behaviors" at some point >> and that's ok. >> >> And no, I am not saying that we should consider it completely >> "undefined behavior" as soon as something is manually tweaked in >> ".git/", and yes, tests would be nice, and your comments and manual >> tests on this are very much appreciated. It's just that I don't think >> we should require too much when a patch, especially from a first time >> contributor, is already improving things, though it also changes a few >> things in a grey area. >> >> > Unfortunately, in this as in so many edge case we have no existing >> > tests. >> > >> > This would be much easier to review and would give reviewers more >> > confidence if the parts of this that changed behavior started with a >> > patch or patches that just manually objects/ dirs with various >> >> I think "created" is missing between "manually" and "objects/" in the >> above sentence. >> >> > combinations of symlinks, hardlinks etc., and asserted that the various >> > options did exactly what they're doing now, and made sure the >> > source/target repos were the same after/both passed "fsck". >> > >> > Then followed by some version of this patch which changes the behavior, >> > and would be forced to tweak those tests. To make it clear e.g. that >> > some cases where we have a working "clone" are now a hard error. >> >> Unfortunately this would be a lot of work and not appropriate for a >> GSoC micro-project. >> >> Thanks, >> Christian.
