From 0040d2c129a86bd7e1ba30554eded084c513cea1 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 5 May 2020 13:57:20 -0400 Subject: [PATCH] sync: Avoid an ugly error message when nothing has been committed to master yet and there is a synced master branch to merge from Now the warning gets displayed, which is better than an arcane git error. The warning is still kind of ugly, especially when the pull later in the sync will clear up what it warns about. But, this is an unusual situation not likely to happen, and if there is no remote to pull from, the warning message is needed or the sync will seem to succeed despite not merging the synced master branch. Would still be better if it could merge the synced master branch in this situation, making an empty commit to master to do it seems wrong, and otherwise it would need a whole separate code path, and would bypass using git merge in favor of say, setting master to the syned branch. Which would bypass git configs like arguably merge.ff and certianly merge.verifySignatures. So don't want to do that. --- CHANGELOG | 2 + Command/Sync.hs | 37 +++++++++++-------- ...config_merge.ff__61__only_breaks_sync.mdwn | 3 ++ ..._3f78dd62e99f55388e415a2e2fb86a3f._comment | 35 ++++++++++++++++++ ..._1b7133c5915baabb3076e95f0ac9807a._comment | 15 ++++++++ 5 files changed, 76 insertions(+), 16 deletions(-) create mode 100644 doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_3_3f78dd62e99f55388e415a2e2fb86a3f._comment create mode 100644 doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_4_1b7133c5915baabb3076e95f0ac9807a._comment diff --git a/CHANGELOG b/CHANGELOG index 948b54d0ef..f7ac9f1a0d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -7,6 +7,8 @@ git-annex (8.20200502) UNRELEASED; urgency=medium is located in a directory that does not currently exist, to avoid silently skipping such a remote. * repair: Improve fetching from a remote with an url in host:path format. + * sync: Avoid an ugly error message when nothing has been committed to + master yet and there is a synced master branch to merge from. -- Joey Hess Mon, 04 May 2020 12:46:11 -0400 diff --git a/Command/Sync.hs b/Command/Sync.hs index ac19f6ab9f..837b7422a1 100644 --- a/Command/Sync.hs +++ b/Command/Sync.hs @@ -346,36 +346,41 @@ mergeLocal mergeconfig o currbranch = stopUnless (notOnlyAnnex o) $ mergeLocal' mergeconfig o currbranch mergeLocal' :: [Git.Merge.MergeConfig] -> SyncOptions -> CurrBranch -> CommandStart -mergeLocal' mergeconfig o currbranch@(Just _, _) = - needMerge currbranch >>= \case +mergeLocal' mergeconfig o currbranch@(Just branch, _) = + needMerge currbranch branch >>= \case Nothing -> stop Just syncbranch -> starting "merge" (ActionItemOther (Just $ Git.Ref.describe syncbranch)) $ next $ merge currbranch mergeconfig o Git.Branch.ManualCommit syncbranch -mergeLocal' _ _ (Nothing, madj) = do - b <- inRepo Git.Branch.currentUnsafe - needMerge (b, madj) >>= \case +mergeLocal' _ _ currbranch@(Nothing, _) = inRepo Git.Branch.currentUnsafe >>= \case + Just branch -> needMerge currbranch branch >>= \case Nothing -> stop Just syncbranch -> starting "merge" (ActionItemOther (Just $ Git.Ref.describe syncbranch)) $ do - warning $ "There are no commits yet in the currently checked out branch, so cannot merge any remote changes into it." + warning $ "There are no commits yet to branch " ++ Git.fromRef branch ++ ", so cannot merge " ++ Git.fromRef syncbranch ++ " into it." next $ return False + Nothing -> stop -- Returns the branch that should be merged, if any. -needMerge :: CurrBranch -> Annex (Maybe Git.Branch) -needMerge (Nothing, _) = return Nothing -needMerge (Just branch, madj) = ifM (allM id checks) +needMerge :: CurrBranch -> Git.Branch -> Annex (Maybe Git.Branch) +needMerge currbranch headbranch = ifM (allM id checks) ( return (Just syncbranch) , return Nothing ) where - checks = - [ not <$> isBareRepo - , inRepo (Git.Ref.exists syncbranch) - , inRepo (Git.Branch.changed branch' syncbranch) - ] - syncbranch = syncBranch branch - branch' = maybe branch (adjBranch . originalToAdjusted branch) madj + syncbranch = syncBranch headbranch + checks = case currbranch of + (Just _, madj) -> + let branch' = maybe headbranch (adjBranch . originalToAdjusted headbranch) madj + in + [ not <$> isBareRepo + , inRepo (Git.Ref.exists syncbranch) + , inRepo (Git.Branch.changed branch' syncbranch) + ] + (Nothing, _) -> + [ not <$> isBareRepo + , inRepo (Git.Ref.exists syncbranch) + ] pushLocal :: SyncOptions -> CurrBranch -> CommandStart pushLocal o b = stopUnless (notOnlyAnnex o) $ do diff --git a/doc/bugs/git_config_merge.ff__61__only_breaks_sync.mdwn b/doc/bugs/git_config_merge.ff__61__only_breaks_sync.mdwn index 0d0e488ef4..a38c8926c7 100644 --- a/doc/bugs/git_config_merge.ff__61__only_breaks_sync.mdwn +++ b/doc/bugs/git_config_merge.ff__61__only_breaks_sync.mdwn @@ -445,3 +445,6 @@ fatal: Not possible to fast-forward, aborting. ### Have you had any luck using git-annex before? (Sometimes we get tired of reading bug reports all day and a lil' positive end note does wonders) Sure! Using git-annex to keep artifacts in development repos works great usually :) + +> Closing as I don't think this behavior is actually a bug. [[done]] +> --[[Joey]] diff --git a/doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_3_3f78dd62e99f55388e415a2e2fb86a3f._comment b/doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_3_3f78dd62e99f55388e415a2e2fb86a3f._comment new file mode 100644 index 0000000000..39976671df --- /dev/null +++ b/doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_3_3f78dd62e99f55388e415a2e2fb86a3f._comment @@ -0,0 +1,35 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 3""" + date="2020-05-05T17:05:32Z" + content=""" +The sync man page does mention fetching and merging: + + The sync process involves first committing any local changes to files + that have previously been added to the repository, + then fetching and merging the current branch + +Not that I really buy the argument that not mentioning use of some specific git +command means that it should bypass git configurations that apply to that +command. git commands often don't document other git commands that they use +as plumbing, for example. + +The output with this configuration is essentially: + + pull repo-b + fatal: Not possible to fast-forward, aborting. + failed + +So the user is told right there that sync tried to pull. +And the pull failed. Why? Something to do with fast-forward. While this would +be clearer if git mentioned the config that is making it require a fast-forward, +it does not seem an indecipherable behavior. + +And also, you have to have *globally* configured an option that prevents fast forwarding. +That is a major, massive change to git's behavior. git is going to be falling over on +pull all the time in many circumstances with the same error message. + +Similarly, if you had configured merge.verifySignatures globally, git-annex +sync would be likely to fail. All your arguments would seem to require I also +override that option. But that could be a big security hole. +"""]] diff --git a/doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_4_1b7133c5915baabb3076e95f0ac9807a._comment b/doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_4_1b7133c5915baabb3076e95f0ac9807a._comment new file mode 100644 index 0000000000..a3ad330a46 --- /dev/null +++ b/doc/bugs/git_config_merge.ff__61__only_breaks_sync/comment_4_1b7133c5915baabb3076e95f0ac9807a._comment @@ -0,0 +1,15 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 4""" + date="2020-05-05T17:27:31Z" + content=""" +As to the "fatal: ambiguous argument" message, it happens in the same test +case w/o that global git config. + + [2020-05-05 13:28:27.204706608] read: git ["--git-dir=.git","--work-tree=.","--literal-pathspecs","log","refs/heads/master..refs/heads/synced/master","--pretty=%H","-n1"] + fatal: ambiguous argument 'refs/heads/master..refs/heads/synced/master': unknown revision or path not in the working tree. + +It's caused by sync being run with no master branch yet, and nothing to commit +to create one. refs/heads/synced/master does exist; it was synced from the +other repo. Fixed that message. +"""]]