[v3,7/8] rebase: update refs from 'update-ref' commands

Commit Message

Derrick Stolee June 28, 2022, 1:25 p.m. UTC

From: Derrick Stolee <[e-mail]>

The previous change introduced the 'git rebase --update-refs' option
which added 'update-ref <ref>' commands to the todo list of an
interactive rebase.

Teach Git to record the HEAD position when reaching these 'update-ref'
commands. The ref/OID pair is stored in the
$GIT_DIR/rebase-merge/update-refs file. A previous change parsed this
file to avoid having other processes updating the refs in that file
while the rebase is in progress.

Not only do we update the file when the sequencer reaches these
'update-ref' commands, we then update the refs themselves at the end of
the rebase sequence. If the rebase is aborted before this final step,
then the refs are not updated.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 sequencer.c                   | 114 +++++++++++++++++++++++++++++++++-
 t/t3404-rebase-interactive.sh |  23 +++++++
 2 files changed, 136 insertions(+), 1 deletion(-)

Comments

Junio C Hamano June 28, 2022, 9:15 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> From: Derrick Stolee <[e-mail]>
>
> The previous change introduced the 'git rebase --update-refs' option
> which added 'update-ref <ref>' commands to the todo list of an
> interactive rebase.
>
> Teach Git to record the HEAD position when reaching these 'update-ref'
> commands. The ref/OID pair is stored in the
> $GIT_DIR/rebase-merge/update-refs file. A previous change parsed this
> file to avoid having other processes updating the refs in that file
> while the rebase is in progress.
>
> Not only do we update the file when the sequencer reaches these
> 'update-ref' commands, we then update the refs themselves at the end of
> the rebase sequence. If the rebase is aborted before this final step,
> then the refs are not updated.

So, in general, we would

 * first scan the range of commits being rebased
 * compute what should happen and write the "todo" thing
 * write also "update-refs" to lick the branches to repel others
 * execute the "todo" insns one by one, possibly giving control back

And this order is important---update-refs file is written fairly
early, and the branches potentially involved in the rebase are all
protected during the time-consuming (due to possibility of manual
conflict resolution) execution step.

Makes sense.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 3cd20733bc8..63a69bc073e 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1813,6 +1813,29 @@ test_expect_success '--update-refs adds commands with --rebase-merges' '
>  	)
>  '
>  
> +compare_two_refs () {
> +	git rev-parse $1 >expect &&
> +	git rev-parse $2 >actual &&
> +	test_cmp expect actual
> +}
> +
> +test_expect_success '--update-refs updates refs correctly' '
> +	git checkout -B update-refs no-conflict-branch &&
> +	git branch -f base HEAD~4 &&
> +	git branch -f first HEAD~3 &&
> +	git branch -f second HEAD~3 &&
> +	git branch -f third HEAD~1 &&
> +	test_commit extra2 fileX &&
> +	git commit --amend --fixup=L &&
> +
> +	git rebase -i --autosquash --update-refs primary &&
> +
> +	compare_two_refs HEAD~3 refs/heads/first &&
> +	compare_two_refs HEAD~3 refs/heads/second &&
> +	compare_two_refs HEAD~1 refs/heads/third &&
> +	compare_two_refs HEAD refs/heads/no-conflict-branch
> +'
> +
>  # This must be the last test in this file
>  test_expect_success '$EDITOR and friends are unchanged' '
>  	test_editor_unchanged

It would be nice to also have a test that makes sure that other
people will be prevented from checking out a branch whose tips may
be updated at the end.

Thanks.

Derrick Stolee June 29, 2022, 1:05 p.m. UTC | #2

On 6/28/22 5:15 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
...
> It would be nice to also have a test that makes sure that other
> people will be prevented from checking out a branch whose tips may
> be updated at the end.

Patch 3 adds the tests that 'git branch -f' cannot update these refs,
but I could add 'git checkout' tests, too. They both run the same
branch_checked_out() helper, but they are doing different things once
they have "access" to the branch.

Thanks,
-Stolee

Derrick Stolee June 29, 2022, 1:06 p.m. UTC | #3

