Git development
 help / color / mirror / Atom feed
* Re: [RFC] Git Wiki Move
From: Matthieu Moy @ 2010-01-17 12:00 UTC (permalink / raw)
  To: J.H.; +Cc: Petr Baudis, git
In-Reply-To: <4B50F7DB.7020204@eaglescrag.net>

"J.H." <warthog19@eaglescrag.net> writes:

> Quick update - I think I've got the vast majority of the obvious and
> simple to correct problems fixed at http://git.wiki.kernel.org anyone
> want to run through and see if there's anything else that would be
> considered a show stopper?

The main page is locked, and there are some broken links formatting in
the News section: http://git.wiki.kernel.org/index.php/Main_Page#News
I'm user "Moy" there if you want to let me fix them.

You should set $wgLogo to some Git logo, among
http://git.or.cz/gitwiki/GitRelatedLogos

You can also add a few links to the sidebar, by editting:
http://git.wiki.kernel.org/index.php/MediaWiki:Sidebar

(it seems I don't have permission to do it myself). I suggest taking
the ones of the front-page:

* Starting points
** Installation|Installation
** InterfacesFrontendsAndTools|Git Tools
** GitDocumentation|Git Documentation
** GitCommunity|Git Community Support
** GitProjects|Projects using Git
** GitFaq|FAQ
** GitHosting|Git Hosting
** GitLinks|Git Links
** GitComparison|Git Compared

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply

* [PATCH] rev-parse --namespace
From: Ilari Liusvaara @ 2010-01-17 13:45 UTC (permalink / raw)
  To: git

Add --namespace=<namespace> option to rev-parse and everything that
accepts its options. This option matches all refs in some subnamespace
of refs hierarchy, and is useful for selecting everything reachable from
one or few, but not all remotes (--namespace=remotes/foo).

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
Another one from my todo list. Based on master branch.

 Documentation/git-rev-list.txt     |    1 +
 Documentation/git-rev-parse.txt    |    4 ++
 Documentation/rev-list-options.txt |    6 ++
 builtin-rev-parse.c                |    6 ++
 refs.c                             |   33 +++++++++++
 refs.h                             |    9 +++
 revision.c                         |    5 ++
 t/t6018-rev-list-namespace.sh      |  103 ++++++++++++++++++++++++++++++++++++
 8 files changed, 167 insertions(+), 0 deletions(-)
 create mode 100755 t/t6018-rev-list-namespace.sh

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 3341d1b..a8f8f22 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -24,6 +24,7 @@ SYNOPSIS
 	     [ \--branches ]
 	     [ \--tags ]
 	     [ \--remotes ]
+	     [ \--namespace=namespace-prefix ]
 	     [ \--stdin ]
 	     [ \--quiet ]
 	     [ \--topo-order ]
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 82045a2..af4605a 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -112,6 +112,10 @@ OPTIONS
 --remotes::
 	Show tag refs found in `$GIT_DIR/refs/remotes`.
 
+--namespace=namespace-prefix::
+	Show refs found in `$GIT_DIR/namespace-prefix`. If namespace
+	specified lacks leading 'refs/', it is automatically prepended.
+
 --show-prefix::
 	When the command is invoked from a subdirectory, show the
 	path of the current directory relative to the top-level
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1f57aed..c824a7b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -243,6 +243,12 @@ endif::git-rev-list[]
 	Pretend as if all the refs in `$GIT_DIR/refs/remotes` are listed
 	on the command line as '<commit>'.
 
+--namespace=namespace-prefix::
+	Pretend as if all the refs in `$GIT_DIR/namespace-prefix` are
+	listed on the command line as '<commit>'. Leading 'refs/', it
+	is automatically prepended if missing.
+
+
 ifndef::git-rev-list[]
 --bisect::
 
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 37d0233..d7cb3c1 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -52,6 +52,7 @@ static int is_rev_argument(const char *arg)
 		"--parents",
 		"--pretty",
 		"--remotes",
+		"--namespace=",
 		"--sparse",
 		"--tags",
 		"--topo-order",
@@ -577,6 +578,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				for_each_tag_ref(show_reference, NULL);
 				continue;
 			}
