date:20130608

Fredrik Gustafsson wrote:

 I've looked into this a bit.

Thanks for investigating.

[...]
 Why don't we always print names quoted? IMHO the choose of line
 termination should not do anything else than alter the line termination.

 However, an other solution would be to use git ls-files -z in
 git-submodule.sh and then rewrite the perl-code to handle \0 instead
 of \n.

The whole point of -z is that by using a terminator that is guaranteed
not to appear in filenames, it avoids the need to quote filenames.
Otherwise at least \n would need to be quoted.

How about something like this patch?

-- 8 --
Subject: ls-files doc: clarify purpose of -z option

The purpose of the -z option is to avoid quoting issues by using a
delimiter that implies a binary-clean parser and cannot appear in
filenames, and in that spirit, -z turns off C-style quoting.  But
without looking carefully through the entire manpage, it is too easy
to miss that.

Reported-by: Fredrik Gustafsson iv...@iveqy.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
 Documentation/git-ls-files.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index c0856a6e..753c223f 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -75,7 +75,9 @@ OPTIONS
succeed.
 
 -z::
-   \0 line termination on output.
+   Terminate lines with NUL instead of LF.
+   This avoids the need to quote filenames; see the Output section
+   below for details.
 
 -x pattern::
 --exclude=pattern::
-- 
1.8.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] status: introduce status.branch to enable --branch by default

2013-06-08 Thread Jorge Juan Garcia Garcia

Some people often run 'git status -b'.
The config variable status.branch allows to set it by default.

Signed-off-by: Jorge Juan Garcia Garcia 
jorge-juan.garcia-gar...@ensimag.imag.fr
Signed-off-by: Mathieu Lienard--Mayor mathieu.lienard--ma...@ensimag.imag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
 Documentation/config.txt |3 +++
 builtin/commit.c |6 ++
 t/t7508-status.sh|   30 ++
 3 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 80cdf75..21ba9c2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2069,6 +2069,9 @@ status.relativePaths::
 status.short::
Set to true to enable --short by default in linkgit:git-status[1].
 
+status.branch::
+   Set to true to enable --branch by default in linkgit:git-status[1].
+
 status.showUntrackedFiles::
By default, linkgit:git-status[1] and linkgit:git-commit[1] show
files which are not currently tracked by Git. Directories which
diff --git a/builtin/commit.c b/builtin/commit.c
index 0f3429f..d447857 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1121,6 +1121,12 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
}
return 0;
}
+   if (!strcmp(k, status.branch)) {
+   if (!v)
+   return config_error_nonbool(k);
+   s-show_branch = git_config_bool(k,v);
+   return 0;
+   }
if (!strcmp(k, status.color) || !strcmp(k, color.status)) {
s-use_color = git_config_colorbool(k, v);
return 0;
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 4cb2b62..34cf30f 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1369,4 +1369,34 @@ test_expect_success 'status.short=false weaker than 
-s' '
 test_cmp status1 status2
 '
 
+test_expect_success 'status.branch=true same as -b' '
+git -c status.branch=true status -s status2 
+git status -sb status1 
+test_cmp status1 status2
+'
+
+test_expect_success 'status.branch=true different from --no-branch' '
+git -c status.branch=true status -s status2 
+git status -s --no-branch  status1 
+test_must_fail test_cmp status1 status2
+'
+
+test_expect_success 'status.branch=true weaker than --no-branch' '
+git -c status.branch=true status -s --no-branch status2 
+git status -s --no-branch status1 
+test_cmp status1 status2
+'
+
+test_expect_success 'status.branch=false same as --no-branch' '
+git -c status.branch=false status -s status2 
+git status -s --no-branch status1 
+test_cmp status1 status2
+'
+
+test_expect_success 'status.branch=false weaker than -b' '
+git -c status.branch=false status -sb status2 
+git status -sb status1 
+test_cmp status1 status2
+'
+
 test_done
-- 
1.7.8

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] status: introduce status.short to enable --short by default

2013-06-08 Thread Jorge Juan Garcia Garcia

Some people always run 'git status -s'.
The configuration variable status.short allows to set it by default.

Signed-off-by: Jorge Juan Garcia Garcia 
jorge-juan.garcia-gar...@ensimag.imag.fr
Signed-off-by: Mathieu Lienard--Mayor mathieu.lienard--ma...@ensimag.imag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
 Documentation/config.txt |3 +++
 builtin/commit.c |9 +
 config.c |1 +
 t/t7508-status.sh|   34 ++
 4 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e53fc5..80cdf75 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2066,6 +2066,9 @@ status.relativePaths::
relative to the repository root (this was the default for Git
prior to v1.5.4).
 
+status.short::
+   Set to true to enable --short by default in linkgit:git-status[1].
+
 status.showUntrackedFiles::
By default, linkgit:git-status[1] and linkgit:git-commit[1] show
files which are not currently tracked by Git. Directories which
diff --git a/builtin/commit.c b/builtin/commit.c
index 1621dfc..0f3429f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1112,6 +1112,15 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
s-submodule_summary = -1;
return 0;
}
+   if (!strcmp(k, status.short)) {
+   if (!v)
+   return config_error_nonbool(k);
+   if (git_config_bool(k,v)) {
+   status_format = STATUS_FORMAT_SHORT;
+   wt_shortstatus_print(s);
+   }
+   return 0;
+   }
if (!strcmp(k, status.color) || !strcmp(k, color.status)) {
s-use_color = git_config_colorbool(k, v);
return 0;
diff --git a/config.c b/config.c
index 7a85ebd..85ddbf2 100644
--- a/config.c
+++ b/config.c
@@ -9,6 +9,7 @@
 #include exec_cmd.h
 #include strbuf.h
 #include quote.h
+#include commit.h
 
 typedef struct config_file {
struct config_file *prev;
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index e2ffdac..4cb2b62 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1334,5 +1334,39 @@ test_expect_failure '.git/config ignore=all suppresses 
submodule summary' '
git config --remove-section submodule.subname 
git config -f .gitmodules  --remove-section submodule.subname
 '
+test_expect_success 'setup for testing status.short' '
+status1 
+status2
+'
+
+test_expect_success 'status.short=true same as -s' '
+git -c status.short=true status status2 
+git status -s status1 
+test_cmp status1 status2
+'
+
+test_expect_success 'status.short=true different from --no-short' '
+git -c status.short=true status status2 
+git status --no-short status1 
+test_must_fail test_cmp status1 status2
+'
+
+test_expect_success 'status.short=true weaker than --no-short' '
+git -c status.short=true status --no-short status2 
+git status --no-short status1 
+test_cmp status1 status2
+'
+
+test_expect_success 'status.short=false same as --no-short' '
+git -c status.short=false status status2 
+git status --no-short status1 
+test_cmp status1 status2
+'
+
+test_expect_success 'status.short=false weaker than -s' '
+git -c status.short=false status -s status2 
+git status -s status1 
+test_cmp status1 status2
+'
 
 test_done
-- 
1.7.8

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Fri, Jun 7, 2013 at 9:17 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Thu, Jun 6, 2013 at 11:22 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
 Hi Greg,

 On Thu, 6 Jun 2013, Greg Troxel wrote:

 As one of the people who helps maintain git packages in pkgsrc, my
 initial reaction is negative to adding a ruby dependency.

 My initial reaction, too. It was hard enough to get Perl included with Git
 for Windows (because of that pesky Subversion dependency).

 As you can see from the commit history, I was the primary force behind
 trying to get everything core in Git away from requiring scripting
 languages (I think it is an awesome thing to provide APIs for as many
 languages as possible, but a not-so-cool thing to use more than one
 language in the core code). It does not seem that anybody picked up that
 task when I left, though.

 Nobody seems to mention it yet. There's another reason behind the C
 rewrite effort: fork is costly on Windows. The C rewrite allows us to
 run with one process (most of the time). This applies for shell, perl
 and even ruby scripts because libgit.a is never meant to be used
 outside git.c context (unlike libgit2). In this regard, ruby is just
 as bad as currently supported non-C languages.

Are you sure?

---
#!/bin/sh

cat  /tmp/test EOF
require './git'

(1..100).each do |e|
  puts \`git rev-parse HEAD~#{e}\`
end
EOF

strace -o /tmp/log -e fork,clone ruby /tmp/test
cat /tmp/log
---

---
clone(child_stack=0x7f84131dbff0,
flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID,
parent_tidptr=0x7f84131dc9d0, tls=0x7f84131dc700,
child_tidptr=0x7f84131dc9d0) = 17455
+++ exited with 0 +++
---

I wrote a simple Ruby extension to access Git builtins so `git
rev-parse` actually calls cmd_rev_parse directly. I don't know of any
other language that supports so much extensibility. Of course, as soon
as one command does exit(), the script ends too. It could be useful to
do experiments though.

-- 
Felipe Contreras
#include builtin.h
#include cache.h
#include fcntl.h

#undef NORETURN
#undef PATH_SEP

#include ruby.h

static VALUE shellwords;

struct cmd_struct {
	const char *cmd;
	int (*fn)(int, const char **, const char *);
	int option;
};

#define RUN_SETUP (1  0)
#define RUN_SETUP_GENTLY (1  1)
#define USE_PAGER (1  2)
#define NEED_WORK_TREE (1  3)

static struct cmd_struct commands[] = {
	{ rev-parse, cmd_rev_parse },
	{ show, cmd_show, RUN_SETUP },
};

static VALUE git_rb_backticks(int o_argc, VALUE *o_argv, VALUE ctx)
{
	int argc, i, old;
	int pipefd[2];
	const char **argv;
	char buf[0x1000];
	VALUE command;
	int do_read;
	struct cmd_struct *cmd = NULL;
	const char *prefix = NULL;

	if (strncmp(RSTRING_PTR(o_argv[0]), git , 4)) {
		VALUE port, result;
		port = rb_funcall(rb_cIO, rb_intern(popen), 1, o_argv[0]);
		result = rb_funcall(port, rb_intern(read), 0);
		rb_funcall(result, rb_intern(chomp!), 0);
		rb_io_close(port);
		return result;
	}

	command = rb_funcall(shellwords, rb_intern(shellsplit), 1, o_argv[0]);
	rb_ary_shift(command);

	argc = RARRAY_LEN(command);
	argv = xcalloc(sizeof(*argv), argc);

	VALUE *rarray = RARRAY_PTR(command);
	for (i = 0; i  argc; i++)
		argv[i] = RSTRING_PTR(rarray[i]);

	old = dup(1);
	i = pipe(pipefd);
	dup2(pipefd[1], 1);
	close(pipefd[1]);

	for (i = 0; i  ARRAY_SIZE(commands); i++) {
		struct cmd_struct *p = commands[i];
		if (strcmp(p-cmd, argv[0]))
			continue;
		cmd = p;
	}

	if (!cmd)
		rb_raise(rb_eArgError, unknown command: %s, argv[0]);

	if (cmd-option  RUN_SETUP)
		prefix = setup_git_directory();

	i = cmd-fn(argc, argv, prefix);
	rb_last_status_set(i, getpid());

	fflush(stdout);
	dup2(old, 1);

	i = read(pipefd[0], buf, sizeof(buf));
	if (buf[i - 1] == '\n')
		i -= 1;
	buf[i] = '\0';

	return rb_str_new(buf, i);
}

void Init_git(void)
{
	rb_require(shellwords);
	shellwords = rb_define_module(Shellwords);
	rb_define_global_function(`, git_rb_backticks, -1);
}

Re: [Administrivia] On ruby and contrib/

