{"thread":{"id":"54291","subject":"[RFH] sequencer: simplify logic around stopped-sha file","startedAt":"2020-09-25T05:48:55Z","lastAt":"2020-09-28T19:51:37Z","messageCount":4,"participants":["Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"406297","messageId":"xmqq8scymmo1.fsf@gitster.c.googlers.com","threadId":"54291","inReplyTo":null,"subject":"[RFH] sequencer: simplify logic around stopped-sha file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-25T05:48:46Z","receivedAt":"2020-09-25T05:48:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"While I was auditing the use of get_oid_committish(), I noticed that\nan abbreviated object name is written to rebase-merge/stopped-sha\nonly to be read later and expanded to a full object name to be\npassed to record_in_rewritten().  Nobody seems to expose this to the\nend-user and it is unclear if there is a point in keeping it short\nby abbreviating and risking to make it ambiguous as the rebase\nprogresses and creates new objects.\n\nThe attached patch tries to simplify the logic involved around this\nfile, to write and read full object names to/from the file, and the\nresult seems to pass testsuite---which in the ideal world would be\nsufficient signal that this change is safe and sane, but it could be\nthat original authors thought that a change to stop abbreviating is\nnonsense and not worth protecting the code against, hence RFH here.\n\nIs there something obvious I am not seeing that makes this change a\nbad idea (other than \"somebody may be in the middle of a rebase and\nall of a sudden, version of Git gets updated to contain this one,\nwhich is unable to read abbreviated object name the current version\nleft on disk\", which I am deliberately ignoring)?\n\nThanks.\n\n\n sequencer.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fd7701c88a..7dc9088d09 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -120,7 +120,7 @@ static GIT_PATH_FUNC(rebase_path_author_script, \"rebase-merge/author-script\")\n static GIT_PATH_FUNC(rebase_path_amend, \"rebase-merge/amend\")\n /*\n  * When we stop at a given patch via the \"edit\" command, this file contains\n- * the abbreviated commit name of the corresponding patch.\n+ * the commit object name of the corresponding patch.\n  */\n static GIT_PATH_FUNC(rebase_path_stopped_sha, \"rebase-merge/stopped-sha\")\n /*\n@@ -3012,11 +3012,12 @@ static int make_patch(struct repository *r,\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct rev_info log_tree_opt;\n-\tconst char *subject, *p;\n+\tconst char *subject;\n+\tchar hex[GIT_MAX_HEXSZ + 1];\n \tint res = 0;\n \n-\tp = short_commit_name(commit);\n-\tif (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)\n+\toid_to_hex_r(hex, &commit->object.oid);\n+\tif (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)\n \t\treturn -1;\n \tres |= write_rebase_head(&commit->object.oid);\n \n@@ -4396,7 +4397,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n \n \t\tif (read_oneliner(&buf, rebase_path_stopped_sha(),\n \t\t\t\t  READ_ONELINER_SKIP_IF_EMPTY) &&\n-\t\t    !get_oid_committish(buf.buf, &oid))\n+\t\t    !get_oid_hex(buf.buf, &oid))\n \t\t\trecord_in_rewritten(&oid, peek_command(&todo_list, 0));\n \t\tstrbuf_release(&buf);\n \t}\n"},{"id":"406447","messageId":"nycvar.QRO.7.76.6.2009262320530.50@tvgsbejvaqbjf.bet","threadId":"54291","inReplyTo":"xmqq8scymmo1.fsf@gitster.c.googlers.com","subject":"Re: [RFH] sequencer: simplify logic around stopped-sha file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-26T21:28:14Z","receivedAt":"2020-09-26T21:28:24Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 24 Sep 2020, Junio C Hamano wrote:\n\n> While I was auditing the use of get_oid_committish(), I noticed that\n> an abbreviated object name is written to rebase-merge/stopped-sha\n> only to be read later and expanded to a full object name to be\n> passed to record_in_rewritten().  Nobody seems to expose this to the\n> end-user and it is unclear if there is a point in keeping it short\n> by abbreviating and risking to make it ambiguous as the rebase\n> progresses and creates new objects.\n>\n> The attached patch tries to simplify the logic involved around this\n> file, to write and read full object names to/from the file, and the\n> result seems to pass testsuite---which in the ideal world would be\n> sufficient signal that this change is safe and sane, but it could be\n> that original authors thought that a change to stop abbreviating is\n> nonsense and not worth protecting the code against, hence RFH here.\n>\n> Is there something obvious I am not seeing that makes this change a\n> bad idea (other than \"somebody may be in the middle of a rebase and\n> all of a sudden, version of Git gets updated to contain this one,\n> which is unable to read abbreviated object name the current version\n> left on disk\", which I am deliberately ignoring)?\n\nAt least in my understanding, you are not missing anything:\n\n- this file is an implementation detail,\n\n- it is not exposed directly via any user-visible interface,\n\n- any reader will _have_ to be prepared for an unabbreviated object ID (in\n  the highly unlikely case that an object ID would be ambiguous if\n  abbreviated even by one hex character),\n\n- and most importantly: just like we expand the commit IDs in the todo\n  list, we actually want to expand them in `stopped-sha` because it _is_\n  possible that a new object is written that makes the previous\n  unambiguously abbreviated object ID now ambiguous (e.g. when the user\n  commits in a separate worktree while the rebase is interrupted, before\n  continuing the rebase).\n\nIn short: ACK\n\nThank you,\nDscho\n\n>\n> Thanks.\n>\n>\n>  sequencer.c | 11 ++++++-----\n>  1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index fd7701c88a..7dc9088d09 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -120,7 +120,7 @@ static GIT_PATH_FUNC(rebase_path_author_script, \"rebase-merge/author-script\")\n>  static GIT_PATH_FUNC(rebase_path_amend, \"rebase-merge/amend\")\n>  /*\n>   * When we stop at a given patch via the \"edit\" command, this file contains\n> - * the abbreviated commit name of the corresponding patch.\n> + * the commit object name of the corresponding patch.\n>   */\n>  static GIT_PATH_FUNC(rebase_path_stopped_sha, \"rebase-merge/stopped-sha\")\n>  /*\n> @@ -3012,11 +3012,12 @@ static int make_patch(struct repository *r,\n>  {\n>  \tstruct strbuf buf = STRBUF_INIT;\n>  \tstruct rev_info log_tree_opt;\n> -\tconst char *subject, *p;\n> +\tconst char *subject;\n> +\tchar hex[GIT_MAX_HEXSZ + 1];\n>  \tint res = 0;\n>\n> -\tp = short_commit_name(commit);\n> -\tif (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)\n> +\toid_to_hex_r(hex, &commit->object.oid);\n> +\tif (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0)\n>  \t\treturn -1;\n>  \tres |= write_rebase_head(&commit->object.oid);\n>\n> @@ -4396,7 +4397,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)\n>\n>  \t\tif (read_oneliner(&buf, rebase_path_stopped_sha(),\n>  \t\t\t\t  READ_ONELINER_SKIP_IF_EMPTY) &&\n> -\t\t    !get_oid_committish(buf.buf, &oid))\n> +\t\t    !get_oid_hex(buf.buf, &oid))\n>  \t\t\trecord_in_rewritten(&oid, peek_command(&todo_list, 0));\n>  \t\tstrbuf_release(&buf);\n>  \t}\n>\n"},{"id":"406464","messageId":"xmqqo8lsgjxt.fsf@gitster.c.googlers.com","threadId":"54291","inReplyTo":"nycvar.QRO.7.76.6.2009262320530.50@tvgsbejvaqbjf.bet","subject":"Re: [RFH] sequencer: simplify logic around stopped-sha file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-27T00:08:46Z","receivedAt":"2020-09-27T00:08:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> Is there something obvious I am not seeing that makes this change a\n>> bad idea (other than \"somebody may be in the middle of a rebase and\n>> all of a sudden, version of Git gets updated to contain this one,\n>> which is unable to read abbreviated object name the current version\n>> left on disk\", which I am deliberately ignoring)?\n>\n> At least in my understanding, you are not missing anything:\n>\n> - this file is an implementation detail,\n>\n> - it is not exposed directly via any user-visible interface,\n>\n> - any reader will _have_ to be prepared for an unabbreviated object ID (in\n>   the highly unlikely case that an object ID would be ambiguous if\n>   abbreviated even by one hex character),\n\nAh, no that was not what I was worried about.  If an existing code,\nafter restarting and reading the abbreviated object name back from\nthe file, compares that shortened string against list of shortened\nstrings it has in-core (perhaps it reads todo back from the user in\nabbreviated form, does comparison with stopped-sha as-is, before it\nexpands the object names from todo to full length), this change\nwould break it.\n\n> - and most importantly: just like we expand the commit IDs in the todo\n>   list, we actually want to expand them in `stopped-sha` because it _is_\n>   possible that a new object is written that makes the previous\n>   unambiguously abbreviated object ID now ambiguous (e.g. when the user\n>   commits in a separate worktree while the rebase is interrupted, before\n>   continuing the rebase).\n\nExactly.  I just wasn't sure if stopped-sha is handled with the same\ncarefulness as the object names in todo, which are expanded after\nread and shortened before given back to the users.\n\nThanks.\n"},{"id":"406563","messageId":"nycvar.QRO.7.76.6.2009282150190.50@tvgsbejvaqbjf.bet","threadId":"54291","inReplyTo":"xmqqo8lsgjxt.fsf@gitster.c.googlers.com","subject":"Re: [RFH] sequencer: simplify logic around stopped-sha file","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-09-28T19:51:28Z","receivedAt":"2020-09-28T19:51:37Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 26 Sep 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n> >> Is there something obvious I am not seeing that makes this change a\n> >> bad idea (other than \"somebody may be in the middle of a rebase and\n> >> all of a sudden, version of Git gets updated to contain this one,\n> >> which is unable to read abbreviated object name the current version\n> >> left on disk\", which I am deliberately ignoring)?\n> >\n> > [...]\n> > - and most importantly: just like we expand the commit IDs in the todo\n> >   list, we actually want to expand them in `stopped-sha` because it _is_\n> >   possible that a new object is written that makes the previous\n> >   unambiguously abbreviated object ID now ambiguous (e.g. when the user\n> >   commits in a separate worktree while the rebase is interrupted, before\n> >   continuing the rebase).\n>\n> Exactly.  I just wasn't sure if stopped-sha is handled with the same\n> carefulness as the object names in todo, which are expanded after\n> read and shortened before given back to the users.\n\nThe main purpose of `stopped-sha` is to let `git rebase --continue` after\nan `edit` command amend the commit where it stopped _iff_ it is still\n`HEAD`.\n\nSo yes, I think we need to be as careful here.\n\nCiao,\nDscho\n"}]}