On Wed, Jul 10, 2024 at 06:12:21AM -0700, Karthik Nayak wrote:
> > In any case, an obvious additional fix on top of your change might
> > be to do something like this:
> >
> > diff --git i/remote.c w/remote.c
> > index 5fa046c8f8..d7f9ba3571 100644
> > --- i/remote.c
> > +++ w/remote.c
> > @@ -682,7 +682,7 @@ remotes_remote_get_1(
> > struct remote *ret;
> > int name_given = 0;
> >
> > - if (name)
> > + if (name && *name)
> > name_given = 1;
> > else
> > name = get_default(remote_state, remote_state->current_branch,
> >
> > which would give us the default remote name, and we would not call
> > add_url_alias() with a bogus empty string to nuke the list.
> >
>
> I'm a bit skeptical of making this change. Mostly from the user's
> perspective.
>
> With my patch currently:
>
> $ git push "" refs/heads/master
> fatal: bad repository ''
>
> But with this added, we'd be doing
>
> $ git push "" refs/heads/master
> Everything up-to-date
>
> This is because we actually obtained the default remote here. Isn't this
> confusing from a user's perspective? I mean I agree that an empty repo
> name is something we should support, but it also shouldn't be something
> we simply ignore?
Oh, I misread Junio's patch in my earlier response. I was focused on not
setting name_given, which I thought would result in a NULL return value,
and didn't notice that it would also mean using the default remote.
Something like:
diff --git a/remote.c b/remote.c
index 7f6406aaa2..883cf6086e 100644
--- a/remote.c
+++ b/remote.c
@@ -703,7 +703,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
if (!valid_remote(ret))
read_branches_file(remote_state, ret);
}
- if (name_given && !valid_remote(ret))
+ if (name_given && *name && !valid_remote(ret))
add_url_alias(remote_state, ret, name);
if (!valid_remote(ret))
return NULL;
was more what I was thinking. That is, inhibit the empty string
explicitly rather than letting the emergent behavior of add_url_alias()
do it for us. Or maybe even just:
diff --git a/remote.c b/remote.c
index 7f6406aaa2..a0b166131f 100644
--- a/remote.c
+++ b/remote.c
@@ -690,9 +690,11 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
struct remote *ret;
int name_given = 0;
- if (name)
+ if (name) {
+ if (!name)
+ return NULL;
name_given = 1;
- else
+ } else
name = get_default(remote_state, remote_state->current_branch,
&name_given);
to bail immediately.
But all of that would be internal refactoring / cleanup on top of your
patch. The user-facing behavior would be the same.
-Peff
