Git development
 help / color / mirror / Atom feed
* [PATCH 3/3] Added diff hunk coloring to git-add--interactive
From: Dan Zwell @ 2007-11-11  0:03 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, Shawn O. Pearce, Wincent Colaiuta,
	Git Mailing List, Jonathan del Strother, Johannes Schindelin,
	Frank Lichtenheld
In-Reply-To: <20071104054305.GA13929@sigill.intra.peff.net>

Added and integrated method "color_diff_hunk", which colors
lines, and returns them in an array. Coloring bad whitespace is
not yet supported.

Signed-off-by: Dan Zwell <dzwell@zwell.net>
---
 git-add--interactive.perl |   58 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 508531f..d92e8ed 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -3,7 +3,11 @@
 use strict;
 use Git;
 
+# Prompt colors:
 my ($use_color, $prompt_color, $header_color, $help_color, $normal_color);
+# Diff colors:
+my ($diff_use_color, $new_color, $old_color, $fraginfo_color,
+    $metainfo_color, $whitespace_color);
 my $color_config = qx(git config --get color.interactive);
 if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
 	eval { require Term::ANSIColor; };
@@ -21,6 +25,24 @@ if ($color_config=~/true|always/ || -t STDOUT && $color_config=~/auto/) {
 		$help_color = Git::color_to_ansi_code(
 			Git::config($repo, "color.interactive.help") || "red bold");
 		$normal_color = Git::color_to_ansi_code("normal");
+
+		# Do we also set diff colors?
+		my $diff_colors = Git::config($repo, "color.diff");
+		if ($diff_colors=~/true/ ||
+			-t STDOUT && $diff_colors=~/auto/) {
+			$diff_use_color = 1;
+			$new_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.new") || "green");
+			$old_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.old") || "red");
+			$fraginfo_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.frag") || "cyan");
+			$metainfo_color = Git::color_to_ansi_code(
+				Git::config($repo, "color.diff.meta") || "bold");
+			# Not implemented:
+			#$whitespace_color = Git::color_to_ansi_code(
+				#Git::config($repo, "color.diff.whitespace") || "normal red");
+		}
 	}
 }
 
@@ -389,6 +411,31 @@ sub parse_diff {
 	return @hunk;
 }
 
