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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 3, 2024, 18:17 UTC
Message-ID: <xmqq1q4acpcv.fsf@gitster.g>
In-Reply-To: <CAOLa=ZR-g4G0FaxnQjjkOST-zeRxBXXK1gpJ=P3xdbi_9eN_rg@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 17 quoted lines

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