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.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,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 574E81F619 for ; Mon, 24 Feb 2020 22:27:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728040AbgBXW11 (ORCPT ); Mon, 24 Feb 2020 17:27:27 -0500 Received: from mail-wr1-f68.google.com ([209.85.221.68]:40890 "EHLO mail-wr1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727219AbgBXW11 (ORCPT ); Mon, 24 Feb 2020 17:27:27 -0500 Received: by mail-wr1-f68.google.com with SMTP id t3so12318761wru.7 for ; Mon, 24 Feb 2020 14:27:25 -0800 (PST) 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=1S86DWZ2MynkSrDs1nwMWGVqtun8tCeIgacSF9XFqUQ=; b=BcG2Ch1r88MbDXlerf/KQb0akH000M7umH3M0q37CeB0STeY4ru1F4NMaNlcOkmHVz QZLzjfKoaZkHtCEriYd/ekgO7S1GApCUplOTpnxj3Sdpq7aMGnRS4PzLcKErSTAZQOjk VkwfPMZC3WZCn1IlYN4zs1jQbNvMGTUmf590gEaVFKZcup5ldNs/9aq7EkNQc/chNjO4 BMDVhBLnE+KQAE/oKmaaWNTW7vMDZGcjJRaJPNJvQUcYVdemgtjk9B0jhCTgCIvChbco kGynV0dv5+DKH9oCS4gJqaHMOm/K3a7CVNe0ihaIDSeVd4s1+20xW3ooMH3mczgm1XF2 OQSA== X-Gm-Message-State: APjAAAXSab4u6Kr0H3oQRzLyiHMuRGFbKG9rV+Kt8ofSls1DZAIJGgKE dpNAfy0K9TWStdNDs1smk9IKn7sA1kUNOpsOKqY= X-Google-Smtp-Source: APXvYqzYLarr7vFPOTymWSUrl1awezPLDLJyCI29b5atcTXyyc8HqPnLJ2WC3aCJze1OOrgPiE3hC+cW6DOXD0jCEdg= X-Received: by 2002:adf:fd87:: with SMTP id d7mr72482427wrr.226.1582583245161; Mon, 24 Feb 2020 14:27:25 -0800 (PST) MIME-Version: 1.0 References: <8718facbc951614f19407afa6ca8d6110507483d.1582484231.git.gitgitgadget@gmail.com> In-Reply-To: From: Eric Sunshine Date: Mon, 24 Feb 2020 17:27:14 -0500 Message-ID: Subject: Re: [PATCH v3 1/3] get_main_worktree(): allow it to be called in the Git directory To: Johannes Schindelin Cc: Hariom verma , git , Junio C Hamano 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 Mon, Feb 24, 2020 at 1:58 PM Johannes Schindelin wrote: > > On Mon, Feb 24, 2020 at 7:12 AM Eric Sunshine wrote: > > > This change makes the code unnecessarily confusing and effectively > > > turns the final line into dead code. I would much rather see the three > > > cases spelled out explicitly, perhaps like this: > > > > > > if (!strbuf_strip_suffix(&worktree_path, "/.git/.") && /* in .git dir */ > > > !strbuf_strip_suffix(&worktree_path, "/.git/")) /* in worktree */ > > > strbuf_strip_suffix(&worktree_path, "/."); /* in bare repo */ > > I would be really cautious about that. > > To me, the originally proposed change says: strip `/.`, if any. Then, > strip `/.git`, and if successful, strip another `/.`, if any. That's not at all what the original said, which is reproduced here: if (!strbuf_strip_suffix(&worktree_path, "/.git")) strbuf_strip_suffix(&worktree_path, "/."); It says "try stripping '/.git'; if that fails, try stripping '/.'". That is, it recognizes and handles two distinct cases: (1) the path to the .git directory of a non-bare repository, which always ends with "/.git", and (2) the path to a bare git repository, which always ends with "/.". So, the original code wasn't doing any sort of incremental stripping of suffixes; it was just handling two known distinct cases. Perhaps you missed the '!' in the conditional? > That reads pretty fine to me. It makes sense. To me, it doesn't make sense to update the code as done by the patch since that just muddies the issue by making it seem as if get_git_common_dir() is indeterminately tacking on various suffixes rather than giving us deterministic results. > Above-mentioned proposal, however, puts quite a few twists into my brain, > as is a "if neither X nor Y then Z", and I find the code comments outright > confusing. It's just three distinct cases my proposed code is handling; there are no twists. > The scenario in which we found the buggy behavior involved calling > `find_shared_symref()`. I imagine that we could use `git branch -D` inside > the `.git` directory for the new regression test. > > But yes, in my testing, `git worktree list` and `git -C .git worktree > list` do show a different top-level directory (the latter shows an > incorrect one). Such a test case would find a splendid home in t2402. I don't have strong feelings about how it is tested, but would like to see some sort of test proving that it works as expected following this change.