From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mon, 13 Sep 2021 19:27:11 GMT
Subject: Re: [PATCH v7 5/6] bisect--helper: reimplement `bisect_run` shell function in C
Message-ID: <875yv446hj.fsf@evledraar.gmail.com>
In-Reply-To: <20210913173905.44438-6-mirucam@gmail.com>


On Mon, Sep 13 2021, Miriam Rubio wrote:

> +static int print_file_to_stdout(const char *path)
> +{
> +	int fd = open(path, O_RDONLY);
> +	int ret = 0;
> +
> +	if (fd < 0)
> +		return error_errno(_("cannot open file '%s' for reading"), path);
> +	if (copy_fd(fd, 1) < 0)
> +		ret = error_errno(_("failed to read '%s'"), path);
> +	close(fd);
> +	return ret;
> +}

Returns int, but that return value is ignored here, and we don't seem to
gain a caller in 6/6?

> +	if (argc)
> +		sq_quote_argv(&command, argv);
> +	else {
> +		error(_("bisect run failed: no command provided."));
> +		return BISECT_FAILED;
> +	}

Not new in this series & I see this is v7 already, so ....

Just odd to see this BISECT_FAILED pattern (which is defined to -1),
instead of "return error(..." like elsewhere.

Then we take that enum and do a "return -res" from main(), i.e. it's not
even that we're somehow guarding everything with these BISECT_* codes in
this file (see 30276765c11 (bisect--helper: use '-res' in
'cmd_bisect__helper' return, 2020-08-28)).

Anyway, can be cleaned up some other time, but...

> +		if (temporary_stdout_fd < 0)
> +			return error_errno(_("cannot open file '%s' for writing"), git_path_bisect_run());

Here we're doing a "return error...(" directly.

> +	case BISECT_RUN:
> +		if (!argc)
> +			return error(_("bisect run failed: no command provided."));
> +		get_terms(&terms);
> +		res = bisect_run(&terms, argv, argc);
> +		break;
>  	default:
>  		BUG("unknown subcommand %d", cmdmode);
>  	}

Also not a new issue, but if we just covered the BISECT_AUTOSTART case
here, then we wouldn't need this default/BUG, the compiler would check
that we checked all existing enum arms.