| Message ID | 20210529071115.1908310-1-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Unconvolutize push.default=simple | expand |
On 29/05/2021 08:11, Felipe Contreras wrote: > Tired of jumping through hoops trying to understand what the "simple" > mode does, I decided to reorganize it up for good so it's crystal > clear. > > There are no functional changes. > > Basically the simple mode pushes the current branch with the same name > on the remote. > > Except... when there's no upstream branch configured with the same name. > > Now the code and the documentation are clear. Not your problem, but I do note, as a general point, that we don't explain the different variants of Triangular workflow anywhere [just two mentions in gitrevisions.txt] (e.g. patch flow versus server-side merges, etc.). Philip > > This is basically the same as v1, except I removed a bunch of patches which are now part of a > different series. Also, I updated some commit messages with suggestions from Elijah Newren. > > The result of this series is in short this function: > > static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular) > { > if (!branch) > die(_(message_detached_head_die), remote->name); > > if (!triangular) { > 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); > > /* Additional safety */ > if (strcmp(branch->refname, branch->merge[0]->src)) > die_push_simple(branch, remote); > } > refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); > } > > Felipe Contreras (6): > push: hedge code of default=simple > push: move code to setup_push_simple() > push: reorganize setup_push_simple() > push: simplify setup_push_simple() > push: remove unused code in setup_push_upstream() > doc: push: explain default=simple correctly > > Documentation/config/push.txt | 13 ++++++------ > builtin/push.c | 40 ++++++++++++++++++++++++----------- > 2 files changed, 34 insertions(+), 19 deletions(-) > > Range-diff against v1: > 1: 2856711eb3 ! 1: f1f42bda32 push: hedge code of default=simple > @@ Commit message > `simple` is the most important mode so move the relevant code to its own > function to make it easier to see what it's doing. > > + Reviewed-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > ## builtin/push.c ## > 2: 33acb09e82 ! 2: bb6d810011 push: move code to setup_push_simple() > @@ Commit message > > The code is copied exactly as-is; no functional changes. > > + Reviewed-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > ## builtin/push.c ## > 3: de1b621b7e ! 3: d66a442fba push: reorganize setup_push_simple() > @@ Metadata > ## Commit message ## > push: reorganize setup_push_simple() > > - Simply move the code around. > + Simply move the code around and remove dead code. In particular the > + 'trivial' conditional is a no-op since that part of the code is the > + !trivial leg of the conditional beforehand. > > No functional changes. > > + Suggestions-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > ## builtin/push.c ## > 4: 83efcad143 ! 4: eaae6a826a push: simplify setup_push_simple() > @@ Metadata > ## Commit message ## > push: simplify setup_push_simple() > > - branch->refname can never be different from branch->merge[0]->src. > + There's a safety check to make sure branch->refname is not different > + from branch->merge[0]->src, otherwise we die(). > > + Therefore we always push to branch->refname. > + > + Suggestions-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > ## builtin/push.c ## > 5: d7489e9545 = 5: 8d9ae5e552 push: remove unused code in setup_push_upstream() > 6: e693341403 < -: ---------- push: merge current and simple > 7: 830a57c867 < -: ---------- push: remove redundant check > 8: d2dded5998 < -: ---------- push: fix Yoda condition > 9: 7528738091 < -: ---------- push: remove trivial function > 10: 1ae0546df6 < -: ---------- push: flip !triangular for centralized > 11: 3acd42e385 ! 6: b35bdf14dc doc: push: explain default=simple correctly > @@ Metadata > ## Commit message ## > doc: push: explain default=simple correctly > > - Now that the code has been unconvolutized and it's clear what it's > + Now that the code has been simplified and it's clear what it's > actually doing, update the documentation to reflect that. > > Namely; the simple mode only barfs when working on a centralized
On Sat, May 29 2021, Felipe Contreras wrote: > Tired of jumping through hoops trying to understand what the "simple" > mode does, I decided to reorganize it up for good so it's crystal > clear. I'd find the end-state even more readable if this "triangular" wasn't passed as a parameter but we just did the top-level dispatch based on that, i.e. "simple" is really internally SIMPLE_SIMPLE and SIMPLE_TRIANGULAR, why not dispatch on that? Our internal enum doesn't need to 1=1 map to the config setting. Part of that's that I prefer reading code in the current "master" which is if -> die -> most of the rest of function, v.s. your end-state of if -> stuff -> else -> most of the function being indented to that "else".
Ævar Arnfjörð Bjarmason wrote: > > On Sat, May 29 2021, Felipe Contreras wrote: > > > Tired of jumping through hoops trying to understand what the "simple" > > mode does, I decided to reorganize it up for good so it's crystal > > clear. > > I'd find the end-state even more readable if this "triangular" wasn't > passed as a parameter but we just did the top-level dispatch based on > that, i.e. "simple" is really internally SIMPLE_SIMPLE and > SIMPLE_TRIANGULAR, why not dispatch on that? Our internal enum doesn't > need to 1=1 map to the config setting. Yes, but where are you going to make sure you are in SIMPLE, and not in CURRENT? Surely you are not suggesting to modify git_default_push_config(), which is pretty straightforward. So we would need yet another enum. I don't think there's a need for that, as you can see on the patch series on top of this, there's not even a need to dispatch anything. > Part of that's that I prefer reading code in the current "master" which > is if -> die -> most of the rest of function, v.s. your end-state of if > -> stuff -> else -> most of the function being indented to that "else". I prefer that as well, but unfortunately in this case we need to do something after the checks to die(). I could add a goto, but for just one conditional seems like it's not worth it. Either way this is a temporary thing, because in the next patch series the code becomes pretty quickly: if (!triangular && strcmp(branch->refname, get_upstream_ref(branch, remote->name))) die_push_simple(branch, remote); And then: if (triangular) break; if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) die_push_simple(branch, remote); Cheers.
Philip Oakley wrote: > On 29/05/2021 08:11, Felipe Contreras wrote: > > Tired of jumping through hoops trying to understand what the "simple" > > mode does, I decided to reorganize it up for good so it's crystal > > clear. > > > > There are no functional changes. > > > > Basically the simple mode pushes the current branch with the same name > > on the remote. > > > > Except... when there's no upstream branch configured with the same name. > > > > Now the code and the documentation are clear. > > Not your problem, but I do note, as a general point, that we don't > explain the different variants of Triangular workflow anywhere [just two > mentions in gitrevisions.txt] (e.g. patch flow versus server-side > merges, etc.). All my documentation improvements get stuck in tar, so somebody else would need to do that. If and when they do, it will become clear the big pit in functionality there is.
Tired of jumping through hoops trying to understand what the "simple" mode does, I decided to reorganize it up for good so it's crystal clear. There are no functional changes. Basically the simple mode pushes the current branch with the same name on the remote. Except... when there's no upstream branch configured with the same name. Now the code and the documentation are clear. This is basically the same as v1, except I removed a bunch of patches which are now part of a different series. Also, I updated some commit messages with suggestions from Elijah Newren. The result of this series is in short this function: static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular) { if (!branch) die(_(message_detached_head_die), remote->name); if (!triangular) { 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); /* Additional safety */ if (strcmp(branch->refname, branch->merge[0]->src)) die_push_simple(branch, remote); } refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); } Felipe Contreras (6): push: hedge code of default=simple push: move code to setup_push_simple() push: reorganize setup_push_simple() push: simplify setup_push_simple() push: remove unused code in setup_push_upstream() doc: push: explain default=simple correctly Documentation/config/push.txt | 13 ++++++------ builtin/push.c | 40 ++++++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 19 deletions(-) Range-diff against v1: 1: 2856711eb3 ! 1: f1f42bda32 push: hedge code of default=simple @@ Commit message `simple` is the most important mode so move the relevant code to its own function to make it easier to see what it's doing. + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> ## builtin/push.c ## 2: 33acb09e82 ! 2: bb6d810011 push: move code to setup_push_simple() @@ Commit message The code is copied exactly as-is; no functional changes. + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> ## builtin/push.c ## 3: de1b621b7e ! 3: d66a442fba push: reorganize setup_push_simple() @@ Metadata ## Commit message ## push: reorganize setup_push_simple() - Simply move the code around. + Simply move the code around and remove dead code. In particular the + 'trivial' conditional is a no-op since that part of the code is the + !trivial leg of the conditional beforehand. No functional changes. + Suggestions-by: Elijah Newren <newren@gmail.com> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> ## builtin/push.c ## 4: 83efcad143 ! 4: eaae6a826a push: simplify setup_push_simple() @@ Metadata ## Commit message ## push: simplify setup_push_simple() - branch->refname can never be different from branch->merge[0]->src. + There's a safety check to make sure branch->refname is not different + from branch->merge[0]->src, otherwise we die(). + Therefore we always push to branch->refname. + + Suggestions-by: Elijah Newren <newren@gmail.com> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> ## builtin/push.c ## 5: d7489e9545 = 5: 8d9ae5e552 push: remove unused code in setup_push_upstream() 6: e693341403 < -: ---------- push: merge current and simple 7: 830a57c867 < -: ---------- push: remove redundant check 8: d2dded5998 < -: ---------- push: fix Yoda condition 9: 7528738091 < -: ---------- push: remove trivial function 10: 1ae0546df6 < -: ---------- push: flip !triangular for centralized 11: 3acd42e385 ! 6: b35bdf14dc doc: push: explain default=simple correctly @@ Metadata ## Commit message ## doc: push: explain default=simple correctly - Now that the code has been unconvolutized and it's clear what it's + Now that the code has been simplified and it's clear what it's actually doing, update the documentation to reflect that. Namely; the simple mode only barfs when working on a centralized