Re: [PATCH 02/15] read-cache: Improve readability
Stefan Beller sbel...@google.com writes:
Maybe I have read too much of the refs code lately as there we
have these long chains which combine precondition with error
checking.
Of course, things are not so black-and-white in the real world.
You can also take an extreme stance and view
if (!pretend do_it_and_report_error())
error(...);
differently. I would explain that what the whole thing is trying to
achieve as 'do it' part is the primary thing we want to do, but it
only can be done when we are not pretending, and we show an error
when the 'do it' part fails. and would suggest structuring it more
like this:
if (pretend)
; /* nothing */
else if (do_it_and_report_error())
error(...);
or
if (!pretend) {
if (do_it_and_report_error())
error(...);
}
But you could say The final objective of the whole thing is to show
an error message, but if we are pretending or if our attempt to 'do
it' succeeds, we do not have to show the error, and such a view may
make sense depending on what that 'do it' is. The original may be
justified under such an interpretation.
We may be tempted to write (note: each boolean term may be a call
with many complex arguments)
if (A B C D E)
...
when it is in fact logically is more like this:
/* does not make sense to attempt C when A B does not hold */
if (A B) {
if (C D E)
...
}
But it becomes a judgement call if splitting that into nested two if
statements is better for overall readability when the top-level if
statement has an else clause. You cannot turn it into
/* does not make sense to attempt C when A B does not hold */
if (A B) {
if (C D E)
...
} else {
... /* this cannot be what the original's else clause did */
}
blindly. It would need further restructuring. I think the code
inside the refs.c is a result of making such judgement calls.
By the way, aren't we leaking ce when we are merely pretending?
Yes we are, that's how I found this spot. (coverity pointed out ce was
leaking, so I was refactoring to actually make it easier to fix it, and then
heavily reordered the patch series afterwards. That spot was forgotten
apparently.
I dropped 2/15 and expect the real fix in the future; no rush,
though.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/15] read-cache: Improve readability
Signed-off-by: Stefan Beller sbel...@google.com --- read-cache.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/read-cache.c b/read-cache.c index f72ea9f..769897e 100644 --- a/read-cache.c +++ b/read-cache.c @@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, !hashcmp(alias-sha1, ce-sha1) ce-ce_mode == alias-ce_mode); - if (pretend) - ; - else if (add_index_entry(istate, ce, add_option)) + if (!pretend add_index_entry(istate, ce, add_option)) return error(unable to add %s to index,path); if (verbose !was_same) printf(add '%s'\n, path); -- 2.3.0.81.gc37f363 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/15] read-cache: Improve readability
On Fri, Mar 20, 2015 at 9:19 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
Signed-off-by: Stefan Beller sbel...@google.com
---
read-cache.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index f72ea9f..769897e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char
*path, struct stat *st,
!hashcmp(alias-sha1, ce-sha1)
ce-ce_mode == alias-ce_mode);
- if (pretend)
- ;
- else if (add_index_entry(istate, ce, add_option))
+ if (!pretend add_index_entry(istate, ce, add_option))
return error(unable to add %s to index,path);
if (verbose !was_same)
printf(add '%s'\n, path);
I have a moderately strong feeling against this change, as the code
was done this way quite deliberately to keep it readable, namely, to
avoid using to concatenate two boolean expressions that are in
totally different class inside condition part of if/while, where A
is a precondition guard for B (i.e. you cannot evaluate B unless A
holds) and B is called primarily for its side effect. The problem
is that, once you start liberally doing
if (A B C D ...)
with booleans with mixed semantics (guards and actions), it will
quickly get harder to tell which one is which.
I could have written it as
if (!pretend) {
if (add_index_entry(...))
return error(...);
}
This makes sense to point out the different semantics to me.
Maybe I have read too much of the refs code lately as there we
have these long chains which combine precondition with error
checking. :/ That's why I thought it would be global to git to not
care much about this semantics distinction.
and that would have been just as readable as the original; it
clearly separates the guard (i.e. only do the add-index thing when
we are not pretending) and the operation that is done for the side
effect.
But I find the original tells you if pretend mode, do *nothing*
and otherwise, try add_index_entry() and act on its error very
clearly. Of course, I am biased as the original is my code from
38ed1d89 (git-add -n -u should not add but just report,
2008-05-21).
FYI, between preference and taste, I'd say this one is much closer
to the latter than the former.
By the way, aren't we leaking ce when we are merely pretending?
Yes we are, that's how I found this spot. (coverity pointed out ce was
leaking, so I was refactoring to actually make it easier to fix it, and then
heavily reordered the patch series afterwards. That spot was forgotten
apparently.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/15] read-cache: Improve readability
Stefan Beller sbel...@google.com writes:
Signed-off-by: Stefan Beller sbel...@google.com
---
read-cache.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index f72ea9f..769897e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -703,9 +703,7 @@ int add_to_index(struct index_state *istate, const char
*path, struct stat *st,
!hashcmp(alias-sha1, ce-sha1)
ce-ce_mode == alias-ce_mode);
- if (pretend)
- ;
- else if (add_index_entry(istate, ce, add_option))
+ if (!pretend add_index_entry(istate, ce, add_option))
return error(unable to add %s to index,path);
if (verbose !was_same)
printf(add '%s'\n, path);
I have a moderately strong feeling against this change, as the code
was done this way quite deliberately to keep it readable, namely, to
avoid using to concatenate two boolean expressions that are in
totally different class inside condition part of if/while, where A
is a precondition guard for B (i.e. you cannot evaluate B unless A
holds) and B is called primarily for its side effect. The problem
is that, once you start liberally doing
if (A B C D ...)
with booleans with mixed semantics (guards and actions), it will
quickly get harder to tell which one is which.
I could have written it as
if (!pretend) {
if (add_index_entry(...))
return error(...);
}
and that would have been just as readable as the original; it
clearly separates the guard (i.e. only do the add-index thing when
we are not pretending) and the operation that is done for the side
effect.
But I find the original tells you if pretend mode, do *nothing*
and otherwise, try add_index_entry() and act on its error very
clearly. Of course, I am biased as the original is my code from
38ed1d89 (git-add -n -u should not add but just report,
2008-05-21).
FYI, between preference and taste, I'd say this one is much closer
to the latter than the former.
By the way, aren't we leaking ce when we are merely pretending?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
