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

[Derrick Stolee <derrickstolee@github.com>]

On 7/1/22 5:18 PM, Elijah Newren wrote:
> On Tue, Jun 28, 2022 at 6:26 AM Derrick Stolee via GitGitGadget

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

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

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