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

[Junio C Hamano <gitster@pobox.com>]

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);