On Wed, Dec 7, 2016 at 1:24 PM, vi0oss <vi0oss@xxxxxxxxx> wrote: > On 12/07/2016 11:09 PM, Stefan Beller wrote: >>> >>> As submodule's alternate target does not end in .git/objects >>> (rather .git/modules/qqqqqq/objects), this alternate target >>> path restriction for in add_possible_reference_from_superproject >>> relates from "*.git/objects" to just */objects". >> >> I wonder if this check is too weak and we actually have to check for >> either .git/objects or modules/<name/possibly/having/slashes>/objects. >> When writing the referenced commit I assumed we'd need a stronger check >> to be safer and not add some random location as a possible alternate. >> > 1. Do we really need to check that it is named ".git"? Although > "git clone --mirror --recursive" is not (directly) supported > now, user may create one bare repository with [remnants of] > submodules by converting reqular repository into bare one. > Why not take advantage of additional information available locally > in this case? Oh, great point. > > 2. Is the check need to be strict because of we need to traverse > one directory level up? Normally this "/objects" part is added by > Git, so just one "../" seems to be OK. User can't specify "--reference > somerepo/.git/objects", a strange reference can appear only if user > manually creates alternates. Maybe better to document this case > instead of restricting the feature? Not sure I understand what needs better documentation here? > > 3. If nonetheless check for ".git/*/objects", then > a. What functions should be used in Git codebase for such checks? > b. Should we handle tricks like "smth/.git/../../objects" and so on? I see we're getting into problems here. > > 4. Should we print (or print in verbose mode) each used alternate, > to inform operator what his or her new clone will depend on? > > P.S. Actually I discovered the --recursive --reference feature when tried to > put reference to a mega-repo with all possible submodules added as remotes. > I expected --reference to just get though across all submodules, but it > complained > to missing "/modules/..." instead (the check went though becase the > repository > was named like "megarepo.git", so it did ended in ".git/objects"). Oh :( With that said, I think the original patch is a sensible approach. Thanks, Stefan