Git development
 help / color / mirror / Atom feed
* Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()
From: Mike Hommey @ 2009-01-18 19:30 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, gitster
In-Reply-To: <alpine.DEB.1.00.0901182010380.3586@pacific.mpi-cbg.de>

On Sun, Jan 18, 2009 at 08:11:07PM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 18 Jan 2009, Johannes Schindelin wrote:
> 
> > On Sun, 18 Jan 2009, Mike Hommey wrote:
> > 
> > > diff --git a/transport.c b/transport.c
> > > index 56831c5..6919ff1 100644
> > > --- a/transport.c
> > > +++ b/transport.c
> > > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> > >  		free(ref);
> > >  	}
> > >  
> > > +	http_cleanup();
> > > +	free(refs_url);
> > >  	return refs;
> > >  }
> > 
> > You cannot http_cleanup() here, as http-push calls that function, but 
> > continues to want to use curl.
> 
> Worse, a http clone will hit the same issue.

IIRC, it doesn't break anything because getting refs is going to happen
at the beginning, and the following http request is going to
reinitialize the whole thing.

It is suboptimal, I agree.

Mike

^ permalink raw reply

* Re: [PATCH 2/2] expand --pretty=format color options
From: Jeff King @ 2009-01-18 19:43 UTC (permalink / raw)
  To: René Scharfe; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <20090118173753.GB17434@coredump.intra.peff.net>

On Sun, Jan 18, 2009 at 12:37:53PM -0500, Jeff King wrote:

> On Sun, Jan 18, 2009 at 06:13:21PM +0100, René Scharfe wrote:
> > Another step would be for --pretty=format to respect the color config,
> > i.e. it shouldn't print any colour codes if colouring is turned off or
> > if set to auto while writing to file or pipe.
> 
> That makes sense to me. In theory, we could offer an "always use this
> color" and a "conditionally use this color" substitution. But I don't
> really see why anyone would want the "always use this color" one (they
> could just say --color, then).

Here is a patch that seems to work. It predicates pretty format colors
on diff colors, which is the same way the yellow commit header works in
log-tree. I don't know if it makes more sense to introduce yet another
color config option.

And I say "seems to work" because I remember there being some trickery
with color flags sometimes not getting set properly. However, since this
is the same flag as the yellow commit header, and called around the same
time, I think it should be fine.

One final note: if you are writing "generic" format strings, you
probably don't want to say "yellow". You want to say "whatever the user
has configured for color.diff.commit". I don't know if %C should be
overloaded for that or not (i.e., %C(commit) would be a valid color).

-- >8 --
respect color settings for --pretty=format colors

Previously, we would unconditionally print any colors the
user put into the --pretty=format specifier, regardless of
the setting of color configuration. This is not a problem if
the user writes a custom string each time git-log is
invoked. However, it makes use in scripts unnecessarily
complex, since they would have to change the format string
based on user preferences (and whether output is going to a
tty).

This patch will ignore any colors in --pretty=format if diff
colors are not turned on (they will still be parsed and
treated as placeholders, but no color will be output). This
matches the color.diff.commit colorization, which depends on
diff coloring even though it is not technically part of a
diff.
---
 pretty.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/pretty.c b/pretty.c
index b1b8620..fe606c5 100644
--- a/pretty.c
+++ b/pretty.c
@@ -563,20 +563,25 @@ static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
 			color_parse_mem(placeholder + 2,
 					end - (placeholder + 2),
 					"--pretty format", color);
-			strbuf_addstr(sb, color);
+			if (diff_use_color_default)
+				strbuf_addstr(sb, color);
 			return end - placeholder + 1;
 		}
 		if (!prefixcmp(placeholder + 1, "red")) {
-			strbuf_addstr(sb, "\033[31m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[31m");
 			return 4;
 		} else if (!prefixcmp(placeholder + 1, "green")) {
-			strbuf_addstr(sb, "\033[32m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[32m");
 			return 6;
 		} else if (!prefixcmp(placeholder + 1, "blue")) {
-			strbuf_addstr(sb, "\033[34m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[34m");
 			return 5;
 		} else if (!prefixcmp(placeholder + 1, "reset")) {
-			strbuf_addstr(sb, "\033[m");
+			if (diff_use_color_default)
+				strbuf_addstr(sb, "\033[m");
 			return 6;
 		} else
 			return 0;
-- 
1.6.1.151.g5a7da.dirty

^ permalink raw reply related

* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into  submodules
From: Lars Hjemli @ 2009-01-18 19:45 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0901181929220.3586@pacific.mpi-cbg.de>

On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>> Actually, I want this to work for bare repositories by specifying the
>> submodule odbs in the alternates file. So if the current submodule odb
>> wasn't found my plan was to check if the commit object was accessible
>> anyways but don't die() if it wasn't.
>
> Please make that an explicit option (cannot think of a good name, though),
> otherwise I will not be able to use your feature.  Making it the default
> would be inconsistent with the rest of our submodules framework.

Would a test on is_bare_repository() suffice for your use-case? That
is, something like this:

	if (!add_gitlink_odb(path->buf)) {
		linked_odb = 1;
		if (resolve_gitlink_ref(path->buf, "HEAD", sha1))
			die("Unable to lookup HEAD in %s", path->buf);
	} else if (!is_bare_repository())
		return 0;

If this isn't good enough, how do you propose it be solved?


>
>> >> +     commit = lookup_commit(sha1);
>> >> +     if (!commit)
>> >> +             die("traverse_gitlink(): internal error");
>> >
>> > s/internal error/could not access commit '%s' of submodule '%s'",
>> >                        sha1_to_hex(sha1), path);/
>>
>> Ok (I belive this codepath is virtually impossible to hit, hence the
>> "internal error", but I could of course be mistaken).
>
> You make it a function that is exported to other parts of Git in cache.h.
> So you might just as well expect it to be used by other parts at some
> stage.

This function is local to tree.c, but your point is still valid.

--
larsh

^ permalink raw reply

* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into  submodules
From: Lars Hjemli @ 2009-01-18 19:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: René Scharfe, git
In-Reply-To: <7vfxjgxwv7.fsf@gitster.siamese.dyndns.org>

