Thread (2 messages) 2 messages, 2 authors, 2024-07-03

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

From: Junio C Hamano <hidden>
Date: 2024-07-03 18:17:07

Karthik Nayak [off-list ref] writes:
Junio C Hamano [off-list ref] writes:
quoted
Karthik Nayak [off-list ref] writes:
quoted
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);
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help