threads / patch / 745

patchAdd -O<orderfile> option to diff-* brothers.

Subject: [PATCH] Add -O<orderfile> option to diff-* brothers.

## tl;dr

13 messages between May 28, 2005 and May 30, 2005. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Junio C Hamano· May 28, 2005, 09:54 UTC · lore

A new diffcore filter diffcore-order is introduced. This takes a text file each of whose line is a shell glob pattern. Patches matching the glob pattern on an earlier line in the file are output before patches matching the glob pattern on a later line, and patches not matching any glob pattern is output last.

A typical orderfile for git project would probably look like this:

    README
    Documentation
    Makefile
    *.h
    *.c
Signed-off-by: Junio C Hamano <junkio@cox.net>
---

Documentation/git-diff-cache.txt | 6 + Documentation/git-diff-files.txt | 6 + Documentation/git-diff-tree.txt | 6 + Makefile | 3 diff-cache.c | 7 ++ diff-files.c | 5 + diff-tree.c | 7 ++ diff.h | 2 diffcore-order.c | 121 +++++++++++++++++++++++++++++++++++++++ 9 files changed, 159 insertions(+), 4 deletions(-) new file (100644): diffcore-order.c

Show changes to 9 files +159 −4

Documentation/git-diff-cache.txt, Documentation/git-diff-files.txt, Documentation/git-diff-tree.txt, Makefile, diff-cache.c, diff-files.c, diff-tree.c, diff.h, diffcore-order.c

diff --git a/Documentation/git-diff-cache.txt b/Documentation/git-diff-cache.txt
--- a/Documentation/git-diff-cache.txt
+++ b/Documentation/git-diff-cache.txt
@@ -9,7 +9,7 @@ git-diff-cache - Compares content and mo
 
 SYNOPSIS
 --------
-'git-diff-cache' [-p] [-r] [-z] [-m] [-M] [-R] [-C] [-S<string>] [--pickaxe-all] [--cached] <tree-ish> [<path>...]
+'git-diff-cache' [-p] [-r] [-z] [-m] [-M] [-R] [-C] [-O<orderfile>] [-S<string>] [--pickaxe-all] [--cached] <tree-ish> [<path>...]
 
 DESCRIPTION
 -----------
@@ -49,6 +49,10 @@ OPTIONS
 	changeset, not just the files that contains the change
 	in <string>.
 
+-O<orderfile>::
+	Output the patch in the order specified in the
+	<orderfile>, which has one shell glob pattern per line.
+
 -R::
 	Output diff in reverse.
 
diff --git a/Documentation/git-diff-files.txt b/Documentation/git-diff-files.txt
--- a/Documentation/git-diff-files.txt
+++ b/Documentation/git-diff-files.txt
@@ -9,7 +9,7 @@ git-diff-files - Compares files in the w
 
 SYNOPSIS
 --------
-'git-diff-files' [-p] [-q] [-r] [-z] [-M] [-C] [-R] [-S<string>] [--pickaxe-all] [<pattern>...]
+'git-diff-files' [-p] [-q] [-r] [-z] [-M] [-C] [-R] [-O<orderfile>] [-S<string>] [--pickaxe-all] [<pattern>...]
 
 DESCRIPTION
 -----------
@@ -43,6 +43,10 @@ OPTIONS
 	changeset, not just the files that contains the change
 	in <string>.
 
+-O<orderfile>::
+	Output the patch in the order specified in the
+	<orderfile>, which has one shell glob pattern per line.
+
 -r::
 	This flag does not mean anything.  It is there only to match
 	git-diff-tree.  Unlike git-diff-tree, git-diff-files always looks
diff --git a/Documentation/git-diff-tree.txt b/Documentation/git-diff-tree.txt
--- a/Documentation/git-diff-tree.txt
+++ b/Documentation/git-diff-tree.txt
@@ -9,7 +9,7 @@ git-diff-tree - Compares the content and
 
 SYNOPSIS
 --------
-'git-diff-tree' [-p] [-r] [-z] [--stdin] [-M] [-R] [-C] [-S<string>] [--pickaxe-all] [-m] [-s] [-v] [-t] <tree-ish> <tree-ish> [<pattern>]\*
+'git-diff-tree' [-p] [-r] [-z] [--stdin] [-M] [-R] [-C] [-O<orderfile>] [-S<string>] [--pickaxe-all] [-m] [-s] [-v] [-t] <tree-ish> <tree-ish> [<pattern>]\*
 
 DESCRIPTION
 -----------
