Re: [PATCH 2/2] object-file: return error on index-pack abnormal exit
Justin Tobler <jltobler@gmail.com> writes:
Show 24 quoted lines
> diff --git a/object-file.c b/object-file.c
> index a9800143bd..e04ce0bc11 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1429,8 +1429,7 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
> struct pack_header hdr;
> const char *hdr_err;
> int err_fd = opts->err_fd;
> - int ret = -1;
> - int status;
> + int ret;
>
> hdr_err = parse_pack_header(&hdr, pack_fd);
> if (hdr_err) {
> @@ -1462,12 +1461,13 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
> child.in = pack_fd;
> child.err = err_fd;
> child.git_cmd = 1;
> - status = run_command(&child);
> - if (status) {
> + if (run_command(&child)) {
> strbuf_addstr(err_msg, "unpack-objects abnormal exit");
> return -1;
> }OK. A slight simplication that does not change any behaviour.
Show 29 quoted lines
> } else {
> + int report_err;
> +
> strvec_pushl(&child.args, "index-pack", "--stdin", NULL);
> push_header_arg(&child.args, &hdr);
>
> @@ -1487,24 +1487,24 @@ static int odb_transaction_files_write_pack(struct odb_transaction *base,
> child.in = pack_fd;
> child.err = err_fd;
> child.git_cmd = 1;
> - status = start_command(&child);
> - if (status) {
> + if (start_command(&child)) {
> strbuf_addstr(err_msg, "index-pack fork failed");
> return -1;
> }
>
> - ret = read_index_pack_report(repo, child.out, &pack_name, NULL);
> + report_err = read_index_pack_report(repo, child.out, &pack_name, NULL);
>
> close(child.out);
>
> - status = finish_command(&child);
> - if (status) {
> + if (finish_command(&child)) {
> strbuf_addstr(err_msg, "index-pack abnormal exit");
> + ret = -1;
> goto out;
> }OK. A slight simplication that does not change any behaviour again.
Show 6 quoted lines
> - if (ret) {
> + if (report_err) {
> strbuf_addstr(err_msg, "index-pack failed to report pack name");
> + ret = -1;
> goto out;
> }I guess this also does not change a behaviour? We used to relay the error status returned by read_index_pack_report(), but now we force the error code to an explicit -1.
Personally, I somehow find that the original "unless we positively declare success, default to 'ret = -1', and if we may have detected more specific error, return it back, and reset ret to 0 when we see a success" easier to follow, but that may be subjective preference.
Will queue. Thanks.
Justin Tobler
