Re: [PATCH] Generalize and libify...

Re: [PATCH] Generalize and libify index_is_dirty() to index_differs_from(...)

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:08

Stephan Beyer [off-list ref] writes:

  This is one of the sequencer-preparing patches.
  (The function is used in sequencer several times, most of the time
   with diff_flags set to DIFF_OPT_IGNORE_SUBMODULES.)

  Alex is on Cc because he introduced the "Is commitable?" (i.e.
  "Is index dirty?") part in builtin-commit.c.

  Peff is on Cc because he introduced index_is_dirty() in
  builtin-revert.c.

 builtin-commit.c |   13 ++-----------
 builtin-revert.c |   13 +------------
 revision.c       |   15 +++++++++++++++
 revision.h       |    2 ++
 4 files changed, 20 insertions(+), 23 deletions(-)

It is a straightforward and clean restructuring, but please do not
contaminate revision.[ch] with this function about "internally running
diff-index".  

revision.[ch] is a library for revision/ancestry traversal and it is
already one of the largest library-ish files. It does not know nor care
about the index, and we want to keep it that way.  Please keep its focus
to revision traversal.

Perhaps diff-lib.c would be a better home for your helper function.

[PATCH v2] Generalize and libify index_is_dirty() to index_differs_from(...)

From: Stephan Beyer <hidden>
Date: 2016-06-15 22:46:08

index_is_dirty() in builtin-revert.c checks if the index is dirty.
This patch generalizes this function to check if the index differs
from a revision, i.e. the former index_is_dirty() behavior can now be
achieved by index_differs_from("HEAD", 0).

The second argument "diff_flags" allows to set further diff option
flags like DIFF_OPT_IGNORE_SUBMODULES. See DIFF_OPT_* macros in diff.h
for a list.

index_differs_from() seems to be useful for more than builtin-revert.c,
so it is moved into diff-lib.c and also used in builtin-commit.c.

Yet to mention:

 - "rev.abbrev = 0;" can be safely removed.
   This has no impact on performance or functioning of neither
   setup_revisions() nor run_diff_index().

 - rev.pending.objects is free()d because this fixes a leak.
   (Also see 295dd2ad "Fix memory leak in traverse_commit_list")

Mentored-by: Daniel Barkalow [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Signed-off-by: Stephan Beyer <redacted>
---

  Err, this didn't get to the list, did it?

 builtin-commit.c |   13 ++-----------
 builtin-revert.c |   13 +------------
 diff-lib.c       |   15 +++++++++++++++
 diff.h           |    2 ++
 4 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index d6a3a62..46e649c 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c

@@ -561,7 +561,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 		commitable = run_status(fp, index_file, prefix, 1);
 		wt_status_use_color = saved_color_setting;
 	} else {
-		struct rev_info rev;
 		unsigned char sha1[20];
 		const char *parent = "HEAD";

@@ -573,16 +572,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 
 		if (get_sha1(parent, sha1))
 			commitable = !!active_nr;
-		else {
-			init_revisions(&rev, "");
-			rev.abbrev = 0;
-			setup_revisions(0, NULL, &rev, parent);
-			DIFF_OPT_SET(&rev.diffopt, QUIET);
-			DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
-			run_diff_index(&rev, 1 /* cached */);
-
-			commitable = !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
-		}
+		else
+			commitable = index_differs_from(parent, 0);
 	}
 
 	fclose(fp);

diff --git a/builtin-revert.c b/builtin-revert.c
index d48313c..d210150 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c

@@ -223,17 +223,6 @@ static char *help_msg(const unsigned char *sha1)
 	return helpbuf;
 }
 
-static int index_is_dirty(void)
-{
-	struct rev_info rev;
-	init_revisions(&rev, NULL);
-	setup_revisions(0, NULL, &rev, "HEAD");
-	DIFF_OPT_SET(&rev.diffopt, QUIET);
-	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
-	run_diff_index(&rev, 1);
-	return !!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES);
-}
-
 static struct tree *empty_tree(void)
 {
 	struct tree *tree = xcalloc(1, sizeof(struct tree));

@@ -279,7 +268,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	} else {
 		if (get_sha1("HEAD", head))
 			die ("You do not have a valid HEAD");
-		if (index_is_dirty())
+		if (index_differs_from("HEAD", 0))
 			die ("Dirty index: cannot %s", me);
 	}
 	discard_cache();

diff --git a/diff-lib.c b/diff-lib.c
index a41e1ec..79d0606 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -513,3 +513,18 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 		exit(128);
 	return 0;
 }
+
+int index_differs_from(const char *def, int diff_flags)
+{
+	struct rev_info rev;
+
+	init_revisions(&rev, NULL);
+	setup_revisions(0, NULL, &rev, def);
+	DIFF_OPT_SET(&rev.diffopt, QUIET);
+	DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS);
+	rev.diffopt.flags |= diff_flags;
+	run_diff_index(&rev, 1);
+	if (rev.pending.alloc)
+		free(rev.pending.objects);
+	return (DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0);
+}

diff --git a/diff.h b/diff.h
index 23cd90c..6703a4f 100644
--- a/diff.h
+++ b/diff.h

@@ -265,4 +265,6 @@ extern int diff_result_code(struct diff_options *, int);
 
 extern void diff_no_index(struct rev_info *, int, const char **, int, const char *);
 
+extern int index_differs_from(const char *def, int diff_flags);
+
 #endif /* DIFF_H */

-- 
1.6.2.rc0.464.g3ec3

`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