Re: [PATCH 1/2] Check order when reading index

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

Re: [PATCH 1/2] Check order when reading index

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:20 

Jaime Soriano Pastor [off-list ref] writes:

Subject: Re: [PATCH 1/2] Check order when reading index
Please be careful when crafting the commit title.  This single line
will be the only one that readers will have to identify the change
among hundreds of entries in "git shortlog" output when trying to
see what kind of change went into the project during the given
period.  Something like:

    read_index_from(): catch out of order entries while reading an index file

perhaps?
quoted hunk
Signed-off-by: Jaime Soriano Pastor <redacted>
---
 read-cache.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 7f5645e..c1a9619 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1438,6 +1438,21 @@ static struct cache_entry *create_from_disk(struct ondisk_cache_entry *ondisk,
 	return ce;
 }
 
+void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)
Does this have to be global, i.e. not "static void ..."?

+{
+	int name_compare = strcmp(ce->name, next_ce->name);
+	if (0 < name_compare)
+		die("Unordered stage entries in index");
+	if (!name_compare) {
+		if (!ce_stage(ce))
+			die("Multiple stage entries for merged file '%s'",
+				ce->name);
OK.  If ce is at stage #0, no other entry can have the same name
regardless of the stage, and next_ce having the same name violates
that rule.

+		if (ce_stage(ce) >= ce_stage(next_ce))
+			die("Unordered stage entries for '%s'",
+				ce->name);
Not quite.  We do allow multiple higher stage entries; having two or
more stage #1 entries is perfectly fine during a merge resolution,
and both ce and next_ce may be pointing at the stage #1 entries of
the same path.  Replacing the comparison with ">" is sufficient, I
think.

Thanks.
quoted hunk
+	}
+}
+
 /* remember to discard_cache() before reading a different cache! */
 int read_index_from(struct index_state *istate, const char *path)
 {
@@ -1499,6 +1514,9 @@ int read_index_from(struct index_state *istate, const char *path)
 		ce = create_from_disk(disk_ce, &consumed, previous_name);
 		set_index_entry(istate, i, ce);
 
+		if (i > 0)
+			check_ce_order(istate->cache[i - 1], ce);
+
 		src_offset += consumed;
 	}
 	strbuf_release(&previous_name_buf);

Re: [PATCH 1/2] Check order when reading index

From: Jeff King <hidden>
Date: 2016-06-15 23:02:20 

On Mon, Aug 25, 2014 at 10:21:58AM -0700, Junio C Hamano wrote:
quoted
+		if (ce_stage(ce) >= ce_stage(next_ce))
+			die("Unordered stage entries for '%s'",
+				ce->name);
Not quite.  We do allow multiple higher stage entries; having two or
more stage #1 entries is perfectly fine during a merge resolution,
and both ce and next_ce may be pointing at the stage #1 entries of
the same path.  Replacing the comparison with ">" is sufficient, I
think.
For my own curiosity, how do you get into this situation, and what does
it mean to have multiple stage#1 entries for the same path? What would
"git cat-file :1:path" output?

-Peff

Re: [PATCH 1/2] Check order when reading index

From: Jaime Soriano Pastor <hidden>
Date: 2016-06-15 23:02:21 

On Mon, Aug 25, 2014 at 7:21 PM, Junio C Hamano [off-list ref] wrote:
Jaime Soriano Pastor [off-list ref] writes:
quoted
Subject: Re: [PATCH 1/2] Check order when reading index
Please be careful when crafting the commit title.  This single line
will be the only one that readers will have to identify the change
among hundreds of entries in "git shortlog" output when trying to
see what kind of change went into the project during the given
period.  Something like:

    read_index_from(): catch out of order entries while reading an index file

perhaps?
Ok, reprashing it.
quoted
+void check_ce_order(struct cache_entry *ce, struct cache_entry *next_ce)
Does this have to be global, i.e. not "static void ..."?
Not really, changing it to static.
quoted
+             if (ce_stage(ce) >= ce_stage(next_ce))
+                     die("Unordered stage entries for '%s'",
+                             ce->name);
Not quite.  We do allow multiple higher stage entries; having two or
more stage #1 entries is perfectly fine during a merge resolution,
and both ce and next_ce may be pointing at the stage #1 entries of
the same path.  Replacing the comparison with ">" is sufficient, I
think.
Ok, but like Jeff, I'm also curious about how to have multiple stage
#1 entries for the same path.

Thanks.

Re: [PATCH 1/2] Check order when reading index

From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:21 

