git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: "Matthias Aßhauer" <mha1993@live.de>,
	"René Scharfe" <l.s.r@web.de>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH] 2.36 gitk/diff-tree --stdin regression fix
Date: Mon, 25 Apr 2022 10:45:47 -0700	[thread overview]
Message-ID: <xmqqbkwpvyyc.fsf@gitster.g> (raw)
In-Reply-To: xmqqh76j3i3r.fsf@gitster.g

This reverts commit 244c2724 (diff.[ch]: have diff_free() call
clear_pathspec(opts.pathspec), 2022-02-16).

The diff_free() call is to be used after a diffopt structure is used
to compare two sets of paths to release resources that were needed
only for that comparison, and keep the data such as pathspec that
are reused by the diffopt structure to make the next and subsequent
comparison (imagine "git log -p -<options> -- <pathspec>" where the
options and pathspec are kept in the diffopt structure, used to
compare HEAD and HEAD~, then used again when HEAD~ and HEAD~2 are
compared).

We by mistake started clearing the pathspec in diff_free(), so
programs like gitk that runs

    git diff-tree --stdin -- <pathspec>

downstream of a pipe, processing one commit after another, started
showing irrelevant comparison outside the given <pathspec> from the
second commit.

The buggy commit may have been hiding the places where diff
machinery is used only once and called diff_free() to release that
per-comparison resources, but forgetting to call clear_pathspec() to
release the resource held for the (potentially) repeated comparison,
and we eventually would want to add clear_pathspec() to clear
resources to be released after a (potentially repeated) diff session
is done (if there are similar resources other than pathspec that
need to be cleared at the end, we should then know where to clear
them), but that is "per program invocation" leak that will be
cleaned up by calling exit(3) and of lower priority than fixing this
behavior-breaking regression.

Reported-by: Matthias Aßhauer <mha1993@live.de>
Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 add-interactive.c | 6 +++---
 blame.c           | 3 +++

 builtin/reset.c   | 1 +
 diff.c            | 1 -
 notes-merge.c     | 2 ++
 5 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index e1ab39cce3..6498ae196f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,
 	diffopt.flags.override_submodule_config = 1;
 	diffopt.repo = s->r;
 
-	if (do_diff_cache(&oid, &diffopt)) {
-		diff_free(&diffopt);
+	if (do_diff_cache(&oid, &diffopt))
 		res = -1;
-	} else {
+	else {
 		diffcore_std(&diffopt);
 		diff_flush(&diffopt);
 	}
 	free(paths);
+	clear_pathspec(&diffopt.pathspec);
 
 	if (!res && write_locked_index(s->r->index, &index_lock,
 				       COMMIT_LOCK) < 0)
diff --git a/blame.c b/blame.c
index 401990726e..206c295660 100644
--- a/blame.c
+++ b/blame.c
@@ -1403,6 +1403,7 @@ static struct blame_origin *find_origin(struct repository *r,
 		}
 	}
 	diff_flush(&diff_opts);
+	clear_pathspec(&diff_opts.pathspec);
 	return porigin;
 }
 
@@ -1446,6 +1447,7 @@ static struct blame_origin *find_rename(struct repository *r,
 		}
 	}
 	diff_flush(&diff_opts);
+	clear_pathspec(&diff_opts.pathspec);
 	return porigin;
 }
 
@@ -2326,6 +2328,7 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,
 	} while (unblamed);
 	target->suspects = reverse_blame(leftover, NULL);
 	diff_flush(&diff_opts);
+	clear_pathspec(&diff_opts.pathspec);
 }
 
 /*
diff --git a/builtin/reset.c b/builtin/reset.c
index 24968dd628..b97745ee94 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -274,6 +274,7 @@ static int read_from_tree(const struct pathspec *pathspec,
 		return 1;
 	diffcore_std(&opt);
 	diff_flush(&opt);
+	clear_pathspec(&opt.pathspec);
 
 	return 0;
 }
diff --git a/diff.c b/diff.c
index 0aef3db6e1..c862771a58 100644
--- a/diff.c
+++ b/diff.c
@@ -6345,7 +6345,6 @@ void diff_free(struct diff_options *options)
 
 	diff_free_file(options);
 	diff_free_ignore_regex(options);
-	clear_pathspec(&options->pathspec);
 }
 
 void diff_flush(struct diff_options *options)
diff --git a/notes-merge.c b/notes-merge.c
index 7ba40cfb08..b4a3a903e8 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -175,6 +175,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
 		       oid_to_hex(&mp->remote));
 	}
 	diff_flush(&opt);
+	clear_pathspec(&opt.pathspec);
 
 	*num_changes = len;
 	return changes;
@@ -260,6 +261,7 @@ static void diff_tree_local(struct notes_merge_options *o,
 		       oid_to_hex(&mp->local));
 	}
 	diff_flush(&opt);
+	clear_pathspec(&opt.pathspec);
 }
 
 static void check_notes_merge_worktree(struct notes_merge_options *o)
-- 
2.36.0-194-g075c326b54



  reply	other threads:[~2022-04-25 17:45 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-23  5:25 gitk regression in version 2.36.0 Matthias Aßhauer
2022-04-23  5:54 ` Junio C Hamano
2022-04-23  6:05   ` Junio C Hamano
2022-04-23 10:13   ` René Scharfe
2022-04-23 16:00     ` Junio C Hamano
2022-04-25 17:45       ` Junio C Hamano [this message]
2022-04-25 22:37         ` [PATCH] t4013: diff-tree --stdin with pathspec Junio C Hamano
2022-04-26 10:09         ` [PATCH] 2.36 gitk/diff-tree --stdin regression fix Phillip Wood
2022-04-26 13:45           ` Phillip Wood
2022-04-26 15:16             ` Junio C Hamano
2022-04-26 15:26             ` Junio C Hamano
2022-04-26 16:11               ` Junio C Hamano
2022-04-27 16:42                 ` René Scharfe
2022-04-27 18:06                   ` René Scharfe
2022-04-27 20:03                     ` Junio C Hamano
2022-04-23  9:27 ` gitk regression in version 2.36.0 René Scharfe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqbkwpvyyc.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --cc=mha1993@live.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).