Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream
On Thu, May 21, 2015 at 08:46:43PM -0400, Jeff King wrote:
> On Thu, May 21, 2015 at 12:25:57PM -0700, Junio C Hamano wrote:
>
> > > Note also that the original may dereference branch->merge[0] even if it
> > > is NULL. I think that can't actually happen in practice (we only
> > > allocate branch->merge if we have at least one item to put in it, and
> > > all of the checks of branch->merge[0] are actually being over-careful).
> > > But the code I just wrote above does not have that problem.
> >
> > Perhaps update the patch with this explanation in the log message,
> > as a separate preparatory step?
>
> I decided on a separate patch on top which improves the logic and
> explains the issues. Here it is (it goes on top of the existing patch 8,
> "report specific errors from branch_get_upstream").
>
> -- >8 --
> Subject: remote.c: untangle error logic in branch_get_upstream
And then on top of that, we can add in this cleanup I showed earlier.
Both of these should insert into the series without any trouble, but let
me know if you run into problems and I can repost the whole thing.
-- >8 --
Subject: remote.c: return upstream name from stat_tracking_info
After calling stat_tracking_info, callers often want to
print the name of the upstream branch (in addition to the
tracking count). To do this, they have to access
branch->merge->dst[0] themselves. This is not wrong, as the
return value from stat_tracking_info tells us whether we
have an upstream branch or not. But it is a bit leaky, as we
make an assumption about how it calculated the upstream
name.
Instead, let's add an out-parameter that lets the caller
know the upstream name we found.
As a bonus, we can get rid of the unusual tri-state return
from the function. We no longer need to use it to
differentiate between "no tracking config" and "tracking ref
does not exist" (since you can check the upstream_name for
that), so we can just use the usual 0/-1 convention for
success/error.
Signed-off-by: Jeff King
---
builtin/branch.c | 16 +---
builtin/for-each-ref.c | 4 ++--
remote.c | 35 +--
remote.h | 3 ++-
wt-status.c| 18 ++
5 files changed, 32 insertions(+), 44 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index cc55ff2..8ecabd1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -425,25 +425,19 @@ static void fill_tracking_info(struct strbuf *stat, const
char *branch_name,
int ours, theirs;
char *ref = NULL;
struct branch *branch = branch_get(branch_name);
+ const char *upstream;
struct strbuf fancy = STRBUF_INIT;
int upstream_is_gone = 0;
int added_decoration = 1;
- switch (stat_tracking_info(branch, &ours, &theirs)) {
- case 0:
- /* no base */
- return;
- case -1:
- /* with "gone" base */
+ if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
+ if (!upstream)
+ return;
upstream_is_gone = 1;
- break;
- default:
- /* with base */
- break;
}
if (show_upstream_ref) {
- ref = shorten_unambiguous_ref(branch->merge[0]->dst, 0);
+ ref = shorten_unambiguous_ref(upstream, 0);
if (want_color(branch_use_color))
strbuf_addf(&fancy, "%s%s%s",
branch_get_color(BRANCH_COLOR_UPSTREAM),
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 18d209b..92bd2b2 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -716,7 +716,7 @@ static void populate_value(struct refinfo *ref)
char buf[40];
if (stat_tracking_info(branch, &num_ours,
- &num_theirs) != 1)
+ &num_theirs, NULL))
continue;
if (!num_ours && !num_theirs)
@@ -738,7 +738,7 @@ static void populate_value(struct refinfo *ref)
assert(branch);
if (stat_tracking_info(branch, &num_ours,
- &num_theirs) != 1)
+ &num_theirs, NULL))
continue;
if (!num_ours && !num_theirs)
diff --git a/remote.c b/remote.c
index d2519c2..c884574 100644
--- a/remote.c
+++ b/remote.c
@@ -1938,12 +1938,15 @@ int ref_newer(const unsigned char *new_sha1, const
unsigned char *old_sha1)
/*
* Compare a branch with its upstream, and save their differences (number
- * of commits) in *num_ours and *num_theirs.
+ * of commits) in *num_ours and *num_the
Re: [PATCH v3 08/14] remote.c: report specific errors from branch_get_upstream
On Thu, May 21, 2015 at 12:25:57PM -0700, Junio C Hamano wrote:
> > Note also that the original may dereference branch->merge[0] even if it
> > is NULL. I think that can't actually happen in practice (we only
> > allocate branch->merge if we have at least one item to put in it, and
> > all of the checks of branch->merge[0] are actually being over-careful).
> > But the code I just wrote above does not have that problem.
>
> Perhaps update the patch with this explanation in the log message,
> as a separate preparatory step?
I decided on a separate patch on top which improves the logic and
explains the issues. Here it is (it goes on top of the existing patch 8,
"report specific errors from branch_get_upstream").
-- >8 --
Subject: remote.c: untangle error logic in branch_get_upstream
The error-diagnosis logic in branch_get_upstream was copied
straight from sha1_name.c in the previous commit. However,
because we check all error cases and upfront and then later
diagnose them, the logic is a bit tangled. In particular:
- if branch->merge[0] is NULL, we may end up dereferencing
it for an error message (in practice, it should never be
NULL, so this is probably not a triggerable bug).
- We may enter the code path because branch->merge[0]->dst
is NULL, but we then start our error diagnosis by
checking whether our local branch exists. But that is
only relevant to diagnosing missing merge config, not a
missing tracking ref; our diagnosis may hide the real
problem.
Instead, let's just use a sequence of "if" blocks to check
for each error type, diagnose it, and return NULL.
Signed-off-by: Jeff King
---
remote.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/remote.c b/remote.c
index 1b7051a..d2519c2 100644
--- a/remote.c
+++ b/remote.c
@@ -1721,18 +1721,25 @@ const char *branch_get_upstream(struct branch *branch,
struct strbuf *err)
{
if (!branch)
return error_buf(err, _("HEAD does not point to a branch"));
- if (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {
+
+ if (!branch->merge || !branch->merge[0]) {
+ /*
+* no merge config; is it because the user didn't define any,
+* or because it is not a real branch, and get_branch
+* auto-vivified it?
+*/
if (!ref_exists(branch->refname))
return error_buf(err, _("no such branch: '%s'"),
branch->name);
- if (!branch->merge)
- return error_buf(err,
-_("no upstream configured for branch
'%s'"),
-branch->name);
+ return error_buf(err,
+_("no upstream configured for branch '%s'"),
+branch->name);
+ }
+
+ if (!branch->merge[0]->dst)
return error_buf(err,
_("upstream branch '%s' not stored as a
remote-tracking branch"),
branch->merge[0]->src);
- }
return branch->merge[0]->dst;
}
--
2.4.1.528.g00591e3
--
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 v3 08/14] remote.c: report specific errors from branch_get_upstream
Jeff King writes:
> On Thu, May 21, 2015 at 11:33:58AM -0700, Junio C Hamano wrote:
>
>> > +static const char *error_buf(struct strbuf *err, const char *fmt, ...)
>> > {
>> > - if (!branch || !branch->merge || !branch->merge[0])
>> > - return NULL;
>> > + if (err) {
>> > + va_list ap;
>> > + va_start(ap, fmt);
>> > + strbuf_vaddf(err, fmt, ap);
>> > + va_end(ap);
>> > + }
>> > + return NULL;
>> > +}
>>
>> Many of our functions return -1 to signal an error, and that is why
>> it makes sense for our error() helper to return -1 to save code in
>> the caller, but only because the callers of this private helper use
>> a NULL to signal an error, this also returns NULL. If we were to
>> use the "callers can opt into detailed message by passing strbuf"
>> pattern more widely, we would want a variant of the above that
>> returns -1, too. And such a helper would do the same thing as
>> above, with only difference from the above is to return -1.
>
> Yeah, this is not really a good general-purpose purpose function in that
> sense. I have often wanted an error() that returns NULL, but avoided
> adding just because it seemed like needless proliferation.
>
> The real reason to include the return value at all in error() is to let
> you turn two-liners into one-liners.
Yeah, our error() returns -1 to save code in the caller. And the
callers of this private helper all want to return NULL, so this
returns NULL for the same reason.
> We could drop the return value from
> this helper entirely (or make it -1, but of course no callers would use
> it) and write it out long-hand in the callers:
>
> if (!branch->merge) {
> error_buf(err, ...);
> return NULL;
> }
>
> That is really not so bad, as there are only a handful of callers.
That may happen when somebody (perhaps Jonathan?) wants to push the
"let's optionally pass strbuf to format messages into" approach
forward, and most likely be done by adding a similar function to
usage.c next to error_builtin() and friends. As long as we do not
forget to reimplement this helper in terms of that public function
when it happens, what we have right now after this patch would be
fine.
> Yeah, my goal here was to faithfully keep the same logic, but I had a
> similar head-scratching moment. What would make more sense to me is:
>
> if (!branch)
> return error("HEAD does not point to a branch");
>
> if (!branch->merge || !branch->merge[0]) {
> /*
>* no merge config; is it because the user didn't define any, or
>* because it is not a real branch, and get_branch auto-vivified
>* it?
>*/
> if (!ref_exists(branch->refname))
> return error("no such branch");
> return error("no upstream configured for branch");
> }
>
> if (!branch->merge[0]->dst)
> return error("upstream branch not stored as a remote-tracking branch");
>
> return branch->merge[0]->dst;
>
> Hopefully the comment there answers your question; it is not that we
> truly care whether the ref exists, but only that we are trying to tell
> the difference between a "real" branch and a struct that is an artifact
> of our internal code.
Well, answering "my" question here on the list wouldn't help future
readers of the code very much ;-)
> Note also that the original may dereference branch->merge[0] even if it
> is NULL. I think that can't actually happen in practice (we only
> allocate branch->merge if we have at least one item to put in it, and
> all of the checks of branch->merge[0] are actually being over-careful).
> But the code I just wrote above does not have that problem.
Perhaps update the patch with this explanation in the log message,
as a separate preparatory step?
Thanks.
--
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 v3 08/14] remote.c: report specific errors from branch_get_upstream
On Thu, May 21, 2015 at 11:33:58AM -0700, Junio C Hamano wrote:
> > +static const char *error_buf(struct strbuf *err, const char *fmt, ...)
> > {
> > - if (!branch || !branch->merge || !branch->merge[0])
> > - return NULL;
> > + if (err) {
> > + va_list ap;
> > + va_start(ap, fmt);
> > + strbuf_vaddf(err, fmt, ap);
> > + va_end(ap);
> > + }
> > + return NULL;
> > +}
>
> Many of our functions return -1 to signal an error, and that is why
> it makes sense for our error() helper to return -1 to save code in
> the caller, but only because the callers of this private helper use
> a NULL to signal an error, this also returns NULL. If we were to
> use the "callers can opt into detailed message by passing strbuf"
> pattern more widely, we would want a variant of the above that
> returns -1, too. And such a helper would do the same thing as
> above, with only difference from the above is to return -1.
Yeah, this is not really a good general-purpose purpose function in that
sense. I have often wanted an error() that returns NULL, but avoided
adding just because it seemed like needless proliferation.
The real reason to include the return value at all in error() is to let
you turn two-liners into one-liners. We could drop the return value from
this helper entirely (or make it -1, but of course no callers would use
it) and write it out long-hand in the callers:
if (!branch->merge) {
error_buf(err, ...);
return NULL;
}
That is really not so bad, as there are only a handful of callers.
> > +const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
> > +{
> > + if (!branch)
> > + return error_buf(err, _("HEAD does not point to a branch"));
> > + if (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {
> > + if (!ref_exists(branch->refname))
> > + return error_buf(err, _("no such branch: '%s'"),
> > +branch->name);
> > + if (!branch->merge)
> > + return error_buf(err,
> > +_("no upstream configured for branch
> > '%s'"),
> > +branch->name);
> > + return error_buf(err,
> > +_("upstream branch '%s' not stored as a
> > remote-tracking branch"),
> > +branch->merge[0]->src);
> > + }
> > +
> > return branch->merge[0]->dst;
> > }
>
> This is a faithful conversion of what the get_upstream_branch() used
> to do, but that ref_exists() check and the error checking there look
> somewhat out of place.
>
> It makes the reader wonder what should happen when "branch->refname"
> does not exist as a ref, but "branch->merge[0]->dst" can be fully
> dereferenced. Should it be an error, or if it is OK, the reason why
> it is OK is...?
Yeah, my goal here was to faithfully keep the same logic, but I had a
similar head-scratching moment. What would make more sense to me is:
if (!branch)
return error("HEAD does not point to a branch");
if (!branch->merge || !branch->merge[0]) {
/*
* no merge config; is it because the user didn't define any, or
* because it is not a real branch, and get_branch auto-vivified
* it?
*/
if (!ref_exists(branch->refname))
return error("no such branch");
return error("no upstream configured for branch");
}
if (!branch->merge[0]->dst)
return error("upstream branch not stored as a remote-tracking branch");
return branch->merge[0]->dst;
Hopefully the comment there answers your question; it is not that we
truly care whether the ref exists, but only that we are trying to tell
the difference between a "real" branch and a struct that is an artifact
of our internal code.
Note also that the original may dereference branch->merge[0] even if it
is NULL. I think that can't actually happen in practice (we only
allocate branch->merge if we have at least one item to put in it, and
all of the checks of branch->merge[0] are actually being over-careful).
But the code I just wrote above does not have that problem.
-Peff
--
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 v3 08/14] remote.c: report specific errors from branch_get_upstream
Jeff King writes:
> diff --git a/remote.c b/remote.c
> index dca3442..1b7051a 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1705,10 +1705,35 @@ int branch_merge_matches(struct branch *branch,
> return refname_match(branch->merge[i]->src, refname);
> }
>
> -const char *branch_get_upstream(struct branch *branch)
> +__attribute((format (printf,2,3)))
> +static const char *error_buf(struct strbuf *err, const char *fmt, ...)
> {
> - if (!branch || !branch->merge || !branch->merge[0])
> - return NULL;
> + if (err) {
> + va_list ap;
> + va_start(ap, fmt);
> + strbuf_vaddf(err, fmt, ap);
> + va_end(ap);
> + }
> + return NULL;
> +}
Many of our functions return -1 to signal an error, and that is why
it makes sense for our error() helper to return -1 to save code in
the caller, but only because the callers of this private helper use
a NULL to signal an error, this also returns NULL. If we were to
use the "callers can opt into detailed message by passing strbuf"
pattern more widely, we would want a variant of the above that
returns -1, too. And such a helper would do the same thing as
above, with only difference from the above is to return -1.
It's a shame that we have to return something from this function,
whose primary purpose is "we may or may not want an error message in
a strbuf, so format the message when and only when we give you a
strbuf", but C forces us to make it "always return NULL to signal an
error to the caller, and optionally format the message into a strbuf
if given".
And the name of this helper function only captures the "optionally
format the message" part, not the "always return NULL" part.
> +const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
> +{
> + if (!branch)
> + return error_buf(err, _("HEAD does not point to a branch"));
> + if (!branch->merge || !branch->merge[0] || !branch->merge[0]->dst) {
> + if (!ref_exists(branch->refname))
> + return error_buf(err, _("no such branch: '%s'"),
> + branch->name);
> + if (!branch->merge)
> + return error_buf(err,
> + _("no upstream configured for branch
> '%s'"),
> + branch->name);
> + return error_buf(err,
> + _("upstream branch '%s' not stored as a
> remote-tracking branch"),
> + branch->merge[0]->src);
> + }
> +
> return branch->merge[0]->dst;
> }
This is a faithful conversion of what the get_upstream_branch() used
to do, but that ref_exists() check and the error checking there look
somewhat out of place.
It makes the reader wonder what should happen when "branch->refname"
does not exist as a ref, but "branch->merge[0]->dst" can be fully
dereferenced. Should it be an error, or if it is OK, the reason why
it is OK is...?
--
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
