git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 3/5] rebase: factor out merge_base calculation

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 15, 2022, 17:22 UTC
Message-ID
<xmqqczd12zhv.fsf@gitster.g>
In-Reply-To
<019158db9d2dbb371705ba79a96a907e4a17cdb1.1660576283.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 33 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Separate out calculating the merge base between onto and head from the
> check for whether we can fast-forward or not. This means we can skip
> the fast-forward checks when the rebase is forced and avoid
> calculating the merge-base twice when --keep-base is given.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> Note the unnecessary braces around "if (keep_base)" are added here
> reduce the churn on the next commit.
> ---
>  builtin/rebase.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6cf9c95f4e1..86ea731ca3a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>  	struct commit_list *merge_bases = NULL;
>  	int res = 0;
>  
> -	merge_bases = get_merge_bases(onto, head);
> -	if (!merge_bases || merge_bases->next) {
> -		oidcpy(merge_base, null_oid());
> +	if (is_null_oid(merge_base))
>  		goto done;
> -	}
>  
> -	oidcpy(merge_base, &merge_bases->item->object.oid);
>  	if (!oideq(merge_base, &onto->object.oid))
>  		goto done;

Looking at the change in "git show -W", it seems that this function no longer touches merge_bases at all, other than initializing it to NULL at the beginning and then calling free_commit_list() on it at the end. Shouldn't it be removed?

Show 31 quoted lines
> @@ -902,6 +898,20 @@ done:
>  	return res && is_linear_history(onto, head);
>  }
>  
> +static void fill_merge_base(struct rebase_options *options,
> +			    struct object_id *merge_base)
> +{
> +	struct commit_list *merge_bases = NULL;
> +
> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
> +	if (!merge_bases || merge_bases->next)
> +		oidcpy(merge_base, null_oid());
> +	else
> +		oidcpy(merge_base, &merge_bases->item->object.oid);
> +
> +	free_commit_list(merge_bases);
> +}
> +
>  static int parse_opt_am(const struct option *opt, const char *arg, int unset)
>  {
>  	struct rebase_options *opts = opt->value;
> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			die(_("Does not point to a valid commit '%s'"),
>  				options.onto_name);
>  	}
> -
> +	if (keep_base) {
> +		oidcpy(&merge_base, &options.onto->object.oid);
> +	} else {
> +		fill_merge_base(&options, &merge_base);
> +	}
No need for braces around single-statement block on either side.

This is not a new issue introduced by this patch per-se, but "merge_base" is becoming less and less accurate description of what this variable really is. Perhaps it is a good time to rename it?

It is "the base commit to rebuild the history on top of", aka "onto commit", isn't it? We often use merge-base between the upstream and our tip of the history for it, but the variable often does not even hold the merge-base in it, not because we cannot compute a single merge-base but because depending on the operation mode we do not want to use merge-base for further operation using that variable.

