From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.6 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI shortcircuit=no autolearn=ham autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id D677D1F6AC for ; Wed, 4 Jul 2018 09:30:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932997AbeGDJ36 (ORCPT ); Wed, 4 Jul 2018 05:29:58 -0400 Received: from mail-yb0-f193.google.com ([209.85.213.193]:34441 "EHLO mail-yb0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932517AbeGDJ35 (ORCPT ); Wed, 4 Jul 2018 05:29:57 -0400 Received: by mail-yb0-f193.google.com with SMTP id e9-v6so1845261ybq.1 for ; Wed, 04 Jul 2018 02:29:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=5D1htwIm4a8DXcAXG27GSf6Y+D+w+cpNt7Xbv7czQgQ=; b=Ez2EP8eY9zZJC6oC8fwWH96z7DpUbz2rfcIlmcbBP97HcDSYzgx1FO5vw+Oo/PmyTZ hEAqzuUu+TL9A1fd7duMpnm+R42JC4tOT8ZpwxdhDzA2qnFPbE5X7j752aOUSYJqFoD4 tGE2VZL1njZ4s7hjYfRvyoluXbq0o3bY0/BOaEIlcQScN+NC3FcSD/FM57/CaEPNX5Nm /RJcIA480v509qMok4JVil954pRJlFAKq4ZxqFv32jsi7ESdY2tyu3clAELpVolG8/cw tLwzRJEA9f7+I2UWfUfQt0tLrCIEmpRn/PSt95T4hh310o75x9BMM0d6zKSxC1CusGa4 fupw== X-Gm-Message-State: APt69E16mJVmAXOoivuKbg8QdG4b/si0V/HYiZ8qz8qM6BOg1trmOXV2 8/HCm+0tAQI7NUq5EppSeeZE2nXHTiPUHxn+MMnALQ== X-Google-Smtp-Source: AAOMgpdHnGVhbGY2SMaYhm3bVaocazCOfXNL4W1rj8w+13lorirYqHYoYG2T6LCShW3WO59hKzFQ0phGjc1gFf8l2t4= X-Received: by 2002:a25:c0c1:: with SMTP id c184-v6mr563556ybf.76.1530696596475; Wed, 04 Jul 2018 02:29:56 -0700 (PDT) MIME-Version: 1.0 References: <20180703035802.24060-1-jyn514@gmail.com> <26b538bd-df59-d9a6-460d-0b1042b35250@gmail.com> In-Reply-To: <26b538bd-df59-d9a6-460d-0b1042b35250@gmail.com> From: Eric Sunshine Date: Wed, 4 Jul 2018 05:29:45 -0400 Message-ID: Subject: Re: [PATCH 1/3] ls-tree: make optional To: jyn514@gmail.com Cc: Git List Content-Type: text/plain; charset="UTF-8" Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Tue, Jul 3, 2018 at 7:15 PM Joshua Nelson wrote: > On 07/03/2018 03:15 AM, Eric Sunshine wrote: > >> + /* taken from checkout.c; > >> + * we have a simpler case because we never create a branch */ > > > > However, this comment doesn't belong in the code, as it won't be > > particularly helpful to anyone reading the code in the future. This > > sort of note would be more appropriate in the commit message or, even > > better, in the commentary section just below the "---" lines following > > your Signed-off-by:. > > I wasn't aware I could put comments in email generated by > git-send-email, thanks for the tip :) An even more common workflow is to use git-format-patch to generate the patches locally, then edit the patches to add commentary below the "---" line if needed, proof-read everything, and finally use git-send-email to send out the already-generated patches. > >> + object = "HEAD"; > >> + else { > >> + argv++, argc++; > >> + initialized = 1; > >> + } > >> + } > >> + > >> + if (!initialized) // if we've already run get_oid, don't run it again > >> + if (get_oid(object, &oid)) > >> + die("Not a valid object name %s", object); > > > > Can't you accomplish the same without the 'initialized' variable by > > instead initializing 'object' to NULL and then checking it here? > > I think my code wasn't very clear here - 'initialized' checks if 'oid' > has been initialized, not 'object'. AFAIK, structs can't be initialized > to NULL, but my C is not very good so I'd be interested to learn otherwise. Oh, the intention of 'initialized' was quite clear, but it seems unnecessary, which is why I was suggesting an alternative. Unless I'm misunderstanding the code (certainly a possibility), I think 'initialized' is redundant, thus you can get by without it Specifically, the only time you set 'initialized' is when you don't set 'object', which means that you can infer whether 'oid' was initialized based upon whether 'object' was set. So, my suggestion was: const char *object = NULL; ... ... conditionals possibly assigning to 'object' ... ... if (object && get_oid(object, &oid)) die(_("not a valid object name: %s", object); If you arrived at that final 'if' statement and object is still NULL, then you know that 'oid' is already initialized; if 'object' is not NULL, then you use it to initialized 'oid'.