On 12/13/2020 2:47 AM, Elijah Newren wrote:
> Hi,
>
> Sorry for two different email responses to the same email...
>
> Addressing the comments on this patchset mean re-submitting
> en/merge-ort-impl, and causing conflicts in en/merge-ort-2 and this
> series en/merge-ort-3. Since gitgitgadget will not allow me to submit
> patches against a series that isn't published by Junio, I'll need to
> ask Junio to temporarily drop both of these series, then later
> resubmit en/merge-ort-2 after he publishes my updates to
> en/merge-ort-impl. Then when he publishes my updates to
> en/merge-ort-2, I'll be able to submit my already-rebased patches for
> en/merge-ort-3.
Let's chat privately about perhaps creatin
> A couple extra comments below...
>>> + int s, clean = 1;
>>> +
>>> + memset(&combined, 0, sizeof(combined));
>>> +
>>> + detect_regular_renames(opt, merge_base, side1, 1);
>>> + detect_regular_renames(opt, merge_base, side2, 2);
>>
>> Find the renames in each side's diff.
>>
>> I think the use of "1" and "2" here might be better situated
>> for an enum. Perhaps:
>>
>> enum merge_side {
>> MERGE_SIDE1 = 0,
>> MERGE_SIDE2 = 1,
>> };
>>
>> (Note, I shift these values to 0 and 1, respectively, allowing
>> us to truncate the pairs array to two entries while still
>> being mentally clear.)
>
> So, after mulling it over for a while, I created a
>
> enum merge_side {
> MERGE_BASE = 0,
> MERGE_SIDE1 = 1,
> MERGE_SIDE2 = 2
> };
>
> and I made use of it in several places. I just avoided going to an
> extreme with it (e.g. adding another enum for masks or changing all
> possibly relevant variables from ints to enum merge_side), and used it
> more as a document-when-values-are-meant-to-refer-to-sides-of-the-merge
> kind of thing. Of course, this affects two previous patchsets and not
> just this one, so I'll have to post a _lot_ of new patches... :-)
I appreciate using names for the meaning behind a numerical constant.
You mentioned in the other thread that this will eventually expand to
a list of 10 entries, which is particularly frightening if we don't
get some control over it now.
I generally prefer using types to convey meaning as well, but I'm
willing to relax on this because I believe C won't complain if you
pass a literal int into an enum-typed parameter, so the compiler
doesn't help enough in that sense.
> Something I missed in my reply yesterday...
>
> Note that mi->clean is NOT from struct merge_result. It is from
> struct merged_info, and in that struct it IS defined as "unsigned
> clean:1", i.e. it is a true boolean. The merged_info.clean field is
> used to determine whether a specific path merged cleanly.
>
> "clean" from struct merge_result is whether the entirety of the merge
> was clean or not. It's almost a boolean, but allows for a
> "catastrophic problem encountered" value. I added the following
> comment:
> /*
> * Whether the merge is clean; possible values:
> * 1: clean
> * 0: not clean (merge conflicts)
> * <0: operation aborted prematurely. (object database
> * unreadable, disk full, etc.) Worktree may be left in an
> * inconsistent state if operation failed near the end.
> */
>
> This also means that I either abort and return a negative value, or I
> can continue treating merge_result's "clean" field as a boolean.
Having this comment helps a lot!
> But again, this isn't new to this patchset; it affects the patchset
> before the patchset before this one.
Right, when I had the current change checked out, I don't see the
patch that introduced the 'clean' member (though, I _could_ have
blamed to find out). Instead, I just got confused and thought it
worth a question. Your comment prevents this question in the future.
Thanks,
-Stolee
