Re: [PATCH 27/30] merge-recursive: Apply necessary modifications for directory renames

From: Elijah Newren <newren@gmail.com>
Date: Nov 16, 2017, 03:54 UTC
Message-ID: <CABPp-BHfRtfOhD5bt-DCBgLW8NjzKsohaf4C5ZatWwPS-adYfQ@mail.gmail.com>
In-Reply-To: <CAGZ79kanxn7+bgMxbEfSPZtSr4KBWzk9NHE9F-g4J+2TXbz_mQ@mail.gmail.com>

On Wed, Nov 15, 2017 at 12:23 PM, Stefan Beller <sbeller@google.com> wrote:

Show 23 quoted lines

>> +               if (!strcmp(pair->one->path, pair->two->path)) {
>> +                       /*
>> +                        * Paths should only match if this was initially a
>> +                        * non-rename that is being turned into one by
>> +                        * directory rename detection.
>> +                        */
>> +                       assert(pair->status != 'R');
>> +               } else {
>> +                       assert(pair->status == 'R');
>
> assert() is compiled conditionally depending on whether
> NDEBUG is set, which makes potential error reports more interesting and
> head-scratching. But we'd rather prefer easy bug reports, therefore
> we'd want to (a) either have the condition checked always, when
> you know this could occur, e.g. via
>
>   if (<condition>)
>     BUG("Git is broken, because...");
>
> or (b) you could omit the asserts if they are more of a developer guideline?
>
> I wonder if we want to introduce a BUG_ON(<condition>, <msg>) macro
> that contains (a).

Yeah, I added a few other asserts in other commits too. None of these were written with the expectation that they should or could ever occur for a user; it was just a developer guideline to make sure I (and future others) didn't break certain invariants during the implementation or while making modifications to it.

So that makes it more like (b), but I feel that there is something to be said for having a convenient syntax for expressing pre-conditions that others shouldn't violate when changing the code, and which will be given more weight than a comment. For that, something that is compiled out on many users systems seemed just fine.

But, I have certainly seen abuses of asserts in my time as well (e.g. function calls with important side-effects being placed inside asserts), so if folks have decided it's against git's style, then I understand. I'll remove some, and switch the cheaper checks over to BUG().

>> +                       re->dst_entry->processed = 1;
>> +                       //string_list_remove(entries, pair->two->path, 0);
>
> commented code?

Ugh, that's embarrassing.  I'll clean that out.