On 6/28/22 9:25 AM, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>

> +static int write_update_refs_state(struct string_list *refs_to_oids)
> +{
> +	int result = 0;
> +	FILE *fp = NULL;
> +	struct string_list_item *item;
> +	char *path = xstrdup(rebase_path_update_refs());
> +
> +	if (safe_create_leading_directories(path)) {
> +		result = error(_("unable to create leading directories of %s"),
> +			       path);
> +		goto cleanup;
> +	}
> +
> +	fp = fopen(path, "w");

I realized (while trying to go to sleep, of course) that I need
to use a lockfile here. Parallel processes might be reading this
file while we are writing it.

Thanks,
-Stolee

Junio C Hamano June 30, 2022, 5:09 p.m. UTC | #4

Derrick Stolee <[e-mail]> writes:

> On 6/28/22 5:15 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <[e-mail]> writes:
> ...
>> It would be nice to also have a test that makes sure that other
>> people will be prevented from checking out a branch whose tips may
>> be updated at the end.
>
> Patch 3 adds the tests that 'git branch -f' cannot update these refs,

Ah, I didn't notice.  We are covered then.  Thanks.

Phillip Wood July 1, 2022, 9:31 a.m. UTC | #5

Hi Stolee

On 28/06/2022 14:25, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>
> 
> The previous change introduced the 'git rebase --update-refs' option
> which added 'update-ref <ref>' commands to the todo list of an
> interactive rebase.
> 
> Teach Git to record the HEAD position when reaching these 'update-ref'
> commands. The ref/OID pair is stored in the
> $GIT_DIR/rebase-merge/update-refs file. A previous change parsed this
> file to avoid having other processes updating the refs in that file
> while the rebase is in progress.
> 
> Not only do we update the file when the sequencer reaches these
> 'update-ref' commands, we then update the refs themselves at the end of
> the rebase sequence. If the rebase is aborted before this final step,
> then the refs are not updated.

This looks good, I've left a few comments but it seems basically sound 
to me.

> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>   sequencer.c                   | 114 +++++++++++++++++++++++++++++++++-
>   t/t3404-rebase-interactive.sh |  23 +++++++
>   2 files changed, 136 insertions(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 915d87a0336..4fd1c0b5bce 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -36,6 +36,7 @@
>   #include "rebase-interactive.h"
>   #include "reset.h"
>   #include "branch.h"
> +#include "log-tree.h"
>   
>   #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>   
> @@ -148,6 +149,14 @@ static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
>    */
>   static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
>   
> +/*
> + * The update-refs file stores a list of refs that will be updated at the end
> + * of the rebase sequence. The 'update-ref <ref>' commands in the todo file
> + * update the OIDs for the refs in this file, but the refs are not updated
> + * until the end of the rebase sequence.
> + */
> +static GIT_PATH_FUNC(rebase_path_update_refs, "rebase-merge/update-refs")
> +
>   /*
>    * The following files are written by git-rebase just after parsing the
>    * command-line.
> @@ -4058,11 +4067,110 @@ leave_merge:
>   	return ret;
>   }
>   
> -static int do_update_ref(struct repository *r, const char *ref_name)
> +static int write_update_refs_state(struct string_list *refs_to_oids)
> +{
> +	int result = 0;
> +	FILE *fp = NULL;
> +	struct string_list_item *item;
> +	char *path = xstrdup(rebase_path_update_refs());

This is leaked

> +	if (safe_create_leading_directories(path)) {
> +		result = error(_("unable to create leading directories of %s"),
> +			       path);
> +		goto cleanup;
> +	}
> +
> +	fp = fopen(path, "w");
> +	if (!fp) {
> +		result = error_errno(_("could not open '%s' for writing"), path);
> +		goto cleanup;
> +	}

Can we use fopen_or_warn() here? It ignores ENOENT and ENOTDIR but I 
don't think that should matter here.

> +
> +	for_each_string_list_item(item, refs_to_oids)
> +		fprintf(fp, "%s\n%s\n", item->string, oid_to_hex(item->util));
> +
> +cleanup:
> +	if (fp)
> +		fclose(fp);
> +	return result;
> +}

> +compare_two_refs () {
> +	git rev-parse $1 >expect &&
> +	git rev-parse $2 >actual &&
> +	test_cmp expect actual
> +}

This is just test_cmp_rev

> +test_expect_success '--update-refs updates refs correctly' '
> +	git checkout -B update-refs no-conflict-branch &&
> +	git branch -f base HEAD~4 &&
> +	git branch -f first HEAD~3 &&
> +	git branch -f second HEAD~3 &&
> +	git branch -f third HEAD~1 &&
> +	test_commit extra2 fileX &&
> +	git commit --amend --fixup=L &&
> +
> +	git rebase -i --autosquash --update-refs primary &&
> +
> +	compare_two_refs HEAD~3 refs/heads/first &&
> +	compare_two_refs HEAD~3 refs/heads/second &&
> +	compare_two_refs HEAD~1 refs/heads/third &&
> +	compare_two_refs HEAD refs/heads/no-conflict-branch
> +'

Do we need a new test for this or can we just check the refs at the end 
of one of the tests added in the last patch?

>   # This must be the last test in this file
>   test_expect_success '$EDITOR and friends are unchanged' '
>   	test_editor_unchanged

I forgot to say on the last patch but you could maybe add a 
test_editor_unchanged at the end of t2407 now that there are tests which 
call test_set_editor

Thanks for working on this, it will be a really useful addition to rebase.

Phillip

Junio C Hamano July 1, 2022, 6:35 p.m. UTC | #6

Phillip Wood <[e-mail]> writes:

>> +static int write_update_refs_state(struct string_list *refs_to_oids)
>> +{
>> +	int result = 0;
>> +	FILE *fp = NULL;
>> +	struct string_list_item *item;
>> +	char *path = xstrdup(rebase_path_update_refs());
>
> This is leaked

Good eyes.

>> +	if (safe_create_leading_directories(path)) {
>> +		result = error(_("unable to create leading directories of %s"),
>> +			       path);
>> +		goto cleanup;
>> +	}
>> +
>> +	fp = fopen(path, "w");
>> +	if (!fp) {
>> +		result = error_errno(_("could not open '%s' for writing"), path);
>> +		goto cleanup;
>> +	}
>
> Can we use fopen_or_warn() here? It ignores ENOENT and ENOTDIR but I
> don't think that should matter here.

fopen("no-such-directory/file", "w") would fail with ENOENT, and
fopen("README.md/file", "w") would fail with ENOTDIR, I would think,
so "should not matter because we are writing" is not the reason, but
because we know the path is in a subdirectory of ".git/" that we
know should exist, the most likely reason for the fopen to fail is
because (1) the repository is broken (we will get ENOENT, ENOTDIR,
which we want to warn about but fopen_or_warn() ignores, as well as
other errors such as EISDIR), (2) the repository is unwritable (we
will get EACCES), or (3) we are running low on diskspace (ENOSPC).

I think that the fopen_or_warn() helper was primarily invented to
read an optional file (so we deliberately ignore a failure to open
one due to ENOENT and ENOTDIR), and we should be careful of its use
for any other purpose, i.e. write access for any purpose and read
access for files that we know we should be able to.

>> +compare_two_refs () {
>> +	git rev-parse $1 >expect &&
>> +	git rev-parse $2 >actual &&
>> +	test_cmp expect actual
>> +}
>
> This is just test_cmp_rev

I love to see reviewers who know the existing API and helpers very
well (including the fopen-or-warn above).

Very much appreciated.

> Thanks for working on this, it will be a really useful addition to rebase.

Ditto.

Thanks.

Elijah Newren July 1, 2022, 11:18 p.m. UTC | #7

On Tue, Jun 28, 2022 at 6:26 AM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> From: Derrick Stolee <[e-mail]>
>
> The previous change introduced the 'git rebase --update-refs' option
> which added 'update-ref <ref>' commands to the todo list of an
> interactive rebase.
>
> Teach Git to record the HEAD position when reaching these 'update-ref'
> commands. The ref/OID pair is stored in the
> $GIT_DIR/rebase-merge/update-refs file. A previous change parsed this
> file to avoid having other processes updating the refs in that file
> while the rebase is in progress.
>
> Not only do we update the file when the sequencer reaches these
> 'update-ref' commands, we then update the refs themselves at the end of
> the rebase sequence. If the rebase is aborted before this final step,
> then the refs are not updated.

Why update with each update-ref command?  Couldn't the values be
stored in memory and only written when we hit a conflict?

> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  sequencer.c                   | 114 +++++++++++++++++++++++++++++++++-
>  t/t3404-rebase-interactive.sh |  23 +++++++
>  2 files changed, 136 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 915d87a0336..4fd1c0b5bce 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -36,6 +36,7 @@
>  #include "rebase-interactive.h"
>  #include "reset.h"
>  #include "branch.h"
> +#include "log-tree.h"
>
>  #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> @@ -148,6 +149,14 @@ static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
>   */
>  static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
>
> +/*
> + * The update-refs file stores a list of refs that will be updated at the end
> + * of the rebase sequence. The 'update-ref <ref>' commands in the todo file
> + * update the OIDs for the refs in this file, but the refs are not updated
> + * until the end of the rebase sequence.
> + */
> +static GIT_PATH_FUNC(rebase_path_update_refs, "rebase-merge/update-refs")
> +
>  /*
>   * The following files are written by git-rebase just after parsing the
>   * command-line.
> @@ -4058,11 +4067,110 @@ leave_merge:
>         return ret;
>  }
>
> -static int do_update_ref(struct repository *r, const char *ref_name)
> +static int write_update_refs_state(struct string_list *refs_to_oids)
> +{
> +       int result = 0;
> +       FILE *fp = NULL;
> +       struct string_list_item *item;
> +       char *path = xstrdup(rebase_path_update_refs());
> +
> +       if (safe_create_leading_directories(path)) {
> +               result = error(_("unable to create leading directories of %s"),
> +                              path);
> +               goto cleanup;
> +       }
> +
> +       fp = fopen(path, "w");
> +       if (!fp) {
> +               result = error_errno(_("could not open '%s' for writing"), path);
> +               goto cleanup;
> +       }
> +
> +       for_each_string_list_item(item, refs_to_oids)
> +               fprintf(fp, "%s\n%s\n", item->string, oid_to_hex(item->util));

Here you are storing the ref and the new oid to move it to.  Any
reason you don't store the previous oid for the branch?  In
particular, if someone hits a conflict, needs to resolve, and comes
back some time much later and these intermediate branches have since
moved on, should we actually update those branches?  (And, if we do,
should we at least give a warning?)

> +cleanup:
> +       if (fp)
> +               fclose(fp);
> +       return result;
> +}
> +
> +static int do_update_ref(struct repository *r, const char *refname)
>  {
> +       struct string_list_item *item;
> +       struct string_list list = STRING_LIST_INIT_DUP;
> +       int found = 0;
> +
> +       sequencer_get_update_refs_state(r->gitdir, &list);
> +
> +       for_each_string_list_item(item, &list) {
> +               if (!strcmp(item->string, refname)) {
> +                       struct object_id oid;
> +                       free(item->util);
> +                       found = 1;
> +
> +                       if (!read_ref("HEAD", &oid)) {
> +                               item->util = oiddup(&oid);
> +                               break;
> +                       }
> +               }
> +       }
> +
> +       if (!found) {
> +               struct object_id oid;
> +               item = string_list_append(&list, refname);
> +
> +               if (!read_ref("HEAD", &oid))
> +                       item->util = oiddup(&oid);
> +               else
> +                       item->util = oiddup(the_hash_algo->null_oid);

Seems a little unfortunate to not use a cached value from the latest
commit we picked (and wrote to HEAD) and instead need to re-read HEAD.
But, I guess sequencer is written to round-trip through files, so
maybe optimizing this here doesn't make sense given all the other
places in sequencer where we do lots of file reading and writing.

> +       }
> +
> +       write_update_refs_state(&list);
> +       string_list_clear(&list, 1);
>         return 0;
>  }
>
> +static int do_update_refs(struct repository *r)
> +{
> +       int res = 0;
> +       struct string_list_item *item;
> +       struct string_list refs_to_oids = STRING_LIST_INIT_DUP;
> +       struct ref_store *refs = get_main_ref_store(r);
> +
> +       sequencer_get_update_refs_state(r->gitdir, &refs_to_oids);
> +
> +       for_each_string_list_item(item, &refs_to_oids) {
> +               struct object_id *oid_to = item->util;
> +               struct object_id oid_from;
> +
> +               if (oideq(oid_to, the_hash_algo->null_oid)) {
> +                       /*
> +                        * Ref was not updated. User may have deleted the
> +                        * 'update-ref' step.
> +                        */
> +                       continue;
> +               }
> +
> +               if (read_ref(item->string, &oid_from)) {
> +                       /*
> +                        * The ref does not exist. The user probably
> +                        * inserted a new 'update-ref' step with a new
> +                        * branch name.
> +                        */
> +                       oidcpy(&oid_from, the_hash_algo->null_oid);
> +               }
> +
> +               res |= refs_update_ref(refs, "rewritten during rebase",
> +                               item->string,
> +                               oid_to, &oid_from,
> +                               0, UPDATE_REFS_MSG_ON_ERR);
> +       }
> +
> +       string_list_clear(&refs_to_oids, 1);
> +       return res;
> +}
> +
>  static int is_final_fixup(struct todo_list *todo_list)
>  {
>         int i = todo_list->current;
> @@ -4580,6 +4688,8 @@ cleanup_head_ref:
>                 strbuf_release(&head_ref);
>         }
>
> +       do_update_refs(r);
> +
>         /*
>          * Sequence of picks finished successfully; cleanup by
>          * removing the .git/sequencer directory
> @@ -5709,6 +5819,8 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
>                 }
>         }
>
> +       write_update_refs_state(&ctx.refs_to_oids);
> +
>         string_list_clear(&ctx.refs_to_oids, 1);
>         free(todo_list->items);
>         todo_list->items = ctx.items;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 3cd20733bc8..63a69bc073e 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1813,6 +1813,29 @@ test_expect_success '--update-refs adds commands with --rebase-merges' '
>         )
>  '
>
> +compare_two_refs () {
> +       git rev-parse $1 >expect &&
> +       git rev-parse $2 >actual &&
> +       test_cmp expect actual
> +}
> +
> +test_expect_success '--update-refs updates refs correctly' '
> +       git checkout -B update-refs no-conflict-branch &&
> +       git branch -f base HEAD~4 &&
> +       git branch -f first HEAD~3 &&
> +       git branch -f second HEAD~3 &&
> +       git branch -f third HEAD~1 &&
> +       test_commit extra2 fileX &&
> +       git commit --amend --fixup=L &&
> +
> +       git rebase -i --autosquash --update-refs primary &&
> +
> +       compare_two_refs HEAD~3 refs/heads/first &&
> +       compare_two_refs HEAD~3 refs/heads/second &&
> +       compare_two_refs HEAD~1 refs/heads/third &&
> +       compare_two_refs HEAD refs/heads/no-conflict-branch
> +'
> +
>  # This must be the last test in this file
>  test_expect_success '$EDITOR and friends are unchanged' '
>         test_editor_unchanged
> --
> gitgitgadget
>

Derrick Stolee July 4, 2022, 1:05 p.m. UTC | #8

On 7/1/22 5:18 PM, Elijah Newren wrote:
> On Tue, Jun 28, 2022 at 6:26 AM Derrick Stolee via GitGitGadget

>> Not only do we update the file when the sequencer reaches these
>> 'update-ref' commands, we then update the refs themselves at the end of
>> the rebase sequence. If the rebase is aborted before this final step,
>> then the refs are not updated.
> 
> Why update with each update-ref command?  Couldn't the values be
> stored in memory and only written when we hit a conflict?

I think that is a natural way to optimize the feature. I didn't
look into that option as it seemed more error-prone to me. I'd
be happy to be wrong here if an easy tweak makes this possible.

>> -static int do_update_ref(struct repository *r, const char *ref_name)
>> +static int write_update_refs_state(struct string_list *refs_to_oids)
>> +{
>> +       int result = 0;
>> +       FILE *fp = NULL;
>> +       struct string_list_item *item;
>> +       char *path = xstrdup(rebase_path_update_refs());
>> +
>> +       if (safe_create_leading_directories(path)) {
>> +               result = error(_("unable to create leading directories of %s"),
>> +                              path);
>> +               goto cleanup;
>> +       }
>> +
>> +       fp = fopen(path, "w");
>> +       if (!fp) {
>> +               result = error_errno(_("could not open '%s' for writing"), path);
>> +               goto cleanup;
>> +       }
>> +
>> +       for_each_string_list_item(item, refs_to_oids)
>> +               fprintf(fp, "%s\n%s\n", item->string, oid_to_hex(item->util));
> 
> Here you are storing the ref and the new oid to move it to.  Any
> reason you don't store the previous oid for the branch?  In
> particular, if someone hits a conflict, needs to resolve, and comes
> back some time much later and these intermediate branches have since
> moved on, should we actually update those branches?  (And, if we do,
> should we at least give a warning?)

The branches don't move forward because other Git processes respect
the update-refs file (this does not take into account third-party
tools that don't know about that file, but we need to start somewhere).

You're right that storing the old value would help us in this case
where another tool updated the ref while we were in the middle of the
rebase. The storage of the null OID is only to prevent writing over a
ref when the user has removed the 'update-ref <ref>' command from the
todo-list. It's probably better to remove the ref from the list when
that happens.

>> +       if (!found) {
>> +               struct object_id oid;
>> +               item = string_list_append(&list, refname);
>> +
>> +               if (!read_ref("HEAD", &oid))
>> +                       item->util = oiddup(&oid);
>> +               else
>> +                       item->util = oiddup(the_hash_algo->null_oid);
> 
> Seems a little unfortunate to not use a cached value from the latest
> commit we picked (and wrote to HEAD) and instead need to re-read HEAD.
> But, I guess sequencer is written to round-trip through files, so
> maybe optimizing this here doesn't make sense given all the other
> places in sequencer where we do lots of file reading and writing.

It's also possible to optimize several things, but I tried to
minimize the amount of change to the existing methods. This is
my first foray into this part of the code, so I don't always know
which information is readily available. I appreciate your attention
here.

Thanks,
-Stolee

Patch

diff --git a/sequencer.c b/sequencer.c
index 915d87a0336..4fd1c0b5bce 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -36,6 +36,7 @@ 
 #include "rebase-interactive.h"
 #include "reset.h"
 #include "branch.h"
+#include "log-tree.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -148,6 +149,14 @@  static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
  */
 static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
 
