Re: A note on merging conflicts..

Subsystems: the rest

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

Re: A note on merging conflicts..

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:42:32

Junio C Hamano [off-list ref] writes:

Linus Torvalds [off-list ref] writes:

quoted

On Sat, 1 Jul 2006, Rene Scharfe wrote:

quoted

I wonder why the two clear_commit_marks() calls at the end of
get_merge_bases() are not sufficient, though.

Why does that thing check for "parent->object.parsed"?

Oh, I thought I fixed that up when I merged.  Sorry.

quoted

Also, it only clears commit marks if they are contiguous, but some commit 
marking may not be dense (eg, the "UNINTERESTING" mark may have been set 
by (PARENT1 && PARENT2) triggering, but is not set in the commits that 
reach it.

That is true.

I'll be offline for a few hours, but if anybody is inclined to
fix this up, I'd appreciate it to be based on top of 7c6f8aa
commit (which is the tip of js/merge-base topic branch).

Perhaps as a starter (not even compile tested)...

diff --git a/commit.c b/commit.c
index 0431027..23ac210 100644
--- a/commit.c
+++ b/commit.c

@@ -397,13 +397,13 @@ void clear_commit_marks(struct commit *c
 {
 	struct commit_list *parents;
 
-	parents = commit->parents;
+	if (!commit)
+		return;
 	commit->object.flags &= ~mark;
+	parents = commit->parents;
 	while (parents) {
 		struct commit *parent = parents->item;
-		if (parent && parent->object.parsed &&
-		    (parent->object.flags & mark))
-			clear_commit_marks(parent, mark);
+		clear_commit_marks(parent, mark);
 		parents = parents->next;
 	}
 }

@@ -1012,7 +1012,8 @@ static void mark_reachable_commits(struc
 	}
 }
 
-struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2)
+struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2,
+				    int cleanup_needed)
 {
 	struct commit_list *list = NULL;
 	struct commit_list *result = NULL;

@@ -1081,8 +1082,10 @@ struct commit_list *get_merge_bases(stru
 	}
 
 	/* reset flags */
-	clear_commit_marks(rev1, PARENT1 | PARENT2 | UNINTERESTING);
-	clear_commit_marks(rev2, PARENT1 | PARENT2 | UNINTERESTING);
+	if (cleanup_needed) {
+		clear_commit_marks(rev1, PARENT1 | PARENT2 | UNINTERESTING);
+		clear_commit_marks(rev2, PARENT1 | PARENT2 | UNINTERESTING);
+	}
 
 	return result;
 }

diff --git a/commit.h b/commit.h
index 89b9dad..53bc697 100644
--- a/commit.h
+++ b/commit.h

@@ -105,6 +105,6 @@ struct commit_graft *read_graft_line(cha
 int register_commit_graft(struct commit_graft *, int);
 int read_graft_file(const char *graft_file);
 
-extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
+extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup_needed);
 
 #endif /* COMMIT_H */

diff --git a/merge-base.c b/merge-base.c
index b41f76c..59f723f 100644
--- a/merge-base.c
+++ b/merge-base.c