On Sun, Jan 18, 2009 at 20:00, Junio C Hamano <gitster@pobox.com> wrote:
> Lars Hjemli <hjemli@gmail.com> writes:
>
>> I like the idea, but it will require thorough review of all
>> read_tree_recursive() consumers. So now we've got three different
>> approaches:
>> * me: global setting
>> * dscho: parameter to read_tree_recursive()
>> * you: accept the return value from the callback function
>>
>> Junio, what would you prefer?
>
> As usual René has the best taste in designing things ;-)

Agreed. I've pushed an updated series using his approach to the
lh/traverse-gitlinks branch in git://hjemli.net/pub/git/git
(http://hjemli.net/git/git/log/?h=lh/traverse-gitlinks), but without a
fix for Dscho's concern about the behaviour in
tree.c::traverse_gitlink(). Hopefully I'll get to send a reworked
series later tonight.

--
larsh

^ permalink raw reply

* Re: [PATCH 2/2] expand --pretty=format color options
From: Jeff King @ 2009-01-18 19:53 UTC (permalink / raw)
  To: René Scharfe; +Cc: Junio C Hamano, Markus Heidelberg, git
In-Reply-To: <20090118194328.GA31180@coredump.intra.peff.net>

On Sun, Jan 18, 2009 at 02:43:28PM -0500, Jeff King wrote:

> Here is a patch that seems to work. It predicates pretty format colors
> on diff colors, which is the same way the yellow commit header works in
> log-tree. I don't know if it makes more sense to introduce yet another
> color config option.
> 
> And I say "seems to work" because I remember there being some trickery
> with color flags sometimes not getting set properly. However, since this
> is the same flag as the yellow commit header, and called around the same
> time, I think it should be fine.

Hrm. OK, it doesn't actually work always. It does for git-log, but not
for rev-list, which leaves diff_use_color_default as -1. I don't know if
there are any other ways you can get to this code path without having
set diff_use_color_default.

Maybe it is time to do a cleanup on the color handling, which has
provided no end of these bugs. I will have to leave that for another
day, though.

-Peff

^ permalink raw reply

* Re: [PATCH] contrib/workdir: create logs/refs and rr-cache in the origin repository
From: Junio C Hamano @ 2009-01-18 19:59 UTC (permalink / raw)
  To: Adeodato Simó; +Cc: git
In-Reply-To: <20090118113830.GA1394@chistera.yi.org>

Adeodato Simó <dato@net.com.org.es> writes:

> However, I've as of late directly created bare repositories knowing that
> I wanted to work just with workdirs against it. In this case, the logs
> for each checkout'ed branch will be stored in the workdirs and not the
> repo, so deleting the workdir will make you lose those logs. Which is
> bad, since workdirs should always be safe to delete.

I had to think about the above for a while, but after realizing that you
have a strict distinction between a "workdir" and a normal "repository
with a work tree" in mind, I can see where you are coming from.  A workdir
is transient in nature and you should be able to dismiss it safely as long
as the repository it borrows from is intact.

But "safely" is somewhat relative.

What state would you be discarding when you remove a workdir?  I can think
of:

 - local uncommitted changes, in the work tree contents and in the index
   (obviously);

 - reflog for the HEAD (aka branch switching);

 - what branch you had checked out when you discarded the workdir;

and everything else (commits created, the tips and histories of refs,
configuration changes) are kept in the .git repository of the original.

But what if the original does _not_ want to keep track of changes of
certain nature?  It is nonsensical for the original not to want to keep
the commits nor the tips of the refs, but it is not unreasonable for a
bare repository used as a distribution point not want to keep reflogs, for
example.  A workdir could be defined as "a transient work tree created on
an existing repository, the side effects of working in which are saved to
the original repository (except for the ones listed above).  The kind of
side effects saved are however limited to the ones that are saved while
working in the original repository."

With such a definition, you can "safely" create a workdir out of a bare
repository, without fear of contaminating it with unwanted reflogs.

I tend to think the definition your patch seems to use would be more
useful in practice, though.

    A workdir is a new work area that is not a normal "work tree with a
    full repository", but borrows from an existing repository.  Any side
    effect from the work you do in a workdir will be saved in the original
    repository, and removing one would lose only the three kind of
    information listed above.  Creating a new workdir has the side effect
    of enabling reflogs and rerere in the original repository.

But the last sentence somehow feels dirty.

^ permalink raw reply

* Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server
From: Junio C Hamano @ 2009-01-18 20:05 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Ray Chuan, git
In-Reply-To: <alpine.DEB.1.00.0901181425420.3586@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The point is: the repository inside the document root of the web server is 
> still a valid repository.
>
> And the assumption is that whenever you have a file that looks like a 
> valid object/pack inside a valid repository, that it does not need 
> replacing.
>
> So even when optimizing the uncommon (HTTP push is 2nd class citizen), we 
> have to keep the common workflow intact (1st class citizens _are_ push by 
> file system, ssh or git://).

The first class citizens are "local use", not "copying out of the area
that looks like a repository only to http:// transport users".

> Which unfortunately means that put && move must stay.

I still do not understand why it is unfortunate.

As far as I understand, Ray's issue was that his filesystem did not like
the temporary file name that is used for the initial "put" phase because
it contained a character it did not like (colon, perhaps).  Why isn't the
patch about changing _that_ single issue?

^ permalink raw reply

* Re: [JGIT PATCH 8/8] Define a basic merge API, and a two-way tree merge strategy
From: Robin Rosenberg @ 2009-01-18 20:21 UTC (permalink / raw)
  To: tomi.pakarinen; +Cc: Shawn O. Pearce, git
In-Reply-To: <f299b4f30901171116y216835c9jc11df2d424ee0377@mail.gmail.com>

lördag 17 januari 2009 20:16:21 skrev Tomi Pakarinen:
> testTrivialTwoWay_disjointhistories() failed because merge strategy
> didn't handle missing base
> version. Am'i right?
Kyllä

Or so it seems. My test cases pass. I'll see if I can come up with somehing nasty.
If not I'll apply 

-- robin

^ permalink raw reply

* Re: [PATCH] mergetool: put the cursor on the editable file for Vim
From: Junio C Hamano @ 2009-01-18 20:29 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: markus.heidelberg, git
In-Reply-To: <20090118141854.GA25155@neumann>

SZEDER Gábor <szeder@ira.uka.de> writes:

> On Sat, Jan 17, 2009 at 05:35:22PM -0800, Junio C Hamano wrote:
>> Markus Heidelberg <markus.heidelberg@web.de> writes:
>> 
>> > When resolving conflicts, you only need to edit the $MERGED file. Put
>> > the cursor automatically into its window for vimdiff and gvimdiff to
>> > avoid doing <C-w>l every time.
>> 
>> I think this is sensible.
>> 
>> I do not use vim, and I do not know if the patch does what it claims to
>> do, though.
>
> It does.
>
> Tested-by: SZEDER Gábor <szeder@ira.uka.de>

Thanks, both.

^ permalink raw reply

* [PATCH 1/2] handle color.ui at a central place
From: Markus Heidelberg @ 2009-01-18 20:37 UTC (permalink / raw)
  To: Jeff King; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <20090118195342.GA612@coredump.intra.peff.net>

Jeff King, 18.01.2009:
> On Sun, Jan 18, 2009 at 02:43:28PM -0500, Jeff King wrote:
> 
> > Here is a patch that seems to work. It predicates pretty format colors
> > on diff colors, which is the same way the yellow commit header works in
> > log-tree. I don't know if it makes more sense to introduce yet another
> > color config option.
> > 
> > And I say "seems to work" because I remember there being some trickery
> > with color flags sometimes not getting set properly. However, since this
> > is the same flag as the yellow commit header, and called around the same
> > time, I think it should be fine.
> 
> Hrm. OK, it doesn't actually work always. It does for git-log, but not
> for rev-list, which leaves diff_use_color_default as -1. I don't know if
> there are any other ways you can get to this code path without having
> set diff_use_color_default.
> 
> Maybe it is time to do a cleanup on the color handling, which has
> provided no end of these bugs. I will have to leave that for another
> day, though.

Not sure, if you it has something to do with the following, but I had
this in my tree for some days now, waiting for the 2 commits mentioned
in the log message to graduate to master, which happend just an hour or
so ago.

And a good opportunity to test the 8< scissors :)

-- 8< --
The color.ui variable had to be evaluated in the commands that use
colors. This could lead to missing colors, if it was forgotten for
certain git commands. Centralizing the handling of color.ui for branch,
diff and status color avoids this problem and also reduces code
duplication.

See commit 3f4b609 (git-commit: color status output when color.ui is set,
2009-01-08) and commit 38920dd (git-status -v: color diff output when
color.ui is set, 2009-01-08) for fixes of these bugs.

There is a fourth variable color.interactive, but this is currently only
used in git-add-interactive.perl, so there is no need for handling this
as well, so far.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 builtin-branch.c |    4 ----
 builtin-commit.c |    9 ---------
 builtin-diff.c   |    3 ---
 builtin-log.c    |   12 ------------
 builtin-merge.c  |    4 ----
 color.c          |   16 ++++++++++++++++
 color.h          |    2 ++
 config.c         |    9 +++++++--
 8 files changed, 25 insertions(+), 34 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 02fa38f..6aa329b 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -30,7 +30,6 @@ static const char * const builtin_branch_usage[] = {
 static const char *head;
 static unsigned char head_sha1[20];
 
-static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */
 	"",		/* PLAIN (normal) */
@@ -553,9 +552,6 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	git_config(git_branch_config, NULL);
 
-	if (branch_use_color == -1)
-		branch_use_color = git_use_color_default;
-
 	track = git_branch_track;
 
 	head = resolve_ref("HEAD", head_sha1, 0, NULL);
diff --git a/builtin-commit.c b/builtin-commit.c
index 2f0b00a..5cac034 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -862,12 +862,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 
 	git_config(git_status_config, NULL);
 
-	if (wt_status_use_color == -1)
-		wt_status_use_color = git_use_color_default;
-
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	argc = parse_and_validate_options(argc, argv, builtin_status_usage, prefix);
 
 	index_file = prepare_index(argc, argv, prefix);
@@ -947,9 +941,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	git_config(git_commit_config, NULL);
 
-	if (wt_status_use_color == -1)
-		wt_status_use_color = git_use_color_default;
-
 	argc = parse_and_validate_options(argc, argv, builtin_commit_usage, prefix);
 
 	index_file = prepare_index(argc, argv, prefix);
diff --git a/builtin-diff.c b/builtin-diff.c
index d75d69b..d8645cf 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -279,9 +279,6 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	prefix = setup_git_directory_gently(&nongit);
 	git_config(git_diff_ui_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 
 	/* If this is a no-index diff, just run it and exit there. */
diff --git a/builtin-log.c b/builtin-log.c
index c7aa48e..84027d4 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -236,9 +236,6 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.simplify_history = 0;
@@ -303,9 +300,6 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.combine_merges = 1;
@@ -373,9 +367,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	init_reflog_walk(&rev.reflog_info);
 	rev.abbrev_commit = 1;
@@ -406,9 +397,6 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 
 	git_config(git_log_config, NULL);
 
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	init_revisions(&rev, prefix);
 	rev.always_show_header = 1;
 	cmd_log_init(argc, argv, prefix, &rev);
diff --git a/builtin-merge.c b/builtin-merge.c
index cf86975..8e726da 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -872,10 +872,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	git_config(git_merge_config, NULL);
 
-	/* for color.ui */
-	if (diff_use_color_default == -1)
-		diff_use_color_default = git_use_color_default;
-
 	argc = parse_options(argc, argv, builtin_merge_options,
 			builtin_merge_usage, 0);
 	if (verbosity < 0)
diff --git a/color.c b/color.c
index fc0b72a..7a8bf6e 100644
--- a/color.c
+++ b/color.c
@@ -1,9 +1,12 @@
 #include "cache.h"
 #include "color.h"
+#include "diff.h"
+#include "wt-status.h"
 
 #define COLOR_RESET "\033[m"
 
 int git_use_color_default = 0;
+int branch_use_color = -1;
 
 static int parse_color(const char *name, int len)
 {
@@ -155,6 +158,19 @@ int git_color_default_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+void git_finish_color_config(void)
+{
+	if (git_use_color_default) {
+		/* Enable colors, if undefined in the config files */
+		if (branch_use_color == -1)
+			branch_use_color = git_use_color_default;
+		if (diff_use_color_default == -1)
+			diff_use_color_default = git_use_color_default;
+		if (wt_status_use_color == -1)
+			wt_status_use_color = git_use_color_default;
+	}
+}
+
 static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 		va_list args, const char *trail)
 {
diff --git a/color.h b/color.h
index 6cf5c88..6924848 100644
--- a/color.h
+++ b/color.h
@@ -9,12 +9,14 @@
  */
 extern int git_use_color_default;
 
+extern int branch_use_color;
 
 /*
  * Use this instead of git_default_config if you need the value of color.ui.
  */
 int git_color_default_config(const char *var, const char *value, void *cb);
 
+void git_finish_color_config(void);
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty);
 void color_parse(const char *var, const char *value, char *dst);
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
diff --git a/config.c b/config.c
index 790405a..e35afbc 100644
--- a/config.c
+++ b/config.c
@@ -7,6 +7,7 @@
  */
 #include "cache.h"
 #include "exec_cmd.h"
+#include "color.h"
 
 #define MAXNAME (256)
 
@@ -637,8 +638,10 @@ int git_config(config_fn_t fn, void *data)
 	const char *home = NULL;
 
 	/* Setting $GIT_CONFIG makes git read _only_ the given config file. */
-	if (config_exclusive_filename)
-		return git_config_from_file(fn, config_exclusive_filename, data);
+	if (config_exclusive_filename) {
+		ret += git_config_from_file(fn, config_exclusive_filename, data);
+		goto finish;
+	}
 	if (git_config_system() && !access(git_etc_gitconfig(), R_OK))
 		ret += git_config_from_file(fn, git_etc_gitconfig(),
 					    data);
@@ -654,6 +657,8 @@ int git_config(config_fn_t fn, void *data)
 	repo_config = git_pathdup("config");
 	ret += git_config_from_file(fn, repo_config, data);
 	free(repo_config);
+ finish:
+	git_finish_color_config();
 	return ret;
 }
 
-- 
1.6.1.208.g3a5f4

^ permalink raw reply related

* [PATCH 2/2] move the color variables to color.c
From: Markus Heidelberg @ 2009-01-18 20:39 UTC (permalink / raw)
  To: Jeff King; +Cc: René Scharfe, Junio C Hamano, git
In-Reply-To: <200901182137.16562.markus.heidelberg@web.de>

To be consistent with where the branch color variable is located now,
move the variables for diff and status color to the same place.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
 color.c     |    4 ++--
 color.h     |    2 ++
 diff.c      |    1 -
 diff.h      |    1 -
 wt-status.c |    1 -
 wt-status.h |    1 -
 6 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/color.c b/color.c
index 7a8bf6e..b23c39c 100644
--- a/color.c
+++ b/color.c
@@ -1,12 +1,12 @@
 #include "cache.h"
 #include "color.h"
-#include "diff.h"
-#include "wt-status.h"
 
 #define COLOR_RESET "\033[m"
 
 int git_use_color_default = 0;
 int branch_use_color = -1;
+int diff_use_color_default = -1;
+int wt_status_use_color = -1;
 
 static int parse_color(const char *name, int len)
 {
diff --git a/color.h b/color.h
index 6924848..3b47d99 100644
--- a/color.h
+++ b/color.h
@@ -10,6 +10,8 @@
 extern int git_use_color_default;
 
 extern int branch_use_color;
+extern int diff_use_color_default;
+extern int wt_status_use_color;
 
 /*
  * Use this instead of git_default_config if you need the value of color.ui.
diff --git a/diff.c b/diff.c
index d235482..4bd068c 100644
--- a/diff.c
+++ b/diff.c
@@ -22,7 +22,6 @@
 static int diff_detect_rename_default;
 static int diff_rename_limit_default = 200;
 static int diff_suppress_blank_empty;
-int diff_use_color_default = -1;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
diff --git a/diff.h b/diff.h
index 4d5a327..f2c9984 100644
--- a/diff.h
+++ b/diff.h
@@ -188,7 +188,6 @@ extern void diff_unmerge(struct diff_options *,
 
 extern int git_diff_basic_config(const char *var, const char *value, void *cb);
 extern int git_diff_ui_config(const char *var, const char *value, void *cb);
-extern int diff_use_color_default;
 extern void diff_setup(struct diff_options *);
 extern int diff_opt_parse(struct diff_options *, const char **, int);
 extern int diff_setup_done(struct diff_options *);
diff --git a/wt-status.c b/wt-status.c
index 96ff2f8..5c3742b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -12,7 +12,6 @@
 #include "remote.h"
 
 int wt_status_relative_paths = 1;
-int wt_status_use_color = -1;
 int wt_status_submodule_summary;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	"",         /* WT_STATUS_HEADER: normal */
diff --git a/wt-status.h b/wt-status.h
index 78add09..6258fd0 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -36,7 +36,6 @@ struct wt_status {
 };
 
 int git_status_config(const char *var, const char *value, void *cb);
-extern int wt_status_use_color;
 extern int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
-- 
1.6.1.208.g3a5f4

^ permalink raw reply related

* Re: [PATCH] interpret_nth_last_branch(): avoid traversing the reflogs twice
From: Johannes Schindelin @ 2009-01-18 20:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Thomas Rast, git, Johannes Sixt, Johan Herland
In-Reply-To: <7vprilyt1w.fsf@gitster.siamese.dyndns.org>

Hi,

On Sat, 17 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Note: these are the memory requirements after some really unrealistically 
> > high activity, and the memory is free()d during parameter parsing.
> >
> > A much more realistical expectation would be to switch branches maybe 20 
> > times a day, which would amount to something like 36 kilobyte.  And again, 
> > they are free()d before the action really starts.
> 
> My HEAD reflog is 7MB long with 39000 entries, and among them, 13100
> entries have "checkout: moving ".
> 
> I know I will never want to switch back to the 10000th from the last 
> branch.  I am quite sure that I would forget which branch I was on after 
> switching branches three or four times (hence my original hardcoded 
> limitation of 10 which "should be plenty").  When I know I only have to 
> keep track of 10 entries, having to keep track of 13100 entries, even if 
> it is 36kB (it would actually be 260kB in my case) feels there is 
> something wrong in the design.

Hrm.  So let's leave it as a two-pass thing?

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into  submodules
From: Johannes Schindelin @ 2009-01-18 21:02 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <8c5c35580901181145x2e14fe0fq4ab0e94c13bad38a@mail.gmail.com>

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Sun, 18 Jan 2009, Lars Hjemli wrote:
> >> Actually, I want this to work for bare repositories by specifying the
> >> submodule odbs in the alternates file. So if the current submodule odb
> >> wasn't found my plan was to check if the commit object was accessible
> >> anyways but don't die() if it wasn't.
> >
> > Please make that an explicit option (cannot think of a good name, though),
> > otherwise I will not be able to use your feature.  Making it the default
> > would be inconsistent with the rest of our submodules framework.
> 
> Would a test on is_bare_repository() suffice for your use-case?

No.  Inconsistent is inconsistent.

> If this isn't good enough, how do you propose it be solved?

As I said, with an extra option that you _have_ to pass when you want 
that behavior.

> >> >> +     commit = lookup_commit(sha1);
> >> >> +     if (!commit)
> >> >> +             die("traverse_gitlink(): internal error");
> >> >
> >> > s/internal error/could not access commit '%s' of submodule '%s'",
> >> >                        sha1_to_hex(sha1), path);/
> >>
> >> Ok (I belive this codepath is virtually impossible to hit, hence the
> >> "internal error", but I could of course be mistaken).
> >
> > You make it a function that is exported to other parts of Git in cache.h.
> > So you might just as well expect it to be used by other parts at some
> > stage.
> 
> This function is local to tree.c, but your point is still valid.

My point is still valid because I never talked about the static function, 
but the non-static one which calls the static one.

Ciao,
Dscho

^ permalink raw reply

* Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()
From: Johannes Schindelin @ 2009-01-18 21:09 UTC (permalink / raw)
  To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <20090118193002.GA25134@glandium.org>

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

> On Sun, Jan 18, 2009 at 08:11:07PM +0100, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Sun, 18 Jan 2009, Johannes Schindelin wrote:
> > 
> > > On Sun, 18 Jan 2009, Mike Hommey wrote:
> > > 
> > > > diff --git a/transport.c b/transport.c
> > > > index 56831c5..6919ff1 100644
> > > > --- a/transport.c
> > > > +++ b/transport.c
> > > > @@ -508,6 +508,8 @@ static struct ref *get_refs_via_curl(struct transport *transport)
> > > >  		free(ref);
> > > >  	}
> > > >  
> > > > +	http_cleanup();
> > > > +	free(refs_url);
> > > >  	return refs;
> > > >  }
> > > 
> > > You cannot http_cleanup() here, as http-push calls that function, but 
> > > continues to want to use curl.
> > 
> > Worse, a http clone will hit the same issue.
> 
> IIRC, it doesn't break anything because getting refs is going to happen
> at the beginning, and the following http request is going to
> reinitialize the whole thing.
> 
> It is suboptimal, I agree.

