Re: [PATCH v8 5/6] diff-lib: refactor out diff_change logic
- From
Glen Choo <chooglen@google.com>
- Date
- Feb 14, 2023, 04:03 UTC
- Message-ID
- <kl6lwn4kopum.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <CAFySSZDz2BE4yrWKrWZhBx_yFBTUcWYtM9Fie-Npa84Ln0RTZA@mail.gmail.com>
Calvin Wan <calvinwan@google.com> writes:
Show 31 quoted lines
>> > @@ -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.