From c62fe5e9a80058de0bba7bf9bed1875fb91e4f81 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 9 Sep 2022 14:43:43 -0400 Subject: [PATCH] avoid redundant prompt for http password in git-annex get that does autoinit autoEnableSpecialRemotes runs a subprocess, and if the uuid for a git remote has not been probed yet, that will do a http get that will prompt for a password. And then the parent process will subsequently prompt for a password when getting annexed files from the remote. So the solution is for autoEnableSpecialRemotes to run remoteList before the subprocess, which will probe for the uuid for the git remote in the same process that will later be used to get annexed files. But, Remote.Git imports Annex.Init, and Remote.List imports Remote.Git, so Annex.Init cannot import Remote.List. Had to pass remoteList into functions in Annex.Init to get around this dependency loop. --- Annex/Init.hs | 24 ++++++++++++------- Command.hs | 3 ++- Command/Assistant.hs | 3 ++- Remote/Git.hs | 6 ++--- ...redentials_for_password_per_each_file.mdwn | 2 ++ ..._4dca41df7506fa7aedf88d7618bbcf39._comment | 19 +++++++++++++++ 6 files changed, 44 insertions(+), 13 deletions(-) create mode 100644 doc/todo/not_ask_git_credentials_for_password_per_each_file/comment_4_4dca41df7506fa7aedf88d7618bbcf39._comment diff --git a/Annex/Init.hs b/Annex/Init.hs index c2dec9237b..94f3ed5079 100644 --- a/Annex/Init.hs +++ b/Annex/Init.hs @@ -201,8 +201,8 @@ getInitializedVersion = do - - Checks repository version and handles upgrades too. -} -ensureInitialized :: Annex () -ensureInitialized = getInitializedVersion >>= maybe needsinit checkUpgrade +ensureInitialized :: Annex [Remote] -> Annex () +ensureInitialized remotelist = getInitializedVersion >>= maybe needsinit checkUpgrade where needsinit = ifM autoInitializeAllowed ( do @@ -210,7 +210,7 @@ ensureInitialized = getInitializedVersion >>= maybe needsinit checkUpgrade Right () -> noop Left e -> giveup $ show e ++ "\n" ++ "git-annex: automatic initialization failed due to above problems" - autoEnableSpecialRemotes + autoEnableSpecialRemotes remotelist , giveup "First run: git-annex init" ) @@ -254,13 +254,13 @@ guardSafeToUseRepo a = ifM (inRepo Git.Config.checkRepoConfigInaccessible) - - Checks repository version and handles upgrades too. -} -autoInitialize :: Annex () -autoInitialize = getInitializedVersion >>= maybe needsinit checkUpgrade +autoInitialize :: Annex [Remote] -> Annex () +autoInitialize remotelist = getInitializedVersion >>= maybe needsinit checkUpgrade where needsinit = whenM (initializeAllowed <&&> autoInitializeAllowed) $ do initialize True Nothing Nothing - autoEnableSpecialRemotes + autoEnableSpecialRemotes remotelist {- Checks if a repository is initialized. Does not check version for ugrade. -} isInitialized :: Annex Bool @@ -440,9 +440,17 @@ fixupUnusualReposAfterInit = do {- Try to enable any special remotes that are configured to do so. - - The enabling is done in a child process to avoid it using stdio. + - + - The remotelist should be Remote.List.remoteList, which cannot + - be imported here due to a dependency loop. -} -autoEnableSpecialRemotes :: Annex () -autoEnableSpecialRemotes = do +autoEnableSpecialRemotes :: Annex [Remote] -> Annex () +autoEnableSpecialRemotes remotelist = do + -- Get all existing git remotes to probe for their uuid here, + -- so it is not done inside the child process. Doing it in there + -- could result in password prompts for http credentials, + -- which would then not end up cached in this process's state. + _ <- remotelist rp <- fromRawFilePath <$> fromRepo Git.repoPath withNullHandle $ \nullh -> gitAnnexChildProcess "init" [ Param "--autoenable" ] diff --git a/Command.hs b/Command.hs index 21607e4962..09e7cb55be 100644 --- a/Command.hs +++ b/Command.hs @@ -28,6 +28,7 @@ import Utility.Daemon import Types.Transfer import Types.ActionItem import Types.WorkerPool as ReExported +import Remote.List {- Generates a normal Command -} command :: String -> CommandSection -> String -> CmdParamsDesc -> (CmdParamsDesc -> CommandParser) -> Command @@ -125,7 +126,7 @@ commonChecks :: [CommandCheck] commonChecks = [repoExists] repoExists :: CommandCheck -repoExists = CommandCheck 0 ensureInitialized +repoExists = CommandCheck 0 (ensureInitialized remoteList) notBareRepo :: Command -> Command notBareRepo = addCheck checkNotBareRepo diff --git a/Command/Assistant.hs b/Command/Assistant.hs index f4dd9083cd..2a9bfc9887 100644 --- a/Command/Assistant.hs +++ b/Command/Assistant.hs @@ -16,6 +16,7 @@ import Config.Files.AutoStart import qualified BuildInfo import Utility.HumanTime import Assistant.Install +import Remote.List import Control.Concurrent.Async @@ -62,7 +63,7 @@ start o stop | otherwise = do liftIO ensureInstalled - ensureInitialized + ensureInitialized remoteList Command.Watch.start True (daemonOptions o) (startDelayOption o) startNoRepo :: AssistantOptions -> IO () diff --git a/Remote/Git.hs b/Remote/Git.hs index 48f2422cd8..41ea016cf2 100644 --- a/Remote/Git.hs +++ b/Remote/Git.hs @@ -350,7 +350,7 @@ tryGitConfigRead autoinit r hasuuid readlocalannexconfig = do let check = do Annex.BranchState.disableUpdate - catchNonAsync autoInitialize $ \e -> + catchNonAsync (autoInitialize (pure [])) $ \e -> warning $ "remote " ++ Git.repoDescribe r ++ ":" ++ show e Annex.getState Annex.repo @@ -605,7 +605,7 @@ repairRemote r a = return $ do s <- Annex.new r Annex.eval s $ do Annex.BranchState.disableUpdate - ensureInitialized + ensureInitialized (pure []) a `finally` stopCoProcesses data LocalRemoteAnnex = LocalRemoteAnnex Git.Repo (MVar [(Annex.AnnexState, Annex.AnnexRead)]) @@ -647,7 +647,7 @@ onLocal' (LocalRemoteAnnex repo mv) a = liftIO (takeMVar mv) >>= \case [] -> do liftIO $ putMVar mv [] v <- newLocal repo - go (v, ensureInitialized >> a) + go (v, ensureInitialized (pure []) >> a) (v:rest) -> do liftIO $ putMVar mv rest go (v, a) diff --git a/doc/todo/not_ask_git_credentials_for_password_per_each_file.mdwn b/doc/todo/not_ask_git_credentials_for_password_per_each_file.mdwn index 27209b5098..1848e92093 100644 --- a/doc/todo/not_ask_git_credentials_for_password_per_each_file.mdwn +++ b/doc/todo/not_ask_git_credentials_for_password_per_each_file.mdwn @@ -31,3 +31,5 @@ and wondered if such excessive prompting could be avoided without engaging `git` [[!meta author=yoh]] [[!tag projects/repronim]] + +> [[fixed|done]] --[[Joey]] diff --git a/doc/todo/not_ask_git_credentials_for_password_per_each_file/comment_4_4dca41df7506fa7aedf88d7618bbcf39._comment b/doc/todo/not_ask_git_credentials_for_password_per_each_file/comment_4_4dca41df7506fa7aedf88d7618bbcf39._comment new file mode 100644 index 0000000000..fc72aba8ff --- /dev/null +++ b/doc/todo/not_ask_git_credentials_for_password_per_each_file/comment_4_4dca41df7506fa7aedf88d7618bbcf39._comment @@ -0,0 +1,19 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 4""" + date="2022-09-09T18:39:06Z" + content=""" +Fixed the autoinit case. + +I'm going to close this as done, despite the cases mentioned above where +subprocesses might redundantly prompt for the credentials. + +Another reason +to not worry about those is that `git-annex sync` runs `git fetch` and `git +push` (more than once), so there will be several password prompts there. +So when git-annex runs a git-annex subprocess, it follows it's just as ok +for it to do its own password prompts as it is for a git subprocess to do +so. The solution to either is certianly to enable git's credential cache. +So the scope of this todo has to be limited to prompting done by a single +git-annex process. +"""]]