From 98a3ba0ea5c1b55c7dfaac761b254fd167386d20 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 5 Apr 2023 16:59:44 -0400 Subject: [PATCH] restore old registerurl location tracking behavior registerurl: When an url is claimed by a special remote other than the web, update location tracking for that special remote. registerurl's behavior was changed in commit 451171b7c1eaccfd0f39d4ec1d64c6964613f55a, apparently accidentially to not update location tracking except for the web. This makes registerurl followed by unregisterurl not be a no-op, when the url happens to be claimed by a remote other than the web. It is a noop when the url is unclaimed except by the web. I don't like the inconsistency, and wish that registerurl and unregisterurl never updated location tracking, which would be more in keeping with them being plumbing. But there is the fact that it used to behave this way, and also it was inconsistent that it updated location tracking for the web but not for other remotes, unlike addurl. And there's an argument that the user might not know what remote to expect to claim an url, so would be considerably in the dark when using registerurl. (Although they have to know what content gets downloaded, since they specify a key..) Sponsored-By: the NIH-funded NICEMAN (ReproNim TR&D3) project --- CHANGELOG | 3 ++ Command/RegisterUrl.hs | 29 +++++++++++++------ Command/RmUrl.hs | 4 +++ Command/UnregisterUrl.hs | 8 +++-- ..._does_not_register_if_external_remote.mdwn | 2 ++ ..._542bd923122ba24fe9cb14d36b97f15d._comment | 14 +++++++++ doc/git-annex-registerurl.mdwn | 4 +-- doc/git-annex-rmurl.mdwn | 4 ++- doc/git-annex-unregisterurl.mdwn | 8 +++-- 9 files changed, 59 insertions(+), 17 deletions(-) create mode 100644 doc/bugs/registerurl_does_not_register_if_external_remote/comment_8_542bd923122ba24fe9cb14d36b97f15d._comment diff --git a/CHANGELOG b/CHANGELOG index f3a5daf159..bb75def430 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -7,6 +7,9 @@ git-annex (10.20230330) UNRELEASED; urgency=medium of --unlock-present and --hide-missing adjusted branches. * Support user.useConfigOnly git config. * registerurl, unregisterurl: Added --remote option. + * registerurl: When an url is claimed by a special remote other than the + web, update location tracking for that special remote. + (This was the behavior before version 6.20181011) -- Joey Hess Fri, 31 Mar 2023 12:48:54 -0400 diff --git a/Command/RegisterUrl.hs b/Command/RegisterUrl.hs index 1f851605e4..b1e13ea567 100644 --- a/Command/RegisterUrl.hs +++ b/Command/RegisterUrl.hs @@ -1,6 +1,6 @@ {- git-annex command - - - Copyright 2015-2022 Joey Hess + - Copyright 2015-2023 Joey Hess - - Licensed under the GNU AGPL version 3 or higher. -} @@ -9,6 +9,7 @@ module Command.RegisterUrl where import Command import Logs.Web +import Logs.Location import Command.FromKey (keyOpt, keyOpt') import qualified Remote import Annex.UUID @@ -33,12 +34,12 @@ optParser desc = RegisterUrlOptions seek :: RegisterUrlOptions -> CommandSeek seek o = case (batchOption o, keyUrlPairs o) of - (Batch fmt, _) -> seekBatch setUrlPresent o fmt + (Batch fmt, _) -> seekBatch registerUrl o fmt -- older way of enabling batch input, does not support BatchNull - (NoBatch, []) -> seekBatch setUrlPresent o (BatchFormat BatchLine (BatchKeys False)) - (NoBatch, ps) -> commandAction (start setUrlPresent o ps) + (NoBatch, []) -> seekBatch registerUrl o (BatchFormat BatchLine (BatchKeys False)) + (NoBatch, ps) -> commandAction (start registerUrl o ps) -seekBatch :: (Key -> URLString -> Annex ()) -> RegisterUrlOptions -> BatchFormat -> CommandSeek +seekBatch :: (Remote -> Key -> URLString -> Annex ()) -> RegisterUrlOptions -> BatchFormat -> CommandSeek seekBatch a o fmt = batchOnly Nothing (keyUrlPairs o) $ batchInput fmt (pure . parsebatch) $ batchCommandAction . start' a o @@ -51,20 +52,20 @@ seekBatch a o fmt = batchOnly Nothing (keyUrlPairs o) $ Left e -> Left e Right k -> Right (k, u) -start :: (Key -> URLString -> Annex ()) -> RegisterUrlOptions -> [String] -> CommandStart +start :: (Remote -> Key -> URLString -> Annex ()) -> RegisterUrlOptions -> [String] -> CommandStart start a o (keyname:url:[]) = start' a o (si, (keyOpt keyname, url)) where si = SeekInput [keyname, url] start _ _ _ = giveup "specify a key and an url" -start' :: (Key -> URLString -> Annex ()) -> RegisterUrlOptions -> (SeekInput, (Key, URLString)) -> CommandStart +start' :: (Remote -> Key -> URLString -> Annex ()) -> RegisterUrlOptions -> (SeekInput, (Key, URLString)) -> CommandStart start' a o (si, (key, url)) = starting "registerurl" ai si $ perform a o key url where ai = ActionItemOther (Just url) -perform :: (Key -> URLString -> Annex ()) -> RegisterUrlOptions -> Key -> URLString -> CommandPerform +perform :: (Remote -> Key -> URLString -> Annex ()) -> RegisterUrlOptions -> Key -> URLString -> CommandPerform perform a o key url = do needremote <- maybe (pure Nothing) (Just <$$> getParsed) (remoteOption o) r <- case needremote of @@ -75,5 +76,15 @@ perform a o key url = do showNote $ "The url " ++ url ++ " is claimed by remote " ++ Remote.name r next $ return False _ -> do - a key (setDownloader' url r) + a r key (setDownloader' url r) next $ return True + +registerUrl :: Remote -> Key -> String -> Annex () +registerUrl remote key url = do + setUrlPresent key url + -- setUrlPresent only updates location tracking when the url + -- does not have an OtherDownloader, but this command needs to do + -- it for urls claimed by other remotes as well. + case snd (getDownloader url) of + OtherDownloader -> logChange key (Remote.uuid remote) InfoPresent + _ -> return () diff --git a/Command/RmUrl.hs b/Command/RmUrl.hs index efd6e4059d..0aa7f36658 100644 --- a/Command/RmUrl.hs +++ b/Command/RmUrl.hs @@ -62,3 +62,7 @@ cleanup url key = do forM_ [minBound..maxBound] $ \dl -> setUrlMissing key (setDownloader url dl) return True + -- Unlike addurl, this does not update location tracking + -- for remotes other than the web special remote. Doing so with + -- a remote that git-annex can drop content from would rather + -- unexpectedly leave content stranded on that remote. diff --git a/Command/UnregisterUrl.hs b/Command/UnregisterUrl.hs index a26cdf56f3..e8bf16c933 100644 --- a/Command/UnregisterUrl.hs +++ b/Command/UnregisterUrl.hs @@ -24,11 +24,15 @@ seek o = case (batchOption o, keyUrlPairs o) of (Batch fmt, _) -> seekBatch unregisterUrl o fmt (NoBatch, ps) -> commandAction (start unregisterUrl o ps) -unregisterUrl :: Key -> String -> Annex () -unregisterUrl key url = do +unregisterUrl :: Remote -> Key -> String -> Annex () +unregisterUrl _remote key url = do -- Remove the url no matter what downloader; -- registerurl can set OtherDownloader, and this should also -- be able to remove urls added by addurl, which may use -- YoutubeDownloader. forM_ [minBound..maxBound] $ \dl -> setUrlMissing key (setDownloader url dl) + -- Unlike unregisterurl, this does not update location tracking + -- for remotes other than the web special remote. Doing so with + -- a remote that git-annex can drop content from would rather + -- unexpectedly leave content stranded on that remote. diff --git a/doc/bugs/registerurl_does_not_register_if_external_remote.mdwn b/doc/bugs/registerurl_does_not_register_if_external_remote.mdwn index 88c60d050c..529d21941a 100644 --- a/doc/bugs/registerurl_does_not_register_if_external_remote.mdwn +++ b/doc/bugs/registerurl_does_not_register_if_external_remote.mdwn @@ -102,3 +102,5 @@ so - both keys have urls, but only 123.dat one is associated with datalad specia [[!meta author=yoh]] [[!tag projects/repronim]] + +> [[fixed|done]] --[[Joey]] diff --git a/doc/bugs/registerurl_does_not_register_if_external_remote/comment_8_542bd923122ba24fe9cb14d36b97f15d._comment b/doc/bugs/registerurl_does_not_register_if_external_remote/comment_8_542bd923122ba24fe9cb14d36b97f15d._comment new file mode 100644 index 0000000000..b839dd2a70 --- /dev/null +++ b/doc/bugs/registerurl_does_not_register_if_external_remote/comment_8_542bd923122ba24fe9cb14d36b97f15d._comment @@ -0,0 +1,14 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 8""" + date="2023-04-05T21:00:04Z" + content=""" +Guess I'll come down on the side of restoring old behavior which was +changed w/o warning (and without the new behavior ever being documented). + +And on the side of user experience showing the current behavior is surprising. + +The future users who get surprised by the resulting inconsistency +of unregisterurl not unsetting location tracking will just have to +live with it.. Sigh. +"""]] diff --git a/doc/git-annex-registerurl.mdwn b/doc/git-annex-registerurl.mdwn index a74abd273b..a309ae7b0b 100644 --- a/doc/git-annex-registerurl.mdwn +++ b/doc/git-annex-registerurl.mdwn @@ -17,8 +17,8 @@ Normally the key is a git-annex formatted key. However, to make it easier to use this to add urls, if the key cannot be parsed as a key, and is a valid url, an URL key is constructed from the url. -Registering an url makes content be treated as being present in the web -special remote, unless some other special remote claims the url. +Registering an url also makes git-annex treat the key as present in the +special remote that claims it. (Usually the web special remote.) # OPTIONS diff --git a/doc/git-annex-rmurl.mdwn b/doc/git-annex-rmurl.mdwn index eb2508d68c..da0a6ba03c 100644 --- a/doc/git-annex-rmurl.mdwn +++ b/doc/git-annex-rmurl.mdwn @@ -11,7 +11,9 @@ git annex rmurl `[file url ..]` Record that the file is no longer available at the url. Removing the last web url will make git-annex no longer treat content as being -present in the web special remote. +present in the web special remote. If some other special remote +claims the url, unregistering the url will not update presence information +for it, because the content may still be present on the remote. # OPTIONS diff --git a/doc/git-annex-unregisterurl.mdwn b/doc/git-annex-unregisterurl.mdwn index bed12b5331..c44eb06c28 100644 --- a/doc/git-annex-unregisterurl.mdwn +++ b/doc/git-annex-unregisterurl.mdwn @@ -11,12 +11,14 @@ git annex unregisterurl `[key url]` This plumbing-level command can be used to unregister urls when keys can no longer be downloaded from them. -Unregistering a key's last web url will make git-annex no longer treat content -as being present in the web special remote. - Normally the key is a git-annex formatted key. However, if the key cannot be parsed as a key, and is a valid url, an URL key is constructed from the url. +Unregistering a key's last web url will make git-annex no longer treat content +as being present in the web special remote. If some other special remote +claims the url, unregistering the url will not update presence information +for it, because the content may still be present on the remote. + # OPTIONS * `--remote=name|uuid`