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

[Elijah Newren]

On Wed, Nov 15, 2017 at 12:23 PM, Stefan Beller  wrote:
>> +   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 ()
> 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(, ) 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.

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

[Stefan Beller]

> +   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 ()
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(, ) macro
that contains (a).


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

commented code?