>From the output of t5540-http-push.sh (which tests http clone 
as case 2, incidentally):

-- snip --
*   ok 1: setup remote repository

* expecting success:
        cd "$ROOT_PATH" &&
        git clone $HTTPD_URL/test_repo.git test_repo_clone

*** glibc detected *** git: double free or corruption (!prev): 
0x000000000077e090 ***
======= Backtrace: =========
/lib/libc.so.6[0x2ba4f5ac8b0a]
/lib/libc.so.6(cfree+0x8c)[0x2ba4f5acc6fc]
git[0x4d7ef2]
git[0x4d9073]
git[0x4d90b7]
git[0x4d1626]
git[0x4d1b2f]
git[0x4c9abb]
git[0x4ca00d]
git[0x4cad1a]
git[0x41c5f3]
git[0x404ee8]
git[0x405095]
git[0x40517b]
git[0x4053b8]
/lib/libc.so.6(__libc_start_main+0xf4)[0x2ba4f5a74b44]
git[0x404589]
======= Memory map: ========
00400000-00528000 r-xp 00000000 08:01 45859192                  
         /home/schindelin/git/git
00727000-0072d000 rw-p 00127000 08:01 45859192                  
         /home/schindelin/git/git
0072d000-007b9000 rw-p 0072d000 00:00 0                         
         [heap]
