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=-3.8 required=3.0 tests=AWL,BAYES_00, 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 2D1921F9E0 for ; Tue, 28 Apr 2020 08:45:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726486AbgD1Ipp (ORCPT ); Tue, 28 Apr 2020 04:45:45 -0400 Received: from cloud.peff.net ([104.130.231.41]:42094 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726363AbgD1Ipo (ORCPT ); Tue, 28 Apr 2020 04:45:44 -0400 Received: (qmail 19737 invoked by uid 109); 28 Apr 2020 08:45:44 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with SMTP; Tue, 28 Apr 2020 08:45:44 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 32707 invoked by uid 111); 28 Apr 2020 08:57:07 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Tue, 28 Apr 2020 04:57:07 -0400 Authentication-Results: peff.net; auth=none Date: Tue, 28 Apr 2020 04:45:43 -0400 From: Jeff King To: Denton Liu Cc: Git Mailing List , Junio C Hamano , Andrew White Subject: Re: [PATCH] Use OPT_CALLBACK and OPT_CALLBACK_F Message-ID: <20200428084543.GC2381876@coredump.intra.peff.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Apr 28, 2020 at 04:36:28AM -0400, Denton Liu wrote: > In the codebase, there are many options which use OPTION_CALLBACK in a > plain ol' struct definition. However, we have the OPT_CALLBACK and > OPT_CALLBACK_F macros which are meant to abstract these plain struct > definitions away. These macros are useful as they semantically signal to > developers that these are just normal callback option with nothing fancy > happening. I think this is worth doing. It's a little easier to read, and sets a better example anyone copying the code. > Replace plain struct definitions of OPTION_CALLBACK with OPT_CALLBACK or > OPT_CALLBACK_F where applicable. The heavy lifting was done using the > following (disgusting) shell script: > [...] I'll admit I gave only a quick read through the results. I think between your script, the manual look-over for style, and the fact that the compiler would catch any minor syntactic screwups, I'm not likely to see any new errors. > I contemplated breaking this down into file-sized patches but I don't > think it really makes sense in this case since it's the same change > which is being made in each file and, imo, it wouldn't really ease > reviewer burden since the same number of changes are being reviewed. As a reviewer I much prefer one big patch like this, since these are all the same case: changing style X to style Y. I'd want patches broken out if there were other case (say, the parameters for some version needed fixed up as you were converting). But I don't see that here. -Peff