Johannes Schindelin [off-list ref] writes:
quoted
@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
"--signoff", opts->signoff,
"--no-commit", opts->no_commit,
"-x", opts->record_origin,
- "--edit", opts->edit,
+ "--edit", opts->edit == 1,
Honestly, I'd prefer `> 0` here.
Unless somebody (including Elijah) is trying to soon introduce yet
another value to .edit member, I'd agree 100%. If it is a tristate
(unspecified, no, yes), I think "is it positive" should be the way
to ask "does the user definitely wants it?", "is it zero" should be
the way to ask "does the user definitely declines it?" and "is it
non-negative" (and "is it negative") the way to ask "does the user
care (or not care)?". Using that consistently is good.
quoted
+static int should_edit(struct replay_opts *opts) {
+ assert(opts->edit >= -1 && opts->edit <= 1);
Do we really want to introduce more of these useless `assert()`s? I know
that we stopped converting them to `BUG()`, but I really dislike
introducing new ones: they have very little effect, being no-ops by
default in most setups.
Yeah, in a new code in flux where programmers can easily make
errors, "if (...) BUG()" may not be a bad thing to add (but then we
may want to see if we can make the codepaths involved less error
prone), but I agree with your view that assert() is mostly useless.
A comment that explains the expectation and why that expectation is
there would be more useful.
quoted
+ if (opts->edit == -1)
Maybe `< 0`, as we do elsewhere for "not specified"?
Yup.
quoted
+ /*
+ * Note that we only handle the case of non-conflicted
+ * commits; continue_single_pick() handles the conflicted
+ * commits itself instead of calling this function.
+ */
+ return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;
Apart from the extra parentheses, that makes sense to me.
I can take it either way (but personally I think this particular one
is easier to see as written---this is subjective).
...
The rest looks good, and the comments are _really_ helpful.
Yup, I agree.
Thanks for a review.
On Tue, Mar 30, 2021 at 11:47 AM Junio C Hamano [off-list ref] wrote:
Johannes Schindelin [off-list ref] writes:
quoted
quoted
@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
"--signoff", opts->signoff,
"--no-commit", opts->no_commit,
"-x", opts->record_origin,
- "--edit", opts->edit,
+ "--edit", opts->edit == 1,
Honestly, I'd prefer `> 0` here.
Unless somebody (including Elijah) is trying to soon introduce yet
another value to .edit member, I'd agree 100%. If it is a tristate
(unspecified, no, yes), I think "is it positive" should be the way
to ask "does the user definitely wants it?", "is it zero" should be
the way to ask "does the user definitely declines it?" and "is it
non-negative" (and "is it negative") the way to ask "does the user
care (or not care)?". Using that consistently is good.
Sounds good; I'll switch it over.
quoted
quoted
+static int should_edit(struct replay_opts *opts) {
+ assert(opts->edit >= -1 && opts->edit <= 1);
Do we really want to introduce more of these useless `assert()`s? I know
that we stopped converting them to `BUG()`, but I really dislike
introducing new ones: they have very little effect, being no-ops by
default in most setups.
Yeah, in a new code in flux where programmers can easily make
errors, "if (...) BUG()" may not be a bad thing to add (but then we
may want to see if we can make the codepaths involved less error
prone), but I agree with your view that assert() is mostly useless.
A comment that explains the expectation and why that expectation is
there would be more useful.
Since you both don't like this assert, I'll remove it. But I strongly
disagree that assert is useless in general. If you two have such a
strong reaction to assert statements, though, would you two prefer
that I add a new affirm() function that is ALSO compiled out in
production? Because I really want to use one of those. My operating
assumptions with asserts are the following:
1) If the check is relevant for production, assert() statements should
NOT be used; if(...) BUG() should be used instead.
2) assert statements will be compiled out in production, almost always
2a) NOTE: don't make asserts expensive, since a few production users
will keep them
3) assert statements will be active for future me and some other folks
doing active git development
Do you two disagree with any of those operating assumptions? I find
asserts very valuable because:
* It's a _concise_ code comment that is readily understood. Any
attempt to word in English the same thing that an assert statement
checks always takes longer
* It helps future folks tweaking the rules to catch additional
locations where assumptions were made about the old rules. In the
development of merge-ort, for example, asserts shortened my debugging
cycles as I found and attempted new optimizations or added new
features or changed data structures and so on. The checks were _only_
assists while developing; once the code is right, the checks could be
removed. But future development might occur, so it'd be nice to have
a way to keep the checks in the code just for those future developers
while production users remove them.
In particular, for merge-ort, I think the second point is very
helpful. What can achieve the "remove these now-unnecessary checks
from the code for production, but keep them there for future
development"? I thought assert() was created exactly for this
purpose. Would you rather I created an affirm() that does essentially
the same thing and is compiled out unless DEVELOPER=1? That would
allow us to declare all assert() calls in the code as buggy, but I'm
not sure affirm() is as readily understood by developers reading the
code as "ooh, a reminder I get to assume these statements are true
while I'm reading the rest of the code".
quoted
quoted
+ if (opts->edit == -1)
Maybe `< 0`, as we do elsewhere for "not specified"?
Yup.
quoted
quoted
+ /*
+ * Note that we only handle the case of non-conflicted
+ * commits; continue_single_pick() handles the conflicted
+ * commits itself instead of calling this function.
+ */
+ return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;
Apart from the extra parentheses, that makes sense to me.
I can take it either way (but personally I think this particular one
is easier to see as written---this is subjective).
quoted
...
The rest looks good, and the comments are _really_ helpful.
Yup, I agree.
Thanks for a review.
Indeed; and thanks to you as well Junio for all your time reviewing.
On Tue, Mar 30 2021, Elijah Newren wrote:
On Tue, Mar 30, 2021 at 11:47 AM Junio C Hamano [off-list ref] wrote:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
quoted
@@ -182,7 +182,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
"--signoff", opts->signoff,
"--no-commit", opts->no_commit,
"-x", opts->record_origin,
- "--edit", opts->edit,
+ "--edit", opts->edit == 1,
Honestly, I'd prefer `> 0` here.
Unless somebody (including Elijah) is trying to soon introduce yet
another value to .edit member, I'd agree 100%. If it is a tristate
(unspecified, no, yes), I think "is it positive" should be the way
to ask "does the user definitely wants it?", "is it zero" should be
the way to ask "does the user definitely declines it?" and "is it
non-negative" (and "is it negative") the way to ask "does the user
care (or not care)?". Using that consistently is good.
Sounds good; I'll switch it over.
quoted
quoted
quoted
+static int should_edit(struct replay_opts *opts) {
+ assert(opts->edit >= -1 && opts->edit <= 1);
Do we really want to introduce more of these useless `assert()`s? I know
that we stopped converting them to `BUG()`, but I really dislike
introducing new ones: they have very little effect, being no-ops by
default in most setups.
Yeah, in a new code in flux where programmers can easily make
errors, "if (...) BUG()" may not be a bad thing to add (but then we
may want to see if we can make the codepaths involved less error
prone), but I agree with your view that assert() is mostly useless.
A comment that explains the expectation and why that expectation is
there would be more useful.
Since you both don't like this assert, I'll remove it. But I strongly
disagree that assert is useless in general. If you two have such a
strong reaction to assert statements, though, would you two prefer
that I add a new affirm() function that is ALSO compiled out in
production? Because I really want to use one of those. My operating
assumptions with asserts are the following:
1) If the check is relevant for production, assert() statements should
NOT be used; if(...) BUG() should be used instead.
2) assert statements will be compiled out in production, almost always
2a) NOTE: don't make asserts expensive, since a few production users
will keep them
3) assert statements will be active for future me and some other folks
doing active git development
Do you two disagree with any of those operating assumptions? I find
asserts very valuable because:
* It's a _concise_ code comment that is readily understood. Any
attempt to word in English the same thing that an assert statement
checks always takes longer
* It helps future folks tweaking the rules to catch additional
locations where assumptions were made about the old rules. In the
development of merge-ort, for example, asserts shortened my debugging
cycles as I found and attempted new optimizations or added new
features or changed data structures and so on. The checks were _only_
assists while developing; once the code is right, the checks could be
removed. But future development might occur, so it'd be nice to have
a way to keep the checks in the code just for those future developers
while production users remove them.
In particular, for merge-ort, I think the second point is very
helpful. What can achieve the "remove these now-unnecessary checks
from the code for production, but keep them there for future
development"? I thought assert() was created exactly for this
purpose. Would you rather I created an affirm() that does essentially
the same thing and is compiled out unless DEVELOPER=1? That would
allow us to declare all assert() calls in the code as buggy, but I'm
not sure affirm() is as readily understood by developers reading the
code as "ooh, a reminder I get to assume these statements are true
while I'm reading the rest of the code".
I don't mind the asserts, or to have them in the default build.
But if you'd like to submit patches for asserts and can't otherwise get
them accepted, then can we please not make DEVELOPER a thing that you
can't turn on in production without thinking twice? Per my
https://lore.kernel.org/git/87wnusj6gt.fsf@evledraar.gmail.com/
quoted
quoted
quoted
+ if (opts->edit == -1)
Maybe `< 0`, as we do elsewhere for "not specified"?
Yup.
quoted
quoted
+ /*
+ * Note that we only handle the case of non-conflicted
+ * commits; continue_single_pick() handles the conflicted
+ * commits itself instead of calling this function.
+ */
+ return (opts->action == REPLAY_REVERT && isatty(0)) ? 1 : 0;
Apart from the extra parentheses, that makes sense to me.
I can take it either way (but personally I think this particular one
is easier to see as written---this is subjective).
quoted
...
The rest looks good, and the comments are _really_ helpful.
Yup, I agree.
Thanks for a review.
Indeed; and thanks to you as well Junio for all your time reviewing.
On Wed, Mar 31, 2021 at 10:36 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
On Tue, Mar 30 2021, Elijah Newren wrote:
quoted
In particular, for merge-ort, I think the second point is very
helpful. What can achieve the "remove these now-unnecessary checks
from the code for production, but keep them there for future
development"? I thought assert() was created exactly for this
purpose. Would you rather I created an affirm() that does essentially
the same thing and is compiled out unless DEVELOPER=1? That would
allow us to declare all assert() calls in the code as buggy, but I'm
not sure affirm() is as readily understood by developers reading the
code as "ooh, a reminder I get to assume these statements are true
while I'm reading the rest of the code".
I don't mind the asserts, or to have them in the default build.
But if you'd like to submit patches for asserts and can't otherwise get
them accepted, then can we please not make DEVELOPER a thing that you
can't turn on in production without thinking twice? Per my
https://lore.kernel.org/git/87wnusj6gt.fsf@evledraar.gmail.com/
Fair enough; if we have to go the affirm() route, I should probably
just make it depend on NDEBUG. :-)