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