From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 01 Jul 2022 09:31:05 GMT
Subject: Re: [PATCH v3 7/8] rebase: update refs from 'update-ref' commands
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:
> 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.

> 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

> +	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