Re: [PATCH v4 5/8] branch: drop non-commit error reporting

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH v4 5/8] branch: drop non-commit error reporting

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:06:32

Karthik Nayak [off-list ref] writes:
Remove the error reporting variable to make the code easier to port
over to using ref-filter APIs. This variable is not required as in
ref-filter we already check for possible errors and report them.
Hmmmm.  What do you exactly mean "possible errors" here?

Unlike generic refs that can point at anything, refs/heads/* refs
must point at a commit [*1*], and that is why the error message says
'does not point at a commit'.

Does ref-filter API have corresponding check to treat the local and
remote branch hierarchies differently from tags and others?

[Footnote]

*1* Strictly speaking, use of lookup_commit_reference_gently() in
    the existing code allows a committish to be there and does not
    limit the tip objects to be commits, but I think it is a bug.
    At least, even with deref_tag(), we reject non committish found
    at the tip of branch refs and explain with the error message
    this patch removes.
quoted hunk
Based-on-patch-by: Jeff King [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Matthieu Moy [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---
 builtin/branch.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 4d9e4d0..8b9da60 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -313,7 +313,6 @@ static char *resolve_symref(const char *src, const char *prefix)
 struct append_ref_cb {
 	struct ref_list *ref_list;
 	const char **pattern;
-	int ret;
 };
 
 static int match_patterns(const char **pattern, const char *refname)
@@ -370,10 +369,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag
 	commit = NULL;
 	if (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {
 		commit = lookup_commit_reference_gently(oid->hash, 1);
-		if (!commit) {
-			cb->ret = error(_("branch '%s' does not point at a commit"), refname);
+		if (!commit)
 			return 0;
-		}
 
 		/* Filter with with_commit if specified */
 		if (!is_descendant_of(commit, ref_list->with_commit))
@@ -617,7 +614,7 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
 	return max;
 }
 
-static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
+static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)
 {
 	int i, index;
 	struct append_ref_cb cb;
@@ -642,7 +639,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 		init_revisions(&ref_list.revs, NULL);
 	cb.ref_list = &ref_list;
 	cb.pattern = pattern;
-	cb.ret = 0;
 	/*
 	 * First we obtain all regular branch refs and if the HEAD is
 	 * detached then we insert that ref to the end of the ref_fist
@@ -702,11 +698,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 			       abbrev, detached, remote_prefix);
 
 	free_ref_list(&ref_list);
-
-	if (cb.ret)
-		error(_("some refs could not be read"));
-
-	return cb.ret;
 }
 
 static void rename_branch(const char *oldname, const char *newname, int force)
@@ -922,15 +913,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			die(_("branch name required"));
 		return delete_branches(argc, argv, delete > 1, kinds, quiet);
 	} else if (list) {
-		int ret;
 		/*  git branch --local also shows HEAD when it is detached */
 		if (kinds & REF_LOCAL_BRANCH)
 			kinds |= REF_DETACHED_HEAD;
-		ret = print_ref_list(kinds, detached, verbose, abbrev,
+		print_ref_list(kinds, detached, verbose, abbrev,
 					 with_commit, argv);
 		print_columns(&output, colopts, NULL);
 		string_list_clear(&output, 0);
-		return ret;
+		return 0;
 	}
 	else if (edit_description) {
 		const char *branch_name;

Re: [PATCH v4 5/8] branch: drop non-commit error reporting

From: Karthik Nayak <hidden>
Date: 2016-06-15 23:06:34

On Tue, Sep 15, 2015 at 1:19 AM, Junio C Hamano [off-list ref] wrote:
Karthik Nayak [off-list ref] writes:
quoted
Remove the error reporting variable to make the code easier to port
over to using ref-filter APIs. This variable is not required as in
ref-filter we already check for possible errors and report them.
Hmmmm.  What do you exactly mean "possible errors" here?
The checking if it points to a commit.
Unlike generic refs that can point at anything, refs/heads/* refs
must point at a commit [*1*], and that is why the error message says
'does not point at a commit'.

Does ref-filter API have corresponding check to treat the local and
remote branch hierarchies differently from tags and others?
This check in the existing code is only done when one of these holds:
ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER

There is a similar check in ref-filter when we filter and obtain refs.

    if (filter->merge_commit || filter->with_commit || filter->verbose) {
        commit = lookup_commit_reference_gently(oid->hash, 1);
        if (!commit)
            return 0;
        /* We perform the filtering for the '--contains' option */
        if (filter->with_commit &&
            !commit_contains(filter, commit))
            return 0;
    }
[Footnote]

*1* Strictly speaking, use of lookup_commit_reference_gently() in
    the existing code allows a committish to be there and does not
    limit the tip objects to be commits, but I think it is a bug.
    At least, even with deref_tag(), we reject non committish found
    at the tip of branch refs and explain with the error message
    this patch removes.
Ah, didn't go through this.

-- 
Regards,
Karthik Nayak
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help