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=-10.6 required=3.0 tests=AWL,BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_PASS,SPF_PASS,USER_IN_DEF_DKIM_WL 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 E78631F934 for ; Tue, 15 Dec 2020 21:44:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730659AbgLOVnL (ORCPT ); Tue, 15 Dec 2020 16:43:11 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53148 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730266AbgLOVl4 (ORCPT ); Tue, 15 Dec 2020 16:41:56 -0500 Received: from mail-pj1-x1041.google.com (mail-pj1-x1041.google.com [IPv6:2607:f8b0:4864:20::1041]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E2EB7C0613D6 for ; Tue, 15 Dec 2020 13:41:14 -0800 (PST) Received: by mail-pj1-x1041.google.com with SMTP id z12so338613pjn.1 for ; Tue, 15 Dec 2020 13:41:14 -0800 (PST) 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; bh=G1EshF5G1UBdpvjIV0c7HLorPlArb/d3TDyWWSH9CLg=; b=BS1Beb21zMVpFz1KegZPS/dHm5lWDNIew+PhiVsZKSw9gY9NbZ59+21w9MOL09eoaW rD/JEoSKaUPfayqH3Fd230vy5zZ5YZsILobOZK+UgyJBFgnPHV2mlcueeLSYSkqyyy6y aS4qV33QdelgDnc9xhSOaMbTt/9JTbjr0J/RNn5E12MAEcMkJFPZwhXQ6XeSsnt6TfXR A/kqo45FAL7mVVuKHPyr8dWvqaeczKK/st3Sj26OpwnQneb6jrCfslSOhVXCY8V+ucha 4zvakwNUyfs1QjlWGtzuPzLjgXPbIZ8VASWFLG08PLaJHif4/4rYQ4hJGAJYnIIcFEdO TIwg== 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; bh=G1EshF5G1UBdpvjIV0c7HLorPlArb/d3TDyWWSH9CLg=; b=JrJKH6hGcXqih2BmeF19uTxZLW5eLjQe63D/XJZBlc+0uZJVOiSY58XPgFRBH1y3au gNhMaFrwgGUYaypFZTxezICmgsmY74IZxUmSS1SX+ursxWEaqZgHwe8EueKeeRdfbvXP 8ptSgXgggF4rV97CPUJUf5H5dPTUm2ivKPA1XjcNmdSGagPSG5XBas3eBN81TN2gZbuA XUvwbH7Rtl1pap7WZVCy55eyJC3mdNWdy9Qn0ebQyt+M2vJ9LZbX5OOjylARsvHrHnJV 6QUg1mBlLI/xiQomHoZMtINKh8RBgB+tFHIcfP+XX6SNvPQ7kusQbY9yGU+HRupmuAnA f0Ww== X-Gm-Message-State: AOAM532jWPRb0oNmVelxJaVwOnA6T7ySjpqC2wUDvn8LMxXc8MO2zEpr J9shzsrinYebjgX843h1/MamDhe+boNRWg== X-Google-Smtp-Source: ABdhPJwcxbhRk5REAdLGBa7yomL1l6QnbfgiYxX76ie6n/AM3k8REsaD71unZzH2kUldUgmIgIULew== X-Received: by 2002:a17:90a:bf88:: with SMTP id d8mr673197pjs.102.1608068474288; Tue, 15 Dec 2020 13:41:14 -0800 (PST) Received: from google.com ([2620:15c:2ce:0:1ea0:b8ff:fe77:f690]) by smtp.gmail.com with ESMTPSA id t23sm56672pfc.0.2020.12.15.13.41.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Dec 2020 13:41:13 -0800 (PST) Date: Tue, 15 Dec 2020 13:41:09 -0800 From: Emily Shaffer To: phillip.wood@dunelm.org.uk Cc: git@vger.kernel.org Subject: Re: [PATCH 08/17] hook: add 'run' subcommand Message-ID: <20201215214109.GD3783238@google.com> References: <20201205014607.1464119-1-emilyshaffer@google.com> <20201205014607.1464119-9-emilyshaffer@google.com> <23cb3575-7706-45a6-7a50-0fc9ef850b9f@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <23cb3575-7706-45a6-7a50-0fc9ef850b9f@gmail.com> Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Fri, Dec 11, 2020 at 10:15:26AM +0000, Phillip Wood wrote: > > Hi Emily > > On 05/12/2020 01:45, Emily Shaffer wrote: > > In order to enable hooks to be run as an external process, by a > > standalone Git command, or by tools which wrap Git, provide an external > > means to run all configured hook commands for a given hook event. > > > > For now, the hook commands will run in config order, in series. As > > alternate ordering or parallelism is supported in the future, we should > > add knobs to use those to the command line as well. > > > > As with the legacy hook implementation, all stdout generated by hook > > commands is redirected to stderr. Piping from stdin is not yet > > supported. > > > > Legacy hooks (those present in $GITDIR/hooks) are run at the end of the > > execution list. For now, there is no way to disable them. > > > > Users may wish to provide hook commands like 'git config > > hook.pre-commit.command "~/linter.sh --pre-commit"'. To enable this, the > > contents of the 'hook.*.command' and 'hookcmd.*.command' strings are > > first split by space or quotes into an argv_array, then expanded with > > 'expand_user_path()'. > > I'm a bit confused by this last paragraph, the docs below say we pass the > string to the shell and that's what the implementation seems to do. If we're > running a lot of hooks then maybe it would be worth using split_cmdline() > and expand_user_path() rather than invoking the shell for each hook we run. Yeah, I think you are right that the commit message is stale. I had some trouble getting things to work correctly with split_cmdline() and expand_user_path(), so I'd prefer to run with shell. > > I'm afraid I've only had time to skip the patch, there are a couple of minor > comments below. No problem. Thanks for having a look. > > +static int should_include_hookdir(const char *path, enum hookdir_opt cfg) > > +{ > > + struct strbuf prompt = STRBUF_INIT; > > + /* > > + * If the path doesn't exist, don't bother adding the empty hook and > > + * don't bother checking the config or prompting the user. > > + */ > > + if (!path) > > + return 0; > > + > > + switch (cfg) > > + { > > + case hookdir_no: > > Style nit: we normally use uppercase for constants and enums. OK. Thanks - will fix where it's introduced and update subsequent patches. > > > + return 0; > > + case hookdir_unknown: > > + fprintf(stderr, > > + _("Unrecognized value for 'hook.runHookDir'. " > > + "Is there a typo? ")); > > What happens at the moment if core.hooksPath does not exist? When core.hooksPath does not exist then $GIT_DIR/hooks/ is used instead. My setup currently doesn't have $GIT_DIR/hooks/ and runs happily. That bit of logic (core.hooksPath or $GIT_DIR/hooks) is done in run-command.h:find_hook() so I don't worry about it manually here. However, your comment caused me to investigate what happens when core.hooksPath DOES exist - and I found a bug. Because the 'git hook' builtin doesn't call the default configuration callback, I miss core.hooksPath hooks during 'git hook list' - but not during hooks invoked during regular Git process runs. Very confusing :) So thanks for the hint. - Emily