+sub colored_diff_hunk {
+	my ($text) = @_;
+	# return the text, so that it can be passed to print()
+	my @ret;
+	for (@$text) {
+		if (!$diff_use_color) {
+			push @ret, $_;
+			next;
+		}
+
+		if (/^\+/) {
+			push @ret, colored($new_color, $_);
+		} elsif (/^\-/) {
+			push @ret, colored($old_color, $_);
+		} elsif (/^\@/) {
+			push @ret, colored($fraginfo_color, $_);
+		} elsif (/^ /) {
+			push @ret, colored($normal_color, $_);
+		} else {
+			push @ret, colored($metainfo_color, $_);
+		}
+	}
+	return @ret;
+}
+
 sub hunk_splittable {
 	my ($text) = @_;
 
@@ -611,9 +658,7 @@ sub patch_update_cmd {
 	my ($ix, $num);
 	my $path = $it->{VALUE};
 	my ($head, @hunk) = parse_diff($path);
-	for (@{$head->{TEXT}}) {
-		print;
-	}
+	print colored_diff_hunk($head->{TEXT});
 	$num = scalar @hunk;
 	$ix = 0;
 
@@ -655,9 +700,7 @@ sub patch_update_cmd {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
-		for (@{$hunk[$ix]{TEXT}}) {
-			print;
-		}
+		print colored_diff_hunk($hunk[$ix]{TEXT});
 		print colored $prompt_color, "Stage this hunk [y/n/a/d$other/?]? ";
 		my $line = <STDIN>;
 		if ($line) {
@@ -795,8 +838,7 @@ sub diff_cmd {
 				     HEADER => $status_head, },
 				   @mods);
 	return if (!@them);
-	system(qw(git diff-index -p --cached HEAD --),
-	       map { $_->{VALUE} } @them);
+	system(qw(git diff -p --cached HEAD --), map { $_->{VALUE} } @them);
 }
 
 sub quit_cmd {
-- 
1.5.3.5.565.gf0b83-dirty

^ permalink raw reply related

* Re: Deprecate git-fetch-pack?
From: Junio C Hamano @ 2007-11-11  0:48 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0711101752490.29952@iabervon.org>

Daniel Barkalow <barkalow@iabervon.org> writes:

> Now that git-fetch is in C, built in, and doing the fetch-pack in the same 
> process, the normal usage patterns don't involve actually executing 
> git-fetch-pack. Can we deprecate it at this point, or it is plausibly 
> being used by scripts? As it is now, I'm not entirely confidant that the 
> tests in t5500 won't be fooled by git-fetch working even with 
> git-fetch-pack being broken in various ways, which should be fixed if we 
> want to keep it.
>
> We also might as well deprecate peek-remote now that it's a synonym for 
> ls-remote.

Especially because git-fetch is no longer as hackable as it used
to be, and because people may still find special needs that can
be hacked up with direct access to low level transports from the
script more easily than going down to the C level, I'd rather
wait and see for a cycle or two to decide.  There is no strong
reason to drop it, is there?

As to peek-remote, ls-remote over the native transport is a
synonym so I do not think anybody doing the scripting would have
problem doing s/peek-/ls-/ to their script, but again I do not
see a heavy maintenance burden to keep the synonym, yet.

^ permalink raw reply

* Re: [PATCH] status&commit: Teach them to show commits of modified submodules.
From: Lars Hjemli @ 2007-11-11  0:07 UTC (permalink / raw)
  To: Ping Yin; +Cc: git
In-Reply-To: <1194722863-14741-1-git-send-email-pkufranky@gmail.com>

On Nov 10, 2007 8:27 PM, Ping Yin <pkufranky@gmail.com> wrote:
> This commit teaches git status/commit to also show commits of user-cared
> modified submodules since HEAD (or HEAD^ if --amend option is on).

Some nitpicks:
-we'll need a config option to enable/disable this output in git-status
-the feature should probably be implemented in git-submodule.sh

--
larsh

^ permalink raw reply

* [PATCH] Reduce the number of connects when fetching
From: Daniel Barkalow @ 2007-11-10 23:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

This shares the connection between getting the remote ref list and
getting objects in the first batch. (A second connection is still used
to follow tags)
---
As far as I can tell, this works and is suitable for pu. But it may well 
have subtle issues, and may nearly totally break git-fetch-pack (as an 
independant program). FWIW, I didn't get any feedback other than general 
encouragement last time I tried.

 builtin-fetch-pack.c |   74 ++++++++++++++++++++++++++-----------------------
 fetch-pack.h         |    2 +
 transport.c          |   41 +++++++++++++++++++--------
 3 files changed, 70 insertions(+), 47 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index bb1742f..688a8dc 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -7,6 +7,7 @@
 #include "pack.h"
 #include "sideband.h"
 #include "fetch-pack.h"
+#include "remote.h"
 #include "run-command.h"
 
 static int transfer_unpack_limit = -1;
@@ -558,14 +559,14 @@ static int get_pack(int xd[2], char **pack_lockfile)
 }
 
 static struct ref *do_fetch_pack(int fd[2],
+		const struct ref *orig_ref,
 		int nr_match,
 		char **match,
 		char **pack_lockfile)
 {
-	struct ref *ref;
+	struct ref *ref = copy_ref_list(orig_ref);
 	unsigned char sha1[20];
 
-	get_remote_heads(fd[0], &ref, 0, NULL, 0);
 	if (is_repository_shallow() && !server_supports("shallow"))
 		die("Server does not support shallow clients");
 	if (server_supports("multi_ack")) {
@@ -583,10 +584,6 @@ static struct ref *do_fetch_pack(int fd[2],
 			fprintf(stderr, "Server supports side-band\n");
 		use_sideband = 1;
 	}
-	if (!ref) {
-		packet_flush(fd[1]);
-		die("no matching remote head");
-	}
 	if (everything_local(&ref, nr_match, match)) {
 		packet_flush(fd[1]);
 		goto all_done;
@@ -660,7 +657,7 @@ static void fetch_pack_setup(void)
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
 	int i, ret, nr_heads;
-	struct ref *ref;
+	struct ref *ref = NULL;
 	char *dest = NULL, **heads;
 
 	nr_heads = 0;
@@ -716,9 +713,34 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 	if (!dest)
 		usage(fetch_pack_usage);
 
-	ref = fetch_pack(&args, dest, nr_heads, heads, NULL);
+	int fd[2];
+	struct child_process *conn = git_connect(fd, (char *)dest, args.uploadpack,
+                          args.verbose ? CONNECT_VERBOSE : 0);
+	if (conn) {
+		get_remote_heads(fd[0], &ref, 0, NULL, 0);
+
+		ref = fetch_pack(&args, fd, conn, ref, dest, nr_heads, heads, NULL);
+		close(fd[0]);
+		close(fd[1]);
+		if (finish_connect(conn))
+			ref = NULL;
+	} else {
+		ref = NULL;
+	}
 	ret = !ref;
 
+	if (!ret && nr_heads) {
+		/* If the heads to pull were given, we should have
+		 * consumed all of them by matching the remote.
+		 * Otherwise, 'git-fetch remote no-such-ref' would
+		 * silently succeed without issuing an error.
+		 */
+		for (i = 0; i < nr_heads; i++)
+			if (heads[i] && heads[i][0]) {
+				error("no such remote ref %s", heads[i]);
+				ret = 1;
+			}
+	}
 	while (ref) {
 		printf("%s %s\n",
 		       sha1_to_hex(ref->old_sha1), ref->name);
@@ -729,16 +751,15 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 }
 
 struct ref *fetch_pack(struct fetch_pack_args *my_args,
+		       int fd[], pid_t pid,
+		       const struct ref *ref,
 		const char *dest,
 		int nr_heads,
 		char **heads,
 		char **pack_lockfile)
 {
-	int i, ret;
-	int fd[2];
-	struct child_process *conn;
-	struct ref *ref;
 	struct stat st;
+	struct ref *ref_cpy;
 
 	fetch_pack_setup();
 	memcpy(&args, my_args, sizeof(args));
@@ -747,29 +768,15 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 			st.st_mtime = 0;
 	}
 
-	conn = git_connect(fd, (char *)dest, args.uploadpack,
-                          args.verbose ? CONNECT_VERBOSE : 0);
 	if (heads && nr_heads)
 		nr_heads = remove_duplicates(nr_heads, heads);
-	ref = do_fetch_pack(fd, nr_heads, heads, pack_lockfile);
-	close(fd[0]);
-	close(fd[1]);
-	ret = finish_connect(conn);
-
-	if (!ret && nr_heads) {
-		/* If the heads to pull were given, we should have
-		 * consumed all of them by matching the remote.
-		 * Otherwise, 'git-fetch remote no-such-ref' would
-		 * silently succeed without issuing an error.
-		 */
-		for (i = 0; i < nr_heads; i++)
-			if (heads[i] && heads[i][0]) {
-				error("no such remote ref %s", heads[i]);
-				ret = 1;
-			}
+	if (!ref) {
+		packet_flush(fd[1]);
+		die("no matching remote head");
 	}
+	ref_cpy = do_fetch_pack(fd, ref, nr_heads, heads, pack_lockfile);
 
-	if (!ret && args.depth > 0) {
+	if (args.depth > 0) {
 		struct cache_time mtime;
 		char *shallow = git_path("shallow");
 		int fd;
@@ -798,8 +805,5 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 		}
 	}
 
-	if (ret)
-		ref = NULL;
-
-	return ref;
+	return ref_cpy;
 }
diff --git a/fetch-pack.h b/fetch-pack.h
index a7888ea..8d35ef6 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -16,6 +16,8 @@ struct fetch_pack_args
 };
 
 struct ref *fetch_pack(struct fetch_pack_args *args,
+		int fd[], struct child_process *conn,
+		const struct ref *ref,
 		const char *dest,
 		int nr_heads,
 		char **heads,
diff --git a/transport.c b/transport.c
index 83677fc..3cb0115 100644
--- a/transport.c
+++ b/transport.c
@@ -568,6 +568,8 @@ struct git_transport_data {
 	unsigned thin : 1;
 	unsigned keep : 1;
 	int depth;
+	struct child_process *conn;
+	int fd[2];
 	const char *uploadpack;
 	const char *receivepack;
 };
@@ -598,20 +600,20 @@ static int set_git_option(struct transport *connection,
 	return 1;
 }
 
+static int connect_setup(struct transport *transport)
+{
+	struct git_transport_data *data = transport->data;
+	data->conn = git_connect(data->fd, transport->url, data->uploadpack, 0);
+	return 0;
+}
+
 static struct ref *get_refs_via_connect(struct transport *transport)
 {
 	struct git_transport_data *data = transport->data;
 	struct ref *refs;
-	int fd[2];
-	char *dest = xstrdup(transport->url);
-	struct child_process *conn = git_connect(fd, dest, data->uploadpack, 0);
 
-	get_remote_heads(fd[0], &refs, 0, NULL, 0);
-	packet_flush(fd[1]);
-
-	finish_connect(conn);
-
-	free(dest);
+	connect_setup(transport);
+	get_remote_heads(data->fd[0], &refs, 0, NULL, 0);
 
 	return refs;
 }
@@ -622,7 +624,7 @@ static int fetch_refs_via_pack(struct transport *transport,
 	struct git_transport_data *data = transport->data;
 	char **heads = xmalloc(nr_heads * sizeof(*heads));
 	char **origh = xmalloc(nr_heads * sizeof(*origh));
-	struct ref *refs;
+	const struct ref *refs;
 	char *dest = xstrdup(transport->url);
 	struct fetch_pack_args args;
 	int i;
@@ -637,13 +639,27 @@ static int fetch_refs_via_pack(struct transport *transport,
 
 	for (i = 0; i < nr_heads; i++)
 		origh[i] = heads[i] = xstrdup(to_fetch[i]->name);
-	refs = fetch_pack(&args, dest, nr_heads, heads, &transport->pack_lockfile);
+
+	refs = transport_get_remote_refs(transport);
+	if (!data->conn) {
+		struct ref *refs_tmp;
+		connect_setup(transport);
+		get_remote_heads(data->fd[0], &refs_tmp, 0, NULL, 0);
+		free_refs(refs_tmp);
+	}
+
+	refs = fetch_pack(&args, data->fd, data->conn, transport->remote_refs,
+			  dest, nr_heads, heads, &transport->pack_lockfile);
+	close(data->fd[0]);
+	close(data->fd[1]);
+	if (finish_connect(data->conn))
+		refs = NULL;
+	data->conn = NULL;
 
 	for (i = 0; i < nr_heads; i++)
 		free(origh[i]);
 	free(origh);
 	free(heads);
-	free_refs(refs);
 	free(dest);
 	return 0;
 }
@@ -725,6 +741,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		ret->disconnect = disconnect_git;
 
 		data->thin = 1;
+		data->conn = NULL;
 		data->uploadpack = "git-upload-pack";
 		if (remote && remote->uploadpack)
 			data->uploadpack = remote->uploadpack;
-- 
1.5.3.5.628.g8e762-dirty

^ permalink raw reply related

* Deprecate git-fetch-pack?
From: Daniel Barkalow @ 2007-11-10 23:11 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

Now that git-fetch is in C, built in, and doing the fetch-pack in the same 
process, the normal usage patterns don't involve actually executing 
git-fetch-pack. Can we deprecate it at this point, or it is plausibly 
being used by scripts? As it is now, I'm not entirely confidant that the 
tests in t5500 won't be fooled by git-fetch working even with 
git-fetch-pack being broken in various ways, which should be fixed if we 
want to keep it.

We also might as well deprecate peek-remote now that it's a synonym for 
ls-remote.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* Re: Inconsistencies with git log
From: Miles Bader @ 2007-11-10 22:51 UTC (permalink / raw)
  To: Jon Smirl; +Cc: Johannes Schindelin, Git Mailing List
In-Reply-To: <9e4733910711071503va92a653s25fd978989d5917d@mail.gmail.com>

"Jon Smirl" <jonsmirl@gmail.com> writes:
> But if you type 'git log' while cd'd into a subdirectory the whole log
> is almost never what you want. It's this kind of thing that makes git
> harder to use.

Actually I almost always want the "whole project history" when cd'd to a
subdir.

Git's convention of doing the whole project by default, but allowing "."
(or any other directory name) to narrow it down seems almost perfect to
me.

-Miles

-- 
"Don't just question authority,
Don't forget to question me."
-- Jello Biafra

^ permalink raw reply

* Re: t7005 and vi in GIT_EXEC_PATH
From: Brian Gernhardt @ 2007-11-10 22:45 UTC (permalink / raw)
  To: David Kastrup; +Cc: Git Mailing List
In-Reply-To: <85abpl69ck.fsf@lola.goethe.zz>


On Nov 10, 2007, at 5:09 PM, David Kastrup wrote:

> Brian Gernhardt <benji@silverinsanity.com> writes:
>
>> If vi is in GIT_EXEC_PATH, then t7005-editor.sh fails because the  
>> real
>> vi is invoked instead of the test vi script.  This is because the git
>> wrapper puts GIT_EXEC_PATH ahead of ".".  I see no easy solution to
>> this problem, and thought I should bring it up with the list.
>
> Putting "." at the front of GIT_EXEC_PATH instead of PATH would appear
> to do the trick then, wouldn't it?

The GIT_EXEC_PATH I was referring to is the one set in the Makefile  
and compiled into git.  The GIT_EXEC_PATH environment variable is set  
to the git repository.  PATH ends up looking like this (paraphrased):   
"git.git:install directory:.:normal PATH".  And since the install  
directory contains vi, the test fails (actually appears to hang  
because vi is waiting for input while it's output is being sent to / 
dev/null).

~~ Brian

^ permalink raw reply

* Re: [PATCH] Make git-clean a builtin
From: Miles Bader @ 2007-11-10 22:43 UTC (permalink / raw)
  To: Bill Lear; +Cc: Johannes Schindelin, Shawn Bohrer, gitster, git
In-Reply-To: <18225.48553.44088.269677@lisa.zopyra.com>

Bill Lear <rael@zopyra.com> writes:
> Why do you find it objectionable?

It bloats the code, and makes it less readable.

[My conjecture is that the latter happens because braces are so visually
striking that they attract the eye; for _long_ blocks, this property of
braces helps, because it makes it easier to find them amongst the rest
of the code, but for _short_ blocks, it hurts, because it draws the eye
away from the actual code, and emphasizes structure at a time when
structure doesn't need emphasizing (because it's utterly obvious already
with such short blocks).]

-Miles

-- 
Run away!  Run away!

^ permalink raw reply

* Re: [PATCH v4] user-manual: Add section "Why bisecting merge commits can be harder ..."
From: Steffen Prohaska @ 2007-11-10 22:41 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Linus Torvalds, Benoit Sigoure, Andreas Ericsson, Johannes Sixt,
	git
In-Reply-To: <7vtzntlufh.fsf@gitster.siamese.dyndns.org>


On Nov 10, 2007, at 9:25 PM, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Sat, 10 Nov 2007, Steffen Prohaska wrote:
>>> +
>>> +But if you already made a merge C instead of rebasing, all
>>> +is not lost.  In the illustrated case, you can easily rebase
>>> +one parent branch on top of the other after the fact, just
>>> +to understand the history and to make the history more
>>> +easily to bisect.
>>
>> I simply don't think this is true.
>>
>> You can *not* easily rebase after the fact. Both the parents may  
>> have lots
>> of merges independently of each other, and rebase won't be able to do
>> *anything* with such a case. In fact, the only reason you think  
>> you can
>> easily rebase after-the-fact is that you made the example history be
>> trivial. In real life, if the example history is that trivial (just a
>> single merge of two otherwise linear histories), you'd probably  
>> generally
>> not have this problem in the first place.
>>
>> The facts are:
>>
>>  - git bisect can handle merges quite well, and points to the right
>>    commit (which is *usually* not a merge).
>>
>>  - but merges by definition introduce changes from two independent  
>> lines
>>    of development, and while both lines may work well on their  
>> own, it is
>>    possible that (a) the merge itself was done incorrectly or (b)  
>> the two
>>    (or more) features that were introduced simply clash.
>>
>>  - "git rebase" won't do a thing for this after the fact, except in
>>    trivial cases, and even then it will generate a new history  
>> that simply
>>    isn't compatible with the original history, so while it could  
>> in theory
>>    help bisect things further in some limited and simple cases, in  
>> general
>>    it's simply not that useful an approach.
>>
>> ..and I think it's simply wrong to even *try* to imply that "git  
>> rebase"
>> can somehow change any of this.
>
> Very true.  It is a suggestion useful _only_ when you can easily
> rebase.  Like the one illustrated in the description, which is
> artificial and of very limited scope.  If you cannot rebase
> easily, then (by definition) rebase would not help you.

I propose to remove this suggestion if no one comes up with a
real world example where rebasing after the fact turned out to
be useful. If nobody has such an example it doesn't make sense
to tell readers of the manual that rebase could be useful in
such a situation.

The patch would be identical except for the last paragraph
removed.

	Steffen

^ permalink raw reply

* Re: [PATCH 0/5] some shell portability fixes
From: Miles Bader @ 2007-11-10 22:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ralf Wildenhues, git
In-Reply-To: <7v8x5bgl04.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:
>       /bin/sh on Solaris does not count as you can configure
>       SHELL_PATH to point at xpg4 shell or ksh on that platform.

That's likely to be a rather confusing point for many people trying to
install however (solaris is rather common), unless git somehow arranges
for the right thing to happen automatically, or at least spits out a big
noticeable warning message at install time...

-Miles
-- 
Saa, shall we dance?  (from a dance-class advertisement)

^ permalink raw reply

* Re: t7005 and vi in GIT_EXEC_PATH
From: David Kastrup @ 2007-11-10 22:09 UTC (permalink / raw)
  To: Brian Gernhardt; +Cc: Git Mailing List
In-Reply-To: <9A9986E7-E03D-458A-9A19-A3EF0E7B203D@silverinsanity.com>

Brian Gernhardt <benji@silverinsanity.com> writes:

> If vi is in GIT_EXEC_PATH, then t7005-editor.sh fails because the real
> vi is invoked instead of the test vi script.  This is because the git
> wrapper puts GIT_EXEC_PATH ahead of ".".  I see no easy solution to
> this problem, and thought I should bring it up with the list.

Putting "." at the front of GIT_EXEC_PATH instead of PATH would appear
to do the trick then, wouldn't it?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

^ permalink raw reply

* t7005 and vi in GIT_EXEC_PATH
From: Brian Gernhardt @ 2007-11-10 22:03 UTC (permalink / raw)
  To: Git Mailing List

If vi is in GIT_EXEC_PATH, then t7005-editor.sh fails because the real  
vi is invoked instead of the test vi script.  This is because the git  
wrapper puts GIT_EXEC_PATH ahead of ".".  I see no easy solution to  
this problem, and thought I should bring it up with the list.

~~ Brian

^ permalink raw reply

* gitk/git-gui misfeature: saving the geometry can the window out of reach
From: Benoit Sigoure @ 2007-11-10 21:48 UTC (permalink / raw)
  To: Git Mailing List

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

Hello list,

gitk (and I think git-gui too) save their "geometry" (which includes  
X/Y position) in ~/.gitk.  So far so good.  The problem is that I  
often use 2 screens at work (one is the screen of my laptop, the  
other one is above) and I happen to put my gitk/git-gui windows on  
the 2nd screen.  When I go back home, I don't have a second screen  
and gitk/git-gui open their windows out of reach.  This could well be  
an ugly wart in Apple's port of Tcl/Tk (I have a MacBook with OSX  
10.4.10) or a more general problem (didn't try with other OSes).

git-gui version 0.8.4
git version 1.5.3.5.654.gdd5ec (tonight's master's HEAD)
Tcl/Tk version 8.4.7 (Apple's)
(gitk doesn't support --version!)

For those struggling with the same problem, here is a quick workaround:
   gnused -i /geometry/d ~/.gitk

Cheers,

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

^ permalink raw reply

* Re: [PATCH] status&commit: Teach them to show commits of modified submodules.
From: Junio C Hamano @ 2007-11-10 21:14 UTC (permalink / raw)
  To: Ping Yin; +Cc: git
In-Reply-To: <1194722863-14741-1-git-send-email-pkufranky@gmail.com>

Ping Yin <pkufranky@gmail.com> writes:

> 	# submodule modifiled: sm1 sm2
> 	#
> 	# * sm1 354cd45...3f751e5:
> 	#   <<<
> 	#   	one line message for C
> 	#   	one line message for B
> 	#   >>>
> 	#   	one line message for D
> 	#   	one line message for E
> 	#
> 	# * sm2 5c8bfb5...ac46d84:
> 	#   <<<
> 	#   	msg
> 	#
> 	# On branch master
> 	# Changes to be committed:
> 	#   (use "git reset HEAD <file>..." to unstage)
> 	#
> 	#	modified:   sm1
> 	#	modified:   sm2

I think this presentation order is horrible.

 * I think everbody preferes to have "On branch master" at the
   very beginning, to avoid committing to a wrong branch by
   mistake.

 * As I understand it, in the real life use, there will be quite
   many commits from the submodule updates when a new commit is
   bound to a submodule in the superproject, as _the_ point of
   having a submodule is to bind a more or less independent
   project that progresses at quite a different pace as a
   submodule to the superproject.  In other words, by design,
   the superproject can stay behind from the tip of subproject
   and rebind it to a different commit only when there are
   significant changes of the subproject that need to be there
   to allow the other parts of the superproject (either
   superproject itself or another submodule) to use the features
   and/or fixes the submodule updates provides.

   Which means it will not be uncommon have hundreds of "one
   line message" for the submodules at the very beginning of the
   commit log message buffer, and your prsentation order will
   make that part overwhelm the overview of what changed _in_
   the supermodule itself (the "Changes to be committed:"
   lines), which gives the birds-eye view.

   And I think it is more important to give the birds-eye view
   of the supermodule itself first, when you are helping to
   prepare a commit message for the supermodule.  The user would
   start the commit log for the superproject with "This updates
   the new frotz feature.  It uses the updated API from the
   submodules A and B so we now use updated versions of them."
   and then continue "Notable changes in submodule A are ...".
   And the new part you are adding would help the user to write
   the latter description.

I also find "<<< lines then >>> other lines" format very hard to
read.  Maybe formatting it like this would make it a bit more
readable and more space efficient?

 	# * sm1 354cd45...3f751e5:
 	#   - one line message for C
 	#   - one line message for B
 	#   + one line message for D
 	#   + one line message for E
 	# * sm2 5c8bfb5...ac46d84:
 	#   - msg

Note that if you swap the order and move this at the tail
(perhaps before "Untracked files:" section, if you do not have a
decent .gitignore set up), you can also lose the "submodules
modified: sm1 sm2" line and the blank line before it, which
would make the output even shorter without losing any useful
information.

^ permalink raw reply

* Re: [PATCH 2/2] --pretty=format: on-demand format expansion
From: Jeff King @ 2007-11-10 20:36 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: René Scharfe, Junio C Hamano, Paul Mackerras,
	Git Mailing List, Pierre Habouzit
In-Reply-To: <Pine.LNX.4.64.0711101605560.4362@racer.site>

On Sat, Nov 10, 2007 at 04:07:10PM +0000, Johannes Schindelin wrote:

> > [...] have cooked up a different one on top of a cleaned up version of 
> > mine.  It plays the dirty trick of reading expansions of repeated 
> > placeholders from the strbuf..
> 
> ... which would not work (likely even segfault) if you work with the same 
> private data on different strbufs.

The nice thing is that the cache validity only lasts for the call to
strbuf_expand (and its callbacks), so there is really very little chance
of somebody mis-using this.

-Peff

^ permalink raw reply

* Re: [PATCH 2/2] --pretty=format: on-demand format expansion
From: Jeff King @ 2007-11-10 20:34 UTC (permalink / raw)
  To: René Scharfe
  Cc: Junio C Hamano, Paul Mackerras, Git Mailing List, Pierre Habouzit,
	Johannes Schindelin
In-Reply-To: <47359221.7090707@lsrfire.ath.cx>

On Sat, Nov 10, 2007 at 12:12:33PM +0100, René Scharfe wrote:

> Yes, the two are independent.  I don't like the malloc()'s in your
> patch, though, and have cooked up a different one on top of a cleaned up
> version of mine.  It plays the dirty trick of reading expansions of
> repeated placeholders from the strbuf..

Yes, the mallocs would obviously not be necessary for the commit
parsing, since we already have a buffer, but I assumed they would be for
the abbreviations.

Your solution leaves me partly disgusted at the hackishness, and partly
delighted at the cleverness. I think the way you have coded it is quite
clear, though, so I don't think anyone is likely to get confused reading
it. I haven't read through it carefully yet, but a tentative Ack from
me.

-Peff

^ permalink raw reply

* Re: [PATCH v4] user-manual: Add section "Why bisecting merge commits can be harder ..."
From: Junio C Hamano @ 2007-11-10 20:25 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Steffen Prohaska, Benoit Sigoure, Andreas Ericsson, Johannes Sixt,
	git
In-Reply-To: <alpine.LFD.0.999.0711101105340.15101@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sat, 10 Nov 2007, Steffen Prohaska wrote:
>> +
>> +But if you already made a merge C instead of rebasing, all
>> +is not lost.  In the illustrated case, you can easily rebase
>> +one parent branch on top of the other after the fact, just
>> +to understand the history and to make the history more
>> +easily to bisect.
>
> I simply don't think this is true. 
>
> You can *not* easily rebase after the fact. Both the parents may have lots 
> of merges independently of each other, and rebase won't be able to do 
> *anything* with such a case. In fact, the only reason you think you can 
> easily rebase after-the-fact is that you made the example history be 
> trivial. In real life, if the example history is that trivial (just a 
> single merge of two otherwise linear histories), you'd probably generally 
> not have this problem in the first place.
>
> The facts are:
>
>  - git bisect can handle merges quite well, and points to the right 
>    commit (which is *usually* not a merge).
>
>  - but merges by definition introduce changes from two independent lines 
>    of development, and while both lines may work well on their own, it is 
>    possible that (a) the merge itself was done incorrectly or (b) the two 
>    (or more) features that were introduced simply clash.
>
>  - "git rebase" won't do a thing for this after the fact, except in 
>    trivial cases, and even then it will generate a new history that simply 
>    isn't compatible with the original history, so while it could in theory 
>    help bisect things further in some limited and simple cases, in general 
>    it's simply not that useful an approach.
>
> ..and I think it's simply wrong to even *try* to imply that "git rebase" 
> can somehow change any of this.

Very true.  It is a suggestion useful _only_ when you can easily
rebase.  Like the one illustrated in the description, which is
artificial and of very limited scope.  If you cannot rebase
easily, then (by definition) rebase would not help you.

^ permalink raw reply

* [PATCH] git-hash-object should honor config variables
From: Nicolas Pitre @ 2007-11-10 20:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

... such as core.compression.

Signed-off-by: Nicolas Pitre <nico@cam.org>
---
diff --git a/hash-object.c b/hash-object.c
index 18f5017..0a58f3f 100644
--- a/hash-object.c
+++ b/hash-object.c
@@ -42,6 +42,8 @@ int main(int argc, char **argv)
 	int prefix_length = -1;
 	int no_more_flags = 0;
 
+	git_config(git_default_config);
+
 	for (i = 1 ; i < argc; i++) {
 		if (!no_more_flags && argv[i][0] == '-') {
 			if (!strcmp(argv[i], "-t")) {

^ permalink raw reply related

* Re: [PATCH] status&commit: Teach them to show commits of modified submodules.
From: Sven Verdoolaege @ 2007-11-10 20:00 UTC (permalink / raw)
  To: Ping Yin; +Cc: git
In-Reply-To: <20071110195509.GI2261MdfPADPa@greensroom.kotnet.org>

On Sat, Nov 10, 2007 at 08:55:09PM +0100, Sven Verdoolaege wrote:
> On Sun, Nov 11, 2007 at 03:27:43AM +0800, Ping Yin wrote:
> > This commit teaches git status/commit to also show commits of user-cared
> 
> Does it?  It looks like you only changed git-commit.

OK.  Never mind about this one.

skimo

^ permalink raw reply

* Re: [PATCH] status&commit: Teach them to show commits of modified submodules.
From: Sven Verdoolaege @ 2007-11-10 19:55 UTC (permalink / raw)
  To: Ping Yin; +Cc: git
In-Reply-To: <1194722863-14741-1-git-send-email-pkufranky@gmail.com>

On Sun, Nov 11, 2007 at 03:27:43AM +0800, Ping Yin wrote:
> This commit teaches git status/commit to also show commits of user-cared

Does it?  It looks like you only changed git-commit.
Shouldn't this be put in wt_status_print, if anywhere?

Also, you have some typos:

> +	test -n "$modules" && echo -e "#\n# submodule modifiled: "$modules"\n#"
[..]
> +				echo "  Warn: $name dosn't contains commit $headone"

skimo

^ permalink raw reply

* [PATCH] status&commit: Teach them to show commits of modified submodules.
From: Ping Yin @ 2007-11-10 19:27 UTC (permalink / raw)
  To: git; +Cc: Ping Yin

git status/commit just treats submodules as ordinary files when reporting status
changes. However, one may also wonder how submodules change (the commits).

This commit teaches git status/commit to also show commits of user-cared
modified submodules since HEAD (or HEAD^ if --amend option is on).
Notes:
	1. Submodules already checked out are considered to be user-cared ones.
	2. For submodules deleted or initially added, commits are not shown.

For example, when commiting, submodule sm1 and sm2 are both changed. sm1 has
commit C in HEAD and commit E in index. The history of sm1 is
	--A-->B-->C (in HEAD:354cd45)
	  \
	   -->D->E (in index:3f751e5)

git status will give the following output (just output commits of submodules
before the original output) to show how to change from commit C (in HEAD) to
commit E (in index) for submodule sm1: backward ('<<<') to commit A, and then
forward ('>>>') to commit E. Similar illustration for output of sm2 is omitted.

	#
	# submodule modifiled: sm1 sm2
	#
	# * sm1 354cd45...3f751e5:
	#   <<<
	#   	one line message for C
	#   	one line message for B
	#   >>>
	#   	one line message for D
	#   	one line message for E
	#
	# * sm2 5c8bfb5...ac46d84:
	#   <<<
	#   	msg
	#
	# On branch master
	# Changes to be committed:
	#   (use "git reset HEAD <file>..." to unstage)
	#
	#	modified:   sm1
	#	modified:   sm2

For sm1, if the commit recorded in HEAD/index (say commit C/E) is not found in
the work tree (say sm1 respository in the work tree), a warning will be issued.

	#
	# submodule modifiled: sm1
	#
	# * sm1 354cd45...3f751e5:
	#   Warn: sm1 doesn't contains commit 354cd45
	#
	# On branch master
	# Changes to be committed:
	#   (use "git reset HEAD <file>..." to unstage)
	#
	#	modified:   sm1

Signed-off-by: Ping Yin <pkufranky@gmail.com>
---
 git-commit.sh |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index fcb8443..29f2ebe 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -33,6 +33,68 @@ save_index () {
 	cp -p "$THIS_INDEX" "$NEXT_INDEX"
 }
 
+# Show log of modified submodule (index modification since HEAD or $1)
+# $1 is the commit to be compared, default 'HEAD'
+show_module_log () {
+	cwd=$(pwd)
+	cd_to_toplevel 
+
+	# get modified modules which have been checked out (i.e. cared by user)
+	modules=$(git diff --cached --name-only $1 |
+	(
+		IFS=''	# handle the module name containing space or tab
+		while read name
+		do
+			git ls-files --stage "$name" | grep '^160000 ' >&/dev/null &&
+			GIT_DIR="$name/.git" git-rev-parse --git-dir >&/dev/null &&
+			echo "$name"
+		done
+	)
+	)
+
+	# TODO: quote module names containing space or tab
+	test -n "$modules" && echo -e "#\n# submodule modifiled: "$modules"\n#"
+	OLDIFS=$IFS
+	IFS=$'\n\r'	# '\r' for mac os 
+	for name in $modules
+	do
+		range=$(git diff --cached -- "$name" | sed -n '/^index.*160000$/ p' | awk '{print $2}')
+		indexone=${range#*..}
+		headone=${range%..*}
+		(
+			echo "* $name $headone...$indexone:"
+			headfail=
+			indexfail=
+			GIT_DIR="$name/.git" git-rev-parse $headone >&/dev/null || headfail='t'
+			GIT_DIR="$name/.git" git-rev-parse $indexone >&/dev/null || indexfail='t'
+			case "$headfail,$indexfail" in
+			t,)
+				echo "  Warn: $name dosn't contains commit $headone"
+				;;
+			,t)
+				echo "  Warn: $name dosn't contains commit $indexone"
+				;;
+			t,t)
+				echo "  Warn: $name dosn't contains commits $headone and $indexone"
+				;;
+			*)
+				left=$(GIT_DIR="$name/.git" git log --pretty=oneline $indexone..$headone 2>&1 |
+				sed 's/^\w\+ /  \t/')
+				right=$(GIT_DIR="$name/.git" git log --pretty=oneline --reverse  $headone..$indexone 2>&1 |
+				sed 's/^\w\+ /  \t/')
+
+				test -n "$left" && echo -e "  <<<\n$left"
+				test -n "$right" && echo -e "  >>>\n$right"
+				;;
+			esac
+			echo
+		) | sed 's/^/# /'
+	done
+	IFS=$OLDIFS
+
+	cd "$cwd"
+}
+
 run_status () {
 	# If TMP_INDEX is defined, that means we are doing
 	# "--only" partial commit, and that index file is used
@@ -55,6 +117,12 @@ run_status () {
 	else
 		color=--nocolor
 	fi
+	if test -z "$amend"
+	then
+		show_module_log
+	else
+		show_module_log "HEAD^"
+	fi
 	git runstatus ${color} \
 		${verbose:+--verbose} \
 		${amend:+--amend} \
-- 
1.5.3.4

^ permalink raw reply related

* [PATCH] gitweb: correct month in date display for atom feeds
From: Vincent Zanotti @ 2007-11-10 18:55 UTC (permalink / raw)
  To: git

Signed-off-by: Vincent Zanotti <vincent.zanotti@m4x.org>

---
 gitweb/gitweb.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 759dff1..e788ef9 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1856,7 +1856,7 @@ sub parse_date {
        $date{'mday-time'} = sprintf "%d %s %02d:%02d",
                             $mday, $months[$mon], $hour ,$min;
        $date{'iso-8601'}  = sprintf "%04d-%02d-%02dT%02d:%02d:%02dZ",
-                            1900+$year, $mon, $mday, $hour ,$min, $sec;
+                            1900+$year, 1+$mon, $mday, $hour ,$min, $sec;

        $tz =~ m/^([+\-][0-9][0-9])([0-9][0-9])$/;
        my $local = $epoch + ((int $1 + ($2/60)) * 3600);
--
1.5.3.4

^ permalink raw reply related

* Re: [PATCH v4] user-manual: Add section "Why bisecting merge commits can be harder ..."
From: Linus Torvalds @ 2007-11-10 19:10 UTC (permalink / raw)
  To: Steffen Prohaska
  Cc: Junio C Hamano, Benoit Sigoure, Andreas Ericsson, Johannes Sixt,
	git
In-Reply-To: <1194702594213-git-send-email-prohaska@zib.de>



On Sat, 10 Nov 2007, Steffen Prohaska wrote:
> +
> +But if you already made a merge C instead of rebasing, all
> +is not lost.  In the illustrated case, you can easily rebase
> +one parent branch on top of the other after the fact, just
> +to understand the history and to make the history more
> +easily to bisect.

I simply don't think this is true. 

You can *not* easily rebase after the fact. Both the parents may have lots 
of merges independently of each other, and rebase won't be able to do 
*anything* with such a case. In fact, the only reason you think you can 
easily rebase after-the-fact is that you made the example history be 
trivial. In real life, if the example history is that trivial (just a 
single merge of two otherwise linear histories), you'd probably generally 
not have this problem in the first place.

The facts are:

 - git bisect can handle merges quite well, and points to the right 
   commit (which is *usually* not a merge).

 - but merges by definition introduce changes from two independent lines 
   of development, and while both lines may work well on their own, it is 
   possible that (a) the merge itself was done incorrectly or (b) the two 
   (or more) features that were introduced simply clash.

 - "git rebase" won't do a thing for this after the fact, except in 
   trivial cases, and even then it will generate a new history that simply 
   isn't compatible with the original history, so while it could in theory 
   help bisect things further in some limited and simple cases, in general 
   it's simply not that useful an approach.

..and I think it's simply wrong to even *try* to imply that "git rebase" 
can somehow change any of this.

			Linus

^ permalink raw reply

* [UPDATE DIFF-CLEANUP 1/2] Make the diff_options bitfields be an unsigned with explicit masks.
From: Pierre Habouzit @ 2007-11-10 19:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vy7d95ji1.fsf@gitster.siamese.dyndns.org>

reverse_diff was a bit-value in disguise, it's merged in the flags now.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---
 builtin-blame.c      |   10 ++--
 builtin-diff-files.c |    4 +-
 builtin-diff-index.c |    4 +-
 builtin-diff-tree.c  |    7 ++-
 builtin-diff.c       |   12 +++---
 builtin-log.c        |   24 ++++------
 combine-diff.c       |   10 ++--
 diff-lib.c           |   24 +++++-----
 diff.c               |  123 ++++++++++++++++++++++++-------------------------
 diff.h               |   40 ++++++++++-------
 log-tree.c           |    6 +--
 merge-recursive.c    |    2 +-
 patch-ids.c          |    2 +-
 revision.c           |   22 +++++-----
 tree-diff.c          |   14 +++---
 15 files changed, 155 insertions(+), 149 deletions(-)

diff --git a/builtin-blame.c b/builtin-blame.c
index ba80bf8..c158d31 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -335,7 +335,7 @@ static struct origin *find_origin(struct scoreboard *sb,
 	 * same and diff-tree is fairly efficient about this.
 	 */
 	diff_setup(&diff_opts);
-	diff_opts.recursive = 1;
+	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	diff_opts.detect_rename = 0;
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	paths[0] = origin->path;
@@ -409,7 +409,7 @@ static struct origin *find_rename(struct scoreboard *sb,
 	const char *paths[2];
 
 	diff_setup(&diff_opts);
-	diff_opts.recursive = 1;
+	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	diff_opts.detect_rename = DIFF_DETECT_RENAME;
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diff_opts.single_follow = origin->path;
@@ -1075,7 +1075,7 @@ static int find_copy_in_parent(struct scoreboard *sb,
 		return 1; /* nothing remains for this target */
 
 	diff_setup(&diff_opts);
-	diff_opts.recursive = 1;
+	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 
 	paths[0] = NULL;
@@ -1093,7 +1093,7 @@ static int find_copy_in_parent(struct scoreboard *sb,
 	if ((opt & PICKAXE_BLAME_COPY_HARDEST)
 	    || ((opt & PICKAXE_BLAME_COPY_HARDER)
 		&& (!porigin || strcmp(target->path, porigin->path))))
-		diff_opts.find_copies_harder = 1;
+		DIFF_OPT_SET(&diff_opts, FIND_COPIES_HARDER);
 
 	if (is_null_sha1(target->commit->object.sha1))
 		do_diff_cache(parent->tree->object.sha1, &diff_opts);
@@ -1102,7 +1102,7 @@ static int find_copy_in_parent(struct scoreboard *sb,
 			       target->commit->tree->object.sha1,
 			       "", &diff_opts);
 
-	if (!diff_opts.find_copies_harder)
+	if (!DIFF_OPT_TST(&diff_opts, FIND_COPIES_HARDER))
 		diffcore_std(&diff_opts);
 
 	retval = 0;
diff --git a/builtin-diff-files.c b/builtin-diff-files.c
index 6cb30c8..046b7e3 100644
--- a/builtin-diff-files.c
+++ b/builtin-diff-files.c
@@ -31,5 +31,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_RAW;
 	result = run_diff_files_cmd(&rev, argc, argv);
-	return rev.diffopt.exit_with_status ? rev.diffopt.has_changes: result;
+	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
+		return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
+	return result;
 }
diff --git a/builtin-diff-index.c b/builtin-diff-index.c
index 81e7167..556c506 100644
--- a/builtin-diff-index.c
+++ b/builtin-diff-index.c
@@ -44,5 +44,7 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)
 		return -1;
 	}
 	result = run_diff_index(&rev, cached);
-	return rev.diffopt.exit_with_status ? rev.diffopt.has_changes: result;
+	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
+		return DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
+	return result;
 }
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 0b591c8..2e13716 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -118,8 +118,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!read_stdin)
-		return opt->diffopt.exit_with_status ?
-		    opt->diffopt.has_changes: 0;
+		return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)
+			&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);
 
 	if (opt->diffopt.detect_rename)
 		opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
@@ -134,5 +134,6 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 		else
 			diff_tree_stdin(line);
 	}
-	return opt->diffopt.exit_with_status ? opt->diffopt.has_changes: 0;
+	return DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS)
+		&& DIFF_OPT_TST(&opt->diffopt, HAS_CHANGES);
 }
diff --git a/builtin-diff.c b/builtin-diff.c
index f557d21..1b61599 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -35,7 +35,7 @@ static void stuff_change(struct diff_options *opt,
 	    !hashcmp(old_sha1, new_sha1) && (old_mode == new_mode))
 		return;
 
-	if (opt->reverse_diff) {
+	if (DIFF_OPT_TST(opt, REVERSE_DIFF)) {
 		unsigned tmp;
 		const unsigned char *tmp_u;
 		const char *tmp_c;
@@ -253,13 +253,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 		if (diff_setup_done(&rev.diffopt) < 0)
 			die("diff_setup_done failed");
 	}
-	rev.diffopt.allow_external = 1;
-	rev.diffopt.recursive = 1;
+	DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
+	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 
 	/* If the user asked for our exit code then don't start a
 	 * pager or we would end up reporting its exit code instead.
 	 */
-	if (!rev.diffopt.exit_with_status)
+	if (!DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
 		setup_pager();
 
 	/* Do we have --cached and not have a pending object, then
@@ -363,8 +363,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	else
 		result = builtin_diff_combined(&rev, argc, argv,
 					     ent, ents);
-	if (rev.diffopt.exit_with_status)
-		result = rev.diffopt.has_changes;
+	if (DIFF_OPT_TST(&rev.diffopt, EXIT_WITH_STATUS))
+		result = DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0;
 
 	if (1 < rev.diffopt.skip_stat_unmatch)
 		refresh_index_quietly();
diff --git a/builtin-log.c b/builtin-log.c
index b6ca04a..72745cd 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -55,13 +55,13 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	rev->abbrev = DEFAULT_ABBREV;
 	rev->commit_format = CMIT_FMT_DEFAULT;
 	rev->verbose_header = 1;
-	rev->diffopt.recursive = 1;
+	DIFF_OPT_SET(&rev->diffopt, RECURSIVE);
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
 	argc = setup_revisions(argc, argv, rev, "HEAD");
 	if (rev->diffopt.pickaxe || rev->diffopt.filter)
 		rev->always_show_header = 0;
-	if (rev->diffopt.follow_renames) {
+	if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) {
 		rev->always_show_header = 0;
 		if (rev->diffopt.nr_paths != 1)
 			usage("git logs can only follow renames on one pathname at a time");
@@ -308,11 +308,9 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 			struct tag *t = (struct tag *)o;
 
 			printf("%stag %s%s\n\n",
-					diff_get_color(rev.diffopt.color_diff,
-						DIFF_COMMIT),
+					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					t->tag,
-					diff_get_color(rev.diffopt.color_diff,
-						DIFF_RESET));
+					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			ret = show_object(o->sha1, 1);
 			objects[i].item = (struct object *)t->tagged;
 			i--;
@@ -320,11 +318,9 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 		}
 		case OBJ_TREE:
 			printf("%stree %s%s\n\n",
-					diff_get_color(rev.diffopt.color_diff,
-						DIFF_COMMIT),
+					diff_get_color_opt(&rev.diffopt, DIFF_COMMIT),
 					name,
-					diff_get_color(rev.diffopt.color_diff,
-						DIFF_RESET));
+					diff_get_color_opt(&rev.diffopt, DIFF_RESET));
 			read_tree_recursive((struct tree *)o, "", 0, 0, NULL,
 					show_tree_object);
 			break;
@@ -620,7 +616,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.combine_merges = 0;
 	rev.ignore_merges = 1;
 	rev.diffopt.msg_sep = "";
-	rev.diffopt.recursive = 1;
+	DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
 
 	rev.subject_prefix = fmt_patch_subject_prefix;
 	rev.extra_headers = extra_headers;
@@ -728,8 +724,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (!rev.diffopt.output_format)
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
 
-	if (!rev.diffopt.text)
-		rev.diffopt.binary = 1;
+	if (!DIFF_OPT_TST(&rev.diffopt, TEXT))
+		DIFF_OPT_SET(&rev.diffopt, BINARY);
 
 	if (!output_directory && !use_stdout)
 		output_directory = prefix;
@@ -887,7 +883,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 	revs.diff = 1;
 	revs.combine_merges = 0;
 	revs.ignore_merges = 1;
-	revs.diffopt.recursive = 1;
+	DIFF_OPT_SET(&revs.diffopt, RECURSIVE);
 
 	if (add_pending_commit(head, &revs, 0))
 		die("Unknown commit %s", head);
diff --git a/combine-diff.c b/combine-diff.c
index fe5a2a1..5a658dc 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -664,7 +664,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	int mode_differs = 0;
 	int i, show_hunks;
 	int working_tree_file = is_null_sha1(elem->sha1);
-	int abbrev = opt->full_index ? 40 : DEFAULT_ABBREV;
+	int abbrev = DIFF_OPT_TST(opt, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
 	mmfile_t result_file;
 
 	context = opt->context;
@@ -784,7 +784,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 
 	if (show_hunks || mode_differs || working_tree_file) {
 		const char *abb;
-		int use_color = opt->color_diff;
+		int use_color = DIFF_OPT_TST(opt, COLOR_DIFF);
 		const char *c_meta = diff_get_color(use_color, DIFF_METAINFO);
 		const char *c_reset = diff_get_color(use_color, DIFF_RESET);
 		int added = 0;
@@ -836,7 +836,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			dump_quoted_path("+++ /dev/", "null", c_meta, c_reset);
 		else
 			dump_quoted_path("+++ b/", elem->path, c_meta, c_reset);
-		dump_sline(sline, cnt, num_parent, opt->color_diff);
+		dump_sline(sline, cnt, num_parent, DIFF_OPT_TST(opt, COLOR_DIFF));
 	}
 	free(result);
 
@@ -929,8 +929,8 @@ void diff_tree_combined(const unsigned char *sha1,
 
 	diffopts = *opt;
 	diffopts.output_format = DIFF_FORMAT_NO_OUTPUT;
-	diffopts.recursive = 1;
-	diffopts.allow_external = 0;
+	DIFF_OPT_SET(&diffopts, RECURSIVE);
+	DIFF_OPT_CLR(&diffopts, ALLOW_EXTERNAL);
 
 	show_log_first = !!rev->loginfo && !rev->no_commit_id;
 	needsep = 0;
diff --git a/diff-lib.c b/diff-lib.c
index da55713..290a170 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -121,7 +121,7 @@ static int queue_diff(struct diff_options *o,
 	} else {
 		struct diff_filespec *d1, *d2;
 
-		if (o->reverse_diff) {
+		if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
 			unsigned tmp;
 			const char *tmp_c;
 			tmp = mode1; mode1 = mode2; mode2 = tmp;
@@ -188,8 +188,8 @@ static int handle_diff_files_args(struct rev_info *revs,
 		else if (!strcmp(argv[1], "-n") ||
 				!strcmp(argv[1], "--no-index")) {
 			revs->max_count = -2;
-			revs->diffopt.exit_with_status = 1;
-			revs->diffopt.no_index = 1;
+			DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
+			DIFF_OPT_SET(&revs->diffopt, NO_INDEX);
 		}
 		else if (!strcmp(argv[1], "-q"))
 			*silent = 1;
@@ -207,7 +207,7 @@ static int handle_diff_files_args(struct rev_info *revs,
 		if (!is_in_index(revs->diffopt.paths[0]) ||
 					!is_in_index(revs->diffopt.paths[1])) {
 			revs->max_count = -2;
-			revs->diffopt.no_index = 1;
+			DIFF_OPT_SET(&revs->diffopt, NO_INDEX);
 		}
 	}
 
@@ -258,7 +258,7 @@ int setup_diff_no_index(struct rev_info *revs,
 			break;
 		} else if (i < argc - 3 && !strcmp(argv[i], "--no-index")) {
 			i = argc - 3;
-			revs->diffopt.exit_with_status = 1;
+			DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
 			break;
 		}
 	if (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&
@@ -296,7 +296,7 @@ int setup_diff_no_index(struct rev_info *revs,
 	else
 		revs->diffopt.paths = argv + argc - 2;
 	revs->diffopt.nr_paths = 2;
-	revs->diffopt.no_index = 1;
+	DIFF_OPT_SET(&revs->diffopt, NO_INDEX);
 	revs->max_count = -2;
 	if (diff_setup_done(&revs->diffopt) < 0)
 		die("diff_setup_done failed");
@@ -310,7 +310,7 @@ int run_diff_files_cmd(struct rev_info *revs, int argc, const char **argv)
 	if (handle_diff_files_args(revs, argc, argv, &silent_on_removed))
 		return -1;
 
-	if (revs->diffopt.no_index) {
+	if (DIFF_OPT_TST(&revs->diffopt, NO_INDEX)) {
 		if (revs->diffopt.nr_paths != 2)
 			return error("need two files/directories with --no-index");
 		if (queue_diff(&revs->diffopt, revs->diffopt.paths[0],
@@ -346,7 +346,8 @@ int run_diff_files(struct rev_info *revs, int silent_on_removed)
 		struct cache_entry *ce = active_cache[i];
 		int changed;
 
-		if (revs->diffopt.quiet && revs->diffopt.has_changes)
+		if (DIFF_OPT_TST(&revs->diffopt, QUIET) &&
+			DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
 			break;
 
 		if (!ce_path_match(ce, revs->prune_data))
@@ -442,7 +443,7 @@ int run_diff_files(struct rev_info *revs, int silent_on_removed)
 			continue;
 		}
 		changed = ce_match_stat(ce, &st, 0);
-		if (!changed && !revs->diffopt.find_copies_harder)
+		if (!changed && !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 			continue;
 		oldmode = ntohl(ce->ce_mode);
 		newmode = ntohl(ce_mode_from_stat(ce, st.st_mode));
@@ -561,7 +562,7 @@ static int show_modified(struct rev_info *revs,
 
 	oldmode = old->ce_mode;
 	if (mode == oldmode && !hashcmp(sha1, old->sha1) &&
-	    !revs->diffopt.find_copies_harder)
+	    !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
 		return 0;
 
 	mode = ntohl(mode);
@@ -581,7 +582,8 @@ static int diff_cache(struct rev_info *revs,
 		struct cache_entry *ce = *ac;
 		int same = (entries > 1) && ce_same_name(ce, ac[1]);
 
-		if (revs->diffopt.quiet && revs->diffopt.has_changes)
+		if (DIFF_OPT_TST(&revs->diffopt, QUIET) &&
+			DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
 			break;
 
 		if (!ce_path_match(ce, pathspec))
diff --git a/diff.c b/diff.c
index 6bb902f..97c9a59 100644
--- a/diff.c
+++ b/diff.c
@@ -827,10 +827,10 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 	}
 
 	/* Find the longest filename and max number of changes */
-	reset = diff_get_color(options->color_diff, DIFF_RESET);
-	set = diff_get_color(options->color_diff, DIFF_PLAIN);
-	add_c = diff_get_color(options->color_diff, DIFF_FILE_NEW);
-	del_c = diff_get_color(options->color_diff, DIFF_FILE_OLD);
+	reset = diff_get_color_opt(options, DIFF_RESET);
+	set   = diff_get_color_opt(options, DIFF_PLAIN);
+	add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
+	del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
 
 	for (i = 0; i < data->nr; i++) {
 		struct diffstat_file *file = data->files[i];
@@ -1256,8 +1256,8 @@ static void builtin_diff(const char *name_a,
 	mmfile_t mf1, mf2;
 	const char *lbl[2];
 	char *a_one, *b_two;
-	const char *set = diff_get_color(o->color_diff, DIFF_METAINFO);
-	const char *reset = diff_get_color(o->color_diff, DIFF_RESET);
+	const char *set = diff_get_color_opt(o, DIFF_METAINFO);
+	const char *reset = diff_get_color_opt(o, DIFF_RESET);
 
 	a_one = quote_two("a/", name_a + (*name_a == '/'));
 	b_two = quote_two("b/", name_b + (*name_b == '/'));
@@ -1290,7 +1290,7 @@ static void builtin_diff(const char *name_a,
 			goto free_ab_and_return;
 		if (complete_rewrite) {
 			emit_rewrite_diff(name_a, name_b, one, two,
-					o->color_diff);
+					DIFF_OPT_TST(o, COLOR_DIFF));
 			o->found_changes = 1;
 			goto free_ab_and_return;
 		}
@@ -1299,13 +1299,13 @@ static void builtin_diff(const char *name_a,
 	if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
 		die("unable to read files to diff");
 
-	if (!o->text &&
+	if (!DIFF_OPT_TST(o, TEXT) &&
 	    (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {
 		/* Quite common confusing case */
 		if (mf1.size == mf2.size &&
 		    !memcmp(mf1.ptr, mf2.ptr, mf1.size))
 			goto free_ab_and_return;
-		if (o->binary)
+		if (DIFF_OPT_TST(o, BINARY))
 			emit_binary_diff(&mf1, &mf2);
 		else
 			printf("Binary files %s and %s differ\n",
@@ -1328,7 +1328,7 @@ static void builtin_diff(const char *name_a,
 		memset(&xecfg, 0, sizeof(xecfg));
 		memset(&ecbdata, 0, sizeof(ecbdata));
 		ecbdata.label_path = lbl;
-		ecbdata.color_diff = o->color_diff;
+		ecbdata.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
 		ecbdata.found_changesp = &o->found_changes;
 		xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
 		xecfg.ctxlen = o->context;
@@ -1344,11 +1344,11 @@ static void builtin_diff(const char *name_a,
 		ecb.outf = xdiff_outf;
 		ecb.priv = &ecbdata;
 		ecbdata.xm.consume = fn_out_consume;
-		if (o->color_diff_words)
+		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
 			ecbdata.diff_words =
 				xcalloc(1, sizeof(struct diff_words_data));
 		xdl_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
-		if (o->color_diff_words)
+		if (DIFF_OPT_TST(o, COLOR_DIFF_WORDS))
 			free_diff_words_data(&ecbdata);
 	}
 
@@ -1422,7 +1422,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
 	data.xm.consume = checkdiff_consume;
 	data.filename = name_b ? name_b : name_a;
 	data.lineno = 0;
-	data.color_diff = o->color_diff;
+	data.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
 
 	if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
 		die("unable to read files to diff");
@@ -1866,7 +1866,7 @@ static void run_diff_cmd(const char *pgm,
 			 struct diff_options *o,
 			 int complete_rewrite)
 {
-	if (!o->allow_external)
+	if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
 		pgm = NULL;
 	else {
 		const char *cmd = external_diff_attr(name);
@@ -1964,9 +1964,9 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
 	}
 
 	if (hashcmp(one->sha1, two->sha1)) {
-		int abbrev = o->full_index ? 40 : DEFAULT_ABBREV;
+		int abbrev = DIFF_OPT_TST(o, FULL_INDEX) ? 40 : DEFAULT_ABBREV;
 
-		if (o->binary) {
+		if (DIFF_OPT_TST(o, BINARY)) {
 			mmfile_t mf;
 			if ((!fill_mmfile(&mf, one) && diff_filespec_is_binary(one)) ||
 			    (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))
@@ -2058,7 +2058,10 @@ void diff_setup(struct diff_options *options)
 
 	options->change = diff_change;
 	options->add_remove = diff_addremove;
-	options->color_diff = diff_use_color_default;
+	if (diff_use_color_default)
+		DIFF_OPT_SET(options, COLOR_DIFF);
+	else
+		DIFF_OPT_CLR(options, COLOR_DIFF);
 	options->detect_rename = diff_detect_rename_default;
 }
 
@@ -2077,7 +2080,7 @@ int diff_setup_done(struct diff_options *options)
 	if (count > 1)
 		die("--name-only, --name-status, --check and -s are mutually exclusive");
 
-	if (options->find_copies_harder)
+	if (DIFF_OPT_TST(options, FIND_COPIES_HARDER))
 		options->detect_rename = DIFF_DETECT_COPY;
 
 	if (options->output_format & (DIFF_FORMAT_NAME |
@@ -2101,12 +2104,12 @@ int diff_setup_done(struct diff_options *options)
 				      DIFF_FORMAT_SHORTSTAT |
 				      DIFF_FORMAT_SUMMARY |
 				      DIFF_FORMAT_CHECKDIFF))
-		options->recursive = 1;
+		DIFF_OPT_SET(options, RECURSIVE);
 	/*
 	 * Also pickaxe would not work very well if you do not say recursive
 	 */
 	if (options->pickaxe)
-		options->recursive = 1;
+		DIFF_OPT_SET(options, RECURSIVE);
 
 	if (options->detect_rename && options->rename_limit < 0)
 		options->rename_limit = diff_rename_limit_default;
@@ -2128,9 +2131,9 @@ int diff_setup_done(struct diff_options *options)
 	 * to have found.  It does not make sense not to return with
 	 * exit code in such a case either.
 	 */
-	if (options->quiet) {
+	if (DIFF_OPT_TST(options, QUIET)) {
 		options->output_format = DIFF_FORMAT_NO_OUTPUT;
-		options->exit_with_status = 1;
+		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	}
 
 	/*
@@ -2138,7 +2141,7 @@ int diff_setup_done(struct diff_options *options)
 	 * upon the first hit.  We need to run diff as usual.
 	 */
 	if (options->pickaxe || options->filter)
-		options->quiet = 0;
+		DIFF_OPT_CLR(options, QUIET);
 
 	return 0;
 }
@@ -2201,15 +2204,12 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_PATCH;
 	else if (!strcmp(arg, "--raw"))
 		options->output_format |= DIFF_FORMAT_RAW;
-	else if (!strcmp(arg, "--patch-with-raw")) {
+	else if (!strcmp(arg, "--patch-with-raw"))
 		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_RAW;
-	}
-	else if (!strcmp(arg, "--numstat")) {
+	else if (!strcmp(arg, "--numstat"))
 		options->output_format |= DIFF_FORMAT_NUMSTAT;
-	}
-	else if (!strcmp(arg, "--shortstat")) {
+	else if (!strcmp(arg, "--shortstat"))
 		options->output_format |= DIFF_FORMAT_SHORTSTAT;
-	}
 	else if (!prefixcmp(arg, "--stat")) {
 		char *end;
 		int width = options->stat_width;
@@ -2241,33 +2241,30 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_CHECKDIFF;
 	else if (!strcmp(arg, "--summary"))
 		options->output_format |= DIFF_FORMAT_SUMMARY;
-	else if (!strcmp(arg, "--patch-with-stat")) {
+	else if (!strcmp(arg, "--patch-with-stat"))
 		options->output_format |= DIFF_FORMAT_PATCH | DIFF_FORMAT_DIFFSTAT;
-	}
 	else if (!strcmp(arg, "-z"))
 		options->line_termination = 0;
 	else if (!prefixcmp(arg, "-l"))
 		options->rename_limit = strtoul(arg+2, NULL, 10);
 	else if (!strcmp(arg, "--full-index"))
-		options->full_index = 1;
+		DIFF_OPT_SET(options, FULL_INDEX);
 	else if (!strcmp(arg, "--binary")) {
 		options->output_format |= DIFF_FORMAT_PATCH;
-		options->binary = 1;
-	}
-	else if (!strcmp(arg, "-a") || !strcmp(arg, "--text")) {
-		options->text = 1;
+		DIFF_OPT_SET(options, BINARY);
 	}
+	else if (!strcmp(arg, "-a") || !strcmp(arg, "--text"))
+		DIFF_OPT_SET(options, TEXT);
 	else if (!strcmp(arg, "--name-only"))
 		options->output_format |= DIFF_FORMAT_NAME;
 	else if (!strcmp(arg, "--name-status"))
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;
 	else if (!strcmp(arg, "-R"))
-		options->reverse_diff = 1;
+		DIFF_OPT_SET(options, REVERSE_DIFF);
 	else if (!prefixcmp(arg, "-S"))
 		options->pickaxe = arg + 2;
-	else if (!strcmp(arg, "-s")) {
+	else if (!strcmp(arg, "-s"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
-	}
 	else if (!prefixcmp(arg, "-O"))
 		options->orderfile = arg + 2;
 	else if (!prefixcmp(arg, "--diff-filter="))
@@ -2277,28 +2274,25 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "--pickaxe-regex"))
 		options->pickaxe_opts = DIFF_PICKAXE_REGEX;
 	else if (!prefixcmp(arg, "-B")) {
-		if ((options->break_opt =
-		     diff_scoreopt_parse(arg)) == -1)
+		if ((options->break_opt = diff_scoreopt_parse(arg)) == -1)
 			return -1;
 	}
 	else if (!prefixcmp(arg, "-M")) {
-		if ((options->rename_score =
-		     diff_scoreopt_parse(arg)) == -1)
+		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
 			return -1;
 		options->detect_rename = DIFF_DETECT_RENAME;
 	}
 	else if (!prefixcmp(arg, "-C")) {
 		if (options->detect_rename == DIFF_DETECT_COPY)
-			options->find_copies_harder = 1;
-		if ((options->rename_score =
-		     diff_scoreopt_parse(arg)) == -1)
+			DIFF_OPT_SET(options, FIND_COPIES_HARDER);
+		if ((options->rename_score = diff_scoreopt_parse(arg)) == -1)
 			return -1;
 		options->detect_rename = DIFF_DETECT_COPY;
 	}
 	else if (!strcmp(arg, "--find-copies-harder"))
-		options->find_copies_harder = 1;
+		DIFF_OPT_SET(options, FIND_COPIES_HARDER);
 	else if (!strcmp(arg, "--follow"))
-		options->follow_renames = 1;
+		DIFF_OPT_SET(options, FOLLOW_RENAMES);
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (!prefixcmp(arg, "--abbrev=")) {
@@ -2309,9 +2303,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 			options->abbrev = 40;
 	}
 	else if (!strcmp(arg, "--color"))
-		options->color_diff = 1;
+		DIFF_OPT_SET(options, COLOR_DIFF);
 	else if (!strcmp(arg, "--no-color"))
-		options->color_diff = 0;
+		DIFF_OPT_CLR(options, COLOR_DIFF);
 	else if (!strcmp(arg, "-w") || !strcmp(arg, "--ignore-all-space"))
 		options->xdl_opts |= XDF_IGNORE_WHITESPACE;
 	else if (!strcmp(arg, "-b") || !strcmp(arg, "--ignore-space-change"))
@@ -2319,17 +2313,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "--ignore-space-at-eol"))
 		options->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
 	else if (!strcmp(arg, "--color-words"))
-		options->color_diff = options->color_diff_words = 1;
+		options->flags |= DIFF_OPT_COLOR_DIFF | DIFF_OPT_COLOR_DIFF_WORDS;
 	else if (!strcmp(arg, "--no-renames"))
 		options->detect_rename = 0;
 	else if (!strcmp(arg, "--exit-code"))
-		options->exit_with_status = 1;
+		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
 	else if (!strcmp(arg, "--quiet"))
-		options->quiet = 1;
+		DIFF_OPT_SET(options, QUIET);
 	else if (!strcmp(arg, "--ext-diff"))
-		options->allow_external = 1;
+		DIFF_OPT_SET(options, ALLOW_EXTERNAL);
 	else if (!strcmp(arg, "--no-ext-diff"))
-		options->allow_external = 0;
+		DIFF_OPT_CLR(options, ALLOW_EXTERNAL);
 	else
 		return 0;
 	return 1;
@@ -3084,7 +3078,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
 			 * to determine how many paths were dirty only
 			 * due to stat info mismatch.
 			 */
-			if (!diffopt->no_index)
+			if (!DIFF_OPT_TST(diffopt, NO_INDEX))
 				diffopt->skip_stat_unmatch++;
 			diff_free_filepair(p);
 		}
@@ -3095,10 +3089,10 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
 
 void diffcore_std(struct diff_options *options)
 {
-	if (options->quiet)
+	if (DIFF_OPT_TST(options, QUIET))
 		return;
 
-	if (options->skip_stat_unmatch && !options->find_copies_harder)
+	if (options->skip_stat_unmatch && !DIFF_OPT_TST(options, FIND_COPIES_HARDER))
 		diffcore_skip_stat_unmatch(options);
 	if (options->break_opt != -1)
 		diffcore_break(options->break_opt);
@@ -3113,7 +3107,10 @@ void diffcore_std(struct diff_options *options)
 	diff_resolve_rename_copy();
 	diffcore_apply_filter(options->filter);
 
-	options->has_changes = !!diff_queued_diff.nr;
+	if (diff_queued_diff.nr)
+		DIFF_OPT_SET(options, HAS_CHANGES);
+	else
+		DIFF_OPT_CLR(options, HAS_CHANGES);
 }
 
 
@@ -3137,7 +3134,7 @@ void diff_addremove(struct diff_options *options,
 	 * Before the final output happens, they are pruned after
 	 * merged into rename/copy pairs as appropriate.
 	 */
-	if (options->reverse_diff)
+	if (DIFF_OPT_TST(options, REVERSE_DIFF))
 		addremove = (addremove == '+' ? '-' :
 			     addremove == '-' ? '+' : addremove);
 
@@ -3152,7 +3149,7 @@ void diff_addremove(struct diff_options *options,
 		fill_filespec(two, sha1, mode);
 
 	diff_queue(&diff_queued_diff, one, two);
-	options->has_changes = 1;
+	DIFF_OPT_SET(options, HAS_CHANGES);
 }
 
 void diff_change(struct diff_options *options,
@@ -3164,7 +3161,7 @@ void diff_change(struct diff_options *options,
 	char concatpath[PATH_MAX];
 	struct diff_filespec *one, *two;
 
-	if (options->reverse_diff) {
+	if (DIFF_OPT_TST(options, REVERSE_DIFF)) {
 		unsigned tmp;
 		const unsigned char *tmp_c;
 		tmp = old_mode; old_mode = new_mode; new_mode = tmp;
@@ -3178,7 +3175,7 @@ void diff_change(struct diff_options *options,
 	fill_filespec(two, new_sha1, new_mode);
 
 	diff_queue(&diff_queued_diff, one, two);
-	options->has_changes = 1;
+	DIFF_OPT_SET(options, HAS_CHANGES);
 }
 
 void diff_unmerge(struct diff_options *options,
diff --git a/diff.h b/diff.h
index 4546aad..6ff2b0e 100644
--- a/diff.h
+++ b/diff.h
@@ -43,26 +43,32 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 
 #define DIFF_FORMAT_CALLBACK	0x1000
 
+#define DIFF_OPT_RECURSIVE           (1 <<  0)
+#define DIFF_OPT_TREE_IN_RECURSIVE   (1 <<  1)
+#define DIFF_OPT_BINARY              (1 <<  2)
+#define DIFF_OPT_TEXT                (1 <<  3)
+#define DIFF_OPT_FULL_INDEX          (1 <<  4)
+#define DIFF_OPT_SILENT_ON_REMOVE    (1 <<  5)
+#define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
+#define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
+#define DIFF_OPT_COLOR_DIFF          (1 <<  8)
+#define DIFF_OPT_COLOR_DIFF_WORDS    (1 <<  9)
+#define DIFF_OPT_HAS_CHANGES         (1 << 10)
+#define DIFF_OPT_QUIET               (1 << 11)
+#define DIFF_OPT_NO_INDEX            (1 << 12)
+#define DIFF_OPT_ALLOW_EXTERNAL      (1 << 13)
+#define DIFF_OPT_EXIT_WITH_STATUS    (1 << 14)
+#define DIFF_OPT_REVERSE_DIFF        (1 << 15)
+#define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
+#define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
+#define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)
+
 struct diff_options {
 	const char *filter;
 	const char *orderfile;
 	const char *pickaxe;
 	const char *single_follow;
-	unsigned recursive:1,
-		 tree_in_recursive:1,
-		 binary:1,
-		 text:1,
-		 full_index:1,
-		 silent_on_remove:1,
-		 find_copies_harder:1,
-		 follow_renames:1,
-		 color_diff:1,
-		 color_diff_words:1,
-		 has_changes:1,
-		 quiet:1,
-		 no_index:1,
-		 allow_external:1,
-		 exit_with_status:1;
+	unsigned flags;
 	int context;
 	int break_opt;
 	int detect_rename;
@@ -71,7 +77,6 @@ struct diff_options {
 	int output_format;
 	int pickaxe_opts;
 	int rename_score;
-	int reverse_diff;
 	int rename_limit;
 	int setup;
 	int abbrev;
@@ -105,6 +110,9 @@ enum color_diff {
 	DIFF_WHITESPACE = 7,
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
+#define diff_get_color_opt(o, ix) \
+	diff_get_color(DIFF_OPT_TST((o), COLOR_DIFF), ix)
+
 
 extern const char mime_boundary_leader[];
 
diff --git a/log-tree.c b/log-tree.c
index a34beb0..1f3fcf1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -245,8 +245,7 @@ void show_log(struct rev_info *opt, const char *sep)
 			opt->diffopt.stat_sep = buffer;
 		}
 	} else if (opt->commit_format != CMIT_FMT_USERFORMAT) {
-		fputs(diff_get_color(opt->diffopt.color_diff, DIFF_COMMIT),
-		      stdout);
+		fputs(diff_get_color_opt(&opt->diffopt, DIFF_COMMIT), stdout);
 		if (opt->commit_format != CMIT_FMT_ONELINE)
 			fputs("commit ", stdout);
 		if (commit->object.flags & BOUNDARY)
@@ -266,8 +265,7 @@ void show_log(struct rev_info *opt, const char *sep)
 			       diff_unique_abbrev(parent->object.sha1,
 						  abbrev_commit));
 		show_decorations(commit);
-		printf("%s",
-		       diff_get_color(opt->diffopt.color_diff, DIFF_RESET));
+		printf("%s", diff_get_color_opt(&opt->diffopt, DIFF_RESET));
 		putchar(opt->commit_format == CMIT_FMT_ONELINE ? ' ' : '\n');
 		if (opt->reflog_info) {
 			show_reflog_message(opt->reflog_info,
diff --git a/merge-recursive.c b/merge-recursive.c
index 6c6f595..9a1e2f2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -366,7 +366,7 @@ static struct path_list *get_renames(struct tree *tree,
 
 	renames = xcalloc(1, sizeof(struct path_list));
 	diff_setup(&opts);
-	opts.recursive = 1;
+	DIFF_OPT_SET(&opts, RECURSIVE);
 	opts.detect_rename = DIFF_DETECT_RENAME;
 	opts.rename_limit = rename_limit;
 	opts.output_format = DIFF_FORMAT_NO_OUTPUT;
diff --git a/patch-ids.c b/patch-ids.c
index a288fac..3be5d31 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -121,7 +121,7 @@ int init_patch_ids(struct patch_ids *ids)
 {
 	memset(ids, 0, sizeof(*ids));
 	diff_setup(&ids->diffopts);
-	ids->diffopts.recursive = 1;
+	DIFF_OPT_SET(&ids->diffopts, RECURSIVE);
 	if (diff_setup_done(&ids->diffopts) < 0)
 		return error("diff_setup_done failed");
 	return 0;
diff --git a/revision.c b/revision.c
index 931f978..040214f 100644
--- a/revision.c
+++ b/revision.c
@@ -252,7 +252,7 @@ static void file_add_remove(struct diff_options *options,
 	}
 	tree_difference = diff;
 	if (tree_difference == REV_TREE_DIFFERENT)
-		options->has_changes = 1;
+		DIFF_OPT_SET(options, HAS_CHANGES);
 }
 
 static void file_change(struct diff_options *options,
@@ -262,7 +262,7 @@ static void file_change(struct diff_options *options,
 		 const char *base, const char *path)
 {
 	tree_difference = REV_TREE_DIFFERENT;
-	options->has_changes = 1;
+	DIFF_OPT_SET(options, HAS_CHANGES);
 }
 
 static int rev_compare_tree(struct rev_info *revs, struct tree *t1, struct tree *t2)
@@ -272,7 +272,7 @@ static int rev_compare_tree(struct rev_info *revs, struct tree *t1, struct tree
 	if (!t2)
 		return REV_TREE_DIFFERENT;
 	tree_difference = REV_TREE_SAME;
-	revs->pruning.has_changes = 0;
+	DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);
 	if (diff_tree_sha1(t1->object.sha1, t2->object.sha1, "",
 			   &revs->pruning) < 0)
 		return REV_TREE_DIFFERENT;
@@ -296,7 +296,7 @@ static int rev_same_tree_as_empty(struct rev_info *revs, struct tree *t1)
 	init_tree_desc(&empty, "", 0);
 
 	tree_difference = REV_TREE_SAME;
-	revs->pruning.has_changes = 0;
+	DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);
 	retval = diff_tree(&empty, &real, "", &revs->pruning);
 	free(tree);
 
@@ -688,8 +688,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 	revs->abbrev = DEFAULT_ABBREV;
 	revs->ignore_merges = 1;
 	revs->simplify_history = 1;
-	revs->pruning.recursive = 1;
-	revs->pruning.quiet = 1;
+	DIFF_OPT_SET(&revs->pruning, RECURSIVE);
+	DIFF_OPT_SET(&revs->pruning, QUIET);
 	revs->pruning.add_remove = file_add_remove;
 	revs->pruning.change = file_change;
 	revs->lifo = 1;
@@ -1086,13 +1086,13 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 			}
 			if (!strcmp(arg, "-r")) {
 				revs->diff = 1;
-				revs->diffopt.recursive = 1;
+				DIFF_OPT_SET(&revs->diffopt, RECURSIVE);
 				continue;
 			}
 			if (!strcmp(arg, "-t")) {
 				revs->diff = 1;
-				revs->diffopt.recursive = 1;
-				revs->diffopt.tree_in_recursive = 1;
+				DIFF_OPT_SET(&revs->diffopt, RECURSIVE);
+				DIFF_OPT_SET(&revs->diffopt, TREE_IN_RECURSIVE);
 				continue;
 			}
 			if (!strcmp(arg, "-m")) {
@@ -1274,7 +1274,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 		revs->diff = 1;
 
 	/* Pickaxe and rename following needs diffs */
-	if (revs->diffopt.pickaxe || revs->diffopt.follow_renames)
+	if (revs->diffopt.pickaxe || DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
 		revs->diff = 1;
 
 	if (revs->topo_order)
@@ -1283,7 +1283,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 	if (revs->prune_data) {
 		diff_tree_setup_paths(revs->prune_data, &revs->pruning);
 		/* Can't prune commits with rename following: the paths change.. */
-		if (!revs->diffopt.follow_renames)
+		if (!DIFF_OPT_TST(&revs->diffopt, FOLLOW_RENAMES))
 			revs->prune = 1;
 		if (!revs->full_diff)
 			diff_tree_setup_paths(revs->prune_data, &revs->diffopt);
diff --git a/tree-diff.c b/tree-diff.c
index 7c261fd..aa0a100 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -39,7 +39,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 		show_entry(opt, "+", t2, base, baselen);
 		return 1;
 	}
-	if (!opt->find_copies_harder && !hashcmp(sha1, sha2) && mode1 == mode2)
+	if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) && mode1 == mode2)
 		return 0;
 
 	/*
@@ -52,10 +52,10 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 		return 0;
 	}
 
-	if (opt->recursive && S_ISDIR(mode1)) {
+	if (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode1)) {
 		int retval;
 		char *newbase = malloc_base(base, baselen, path1, pathlen1);
-		if (opt->tree_in_recursive)
+		if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))
 			opt->change(opt, mode1, mode2,
 				    sha1, sha2, base, path1);
 		retval = diff_tree_sha1(sha1, sha2, newbase, opt);
@@ -206,7 +206,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 	const char *path;
 	const unsigned char *sha1 = tree_entry_extract(desc, &path, &mode);
 
-	if (opt->recursive && S_ISDIR(mode)) {
+	if (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode)) {
 		enum object_type type;
 		int pathlen = tree_entry_len(path, sha1);
 		char *newbase = malloc_base(base, baselen, path, pathlen);
@@ -257,7 +257,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, stru
 	int baselen = strlen(base);
 
 	for (;;) {
-		if (opt->quiet && opt->has_changes)
+		if (DIFF_OPT_TST(opt, QUIET) && DIFF_OPT_TST(opt, HAS_CHANGES))
 			break;
 		if (opt->nr_paths) {
 			skip_uninteresting(t1, base, baselen, opt);
@@ -315,7 +315,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
 	q->nr = 0;
 
 	diff_setup(&diff_opts);
-	diff_opts.recursive = 1;
+	DIFF_OPT_SET(&diff_opts, RECURSIVE);
 	diff_opts.detect_rename = DIFF_DETECT_RENAME;
 	diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	diff_opts.single_follow = opt->paths[0];
@@ -380,7 +380,7 @@ int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const cha
 	init_tree_desc(&t1, tree1, size1);
 	init_tree_desc(&t2, tree2, size2);
 	retval = diff_tree(&t1, &t2, base, opt);
-	if (opt->follow_renames && diff_might_be_rename()) {
+	if (DIFF_OPT_TST(opt, FOLLOW_RENAMES) && diff_might_be_rename()) {
 		init_tree_desc(&t1, tree1, size1);
 		init_tree_desc(&t2, tree2, size2);
 		try_to_follow_renames(&t1, &t2, base, opt);
-- 
1.5.3.5.1630.g4b3b4

^ permalink raw reply related

* Re: [PATCH] for-each-ref: fix setup of option-parsing for --sort
From: Junio C Hamano @ 2007-11-10 18:58 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: Johannes Schindelin, Jakub Narebski, Jon Smirl, git
In-Reply-To: <1194713274-31200-1-git-send-email-hjemli@gmail.com>

Lars Hjemli <hjemli@gmail.com> writes:

> The option value for --sort is already a pointer to a pointer to struct
> ref_sort, so just use it.
>
> Signed-off-by: Lars Hjemli <hjemli@gmail.com>
> ---
>
> On Nov 10, 2007 5:25 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
>> Could you add a test for that too, please?
>
> Is this ok?
>

Testing "for that" would be kind of hard and semi pointless,
isn't it?

If it's mismatch of the expected number of times a pointer is
dereferenced between the caller and the callee, I'd imagine that
it will read from and write to random place in memory and would
lead to unpredictable behaviour.  If you are lucky you would not
get expected results but if you are unlucky who knows what would
happen.

But the new test makes sure --sort takes intended effect, which
is a good thing.

Thanks.

>  builtin-for-each-ref.c  |    2 +-
>  t/t6300-for-each-ref.sh |   22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-for-each-ref.c b/builtin-for-each-ref.c
> index da8c794..e909e66 100644
> --- a/builtin-for-each-ref.c
> +++ b/builtin-for-each-ref.c
> @@ -847,7 +847,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)
>  		OPT_GROUP(""),
>  		OPT_INTEGER( 0 , "count", &maxcount, "show only <n> matched refs"),
>  		OPT_STRING(  0 , "format", &format, "format", "format to use for the output"),
> -		OPT_CALLBACK(0 , "sort", &sort_tail, "key",
> +		OPT_CALLBACK(0 , "sort", sort_tail, "key",
>  		            "field name to sort on", &opt_parse_sort),
>  		OPT_END(),
>  	};
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index d0809eb..c722635 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -148,4 +148,26 @@ test_expect_success 'Check format "rfc2822" date fields output' '
>  	git diff expected actual
>  '
>  
> +cat >expected <<\EOF
> +refs/heads/master
> +refs/tags/testtag
> +EOF
> +
> +test_expect_success 'Verify ascending sort' '
> +	git-for-each-ref --format="%(refname)" --sort=refname >actual &&
> +	git diff expected actual
> +'
> +
> +
> +cat >expected <<\EOF
> +refs/tags/testtag
> +refs/heads/master
> +EOF
> +
> +test_expect_success 'Verify descending sort' '
> +	git-for-each-ref --format="%(refname)" --sort=-refname >actual &&
> +	git diff expected actual
> +'
> +
> +
>  test_done
> -- 
> 1.5.3.5.623.g0a1d

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox