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-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,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 35F241F66F for ; Sat, 31 Oct 2020 18:58:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728420AbgJaS6i (ORCPT ); Sat, 31 Oct 2020 14:58:38 -0400 Received: from pb-smtp1.pobox.com ([64.147.108.70]:56616 "EHLO pb-smtp1.pobox.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726627AbgJaS6i (ORCPT ); Sat, 31 Oct 2020 14:58:38 -0400 Received: from pb-smtp1.pobox.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id 624D18FAB8; Sat, 31 Oct 2020 14:58:34 -0400 (EDT) (envelope-from junio@pobox.com) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=a3GzvdU5evF2FWYNAx8SF+h68aU=; b=l1qd5Z iRivtGlxQPj/GBHnyCXLyqQY4OAA7l0hPN3GlhQCeaA6LOQOqyTevcAApqcOelJD IZfVhCxzoAAyQlXRb6LTZQzsBtfgM8ZBdLDEP5gY78+/e1rqNjWyTaZTkrMl50Fq Bz2OrpAGSrsEeEred3FwuTej3UFiiZfES2dp8= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=Oc2DNH250SmEaAy7dS12bw9lzP+kVArk x6KboL4lReCL8KuEr9J4STs7xdxJDgbyX0IC7Awrmyn8x+SMx5PGaE2XYwuKAXbu I6BrSCWDo4W/vTSpxerz4pva1aeNTJoBEX1ms1oGnE4N58Yyn+IjFBltjwK0Eu+f 4XSBvajTrQg= Received: from pb-smtp1.nyi.icgroup.com (unknown [127.0.0.1]) by pb-smtp1.pobox.com (Postfix) with ESMTP id 5A05C8FAB7; Sat, 31 Oct 2020 14:58:34 -0400 (EDT) (envelope-from junio@pobox.com) Received: from pobox.com (unknown [34.75.7.245]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp1.pobox.com (Postfix) with ESMTPSA id D2A5F8FAB6; Sat, 31 Oct 2020 14:58:33 -0400 (EDT) (envelope-from junio@pobox.com) From: Junio C Hamano To: Philippe Blain Cc: Philippe Blain via GitGitGadget , Git mailing list , Thomas Rast , Eric Sunshine , =?utf-8?Q?Ren=C3=A9?= Scharfe Subject: Re: [PATCH 6/6] blame: enable funcname blaming with userdiff driver References: <81143637-F77C-49C5-B55A-57E92AC45881@gmail.com> Date: Sat, 31 Oct 2020 11:58:33 -0700 In-Reply-To: <81143637-F77C-49C5-B55A-57E92AC45881@gmail.com> (Philippe Blain's message of "Sat, 31 Oct 2020 14:02:16 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 131A847C-1BAB-11EB-85D5-D152C8D8090B-77302942!pb-smtp1.pobox.com Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Philippe Blain writes: > * In patch 6, I considered fixing that bug in a different way by > initializing sb.path inside blame.c::setup_scoreboard. This function > already receives 'path' as its second argument, so changing that would not > impact the API. Probably Thomas thought that sb.path was already > initialized in sb when he modified builtin/blame.c::prepare_blame_range > to also send sb.path to line-range.c::parse_range_arg in 13b8f68c1f (log > -L: :pattern:file syntax to find by funcname, 2013-03-28). > > Initializing the path in setup_scoreboard would mean we could also > simplify the API of blame.c::setup_blame_bloom_data since it would not > need to receive path separately and so its second argument could be > removed. I went for the simpler alternative of just sending 'path' to > parse_range_arg instead of sb.path since it felt simpler, but if we feel > it would be better to actually initialize sb.path in setup_scoreboard, > I'll gladly tweak that for v2. > >> But that is merely a potential future clean-up. The local variable >> path is still used one more time in the error message given when >> this parse_range_arg() fails, so at least this change makes the use >> of path more consistent. I like the simplicity of this fix. > > I also like its simplicity, and that's why I chose to submit this as v1. > But I completely agree with you that it is "dangerous" in the sense > that some further modifications to the code could then make the same mistake > and use 'sb.path' thinking it is defined when it is not. > > So I'm thinking of instead initializing it in setup_scoreboard for v2. That does sound like a sensible way to clean it up. Thanks.