[PATCH 7/4] submodule: clarify logic in show_submodule_summary

[Jeff King]

There are two uses of the left and right commit
variables that make it hard to be sure what values they
have (both for the reader, and for gcc, which wrongly
complains that they might be used uninitialized).

The functions starts with a cascading if statement, checking
that the input sha1s exist, and finally working up to
preparing a revision walk. We only prepare the walk if the
cascading conditional did not find any problems, which we
check by seeing whether it set the message variable or
not. It's simpler and more obvious to just add a condition
to the end of the cascade.

Later, we check the same message variable when deciding
whether to clear commit marks on the left/right commits; if
it is set, we presumably never started the walk. This is
wrong, though; we might have started the walk and munged
commit flags, only to encounter an error afterwards. We
should always clear the flags on left/right if they exist,
whether the walk was successful or not.

Signed-off-by: Jeff King p...@peff.net
---
 submodule.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/submodule.c b/submodule.c
index 9ba1496..975bc87 100644
--- a/submodule.c
+++ b/submodule.c
@@ -261,7 +261,7 @@ void show_submodule_summary(FILE *f, const char *path,
const char *del, const char *add, const char *reset)
 {
struct rev_info rev;
-   struct commit *left = left, *right = right;
+   struct commit *left = NULL, *right = NULL;
const char *message = NULL;
struct strbuf sb = STRBUF_INIT;
int fast_forward = 0, fast_backward = 0;
@@ -275,10 +275,8 @@ void show_submodule_summary(FILE *f, const char *path,
else if (!(left = lookup_commit_reference(one)) ||
 !(right = lookup_commit_reference(two)))
message = (commits not present);
-
-   if (!message 
-   prepare_submodule_summary(rev, path, left, right,
-   fast_forward, fast_backward))
+   else if (prepare_submodule_summary(rev, path, left, right,
+  fast_forward, fast_backward))
message = (revision walker failed);
 
if (dirty_submodule  DIRTY_SUBMODULE_UNTRACKED)
@@ -302,11 +300,12 @@ void show_submodule_summary(FILE *f, const char *path,
strbuf_addf(sb, %s:%s\n, fast_backward ?  (rewind) : , 
reset);
fwrite(sb.buf, sb.len, 1, f);
 
-   if (!message) {
+   if (!message) /* only NULL if we succeeded in setting up the walk */
print_submodule_summary(rev, f, del, add, reset);
+   if (left)
clear_commit_marks(left, ~0);
+   if (right)
clear_commit_marks(right, ~0);
-   }
 
strbuf_release(sb);
 }
-- 
1.8.2.13.g0f18d3c

--
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 7/4] submodule: clarify logic in show_submodule_summary

[Junio C Hamano]

Jeff King p...@peff.net writes:

 There are two uses of the left and right commit
 variables that make it hard to be sure what values they
 have (both for the reader, and for gcc, which wrongly
 complains that they might be used uninitialized).

 The functions starts with a cascading if statement, checking
 that the input sha1s exist, and finally working up to
 preparing a revision walk. We only prepare the walk if the
 cascading conditional did not find any problems, which we
 check by seeing whether it set the message variable or
 not. It's simpler and more obvious to just add a condition
 to the end of the cascade.

 Later, we check the same message variable when deciding
 whether to clear commit marks on the left/right commits; if
 it is set, we presumably never started the walk. This is
 wrong, though; we might have started the walk and munged
 commit flags, only to encounter an error afterwards. We
 should always clear the flags on left/right if they exist,
 whether the walk was successful or not.

 Signed-off-by: Jeff King p...@peff.net
 ---

Looks good.  Thanks.

  submodule.c | 13 ++---
  1 file changed, 6 insertions(+), 7 deletions(-)

 diff --git a/submodule.c b/submodule.c
 index 9ba1496..975bc87 100644
 --- a/submodule.c
 +++ b/submodule.c
 @@ -261,7 +261,7 @@ void show_submodule_summary(FILE *f, const char *path,
   const char *del, const char *add, const char *reset)
  {
   struct rev_info rev;
 - struct commit *left = left, *right = right;
 + struct commit *left = NULL, *right = NULL;
   const char *message = NULL;
   struct strbuf sb = STRBUF_INIT;
   int fast_forward = 0, fast_backward = 0;
 @@ -275,10 +275,8 @@ void show_submodule_summary(FILE *f, const char *path,
   else if (!(left = lookup_commit_reference(one)) ||
!(right = lookup_commit_reference(two)))
   message = (commits not present);
 -
 - if (!message 
 - prepare_submodule_summary(rev, path, left, right,
 - fast_forward, fast_backward))
 + else if (prepare_submodule_summary(rev, path, left, right,
 +fast_forward, fast_backward))
   message = (revision walker failed);
  
   if (dirty_submodule  DIRTY_SUBMODULE_UNTRACKED)
 @@ -302,11 +300,12 @@ void show_submodule_summary(FILE *f, const char *path,
   strbuf_addf(sb, %s:%s\n, fast_backward ?  (rewind) : , 
 reset);
   fwrite(sb.buf, sb.len, 1, f);
  
 - if (!message) {
 + if (!message) /* only NULL if we succeeded in setting up the walk */
   print_submodule_summary(rev, f, del, add, reset);
 + if (left)
   clear_commit_marks(left, ~0);
 + if (right)
   clear_commit_marks(right, ~0);
 - }
  
   strbuf_release(sb);
  }
--
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