Not a huge fan of complexity, but would it make sense to turn these into an enum in a later revision for human readability? I'm game for making the change after you're done if you're busy with other things.

Sorry -- forgot to sign-off in my last submission. An answer to my prior question/request shouldn't block this change landing since this change should definitely be made (regardless of how it's expressed) as it makes the code more "functionally correct". A code mod switching 0/1/2 to a structured enum, e.g., DIFF_EX_SAME, DIFF_EX_DIFF, DIFF_EX_ERROR can be done at a later point as part of a larger cleanup/code mod.

This revision is now accepted and ready to land.Sun, Mar 1, 7:50 PM

I'm sorry -- it's still too early for my brain to be effective at code reviewing. I'll retract my prior acceptance until we get some clarity on some of the changes (some of the changes going from 1->2 look correct, but I'm not sure about some of the edge cases in skip/readin).

usr.bin/diff3/diff3.c
270–290	Is this the correct exit code to make in these scenarios? Without better inspecting the code, it's hard to grok what it's doing here.
567	Does an error signifier need to be checked prior to this, e.g., `feof`?
598	We can't determine if it's 1 or 2 without inspecting the descriptors with feof(3).
655	LGTM!
903–923	This all LGTM!

This revision now requires changes to proceed.Sun, Mar 1, 8:00 PM

man page

Herald added a subscriber: ziaee. · View Herald TranscriptSun, Mar 1, 8:08 PM

Harbormaster completed remote builds in B71119: Diff 172981.Sun, Mar 1, 8:08 PM

I suppose we could, but it would be a huge diff that changed nothing.

des added inline comments.Sun, Mar 1, 8:14 PM

usr.bin/diff3/diff3.c
270–290	Yes, it means `diff` is broken and we can't parse its output.
567	No, EOF here is just as much an unrecoverable error as an actual I/O error.
598	Yes we can. We're reading structured data. This error is only reached if the input is invalid.

lgtm

ngie added inline comments.Sun, Mar 1, 9:41 PM

usr.bin/diff3/diff3.c
270–290	Ok, I reread the comment header for the function, and now better understand what you meant both with your comment and this change. Being helpful to my past self: these conditionals confirm that the output format for number ranges in diffs are valid. In the event the output is invalid, e.g., contains negative steps like "9-1" (invalid) vs 1-9 (valid), now the code will exit with 2 instead of 1.
567	This is the only area in the diff (now) that I'm potentially unsure about. Is there a way to easily test/confirm this with GNU diff3?

ngie added inline comments.Sun, Mar 1, 10:03 PM

usr.bin/diff3/diff3.1
200–203	I think this is the same thing.
200–203	.Ex -std would have been the same thing. Not sure how the `Additionally, if the` part snuck in there.
204–212	Not 100% sold on this rewording, but... just throwing this out there as a potential change. @ziaee : thoughts?

des added inline comments.Sun, Mar 1, 10:26 PM

usr.bin/diff3/diff3.c
567	This function is called when we want to skip over part of the diff we're parsing. If `get_line()` returns `NULL`, it means either a broken pipe or incomplete input. Either way, that's an unrecoverable error.