On Fri, Jun 7, 2013 at 9:23 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 3:24 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 The reviewer pool for code written in a new language _must_ be
 seeded by some from the current set of reviewers whose judgement
 I/we can trust.

 By that standard nothing will ever change. Ever.

 Even twenty years from now, you will still only trust people that are
 familiar with shell, Perl, and C. Because the only way to gain your
 trust, is by being proficient in shell, Perl, and C.

 I don't see why a trusted person cannot learn a new language and
 convince the community to give it a try (well given that enough
 reviewers support the new language, which was Junio's point).

I do. Raise your hand if you are interested in giving a try to Ruby
for Git's core given that somebody gives convincing reasons?

How many hands do you expect?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Fri, Jun 7, 2013 at 9:35 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:16 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 This code is only useful for cherry-pick and revert built-ins, nothing
 else, so let's make it a builtin object, but make sure 'git-sequencer'
 is not generated.

 As you can see, the convention is builtin/foo.c corresponds to git-foo
 (and maybe more). Why make an exception for sequencer?

Why not?

 What do we gain from this?

Organization.

 A lot of code in libgit.a is only used by builtin commands,
 e.g. fetch-pack.c, should we move it to?

Yes.

 I ask because I moved
 fetch-pack from builtin out because of linking issues and I don't want
 the same happen to sequencer.c.

I'm sure those linking issues can be solved.

I don't see why libgit.a couldn't eventually be the same as libgit2.
We need better organization tough (e.g. builtins/lib.a).

If you are arguing favor of a more messy setup, then we should link
all the builtin/*.o to libgit.a, because the current situation just
doesn't cut it.

For example, init_copy_notes_for_rewrite() cannot be accessed by
sequencer.c, and while it's possible to move that function (and
others) to libgit.a, it doesn't make sense, because it can only be
used by builtins.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] test: improve rebase -q test

On Fri, Jun 7, 2013 at 9:44 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 3:32 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 Let's show the output so it's clear why it failed.

 I think you can always look into trash-directory.t3400 and figure why.
 But if you show this, I think you should use test_cmp to make it clear
 what is not wanted. Something like

 : expected 
 test_cmp expected output.out

Feel free to send that patch. I'm done with this one.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

On Fri, Jun 7, 2013 at 10:29 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 Felipe Contreras wrote:
 diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
 index a5e69f3..ff370a3 100755
 --- a/t/t3420-rebase-autostash.sh
 +++ b/t/t3420-rebase-autostash.sh
 @@ -71,8 +71,7 @@ testrebase() {
 test_must_fail git rebase$type related-onto-branch 
 test_path_is_file $dotest/autostash 
 ! grep dirty file3 
 -   rm -rf $dotest 
 -   git reset --hard 
 +   git rebase --abort 
 git checkout feature-branch
 '

 Incorrect.  I don't assume that --abort works yet, in this test.

Yes you do. The rest of the tests expect that the previous rebase has
been aborted.

In fact, all the tests depend on the previous test finishing
correctly, which is not the way tests should be written.

--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -70,7 +70,7 @@ testrebase() {
echo dirty file3 
test_must_fail git rebase$type related-onto-branch 
test_path_is_file $dotest/autostash 
-   ! grep dirty file3 
+   false ! grep dirty file3 
rm -rf $dotest 
git reset --hard 
git checkout feature-branch

# failed 19 among 22 test(s)

Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',
because it relies on the implementation of 'git rebase', which might
need to remove more files than $dotest.

This wouldn't be a problem if the tests were implemented correctly,
but they are not, so 'git rebase --abort' is the only sane option.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Move sequencer

On Fri, Jun 7, 2013 at 10:35 PM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 Felipe Contreras wrote:
  sequencer.c = builtin/sequencer.c | 160 
 +---
  sequencer.h = builtin/sequencer.h |   4 -

 Why exactly?  The plan was to unify continuation semantics, and get
 all the continuation-commands to use the sequencer.  That clearly
 hasn't materialized, but I don't know what this move buys us.

So? The sequencer is only used by builtin commands, and other
continuation commands are also builtins.

The sequencer needs to access methods from builtins/*.o, so unless you
propose that libgit.a includes all the objects in builtins/*.o, this
is the way to go.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 5:08 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:23 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 3:24 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 The reviewer pool for code written in a new language _must_ be
 seeded by some from the current set of reviewers whose judgement
 I/we can trust.

 By that standard nothing will ever change. Ever.

 Even twenty years from now, you will still only trust people that are
 familiar with shell, Perl, and C. Because the only way to gain your
 trust, is by being proficient in shell, Perl, and C.

 I don't see why a trusted person cannot learn a new language and
 convince the community to give it a try (well given that enough
 reviewers support the new language, which was Junio's point).

 I do. Raise your hand if you are interested in giving a try to Ruby
 for Git's core given that somebody gives convincing reasons?

Personally, no additional runtime dependency  Ruby  Python. I don't
think Ruby is available on SunOS and I prefer not to build and install
Python nor Ruby myself to be able to use Git. So no hands from me.

 How many hands do you expect?

If not many hands show up, the Git community clearly is not ready to
adopt Ruby. Maybe ask again next year when Ruby is getting more
popular?
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 5:02 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
On Fri, Jun 7, 2013 at 9:17 PM, Duy Nguyen pclo...@gmail.com wrote:
On Thu, Jun 6, 2013 at 11:22 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
Hi Greg,

On Thu, 6 Jun 2013, Greg Troxel wrote:

As one of the people who helps maintain git packages in pkgsrc, my
initial reaction is negative to adding a ruby dependency.

My initial reaction, too. It was hard enough to get Perl included with Git
for Windows (because of that pesky Subversion dependency).

As you can see from the commit history, I was the primary force behind
trying to get everything core in Git away from requiring scripting
languages (I think it is an awesome thing to provide APIs for as many
languages as possible, but a not-so-cool thing to use more than one
language in the core code). It does not seem that anybody picked up that
task when I left, though.

Nobody seems to mention it yet. There's another reason behind the C
rewrite effort: fork is costly on Windows. The C rewrite allows us to
run with one process (most of the time). This applies for shell, perl
and even ruby scripts because libgit.a is never meant to be used
outside git.c context (unlike libgit2). In this regard, ruby is just
as bad as currently supported non-C languages.

Are you sure?

I'm not saying you can't. I'm saying it's not meant to be used that
way. Which means there may be problems lurking around. You can write a
ruby extension to access libgit.a, sure, but how many people on this
list understand git design and limits _and_ ruby's good enough to spot
the bugs? If a bug is found and requires major restructuring in
libgit.a, how are you sure it's worth the effort and does not
destablize the rest of git? A better way to do it is linking against
libgit2.

---
#!/bin/sh

cat /tmp/test EOF
require './git'

(1..100).each do |e|
puts \`git rev-parse HEAD~#{e}\`
end
EOF

strace -o /tmp/log -e fork,clone ruby /tmp/test
cat /tmp/log
---

I wrote a simple Ruby extension to access Git builtins so `git
rev-parse` actually calls cmd_rev_parse directly. I don't know of any
other language that supports so much extensibility. Of course, as soon
as one command does exit(), the script ends too. It could be useful to
do experiments though.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks


Am 08.06.2013 00:29, schrieb Felipe Contreras:

We are not freeing 'istate-cache' properly.

We can't rely on 'initialized' to keep track of the 'istate-cache',
because it doesn't really mean it's initialized. So assume it always has
data, and free it before overwriting it.


That sounds a bit backwards to me.  If -initialized doesn't mean that 
the index state is initialized then something is fishy.  Would it make 
sense and be sufficient to set -initialized in add_index_entry?  Or to 
get rid of it and check for -cache_alloc instead?



Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
  read-cache.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 5e30746..a1dd04d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate, const 
char *path)
istate-version = ntohl(hdr-hdr_version);
istate-cache_nr = ntohl(hdr-hdr_entries);
istate-cache_alloc = alloc_nr(istate-cache_nr);
+   free(istate-cache);
istate-cache = xcalloc(istate-cache_alloc, sizeof(*istate-cache));
istate-initialized = 1;


You wrote earlier that this change is safe with current callers and that 
it prevents leaks with the following sequence:


discard_cache();
# add entries
read_cache();

Do we currently have such a call sequence somewhere?  Wouldn't that be a 
bug, namely forgetting to call discard_cache before read_cache?


I've added a assert(istate-cache_nr == 0); a few lines above and the 
test suite still passed.  With the hunk below, -cache is also always 
NULL and cache_alloc is always 0 at that point.  So we don't need that 
free call there in the cases covered by the test suite at least -- 
better leave it out.



@@ -1512,6 +1513,9 @@ int discard_index(struct index_state *istate)

for (i = 0; i  istate-cache_nr; i++)
free(istate-cache[i]);
+   free(istate-cache);
+   istate-cache = NULL;
+   istate-cache_alloc = 0;
resolve_undo_clear_index(istate);
istate-cache_nr = 0;
istate-cache_changed = 0;


I still like this part, but also would still recommend to remove the now 
doubly-outdated comment a few lines below that says no need to throw 
away allocated active_cache.  It was valid back when there was only a 
single in-memory instance of the index and no istate parameter.


René

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 5:14 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:35 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:16 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 This code is only useful for cherry-pick and revert built-ins, nothing
 else, so let's make it a builtin object, but make sure 'git-sequencer'
 is not generated.

 As you can see, the convention is builtin/foo.c corresponds to git-foo
 (and maybe more). Why make an exception for sequencer?

 Why not?

And while we are at why not, why don't you fork git?

 I ask because I moved
 fetch-pack from builtin out because of linking issues and I don't want
 the same happen to sequencer.c.

 I'm sure those linking issues can be solved.

Yeah, I scratched my head for hours and finally gave in. Maybe you are
better at the toolchain than me.

 I don't see why libgit.a couldn't eventually be the same as libgit2.
 We need better organization tough (e.g. builtins/lib.a).

 If you are arguing favor of a more messy setup, then we should link
 all the builtin/*.o to libgit.a, because the current situation just
 doesn't cut it.

 For example, init_copy_notes_for_rewrite() cannot be accessed by
 sequencer.c, and while it's possible to move that function (and
 others) to libgit.a, it doesn't make sense, because it can only be
 used by builtins.

libgit.a is just a way of grouping a bunch of objects together, not a
real library and not meant to be. If you aim something more organized,
please show at least a roadmap what to move where.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 6:28 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:02 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:17 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Thu, Jun 6, 2013 at 11:22 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
 Hi Greg,

 On Thu, 6 Jun 2013, Greg Troxel wrote:

 As one of the people who helps maintain git packages in pkgsrc, my
 initial reaction is negative to adding a ruby dependency.

 My initial reaction, too. It was hard enough to get Perl included with Git
 for Windows (because of that pesky Subversion dependency).

 As you can see from the commit history, I was the primary force behind
 trying to get everything core in Git away from requiring scripting
 languages (I think it is an awesome thing to provide APIs for as many
 languages as possible, but a not-so-cool thing to use more than one
 language in the core code). It does not seem that anybody picked up that
 task when I left, though.

 Nobody seems to mention it yet. There's another reason behind the C
 rewrite effort: fork is costly on Windows. The C rewrite allows us to
 run with one process (most of the time). This applies for shell, perl
 and even ruby scripts because libgit.a is never meant to be used
 outside git.c context (unlike libgit2). In this regard, ruby is just
 as bad as currently supported non-C languages.

 Are you sure?

 I'm not saying you can't. I'm saying it's not meant to be used that
 way. Which means there may be problems lurking around.

Code is code. If something is not meant to be used in certain way, you fix it.

 You can write a ruby extension to access libgit.a, sure,

I'm not using libgit.a, I'm using the builtin commands. This is
exactly the same code you run when you type 'git foo'.

 but how many people on this
 list understand git design and limits _and_ ruby's good enough to spot
 the bugs?

Now you are changing the subject. Does that mean that you accept that
'fork' wouldn't be a problem when writing Ruby scripts?

As for the people that know Git and Ruby; they can learn. Didn't you
just said that you didn't see any problem with the community learning
a new language?

 If a bug is found and requires major restructuring in
 libgit.a, how are you sure it's worth the effort and does not
 destablize the rest of git?

There is no need to destabilize anything. I just showed you 100 lines
of code that are able to run git commands without forks, and without
changing anything in libgit.a.

 A better way to do it is linking against libgit2.

I would rather use what the rest of Git uses. It doesn't make any
sense fragment even more the code, and make Ruby scripts 2nd class
citizens along the way. Plus, any script that tries to use libgit2,
would certainly need more than 100 lines.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: `gitsubmodule` does not list modules with unicode characters

2013-06-08 Thread Fredrik Gustafsson

On Sat, Jun 08, 2013 at 02:18:36AM -0700, Jonathan Nieder wrote:
 The whole point of -z is that by using a terminator that is guaranteed
 not to appear in filenames, it avoids the need to quote filenames.
 Otherwise at least \n would need to be quoted.

Thanks, now I understand why.

 
 How about something like this patch?
 
 -- 8 --
 Subject: ls-files doc: clarify purpose of -z option
 
 The purpose of the -z option is to avoid quoting issues by using a
 delimiter that implies a binary-clean parser and cannot appear in
 filenames, and in that spirit, -z turns off C-style quoting.  But
 without looking carefully through the entire manpage, it is too easy
 to miss that.
 
 Reported-by: Fredrik Gustafsson iv...@iveqy.com
 Signed-off-by: Jonathan Nieder jrnie...@gmail.com
 ---
  Documentation/git-ls-files.txt | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
 index c0856a6e..753c223f 100644
 --- a/Documentation/git-ls-files.txt
 +++ b/Documentation/git-ls-files.txt
 @@ -75,7 +75,9 @@ OPTIONS
   succeed.
  
  -z::
 - \0 line termination on output.
 + Terminate lines with NUL instead of LF.
 + This avoids the need to quote filenames; see the Output section
 + below for details.
  
  -x pattern::
  --exclude=pattern::
 -- 
 1.8.3
 

That would be very helpfull. I would suggest to add something about
unicode also (and maybe about the quotes that's added?). I'm a bit
unsure about the formulating but how about something like this:


From 114c34ea482873b39c02e63eeaf866c3e9ebfc14 Mon Sep 17 00:00:00 2001
From: Jonathan Nieder jrnie...@gmail.com
Date: Sat, 8 Jun 2013 02:18:36 -0700
Subject: [PATCH] Subject: ls-files doc: clarify purpose of -z option

The purpose of the -z option is to avoid quoting issues by using a
delimiter that implies a binary-clean parser and cannot appear in
filenames, and in that spirit, -z turns off C-style quoting.  But
without looking carefully through the entire manpage, it is too easy
to miss that.

Reported-by: Fredrik Gustafsson iv...@iveqy.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
---
 Documentation/git-ls-files.txt | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index c0856a6..ef785ba 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -75,7 +75,9 @@ OPTIONS
succeed.
 
 -z::
-   \0 line termination on output.
+   Terminate lines with NUL instead of LF.
+   This avoids the need to quote filenames; see the Output section
+   below for details.
 
 -x pattern::
 --exclude=pattern::
@@ -172,7 +174,8 @@ path. (see linkgit:git-read-tree[1] for more information on 
state)
 
 When `-z` option is not used, TAB, LF, and backslash characters
 in pathnames are represented as `\t`, `\n`, and `\\`,
-respectively.
+respectively. Multibyte characters are represented by they escaped
+equivalents.
 
 
 Exclude Patterns
-- 
1.8.1.5


-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iv...@iveqy.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 6:20 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:08 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:23 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 3:24 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 The reviewer pool for code written in a new language _must_ be
 seeded by some from the current set of reviewers whose judgement
 I/we can trust.

 By that standard nothing will ever change. Ever.

 Even twenty years from now, you will still only trust people that are
 familiar with shell, Perl, and C. Because the only way to gain your
 trust, is by being proficient in shell, Perl, and C.

 I don't see why a trusted person cannot learn a new language and
 convince the community to give it a try (well given that enough
 reviewers support the new language, which was Junio's point).

 I do. Raise your hand if you are interested in giving a try to Ruby
 for Git's core given that somebody gives convincing reasons?

 Personally, no additional runtime dependency  Ruby  Python.

You forgot to list the current ones; shell, perl, python.

 I don't
 think Ruby is available on SunOS and I prefer not to build and install
 Python nor Ruby myself to be able to use Git. So no hands from me.

It doesn't surprise me that you stopped at an assumption, instead of
making sure.

 How many hands do you expect?

 If not many hands show up, the Git community clearly is not ready to
 adopt Ruby.

And they will never be. Nor Ruby nor anything else, which was
precisely my point.

 Maybe ask again next year when Ruby is getting more popular?

You will stop again with another assumption, without ever giving it a chance.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 6:56 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 6:28 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:02 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:17 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Thu, Jun 6, 2013 at 11:22 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
 Hi Greg,

 On Thu, 6 Jun 2013, Greg Troxel wrote:

 As one of the people who helps maintain git packages in pkgsrc, my
 initial reaction is negative to adding a ruby dependency.

 My initial reaction, too. It was hard enough to get Perl included with Git
 for Windows (because of that pesky Subversion dependency).

 As you can see from the commit history, I was the primary force behind
 trying to get everything core in Git away from requiring scripting
 languages (I think it is an awesome thing to provide APIs for as many
 languages as possible, but a not-so-cool thing to use more than one
 language in the core code). It does not seem that anybody picked up that
 task when I left, though.

 Nobody seems to mention it yet. There's another reason behind the C
 rewrite effort: fork is costly on Windows. The C rewrite allows us to
 run with one process (most of the time). This applies for shell, perl
 and even ruby scripts because libgit.a is never meant to be used
 outside git.c context (unlike libgit2). In this regard, ruby is just
 as bad as currently supported non-C languages.

 Are you sure?

 I'm not saying you can't. I'm saying it's not meant to be used that
 way. Which means there may be problems lurking around.

 Code is code. If something is not meant to be used in certain way, you fix it.

Code is code. Bugs can be hard and easy. Hard bugs take a lot of time
and may not be worth it after all.

 You can write a ruby extension to access libgit.a, sure,

 I'm not using libgit.a, I'm using the builtin commands. This is
 exactly the same code you run when you type 'git foo'.

 but how many people on this
 list understand git design and limits _and_ ruby's good enough to spot
 the bugs?

 Now you are changing the subject. Does that mean that you accept that
 'fork' wouldn't be a problem when writing Ruby scripts?

There are a lot of static variables in builtin/ (and outside too),
which make it non-entrant, or at least not safe. fork provides a
process space isolation, some depend on that. And there are die()
everywhere. Good luck controlling them.

 As for the people that know Git and Ruby; they can learn. Didn't you
 just said that you didn't see any problem with the community learning
 a new language?

I said nothing about the community being ready _now_, did I? When you
have the support for Ruby in Git, sure go ahead.

 If a bug is found and requires major restructuring in
 libgit.a, how are you sure it's worth the effort and does not
 destablize the rest of git?

 There is no need to destabilize anything. I just showed you 100 lines
 of code that are able to run git commands without forks, and without
 changing anything in libgit.a.

And how do you deal with, for example die(), or thread safety?
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks

On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:
 Am 08.06.2013 00:29, schrieb Felipe Contreras:

 We are not freeing 'istate-cache' properly.

 We can't rely on 'initialized' to keep track of the 'istate-cache',
 because it doesn't really mean it's initialized. So assume it always has
 data, and free it before overwriting it.


 That sounds a bit backwards to me.  If -initialized doesn't mean that the
 index state is initialized then something is fishy.  Would it make sense and
 be sufficient to set -initialized in add_index_entry?

I don't know.

 Or to get rid of it and check for -cache_alloc instead?

That might make sense. I was thinking on renaming 'initialized' to
'loaded', but I really don't care.

 Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
 ---
   read-cache.c | 4 
   1 file changed, 4 insertions(+)

 diff --git a/read-cache.c b/read-cache.c
 index 5e30746..a1dd04d 100644
 --- a/read-cache.c
 +++ b/read-cache.c
 @@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate,
 const char *path)
 istate-version = ntohl(hdr-hdr_version);
 istate-cache_nr = ntohl(hdr-hdr_entries);
 istate-cache_alloc = alloc_nr(istate-cache_nr);
 +   free(istate-cache);
 istate-cache = xcalloc(istate-cache_alloc,
 sizeof(*istate-cache));
 istate-initialized = 1;


 You wrote earlier that this change is safe with current callers and that it
 prevents leaks with the following sequence:

 discard_cache();
 # add entries
 read_cache();

 Do we currently have such a call sequence somewhere?

I don't know.

 Wouldn't that be a
 bug, namely forgetting to call discard_cache before read_cache?

Why would it be a bug? There's nothing in the API that hints there's a
problem with that.

 I've added a assert(istate-cache_nr == 0); a few lines above and the test
 suite still passed.  With the hunk below, -cache is also always NULL and
 cache_alloc is always 0 at that point.  So we don't need that free call
 there in the cases covered by the test suite at least -- better leave it
 out.

Why leave it out? If somebody makes the mistake of doing the above
sequence, would you prefer that we leak?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2] t0070 mktemp to unwritable directory needs SANITY

2013-06-08 Thread Torsten Bögershausen

Use the SANITY prerequisite when testing if a temp file can
be created in a read only directory.
Skip the test under CYGWIN, or skip it under Unix/Linux when
it is run as root.

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/t0070-fundamental.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index da2c504..986b2a8 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -17,7 +17,7 @@ test_expect_success 'mktemp to nonexistent directory prints 
filename' '
grep doesnotexist/test err
 '
 
-test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' 
'
+test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints 
filename' '
mkdir cannotwrite 
chmod -w cannotwrite 
test_when_finished chmod +w cannotwrite 
-- 
1.8.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2] t0070 mktemp to unwritable directory needs SANITY

2013-06-08 Thread Torsten Bögershausen

Use the SANITY prerequisite when testing if a temp file can
be created in a read only directory.
Skip the test under CYGWIN, or skip it under Unix/Linux when
it is run as root.

Signed-off-by: Torsten Bögershausen tbo...@web.de
---
 t/t0070-fundamental.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index da2c504..986b2a8 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -17,7 +17,7 @@ test_expect_success 'mktemp to nonexistent directory prints 
filename' '
grep doesnotexist/test err
 '
 
-test_expect_success POSIXPERM 'mktemp to unwritable directory prints filename' 
'
+test_expect_success POSIXPERM,SANITY 'mktemp to unwritable directory prints 
filename' '
mkdir cannotwrite 
chmod -w cannotwrite 
test_when_finished chmod +w cannotwrite 
-- 
1.8.3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 6:42 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:14 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:35 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:16 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 This code is only useful for cherry-pick and revert built-ins, nothing
 else, so let's make it a builtin object, but make sure 'git-sequencer'
 is not generated.

 As you can see, the convention is builtin/foo.c corresponds to git-foo
 (and maybe more). Why make an exception for sequencer?

 Why not?

 And while we are at why not, why don't you fork git?

That's not an answer.

 I ask because I moved
 fetch-pack from builtin out because of linking issues and I don't want
 the same happen to sequencer.c.

 I'm sure those linking issues can be solved.

 Yeah, I scratched my head for hours and finally gave in. Maybe you are
 better at the toolchain than me.

I gave it a try, but transport.c needs fetch_pack(), and transport
does belong in libgit.a, so fetch_pack() belongs there too.

This is not the case for sequencer.c.

 I don't see why libgit.a couldn't eventually be the same as libgit2.
 We need better organization tough (e.g. builtins/lib.a).

 If you are arguing favor of a more messy setup, then we should link
 all the builtin/*.o to libgit.a, because the current situation just
 doesn't cut it.

 For example, init_copy_notes_for_rewrite() cannot be accessed by
 sequencer.c, and while it's possible to move that function (and
 others) to libgit.a, it doesn't make sense, because it can only be
 used by builtins.

 libgit.a is just a way of grouping a bunch of objects together, not a
 real library

That's what a library is.

 and not meant to be. If you aim something more organized,
 please show at least a roadmap what to move where.

I already did that; we move code from libgit.a to builtin/*.o until
libgit.a == libgit2. Done.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 7:25 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 6:42 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:14 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:35 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:16 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 This code is only useful for cherry-pick and revert built-ins, nothing
 else, so let's make it a builtin object, but make sure 'git-sequencer'
 is not generated.

 As you can see, the convention is builtin/foo.c corresponds to git-foo
 (and maybe more). Why make an exception for sequencer?

 Why not?

 And while we are at why not, why don't you fork git?

 That's not an answer.

Neither is Why not?

 and not meant to be. If you aim something more organized,
 please show at least a roadmap what to move where.

 I already did that; we move code from libgit.a to builtin/*.o

what code besides sequencer.c?

 until libgit.a == libgit2. Done.

Read up about the introduction of libgit2, why it was created in the
first place instead of moving a few files around renaming libgit.a to
libgit2.a. Unless you have a different definition of == than I do.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

Duy Nguyen wrote:
 until libgit.a == libgit2. Done.

 Read up about the introduction of libgit2, why it was created in the
 first place instead of moving a few files around renaming libgit.a to
 libgit2.a. Unless you have a different definition of == than I do.

As far as I know, there was never an extensive on-list discussion
about why git.git cannot be lib'ified.  The first appearance of
libgit2 is here [1].  I briefly read through the initial history of
libgit2.git too, but I cannot find a single discussion detailing why
lib'ifying git.git is fundamentally unworkable (there's some vague
mention of global state baggage and presence of die(), but that's
about it).  Unless you can point to some detailed discussions, or
write out a really good reason yourself, I don't think there's any
harm in letting fc try.  Ofcourse, he still indicated any sort of plan
yet, and I'm also waiting for that.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/99608
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/22] git-remote-mediawiki: Use the Readonly module instead of the constant pragma

2013-06-08 Thread Célestin Perdu

Le 08/06/2013 05:23, Jeff King a écrit :
 What does this series apply on top of? The existing version in master
 does not have use Readonly in it at all. The first version of your
 series introduced that line, but here it is shown as an existing line.
 Did you miss a commit when putting your patches together?

Oh yes, part of this commit went into the previous one, which was not
formated as an email when I did my git-format-patch. I should check my
patches more carefully before sending them. Sorry for this.

 What advantage does this have over use constant? I do not mind
 following guidelines from perlcritic if they are a matter of style, but
 in this case there is a cost: we now depend on the Readonly module,
 which is not part of the standard distribution. I.e., users now have to
 deal with installing an extra dependency. Is it worth it?

Like Benoit said, the problem is that they sometimes don't interpolate.
I don't know if we should keep this commit or not.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 7:55 PM, Ramkumar Ramachandra artag...@gmail.com wrote:
 Duy Nguyen wrote:
 until libgit.a == libgit2. Done.

 Read up about the introduction of libgit2, why it was created in the
 first place instead of moving a few files around renaming libgit.a to
 libgit2.a. Unless you have a different definition of == than I do.

 As far as I know, there was never an extensive on-list discussion
 about why git.git cannot be lib'ified.  The first appearance of
 libgit2 is here [1].  I briefly read through the initial history of
 libgit2.git too, but I cannot find a single discussion detailing why
 lib'ifying git.git is fundamentally unworkable (there's some vague
 mention of global state baggage and presence of die(), but that's
 about it).  Unless you can point to some detailed discussions, or
 write out a really good reason yourself, I don't think there's any
 harm in letting fc try.  Ofcourse, he still indicated any sort of plan
 yet, and I'm also waiting for that.

 [1]: http://thread.gmane.org/gmane.comp.version-control.git/99608

Hm.. I thought Shawn wrote a bit more in that mail. Apparently I was
wrong. I think it's discuessed in the list from time to time
(otherwise I wouldn't know) but I don't keep bookmarks.

I _think_ the reason is because git was never written as a reusable
library in mind from the beginning.  So global states and die() exist.
Worse, run once and let the OS clean eveything up at process exit
leads to some deliberate memory leak if it's made a library. See
alloc.c for example. The internal API is not really designed to be
usuable/stable as a library. All of these made it very hard to convert
the current code base into a true library. So the effort was put into
creating a new library instead, copying code from git code base over
when possible.

So instead of redoing it again, I think it's better that you help
libgit2 guys improve it to the extend that git commands can be easily
reimplemented. Then bring up the discussion about using libgit2 in C
Git again.
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

Felipe Contreras wrote:
 Yes you do. The rest of the tests expect that the previous rebase has
 been aborted.

 In fact, all the tests depend on the previous test finishing
 correctly, which is not the way tests should be written.

How else am I supposed to write them?  If there is a stale state from
the previous test, there isn't too much I can do.  Or should I be
cleaning up state at the beginning of each test, instead of at the
end?

 Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',
 because it relies on the implementation of 'git rebase', which might
 need to remove more files than $dotest.

Huh?  Tests aren't allowed to rely on how a command is implemented?

  $ git grep test_path t

Ofcourse they're implementation details.  Even in this very test, I
check $dotest/autostash plenty of times.

Have you read rr/rebase-autostash?  The whole idea is to inject
$dotest/autostash and teach various scripts about how their
assumptions about $dotest have changed.

 This wouldn't be a problem if the tests were implemented correctly,
 but they are not, so 'git rebase --abort' is the only sane option.

Then show me how to do it correctly.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 7:07 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 6:56 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 6:28 AM, Duy Nguyen pclo...@gmail.com wrote:

 but how many people on this
 list understand git design and limits _and_ ruby's good enough to spot
 the bugs?

 Now you are changing the subject. Does that mean that you accept that
 'fork' wouldn't be a problem when writing Ruby scripts?

 There are a lot of static variables in builtin/ (and outside too),
 which make it non-entrant, or at least not safe.

So?

 fork provides a process space isolation, some depend on that.

Process space isolation from what?

 And there are die() everywhere. Good luck controlling them.

Done.

--- a/ruby/git.c
+++ b/ruby/git.c
@@ -1,6 +1,7 @@
 #include builtin.h
 #include cache.h
 #include fcntl.h
+#include ucontext.h

 #undef NORETURN
 #undef PATH_SEP
@@ -8,6 +9,8 @@
 #include ruby.h

 static VALUE shellwords;
+static ucontext_t main_context;
+static int status;

 struct cmd_struct {
const char *cmd;
@@ -73,7 +76,14 @@ static VALUE git_rb_backticks(int o_argc, VALUE
*o_argv, VALUE ctx)
if (cmd-option  RUN_SETUP)
prefix = setup_git_directory();

-   i = cmd-fn(argc, argv, prefix);
+   getcontext(main_context);
+   if (status == 0) {
+   status += 1;
+   i = cmd-fn(argc, argv, prefix);
+   } else {
+   i = 1;
+   }
+   status = 0;
rb_last_status_set(i, getpid());

fflush(stdout);
@@ -87,9 +97,19 @@ static VALUE git_rb_backticks(int o_argc, VALUE
*o_argv, VALUE ctx)
return rb_str_new(buf, i);
 }

+static void bye(void)
+{
+   if (status != 1)
+   return;
+   status += 1;
+   setcontext(main_context);
+}
+
 void Init_git(void)
 {
rb_require(shellwords);
shellwords = rb_define_module(Shellwords);
rb_define_global_function(`, git_rb_backticks, -1);
+
+   atexit(bye);
 }

 As for the people that know Git and Ruby; they can learn. Didn't you
 just said that you didn't see any problem with the community learning
 a new language?

 I said nothing about the community being ready _now_, did I?

If they can learn Ruby five years from now, then can learn it now.

 When you have the support for Ruby in Git, sure go ahead.

You are going in circles.

 If a bug is found and requires major restructuring in
 libgit.a, how are you sure it's worth the effort and does not
 destablize the rest of git?

 There is no need to destabilize anything. I just showed you 100 lines
 of code that are able to run git commands without forks, and without
 changing anything in libgit.a.

 And how do you deal with, for example die(), or thread safety?

See above for die(), and I don't see many perl or shell scripts with
multiple threads, why should the Ruby scripts have more than one
thread?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks


Am 08.06.2013 14:15, schrieb Felipe Contreras:

On Sat, Jun 8, 2013 at 6:32 AM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:

diff --git a/read-cache.c b/read-cache.c
index 5e30746..a1dd04d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate,
const char *path)
 istate-version = ntohl(hdr-hdr_version);
 istate-cache_nr = ntohl(hdr-hdr_entries);
 istate-cache_alloc = alloc_nr(istate-cache_nr);
+   free(istate-cache);
 istate-cache = xcalloc(istate-cache_alloc,
sizeof(*istate-cache));
 istate-initialized = 1;



You wrote earlier that this change is safe with current callers and that it
prevents leaks with the following sequence:

discard_cache();
# add entries
read_cache();

Do we currently have such a call sequence somewhere?


I don't know.


Wouldn't that be a
bug, namely forgetting to call discard_cache before read_cache?


Why would it be a bug? There's nothing in the API that hints there's a
problem with that.


A comment before read_index_from says remember to discard_cache() 
before reading a different cache!.  That is probably a reminder that 
read_index_from does nothing if -initialized is set.  Entries added 
before calling read_index_from make up a different cache, however, so I 
think this comment applies for the call sequence above as well.


Only read_index_from and add_index_entry allocate -cache, and only 
discard_index frees it, so the two are a triple like malloc, realloc and 
free.


Granted, these hints are not part of the API -- that looks like a 
documentation bug, however.


Side note: I wonder why we need to guard against multiple 
read_index_from calls in a row with -initialized.  Wouldn't it be 
easier to avoid the duplicate calls in the first place?  Finding them 
now might be not so easy, though.



I've added a assert(istate-cache_nr == 0); a few lines above and the test
suite still passed.  With the hunk below, -cache is also always NULL and
cache_alloc is always 0 at that point.  So we don't need that free call
there in the cases covered by the test suite at least -- better leave it
out.


Why leave it out? If somebody makes the mistake of doing the above
sequence, would you prefer that we leak?


Leaking is better than silently cleaning up after a buggy caller because 
it still allows the underlying bug to be found.  Even better would be an 
assert, but it's important to make sure it doesn't trigger for existing 
use cases.


René

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 7:34 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 7:25 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 6:42 AM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:14 PM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 On Fri, Jun 7, 2013 at 9:35 PM, Duy Nguyen pclo...@gmail.com wrote:
 On Sat, Jun 8, 2013 at 5:16 AM, Felipe Contreras
 felipe.contre...@gmail.com wrote:
 This code is only useful for cherry-pick and revert built-ins, nothing
 else, so let's make it a builtin object, but make sure 'git-sequencer'
 is not generated.

 As you can see, the convention is builtin/foo.c corresponds to git-foo
 (and maybe more). Why make an exception for sequencer?

 Why not?

 And while we are at why not, why don't you fork git?

 That's not an answer.

 Neither is Why not?

The answer is the rest of the e-mail.

 and not meant to be. If you aim something more organized,
 please show at least a roadmap what to move where.

 I already did that; we move code from libgit.a to builtin/*.o

 what code besides sequencer.c?

A roadmap doesn't require code. If you truly think that there's
nothing else that is specific to builtins; alias.c.

 until libgit.a == libgit2. Done.

 Read up about the introduction of libgit2, why it was created in the
 first place instead of moving a few files around renaming libgit.a to
 libgit2.a. Unless you have a different definition of == than I do.

Are you being obtuse on purpose? It doesn't matter how different
libgit.a and libgit2 currently are, there's always a path from one
code-base to another. Unless libgit2 has code for builtin commands,
the first step would invariably be to move the code that is specific
for builtins to builtin/*.o.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 8:15 AM, Duy Nguyen pclo...@gmail.com wrote:

 So instead of redoing it again, I think it's better that you help
 libgit2 guys improve it to the extend that git commands can be easily
 reimplemented. Then bring up the discussion about using libgit2 in C
 Git again.

There's no reason not to move libgit2 closer to libgit.a, and libgit.a
closer to libgit2, both at the same time. I have rewritten a lot of
code using this strategy.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

Duy Nguyen wrote:
 I _think_ the reason is because git was never written as a reusable
 library in mind from the beginning.

We cannot reverse-engineer intents, but I tend to agree with this.  My
question is: so what?  Is it impossible to do now?

 So global states and die() exist.
 Worse, run once and let the OS clean eveything up at process exit
 leads to some deliberate memory leak if it's made a library. See
 alloc.c for example. The internal API is not really designed to be
 usuable/stable as a library. All of these made it very hard to convert
 the current code base into a true library. So the effort was put into
 creating a new library instead, copying code from git code base over
 when possible.

I'm not saying that we can convert libgit.a into something that's
usable as a long-running process by production servers tomorrow.  All
I'm saying is that it might be possible to get ruby (and possibly
other languages) to call into git-core, to make scripting more sane
than shell-spawning everything like brutes.  I think this is what fc
is aiming at, atleast in the foreseeable future.

As far as long-running server-side implementations go, I think jgit is
the way forward (sop is more interested in that now, I believe).
libgit2 might work for GitHub now, but I don't know if they will be
forced to move to the jvm in the future.

 So instead of redoing it again, I think it's better that you help
 libgit2 guys improve it to the extend that git commands can be easily
 reimplemented. Then bring up the discussion about using libgit2 in C
 Git again.

Please look at the code in libgit2.git briefly.  It's _very_ different
from git.git, and the amount of glue code that would be needed to
piece them together is unfathomable.

There are no git.git contributors committing to libgit2.git, or
vice-versa.  libgit2 is primarily developed by vmg, cmn, and (more
recently) rb.  It's quite an active project that's diverging from the
git.git design with every passing day.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-remote-mediawiki: Fix a bug in a regexp

In Perl, '\n' is not a newline, but instead a literal backslash followed by an
n. As the output of rev-list --first-parent is line-oriented, what we want
here is a newline.

Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr
Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
---
 contrib/mw-to-git/git-remote-mediawiki.perl |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl 
b/contrib/mw-to-git/git-remote-mediawiki.perl
index 7af202f..a06bc31 100755
--- a/contrib/mw-to-git/git-remote-mediawiki.perl
+++ b/contrib/mw-to-git/git-remote-mediawiki.perl
@@ -1190,7 +1190,7 @@ sub mw_push_revision {
# history (linearized with --first-parent)
print STDERR Warning: no common ancestor, pushing complete 
history\n;
my $history = run_git(rev-list --first-parent --children 
$local);
-   my @history = split('\n', $history);
+   my @history = split(/\n/, $history);
@history = @history[1..$#history];
foreach my $line (reverse @history) {
my @commit_info_split = split(/ |\n/, $line);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rm: better error message on failure for multiple files

Mathieu Lienard--Mayor wrote:
 @@ -170,30 +175,47 @@ static int check_local_mod(unsigned char *head, int 
 index_only)
  * intent to add entry.
  */
 if (local_changes  staged_changes) {
 -   if (!index_only || !(ce-ce_flags  CE_INTENT_TO_ADD))
 -   errs = error(_('%s' has staged content 
 different 
 -from both the file and the 
 HEAD\n
 -(use -f to force removal)), 
 name);
 +   if (!index_only || !(ce-ce_flags  
 CE_INTENT_TO_ADD)) {
 +   strbuf_addstr(files_staged, \n);

Ouch.  Wouldn't a string-list be more appropriate for this kind of thing?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

On Sat, Jun 8, 2013 at 8:19 AM, Ramkumar Ramachandra artag...@gmail.com wrote:
 Felipe Contreras wrote:

 Doing 'rm -rf $dotest' is even worst than 'git rebase --abort',
 because it relies on the implementation of 'git rebase', which might
 need to remove more files than $dotest.

 Huh?  Tests aren't allowed to rely on how a command is implemented?

   $ git grep test_path t

 Ofcourse they're implementation details.  Even in this very test, I
 check $dotest/autostash plenty of times.

The more the test relies on implementation details, the worst.

 Have you read rr/rebase-autostash?  The whole idea is to inject
 $dotest/autostash and teach various scripts about how their
 assumptions about $dotest have changed.

 This wouldn't be a problem if the tests were implemented correctly,
 but they are not, so 'git rebase --abort' is the only sane option.

 Then show me how to do it correctly.

Something like this.

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 6ba449b..b96a56a 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -33,54 +33,56 @@ test_expect_success setup '
git commit -m related commit
 '

+setup_tmp () {
+   git clone . tmp 
+   cd tmp 
+   git fetch -u origin refs/heads/*:refs/heads/* 
+   test_config rebase.autostash true 
+   git checkout -b rebased-feature-branch feature-branch
+}
+
 testrebase() {
type=$1
dotest=$2

test_expect_success rebase$type: dirty worktree, non-conflicting 
rebase '
-   test_config rebase.autostash true 
-   git reset --hard 
-   git checkout -b rebased-feature-branch feature-branch 
-   test_when_finished git branch -D rebased-feature-branch 
+   test_when_finished rm -rf tmp 
+   (
+   setup_tmp 
echo dirty file3 
git rebase$type unrelated-onto-branch 
grep unrelated file4 
-   grep dirty file3 
-   git checkout feature-branch
+   grep dirty file3
+   )
'

test_expect_success rebase$type: dirty index, non-conflicting rebase '
-   test_config rebase.autostash true 
-   git reset --hard 
-   git checkout -b rebased-feature-branch feature-branch 
-   test_when_finished git branch -D rebased-feature-branch 
+   test_when_finished rm -rf tmp 
+   (
+   setup_tmp 
echo dirty file3 
git add file3 
git rebase$type unrelated-onto-branch 
grep unrelated file4 
-   grep dirty file3 
-   git checkout feature-branch
+   grep dirty file3
+   )
'

test_expect_success rebase$type: conflicting rebase '
-   test_config rebase.autostash true 
-   git reset --hard 
-   git checkout -b rebased-feature-branch feature-branch 
-   test_when_finished git branch -D rebased-feature-branch 
+   test_when_finished rm -rf tmp 
+   (
+   setup_tmp 
echo dirty file3 
test_must_fail git rebase$type related-onto-branch 
test_path_is_file $dotest/autostash 
-   false ! grep dirty file3 
-   rm -rf $dotest 
-   git reset --hard 
-   git checkout feature-branch
+   ! grep dirty file3
+   )
'

test_expect_success rebase$type: --continue '
-   test_config rebase.autostash true 
-   git reset --hard 
-   git checkout -b rebased-feature-branch feature-branch 
-   test_when_finished git branch -D rebased-feature-branch 
+   test_when_finished rm -rf tmp 
+   (
+   setup_tmp 
echo dirty file3 
test_must_fail git rebase$type related-onto-branch 
test_path_is_file $dotest/autostash 
@@ -89,45 +91,43 @@ testrebase() {
git add file2 
git rebase --continue 
test_path_is_missing $dotest/autostash 
-   grep dirty file3 
-   git checkout feature-branch
+   grep dirty file3
+   )
'

test_expect_success rebase$type: --skip '
-   test_config rebase.autostash true 
+   test_when_finished rm -rf tmp 
+   (
+   setup_tmp 
git reset --hard 
-   git checkout -b rebased-feature-branch feature-branch 
-   test_when_finished git branch -D rebased-feature-branch 
echo dirty file3 
test_must_fail git rebase$type related-onto-branch 
test_path_is_file $dotest/autostash 
! grep dirty file3 
git rebase

Re: [PATCH 2/2] rm: introduce advice.rmHints to shorten messages

Mathieu Lienard--Mayor wrote:
 As an example, the message:
 error: 'foo.txt' has changes staged in the index
 (use --cached to keep the file, or -f to force removal)

 would look like, with advice.rmHints=true:
 error: 'foo.txt' has changes staged in the index

Um, have you switched the true with false?  advice.* variables are
true by default, and I turn off all of them.

Also, I think you can extend this to also remove add-advice.  Why
would someone want to turn off advice from rm, but not add?  (Unsure
about this)

 Similarly to advice.*, advice.rmHints has been added
 to the config variables. By default, it is set to false, in order to
 keep the messages the same as before. When set to true,  advice
 are no longer included in the error messages.

Ugh, why this roundabout-passive-past tone?  Use imperative tone like this:

Introduce advice.rmHints to control the whether to display advice when
using 'git rm'.  Defaults to true, preserving current behavior.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks

On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:
 Am 08.06.2013 14:15, schrieb Felipe Contreras:

 Why leave it out? If somebody makes the mistake of doing the above
 sequence, would you prefer that we leak?

 Leaking is better than silently cleaning up after a buggy caller because it
 still allows the underlying bug to be found.

No, it doesn't. The pointer is replaced and forever lost. How is
leaking memory helping anyone to find the bug?

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

Felipe Contreras wrote:
 Ofcourse they're implementation details.  Even in this very test, I
 check $dotest/autostash plenty of times.

 The more the test relies on implementation details, the worst.

I'm not convinced about this.

 Then show me how to do it correctly.

 Something like this.

Yeah, this is definitely better.  Can you submit this patch?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 8:34 AM, Ramkumar Ramachandra artag...@gmail.com wrote:
 Duy Nguyen wrote:
 I _think_ the reason is because git was never written as a reusable
 library in mind from the beginning.

 We cannot reverse-engineer intents, but I tend to agree with this.  My
 question is: so what?  Is it impossible to do now?

Nothing is impossible.

My feeling is that no Git developers are interested in libgit2, so the
idea of me contributing to libgit2 and leave libgit.a alone is more
like a we don't want no reorganization. Then wait until libgit2 is
ready before considering using it in Git's core, but that's never
going to happen if we don't first start to bring the two code-bases
closer together.

IOW; sweep the issue under the carpet.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 8:34 PM, Ramkumar Ramachandra artag...@gmail.com wrote:
 I'm not saying that we can convert libgit.a into something that's
 usable as a long-running process by production servers tomorrow.  All
 I'm saying is that it might be possible to get ruby (and possibly
 other languages) to call into git-core, to make scripting more sane
 than shell-spawning everything like brutes.  I think this is what fc
 is aiming at, atleast in the foreseeable future.

It's technically possible. You can already call into libgit.a as fc
demonstrated with his ruby binding. Assuming that you are willing to
dig in and fix all the problems (in a non-intrusive way) when a call
into libgit.a does not work, there's still API issue. Do we want to
freeze libgit.a API so that scripts will not be audited and changed
unncessarily? Freezing the API at cmd_* level loses a lot of
flexibility. Freezing at lower level may prevent us from making some
changes. I still think that binding new languages to a clean library
like libgit2 is better than to libgit.a. Just thinking of what might
work and what might not is already a headache.
-- 
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

On Sat, Jun 8, 2013 at 9:04 AM, Ramkumar Ramachandra artag...@gmail.com wrote:
 Felipe Contreras wrote:
 Ofcourse they're implementation details.  Even in this very test, I
 check $dotest/autostash plenty of times.

 The more the test relies on implementation details, the worst.

 I'm not convinced about this.

There's even a model called test-driven development, where you start
developing the tests even before there's any implementation. There's
also black-box testing.

There's reasons for that.

 Then show me how to do it correctly.

 Something like this.

 Yeah, this is definitely better.  Can you submit this patch?

Maybe, I don't know when.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

2013-06-08 Thread Antoine Pelisse

On Sat, Jun 8, 2013 at 4:04 PM, Ramkumar Ramachandra artag...@gmail.com wrote:
 Felipe Contreras wrote:
 Ofcourse they're implementation details.  Even in this very test, I
 check $dotest/autostash plenty of times.

 The more the test relies on implementation details, the worst.

 I'm not convinced about this.

My understanding of these tests is that they make sure new/better
implementations don't break the user experience/defined behavior. If
the test relies on the implementation, then they lose most of their
interest.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 9:10 AM, Duy Nguyen pclo...@gmail.com wrote:

 Do we want to
 freeze libgit.a API so that scripts will not be audited and changed
 unncessarily?

No. Until we ship libgit.so the API remains internal, and free to change.

 I still think that binding new languages to a clean library
 like libgit2 is better than to libgit.a. Just thinking of what might
 work and what might not is already a headache.

Let the code speak. Show me a script in any language that does
something useful using libgit2, doing the equivalent to at least a
couple of 'git foo' commands.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] status: introduce status.short to enable --short by default

Jorge Juan Garcia Garcia wrote:
 Some people always run 'git status -s'.
 The configuration variable status.short allows to set it by default.

Good feature.

 @@ -1112,6 +1112,15 @@ static int git_status_config(const char *k, const char 
 *v, void *cb)
 s-submodule_summary = -1;
 return 0;
 }
 +   if (!strcmp(k, status.short)) {
 +   if (!v)
 +   return config_error_nonbool(k);
 +   if (git_config_bool(k,v)) {
 +   status_format = STATUS_FORMAT_SHORT;
 +   wt_shortstatus_print(s);
 +   }
 +   return 0;
 +   }

Incorrect.  This is the wrong place to use config_error_nonbool():
this is very much a bool, and a [status] short in ~/.gitconfig
should not error out (all boolean variables behave in the same
manner).  When in doubt, consult config_error_nonbool(); there's
clearly a comment stating:

/*
 * Call this to report error for your variable that should not
 * get a boolean value (i.e. [my] var means true).
 */

Also, why are you calling wt_shortstatus_print() here, instead of
returning control to cmd_status(), which is going to do it anyway?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jun 2013, #03; Thu, 6)

Junio C Hamano wrote:
 * mm/color-auto-default (2013-05-15) 2 commits
  - make color.ui default to 'auto'
  - config: refactor management of color.ui's default value

  Flip the default for color.ui to 'auto', which is what many
  tutorials recommend new users to do.  The updated code claims the
  switch happened at Git 2.0 in the past tense, but we might want to
  expedite it, as this change is not all that important to deserve a
  major version bump.

  I'd vote for merging this without waiting for 2.0.  Comments?

Yes, please merge.  The commit message looks good as well: it doesn't
say anything about waiting for 2.0.

  Waiting for a reroll.

The one in pu looks fine to me.  What am I missing?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

Antoine Pelisse wrote:
 The more the test relies on implementation details, the worst.

 I'm not convinced about this.

 My understanding of these tests is that they make sure new/better
 implementations don't break the user experience/defined behavior. If
 the test relies on the implementation, then they lose most of their
 interest.

Makes sense, thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/22] git-remote-mediawiki: Check return value of open + remove import of unused open2

Le 08/06/2013 02:14, Eric Sunshine a écrit :
 These two changes are unrelated and could be split into distinct
 patches (IMHO, though others may disagree).

Two distinct patches or two distinct commits?
I assumed it was two distinct commits, but thinking about it, removing a
library could have its own patch.

-- 
Célestin Matte
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks


Am 08.06.2013 16:04, schrieb Felipe Contreras:

On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:

Am 08.06.2013 14:15, schrieb Felipe Contreras:



Why leave it out? If somebody makes the mistake of doing the above
sequence, would you prefer that we leak?


Leaking is better than silently cleaning up after a buggy caller because it
still allows the underlying bug to be found.


No, it doesn't. The pointer is replaced and forever lost. How is
leaking memory helping anyone to find the bug?


Valgrind can tell you where leaked memory was allocated, but not if you 
free it in the wrong place.


René

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

Duy Nguyen wrote:

 libgit.a is just a way of grouping a bunch of objects together, not a
 real library and not meant to be. If you aim something more organized,
 please show at least a roadmap what to move where.

Exactly.  There are some rough plans I would like to help with in the
direction of a more organized source tree (so ls output can be less
intimidating --- see Nico Pitre's mail on this a while ago for more
hints), but randomly moving files one at a time to builtin/ destroys
consistency and just makes things *worse*.  So if you'd like to work
on this, you'll need to start with a description of the endpoint, to
help people work with you to ensure it is something consistent and
usable.

Actually, Felipe, I doubt that would work well.  This project requires
understanding how a variety of people use the git source code, which
requires listening carefully to them and not alienating them so you
can find out what they need.  Someone good at moderating a discussion
could do that on-list, but based on my experience of how threads with
you go, a better strategy might be to cultivate a wiki page somewhere
with a plan and give it some time (a month, maybe) to collect input.

NAK to changing the meaning of builtin/ to built-in commands, plus
sequencer, which seriously hurts consistency.

Sincerely,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks

On Sat, Jun 8, 2013 at 10:56 AM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:
 Am 08.06.2013 16:04, schrieb Felipe Contreras:

 On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe
 rene.scha...@lsrfire.ath.cx wrote:

 Am 08.06.2013 14:15, schrieb Felipe Contreras:


 Why leave it out? If somebody makes the mistake of doing the above
 sequence, would you prefer that we leak?


 Leaking is better than silently cleaning up after a buggy caller because
 it
 still allows the underlying bug to be found.


 No, it doesn't. The pointer is replaced and forever lost. How is
 leaking memory helping anyone to find the bug?

 Valgrind can tell you where leaked memory was allocated, but not if you free
 it in the wrong place.

It is the correct place to free it. And nobody will ever find it with
valgrind, just like nobody has ever tracked down the hundreds of leaks
in Git that happen reliably 100% of the time, much less the ones that
happen rarely.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tests: fix autostash

Ramkumar Ramachandra wrote:

 How else am I supposed to write them?  If there is a stale state from
 the previous test, there isn't too much I can do.  Or should I be
 cleaning up state at the beginning of each test, instead of at the
 end?

That's one strategy.  test_when_finished to restore the set-up
state is another.

Making tests skippable unless labelled otherwise is currently an
aspirational goal rather than a practical one.  Hopefully some day
we'll get there and the test harness can start checking it. :)  It
makes reorganizing test scripts, for example by reordering tests, much
easier.

Thanks,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 11:49 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 Duy Nguyen wrote:

 libgit.a is just a way of grouping a bunch of objects together, not a
 real library and not meant to be. If you aim something more organized,
 please show at least a roadmap what to move where.

 Exactly.  There are some rough plans I would like to help with in the
 direction of a more organized source tree (so ls output can be less
 intimidating --- see Nico Pitre's mail on this a while ago for more
 hints), but randomly moving files one at a time to builtin/ destroys
 consistency and just makes things *worse*.  So if you'd like to work
 on this, you'll need to start with a description of the endpoint, to
 help people work with you to ensure it is something consistent and
 usable.

So lets stash everything together.

--- a/Makefile
+++ b/Makefile
@@ -990,6 +990,8 @@ BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o

+LIB_OBJS += $(BUILTIN_OBJS)
+
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
 EXTLIBS =

@@ -1712,9 +1714,9 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
'-DGIT_MAN_PATH=$(mandir_relative_SQ)' \
'-DGIT_INFO_PATH=$(infodir_relative_SQ)'

-git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
+git$X: git.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
-   $(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
+   $(ALL_LDFLAGS) $(LIBS)

 help.sp help.s help.o: common-cmds.h

@@ -1892,7 +1894,7 @@ VCSSVN_OBJS += vcs-svn/svndiff.o
 VCSSVN_OBJS += vcs-svn/svndump.o

 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
-OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
+OBJECTS := $(LIB_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
$(XDIFF_OBJS) \
$(VCSSVN_OBJS) \
git.o

And stop any delusions that libgit.a has any meaning at all, and along
the way get rid of any hopes of ever having an official public library
similar to libgit2.

 Actually, Felipe, I doubt that would work well.  This project requires
 understanding how a variety of people use the git source code, which
 requires listening carefully to them and not alienating them so you
 can find out what they need.

My patch covers every need. Nobody has come forward with a reason not
to organize the object files. Everything works after my patch the same
way it has worked before.

 Someone good at moderating a discussion
 could do that on-list, but based on my experience of how threads with
 you go, a better strategy might be to cultivate a wiki page somewhere
 with a plan and give it some time (a month, maybe) to collect input.

This has nothing to do with better strategy, it has everything to do
with gut feelings and tradition. Not reasons.

 NAK to changing the meaning of builtin/ to built-in commands, plus
 sequencer, which seriously hurts consistency.

Then apply the patch above and stop wasting our time with a library.
Git is nothing but a bunch of disorganized object files, all squashed
together, there's no library, nor will ever be; libgit.a is a
misnomer.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 08, 2013 at 08:20:28AM -0500, Felipe Contreras wrote:

  There are a lot of static variables in builtin/ (and outside too),
  which make it non-entrant, or at least not safe.
 
 So?
 
  fork provides a process space isolation, some depend on that.
 
 Process space isolation from what?

Manipulation of global variables. Here are a few examples off the top of
my head:

Try running git diff from your Ruby hook, then try running git
diff-files within the same process. I believe the latter will start
respecting porcelain diff config like diff.mnemonicprefix. To clear
state you need to reset a list of global variables back to their initial
states (some of which are the BSS-default zero, but some of which are
not).

Try running git log followed by another git log. The log family of
commands does not clear its marks from the commit objects, since it
expects to exit after the traversal. The second log will sometimes give
wrong answers if its traversal overlaps with the first (e.g., commits
marked SEEN or UNINTERESTING that should not be). You can add a call to
clear them at the end of the process, but that does not cover any cases
where we die().

These are problems that can be solved. But there is a lot of work
involved in finding these subtle bugs and coming up with fixes. I think
you would be better off working on an implementation of git that was
designed from scratch to work in-process, like libgit2. And it even has
an actively developed and maintained Ruby binding[1].

libgit2 doesn't have feature parity with regular git yet, but there are
many clients based around it that use the library internally for speed,
and then exec regular git to fill in the gaps.

-Peff

[1] https://github.com/libgit2/rugged
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks


Am 08.06.2013 18:53, schrieb Felipe Contreras:

On Sat, Jun 8, 2013 at 10:56 AM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:

Am 08.06.2013 16:04, schrieb Felipe Contreras:


On Sat, Jun 8, 2013 at 8:22 AM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:


Am 08.06.2013 14:15, schrieb Felipe Contreras:




Why leave it out? If somebody makes the mistake of doing the above
sequence, would you prefer that we leak?



Leaking is better than silently cleaning up after a buggy caller because
it
still allows the underlying bug to be found.



No, it doesn't. The pointer is replaced and forever lost. How is
leaking memory helping anyone to find the bug?


Valgrind can tell you where leaked memory was allocated, but not if you free
it in the wrong place.


It is the correct place to free it. And nobody will ever find it with
valgrind, just like nobody has ever tracked down the hundreds of leaks
in Git that happen reliably 100% of the time, much less the ones that
happen rarely.


We could argue about freeing it here or adding a discard_index call 
somewhere else (which seems more natural to me) once we had a call 
sequence that actually causes such a leak.  The test suite contains 
none, so I'd say we need more tests first.


Lots of the existing leaks were not worth plugging because the process 
would end right after freeing the memory.  Leaving clean-up to the OS 
was a conscious design choice, simplifying the code.


When such code is reused in a library or run multiple times while it was 
originally meant to be run only a single time (like with cherry-pick 
--stdin) the original assumption doesn't hold any more and we have a 
problem.


Let's find and fix those leaks by freeing memory in the right places. 
Freeing memory just in case in places where we can show that no leak is 
triggered by our test suite doesn't help.


René

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/22] git-remote-mediawiki: Use the Readonly module instead of the constant pragma

On Sat, Jun 08, 2013 at 03:01:03PM +0200, Célestin Perdu wrote:

 Oh yes, part of this commit went into the previous one, which was not
 formated as an email when I did my git-format-patch. I should check my
 patches more carefully before sending them. Sorry for this.

No problem. It is easy to make simple mistakes like that with our
workflow, but it is also easy to fix them and repost. :)

  What advantage does this have over use constant? I do not mind
  following guidelines from perlcritic if they are a matter of style, but
  in this case there is a cost: we now depend on the Readonly module,
  which is not part of the standard distribution. I.e., users now have to
  deal with installing an extra dependency. Is it worth it?
 
 Like Benoit said, the problem is that they sometimes don't interpolate.
 I don't know if we should keep this commit or not.

Thanks both for the explanation.  I don't see us using that to our
advantage anywhere in the patch. So I think this is purely a style
issue, which to me indicates that the extra dependency is not worth it,
and the patch should be dropped.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] build: get rid of the notion of a git library

There's no libgit, and there will never be, every object file in Git is
the same, and there's wish to organize them in any way; they are *all*
for the 'git' binary and its builtin commands.

So let's shatter any hopes of ever having a library, and be clear about
it; both the top-level objects (./*.o) and the builtin objects
(./builtin/*.o) go into git.a, which is not a library, merely a
convenient way to stash objects together.

This way there will not be linking issues when top-level objects try to
access functions of builtin objects.

LIB_OBJS and LIB_H imply a library, but there isn't one, and never will
be; so give them proper names; just a bunch of headers and objects.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Makefile | 564 ---
 1 file changed, 283 insertions(+), 281 deletions(-)

diff --git a/Makefile b/Makefile
index 03524d0..63451b1 100644
--- a/Makefile
+++ b/Makefile
@@ -435,8 +435,8 @@ XDIFF_OBJS =
 VCSSVN_OBJS =
 GENERATED_H =
 EXTRA_CPPFLAGS =
-LIB_H =
-LIB_OBJS =
+HEADERS =
+OBJS =
 PROGRAM_OBJS =
 PROGRAMS =
 SCRIPT_PERL =
@@ -629,270 +629,270 @@ endif
 export PERL_PATH
 export PYTHON_PATH
 
-LIB_FILE = libgit.a
+GIT_LIB = git.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
 
-LIB_H += advice.h
-LIB_H += archive.h
-LIB_H += argv-array.h
-LIB_H += attr.h
-LIB_H += bisect.h
-LIB_H += blob.h
-LIB_H += branch.h
-LIB_H += builtin.h
-LIB_H += bulk-checkin.h
-LIB_H += bundle.h
-LIB_H += cache-tree.h
-LIB_H += cache.h
-LIB_H += color.h
-LIB_H += column.h
-LIB_H += commit.h
-LIB_H += compat/bswap.h
-LIB_H += compat/cygwin.h
-LIB_H += compat/mingw.h
-LIB_H += compat/obstack.h
-LIB_H += compat/poll/poll.h
-LIB_H += compat/precompose_utf8.h
-LIB_H += compat/terminal.h
-LIB_H += compat/win32/dirent.h
-LIB_H += compat/win32/pthread.h
-LIB_H += compat/win32/syslog.h
-LIB_H += connected.h
-LIB_H += convert.h
-LIB_H += credential.h
-LIB_H += csum-file.h
-LIB_H += decorate.h
-LIB_H += delta.h
-LIB_H += diff.h
-LIB_H += diffcore.h
-LIB_H += dir.h
-LIB_H += exec_cmd.h
-LIB_H += fetch-pack.h
-LIB_H += fmt-merge-msg.h
-LIB_H += fsck.h
-LIB_H += gettext.h
-LIB_H += git-compat-util.h
-LIB_H += gpg-interface.h
-LIB_H += graph.h
-LIB_H += grep.h
-LIB_H += hash.h
-LIB_H += help.h
-LIB_H += http.h
-LIB_H += kwset.h
-LIB_H += levenshtein.h
-LIB_H += line-log.h
-LIB_H += line-range.h
-LIB_H += list-objects.h
-LIB_H += ll-merge.h
-LIB_H += log-tree.h
-LIB_H += mailmap.h
-LIB_H += merge-blobs.h
-LIB_H += merge-recursive.h
-LIB_H += mergesort.h
-LIB_H += notes-cache.h
-LIB_H += notes-merge.h
-LIB_H += notes.h
-LIB_H += object.h
-LIB_H += pack-revindex.h
-LIB_H += pack.h
-LIB_H += parse-options.h
-LIB_H += patch-ids.h
-LIB_H += pathspec.h
-LIB_H += pkt-line.h
-LIB_H += progress.h
-LIB_H += prompt.h
-LIB_H += quote.h
-LIB_H += reachable.h
-LIB_H += reflog-walk.h
-LIB_H += refs.h
-LIB_H += remote.h
-LIB_H += rerere.h
-LIB_H += resolve-undo.h
-LIB_H += revision.h
-LIB_H += run-command.h
-LIB_H += send-pack.h
-LIB_H += sequencer.h
-LIB_H += sha1-array.h
-LIB_H += sha1-lookup.h
-LIB_H += shortlog.h
-LIB_H += sideband.h
-LIB_H += sigchain.h
-LIB_H += strbuf.h
-LIB_H += streaming.h
-LIB_H += string-list.h
-LIB_H += submodule.h
-LIB_H += tag.h
-LIB_H += tar.h
-LIB_H += thread-utils.h
-LIB_H += transport.h
-LIB_H += tree-walk.h
-LIB_H += tree.h
-LIB_H += unpack-trees.h
-LIB_H += url.h
-LIB_H += userdiff.h
-LIB_H += utf8.h
-LIB_H += varint.h
-LIB_H += vcs-svn/fast_export.h
-LIB_H += vcs-svn/line_buffer.h
-LIB_H += vcs-svn/repo_tree.h
-LIB_H += vcs-svn/sliding_window.h
-LIB_H += vcs-svn/svndiff.h
-LIB_H += vcs-svn/svndump.h
-LIB_H += walker.h
-LIB_H += wildmatch.h
-LIB_H += wt-status.h
-LIB_H += xdiff-interface.h
-LIB_H += xdiff/xdiff.h
-LIB_H += xdiff/xdiffi.h
-LIB_H += xdiff/xemit.h
-LIB_H += xdiff/xinclude.h
-LIB_H += xdiff/xmacros.h
-LIB_H += xdiff/xprepare.h
-LIB_H += xdiff/xtypes.h
-LIB_H += xdiff/xutils.h
-
-LIB_OBJS += abspath.o
-LIB_OBJS += advice.o
-LIB_OBJS += alias.o
-LIB_OBJS += alloc.o
-LIB_OBJS += archive.o
-LIB_OBJS += archive-tar.o
-LIB_OBJS += archive-zip.o
-LIB_OBJS += argv-array.o
-LIB_OBJS += attr.o
-LIB_OBJS += base85.o
-LIB_OBJS += bisect.o
-LIB_OBJS += blob.o
-LIB_OBJS += branch.o
-LIB_OBJS += bulk-checkin.o
-LIB_OBJS += bundle.o
-LIB_OBJS += cache-tree.o
-LIB_OBJS += color.o
-LIB_OBJS += column.o
-LIB_OBJS += combine-diff.o
-LIB_OBJS += commit.o
-LIB_OBJS += compat/obstack.o
-LIB_OBJS += compat/terminal.o
-LIB_OBJS += config.o
-LIB_OBJS += connect.o
-LIB_OBJS += connected.o
-LIB_OBJS += convert.o
-LIB_OBJS += copy.o
-LIB_OBJS += credential.o
-LIB_OBJS += csum-file.o
-LIB_OBJS += ctype.o
-LIB_OBJS += date.o
-LIB_OBJS += decorate.o
-LIB_OBJS += diffcore-break.o
-LIB_OBJS += diffcore-delta.o
-LIB_OBJS += diffcore-order.o
-LIB_OBJS += diffcore-pickaxe.o
-LIB_OBJS += diffcore-rename.o
-LIB_OBJS += diff-delta.o
-LIB_OBJS += diff-lib.o
-LIB_OBJS += diff-no-index.o
-LIB_OBJS += diff.o
-LIB_OBJS +=

Re: [PATCH 2/2] Move sequencer to builtin

Felipe Contreras wrote:

 This has nothing to do with better strategy, it has everything to do
 with gut feelings and tradition. Not reasons.

I try to help you, and you insult me.  I don't think this is worth it.

If I were managing this list, I would ban mails from you, since this
discussion style does more harm than good.  If I were maintaining git,
I'd still accept your contributions, waiting until times when I had
more patience to read them and sending them to the list when
appropriate to get more feedback.  Of course I am neither managing the
list nor maintaining git, but I thought I should put that out there...

Annoyed,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 12:34 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Felipe Contreras wrote:

 This has nothing to do with better strategy, it has everything to do
 with gut feelings and tradition. Not reasons.

 I try to help you, and you insult me.  I don't think this is worth it.

I didn't direct that comment to you; you took the pellet and threw it
at yourself.

Moreover, following gut feelings and traditions without reason is not
an insult, that's what human beings do.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] build: get rid of the notion of a git library

Felipe Contreras wrote:
 There's no libgit, and there will never be, every object file in Git is
 the same, and there's wish to organize them in any way; they are *all*
 for the 'git' binary and its builtin commands.

Nice joke patch to illustrate your point ;)

On a more serious note, please be a little more patient while everyone
copes with what you're attempting.  I've already made it clear that
I'm in favor of moving forward with your plan to lib'ify git.  The
problem is that you're sending your changes in fragmented comments and
diffs, and nobody is able to piece together what the big picture is.

Please write one cogent email (preferably with code included)
explaining your plan.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] build: get rid of the notion of a git library

On Sat, Jun 8, 2013 at 1:02 PM, Ramkumar Ramachandra artag...@gmail.com wrote:
 Felipe Contreras wrote:
 There's no libgit, and there will never be, every object file in Git is
 the same, and there's wish to organize them in any way; they are *all*
 for the 'git' binary and its builtin commands.

 Nice joke patch to illustrate your point ;)

It's not a joke. This is seriously the direction the others say is the
correct one.

One direction or the other, the problem that top-level objects can't
access code from builtin objects must be fixed. And if the others
don't want to fix it by properly splitting code between library-like
objects, and builtin objects, there's only one other way to fix it;
this way.

 On a more serious note, please be a little more patient while everyone
 copes with what you're attempting.

I don't think patience will help. What do you suggest? Wait until the
problem fixes itself? (I'll be waiting until the end times). Wait
until somebody changed their opinion by themselves? (I don't see that
happening).

 I've already made it clear that
 I'm in favor of moving forward with your plan to lib'ify git.

Unfortunately you are the only one.

 The
 problem is that you're sending your changes in fragmented comments and
 diffs, and nobody is able to piece together what the big picture is.

 Please write one cogent email (preferably with code included)
 explaining your plan.

The plan is simple; make libgit.a a proper library, starting by
clarifying what goes into libgit.a, and what doesn't. If there's any
hopes of ever having a public library, it's clear what code doesn't
belong in libgit.a; code that is meant for builtins, that code belongs
in builtins/lib.a, or similar.

But to be honest, I don't really care, all I want is the problem of
the bogus split to be solved. One way to solve it is going the proper
library way, but the other is to stash everything together into git.a.
Both ways solve the problem.

Give this a try:

--- a/sequencer.c
+++ b/sequencer.c
@@ -14,6 +14,7 @@
 #include merge-recursive.h
 #include refs.h
 #include argv-array.h
+#include builtin.h

 #define GIT_REFLOG_ACTION GIT_REFLOG_ACTION

@@ -102,6 +103,17 @@ static int has_conforming_footer(struct strbuf
*sb, struct strbuf *sob,
return 1;
 }

+static void copy_notes(const char *name)
+{
+   struct notes_rewrite_cfg *cfg;
+
+   cfg = init_copy_notes_for_rewrite(name);
+   if (!cfg)
+   return;
+
+   finish_copy_notes_for_rewrite(cfg);
+}
+
 static void remove_sequencer_state(void)
 {
struct strbuf seq_dir = STRBUF_INIT;
@@ -997,6 +1009,8 @@ static int pick_commits(struct commit_list
*todo_list, struct replay_opts *opts)
return res;
}

+   copy_notes(cherry-pick);
+
/*
 * Sequence of picks finished successfully; cleanup by
 * removing the .git/sequencer directory

What happens?

libgit.a(sequencer.o): In function `copy_notes':
/home/felipec/dev/git/sequencer.c:110: undefined reference to
`init_copy_notes_for_rewrite'
/home/felipec/dev/git/sequencer.c:114: undefined reference to
`finish_copy_notes_for_rewrite'

It is not the first time, nor the last that top-level code needs
builtin code, and the solution is easy; organize the code. Alas, this
simple solution reject on the basis that we shouldn't organize the
code, because the code is not meant to be organized.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 02/22] git-remote-mediawiki: Use the Readonly module instead of the constant pragma

Jeff King p...@peff.net writes:

 Thanks both for the explanation.  I don't see us using that to our
 advantage anywhere in the patch. So I think this is purely a style
 issue, which to me indicates that the extra dependency is not worth it,
 and the patch should be dropped.

Same here: I'd rather keep the dependency list small.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

Célestin Matte celestin.ma...@ensimag.fr writes:

 In Perl, '\n' is not a newline, but instead a literal backslash followed by an
 n. As the output of rev-list --first-parent is line-oriented, what we want
 here is a newline.

This is right, but the code actually worked the way it was. I'm not
sure, but my understanding is that '\n' is the string backslash
followed by n, but interpreted as a regexp, it is a newline.

The new code looks better than the old one, but the log message may be
improved.

In any case, Acked-by: Matthieu Moy matthieu@grenoble-inp.fr

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jun 2013, #03; Thu, 6)

Ramkumar Ramachandra artag...@gmail.com writes:

 Yes, please merge.  The commit message looks good as well: it doesn't
 say anything about waiting for 2.0.

The commit message doesn't, but the doc does. I'll resend without the
2.0 mention.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/22] git-remote-mediawiki: Check return value of open + remove import of unused open2

Célestin Matte celestin.ma...@ensimag.fr writes:

 Le 08/06/2013 02:14, Eric Sunshine a écrit :
 These two changes are unrelated and could be split into distinct
 patches (IMHO, though others may disagree).

 Two distinct patches or two distinct commits?

That's the same. You write commits locally, send them as patches, and
Junio uses git am to turn the patches back into commits.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/22] git-remote-mediawiki: Check return value of open + remove import of unused open2

Le 08/06/2013 20:41, Matthieu Moy a écrit :
 Célestin Matte celestin.ma...@ensimag.fr writes:
 
 Le 08/06/2013 02:14, Eric Sunshine a écrit :
 These two changes are unrelated and could be split into distinct
 patches (IMHO, though others may disagree).

 Two distinct patches or two distinct commits?
 
 That's the same. You write commits locally, send them as patches, and
 Junio uses git am to turn the patches back into commits.
 

Oh, I thought a part of a patch was called a commit. So the question was
rather: Should it be sent to this list independently from this series
of patches?.

-- 
Célestin Matte
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] git-remote-mediawiki: new tool to preview local changes without pushing

benoit.per...@ensimag.fr writes:

 From: Benoit Person benoit.per...@ensimag.fr

 The #7 issue on git-mediawiki's issue tracker [1] states that the ability to
 preview content without pushing would be a nice thing to have.

 This commit is a first attempt to achieve it. It adds a new git command,
 named `git mw`. This command accepts the subcommands `help` and `preview`
 for now.

Review could be easier if you have PATCH 1/2 introducing the command
with only help (essentially to check that the build system works), and
then focus on preview.

I won't insist in splitting this particular commit, just take it as an
advice for future work.

 --- a/contrib/mw-to-git/Makefile
 +++ b/contrib/mw-to-git/Makefile
 @@ -5,13 +5,17 @@
  ## Build git-remote-mediawiki
  
  SCRIPT_PERL=git-remote-mediawiki.perl
 +SCRIPT_PERL_MW=git-mw.perl

Why do you need another variable? Just adding git-mw.perl to SCRIPT_PERL
should do it, no? (Well, probably the make SCRIPT_PERL=$(SCRIPT_PERL)
lacks quotes around the argument, but then you should fix that).

 +# Constants
 +# Mediawiki filenames can contain forward slashes. This variable decides by 
 which pattern they should be replaced
 +Readonly::scalar $SLASH_REPLACEMENT = %2F;

This one is copied from git-remote-mediawiki.perl, and is a
git-mediawiki specific thing, so it would not make sense to put it into
Git.pm. This starts convincing me that we should have a GitMediawiki.pm
or so, and both scripts should use it.

Another option would be to put everything in git-remote-mediawiki.perl,
and probably to have git mw as a wrapper around it (to avoid having to
type git remote-mediawiki which is a bit long).

But the script starts being a bit long, so I think it makes more sense
to split it. So I'd say

PATCH 1/3 : introduce git mw
PATCH 2/3 : move sharable code to a new module (and make sure it's
installed properly by make install)
PATCH 3/3 : actually implement the preview feature

Perhaps others will have other/better advices.

 + # Auto-loading in browser
 + if ($autoload) {
 + open(my $browser, -|:encoding(UTF-8), xdg-open 
 .$preview_file_name);

That could be read from Git's configuration, and default to xdg-open.
But you don't want to hardcode it in the middle of the code.

Also, why use open if you don't want to communicate with the process?
system would be sufficient, and won't need close.

Prefer passing an array of arguments, instead of concatenating strings
and then rely on command-line parsing to re-split it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/22] git-remote-mediawiki: Check return value of open + remove import of unused open2

Célestin Matte celestin.ma...@ensimag.fr writes:

 Oh, I thought a part of a patch was called a commit.

1 patch = 1 commit
1 patch series = several commits sent together. Will normally end-up in
 a branch in Junio's repo (named with initials/topic)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 12:34 PM, Jonathan Nieder jrnie...@gmail.com wrote:

 If I were managing this list, I would ban mails from you, since this
 discussion style does more harm than good.

There is a nice motto around: Talk is cheap. Show me the code.

Just the past three months I've probably done more work than anybody
else[1], and you would ban me because you don't like my words? At the
end of the day the project has benefited from my patches, and a wise
maintainer would do what is best for the project. If you don't like my
words, ignore them.

Taking things personal is more often than not the wrong thing to do.
Specially when they were not even directed to you.

[1] https://www.ohloh.net/p/git/contributors?query=sort=commits_12_mo

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

Le 08/06/2013 20:38, Matthieu Moy a écrit : This is right, but the code
actually worked the way it was. I'm not
 sure, but my understanding is that '\n' is the string backslash
 followed by n, but interpreted as a regexp, it is a newline.

 The new code looks better than the old one, but the log message may be
 improved.

Is this better?


In Perl, '\n' is not a newline, but instead the string composed of a
backslash followed by an n. To match newlines, one has to use the /\n/
regexp. As the output of rev-list --first-parent is line-oriented,
what we want here is to match newlines, and not the \n string.


-- 
Célestin Matte
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 16/22] git-remote-mediawiki: Modify strings for a better coding-style

Le 08/06/2013 02:39, Eric Sunshine a écrit :
 On Fri, Jun 7, 2013 at 5:42 PM, Célestin Matte
 celestin.ma...@ensimag.fr wrote:
 - strings which don't need interpolation are single-quoted for more clarity 
 and
 slight gain of performance
 - interpolation is preferred over concatenation in many cases, for more 
 clarity
 - variables are always used with the ${} operator inside strings
 - strings including double-quotes are written with qq() so that the quotes do
 not have to be escaped
 
 Distinct changes could (IMHO) be split into separate patches for easier 
 review.

This commit is a real pain to cut into 3 distinctive ones. Is this
really necessary?
I will do it if it is, of course.


-- 
Célestin Matte
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] diff: add --ignore-blank-lines option

2013-06-08 Thread Antoine Pelisse

The goal of the patch is to introduce the GNU diff
-B/--ignore-blank-lines as closely as possible. The short option is not
available because it's already used for break-rewrites.

When this option is used, git-diff will not create hunks that simply
adds or removes empty lines, but will still show empty lines
addition/suppression if they are close enough to valuable changes.

There are two differences between this option and GNU diff -B option:
- GNU diff doesn't have --inter-hunk-context, so this must be handled
- The following sequence looks like a bug (context is displayed twice):

$ seq 5 file1
$ cat EOF file2
change
1
2

3
4
5
change
EOF
$ diff -u -B file1 file2
--- file1   2013-06-08 22:13:04.471517834 +0200
+++ file2   2013-06-08 22:13:23.275517855 +0200
@@ -1,5 +1,7 @@
+change
 1
 2
+
 3
 4
 5
@@ -3,3 +5,4 @@
 3
 4
 5
+change

So here is a more thorough description of the option:
- real changes are interesting
- blank lines that are close enough (less than context size) to
interesting changes are considered interesting (recursive definition)
- context lines are used around each hunk of interesting changes
- If two hunks are separated by less than inter-hunk-context, they
will be merged into one.

The current implementation does the interesting changes selection in a
single pass.

Signed-off-by: Antoine Pelisse apeli...@gmail.com
---
Hi,

On Tue, Jun 4, 2013 at 10:46 PM, Junio C Hamano gits...@pobox.com wrote:
 OK. Thanks.

 I think the logic would be more like:

  1. Start from xscr, find the first xchp that is !xchp-ignore;
 if there is none, we are done. There is no more to show.

  2. Remember the xchp as the beginning.

  3. Tangle -next pointer to find the next xch that is !xch-ignore;
 if there is none, we are also done.  xdchanges between the
 beginning you remembered in the step 2. and your current xchp
 are the only things we want to show.

  4. Measure the distance between the end of xchp and the beginning
 of xch.

 - If it is larger than max_common, xdchanges between the
   beginning you remembered in the step 2. and your current xchp
   are the only things we want to show.  The next iteration will
   start by skipping the blank-only changes between xchp and xch.

 - If it is short enough, assign xchp = xch and go back to 3. to
   find more interesting hunks (that is why we remembered the
   real beginning in step 2.).

Actually it doesn't quite work like that because we don't totally ignore
blank lines. We want to keep them if they are close enough to other
changes.

I've tried to improve the number of tests as it helped me during
implementation, and to give a better description of the feature.

The initial reroll was meant to simplify xdl_get_hunk() but I'm afraid
it became kind of voodoo code.  I'm not sure if I should provide some
more comments about it and where.

Please let me know if something is not working as expected.

Cheers, Antoine

 Documentation/diff-options.txt |3 +
 diff.c |2 +
 t/t4015-diff-whitespace.sh |  282 
 xdiff/xdiff.h  |2 +
 xdiff/xdiffi.c |   29 -
 xdiff/xdiffi.h |1 +
 xdiff/xemit.c  |   47 ++-
 xdiff/xemit.h  |2 +-
 xdiff/xutils.c |   13 ++
 xdiff/xutils.h |1 +
 10 files changed, 374 insertions(+), 8 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index b8a9b86..4e042d9 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -439,6 +439,9 @@ endif::git-format-patch[]
differences even if one line has whitespace where the other
line has none.

+--ignore-blank-lines::
+   Ignore changes whose lines are all blank.
+
 --inter-hunk-context=lines::
Show the context between diff hunks, up to the specified number
of lines, thereby fusing hunks that are close to each other.
diff --git a/diff.c b/diff.c
index f0b3e7c..208094f 100644
--- a/diff.c
+++ b/diff.c
@@ -3593,6 +3593,8 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
DIFF_XDL_SET(options, IGNORE_WHITESPACE_CHANGE);
else if (!strcmp(arg, --ignore-space-at-eol))
DIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);
+   else if (!strcmp(arg, --ignore-blank-lines))
+   DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
else if (!strcmp(arg, --patience))
options-xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
else if (!strcmp(arg, --histogram))
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index cc3db13..fc77713 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -142,6 +142,288 @@ EOF
 git diff

[PATCH] build: trivial cleanup

There's no need to list again the prerequisites.

Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 03524d0..fda98d6 100644
--- a/Makefile
+++ b/Makefile
@@ -2067,13 +2067,13 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o 
http-walker.o GIT-LDFLAGS $(GITLIBS
$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
 $(LIB_FILE): $(LIB_OBJS)
-   $(QUIET_AR)$(RM) $@  $(AR) rcs $@ $(LIB_OBJS)
+   $(QUIET_AR)$(RM) $@  $(AR) rcs $@ $^
 
 $(XDIFF_LIB): $(XDIFF_OBJS)
-   $(QUIET_AR)$(RM) $@  $(AR) rcs $@ $(XDIFF_OBJS)
+   $(QUIET_AR)$(RM) $@  $(AR) rcs $@ $^
 
 $(VCSSVN_LIB): $(VCSSVN_OBJS)
-   $(QUIET_AR)$(RM) $@  $(AR) rcs $@ $(VCSSVN_OBJS)
+   $(QUIET_AR)$(RM) $@  $(AR) rcs $@ $^
 
 export DEFAULT_EDITOR DEFAULT_PAGER
 
-- 
1.8.3.698.g079b096

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 08, 2013 at 12:40:19PM -0500, Felipe Contreras wrote:

  These are problems that can be solved. But there is a lot of work
  involved in finding these subtle bugs and coming up with fixes. I think
  you would be better off working on an implementation of git that was
  designed from scratch to work in-process, like libgit2.
 
 So you are in favor of never ever having an official Git library. Got it.

No, I didn't say that at all.

I do think that it would be more work to try to slowly massage the git
code into a library-ready form than it would be to simply start with
more library-friendly code and pull in bits of git.git as appropriate.

That is what the libgit2 project is doing.  Perhaps one day that project
will reach a point where we start building git.git commands off of it or
sometihng like it (for that matter, there is no reason you could not
build external commands off of libgit2 right now).  Would it be the
official Git library then? I don't know. It is not clear to me what
that even means.

In the meantime, I think it cannot be a bad thing for libgit2 to proceed
along its path, and I don't see a good reason for people not to use it.

But hey, you don't need to listen to me. If you think it would be easier
to make the git.git code into a library, go ahead and work on it. But I
think you will find that there are a large number of hard-to-find bugs
caused by implicit assumptions about global state, how file descriptors
are used, and so forth.

 There's a reason why the Git project doesn't use libgit2, and for the
 same reason the official Ruby scripts should not use it.

What reason is that?

 As history indicates, the Git project will never have any pressure to
 fix it's re-entrancy and re-run issues, so these issues will remain
 there forever.
 
 Only if you allow code that exposes those issues will there ever be
 any pressure to fix them.

I think it is a matter of critical mass. If you were to start linking
against libgit.a and 90% of it worked, you might have a reason to fix
the other 10%. But I suspect it is more the other way around.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t0005: skip signal death exit code test on Windows

On Fri, Jun 07, 2013 at 12:12:52PM +0200, Erik Faye-Lund wrote:

  Yeah, if it were mingw_raise responsible for this, I would suggest using
  the POSIX shell 128+sig instead. We could potentially check for
  SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if
  that would create headaches or confusion for other msys programs,
  though. I'd leave that up to the msysgit people to decide whether it is
  worth the trouble.
 
 
 ...and here's the code to do just that:
 [...]
 @@ -1715,6 +1720,13 @@ int mingw_raise(int sig)
   sigint_fn(SIGINT);
   return 0;
 
 + case SIGTERM:
 + if (sigterm_fn == SIG_DFL)
 + exit(128 + SIGTERM);
 + else if (sigterm_fn != SIG_IGN)
 + sigterm_fn(SIGTERM);
 + return 0;

I'm a little negative on handling just SIGTERM. That would make the test
pass, but does it really address the overall issue? To me, the
usefulness is having exit values with consistent meanings. Imagine I run
a very large git hosting site, and I want to log exceptional conditions
(e.g., a git sub-process crashes). What exit code do I get from a
SIGSEGV or SIGBUS (or GPF, or whatever Windows calls these)?

On Unix systems, this is pretty easy. To be honest, I do not care that
much about Windows systems because I would not host a large site on it.
:) But IMHO, the point of such a scheme is to be consistent across all
signals.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 7:10 PM, Jeff King p...@peff.net wrote:
 On Sat, Jun 08, 2013 at 12:40:19PM -0500, Felipe Contreras wrote:

  These are problems that can be solved. But there is a lot of work
  involved in finding these subtle bugs and coming up with fixes. I think
  you would be better off working on an implementation of git that was
  designed from scratch to work in-process, like libgit2.

 So you are in favor of never ever having an official Git library. Got it.

 No, I didn't say that at all.

Then you truly think libgit2 will ever reach the point where it can
replace libgit.a?

It won't. But decreeing that both projects should remain isolated, and
that libgit.a should never be a library, you are effectively
condemning the effort to fail, knowingly or not.

How many years has libgit2 been brewing? Do you think it's closer for
merging so it can be used by Git's core? No, it doesn't, and it will
not in the future, because it was never meant for that.

 I do think that it would be more work to try to slowly massage the git
 code into a library-ready form than it would be to simply start with
 more library-friendly code and pull in bits of git.git as appropriate.

It might be more effort, but the results are guaranteed by our
extensive testing infrastructure and huge user-base. Slowly but
steadily we'll get there.

Waiting for libgit2 to switch directions and reach some hypothetical
point is waiting for hell to freeze.

It won't happen. There's no incentive nor reason for it to happen.

 That is what the libgit2 project is doing.  Perhaps one day that project
 will reach a point where we start building git.git commands off of it or
 sometihng like it (for that matter, there is no reason you could not
 build external commands off of libgit2 right now).  Would it be the
 official Git library then? I don't know. It is not clear to me what
 that even means.

It means 'make install' installs a shared library with a clearly
defined and stable API that other projects can depend on, and it can
be used for all sort of purposes, including the git binary, and it's
builtins.

 In the meantime, I think it cannot be a bad thing for libgit2 to proceed
 along its path, and I don't see a good reason for people not to use it.

Its path will never end as an official Git library, not unless we do something.

 But hey, you don't need to listen to me. If you think it would be easier
 to make the git.git code into a library, go ahead and work on it. But I
 think you will find that there are a large number of hard-to-find bugs
 caused by implicit assumptions about global state, how file descriptors
 are used, and so forth.

That's impossible. Specially since moving irrelevant code out of
libgit.a is not permitted.

 There's a reason why the Git project doesn't use libgit2, and for the
 same reason the official Ruby scripts should not use it.

 What reason is that?

You tell me. Why isn't Git using libgit2?

 As history indicates, the Git project will never have any pressure to
 fix it's re-entrancy and re-run issues, so these issues will remain
 there forever.

 Only if you allow code that exposes those issues will there ever be
 any pressure to fix them.

 I think it is a matter of critical mass. If you were to start linking
 against libgit.a and 90% of it worked, you might have a reason to fix
 the other 10%. But I suspect it is more the other way around.

It doesn't matter if it's 90% or 10%, it's the only thing we have.

Unless you are in favor of including libgit2 and start using it for
Git's core *right now*, the only way forward is to improve libgit.a.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

Felipe Contreras wrote:

 Just the past three months I've probably done more work than anybody
 else[1], and you would ban me because you don't like my words?

Definitely, yes.  Even if you look at the impact on code alone and
don't care about the people, destroying a collegial work environment
is harmful enough to the code to outweigh the (admittedly often
useful) patches.

But I am not the mailing list owner, so what I would do is not too
important.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks


Am 08.06.2013 19:27, schrieb Felipe Contreras:

On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:


Let's find and fix those leaks by freeing memory in the right places.
Freeing memory just in case in places where we can show that no leak is
triggered by our test suite doesn't help.


It helps; it prevents leaks. The real culprit is the bogus API, but I
don't see that changing anytime soon, so there are two options when
somebody makes a mistake the API allows; leak or don't leak. And you
seem to prefer the leak, even though it provides absolutely no
advantage.


It covers up bugs, which may seem helpful, but isn't, because it's 
better to fix the actual mistake.


What would be a better API?  Making discard_index free the array is a 
good first step; what else is bogus?


René

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 8:40 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Felipe Contreras wrote:

 Just the past three months I've probably done more work than anybody
 else[1], and you would ban me because you don't like my words?

 Definitely, yes.  Even if you look at the impact on code alone and
 don't care about the people, destroying a collegial work environment
 is harmful enough to the code to outweigh the (admittedly often
 useful) patches.

A collegial work environment is overrated, and proof of that the Linux
kernel, where honest and straight talk is the bread and butter of the
mailing list. And the Linux kernel is the most successful software
project in history by far. It's code that speaks.

And I have not destroyed anything, except maybe your sense of fairness
and balance when reviewing my patches, but that is not my fault.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 08, 2013 at 08:17:08PM -0500, Felipe Contreras wrote:

  No, I didn't say that at all.
 
 Then you truly think libgit2 will ever reach the point where it can
 replace libgit.a?

I don't know. It may. Or something like it may. It is certainly not
ready to do so yet, but perhaps one day it will be.

 It won't.

Oh, I see, you were not actually interested in my answer and were just
being rhetorical.

 But decreeing that both projects should remain isolated, and
 that libgit.a should never be a library, you are effectively
 condemning the effort to fail, knowingly or not.

Huh? When did I decree anything? You asked Duy what kinds of problems
you would run into with running multiple git commands in the same
process space. I answered with concrete examples, and gave my opinions
on what the path of least work would be to reach a re-entrant library.
You don't have to agree (didn't I even say you don't have to listen to
me in the last email?).

 How many years has libgit2 been brewing? Do you think it's closer for
 merging so it can be used by Git's core? No, it doesn't, and it will
 not in the future, because it was never meant for that.

There has been about 2 years of active development, and there's been
quite a lot of improvement in that time. Closer than what? Than it was 2
years ago? Yes, I think it is. But it still has a ways to go.

I do not think there will be a flag day where we throw away git.git's
code and start using libgit2. But we could slowly start replacing
underlying bits with libgit2 bits, if that implementation proves to be
robust and clean enough to do so.

  But hey, you don't need to listen to me. If you think it would be easier
  to make the git.git code into a library, go ahead and work on it. But I
  think you will find that there are a large number of hard-to-find bugs
  caused by implicit assumptions about global state, how file descriptors
  are used, and so forth.
 
 That's impossible. Specially since moving irrelevant code out of
 libgit.a is not permitted.

I'm not even sure what your second sentence means.

But it seems to me that the first step would be cleaning up the internal
code so that it is more friendly to library callers (both in interface
and in being re-entrant), with those first sets of callers being the
existing code in git.git. Such cleanups would be a good thing for the
modularity of the code, even without an intended library step.

And then you can start to pull out individual interfaces that are known
to be safe for library use, and think about making a coherent library
out of them.

And please don't tell me about not permitted. You are free to fork and
work on this. But do not expect people who have already said that does
not seem like a fruitful path to me to jump into it with you. If you
think it is worth doing and that you can come up with initial results to
convince others, go for it.

  There's a reason why the Git project doesn't use libgit2, and for the
  same reason the official Ruby scripts should not use it.
 
  What reason is that?
 
 You tell me. Why isn't Git using libgit2?

Wait, you indicated you had such a reason in mind, but now you won't
tell me? Is it a secret?

  I think it is a matter of critical mass. If you were to start linking
  against libgit.a and 90% of it worked, you might have a reason to fix
  the other 10%. But I suspect it is more the other way around.
 
 It doesn't matter if it's 90% or 10%, it's the only thing we have.
 
 Unless you are in favor of including libgit2 and start using it for
 Git's core *right now*, the only way forward is to improve libgit.a.

That seems like a false choice to me. You obviously feel that libgit2 is
some kind of dead end. I don't agree. Whatever.

I have very little interest in discussing this further with you, as it
is not leading in a productive direction. In my opinion, the productive
things to do would be one (or both) of:

  1. Work on libgit2.

  2. Clean up non-reentrant bits of git.git, hopefully making the code
 more readable and more modular (and taking care not to make it
 worse in other ways, like maintainability or performance).

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/2] read-cache: plug a few leaks

On Sat, Jun 8, 2013 at 9:11 PM, René Scharfe
rene.scha...@lsrfire.ath.cx wrote:
 Am 08.06.2013 19:27, schrieb Felipe Contreras:

 On Sat, Jun 8, 2013 at 12:22 PM, René Scharfe
 rene.scha...@lsrfire.ath.cx wrote:

 Let's find and fix those leaks by freeing memory in the right places.
 Freeing memory just in case in places where we can show that no leak is
 triggered by our test suite doesn't help.


 It helps; it prevents leaks. The real culprit is the bogus API, but I
 don't see that changing anytime soon, so there are two options when
 somebody makes a mistake the API allows; leak or don't leak. And you
 seem to prefer the leak, even though it provides absolutely no
 advantage.

 It covers up bugs,

It doesn't. I thought you already silently agreed that nobody would
ever find that leak, as they haven't found the hundreds of leaks that
plague Git's code.

 What would be a better API?  Making discard_index free the array is a good
 first step; what else is bogus?

'initialized' for starters; it should be renamed to 'loaded' or
removed, but removing it would require many more changes to make sure
we don't load twice. Also, when loading cache entries, it might make
sense to check if there's already entries that have not been
previously discarded properly.

In the meantime, just in case, the only sane thing to do is free the
entries rather than leak.

That being said I'm not interested in this patch any more. The patch
is good yet after three tries and countless arguments it's still not
applied, nor is there any sign of getting there.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

On Sat, Jun 8, 2013 at 9:23 PM, Jeff King p...@peff.net wrote:
 On Sat, Jun 08, 2013 at 08:17:08PM -0500, Felipe Contreras wrote:

  No, I didn't say that at all.

 Then you truly think libgit2 will ever reach the point where it can
 replace libgit.a?

 I don't know. It may. Or something like it may. It is certainly not
 ready to do so yet, but perhaps one day it will be.

Perhaps one day we would end poverty and hunger, and perhaps one day
we will live in peace, but I wouldn't hold on my breath. I fact, I'll
do the opposite, I bet it won't happen anytime soon.

 It won't.

 Oh, I see, you were not actually interested in my answer and were just
 being rhetorical.

 But decreeing that both projects should remain isolated, and
 that libgit.a should never be a library, you are effectively
 condemning the effort to fail, knowingly or not.

 Huh? When did I decree anything?

When you said in your opinion we should wait until libgit2 is ready,
and not improve libgit.a.

 How many years has libgit2 been brewing? Do you think it's closer for
 merging so it can be used by Git's core? No, it doesn't, and it will
 not in the future, because it was never meant for that.

 There has been about 2 years of active development, and there's been
 quite a lot of improvement in that time. Closer than what? Than it was 2
 years ago? Yes, I think it is. But it still has a ways to go.

Why is it closer? In what ways is it a better fit now than 2 years
ago? What is missing before merging to be used in Git's core?

 I do not think there will be a flag day where we throw away git.git's
 code and start using libgit2. But we could slowly start replacing
 underlying bits with libgit2 bits, if that implementation proves to be
 robust and clean enough to do so.

And what are we waiting for then? Shouldn't we copy the whole libgit2
code and start migrating?

  But hey, you don't need to listen to me. If you think it would be easier
  to make the git.git code into a library, go ahead and work on it. But I
  think you will find that there are a large number of hard-to-find bugs
  caused by implicit assumptions about global state, how file descriptors
  are used, and so forth.

 That's impossible. Specially since moving irrelevant code out of
 libgit.a is not permitted.

 I'm not even sure what your second sentence means.

It means this:
http://article.gmane.org/gmane.comp.version-control.git/226752

I move code that doesn't belong in a libgit library out of libgit.a,
and the change gets rejected.

 But it seems to me that the first step would be cleaning up the internal
 code so that it is more friendly to library callers (both in interface
 and in being re-entrant),

That is the second step. It doesn't make sense to make code
re-entrant, if that code will only be used by builtin commands. First
step is to move irrelevant code out of libgit.a.

  There's a reason why the Git project doesn't use libgit2, and for the
  same reason the official Ruby scripts should not use it.
 
  What reason is that?

 You tell me. Why isn't Git using libgit2?

 Wait, you indicated you had such a reason in mind, but now you won't
 tell me? Is it a secret?

I did not. I made the assumption that there was a reason, if there's
no reason to stay clear of libgit2, then let's merge it already.

  I think it is a matter of critical mass. If you were to start linking
  against libgit.a and 90% of it worked, you might have a reason to fix
  the other 10%. But I suspect it is more the other way around.

 It doesn't matter if it's 90% or 10%, it's the only thing we have.

 Unless you are in favor of including libgit2 and start using it for
 Git's core *right now*, the only way forward is to improve libgit.a.

 That seems like a false choice to me. You obviously feel that libgit2 is
 some kind of dead end. I don't agree. Whatever.

I never said so. It is a dead end *if* we don't do an effort to have a
proper libgit library, which is the path we are taking.

 I have very little interest in discussing this further with you, as it
 is not leading in a productive direction. In my opinion, the productive
 things to do would be one (or both) of:

   1. Work on libgit2.

   2. Clean up non-reentrant bits of git.git, hopefully making the code
  more readable and more modular (and taking care not to make it
  worse in other ways, like maintainability or performance).

You forgot the first step of 2.: move irrelevant code out of libgit.a.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

On Sat, Jun 08, 2013 at 08:38:56PM +0200, Matthieu Moy wrote:

 Célestin Matte celestin.ma...@ensimag.fr writes:
 
  In Perl, '\n' is not a newline, but instead a literal backslash followed by 
  an
  n. As the output of rev-list --first-parent is line-oriented, what we 
  want
  here is a newline.
 
 This is right, but the code actually worked the way it was. I'm not
 sure, but my understanding is that '\n' is the string backslash
 followed by n, but interpreted as a regexp, it is a newline.

Yes, the relevant doc (from perldoc -f split) is:

  The pattern /PATTERN/ may be replaced with an expression to specify
  patterns that vary at runtime.  (To do runtime compilation only once,
  use /$variable/o.)

So it is treating \n as an expression and compiling the regex each
time through (though I think modern perl may be smart enough to realize
it is a constant expression and compile the regex only once). You would
get the same behavior with this:

  split $arg, $data;

if $arg contained '\n'. Of course, you _also_ get the same thing if you
use a literal newline (either \n or if $arg contained a literal
newline), because they function the same in a regex. In other words, it
does not matter which you use because perl's interpolation of \n and
the regex expansion of \n are identical: t hey both mean a newline.

A more subtle example that shows what is going on is this:

  split '.', $data;

If you feed that foo.bar.baz, it does not split it into three words;
each character is a delimiter, because the dot is compiled to a regex.

 The new code looks better than the old one, but the log message may be
 improved.

Agreed. I think the best explanation is something like:

  Perl's split function takes a regex pattern argument. You can also
  feed it an expression, which is then compiled into a regex at runtime.
  It therefore works to pass your pattern via single quotes, but it is
  much less obvious to a reader that the argument is meant to be a
  regex, not a static string. Using the traditional slash-delimiters
  makes this easier to read.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

2013-06-08 Thread Johannes Schindelin

Hi Ram,

On Sat, 8 Jun 2013, Ramkumar Ramachandra wrote:

 Felipe Contreras wrote:
  Also we heard from no regular/high-value reviewers that they feel
  comfortable reviewing additions in Ruby.
 
  Correction; *current* regular/high-value reviewers.
 
 Correct.  The opinions of inactive community members and
 non-contributors are less useful.

I humbly suggest to treat other people's contribution with the same
respect you want yours' to be treated.

Just a suggestion,
J
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

2013-06-08 Thread Johannes Schindelin

Hi Ram,

On Sat, 8 Jun 2013, Ramkumar Ramachandra wrote:

 Felipe Contreras wrote:
  While at it, why not re-evaluate the whole msysgit approach? I bet we
  don't need a whole separate project just to create a Windows
  installer. I've written Windows installers before, it's very easy to
  do from Linux.
 
 Yeah, taking the pain out of msysgit packaging would be a great way to
 counter this new-dependency-fud.  The main problem, as mm pointed out
 is subversion + perlxs.

Yeah, this is the main problem, and you probably will end up with a much
better (Linux-based) solution than the people who contributed to the Git
for Windows project in all those years since August 2007.

Surprise me, with code,
J
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Administrivia] On ruby and contrib/

2013-06-08 Thread Johannes Schindelin

Hi Duy,

On Sat, 8 Jun 2013, Duy Nguyen wrote:

 On Thu, Jun 6, 2013 at 11:22 PM, Johannes Schindelin
 johannes.schinde...@gmx.de wrote:
  Hi Greg,
 
  On Thu, 6 Jun 2013, Greg Troxel wrote:
 
  As one of the people who helps maintain git packages in pkgsrc, my
  initial reaction is negative to adding a ruby dependency.
 
  My initial reaction, too. It was hard enough to get Perl included with Git
  for Windows (because of that pesky Subversion dependency).
 
  As you can see from the commit history, I was the primary force behind
  trying to get everything core in Git away from requiring scripting
  languages (I think it is an awesome thing to provide APIs for as many
  languages as possible, but a not-so-cool thing to use more than one
  language in the core code). It does not seem that anybody picked up that
  task when I left, though.
 
 Nobody seems to mention it yet. There's another reason behind the C
 rewrite effort: fork is costly on Windows. The C rewrite allows us to
 run with one process (most of the time). This applies for shell, perl
 and even ruby scripts because libgit.a is never meant to be used
 outside git.c context (unlike libgit2). In this regard, ruby is just
 as bad as currently supported non-C languages.

I think you should have said on Windows when you said fork is costly.
Oh wait, you did.

It seems that at least some people participating in this discussion are
not overly keen on supporting the platform that -- according to
statistics, i.e. facts -- is the most prevalent.

I am glad that Junio still seems to be interested in giving us poor folks
trying to make the Git experience on Windows a less sucky one enough
credit, even if we only got a little over 900 commits into git.git. But
that does not count because the commits are older than one year. That
makes them useless to some people, apparently.

Hopefully Junio will have more mercy on us poor Windows folks than others
would,
Dscho
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

Felipe Contreras wrote:

 A collegial work environment is overrated, and proof of that the Linux
 kernel, where honest and straight talk is the bread and butter of the
 mailing list.

An aside, since it doesn't bear too much on the topic at hand:

For what it's worth, in my experience the people working on the kernel
are quite sensible and friendly on-list.  Probably you are referring
to some high-profile cases of flames, which perhaps I have just been
lucky to avoid.  I do not think the way the list works normally is a
counterexample to common decency being useful.

So no, I don't find But they are mean, and look how well they are
doing! to be a compelling argument here.

Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 8, 2013 at 10:21 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Felipe Contreras wrote:

 A collegial work environment is overrated, and proof of that the Linux
 kernel, where honest and straight talk is the bread and butter of the
 mailing list.

 An aside, since it doesn't bear too much on the topic at hand:

 For what it's worth, in my experience the people working on the kernel
 are quite sensible and friendly on-list.

They are professional. When they need to be straight-forward, they
are, even if that hurts the feelings of the colleagues.

 Probably you are referring to some high-profile cases of flames,

No, I'm not. Heated discussions happen all the time, specially when
the issue at hand is important.

 I do not think the way the list works normally is a
 counterexample to common decency being useful.

Of course you wouldn't, but you are purposely ignoring the facts.

The Linux kernel mailing lists concentrates on *the code*; he who
writes the code has a voice, he who only has words doesn't. Personal
beefs are not relevant. When there's something horribly wrong with the
code, so are the responses.

 So no, I don't find But they are mean, and look how well they are
 doing! to be a compelling argument here.

Because you dismiss the premise a priori.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin

On Sat, Jun 08, 2013 at 09:20:54AM -0500, Felipe Contreras wrote:

 Let the code speak. Show me a script in any language that does
 something useful using libgit2, doing the equivalent to at least a
 couple of 'git foo' commands.

Sorry that I cannot show you the source code, but you may interested to
know that libgit2 powers:

  1. Microsoft's Visual Studio Tools for Git plugin

  2. GitHub's native Mac and Windows clients (using Objective C and C#
 bindings); some operations still shell out to git where the
 functionality is not yet implemented in libgit2.

  3. Parts of the web view of GitHub.com via Ruby bindings

It is definitely not feature-complete when compared with git.git. But I
do think it is in a state that is usable for quite a few tasks.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 16/22] git-remote-mediawiki: Modify strings for a better coding-style

On Sat, Jun 8, 2013 at 4:32 PM, Célestin Matte
celestin.ma...@ensimag.fr wrote:
 Le 08/06/2013 02:39, Eric Sunshine a écrit :
 On Fri, Jun 7, 2013 at 5:42 PM, Célestin Matte
 celestin.ma...@ensimag.fr wrote:
 - strings which don't need interpolation are single-quoted for more clarity 
 and
 slight gain of performance
 - interpolation is preferred over concatenation in many cases, for more 
 clarity
 - variables are always used with the ${} operator inside strings
 - strings including double-quotes are written with qq() so that the quotes 
 do
 not have to be escaped

 Distinct changes could (IMHO) be split into separate patches for easier 
 review.

 This commit is a real pain to cut into 3 distinctive ones. Is this
 really necessary?
 I will do it if it is, of course.

[I think you meant that it would be split into 4 patches.]

The final decision is up to the submitter (you) and the people signing
off on and accepting the patch (Matthieu and Junio).

Speaking merely as a person reviewing the patch series, I can say that
mixing conceptually unrelated changes into a single patch makes review
more onerous since it requires repeatedly switching mental gears
(often from line to line or even within a single line). Patches
involving simple mechanical changes typically are easy to review,
even when the patches are lengthy. In this case, however, that length
coupled with the mental gear switching, makes the review process more
burdensome that it need be.

Even if you decide ultimately not to bother splitting the patch,
ease-of-review and one-conceptual-change-per-patch are useful notions
to keep in mind for future submissions.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/22] git-remote-mediawiki: Check return value of open + remove import of unused open2

On Sat, Jun 8, 2013 at 2:45 PM, Célestin Matte
celestin.ma...@ensimag.fr wrote:
 Le 08/06/2013 20:41, Matthieu Moy a écrit :
 Célestin Matte celestin.ma...@ensimag.fr writes:

 Le 08/06/2013 02:14, Eric Sunshine a écrit :
 These two changes are unrelated and could be split into distinct
 patches (IMHO, though others may disagree).

 Two distinct patches or two distinct commits?

 That's the same. You write commits locally, send them as patches, and
 Junio uses git am to turn the patches back into commits.


 Oh, I thought a part of a patch was called a commit. So the question was
 rather: Should it be sent to this list independently from this series
 of patches?.

Terms patch and commit tend to be used interchangeably, as Matthieu notes.

There is no need to send the split up patches to the list
independently from this series.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

On Sat, Jun 8, 2013 at 9:35 AM, Célestin Matte
celestin.ma...@ensimag.fr wrote:
 In Perl, '\n' is not a newline, but instead a literal backslash followed by an
 n. As the output of rev-list --first-parent is line-oriented, what we want
 here is a newline.

 Signed-off-by: Célestin Matte celestin.ma...@ensimag.fr
 Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr
 ---
  contrib/mw-to-git/git-remote-mediawiki.perl |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/contrib/mw-to-git/git-remote-mediawiki.perl 
 b/contrib/mw-to-git/git-remote-mediawiki.perl
 index 7af202f..a06bc31 100755
 --- a/contrib/mw-to-git/git-remote-mediawiki.perl
 +++ b/contrib/mw-to-git/git-remote-mediawiki.perl
 @@ -1190,7 +1190,7 @@ sub mw_push_revision {
 # history (linearized with --first-parent)
 print STDERR Warning: no common ancestor, pushing complete 
 history\n;
 my $history = run_git(rev-list --first-parent --children 
 $local);
 -   my @history = split('\n', $history);
 +   my @history = split(/\n/, $history);
 @history = @history[1..$#history];
 foreach my $line (reverse @history) {
 my @commit_info_split = split(/ |\n/, $line);

It would be quite acceptable to include this patch in your existing
patch series.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-remote-mediawiki: Fix a bug in a regexp

On Sat, Jun 8, 2013 at 10:57 PM, Jeff King p...@peff.net wrote:
 On Sat, Jun 08, 2013 at 08:38:56PM +0200, Matthieu Moy wrote:

 Célestin Matte celestin.ma...@ensimag.fr writes:

  In Perl, '\n' is not a newline, but instead a literal backslash followed 
  by an
  n. As the output of rev-list --first-parent is line-oriented, what we 
  want
  here is a newline.

 This is right, but the code actually worked the way it was. I'm not
 sure, but my understanding is that '\n' is the string backslash
 followed by n, but interpreted as a regexp, it is a newline.

 Yes, the relevant doc (from perldoc -f split) is:

   The pattern /PATTERN/ may be replaced with an expression to specify
   patterns that vary at runtime.  (To do runtime compilation only once,
   use /$variable/o.)

 So it is treating \n as an expression and compiling the regex each
 time through ...

I read this as saying only that static /PATTERN/ can also be a
composed /$PATTERN/. It does not indicate how string 'PATTERN' is
treated, nor does any other part of perldoc -f split make special
mention of string 'PATTERN'. In fact, the only explanation I found
regarding string 'PATTERN' is in my Camel book (3rd edition, page 796)
in a parenthesized comment:

(... if you supply a string instead of a regular expression, it'll be
interpreted as a regular expression anyway.)

 The new code looks better than the old one, but the log message may be
 improved.

 Agreed. I think the best explanation is something like:

   Perl's split function takes a regex pattern argument. You can also
   feed it an expression, which is then compiled into a regex at runtime.
   It therefore works to pass your pattern via single quotes, but it is
   much less obvious to a reader that the argument is meant to be a
   regex, not a static string. Using the traditional slash-delimiters
   makes this easier to read.

Sounds good to me.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] git.txt: document GIT_TRACE_PACKET

2013-06-08 Thread Nguyễn Thái Ngọc Duy

This can help with debugging object negotiation or other protocol
issues.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 Documentation/git.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index c760918..72e9045 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -845,6 +845,11 @@ for further details.
recorded. This may be helpful for troubleshooting some
pack-related performance problems.
 
+'GIT_TRACE_PACKET'::
+   If this variable is set, it shows a trace of all packets
+   coming in or out of a given program. This can help with
+   debugging object negotiation or other protocol issues.
+
 GIT_LITERAL_PATHSPECS::
Setting this variable to `1` will cause Git to treat all
pathspecs literally, rather than as glob patterns. For example,
-- 
1.8.2.83.gc99314b

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] core: use env variable instead of config var to turn on logging pack access

2013-06-08 Thread Nguyễn Thái Ngọc Duy

5f44324 (core: log offset pack data accesses happened - 2011-07-06)
provides a way to observe pack access patterns via a config
switch. Setting an environment variable looks more obvious than a
config var, especially when you just need to _observe_, and more
inline with other tracing knobs we have.

Document it as it may be useful for remote troubleshooting.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 Documentation/git.txt |  7 +++
 cache.h   |  3 ---
 config.c  |  3 ---
 environment.c |  1 -
 sha1_file.c   | 14 --
 5 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 68f1ee6..c760918 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -838,6 +838,13 @@ for further details.
as a file path and will try to write the trace messages
into it.
 
+'GIT_TRACE_PACK_ACCESS'::
+   If this variable is set to a path, a file will be created at
+   the given path logging all accesses to any packs. For each
+   access, the pack file name and an offset in the pack is
+   recorded. This may be helpful for troubleshooting some
+   pack-related performance problems.
+
 GIT_LITERAL_PATHSPECS::
Setting this variable to `1` will cause Git to treat all
pathspecs literally, rather than as glob patterns. For example,
diff --git a/cache.h b/cache.h
index df532f8..4f41606 100644
--- a/cache.h
+++ b/cache.h
@@ -772,9 +772,6 @@ extern int parse_sha1_header(const char *hdr, unsigned long 
*sizep);
 /* global flag to enable extra checks when accessing packed objects */
 extern int do_check_packed_object_crc;
 
-/* for development: log offset of pack access */
-extern const char *log_pack_access;
-
 extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned 
long size, const char *type);
 
 extern int move_temp_to_file(const char *tmpfile, const char *filename);
diff --git a/config.c b/config.c
index 7a85ebd..d04e815 100644
--- a/config.c
+++ b/config.c
@@ -688,9 +688,6 @@ static int git_default_core_config(const char *var, const 
char *value)
return 0;
}
 
-   if (!strcmp(var, core.logpackaccess))
-   return git_config_string(log_pack_access, var, value);
-
if (!strcmp(var, core.autocrlf)) {
if (value  !strcasecmp(value, input)) {
if (core_eol == EOL_CRLF)
diff --git a/environment.c b/environment.c
index e2e75c1..0cb67b2 100644
--- a/environment.c
+++ b/environment.c
@@ -37,7 +37,6 @@ size_t packed_git_window_size = 
DEFAULT_PACKED_GIT_WINDOW_SIZE;
 size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT;
 size_t delta_base_cache_limit = 16 * 1024 * 1024;
 unsigned long big_file_threshold = 512 * 1024 * 1024;
-const char *log_pack_access;
 const char *pager_program;
 int pager_use_color = 1;
 const char *editor_program;
diff --git a/sha1_file.c b/sha1_file.c
index b114cc9..4b23bb8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -36,6 +36,9 @@ static inline uintmax_t sz_fmt(size_t s) { return s; }
 
 const unsigned char null_sha1[20];
 
+static const char *no_log_pack_access = no_log_pack_access;
+static const char *log_pack_access;
+
 /*
  * This is meant to hold a *small* number of objects that you would
  * want read_sha1_file() to be able to return, but yet you do not want
@@ -1956,12 +1959,19 @@ static void write_pack_access_log(struct packed_git *p, 
off_t obj_offset)
 {
static FILE *log_file;
 
+   if (!log_pack_access)
+   log_pack_access = getenv(GIT_TRACE_PACK_ACCESS);
+   if (!log_pack_access)
+   log_pack_access = no_log_pack_access;
+   if (log_pack_access == no_log_pack_access)
+   return;
+
if (!log_file) {
log_file = fopen(log_pack_access, w);
if (!log_file) {
error(cannot open pack access log '%s' for writing: 
%s,
  log_pack_access, strerror(errno));
-   log_pack_access = NULL;
+   log_pack_access = no_log_pack_access;
return;
}
}
@@ -1992,7 +2002,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
int delta_stack_nr = 0, delta_stack_alloc = UNPACK_ENTRY_STACK_PREALLOC;
int base_from_cache = 0;
 
-   if (log_pack_access)
+   if (log_pack_access != no_log_pack_access)
write_pack_access_log(p, obj_offset);
 
/* PHASE 1: drill down to the innermost base object */
-- 
1.8.2.83.gc99314b

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Move sequencer to builtin