@@ -50,6 +50,10 @@ OPTIONS
 	changeset, not just the files that contains the change
 	in <string>.
 
+-O<orderfile>::
+	Output the patch in the order specified in the
+	<orderfile>, which has one shell glob pattern per line.
+
 -r::
 	recurse
 
diff --git a/Makefile b/Makefile
--- a/Makefile
+++ b/Makefile
@@ -48,7 +48,7 @@ LIB_OBJS += strbuf.o
 
 LIB_H += diff.h count-delta.h
 LIB_OBJS += diff.o diffcore-rename.o diffcore-pickaxe.o diffcore-pathspec.o \
-	count-delta.o
+	count-delta.o diffcore-order.o
 
 LIB_OBJS += gitenv.o
 
@@ -130,6 +130,7 @@ diff.o: $(LIB_H) diffcore.h
 diffcore-rename.o : $(LIB_H) diffcore.h
 diffcore-pathspec.o : $(LIB_H) diffcore.h
 diffcore-pickaxe.o : $(LIB_H) diffcore.h
+diffcore-order.o : $(LIB_H) diffcore.h
 
 test: all
 	$(MAKE) -C t/ all
diff --git a/diff-cache.c b/diff-cache.c
--- a/diff-cache.c
+++ b/diff-cache.c
@@ -9,6 +9,7 @@ static int diff_setup_opt = 0;
 static int diff_score_opt = 0;
 static const char *pickaxe = NULL;
 static int pickaxe_opts = 0;
+static const char *orderfile = NULL;
 
 /* A file entry went away or appeared */
 static void show_file(const char *prefix, struct cache_entry *ce, unsigned char *sha1, unsigned int mode)
@@ -210,6 +211,10 @@ int main(int argc, const char **argv)
 			pickaxe = arg + 2;
 			continue;
 		}
+		if (!strncmp(arg, "-O", 2)) {
+			orderfile = arg + 2;
+			continue;
+		}
 		if (!strcmp(arg, "--pickaxe-all")) {
 			pickaxe_opts = DIFF_PICKAXE_ALL;
 			continue;
@@ -246,6 +251,8 @@ int main(int argc, const char **argv)
 		diffcore_rename(detect_rename, diff_score_opt);
 	if (pickaxe)
 		diffcore_pickaxe(pickaxe, pickaxe_opts);
+	if (orderfile)
+		diffcore_order(orderfile);
 	diff_flush(diff_output_format, 1);
 	return ret;
 }
diff --git a/diff-files.c b/diff-files.c
--- a/diff-files.c
+++ b/diff-files.c
@@ -15,6 +15,7 @@ static int diff_setup_opt = 0;
 static int diff_score_opt = 0;
 static const char *pickaxe = NULL;
 static int pickaxe_opts = 0;
+static const char *orderfile = NULL;
 static int silent = 0;
 
 static void show_unmerge(const char *path)
@@ -55,6 +56,8 @@ int main(int argc, const char **argv)
 			diff_setup_opt |= DIFF_SETUP_REVERSE;
 		else if (!strncmp(argv[1], "-S", 2))
 			pickaxe = argv[1] + 2;
+		else if (!strncmp(argv[1], "-O", 2))
+			orderfile = argv[1] + 2;
 		else if (!strcmp(argv[1], "--pickaxe-all"))
 			pickaxe_opts = DIFF_PICKAXE_ALL;
 		else if (!strncmp(argv[1], "-M", 2)) {
@@ -122,6 +125,8 @@ int main(int argc, const char **argv)
 		diffcore_rename(detect_rename, diff_score_opt);
 	if (pickaxe)
 		diffcore_pickaxe(pickaxe, pickaxe_opts);
+	if (orderfile)
+		diffcore_order(orderfile);
 	diff_flush(diff_output_format, 1);
 	return 0;
 }
diff --git a/diff-tree.c b/diff-tree.c
--- a/diff-tree.c
+++ b/diff-tree.c
@@ -14,6 +14,7 @@ static int diff_setup_opt = 0;
 static int diff_score_opt = 0;
 static const char *pickaxe = NULL;
 static int pickaxe_opts = 0;
+static const char *orderfile = NULL;
 static const char *header = NULL;
 static const char *header_prefix = "";
 
@@ -269,6 +270,8 @@ static int call_diff_flush(void)
 		diff_flush(DIFF_FORMAT_NO_OUTPUT, 0);
 		return 0;
 	}
