From 9012fa01877a78542c64f104bd60a80825ce3c86 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 1 Oct 2021 14:04:18 -0400 Subject: [PATCH] reinject: Fix crash when reinjecting a file from outside the repository Commit 4bf7940d6b912fbf692b268f621ebd41ed871125 introduced this problem, but was otherwise doing a good thing. Problem being that fileRef "/foo" used to return ":./foo", which was actually wrong, but as long as there was no foo in the local repository, catKey could operate on it without crashing. After that fix though, fileRef would return eg "../../foo", resulting in fileRef returning ":./../../foo", which will make git cat-file crash since that's not a valid path in the repo. Fix is simply to make fileRef detect paths outside the repo and return Nothing. Then catKey can be skipped. This needed several bugfixes to dirContains as well, in previous commits. In Command.Smudge, this led to needing to check for Nothing. That case should actually never happen, because the fileoutsiderepo check will detect it earlier. Sponsored-by: Brock Spratlen on Patreon --- Annex/CatFile.hs | 8 +++-- CHANGELOG | 2 ++ Command/Smudge.hs | 15 +++++----- Git.hs | 2 +- Git/Ref.hs | 30 ++++++++++++------- ...ge_to_fileRef_breaks_reinject_--known.mdwn | 2 ++ ..._5d3ade0367d3913bc115082ef2abd842._comment | 11 +++++++ 7 files changed, 48 insertions(+), 22 deletions(-) create mode 100644 doc/todo/recent_change_to_fileRef_breaks_reinject_--known/comment_1_5d3ade0367d3913bc115082ef2abd842._comment diff --git a/Annex/CatFile.hs b/Annex/CatFile.hs index 32a7c24f8c..7048fe77af 100644 --- a/Annex/CatFile.hs +++ b/Annex/CatFile.hs @@ -198,11 +198,12 @@ catSymLinkTarget sha = fromInternalGitPath . L.toStrict <$> get catKeyFile :: RawFilePath -> Annex (Maybe Key) catKeyFile f = ifM (Annex.getState Annex.daemon) ( catKeyFileHEAD f - , catKey =<< liftIO (Git.Ref.fileRef f) + , maybe (pure Nothing) catKey =<< inRepo (Git.Ref.fileRef f) ) catKeyFileHEAD :: RawFilePath -> Annex (Maybe Key) -catKeyFileHEAD f = catKey =<< liftIO (Git.Ref.fileFromRef Git.Ref.headRef f) +catKeyFileHEAD f = maybe (pure Nothing) catKey + =<< inRepo (Git.Ref.fileFromRef Git.Ref.headRef f) {- Look in the original branch from whence an adjusted branch is based - to find the file. But only when the adjustment hides some files. -} @@ -215,5 +216,6 @@ catObjectMetaDataHidden = hiddenCat catObjectMetaData hiddenCat :: (Ref -> Annex (Maybe a)) -> RawFilePath -> CurrBranch -> Annex (Maybe a) hiddenCat a f (Just origbranch, Just adj) | adjustmentHidesFiles adj = - a =<< liftIO (Git.Ref.fileFromRef origbranch f) + maybe (pure Nothing) a + =<< inRepo (Git.Ref.fileFromRef origbranch f) hiddenCat _ _ _ = return Nothing diff --git a/CHANGELOG b/CHANGELOG index 7358eefbac..64d99ffdaf 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -13,6 +13,8 @@ git-annex (8.20210904) UNRELEASED; urgency=medium was interrupted. * sync --content: Avoid a redundant checksum of a file that was incrementally verified, when used on NTFS and perhaps other filesystems. + * reinject: Fix crash when reinjecting a file from outside the repository. + (Reversion in version 8.20210621) -- Joey Hess Fri, 03 Sep 2021 12:02:55 -0400 diff --git a/Command/Smudge.hs b/Command/Smudge.hs index bfb5916c01..64b66670ad 100644 --- a/Command/Smudge.hs +++ b/Command/Smudge.hs @@ -105,13 +105,14 @@ clean file = do addingExistingLink file k $ do getMoveRaceRecovery k file liftIO $ L.hPut stdout b - Nothing -> do - fileref <- liftIO $ Git.Ref.fileRef file - indexmeta <- catObjectMetaData fileref - oldkey <- case indexmeta of - Just (_, sz, _) -> catKey' fileref sz - Nothing -> return Nothing - go' b indexmeta oldkey + Nothing -> inRepo (Git.Ref.fileRef file) >>= \case + Just fileref -> do + indexmeta <- catObjectMetaData fileref + oldkey <- case indexmeta of + Just (_, sz, _) -> catKey' fileref sz + Nothing -> return Nothing + go' b indexmeta oldkey + Nothing -> liftIO $ L.hPut stdout b go' b indexmeta oldkey = ifM (shouldAnnex file indexmeta oldkey) ( do -- Before git 2.5, failing to consume all stdin here diff --git a/Git.hs b/Git.hs index f8eedc01ea..56cfffbe2d 100644 --- a/Git.hs +++ b/Git.hs @@ -71,7 +71,7 @@ repoLocation Repo { location = LocalUnknown dir } = fromRawFilePath dir repoLocation Repo { location = Unknown } = error "unknown repoLocation" {- Path to a repository. For non-bare, this is the worktree, for bare, - - it's the gitdit, and for URL repositories, is the path on the remote + - it's the gitdir, and for URL repositories, is the path on the remote - host. -} repoPath :: Repo -> RawFilePath repoPath Repo { location = Url u } = toRawFilePath $ unEscapeString $ uriPath u diff --git a/Git/Ref.hs b/Git/Ref.hs index a26e32fb49..b4b81d01e6 100644 --- a/Git/Ref.hs +++ b/Git/Ref.hs @@ -64,17 +64,22 @@ branchRef = underBase "refs/heads" {- A Ref that can be used to refer to a file in the repository, as staged - in the index. + - + - If the input file is located outside the repository, returns Nothing. -} -fileRef :: RawFilePath -> IO Ref -fileRef f = do +fileRef :: RawFilePath -> Repo -> IO (Maybe Ref) +fileRef f repo = do -- The filename could be absolute, or contain eg "../repo/file", -- neither of which work in a ref, so convert it to a minimal -- relative path. f' <- relPathCwdToFile f - -- Prefixing the file with ./ makes this work even when in a - -- subdirectory of a repo. Eg, ./foo in directory bar refers - -- to bar/foo, not to foo in the top of the repository. - return $ Ref $ ":./" <> toInternalGitPath f' + print ("f'", f', repoPath repo, repoPath repo `dirContains` f') + return $ if repoPath repo `dirContains` f' + -- Prefixing the file with ./ makes this work even when in a + -- subdirectory of a repo. Eg, ./foo in directory bar refers + -- to bar/foo, not to foo in the top of the repository. + then Just $ Ref $ ":./" <> toInternalGitPath f' + else Nothing {- A Ref that can be used to refer to a file in a particular branch. -} branchFileRef :: Branch -> RawFilePath -> Ref @@ -85,11 +90,14 @@ dateRef :: Ref -> RefDate -> Ref dateRef r (RefDate d) = Ref $ fromRef' r <> "@" <> encodeBS d {- A Ref that can be used to refer to a file in the repository as it - - appears in a given Ref. -} -fileFromRef :: Ref -> RawFilePath -> IO Ref -fileFromRef r f = do - (Ref fr) <- fileRef f - return (Ref (fromRef' r <> fr)) + - appears in a given Ref. + - + - If the file path is located outside the repository, returns Nothing. + -} +fileFromRef :: Ref -> RawFilePath -> Repo -> IO (Maybe Ref) +fileFromRef r f repo = fileRef f repo >>= return . \case + Just (Ref fr) -> Just (Ref (fromRef' r <> fr)) + Nothing -> Nothing {- Checks if a ref exists. Note that it must be fully qualified, - eg refs/heads/master rather than master. -} diff --git a/doc/todo/recent_change_to_fileRef_breaks_reinject_--known.mdwn b/doc/todo/recent_change_to_fileRef_breaks_reinject_--known.mdwn index 5c1ff3a873..83b9695c51 100644 --- a/doc/todo/recent_change_to_fileRef_breaks_reinject_--known.mdwn +++ b/doc/todo/recent_change_to_fileRef_breaks_reinject_--known.mdwn @@ -24,3 +24,5 @@ By means of bisection I have determined that commit 4bf7940d6b912fbf692b268f621e git-annex: fd:15: Data.ByteString.hGetLine: end of file --spwhitton + +> [[fixed|done]] --[[Joey]] diff --git a/doc/todo/recent_change_to_fileRef_breaks_reinject_--known/comment_1_5d3ade0367d3913bc115082ef2abd842._comment b/doc/todo/recent_change_to_fileRef_breaks_reinject_--known/comment_1_5d3ade0367d3913bc115082ef2abd842._comment new file mode 100644 index 0000000000..107d3f205f --- /dev/null +++ b/doc/todo/recent_change_to_fileRef_breaks_reinject_--known/comment_1_5d3ade0367d3913bc115082ef2abd842._comment @@ -0,0 +1,11 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 1""" + date="2021-10-01T16:42:36Z" + content=""" +Also happens with a relative path to the file. And also +`git annex reinject ../bar bar` fails the same way. + +Fixed. In case you want to cherry-pick the fix, it's the commit adding +this comment, as well as the 2 prior commits fixing bugs in dirContains. +"""]] -- 2.30.2