Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
>> +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());
>
> This is leaked
Good eyes.
>> + 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;
>> + }
>
> Can we use fopen_or_warn() here? It ignores ENOENT and ENOTDIR but I
> don't think that should matter here.
fopen("no-such-directory/file", "w") would fail with ENOENT, and
fopen("README.md/file", "w") would fail with ENOTDIR, I would think,
so "should not matter because we are writing" is not the reason, but
because we know the path is in a subdirectory of ".git/" that we
know should exist, the most likely reason for the fopen to fail is
because (1) the repository is broken (we will get ENOENT, ENOTDIR,
which we want to warn about but fopen_or_warn() ignores, as well as
other errors such as EISDIR), (2) the repository is unwritable (we
will get EACCES), or (3) we are running low on diskspace (ENOSPC).
I think that the fopen_or_warn() helper was primarily invented to
read an optional file (so we deliberately ignore a failure to open
one due to ENOENT and ENOTDIR), and we should be careful of its use
for any other purpose, i.e. write access for any purpose and read
access for files that we know we should be able to.
>> +compare_two_refs () {
>> + git rev-parse $1 >expect &&
>> + git rev-parse $2 >actual &&
>> + test_cmp expect actual
>> +}
>
> This is just test_cmp_rev
I love to see reviewers who know the existing API and helpers very
well (including the fopen-or-warn above).
Very much appreciated.
> Thanks for working on this, it will be a really useful addition to rebase.
Ditto.
Thanks.
