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=-3.9 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_NONE shortcircuit=no autolearn=ham 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 C431C1F463 for ; Wed, 11 Dec 2019 11:43:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728401AbfLKLnz (ORCPT ); Wed, 11 Dec 2019 06:43:55 -0500 Received: from smtprelay03.ispgateway.de ([80.67.18.15]:11105 "EHLO smtprelay03.ispgateway.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727365AbfLKLnz (ORCPT ); Wed, 11 Dec 2019 06:43:55 -0500 Received: from [24.134.116.61] (helo=[192.168.92.208]) by smtprelay03.ispgateway.de with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.92.3) (envelope-from ) id 1if0PG-00075S-FF; Wed, 11 Dec 2019 12:43:50 +0100 Subject: Re: [PATCH 5/5] commit: support the --pathspec-from-file option To: phillip.wood@dunelm.org.uk, Alexandr Miloslavskiy via GitGitGadget , git@vger.kernel.org Cc: Junio C Hamano References: <9ca7fa57-c438-7243-6ab1-956d8f132d37@gmail.com> <25aaaca1-1c88-d2c6-b502-cd35752ce745@syntevo.com> <4401823b-8039-99b4-2436-ed2f1a571d78@gmail.com> From: Alexandr Miloslavskiy Message-ID: <2b573436-0ed2-9d24-f375-dfea0825a39e@syntevo.com> Date: Wed, 11 Dec 2019 12:43:49 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.0 MIME-Version: 1.0 In-Reply-To: <4401823b-8039-99b4-2436-ed2f1a571d78@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Df-Sender: YWxleGFuZHIubWlsb3NsYXZza2l5QHN5bnRldm8uY29t Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On 10.12.2019 11:42, Phillip Wood wrote: > I don't think it's so bad if the pathspec is cleaned up just after it is used, > the diff below applies on top of your patch - see what you think. The diff > also adds dies if --all is given with --pathspec-from-file which (together > with a test) would be worth adding to you series I think. Unfortunately, your reply came too late, topic was cooking in pu/next for a while and merged into master yesterday: c58ae96f. I understand that your patch consists of two parts: 1) Adding test for --all ------------------------ I must admit that I overlooked that there was a similar test for args-based pathspec. I will add this part in my next topic for --pathspec-from-file. I expect it to appear in the next day or two. I will try to remember to CC you to it. 2) Moving parsing/validation into `parse_and_validate_options()` ------------------------ Again, I agree that having parsing/validation outside is suboptimal. However, with current code, I find it to be a choice between two evils, and my choice was "outside but clear" to "inside but obscure". What I find obscure in your suggestion/patch is that innocently looking `prepare_index()` suddenly clears pathspec as well. It's even harder to see when called through `dry_run_commit()`. Now, let me illustrate. There's a similar case in my TODO list, this time involving a real bug in git. In `init_db()`, the bug occurs in `set_git_dir(real_path(git_dir))`, also due to unexpected clearing. Now that I pointed my finger at it, please try to find what's wrong. I imagine that it won't be easy at all. And it's way harder when there's no reason to dig deep into a very specific line of code. I really try to avoid such type of pitfalls. That's why my choice between two evils is what I did.