Junio C Hamano <gitster@xxxxxxxxx> writes:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> Junio C Hamano <gitster@xxxxxxxxx> writes:
>>
>>> Karthik Nayak <karthik.188@xxxxxxxxx> 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 ...
>
Yeah, I should've been more verbose there.
>> 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).
>
I think it is always best to err on the side of more documentation than
less in such situations.
> 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);
This looks perfect, thanks!
Attachment:
signature.asc
Description: PGP signature
