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=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,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 9A66B1F466 for ; Wed, 29 Jan 2020 20:43:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726683AbgA2UnK (ORCPT ); Wed, 29 Jan 2020 15:43:10 -0500 Received: from mail-lj1-f193.google.com ([209.85.208.193]:34184 "EHLO mail-lj1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726317AbgA2UnK (ORCPT ); Wed, 29 Jan 2020 15:43:10 -0500 Received: by mail-lj1-f193.google.com with SMTP id x7so900347ljc.1 for ; Wed, 29 Jan 2020 12:43:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=usp-br.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=kg2O4NEG5TSK+ezP1YUyeJ5l5IY/DIGIbvDkIeYdbz0=; b=BMyf9sbFT/MP+5PQOETnregS7yjbBZkjtQt6Ueq2CHfbk+LIONBbsfN8MWhwqJRAF8 4t9wA94LdoEWGl4WNoULcb0oZ6Mdyv2PXegyp8AzPMgSapbGQX3TL7R7DzkJlH6GiU4y rcDf2DynZQtTvH7q1m8iHKBe3k0zCwFgjZnGVio+kBao4BkBn55exGFIVkc+SOsQhwiw jMZsx9it4Dcwiz+86CTVOT9URVImNFrx8ITu8NtazsQtQrM/G04qUMt2ZBVCvqj5L8uO va8o84iNnE2UMAuaTnmhH7+cdziddT3AWoEA7HXjVteSuWnxy5ujad3PVvgJELD0OkOy PAgw== 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:content-transfer-encoding; bh=kg2O4NEG5TSK+ezP1YUyeJ5l5IY/DIGIbvDkIeYdbz0=; b=NJmSceuYIvM6IJEn6ZGgK/euoPG+AhIV3lF8bvtzR+dzyjxg0nVRgklnxlpgOszNOU +mQogLKRzgBHCtixcPmqix/61xijv3NRfdMnEx7mWmZT0HCrkwvHUSLc35FTOE1FDjs7 tqw0KQF06A6kSgVXqZqUHihH7we9IF8QBC4BCzEwJxUjkn+HyZ0fyvVOs2WXNphnJ1Rg OIX6fFnVZ6J+POtpm6KaOxJ0Qdk25gyBhn1YC+TyOtNUimbxBUCVfjx0+ex0GqMoUweE kSqH6ZE9NX/peDoMvaJ4VF7Kys5hwIxUWhTG/3pEGnajBDWVCuhNoJMjWuZcMD0zA3ct DGmA== X-Gm-Message-State: APjAAAU6lpGU8vEaZidAL35dJMt3S3A2ppTxCWoRUeVIaz2Ge8RkV+vb XxuR3vnjocUAkSCrIAhGtxeBAvSKQgChy+D7GCtONA== X-Google-Smtp-Source: APXvYqx3qtwyg1LoAK/uLT1+FD8Q99CkEHY/FN+6/iaU/xWITWHT9SqbmcByniD/gJh9+nRkmoQmTJnKXtvZ1G2MQwU= X-Received: by 2002:a2e:8646:: with SMTP id i6mr626080ljj.122.1580330588431; Wed, 29 Jan 2020 12:43:08 -0800 (PST) MIME-Version: 1.0 References: <20200129112613.GE10482@szeder.dev> In-Reply-To: From: Matheus Tavares Bernardino Date: Wed, 29 Jan 2020 17:42:57 -0300 Message-ID: Subject: Re: [PATCH v3 08/12] grep: allow submodule functions to run in parallel To: Junio C Hamano Cc: =?UTF-8?Q?SZEDER_G=C3=A1bor?= , Philippe Blain , git , Christian Couder , Jonathan Nieder , =?UTF-8?B?0J7Qu9GPINCi0LXQu9C10LbQvdCw0Y8=?= , =?UTF-8?B?Tmd1eeG7hW4gVGjDoWkgTmfhu41jIER1eQ==?= , Jonathan Tan , Jeff King , Brandon Williams , Stefan Beller Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Jan 29, 2020 at 3:57 PM Junio C Hamano wrote: > > SZEDER G=C3=A1bor writes: > > [...] > > @@ -1071,6 +1072,14 @@ int cmd_grep(int argc, const char **argv, const = char *prefix) > > && (opt.pre_context || opt.post_context || > > opt.file_break || opt.funcbody)) > > skip_first_line =3D 1; > > + > > + /* > > + * Pre-read gitmodules (if not read already) to prevent r= acy > > + * lazy reading in worker threads. > > + */ > > + if (recurse_submodules) > > + repo_read_gitmodules(the_repository, 1); > > > > ... and eventually reach this condition, which then reads the > > submodules even with '--no-index', which is just what a7f3240877 tried > > to avoid, thus triggering the test failure. > > > > It might be that all we need is changing this condition to: > > > > if (recurse_submodules && use_index) Yes, I think that would work. I was only worried that, in case of !use_index, the path taken could somehow lead to an unprotected call to repo_read_gitmodules() (with threads spawned).Then, since the file would not have been pre-loaded by the sequential code, we could encounter a race condition. But by what I've inspected, when use_index is false, grep_directory() will be called to traverse the files, and it does not have repo_read_gitmodules() in its call graph[1]. So the solution should be fine in the point of view of thread-safeness. > Hmph, I wonder if "ignore --recurse-submodules if --no-index" should > have been done as a single liner patch, something along the lines of > "after parse_options() returns, drop recurse_submodules if no-index > was given", i.e. > > @@ -958,6 +946,8 @@ int cmd_grep(int argc, const char **argv, const char = *prefix) > /* die the same way as if we did it at the beginn= ing */ > setup_git_directory(); > } > + if (!use_index) > + recurse_submodules =3D 0; /* ignore */ > > /* > * skip a -- separator; we know it cannot be Yeah, this seems more meaningful, IMHO, as we can easily see that the recurse_submodules option was dropped in favor of using --no-index. [1]: Well, in fact repo_read_gitmodules() *is* in grep_directory()'s call graph, but the only path to it is through the fill_textconv_grep() > fill_textconv() call, which is already guarded by the obj_read_mutex. So there is no problem here.