+	if (orderfile)
+		diffcore_order(orderfile);
 	if (header) {
 		const char *fmt = "%s";
 		if (diff_output_format == DIFF_FORMAT_MACHINE)
@@ -510,6 +513,10 @@ int main(int argc, const char **argv)
 			pickaxe = arg + 2;
 			continue;
 		}
+		if (!strncmp(arg, "-O", 2)) {
+			orderfile = arg + 2;
+			continue;
+		}
 		if (!strcmp(arg, "--pickaxe-all")) {
 			pickaxe_opts = DIFF_PICKAXE_ALL;
 			continue;
diff --git a/diff.h b/diff.h
--- a/diff.h
+++ b/diff.h
@@ -43,6 +43,8 @@ extern void diffcore_pickaxe(const char 
 
 extern void diffcore_pathspec(const char **pathspec);
 
+extern void diffcore_order(const char *orderfile);
+
 extern int diff_queue_is_empty(void);
 
 #define DIFF_FORMAT_HUMAN	0
diff --git a/diffcore-order.c b/diffcore-order.c
new file mode 100644
--- /dev/null
+++ b/diffcore-order.c
@@ -0,0 +1,121 @@
+/*
+ * Copyright (C) 2005 Junio C Hamano
+ */
+#include "cache.h"
+#include "diff.h"
+#include "diffcore.h"
+#include <fnmatch.h>
+
+static char **order;
+static int order_cnt;
+
+static void prepare_order(const char *orderfile)
+{
+	int fd, cnt, pass;
+	void *map;
+	char *cp, *endp;
+	struct stat st;
+
+	if (order)
+		return;
+
+	fd = open(orderfile, O_RDONLY);
+	if (fd < 0)
+		return;
+	if (fstat(fd, &st)) {
+		close(fd);
+		return;
+	}
+	map = mmap(NULL, st.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd, 0);
+	close(fd);
+	if (-1 == (int)(long)map)
+		return;
+	endp = map + st.st_size;
+	for (pass = 0; pass < 2; pass++) {
+		cnt = 0;
+		cp = map;
+		while (cp < endp) {
+			char *ep;
+			for (ep = cp; ep < endp && *ep != '\n'; ep++)
+				;
+			/* cp to ep has one line */
+			if (*cp == '\n' || *cp == '#')
+				; /* comment */
+			else if (pass == 0)
+				cnt++;
+			else {
+				if (*ep == '\n') {
+					*ep = 0;
+					order[cnt] = cp;
+				}
+				else {
+					order[cnt] = xmalloc(ep-cp+1);
+					memcpy(order[cnt], cp, ep-cp);
+					order[cnt][ep-cp] = 0;
+				}
+				cnt++;
+			}
+			if (ep < endp)
+				ep++;
+			cp = ep;
+		}
+		if (pass == 0) {
+			order_cnt = cnt;
+			order = xmalloc(sizeof(*order) * cnt);
+		}
+	}
+}
+
+struct pair_order {
+	struct diff_filepair *pair;
+	int orig_order;
+	int order;
+};
+
+static int match_order(const char *path)
+{
+	int i;
+	char p[PATH_MAX];
+
+	for (i = 0; i < order_cnt; i++) {
+		strcpy(p, path);
+		while (p[0]) {
+			char *cp;
+			if (!fnmatch(order[i], p, 0))
+				return i;
+			cp = strrchr(p, '/');
+			if (!cp)
+				break;
+			*cp = 0;
+		}
+	}
+	return order_cnt;
+}
+
+static int compare_pair_order(const void *a_, const void *b_)
+{
+	struct pair_order const *a, *b;
+	a = (struct pair_order const *)a_;
+	b = (struct pair_order const *)b_;
+	if (a->order != b->order)
+		return a->order - b->order;
+	return a->orig_order - b->orig_order;
+}
+
+void diffcore_order(const char *orderfile)
+{
+	struct diff_queue_struct *q = &diff_queued_diff;
+	struct pair_order *o = xmalloc(sizeof(*o) * q->nr);
+	int i;
+
+	prepare_order(orderfile);
+	for (i = 0; i < q->nr; i++) {
+		o[i].pair = q->queue[i];
+		o[i].orig_order = i;
+		o[i].order = match_order(o[i].pair->two->path);
+	}
+	qsort(o, q->nr, sizeof(*o), compare_pair_order);
+	for (i = 0; i < q->nr; i++)
+		q->queue[i] = o[i].pair;
+	return;
+}
------------------------------------------------
Thomas Glanzmann· May 28, 2005, 10:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] Pickaxe fixes.

Hello,
> Also updates the documentation to describe the --pickaxe-all option.
> A typical orderfile for git project would probably look like this ...

I can't believe it. I began to support pickaxe this morning in my little git frontend and thought about exactly these two questions (patch ordering and show me all deltas in the matched changeset). Thanks for the patches! Btw. When doing the. 'git-whatchanged drivers/ide' thing ... does that show only deltas which happened actually in this tree or if a changeset contains deltas inside and outside?

	Thomas
Junio C Hamano· May 28, 2005, 16:16 UTC · re: Thomas Glanzmann · lore

Re: [PATCH] Pickaxe fixes.

>>>>> "TG" == Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:

TG> Btw. When doing the. 'git-whatchanged drivers/ide' thing TG> ... does that show only deltas which happened actually in TG> this tree or if a changeset contains deltas inside and TG> outside?

That's up to Linus right now. The behaviour is different before and after the 12-part patch I sent out yesterday [*1*].

My earlier thinking was to use the pathspec (e.g. drivers/ide) as the output filter, but Linus corrected me yesterday morning that he wants the pathspec as the input filter. What this means to your question is:

 - Under my earlier thinking, if drivers/ide/foo is a rename
   from drivers/usb/bar, diffcore gets both creation of foo and
   deletion of bar, diffcore-rename matches them up, and
   diffcore-pathspec filters the output to drivers/ide tree.
   Earlier pathspec picked up filepair that had the specified
   path on either src or dst side, so this would have been shown
   as a rename from usb/bar to ide/foo.
 - After the 12-part patch, the pathspec comes first and
   establishes a narrowed down world we operate in.  So in the
   same situation, diffcore is fed only creation of foo but does
   not see deletion of bar (which happened in drivers/usb, which
   is outside our world).  Hence diffcore-rename does not have
   anything to match up and this would be shown as a creation of
   ide/foo.

I agree with Linus that the latter is the semantics we usually want. If we make diffcore filters stackable (I mean, the order of applications controllable by the program and the user), we could also have the older semantics when the user wants it, but I would do that after this series stabilizes.

[Footnote]

*1* When I talk about time of the day, it is in US Pacific timezone.

Thomas Glanzmann· May 28, 2005, 16:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] Pickaxe fixes.

