Return empty default when git fails

[BasLaa]

As mentioned in #8478, cabal init throws an error when git is not installed as it fails to guess a default name and email for the user. Instead, it should give an empty default (or no default perhaps?) and perhaps a note.

[BasLaa]

I was thinking we could do a check in authorPrompt and emailPrompt in Client.init.Interactive.Command to see if the returned guess is empty. If it's empty, we can pass the MandatoryPrompt flag instead of DefaultPrompt ...?

[BasLaa]

In the function authorHeuristics, the guessAuthorEmail is used whereas in emailHeuristics, the guessAuthorName function is used. I left it in this way for now, but this seems like a mistake and they should be the other way around?

[BasLaa]

I'm having trouble understanding what is going wrong exactly in the unit test. It seems to me that running getAuthor flags $ return "" should prompt the user, which I think would be the desired behaviour when there is no flag and we cannot guess using git.

[ffaf1]

I suspect the tests mock system calls, like

cabal/cabal-install/tests/UnitTests/Distribution/Client/Init/NonInteractive.hs

Lines 151 to 163 in fd09059

	inputs = NEL.fromList
	-- extra sources
	[ "[\"CHANGELOG.md\"]"
	-- lib other modules
	, "False"
	-- exe other modules
	, "False"
	-- test main file
	, "True"
	, "[\"quxTest/Main.hs\"]"
	-- test other modules
	, "False"
	]

should be changed. a63fe9b (#7344) shows a previous adjustment, but I am not sure how to read those. There is this Interactive instance at play:

cabal/cabal-install/src/Distribution/Client/Init/Types.hs

Lines 366 to 399 in fd09059

	instance Interactive PurePrompt where
	getLine = pop
	readFile !_ = pop
	getCurrentDirectory = popAbsolute
	getHomeDirectory = popAbsolute
	-- expects stack input of form "[\"foo\", \"bar\", \"baz\"]"
	getDirectoryContents !_ = popList
	listDirectory !_ = popList
	doesDirectoryExist !_ = popBool
	doesFileExist !_ = popBool
	canonicalizePathNoThrow !_ = popAbsolute
	readProcessWithExitCode !_ !_ !_ = do
	input <- pop
	return (ExitSuccess, input, "")
	getEnvironment = fmap (map read) popList
	getCurrentYear = fmap read pop
	listFilesInside pred' !_ = do
	input <- map splitDirectories <$> popList
	map joinPath <$> filterM (fmap and . traverse pred') input
	listFilesRecursive !_ = popList
	putStr !_ = return ()
	putStrLn !_ = return ()
	createDirectory !d = checkInvalidPath d ()
	removeDirectory !d = checkInvalidPath d ()
	writeFile !f !_ = checkInvalidPath f ()
	removeExistingFile !f = checkInvalidPath f ()
	copyFile !f !_ = checkInvalidPath f ()
	renameDirectory !d !_ = checkInvalidPath d ()
	hFlush _ = return ()
	message !_ !severity !msg = case severity of
	Error -> PurePrompt $ \_ -> Left $ BreakException
	(show severity ++ ": " ++ msg)
	_ -> return ()

One of the popList calls is the offending one.

[BasLaa]

I changed the unit test to include inputs, let's see what works

[BasLaa]

Everything seems to be working, I'm just wondering how to write a separate test for this. Testing this behaviour is dependent on whether the test environment has git installed. I would assume that since all the tests passed before this change, git was in the test environment. Is there a way to run a test without git installed to see if the behaviour works?

[BasLaa]

Is there someone who could take a look at this?

[ulysses4ever]

Thank you!

[ulysses4ever]

@ffaf1 your comments seem to be stressed. Do you want to OK this one?

As a bug fix, I think we should try to squeeze it in 3.10. Most egregious is Maintainer and Author fields were swapped under cabal init -n.

[ffaf1]

good job!

[ulysses4ever]

@ffaf1 thank you!

@BasLaa could you please put the label on. Either squash+merge if you leave the history as is, or merge-me if you want to cleanup the history manually. The latter may make sense to have the author-email fixup separately.

[BasLaa]

@ulysses4ever I would be happy to separate the fixes, I'm just not sure how to do that. Should I make two separate new PRs with the changes in each?

[ffaf1]

No he means separate commits. Rebase on your machine to a history that you like and force push.

[ulysses4ever]

Yes, Francesco is right: that's what I meant. You'd need to rebase interactively and then force-push.

[BasLaa]

I think I did it correctly? Only changing the first commit message didn't go as I'd hoped.

[ulysses4ever]

@BasLaa looks good, thank you!

[mergify]

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

[BasLaa]

Something seems to have gone wrong with merging here, a test failed for some reason?

[ffaf1]

Force push, sometimes CI is finicky.

[Kleidukos]

@Mergifyio backport 3.10

[mergify]

backport 3.10

✅ Backports have been created

• #9032 Return empty default when git fails (backport #8755) has been created for branch 3.10