+/*
+ * The update-refs file stores a list of refs that will be updated at the end
+ * of the rebase sequence. The 'update-ref <ref>' commands in the todo file
+ * update the OIDs for the refs in this file, but the refs are not updated
+ * until the end of the rebase sequence.
+ */
+static GIT_PATH_FUNC(rebase_path_update_refs, "rebase-merge/update-refs")
+
 /*
  * The following files are written by git-rebase just after parsing the
  * command-line.
@@ -4058,11 +4067,110 @@  leave_merge:
 	return ret;
 }
 
-static int do_update_ref(struct repository *r, const char *ref_name)
+static int write_update_refs_state(struct string_list *refs_to_oids)
+{
+	int result = 0;
+	FILE *fp = NULL;
+	struct string_list_item *item;
+	char *path = xstrdup(rebase_path_update_refs());
+
+	if (safe_create_leading_directories(path)) {
+		result = error(_("unable to create leading directories of %s"),
+			       path);
+		goto cleanup;
+	}
+
+	fp = fopen(path, "w");
+	if (!fp) {
+		result = error_errno(_("could not open '%s' for writing"), path);
+		goto cleanup;
+	}
+
+	for_each_string_list_item(item, refs_to_oids)
+		fprintf(fp, "%s\n%s\n", item->string, oid_to_hex(item->util));
+
+cleanup:
+	if (fp)
+		fclose(fp);
+	return result;
+}
+
+static int do_update_ref(struct repository *r, const char *refname)
 {
+	struct string_list_item *item;
+	struct string_list list = STRING_LIST_INIT_DUP;
+	int found = 0;
+
+	sequencer_get_update_refs_state(r->gitdir, &list);
+
+	for_each_string_list_item(item, &list) {
+		if (!strcmp(item->string, refname)) {
+			struct object_id oid;
+			free(item->util);
+			found = 1;
+
+			if (!read_ref("HEAD", &oid)) {
+				item->util = oiddup(&oid);
+				break;
+			}
+		}
+	}
+
+	if (!found) {
+		struct object_id oid;
+		item = string_list_append(&list, refname);
+
+		if (!read_ref("HEAD", &oid))
+			item->util = oiddup(&oid);
+		else
+			item->util = oiddup(the_hash_algo->null_oid);
+	}
+
+	write_update_refs_state(&list);
+	string_list_clear(&list, 1);
 	return 0;
 }
 
+static int do_update_refs(struct repository *r)
+{
+	int res = 0;
+	struct string_list_item *item;
+	struct string_list refs_to_oids = STRING_LIST_INIT_DUP;
+	struct ref_store *refs = get_main_ref_store(r);
+
+	sequencer_get_update_refs_state(r->gitdir, &refs_to_oids);
+
+	for_each_string_list_item(item, &refs_to_oids) {
+		struct object_id *oid_to = item->util;
+		struct object_id oid_from;
+
+		if (oideq(oid_to, the_hash_algo->null_oid)) {
+			/*
+			 * Ref was not updated. User may have deleted the
+			 * 'update-ref' step.
+			 */
+			continue;
+		}
+
+		if (read_ref(item->string, &oid_from)) {
+			/*
+			 * The ref does not exist. The user probably
+			 * inserted a new 'update-ref' step with a new
+			 * branch name.
+			 */
+			oidcpy(&oid_from, the_hash_algo->null_oid);
+		}
+
+		res |= refs_update_ref(refs, "rewritten during rebase",
+				item->string,
+				oid_to, &oid_from,
+				0, UPDATE_REFS_MSG_ON_ERR);
+	}
+
+	string_list_clear(&refs_to_oids, 1);
+	return res;
+}
+
 static int is_final_fixup(struct todo_list *todo_list)
 {
 	int i = todo_list->current;
@@ -4580,6 +4688,8 @@  cleanup_head_ref:
 		strbuf_release(&head_ref);
 	}
 
