Re: [PATCH v4 6/6] fetch: delay user information post committing of transaction

Junio C Hamano [off-list ref] writes:

Karthik Nayak [off-list ref] writes:

quoted

+struct ref_update_display_info {
+	bool failed;
+	char success_code;
+	char fail_code;
+	const char *summary;
+	const char *fail_detail;
+	const char *success_detail;
+	const char *ref;
+	const char *remote;
+	struct object_id old_oid;
+	struct object_id new_oid;
+};
+
+struct ref_update_display_info_array {
+	struct ref_update_display_info *info;
+	size_t alloc, nr;
+};

OK.  The ref_update_display_info structure is full of pointers.
They are of "const char *" type, hinting that they are borrowed
pieces of memory, and there is nothing to clean inside, other than
the .info member itself?

quoted

+static struct ref_update_display_info *ref_update_display_info_append(
+					   struct ref_update_display_info_array *array,
+					   char success_code,
+					   char fail_code,
+					   const char *summary,
+					   const char *success_detail,
+					   const char *fail_detail,
+					   const char *ref,
+					   const char *remote,
+					   const struct object_id *old_oid,
+					   const struct object_id *new_oid)
+{

This helper that consumes the structure is used throughout the
patch, and relative to the previous round it got easier to read.

quoted

+static void ref_update_display_info_free(struct ref_update_display_info *info)
+{
+	free((char *)info->summary);
+	free((char *)info->success_detail);
+	free((char *)info->fail_detail);
+	free((char *)info->remote);
+	free((char *)info->ref);
+}

This answers "no" to my previous question.  These are not borrowed,
but are owned by this structure.

Yup, cannot be borrowed, since those go out of scope much earlier.

quoted

@@ -1965,7 +2090,17 @@ static int do_fetch(struct transport *transport,
 	 */
 	if (retcode && !atomic_fetch && transaction)
 		commit_ref_transaction(&transaction, false,
-				       transport->remote->name, &err);
+				       transport->remote->name,
+				       &rejected_refs, &err);
+
+	for (size_t i = 0; i < display_array.nr; i++) {
+		struct ref_update_display_info *info = &display_array.info[i];
+
+		if (!info->failed && strmap_contains(&rejected_refs, info->ref))
+			ref_update_display_info_set_failed(info);
+		ref_update_display_info_display(info, &display_state, summary_width);
+		ref_update_display_info_free(info);
+	}

And after a fetch finishes and we consume the display_info, we call
_free() to release the resource held there, plus ...

quoted

 	if (retcode) {
 		if (err.len) {

@@ -1980,6 +2115,9 @@ static int do_fetch(struct transport *transport,

 	if (transaction)
 		ref_transaction_free(transaction);
+
+	free(display_array.info);

... of course the array itself, which makes sense.

Yeah, the CI also didn't show any leaks, so we should be good.