git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, jonathantanmy@google.com, gitster@pobox.com,
	newren@gmail.com, Jay Conrod <jayconrod@google.com>,
	Derrick Stolee <stolee@gmail.com>
Subject: Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
Date: Wed, 3 Jun 2020 16:06:11 -0700	[thread overview]
Message-ID: <20200603230611.GA1925@gmail.com> (raw)
In-Reply-To: <20200603221453.GA36237@syl.local>

Taylor Blau wrote:
> On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:

>> --- i/commit-graph.c
>> +++ w/commit-graph.c
>> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)
>>  	}
>>
>>  	prepare_commit_graft(r);
>> -	if (r->parsed_objects && r->parsed_objects->grafts_nr)
>> +	if (r->parsed_objects &&
>> +	    (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))
>
> This is a little tricky. Why would we set substituted_parent without
> also incrementing grafts_nr? That seems like the real bug here: if we
> incremented grafts_nr, then we would return a non-zero value from
> 'commit_graph_compatible' and rightly stop even without this sticky-bit.
> 
> I don't quite understand this myself. If it's an oversight, it's a
> remarkably long-lived one. Do you have a better sense of this?

The idea here is to check for two different things:

 1. Do we have grafts (either from a grafts file or from a shallow
    file)?  If so, this repository is not commit graph compatible
    because we could encounter one of them.

 2. Have cached parsed objects taken any history modifications into
    account?  If so, this in-memory state is not commit graph
    compatible because we could encounter one of them.

The check (1) might seem sufficient if the set of grafts is constant
for the lifetime of a git process.  But since 37b9dcabfc (shallow.c:
use '{commit,rollback}_shallow_file', 2020-04-22), it is not constant
for the process lifetime, so we need the check (2) as well.

We might want a similar check for replace refs as well some day, but
not today (there is not a way to remove entries from replace_map
during the life of a process).

I can try sending a proper patch with commit message and tests
tomorrow morning (or if someone else takes care of it, that's fine,
too).  Thanks, both, for your help --- it was nice seeing a clear
explanation of the cause already figured out and explained when I woke
up.

Regards,
Jonathan

  reply	other threads:[~2020-06-03 23:06 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-21 18:09 [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Taylor Blau
2020-04-21 20:41 ` Junio C Hamano
2020-04-21 20:45   ` Taylor Blau
2020-04-21 20:52     ` Junio C Hamano
2020-04-21 22:21       ` Taylor Blau
2020-04-21 23:06         ` Junio C Hamano
2020-04-22 18:05       ` Jonathan Tan
2020-04-22 18:02 ` Jonathan Tan
2020-04-22 18:15   ` Junio C Hamano
2020-04-23  0:14     ` Taylor Blau
2020-04-23  0:25       ` [PATCH v2 0/2] shallow.c: reset shallow-ness after updating Taylor Blau
2020-04-23  0:25         ` [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability Taylor Blau
2020-04-23  1:14           ` Jonathan Nieder
2020-04-24 17:11             ` Taylor Blau
2020-04-24 17:17               ` Jonathan Nieder
2020-04-24 20:45               ` Junio C Hamano
2020-04-23  0:25         ` [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file' Taylor Blau
2020-04-23  1:23           ` Jonathan Nieder
2020-04-23 18:09           ` Jonathan Tan
2020-04-23 20:40             ` Junio C Hamano
2020-04-24 17:13               ` Taylor Blau
2020-06-03  3:42           ` Jonathan Nieder
2020-06-03  4:52             ` Taylor Blau
2020-06-03  5:16               ` Taylor Blau
2020-06-03 13:08                 ` Derrick Stolee
2020-06-03 19:26                   ` Taylor Blau
2020-06-03 21:23                   ` Jonathan Nieder
2020-06-03 20:51                 ` Jonathan Nieder
2020-06-03 22:14                   ` Taylor Blau
2020-06-03 23:06                     ` Jonathan Nieder [this message]
2020-06-04 17:45                       ` Taylor Blau
2020-04-23 19:05       ` [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Junio C Hamano

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=20200603230611.GA1925@gmail.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jayconrod@google.com \
    --cc=jonathantanmy@google.com \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    --cc=stolee@gmail.com \
    /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).