# [PATCH] Add -O option to diff-* brothers. 13 messages from 2005-05-28 to 2005-05-30. Participants: Junio C Hamano, Thomas Glanzmann, Petr Baudis, Linus Torvalds. Thread: https://gitlist.dev/t/745 ## Junio C Hamano, 2005-05-28 09:54 Subject: [PATCH] Add -O option to diff-* brothers. Message-ID: <7vzmufwu8w.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vzmufwu8w.fsf%40assigned-by-dhcp.cox.net ``` 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 --- 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 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] [--pickaxe-all] [--cached] [...] +'git-diff-cache' [-p] [-r] [-z] [-m] [-M] [-R] [-C] [-O] [-S] [--pickaxe-all] [--cached] [...] DESCRIPTION ----------- @@ -49,6 +49,10 @@ OPTIONS changeset, not just the files that contains the change in . +-O:: + Output the patch in the order specified in the + , 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] [--pickaxe-all] [...] +'git-diff-files' [-p] [-q] [-r] [-z] [-M] [-C] [-R] [-O] [-S] [--pickaxe-all] [...] DESCRIPTION ----------- @@ -43,6 +43,10 @@ OPTIONS changeset, not just the files that contains the change in . +-O:: + Output the patch in the order specified in the + , 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] [--pickaxe-all] [-m] [-s] [-v] [-t]

[]\* +'git-diff-tree' [-p] [-r] [-z] [--stdin] [-M] [-R] [-C] [-O] [-S] [--pickaxe-all] [-m] [-s] [-v] [-t]

[]\* DESCRIPTION ----------- @@ -50,6 +50,10 @@ OPTIONS changeset, not just the files that contains the change in . +-O:: + Output the patch in the order specified in the + , 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 + +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, 2005-05-28 10:59 Subject: Re: [PATCH] Pickaxe fixes. Message-ID: <20050528105914.GA8467@cip.informatik.uni-erlangen.de> URL: https://gitlist.dev/e/20050528105914.GA8467%40cip.informatik.uni-erlangen.de In-Reply-To: <7vzmufwu8w.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Petr Baudis, 2005-05-28 11:12 Subject: Re: [PATCH] Add -O option to diff-* brothers. Message-ID: <20050528111238.GA1036@pasky.ji.cz> URL: https://gitlist.dev/e/20050528111238.GA1036%40pasky.ji.cz In-Reply-To: <7vzmufwu8w.fsf@assigned-by-dhcp.cox.net> ``` Dear diary, on Sat, May 28, 2005 at 11:54:39AM CEST, I got a letter where Junio C Hamano told me that... > 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 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, 2005-05-28 11:19 Subject: Re: [PATCH] Add -O option to diff-* brothers. Message-ID: <20050528111959.GA28780@cip.informatik.uni-erlangen.de> URL: https://gitlist.dev/e/20050528111959.GA28780%40cip.informatik.uni-erlangen.de In-Reply-To: <20050528111238.GA1036@pasky.ji.cz> ``` 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, 2005-05-28 16:14 Subject: Re: [PATCH] Add -O option to diff-* brothers. Message-ID: <7vu0knuy36.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vu0knuy36.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20050528111238.GA1036@pasky.ji.cz> ``` >>>>> "PB" == Petr Baudis 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, 2005-05-28 16:16 Subject: Re: [PATCH] Pickaxe fixes. Message-ID: <7vpsvbuxzb.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vpsvbuxzb.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20050528105914.GA8467@cip.informatik.uni-erlangen.de> ``` >>>>> "TG" == Thomas Glanzmann 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, 2005-05-28 16:22 Subject: Re: [PATCH] Pickaxe fixes. Message-ID: <20050528162257.GE4881@cip.informatik.uni-erlangen.de> URL: https://gitlist.dev/e/20050528162257.GE4881%40cip.informatik.uni-erlangen.de In-Reply-To: <7vpsvbuxzb.fsf@assigned-by-dhcp.cox.net> ``` Hello, > 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, 2005-05-28 16:25 Subject: Re: [PATCH] Add -O option to diff-* brothers. Message-ID: <7vk6ljuxlt.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk6ljuxlt.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20050528111959.GA28780@cip.informatik.uni-erlangen.de> ``` >>>>> "TG" == Thomas Glanzmann 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, 2005-05-28 16:44 Subject: [PATCH] Fix leak in diffcore-order.c Message-ID: <7vekbruwom.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vekbruwom.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7vzmufwu8w.fsf@assigned-by-dhcp.cox.net> ``` A temporary array used for sorting the filepairs were not freed after use. Signed-off-by: Junio C Hamano --- diffcore-order.c | 1 + 1 files changed, 1 insertion(+) 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, 2005-05-29 18:56 Subject: Re: [PATCH] Add -O option to diff-* brothers. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0505291154030.10545%40ppc970.osdl.org In-Reply-To: <7vzmufwu8w.fsf@assigned-by-dhcp.cox.net> ``` On Sat, 28 May 2005, Junio C Hamano wrote: > > 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, 2005-05-29 20:32 Subject: Re: [PATCH] Add -O option to diff-* brothers. Message-ID: <7vll5xkc3a.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vll5xkc3a.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` >>>>> "LT" == Linus Torvalds 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, 2005-05-30 00:23 Subject: Re: [PATCH] Add -O option to diff-* brothers. Message-ID: <7vekbpftp4.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vekbpftp4.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` >>>>> "LT" == Linus Torvalds 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. ``` ## Junio C Hamano, 2005-05-30 07:35 Subject: Re: [PATCH] Pickaxe fixes. Message-ID: <7vk6lh6u9g.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk6lh6u9g.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20050528162257.GE4881@cip.informatik.uni-erlangen.de> ``` >>>>> "TG" == Thomas Glanzmann 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. ```