Here is my proposal for a cleaner and clearer error code management strategy. You can add your strategies below, and we discuss the strategy to validate and adopt one when we can !

Strategy 1 - unified macro system

We found inconsistencies in how our builtins handle exit codes compared to bash. Some builtins like pwd and echo just return 0 or 1, but others like env and export were doing their own thing, updating data->status directly AND returning different values.

response I think the inconsistencies lies in what we were returning in our builtin functions. It wasn’t that env or export were doing their own thing. It was the fact that the other functions did not have to return a different error code other than 1. I think the most important thing to distinguish is… what is the role of the builtins? I’m now convinced that the builtins should return the data→status, and not just an indicator of failure or success. Data→status will determine failure or success anyway.

This mixed approach caused real bugs. For example, env was supposed to return exit code 2 for "too many arguments" but was actually returning 1 because the function's return value overwrote what we set in data->status. We only caught this during testing, not during development.

response Initially, I thought, the builtin functions were just signaling either success or failure, letting us know that an operation was carried out successfully or to return us to the minishell prompt — all the while data→status was still being updated. And my understanding is that data→status is like a global variable, storing the latest error code as the program continues. So, it didn’t matter whether the builtin functions returned data→status or exit_success or exit_failure. The only functions affected by this, is export and exit Exit, for obvious reasons, but export because it needed to distinguish between a fatal error (return user back to minishell prompt) or just an invalid key error (don’t stop, ignore and keep going). The error was in the execute_builtin function which was over-writing the data→status updated in the env function. I am now convinced that we should return data→status in ALL builtin_functions (even if it is not updated, since if it’s not updated it was initialized to 0 anyway which represents success). This gives us more consistency and prevents overwrites.

Having different approaches for each builtin (except exit which is genuinely special) makes the code messy and hard to debug. We need a consistent system that everyone can understand and follow.

response Agree with you on this, that is why I vote that, as a rule, we just return data→status for all builtin functions.

Solution

Nothing changes much, that’s the beauty of it. And mostly because, in my mind, we need to keep the overall logic for future steps (signals and process). The fix are pretty straightforward, we're just making everything consistent:

  1. update the centralized error handling system using more descriptive macros
#define BUILTIN_SUCCESS 0
#define GENERAL_ERROR 1
#define MISUSAGE_ERROR 2
#define INTERNAL_ERROR  125
#define CMD_NOT_FOUND_ERROR 127
// add more errors macros along the way if needed

Instead of mixing EXIT_SUCCESS, EXIT_FAILURE, and random numbers, every builtin uses these descriptive macros. Same values, clearer names. execute_builtin handles putting the result into data->status only once. No more double path for assignments.

  1. update builtin_env to follow the same pattern as the other builtin

Remove the data->status = whatever lines from inside this functions. Just return the macro like everyone else:

int	builtin_env(char **tokens, t_shell *data)
{
	if (tokens && tokens[1])
	{
		print_error(ERR_ENV, ERR_TOO_MANY_ARGS, NULL, NULL);
		return (MISUSAGE_ERROR);
	}
	if (!data->env_list)
		return (INTERNAL_ERROR);
	if (print_env_list(data->env_list) == -1)
		return (INTERNAL_ERROR);
	return (BUILTIN_SUCCESS);
}
  1. update builtin_export to follow the same pattern as the other builtin