date:20140303


On 3/3/2014 4:08 PM, Junio C Hamano wrote:

Ilya Bobyr  writes:


It might be that we are looking at different use cases, as you are
talking about whole test suits.

I do not think so.


Good :)
I am trying to understand the use cases.  And make sure we are talking 
about the same ones.

I am not sure what are the use cases for GIT_SKIP_TESTS.

I think that while in it really nice when an interface allows to do new 
things, the main use case (or use cases) should be as easy and obvious 
in the first place.
If the target is the TDD use case I described, then it appears that a 
user needs to do double negation.  That is what concerns me.



I do not see anything prevents you from saying

GIT_SKIP_TESTS='t !t.1 !t.4'

to specify test-pieces in individual tests so that you can run the
setup step (step .1) and the specific test (step .4) without running
two tests in between.


While it could be done, it looks less obvious than this:

GIT_TEST_ONLY='1 4' ./t0001-init.sh

What if we do what you proposed, but with GIT_RUN_TESTS?
If we want to have one interface, maybe, building on top of a "negation" 
(GIT_SKIP_TESTS) is not very good.
At the same time, GIT_TEST_ONLY is also too specific for a generic 
interface.
I could add GIT_RUN_TESTS and allow it to have all of the features, thus 
making GIT_SKIP_TESTS a working but deprecated tool.


Running specific tests:
GIT_RUN_TESTS='t'
GIT_RUN_TESTS='t.1 t0002'
GIT_RUN_TESTS='1 3 7'

Negating some tests:
GIT_RUN_TESTS='!t'
GIT_RUN_TESTS='!t.1'
GIT_RUN_TESTS='!1 !3 !7'

The above would work exactly as you described but with one less level of 
negation.  Default is everything, unless at least one positive pattern 
is given.


Later on range specification could added:
GIT_RUN_TESTS='<11'

At least for now that would cover most use cases that I can think of 
that look reasonable.

--
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 v4 19/27] wrapper.c: wrapper to open a file, fprintf then close

On Sat, Mar 1, 2014 at 12:11 PM, Torsten Bögershausen  wrote:
> On 2014-03-01 13.12, Nguyễn Thái Ngọc Duy wrote:
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  cache.h   |  2 ++
>>  wrapper.c | 31 +++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index 98b5dd3..99b86d9 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1239,6 +1239,8 @@ static inline ssize_t write_str_in_full(int fd, const 
>> char *str)
>>  {
>>   return write_in_full(fd, str, strlen(str));
>>  }
>> +__attribute__((format (printf,3,4)))
>> +extern int write_file(const char *path, int fatal, const char *fmt, ...);
>>
>>  /* pager.c */
>>  extern void setup_pager(void);
>> diff --git a/wrapper.c b/wrapper.c
>> index 0cc5636..5ced50d 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -455,3 +455,34 @@ struct passwd *xgetpwuid_self(void)
>>   errno ? strerror(errno) : _("no such user"));
>>   return pw;
>>  }
>> +
>> +int write_file(const char *path, int fatal, const char *fmt, ...)
>> +{
>> + struct strbuf sb = STRBUF_INIT;
>> + int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
>> + va_list params;
>> + if (fd < 0) {

Micro nit atop Torsten's micro nit:

It is 3% easier to understand the code if the check for open() failure
immediately follows the open() attempt:

va_list params;
int fd = open(...);
if (fd < 0) {

>> + if (fatal)
>> + die_errno(_("could not open %s for writing"), path);
>> + return -1;
>> + }
>> + va_start(params, fmt);
>> + strbuf_vaddf(&sb, fmt, params);
>> + va_end(params);
>> + if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
>> + int err = errno;
>> + close(fd);
>> + errno = err;
>> + strbuf_release(&sb);
> Micro nit:
> Today we now what strbuf_release() is doing, but if we ever change the
> implementation, it is 3% safer to keep err a little bit longer like this:
>> + strbuf_release(&sb);
>> + errno = err;
--
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-bisect.sh: fix a few style issues

Jacopo Notarstefano  writes:

> Redirection operators should have a space before them, but not after them.
>
> Signed-off-by: Jacopo Notarstefano 
> ---

Looks obviously harmless ;-)

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 3/3] rebase: new convenient option to edit a single commit

2014-03-03 Thread Duy Nguyen

On Tue, Mar 4, 2014 at 3:28 AM, Eric Sunshine  wrote:
> On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy  
> wrote:
>> "git rebase -e XYZ" is basically the same as
>>
>> EDITOR="sed -i '1s/pick XYZ/edit XYZ/' $@" \
>> git rebase -i XYZ^
>>
>> In English, it prepares the todo list for you to edit only commit XYZ
>> to save your time. The time saving is only significant when you edit a
>> lot of commits separately.
>
> Is it correct to single out only "edit" for special treatment? If
> allowing "edit" on the command-line, then shouldn't command-line
> "reword" also be supported? I, for one, often need to reword a commit
> message (or two or three); far more frequently than I need to edit a
> commit.
>
> (This is a genuine question about perceived favoritism of "edit", as
> opposed to a request to further bloat the interface.)

Heh I had the same thought yesterday. The same thing could be asked
for "git commit --fixup" to send us back to the fixed up commit so we
can do something about it. If we go along that line, then "git commit"
may be a better interface to reword older commits..
-- 
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] skip_prefix: rewrite so that prefix is scanned once

2014-03-03 Thread Duy Nguyen

On Tue, Mar 04, 2014 at 01:09:39AM +0100, David Kastrup wrote:
> Duy Nguyen  writes:
> 
> > On Tue, Mar 4, 2014 at 5:43 AM, Junio C Hamano  wrote:
> >> diff --git a/git-compat-util.h b/git-compat-util.h
> >> index cbd86c3..68ffaef 100644
> >> --- a/git-compat-util.h
> >> +++ b/git-compat-util.h
> >> @@ -357,8 +357,14 @@ extern int suffixcmp(const char *str, const char 
> >> *suffix);
> >>
> >>  static inline const char *skip_prefix(const char *str, const char *prefix)
> >>  {
> >> -   size_t len = strlen(prefix);
> >> -   return strncmp(str, prefix, len) ? NULL : str + len;
> >
> > Just a note. gcc does optimize strlen("abcdef") to 6, and with that
> > information at compile time built-in strncmp might do better.
> 
> Indeed, most (but not all) of the calls have a constant string as
> prefix.  However, strncmp in each iteration checks for both *str as well
> as *prefix to be different from '\0' independently (and it appears
> unlikely to me that the optimizer will figure out that it's unnecessary
> for either) _and_ compares them for equality so it's not likely to be
> faster than the open-coded loop.
> 
> One could, however, use memcmp instead of strncmp.  I'm just not sure
> whether memcmp is guaranteed not to peek beyond the first mismatching
> byte even if the count would allow for more.  It could lead to undefined
> behavior if the first mismatching byte would be the ending NUL byte of
> str.

It turns out gcc does not generate a call to strncmp either. It
inlines repz cmpsb instead. I recall we had a discussion long ago
about the inefficiency of repz and and open-coded loop is preferred,
maybe that was about hashcmp. Anyway this does not matter much as
skip_prefix() is unlikely to be in any critical path, so I think
readability has higher priority.

For the curious, this small C program

-- 8< --
#include 
#include 

static inline const char *skip(const char *str, const char *prefix)
{
int len = strlen(prefix);
return strncmp(str, prefix, len) ? NULL : str + len;
}
int main(int ac, char **av)
{
const char *s = skip(av[1], "foo");
printf("%s\n", s);
return 0;
}
-- 8< --

produces this assembly with gcc -O2 (on x86, apparently)

-- 8< --
 :
   0:   55  push   %ebp
   1:   b9 03 00 00 00  mov$0x3,%ecx
   6:   89 e5   mov%esp,%ebp
   8:   57  push   %edi
   9:   bf 00 00 00 00  mov$0x0,%edi
   e:   56  push   %esi
   f:   53  push   %ebx
  10:   83 e4 f0and$0xfff0,%esp
  13:   83 ec 10sub$0x10,%esp
  16:   8b 45 0cmov0xc(%ebp),%eax
  19:   8b 40 04mov0x4(%eax),%eax
  1c:   89 c6   mov%eax,%esi
  1e:   f3 a6   repz cmpsb %es:(%edi),%ds:(%esi)
  20:   0f 97 c3seta   %bl
  23:   0f 92 c1setb   %cl
  26:   83 c0 03add$0x3,%eax
  29:   31 d2   xor%edx,%edx
  2b:   38 cb   cmp%cl,%bl
  2d:   0f 44 d0cmove  %eax,%edx
  30:   89 14 24mov%edx,(%esp)
  33:   e8 fc ff ff ff  call   34 
  38:   8d 65 f4lea-0xc(%ebp),%esp
  3b:   31 c0   xor%eax,%eax
  3d:   5b  pop%ebx
  3e:   5e  pop%esi
  3f:   5f  pop%edi
  40:   5d  pop%ebp
  41:   c3  ret
-- 8< --
--
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: [BUG] Halt during fetch on MacOS

2014-03-03 Thread Conley Owens

On Fri, Feb 28, 2014 at 10:15 PM, Jeff King  wrote:
> On Fri, Feb 28, 2014 at 03:26:28PM -0800, Conley Owens wrote:
>
>> test.sh
>> "
>> #!/bin/bash
>> rungit() {
>> mkdir $1
>> GIT_DIR=$1 git init --bare
>> echo '[remote "aosp"]' > $1/config
>> echo 'url =
>> https://android.googlesource.com/platform/external/tinyxml2' >>
>> $1/config
>> GIT_DIR=$1 git fetch aosp +refs/heads/master:refs/remotes/aosp/master
>
> I don't think this is affecting your test, but you probably want ">>" to
> append to the config for the first line, too. Otherwise you are
> overwriting some of git's default settings.
>
>> When everything cools, you can see that there are some fetches hanging
>> (typically).
>> $ ps | grep 'git fetch'
>> ...
>> 63310 ttys0040:00.01 git fetch aosp
>> +refs/heads/master:refs/remotes/aosp/master
>> [...]
>
> I can't reproduce here on Linux. Can you find out what the processes are
> doing with something like strace?

Yes, none of my Linux users have had any problems with this, but many
of my Mac users have.

I'm trying to run it under dtruss, but it's slowing the entire system
down to a halt.

>
>> You can look at the parent process of each and see that one half
>> spawned the other half, or you can look at the environment variables
>> for each to see that there are two processes operating in the same
>> directory for each directory where there's an issue.
>> $ echo "$(for pid in $(ps | grep 'git fetch' | grep -o '^[0-9]*'); do
>> ps -p $pid -wwwE | grep 'GIT_DIR=[^ ]*' -o; done)" | sort
>> GIT_DIR=testdir14
>> GIT_DIR=testdir14
>> GIT_DIR=testdir32
>> GIT_DIR=testdir32
>> GIT_DIR=testdir47
>> GIT_DIR=testdir47
>
> A fetch will start many sub-processes. Try:
>
>   GIT_TRACE=1 git fetch \
> https://android.googlesource.com/platform/external/tinyxml2
>
> which shows git-fetch starting the git-remote-https helper, which in
> turn starts git-fetch-pack to do the actual protocol, which uses
> git-unpack-objects to manage the incoming pack.

This is the full output of a fetch that ends up hanging:
trace: built-in: git 'fetch' 'aosp'
'+refs/heads/master:refs/remotes/aosp/master'
trace: run_command: 'git-remote-https' 'aosp'
'https://android.googlesource.com/platform/external/tinyxml2'

>
> -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] git-bisect.sh: fix a few style issues

2014-03-03 Thread Jacopo Notarstefano

Redirection operators should have a space before them, but not after them.

Signed-off-by: Jacopo Notarstefano 
---
 git-bisect.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-bisect.sh b/git-bisect.sh
index 73b4c14..af4d04c 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -365,7 +365,7 @@ bisect_reset() {
}
case "$#" in
0) branch=$(cat "$GIT_DIR/BISECT_START") ;;
-   1) git rev-parse --quiet --verify "$1^{commit}" > /dev/null || {
+   1) git rev-parse --quiet --verify "$1^{commit}" >/dev/null || {
invalid="$1"
die "$(eval_gettext "'\$invalid' is not a valid 
commit")"
}
@@ -458,13 +458,13 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
fi
 
# We have to use a subshell because "bisect_state" can exit.
-   ( bisect_state $state > "$GIT_DIR/BISECT_RUN" )
+   ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
res=$?
 
cat "$GIT_DIR/BISECT_RUN"
 
if sane_grep "first bad commit could be any of" 
"$GIT_DIR/BISECT_RUN" \
-   > /dev/null
+   >/dev/null
then
gettextln "bisect run cannot continue any more" >&2
exit $res
@@ -477,7 +477,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
exit $res
fi
 
-   if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" > 
/dev/null
+   if sane_grep "is the first bad commit" "$GIT_DIR/BISECT_RUN" 
>/dev/null
then
gettextln "bisect run success"
exit 0;
-- 
1.9.0.1.g641f09f

--
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] skip_prefix: rewrite so that prefix is scanned once

Duy Nguyen  writes:

> On Tue, Mar 4, 2014 at 5:43 AM, Junio C Hamano  wrote:
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index cbd86c3..68ffaef 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -357,8 +357,14 @@ extern int suffixcmp(const char *str, const char 
>> *suffix);
>>
>>  static inline const char *skip_prefix(const char *str, const char *prefix)
>>  {
>> -   size_t len = strlen(prefix);
>> -   return strncmp(str, prefix, len) ? NULL : str + len;
>
> Just a note. gcc does optimize strlen("abcdef") to 6, and with that
> information at compile time built-in strncmp might do better.

Indeed, most (but not all) of the calls have a constant string as
prefix.  However, strncmp in each iteration checks for both *str as well
as *prefix to be different from '\0' independently (and it appears
unlikely to me that the optimizer will figure out that it's unnecessary
for either) _and_ compares them for equality so it's not likely to be
faster than the open-coded loop.

One could, however, use memcmp instead of strncmp.  I'm just not sure
whether memcmp is guaranteed not to peek beyond the first mismatching
byte even if the count would allow for more.  It could lead to undefined
behavior if the first mismatching byte would be the ending NUL byte of
str.

-- 
David Kastrup
--
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] test-lib: GIT_TEST_ONLY to run only specific tests

Ilya Bobyr  writes:

> It might be that we are looking at different use cases, as you are
> talking about whole test suits.

I do not think so.

I do not see anything prevents you from saying

GIT_SKIP_TESTS='t !t.1 !t.4'

to specify test-pieces in individual tests so that you can run the
setup step (step .1) and the specific test (step .4) without running
two tests in between.

--
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] commit.c: Replace starts_with() with skip_prefix()

On Mon, Mar 3, 2014 at 10:23 AM, karthik nayak  wrote:
> Hello Eric,
> Thanks for Pointing out everything, i had a thorough look and fixed a
> couple of things.
> Here is an Updated Patch.
> - Removed unnecessary code and variables.
> - Replaced all instances of starts_with() with skip_prefix()

This commentary is important for the on-going email discussion but
does not belong in the commit message for the permanent project
history, so place it below the "---" line just under your sign-off.

Explaining what changed since the last attempt, as you do here, is a
good thing. To further ease the review process, supply a link to the
previous attempt, like this [1].

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

> Replace starts_with() with skip_prefix() for better reading purposes.
> Also to replace "buf + strlen(author )" by skip_prefix(), which is
> saved in a new "const char" variable "buf_skipprefix".

The second sentence is really the meat of this change, and not at all
incidental as "Also" implies. You should use it (or a refinement of
it) as your commit message. The first sentence doesn't particularly
add much and can easily be dropped.

