On Wed, Nov 15, 2017 at 12:23 PM, Stefan Beller [off-list ref] wrote:

quoted

+               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().

quoted

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

commented code?

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