Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)

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

Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)

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

Jonathan Nieder [off-list ref] writes:

Junio C Hamano wrote:

quoted

Jonathan Nieder [off-list ref] writes:

quoted

These patches are also available from the git repository at

  git://repo.or.cz/git/jrn.git tags/rs/ref-transaction

The tag fetched and built as-is seems to break 5514 among other
things ("git remote rm" segfaults).

Yeah, I noticed that right after sending the series out. :/

The patch below fixes it[1].

Is this meant to replace anything, or is it "Oops, the previous ones
are broken, and this is to patch it up on top"?

quoted hunk

-- >8 --
From: Ronnie Sahlberg <redacted>
Date: Thu, 11 Sep 2014 08:42:57 -0700
Subject: remote rm/prune: print a message when writing packed-refs fails

Until v2.1.0-rc0~22^2~11 (refs.c: add an err argument to
repack_without_refs, 2014-06-20), repack_without_refs forgot to
provide an error message when commit_packed_refs fails.  Even today,
it only provides a message for callers that pass a non-NULL err
parameter.  Internal callers in refs.c pass non-NULL err but
"git remote" does not.

That means that "git remote rm" and "git remote prune" can fail
without printing a message about why.  Fix them by passing in a
non-NULL err parameter and printing the returned message.

This is the last caller to a ref handling function passing err ==
NULL.  A later patch can drop support for err == NULL, avoiding such
problems in the future.

Change-Id: Ifb8a726ef03d0aa282a25a102313064d2e8ec283
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
[1] https://code-review.googlesource.com/1110
    https://code-review.googlesource.com/1060

 builtin/remote.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 6eaeee7..ef1ffc3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -750,13 +750,16 @@ static int mv(int argc, const char **argv)
 
 static int remove_branches(struct string_list *branches)
 {
+	struct strbuf err = STRBUF_INIT;
 	const char **branch_names;
 	int i, result = 0;
 
 	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
 	for (i = 0; i < branches->nr; i++)
 		branch_names[i] = branches->items[i].string;
-	result |= repack_without_refs(branch_names, branches->nr, NULL);
+	if (repack_without_refs(branch_names, branches->nr, &err))
+		result |= error("%s", err.buf);
+	strbuf_release(&err);
 	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {

@@ -1333,9 +1336,13 @@ static int prune_remote(const char *remote, int dry_run)
 		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
 		for (i = 0; i < states.stale.nr; i++)
 			delete_refs[i] = states.stale.items[i].util;
-		if (!dry_run)
-			result |= repack_without_refs(delete_refs,
-						      states.stale.nr, NULL);
+		if (!dry_run) {
+			struct strbuf err = STRBUF_INIT;
+			if (repack_without_refs(delete_refs, states.stale.nr,
+						&err))
+				result |= error("%s", err.buf);
+			strbuf_release(&err);
+		}
 		free(delete_refs);
 	}

Re: [PATCH v21 0/19] rs/ref-transaction (Re: Transaction patch series overview)

From: Jonathan Nieder <hidden>
Date: 2016-06-15 23:02:30

Junio C Hamano wrote:

Jonathan Nieder [off-list ref] writes:

quoted

Junio C Hamano wrote:

quoted

The tag fetched and built as-is seems to break 5514 among other
things ("git remote rm" segfaults).

Yeah, I noticed that right after sending the series out. :/

The patch below fixes it[1].

Is this meant to replace anything, or is it "Oops, the previous ones
are broken, and this is to patch it up on top"?

It's "Oops, the next one (refs.c: do not permit err == NULL) is broken,
and this is to patch it up in advance". :)

But it should apply on top, too.

There were a few other minor changes, and I think with them the series
should be ready for "next".  My "send and hope that more reviewers
appear" strategy didn't really work, so I'll send a reroll of the
series as-is in an hour or so.

Here's an interdiff as a preview.

diff --git a/builtin/branch.c b/builtin/branch.c
index 5d5bc56..4bf931e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -238,9 +238,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 					    RESOLVE_REF_READING
 					    | RESOLVE_REF_NODEREF
 					    | RESOLVE_REF_ALLOW_BAD_NAME);
-		if (!target ||
-		    (!(flags & (REF_ISSYMREF|REF_ISBROKEN)) &&
-		     is_null_sha1(sha1))) {
+		if (!target) {
 			error(remote_branch
 			      ? _("remote branch '%s' not found.")
 			      : _("branch '%s' not found."), bname.buf);

@@ -268,8 +266,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 			       ? _("Deleted remote branch %s (was %s).\n")
 			       : _("Deleted branch %s (was %s).\n"),
 			       bname.buf,
-			       (flags & REF_ISSYMREF)
-			       ? target
+			       (flags & REF_ISBROKEN) ? "broken"
+			       : (flags & REF_ISSYMREF) ? target
 			       : find_unique_abbrev(sha1, DEFAULT_ABBREV));
 		}
 		delete_branch_config(bname.buf);

diff --git a/builtin/remote.c b/builtin/remote.c
index 6eaeee7..ef1ffc3 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -750,13 +750,16 @@ static int mv(int argc, const char **argv)
 
 static int remove_branches(struct string_list *branches)
 {
+	struct strbuf err = STRBUF_INIT;
 	const char **branch_names;
 	int i, result = 0;
 
 	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
 	for (i = 0; i < branches->nr; i++)
 		branch_names[i] = branches->items[i].string;
-	result |= repack_without_refs(branch_names, branches->nr, NULL);
+	if (repack_without_refs(branch_names, branches->nr, &err))
+		result |= error("%s", err.buf);
+	strbuf_release(&err);
 	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {

@@ -1333,9 +1336,13 @@ static int prune_remote(const char *remote, int dry_run)
 		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
 		for (i = 0; i < states.stale.nr; i++)
 			delete_refs[i] = states.stale.items[i].util;
-		if (!dry_run)
-			result |= repack_without_refs(delete_refs,
-						      states.stale.nr, NULL);
+		if (!dry_run) {
+			struct strbuf err = STRBUF_INIT;
+			if (repack_without_refs(delete_refs, states.stale.nr,
+						&err))
+				result |= error("%s", err.buf);
+			strbuf_release(&err);
+		}
 		free(delete_refs);
 	}

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help