| Message ID | 20210528201014.2175179-4-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Unconvolutize push.default=simple | expand |
On Fri, May 28, 2021 at 1:10 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Simply move the code around. Not quite, you also deleted dead code. Made the patch a bit harder to read because I was trying to verify you did what the commit message said and it took me longer than it should have to realize that you were also deleting dead code. Might be worth including that fact in this sentence here. > No functional changes. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/push.c | 29 ++++++++++++----------------- > 1 file changed, 12 insertions(+), 17 deletions(-) > > diff --git a/builtin/push.c b/builtin/push.c > index d173c39283..9c807ed707 100644 > --- a/builtin/push.c > +++ b/builtin/push.c > @@ -225,13 +225,14 @@ static void setup_push_current(struct remote *remote, struct branch *branch) > > static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular) > { > + const char *dst; > + > + if (!branch) > + die(_(message_detached_head_die), remote->name); > + > if (triangular) { > - if (!branch) > - die(_(message_detached_head_die), remote->name); > - refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); > + dst = 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" > @@ -243,20 +244,14 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int > 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); This if-block is safe to delete because we're already in the !triangular case. > - > - 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); > + /* Additional safety */ > + if (strcmp(branch->refname, branch->merge[0]->src)) > + die_push_simple(branch, remote); > + > + dst = branch->merge[0]->src; > } > + refspec_appendf(&rs, "%s:%s", branch->refname, dst); > } > > static int is_workflow_triangular(struct remote *remote) > -- > 2.32.0.rc0 Everything else is, as you say, just moving code around.
Elijah Newren wrote: > On Fri, May 28, 2021 at 1:10 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > Simply move the code around. > > Not quite, you also deleted dead code. Made the patch a bit harder to > read because I was trying to verify you did what the commit message > said and it took me longer than it should have to realize that you > were also deleting dead code. Might be worth including that fact in > this sentence here. OK. I thought that was obvious. Shall I update the commit message to include that fact, or shall I add a separate patch to remove the dead code? Either are fine by me. > > - 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); > > This if-block is safe to delete because we're already in the !triangular case. > > > - > > - if (1) { Techically this is removing dead code too.
On Fri, May 28, 2021 at 2:27 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > Elijah Newren wrote: > > On Fri, May 28, 2021 at 1:10 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > > > Simply move the code around. > > > > Not quite, you also deleted dead code. Made the patch a bit harder to > > read because I was trying to verify you did what the commit message > > said and it took me longer than it should have to realize that you > > were also deleting dead code. Might be worth including that fact in > > this sentence here. > > OK. I thought that was obvious. It is if you are familiar with the code, or if you still remember that condition when you get to that point in the review, or if you happen to look at the right line of code while mulling it over. Unfortunately, I'm not familiar with this code, didn't happen to remember the outer if-block when I got to that point in the comparison, and didn't at first look back to the relevant line to realize this. So I started looking elsewhere for why removing that condition didn't amount to functional changes and started trying to figure out a testcase that would give different behavior. I suspect I would have realized sooner if not for the claim that you "simply moved code around", so I just flagged it. Not a big deal, just something that I think could make it easier for other reviewers. > Shall I update the commit message to include that fact, or shall I add a > separate patch to remove the dead code? I'd just tweak the commit message to mention you are just deleting dead code and moving code around. > Either are fine by me. > > > > - 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); > > > > This if-block is safe to delete because we're already in the !triangular case. > > > > > - > > > - if (1) { > > Techically this is removing dead code too. Yep, true.
diff --git a/builtin/push.c b/builtin/push.c index d173c39283..9c807ed707 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -225,13 +225,14 @@ static void setup_push_current(struct remote *remote, struct branch *branch) static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular) { + const char *dst; + + if (!branch) + die(_(message_detached_head_die), remote->name); + if (triangular) { - if (!branch) - die(_(message_detached_head_die), remote->name); - refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); + dst = 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" @@ -243,20 +244,14 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int 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); + /* Additional safety */ + if (strcmp(branch->refname, branch->merge[0]->src)) + die_push_simple(branch, remote); + + dst = branch->merge[0]->src; } + refspec_appendf(&rs, "%s:%s", branch->refname, dst); } static int is_workflow_triangular(struct remote *remote)
Simply move the code around. No functional changes. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/push.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-)