Re: [PATCH v7] describe: refresh the index when 'broken' flag is used

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Karthik Nayak <karthik.188@gmail.com> writes:
>>>
>>>> This explains for why 'broken' must use a subprocess, but there is
>>>> nothing stopping 'dirty' from also using a subprocess, right? It
>>>> currently uses an in-process index refresh but it _could_ be a
>>>> subprocess too.
>>>
>>> Correct, except that it does not make sense to do any and all things
>>> that you _could_ do.  So...
>>
>> Well, In this context, I think there is some merit though. There are two
>> blocks of code `--broken` and `--dirty` one after the other which both
>> need to refresh the index. With this patch, 'broken' will use a child
>> process to do so while 'dirty' will use `refresh_index(...)`. To someone
>> reading the code it would seem a bit confusing.
>
> Yes, that much I very much agree.
>
>> I agree there is no
>> merit in using a child process in 'dirty' by itself.
>
> Yes, that made me puzzled why you brought it up, as it was way too
> oblique suggestion to ...
>

>> But I also think we
>> should leave a comment there for readers to understand the distinction.
>
> ... improve the "documentation" to help future developers who wonder
> why the code are in the shape as it is.
>
> In this particular case, I think it is borderline if the issue
> warrants in-code comment or if it is a bit too much.  Describing the
> same thing in the log message would probably be a valid alternative,
> as "git blame" can lead those readers to the commit that introduced
> the distinction (in other words, this one).
>

> Thanks.
>
> diff --git i/builtin/describe.c w/builtin/describe.c
> index e936d2c19f..bc2ad60b35 100644
> --- i/builtin/describe.c
> +++ w/builtin/describe.c
> @@ -648,6 +648,14 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>
>  	if (argc == 0) {
>  		if (broken) {
> +			/*
> +			 * Avoid doing these "update-index --refresh"
> +			 * and "diff-index" operations in-process
> +			 * (like how the code path for "--dirty"
> +			 * without "--broken" does so below), as we
> +			 * are told to prepare for a broken repository
> +			 * where running these may lead to die().
> +			 */
>  			struct child_process cp = CHILD_PROCESS_INIT;
>
>  			strvec_pushv(&cp.args, update_index_args);