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

[Pranit Bauva]

Hey Junio,

On Thu, Aug 4, 2016 at 10:20 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> Hey Junio,
>>
>> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
>>> Pranit Bauva  writes:
>>>
> 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.
>>>
>>> Isn't that what unlink_or_warn() do?  Call unlink() and happily
>>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
>>> exist in the first place), but otherwise reports an error (imagine:
>>> EPERM).
>>
>> Umm, I am confused. I tried "rm -f" with a non-existing file and it
>> does not show any warning or error.
>
> You are, or you were?  I hope it is the latter, iow, you are no
> longer confused and now understand why unlink_or_warn() was
> suggested.

I meant to use past tense. Did not re-check before sending it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  writes:

> Hey Junio,
>
> On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
>> Pranit Bauva  writes:
>>
 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.
>>
>> Isn't that what unlink_or_warn() do?  Call unlink() and happily
>> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
>> exist in the first place), but otherwise reports an error (imagine:
>> EPERM).
>
> Umm, I am confused. I tried "rm -f" with a non-existing file and it
> does not show any warning or error.

You are, or you were?  I hope it is the latter, iow, you are no
longer confused and now understand why unlink_or_warn() was
suggested.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

Hey Junio,

On Thu, Aug 4, 2016 at 9:15 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>>> 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.
>
> Isn't that what unlink_or_warn() do?  Call unlink() and happily
> return if unlink() succeeds or errors with ENOENT (i.e. path didn't
> exist in the first place), but otherwise reports an error (imagine:
> EPERM).

Umm, I am confused. I tried "rm -f" with a non-existing file and it
does not show any warning or error.

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  writes:

>> 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.

Isn't that what unlink_or_warn() do?  Call unlink() and happily
return if unlink() succeeds or errors with ENOENT (i.e. path didn't
exist in the first place), but otherwise reports an error (imagine:
EPERM).

> 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"?

Read it again.

>>> + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

Hey Junio,

On Tue, Aug 2, 2016 at 11:16 PM, Junio C Hamano  wrote:
> Pranit Bauva  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 *) 
>> _for_removal);
>> + string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
>> + result = delete_refs(_for_removal);
>> + refs_for_removal.strdup_strings = 1;
>> + string_list_clear(_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"?

>> + return result;
>> +}

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  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 *) 
> _for_removal);
> + string_list_append(_for_removal, xstrdup("BISECT_HEAD"));
> + result = delete_refs(_for_removal);
> + refs_for_removal.strdup_strings = 1;
> + string_list_clear(_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;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html