On Wed, Jun 05, 2024 at 06:12:00AM -0400, Jeff King wrote:

On Mon, Jun 03, 2024 at 11:30:55AM +0200, Patrick Steinhardt wrote:

quoted

+static int files_ref_store_remove_on_disk(struct ref_store *ref_store,
+					  struct strbuf *err)
+{
[...]
+	strbuf_addf(&sb, "%s/refs", refs->base.gitdir);
+	if (remove_dir_recursively(&sb, 0) < 0) {
+		strbuf_addf(err, "could not delete refs: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+	strbuf_reset(&sb);
+
+	strbuf_addf(&sb, "%s/logs", refs->base.gitdir);
+	if (remove_dir_recursively(&sb, 0) < 0) {
+		strbuf_addf(err, "could not delete logs: %s",
+			    strerror(errno));
+		ret = -1;
+	}
+	strbuf_reset(&sb);

If removing either of the directories fails, we set ret to "-1". Make
sense. But...

quoted

+	ret = for_each_root_ref(refs, remove_one_root_ref, &data);
+	if (ret < 0)
+		ret = -1;

...then we unconditionally overwrite it, forgetting the earlier error.
Either we should jump to the end on the first failure, or if the goal is
to do as much as possible, should we |= the result? I'm not clear why we
assign "ret" and then immediately check it to assign "-1" again.

Is that a mistake, or are we normalizing other negative values? Maybe
just:

  if (for_each_root_ref(refs, remove_one_root_ref, &data) < 0)
	ret = -1;

would work?

Yup, that would work, good catch.

Patrick