| Message ID | 20210529071115.1908310-3-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Unconvolutize push.default=simple | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > In order to avoid doing unnecessary things and simplify it in further > patches. > > The code is copied exactly as-is; no functional changes. The title says "move" and I expected to see corresponding deletions, but it seems that this (possibly temporarily) duplicates what these two functions do, without removing them, which (possibly temporarily) risks the two to drift apart. So, the resulting code from this step will do nothing wrong (it's just two function's bodies inlined in duplicated form), but depending on how the code evolves, it might turn out to be a good change or a bad change---we cannot judge by this step alone. From a quick scanning of the remainder of the series, it seems that 3/6 and 4/6 improve the copied code without changing the behaviour, and at the end these two functions remain, so we have duplicated logic for these two functions and improvements only live in one of the copies (namely, in the setup_push_simple())? Would that be a problem, or it is too much work to do better, I wonder? Let's keep reading. > Reviewed-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/push.c | 36 ++++++++++++++++++++++++++++++++---- > 1 file changed, 32 insertions(+), 4 deletions(-) > > diff --git a/builtin/push.c b/builtin/push.c > index 7045e4ef0c..d173c39283 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -225,10 +225,38 @@ static void setup_push_current(struct remote *remote, struct branch *branch) > > static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular) > { > - if (triangular) > - setup_push_current(remote, branch); > - else > - setup_push_upstream(remote, branch, triangular, 1); > + if (triangular) { > + if (!branch) > + die(_(message_detached_head_die), remote->name); > + refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); > + } else { > + if (!branch) > + die(_(message_detached_head_die), remote->name); > + if (!branch->merge_nr || !branch->merge || !branch->remote_name) > + die(_("The current branch %s has no upstream branch.\n" > + "To push the current branch and set the remote as upstream, use\n" > + "\n" > + " git push --set-upstream %s %s\n"), > + branch->name, > + remote->name, > + branch->name); > + if (branch->merge_nr != 1) > + die(_("The current branch %s has multiple upstream branches, " > + "refusing to push."), branch->name); > + if (triangular) > + die(_("You are pushing to remote '%s', which is not the upstream of\n" > + "your current branch '%s', without telling me what to push\n" > + "to update which remote branch."), > + remote->name, branch->name); > + > + if (1) { > + /* Additional safety */ > + if (strcmp(branch->refname, branch->merge[0]->src)) > + die_push_simple(branch, remote); > + } > + > + refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src); > + } > } > > static int is_workflow_triangular(struct remote *remote)
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > In order to avoid doing unnecessary things and simplify it in further > > patches. > > > > The code is copied exactly as-is; no functional changes. > > The title says "move" and I expected to see corresponding deletions, > but it seems that this (possibly temporarily) duplicates what these > two functions do, without removing them, which (possibly > temporarily) risks the two to drift apart. OK. Copy then. > From a quick scanning of the remainder of the series, it seems that > 3/6 and 4/6 improve the copied code without changing the behaviour, > and at the end these two functions remain, so we have duplicated > logic for these two functions and improvements only live in one of > the copies (namely, in the setup_push_simple())? Not quite: setup_push_upstream() was simplified too, in 5/6. Maybe I can mention that part of the code of the `simple` mode lives in setup_push_upstream() and by copying the code to the right function both can be simplified.
diff --git a/builtin/push.c b/builtin/push.c index 7045e4ef0c..d173c39283 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -225,10 +225,38 @@ static void setup_push_current(struct remote *remote, struct branch *branch) static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular) { - if (triangular) - setup_push_current(remote, branch); - else - setup_push_upstream(remote, branch, triangular, 1); + if (triangular) { + if (!branch) + die(_(message_detached_head_die), remote->name); + refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); + } else { + if (!branch) + die(_(message_detached_head_die), remote->name); + if (!branch->merge_nr || !branch->merge || !branch->remote_name) + die(_("The current branch %s has no upstream branch.\n" + "To push the current branch and set the remote as upstream, use\n" + "\n" + " git push --set-upstream %s %s\n"), + branch->name, + remote->name, + branch->name); + if (branch->merge_nr != 1) + die(_("The current branch %s has multiple upstream branches, " + "refusing to push."), branch->name); + if (triangular) + die(_("You are pushing to remote '%s', which is not the upstream of\n" + "your current branch '%s', without telling me what to push\n" + "to update which remote branch."), + remote->name, branch->name); + + if (1) { + /* Additional safety */ + if (strcmp(branch->refname, branch->merge[0]->src)) + die_push_simple(branch, remote); + } + + refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src); + } } static int is_workflow_triangular(struct remote *remote)