Re: [PATCH] rerere: exit silently on "forget" when rerere is disabled

5 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] rerere: exit silently on "forget" when rerere is disabled

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:04:46

Jeff King [off-list ref] writes:

On Thu, May 14, 2015 at 12:19:25PM -0700, Junio C Hamano wrote:

quoted

It looks like we need to pay more attention to the return value of
setup_rerere, which is what is supposed to take the lock.

Good spotting.  The normal rerere does check, but rerere-forget
codepath seems to forget it.

Here's a patch.

Thanks.  This is obviously correct to fix your "init -q" one.

I am still puzzled by the original, though.  I assumed that rerere
was enabled and working correctly (in the sense that it correctly
replayed a mistaken resolution recorded earlier, which Alex wanted
to correct by forgetting).

quoted hunk

-- >8 --
Subject: rerere: exit silently on "forget" when rerere is disabled

If you run "git rerere forget foo" in a repository that does
not have rerere enabled, git hits an internal error:

  $ git init -q
  $ git rerere forget foo
  fatal: BUG: attempt to commit unlocked object

The problem is that setup_rerere() will not actually take
the lock if the rerere system is disabled. We should notice
this and return early. We can return with a success code
here, because we know there is nothing to forget.

Signed-off-by: Jeff King <redacted>
---
 rerere.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/rerere.c b/rerere.c
index 31644de..94aea9a 100644
--- a/rerere.c
+++ b/rerere.c

@@ -659,6 +659,8 @@ int rerere_forget(struct pathspec *pathspec)
 		return error("Could not read index");
 
 	fd = setup_rerere(&merge_rr, RERERE_NOAUTOUPDATE);
+	if (fd < 0)
+		return 0;
 
 	unmerge_cache(pathspec);
 	find_conflict(&conflict);

Re: [PATCH] rerere: exit silently on "forget" when rerere is disabled

From: Jeff King <hidden>
Date: 2016-06-15 23:04:46

On Thu, May 14, 2015 at 12:33:02PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Thu, May 14, 2015 at 12:19:25PM -0700, Junio C Hamano wrote:

quoted

It looks like we need to pay more attention to the return value of
setup_rerere, which is what is supposed to take the lock.

Good spotting.  The normal rerere does check, but rerere-forget
codepath seems to forget it.

Here's a patch.

Thanks.  This is obviously correct to fix your "init -q" one.

I am still puzzled by the original, though.  I assumed that rerere
was enabled and working correctly (in the sense that it correctly
replayed a mistaken resolution recorded earlier, which Alex wanted
to correct by forgetting).

Yeah, agreed. I don't see any other code paths that could end up trying
to commit a lock we haven't taken, though.

Alex, can you tell us more about your repository? And possibly show us
the output of:

  ls -d .git/rr-cache
  git config rerere.enabled

in the repository?

-Peff

Re: [PATCH] rerere: exit silently on "forget" when rerere is disabled

From: Alex Coppens <hidden>
Date: 2016-06-15 23:04:46

Here are the outputs I have:

$ ls -d .git/rr-cache
ls: .git/rr-cache: No such file or directory

$ git config rerere.enabled
$

My repository is a ruby on rails project, I am currently on the
development branch. It's a private repository hosted on Github. Not
sure what other information you want.

Alex




On Thu, May 14, 2015 at 4:22 PM, Jeff King [off-list ref] wrote:

On Thu, May 14, 2015 at 12:33:02PM -0700, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

On Thu, May 14, 2015 at 12:19:25PM -0700, Junio C Hamano wrote:

quoted

It looks like we need to pay more attention to the return value of
setup_rerere, which is what is supposed to take the lock.

Good spotting.  The normal rerere does check, but rerere-forget
codepath seems to forget it.

Here's a patch.

Thanks.  This is obviously correct to fix your "init -q" one.

I am still puzzled by the original, though.  I assumed that rerere
was enabled and working correctly (in the sense that it correctly
replayed a mistaken resolution recorded earlier, which Alex wanted
to correct by forgetting).

Yeah, agreed. I don't see any other code paths that could end up trying
to commit a lock we haven't taken, though.

Alex, can you tell us more about your repository? And possibly show us
the output of:

  ls -d .git/rr-cache
  git config rerere.enabled

in the repository?

-Peff

Re: [PATCH] rerere: exit silently on "forget" when rerere is disabled

From: Jeff King <hidden>
Date: 2016-06-15 23:04:46

On Thu, May 14, 2015 at 04:51:25PM -0400, Alex Coppens wrote:

Here are the outputs I have:

$ ls -d .git/rr-cache
ls: .git/rr-cache: No such file or directory

$ git config rerere.enabled
$

My repository is a ruby on rails project, I am currently on the
development branch. It's a private repository hosted on Github. Not
sure what other information you want.

OK, so it looks like rerere is not enabled (and my patch will fix the
case you saw). But then I am puzzled why you were running "rerere
forget" in the first place. Just experimenting, or did you expect it to
do something?

-Peff

Re: [PATCH] rerere: exit silently on "forget" when rerere is disabled

From: Alex Coppens <hidden>
Date: 2016-06-15 23:04:46

I really thought my boss setup rerere on my project a while ago.
I had a merge without conflicts that went wrong (the whole app
breaking because of a missing comma) so I assumed a conflict was
automatically resolved.

Alex

On Thu, May 14, 2015 at 4:56 PM, Jeff King [off-list ref] wrote:

On Thu, May 14, 2015 at 04:51:25PM -0400, Alex Coppens wrote:

quoted

Here are the outputs I have:

$ ls -d .git/rr-cache
ls: .git/rr-cache: No such file or directory

$ git config rerere.enabled
$

My repository is a ruby on rails project, I am currently on the
development branch. It's a private repository hosted on Github. Not
sure what other information you want.

OK, so it looks like rerere is not enabled (and my patch will fix the
case you saw). But then I am puzzled why you were running "rerere
forget" in the first place. Just experimenting, or did you expect it to
do something?

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help