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: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-7.9 required=3.0 tests=AWL,BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_NONE,USER_IN_DEF_DKIM_WL shortcircuit=no autolearn=no autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 4F9E01F462 for ; Fri, 14 Jun 2019 20:59:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726052AbfFNU75 (ORCPT ); Fri, 14 Jun 2019 16:59:57 -0400 Received: from mail-pl1-f194.google.com ([209.85.214.194]:38573 "EHLO mail-pl1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725809AbfFNU75 (ORCPT ); Fri, 14 Jun 2019 16:59:57 -0400 Received: by mail-pl1-f194.google.com with SMTP id f97so1481069plb.5 for ; Fri, 14 Jun 2019 13:59:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=OJYJhKf6rSb5+FY8ww33ZMIP3QAbgBmY7gCPcC+sfe4=; b=M33gTVpcHevvFgM/s6TqR1j+x/FJ5jUtc58QvBBciX7+ZHIbVDhribkVTHz13uZ1ox uy4hBsO1UXIIwRJOQAX0X0GUqsIQNkFYyeMrYYL5VOt3wskYaVfxSLc3WIS8qxOAqtsN WBQIV7ADm9KLkV1n7/lqxDm9Re4qcMLy4Yc7OF2Zx+tRv6ILXIBF2bd9fUc5J6xItb37 rSPk8RVWfD0ZqHn5IYu2dP3wQ8yD6U/6H3k9EzD+tzzxWXC9BIVte2fPRf7Gf59bw2Fd skLQ0CshH6Nq5/vufUCsS5vDTlEvPthTJnFlMVENksojz/WpOJN7uCi7phRDEuWxrcWQ Xiiw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=OJYJhKf6rSb5+FY8ww33ZMIP3QAbgBmY7gCPcC+sfe4=; b=eZOUvolJ8qUrcr6tv9iP+5VbLEdrM+AkehG1VIXPxvmYqBv+5y8+ZbiowmxJwQYy3X tyWko8gYnLjWAsrXlZ5K7dFqSPQMck63DajOB7R0Hd8G3Q/aGPGJbsrGSt0nN36NXuaZ GkOZ7IZM4KjkTqkQY95mqLowUdtDcrBgrjwvA2aBMzGVFp+qX7dsIL81NptAmjrZIung VtUDN45GWRhqY2t/EVqJCvrk/5J1adFsIV4hKV5rvkFtB+hmpAupqRoPE8rhb5RoiUhD vy8dWii8lk7lom74pLXKOj1tQTbIpDi5fwwHY+4ndzrww581At9VuE9SXeE6eDloAgpw VZBQ== X-Gm-Message-State: APjAAAX170ItlqT8wrvyAXv/EElp5o11tKyctxjz2lMCFHIKmajFbmkD oNkysi4NoDpdX2xfwRRFdlvygd6QmlY= X-Google-Smtp-Source: APXvYqyhvscIA6jSKwaptIY2EA7KNSNVj8W3XCDWRp7vvy6m/3OcD6KVOTPAIOnauoIFUEH6paKIFg== X-Received: by 2002:a17:902:9006:: with SMTP id a6mr90051195plp.305.1560545995698; Fri, 14 Jun 2019 13:59:55 -0700 (PDT) Received: from google.com ([2620:15c:2ce:0:b186:acdd:e7ae:3d4c]) by smtp.gmail.com with ESMTPSA id s12sm3207821pjp.10.2019.06.14.13.59.54 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Fri, 14 Jun 2019 13:59:55 -0700 (PDT) Date: Fri, 14 Jun 2019 13:59:50 -0700 From: Emily Shaffer To: Jeff King Cc: git@vger.kernel.org Subject: Re: [RFC PATCH] rev-list: clarify --abbrev and --abbrev-commit usage Message-ID: <20190614205950.GC233791@google.com> References: <20190613221541.10007-1-emilyshaffer@google.com> <20190614161841.GB30083@sigill.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190614161841.GB30083@sigill.intra.peff.net> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Fri, Jun 14, 2019 at 12:18:41PM -0400, Jeff King wrote: > On Thu, Jun 13, 2019 at 03:15:41PM -0700, Emily Shaffer wrote: > > > I thought this was odd when I was working on the other rev-list changes > > - --abbrev doesn't do anything on its own. It looks like it does work by > > itself in other commands, but apparently not in rev-list. > > > > Listed this patch as RFC because maybe instead it's better to fix > > something so --abbrev can be used alone, or teach --abbrev-commit=. > > It looks like `git log --abbrev=5` also doesn't work the way one might > > expect, which makes sense to me, as they use the same internals for > > option parsing (parse_revisions()). > > > > The manpages for log and rev-list both correctly indicate that > > --abbrev= is an optional addition to --abbrev-commit. `git log -h` is > > generated by parse-options tooling and doesn't cover --abbrev-commit at > > all, but `git rev-list` doesn't use an option parser on its own and the > > usage is hardcoded. > > Yeah, "--abbrev" is a bit tricky here. It is really "when you abbrev, do > it to this level". In "log", that means that "git log --abbrev=5 --raw" > _does_ do something useful (it abbreviates the blob hashes). And then > you may add "--abbrev-commit" on top to ask to abbreviate the commit > ids. > > And rev-list follows that same pattern, except that rev-list _never_ > shows diff output. You'd traditionally do (and this is how log was > implemented once upon a time): > > git rev-list HEAD | git diff-tree --stdin --abbrev=5 --raw > > But even there, we are not seeing the commit id output by rev-list. It > goes to diff-tree, which then formats it separately. So if you wanted > abbreviated commits there, you'd add "--abbrev-commit" to the diff-tree > invocation, not rev-list! > > So no, I cannot see a way in which "rev-list --abbrev" is useful without > "--abbrev-commit". Which means that perhaps the former should imply the > latter. Maybe it should; maybe this patch is a good excuse to do so, and enforce mutual exclusion of --abbrev-commit/--abbrev and --no-abbrev. Maybe it's also a good time to add --abbrev-commit=? > > And as you noticed in your other patch, there is no way to abbreviate > "--objects" output at all. I am not sure I have ever seen a good use for > that. Though to be honest, I am not sure that "--abbrev" is really all > that useful in the first place. Machine-readable output should never > abbreviate, and human-readable ones generally already do. > > But at any rate, before making any behavior changes it may make sense to > think about how they'd interact with "rev-list --objects" abbreviation, > if it were to be added. > > As for the patch itself: > > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c > > index 9f31837d30..6ae0087b01 100644 > > --- a/builtin/rev-list.c > > +++ b/builtin/rev-list.c > > @@ -49,8 +49,8 @@ static const char rev_list_usage[] = > > " --objects | --objects-edge\n" > > " --unpacked\n" > > " --header | --pretty\n" > > -" --abbrev= | --no-abbrev\n" > > -" --abbrev-commit\n" > > +" --abbrev-commit [--abbrev=]\n" > > +" --no-abbrev\n" > > So --no-abbrev clears both --abbrev and --abbrev-commit. That sort of > makes sense, though I did not expect it. But it also means that: > > --abbrev-commit [--abbrev= | --no-abbrev] > > is not right. Possibly: > > --abbrev-commit [--abbrev=] | --no-abbrev > > would show the interaction more clearly, but I don't have a strong > opinion. I did consider demonstrating it this way, but when both --abbrev-commit and --no-abbrev are used together, we don't complain or reject the invocation - which I would expect if the usage states the two options are mutually exclusive. I've been trying to think of good reasons not to enforce their mutual exclusion, and the one I keep coming back to is that --no-abbrev might be desired to override a git config'd abbreviation length - although I didn't check to see whether we have one, maybe we would want one later. And even in that case, I suppose that --abbrev-commit would not be explicitly added to the call (because we'd infer from the config), or that if it did need to be explicitly added (like if we need the user to say they want abbreviation, but we want to use their configured preferred length) then we could still reject the addition of --no-abbrev. So maybe it makes even more sense to take this patch as an opportunity to make these options mutually exclusive... although that checking I think would wind up in revision.c, and therefore widen the impact of the change significantly. >From a practical standpoint, I guess you could consider --abbrev-commit and --no-abbrev mutually exclusive, but since it's not enforced, I have a little preference not to document it as though it is... - Emily