From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 5CDDB1F8C8 for ; Thu, 30 Sep 2021 22:06:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344332AbhI3WHy (ORCPT ); Thu, 30 Sep 2021 18:07:54 -0400 Received: from pb-smtp21.pobox.com ([173.228.157.53]:62093 "EHLO pb-smtp21.pobox.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S245172AbhI3WHx (ORCPT ); Thu, 30 Sep 2021 18:07:53 -0400 Received: from pb-smtp21.pobox.com (unknown [127.0.0.1]) by pb-smtp21.pobox.com (Postfix) with ESMTP id 497E314AAFD; Thu, 30 Sep 2021 18:06:10 -0400 (EDT) (envelope-from junio@pobox.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=Hn9FwAylS+zMpPCZLKBG6I15j3FZctaEqkTytd 43x7U=; b=HzHRjy45P042+3dNOgQjAWcDf5SPyluTlF1jedjp3yvw48//Av2BxA QsIcf08s7+e5kBIuo8x8le/zRPGIifozeq503rQzUG4JiwHyYeF12Zw0osMAUylQ umClPmYkr26Jp7Gn+JNQFXiCUhgODPQkWogRuQ1Il2U1mt7KomMr4= Received: from pb-smtp21.sea.icgroup.com (unknown [127.0.0.1]) by pb-smtp21.pobox.com (Postfix) with ESMTP id 42C3714AAFC; Thu, 30 Sep 2021 18:06:10 -0400 (EDT) (envelope-from junio@pobox.com) Received: from pobox.com (unknown [104.133.2.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp21.pobox.com (Postfix) with ESMTPSA id 9C44014AAF9; Thu, 30 Sep 2021 18:06:07 -0400 (EDT) (envelope-from junio@pobox.com) From: Junio C Hamano To: David Aguilar Cc: Git Mailing List , =?utf-8?B?w4Z2YXIgQXJuZmo=?= =?utf-8?B?w7Zyw7A=?= Bjarmason , Johannes Schindelin Subject: Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff() References: <20210930170146.61489-1-davvid@gmail.com> <20210930170146.61489-3-davvid@gmail.com> Date: Thu, 30 Sep 2021 15:06:06 -0700 In-Reply-To: <20210930170146.61489-3-davvid@gmail.com> (David Aguilar's message of "Thu, 30 Sep 2021 10:01:44 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 9CD85978-223A-11EC-B9EF-98D80D944F46-77302942!pb-smtp21.pobox.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org David Aguilar writes: > difftool was forwarding the -1 result from error() to cmd_main(), which > is implementation-defined since it is outside of the 0-255 range > specified by POSIX for program exit codes. > > Stop assigning the result of error() to `ret`. Assign a value of 1 > whenever internal errors are detected instead. Many existing codepaths take advantage of error() returning -1 and I do not see anything is wrong to keep that "negative is error" convention for the value of "ret" variable. Most lines in this patch however split that "ret = error(...)" pattern into two statements and I do not see a very good reason for it. Worse yet, after applying this patch, there still are at least three assignments to "ret" that can give it a negative value: if (!mkdtemp(tmpdir.buf)) { ret = error("could not create '%s'", tmpdir.buf); goto finish; } ret = run_command_v_opt(helper_argv, flags); strbuf_addf(&buf, "%s/wtindex", tmpdir.buf); if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 || write_locked_index(&wtindex, &lock, COMMIT_LOCK)) { ret = error("could not write %s", buf.buf); goto finish; } Among them, the return value from run_command_v_opt() eventually come from wait_or_whine(), I think, so it may be generic -1 or it may be WEXITSTATUS() of the child process. But I am not sure if this partcular caller cares. It is not prepared to handle -1 and positive return from run_command_v_opt() any differently. So I think a single - return ret; + return !!ret; at the end would be much easier to reason about and maintain.