On Mon, Aug 25, 2014 at 12:44 PM, Jeff King [off-list ref] wrote:
For my own curiosity, how do you get into this situation, and what does
it mean to have multiple stage#1 entries for the same path? What would
"git cat-file :1:path" output?
That is how we natively (read: not with the funky "virtual" stuff
merge-recursive does) express a merge with multiple merge bases.
You also should be able to read this in the way how "git merge" invokes
merge strategies (one or more bases, double-dash and then current
HEAD and the other branches). I think there are some tests in 3way
merge tests that checks what should happen when our HEAD matches
one of the stage #1 while their branch matches a different one of the
stage #1, too.

Re: [PATCH 1/2] Check order when reading index

From: Jaime Soriano Pastor <hidden>
Date: 2016-06-15 23:02:21 

On Mon, Aug 25, 2014 at 10:52 PM, Junio C Hamano [off-list ref] wrote:
On Mon, Aug 25, 2014 at 12:44 PM, Jeff King [off-list ref] wrote:
quoted
For my own curiosity, how do you get into this situation, and what does
it mean to have multiple stage#1 entries for the same path? What would
"git cat-file :1:path" output?
That is how we natively (read: not with the funky "virtual" stuff
merge-recursive does) express a merge with multiple merge bases.
You also should be able to read this in the way how "git merge" invokes
merge strategies (one or more bases, double-dash and then current
HEAD and the other branches). I think there are some tests in 3way
merge tests that checks what should happen when our HEAD matches
one of the stage #1 while their branch matches a different one of the
stage #1, too.
I'm a bit lost with this, conceptually it doesn't seem to be any
problem with having multiple merge bases, but I don't manage to
reproduce it.
With "natively" do you mean some internal state that is never written
into the index? If this were the case then there wouldn't be any
problem with the restriction when reading the index file.

I have also tried to reproduce it by directly calling
git-merge-recursive and the most I have got is what it seemed to be
like a conflict in the stage #1:

$ git ls-files -s
100644 1036ba5101378f06aa10c5fa249b67e14f83166b 1 conflict
100644 2638c45f8e7bc5b56f70e92390153572811782c3 2 conflict
100644 178481050188cf00d7d9cd5a11e43ab8fab9294f 3 conflict

$ git cat-file blob 1036ba5101378f06aa10c5fa249b67e14f83166b
<<<<<<< Temporary merge branch 1
G
=======
E
F
quoted
quoted
quoted
quoted
quoted
quoted
Temporary merge branch 2
And after thinking a bit more about it, I don't see a way to have two
stage #1 entries for the same path with git commands only. It seems
that all entries are added through the add_index_entry_with_check()
function (except maybe the added to the cached tree extension), and
this function replaces existing entries if they have the same name and
stage.
Also, all tests pass with the patch, without allowing two entries for
the same stage.

So I'm afraid that I don't fully understand this case :/

Re: [PATCH 1/2] Check order when reading index

From: Jeff King <hidden>
Date: 2016-06-15 23:02:21 

On Tue, Aug 26, 2014 at 02:08:35PM +0200, Jaime Soriano Pastor wrote:
quoted
That is how we natively (read: not with the funky "virtual" stuff
merge-recursive does) express a merge with multiple merge bases.
You also should be able to read this in the way how "git merge" invokes
merge strategies (one or more bases, double-dash and then current
HEAD and the other branches). I think there are some tests in 3way
merge tests that checks what should happen when our HEAD matches
one of the stage #1 while their branch matches a different one of the
stage #1, too.
I'm a bit lost with this, conceptually it doesn't seem to be any
problem with having multiple merge bases, but I don't manage to
reproduce it.
With "natively" do you mean some internal state that is never written
into the index? If this were the case then there wouldn't be any
problem with the restriction when reading the index file.
FWIW, that was my question on reading Junio's response, too.

I have also tried to reproduce it by directly calling
git-merge-recursive and the most I have got is what it seemed to be
like a conflict in the stage #1:

$ git ls-files -s
100644 1036ba5101378f06aa10c5fa249b67e14f83166b 1 conflict
100644 2638c45f8e7bc5b56f70e92390153572811782c3 2 conflict
100644 178481050188cf00d7d9cd5a11e43ab8fab9294f 3 conflict

$ git cat-file blob 1036ba5101378f06aa10c5fa249b67e14f83166b
<<<<<<< Temporary merge branch 1
G
=======
E
F
quoted
quoted
quoted
quoted
quoted
quoted
quoted
Temporary merge branch 2
Yes, I think merge-recursive resolves the earlier merges and then feeds
the result into the main merge. And that's how you end up with the
"temporary merge branch" name instead of something useful.

It might work to have a recursive merge that causes a conflict on path
X, and then we further need to resolve that conflict. I'm not sure if we
try to represent that in the index somehow, or if merge-recursive just
bails in this case (I didn't try it).

-Peff
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help