Previous: Phillip Wood via GitGitGadgetNext: Johannes Schindelin
Message 12 of 82 in “rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point”
  1. 0/5 rebase --keep-base: imply --reapply-cherry-picks and --no-fork-pointPhillip Wood via GitGitGadget, Aug 15, 2022
  2. 1/5 t3416: set $EDITOR in subshellPhillip Wood via GitGitGadget, Aug 15, 2022
  3. Junio C HamanoAug 15, 2022
  4. Phillip WoodAug 16, 2022
  5. Jonathan TanAug 24, 2022
  6. Phillip WoodAug 30, 2022
  7. 2/5 rebase: store orig_head as a commitPhillip Wood via GitGitGadget, Aug 15, 2022
  8. Junio C HamanoAug 15, 2022
  9. Johannes SchindelinAug 16, 2022
  10. Elijah NewrenAug 18, 2022
  11. 3/5 rebase: factor out merge_base calculationPhillip Wood via GitGitGadget, Aug 15, 2022
  12. Junio C HamanoAug 15, 2022
  13. Johannes SchindelinAug 16, 2022
  14. Junio C HamanoAug 16, 2022
  15. Phillip WoodAug 16, 2022
  16. Junio C HamanoAug 16, 2022
  17. Elijah NewrenAug 18, 2022
  18. Jonathan TanAug 24, 2022
  19. Phillip WoodAug 30, 2022
  20. 4/5 rebase --keep-base: imply --reapply-cherry-picksPhillip Wood via GitGitGadget, Aug 15, 2022
  21. Junio C HamanoAug 15, 2022
  22. Jonathan TanAug 24, 2022
  23. Phillip WoodAug 30, 2022
  24. Philippe BlainAug 25, 2022
  25. Phillip WoodSep 5, 2022
  26. 5/5 rebase --keep-base: imply --no-fork-pointPhillip Wood via GitGitGadget, Aug 15, 2022
  27. Junio C HamanoAug 15, 2022
  28. Jonathan TanAug 24, 2022
  29. Phillip WoodSep 5, 2022
  30. Johannes SchindelinAug 16, 2022
  31. Jonathan TanAug 24, 2022
  32. 0/7 rebase --keep-base: imply --reapply-cherry-picks and --no-fork-pointPhillip Wood via GitGitGadget, Sep 7, 2022
  33. 1/7 t3416: tighten two testsPhillip Wood via GitGitGadget, Sep 7, 2022
  34. Junio C HamanoSep 7, 2022
  35. 2/7 t3416: set $EDITOR in subshellPhillip Wood via GitGitGadget, Sep 7, 2022
  36. Junio C HamanoSep 7, 2022
  37. 3/7 rebase: store orig_head as a commitPhillip Wood via GitGitGadget, Sep 7, 2022
  38. Junio C HamanoSep 7, 2022
  39. Phillip WoodSep 8, 2022
  40. 5/7 rebase: factor out branch_base calculationPhillip Wood via GitGitGadget, Sep 7, 2022
  41. Junio C HamanoSep 7, 2022
  42. 4/7 rebase: rename merge_base to branch_basePhillip Wood via GitGitGadget, Sep 7, 2022
  43. Junio C HamanoSep 7, 2022
  44. 6/7 rebase --keep-base: imply --reapply-cherry-picksPhillip Wood via GitGitGadget, Sep 7, 2022
  45. 7/7 rebase --keep-base: imply --no-fork-pointPhillip Wood via GitGitGadget, Sep 7, 2022
  46. Denton LiuSep 8, 2022
  47. Phillip WoodSep 8, 2022
  48. 0/8 rebase --keep-base: imply --reapply-cherry-picks and --no-fork-pointPhillip Wood via GitGitGadget, Oct 13, 2022
  49. 1/8 t3416: tighten two testsPhillip Wood via GitGitGadget, Oct 13, 2022
  50. 2/8 t3416: set $EDITOR in subshellPhillip Wood via GitGitGadget, Oct 13, 2022
  51. 3/8 rebase: be stricter when reading state files containing oidsPhillip Wood via GitGitGadget, Oct 13, 2022
  52. Junio C HamanoOct 13, 2022
  53. Ævar Arnfjörð BjarmasonOct 13, 2022
  54. Junio C HamanoOct 13, 2022
  55. 4/8 rebase: store orig_head as a commitPhillip Wood via GitGitGadget, Oct 13, 2022
  56. Junio C HamanoOct 13, 2022
  57. Phillip WoodOct 13, 2022
  58. Junio C HamanoOct 13, 2022
  59. 6/8 rebase: factor out branch_base calculationPhillip Wood via GitGitGadget, Oct 13, 2022
  60. Ævar Arnfjörð BjarmasonOct 13, 2022
  61. Phillip WoodOct 17, 2022
  62. Ævar Arnfjörð BjarmasonOct 17, 2022
  63. 5/8 rebase: rename merge_base to branch_basePhillip Wood via GitGitGadget, Oct 13, 2022
  64. Ævar Arnfjörð BjarmasonOct 13, 2022
  65. Phillip WoodOct 17, 2022
  66. Ævar Arnfjörð BjarmasonOct 17, 2022
  67. Phillip WoodOct 17, 2022
  68. Ævar Arnfjörð BjarmasonOct 17, 2022
  69. Phillip WoodOct 19, 2022
  70. 7/8 rebase --keep-base: imply --reapply-cherry-picksPhillip Wood via GitGitGadget, Oct 13, 2022
  71. 8/8 rebase --keep-base: imply --no-fork-pointPhillip Wood via GitGitGadget, Oct 13, 2022
  72. 0/8 rebase --keep-base: imply --reapply-cherry-picks and --no-fork-pointPhillip Wood via GitGitGadget, Oct 17, 2022
  73. 1/8 t3416: tighten two testsPhillip Wood via GitGitGadget, Oct 17, 2022
  74. 2/8 t3416: set $EDITOR in subshellPhillip Wood via GitGitGadget, Oct 17, 2022
  75. 3/8 rebase: be stricter when reading state files containing oidsPhillip Wood via GitGitGadget, Oct 17, 2022
  76. Junio C HamanoOct 17, 2022
  77. Phillip WoodOct 19, 2022
  78. 4/8 rebase: store orig_head as a commitPhillip Wood via GitGitGadget, Oct 17, 2022
  79. 6/8 rebase: factor out branch_base calculationPhillip Wood via GitGitGadget, Oct 17, 2022
  80. 5/8 rebase: rename merge_base to branch_basePhillip Wood via GitGitGadget, Oct 17, 2022
  81. 7/8 rebase --keep-base: imply --reapply-cherry-picksPhillip Wood via GitGitGadget, Oct 17, 2022
  82. 8/8 rebase --keep-base: imply --no-fork-pointPhillip Wood via GitGitGadget, Oct 17, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.