Re: [PATCH v3 7/8] rebase: update refs from 'update-ref' commands
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 1, 2022, 09:31 UTC
- Message-ID
- <bee9eb82-6c6a-6467-37d1-d3797c583929@gmail.com>
- In-Reply-To
- <72e0481b643e98c5670eee0bf5c199c4fb693b16.1656422759.git.gitgitgadget@gmail.com>
Hi Stolee
On 28/06/2022 14:25, Derrick Stolee via GitGitGadget wrote:
Show 16 quoted lines
> From: Derrick Stolee <derrickstolee@github.com> > > 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.
Show 44 quoted lines
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> 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
Show 11 quoted lines
> + 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.
Show 9 quoted lines
> + > + 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; > +}
Show 5 quoted lines
> +compare_two_refs () {
> + git rev-parse $1 >expect &&
> + git rev-parse $2 >actual &&
> + test_cmp expect actual
> +}This is just test_cmp_rev
Show 16 quoted lines
> +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