Hello,
Show 5 quoted lines
> I agree with Linus that the latter is the semantics we usually
> want.  If we make diffcore filters stackable (I mean, the order
> of applications controllable by the program and the user), we
> could also have the older semantics when the user wants it, but
> I would do that after this series stabilizes.

thanks for the elaboration on this topic. However at the moment I don't have an opinion on this, I have to use it a bit longer. But it is a good thing that I know by now that it limits its view to the subdirectory after your patch-train is applied.

	Thomas
Junio C Hamano· May 30, 2005, 07:35 UTC · re: Thomas Glanzmann · lore

Re: [PATCH] Pickaxe fixes.

>>>>> "TG" == Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:

TG> ... However at the moment I don't TG> have an opinion on this, I have to use it a bit longer. But it is a good TG> thing that I know by now that it limits its view to the subdirectory TG> after your patch-train is applied.

Your opinion on this would not count anymore ;-). Pathspec is now at the beginning of the processiong chain.

Petr Baudis· May 28, 2005, 11:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add -O<orderfile> option to diff-* brothers.

Dear diary, on Sat, May 28, 2005 at 11:54:39AM CEST, I got a letter where Junio C Hamano <junkio@cox.net> told me that...

Show 16 quoted lines
> A new diffcore filter diffcore-order is introduced.  This takes
> a text file each of whose line is a shell glob pattern.  Patches
> matching the glob pattern on an earlier line in the file are
> output before patches matching the glob pattern on a later line,
> and patches not matching any glob pattern is output last.
> 
> A typical orderfile for git project would probably look like
> this:
> 
>     README
>     Documentation
>     Makefile
>     *.h
>     *.c
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

Hmm, is this really useful in practice? diff itself doesn't appear to have it either, and I haven't seen something like this before. So is it worth the code?

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
C++: an octopus made by nailing extra legs onto a dog. -- Steve Taylor
Thomas Glanzmann· May 28, 2005, 11:19 UTC · re: Petr Baudis · lore

Re: [PATCH] Add -O<orderfile> option to diff-* brothers.

Hello,
> Hmm, is this really useful in practice? diff itself doesn't appear to
> have it either, and I haven't seen something like this before. So is it
> worth the code?

I think it is. For example on git development it is nice to see the Documentation first. So you know what it is supposed to do and later you see the actual implementation. I think Junio requested this exactly before.

	Thoms