> Signed-off-by: Karthik Nayak 
> ---
>  commit.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..e5dc2e2 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -552,6 +552,7 @@ static void record_author_date(struct
> author_date_slab *author_date,
>   char *buffer = NULL;
>   struct ident_split ident;
>   char *date_end;
> + const char *buf_skipprefix;

Read this response by Junio [2] to another GSoC candidate which
explains why this is a poor variable name and how to craft a better
one.

[2]: http://thread.gmane.org/gmane.comp.version-control.git/243231/focus=243259

>   unsigned long date;
>
>   if (!commit->buffer) {
> @@ -562,18 +563,20 @@ static void record_author_date(struct
> author_date_slab *author_date,
>   return;
>   }
>
> + buf_skipprefix = skip_prefix(buf, "author ");

Unfortunately, your patch is badly whitespace-damaged as if it was
pasted into the mailer and mangled. (Your first submission did not
have this problem.) If you can, use "git send-email" to avoid such
corruption.

> +
>   for (buf = commit->buffer ? commit->buffer : buffer;
>   buf;
>   buf = line_end + 1) {
>   line_end = strchrnul(buf, '\n');
> - if (!starts_with(buf, "author ")) {
> + if (!buf_skipprefix) {
>   if (!line_end[0] || line_end[1] == '\n')
>   return; /* end of header */
>   continue;
>   }
>   if (split_ident_line(&ident,
> - buf + strlen("author "),
> - line_end - (buf + strlen("author "))) ||
> + buf_skipprefix,
> + line_end - buf_skipprefix) ||

Looks reasonable (sans whitespace damage).

>  !ident.date_begin || !ident.date_end)
>   goto fail_exit; /* malformed "author" line */
>   break;
> @@ -1113,7 +1116,7 @@ int parse_signed_commit(const unsigned char *sha1,
>   next = next ? next + 1 : tail;
>   if (in_signature && line[0] == ' ')
>   sig = line + 1;
> - else if (starts_with(line, gpg_sig_header) &&
> + else if (skip_prefix(line, gpg_sig_header) &&
>   line[gpg_sig_header_len] == ' ')
>   sig = line + gpg_sig_header_len + 1;
>   if (sig) {
> @@ -1193,7 +1196,7 @@ static void parse_gpg_output(struct signature_check 
> *sigc)
>   for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>   const char *found, *next;
>
> - if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
> + if (skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) {
>   /* At the very beginning of the buffer */
>   found = buf + strlen(sigcheck_gpg_status[i].check + 1);
>   } else {

For these two sets of changes, unless you are actually taking
advantage of the return value of skip_prefix(), the mere mechanical
replacement of starts_with() with skip_prefix() actually hurts
readability. Examine each of these cases more carefully to determine
whether skip_prefix() is in fact useful. If so, take advantage of it.
If not, explain in the patch commentary why skip_prefix() doesn't
help.

> --
> 1.9.0.138.g2de3478
--
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] test-lib: tests skipped by GIT_SKIP_TESTS say so


On 3/3/2014 3:18 PM, Eric Sunshine wrote:

On Mon, Mar 3, 2014 at 6:12 PM, Ilya Bobyr  wrote:

On 3/3/2014 2:59 PM, Eric Sunshine wrote:

On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:

We used to show "(missing )" next to tests skipped because they are
specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.

Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?


The next patch adds another reason for the test to be skipped, so it seems
reasonable to say why exactly.
The patch actually makes it say "matched GIT_SKIP_TESTS".
It looks OK on the console.

Still just bikeshedding:

That new message in patch #2 says "not in GIT_TEST_ONLY", but isn't
"(excluded)" also applicable to that case? Is it important to be able
to distinguish between the two "excluded" reasons?

(No more bikeshedding for me.)


Makes sense.  I guess it is unlikely you would want to use both include 
and exclude filters in one run.

On the other hand it seems nice to see the reason why it was skipped.

Here are both options for comparison.  "Longer":

$ GIT_SKIP_TESTS='t.[236789] t0001.??' ./t0001-init.sh
ok 1 - plain
ok 2 # skip plain nested in bare (matched GIT_SKIP_TESTS)
ok 3 # skip plain through aliased command, outside any git repo 
(matched GIT_SKIP_TESTS)

not ok 4 - plain nested through aliased command # TODO known breakage
not ok 5 - plain nested in bare through aliased command # TODO 
known breakage

ok 6 # skip plain with GIT_WORK_TREE (matched GIT_SKIP_TESTS)

and changed to "excluded":

$ GIT_SKIP_TESTS='t.[236789] t0001.??' ./t0001-init.sh
ok 1 - plain
ok 2 # skip plain nested in bare (excluded)
ok 3 # skip plain through aliased command, outside any git repo 
(excluded)

not ok 4 - plain nested through aliased command # TODO known breakage
not ok 5 - plain nested in bare through aliased command # TODO 
known breakage

ok 6 # skip plain with GIT_WORK_TREE (excluded)

P.S. It seems that now the whole interface may change :)
--
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] test-lib: GIT_TEST_ONLY to run only specific tests


On 3/3/2014 3:26 PM, Junio C Hamano wrote:

Eric Sunshine  writes:


On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:

This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.

To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?

I actually do not like the interface to use two variables very much.
Can't we just allow negative entries on "to be skipped" list?

That is

GIT_SKIP_TESTS='t9??? !t91??'

would skip nine-thousand series, but would run 91xx series, and all
the others are not excluded.

Simple rules to consider:

  - If the list consists of _only_ negated patterns, pretend that
there is "unless otherwise specified with negatives, skip all
tests", i.e. treat GIT_SKIP_TESTS='!t91??' just the same way you
would treat GIT_SKIP_TESTS='* !t91??'.

  - The orders should not matter for simplicity of the semantics;
before running each test, check if it matches any negative (and
run it if it matches, without looking at any positives), and
otherwise check if it matches any positive (and skip it if it
does not).

Hmm?


I can do that.  But I am not sure that matches the use cases I had in 
mind the best.


First use case is that while developing I want to run tests frequently 
and I have a specific test that I am working on at the moment.

That test is broken and I am trying to fix it (TDD).
I want to run just the initialization test(s) and then that specific test.
Running everything is quite slow.

GIT_RUN_ONLY addresses the TDD case.

Second case is when I broke one or more tests and want to figure out 
what is wrong.
In this case running tests after the broken one will clutter the output 
directory and will make debugging somewhat harder, especially if I am 
not familiar with all the tests.


For the second case I was actually thinking that something like 
"

Maybe we can come up with an interface that covers all 3 cases?

While exclusion can be used it adds an extra step to both cases, as you 
need to mentally negate what you want first.


It might be that we are looking at different use cases, as you are 
talking about whole test suits.

--
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] commit.c: Use skip_prefix() instead of starts_with()

Max Horn  writes:

> On 03.03.2014, at 20:43, Junio C Hamano  wrote:
>
>> Tanay Abhra  writes:
>> 
>>> @@ -1193,9 +1194,9 @@ static void parse_gpg_output(struct signature_check 
>>> *sigc)
>>> for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>>> const char *found, *next;
>>> 
>>> -   if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
>>> +   if (found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) 
>>> {
>>> /* At the very beginning of the buffer */
>>> -   found = buf + strlen(sigcheck_gpg_status[i].check + 1);
>>> +   ;
>>> } else {
>>> found = strstr(buf, sigcheck_gpg_status[i].check);
>>> if (!found)
>> 
>> This hunk looks good.  It can be a separate patch but they are both
>> minor changes so it is OK to have it in a single patch.
>
> Hm, but that hunk also introduces an assignment in a conditional,
> and introduces an empty block. Maybe like this?

Much better.

If we anticipate that we may add _more_ ways to find the thing
later, then I would say this code structure is better:

/* Is it at the beginning of the buffer? */
found = skip_prefix(...);
if (!found) {
/* Oh, maybe it is on the second or later line? */
found = ... find it on a later line...
}
if (!found)
continue;

but I do not think that is the case for this particular one.  We
either try to find it at the very beginning (i.e. no leading
newline), or we have some other lines before it (i.e. require
leading newline), and there will be no future extension, so what you
suggested below looks sensible.

> diff --git a/commit.c b/commit.c
> index 6bf4fe0..0ee0725 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1193,10 +1193,8 @@ static void parse_gpg_output(struct signature_check 
> *sigc)
> for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
> const char *found, *next;
>
> -   if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
> -   /* At the very beginning of the buffer */
> -   found = buf + strlen(sigcheck_gpg_status[i].check + 
> 1);
> -   } else {
> +   found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
> +   if (!found) {
> found = strstr(buf, sigcheck_gpg_status[i].check);
> if (!found)
> continue;
--
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] test-lib: tests skipped by GIT_SKIP_TESTS say so

Eric Sunshine  writes:

> That new message in patch #2 says "not in GIT_TEST_ONLY", but isn't
> "(excluded)" also applicable to that case? Is it important to be able
> to distinguish between the two "excluded" reasons?

An obvious solution is not to *have* two reasons in the first place
;-)
--
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] skip_prefix: rewrite so that prefix is scanned once

2014-03-03 Thread Duy Nguyen

On Tue, Mar 4, 2014 at 5:43 AM, Junio C Hamano  wrote:
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cbd86c3..68ffaef 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -357,8 +357,14 @@ extern int suffixcmp(const char *str, const char 
> *suffix);
>
>  static inline const char *skip_prefix(const char *str, const char *prefix)
>  {
> -   size_t len = strlen(prefix);
> -   return strncmp(str, prefix, len) ? NULL : str + len;

Just a note. gcc does optimize strlen("abcdef") to 6, and with that
information at compile time built-in strncmp might do better.

> +   while (1) {
> +   if (!*prefix)
> +   return str;
> +   if (*str != *prefix)
> +   return NULL;
> +   prefix++;
> +   str++;
> +   }
>  }
>
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
-- 
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] skip_prefix: rewrite so that prefix is scanned once

David Kastrup  writes:

> How about a function body of
>
>   do {
>   if (!*prefix)
>   return str;
> } while (*str++ == *prefix++);
> return NULL;
>
> I'm not too fond of while (1) and tend to use for (;;) instead, but that
> may again partly be due to some incredibly non-optimizing compiler back
> in the days of my youth.  At any rate, the do-while loop seems a bit
> brisker.

I do not have strong preference between "while (1)" and "for (;;)",
but I tend to agree

for (;; prefix++, str++) {
if (!*prefix)
return str;
if (*str != *prefix)
return NULL;
}

may be easier to read than what I suggested.  Your do-while loop is
concise and very readable, so let's take that one (I'll forge your
Sign-off ;-)).

I haven't looked at the generated assembly of any of these, though.
--
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] test-lib: GIT_TEST_ONLY to run only specific tests

Eric Sunshine  writes:

> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:
>> This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.
>
> To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?

I actually do not like the interface to use two variables very much.
Can't we just allow negative entries on "to be skipped" list?

That is

GIT_SKIP_TESTS='t9??? !t91??'

would skip nine-thousand series, but would run 91xx series, and all
the others are not excluded.

Simple rules to consider:

 - If the list consists of _only_ negated patterns, pretend that
   there is "unless otherwise specified with negatives, skip all
   tests", i.e. treat GIT_SKIP_TESTS='!t91??' just the same way you
   would treat GIT_SKIP_TESTS='* !t91??'.

 - The orders should not matter for simplicity of the semantics;
   before running each test, check if it matches any negative (and
   run it if it matches, without looking at any positives), and
   otherwise check if it matches any positive (and skip it if it
   does not).

Hmm?

>> ---
>>  t/README  |   15 +++
>>  t/test-lib.sh |8 
>>  2 files changed, 23 insertions(+), 0 deletions(-)
>>
>> diff --git a/t/README b/t/README
>> index caeeb9d..f939987 100644
>> --- a/t/README
>> +++ b/t/README
>> @@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the 
>> whole
>>  test, or t[0-9]{4} followed by ".$number" to say which
>>  particular test to skip.
>>
>> +Sometimes the opposite is desired - ability to execute only one or
>> +several tests.  Mostly while debugging tests.  For that you can say
>> +
>> +$ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
>> +
>> +or, similrary to GIT_SKIP_TESTS
>> +
>> +$ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
>> +
>> +In additiona to matching against "."
>
> s/additiona/addition/
>
> Plus the other typos already mentioned by Philip...
>
>> +GIT_TEST_ONLY is matched against just the test numbes.  This comes
>> +handy when you are running only one test:
>> +
>> +$ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
>> +
>>  Note that some tests in the existing test suite rely on previous
>>  test item, so you cannot arbitrarily disable one and expect the
>>  remainder of test to check what the test originally was intended
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 89a405b..12bf436 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -464,6 +464,14 @@ test_skip () {
>> fi
>> skipped_reason="missing $missing_prereq${of_prereq}"
>> fi
>> +   if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
>> +   ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
>> +   ! match_pattern_list $test_count $GIT_TEST_ONLY
>> +   then
>> +   to_skip=t
>> +   skipped_reason="not in GIT_TEST_ONLY"
>> +   fi
>> +
>> case "$to_skip" in
>> t)
>> say_color skip >&3 "skipping test: $@"
>> --
>> 1.7.9
>>
>> --
>> 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
--
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: difftool sends malformed path to exernal tool on Windows

2014-03-03 Thread Paul Lotz

David,

OK, I did as you suggested, and the results were revealing.

First, I replaced "echo" with "cat".  Result: The contents of both files 
appeared in the Git Bash Window.

Then I tried calling LVCompare from the Git Bash and Windows Command Prompt 
windows with variations on the paths.

Here are the most relevant results:
First from the Windows Command Prompt:
1) This command works:
C:\LSST_TS\SystemSW\M2AADT>"C:\Program Files (x86)\National Instruments\Shared\L
abVIEW Compare\LVCompare.exe" C:\Users\Paul\AppData\Local\Temp\Typedefs_TestStat
us_Before.ctl C:\LSST_TS\SystemSW\M2AADT\Typedefs\TestStatus.ctl
[General note:
I saved a copy of the temp file and replaced the hex string with the string 
'Before' to make the file stick around.  The paths are otherwise the same.]

2) C:\LSST_TS\SystemSW\M2AADT>"C:\Program Files (x86)\National 
Instruments\Shared\L
abVIEW Compare\LVCompare.exe" C:\Users\Paul\AppData\Local\Temp\Typedefs_TestStat
us_Before.ctl Typedefs\TestStatus.ctl

Result: Error message with reference to C:\Program Files (x86)\National 
Instruments\Shared\L
abVIEW Compare\supportVIs\_prolvcmp.llb\Typedefs\TestStatus.ctl

Observation: The second path has to be the full path, not the relative path we 
get back using "echo".

>From the Git Bash window:
1) I tried the command that worked in the Windows Command Prompt:
"C:\Program Files (x86)\National Instruments\Shared\L
abVIEW Compare\LVCompare.exe" C:\Users\Paul\AppData\Local\Temp\Typedefs_TestStat
us_Before.ctl C:\LSST_TS\SystemSW\M2AADT\Typedefs\TestStatus.ctl

Result: Error message with reference to 
C:\UsersPaulAppDataLocalTempTypedefs_TestStatus_Before.ctl

2) So I tried a variation with forward slashes instead in the parameters:
"C:\Program Files (x86)\National Instruments\Shared\L
abVIEW Compare\LVCompare.exe" C:/Users/Paul/AppData/Local/Temp/Typedefs_TestStat
us_Before.ctl C:/LSST_TS/SystemSW/M2AADT/Typedefs/TestStatus.ctl

Result: Error message with reference to 
C:\/Users/Paul/AppData/Local/Temp/Typedefs_TestStatus_Before.ctl
[Note: This is the familiar problem we saw using the Git difftool command.]

3) So I tried forward slashes even in the LVCompare.exe path:
"C:/Program Files (x86)/National Instruments\Shared\L
abVIEW Compare\LVCompare.exe" C:/Users/Paul/AppData/Local/Temp/Typedefs_TestStat
us_Before.ctl C:/LSST_TS/SystemSW/M2AADT/Typedefs/TestStatus.ctl

Result: Error message with reference to 
C:\/Users/Paul/AppData/Local/Temp/Typedefs_TestStatus_Before.ctl

[No difference.]

Paul

-Original Message-
From: David Aguilar [mailto:dav...@gmail.com] 
Sent: Sunday, March 2, 2014 5:35 PM
To: Paul Lotz
Cc: Git Mailing List
Subject: Re: difftool sends malformed path to exernal tool on Windows

On Mon, Feb 24, 2014 at 8:44 AM, Paul Lotz  wrote:
> David,
>
> Thanks for the helpful reply.
>
> As you suggested, I modified the .gitconfig file to have:
> [difftool "test"]
> cmd = echo \"$LOCAL\" \"$REMOTE\"
>
> and ran
> $ git difftool -t test
>
> An example of the the resulting console output is:
> C:/Users/Paul/AppData/Local/Temp/I8L2Bc_WriteTestParameters.vi 
> Commands/StartAutomatedTest/WriteTestParameters.vi

Hmm. That's interesting.

The next test: replace "echo" with "cat".

Are the contents of both files printed?

If so, then the next thing to try is running:

LVCompare.exe [same paths printed by echo]

and then seeing if it does the right thing.

Could it be that LVCompare.exe is getting tripped up by the forward slashes?

I'm not familiar enough with how msysgit mangles paths before launching 
programs. It may be that the C:/foo/bar/baz path is getting manged (that's my 
current guess), but I really don't know.

Another tip I've read online is that launching git via "cmd.exe" may avoid the 
path mangling. Sorry I'm not more helpful in this area.

Another workaround you can do is to place a shell script wrapper around 
LVCompare.exe that replaces C:\/ with / and then launches the real 
LVCompare.exe; that's a workaround, but it could work.

I hope that helps shed some light on what may be going on.
--
David

--
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] skip_prefix: rewrite so that prefix is scanned once

Junio C Hamano  writes:

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -357,8 +357,14 @@ extern int suffixcmp(const char *str, const char 
> *suffix);
>  
>  static inline const char *skip_prefix(const char *str, const char *prefix)
>  {
> - size_t len = strlen(prefix);
> - return strncmp(str, prefix, len) ? NULL : str + len;
> + while (1) {
> + if (!*prefix)
> + return str;
> + if (*str != *prefix)
> + return NULL;
> + prefix++;
> + str++;
> + }
>  }

How about a function body of

do {
if (!*prefix)
return str;
} while (*str++ == *prefix++);
return NULL;

I'm not too fond of while (1) and tend to use for (;;) instead, but that
may again partly be due to some incredibly non-optimizing compiler back
in the days of my youth.  At any rate, the do-while loop seems a bit
brisker.

-- 
David Kastrup
--
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] test-lib: tests skipped by GIT_SKIP_TESTS say so

On Mon, Mar 3, 2014 at 6:12 PM, Ilya Bobyr  wrote:
> On 3/3/2014 2:59 PM, Eric Sunshine wrote:
>>
>> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:
>>>
>>> We used to show "(missing )" next to tests skipped because they are
>>> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
>>
>> Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?
>
>
> The next patch adds another reason for the test to be skipped, so it seems
> reasonable to say why exactly.
> The patch actually makes it say "matched GIT_SKIP_TESTS".
> It looks OK on the console.

Still just bikeshedding:

That new message in patch #2 says "not in GIT_TEST_ONLY", but isn't
"(excluded)" also applicable to that case? Is it important to be able
to distinguish between the two "excluded" reasons?

(No more bikeshedding for 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

Re: [PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests


On 3/3/2014 3:03 PM, Eric Sunshine wrote:

On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:

This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.

To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?


There is actually an upside in the fact that the name is "different 
enough".  When you pull a command from a history it is easier to see if 
it is the excluding or the including one.

Maybe we can have a third opinion here?


---
  t/README  |   15 +++
  t/test-lib.sh |8 
  2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index caeeb9d..f939987 100644
--- a/t/README
+++ b/t/README
@@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the whole
  test, or t[0-9]{4} followed by ".$number" to say which
  particular test to skip.

+Sometimes the opposite is desired - ability to execute only one or
+several tests.  Mostly while debugging tests.  For that you can say
+
+$ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
+
+or, similrary to GIT_SKIP_TESTS
+
+$ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
+
+In additiona to matching against "."

s/additiona/addition/

Plus the other typos already mentioned by Philip...


Thank you.  I will include all of those in the next version of the 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 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

Eric Sunshine  writes:

> On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:
>> We used to show "(missing )" next to tests skipped because they are
>> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
>
> Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?

Sounds good, or at least better than matched GIT_SKIP_TESTS, to me ;-).

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 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so


On 3/3/2014 2:59 PM, Eric Sunshine wrote:

On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:

We used to show "(missing )" next to tests skipped because they are
specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.

Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?


The next patch adds another reason for the test to be skipped, so it 
seems reasonable to say why exactly.

The patch actually makes it say "matched GIT_SKIP_TESTS".
It looks OK on the console.
--
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] test-lib: tests skipped by GIT_SKIP_TESTS say so


On 3/3/2014 7:11 AM, Philip Oakley wrote:

From: "Ilya Bobyr" 

We used to show "(missing )" next to tests skipped because they are
specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.


The message below forgets the "by".


I'll fix the commit message.  I think the output is long enough, while 
"by" does not add any information.



Otherwise looks sensible.


Thanks for looking at it :)




---
t/test-lib.sh |   13 -
1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..89a405b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,25 +446,28 @@ test_finish_ () {

test_skip () {
 to_skip=
+ skipped_reason=
 if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
 then
 to_skip=t
+ skipped_reason="matched GIT_SKIP_TESTS"


s/matched GIT_SKIP_TESTS/matched by GIT_SKIP_TESTS/


 [...]

--
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 v4 00/14] Use ALLOC_GROW() instead of inline code

> Dmitry S. Dolzhenko (14):
>   builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
>   bundle.c: use ALLOC_GROW() in add_to_ref_list()
>   cache-tree.c: use ALLOC_GROW() in find_subtree()
>   commit.c: use ALLOC_GROW() in register_commit_graft()
>   diff.c: use ALLOC_GROW()
>   diffcore-rename.c: use ALLOC_GROW()
>   patch-ids.c: use ALLOC_GROW() in add_commit()
>   replace_object.c: use ALLOC_GROW() in register_replace_object()
>   reflog-walk.c: use ALLOC_GROW()
>   dir.c: use ALLOC_GROW() in create_simplify()
>   attr.c: use ALLOC_GROW() in handle_attr_line()
>   builtin/mktree.c: use ALLOC_GROW() in append_to_tree()
>   read-cache.c: use ALLOC_GROW() in add_index_entry()
>   sha1_file.c: use ALLOC_GROW() in pretend_sha1_file()

All looked cleanly done.

The resulting code of 1, 3, 4, 6 and 8 share this pattern:

ALLOC_GROW(table, number + 1, alloc);
number++;

which may be easier to understand if done the other way around:

number++;
ALLOC_GROW(table, number, alloc);

That is, "we know we want one more, so make sure they fit in the
table".

But that is just a minor issue; I suspect many existing callsites to
ALLOC_GROW() already follow the former pattern, and if we decide to
to switch the former to the latter, we shouldn't be doing so within
this series (we should do that as a separate series on top of this).

Thanks; will queue.


--
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] test-lib: GIT_TEST_ONLY to run only specific tests

On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:
> This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.

To be grammatically similar to GIT_SKIP_TESTS, perhaps name it GIT_RUN_TESTS?

> ---
>  t/README  |   15 +++
>  t/test-lib.sh |8 
>  2 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/t/README b/t/README
> index caeeb9d..f939987 100644
> --- a/t/README
> +++ b/t/README
> @@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the 
> whole
>  test, or t[0-9]{4} followed by ".$number" to say which
>  particular test to skip.
>
> +Sometimes the opposite is desired - ability to execute only one or
> +several tests.  Mostly while debugging tests.  For that you can say
> +
> +$ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
> +
> +or, similrary to GIT_SKIP_TESTS
> +
> +$ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
> +
> +In additiona to matching against "."

s/additiona/addition/

Plus the other typos already mentioned by Philip...

> +GIT_TEST_ONLY is matched against just the test numbes.  This comes
> +handy when you are running only one test:
> +
> +$ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
> +
>  Note that some tests in the existing test suite rely on previous
>  test item, so you cannot arbitrarily disable one and expect the
>  remainder of test to check what the test originally was intended
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 89a405b..12bf436 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -464,6 +464,14 @@ test_skip () {
> fi
> skipped_reason="missing $missing_prereq${of_prereq}"
> fi
> +   if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
> +   ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
> +   ! match_pattern_list $test_count $GIT_TEST_ONLY
> +   then
> +   to_skip=t
> +   skipped_reason="not in GIT_TEST_ONLY"
> +   fi
> +
> case "$to_skip" in
> t)
> say_color skip >&3 "skipping test: $@"
> --
> 1.7.9
>
> --
> 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
--
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] commit.c: Use skip_prefix() instead of starts_with()

2014-03-03 Thread Max Horn


On 03.03.2014, at 20:43, Junio C Hamano  wrote:

> Tanay Abhra  writes:
> 
>> @@ -1193,9 +1194,9 @@ static void parse_gpg_output(struct signature_check 
>> *sigc)
>>  for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>>  const char *found, *next;
>> 
>> -if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
>> +if (found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) 
>> {
>>  /* At the very beginning of the buffer */
>> -found = buf + strlen(sigcheck_gpg_status[i].check + 1);
>> +;
>>  } else {
>>  found = strstr(buf, sigcheck_gpg_status[i].check);
>>  if (!found)
> 
> This hunk looks good.  It can be a separate patch but they are both
> minor changes so it is OK to have it in a single patch.

Hm, but that hunk also introduces an assignment in a conditional, and 
introduces an empty block. Maybe like this?


diff --git a/commit.c b/commit.c
index 6bf4fe0..0ee0725 100644
--- a/commit.c
+++ b/commit.c
@@ -1193,10 +1193,8 @@ static void parse_gpg_output(struct signature_check 
*sigc)
for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;

-   if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
-   /* At the very beginning of the buffer */
-   found = buf + strlen(sigcheck_gpg_status[i].check + 1);
-   } else {
+   found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
+   if (!found) {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
continue;




signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

On Mon, Mar 3, 2014 at 5:24 AM, Ilya Bobyr  wrote:
> We used to show "(missing )" next to tests skipped because they are
> specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.

Bikeshedding: That's pretty verbose. Perhaps just say "(excluded)"?

> ---
>  t/test-lib.sh |   13 -
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 1531c24..89a405b 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -446,25 +446,28 @@ test_finish_ () {
>
>  test_skip () {
> to_skip=
> +   skipped_reason=
> if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
> then
> to_skip=t
> +   skipped_reason="matched GIT_SKIP_TESTS"
> fi
> if test -z "$to_skip" && test -n "$test_prereq" &&
>! test_have_prereq "$test_prereq"
> then
> to_skip=t
> -   fi
> -   case "$to_skip" in
> -   t)
> +
> of_prereq=
> if test "$missing_prereq" != "$test_prereq"
> then
> of_prereq=" of $test_prereq"
> fi
> -
> +   skipped_reason="missing $missing_prereq${of_prereq}"
> +   fi
> +   case "$to_skip" in
> +   t)
> say_color skip >&3 "skipping test: $@"
> -   say_color skip "ok $test_count # skip $1 (missing 
> $missing_prereq${of_prereq})"
> +   say_color skip "ok $test_count # skip $1 ($skipped_reason)"
> : true
> ;;
> *)
> --
> 1.7.9
>
> --
> 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
--
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] Halt during fetch on MacOS

2014-03-03 Thread Conley Owens

On Sun, Mar 2, 2014 at 6:36 AM, Max Horn  wrote:
>
> On 01.03.2014, at 00:26, Conley Owens  wrote:
>
>> $ git --version  # This is just the git from MacPorts
>> git version 1.8.5.5
>> $ sw_vers
>> ProductName:Mac OS X
>> ProductVersion: 10.8.5
>> BuildVersion:   12F45
>
> I cannot reproduce this, neither with 1.8.5.5 nor with 1.9.0. I am also 
> running Mac OS X 10.8.5.
>
> Note: I tried this both with you original test.sh, and also with a version 
> were I replaced the ">" by ">>", as per Jeff's suggestion. It (as predicted) 
> didn't make any difference).
>
> If this is a race condition, it might be easier to trigger it on slower 
> hardware. I am running this on a 2012 MBP with 2.3 Ghz i7 and an SSD. What is 
> your test machine?

Mac mini i7 with 1TB fusion drive and 16GB ram.

It also seemed to trigger more when grabbing larger repositories.
Replace"external/tinyxml2" with "docs/source.android.com" and you
might be more likely to trigger the issue.  You can also try
increasing the number of processes to spawn from 100 to 150.

