On Thu, 29 Aug 2019 at 20:28, Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
> +int repo_refresh_and_write_index(struct repository *repo,
> + unsigned int refresh_flags,
> + unsigned int write_flags,
> + const struct pathspec *pathspec,
> + char *seen, const char *header_msg)
> +{
> + struct lock_file lock_file = LOCK_INIT;
> +
> + repo_hold_locked_index(repo, &lock_file, LOCK_DIE_ON_ERROR);
> + if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
> + return 1;
> + if (write_locked_index(repo->index, &lock_file, COMMIT_LOCK | write_flags))
> + return -1;
> + return 0;
> +}
AFAIU, the_repository->index == &the_index so this patch is a noop on
the converted user as far as that aspect is concerned.
There's a difference in behavior that I'm not sure about: We used
to ignore the return value of `refresh_cache()`, i.e. we didn't care
whether it had any errors. I have no idea whether that's safe to do --
especially as we go on to write the index. So I don't know whether this
patch fixes a bug by introducing the early return. Or if it *introduces*
a bug by bailing too aggressively. Do you know more?
(This conversion provides REFRESH_QUIET, which seems to suppress certain
errors, but not all.)
In any case, that early return introduces a bug with the lockfile, that
much I know. We need to roll back the lockfile before doing the early
return. I should have seen that already in your previous version.. :-(
The above makes me think that once this new function is in good shape,
the commit introducing it could sell it as "this is hard to get right --
let's implement it correctly once and for all". ;-)
Martin