@@ -6,7 +6,7 @@ static int show_all = 0;
 
 static int merge_base(struct commit *rev1, struct commit *rev2)
 {
-	struct commit_list *result = get_merge_bases(rev1, rev2);
+	struct commit_list *result = get_merge_bases(rev1, rev2, 0);
 
 	if (!result)
 		return 1;

[PATCH 1/3] Add get_merge_bases_clean()

From: Rene Scharfe <hidden>
Date: 2016-06-15 22:42:32

Add get_merge_bases_clean(), a wrapper for get_merge_bases() that cleans
up after doing its work and make get_merge_bases() NOT clean up.
Single-shot programs like git-merge-base can use the dirty and fast
version.

Also move the object flags used in get_merge_bases() out of the range
defined in revision.h.  This fixes the "66ae0c77...ced9456a
89719209...262a6ef7" test of the ... operator which is introduced with
the next patch.

Signed-off-by: Rene Scharfe <redacted>

---
All object flags should probably be gathered into one place to avoid
future conflicts.

 commit.c |   20 ++++++++++++++++----
 commit.h |    1 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index 0431027..593414d 100644
--- a/commit.c
+++ b/commit.c

@@ -849,9 +849,10 @@ void sort_in_topological_order_fn(struct
 
 /* merge-rebase stuff */
 
-#define PARENT1 1
-#define PARENT2 2
-#define UNINTERESTING 4
+/* bits #0..7 in revision.h */
+#define PARENT1		(1u<< 8)
+#define PARENT2		(1u<< 9)
+#define UNINTERESTING	(1u<<10)
 
 static struct commit *interesting(struct commit_list *list)
 {

@@ -1080,9 +1081,20 @@ struct commit_list *get_merge_bases(stru
 		tmp = next;
 	}
 
-	/* reset flags */
+	return result;
+}
+
+struct commit_list *get_merge_bases_clean(struct commit *rev1,
+                                          struct commit *rev2)
+{
+	unsigned int flags1 = rev1->object.flags;
+	unsigned int flags2 = rev2->object.flags;
+	struct commit_list *result = get_merge_bases(rev1, rev2);
+
 	clear_commit_marks(rev1, PARENT1 | PARENT2 | UNINTERESTING);
 	clear_commit_marks(rev2, PARENT1 | PARENT2 | UNINTERESTING);
+	rev1->object.flags = flags1;
+	rev2->object.flags = flags2;
 
 	return result;
 }

diff --git a/commit.h b/commit.h
index 89b9dad..3a26e29 100644
--- a/commit.h
+++ b/commit.h

@@ -106,5 +106,6 @@ int register_commit_graft(struct commit_
 int read_graft_file(const char *graft_file);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
+extern struct commit_list *get_merge_bases_clean(struct commit *rev1, struct commit *rev2);
 
 #endif /* COMMIT_H */

-- 
1.4.1.rc2.gfc04

[PATCH 2/3] Add '...' operator for revisions

From: Rene Scharfe <hidden>
Date: 2016-06-15 22:42:32

'A...B' is a shortcut for 'A B --not $(git-merge-base --all A B)'.
This XOR-like operation is called symmetric difference in set
theory.

The symbol '...' has been chosen because it's rather similar to the
existing '..' operator and the somewhat more natural caret ('^') is
already taken.

Signed-off-by: Rene Scharfe <redacted>
---
 Documentation/git-rev-list.txt |   14 ++++++++++++
 revision.c                     |   48 +++++++++++++++++++++++++++++++++-------
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index ad6d14c..6c370e1 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt

@@ -15,6 +15,7 @@ SYNOPSIS
 	     [ \--sparse ]
 	     [ \--no-merges ]
 	     [ \--remove-empty ]
+	     [ \--not ]
 	     [ \--all ]
 	     [ \--topo-order ]
 	     [ \--parents ]

@@ -37,6 +38,14 @@ not in 'baz'".
 A special notation <commit1>..<commit2> can be used as a
 short-hand for {caret}<commit1> <commit2>.
 
+Another special notation is <commit1>...<commit2> which is useful for
+merges.  The resulting set of commits is the symmetric difference
+between the two operands.  The following two commands are equivalent:
+
+------------
+$ git-rev-list A B --not $(git-merge-base --all A B)
+$ git-rev-list A...B
+------------
 
 OPTIONS
 -------

@@ -93,6 +102,11 @@ OPTIONS
 --remove-empty::
 	Stop when a given path disappears from the tree.
 
+--not::
+	Reverses the meaning of the '{caret}' prefix (or lack
+	thereof) for all following revision specifiers, up to
+	the next `--not`.
+
 --all::
 	Pretend as if all the refs in `$GIT_DIR/refs/` are
 	listed on the command line as <commit>.

diff --git a/revision.c b/revision.c
index b963f2a..9eb0b6d 100644
--- a/revision.c
+++ b/revision.c

@@ -536,6 +536,18 @@ void init_revisions(struct rev_info *rev
 	diff_setup(&revs->diffopt);
 }
 
+static void add_pending_commit_list(struct rev_info *revs,
+                                    struct commit_list *commit_list,
+                                    unsigned int flags)
+{
+	while (commit_list) {
+		struct object *object = &commit_list->item->object;
+		object->flags |= flags;
+		add_pending_object(revs, object, sha1_to_hex(object->sha1));
+		commit_list = commit_list->next;
+	}
+}
+
 /*
  * Parse revision information, filling in the "rev_info" structure,
  * and removing the used arguments from the argument list.

@@ -771,27 +783,45 @@ int setup_revisions(int argc, const char
 			unsigned char from_sha1[20];
 			const char *next = dotdot + 2;
 			const char *this = arg;
+			int symmetric = *next == '.';
+			unsigned int flags_exclude = flags ^ UNINTERESTING;
+
 			*dotdot = 0;
+			next += symmetric;
+
 			if (!*next)
 				next = "HEAD";
 			if (dotdot == arg)
 				this = "HEAD";
 			if (!get_sha1(this, from_sha1) &&
 			    !get_sha1(next, sha1)) {
-				struct object *exclude;
-				struct object *include;
-
-				exclude = get_reference(revs, this, from_sha1, flags ^ UNINTERESTING);
-				include = get_reference(revs, next, sha1, flags);
-				if (!exclude || !include)
-					die("Invalid revision range %s..%s", arg, next);
+				struct commit *a, *b;
+				struct commit_list *exclude;
+
+				a = lookup_commit_reference(from_sha1);
+				b = lookup_commit_reference(sha1);
+				if (!a || !b) {
+					die(symmetric ?
+					    "Invalid symmetric difference expression %s...%s" :
+					    "Invalid revision range %s..%s",
+					    arg, next);
+				}
 
 				if (!seen_dashdash) {
 					*dotdot = '.';
 					verify_non_filename(revs->prefix, arg);
 				}
-				add_pending_object(revs, exclude, this);
-				add_pending_object(revs, include, next);
+
+				if (symmetric) {
+					exclude = get_merge_bases_clean(a, b);
+					add_pending_commit_list(revs, exclude,
+					                        flags_exclude);
+					a->object.flags |= flags;
+				} else
+					a->object.flags |= flags_exclude;
+				b->object.flags |= flags;
+				add_pending_object(revs, &a->object, this);
+				add_pending_object(revs, &b->object, next);
 				continue;
 			}
 			*dotdot = '.';

-- 
1.4.1.rc2.gfc04

[PATCH 3/3] Make clear_commit_marks() clean harder

From: Rene Scharfe <hidden>
Date: 2016-06-15 22:42:32

Don't care if objects have been parsed or not and don't stop when we
reach a commit that is already clean -- its parents could be dirty.

Signed-off-by: Rene Scharfe <redacted>
---
 commit.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index 593414d..70a4eff 100644
--- a/commit.c
+++ b/commit.c

@@ -397,13 +397,12 @@ void clear_commit_marks(struct commit *c
 {
 	struct commit_list *parents;
 
+	if (!commit)
+		return;
 	parents = commit->parents;
 	commit->object.flags &= ~mark;
 	while (parents) {
-		struct commit *parent = parents->item;
-		if (parent && parent->object.parsed &&
-		    (parent->object.flags & mark))
-			clear_commit_marks(parent, mark);
+		clear_commit_marks(parents->item, mark);
 		parents = parents->next;
 	}
 }

-- 
1.4.1.rc2.gfc04

Re: [PATCH 1/3] Add get_merge_bases_clean()

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:42:32

Hi,

On Sun, 2 Jul 2006, Rene Scharfe wrote:

Also move the object flags used in get_merge_bases() out of the range 
defined in revision.h.  This fixes the "66ae0c77...ced9456a 
89719209...262a6ef7" test of the ... operator which is introduced with 
the next patch.

Of COURSE! *hits his head on the table; table breaks; has to buy a new 
table*

Ciao,
Dscho

[PATCH 4/3] Fold get_merge_bases_clean() into get_merge_bases()

From: Rene Scharfe <hidden>
Date: 2016-06-15 22:42:32

Due to popular request ;-), change get_merge_bases() to be able to
clean up after itself if needed by adding a cleanup parameter.

We don't need to save the flags and restore them afterwards anymore;
that was a leftover from before the flags were moved out of the
range used in revision.c.  clear_commit_marks() sets them to zero,
which is enough.

Signed-off-by: Rene Scharfe <redacted>

diff --git a/commit.c b/commit.c
index 70a4eff..94c1d0e 100644
--- a/commit.c
+++ b/commit.c

@@ -1012,7 +1012,8 @@ static void mark_reachable_commits(struc
 	}
 }
 
-struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2)
+struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2,
+                                    int cleanup)
 {
 	struct commit_list *list = NULL;
 	struct commit_list *result = NULL;

@@ -1080,20 +1081,10 @@ struct commit_list *get_merge_bases(stru
 		tmp = next;
 	}
 
-	return result;
-}
-
-struct commit_list *get_merge_bases_clean(struct commit *rev1,
-                                          struct commit *rev2)
-{
-	unsigned int flags1 = rev1->object.flags;
-	unsigned int flags2 = rev2->object.flags;
-	struct commit_list *result = get_merge_bases(rev1, rev2);
-
-	clear_commit_marks(rev1, PARENT1 | PARENT2 | UNINTERESTING);
-	clear_commit_marks(rev2, PARENT1 | PARENT2 | UNINTERESTING);
-	rev1->object.flags = flags1;
-	rev2->object.flags = flags2;
+	if (cleanup) {
+		clear_commit_marks(rev1, PARENT1 | PARENT2 | UNINTERESTING);
+		clear_commit_marks(rev2, PARENT1 | PARENT2 | UNINTERESTING);
+	}
 
 	return result;
 }

diff --git a/commit.h b/commit.h
index 3a26e29..779ed82 100644
--- a/commit.h
+++ b/commit.h

@@ -105,7 +105,6 @@ struct commit_graft *read_graft_line(cha
 int register_commit_graft(struct commit_graft *, int);
 int read_graft_file(const char *graft_file);
 
-extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
-extern struct commit_list *get_merge_bases_clean(struct commit *rev1, struct commit *rev2);
+extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);
 
 #endif /* COMMIT_H */

diff --git a/merge-base.c b/merge-base.c
index b41f76c..59f723f 100644
--- a/merge-base.c
+++ b/merge-base.c

@@ -6,7 +6,7 @@ static int show_all = 0;
 
 static int merge_base(struct commit *rev1, struct commit *rev2)
 {
-	struct commit_list *result = get_merge_bases(rev1, rev2);
+	struct commit_list *result = get_merge_bases(rev1, rev2, 0);
 
 	if (!result)
 		return 1;

diff --git a/revision.c b/revision.c
index 1da6ce3..7865fa4 100644
--- a/revision.c
+++ b/revision.c

@@ -814,7 +814,7 @@ int setup_revisions(int argc, const char
 				}
 
 				if (symmetric) {
-					exclude = get_merge_bases_clean(a, b);
+					exclude = get_merge_bases(a, b, 1);
 					add_pending_commit_list(revs, exclude,
 					                        flags_exclude);
 					a->object.flags |= flags;

Re: [PATCH 4/3] Fold get_merge_bases_clean() into get_merge_bases()

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:42:32

Hi,

On Sun, 2 Jul 2006, Rene Scharfe wrote:

Due to popular request ;-), change get_merge_bases() to be able to
clean up after itself if needed by adding a cleanup parameter.

We don't need to save the flags and restore them afterwards anymore;
that was a leftover from before the flags were moved out of the
range used in revision.c.  clear_commit_marks() sets them to zero,
which is enough.

Acked-by: Johannes Schindelin <redacted>

Ciao,
Dscho

Re: [PATCH 4/3] Fold get_merge_bases_clean() into get_merge_bases()

From: Linus Torvalds <torvalds@osdl.org>
Date: 2016-06-15 22:42:32


On Sun, 2 Jul 2006, Rene Scharfe wrote:

Due to popular request ;-), change get_merge_bases() to be able to
clean up after itself if needed by adding a cleanup parameter.

Btw, I think the symmetric thing is still wrong.

Try this:

	git rev-list ^HEAD~1 HEAD...HEAD~2

which _should_ return just HEAD ("HEAD...HEAD~2" should basically expand 
into "HEAD HEAD~2 ^HEAD~2")

I haven't actually tested your patch, but I think it returns HEAD and 
HEAD~1.

The reason? You clear UNINTERESTING as part of clear_commit_marks(), so 
HEAD~1, which was marked thus by the user, gets cleared of its mark as 
part of the get_merge_bases() invocation.

I suspect the only way to fix that is to make "get_merge_bases()" not use 
UNINTERESTING at all, but instead just explicitly use something like

	(obj.flags & (LEFT | RIGHT)) == (LEFT | RIGHT)

instead.

			Linus

Re: [PATCH 4/3] Fold get_merge_bases_clean() into get_merge_bases()

From: Rene Scharfe <hidden>
Date: 2016-06-15 22:42:32

Linus Torvalds schrieb:

On Sun, 2 Jul 2006, Rene Scharfe wrote:

quoted

Due to popular request ;-), change get_merge_bases() to be able to
clean up after itself if needed by adding a cleanup parameter.

Btw, I think the symmetric thing is still wrong.

Try this:

	git rev-list ^HEAD~1 HEAD...HEAD~2

which _should_ return just HEAD ("HEAD...HEAD~2" should basically expand 
into "HEAD HEAD~2 ^HEAD~2")

I haven't actually tested your patch, but I think it returns HEAD and 
HEAD~1.

The reason? You clear UNINTERESTING as part of clear_commit_marks(), so 
HEAD~1, which was marked thus by the user, gets cleared of its mark as 
part of the get_merge_bases() invocation.

I suspect the only way to fix that is to make "get_merge_bases()" not use 
UNINTERESTING at all, but instead just explicitly use something like

No and yes.  Patch 1 in the 3+1 series changes the flags used in
commit.c to not conflict with the ones in revision.h[*].  So we have two
different UNINTERESTINGs, and get_merge_bases() doesn't mess up the
show/no-show markings.

René


[*] That fix was the one which reportedly made Dscho break a table.
Sorry for that by the way. :-P

Re: [PATCH 4/3] Fold get_merge_bases_clean() into get_merge_bases()

From: Linus Torvalds <torvalds@osdl.org>
Date: 2016-06-15 22:42:32


On Sun, 2 Jul 2006, Rene Scharfe wrote:

No and yes.  Patch 1 in the 3+1 series changes the flags used in
commit.c to not conflict with the ones in revision.h[*].  So we have two
different UNINTERESTINGs, and get_merge_bases() doesn't mess up the
show/no-show markings.

Gaah. So UNINTERESTING in commit.c needs something else than everywhere 
else? That's a bug waiting to happen.

Please give it a name of its own.

		Linus

`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