Re: [PATCH 10/33] refs: extract a function ref_resolves_to_object()
On 04/15/2013 06:51 PM, Junio C Hamano wrote:
Michael Haggerty mhag...@alum.mit.edu writes:
It is a nice unit of work and soon will be needed from multiple
locations.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index c523978..dfc8600 100644
--- a/refs.c
+++ b/refs.c
@@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)
#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
/*
+ * Return true iff the reference described by entry can be resolved to
+ * an object in the database. Emit a warning if the referred-to
+ * object does not exist.
+ */
+static int ref_resolves_to_object(struct ref_entry *entry)
+{
+if (entry-flag REF_ISBROKEN)
+return 0;
+if (!has_sha1_file(entry-u.value.sha1)) {
+error(%s does not point to a valid object!, entry-name);
+return 0;
+}
+return 1;
+}
+
+/*
* current_ref is a performance hack: when iterating over references
* using the for_each_ref*() functions, current_ref is set to the
* current reference's entry before calling the callback function. If
@@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn
fn, int trim,
if (prefixcmp(entry-name, base))
return 0;
-if (!(flags DO_FOR_EACH_INCLUDE_BROKEN)) {
-if (entry-flag REF_ISBROKEN)
-return 0; /* ignore broken refs e.g. dangling symref */
-if (!has_sha1_file(entry-u.value.sha1)) {
-error(%s does not point to a valid object!,
entry-name);
-return 0;
-}
-}
+if (!((flags DO_FOR_EACH_INCLUDE_BROKEN) ||
+ ref_resolves_to_object(entry)))
+return 0;
+
The original says Unless flags tell us to include broken ones,
return 0 for the broken ones and the ones that point at invalid
objects.
The updated says Unless flags tell us to include broken ones or the
ref is a good one, return 0.
Are they the same? If we have a good one, and if we are not told to
include broken one, the original just passes the control down to the
remainder of the function. The updated one will return 0.
Oh, wait, that is not the case. The OR is inside !( A || B ), so
what it really wants to say is:
if (!(flags DO_FOR_EACH_INCLUDE_BROKEN)
!ref_resolves_to_object(entry))
return 0;
that is, When we are told to exclude broken ones and the one we are
looking at is broken, return 0.
Am I the only one who was confused with this, or was the way the
patch wrote this logic unnecessarily convoluted?
I find either way of writing it hard to read quickly. I find that the
construct
if (!(situation in which the function should continue))
return
makes it easier to pick out the situation in which the function should
continue. But granted, the nesting of multiple parentheses across
lines compromises the clarity.
In projects where I can choose the coding standard, I like to use extra
whitespace to help the eye pick out the range of parentheses, like
if (!(
(flags DO_FOR_EACH_INCLUDE_BROKEN) ||
ref_resolves_to_object(entry)
))
return 0;
but I've never seen this style in the Git project so I haven't used it here.
Since you prefer the other version, I will change it.
Michael
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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 10/33] refs: extract a function ref_resolves_to_object()
Michael Haggerty mhag...@alum.mit.edu writes: In projects where I can choose the coding standard, I like to use extra whitespace to help the eye pick out the range of parentheses, like if (!( (flags DO_FOR_EACH_INCLUDE_BROKEN) || ref_resolves_to_object(entry) )) return 0; but I've never seen this style in the Git project so I haven't used it here. I _think_ at least to me, it is not the textual grouping style but the unless X we skip the rest logic itself that confused me. It took the same number of minutes to grok the above as the original. We skip the rest of this function unless the condition inside !() holds, and the condition is we are told to include broken ones, in which case we know we will do the remainder without checking anything else, or we do the checking and find that it is not broken. I suspect the root cause of the confusion to force the double negation is INCLUDE_BROKEN; we have to express when we are told to ignore broken one and this thing is broken, ignore it without negation, i.e. if ( !(flags INCLUDE_BROKEN) is_broken(thing) ) return; which would have been much more pleasant to read if it were if ( (flags IGNORE_BROKEN) is_broken(thing) ) return; Unfortunately, we cannot change it to IGNORE_BROKEN and flip the meaning of the bit, because almost all callers do want to ignore broken ones. Either way is fine by me, even though I find that !(A || B) needs a bit more mental exercise to grok than (!A !B). Perhaps it is just me who is not so strong at math ;-) -- 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 10/33] refs: extract a function ref_resolves_to_object()
Michael Haggerty mhag...@alum.mit.edu writes:
It is a nice unit of work and soon will be needed from multiple
locations.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index c523978..dfc8600 100644
--- a/refs.c
+++ b/refs.c
@@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)
#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
/*
+ * Return true iff the reference described by entry can be resolved to
+ * an object in the database. Emit a warning if the referred-to
+ * object does not exist.
+ */
+static int ref_resolves_to_object(struct ref_entry *entry)
+{
+ if (entry-flag REF_ISBROKEN)
+ return 0;
+ if (!has_sha1_file(entry-u.value.sha1)) {
+ error(%s does not point to a valid object!, entry-name);
+ return 0;
+ }
+ return 1;
+}
+
+/*
* current_ref is a performance hack: when iterating over references
* using the for_each_ref*() functions, current_ref is set to the
* current reference's entry before calling the callback function. If
@@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn fn,
int trim,
if (prefixcmp(entry-name, base))
return 0;
- if (!(flags DO_FOR_EACH_INCLUDE_BROKEN)) {
- if (entry-flag REF_ISBROKEN)
- return 0; /* ignore broken refs e.g. dangling symref */
- if (!has_sha1_file(entry-u.value.sha1)) {
- error(%s does not point to a valid object!,
entry-name);
- return 0;
- }
- }
+ if (!((flags DO_FOR_EACH_INCLUDE_BROKEN) ||
+ ref_resolves_to_object(entry)))
+ return 0;
+
The original says Unless flags tell us to include broken ones,
return 0 for the broken ones and the ones that point at invalid
objects.
The updated says Unless flags tell us to include broken ones or the
ref is a good one, return 0.
Are they the same? If we have a good one, and if we are not told to
include broken one, the original just passes the control down to the
remainder of the function. The updated one will return 0.
Oh, wait, that is not the case. The OR is inside !( A || B ), so
what it really wants to say is:
if (!(flags DO_FOR_EACH_INCLUDE_BROKEN)
!ref_resolves_to_object(entry))
return 0;
that is, When we are told to exclude broken ones and the one we are
looking at is broken, return 0.
Am I the only one who was confused with this, or was the way the
patch wrote this logic unnecessarily convoluted?
current_ref = entry;
retval = fn(entry-name + trim, entry-u.value.sha1, entry-flag,
cb_data);
current_ref = NULL;
--
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 10/33] refs: extract a function ref_resolves_to_object()
It is a nice unit of work and soon will be needed from multiple
locations.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
refs.c | 28
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index c523978..dfc8600 100644
--- a/refs.c
+++ b/refs.c
@@ -529,6 +529,22 @@ static void sort_ref_dir(struct ref_dir *dir)
#define DO_FOR_EACH_INCLUDE_BROKEN 0x01
/*
+ * Return true iff the reference described by entry can be resolved to
+ * an object in the database. Emit a warning if the referred-to
+ * object does not exist.
+ */
+static int ref_resolves_to_object(struct ref_entry *entry)
+{
+ if (entry-flag REF_ISBROKEN)
+ return 0;
+ if (!has_sha1_file(entry-u.value.sha1)) {
+ error(%s does not point to a valid object!, entry-name);
+ return 0;
+ }
+ return 1;
+}
+
+/*
* current_ref is a performance hack: when iterating over references
* using the for_each_ref*() functions, current_ref is set to the
* current reference's entry before calling the callback function. If
@@ -549,14 +565,10 @@ static int do_one_ref(const char *base, each_ref_fn fn,
int trim,
if (prefixcmp(entry-name, base))
return 0;
- if (!(flags DO_FOR_EACH_INCLUDE_BROKEN)) {
- if (entry-flag REF_ISBROKEN)
- return 0; /* ignore broken refs e.g. dangling symref */
- if (!has_sha1_file(entry-u.value.sha1)) {
- error(%s does not point to a valid object!,
entry-name);
- return 0;
- }
- }
+ if (!((flags DO_FOR_EACH_INCLUDE_BROKEN) ||
+ ref_resolves_to_object(entry)))
+ return 0;
+
current_ref = entry;
retval = fn(entry-name + trim, entry-u.value.sha1, entry-flag,
cb_data);
current_ref = NULL;
--
1.8.2.1
--
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
