Pranit Bauva [off-list ref] writes:
+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;
+}
Hey Junio,
On Tue, Aug 2, 2016 at 11:16 PM, Junio C Hamano [off-list ref] wrote:
Pranit Bauva [off-list ref] writes:
quoted
+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.
Yes I should combine the results from every removal.
Isn't unlink_or_warn() more correct helper to use here?
The shell code uses rm -f which is silent and it removes only if
present. So it makes me wonder which would be more appropriate
unlink_or_warn() or remove_or_warn() or remove_path(). Is
remove_path() different from its shell equivalent "rm -f"?
quoted
+ return result;
+}
Regards,
Pranit Bauva