From: Junio C Hamano <gitster@pobox.com>
To: Elijah Newren <newren@gmail.com>
Cc: "Derrick Stolee" <stolee@gmail.com>,
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>,
"Git Mailing List" <git@vger.kernel.org>,
"Derrick Stolee" <dstolee@microsoft.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Jonathan Tan" <jonathantanmy@google.com>,
"Taylor Blau" <me@ttaylorr.com>
Subject: Re: [PATCH 3/8] merge-ort: record the reason that we want a rename for a directory
Date: Sat, 27 Mar 2021 19:01:45 -0700 [thread overview]
Message-ID: <xmqq4kgwox2u.fsf@gitster.g> (raw)
In-Reply-To: <CABPp-BHLL_u_E-Ss=AYWUZM+_9hbC6D6WzRjxEUwmexTEgQ2zw@mail.gmail.com> (Elijah Newren's message of "Mon, 15 Mar 2021 08:27:48 -0700")
Elijah Newren <newren@gmail.com> writes:
> On Mon, Mar 15, 2021 at 7:31 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 3/13/2021 5:22 PM, Elijah Newren via GitGitGadget wrote:
>> > From: Elijah Newren <newren@gmail.com>
>> >
>> > When one side of history renames a directory, and the other side of
>> > history added files to the old directory, directory rename detection is
>> > used to warn about the location of the added files so the user can
>> > move them to the old directory or keep them with the new one.
>> >
>> > This sets up three different types of directories:
>> > * directories that had new files added to them
>> > * directories underneath a directory that had new files added to them
>> > * directories where no new files were added to it or any leading path
>> >
>> > Save this information in dirs_removed; the next several commits will
>> > make use of this information.
>> ...
>> > +/* dir_rename_relevance: the reason we want rename information for a dir */
>> > +enum dir_rename_relevance {
>> > + NOT_RELEVANT = 0,
>> > + RELEVANT_FOR_ANCESTOR = 1,
>> > + RELEVANT_FOR_SELF = 2
>> > +};
>>
>> Is this potentially a flag list? It's hard to tell because we don't
>> have another item (3 or 4?).
>>
>> > unsigned sides = (0x07 - dirmask)/2;
>> > + unsigned relevance = (renames->dir_rename_mask == 0x07) ?
>> > + RELEVANT_FOR_ANCESTOR : NOT_RELEVANT;
>> > + /*
>> > + * Record relevance of this directory. However, note that
>> > + * when collect_merge_info_callback() recurses into this
>> > + * directory and calls collect_rename_info() on paths
>> > + * within that directory, if we find a path that was added
>> > + * to this directory on the other side of history, we will
>> > + * upgrade this value to RELEVANT_FOR_SELF; see below.
>> > + */
>>
>> This comment seems to imply that RELEVANT_FOR_SELF is "more important"
>> than RELEVANT_FOR_ANCESTOR, so the value will just be changed (not a
>> flag).
>
> Yes.
>
>> > + /*
>> > + * Here's the block that potentially upgrades to RELEVANT_FOR_SELF.
>> > + * When we run across a file added to a directory. In such a case,
>> > + * find the directory of the file and upgrade its relevance.
>> > + */
>> > + if (renames->dir_rename_mask == 0x07 &&
>> > + (filemask == 2 || filemask == 4)) {
>> > + /*
>> > + * Need directory rename for parent directory on other side
>> > + * of history from added file. Thus
>> > + * side = (~filemask & 0x06) >> 1
>> > + * or
>> > + * side = 3 - (filemask/2).
>> > + */
>> > + unsigned side = 3 - (filemask >> 1);
>> > + strintmap_set(&renames->dirs_removed[side], dirname,
>> > + RELEVANT_FOR_SELF);
>>
>> Yes, using "RELEVANT_FOR_SELF" here, not "relevance | RELEVANT_FOR_SELF".
>> OK. This should make the later consumers simpler.
>
> Yep, indeed. Would it make it clearer earlier if I were to stop
> assigning the explicit values in the enum? Would adding a comment
> when I introduce the enum be easier? Or was it just "thinking out
> loud"?
You are not asking me, but if you were, I'd say not using enum for
bitmask would be a good discipline to follow, and an enum like this
one that is used only for uniqueness of the values would benefit from
not having explicit value assignments.
next prev parent reply other threads:[~2021-03-28 2:02 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-13 22:22 [PATCH 0/8] Optimization batch 10: avoid detecting even more irrelevant renames Elijah Newren via GitGitGadget
2021-03-13 22:22 ` [PATCH 1/8] diffcore-rename: take advantage of "majority rules" to skip more renames Elijah Newren via GitGitGadget
2021-03-13 22:22 ` [PATCH 2/8] merge-ort, diffcore-rename: tweak dirs_removed and relevant_source type Elijah Newren via GitGitGadget
2021-03-13 22:22 ` [PATCH 3/8] merge-ort: record the reason that we want a rename for a directory Elijah Newren via GitGitGadget
2021-03-15 14:31 ` Derrick Stolee
2021-03-15 15:27 ` Elijah Newren
2021-03-28 2:01 ` Junio C Hamano [this message]
2021-03-13 22:22 ` [PATCH 4/8] diffcore-rename: only compute dir_rename_count for relevant directories Elijah Newren via GitGitGadget
2021-03-13 22:22 ` [PATCH 5/8] diffcore-rename: check if we have enough renames for directories early on Elijah Newren via GitGitGadget
2021-03-13 22:22 ` [PATCH 6/8] diffcore-rename: add computation of number of unknown renames Elijah Newren via GitGitGadget
2021-03-13 22:22 ` [PATCH 7/8] merge-ort: record the reason that we want a rename for a file Elijah Newren via GitGitGadget
2021-03-13 22:22 ` [PATCH 8/8] diffcore-rename: determine which relevant_sources are no longer relevant Elijah Newren via GitGitGadget
2021-03-15 15:21 ` [PATCH 0/8] Optimization batch 10: avoid detecting even more irrelevant renames Derrick Stolee
2021-03-15 15:34 ` Elijah Newren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq4kgwox2u.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jonathantanmy@google.com \
--cc=me@ttaylorr.com \
--cc=newren@gmail.com \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).