>
>
> Cheers,
> Max
>
>>
>> test.sh
>> "
>> #!/bin/bash
>> rungit() {
>>mkdir $1
>>GIT_DIR=$1 git init --bare
>>echo '[remote "aosp"]' > $1/config
>>echo 'url =
>> https://android.googlesource.com/platform/external/tinyxml2' >>
>> $1/config
>>GIT_DIR=$1 git fetch aosp +refs/heads/master:refs/remotes/aosp/master
>>rm -rf $1
>> }
>>
>> for i in $(seq 1 100)
>> do
>>rungit testdir$i &
>> done
>> """
>> $ ./test.sh  # Warning! This script fetches ~40MB of data
>>
>> When everything cools, you can see that there are some fetches hanging
>> (typically).
>> $ ps | grep 'git fetch'
>> ...
>> 63310 ttys0040:00.01 git fetch aosp
>> +refs/heads/master:refs/remotes/aosp/master
>> 63314 ttys0040:00.01 git fetch aosp
>> +refs/heads/master:refs/remotes/aosp/master
>> 63319 ttys0040:00.01 git fetch aosp
>> +refs/heads/master:refs/remotes/aosp/master
>> 63407 ttys0040:00.00 git fetch aosp
>> +refs/heads/master:refs/remotes/aosp/master
>> 63414 ttys0040:00.00 git fetch aosp
>> +refs/heads/master:refs/remotes/aosp/master
>> 63420 ttys0040:00.00 git fetch aosp
>> +refs/heads/master:refs/remotes/aosp/master
>> ...
>>
>> You can look at the parent process of each and see that one half
>> spawned the other half, or you can look at the environment variables
>> for each to see that there are two processes operating in the same
>> directory for each directory where there's an issue.
>> $ echo "$(for pid in $(ps | grep 'git fetch' | grep -o '^[0-9]*'); do
>> ps -p $pid -wwwE | grep 'GIT_DIR=[^ ]*' -o; done)" | sort
>> GIT_DIR=testdir14
>> GIT_DIR=testdir14
>> GIT_DIR=testdir32
>> GIT_DIR=testdir32
>> GIT_DIR=testdir47
>> GIT_DIR=testdir47
>>
>> I've searched through the mailing list, but this doesn't seem to be a
>> known issue.  I've only seen this occur on macs (and with a good deal
>> of regularity).  It doesn't occur on my Ubuntu box.
>>
>> ~cco3
>> --
>> 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
>>
>
--
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 v4 05/14] diff.c: use ALLOC_GROW()

"Dmitry S. Dolzhenko"  writes:

> Use ALLOC_GROW() instead inline code in
> diffstat_add() and diff_q()

"...instead of open coding it in..." may read better.

>
> Signed-off-by: Dmitry S. Dolzhenko 
> ---
>  diff.c | 12 ++--
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index e800666..aebdfda 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1361,11 +1361,7 @@ static struct diffstat_file *diffstat_add(struct 
> diffstat_t *diffstat,
>  {
>   struct diffstat_file *x;
>   x = xcalloc(sizeof (*x), 1);
> - if (diffstat->nr == diffstat->alloc) {
> - diffstat->alloc = alloc_nr(diffstat->alloc);
> - diffstat->files = xrealloc(diffstat->files,
> - diffstat->alloc * sizeof(x));
> - }
> + ALLOC_GROW(diffstat->files, diffstat->nr + 1, diffstat->alloc);
>   diffstat->files[diffstat->nr++] = x;
>   if (name_b) {
>   x->from_name = xstrdup(name_a);
> @@ -3965,11 +3961,7 @@ struct diff_queue_struct diff_queued_diff;
>  
>  void diff_q(struct diff_queue_struct *queue, struct diff_filepair *dp)
>  {
> - if (queue->alloc <= queue->nr) {
> - queue->alloc = alloc_nr(queue->alloc);
> - queue->queue = xrealloc(queue->queue,
> - sizeof(dp) * queue->alloc);
> - }
> + ALLOC_GROW(queue->queue, queue->nr + 1, queue->alloc);
>   queue->queue[queue->nr++] = dp;
>  }
--
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] skip_prefix: rewrite so that prefix is scanned once

Junio C Hamano  writes:

> Siddharth Goel  writes:
>
>> Helped-by: Eric Sunshine 
>> Signed-off-by: Siddharth Goel 
>> ---
>> Added a space after colon in the subject as compared to previous 
>> patch [PATCH v2].
>>
>> [PATCH v2]: http://thread.gmane.org/gmane.comp.version-control.git/243150
>
> Whenever you see "Change", "Rewrite", etc. in the subject of a patch
> that touches existing code, think twice.  The subject line is a
> scarce real estate to be wasted on a noiseword that carries no real
> information, and we already know a patch that touches existing code
> changes or rewrites things.
>
> Subject: [PATCH v3] skip_prefix: scan prefix only once
>
> perhaps?
>
>>  git-compat-util.h | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index 614a5e9..550dce3 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char 
>> *suffix);
>>  
>>  static inline const char *skip_prefix(const char *str, const char *prefix)
>>  {
>> -size_t len = strlen(prefix);
>> -return strncmp(str, prefix, len) ? NULL : str + len;
>> +while (*prefix != '\0' && *str == *prefix) {
>> +str++;
>> +prefix++;
>> +}
>> +return (*prefix == '\0' ? str : NULL);
>
> Unlike another patch I saw the other day on the same topic, this
> checks *prefix twice for the last round, even though I think this
> one is probably slightly easier to read.  I dunno.

That is, something like this instead.  After looking at it again, I
do not think it is less readable than the above.

 git-compat-util.h | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index cbd86c3..68ffaef 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,14 @@ extern int suffixcmp(const char *str, const char *suffix);
 
 static inline const char *skip_prefix(const char *str, const char *prefix)
 {
-   size_t len = strlen(prefix);
-   return strncmp(str, prefix, len) ? NULL : str + len;
+   while (1) {
+   if (!*prefix)
+   return str;
+   if (*str != *prefix)
+   return NULL;
+   prefix++;
+   str++;
+   }
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
--
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 v4 12/14] builtin/mktree.c: use ALLOC_GROW() in append_to_tree()

Helped-by: He Sun 
Signed-off-by: Dmitry S. Dolzhenko 
---
 builtin/mktree.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/mktree.c b/builtin/mktree.c
index f92ba40..a964d6b 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -23,10 +23,7 @@ static void append_to_tree(unsigned mode, unsigned char 
*sha1, char *path)
if (strchr(path, '/'))
die("path %s contains slash", path);
 
-   if (alloc <= used) {
-   alloc = alloc_nr(used);
-   entries = xrealloc(entries, sizeof(*entries) * alloc);
-   }
+   ALLOC_GROW(entries, used + 1, alloc);
ent = entries[used++] = xmalloc(sizeof(**entries) + len + 1);
ent->mode = mode;
ent->len = len;
-- 
1.8.3.2

--
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 v4 09/14] reflog-walk.c: use ALLOC_GROW()

Use ALLOC_GROW() instead inline code in
add_commit_info() and read_one_reflog()

Signed-off-by: Dmitry S. Dolzhenko 
---
 reflog-walk.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index b2fbdb2..2899729 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -26,11 +26,7 @@ static int read_one_reflog(unsigned char *osha1, unsigned 
char *nsha1,
struct complete_reflogs *array = cb_data;
struct reflog_info *item;
 
-   if (array->nr >= array->alloc) {
-   array->alloc = alloc_nr(array->nr + 1);
-   array->items = xrealloc(array->items, array->alloc *
-   sizeof(struct reflog_info));
-   }
+   ALLOC_GROW(array->items, array->nr + 1, array->alloc);
item = array->items + array->nr;
memcpy(item->osha1, osha1, 20);
memcpy(item->nsha1, nsha1, 20);
@@ -114,11 +110,7 @@ static void add_commit_info(struct commit *commit, void 
*util,
struct commit_info_lifo *lifo)
 {
struct commit_info *info;
-   if (lifo->nr >= lifo->alloc) {
-   lifo->alloc = alloc_nr(lifo->nr + 1);
-   lifo->items = xrealloc(lifo->items,
-   lifo->alloc * sizeof(struct commit_info));
-   }
+   ALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);
info = lifo->items + lifo->nr;
info->commit = commit;
info->util = util;
-- 
1.8.3.2

--
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: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling

2014-03-03 Thread Jeff King

On Sun, Mar 02, 2014 at 06:04:39AM +0900, Brian Gesiak wrote:

> My name is Brian Gesiak. I'm a research student at the University of
> Tokyo, and I'm hoping to participate in this year's Google Summer of
> Code by contributing to Git. I'm a longtime user, first-time
> contributor--some of you may have noticed my "microproject"
> patches.[1][2]

Yes, we did notice them. Thanks, and welcome. :)

> The ideas page points out that while lock files are closed and
> unlinked[3] when the program exits[4], object pack files implement
> their own brand of temp file creation and deletion. This
> implementation doesn't share the same guarantees as lock files--it is
> possible that the program terminates before the temp file is
> unlinked.[5]
> 
> Lock file references are stored in a linked list. When the program
> exits, this list is traversed and each file is closed and unlinked. It
> seems to me that this mechanism is appropriate for temp files in
> general, not just lock files. Thus, my proposal would be to extract
> this logic into a separate module--tempfile.h, perhaps. Lock and
> object files would share the tempfile implementation.
> 
> That is, both object and lock temp files would be stored in a linked
> list, and all of these would be deleted at program exit.

Yes, I think this is definitely the right way to go. We should be able
to unify the tempfile handling for all of git. Once the logic is
extracted into a nice API, there are several other places that can use
it, too:

  - the external diff code creates tempfiles and uses its own cleanup
routines

  - the shallow_XX tempfiles (these are not cleaned right now,
though I sent a patch recently for them to do their own cleanup)

Those are just off the top of my head. There may be other spots, too.

It is worth thinking in your proposal about some of the things that the
API will want to handle. What are the mismatches in how lockfiles and
object files are handled? E.g., how do we finalize them into place?
How should the API be designed to minimize race conditions (e.g., if we
get a signal delivered while we are committing or cleaning up a file)?

> Please let me know if this seems like it would make for an interesting
> proposal, or if perhaps there is something I am overlooking. Any
> feedback at all would be appreciated. Thank you!

You definitely have a grasp of what the project is aiming for, and which
areas need to be touched.

-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: My advice for GSoC applicants

Michael Haggerty  writes:

> Based on my experience so far as a first-time Google Summer of Code
> mentor, I just wrote a blog article containing some hopefully useful
> advice for students applying to the program.  Please note that this is
> my personal opinion only and doesn't necessarily reflect the views of
> the Git/libgit2 projects as a whole.
>
> My secret tip for GSoC success
>
> http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html

Thanks for writing this.

Also thanks for the MicroProject approach to introduce potential
students and the community.

Multiple students seem to be hitting the same microprojects (aka "we
are running out of micros"), which might be a bit unfortunate.  I
think the original plan might have been that for a student candidate
to pass, his-or-her patch must hit my tree and queued somewhere, but
with these duplicates I do not think it is fair to disqualify those
who interacted with reviewers well but solved an already solved
micro.

Even with the duplicates I think we are learning how well each
student respond to reviews (better ones even seem to pick up lessons
from reviews on others' threads that tackle micros different from
their own) and what his-or-her general cognitive ability is.
--
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] rebase: accept - as another way of saying HEAD~

2014-03-03 Thread Matthieu Moy

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Michael Haggerty  writes:
>>
>>> Or perhaps "-NUM" should fail with an error message if any of the last
>>> NUM commits are merges.  In that restricted scenario (which probably
>>> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".
>>
>> Makes sense to me. So, -NUM would actually mean "rebase the last NUM
>> commits" (as well as being an alias for HEAD~NUM), but would fail when
>> it does not make sense (with an error message explaining the situation
>> and pointing the user to HEAD~N if this is what he wanted).
>>
>> This would actually be a feature for me: I often want to rebase "recent
>> enough" history, and when my @{upstream} isn't well positionned,...
>
> Could you elaborate on this a bit?  What does "isn't well
> positioned" mean?

The most common case is when @{upstream} is not positionned at all,
because I'm on a temporary branch on which I did not configure it. The
other case is when I did a manual merge of a branch other than upstream.

As I said in another message, there would probably be cleaner solutions,
but the trial and error one does not work that bad.

-- 
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

[PATCH v4 04/14] commit.c: use ALLOC_GROW() in register_commit_graft()

Signed-off-by: Dmitry S. Dolzhenko 
---
 commit.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/commit.c b/commit.c
index 6bf4fe0..e004314 100644
--- a/commit.c
+++ b/commit.c
@@ -147,12 +147,8 @@ int register_commit_graft(struct commit_graft *graft, int 
ignore_dups)
return 1;
}
pos = -pos - 1;
-   if (commit_graft_alloc <= ++commit_graft_nr) {
-   commit_graft_alloc = alloc_nr(commit_graft_alloc);
-   commit_graft = xrealloc(commit_graft,
-   sizeof(*commit_graft) *
-   commit_graft_alloc);
-   }
+   ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
+   commit_graft_nr++;
if (pos < commit_graft_nr)
memmove(commit_graft + pos + 1,
commit_graft + pos,
-- 
1.8.3.2

--
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 v4 13/14] read-cache.c: use ALLOC_GROW() in add_index_entry()

Signed-off-by: Dmitry S. Dolzhenko 
---
 read-cache.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index fb440b4..cbdf954 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -990,11 +990,7 @@ int add_index_entry(struct index_state *istate, struct 
cache_entry *ce, int opti
}
 
/* Make sure the array is big enough .. */
-   if (istate->cache_nr == istate->cache_alloc) {
-   istate->cache_alloc = alloc_nr(istate->cache_alloc);
-   istate->cache = xrealloc(istate->cache,
-   istate->cache_alloc * 
sizeof(*istate->cache));
-   }
+   ALLOC_GROW(istate->cache, istate->cache_nr + 1, istate->cache_alloc);
 
/* Add it in.. */
istate->cache_nr++;
-- 
1.8.3.2

--
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 v4 07/14] patch-ids.c: use ALLOC_GROW() in add_commit()

Signed-off-by: Dmitry S. Dolzhenko 
---
 patch-ids.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/patch-ids.c b/patch-ids.c
index bc8a28f..bf81b92 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -83,10 +83,7 @@ static struct patch_id *add_commit(struct commit *commit,
ent = &bucket->bucket[bucket->nr++];
hashcpy(ent->patch_id, sha1);
 
-   if (ids->alloc <= ids->nr) {
-   ids->alloc = alloc_nr(ids->nr);
-   ids->table = xrealloc(ids->table, sizeof(ent) * ids->alloc);
-   }
+   ALLOC_GROW(ids->table, ids->nr + 1, ids->alloc);
if (pos < ids->nr)
memmove(ids->table + pos + 1, ids->table + pos,
sizeof(ent) * (ids->nr - pos));
-- 
1.8.3.2

--
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 v4 11/14] attr.c: use ALLOC_GROW() in handle_attr_line()

Signed-off-by: Dmitry S. Dolzhenko 
---
 attr.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/attr.c b/attr.c
index 8d13d70..734222d 100644
--- a/attr.c
+++ b/attr.c
@@ -338,12 +338,7 @@ static void handle_attr_line(struct attr_stack *res,
a = parse_attr_line(line, src, lineno, macro_ok);
if (!a)
return;
-   if (res->alloc <= res->num_matches) {
-   res->alloc = alloc_nr(res->num_matches);
-   res->attrs = xrealloc(res->attrs,
- sizeof(struct match_attr *) *
- res->alloc);
-   }
+   ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);
res->attrs[res->num_matches++] = a;
 }
 
-- 
1.8.3.2

--
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 v4 06/14] diffcore-rename.c: use ALLOC_GROW()

Use ALLOC_GROW() instead inline code in
locate_rename_dst() and register_rename_src()

Signed-off-by: Dmitry S. Dolzhenko 
---
 diffcore-rename.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 9b4f068..fbf3272 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -38,11 +38,7 @@ static struct diff_rename_dst *locate_rename_dst(struct 
diff_filespec *two,
if (!insert_ok)
return NULL;
/* insert to make it at "first" */
-   if (rename_dst_alloc <= rename_dst_nr) {
-   rename_dst_alloc = alloc_nr(rename_dst_alloc);
-   rename_dst = xrealloc(rename_dst,
- rename_dst_alloc * sizeof(*rename_dst));
-   }
+   ALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc);
rename_dst_nr++;
if (first < rename_dst_nr)
memmove(rename_dst + first + 1, rename_dst + first,
@@ -82,11 +78,7 @@ static struct diff_rename_src *register_rename_src(struct 
diff_filepair *p)
}
 
/* insert to make it at "first" */
-   if (rename_src_alloc <= rename_src_nr) {
-   rename_src_alloc = alloc_nr(rename_src_alloc);
-   rename_src = xrealloc(rename_src,
- rename_src_alloc * sizeof(*rename_src));
-   }
+   ALLOC_GROW(rename_src, rename_src_nr + 1, rename_src_alloc);
rename_src_nr++;
if (first < rename_src_nr)
memmove(rename_src + first + 1, rename_src + first,
-- 
1.8.3.2

--
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 v4 05/14] diff.c: use ALLOC_GROW()

Use ALLOC_GROW() instead inline code in
diffstat_add() and diff_q()

Signed-off-by: Dmitry S. Dolzhenko 
---
 diff.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index e800666..aebdfda 100644
--- a/diff.c
+++ b/diff.c
@@ -1361,11 +1361,7 @@ static struct diffstat_file *diffstat_add(struct 
diffstat_t *diffstat,
 {
struct diffstat_file *x;
x = xcalloc(sizeof (*x), 1);
-   if (diffstat->nr == diffstat->alloc) {
-   diffstat->alloc = alloc_nr(diffstat->alloc);
-   diffstat->files = xrealloc(diffstat->files,
-   diffstat->alloc * sizeof(x));
-   }
+   ALLOC_GROW(diffstat->files, diffstat->nr + 1, diffstat->alloc);
diffstat->files[diffstat->nr++] = x;
if (name_b) {
x->from_name = xstrdup(name_a);
@@ -3965,11 +3961,7 @@ struct diff_queue_struct diff_queued_diff;
 
 void diff_q(struct diff_queue_struct *queue, struct diff_filepair *dp)
 {
-   if (queue->alloc <= queue->nr) {
-   queue->alloc = alloc_nr(queue->alloc);
-   queue->queue = xrealloc(queue->queue,
-   sizeof(dp) * queue->alloc);
-   }
+   ALLOC_GROW(queue->queue, queue->nr + 1, queue->alloc);
queue->queue[queue->nr++] = dp;
 }
 
-- 
1.8.3.2

--
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 v4 02/14] bundle.c: use ALLOC_GROW() in add_to_ref_list()

Signed-off-by: Dmitry S. Dolzhenko 
---
 bundle.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index e99065c..1388a3e 100644
--- a/bundle.c
+++ b/bundle.c
@@ -14,11 +14,7 @@ static const char bundle_signature[] = "# v2 git bundle\n";
 static void add_to_ref_list(const unsigned char *sha1, const char *name,
struct ref_list *list)
 {
-   if (list->nr + 1 >= list->alloc) {
-   list->alloc = alloc_nr(list->nr + 1);
-   list->list = xrealloc(list->list,
-   list->alloc * sizeof(list->list[0]));
-   }
+   ALLOC_GROW(list->list, list->nr + 1, list->alloc);
memcpy(list->list[list->nr].sha1, sha1, 20);
list->list[list->nr].name = xstrdup(name);
list->nr++;
-- 
1.8.3.2

--
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 v4 01/14] builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()

Signed-off-by: Dmitry S. Dolzhenko 
---
 builtin/pack-objects.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..0ffad6f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1213,12 +1213,9 @@ static int check_pbase_path(unsigned hash)
if (0 <= pos)
return 1;
pos = -pos - 1;
-   if (done_pbase_paths_alloc <= done_pbase_paths_num) {
-   done_pbase_paths_alloc = alloc_nr(done_pbase_paths_alloc);
-   done_pbase_paths = xrealloc(done_pbase_paths,
-   done_pbase_paths_alloc *
-   sizeof(unsigned));
-   }
+   ALLOC_GROW(done_pbase_paths,
+  done_pbase_paths_num + 1,
+  done_pbase_paths_alloc);
done_pbase_paths_num++;
if (pos < done_pbase_paths_num)
memmove(done_pbase_paths + pos + 1,
-- 
1.8.3.2

--
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 v4 14/14] sha1_file.c: use ALLOC_GROW() in pretend_sha1_file()

Helped-by: He Sun 
Signed-off-by: Dmitry S. Dolzhenko 
---
 sha1_file.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 019628a..3cb17b8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2624,12 +2624,7 @@ int pretend_sha1_file(void *buf, unsigned long len, enum 
object_type type,
hash_sha1_file(buf, len, typename(type), sha1);
if (has_sha1_file(sha1) || find_cached_object(sha1))
return 0;
-   if (cached_object_alloc <= cached_object_nr) {
-   cached_object_alloc = alloc_nr(cached_object_alloc);
-   cached_objects = xrealloc(cached_objects,
- sizeof(*cached_objects) *
- cached_object_alloc);
-   }
+   ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);
co = &cached_objects[cached_object_nr++];
co->size = len;
co->type = type;
-- 
1.8.3.2

--
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 v4 03/14] cache-tree.c: use ALLOC_GROW() in find_subtree()

Signed-off-by: Dmitry S. Dolzhenko 
---
 cache-tree.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 0bbec43..30149d1 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -75,11 +75,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree 
*it,
return NULL;
 
pos = -pos-1;
-   if (it->subtree_alloc <= it->subtree_nr) {
-   it->subtree_alloc = alloc_nr(it->subtree_alloc);
-   it->down = xrealloc(it->down, it->subtree_alloc *
-   sizeof(*it->down));
-   }
+   ALLOC_GROW(it->down, it->subtree_nr + 1, it->subtree_alloc);
it->subtree_nr++;
 
down = xmalloc(sizeof(*down) + pathlen + 1);
-- 
1.8.3.2

--
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 v4 10/14] dir.c: use ALLOC_GROW() in create_simplify()

Signed-off-by: Dmitry S. Dolzhenko 
---
 dir.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 98bb50f..4ae38e4 100644
--- a/dir.c
+++ b/dir.c
@@ -1341,10 +1341,7 @@ static struct path_simplify *create_simplify(const char 
**pathspec)
 
for (nr = 0 ; ; nr++) {
const char *match;
-   if (nr >= alloc) {
-   alloc = alloc_nr(alloc);
-   simplify = xrealloc(simplify, alloc * 
sizeof(*simplify));
-   }
+   ALLOC_GROW(simplify, nr + 1, alloc);
match = *pathspec++;
if (!match)
break;
-- 
1.8.3.2

--
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 v4 08/14] replace_object.c: use ALLOC_GROW() in register_replace_object()

Signed-off-by: Dmitry S. Dolzhenko 
---
 replace_object.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index cdcaf8c..843deef 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -36,12 +36,8 @@ static int register_replace_object(struct replace_object 
*replace,
return 1;
}
pos = -pos - 1;
-   if (replace_object_alloc <= ++replace_object_nr) {
-   replace_object_alloc = alloc_nr(replace_object_alloc);
-   replace_object = xrealloc(replace_object,
- sizeof(*replace_object) *
- replace_object_alloc);
-   }
+   ALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc);
+   replace_object_nr++;
if (pos < replace_object_nr)
memmove(replace_object + pos + 1,
replace_object + pos,
-- 
1.8.3.2

--
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 v4 00/14] Use ALLOC_GROW() instead of inline code

This version differs from previous [1] the following changes:
  - added three new commits with similar changes in "builtin/mktree.c",
"cache-tree.c" and "sha1_file.c".
  - updated commit messages: "use ALLOC_GROW() in function_name()" instead of
"change function_name() to use ALLOC_GROW()"
  - updated [PATCH v2 01/11] [2] to keep code lines within 80 columns in 
"builtin/pack-objects.c"

Duy Nguyen, Michael Haggerty, Junio C Hamano, Eric Sunshine, and He Sun, 
thanks you very much for your remarks and advices

[1] http://thread.gmane.org/gmane.comp.version-control.git/242919
[2] http://thread.gmane.org/gmane.comp.version-control.git/242920

Dmitry S. Dolzhenko (14):
  builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
  bundle.c: use ALLOC_GROW() in add_to_ref_list()
  cache-tree.c: use ALLOC_GROW() in find_subtree()
  commit.c: use ALLOC_GROW() in register_commit_graft()
  diff.c: use ALLOC_GROW()
  diffcore-rename.c: use ALLOC_GROW()
  patch-ids.c: use ALLOC_GROW() in add_commit()
  replace_object.c: use ALLOC_GROW() in register_replace_object()
  reflog-walk.c: use ALLOC_GROW()
  dir.c: use ALLOC_GROW() in create_simplify()
  attr.c: use ALLOC_GROW() in handle_attr_line()
  builtin/mktree.c: use ALLOC_GROW() in append_to_tree()
  read-cache.c: use ALLOC_GROW() in add_index_entry()
  sha1_file.c: use ALLOC_GROW() in pretend_sha1_file()

 attr.c |  7 +--
 builtin/mktree.c   |  5 +
 builtin/pack-objects.c |  9 +++--
 bundle.c   |  6 +-
 cache-tree.c   |  6 +-
 commit.c   |  8 ++--
 diff.c | 12 ++--
 diffcore-rename.c  | 12 ++--
 dir.c  |  5 +
 patch-ids.c|  5 +
 read-cache.c   |  6 +-
 reflog-walk.c  | 12 ++--
 replace_object.c   |  8 ++--
 sha1_file.c|  7 +--
 14 files changed, 21 insertions(+), 87 deletions(-)

-- 
1.8.3.2

--
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: Help needed: Tests failed While replacing char array with strbuf in bulk-checkin.c

On Mon, Mar 3, 2014 at 4:11 PM, saikrishna.sripada
 wrote:
> I am trying do complete the microproject 4, inorder to apply to GSOC.
> I have made the below changes:
>
> https://gist.github.com/anhsirksai/9334565
>
> Post my changes compilation is succes in the source directory.
> But when I ran the tests[make in t/ directory] my tests are failing saying
>
> "
>  free(): invalid pointer: 0x3630376532353636 ***
> === Backtrace: =
> /lib/x86_64-linux-gnu/libc.so.6(+0x7eb96)[0x2b5f3b540b96]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4fb829]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x47d425]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4064ad]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405a04]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x404cbd]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x2b5f3b4e376d]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405109]
> "
>
> Can some one please help me with the memory allacation and strbuf_release()

Read the microproject text carefully and _fully_. It provides the clue
you need to understand the problem.

Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for
handling packname, and explain why this is useful. Also check if
the first argument of pack-write.c:finish_tmp_packfile() can be
made const.

If, after making a closer examination of the mentioned functions, the
problem still eludes you, ask here again.
--
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] rebase: accept - as another way of saying HEAD~

Matthieu Moy  writes:

> Michael Haggerty  writes:
>
>> Or perhaps "-NUM" should fail with an error message if any of the last
>> NUM commits are merges.  In that restricted scenario (which probably
>> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".
>
> Makes sense to me. So, -NUM would actually mean "rebase the last NUM
> commits" (as well as being an alias for HEAD~NUM), but would fail when
> it does not make sense (with an error message explaining the situation
> and pointing the user to HEAD~N if this is what he wanted).
>
> This would actually be a feature for me: I often want to rebase "recent
> enough" history, and when my @{upstream} isn't well positionned,...

Could you elaborate on this a bit?  What does "isn't well
positioned" mean?  Do you mean "the upstream has advanced but there
is no reason for my topic to build on that---I'd rather want to make
sure I can view 'diff @{1} HEAD' and understand what my changes
before the rebase was"?  That is, what you really want is

git rebase -i --onto $(git merge-base @{upstream} HEAD) @{upstream}

but that is too long to type?

If it is very common (and I suspect it is), we may want to support
such a short-hand---the above does not make any sense without '-i',
but I would say with '-i' you do not want to reBASE on an updated
base most of the time.  "git rebase -i @{upstream}...HEAD" or
something?
--
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] rebase: accept - as another way of saying HEAD~

Michael Haggerty  writes:

> This might be a reason that "-NUM" is a bad idea.
>
> Or perhaps "-NUM" should fail with an error message if any of the last
> NUM commits are merges.  In that restricted scenario (which probably
> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".

That sounds like one possible way out; the opposite would be to
enable mode that preserges merges when we see any.

If "rebase" had a "--first-parent" mode that simply replays only the
commits on the first parent chain, merging the same second and later
parents without looking at their history when dealing with merge
commits, and always used that mode unless "--flatten-history" was
given, the world might have been a better place.  We cannot go there
in a single step, but we could plan such a backward incompatible
migration if we wanted to.
--
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] implemented strbuf_write_or_die()

On Mon, Mar 3, 2014 at 3:35 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano  wrote:
>>> Eric Sunshine  writes:
 It's not obvious from the patch fragment, but 'heads' is not a strbuf,
 so Faiz correctly left this invocation alone.
>>>
>>> That is a very good sign why this change is merely a code-churn and
>>> not an improvement, isn't it?  We know (and any strbuf user should
>>> know) that ->buf and ->len are the ways to learn the pointer and the
>>> length the strbuf holds.  Why anybody thinks it is benefitial to
>>> introduce another function that is _only_ for writing out strbuf and
>>> cannot be used to write out a plain buffer is simply beyond me.
>>
>> As a potential GSoC student and newcomer to the project, Faiz would
>> not have known that this would be considered unwanted churn when he
>> chose the task from the GSoC microproject page [1]. Perhaps it would
>> be a good idea to retire this item from the list?
>
> I don't think I saw this on the microproject suggestion page when I
> last looked at it, and assumed that this was on the student's own
> initiative.

I also had not seen it earlier on the microprojects page and had the
same reaction until I re-checked the page and found that it had been
added [1].

The microprojects page already instructs students to indicate that a
submission is for GSoC [2] (and many have followed the advice), but
perhaps we can avoid this sort of misunderstanding in the future by
making it more explicit: for instance, tell them to add [GSoC] to the
Subject:.

[1]: 
https://github.com/git/git.github.io/commit/f314120a2b5e831459673c612a3630ad953d9954
[2]: 
https://github.com/git/git.github.io/blame/master/SoC-2014-Microprojects.md#L83

>> On the other hand, it did expose Faiz to the iterative code review
>> process on this project and gave him a taste of what would be expected
>> of him as a GSoC student, so the microproject achieved that important
>> goal, and thus wasn't an utter failure.
>>
>> [1]: 
>> https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
>
> Surely.
>
> I would have to say that this is not a good sample exercise to
> suggest to new students and I'd encourage dropping it from the list.
> You could argue that it is an effective way to cull people with bad
> design taste to mix suggestions to make the codebase worse and see
> who picks them, but I do not think it is very fair ;-)

Agreed. The item should be dropped from the list.
--
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

Help needed: Tests failed While replacing char array with strbuf in bulk-checkin.c

2014-03-03 Thread saikrishna.sripada


Hi ,
I am trying do complete the microproject 4, inorder to apply to GSOC.
I have made the below changes:

https://gist.github.com/anhsirksai/9334565

Post my changes compilation is succes in the source directory.
But when I ran the tests[make in t/ directory] my tests are failing 
saying


"
 free(): invalid pointer: 0x3630376532353636 ***
=== Backtrace: =
/lib/x86_64-linux-gnu/libc.so.6(+0x7eb96)[0x2b5f3b540b96]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4fb829]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x47d425]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4064ad]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405a04]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x404cbd]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x2b5f3b4e376d]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405109]
"

Can some one please help me with the memory allacation and 
strbuf_release()


Thanks,
--sai krishna

--
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] git-config: document interactive.singlekey requires Term::ReadKey

2014-03-03 Thread Simon Ruderich

Most distributions don't require Term::ReadKey as dependency, leaving
the user to wonder why the setting doesn't work.

Signed-off-by: Simon Ruderich 
---

On Mon, Mar 03, 2014 at 10:58:58AM -0800, Junio C Hamano wrote:
> Thanks, but is it true that interactive.singlekey "requries"
> Term::ReadKey?

Yes, it requires it. The code also works fine without
Term::ReadKey, but the feature "singlekey" requires this module.
I assumed a user enabling this option would also want to use the
feature, therefore "requires" is fine IMHO.

> The implementation of prompt_single_character sub wants to use
> ReadKey, but can still let the user interact with the program by
> falling back to a cooked input when it is not available, so perhaps
> a better fix might be something like this:
>
> if (!$use_readkey) {
>   print STDERR "missing Term::ReadKey, disabling 
> interactive.singlekey\n";
> }
>
> inside the above if() that prepares $use_readkey?

Good idea. Implemented in an additional patch.

I think the documentation should also be updated (this patch) to
make it clear to a reader of the man page, that an additional
module is required, without having him to try to use the option.

> You also misspelled the package name it seems ;-)

Oops, sorry. Fixed in this reroll.

Regards
Simon

 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f4d793..406a582 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1633,7 +1633,7 @@ interactive.singlekey::
linkgit:git-add[1], linkgit:git-checkout[1], linkgit:git-commit[1],
linkgit:git-reset[1], and linkgit:git-stash[1]. Note that this
setting is silently ignored if portable keystroke input
-   is not available.
+   is not available; requires the Perl module Term::ReadKey.
 
 log.abbrevCommit::
If true, makes linkgit:git-log[1], linkgit:git-show[1], and
-- 
1.9.0.11.g9a08b42

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
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-add--interactive: warn if module for interactive.singlekey is missing

2014-03-03 Thread Simon Ruderich

Suggested-by: Junio C Hamano 
Signed-off-by: Simon Ruderich 
---
 git-add--interactive.perl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 24bb1ab..d3bca12 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -58,6 +58,9 @@ if ($repo->config_bool("interactive.singlekey")) {
Term::ReadKey->import;
$use_readkey = 1;
};
+   if (!$use_readkey) {
+   print STDERR "missing Term::ReadKey, disabling 
interactive.singlekey\n";
+   }
eval {
require Term::Cap;
my $termcap = Term::Cap->Tgetent;
-- 
1.9.0.11.g9a08b42

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
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] implemented strbuf_write_or_die()

Eric Sunshine  writes:

> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano  wrote:
>> Eric Sunshine  writes:
>>
>>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
> diff --git a/remote-curl.c b/remote-curl.c
> index 10cb011..dee8716 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
> discovery *heads)
> if (start_command(&client))
> exit(1);
> if (preamble)
> -   write_or_die(client.in, preamble->buf, preamble->len);
> +   strbuf_write_or_die(client.in, preamble);
> if (heads)
> write_or_die(client.in, heads->buf, heads->len);

 This should be changed. May be you can use Ctrl-F to search write_or_die().
 Or if you are using vim, use "/ and n" to find all.
>>>
>>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
>>> so Faiz correctly left this invocation alone.
>>
>> That is a very good sign why this change is merely a code-churn and
>> not an improvement, isn't it?  We know (and any strbuf user should
>> know) that ->buf and ->len are the ways to learn the pointer and the
>> length the strbuf holds.  Why anybody thinks it is benefitial to
>> introduce another function that is _only_ for writing out strbuf and
>> cannot be used to write out a plain buffer is simply beyond me.
>
> As a potential GSoC student and newcomer to the project, Faiz would
> not have known that this would be considered unwanted churn when he
> chose the task from the GSoC microproject page [1]. Perhaps it would
> be a good idea to retire this item from the list?

I don't think I saw this on the microproject suggestion page when I
last looked at it, and assumed that this was on the student's own
initiative.

> On the other hand, it did expose Faiz to the iterative code review
> process on this project and gave him a taste of what would be expected
> of him as a GSoC student, so the microproject achieved that important
> goal, and thus wasn't an utter failure.
>
> [1]: 
> https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md

Surely.

I would have to say that this is not a good sample exercise to
suggest to new students and I'd encourage dropping it from the list.
You could argue that it is an effective way to cull people with bad
design taste to mix suggestions to make the codebase worse and see
who picks them, but I do not think it is very fair ;-)
--
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] submodule : Add --no-separate-git-dir option to add and update command.

2014-03-03 Thread Henri GEIST

Le lundi 03 mars 2014 à 17:45 +, Jens Lehmann a écrit :
> Am 03.03.2014 14:47, schrieb Henri GEIST:
> > This new option prevent git submodule  to clone the missing
> > submodules with the --separate-git-dir option.
> > Then the submodule will be regular repository and their gitdir will not
> > be placed in the superproject gitdir/modules directory.
> 
> And what is your motivation for this? After all submodules containing
> a .git directory are second class citizens (because they can never be
> safely removed by regular git commands).
>

I recognize most people will prefer to have the .git directory separate.
And I do not intend to make this option the default.

My reasons are:

  - As it is not clearly stated in the doc that the gitdir is separate.
The first time I have copied one module to an USB key I had a big
surprise.

  - This will not change anything for people not using it.

  - I use an other patch which I plane to send later which enable multiple
level of superproject to add a gitlink to the same submodule.
And in this case the superproject containing the separate gitdir will be
arbitrary and depend on the processing order of the
'git submodule update --recursive' command.

  - I have written this for myself and have using it since 2012 and send it in
the hope it could be useful for someone else even if it is only a few
people. But if its not the case no problem I will keep using it for myself.


> > Signed-off-by: Henri GEIST 
> > ---
> >  Documentation/git-submodule.txt |   18 --
> >  git-submodule.sh|   22 --
> >  t/t7400-submodule-basic.sh  |   12 
> >  3 files changed, 48 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/git-submodule.txt 
> > b/Documentation/git-submodule.txt
> > index 21cb59a..303a475 100644
> > --- a/Documentation/git-submodule.txt
> > +++ b/Documentation/git-submodule.txt
> > @@ -10,13 +10,14 @@ SYNOPSIS
> >  
> >  [verse]
> >  'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
> > - [--reference ] [--depth ] [--]  
> > []
> > + [--reference ] [--depth ] 
> > [--no-separate-git-dir]
> > + [--]  []
> >  'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
> >  'git submodule' [--quiet] init [--] [...]
> >  'git submodule' [--quiet] deinit [-f|--force] [--] ...
> >  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
> >   [-f|--force] [--rebase|--merge|--checkout] [--reference 
> > ]
> > - [--depth ] [--recursive] [--] [...]
> > + [--depth ] [--recursive] [--no-separate-git-dir] [--] 
> > [...]
> >  'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) 
> > ]
> >   [commit] [--] [...]
> >  'git submodule' [--quiet] foreach [--recursive] 
> > @@ -107,6 +108,10 @@ is the superproject and submodule repositories will be 
> > kept
> >  together in the same relative location, and only the
> >  superproject's URL needs to be provided: git-submodule will correctly
> >  locate the submodule using the relative URL in .gitmodules.
> > ++
> > +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> > +has normal git repository without the option `--separate-git-dir` pointing
> > +to the modules directory of the superproject gitdir.
> >  
> >  status::
> > Show the status of the submodules. This will print the SHA-1 of the
> > @@ -185,6 +190,10 @@ If the submodule is not yet initialized, and you just 
> > want to use the
> >  setting as stored in .gitmodules, you can automatically initialize the
> >  submodule with the `--init` option.
> >  +
> > +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> > +has normal git repository without the option `--separate-git-dir` pointing
> > +to the modules directory of the superproject gitdir.
> > ++
> >  If `--recursive` is specified, this command will recurse into the
> >  registered submodules, and update any nested submodules within.
> >  +
> > @@ -363,6 +372,11 @@ for linkgit:git-clone[1]'s `--reference` and 
> > `--shared` options carefully.
> > clone with a history truncated to the specified number of revisions.
> > See linkgit:git-clone[1]
> >  
> > +--no-separate-git-dir::
> > +   This option is valid for add and update commands. Specify that missing
> > +   submodules should be clonned as self contain repository without a
> > +   separate gitdir placed in the modules directory of the superproject
> > +   gitdir.
> >  
> >  ...::
> > Paths to submodule(s). When specified this will restrict the command
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index a33f68d..36eaf31 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -5,11 +5,11 @@
> >  # Copyright (c) 2007 Lars Hjemli
> >  
> >  dashless=$(basename "$0" | sed -e 's/-/ /')
> > -USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
> > [--reference ] [--]  []

Re: [PATCH 3/3] rebase: new convenient option to edit a single commit

On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy  wrote:
> "git rebase -e XYZ" is basically the same as
>
> EDITOR="sed -i '1s/pick XYZ/edit XYZ/' $@" \
> git rebase -i XYZ^
>
> In English, it prepares the todo list for you to edit only commit XYZ
> to save your time. The time saving is only significant when you edit a
> lot of commits separately.

Is it correct to single out only "edit" for special treatment? If
allowing "edit" on the command-line, then shouldn't command-line
"reword" also be supported? I, for one, often need to reword a commit
message (or two or three); far more frequently than I need to edit a
commit.

(This is a genuine question about perceived favoritism of "edit", as
opposed to a request to further bloat the interface.)
--
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: RFC GSoC idea: new "git config" features

Jeff King  writes:

> Most callbacks would convert to a query system in a pretty
> straightforward way, but some that have side effects might be tricky.
> Converting them all may be too large for a GSoC project, but I think you
> could do it gradually:
>
>   1. Convert the parser to read into an in-memory representation, but
>  leave git_config() as a wrapper which iterates over it.
>
>   2. Add query functions like config_string_get() above.
>
>   3. Convert callbacks to query functions one by one.
>
>   4. Eventually drop git_config().
>
> A GSoC project could take us partway through (3).

I actually discarded the "read from these config files to preparsed
structure to memory, later to be consumed by repeated calls to the
git_config() callback functions, making the only difference from the
current scheme that the preparsed structure will be reset when there
is the new 'reset to the original' definition" as obvious and
uninteresting.

This is one of these times that I find myself blessed with capable
others that can go beyond, building on top of such an idea that I
may have discarded without thinking it through, around me ;-)

Yes, the new abstraction like config__get() that can live
alongside the existing "git_config() feeds callback chain
everything" and gradually replace the latter, would be a good way
forward.  Given that we read configuration multiple times anyway for
different purposes, even without the new abstraction, the end result
might perform better if we read the files once and reused in later
calls to git_config().

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] repack: add `repack.honorpackkeep` config var

2014-03-03 Thread Jeff King

On Mon, Mar 03, 2014 at 11:51:06AM -0800, Junio C Hamano wrote:

> > Yes. Do you need a re-roll from me? I think the last version I sent +
> > the squash to tie the default to bitmap-writing makes the most sense.
> 
> I have 9e20b390 (repack: add `repack.packKeptObjects` config var,
> 2014-02-26); I do not recall I've squashed anything into it, though.
> 
> Do you mean this one?
> 
> Here's the interdiff for doing the fallback:
> [...]

Yes. Though I just noticed that the commit message needs updating if
that is squashed in. Here is the whole patch, with that update.

And I am dropping Vicent as the author, since I think there are now
literally zero lines of his left. ;)

-- >8 --
Subject: [PATCH] repack: add `repack.packKeptObjects` config var

The git-repack command always passes `--honor-pack-keep`
to pack-objects. This has traditionally been a good thing,
as we do not want to duplicate those objects in a new pack,
and we are not going to delete the old pack.

However, when bitmaps are in use, it is important for a full
repack to include all reachable objects, even if they may be
duplicated in a .keep pack. Otherwise, we cannot generate
the bitmaps, as the on-disk format requires the set of
objects in the pack to be fully closed.

Even if the repository does not generally have .keep files,
a simultaneous push could cause a race condition in which a
.keep file exists at the moment of a repack. The repack may
try to include those objects in one of two situations:

  1. The pushed .keep pack contains objects that were
 already in the repository (e.g., blobs due to a revert of
 an old commit).

  2. Receive-pack updates the refs, making the objects
 reachable, but before it removes the .keep file, the
 repack runs.

In either case, we may prefer to duplicate some objects in
the new, full pack, and let the next repack (after the .keep
file is cleaned up) take care of removing them.

This patch introduces both a command-line and config option
to disable the `--honor-pack-keep` option.  By default, it
is triggered when pack.writeBitmaps (or `--write-bitmap-index`
is turned on), but specifying it explicitly can override the
behavior (e.g., in cases where you prefer .keep files to
bitmaps, but only when they are present).

Note that this option just disables the pack-objects
behavior. We still leave packs with a .keep in place, as we
do not necessarily know that we have duplicated all of their
objects.

Signed-off-by: Jeff King 
---
 Documentation/config.txt |  7 +++
 Documentation/git-repack.txt |  8 
 builtin/repack.c | 13 -
 t/t7700-repack.sh| 18 +-
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index becbade..a8ddc7f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2136,6 +2136,13 @@ repack.usedeltabaseoffset::
"false" and repack. Access from old Git versions over the
native protocol are unaffected by this option.

+repack.packKeptObjects::
+   If set to true, makes `git repack` act as if
+   `--pack-kept-objects` was passed. See linkgit:git-repack[1] for
+   details. Defaults to `false` normally, but `true` if a bitmap
+   index is being written (either via `--write-bitmap-index` or
+   `pack.writeBitmaps`).
+
 rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
resulting contents after it cleanly resolves conflicts using
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 002cfd5..4786a78 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -117,6 +117,14 @@ other objects in that pack they already have locally.
must be able to refer to all reachable objects. This option
overrides the setting of `pack.writebitmaps`.

+--pack-kept-objects::
+   Include objects in `.keep` files when repacking.  Note that we
+   still do not delete `.keep` packs after `pack-objects` finishes.
+   This means that we may duplicate objects, but this makes the
+   option safe to use when there are concurrent pushes or fetches.
+   This option is generally only useful if you are writing bitmaps
+   with `-b` or `pack.writebitmaps`, as it ensures that the
+   bitmapped packfile has the necessary objects.

 Configuration
 -
diff --git a/builtin/repack.c b/builtin/repack.c
index 49f5857..6b0b62d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,6 +9,7 @@
 #include "argv-array.h"

 static int delta_base_offset = 1;
+static int pack_kept_objects = -1;
 static char *packdir, *packtmp;

 static const char *const git_repack_usage[] = {
@@ -22,6 +23,10 @@ static int repack_config(const char *var, const char *value, 
void *cb)
delta_base_offset = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "repack.packkeptob

Re: [PATCH 2/2] branch: use skip_prefix

Jeff King  writes:

> On Fri, Feb 28, 2014 at 12:04:19PM +0900, Brian Gesiak wrote:
>
>> From: modocache 
>
> Both your emailed patches have this, which is due to your author name
> not matching your sending identity. You probably want to set user.name,
> or if you already have (which it looks like you might have from your
> Signed-off-by), use "git commit --amend --reset-author" to update the
> author information.
>
>> The install_branch_config function reimplemented the skip_prefix
>> function inline. Use skip_prefix function instead for brevity.
>> 
>> Signed-off-by: Brian Gesiak 
>> Reported-by: Michael Haggerty 
>
> It's a minor thing, but usually these footer lines try to follow a
> chronological order. So the report would come before the signoff (and a
> further signoff from the maintainer would go after yours).
>
>> diff --git a/branch.c b/branch.c
>> index 723a36b..e163f3c 100644
>> --- a/branch.c
>> +++ b/branch.c
>> [...]
>
> The patch itself looks OK to me.
>
> -Peff

Thanks.  Queued and pushed out on 'pu' with fixups already.
--
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] commit.c:record_author_date() use skip_prefix() instead of starts_with()

On Mon, Mar 3, 2014 at 1:40 PM, Junio C Hamano  wrote:
> Michael Haggerty  writes:
>
>>> -if (!starts_with(buf, "author ")) {
>>> +if (!skip_prefix(buf, "author ")) {
>>
>> If this is the only change, there is not much point, is there?  How does
>> this help?  Perhaps there is some way to take advantage of the
>> difference between starts_with() and skip_prefix() to simplify the rest
>> of the function?
>
> I admit I lost track, but wasn't there a discussion to use
> starts_with/ends_with when appropriate (namely, the caller is
> absolutely not interested in what the remainder of the string is
> after skipping the prefix), moving away from skip_prefix()?  Isn't
> this change going in the wrong direction?

Yes, it would be going in the wrong direction if this was all there
was to it, but the particular GSoC microproject [1] which inspired
this (incomplete) submission expects that the potential student will
dig deeper and discover how skip_prefix() can be used to achieve
greater simplification in record_author_date() and in other places in
the same file.

[1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
--
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] implemented strbuf_write_or_die()

Eric Sunshine  writes:

> As a potential GSoC student and newcomer to the project, Faiz would
> not have known that this would be considered unwanted churn when he
> chose the task from the GSoC microproject page [1]. Perhaps it would
> be a good idea to retire this item from the list?
>
> On the other hand, it did expose Faiz to the iterative code review
> process on this project and gave him a taste of what would be expected
> of him as a GSoC student, so the microproject achieved that important
> goal, and thus wasn't an utter failure.

And the microproject has the fabulous property that we can use it over
and over again to have a newcomer try committing patches: the previously
reported problem that we were running out of microprojects will not
occur when every patch is eventually going to be rejected.

Joking aside, this is a motivational disaster.  It should be retired.

-- 
David Kastrup
--
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] repack: add `repack.honorpackkeep` config var

Jeff King  writes:

> On Mon, Mar 03, 2014 at 10:13:47AM -0800, Junio C Hamano wrote:
>
>> > Or the flip side: if the user wants to use .keep, we should drop
>> > bitmaps. My point is that we do not know which way the user wants to
>> > go, so we should not tie the options together.
>> 
>> Hmph.  I think the short of your later explanation is "global config
>> may tell us to use bitmap, in which case we would need a way to
>> defeat that and have existing .keep honored, and it makes it easier
>> to do so if these two are kept separate, because you do not want to
>> run around and selectively disable bitmaps in these repositories.
>> We can instead do so with repack.packKeptObjects in the global
>> configuration." and I tend to agree with the reasoning.
>
> Yes. Do you need a re-roll from me? I think the last version I sent +
> the squash to tie the default to bitmap-writing makes the most sense.

I have 9e20b390 (repack: add `repack.packKeptObjects` config var,
2014-02-26); I do not recall I've squashed anything into it, though.

Do you mean this one?

Here's the interdiff for doing the fallback:

---
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3a3d84f..a8ddc7f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::
 repack.packKeptObjects::
If set to true, makes `git repack` act as if
`--pack-kept-objects` was passed. See linkgit:git-repack[1] for
-   details. Defaults to false.
+   details. Defaults to `false` normally, but `true` if a bitmap
+   index is being written (either via `--write-bitmap-index` or
+   `pack.writeBitmaps`).
 
 rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
diff --git a/builtin/repack.c b/builtin/repack.c
index 49947b2..6b0b62d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,7 +9,7 @@
 #include "argv-array.h"
 
 static int delta_base_offset = 1;
-static int pack_kept_objects;
+static int pack_kept_objects = -1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, builtin_repack_options,
git_repack_usage, 0);
 
+   if (pack_kept_objects < 0)
+   pack_kept_objects = write_bitmap;
+
packdir = mkpathdup("%s/pack", get_object_directory());
packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
 
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f8431a8..b1eed5c 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not 
repacked' '
objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |
sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") &&
mv pack-* .git/objects/pack/ &&
-   git repack -A -d -l &&
+   git repack --no-pack-kept-objects -A -d -l &&
git prune-packed &&
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
@@ -35,9 +35,9 @@ test_expect_success 'objects in packs marked .keep are not 
repacked' '
test -z "$found_duplicate_object"
 '
 
-test_expect_success '--pack-kept-objects duplicates objects' '
+test_expect_success 'writing bitmaps duplicates .keep objects' '
# build on $objsha1, $packsha1, and .keep state from previous
-   git repack -Adl --pack-kept-objects &&
+   git repack -Adl &&
test_when_finished "found_duplicate_object=" &&
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
--
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] implemented strbuf_write_or_die()

On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
 diff --git a/remote-curl.c b/remote-curl.c
 index 10cb011..dee8716 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
 discovery *heads)
 if (start_command(&client))
 exit(1);
 if (preamble)
 -   write_or_die(client.in, preamble->buf, preamble->len);
 +   strbuf_write_or_die(client.in, preamble);
 if (heads)
 write_or_die(client.in, heads->buf, heads->len);
>>>
>>> This should be changed. May be you can use Ctrl-F to search write_or_die().
>>> Or if you are using vim, use "/ and n" to find all.
>>
>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
>> so Faiz correctly left this invocation alone.
>
> That is a very good sign why this change is merely a code-churn and
> not an improvement, isn't it?  We know (and any strbuf user should
> know) that ->buf and ->len are the ways to learn the pointer and the
> length the strbuf holds.  Why anybody thinks it is benefitial to
> introduce another function that is _only_ for writing out strbuf and
> cannot be used to write out a plain buffer is simply beyond me.

As a potential GSoC student and newcomer to the project, Faiz would
not have known that this would be considered unwanted churn when he
chose the task from the GSoC microproject page [1]. Perhaps it would
be a good idea to retire this item from the list?

On the other hand, it did expose Faiz to the iterative code review
process on this project and gave him a taste of what would be expected
of him as a GSoC student, so the microproject achieved that important
goal, and thus wasn't an utter failure.

[1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
--
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] commit.c: Use skip_prefix() instead of starts_with()

Tanay Abhra  writes:

> In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of
> starts_with() is more elegant and abstracts away the details.

Avoid subjective judgement like "more elegant" when justifying your
change; you are not your own judge.

The caller of starts_with() actually can use the string that follows
the expected prefix and that is the reason why using skip_prefix()
in these places is a good idea.  There is no need to be subjective
to justify that change.

I do not think there is any more abstracting away of the details in
this change.  The updated uses a different and more suitable
abstraction than the original.

> diff --git a/commit.c b/commit.c
> index 6bf4fe0..668c703 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
>  static void record_author_date(struct author_date_slab *author_date,
>  struct commit *commit)
>  {
> - const char *buf, *line_end;
> + const char *buf, *line_end, *skip;
>   char *buffer = NULL;
>   struct ident_split ident;
>   char *date_end;
> @@ -566,14 +566,15 @@ static void record_author_date(struct author_date_slab 
> *author_date,
>buf;
>buf = line_end + 1) {
>   line_end = strchrnul(buf, '\n');
> - if (!starts_with(buf, "author ")) {
> + if (!(skip = skip_prefix(buf, "author "))) {

We tend to avoid assignments in conditionals.

>   if (!line_end[0] || line_end[1] == '\n')
>   return; /* end of header */
>   continue;
>   }
> + buf = skip;
>   if (split_ident_line(&ident,
> -  buf + strlen("author "),
> -  line_end - (buf + strlen("author "))) ||
> +  buf,
> +  line_end - buf) ||
>   !ident.date_begin || !ident.date_end)
>   goto fail_exit; /* malformed "author" line */
>   break;

If you give a sensible name to what 'buf + strlen("author ")' is,
then the result becomes a lot more readable compared to the
original, and I think that is what this change is about.

And "skip" is not a good name for that.  'but + strlen("author ")'
is what split_ident_line() expects its input to be split; let's
tentatively call it "ident_line" and see what the call looks like:

split_ident_line(&ident, ident_line, line_end - ident_line))

And that is what we want to see here.  It is a bit more clear than
the original that we are splitting the ident information on the line,
ident_line (you could call it ident_begin) points at the beginning
and line_end points at the end of that ident information.

Use of skip_prefix(), which I am sure you took the name of the new
variable "skip" from, is merely an implementation detail of finding
where the ident begins.  A good rule of thumb to remember is to name
things after what they are, not how you obtain them, how they are
used or what they are used for/as.

> @@ -1193,9 +1194,9 @@ static void parse_gpg_output(struct signature_check 
> *sigc)
>   for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>   const char *found, *next;
>  
> - if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
> + if (found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) 
> {
>   /* At the very beginning of the buffer */
> - found = buf + strlen(sigcheck_gpg_status[i].check + 1);
> + ;
>   } else {
>   found = strstr(buf, sigcheck_gpg_status[i].check);
>   if (!found)

This hunk looks good.  It can be a separate patch but they are both
minor changes so it is OK to have it in a single 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] submodule : Add --no-separate-git-dir option to add and update command.

[CC'ing the submodule area experts.]

Henri GEIST  writes:

> This new option prevent git submodule  to clone the missing
> submodules with the --separate-git-dir option.
> Then the submodule will be regular repository and their gitdir will not
> be placed in the superproject gitdir/modules directory.
>
> Signed-off-by: Henri GEIST 
> ---

Thanks.

The above describes what the new option does, but does not explain
why the new option is a good idea in the first place.

Given that we used to directly clone into the superproject's working
tree like this patch does, realized that it was a very bad idea and
are trying to move to the direction of keeping it in modules/
subdirectory of the superproject's .git directory, there needs to be
a very good explanation to justify why this "going backwards" is
sometimes a desirable 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] repack: add `repack.honorpackkeep` config var

2014-03-03 Thread Shawn Pearce

On Fri, Feb 28, 2014 at 10:05 PM, Jeff King  wrote:
> On Fri, Feb 28, 2014 at 10:09:08AM -0700, Nasser Grainawi wrote:
>
>> > Exactly. The two features (bitmaps and .keep) are not compatible with
>> > each other, so you have to prioritize one. If you are using static .keep
>> > files, you might want them to continue being respected at the expense of
>> > using bitmaps for that repo. So I think you want a separate option from
>> > --write-bitmap-index to allow the appropriate flexibility.
>>
>> Has anyone thought about how to make them compatible?
>
> Yes, but it's complicated and not likely to happen soon.
>
> Having .keep files means that you are not including some objects in the
> newly created pack. Each bit in a commit's bitmap corresponds to one
> object in the pack, and whether it is reachable from that commit. The
> bitmap is only useful if we can calculate the full reachability from it,
> and it has no way to specify objects outside of the pack.
>
> To fix this, you would need to change the on-disk format of the bitmaps
> to somehow reference objects outside of the pack. Either by having the
> bitmaps index a repo-global set of objects, or by permitting a list of
> "edge" objects that are referenced from the pack, but not included (and
> then when assembling the full reachable list, you would have to recurse
> across "edge" objects to find their reachable list in another pack,
> etc).
>
> So it's possible, but it would complicate the scheme quite a bit, and
> would not be backwards compatible with either JGit or C Git.

Colby Ranger always wanted to add this to the bitmap scheme. Construct
a partial pack bitmap on a partial pack of "recent" objects, with edge
pointers naming objects that are not in this pack but whose closures
need to be considered part of the bitmap. Its complicated in-memory
because you need to fuse together two or more bitmaps (the partial
pack one, and the larger historical kept pack) before running the
"want AND NOT have" computation.

Colby did not find time to work on this in JGit, so it just didn't get
implemented. But we did consider it, as the servers at Google we built
bitmap for use a multi-level pack scheme and don't want to rebuild
packs all of the time.

>> We're using Martin Fick's git-exproll script which makes heavy use of
>> keeps to reduce pack file churn. In addition to the on-disk benefits
>> we get there, the driving factor behind creating exproll was to
>> prevent Gerrit from having two large (30GB+) mostly duplicated pack
>> files open in memory at the same time. Repacking in JGit would help in
>> a single-master environment, but we'd be back to having this problem
>> once we go to a multi-master setup.
>>
>> Perhaps the solution here is actually something in JGit where it could
>> aggressively try to close references to pack files
>
> In C git we don't worry about this too much, because our programs tend
> to be short-lived, and references to the old pack will go away quickly.
> Plus it is all mmap'd, so as we simply stop accessing the pages of the
> old pack, they should eventually be dropped if there is memory pressure.
>
> I seem to recall that JGit does not mmap its packfiles. Does it pread?

JGit does not mmap because you can't munmap() until the Java GC gets
around to freeing the tiny little header object that contains the
memory address of the start of the mmap segment. This can take ages,
to the point where you run out of virtual address space in the process
and s**t starts to fail left and right inside of the JVM. The GC is
just unable to prioritize finding those tiny headers and getting them
out of the heap so the munmap can take place safely.

So yea, JGit does pread() for the blocks but it holds those in its own
buffer cache inside of the Java heap. Where a 4K disk block is a 4K
memory array that puts pressure on the GC to actually wake up and free
resources that are unused. What Nasser is talking about is JGit may
take a long time to realize one pack is unused and start kicking those
blocks out of its buffer cache. Those blocks are reference counted and
the file descriptor JGit preads from is held open so long as at least
one block is in the buffer cache. By keeping the file open we force
the filesystem to keep the inode alive a lot longer, which means the
disk needs a huge amount of free space to store the unlinked but still
open 30G pack files from prior GC generations.

> In that case, I'd expect unused bits from the duplicated packfile to get
> dropped from the disk cache over time. If it loads whole packfiles into
> memory, then yes, it should probably close more aggressively.

Its more than that, its the inode being kept alive by the open file
descriptor...
--
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 00/11] Use ALLOC_GROW() instead of inline code

Eric Sunshine  writes:

> On Mon, Mar 3, 2014 at 2:18 AM, Dmitry S. Dolzhenko
>  wrote:
>> Dmitry S. Dolzhenko (11):
>>   builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
>>   bundle.c: use ALLOC_GROW() in add_to_ref_list()
>>   cache-tree.c: use ALLOC_GROW() in find_subtree()
>>   commit.c: use ALLOC_GROW() in register_commit_graft()
>>   diff.c: use ALLOC_GROW()
>>   diffcore-rename.c: use ALLOC_GROW()
>>   patch-ids.c: use ALLOC_GROW() in add_commit()
>>   replace_object.c: use ALLOC_GROW() in register_replace_object()
>>   reflog-walk.c: use ALLOC_GROW()
>>   dir.c: use ALLOC_GROW() in create_simplify()
>>   attr.c: use ALLOC_GROW() in handle_attr_line()
>>
>>  attr.c |  7 +--
>>  builtin/pack-objects.c |  9 +++--
>>  bundle.c   |  6 +-
>>  cache-tree.c   |  6 +-
>>  commit.c   |  8 ++--
>>  diff.c | 12 ++--
>>  diffcore-rename.c  | 12 ++--
>>  dir.c  |  5 +
>>  patch-ids.c|  5 +
>>  reflog-walk.c  | 12 ++--
>>  replace_object.c   |  8 ++--
>>  11 files changed, 18 insertions(+), 72 deletions(-)
>>
>> --
>> 1.8.5.3
>>
>> This version differs from previous only minor changes:
>>   - update commit messages
>>   - keep code lines within 80 columns
>
> Place this commentary at the top of the cover letter since that's
> where people look for it.
>
> You want to ease the reviewer's job as much as possible, so it helps
> to link to the previous submission, like this [1].
>
> Likewise, you can help the reviewer by being more specific about how
> you updated the commit messages (and perhaps by linking to the
> relevant discussion points, like this [2][3]).
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/242857
> [2]: http://article.gmane.org/gmane.comp.version-control.git/243004
> [3]: http://article.gmane.org/gmane.comp.version-control.git/243049

It would be helpful for people to also pay attention to what is
pushed out on 'pu' ;-)

--
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] skip_prefix: rewrite so that prefix is scanned once

Siddharth Goel  writes:

> Helped-by: Eric Sunshine 
> Signed-off-by: Siddharth Goel 
> ---
> Added a space after colon in the subject as compared to previous 
> patch [PATCH v2].
>
> [PATCH v2]: http://thread.gmane.org/gmane.comp.version-control.git/243150

Whenever you see "Change", "Rewrite", etc. in the subject of a patch
that touches existing code, think twice.  The subject line is a
scarce real estate to be wasted on a noiseword that carries no real
information, and we already know a patch that touches existing code
changes or rewrites things.

Subject: [PATCH v3] skip_prefix: scan prefix only once

perhaps?

>  git-compat-util.h | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 614a5e9..550dce3 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char 
> *suffix);
>  
>  static inline const char *skip_prefix(const char *str, const char *prefix)
>  {
> - size_t len = strlen(prefix);
> - return strncmp(str, prefix, len) ? NULL : str + len;
> + while (*prefix != '\0' && *str == *prefix) {
> + str++;
> + prefix++;
> + }
> + return (*prefix == '\0' ? str : NULL);

Unlike another patch I saw the other day on the same topic, this
checks *prefix twice for the last round, even though I think this
one is probably slightly easier to read.  I dunno.

--
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-config: document interactive.singlekey requires Term::Readkey

Simon Ruderich  writes:

> Most distributions don't require Term::Readkey as dependency,
> leaving the user to wonder why the setting doesn't work.
>
> Signed-off-by: Simon Ruderich 

Thanks, but is it true that interactive.singlekey "requries"
Term::ReadKey?

The relevant part of git-add--interactive reads like so:

if ($repo->config_bool("interactive.singlekey")) {
eval {
require Term::ReadKey;
Term::ReadKey->import;
$use_readkey = 1;
};
eval {
require Term::Cap;
my $termcap = Term::Cap->Tgetent;
foreach (values %$termcap) {
$term_escapes{$_} = 1 if /^\e/;
}
$use_termcap = 1;
};
}

The implementation of prompt_single_character sub wants to use
ReadKey, but can still let the user interact with the program by
falling back to a cooked input when it is not available, so perhaps
a better fix might be something like this:

if (!$use_readkey) {
print STDERR "missing Term::ReadKey, disabling 
interactive.singlekey\n";
}

inside the above if() that prepares $use_readkey?

You also misspelled the package name it seems ;-)
--
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] branch.c: change install_branch_config() to use skip_prefix()

Eric Sunshine  writes:

>> diff --git a/branch.c b/branch.c
>> index 723a36b..ca4e824 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -50,7 +50,7 @@ static int should_setup_rebase(const char *origin)
>>  void install_branch_config(int flag, const char *local, const char *origin, 
>> const char *remote)
>>  {
>> const char *shortname = remote + 11;
>> -   int remote_is_branch = starts_with(remote, "refs/heads/");
>> +   int remote_is_branch = (NULL != skip_prefix(remote ,"refs/heads/"));
>
> This actually makes the code more difficult to read and understand.
> There's a more fundamental reason to use skip_prefix() here. See if
> you can figure it out. (Hint: shortname)

I've already queued 0630aa49 (branch: use skip_prefix() in
install_branch_config(), 2014-02-28) on this topic, by the way.

--
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

Fwd: [PATCH] implemented strbuf_write_or_die()

2014-03-03 Thread Faiz Kothari

On Tue, Mar 4, 2014 at 12:01 AM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
 diff --git a/remote-curl.c b/remote-curl.c
 index 10cb011..dee8716 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
 discovery *heads)
 if (start_command(&client))
 exit(1);
 if (preamble)
 -   write_or_die(client.in, preamble->buf, preamble->len);
 +   strbuf_write_or_die(client.in, preamble);
 if (heads)
 write_or_die(client.in, heads->buf, heads->len);
>>>
>>> This should be changed. May be you can use Ctrl-F to search write_or_die().
>>> Or if you are using vim, use "/ and n" to find all.
>>
>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
>> so Faiz correctly left this invocation alone.
>
> That is a very good sign why this change is merely a code-churn and
> not an improvement, isn't it?  We know (and any strbuf user should
> know) that ->buf and ->len are the ways to learn the pointer and the
> length the strbuf holds.  Why anybody thinks it is benefitial to
> introduce another function that is _only_ for writing out strbuf and
> cannot be used to write out a plain buffer is simply beyond me.
>
Hi,
Thanks for the feedback. Yes, I do realize, its kind of a code churn.
I didn't realize it until I looked at the sign you pointed out.
But it was a good exercise to go through the code as this is one of
the GSoC microprojects.
Sorry, it didn't turn out to be a beneficial one. My bad.

Thanks a lot again for the suggestions and feedback.

-Faiz
--
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] write_pack_file: use correct variable in diagnostic

Sun He  writes:

> 'pack_tmp_name' is the subject of the utime() check, so report it in the
> warning, not the uninitialized 'tmpname'
>
> Signed-off-by: Sun He 
> ---
>
>  Changing the subject and adding valid information as tutored by 
>  Eric Sunshine.
>  Thanks to him.
>
>  builtin/pack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index c733379..4922ce5 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -823,7 +823,7 @@ static void write_pack_file(void)
>   utb.modtime = --last_mtime;
>   if (utime(pack_tmp_name, &utb) < 0)
>   warning("failed utime() on %s: %s",
> - tmpname, strerror(errno));
> + pack_tmp_name, strerror(errno));
>   }
>  
>   /* Enough space for "-.pack"? */

Very nicely done.  Thanks.

And big Thanks to Eric guiding this patch through.

--
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] commit.c:record_author_date() use skip_prefix() instead of starts_with()

Michael Haggerty  writes:

>> -if (!starts_with(buf, "author ")) {
>> +if (!skip_prefix(buf, "author ")) {
>
> If this is the only change, there is not much point, is there?  How does
> this help?  Perhaps there is some way to take advantage of the
> difference between starts_with() and skip_prefix() to simplify the rest
> of the function?

I admit I lost track, but wasn't there a discussion to use
starts_with/ends_with when appropriate (namely, the caller is
absolutely not interested in what the remainder of the string is
after skipping the prefix), moving away from skip_prefix()?  Isn't
this change going in the wrong direction?
--
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

Rewriting git history during git-svn conversion

2014-03-03 Thread Robert Dailey

Hi,

I am converting my team's SVN server to GIT but they aren't ready to
transition yet. People are still working out of SVN, so I need to keep
the git-svn clone around to do 'git svn fetch' to continue to keep it
synchronized with SVN.

Once everyone is ready to switch, and after converting all of the
tag-branches to real GIT tags, I plan to push all branches & tags to
the new central git repository.

However, before pushing to the central GIT repo, I want to remove some
giant directories from the repository history. Specifically some third
party library directories. I have found a way to do this here:
http://stackoverflow.com/questions/10067848/remove-folder-and-its-contents-from-git-githubs-history

Is it safe to do this while still using git svn fetch? Will it
properly continue to convert SVN commits on top of my rewritten
history? If not, what changes can I make after I run the commands
linked by the URL above so that git svn continues to work normally?

Note that I plan to delete the third party libraries from SVN side and
then pull down the changes to git through git-svn, then at that point
clean it up (hopefully the git filter-branch command ignores the
commit that deleted the files and only hunts down the commits where
they were added or modified).

Any guidance on this would be much appreciated.
--
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: An idea for "git bisect" and a GSoC enquiry

Jacopo Notarstefano  writes:

> Here my proposal differs in that I have no way of knowing which label
> is good and which label is bad: I blindly accept two distinct labels
> and bisect with those. I gave an example of this behaviour above.

I think you fundamentally cannot use two labels that are merely
"distinct" and bisect correctly.  You need to know which ones
(i.e. good) are to be excluded and the other (i.e. bad) are to be
included when computing the "remaining to be tested" set of commits.
--
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] implemented strbuf_write_or_die()

Eric Sunshine  writes:

> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
>>> diff --git a/remote-curl.c b/remote-curl.c
>>> index 10cb011..dee8716 100644
>>> --- a/remote-curl.c
>>> +++ b/remote-curl.c
>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
>>> discovery *heads)
>>> if (start_command(&client))
>>> exit(1);
>>> if (preamble)
>>> -   write_or_die(client.in, preamble->buf, preamble->len);
>>> +   strbuf_write_or_die(client.in, preamble);
>>> if (heads)
>>> write_or_die(client.in, heads->buf, heads->len);
>>
>> This should be changed. May be you can use Ctrl-F to search write_or_die().
>> Or if you are using vim, use "/ and n" to find all.
>
> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
> so Faiz correctly left this invocation alone.

That is a very good sign why this change is merely a code-churn and
not an improvement, isn't it?  We know (and any strbuf user should
know) that ->buf and ->len are the ways to learn the pointer and the
length the strbuf holds.  Why anybody thinks it is benefitial to
introduce another function that is _only_ for writing out strbuf and
cannot be used to write out a plain buffer is simply beyond 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

Re: [PATCH] implemented strbuf_write_or_die()

Faiz Kothari  writes:

> Signed-off-by: Faiz Kothari 
> ---
> Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places
> to substitute write_or_die(). I spotted other places where strbuf can be used
> in place of buf[MAX_PATH] but that would require a change in prototype of a 
> lot of functions and functions calling them and so on
> I'll look for more places where strbuf can be used safely.
>
> Thanks.
>
>  builtin/cat-file.c |2 +-
>  builtin/notes.c|4 ++--
>  builtin/receive-pack.c |2 +-
>  builtin/send-pack.c|2 +-
>  builtin/stripspace.c   |2 +-
>  builtin/tag.c  |2 +-
>  bundle.c   |2 +-
>  cache.h|1 +
>  credential-store.c |2 +-
>  fetch-pack.c   |2 +-
>  http-backend.c |2 +-
>  remote-curl.c  |8 +---
>  write_or_die.c |9 +
>  13 files changed, 26 insertions(+), 14 deletions(-)

It does not reduce the code, it does not make the resulting code
read any easier.

What is the benefit of this change?
--
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 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()

Jeff King  writes:

> I realize that I just bikeshedded on subject lines for half a page, and
> part of me wants to go kill myself in shame. But I feel like I see the
> technique misapplied often enough that maybe some guidance is merited.

Thanks.  What I queued read like these:

$ git shortlog ..dd/use-alloc-grow

Dmitry S. Dolzhenko (11):
  builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
  bundle.c: use ALLOC_GROW() in add_to_ref_list()
  cache-tree.c: use ALLOC_GROW() in find_subtree()
  commit.c: use ALLOC_GROW() in register_commit_graft()
  diff.c: use ALLOC_GROW() instead of inline code
  diffcore-rename.c: use ALLOC_GROW() instead of inline code
  patch-ids.c: use ALLOC_GROW() in add_commit()
  replace_object.c: use ALLOC_GROW() in register_replace_object()
  reflog-walk.c: use ALLOC_GROW() instead of inline code
  dir.c: use ALLOC_GROW() in create_simplify()
  attr.c: use ALLOC_GROW() in handle_attr_line()

but I tend to agree with you that we can just stop at "use ALLOC_GROW"
after the filename.
--
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 00/11] Use ALLOC_GROW() instead of inline code

Michael Haggerty  writes:

> On 02/28/2014 10:29 AM, Dmitry S. Dolzhenko wrote:
>> Thank you for your remarks. In this patch I tried to take them into account.
>> 
>> Dmitry S. Dolzhenko (11):
>>   builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
>>   bundle.c: change add_to_ref_list() to use ALLOC_GROW()
>>   cache-tree.c: change find_subtree() to use ALLOC_GROW()
>>   commit.c: change register_commit_graft() to use ALLOC_GROW()
>>   diff.c: use ALLOC_GROW() instead of inline code
>>   diffcore-rename.c: use ALLOC_GROW() instead of inline code
>>   patch-ids.c: change add_commit() to use ALLOC_GROW()
>>   replace_object.c: change register_replace_object() to use ALLOC_GROW()
>>   reflog-walk.c: use ALLOC_GROW() instead of inline code
>>   dir.c: change create_simplify() to use ALLOC_GROW()
>>   attr.c: change handle_attr_line() to use ALLOC_GROW()
>> 
>>  attr.c |  7 +--
>>  builtin/pack-objects.c |  7 +--
>>  bundle.c   |  6 +-
>>  cache-tree.c   |  6 +-
>>  commit.c   |  8 ++--
>>  diff.c | 12 ++--
>>  diffcore-rename.c  | 12 ++--
>>  dir.c  |  5 +
>>  patch-ids.c|  5 +
>>  reflog-walk.c  | 13 +++--
>>  replace_object.c   |  8 ++--
>>  11 files changed, 17 insertions(+), 72 deletions(-)
>
> Everything looks fine to me.  Assuming the test suite ran 100%,
>
> Acked-by: Michael Haggerty 

Looked good (modulo titles, which I think we already discussed),
and queued on 'pu'.

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] repack: add `repack.honorpackkeep` config var

2014-03-03 Thread Jeff King

On Mon, Mar 03, 2014 at 10:13:47AM -0800, Junio C Hamano wrote:

> > Or the flip side: if the user wants to use .keep, we should drop
> > bitmaps. My point is that we do not know which way the user wants to
> > go, so we should not tie the options together.
> 
> Hmph.  I think the short of your later explanation is "global config
> may tell us to use bitmap, in which case we would need a way to
> defeat that and have existing .keep honored, and it makes it easier
> to do so if these two are kept separate, because you do not want to
> run around and selectively disable bitmaps in these repositories.
> We can instead do so with repack.packKeptObjects in the global
> configuration." and I tend to agree with the reasoning.

Yes. Do you need a re-roll from me? I think the last version I sent +
the squash to tie the default to bitmap-writing makes the most sense.

-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] repack: add `repack.honorpackkeep` config var

Jeff King  writes:

> On Fri, Feb 28, 2014 at 10:45:39AM -0800, Junio C Hamano wrote:
>
>> > Exactly. The two features (bitmaps and .keep) are not compatible with
>> > each other, so you have to prioritize one. If you are using static .keep
>> > files, you might want them to continue being respected at the expense of
>> > using bitmaps for that repo. So I think you want a separate option from
>> > --write-bitmap-index to allow the appropriate flexibility.
>> 
>> What is "the appropriate flexibility", though?  If the user wants to
>> use bitmap, we would need to drop .keep, no?
>
> Or the flip side: if the user wants to use .keep, we should drop
> bitmaps. My point is that we do not know which way the user wants to
> go, so we should not tie the options together.

Hmph.  I think the short of your later explanation is "global config
may tell us to use bitmap, in which case we would need a way to
defeat that and have existing .keep honored, and it makes it easier
to do so if these two are kept separate, because you do not want to
run around and selectively disable bitmaps in these repositories.
We can instead do so with repack.packKeptObjects in the global
configuration." and I tend to agree with the reasoning.

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 v3 07/25] reflog: avoid constructing .lock path with git_path

Duy Nguyen  writes:

> On Wed, Feb 26, 2014 at 5:44 AM, Junio C Hamano  wrote:
>> Nguyễn Thái Ngọc Duy   writes:
>>
>>> git_path() soon understands the path given to it. Some paths "abc" may
>>> become "def" while other "ghi" may become "ijk". We don't want
>>> git_path() to interfere with .lock path construction. Concatenate
>>> ".lock" after the path has been resolved by git_path() so if "abc"
>>> becomes "def", we'll have "def.lock", not "ijk".
>>
>> Hmph.  I am not sure if the above is readable (or if I am reading it
>> correctly).
>
> Definitely not now that I have had my break from the series and reread it.
>
>> If "abc" becomes "def", it would take deliberate work to make
>> "abc.lock" into "ijk", and it would be natural to expect that
>> "abc.lock" would become "def.lock" without any fancy trick, no?
>
> A better explanation may be, while many paths are not converted by
> git_path() ("abc" -> "abc"), some of them will be based on the given
> path ("def" -> "ghi"). Giving path def.lock to git_path() may confuse
> it and make it believe def.lock should not be converted because the
> signature is "def.lock" not "def". But we want the lock file to have
> the same base name with the locked file (e.g. "ghi.lock", not
> "def.lock"). So it's best to append ".lock" after git_path() has done
> its conversion.
>
> The alternative is teach git_path about ".lock", but I don't really
> want to put more logic down there.

I think your attempt to sound generic by using "abc", "def",
etc. backfired and made the description only obscure.  It would have
been immediately understandable if it were more like this:

Among pathnames in $GIT_DIR, e.g. "index" or "packed-refs",
we want to automatically and silently map some of them to
the $GIT_DIR of the repository we are borrowing from via
$GIT_COMMON_DIR mechanism.  When we formulate the pathname
for its lockfile, we want it to be in the same location as
its final destination.  "index" is not shared and needs to
remain in the borrowing repository, while "packed-refs" is
shared and needs to go to the borrowed repository.

git_path() could be taught about the ".lock" suffix and map
"index.lock" and "packed-refs.lock" the same way their
basenames are mapped, but instead the caller can help by
asking where the basename (e.g. "index") is mapped to
git_path() and then appending ".lock" after the mapping is
done.

Thanks for an explanation.  With that understanding, the patch makes
sense.

--
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: Branch Name Case Sensitivity

Lee Hopkins  writes:

> I went ahead and took a stab at a solution. My solution is more
> aggressive than a warning, I actually prevent the creation of
> ambiguous refs. My changes are also in refs.c, which may not be
> appropriate, but it seemed like the natural place.
>
> I have never contributed to Git (in fact this is my first dive into
> the source) and my C is a bit rusty, so bear with me, this is just a
> suggestion:
>
> ---
>  refs.c |   31 ---
>  1 files changed, 24 insertions(+), 7 deletions(-)

Starting something like this from forbidding is likely to turn out
to be a very bad idea that can break existing repositories.

A new configuration

refs.caseInsensitive = {warn|error|allow}

that defaults to "warn" and the user can choose to set to "error" to
forbid, would be more palatable, I would say.

If the variable is not in 'core.' namespace, you should implement
this check at the Porcelain level, allowing lower-level tools like
update-ref as an escape hatch that let users bypass the restriction
to be used to correct breakages; it would mean an unconditional "if
!stricmp(), it is an error" in refs.c will not work well.

I think it might be OK to have

core.allowCaseInsentitiveRefs = {yes|no|warn}

which defaults to 'warn' (and 'yes' corresponds to 'allow', 'no'
corresponds to 'error', in the previous suggestion), instead. If we
wanted to prevent even lower-level tools like update-ref from
bypassing the check, that is.

--
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 01/25] path.c: make get_pathname() return strbuf instead of static buffer

Duy Nguyen  writes:

> On Thu, Feb 20, 2014 at 6:26 AM, Junio C Hamano  wrote:
>> Is there a reason not to do just an equivalent of
>>
>> #define git_pathdup mkpathdup
>>
>> and be done with it?  Am I missing something?
>
> They have a subtle difference: mkpathdup() calls cleanup_path() while
> git_pathdup() does not. Without auditing all call sites, we can't be
> sure this difference is insignificant. So I keep both functions
> separate for now.

Thanks; that is a very good explanation why #define'ing two to be
the same would not be a workable solution to the ownership issue I
raised.

>  char *git_pathdup(const char *fmt, ...)
>  {
> - char path[PATH_MAX], *ret;
> + struct strbuf *path = get_pathname();
>   va_list args;
>   va_start(args, fmt);
> - ret = vsnpath(path, sizeof(path), fmt, args);
> + vsnpath(path, fmt, args);
>   va_end(args);
> - return xstrdup(ret);
> + return strbuf_detach(path, NULL);
>  }

This feels somewhat wrong.

This function is for callers who are willing to take ownership of
the path buffer and promise to free the returned buffer when they
are done, because you are returning strbuf_detach()'ed piece of
memory, giving the ownership away.

The whole point of using get_pathname() is to allow callers not to
care about allocation issues on the paths they scribble on during
their short-and-simple codepaths that do not have too many uses of
similar temporary path buffers.  Why borrow from that round-robin
pool (which may now cause some codepaths to overflow the number of
such active temporary path buffers---have they been all audited)?
--
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] submodule : Add --no-separate-git-dir option to add and update command.

2014-03-03 Thread Jens Lehmann

Am 03.03.2014 14:47, schrieb Henri GEIST:
> This new option prevent git submodule  to clone the missing
> submodules with the --separate-git-dir option.
> Then the submodule will be regular repository and their gitdir will not
> be placed in the superproject gitdir/modules directory.

And what is your motivation for this? After all submodules containing
a .git directory are second class citizens (because they can never be
safely removed by regular git commands).

> Signed-off-by: Henri GEIST 
> ---
>  Documentation/git-submodule.txt |   18 --
>  git-submodule.sh|   22 --
>  t/t7400-submodule-basic.sh  |   12 
>  3 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 21cb59a..303a475 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -10,13 +10,14 @@ SYNOPSIS
>  
>  [verse]
>  'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
> -   [--reference ] [--depth ] [--]  
> []
> +   [--reference ] [--depth ] 
> [--no-separate-git-dir]
> +   [--]  []
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
>  'git submodule' [--quiet] init [--] [...]
>  'git submodule' [--quiet] deinit [-f|--force] [--] ...
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
> [-f|--force] [--rebase|--merge|--checkout] [--reference 
> ]
> -   [--depth ] [--recursive] [--] [...]
> +   [--depth ] [--recursive] [--no-separate-git-dir] [--] 
> [...]
>  'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) 
> ]
> [commit] [--] [...]
>  'git submodule' [--quiet] foreach [--recursive] 
> @@ -107,6 +108,10 @@ is the superproject and submodule repositories will be 
> kept
>  together in the same relative location, and only the
>  superproject's URL needs to be provided: git-submodule will correctly
>  locate the submodule using the relative URL in .gitmodules.
> ++
> +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> +has normal git repository without the option `--separate-git-dir` pointing
> +to the modules directory of the superproject gitdir.
>  
>  status::
>   Show the status of the submodules. This will print the SHA-1 of the
> @@ -185,6 +190,10 @@ If the submodule is not yet initialized, and you just 
> want to use the
>  setting as stored in .gitmodules, you can automatically initialize the
>  submodule with the `--init` option.
>  +
> +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> +has normal git repository without the option `--separate-git-dir` pointing
> +to the modules directory of the superproject gitdir.
> ++
>  If `--recursive` is specified, this command will recurse into the
>  registered submodules, and update any nested submodules within.
>  +
> @@ -363,6 +372,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
> options carefully.
>   clone with a history truncated to the specified number of revisions.
>   See linkgit:git-clone[1]
>  
> +--no-separate-git-dir::
> + This option is valid for add and update commands. Specify that missing
> + submodules should be clonned as self contain repository without a
> + separate gitdir placed in the modules directory of the superproject
> + gitdir.
>  
>  ...::
>   Paths to submodule(s). When specified this will restrict the command
> diff --git a/git-submodule.sh b/git-submodule.sh
> index a33f68d..36eaf31 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -5,11 +5,11 @@
>  # Copyright (c) 2007 Lars Hjemli
>  
>  dashless=$(basename "$0" | sed -e 's/-/ /')
> -USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
> ] [--]  []
> +USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
> ] [--no-separate-git-dir] [--]  []
> or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
> or: $dashless [--quiet] init [--] [...]
> or: $dashless [--quiet] deinit [-f|--force] [--] ...
> -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
> [-f|--force] [--rebase] [--reference ] [--merge] [--recursive] 
> [--] [...]
> +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
> [-f|--force] [--rebase] [--reference ] [--merge] [--recursive] 
> [--no-separate-git-dir] [--] [...]
> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
> [commit] [--] [...]
> or: $dashless [--quiet] foreach [--recursive] 
> or: $dashless [--quiet] sync [--recursive] [--] [...]"
> @@ -36,6 +36,7 @@ update=
>  prefix=
>  custom_name=
>  depth=
> +noseparategitdir=
>  
>  # The function takes at most 2 arguments. The first argument is the
>  # URL that navigates to the submodule origin repo. When relative, this URL
> @@ -270,6 +271,17 @@ module_clone()
>   quiet=-q
>

Re: [PATCH] cache_tree_find(): remove redundant check in while condition

Michael Haggerty  writes:

> On 03/03/2014 05:32 PM, David Kastrup wrote:
>> [...]
>> So perhaps all of that should just be
>> 
>>  while (*slash == '/')
>>  slash++;
>>  if (!*slash)
>>  return it;
>> 
>
> I just fixed a little thing that caught my eye.  You OWNED it.  You are
> absolutely right.
>
> Will you prepare a patch or would you like me to do it?

Feel free.  I really should be finishing some other patches instead...

-- 
David Kastrup
--
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: My advice for GSoC applicants

2014-03-03 Thread Rick Umali

On Mon, Mar 3, 2014 at 5:45 AM, Michael Haggerty  wrote:
> My secret tip for GSoC success
>
> http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html

I enjoyed reading that BLOG post. I daresay some of the points you
raise are pertinent for any new contributor to any open-source code
base.
--
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] cache_tree_find(): remove redundant check in while condition

2014-03-03 Thread Michael Haggerty

On 03/03/2014 05:32 PM, David Kastrup wrote:
> [...]
> So perhaps all of that should just be
> 
>   while (*slash == '/')
>   slash++;
>   if (!*slash)
>   return it;
> 

I just fixed a little thing that caught my eye.  You OWNED it.  You are
absolutely right.

Will you prepare a patch or would you like me to do it?

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.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: My advice for GSoC applicants

2014-03-03 Thread Philip Oakley


From: "Michael Haggerty" 
Cc: "Dmitry Dolzhenko" ; "Sun He"
; "Brian Gesiak" ; "Tanay
Abhra" ; "Kyriakos Georgiou"
; "Siddharth Goel"
; "Guanglin Xu" ;
"Karthik Nayak" ; "Alberto Corona"
; "Jacopo Notarstefano"



Hi,

Based on my experience so far as a first-time Google Summer of Code
mentor, I just wrote a blog article containing some hopefully useful
advice for students applying to the program.  Please note that this is
my personal opinion only and doesn't necessarily reflect the views of
the Git/libgit2 projects as a whole.

   My secret tip for GSoC success

http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html

Michael

--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--


In particular I liked : " If the documentation is unclear, it is OK to
ask for a clarification, but then _fix the documentation_ so that your
mentor never has to answer the same question again."

So the rhetorical question(s) for students would be :-

- was the Git documentation useful - did you see the README?, did it
lead, easily, to the useful places [1]? How could the wording/layout be
improved for the first time reader?

- which points of clarification were most useful and are they in the
documentation? Where should they be included?

- which points needed repeating often, and why? Where was the
disconnect?

- what would a patch look like...

Philip Oakley

[1] README; INSTALL; Documentation/SubmittingPatches;
Documentation/CodingGuidelines;

--
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] cache_tree_find(): remove redundant check in while condition