Re: [RFC/PATCH v11 04/13] bisect--helper: `bisect_clean_state` shell function in C

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 2, 2016, 17:46 UTC
Message-ID: <xmqqy44fdpxd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102015640423ce6-5b11201e-736d-413f-be12-7fed613ae484-000000@eu-west-1.amazonses.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 23 quoted lines

> +static int bisect_clean_state(void)
> +{
> +	int result = 0;
> +
> +	/* There may be some refs packed during bisection */
> +	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> +	for_each_ref_in("refs/bisect/", mark_for_removal, (void *) &refs_for_removal);
> +	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> +	result = delete_refs(&refs_for_removal);
> +	refs_for_removal.strdup_strings = 1;
> +	string_list_clear(&refs_for_removal, 0);
> +	remove_path(git_path_bisect_expected_rev());
> +	remove_path(git_path_bisect_ancestors_ok());
> +	remove_path(git_path_bisect_log());
> +	remove_path(git_path_bisect_names());
> +	remove_path(git_path_bisect_run());
> +	remove_path(git_path_bisect_terms());
> +	/* Cleanup head-name if it got left by an old version of git-bisect */
> +	remove_path(git_path_head_name());
> +	 * Cleanup BISECT_START last to support the --no-checkout option
> +	 * introduced in the commit 4796e823a.
> +	 */
> +	remove_path(git_path_bisect_start());

I can see that refs/files-backend.c misuses it already, but remove_path() helper is about removing a path in the working tree, together with any parent directory that becomes empty due to the removal. You do not expect $GIT_DIR/ to become an empty directory after removing $GIT_DIR/BISECT_LOG nor want to rmdir $GIT_DIR even if it becomes empty. It is a wrong helper function to use here.

Also you do not seem to check the error from the function to smudge the "result" you are returning from this function.

Isn't unlink_or_warn() more correct helper to use here?

> +	return result;
> +}