+			if (!prefixcmp(arg, "--namespace=")) {
+				set_for_each_namespace(arg + 12);
+				for_each_namespace_ref(show_reference, NULL);
+				continue;
+			}
 			if (!strcmp(arg, "--remotes")) {
 				for_each_remote_ref(show_reference, NULL);
 				continue;
diff --git a/refs.c b/refs.c
index 3e73a0a..ae3f1a4 100644
--- a/refs.c
+++ b/refs.c
@@ -7,6 +7,9 @@
 /* ISSYMREF=01 and ISPACKED=02 are public interfaces */
 #define REF_KNOWS_PEELED 04
 
+/* Current prefix namespace in use. NULL means none. */
+static char* prefix_namespace = NULL;
+
 struct ref_list {
 	struct ref_list *next;
 	unsigned char flag; /* ISSYMREF? ISPACKED? */
@@ -674,6 +677,36 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
 	return do_for_each_ref("refs/replace/", fn, 13, 0, cb_data);
 }
 
+void set_for_each_namespace(const char *ref_namespace)
+{
+	size_t alloclen, origlen;
+	if (prefix_namespace)
+		free(prefix_namespace);
+
+	/* Compute the length of true prefix. */
+	origlen = alloclen = strlen(ref_namespace);
+	if (*ref_namespace && ref_namespace[origlen - 1] != '/')
+		alloclen++;
+	if (prefixcmp(ref_namespace, "refs/"))
+		alloclen += 5;		/* 'refs/' */
+
+	/* Allocate and build it (assume alloclen is "small") */
+	prefix_namespace = xmalloc(alloclen + 1);
+	*prefix_namespace = 0;
+	if (prefixcmp(ref_namespace, "refs/"))
+		strcat(prefix_namespace, "refs/");
+	strcat(prefix_namespace, ref_namespace);
+	if (*ref_namespace && ref_namespace[origlen - 1] != '/')
+		strcat(prefix_namespace, "/");
+}
+
+int for_each_namespace_ref(each_ref_fn fn, void *cb_data)
+{
+	if (!prefix_namespace)
+		die("BUG: Call set_for_each_namespace() before for_each_namespace_ref()!");
+	return for_each_ref_in(prefix_namespace, fn, cb_data);
+}
+
 int for_each_rawref(each_ref_fn fn, void *cb_data)
 {
 	return do_for_each_ref("refs/", fn, 0,
diff --git a/refs.h b/refs.h
index e141991..be493ca 100644
--- a/refs.h
+++ b/refs.h
@@ -25,6 +25,15 @@ extern int for_each_tag_ref(each_ref_fn, void *);
 extern int for_each_branch_ref(each_ref_fn, void *);
 extern int for_each_remote_ref(each_ref_fn, void *);
 extern int for_each_replace_ref(each_ref_fn, void *);
+extern int for_each_namespace_ref(each_ref_fn, void *);
+
+/*
+ * Set namespace for_each_namespace_ref() operates in. Must be called before
+ * calling that function. Autoprepends 'refs/' if it is missing. Autoappends
+ * '/' if it is missing.
+ */
+void set_for_each_namespace(const char *ref_namespace);
+
 
 /* can be used to learn about broken ref and symref */
 extern int for_each_rawref(each_ref_fn, void *);
diff --git a/revision.c b/revision.c
index 25fa14d..9e1d960 100644
--- a/revision.c
+++ b/revision.c
@@ -1352,6 +1352,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 				handle_refs(revs, flags, for_each_remote_ref);
 				continue;
 			}
+			if (!prefixcmp(arg, "--namespace=")) {
+				set_for_each_namespace(arg + 12);
+				handle_refs(revs, flags, for_each_namespace_ref);
+				continue;
+			}
 			if (!strcmp(arg, "--reflog")) {
 				handle_reflog(revs, flags);
 				continue;
diff --git a/t/t6018-rev-list-namespace.sh b/t/t6018-rev-list-namespace.sh
new file mode 100755
index 0000000..f04bde1
--- /dev/null
+++ b/t/t6018-rev-list-namespace.sh
@@ -0,0 +1,103 @@
+#!/bin/sh
+
+test_description='rev-list/rev-parse --namespace'
+
+. ./test-lib.sh
+
+
+commit () {
+	test_tick &&
+	echo $1 > foo &&
+	git add foo &&
+	git commit -m "$1"
+}
+
+test_expect_success 'setup' '
+
+	commit master &&
+	git checkout -b subspace/one master
+	commit one &&
+	git checkout -b subspace/two master
+	commit two &&
+	git checkout -b other/three master
+	commit three &&
+	git checkout -b someref master
+	commit some &&
+	git checkout master
+	commit master2
+'
+
+test_expect_success 'rev-parse --namespace=refs/heads/subspace/' '
+
+	test 2 = $(git rev-parse --namespace=refs/heads/subspace/ | wc -l)
+
+'
+
+test_expect_success 'rev-parse --namespace=refs/heads/subspace' '
+
+	test 2 = $(git rev-parse --namespace=refs/heads/subspace | wc -l)
+
+'
+
+test_expect_success 'rev-parse --namespace=heads/subspace' '
+
+	test 2 = $(git rev-parse --namespace=heads/subspace | wc -l)
+
+'
+
+test_expect_success 'rev-parse --namespace=heads/subspace --namespace=heads/other' '
+
+	test 3 = $(git rev-parse --namespace=heads/subspace --namespace=heads/other | wc -l)
+
+'
+
+test_expect_success 'rev-parse --namespace=heads/someref master' '
+
+	test 1 = $(git rev-parse --namespace=heads/someref master | wc -l)
+
+'
+
+test_expect_success 'rev-parse --namespace=heads' '
+
+	test 4 = $(git rev-parse --namespace=heads | wc -l)
+
+'
+
+test_expect_success 'rev-list --namespace=refs/heads/subspace/' '
+
+	test 3 = $(git rev-list --namespace=refs/heads/subspace/ | wc -l)
+
+'
+
+test_expect_success 'rev-list --namespace=refs/heads/subspace' '
+
+	test 3 = $(git rev-list --namespace=refs/heads/subspace | wc -l)
+
+'
+
+test_expect_success 'rev-list --namespace=heads/subspace' '
+
+	test 3 = $(git rev-list --namespace=heads/subspace | wc -l)
+
+'
+
+test_expect_success 'rev-list --namespace=heads/someref master' '
+
+	test 2 = $(git rev-list --namespace=heads/someref master | wc -l)
+
+'
+
+test_expect_success 'rev-list --namespace=heads/subspace --namespace=heads/other' '
+
+	test 4 = $(git rev-list --namespace=heads/subspace --namespace=heads/other | wc -l)
+
+'
+
+test_expect_success 'rev-list --namespace=heads' '
+
+	test 5 = $(git rev-list --namespace=heads | wc -l)
+
+'
+
+
+test_done
-- 
1.6.6.199.gff4b0

^ permalink raw reply related

* [PATCH 0/2] Make it easy to use branch --track on existing branch
From: Matthieu Moy @ 2010-01-17 14:06 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy

I'm starting a new thread to avoid hiding the message in another one,
but this is a followup to a message in the "git push --track" thread:

http://article.gmane.org/gmane.comp.version-control.git/137066

I wrote:
> Junio C Hamano <gitster <at> pobox.com> writes:
> 
> > The small nit is that "branch -f --track me origin/me" will happily
> > overwrite "me", even when your "me" is not up to date with "origin/me",
> > losing commits.
> 
> And another issue is:
> 
> $ git branch -f --track my-branch origin/my-branch
> fatal: Cannot force update the current branch.
> $ git branch --track my-branch origin/my-branch
> fatal: A branch named 'my-branch' already exists.
> 
> Actually, I just can't find a natural set of commands doing:
> 
> 1. create a branch (git checkout -b)
> 2. work on it
> 3. send it upstream (git push)
> 4. set the upstream as tracking (???)
> 
> with the current version of Git. I just do 4. with $EDITOR
> .git/config ...

The first patch makes it possible to use branch --track on an existing
branch (checked-out or not, regardless of -f), and the second warns on
a newly introduced irrelevant case.

This should be a nice complement to "push --set-upstream". I think
"push --set-upstream" is the most natural in 99% of cases, but using
"git branch" should work too.

Matthieu Moy (2):
  branch: allow creating a branch with same name and same starting
    point.
  branch: warn and refuse to set a branch as a tracking branch of
    itself.

 branch.c                 |   59 ++++++++++++++++++++++++++++++++-------------
 t/t6040-tracking-info.sh |    8 ++++++
 2 files changed, 50 insertions(+), 17 deletions(-)

^ permalink raw reply

* [PATCH 1/2] branch: allow creating a branch with same name and same starting point.
From: Matthieu Moy @ 2010-01-17 14:06 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy
In-Reply-To: <1263737212-8101-1-git-send-email-Matthieu.Moy@imag.fr>

Previously, "git branch --track newname old" was rejected if newname
existed, even if it already had the same value. This patch allows it,
even without --force. This has two advantages:

* Not requiring --force for a safe operation, hence allowing the user to
  benefit from the other safety checks.

* Allow changing the configuration of the checked-out branch.

As a result, "git branch --track" becomes a conveinient and safe way to
set up upstream information for an existing branch.

The error message "A branch named '%s' already exists" is no longer
precise enough to refuse to act, since part of the reason of the failure
is the old branch value, so we include this value in the message.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 branch.c                 |   45 ++++++++++++++++++++++++++++++---------------
 t/t6040-tracking-info.sh |    8 ++++++++
 2 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/branch.c b/branch.c
index 05ef3f5..d187891 100644
--- a/branch.c
+++ b/branch.c
@@ -128,27 +128,39 @@ void create_branch(const char *head,
 		   const char *name, const char *start_name,
 		   int force, int reflog, enum branch_track track)
 {
-	struct ref_lock *lock;
+	struct ref_lock *lock = NULL;
 	struct commit *commit;
-	unsigned char sha1[20];
+	unsigned char old_sha1[20], sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
 	struct strbuf ref = STRBUF_INIT;
 	int forcing = 0;
+	int dont_change_ref = 0;
 
 	if (strbuf_check_branch_ref(&ref, name))
 		die("'%s' is not a valid branch name.", name);
 
-	if (resolve_ref(ref.buf, sha1, 1, NULL)) {
-		if (!force)
-			die("A branch named '%s' already exists.", name);
-		else if (!is_bare_repository() && !strcmp(head, name))
-			die("Cannot force update the current branch.");
+	if (get_sha1(start_name, sha1))
+		die("Not a valid object name: '%s'.", start_name);
+
+	if (resolve_ref(ref.buf, old_sha1, 1, NULL)) {
+		if (hashcmp(old_sha1, sha1)) {
+			if (!force)
+				die("A branch named '%s' already exists pointing to %.*s.",
+				    name, 8, sha1_to_hex(old_sha1));
+			else if (!is_bare_repository() && !strcmp(head, name))
+				die("Cannot force update the current branch.");
+		} else {
+			/*
+			 * Re-creating a branch with the same value.
+			 * Safe, and usefull to set up the upstream
+			 * branch without touching the ref.
+			 */
+			dont_change_ref = 1;
+		}
 		forcing = 1;
 	}
 
 	real_ref = NULL;
-	if (get_sha1(start_name, sha1))
-		die("Not a valid object name: '%s'.", start_name);
 
 	switch (dwim_ref(start_name, strlen(start_name), sha1, &real_ref)) {
 	case 0:
@@ -170,9 +182,11 @@ void create_branch(const char *head,
 		die("Not a valid branch point: '%s'.", start_name);
 	hashcpy(sha1, commit->object.sha1);
 
-	lock = lock_any_ref_for_update(ref.buf, NULL, 0);
-	if (!lock)
-		die_errno("Failed to lock ref for update");
+	if (!dont_change_ref) {
+		lock = lock_any_ref_for_update(ref.buf, NULL, 0);
+		if (!lock)
+			die_errno("Failed to lock ref for update");
+	}
 
 	if (reflog)
 		log_all_ref_updates = 1;
@@ -180,15 +194,16 @@ void create_branch(const char *head,
 	if (forcing)
 		snprintf(msg, sizeof msg, "branch: Reset from %s",
 			 start_name);
-	else
+	else if (!dont_change_ref)
 		snprintf(msg, sizeof msg, "branch: Created from %s",
 			 start_name);
 
 	if (real_ref && track)
 		setup_tracking(name, real_ref, track);
 
-	if (write_ref_sha1(lock, sha1, msg) < 0)
-		die_errno("Failed to write ref");
+	if (!dont_change_ref)
+		if (write_ref_sha1(lock, sha1, msg) < 0)
+			die_errno("Failed to write ref");
 
 	strbuf_release(&ref);
 	free(real_ref);
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 664b0f8..3c200b6 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -89,4 +89,12 @@ test_expect_success 'status when tracking annotated tags' '
 	grep "set up to track" actual &&
 	git checkout heavytrack
 '
+
+test_expect_success 'setup tracking with branch --track on existing branch' '
+	git branch from-master master &&
+	test_must_fail git config branch.from-master.merge > actual &&
+	git branch from-master master --track &&
+	git config branch.from-master.merge > actual &&
+	grep -q "^refs/heads/master$" actual
+'
 test_done
-- 
1.6.6.198.gbaea2

^ permalink raw reply related

* [PATCH 2/2] branch: warn and refuse to set a branch as a tracking branch of itself.
From: Matthieu Moy @ 2010-01-17 14:06 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy
In-Reply-To: <1263737212-8101-1-git-send-email-Matthieu.Moy@imag.fr>

Previous patch allows commands like "git branch foo foo --track", which
doesn't make much sense. Warn the user and don't change the configuration
in this case. Don't die to let the caller finish its job in such case.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 branch.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index d187891..d769c8a 100644
--- a/branch.c
+++ b/branch.c
@@ -49,9 +49,19 @@ static int should_setup_rebase(const char *origin)
 
 void install_branch_config(int flag, const char *local, const char *origin, const char *remote)
 {
+	const char *shortname = remote + 11;
+	int remote_is_branch = !prefixcmp(remote, "refs/heads/");
 	struct strbuf key = STRBUF_INIT;
 	int rebasing = should_setup_rebase(origin);
 
+	if (remote_is_branch
+	    && !strcmp(local, shortname)
+	    && !origin) {
+		warning("Not setting branch %s as its own upstream.",
+			local);
+		return;
+	}
+
 	strbuf_addf(&key, "branch.%s.remote", local);
 	git_config_set(key.buf, origin ? origin : ".");
 
@@ -71,8 +81,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
 		strbuf_addstr(&key, origin ? "remote" : "local");
 
 		/* Are we tracking a proper "branch"? */
-		if (!prefixcmp(remote, "refs/heads/")) {
-			strbuf_addf(&key, " branch %s", remote + 11);
+		if (remote_is_branch) {
+			strbuf_addf(&key, " branch %s", shortname);
 			if (origin)
 				strbuf_addf(&key, " from %s", origin);
 		}
-- 
1.6.6.198.gbaea2

^ permalink raw reply related

* Re: [PATCH 0/2] Make it easy to use branch --track on existing branch
From: Ilari Liusvaara @ 2010-01-17 14:40 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster
In-Reply-To: <1263737212-8101-1-git-send-email-Matthieu.Moy@imag.fr>

On Sun, Jan 17, 2010 at 03:06:50PM +0100, Matthieu Moy wrote:
> 
> The first patch makes it possible to use branch --track on an existing
> branch (checked-out or not, regardless of -f), and the second warns on
> a newly introduced irrelevant case.

Yay. This is one of smaller entries from my todo list (the remaining
entry that's small enough to still make it into 1.7 is command to
set URL remote points to[*]).

But If I read the patch correctly, you can't just arbitrarily set the
tracking branch since the IDs must match. So if somebody screws the
ref creation so that it lacks upstream data, then both local and upstream
change. Push will fail. Pull will fail. And now neither checkout --track
(due to same-hash check) nor push --set-upstream (due to pushed or up to
date) help.

[*] Yes, I know you can edit .git/config, but I would want "official sounding"
(read: git remote subcommand) command to edit it (and no, delete & recreate
doesn't do the right thing).

-Ilari

^ permalink raw reply

* Re: [PATCH 0/2] Make it easy to use branch --track on existing branch
From: Matthieu Moy @ 2010-01-17 14:53 UTC (permalink / raw)
  To: Ilari Liusvaara; +Cc: git, gitster
In-Reply-To: <20100117144031.GA20335@Knoppix>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> But If I read the patch correctly, you can't just arbitrarily set the
> tracking branch since the IDs must match.

Yes. The patch really helps when you already have synchronized, or
when you haven't already desynchronized your local branch and its
upstream.

We could go one step further, and allow the new branch point to be
different as long as it has the old point as an ancestor, but that
would still be a ref update.

> [*] Yes, I know you can edit .git/config, but I would want "official sounding"
> (read: git remote subcommand) command to edit it (and no, delete & recreate
> doesn't do the right thing).

Having a "git remote subcommand" to do the same thing could help too,
but it could just come in addition to my patch.

My patch doesn't introduce new complexity: the command is already
there, and the use-case I'm allowing are legitimate. So, it can help
to let users run in, and it doesn't harm.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply

* Re: [PATCH 0/2] Make it easy to use branch --track on existing branch
From: Ilari Liusvaara @ 2010-01-17 15:29 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git, gitster
In-Reply-To: <vpqska4n5pr.fsf@bauges.imag.fr>

On Sun, Jan 17, 2010 at 03:53:04PM +0100, Matthieu Moy wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> > [*] Yes, I know you can edit .git/config, but I would want "official sounding"
> > (read: git remote subcommand) command to edit it (and no, delete & recreate
> > doesn't do the right thing).
> 
> Having a "git remote subcommand" to do the same thing could help too,
> but it could just come in addition to my patch.

Err... That was about changing URL, not about changing tracking branch.

-Ilari

^ permalink raw reply

* Re: [PATCH v2 3/3] commit: show interesting ident information in summary
From: Jeff King @ 2010-01-17 16:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Adam Megacz, git
In-Reply-To: <7vhbql85ti.fsf@alter.siamese.dyndns.org>

On Sun, Jan 17, 2010 at 12:59:53AM -0800, Junio C Hamano wrote:

> > @@ -1085,7 +1118,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
> >  		struct pretty_print_context ctx = {0};
> >  		struct strbuf buf = STRBUF_INIT;
> >  		ctx.date_mode = DATE_NORMAL;
> > -		format_commit_message(commit, format + 7, &buf, &ctx);
> > +		format_commit_message(commit, format.buf + 7, &buf, &ctx);
> >  		printf("%s\n", buf.buf);
> 
> But sometimes log_tree_commit() doesn't show the header.  Most notably for
> merges.  What string are we using for format_commit_message()?  Oops.

Ugh. Thank you and good catch.

-Peff

^ permalink raw reply

* Re: [PATCH] rev-parse --namespace
From: Jeff King @ 2010-01-17 16:27 UTC (permalink / raw)
  To: Ilari Liusvaara; +Cc: git
In-Reply-To: <1263735931-20227-1-git-send-email-ilari.liusvaara@elisanet.fi>

On Sun, Jan 17, 2010 at 03:45:31PM +0200, Ilari Liusvaara wrote:

> Add --namespace=<namespace> option to rev-parse and everything that
> accepts its options. This option matches all refs in some subnamespace
> of refs hierarchy, and is useful for selecting everything reachable from
> one or few, but not all remotes (--namespace=remotes/foo).

If I understand it correctly, isn't the same as

  git for-each-ref refs/remotes/foo

?

-Peff

^ permalink raw reply

* Re: [PATCH] rev-parse --namespace
From: Ilari Liusvaara @ 2010-01-17 16:40 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20100117162712.GB7153@sigill.intra.peff.net>

On Sun, Jan 17, 2010 at 11:27:12AM -0500, Jeff King wrote:
> On Sun, Jan 17, 2010 at 03:45:31PM +0200, Ilari Liusvaara wrote:
> 
> > Add --namespace=<namespace> option to rev-parse and everything that
> > accepts its options. This option matches all refs in some subnamespace
> > of refs hierarchy, and is useful for selecting everything reachable from
> > one or few, but not all remotes (--namespace=remotes/foo).
> 
> If I understand it correctly, isn't the same as
> 
>   git for-each-ref refs/remotes/foo

Nope. Compare:

'git log --branches --not --namespace=remotes/origin' 

with whatever you would have to currently type...

-Ilari

^ permalink raw reply

* [PATCH] cvsimport: modernize and standardize external tool calling
From: Ben Walton @ 2010-01-17 17:35 UTC (permalink / raw)
  To: git; +Cc: Ben Walton
In-Reply-To: <1263700523-2111-1-git-send-email-bwalton@artsci.utoronto.ca>

The cvsimport module was mixing old (git-foo) and new (git foo)
conventions when calling git tools.  This patch standardizes the
calling conventions used in system(), open(), exec() and backticks.

Reported-by: Alexander Maier <amaier@opencsw.org>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
This takes into account the feedback from Junio.  It's a deeper change,
but should be better, overall.

 git-cvsimport.perl |   70 ++++++++++++++++++++++++++--------------------------
 1 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index a7d215c..24a31b3 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -609,16 +609,16 @@ $orig_git_index = $ENV{GIT_INDEX_FILE} if exists $ENV{GIT_INDEX_FILE};
 my %index; # holds filenames of one index per branch
 
 unless (-d $git_dir) {
-	system("git-init");
+	system(qw(git init));
 	die "Cannot init the GIT db at $git_tree: $?\n" if $?;
-	system("git-read-tree");
+	system(qw(git read-tree));
 	die "Cannot init an empty tree: $?\n" if $?;
 
 	$last_branch = $opt_o;
 	$orig_branch = "";
 } else {
-	open(F, "git-symbolic-ref HEAD |") or
-		die "Cannot run git-symbolic-ref: $!\n";
+	open(F, '-|', 'git symbolic-ref HEAD') or
+		die "Cannot run git symbolic-ref: $!\n";
 	chomp ($last_branch = <F>);
 	$last_branch = basename($last_branch);
 	close(F);
@@ -627,12 +627,12 @@ unless (-d $git_dir) {
 		$last_branch = "master";
 	}
 	$orig_branch = $last_branch;
-	$tip_at_start = `git-rev-parse --verify HEAD`;
+	$tip_at_start = `git rev-parse --verify HEAD`;
 
 	# Get the last import timestamps
 	my $fmt = '($ref, $author) = (%(refname), %(author));';
-	open(H, "git-for-each-ref --perl --format='$fmt' $remote |") or
-		die "Cannot run git-for-each-ref: $!\n";
+	open(H, '-|', "git for-each-ref --perl --format='$fmt' $remote") or
+		die "Cannot run git for-each-ref: $!\n";
 	while (defined(my $entry = <H>)) {
 		my ($ref, $author);
 		eval($entry) || die "cannot eval refs list: $@";
@@ -687,7 +687,7 @@ unless ($opt_P) {
 	    print $cvspsfh $_;
 	}
 	close CVSPS;
-	$? == 0 or die "git-cvsimport: fatal: cvsps reported error\n";
+	$? == 0 or die "git cvsimport: fatal: cvsps reported error\n";
 	close $cvspsfh;
 } else {
 	$cvspsfile = munge_user_filename($opt_P);
@@ -716,27 +716,27 @@ my $state = 0;
 sub update_index (\@\@) {
 	my $old = shift;
 	my $new = shift;
-	open(my $fh, '|-', qw(git-update-index -z --index-info))
-		or die "unable to open git-update-index: $!";
+	open(my $fh, '|-', 'git update-index -z --index-info')
+		or die "unable to open git update-index: $!";
 	print $fh
 		(map { "0 0000000000000000000000000000000000000000\t$_\0" }
 			@$old),
 		(map { '100' . sprintf('%o', $_->[0]) . " $_->[1]\t$_->[2]\0" }
 			@$new)
-		or die "unable to write to git-update-index: $!";
+		or die "unable to write to git update-index: $!";
 	close $fh
-		or die "unable to write to git-update-index: $!";
-	$? and die "git-update-index reported error: $?";
+		or die "unable to write to git update-index: $!";
+	$? and die "git update-index reported error: $?";
 }
 
 sub write_tree () {
-	open(my $fh, '-|', qw(git-write-tree))
-		or die "unable to open git-write-tree: $!";
+	open(my $fh, '-|', 'git write-tree')
+		or die "unable to open git write-tree: $!";
 	chomp(my $tree = <$fh>);
 	is_sha1($tree)
 		or die "Cannot get tree id ($tree): $!";
 	close($fh)
-		or die "Error running git-write-tree: $?\n";
+		or die "Error running git write-tree: $?\n";
 	print "Tree ID $tree\n" if $opt_v;
 	return $tree;
 }
@@ -751,7 +751,7 @@ sub commit {
 	if ($branch eq $opt_o && !$index{branch} &&
 		!get_headref("$remote/$branch")) {
 	    # looks like an initial commit
-	    # use the index primed by git-init
+	    # use the index primed by git init
 	    $ENV{GIT_INDEX_FILE} = "$git_dir/index";
 	    $index{$branch} = "$git_dir/index";
 	} else {
@@ -761,9 +761,9 @@ sub commit {
 		$index{$branch} = tmpnam();
 		$ENV{GIT_INDEX_FILE} = $index{$branch};
 		if ($ancestor) {
-		    system("git-read-tree", "$remote/$ancestor");
+		    system(qw(git read-tree), "$remote/$ancestor");
 		} else {
-		    system("git-read-tree", "$remote/$branch");
+		    system(qw(git read-tree), "$remote/$branch");
 		}
 		die "read-tree failed: $?\n" if $?;
 	    }
@@ -798,7 +798,7 @@ sub commit {
 	$ENV{GIT_COMMITTER_EMAIL} = $author_email;
 	$ENV{GIT_COMMITTER_DATE} = $commit_date;
 	my $pid = open2(my $commit_read, my $commit_write,
-		'git-commit-tree', $tree, @commit_args);
+		'git', 'commit-tree', $tree, @commit_args);
 
 	# compatibility with git2cvs
 	substr($logmsg,32767) = "" if length($logmsg) > 32767;
@@ -811,7 +811,7 @@ sub commit {
 	}
 
 	print($commit_write "$logmsg\n") && close($commit_write)
-		or die "Error writing to git-commit-tree: $!\n";
+		or die "Error writing to git commit-tree: $!\n";
 
 	print "Committed patch $patchset ($branch $commit_date)\n" if $opt_v;
 	chomp(my $cid = <$commit_read>);
@@ -820,9 +820,9 @@ sub commit {
 	close($commit_read);
 
 	waitpid($pid,0);
-	die "Error running git-commit-tree: $?\n" if $?;
+	die "Error running git commit-tree: $?\n" if $?;
 
-	system('git-update-ref', "$remote/$branch", $cid) == 0
+	system(qw(git update-ref), "$remote/$branch", $cid) == 0
 		or die "Cannot write branch $branch for update: $!\n";
 
 	if ($tag) {
@@ -832,7 +832,7 @@ sub commit {
 		$xtag =~ s/[\/]/$opt_s/g;
 		$xtag =~ s/\[//g;
 
-		system('git-tag', '-f', $xtag, $cid) == 0
+		system(qw(git tag -f), $xtag, $cid) == 0
 			or die "Cannot create tag $xtag: $!\n";
 
 		print "Created tag '$xtag' on '$branch'\n" if $opt_v;
@@ -969,7 +969,7 @@ while (<CVS>) {
 			my $pid = open(my $F, '-|');
 			die $! unless defined $pid;
 			if (!$pid) {
-			    exec("git-hash-object", "-w", $tmpname)
+			    exec("git", "hash-object", "-w", $tmpname)
 				or die "Cannot create object: $!\n";
 			}
 			my $sha = <$F>;
@@ -993,7 +993,7 @@ while (<CVS>) {
 		}
 		commit();
 		if (($commitcount & 1023) == 0) {
-			system("git repack -a -d");
+			system(qw(git repack -a -d));
 		}
 		$state = 1;
 	} elsif ($state == 11 and /^-+$/) {
@@ -1013,11 +1013,11 @@ unless ($opt_P) {
 # The heuristic of repacking every 1024 commits can leave a
 # lot of unpacked data.  If there is more than 1MB worth of
 # not-packed objects, repack once more.
-my $line = `git-count-objects`;
+my $line = `git count-objects`;
 if ($line =~ /^(\d+) objects, (\d+) kilobytes$/) {
   my ($n_objects, $kb) = ($1, $2);
   1024 < $kb
-    and system("git repack -a -d");
+    and system(qw(git repack -a -d));
 }
 
 foreach my $git_index (values %index) {
@@ -1038,28 +1038,28 @@ if ($orig_branch) {
 	if ($opt_i) {
 		exit 0;
 	}
-	my $tip_at_end = `git-rev-parse --verify HEAD`;
+	my $tip_at_end = `git rev-parse --verify HEAD`;
 	if ($tip_at_start ne $tip_at_end) {
 		for ($tip_at_start, $tip_at_end) { chomp; }
 		print "Fetched into the current branch.\n" if $opt_v;
-		system(qw(git-read-tree -u -m),
+		system(qw(git read-tree -u -m),
 		       $tip_at_start, $tip_at_end);
 		die "Fast-forward update failed: $?\n" if $?;
 	}
 	else {
-		system(qw(git-merge cvsimport HEAD), "$remote/$opt_o");
+		system(qw(git merge cvsimport HEAD), "$remote/$opt_o");
 		die "Could not merge $opt_o into the current branch.\n" if $?;
 	}
 } else {
 	$orig_branch = "master";
 	print "DONE; creating $orig_branch branch\n" if $opt_v;
-	system("git-update-ref", "refs/heads/master", "$remote/$opt_o")
+	system(qw(git update-ref refs/heads/master), "$remote/$opt_o")
 		unless defined get_headref('refs/heads/master');
-	system("git-symbolic-ref", "$remote/HEAD", "$remote/$opt_o")
+	system(qw(git symbolic-ref), "$remote/HEAD", "$remote/$opt_o")
 		if ($opt_r && $opt_o ne 'HEAD');
-	system('git-update-ref', 'HEAD', "$orig_branch");
+	system(qw(git update-ref HEAD), "$orig_branch");
 	unless ($opt_i) {
-		system('git checkout -f');
+		system(qw(git checkout -f));
 		die "checkout failed: $?\n" if $?;
 	}
 }
-- 
1.6.5.3

^ permalink raw reply related

* Re: [PATCH 5/8] rerere: use ll_merge() instead of using xdl_merge()
From: Junio C Hamano @ 2010-01-17 19:01 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git
In-Reply-To: <201001171252.38826.j6t@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

> On Sonntag, 17. Januar 2010, Junio C Hamano wrote:
>> This allows us to pay attention to the attribute settings and custom
>> merge driver the user sets up.
>
> I do not think that this change is necessary; I even think that it is wrong, 
> in particular, custom merge drivers should *not* be used anymore.

You are right in that nothing is strictly necessary as long as there are
other ways to do so.  This does not have to be how the issue is solved,
but I found this to be one and the most natural way to allow rerere to pay
attention to per-path conflict marker length attribute.

Contents that you would want to use custom merge drivers would not benefit
from the current rerere that uses the default textual merge. In your
customized XML merge editor example, the merged contents have irrelevant
line breaks on either side of the merge that break textual merge (and that
is the reason you are using a custom XML aware merge script to begin with).

So I didn't think using ll_merge() makes things worse, and that was the
reason why I did it this way.

But I admit I didn't think things through (and that is why your name was
on the Cc: line).  If you really want to forbid custom merge drivers, I
think we can add an option to ll_merge() to specify which attribute to
ignore, and force the default textual merge in the codepath, or we can go
back to the xdl_merge() but pass a custom conflict marker length in
xmparam_t, as a follow-up fix.

^ permalink raw reply

* Re: [PATCH] rev-parse --namespace
From: Jeff King @ 2010-01-17 19:08 UTC (permalink / raw)
  To: Ilari Liusvaara; +Cc: git
In-Reply-To: <20100117164057.GA20554@Knoppix>

On Sun, Jan 17, 2010 at 06:40:57PM +0200, Ilari Liusvaara wrote:

> On Sun, Jan 17, 2010 at 11:27:12AM -0500, Jeff King wrote:
> > On Sun, Jan 17, 2010 at 03:45:31PM +0200, Ilari Liusvaara wrote:
> > 
> > > Add --namespace=<namespace> option to rev-parse and everything that
> > > accepts its options. This option matches all refs in some subnamespace
> > > of refs hierarchy, and is useful for selecting everything reachable from
> > > one or few, but not all remotes (--namespace=remotes/foo).
> > 
> > If I understand it correctly, isn't the same as
> > 
> >   git for-each-ref refs/remotes/foo
> 
> Nope. Compare:
> 
> 'git log --branches --not --namespace=remotes/origin' 
> 
> with whatever you would have to currently type...

OK, that makes sense and is actually useful. That is the sort of
motivation that should go in the commit message. Mentioning "rev-parse"
is a bit of a red herring.

As for the patch text itself, it looks correct, though I have a few
nits:

> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index 82045a2..af4605a 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -112,6 +112,10 @@ OPTIONS
>  --remotes::
>  	Show tag refs found in `$GIT_DIR/refs/remotes`.
>  
> +--namespace=namespace-prefix::
> +	Show refs found in `$GIT_DIR/namespace-prefix`. If namespace
> +	specified lacks leading 'refs/', it is automatically prepended.
> +

This is not a new problem you are introducing, as you are just following
the template of of --remotes and others above, but is "show" really the
right word? In the rev-list page these entries talk about "pretend as if
these refs were given on the command line". Isn't that also the case
here? If I say "git rev-parse --not --remotes" (or --namespace=), I will
get "^"-lines.

Also, I notice in the context that the remotes entry says "Show tag
refs" which is obviously wrong. Again, not your problem, but something
to clean up while in the area.

> +			if (!prefixcmp(arg, "--namespace=")) {
> +				set_for_each_namespace(arg + 12);
> +				for_each_namespace_ref(show_reference, NULL);
> +				continue;
> +			}

I was going to complain about the use of a global variable here when we
could simply pass the namespace into the function, but I see that in the
other call here:

> diff --git a/revision.c b/revision.c
> index 25fa14d..9e1d960 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1352,6 +1352,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
>  				handle_refs(revs, flags, for_each_remote_ref);
>  				continue;
>  			}
> +			if (!prefixcmp(arg, "--namespace=")) {
> +				set_for_each_namespace(arg + 12);
> +				handle_refs(revs, flags, for_each_namespace_ref);
> +				continue;
> +			}

...that giving the for_each_namespace_ref function a different signature
than the other for_each_* functions would complicate things. OTOH,
handle_refs is only a 4 line helper to set up the callback struct, so
perhaps it is better to simply duplicate it rather than introduce a new
global.

Too bad we can't do currying in C. :)

> +void set_for_each_namespace(const char *ref_namespace)
> +{
> +	size_t alloclen, origlen;
> +	if (prefix_namespace)
> +		free(prefix_namespace);
> +
> +	/* Compute the length of true prefix. */
> +	origlen = alloclen = strlen(ref_namespace);
> +	if (*ref_namespace && ref_namespace[origlen - 1] != '/')
> +		alloclen++;
> +	if (prefixcmp(ref_namespace, "refs/"))
> +		alloclen += 5;		/* 'refs/' */
> +
> +	/* Allocate and build it (assume alloclen is "small") */
> +	prefix_namespace = xmalloc(alloclen + 1);
> +	*prefix_namespace = 0;
> +	if (prefixcmp(ref_namespace, "refs/"))
> +		strcat(prefix_namespace, "refs/");
> +	strcat(prefix_namespace, ref_namespace);
> +	if (*ref_namespace && ref_namespace[origlen - 1] != '/')
> +		strcat(prefix_namespace, "/");
> +}

Wouldn't this be much simpler and more readable using a strbuf? As in:

  strbuf_reset(&prefix_namespace);
  if (prefixcmp(ref_namespace, "refs/"))
    strbuf_addstr(&prefix_namespace, "refs/");
  strbuf_addstr(&prefix_namespace, ref_namespace);
  if (prefix_namespace.buf[prefix_namespace.len-1] != '/')
    strbuf_addch(&prefix_namespace, '/');

?

> +test_expect_success 'setup' '
> +
> +	commit master &&
> +	git checkout -b subspace/one master
> +	commit one &&
> +	git checkout -b subspace/two master
> +	commit two &&
> +	git checkout -b other/three master
> +	commit three &&
> +	git checkout -b someref master
> +	commit some &&
> +	git checkout master
> +	commit master2
> +'

Missing '&&' on the checkout lines?

> +test_expect_success 'rev-parse --namespace=refs/heads/subspace/' '
> +
> +	test 2 = $(git rev-parse --namespace=refs/heads/subspace/ | wc -l)
> +
> +'

This is perhaps a minor nit, but I really prefer to write this by
putting the expected output in a file and using test_cmp, because:

  1. You actually test that rev-parse exits correctly.

  2. You test that it actually produced the correct output, and not
     simply two lines which are presumably correct.

  3. If it _does_ fail, the output from test_cmp is a nicely readable
     diff between expected and actual output. Your test produces no
     output.

We also had some problems with different implementations of "wc"
producing different amounts of whitespace, but I don't think that is a
problem here since the shell should eat any whitespace outside of
quotation marks.

-Peff

^ permalink raw reply

* [PATCH] Performance optimization for detection of modified submodules
From: Jens Lehmann @ 2010-01-17 19:01 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Johannes Schindelin, Shawn O. Pearce,
	Heiko Voigt, Lars Hjemli
In-Reply-To: <7v3a288em2.fsf@alter.siamese.dyndns.org>

In the worst case is_submodule_modified() got called three times for
each submodule. The information we got from scanning the whole
submodule tree the first time should not be thrown away.

A new parameter has been added to diff_change() and diff_addremove(),
the information is stored in a new member of struct diff_filespec. Its
value is then reused instead of calling is_submodule_modified() again.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---


Am 15.01.2010 00:13, schrieb Junio C Hamano:
> The existing code is a bit unfortunate; by the time we come to the output
> routine, the information we found from is_submodule_modified() is lost;
> that is why my "would look like this" patch calls is_submodule_modified().
>
> We may want to add one parameter to diff_change() and diff_addremove(), to
> tell them if the work-tree side (if we are comparing something with the
> work tree) is a modified submodule, and add one bit to the diff_filespec
> structure to record that in diff_change() and diff_addremove() (obviously
> only when adding).  That way, my "would looks like this" patch needs to
> check the result of is_submodule_modified() the front-ends left in the
> filespec, instead of running it again.

So here is my first attempt of implementing your proposal. The test suite
runs fine, but a few more eyeballs would really be appreciated as i am not
very familiar with the code and its corner cases (See diff_change(), is it
sufficient to only set "two->dirty_submodule", even if the REVERSE_DIFF
option is set? Apart from that i am not so sure about the four changes to
tree-diff.c).

I think we could skip the call to is_submodule_modified() in
run_diff_files() and get_stat_data() when the changed flag is already
set and only short output (without calling diff_populate_gitlink(), e.g.
"git status -s" or "git diff-files") is requested. What do you think
about doing that in a seperate patch?



 diff-lib.c  |   42 +++++++++++++++++++++++++++---------------
 diff.c      |   11 +++++++----
 diff.h      |    8 ++++----
 diffcore.h  |    1 +
 revision.c  |    4 ++--
 tree-diff.c |    8 ++++----
 6 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 29c5915..5b18d86 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -73,6 +73,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		unsigned int oldmode, newmode;
 		struct cache_entry *ce = active_cache[i];
 		int changed;
+		int dirty_submodule = 0;

 		if (DIFF_OPT_TST(&revs->diffopt, QUICK) &&
 			DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
@@ -173,12 +174,14 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			if (silent_on_removed)
 				continue;
 			diff_addremove(&revs->diffopt, '-', ce->ce_mode,
-				       ce->sha1, ce->name);
+				       ce->sha1, ce->name, 0);
 			continue;
 		}
 		changed = ce_match_stat(ce, &st, ce_option);
-		if (S_ISGITLINK(ce->ce_mode) && !changed)
-			changed = is_submodule_modified(ce->name);
+		if (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name)) {
+			changed = 1;
+			dirty_submodule = 1;
+		}
 		if (!changed) {
 			ce_mark_uptodate(ce);
 			if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
@@ -188,7 +191,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		newmode = ce_mode_from_stat(ce, st.st_mode);
 		diff_change(&revs->diffopt, oldmode, newmode,
 			    ce->sha1, (changed ? null_sha1 : ce->sha1),
-			    ce->name);
+			    ce->name, dirty_submodule);

 	}
 	diffcore_std(&revs->diffopt);
@@ -204,16 +207,18 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 static void diff_index_show_file(struct rev_info *revs,
 				 const char *prefix,
 				 struct cache_entry *ce,
-				 const unsigned char *sha1, unsigned int mode)
+				 const unsigned char *sha1, unsigned int mode,
+				 int dirty_submodule)
 {
 	diff_addremove(&revs->diffopt, prefix[0], mode,
-		       sha1, ce->name);
+		       sha1, ce->name, dirty_submodule);
 }

 static int get_stat_data(struct cache_entry *ce,
 			 const unsigned char **sha1p,
 			 unsigned int *modep,
-			 int cached, int match_missing)
+			 int cached, int match_missing,
+			 int *dirty_submodule)
 {
 	const unsigned char *sha1 = ce->sha1;
 	unsigned int mode = ce->ce_mode;
@@ -233,8 +238,11 @@ static int get_stat_data(struct cache_entry *ce,
 			return -1;
 		}
 		changed = ce_match_stat(ce, &st, 0);
-		if (changed
-		    || (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name))) {
+		if (S_ISGITLINK(ce->ce_mode) && is_submodule_modified(ce->name)) {
+			changed = 1;
+			*dirty_submodule = 1;
+		}
+		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			sha1 = null_sha1;
 		}
@@ -251,15 +259,17 @@ static void show_new_file(struct rev_info *revs,
 {
 	const unsigned char *sha1;
 	unsigned int mode;
+	int dirty_submodule = 0;

 	/*
 	 * New file in the index: it might actually be different in
 	 * the working copy.
 	 */
-	if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0)
+	if (get_stat_data(new, &sha1, &mode, cached, match_missing,
+	    &dirty_submodule) < 0)
 		return;

-	diff_index_show_file(revs, "+", new, sha1, mode);
+	diff_index_show_file(revs, "+", new, sha1, mode, dirty_submodule);
 }

 static int show_modified(struct rev_info *revs,
@@ -270,11 +280,13 @@ static int show_modified(struct rev_info *revs,
 {
 	unsigned int mode, oldmode;
 	const unsigned char *sha1;
+	int dirty_submodule = 0;

-	if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0) {
+	if (get_stat_data(new, &sha1, &mode, cached, match_missing,
+			  &dirty_submodule) < 0) {
 		if (report_missing)
 			diff_index_show_file(revs, "-", old,
-					     old->sha1, old->ce_mode);
+					     old->sha1, old->ce_mode, 0);
 		return -1;
 	}

@@ -309,7 +321,7 @@ static int show_modified(struct rev_info *revs,
 		return 0;

 	diff_change(&revs->diffopt, oldmode, mode,
-		    old->sha1, sha1, old->name);
+		    old->sha1, sha1, old->name, dirty_submodule);
 	return 0;
 }

@@ -356,7 +368,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	 * Something removed from the tree?
 	 */
 	if (!idx) {
-		diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode);
+		diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode, 0);
 		return;
 	}

diff --git a/diff.c b/diff.c
index 012b3d3..490a7ec 100644
--- a/diff.c
+++ b/diff.c
@@ -2032,7 +2032,7 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
 	char *data = xmalloc(100), *dirty = "";

 	/* Are we looking at the work tree? */
-	if (!s->sha1_valid && is_submodule_modified(s->path))
+	if (!s->sha1_valid && s->dirty_submodule)
 		dirty = "-dirty";

 	len = snprintf(data, 100,
@@ -3736,7 +3736,7 @@ int diff_result_code(struct diff_options *opt, int status)
 void diff_addremove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
-		    const char *concatpath)
+		    const char *concatpath, int dirty_submodule)
 {
 	struct diff_filespec *one, *two;

@@ -3768,8 +3768,10 @@ void diff_addremove(struct diff_options *options,

 	if (addremove != '+')
 		fill_filespec(one, sha1, mode);
-	if (addremove != '-')
+	if (addremove != '-') {
 		fill_filespec(two, sha1, mode);
+		two->dirty_submodule = dirty_submodule;
+	}

 	diff_queue(&diff_queued_diff, one, two);
 	if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
@@ -3780,7 +3782,7 @@ void diff_change(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
-		 const char *concatpath)
+		 const char *concatpath, int dirty_submodule)
 {
 	struct diff_filespec *one, *two;

@@ -3803,6 +3805,7 @@ void diff_change(struct diff_options *options,
 	two = alloc_filespec(concatpath);
 	fill_filespec(one, old_sha1, old_mode);
 	fill_filespec(two, new_sha1, new_mode);
+	two->dirty_submodule = dirty_submodule;

 	diff_queue(&diff_queued_diff, one, two);
 	if (!DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
diff --git a/diff.h b/diff.h
index 06a9a88..13596c2 100644
--- a/diff.h
+++ b/diff.h
@@ -14,12 +14,12 @@ typedef void (*change_fn_t)(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
-		 const char *fullpath);
+		 const char *fullpath, int dirty_submodule);

 typedef void (*add_remove_fn_t)(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
-		    const char *fullpath);
+		    const char *fullpath, int dirty_submodule);

 typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 		struct diff_options *options, void *data);
@@ -177,13 +177,13 @@ extern void diff_addremove(struct diff_options *,
 			   int addremove,
 			   unsigned mode,
 			   const unsigned char *sha1,
-			   const char *fullpath);
+			   const char *fullpath, int dirty_submodule);

 extern void diff_change(struct diff_options *,
 			unsigned mode1, unsigned mode2,
 			const unsigned char *sha1,
 			const unsigned char *sha2,
-			const char *fullpath);
+			const char *fullpath, int dirty_submodule);

 extern void diff_unmerge(struct diff_options *,
 			 const char *path,
diff --git a/diffcore.h b/diffcore.h
index 5b63458..66687c3 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -42,6 +42,7 @@ struct diff_filespec {
 #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)
 	unsigned should_free : 1; /* data should be free()'ed */
 	unsigned should_munmap : 1; /* data should be munmap()'ed */
+	unsigned dirty_submodule : 1;  /* For submodules: its work tree is dirty */

 	struct userdiff_driver *driver;
 	/* data should be considered "binary"; -1 means "don't know yet" */
diff --git a/revision.c b/revision.c
index 25fa14d..e95cc41 100644
--- a/revision.c
+++ b/revision.c
@@ -268,7 +268,7 @@ static int tree_difference = REV_TREE_SAME;
 static void file_add_remove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
-		    const char *fullpath)
+		    const char *fullpath, int dirty_submodule)
 {
 	int diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;

@@ -281,7 +281,7 @@ static void file_change(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
-		 const char *fullpath)
+		 const char *fullpath, int dirty_submodule)
 {
 	tree_difference = REV_TREE_DIFFERENT;
 	DIFF_OPT_SET(options, HAS_CHANGES);
diff --git a/tree-diff.c b/tree-diff.c
index 7d745b4..2ef0c77 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -68,7 +68,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 		if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {
 			newbase[baselen + pathlen1] = 0;
 			opt->change(opt, mode1, mode2,
-				    sha1, sha2, newbase);
+				    sha1, sha2, newbase, 0);
 			newbase[baselen + pathlen1] = '/';
 		}
 		retval = diff_tree_sha1(sha1, sha2, newbase, opt);
@@ -77,7 +77,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 	}

 	fullname = malloc_fullname(base, baselen, path1, pathlen1);
-	opt->change(opt, mode1, mode2, sha1, sha2, fullname);
+	opt->change(opt, mode1, mode2, sha1, sha2, fullname, 0);
 	free(fullname);
 	return 0;
 }
@@ -241,7 +241,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree

 		if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {
 			newbase[baselen + pathlen] = 0;
-			opt->add_remove(opt, *prefix, mode, sha1, newbase);
+			opt->add_remove(opt, *prefix, mode, sha1, newbase, 0);
 			newbase[baselen + pathlen] = '/';
 		}

@@ -252,7 +252,7 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 		free(newbase);
 	} else {
 		char *fullname = malloc_fullname(base, baselen, path, pathlen);
-		opt->add_remove(opt, prefix[0], mode, sha1, fullname);
+		opt->add_remove(opt, prefix[0], mode, sha1, fullname, 0);
 		free(fullname);
 	}
 }
-- 
1.6.6.327.g4c0c1.dirty

^ permalink raw reply related

* Re: [PATCH] cvsimport: modernize and standardize external tool calling
From: Junio C Hamano @ 2010-01-17 19:15 UTC (permalink / raw)
  To: Ben Walton; +Cc: git
In-Reply-To: <1263749749-3939-1-git-send-email-bwalton@artsci.utoronto.ca>

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> The cvsimport module was mixing old (git-foo) and new (git foo)
> conventions when calling git tools.  This patch standardizes the
> calling conventions used in system(), open(), exec() and backticks.
>
> Reported-by: Alexander Maier <amaier@opencsw.org>
> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
> ---
> This takes into account the feedback from Junio.

Hmph...

> -	open(my $fh, '|-', qw(git-update-index -z --index-info))
> -		or die "unable to open git-update-index: $!";
> +	open(my $fh, '|-', 'git update-index -z --index-info')
> +		or die "unable to open git update-index: $!";

I think this change is backwards (and there probably are others).

It used to use a shell-less one-element-per-argument list form to spawn
the process that reads from the pipe, but now you are passing a single
string to be split by the shell.

How about doing this as a two (perhaps three?) patch series instead?

 (1) s/git-foo/git foo/ and _nothing else_;
 (2) s/open fh, '|-', 'string'/open fh, '|-', qw(string)/ and
     s/system 'string'/system qw(string)/.

^ permalink raw reply

* Re: [PATCH] rev-parse --namespace
From: Junio C Hamano @ 2010-01-17 19:28 UTC (permalink / raw)
  To: Ilari Liusvaara; +Cc: Jeff King, git
In-Reply-To: <20100117164057.GA20554@Knoppix>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

> 'git log --branches --not --namespace=remotes/origin' 

This sounds sensible; it essentially is a natural extention of the
existing --branches, --tags and --remotes options.  While I don't care too
much, people may want to have an even shorter name (perhaps --under=heads
or even --in=tags.

In any case, we would want to have your "log" command line above both in
the proposed commit log message and the documentation in the Examples
section, given that this is not immediately obvious to even veterans like
Peff and I; it didn't "click" for me until I saw the follow-up exchange
between you two.

^ permalink raw reply

* [PATCH] commit: match explicit-ident semantics for summary and template
From: Jeff King @ 2010-01-17 19:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Currently the commit summary will show the committer only if
neither the name or email was set explicitly. However, the
commit message template will show the committer unless
_both_ are set explicitly. This patch gives the same
behavior.

The difference came about because the two topics (one
enhancing the template semantics, and the other adding the
summary warning) were developed independently.

Signed-off-by: Jeff King <peff@peff.net>
---
This goes on top of 'next' as a result of merging jc/ident and
jk/warn-author-committer-after-commit. It's not correct on top of either
topic individually.

 builtin-commit.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 37c902c..d4eef6d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1099,7 +1099,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		strbuf_addstr(&format, "\n Author: ");
 		strbuf_addbuf_percentquote(&format, &author_ident);
 	}
-	if (!user_ident_explicitly_given) {
+	if (user_ident_explicitly_given != IDENT_ALL_GIVEN) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
 		if (advice_implicit_identity) {
-- 
1.6.6.418.gd5443

^ permalink raw reply related

* Re: [PATCH 1/2] branch: allow creating a branch with same name and same starting point.
From: Junio C Hamano @ 2010-01-17 19:38 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git
In-Reply-To: <1263737212-8101-2-git-send-email-Matthieu.Moy@imag.fr>

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Previously, "git branch --track newname old" was rejected if newname
> existed, even if it already had the same value. This patch allows it,
> even without --force. This has two advantages:
>
> * Not requiring --force for a safe operation, hence allowing the user to
>   benefit from the other safety checks.
>
> * Allow changing the configuration of the checked-out branch.

Two issues I have to ask because you didn't cover these cases in your
tests [*1*]:

 - What should "git branch new old" do when (0) local branch new already
   exists and points at old, (1) branch.new.{remote,rebase,merge} do not
   currently exist, and (2) branch.autosetup{merge,rebase} is set?

 - What should "git branch --no-track new old" do when (0) local branch
   new already exists and points at old, (1)
   branch.new.{remote,rebase,merge} do currently exist?

No matter what they do, should there be some extra message when the new
code does something that the old code didn't do?

> +test_expect_success 'setup tracking with branch --track on existing branch' '
> ...
> +	git branch from-master master --track &&

The parser may be too loosely implemented and might allow this today, but
please stick to "command names then dashed options and then arguments".

We have to fix the parser someday to be more strict and broken order in
tests like this will get in the way.

> +	git config branch.from-master.merge > actual &&
> +	grep -q "^refs/heads/master$" actual
> +'
>  test_done

[Footnote]

*1* it is an easy trap to fall into to test only what it newly does while
showing your shiny new toy and forget to test conditions that the shiny
new toy should not kick in.

^ permalink raw reply

* Re: What's cooking in git.git (Jan 2010, #05; Sat, 16)
From: Jeff King @ 2010-01-17 19:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vljfxa1o6.fsf@alter.siamese.dyndns.org>

On Sat, Jan 16, 2010 at 06:46:33PM -0800, Junio C Hamano wrote:

> * jc/ident (2010-01-08) 3 commits
>  - ident.c: treat $EMAIL as giving user.email identity explicitly
>   (merged to 'next' on 2010-01-10 at f1f9ded)
>  + ident.c: check explicit identity for name and email separately
>  + ident.c: remove unused variables
> 
> Opinions on the topmost one?

I don't see a problem with the top one as long as the order is (in order
of decreasing precedence):

  1. GIT_*_EMAIL
  2. git config
  3. EMAIL
  4. `whoami`@`hostname`

which from my tests seems to be the case. That is, even though
environment variables generally take precedence over config,
git-specific environment variables and config take precedence over
generic config.

Otherwise I think we may surprise some people who have EMAIL set.

-Peff

^ permalink raw reply

* [PATCH] git status: Show uncommitted submodule changes too when enabled
From: Jens Lehmann @ 2010-01-17 19:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: pkufranky, Git Mailing List, Lars Hjemli

When the configuration variable status.submodulesummary is not 0 or
false, "git status" shows the submodule summary of the staged submodule
commits. But it did not show the summary of those commits not yet
staged in the supermodule, making it hard to see what will not be
committed.

The output of "submodule summary --for-status" has been changed from
"# Modified submodules:" to "# Submodule changes to be committed:" for
the already staged changes. "# Submodules changed but not updated:" has
been added for changes that will not be committed. This is much clearer
and consistent with the output for regular files.

Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---


While looking for ways to teach "git status" how to show dirty
submodules i came across this functionality. I think the output of
only the staged submodule changes is not enough, submodule commits
not staged in the superproject should be shown too (At the time this
functionality was introduced the "--files" option to "git submodule
summary" was missing, so there was no way to show the unstaged
submodule changes back then).


 git-submodule.sh             |    6 +++++-
 t/t7401-submodule-summary.sh |    2 +-
 t/t7508-status.sh            |    4 ++--
 wt-status.c                  |   12 +++++++-----
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 77d2232..664f217 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -688,7 +688,11 @@ cmd_summary() {
 		echo
 	done |
 	if test -n "$for_status"; then
-		echo "# Modified submodules:"
+		if [ -n "$files" ]; then
+			echo "# Submodules changed but not updated:"
+		else
+			echo "# Submodule changes to be committed:"
+		fi
 		echo "#"
 		sed -e 's|^|# |' -e 's|^# $|#|'
 	else
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 6cc16c3..d3c039f 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -213,7 +213,7 @@ EOF
 test_expect_success '--for-status' "
     git submodule summary --for-status HEAD^ >actual &&
     test_cmp actual - <<EOF
-# Modified submodules:
+# Submodule changes to be committed:
 #
 # * sm1 $head6...0000000:
 #
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index cf67fe3..556d0fa 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -579,7 +579,7 @@ cat >expect <<EOF
 #
 #	modified:   dir1/modified
 #
-# Modified submodules:
+# Submodule changes to be committed:
 #
 # * sm 0000000...$head (1):
 #   > Add foo
@@ -672,7 +672,7 @@ cat >expect <<EOF
 #
 #	modified:   dir1/modified
 #
-# Modified submodules:
+# Submodule changes to be committed:
 #
 # * sm 0000000...$head (1):
 #   > Add foo
diff --git a/wt-status.c b/wt-status.c
index 65feb29..5807fc3 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -459,7 +459,7 @@ static void wt_status_print_changed(struct wt_status *s)
 	wt_status_print_trailer(s);
 }

-static void wt_status_print_submodule_summary(struct wt_status *s)
+static void wt_status_print_submodule_summary(struct wt_status *s, int uncommitted)
 {
 	struct child_process sm_summary;
 	char summary_limit[64];
@@ -468,11 +468,11 @@ static void wt_status_print_submodule_summary(struct wt_status *s)
 	const char *argv[] = {
 		"submodule",
 		"summary",
-		"--cached",
+		uncommitted ? "--files" : "--cached",
 		"--for-status",
 		"--summary-limit",
 		summary_limit,
-		s->amend ? "HEAD^" : "HEAD",
+		uncommitted ? NULL : (s->amend ? "HEAD^" : "HEAD"),
 		NULL
 	};

@@ -580,8 +580,10 @@ void wt_status_print(struct wt_status *s)
 	wt_status_print_updated(s);
 	wt_status_print_unmerged(s);
 	wt_status_print_changed(s);
-	if (s->submodule_summary)
-		wt_status_print_submodule_summary(s);
+	if (s->submodule_summary) {
+		wt_status_print_submodule_summary(s, 0);  /* staged */
+		wt_status_print_submodule_summary(s, 1);  /* unstaged */
+	}
 	if (s->show_untracked_files)
 		wt_status_print_untracked(s);
 	else if (s->commitable)
-- 
1.6.6.327.g4c0c1.dirty

^ permalink raw reply related

* Re: [PATCH] Performance optimization for detection of modified submodules
From: Junio C Hamano @ 2010-01-17 20:01 UTC (permalink / raw)
  To: Jens Lehmann
  Cc: Git Mailing List, Johannes Schindelin, Shawn O. Pearce,
	Heiko Voigt, Lars Hjemli
In-Reply-To: <4B535E97.1020809@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

> So here is my first attempt of implementing your proposal. The test suite
> runs fine, but a few more eyeballs would really be appreciated as i am not
> very familiar with the code and its corner cases (See diff_change(), is it
> sufficient to only set "two->dirty_submodule", even if the REVERSE_DIFF
> option is set? Apart from that i am not so sure about the four changes to
> tree-diff.c).

The effect of REVERSE_DIFF bit is contained in the output layer.  The
order frontends (e.g. diff-files and diff-lib.c::run_diff_files()) feed
the entries from two hierarchies is not affected.

The current callers of addremove() may always give the work tree side as
the second one, but the API is meant to be usable by any other new callers
and for some of them feeding the work tree side as the first one _might_
be more sensible (we are talking about futureproofing, so by definition we
won't know).  It might even be the case where an unanticipated new caller
might be comparing two trees both living in the work tree (hence you might
require two independent dirty_submodule bits to the call to show which
side is dirty, and such a caller may say "both sides are dirty").

So it would be most future-proof if you add two independent "dirty" bits
to the API if you are changing it: "is the left side of the comparision a
dirty submodule?" and "is the right side ...?".  Especially I don't think
assuming "setting two->dirty is enough for the current implementation" is
the right way going forward.

> I think we could skip the call to is_submodule_modified() in
> run_diff_files() and get_stat_data() when the changed flag is already
> set and only short output (without calling diff_populate_gitlink(), e.g.
> "git status -s" or "git diff-files") is requested.

I am puzzled by your "we could skip"; isn't it what you already have done
in this patch?  More importantly, I think that is the whole point of the
change to diff API this patch brings in.

> What do you think
> about doing that in a seperate patch?

Doing these in this same patch like you did is better, as it demonstrates
how the callers benefit by the addition of these new bits to the API.

^ permalink raw reply

* Re: [PATCH] Performance optimization for detection of modified submodules
From: Jens Lehmann @ 2010-01-17 20:33 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Johannes Schindelin, Shawn O. Pearce,
	Heiko Voigt, Lars Hjemli
In-Reply-To: <7vska4xzzf.fsf@alter.siamese.dyndns.org>

Am 17.01.2010 21:01, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> So here is my first attempt of implementing your proposal. The test suite
>> runs fine, but a few more eyeballs would really be appreciated as i am not
>> very familiar with the code and its corner cases (See diff_change(), is it
>> sufficient to only set "two->dirty_submodule", even if the REVERSE_DIFF
>> option is set? Apart from that i am not so sure about the four changes to
>> tree-diff.c).
> 
> The effect of REVERSE_DIFF bit is contained in the output layer.  The
> order frontends (e.g. diff-files and diff-lib.c::run_diff_files()) feed
> the entries from two hierarchies is not affected.
> 
> The current callers of addremove() may always give the work tree side as
> the second one, but the API is meant to be usable by any other new callers
> and for some of them feeding the work tree side as the first one _might_
> be more sensible (we are talking about futureproofing, so by definition we
> won't know).  It might even be the case where an unanticipated new caller
> might be comparing two trees both living in the work tree (hence you might
> require two independent dirty_submodule bits to the call to show which
> side is dirty, and such a caller may say "both sides are dirty").
> 
> So it would be most future-proof if you add two independent "dirty" bits
> to the API if you are changing it: "is the left side of the comparision a
> dirty submodule?" and "is the right side ...?".  Especially I don't think
> assuming "setting two->dirty is enough for the current implementation" is
> the right way going forward.

Thanks for your explanation, will provide two independent dirty_submodule
bits there.


>> I think we could skip the call to is_submodule_modified() in
>> run_diff_files() and get_stat_data() when the changed flag is already
>> set and only short output (without calling diff_populate_gitlink(), e.g.
>> "git status -s" or "git diff-files") is requested.
> 
> I am puzzled by your "we could skip"; isn't it what you already have done
> in this patch?  More importantly, I think that is the whole point of the
> change to diff API this patch brings in.

This patch was about calling is_submodule_modified() /only once/, either
in run_diff_files() or in get_stat_data(), and reuse the result. But i
think we don't have to call it /at all/ when for example executing "git
status -s" and the HEAD of the submodule does not match the commit in the
index of the superproject. This information is enough to display an 'M'.
No need to check the submodule work tree for dirtyness, as it won't
change the output of the command anymore.

^ permalink raw reply

* Re: [PATCH 5/8] rerere: use ll_merge() instead of using xdl_merge()
From: Johannes Sixt @ 2010-01-17 20:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v4omk8sjg.fsf@alter.siamese.dyndns.org>

On Sonntag, 17. Januar 2010, Junio C Hamano wrote:
> So I didn't think using ll_merge() makes things worse, and that was the
> reason why I did it this way.

Thinking a bit more about it, the problematic point is not that a custom merge 
driver is used, but it is the way how conflicts are marked. For example, it 
seems a bit strange that an XML merge driver would mark-up conflicts using 
<<<<<<< and >>>>>>> in random points of XML text, when it knows that the 
result would be invalid XML (and subsequently an XML editor could fail to 
parse the result). It would be more apropriate when it used a different way 
to mark conflicts.

The conclusion is that your approach goes in the right direction. But since 
the result depends on the traditional conflict markers, it keeps rerere tied 
to the standard text merge driver. The capability that rerere will use custom 
merge drivers is of little use (unless the merge driver uses standard 
conflict markers); that it allows attributes for the standard text merge, is, 
of course, a plus.

-- Hannes

^ permalink raw reply

* Re: [PATCH] commit: match explicit-ident semantics for summary and template
From: Johannes Sixt @ 2010-01-17 20:53 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20100117193401.GA28448@coredump.intra.peff.net>

On Sonntag, 17. Januar 2010, Jeff King wrote:
> -	if (!user_ident_explicitly_given) {
> +	if (user_ident_explicitly_given != IDENT_ALL_GIVEN) {
>  		strbuf_addstr(&format, "\n Committer: ");

Sorry for chiming in so late, but this new condition worries me a bit. On all 
of my machines I have the GECOS field filled in with "Johannes Sixt", i.e., I 
do not need user.name. But of course the automatically derived email address 
is nonsense, so I've set up only user.email. Now I would always this hint, 
wouldn't I? Do most others fill in GECOS in ways that are inappropriate for 
git?

-- Hannes

^ 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