Re: [PATCH v8 5/6] diff-lib: refactor out diff_change logic

From: Glen Choo <chooglen@google.com>
To: Calvin Wan <calvinwan@google.com>
Cc: git@vger.kernel.org, avarab@gmail.com, newren@gmail.com,
	jonathantanmy@google.com, phillip.wood123@gmail.com
Subject: Re: [PATCH v8 5/6] diff-lib: refactor out diff_change logic
Date: Tue, 14 Feb 2023 12:03:45 +0800	[thread overview]
Message-ID: <kl6lwn4kopum.fsf@chooglen-macbookpro.roam.corp.google.com> (raw)
In-Reply-To: <CAFySSZDz2BE4yrWKrWZhBx_yFBTUcWYtM9Fie-Npa84Ln0RTZA@mail.gmail.com>

Calvin Wan <calvinwan@google.com> writes:

>> > @@ -245,21 +269,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
>> >                       newmode = ce_mode_from_stat(ce, st.st_mode);
>> >               }
>> >
>> > -             if (!changed && !dirty_submodule) {
>> > -                     ce_mark_uptodate(ce);
>> > -                     mark_fsmonitor_valid(istate, ce);
>> > -                     if (!revs->diffopt.flags.find_copies_harder)
>> > -                             continue;
>> > -             }
>> > -             oldmode = ce->ce_mode;
>> > -             old_oid = &ce->oid;
>> > -             new_oid = changed ? null_oid() : &ce->oid;
>> > -             diff_change(&revs->diffopt, oldmode, newmode,
>> > -                         old_oid, new_oid,
>> > -                         !is_null_oid(old_oid),
>> > -                         !is_null_oid(new_oid),
>> > -                         ce->name, 0, dirty_submodule);
>> > -
>> > +             if (diff_change_helper(&revs->diffopt, newmode, dirty_submodule,
>> > +                                    changed, istate, ce))
>> > +                     continue;
>> >       }
>>
>> If I'm reading the indentation correctly, the "continue" comes right
>> before the end of the for-loop block, so it's a no-op and should be
>> removed.
>
> It is a no-op, but I left it in as future-proofing in case more code is
> added after that block later. I'm not sure whether that line of
> reasoning is enough to leave it in though.

I don't think it is. If we haven't thought of the reason why we would
need to skip code, that seems like YAGNI to me.

As a matter of personal taste, I wouldn't leave a trailing "continue" in
an earlier patch even if I were going to change it in a later patch,
because it looks too much like an unintentional mistake.