2ba4f4e47000-2ba4f4e64000 r-xp 00000000 08:01 54051869          
         /lib/ld-2.6.1.so
2ba4f4e64000-2ba4f4e68000 rw-p 2ba4f4e64000 00:00 0
2ba4f5063000-2ba4f5065000 rw-p 0001c000 08:01 54051869          
         /lib/ld-2.6.1.so
2ba4f5065000-2ba4f50a2000 r-xp 00000000 08:01 49956909          
         /usr/lib/libcurl.so.4.0.0
2ba4f50a2000-2ba4f52a1000 ---p 0003d000 08:01 49956909          
         /usr/lib/libcurl.so.4.0.0
2ba4f52a1000-2ba4f52a3000 rw-p 0003c000 08:01 49956909          
         /usr/lib/libcurl.so.4.0.0
2ba4f52a3000-2ba4f52b9000 r-xp 00000000 08:01 49956837          
         /usr/lib/libz.so.1.2.3.3
2ba4f52b9000-2ba4f54b9000 ---p 00016000 08:01 49956837          
         /usr/lib/libz.so.1.2.3.3
2ba4f54b9000-2ba4f54ba000 rw-p 00016000 08:01 49956837          
         /usr/lib/libz.so.1.2.3.3
2ba4f54ba000-2ba4f5615000 r-xp 00000000 08:01 49957372          
         /usr/lib/libcrypto.so.0.9.8
2ba4f5615000-2ba4f5815000 ---p 0015b000 08:01 49957372          
         /usr/lib/libcrypto.so.0.9.8
2ba4f5815000-2ba4f5838000 rw-p 0015b000 08:01 49957372          
         /usr/lib/libcrypto.so.0.9.8
2ba4f5838000-2ba4f583b000 rw-p 2ba4f5838000 00:00 0
2ba4f583b000-2ba4f5851000 r-xp 00000000 08:01 54051886          
         /lib/libpthread-2.6.1.so
2ba4f5851000-2ba4f5a50000 ---p 00016000 08:01 54051886          
         /lib/libpthread-2.6.1.so
2ba4f5a50000-2ba4f5a52000 rw-p 00015000 08:01 54051886          
         /lib/libpthread-2.6.1.so
2ba4f5a52000-2ba4f5a57000 rw-p 2ba4f5a52000 00:00 0
2ba4f5a57000-2ba4f5ba9000 r-xp 00000000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5ba9000-2ba4f5da8000 ---p 00152000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5da8000-2ba4f5dab000 r--p 00151000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5dab000-2ba4f5dad000 rw-p 00154000 08:01 54051872          
         /lib/libc-2.6.1.so
2ba4f5dad000-2ba4f5db2000 rw-p 2ba4f5dad000 00:00 0
2ba4f5db2000-2ba4f5ddc000 r-xp 00000000 08:01 49957359          
         /usr/lib/libgssapi_krb5.so.2.2
2ba4f5ddc000-2ba4f5fdb000 ---p 0002a000 08:01 49957359          
         /usr/lib/libgssapi_krb5.so.2.2
2ba4f5fdb000-2ba4f5fdd000 rw-p 00029000 08:01 49957359          
         /usr/lib/libgssapi_krb5.so.2.2
2ba4f5fdd000-2ba4f600e000 r-xp 00000000 08:01 49957876          
         /usr/lib/libidn.so.11.5.29
2ba4f600e000-2ba4f620e000 ---p 00031000 08:01 49957876          
         /usr/lib/libidn.so.11.5.29
2ba4f620e000-2ba4f620f000 rw-p 00031000 08:01 49957876          
         /usr/lib/libidn.so.11.5.29
2ba4f620f000-2ba4f6210000 rw-p 2ba4f620f000 00:00 0
2ba4f6210000-2ba4f6212000 r-xp 00000000 08:01 54051875          
         /lib/libdl-2.6.1.so
2ba4f6212000-2ba4f6412000 ---p 00002000 08:01 54051875          
         /lib/libdl-2.6.1.so
2ba4f6412000-2ba4f6414000 rw-p 00002000 08:01 54051875          
         /lib/libdl-2.6.1.so
2ba4f6414000-2ba4f6458000 r-xp 00000000 08:01 49957373          
         /usr/lib/libssl.so.0.9.8
2ba4f6458000-2ba4f6657000 ---p 00044000 08:01 49957373          
         /usr/lib/libssl.so.0.9.8
2ba4f6657000-2ba4f665d000 rw-p 00043000 08:01 49957373          
         /usr/lib/libssl.so.0.9.8
2ba4f665d000-2ba4f66ed000 r-xp 00000000 08:01 49957362          
         /usr/lib/libkrb5.so.3.3
2ba4f66ed000-2ba4f68ed000 ---p 00090000 08:01 49957362          
         /usr/lib/libkrb5.so.3.3
2ba4f68ed000-2ba4f68f1000 rw-p 00090000 08:01 49957362          
         /usr/lib/libkrb5.so.3.3
2ba4f68f1000-2ba4f68f2000 rw-p 2ba4f68f1000 00:00 0
2ba4f68f2000-2ba4f6916000 r-xp 00000000 08:01 49957360          
         /usr/lib/libk5crypto.so.3.1
2ba4f6916000-2ba4f6b15000 ---p 00024000 08:01 49957360          
         /usr/lib/libk5crypto.so.3.1
2ba4f6b15000-2ba4f6b17000 rw-p 00023000 08:01 49957360          
         /usr/lib/libk5crypto.so.3.1
2ba4f6b17000-2ba4f6b19000 r-xp 00000000 08:01 54050832          
         /lib/libcom_err.so.2.1
2ba4f6b19000-2ba4f6d18000 ---p 00002000 08:01 54050832          
         /lib/libcom_err.so.2.1
2ba4f6d18000-2ba4f6d19000 rw-p 00001000 08:01 54050832          
         /lib/libcom_err.so.2.1
2ba4f6d19000-2ba4f6d20000 r-xp 00000000 08:01 49957363          
         /usr/lib/libkrb5support.so.0.1
2ba4f6d20000-2ba4f6f20000 ---p 00007000 08:01 49957363          
         /usr/lib/libkrb5support.so.0.1
2ba4f6f20000-2ba4f6f21000 rw-p 00007000 08:01 49957363          
         /usr/lib/libkrb5support.so.0.1
2ba4f6f21000-2ba4f6f22000 rw-p 2ba4f6f21000 00:00 0
2ba4f6f22000-2ba4f6f24000 r-xp 00000000 08:01 54050958          
         /lib/libkeyutils-1.2.so
2ba4f6f24000-2ba4f7123000 ---p 00002000 08:01 54050958          
         /lib/libkeyutils-1.2.so
2ba4f7123000-2ba4f7124000 rw-p 00001000 08:01 54050958          
         /lib/libkeyutils-1.2.so
2ba4f7124000-2ba4f7136error: Request for 
0c973ae9bd51902a28466f3850b543fa66a6aaf4 aborted
Initialized empty Git repository in 
/home/schindelin/git/t/trash 
directory.t5540-http-push/test_repo_clone/.git/
Aborted
* FAIL 2: clone remote repository

                cd "$ROOT_PATH" &&
                git clone $HTTPD_URL/test_repo.git test_repo_clone
-- snap --

It might be a strange interaction with my patches, though.  valgrind 
pointed to the http_cleanup() call, and after removing that call, the 
message goes away.

Ciao,
Dscho

^ permalink raw reply

* Re: [WIP Patch 02/12] Some cleanup in get_refs_via_curl()
From: Johannes Schindelin @ 2009-01-18 21:10 UTC (permalink / raw)
  To: Mike Hommey; +Cc: git, gitster
In-Reply-To: <20090118191928.GA23108@glandium.org>

Hi,

On Sun, 18 Jan 2009, Mike Hommey wrote:

> On Sun, Jan 18, 2009 at 08:06:17PM +0100, Johannes Schindelin wrote:
> 
> > You cannot http_cleanup() here, as http-push calls that function, but 
> > continues to want to use curl.
> 
> Are you really sure? It doesn't seem so, to me.

Nope, sorry.  It was http clone.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server
From: Johannes Schindelin @ 2009-01-18 21:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ray Chuan, git
In-Reply-To: <7vocy4wfb2.fsf@gitster.siamese.dyndns.org>

Hi,

On Sun, 18 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Which unfortunately means that put && move must stay.
> 
> I still do not understand why it is unfortunate.

Because it is slow.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH next v4] git-notes: fix printing of multi-line notes
From: Junio C Hamano @ 2009-01-18 21:27 UTC (permalink / raw)
  To: Tor Arne Vestbø; +Cc: Johannes Schindelin, Johannes Sixt, Jeff King, git
In-Reply-To: <49708644.7040809@trolltech.com>

Tor Arne Vestbø <tavestbo@trolltech.com> writes:

> The line length was read from the same position every time,
> causing mangled output when printing notes with multiple lines.
>
> Also, adding new-line manually for each line ensures that we
> get a new-line between commits, matching git-log for commits
> without notes.
>
> Test case added to t3301-notes.sh.
>
> Signed-off-by: Tor Arne Vestbø <tavestbo@trolltech.com>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> Sorry about the delay. Here's a squashed patch.

Thanks.  This exactly matches 22a3d06 (git-notes: fix printing of
multi-line notes, 2009-01-13) I already have, so we are in a good shape.

^ permalink raw reply

* Re: [RFC PATCH] Fix gitdir detection when in subdir of gitdir
From: Junio C Hamano @ 2009-01-18 21:27 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Johannes Schindelin, SZEDER Gábor, git
In-Reply-To: <7vr63386rc.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> I think (1) the solution (almost) makes sense, (2) the patch needs to be
> explained a lot better as you mentioned in your two messages, and (3) if
> it does not affect any other case than when you are in a subdirectory of
> the .git/ directory, then you are doing something funny anyway and
> performance issue Dscho mentions, if any, is not a concern.
>
> My "(almost)" in (1) above is because the patch uses this new behaviour
> even when you are inside the .git/ directory itself (or at the root of a
> bare repository), which is a very common case that we do not have to nor
> want to change the behaviour.  It also invalidates the precondition of (3)
> above.

And this is a trivial follow-up on top of Szeder's patch.

 setup.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git c/setup.c w/setup.c
index 4049298..dd7c039 100644
--- c/setup.c
+++ w/setup.c
@@ -456,8 +456,11 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			inside_git_dir = 1;
 			if (!work_tree_env)
 				inside_work_tree = 0;
-			cwd[offset] = '\0';
-			setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
+			if (offset != len) {
+				cwd[offset] = '\0';
+				setenv(GIT_DIR_ENVIRONMENT, cwd, 1);
+			} else
+				setenv(GIT_DIR_ENVIRONMENT, ".", 1);
 			check_repository_format_gently(nongit_ok);
 			return NULL;
 		}

^ permalink raw reply related

* Re: [PATCH] http-push: support full URI in handle_remote_ls_ctx()
From: Junio C Hamano @ 2009-01-18 21:28 UTC (permalink / raw)
  To: Kirill A. Korinskiy; +Cc: git
In-Reply-To: <1232278116-6631-1-git-send-email-catap@catap.ru>

"Kirill A. Korinskiy" <catap@catap.ru> writes:

> @@ -1424,9 +1425,18 @@ static void handle_remote_ls_ctx(struct xml_ctx *ctx, int tag_closed)
>  				ls->userFunc(ls);
>  			}
>  		} else if (!strcmp(ctx->name, DAV_PROPFIND_NAME) && ctx->cdata) {
> -			ls->dentry_name = xmalloc(strlen(ctx->cdata) -
> +			char *path = ctx->cdata;
> +			if (!strcmp(ctx->cdata, "http://")) {
> +				path = strchr(path + sizeof("http://") - 1, '/');
> +			} else if (!strcmp(ctx->cdata, "https://")) {
> +				path = strchr(path + sizeof("https://") - 1, '/');
> +			}
> +
> +			path += remote->path_len;

I see you chose to address the issue I pointed out in:

    http://thread.gmane.org/gmane.comp.version-control.git/103804/focus=104363

by being more strict.  That's the only change I can spot compared to
e1f33ef (http-push: support full URI in handle_remote_ls_ctx(),
2008-12-23) that is already in maint.

Could you make this into an incremental patch?

^ permalink raw reply

* Re: [PATCH 3/7 v2] git_extract_argv0_path(): Move check for valid argv0 from caller to callee
From: Junio C Hamano @ 2009-01-18 21:28 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Steffen Prohaska, git, Johannes Schindelin
In-Reply-To: <200901182028.11345.j6t@kdbg.org>

Johannes Sixt <j6t@kdbg.org> writes:

> On Sonntag, 18. Januar 2009, Johannes Sixt wrote:
>> On Sonntag, 18. Januar 2009, Steffen Prohaska wrote:
>> > This simplifies the calling code.
>>
>> But it could really be squashed into the previous patch, after fixing...
>>
>> > @@ -23,6 +23,9 @@ const char *system_path(const char *path)
>> >
>> >  const char *git_extract_argv0_path(const char *argv0)
>> >  {
>> > +	if (!argv0 || !*argv0)
>> > +		return 0;
>> > +
>> >  	const char *slash = argv0 + strlen(argv0);
>>
>> ... this declaration after statement.
>
> And we prefer NULL over 0 for the null pointer.
>
> The series is nicely done, thank you! I am using it (the previous round) 
> without problems so far. I hope we can get this in RSN.

Thanks, both.  I take that as your Ack to the whole series.

I've rebased the series to master, squashed in fixes like the above (there
were others) so that each step can compile.  Will queue the result.

^ permalink raw reply

* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into  submodules
From: Lars Hjemli @ 2009-01-18 21:31 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0901182201140.3586@pacific.mpi-cbg.de>

On Sun, Jan 18, 2009 at 22:02, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>
>> On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > On Sun, 18 Jan 2009, Lars Hjemli wrote:
>> >> Actually, I want this to work for bare repositories by specifying the
>> >> submodule odbs in the alternates file. So if the current submodule odb
>> >> wasn't found my plan was to check if the commit object was accessible
>> >> anyways but don't die() if it wasn't.
>> >
>> > Please make that an explicit option (cannot think of a good name, though),
>> > otherwise I will not be able to use your feature.  Making it the default
>> > would be inconsistent with the rest of our submodules framework.
>>
>> Would a test on is_bare_repository() suffice for your use-case?
>
> No.  Inconsistent is inconsistent.
>
>> If this isn't good enough, how do you propose it be solved?
>
> As I said, with an extra option that you _have_ to pass when you want
> that behavior.

My concern is how to discern between wanted and unwanted submodules in
a bare repository.

With my proposed solution `git archive --submodules HEAD` in a bare
repository would only include the content of the submodule repos
listed in objects/info/alternates (since the commit referenced by the
gitlink would then be reachable).

But you mentioned that you had a repository where all the objects of
all the submodules where stored in the odb of the superproject. With
my solution, `git archive --submodules HEAD` in your (bare) repo would
then always include the content of all the submodules (since all the
objects would always be reachable), and I believe this is the behavior
you don't like.

So, would you rather have something like `git archive --submodules=foo
--submodules=bar HEAD` to explicitly tell which submodule paths to
include in the archive when executed in a bare repo?

--
larsh

^ permalink raw reply

* Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server
From: Junio C Hamano @ 2009-01-18 21:43 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Ray Chuan, git
In-Reply-To: <alpine.DEB.1.00.0901182214180.3586@pacific.mpi-cbg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 18 Jan 2009, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > Which unfortunately means that put && move must stay.
>> 
>> I still do not understand why it is unfortunate.
>
> Because it is slow.

If "slow" is a problem, why are you using http to begin with ;-)?

I'd take slow but reliable any day over fast and mostly works but
unreliable.

^ permalink raw reply

* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into  submodules
From: Johannes Schindelin @ 2009-01-18 21:55 UTC (permalink / raw)
  To: Lars Hjemli; +Cc: Junio C Hamano, git
In-Reply-To: <8c5c35580901181331v5e54f82fxc6a042962ff1cd06@mail.gmail.com>

Hi,

On Sun, 18 Jan 2009, Lars Hjemli wrote:

> So, would you rather have something like `git archive --submodules=foo 
> --submodules=bar HEAD` to explicitly tell which submodule paths to 
> include in the archive when executed in a bare repo?

That does not quite say what you tried to do, does it?  You tried to 
traverse submodules whose commit can be found in the object database.

Setting aside the fact that we usually try to avoid accessing unreachable 
objects, which your handling does not do, our "git submodule" does not do 
that either; it only handles submodules that are checked out.

Now, this behavior might be wanted, in bare as well as in non-bare 
repositories, but I think it should be triggered by an option, such as 
"--submodules=look-in-superprojects-odb".

I know, I know, the naming is horrible, but I find it just wrong to 
introduce a behavior that would only confuse users because it introduces 
inconsistent behavior.  As it is, we see too many confused users in #git 
already with consistent behavior [*1*].

Ciao,
Dscho

[*1*] Maybe we should allow cloning empty repositories (with no default 
branch at all), disable pushing into checked out branches by default, and 
make "git add empty-dir/" add a .gitignore and add that -- to squash at 
least half of the questions inside #git so that we can go back to fooling 
around there.

^ permalink raw reply

* Re: [PATCH 2/2] http-push: remove MOVE step after PUT when sending objects to server
From: Johannes Schindelin @ 2009-01-18 21:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ray Chuan, git
In-Reply-To: <7vvdscthmq.fsf@gitster.siamese.dyndns.org>

Hi,

On Sun, 18 Jan 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 18 Jan 2009, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >> > Which unfortunately means that put && move must stay.
> >> 
> >> I still do not understand why it is unfortunate.
> >
> > Because it is slow.
> 
> If "slow" is a problem, why are you using http to begin with ;-)?

Heh, indeed.

> I'd take slow but reliable any day over fast and mostly works but 
> unreliable.

Right.  Issue settled,
Dscho

^ permalink raw reply

* Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into  submodules
From: Lars Hjemli @ 2009-01-18 22:46 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <alpine.DEB.1.00.0901182244310.3586@pacific.mpi-cbg.de>

On Sun, Jan 18, 2009 at 22:55, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Sun, 18 Jan 2009, Lars Hjemli wrote:
>
>> So, would you rather have something like `git archive --submodules=foo
>> --submodules=bar HEAD` to explicitly tell which submodule paths to
>> include in the archive when executed in a bare repo?
>
> That does not quite say what you tried to do, does it?  You tried to
> traverse submodules whose commit can be found in the object database.
>
> Setting aside the fact that we usually try to avoid accessing unreachable
> objects, which your handling does not do, our "git submodule" does not do
> that either; it only handles submodules that are checked out.
>
> Now, this behavior might be wanted, in bare as well as in non-bare
> repositories, but I think it should be triggered by an option, such as
> "--submodules=look-in-superprojects-odb".

Sorry, but if your concern is whether to traverse a submodule in a
bare repo when the submodule isn't checked out (yeah, contradiction in
terms), I just don't see the point.

For non-bare repositories the policy has always been to ignore
submodules which isn't checked out, but for bare repositories there is
no obvious way (for me, at least) to apply the same policy. Therefore
I proposed to traverse all submodules where the linked commit is
reachable, but as you pointed out this would be wrong for non-bare
repositories.

I then modified my proposal to include a check on
is_bare_repository(): If we're not in a bare repository,
read_tree_recursive() is only allowed to recurse into checked out
submodules. But if we're in a bare repository, read_tree_recursive()
is allowed to recurse into any submodule with a reachable commit.

Now then, if `--submodules=look-in-superprojects-odb` should be
required to trigger the latter behavior, running `git archive
--submodules HEAD` in a bare repository would always produce identical
output as `git archive HEAD` and this is why I don't understand the
gain of 'look-in-superprojects-odb' (I thought you wanted to limit
which of the reachable submodules to recurse into).

--
larsh

^ 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