+	do_update_refs(r);
+
 	/*
 	 * Sequence of picks finished successfully; cleanup by
 	 * removing the .git/sequencer directory
@@ -5709,6 +5819,8 @@  static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
 		}
 	}
 
+	write_update_refs_state(&ctx.refs_to_oids);
+
 	string_list_clear(&ctx.refs_to_oids, 1);
 	free(todo_list->items);
 	todo_list->items = ctx.items;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3cd20733bc8..63a69bc073e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1813,6 +1813,29 @@  test_expect_success '--update-refs adds commands with --rebase-merges' '
 	)
 '
 
+compare_two_refs () {
+	git rev-parse $1 >expect &&
+	git rev-parse $2 >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success '--update-refs updates refs correctly' '
+	git checkout -B update-refs no-conflict-branch &&
+	git branch -f base HEAD~4 &&
+	git branch -f first HEAD~3 &&
+	git branch -f second HEAD~3 &&
+	git branch -f third HEAD~1 &&
+	test_commit extra2 fileX &&
+	git commit --amend --fixup=L &&
+
+	git rebase -i --autosquash --update-refs primary &&
+
+	compare_two_refs HEAD~3 refs/heads/first &&
+	compare_two_refs HEAD~3 refs/heads/second &&
+	compare_two_refs HEAD~1 refs/heads/third &&
+	compare_two_refs HEAD refs/heads/no-conflict-branch
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged