On Thu, Mar 31, 2016 at 11:04:05AM -0700, Stefan Beller wrote:
> diff --git a/bundle.c b/bundle.c
> index 506ac49..31ae1da 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const char *path,
>
> /* write prerequisites */
> if (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))
> - return -1;
> + goto err;
>
> argc = setup_revisions(argc, argv, &revs, NULL);
>
> - if (argc > 1)
> - return error(_("unrecognized argument: %s"), argv[1]);
> + if (argc > 1) {
> + error(_("unrecognized argument: %s"), argv[1]);
> + goto err;
> + }
>
> object_array_remove_duplicates(&revs.pending);
>
> @@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const char *path,
> if (!ref_count)
> die(_("Refusing to create empty bundle."));
> else if (ref_count < 0)
> - return -1;
> + goto err;
>
> /* write pack */
> if (write_pack_data(bundle_fd, &revs))
> - return -1;
> + goto err;
Sorry for not noticing this before, but if we assume write_pack_data
always closes bundle_fd, even on error (and I think it does), then the
close() in the "err" code path is redundant from this goto, right?
I guess it is harmless, as nobody could have opened a new descriptor in
the interim, so our close(bundle_fd) will just get EBADF. But I guess we
could also do:
if (write_pack_data(bundle_fd, &revs)) {
bundle_fd = -1;
goto err;
}
or even:
ret = write_pack_data(bundle_fd, &revs);
bundle_fd = -1; /* closed by write_pack_data */
if (ret)
goto err;
to ensure that nobody (including the non-error code paths) uses it
again.
Like I said, I don't think it's buggy in the current code, but it does
seem a little fragile.
> +err:
> + if (!bundle_to_stdout)
> + close(bundle_fd);
> + rollback_lock_file(&lock);
> + return -1;
Similarly, I think the rollback_lock_file() call is redundant if
bundle_to_stdout is set (because we don't have created a lockfile in the
first place). I think this is more explicitly OK, because the lockfile
keeps an "am I initialized" flag, but perhaps sticking inside the "if
(!bundle_to_stdout)" condition makes it more clear that it's not an
error (especially because the "commit_lock_file" call above is inside
one).
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html