Junio C Hamano· May 28, 2005, 16:25 UTC · re: Thomas Glanzmann · lore

Re: [PATCH] Add -O<orderfile> option to diff-* brothers.

>>>>> "TG" == Thomas Glanzmann <sithglan@stud.uni-erlangen.de> writes:

TG> I think it is. For example on git development it is nice to see the TG> Documentation first. So you know what it is supposed to do and later you TG> see the actual implementation. I think Junio requested this exactly TG> before.

Well, yes and no. I do want to see the patches in the suggested order because that would make things easier to understand if they are related.

The request you are referring to was not about the order of patches in one e-mail message. It was more like "please show us documentation first so that we can pick nits in the design (goal of implementation) before you start to code". That way, if there were a change (like -A flag in checkout-cache) that people would find objectionable, the effort to code the change and the effort to review the code can both be saved.

Junio C Hamano· May 28, 2005, 16:14 UTC · re: Petr Baudis · lore

Re: [PATCH] Add -O<orderfile> option to diff-* brothers.

>>>>> "PB" == Petr Baudis <pasky@ucw.cz> writes:

PB> Hmm, is this really useful in practice? diff itself doesn't appear to PB> have it either, and I haven't seen something like this before. So is it PB> worth the code?

CVS nor SVN does not do pickaxe as far as I know either, but I think some people in GIT community are already having fun with pickaxe.

The amount of the code is small, and it does not impact if you do not use the option. I am more concerned by (1) the fact that each new diffcore filters would carve out its own niche from the scarse command option namespace; (2) you have to duplicate the option parsing in all three diff-* brothers; and (3) diff-tree is sometimes an odd-man-out among those three and it is not so simple to unify the option parsing. A help in that area is very much appreciated.

Junio C Hamano· May 28, 2005, 16:44 UTC · re: Junio C Hamano · lore

[PATCH] Fix leak in diffcore-order.c

A temporary array used for sorting the filepairs were not freed after use.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

diffcore-order.c | 1 + 1 files changed, 1 insertion(+)

Show changes to diffcore-order.c +1 −0
diff --git a/diffcore-order.c b/diffcore-order.c
--- a/diffcore-order.c
+++ b/diffcore-order.c
@@ -117,5 +117,6 @@ void diffcore_order(const char *orderfil
 	qsort(o, q->nr, sizeof(*o), compare_pair_order);
 	for (i = 0; i < q->nr; i++)
 		q->queue[i] = o[i].pair;
+	free(o);
 	return;
 }
------------------------------------------------
Linus Torvalds· May 29, 2005, 18:56 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add -O<orderfile> option to diff-* brothers.

On Sat, 28 May 2005, Junio C Hamano wrote:
Show 6 quoted lines
>
> A new diffcore filter diffcore-order is introduced.  This takes
> a text file each of whose line is a shell glob pattern.  Patches
> matching the glob pattern on an earlier line in the file are
> output before patches matching the glob pattern on a later line,
> and patches not matching any glob pattern is output last.

This smells to me like a "cool feature, because we can" rather than a "this is useful because of xxx".

You could equally well do it with 
	git-diff-cache .. | sort-filenames | git-diff-helper

in porcelain, if somebody really wants this (but I can't see people clamoring for it, since nobody else has ever done soemthing like this, afaik).

In other words: what is the problem this is trying to solve?
		Linus
Junio C Hamano· May 29, 2005, 20:32 UTC · re: Linus Torvalds · lore

Re: [PATCH] Add -O<orderfile> option to diff-* brothers.

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> This smells to me like a "cool feature, because we can" LT> rather than a "this is useful because of xxx".

You could even do the rename/copy detection outside with a standalone diff-raw filter, just like your "sort-filenames" pipe example. Rename/copy is not done that way primarily for the performance reasons. Pickaxe is not done that way because we wanted to conditionally suppress diff-tree headers, and it is easier to implement the suppression inside rather than as a "sort-filename" like filter.

This diffcore-order is more like "cool feature, because we can, _and_ it is simpler to do it inside rather than outside, now we already have the infrastructure inside to do this kind of thing for other purposes anyway".

Junio C Hamano· May 30, 2005, 00:23 UTC · re: Linus Torvalds · lore

Re: [PATCH] Add -O<orderfile> option to diff-* brothers.

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:
I realize I did not answer your question.
LT> In other words: what is the problem this is trying to solve?

To produce a patch that is easier to review, using customized patch order list for projects. I envision that Porcelain noticing the existence of ${GIT-.git}/patch-order file and adding -O to its diff-* argument would make the world a better place.

← back to recent threads