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: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-4.2 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_LOW,SPF_HELO_PASS,SPF_PASS 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 09AD41F5AE for ; Mon, 17 May 2021 13:51:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235731AbhEQNxH (ORCPT ); Mon, 17 May 2021 09:53:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60604 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237598AbhEQNww (ORCPT ); Mon, 17 May 2021 09:52:52 -0400 Received: from mail-qk1-x731.google.com (mail-qk1-x731.google.com [IPv6:2607:f8b0:4864:20::731]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E605EC06134E for ; Mon, 17 May 2021 06:51:30 -0700 (PDT) Received: by mail-qk1-x731.google.com with SMTP id 197so5702999qkl.12 for ; Mon, 17 May 2021 06:51:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=EHAOLDinmLsHKSM9bvctORtaTzPHr8dl4l6YsCK4lEY=; b=OxKdTn6cIG04Z/KSFcycIhjG/K5SxOFmQC8GqyOfKQwhDUdFnBGBSWHdukD5w0kbN/ XwCHbZl6Wh9rFD/8CXsvn3Mvl2piUdjbySQ0QnKv8frhs7Bsc4CEFak0xknQbOKer5rG 9SxGZxFDSyBltXQdG4aGOt5c6WCMsboh7SGkhYJ2DhE5jObG4kWR1YbTG+V1vnxmamEt E8FaC7wtL+sZdI5QT7nkbqrB07LDUA18a6dtU3kdB8H7y6NPRyT/G+JFaflBqTx71vla ewYl4YQj+agjevps2KWXu18WPgHQV5n8oMELmtjfeFg+4P1kKX1+O0fcjeixBkyN//sQ isiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=EHAOLDinmLsHKSM9bvctORtaTzPHr8dl4l6YsCK4lEY=; b=Wn+2btyS2X1NHDTr6Yeee5GHrHtEmpNUZEb1Tgik+vw1V6D/DrSGqMU8MdZaZhGy0Y xPNpmerF7s1Mxc7kKGIX0lk8oYGu3otNsAsFDICbxIx1La+eQ+lIArMp1IFxjSXrchcQ pmJ/lqFBcoxG8Lj+vOD0mu+aEuxuzdmQLS7lMfiQAcfAO0aRrT1lntbMAipSEsXHIZkv 6AfSH0gF+Tc+EqPL6PBO5jSZblC92RyN1lv2Ki9dAK+d7usgQnWMCVWVH3ffQmQXltw3 SZbgQ8GJW8+bw3FA6bcm1jkiUZDhkvph8ZKAmElfn70SWQp9xcw5NYb8QZsNdJD4rtit po0Q== X-Gm-Message-State: AOAM533r+pA1JspZMzH0fgagbd3ugGPWWqgg/tieET3weqlE6l9qXLy4 nZOq1X6Rx8QbJmlL+D26zXA= X-Google-Smtp-Source: ABdhPJzFCrDjp2u9Iqr4Jt527Ud2AQSkX8+OGHHxqJkZNNakS37sM2doXlvSRGQK/sbw67WtaQ7p9A== X-Received: by 2002:a37:4047:: with SMTP id n68mr2399078qka.440.1621259490025; Mon, 17 May 2021 06:51:30 -0700 (PDT) Received: from Derricks-MBP.attlocal.net ([2600:1700:e72:80a0:4c59:3ee5:6b74:d819]) by smtp.gmail.com with ESMTPSA id a19sm11090192qtn.97.2021.05.17.06.51.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 May 2021 06:51:29 -0700 (PDT) Subject: Re: [PATCH v2 07/13] merge-ort: populate caches of rename detection results To: Elijah Newren via GitGitGadget , git@vger.kernel.org Cc: Derrick Stolee , Jonathan Tan , Taylor Blau , Elijah Newren References: <02d517f052a35a952c726e7e941650ce424abb85.1620094339.git.gitgitgadget@gmail.com> From: Derrick Stolee Message-ID: Date: Mon, 17 May 2021 09:51:28 -0400 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <02d517f052a35a952c726e7e941650ce424abb85.1620094339.git.gitgitgadget@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On 5/3/21 10:12 PM, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren > > Fill in cache_pairs, cached_target_names, and cached_irrelevant based on > rename detection results. Future commits will make use of these values. Thank you for continuing to break this down into nice-sized pieces. > +static void possibly_cache_new_pair(struct rename_info *renames, > + struct diff_filepair *p, > + unsigned side, > + char *new_path) > +{ > + char *old_value; > + int dir_renamed_side = 0; > + > + if (new_path) { > + /* > + * Directory renames happen on the other side of history from > + * the side that adds new files to the old directory. > + */ > + dir_renamed_side = 3 - side; Neat trick. Side is in { 1, 2 } so this makes sense. > + } else { > + int val = strintmap_get(&renames->relevant_sources[side], > + p->one->path); > + if (val == RELEVANT_NO_MORE) { > + assert(p->status == 'D'); > + strset_add(&renames->cached_irrelevant[side], > + p->one->path); Ok, I see a transition here from a relevant side to an irrelevant one. > + } > + if (val <= 0) > + return; > + } > + > + if (p->status == 'D') { > + /* > + * If we already had this delete, we'll just set it's value > + * to NULL again, so no harm. > + */ > + strmap_put(&renames->cached_pairs[side], p->one->path, NULL); > + } else if (p->status == 'R') { > + if (new_path) { > + new_path = xstrdup(new_path); > + old_value = strmap_put(&renames->cached_pairs[dir_renamed_side], > + p->two->path, new_path); > + strset_add(&renames->cached_target_names[dir_renamed_side], > + new_path); > + assert(!old_value); This assert implies that p->status == 'R' only if this is the first side (and first commit) to show a rename, right? > + } > + if (!new_path) > + new_path = p->two->path; > + new_path = xstrdup(new_path); If new_path was provided as non-NULL, then this is the second time we are dup-ing it. However, that seems correct because we want a different copy or every time we add it to the cached_pairs and cached_target_names data. > + old_value = strmap_put(&renames->cached_pairs[side], > + p->one->path, new_path); > + strset_add(&renames->cached_target_names[side], > + new_path); Since we appear to be doing this in multiple places, would this be a good place for a helper method? We could have it take a `const char *new_path` and have the helper manage the `xstrdup()` so we never forget to do that exactly once per insert to these sets. > + free(old_value); > + } else if (p->status == 'A' && new_path) { > + new_path = xstrdup(new_path); > + old_value = strmap_put(&renames->cached_pairs[dir_renamed_side], > + p->two->path, new_path); > + strset_add(&renames->cached_target_names[dir_renamed_side], > + new_path); > + assert(!old_value); And here's the third instance, making the "three is many" rule kick in. A helper method would help make this easier. You can also have a parameter corresponding to whether you need to free() the old_value or assert it is NULL. > + } > +} > + > static int compare_pairs(const void *a_, const void *b_) > { > const struct diff_filepair *a = *((const struct diff_filepair **)a_); > @@ -2415,6 +2474,7 @@ static int collect_renames(struct merge_options *opt, > char *new_path; /* non-NULL only with directory renames */ > > if (p->status != 'A' && p->status != 'R') { > + possibly_cache_new_pair(renames, p, side_index, NULL); > diff_free_filepair(p); > continue; > } > @@ -2426,11 +2486,11 @@ static int collect_renames(struct merge_options *opt, > &collisions, > &clean); > > + possibly_cache_new_pair(renames, p, side_index, new_path); > if (p->status != 'R' && !new_path) { > diff_free_filepair(p); > continue; > } > - nit: this deletion seems unnecessary. > if (new_path) > apply_directory_rename_modifications(opt, p, new_path); > > @@ -3701,8 +3761,16 @@ static void merge_start(struct merge_options *opt, struct merge_result *result) > NULL, 1); > strmap_init_with_options(&renames->dir_renames[i], > NULL, 0); > + /* > + * relevant_sources uses -1 for the default, because we need > + * to be able to distinguish not-in-strintmap from valid > + * relevant_source values from enum file_rename_relevance. > + * In particular, possibly_cache_new_pair() expects a negative > + * value for not-found entries. > + */ > strintmap_init_with_options(&renames->relevant_sources[i], > - 0, NULL, 0); > + -1 /* explicitly invalid */, > + NULL, 0); > strmap_init_with_options(&renames->cached_pairs[i], > NULL, 1); > strset_init_with_options(&renames->cached_irrelevant[i], > Functionally looks good. I just had some nits about organization. Thanks, -Stolee