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

From: Stefan Beller <sbeller@google.com>
Date: Nov 15, 2017, 20:23 UTC
Message-ID: <CAGZ79kanxn7+bgMxbEfSPZtSr4KBWzk9NHE9F-g4J+2TXbz_mQ@mail.gmail.com>
In-Reply-To: <20171110190550.27059-28-newren@gmail.com>

Show 9 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).

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

commented code?