git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: Graham Perks <gperks@ausperks.net>, Jeff King <peff@peff.net>,
	Git List <git@vger.kernel.org>
Subject: Clean up and simplify rev_compare_tree()
Date: Tue, 2 Jun 2009 18:34:01 -0700 (PDT)	[thread overview]
Message-ID: <alpine.LFD.2.01.0906021820570.4880@localhost.localdomain> (raw)
In-Reply-To: <alpine.LFD.2.01.0906021439030.4880@localhost.localdomain>


This simplifies the logic of rev_compare_tree() by removing a special 
case. 

It does so by turning the special case of finding a diff to be "all new 
files" into a more generic case of "all new" vs "all removed" vs "mixed 
changes", so now the code is actually more powerful and more generic, and 
the added symmetry actually makes it simpler too.

This makes no changes to any existing behavior, but apart from the 
simplification it does make it possible to some day care about whether all 
changes were just deletions if we want to. Which we may well want to for 
merge handling.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This is just a cleanup while I'm looking at the code. There's two things 
that are relevant to merges - the "TREESAME" logic that determines whether 
we should simplify the merge away and pick just one parent, and the actual 
diff creation logic that then creates diffs of the merges that we don't 
simplify away.

The two are totally independent, and this patch just cleans up the helper 
function that we use for the commit simplification logic. 

The only half-way subtle part here (and it really isn't that subtle) is 
that "REV_TREE_NEW | REV_TREE_OLD == REV_TREE_DIFFERENT", which makes 
sense and just simplifies the logic in general. It used to be that mixing 
REV_TREE_NEW state with REV_TREE_DIFFERENT was complicated. Now it's 
trivial, thanks to REV_TREE_OLD that is currently otherwise unused (we 
treat it the same way as REV_TREE_DIFFERENT, which is what that case used 
to result in).

There's an unrelated cleanup which is to just move the "can't happen" 
special case code of a missing commit tree up to the same point as the 
"can't happen" missing parent tree.

On Tue, 2 Jun 2009, Linus Torvalds wrote:
> 
> Now, I admit that in this case the matching heuristic is dubious, and 
> maybe we should consider "does not exist in result" to not match any 
> parent. We already think that "all new" is special ("REV_TREE_NEW" vs 
> "REV_TREE_DIFFERENT"), so maybe we should think that "all deleted" is also 
> special ("REV_TREE_DEL")
> 
> 		Linus
> 

 revision.c |   33 ++++++++++++---------------------
 revision.h |    5 +++--
 2 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/revision.c b/revision.c
index 18b7ebb..bf58448 100644
--- a/revision.c
+++ b/revision.c
@@ -256,10 +256,12 @@ static int everybody_uninteresting(struct commit_list *orig)
 
 /*
  * The goal is to get REV_TREE_NEW as the result only if the
- * diff consists of all '+' (and no other changes), and
- * REV_TREE_DIFFERENT otherwise (of course if the trees are
- * the same we want REV_TREE_SAME).  That means that once we
- * get to REV_TREE_DIFFERENT, we do not have to look any further.
+ * diff consists of all '+' (and no other changes), REV_TREE_OLD
+ * if the whole diff is removal of old data, and otherwise
+ * REV_TREE_DIFFERENT (of course if the trees are the same we
+ * want REV_TREE_SAME).
+ * That means that once we get to REV_TREE_DIFFERENT, we do not
+ * have to look any further.
  */
 static int tree_difference = REV_TREE_SAME;
 
@@ -268,22 +270,9 @@ static void file_add_remove(struct diff_options *options,
 		    const unsigned char *sha1,
 		    const char *fullpath)
 {
-	int diff = REV_TREE_DIFFERENT;
+	int diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;
 
-	/*
-	 * Is it an add of a new file? It means that the old tree
-	 * didn't have it at all, so we will turn "REV_TREE_SAME" ->
-	 * "REV_TREE_NEW", but leave any "REV_TREE_DIFFERENT" alone
-	 * (and if it already was "REV_TREE_NEW", we'll keep it
-	 * "REV_TREE_NEW" of course).
-	 */
-	if (addremove == '+') {
-		diff = tree_difference;
-		if (diff != REV_TREE_SAME)
-			return;
-		diff = REV_TREE_NEW;
-	}
-	tree_difference = diff;
+	tree_difference |= diff;
 	if (tree_difference == REV_TREE_DIFFERENT)
 		DIFF_OPT_SET(options, HAS_CHANGES);
 }
@@ -305,6 +294,8 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct
 
 	if (!t1)
 		return REV_TREE_NEW;
+	if (!t2)
+		return REV_TREE_OLD;
 
 	if (revs->simplify_by_decoration) {
 		/*
@@ -323,8 +314,7 @@ static int rev_compare_tree(struct rev_info *revs, struct commit *parent, struct
 		if (!revs->prune_data)
 			return REV_TREE_SAME;
 	}
-	if (!t2)
-		return REV_TREE_DIFFERENT;
+
 	tree_difference = REV_TREE_SAME;
 	DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);
 	if (diff_tree_sha1(t1->object.sha1, t2->object.sha1, "",
@@ -429,6 +419,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit)
 				p->parents = NULL;
 			}
 		/* fallthrough */
+		case REV_TREE_OLD:
 		case REV_TREE_DIFFERENT:
 			tree_changed = 1;
 			pp = &parent->next;
diff --git a/revision.h b/revision.h
index be39e7d..227164c 100644
--- a/revision.h
+++ b/revision.h
@@ -118,8 +118,9 @@ struct rev_info {
 };
 
 #define REV_TREE_SAME		0
-#define REV_TREE_NEW		1
-#define REV_TREE_DIFFERENT	2
+#define REV_TREE_NEW		1	/* Only new files */
+#define REV_TREE_OLD		2	/* Only files removed */
+#define REV_TREE_DIFFERENT	3	/* Mixed changes */
 
 /* revision.c */
 void read_revisions_from_stdin(struct rev_info *revs);

  parent reply	other threads:[~2009-06-03  1:34 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-06-02 19:33 Am able to delete a file with no trace in the log Graham Perks
2009-06-02 20:29 ` Tony Finch
2009-06-02 21:34 ` Jeff King
2009-06-02 21:55   ` Linus Torvalds
2009-06-03  0:47     ` Sitaram Chamarty
2009-06-03  1:20       ` Linus Torvalds
2009-06-03  1:34     ` Linus Torvalds [this message]
2009-06-03  1:57     ` Junio C Hamano
2009-06-03  3:03       ` Linus Torvalds
2009-06-03 21:18         ` Junio C Hamano
2009-06-03 21:26           ` Linus Torvalds
2009-06-03 21:33           ` Linus Torvalds
2009-06-03 21:59             ` Avery Pennarun
2009-06-03 22:02             ` Junio C Hamano
2009-06-03 22:17               ` Linus Torvalds
2009-06-03 22:38                 ` Junio C Hamano
2009-06-03 22:44                   ` Jeff King
2009-06-03 22:56                     ` Linus Torvalds
2009-06-03 23:06                       ` Jeff King
2009-06-03 23:27                         ` Linus Torvalds
2009-06-03 23:37                           ` Jeff King
2009-06-04  1:58                             ` Linus Torvalds
2009-06-04  6:57                       ` Junio C Hamano
2009-06-03 22:47                   ` Linus Torvalds
2009-06-03 22:56                 ` Junio C Hamano
2009-06-03 21:55         ` 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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=alpine.LFD.2.01.0906021820570.4880@localhost.localdomain \
    --to=torvalds@linux-foundation.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=gperks@ausperks.net \
    --cc=peff@peff.net \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).