| Message ID | 20220930140948.80367-7-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase --update-refs: smooth out some rough edges | expand |
On Fri, Sep 30 2022, SZEDER Gábor wrote: > + if (item->command == TODO_UPDATE_REF) { > + struct strbuf ref = STRBUF_INIT; > + int ret = 0; > + > + item->commit = NULL; > + item->arg_offset = bol - buf; > + item->arg_len = (int)(eol - bol); > + > + strbuf_add(&ref, bol, item->arg_len); Just a nit and maybe not worth it, but we've done this allocation dance just because.. > + if (!starts_with(ref.buf, "refs/") || > + check_refname_format(ref.buf, 0)) ...there isn't such a thing as checkn_refname_format() taking a "size_t len" or whatever. So maybe not worth it, but if we do the equivalent of: static checkn_refname_format(const char *refname, size_t len, unsigned int flags) { struct strbuf ref = STRBUF_INIT; int ret; strbuf_add(&ref, refname, len); ret = check_refname_format(ref,buf, flags); strbuf_release(&ref); return ret; } This caller could just (untested): if (!starts_with(bol, "refs/") || checkn_refname_format(bol, eol - bol, 0)) return error(_("...%.*s", item->arg_len, bol)); Which saves us the copy in case the "starts_with" test is all we need. Even without such a helper, maybe: int bad; [...] bad = (!starts_with(ref.buf, "refs/") || check_refname_format(ref.buf, 0)); strbuf_release(&buf); if (bad) return error(_("...%.*s", item->arg_len, bol)); return 0; Would make it clearer that the strbuf is just for the use of check_refname_format(). What you have already is also fine, this just sent me down a rabbit hole of re-learning that most of the string duplication we do for check_refname_format() could be avoided if it was slightly less stupid, i.e. accepted a "len" and "prefix" (i.e. "pretend your refname argument started with 'refs/heads/'", or whatever). > + ret = error(_("invalid ref for update-ref instruction: %s"), ref.buf); > + > + strbuf_release(&ref); > + return ret; > + } > + > end_of_object_name = (char *) bol + strcspn(bol, " \t\n"); > saved = *end_of_object_name; > *end_of_object_name = '\0'; > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 2e081b3914..b97f1e8b31 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' ' > test_cmp_rev HEAD refs/heads/no-conflict-branch > ' > > +test_expect_success 'update-refs with invalid refs' ' > + cat >fake-todo-4 <<-EOF && > + update-ref refs/heads/foo..bar > + update-ref refs/heads/foo.lock > + update-ref foo > + update-ref foo/bar > + pick $(git rev-parse HEAD) Another potentially hidden segfault/exit code for "git"
diff --git a/sequencer.c b/sequencer.c index f1732f88f3..ababfa6352 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2522,7 +2522,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, command_to_string(item->command)); if (item->command == TODO_EXEC || item->command == TODO_LABEL || - item->command == TODO_RESET || item->command == TODO_UPDATE_REF) { + item->command == TODO_RESET) { item->commit = NULL; item->arg_offset = bol - buf; item->arg_len = (int)(eol - bol); @@ -2556,6 +2556,23 @@ static int parse_insn_line(struct repository *r, struct todo_item *item, } } + if (item->command == TODO_UPDATE_REF) { + struct strbuf ref = STRBUF_INIT; + int ret = 0; + + item->commit = NULL; + item->arg_offset = bol - buf; + item->arg_len = (int)(eol - bol); + + strbuf_add(&ref, bol, item->arg_len); + if (!starts_with(ref.buf, "refs/") || + check_refname_format(ref.buf, 0)) + ret = error(_("invalid ref for update-ref instruction: %s"), ref.buf); + + strbuf_release(&ref); + return ret; + } + end_of_object_name = (char *) bol + strcspn(bol, " \t\n"); saved = *end_of_object_name; *end_of_object_name = '\0'; diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 2e081b3914..b97f1e8b31 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' ' test_cmp_rev HEAD refs/heads/no-conflict-branch ' +test_expect_success 'update-refs with invalid refs' ' + cat >fake-todo-4 <<-EOF && + update-ref refs/heads/foo..bar + update-ref refs/heads/foo.lock + update-ref foo + update-ref foo/bar + pick $(git rev-parse HEAD) + EOF + cat >expect.err <<-EOF && + error: invalid ref for update-ref instruction: refs/heads/foo..bar + error: invalid line 1: update-ref refs/heads/foo..bar + error: invalid ref for update-ref instruction: refs/heads/foo.lock + error: invalid line 2: update-ref refs/heads/foo.lock + error: invalid ref for update-ref instruction: foo + error: invalid line 3: update-ref foo + error: invalid ref for update-ref instruction: foo/bar + error: invalid line 4: update-ref foo/bar + You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}. + Or you can abort the rebase with ${SQ}git rebase --abort${SQ}. + EOF + test_when_finished "test_might_fail git rebase --abort" && + ( + set_replace_editor fake-todo-4 && + test_must_fail git rebase -i HEAD^ 2>err + ) && + test_cmp expect.err err +' + test_expect_success REFFILES '--update-refs: check failed ref update' ' git checkout -B update-refs-error no-conflict-branch && git branch -f base HEAD~4 &&
Users can add their own 'update-ref <ref>' instructions to the rebase todo list, which also gives them the possibility to specify an invalid ref as argument. Now, while git does catch any invalid ref and errors out, it does so at the very end of the rebase process, when the invalid ref causes the transaction updating all involved refs to fail, leaving users on their own to figure out where each of those refs should point now and to update them themselves. Let's do better, and catch invalid refs early on by calling check_refname_format() for the argument of each 'update-ref' instruction while parsing the todo file. This way 'git rebase' would error out right after the user finished editing the todo file, and would show the same generic advice to rectify the situation that is shown e.g. after an unknown instruction or a missing argument for a 'pick' instruction, etc. Furthermore, require that all refs given to 'update-ref' instructions live under the "refs/" hierarchy. The argument of the 'update-ref' instruction is treated as a fully qualified ref, so if the todo list were to contain the 'update-ref foo' instruction, then 'git rebase' would happily create the ref file '.git/foo' containing the appropriate object id. This is most likely not what the user wanted and will cause confusion. I assume it's much more probable that some users simply forgot about the "refs/heads/" prefix than that they have a use-case for using 'git rebase' to create/update a ref outside the "refs/" hierarchy. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- sequencer.c | 19 ++++++++++++++++++- t/t3404-rebase-interactive.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-)