From: Phillip Wood <phillip.wood123@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Cc: gitster@pobox.com, johannes.schindelin@gmx.de, me@ttaylorr.com,
Jeff Hostetler <git@jeffhostetler.com>,
Elijah Newren <newren@gmail.com>,
Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v4 00/12] rebase: update branches in multi-part topic
Date: Fri, 15 Jul 2022 16:41:02 +0100 [thread overview]
Message-ID: <e620bb51-6c58-cfbb-936b-be46c2f5216a@gmail.com> (raw)
In-Reply-To: <pull.1247.v4.git.1657631225.gitgitgadget@gmail.com>
On 12/07/2022 14:06, Derrick Stolee via GitGitGadget wrote:
I haven't looked at the first two patches but I think I read everything
else that has changed in V4. It all looks good to me. I've left some
comments on the later patches but I don't think there are any
showstoppers and this version could be merged into next.
Thanks for working on this
Phillip
> This series is based on ds/branch-checked-out.
>
> This is a feature I've wanted for quite a while. When working on the sparse
> index topic, I created a long RFC that actually broke into three topics for
> full review upstream. These topics were sequential, so any feedback on an
> earlier one required updates to the later ones. I would work on the full
> feature and use interactive rebase to update the full list of commits.
> However, I would need to update the branches pointing to those sub-topics.
>
> This series adds a new --update-refs option to 'git rebase' (along with a
> rebase.updateRefs config option) that adds 'update-ref' commands into the
> TODO list. This is powered by the commit decoration machinery.
>
> As an example, here is my in-progress bundle URI RFC split into subtopics as
> they appear during the TODO list of a git rebase -i --update-refs:
>
> pick 2d966282ff3 docs: document bundle URI standard
> pick 31396e9171a remote-curl: add 'get' capability
> pick 54c6ab70f67 bundle-uri: create basic file-copy logic
> pick 96cb2e35af1 bundle-uri: add support for http(s):// and file://
> pick 6adaf842684 fetch: add --bundle-uri option
> pick 6c5840ed77e fetch: add 'refs/bundle/' to log.excludeDecoration
> update-ref refs/heads/bundle-redo/fetch
>
> pick 1e3f6546632 clone: add --bundle-uri option
> pick 9e4a6fe9b68 clone: --bundle-uri cannot be combined with --depth
> update-ref refs/heads/bundle-redo/clone
>
> pick 5451cb6599c bundle-uri: create bundle_list struct and helpers
> pick 3029c3aca15 bundle-uri: create base key-value pair parsing
> pick a8b2de79ce8 bundle-uri: create "key=value" line parsing
> pick 92625a47673 bundle-uri: unit test "key=value" parsing
> pick a8616af4dc2 bundle-uri: limit recursion depth for bundle lists
> pick 9d6809a8d53 bundle-uri: parse bundle list in config format
> pick 287a732b54c bundle-uri: fetch a list of bundles
> update-ref refs/heads/bundle-redo/list
>
> pick b09f8226185 protocol v2: add server-side "bundle-uri" skeleton
> pick 520204dcd1c bundle-uri client: add minimal NOOP client
> pick 62e8b457b48 bundle-uri client: add "git ls-remote-bundle-uri"
> pick 00eae925043 bundle-uri: serve URI advertisement from bundle.* config
> pick 4277440a250 bundle-uri client: add boolean transfer.bundleURI setting
> pick caf4599a81d bundle-uri: allow relative URLs in bundle lists
> pick df255000b7e bundle-uri: download bundles from an advertised list
> pick d71beabf199 clone: unbundle the advertised bundles
> pick c9578391976 t5601: basic bundle URI tests
> # Ref refs/heads/bundle-redo/rfc-3 checked out at '/home/stolee/_git/git-bundles'
>
> update-ref refs/heads/bundle-redo/advertise
>
>
> Here is an outline of the series:
>
> * Patch 1 updates some tests for branch_checked_out() to use 'git bisect'
> and 'git rebase' as black-boxes instead of manually editing files inside
> $GIT_DIR. (Thanks, Junio!)
> * Patch 2 updates some tests for branch_checked_out() for the 'apply'
> backend.
> * Patch 3 updates branch_checked_out() to parse the
> rebase-merge/update-refs file to block concurrent ref updates and
> checkouts on branches selected by --update-refs.
> * Patch 4 updates the todo list documentation to remove some unnecessary
> dots in the 'merge' command. This makes it consistent with the 'fixup'
> command before we document the 'update-ref' command.
> * Patch 5 updates the definition of todo_command_info to use enum values as
> array indices.
> * Patches 6-8 implement the --update-refs logic itself.
> * Patch 9 specifically updates the update-refs file every time the user
> edits the todo-list (Thanks Phillip!)
> * Patch 10 adds the rebase.updateRefs config option similar to
> rebase.autoSquash.
> * Patch 11 ignores the HEAD ref when creating the todo list instead of
> making a comment (Thanks Elijah!)
> * Patch 12 adds messaging to the end of the rebase stating which refs were
> updated (Thanks Elijah!)
>
>
> Updates in v4
> =============
>
> This version took longer than I'd hoped (I had less time to work on it than
> anticipated) but it also has some major updates. These major updates are
> direct responses to the significant review this series has received. Thank
> you!
>
> * The update-refs file now stores "ref/before/after" triples (still
> separated by lines). This allows us to store the "before" OID of a ref in
> addition to the "after" that we will write to that ref at the end of the
> rebase. This allows us to do a "force-with-lease" update. The
> branch_checked_out() updates should prevent Git from updating those refs
> while under the rebase, but older versions and third-party tools don't
> have that protection.
> * The update-refs file is updated with every update to the todo-list file.
> This allows for some advanced changes to the file, including removing,
> adding, and duplicating 'update-ref' commands.
> * The message at the end of the rebase process now lists which refs were
> updated with the update-ref steps. This includes any ref updates that
> fail.
> * The branch_checked_out() tests now use 'git bisect' and 'git rebase' as
> black-boxes instead of testing their internals directly.
>
> Here are the more minor updates:
>
> * Dropped an unnecessary stat() call.
> * Updated commit messages to include extra details, based on confusion in
> last round.
> * The HEAD branch no longer appears as a comment line in the initial todo
> list.
> * The update-refs file is now written using a lockfile.
> * Tests now use test_cmp_rev.
> * A memory leak ('path' variable) is resolved.
>
>
> Updates in v3
> =============
>
> * The branch_checked_out() API was extracted to its own topic and is now
> the ds/branch-checked-out branch. This series is now based on that one.
> * The for_each_decoration() API was removed, since it became trivial once
> it did not take a commit directly.
> * The branch_checked_out() tests did not verify the rebase-apply data (for
> the apply backend), so that is fixed.
> * Instead of using the 'label' command and a final 'update-refs' command in
> the todo list, use a new 'update-ref ' command. This command updates the
> rebase-merge/update-refs file with the OID of HEAD at these steps. At the
> very end of the rebase sequence, those refs are updated to the stored OID
> values (assuming that they were not removed by the user, in which case we
> notice that the OID is the null OID and we do nothing).
> * New tests are added.
> * The todo-list comment documentation has some new formatting updates, but
> also includes a description of 'update-refs' in this version.
>
>
> Updates in v2
> =============
>
> As recommended by the excellent feedback, I have removed the 'exec' commands
> in favor of the 'label' commands and a new 'update-refs' command at the very
> end. This way, there is only one step that updates all of the refs at the
> end instead of updating refs during the rebase. If a user runs 'git rebase
> --abort' in the middle, then their refs are still where they need to be.
>
> Based on some of the discussion, it seemed like one way to do this would be
> to have an 'update-ref ' command that would take the place of these 'label'
> commands. However, this would require two things that make it a bit awkward:
>
> 1. We would need to replicate the storage of those positions during the
> rebase. 'label' already does this pretty well. I've added the
> "for-update-refs/" label to help here.
> 2. If we want to close out all of the refs as the rebase is finishing, then
> that "step" becomes invisible to the user (and a bit more complicated to
> insert). Thus, the 'update-refs' step performs this action. If the user
> wants to do things after that step, then they can do so by editing the
> TODO list.
>
> Other updates:
>
> * The 'keep_decorations' parameter was renamed to 'update_refs'.
> * I added tests for --rebase-merges=rebase-cousins to show how these labels
> interact with other labels and merge commands.
> * I changed the order of the insertion of these update-refs labels to be
> before the fixups are rearranged. This fixes a bug where the tip commit
> is a fixup! so its decorations are never inspected (and they would be in
> the wrong place even if they were). The fixup! commands are properly
> inserted between a pick and its following label command. Tests
> demonstrate this is correct.
> * Numerous style choices are updated based on feedback.
>
> Thank you for all of the detailed review and ideas in this space. I
> appreciate any more ideas that can make this feature as effective as it can
> be.
>
> Thanks, -Stolee
>
> Derrick Stolee (12):
> t2407: test bisect and rebase as black-boxes
> t2407: test branches currently using apply backend
> branch: consider refs under 'update-refs'
> rebase-interactive: update 'merge' description
> sequencer: define array with enum values
> sequencer: add update-ref command
> rebase: add --update-refs option
> rebase: update refs from 'update-ref' commands
> sequencer: rewrite update-refs as user edits todo list
> rebase: add rebase.updateRefs config option
> sequencer: ignore HEAD ref under --update-refs
> sequencer: notify user of --update-refs activity
>
> Documentation/config/rebase.txt | 3 +
> Documentation/git-rebase.txt | 11 +
> branch.c | 13 +
> builtin/rebase.c | 10 +
> rebase-interactive.c | 15 +-
> sequencer.c | 469 +++++++++++++++++++++++++++++++-
> sequencer.h | 23 ++
> t/lib-rebase.sh | 15 +
> t/t2407-worktree-heads.sh | 103 +++++--
> t/t3404-rebase-interactive.sh | 269 ++++++++++++++++++
> 10 files changed, 887 insertions(+), 44 deletions(-)
>
>
> base-commit: 9bef0b1e6ec371e786c2fba3edcc06ad040a536c
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1247%2Fderrickstolee%2Frebase-keep-decorations-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1247/derrickstolee/rebase-keep-decorations-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1247
>
> Range-diff vs v3:
>
> -: ----------- > 1: 9e53a27017a t2407: test bisect and rebase as black-boxes
> 1: fbaedc7f1f0 ! 2: 540a3be256f t2407: test branches currently using apply backend
> @@ Commit message
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>
> ## t/t2407-worktree-heads.sh ##
> -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in bisect' '
> - grep "cannot force update the branch '\''fake-2'\'' checked out at.*wt-4" err
> +@@ t/t2407-worktree-heads.sh: test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in bisect' '
> + grep "cannot force update the branch '\''wt-4'\'' checked out at.*wt-4" err
> '
>
> --test_expect_success 'refuse to overwrite: worktree in rebase' '
> -+test_expect_success 'refuse to overwrite: worktree in rebase (apply)' '
> -+ test_when_finished rm -rf .git/worktrees/wt-*/rebase-apply &&
> +-test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase' '
> ++test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase (apply)' '
> ++ test_when_finished git -C wt-2 rebase --abort &&
> +
> -+ mkdir -p .git/worktrees/wt-3/rebase-apply &&
> -+ echo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-apply/head-name &&
> -+ echo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-apply/onto &&
> ++ # This will fail part-way through due to a conflict.
> ++ test_must_fail git -C wt-2 rebase --apply conflict-2 &&
> +
> -+ test_must_fail git branch -f fake-1 HEAD 2>err &&
> -+ grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
> ++ test_must_fail git branch -f wt-2 HEAD 2>err &&
> ++ grep "cannot force update the branch '\''wt-2'\'' checked out at.*wt-2" err
> +'
> +
> -+test_expect_success 'refuse to overwrite: worktree in rebase (merge)' '
> - test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
> ++test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase (merge)' '
> + test_when_finished git -C wt-2 rebase --abort &&
>
> - mkdir -p .git/worktrees/wt-3/rebase-merge &&
> + # This will fail part-way through due to a conflict.
> 2: 2bc647b6fcd ! 3: bf301a054e3 branch: consider refs under 'update-refs'
> @@ Commit message
> branch_checked_out().
>
> The data store is a plaintext file inside the 'rebase-merge' directory
> - for that worktree. The file alternates refnames and OIDs. The OIDs will
> - be used to store the to-be-written values as the rebase progresses, but
> - can be ignored at the moment.
> + for that worktree. The file lists refnames followed by two OIDs, each on
> + separate lines. The OIDs will be used to store the original values of
> + the refs and the to-be-written values as the rebase progresses, but can
> + be ignored at the moment.
>
> Create a new sequencer_get_update_refs_state() method that parses this
> file and populates a struct string_list with the ref-OID pairs. We can
> @@ Commit message
> method.
>
> We can test that this works without having Git write this file by
> - artificially creating one in our test script.
> + artificially creating one in our test script, at least until 'git rebase
> + --update-refs' is implemented and we can use it directly.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>
> @@ branch.c: static void prepare_checked_out_branches(void)
> free_worktrees(worktrees);
>
> ## sequencer.c ##
> +@@ sequencer.c: 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.
> ++ *
> ++ * rebase_path_update_refs() returns the path to this file for a given
> ++ * worktree directory. For the current worktree, pass the_repository->gitdir.
> ++ */
> ++static char *rebase_path_update_refs(const char *wt_dir)
> ++{
> ++ return xstrfmt("%s/rebase-merge/update-refs", wt_dir);
> ++}
> ++
> + /*
> + * The following files are written by git-rebase just after parsing the
> + * command-line.
> +@@ sequencer.c: static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-res
> + static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
> + static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
> +
> ++/**
> ++ * A 'struct update_refs_record' represents a value in the update-refs
> ++ * list. We use a string_list to map refs to these (before, after) pairs.
> ++ */
> ++struct update_ref_record {
> ++ struct object_id before;
> ++ struct object_id after;
> ++};
> ++
> + static int git_sequencer_config(const char *k, const char *v, void *cb)
> + {
> + struct replay_opts *opts = cb;
> @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
>
> return 0;
> @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_wh
> + struct string_list *refs)
> +{
> + int result = 0;
> -+ struct stat st;
> + FILE *fp = NULL;
> + struct strbuf ref = STRBUF_INIT;
> + struct strbuf hash = STRBUF_INIT;
> -+ char *path = xstrfmt("%s/rebase-merge/update-refs", wt_dir);
> ++ struct update_ref_record *rec = NULL;
> +
> -+ if (stat(path, &st))
> -+ goto cleanup;
> ++ char *path = rebase_path_update_refs(wt_dir);
> +
> + fp = fopen(path, "r");
> + if (!fp)
> + goto cleanup;
> +
> + while (strbuf_getline(&ref, fp) != EOF) {
> -+ struct object_id oid;
> + struct string_list_item *item;
> +
> ++ CALLOC_ARRAY(rec, 1);
> ++
> + if (strbuf_getline(&hash, fp) == EOF ||
> -+ get_oid_hex(hash.buf, &oid)) {
> ++ get_oid_hex(hash.buf, &rec->before)) {
> + warning(_("update-refs file at '%s' is invalid"),
> + path);
> + result = -1;
> + goto cleanup;
> + }
> +
> -+ item = string_list_append(refs, ref.buf);
> -+ item->util = oiddup(&oid);
> ++ if (strbuf_getline(&hash, fp) == EOF ||
> ++ get_oid_hex(hash.buf, &rec->after)) {
> ++ warning(_("update-refs file at '%s' is invalid"),
> ++ path);
> ++ result = -1;
> ++ goto cleanup;
> ++ }
> ++
> ++ item = string_list_insert(refs, ref.buf);
> ++ item->util = rec;
> ++ rec = NULL;
> + }
> +
> +cleanup:
> + if (fp)
> + fclose(fp);
> + free(path);
> ++ free(rec);
> + strbuf_release(&ref);
> + strbuf_release(&hash);
> + return result;
> @@ sequencer.h: void sequencer_post_commit_cleanup(struct repository *r, int verbos
> #endif /* SEQUENCER_H */
>
> ## t/t2407-worktree-heads.sh ##
> -@@ t/t2407-worktree-heads.sh: TEST_PASSES_SANITIZE_LEAK=true
> +@@ t/t2407-worktree-heads.sh: test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase (mer
> + grep "cannot force update the branch '\''wt-2'\'' checked out at.*wt-2" err
> + '
>
> - test_expect_success 'setup' '
> - test_commit init &&
> -- git branch -f fake-1 &&
> -- git branch -f fake-2 &&
> ++test_expect_success 'refuse to overwrite: worktree in rebase with --update-refs' '
> ++ test_when_finished rm -rf .git/worktrees/wt-3/rebase-merge &&
> ++
> ++ mkdir -p .git/worktrees/wt-3/rebase-merge &&
> ++ touch .git/worktrees/wt-3/rebase-merge/interactive &&
> +
> -+ for i in 1 2 3 4
> -+ do
> -+ git branch -f fake-$i || return 1
> -+ done &&
> -
> - for i in 1 2 3 4
> - do
> -@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite: worktree in rebase (merge)' '
> - echo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-merge/head-name &&
> - echo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-merge/onto &&
> -
> -- test_must_fail git branch -f fake-1 HEAD 2>err &&
> -- grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
> + cat >.git/worktrees/wt-3/rebase-merge/update-refs <<-EOF &&
> + refs/heads/fake-3
> + $(git rev-parse HEAD~1)
> ++ $(git rev-parse HEAD)
> + refs/heads/fake-4
> + $(git rev-parse HEAD)
> ++ $(git rev-parse HEAD)
> + EOF
> +
> -+ for i in 1 3 4
> ++ for i in 3 4
> + do
> + test_must_fail git branch -f fake-$i HEAD 2>err &&
> + grep "cannot force update the branch '\''fake-$i'\'' checked out at.*wt-3" err ||
> + return 1
> + done
> - '
> -
> ++'
> ++
> test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '
> + test_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&
> + grep "refusing to fetch into branch '\''refs/heads/wt-3'\''" err &&
> 3: 669f4abd59e ! 4: dec95681d2b rebase-interactive: update 'merge' description
> @@ Commit message
> The 'merge' command description for the todo list documentation in an
> interactive rebase has multiple lines. The lines other than the first
> one start with dots ('.') while the similar multi-line documentation for
> - 'fixup' does not.
> + 'fixup' does not. This description only appears in the comment text of
> + the todo file during an interactive rebase.
>
> The 'merge' command was documented when interactive rebase was first
> ported to C in 145e05ac44b (rebase -i: rewrite append_todo_help() in C,
> 4: 6528a50343f = 5: b2c09600918 sequencer: define array with enum values
> 5: e95ad41d355 = 6: fa7ecb718cf sequencer: add update-ref command
> 6: 918b398d6a2 ! 7: 3ec2cc922f9 rebase: add --update-refs option
> @@ Commit message
> 'update-ref' commands. Tests are added to ensure that these todo
> commands are added in the correct locations.
>
> - A future change will update the behavior to actually update the refs
> - at the end of the rebase sequence.
> + This change does _not_ include the actual behavior of tracking the
> + updated refs and writing the new ref values at the end of the rebase
> + process. That is deferred to a later change.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>
> @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
> + item->command = TODO_UPDATE_REF;
> + strbuf_addf(ctx->buf, "%s\n", decoration->name);
> +
> -+ sti = string_list_append(&ctx->refs_to_oids,
> ++ sti = string_list_insert(&ctx->refs_to_oids,
> + decoration->name);
> + sti->util = oiddup(the_hash_algo->null_oid);
> + }
> @@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite when in erro
> + grep "update-ref refs/heads/allow-update" todo
> + )
> +'
> ++
> ++# This must be the last test in this file
> ++test_expect_success '$EDITOR and friends are unchanged' '
> ++ test_editor_unchanged
> ++'
> +
> test_done
>
> 7: 72e0481b643 ! 8: fb5f64c5201 rebase: update refs from 'update-ref' commands
> @@ Commit message
> interactive rebase.
>
> Teach Git to record the HEAD position when reaching these 'update-ref'
> - commands. The ref/OID pair is stored in the
> + commands. The ref/before/after triple 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.
> @@ Commit message
> 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.
> + then the refs are not updated. The 'before' value is used to ensure that
> + we do not accidentally obliterate a ref that was updated concurrently
> + (say, by an older version of Git or a third-party tool).
> +
> + Now that the 'git rebase --update-refs' command is implemented to write
> + to the update-refs file, we can remove the fake construction of the
> + update-refs file from a test in t2407-worktree-heads.sh.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>
> @@ sequencer.c
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> -@@ sequencer.c: 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")
> +@@ sequencer.c: struct update_ref_record {
> + struct object_id after;
> + };
>
> -+/*
> -+ * 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.
> ++static struct update_ref_record *init_update_ref_record(const char *ref)
> ++{
> ++ struct update_ref_record *rec = xmalloc(sizeof(*rec));
> ++
> ++ oidcpy(&rec->before, null_oid());
> ++ oidcpy(&rec->after, null_oid());
> ++
> ++ /* This may fail, but that's fine, we will keep the null OID. */
> ++ read_ref(ref, &rec->before);
> ++
> ++ return rec;
> ++}
> ++
> + static int git_sequencer_config(const char *k, const char *v, void *cb)
> + {
> + struct replay_opts *opts = cb;
> @@ sequencer.c: 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;
> ++ struct lock_file lock = LOCK_INIT;
> + FILE *fp = NULL;
> + struct string_list_item *item;
> -+ char *path = xstrdup(rebase_path_update_refs());
> ++ char *path;
> ++
> ++ if (!refs_to_oids->nr)
> ++ return 0;
> ++
> ++ path = rebase_path_update_refs(the_repository->gitdir);
> +
> + if (safe_create_leading_directories(path)) {
> + result = error(_("unable to create leading directories of %s"),
> @@ sequencer.c: leave_merge:
> + goto cleanup;
> + }
> +
> -+ fp = fopen(path, "w");
> ++ if (hold_lock_file_for_update(&lock, path, 0) < 0) {
> ++ result = error(_("another 'rebase' process appears to be running; "
> ++ "'%s.lock' already exists"),
> ++ path);
> ++ goto cleanup;
> ++ }
> ++
> ++ fp = fdopen_lock_file(&lock, "w");
> + if (!fp) {
> + result = error_errno(_("could not open '%s' for writing"), path);
> ++ rollback_lock_file(&lock);
> + goto cleanup;
> + }
> +
> -+ for_each_string_list_item(item, refs_to_oids)
> -+ fprintf(fp, "%s\n%s\n", item->string, oid_to_hex(item->util));
> ++ for_each_string_list_item(item, refs_to_oids) {
> ++ struct update_ref_record *rec = item->util;
> ++ fprintf(fp, "%s\n%s\n%s\n", item->string,
> ++ oid_to_hex(&rec->before), oid_to_hex(&rec->after));
> ++ }
> ++
> ++ result = commit_lock_file(&lock);
> +
> +cleanup:
> -+ if (fp)
> -+ fclose(fp);
> ++ free(path);
> + 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;
> -+ }
> ++ struct update_ref_record *rec = item->util;
> ++ read_ref("HEAD", &rec->after);
> ++ 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;
> @@ sequencer.c: leave_merge:
> + 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;
> ++ struct update_ref_record *rec = item->util;
> +
> -+ if (oideq(oid_to, the_hash_algo->null_oid)) {
> ++ if (oideq(&rec->after, the_hash_algo->null_oid)) {
> + /*
> + * Ref was not updated. User may have deleted the
> + * 'update-ref' step.
> @@ sequencer.c: leave_merge:
> + 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);
> ++ item->string,
> ++ &rec->after, &rec->before,
> ++ 0, UPDATE_REFS_MSG_ON_ERR);
> + }
> +
> + string_list_clear(&refs_to_oids, 1);
> @@ sequencer.c: cleanup_head_ref:
> /*
> * Sequence of picks finished successfully; cleanup by
> * removing the .git/sequencer directory
> +@@ sequencer.c: static int add_decorations_to_list(const struct commit *commit,
> +
> + sti = string_list_insert(&ctx->refs_to_oids,
> + decoration->name);
> +- sti->util = oiddup(the_hash_algo->null_oid);
> ++ sti->util = init_update_ref_record(decoration->name);
> + }
> +
> + item->offset_in_buf = base_offset;
> @@ sequencer.c: static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
> }
> }
> @@ sequencer.c: static int todo_list_add_update_ref_commands(struct todo_list *todo
> free(todo_list->items);
> todo_list->items = ctx.items;
>
> + ## t/t2407-worktree-heads.sh ##
> +@@ t/t2407-worktree-heads.sh: test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase (mer
> + grep "cannot force update the branch '\''wt-2'\'' checked out at.*wt-2" err
> + '
> +
> +-test_expect_success 'refuse to overwrite: worktree in rebase with --update-refs' '
> +- test_when_finished rm -rf .git/worktrees/wt-3/rebase-merge &&
> +-
> +- mkdir -p .git/worktrees/wt-3/rebase-merge &&
> +- touch .git/worktrees/wt-3/rebase-merge/interactive &&
> ++test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase with --update-refs' '
> ++ test_when_finished git -C wt-3 rebase --abort &&
> +
> +- cat >.git/worktrees/wt-3/rebase-merge/update-refs <<-EOF &&
> +- refs/heads/fake-3
> +- $(git rev-parse HEAD~1)
> +- $(git rev-parse HEAD)
> +- refs/heads/fake-4
> +- $(git rev-parse HEAD)
> +- $(git rev-parse HEAD)
> +- EOF
> ++ git branch -f can-be-updated wt-3 &&
> ++ test_must_fail git -C wt-3 rebase --update-refs conflict-3 &&
> +
> + for i in 3 4
> + do
> +- test_must_fail git branch -f fake-$i HEAD 2>err &&
> +- grep "cannot force update the branch '\''fake-$i'\'' checked out at.*wt-3" err ||
> ++ test_must_fail git branch -f can-be-updated HEAD 2>err &&
> ++ grep "cannot force update the branch '\''can-be-updated'\'' checked out at.*wt-3" err ||
> + return 1
> + done
> + '
> +
> ## t/t3404-rebase-interactive.sh ##
> @@ t/t3404-rebase-interactive.sh: 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 &&
> @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs adds commands
> +
> + 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
> ++ test_cmp_rev HEAD~3 refs/heads/first &&
> ++ test_cmp_rev HEAD~3 refs/heads/second &&
> ++ test_cmp_rev HEAD~1 refs/heads/third &&
> ++ test_cmp_rev HEAD refs/heads/no-conflict-branch
> +'
> +
> # This must be the last test in this file
> -: ----------- > 9: 29c7c76805a sequencer: rewrite update-refs as user edits todo list
> 8: d2cfdbfc431 = 10: c0022d07579 rebase: add rebase.updateRefs config option
> -: ----------- > 11: d53b4ff2cee sequencer: ignore HEAD ref under --update-refs
> -: ----------- > 12: d5cd4b49e46 sequencer: notify user of --update-refs activity
>
next prev parent reply other threads:[~2022-07-15 15:41 UTC|newest]
Thread overview: 144+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-03 13:37 [PATCH 0/4] rebase: update branches in multi-part topic Derrick Stolee via GitGitGadget
2022-06-03 13:37 ` [PATCH 1/4] log-tree: create for_each_decoration() Derrick Stolee via GitGitGadget
2022-06-03 17:39 ` Junio C Hamano
2022-06-03 17:58 ` Derrick Stolee
2022-06-03 18:40 ` Junio C Hamano
2022-06-03 13:37 ` [PATCH 2/4] branch: add branch_checked_out() helper Derrick Stolee via GitGitGadget
2022-06-03 18:31 ` Junio C Hamano
2022-06-03 13:37 ` [PATCH 3/4] rebase: add --update-refs option Derrick Stolee via GitGitGadget
2022-06-07 10:25 ` Phillip Wood
2022-06-03 13:37 ` [PATCH 4/4] rebase: add rebase.updateRefs config option Derrick Stolee via GitGitGadget
2022-06-03 16:56 ` [PATCH 0/4] rebase: update branches in multi-part topic Junio C Hamano
2022-06-03 18:27 ` Taylor Blau
2022-06-03 18:52 ` Junio C Hamano
2022-06-03 19:59 ` Jeff Hostetler
2022-06-03 20:03 ` Taylor Blau
2022-06-03 21:23 ` Junio C Hamano
2022-06-04 15:28 ` Phillip Wood
2022-06-06 15:12 ` Derrick Stolee
2022-06-07 10:11 ` Phillip Wood
2022-06-07 19:39 ` Derrick Stolee
2022-06-08 16:03 ` Junio C Hamano
2022-06-06 16:36 ` Junio C Hamano
2022-06-07 6:25 ` Elijah Newren
2022-06-07 20:42 ` [PATCH v2 0/7] " Derrick Stolee via GitGitGadget
2022-06-07 20:42 ` [PATCH v2 1/7] log-tree: create for_each_decoration() Derrick Stolee via GitGitGadget
2022-06-07 20:42 ` [PATCH v2 2/7] branch: add branch_checked_out() helper Derrick Stolee via GitGitGadget
2022-06-07 22:09 ` Junio C Hamano
2022-06-08 2:14 ` Derrick Stolee
2022-06-08 2:43 ` Derrick Stolee
2022-06-08 4:52 ` Junio C Hamano
2022-06-07 20:42 ` [PATCH v2 3/7] sequencer: define array with enum values Derrick Stolee via GitGitGadget
2022-06-07 22:11 ` Junio C Hamano
2022-06-07 20:42 ` [PATCH v2 4/7] sequencer: add update-refs command Derrick Stolee via GitGitGadget
2022-06-07 20:42 ` [PATCH v2 5/7] rebase: add --update-refs option Derrick Stolee via GitGitGadget
2022-06-07 20:42 ` [PATCH v2 6/7] sequencer: implement 'update-refs' command Derrick Stolee via GitGitGadget
2022-06-07 22:23 ` Junio C Hamano
2022-06-07 20:42 ` [PATCH v2 7/7] rebase: add rebase.updateRefs config option Derrick Stolee via GitGitGadget
2022-06-08 14:32 ` [PATCH v2 0/7] rebase: update branches in multi-part topic Phillip Wood
2022-06-08 18:09 ` Derrick Stolee
2022-06-09 10:04 ` Phillip Wood
2022-06-28 13:25 ` [PATCH v3 0/8] " Derrick Stolee via GitGitGadget
2022-06-28 13:25 ` [PATCH v3 1/8] t2407: test branches currently using apply backend Derrick Stolee via GitGitGadget
2022-06-28 20:44 ` Junio C Hamano
2022-06-29 12:54 ` Derrick Stolee
2022-06-30 16:44 ` Junio C Hamano
2022-06-30 17:35 ` Derrick Stolee
2022-06-28 13:25 ` [PATCH v3 2/8] branch: consider refs under 'update-refs' Derrick Stolee via GitGitGadget
2022-06-28 20:48 ` Junio C Hamano
2022-06-29 12:58 ` Derrick Stolee
2022-06-30 9:47 ` Phillip Wood
2022-06-30 16:50 ` Junio C Hamano
2022-06-30 16:49 ` Junio C Hamano
2022-06-30 9:32 ` Phillip Wood
2022-06-30 13:35 ` Derrick Stolee
2022-07-01 13:40 ` Phillip Wood
2022-06-28 13:25 ` [PATCH v3 3/8] rebase-interactive: update 'merge' description Derrick Stolee via GitGitGadget
2022-06-28 21:00 ` Junio C Hamano
2022-06-29 13:02 ` Derrick Stolee
2022-06-30 17:05 ` Junio C Hamano
2022-06-30 9:34 ` Phillip Wood
2022-06-28 13:25 ` [PATCH v3 4/8] sequencer: define array with enum values Derrick Stolee via GitGitGadget
2022-06-28 21:02 ` Junio C Hamano
2022-06-28 13:25 ` [PATCH v3 5/8] sequencer: add update-ref command Derrick Stolee via GitGitGadget
2022-06-30 9:39 ` Phillip Wood
2022-06-28 13:25 ` [PATCH v3 6/8] rebase: add --update-refs option Derrick Stolee via GitGitGadget
2022-06-28 21:09 ` Junio C Hamano
2022-06-29 13:03 ` Derrick Stolee
2022-07-01 9:20 ` Phillip Wood
2022-07-01 21:20 ` Elijah Newren
2022-07-04 12:56 ` Derrick Stolee
2022-07-04 17:57 ` Elijah Newren
2022-07-05 22:22 ` Derrick Stolee
2022-07-08 2:27 ` Elijah Newren
2022-06-28 13:25 ` [PATCH v3 7/8] rebase: update refs from 'update-ref' commands Derrick Stolee via GitGitGadget
2022-06-28 21:15 ` Junio C Hamano
2022-06-29 13:05 ` Derrick Stolee
2022-06-30 17:09 ` Junio C Hamano
2022-06-29 13:06 ` Derrick Stolee
2022-07-01 9:31 ` Phillip Wood
2022-07-01 18:35 ` Junio C Hamano
2022-07-01 23:18 ` Elijah Newren
2022-07-04 13:05 ` Derrick Stolee
2022-06-28 13:25 ` [PATCH v3 8/8] rebase: add rebase.updateRefs config option Derrick Stolee via GitGitGadget
2022-06-28 21:19 ` [PATCH v3 0/8] rebase: update branches in multi-part topic Junio C Hamano
2022-07-01 13:43 ` Phillip Wood
2022-07-12 13:06 ` [PATCH v4 00/12] " Derrick Stolee via GitGitGadget
2022-07-12 13:06 ` [PATCH v4 01/12] t2407: test bisect and rebase as black-boxes Derrick Stolee via GitGitGadget
2022-07-12 13:06 ` [PATCH v4 02/12] t2407: test branches currently using apply backend Derrick Stolee via GitGitGadget
2022-07-12 13:06 ` [PATCH v4 03/12] branch: consider refs under 'update-refs' Derrick Stolee via GitGitGadget
2022-07-15 15:37 ` Phillip Wood
2022-07-12 13:06 ` [PATCH v4 04/12] rebase-interactive: update 'merge' description Derrick Stolee via GitGitGadget
2022-07-12 13:06 ` [PATCH v4 05/12] sequencer: define array with enum values Derrick Stolee via GitGitGadget
2022-07-12 13:06 ` [PATCH v4 06/12] sequencer: add update-ref command Derrick Stolee via GitGitGadget
2022-07-12 13:07 ` [PATCH v4 07/12] rebase: add --update-refs option Derrick Stolee via GitGitGadget
2022-07-16 19:30 ` Elijah Newren
2022-07-19 15:50 ` Derrick Stolee
2022-07-18 9:05 ` SZEDER Gábor
2022-07-18 16:55 ` Derrick Stolee
2022-07-18 19:35 ` Junio C Hamano
2022-07-19 15:53 ` Derrick Stolee
2022-07-19 16:44 ` Junio C Hamano
2022-07-19 16:47 ` Derrick Stolee
2022-07-12 13:07 ` [PATCH v4 08/12] rebase: update refs from 'update-ref' commands Derrick Stolee via GitGitGadget
2022-07-15 13:25 ` Phillip Wood
2022-07-19 16:04 ` Derrick Stolee
2022-07-12 13:07 ` [PATCH v4 09/12] sequencer: rewrite update-refs as user edits todo list Derrick Stolee via GitGitGadget
2022-07-15 10:27 ` Phillip Wood
2022-07-15 13:13 ` Derrick Stolee
2022-07-18 13:09 ` Phillip Wood
2022-07-16 19:20 ` Elijah Newren
2022-07-12 13:07 ` [PATCH v4 10/12] rebase: add rebase.updateRefs config option Derrick Stolee via GitGitGadget
2022-07-12 13:07 ` [PATCH v4 11/12] sequencer: ignore HEAD ref under --update-refs Derrick Stolee via GitGitGadget
2022-07-12 13:07 ` [PATCH v4 12/12] sequencer: notify user of --update-refs activity Derrick Stolee via GitGitGadget
2022-07-15 10:12 ` Phillip Wood
2022-07-15 13:20 ` Derrick Stolee
2022-07-16 20:51 ` Elijah Newren
2022-07-16 22:09 ` Elijah Newren
2022-07-19 16:09 ` Derrick Stolee
2022-07-12 15:37 ` [PATCH v4 00/12] rebase: update branches in multi-part topic Junio C Hamano
2022-07-14 14:50 ` Derrick Stolee
2022-07-14 18:11 ` Junio C Hamano
2022-07-16 21:23 ` Elijah Newren
2022-07-16 20:56 ` Elijah Newren
2022-07-15 15:41 ` Phillip Wood [this message]
2022-07-19 18:33 ` [PATCH v5 " Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 01/12] t2407: test bisect and rebase as black-boxes Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 02/12] t2407: test branches currently using apply backend Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 03/12] branch: consider refs under 'update-refs' Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 04/12] rebase-interactive: update 'merge' description Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 05/12] sequencer: define array with enum values Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 06/12] sequencer: add update-ref command Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 07/12] rebase: add --update-refs option Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 08/12] rebase: update refs from 'update-ref' commands Derrick Stolee via GitGitGadget
2022-07-21 14:03 ` Phillip Wood
2022-07-19 18:33 ` [PATCH v5 09/12] sequencer: rewrite update-refs as user edits todo list Derrick Stolee via GitGitGadget
2022-07-21 14:04 ` Phillip Wood
2022-07-19 18:33 ` [PATCH v5 10/12] rebase: add rebase.updateRefs config option Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 11/12] sequencer: ignore HEAD ref under --update-refs Derrick Stolee via GitGitGadget
2022-07-19 18:33 ` [PATCH v5 12/12] sequencer: notify user of --update-refs activity Derrick Stolee via GitGitGadget
2022-07-21 4:35 ` [PATCH v5 00/12] rebase: update branches in multi-part topic Elijah Newren
2022-07-21 12:12 ` Derrick Stolee
2022-07-21 19:43 ` Elijah Newren
2022-07-21 20:05 ` Derrick Stolee
2022-07-21 14:04 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e620bb51-6c58-cfbb-936b-be46c2f5216a@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@jeffhostetler.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=johannes.schindelin@gmx.de \
--cc=me@ttaylorr.com \
--cc=newren@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).