Re: [PATCH v2] pull, fetch: fix segfault in --set-upstream option

On Tue, Aug 24 2021, Clemens Fruhwirth wrote:

On Mon, 23 Aug 2021 at 14:56, Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

A proposed replacement of cf/fetch-set-upstream-while-detached which
as noted in What's Cooking has been languishing for a while. I changed
the die() in my version to a warning() as suggested by Junio & updated
the test and commit message accordingly.

Thank you Ævar, for taking care of that and adding tests and my
apologizes for not responding in a timely manner.

No worries.

quoted

 builtin/fetch.c         | 11 +++++++++++
 t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 25740c13df1..ca487edd805 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1623,6 +1623,17 @@ static int do_fetch(struct transport *transport,
                        }
                }
                if (source_ref) {
+                       if (!branch) {
+                               const char *shortname = NULL;
+                               if (!skip_prefix(source_ref->name,
+                                                "refs/heads/", &shortname))
+                                       shortname = source_ref->name;

Is skip_prefix ever able to fail?

- If yes, then shortname will be left pointing to NULL when we print
the warning below. Warning probably won't die because of NULL, but it
still will print something weird.

If it fails we'll use source_ref->name, so it won't be NULL...

- If no, then an assert(!skip_prefix(source_ref->name, "refs/heads/",
&shortname)) might be more idiomatic.

Which means that this would be a bug, since it's not handling the case
where the source doesn't start with refs/heads/*.

I.e. it's the same as doing it this way (on top of this v2), which
perhaps is a clearer way to express the same idea. What do you think?

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ca487edd805..2bc9159690d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c

@@ -1624,10 +1624,9 @@ static int do_fetch(struct transport *transport,
 		}
 		if (source_ref) {
 			if (!branch) {
-				const char *shortname = NULL;
-				if (!skip_prefix(source_ref->name,
-						 "refs/heads/", &shortname))
-					shortname = source_ref->name;
+				const char *shortname = source_ref->name;
+				skip_prefix(shortname, "refs/heads/", &shortname);
+
 				warning(_("could not set upstream of HEAD to '%s' from '%s' when "
 					  "it does not point to any branch."),
 					shortname, transport->remote->name);