Re: [PATCH v3 7/8] rebase: update refs from 'update-ref' commands

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 4, 2022, 13:05 UTC
Message-ID: <5002269d-5cab-3ba4-b99e-dca0c3615118@github.com>
In-Reply-To: <CABPp-BG++8LAwyH6P-QDj5Lu6-cevOBOjpdY7QsSdwiT0w9URA@mail.gmail.com>

On 7/1/22 5:18 PM, Elijah Newren wrote:

> On Tue, Jun 28, 2022 at 6:26 AM Derrick Stolee via GitGitGadget

Show 7 quoted lines

>> 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.
> 
> Why update with each update-ref command?  Couldn't the values be
> stored in memory and only written when we hit a conflict?

I think that is a natural way to optimize the feature. I didn't look into that option as it seemed more error-prone to me. I'd be happy to be wrong here if an easy tweak makes this possible.

Show 29 quoted lines

>> -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());
>> +
>> +       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;
>> +       }
>> +
>> +       for_each_string_list_item(item, refs_to_oids)
>> +               fprintf(fp, "%s\n%s\n", item->string, oid_to_hex(item->util));
> 
> Here you are storing the ref and the new oid to move it to.  Any
> reason you don't store the previous oid for the branch?  In
> particular, if someone hits a conflict, needs to resolve, and comes
> back some time much later and these intermediate branches have since
> moved on, should we actually update those branches?  (And, if we do,
> should we at least give a warning?)

The branches don't move forward because other Git processes respect the update-refs file (this does not take into account third-party tools that don't know about that file, but we need to start somewhere).

You're right that storing the old value would help us in this case where another tool updated the ref while we were in the middle of the rebase. The storage of the null OID is only to prevent writing over a ref when the user has removed the 'update-ref <ref>' command from the todo-list. It's probably better to remove the ref from the list when that happens.

Show 14 quoted lines

>> +       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);
> 
> Seems a little unfortunate to not use a cached value from the latest
> commit we picked (and wrote to HEAD) and instead need to re-read HEAD.
> But, I guess sequencer is written to round-trip through files, so
> maybe optimizing this here doesn't make sense given all the other
> places in sequencer where we do lots of file reading and writing.

It's also possible to optimize several things, but I tried to minimize the amount of change to the existing methods. This is my first foray into this part of the code, so I don't always know which information is readily available. I appreciate your attention here.

Thanks, -Stolee