Re: [PATCH] http: fix some printf format warnings on 32-bit builds

[Lars Schneider]

On 11 Nov 2015, at 18:49, Ramsay Jones  wrote:

> 
> 
> On 11/11/15 02:00, Stefan Beller wrote:
>> On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine  
>> wrote:
>>> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>>>  wrote:
 Commit f8117f55 ("http: use off_t to store partial file size",
 02-11-2015) changed the type of some variables from long to off_t.
 The 32-bit build, which enables the large filesystem interface
 (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
 integer, whereas long is a 32-bit integer. This results in a couple
 of printf format warnings.
 
 In order to suppress the warnings, change the format specifier to use
 the PRIuMAX macro and cast the off_t argument to uintmax_t. (See also
 the http_opt_request_remainder() function, which uses the same
 solution).
>>> 
>>> I just ran across the problem when building 'next' on my Mac and was
>>> about to investigate, so am happy to find that the work has already
>>> been done. Thanks.
>>> 
>>> My machine is 64-bit, though, so perhaps it's misleading to
>>> characterize this as a fix for 32-bit builds. In particular, off_t is
>>> 'long long' on this machine, so it complains about the "long" format
>>> specifier.
>> 
>> +Lars
>> 
>> I wonder if 32 bit compilation can be part of travis.
>> 
> 
> Did this warning show up on the OS X build?

Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI build and 
it breaks the build on OS X.
See here (you need to scroll all the way down):
https://travis-ci.org/larsxschneider/git/jobs/90899656

BTW: I tried to set "-Werror" but then I got a bunch of macro redefined errors 
like this:
./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]

Is this a known issue? Is this an issue at all?

Thanks,
Lars--
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] http: fix some printf format warnings on 32-bit builds

[Eric Sunshine]

On Fri, Nov 13, 2015 at 3:46 AM, Lars Schneider
 wrote:
> On 11 Nov 2015, at 18:49, Ramsay Jones  wrote:
>> On 11/11/15 02:00, Stefan Beller wrote:
>>> On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine  
>>> wrote:
 On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
  wrote:
> Commit f8117f55 ("http: use off_t to store partial file size",
> 02-11-2015) changed the type of some variables from long to off_t.
> The 32-bit build, which enables the large filesystem interface
> (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
> integer, whereas long is a 32-bit integer. This results in a couple
> of printf format warnings.

 My machine is 64-bit, though, so perhaps it's misleading to
 characterize this as a fix for 32-bit builds. In particular, off_t is
 'long long' on this machine, so it complains about the "long" format
 specifier.
>>>
>>> I wonder if 32 bit compilation can be part of travis.
>>
>> Did this warning show up on the OS X build?
>
> Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI
> build and it breaks the build on OS X.
> See here (you need to scroll all the way down):
> https://travis-ci.org/larsxschneider/git/jobs/90899656
>
> BTW: I tried to set "-Werror" but then I got a bunch of macro redefined 
> errors like this:
> ./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]
>
> Is this a known issue? Is this an issue at all?

Odd. I don't experience anything like that on my Mac.
--
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] allow hooks to ignore their standard input stream

[Clemens Buchacher]

On Fri, Nov 13, 2015 at 01:17:29AM -0500, Jeff King wrote:
> 
> The test below reliably fails without your patch and passes with it, and
> seems to run reasonably quickly for me:

Thank you. I confirm the same behavior on my system. Below I have added
your change to the patch.

-->o--
Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input stream)
the pre-receive and post-receive hooks ignore SIGPIPE. Do the same for the
remaining hooks pre-push and post-rewrite, which read from standard input. The
same arguments for ignoring SIGPIPE apply.

Performance improvements which allow us to enable the test by
default by Jeff King.

Signed-off-by: Clemens Buchacher 
---
 builtin/commit.c |  3 +++
 t/t5571-pre-push-hook.sh | 41 +++--
 transport.c  | 11 +--
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dca09e2..f2a8b78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
 #include "sequencer.h"
 #include "notes-utils.h"
 #include "mailmap.h"
+#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
@@ -1537,8 +1538,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
return code;
n = snprintf(buf, sizeof(buf), "%s %s\n",
 sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   sigchain_push(SIGPIPE, SIG_IGN);
write_in_full(proc.in, buf, n);
close(proc.in);
+   sigchain_pop(SIGPIPE);
return finish_command(&proc);
 }
 
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..61df2f9 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,28 @@ test_expect_success 'push to URL' '
diff expected actual
 '
 
-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-   printf 'parent1\nrepo1\n' >expected
-   nr=1000
-   while test $nr -lt 2000
-   do
-   nr=$(( $nr + 1 ))
-   git branch b/$nr $COMMIT3
-   echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" 
>>expected
-   done
-
-   test_expect_success 'push many refs' '
-   git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-   diff expected actual
-   '
-fi
+test_expect_success 'set up many-ref tests' '
+   {
+   echo >&3 parent1 &&
+   echo >&3 repo1 &&
+   nr=1000
+   while test $nr -lt 2000
+   do
+   nr=$(( $nr + 1 ))
+   echo "create refs/heads/b/$nr $COMMIT3"
+   echo >&3 "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr 
$_z40"
+   done
+   } 3>expected | git update-ref --stdin
+'
+
+test_expect_success 'filling pipe buffer does not cause failure' '
+   git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
+   test_cmp expected actual
+'
+
+test_expect_success 'sigpipe does not cause pre-push hook failure' '
+   echo "exit 0" | write_script "$HOOK" &&
+   git push parent1 "refs/heads/b/*:refs/heads/c/*"
+'
 
 test_done
diff --git a/transport.c b/transport.c
index 23b2ed6..e34ab92 100644
--- a/transport.c
+++ b/transport.c
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "sigchain.h"
 
 /* rsync support */
 
@@ -1127,6 +1128,8 @@ static int run_pre_push_hook(struct transport *transport,
return -1;
}
 
+   sigchain_push(SIGPIPE, SIG_IGN);
+
strbuf_init(&buf, 256);
 
for (r = remote_refs; r; r = r->next) {
@@ -1140,8 +1143,10 @@ static int run_pre_push_hook(struct transport *transport,
 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 r->name, sha1_to_hex(r->old_sha1));
 
-   if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
-   ret = -1;
+   if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+   /* We do not mind if a hook does not read all refs. */
+   if (errno != EPIPE)
+   ret = -1;
break;
}
}
@@ -1152,6 +1157,8 @@ static int run_pre_push_hook(struct transport *transport,
if (!ret)
ret = x;
 
+   sigchain_pop(SIGPIPE);
+
x = finish_command(&proc);
if (!ret)
ret = x;
-- 
1.9.4

--
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] http: fix some printf format warnings on 32-bit builds

[Torsten Bögershausen]

On 2015-11-13 09.57, Eric Sunshine wrote:
> On Fri, Nov 13, 2015 at 3:46 AM, Lars Schneider
>  wrote:
>> On 11 Nov 2015, at 18:49, Ramsay Jones  wrote:
>>> On 11/11/15 02:00, Stefan Beller wrote:
 On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine  
 wrote:
> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>  wrote:
>> Commit f8117f55 ("http: use off_t to store partial file size",
>> 02-11-2015) changed the type of some variables from long to off_t.
>> The 32-bit build, which enables the large filesystem interface
>> (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
>> integer, whereas long is a 32-bit integer. This results in a couple
>> of printf format warnings.
>
> My machine is 64-bit, though, so perhaps it's misleading to
> characterize this as a fix for 32-bit builds. In particular, off_t is
> 'long long' on this machine, so it complains about the "long" format
> specifier.

 I wonder if 32 bit compilation can be part of travis.
>>>
>>> Did this warning show up on the OS X build?
>>
>> Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI
>> build and it breaks the build on OS X.
>> See here (you need to scroll all the way down):
>> https://travis-ci.org/larsxschneider/git/jobs/90899656
>>
>> BTW: I tried to set "-Werror" but then I got a bunch of macro redefined 
>> errors like this:
>> ./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]
>>
>> Is this a known issue? Is this an issue at all?
> 
> Odd. I don't experience anything like that on my Mac.

Could it be, that strlcpy is present on your system ?
And where does it come from ?

Which OS ?
Which compiler ?
What does `uname -r` say ?
Do you have Macports, Fink, Brew... installed ?




--
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] http: fix some printf format warnings on 32-bit builds

[Lars Schneider]

> On 13 Nov 2015, at 11:32, Torsten Bögershausen  wrote:
> 
> On 2015-11-13 09.57, Eric Sunshine wrote:
>> On Fri, Nov 13, 2015 at 3:46 AM, Lars Schneider
>>  wrote:
>>> On 11 Nov 2015, at 18:49, Ramsay Jones  wrote:
 On 11/11/15 02:00, Stefan Beller wrote:
> On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine  
> wrote:
>> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>>  wrote:
>>> Commit f8117f55 ("http: use off_t to store partial file size",
>>> 02-11-2015) changed the type of some variables from long to off_t.
>>> The 32-bit build, which enables the large filesystem interface
>>> (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
>>> integer, whereas long is a 32-bit integer. This results in a couple
>>> of printf format warnings.
>> 
>> My machine is 64-bit, though, so perhaps it's misleading to
>> characterize this as a fix for 32-bit builds. In particular, off_t is
>> 'long long' on this machine, so it complains about the "long" format
>> specifier.
> 
> I wonder if 32 bit compilation can be part of travis.
 
 Did this warning show up on the OS X build?
>>> 
>>> Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI
>>> build and it breaks the build on OS X.
>>> See here (you need to scroll all the way down):
>>> https://travis-ci.org/larsxschneider/git/jobs/90899656
>>> 
>>> BTW: I tried to set "-Werror" but then I got a bunch of macro redefined 
>>> errors like this:
>>> ./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]
>>> 
>>> Is this a known issue? Is this an issue at all?
>> 
>> Odd. I don't experience anything like that on my Mac.
> 
> Could it be, that strlcpy is present on your system ?
> And where does it come from ?
> 
> Which OS ?
> Which compiler ?
> What does `uname -r` say ?
> Do you have Macports, Fink, Brew... installed ?
> 

Looks like this is a OS X only issue. Happens with clang and gcc on the OS X 
Mavericks TravisCI machines [1]:
https://travis-ci.org/larsxschneider/git/jobs/90919078
https://travis-ci.org/larsxschneider/git/jobs/90919080

On Linux+gcc the following error happens if "-Werror" is present:
https://travis-ci.org/larsxschneider/git/jobs/90919076
Do you have an idea what that might be?

Linux+clang works fine:
https://travis-ci.org/larsxschneider/git/jobs/90919074

- Lars

[1] http://docs.travis-ci.com/user/ci-environment/


--
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: git clean performance issues

[Andreas Krey]

On Sat, 04 Apr 2015 15:55:07 +, Jeff King wrote:
...
> I think this is the same issue that was discussed here:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/265560/focus&5585
> 
> There is some discussion of a possible fix in that thread. I was hoping
> that Andreas was going to look further and produce a patch, but I
> imagine he got busy with other things.

That about sums it up. However I now have a similar issue;
git ls-files shows the same behaviour (takes relatively
forever at 100% CPU), and runs instantly with my patch
from back then. Nothing seems to have changed, so I
may have another chance to look into this.

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] contrib/subtree: Testsuite cleanup

[Alexey Shumkin]

On Thu, Nov 12, 2015 at 08:32:29PM -0600, David Greene wrote:
> Sending again with a proper From: address after rebasing on latest master.
> 
> Copying the maintainers because the origin patchset didn't get any
> comments and I'm unsure of how to proceed.
> 
> These are some old changes I have lying around that should get applied
> to clean up git-subtree's testbase.  With these changes post-mortem
> analysis is much easier and adding new tests can be done in an orderly
> fashion.
> 
> I have a number of future patches and further development ideas for
> git-subtree that require these changes as a prerequisite.
Please, could you take a look to the following thread
http://thread.gmane.org/gmane.comp.version-control.git/277343
to take into account the mentioned bug for your futher work?

Thank you
> 
> -David
> 
>  contrib/subtree/git-subtree.sh |2 +-
>  contrib/subtree/t/Makefile |   31 +-
>  contrib/subtree/t/t7900-subtree.sh | 1366 +--
>  3 files changed, 956 insertions(+), 443 deletions(-)
> 

-- 
Alexey Shumkin
E-mail: alex.crez...@gmail.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: [PATCH] rebase-i-exec: Allow space in SHELL_PATH

[Fredrik Medley]

2015-11-13 7:25 GMT+01:00 Jeff King :
> On Fri, Nov 13, 2015 at 07:03:19AM +0100, Fredrik Medley wrote:
>
>> On Windows, when Git is installed under "C:\Program Files\Git", SHELL_PATH
>> will include a space. Fix "git rebase --interactive --exec" so that it
>> works with spaces in SHELL_PATH.
>>
>> Signed-off-by: Fredrik Medley 
>> ---
>>  git-rebase--interactive.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 30edb17..b938a6d 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -610,7 +610,7 @@ do_next () {
>>   read -r command rest < "$todo"
>>   mark_action_done
>>   printf 'Executing: %s\n' "$rest"
>> - ${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
>> + "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
>
> I think this is the right thing to do (at least I could not think of a
> case that would be harmed by it, and it certainly fixes your case). It
> looks like filter-branch would need a similar fix?
>
> I think this still isn't resilient to weird meta-characters in the
> @SHELL_PATH@, but as this is a build-time option, I think it's OK to let
> people who do
>
>   make SHELL_PATH='}"; rm -rf /'
>
> hang themselves.
>
> -Peff

Okay, that's what @SHELL_PATH@ stands for. I just read the result
in the Windows installation that is something like ${SHELL:-/bin/sh}.
The shell script processor then replaces /bin/sh with
C:\Program Files\...\bin\sh.

I assume the Windows compilation does not fail in building this. I've
never tried building git for Windows, though.

/Fredrik
--
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] refs.c: get_ref_cache: use a bucket hash

[Andreas Krey]

On Tue, 17 Mar 2015 01:48:00 +, Jeff King wrote:
> On Mon, Mar 16, 2015 at 10:35:18PM -0700, Junio C Hamano wrote:
> 
> > > It looks like we don't even really care about the value of HEAD. We just
> > > want to know "is it a git directory?". I think in other places (like
> > > "git add"), we just do an existence check for "$dir/.git". That would
> > > not catch a bare repository, but I do not think the current check does
> > > either (it is looking for submodules, which always have a .git).
> > 
> > If we wanted to be consistent, perhaps we should be reusing the "is
> > this a git repository?" check used by the auto-discovery codepath
> > (setup.c:is_git_directory(), perhaps?), but the idea looks simple
> > enough and sounds sensible.
> 
> Yeah, I almost suggested that, but I'm concerned that would make us
> inconsistent with how we report untracked files. I thought that dir.c
> used ".git" as a magic token there.
> 
> But it seems I'm wrong. We do ignore ".git" directly in treat_path(),
> but treat_directory actually checks resolve_gitlink_ref. I think this
> will suffer the same problem as Andreas's original issue (e.g., if you
> run "git ls-files -o").

Guess what landed on my desk this week. Same repo, same
application test suite, same problem, now with 'git ls-files -o'.

> Likewise, I think dir.c:remove_dir_recurse is in a similar boat.
> Grepping for resolve_gitlink_ref, it looks like there may be others,
> too.

Can't we handle this in resolve_gitlink_ref itself? As I understand it,
it should resolve a ref (here "HEAD") when path points to a submodule.
When there isn't one it should return -1, so:

diff --git a/refs.c b/refs.c
index 132eff5..f8648c5 100644
--- a/refs.c
+++ b/refs.c
@@ -1553,6 +1553,10 @@ int resolve_gitlink_ref(const char *path, const char 
*refname, unsigned char *sh
if (!len)
return -1;
submodule = xstrndup(path, len);
+   if (!is_git_directory(submodule)) {
+   free(submodule);
+   return -1;
+   }
refs = get_ref_cache(submodule);
free(submodule);

I'm way too little into the code to see what may this may get wrong.

But this, as well as the old hash-ref-cache patch speeds me
up considerably, in this case a git ls-files -o from half a
minute of mostly user CPU to a second.

> All of these should be using the same test, I think. Doing that with
> is_git_directory() is probably OK. It is a little more expensive than we
> might want for mass-use (it actually opens and parses the HEAD file in
> each directory),

This happens as well when we let resolve_gitlink_ref run its old course.
(It (ls-files) even seems to try to open .git and then .git/HEAD, even
if the former fails with ENOENT.)

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800
--
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] notes: allow merging from arbitrary references

[Jacob Keller]

From: Jacob Keller 

Create a new expansion function, expand_loose_notes_ref which will
expand any ref using get_sha1, but falls back to expand_notes_ref if
this fails. The contents of the strbuf will be either the hex string of
the sha1, or the expanded notes ref. It is expected to be re-expanded
using get_sha1 inside the notes merge machinery, and there is no real
error checking provided at this layer.

Since we now support merging from non-notes refs, remove the test case
associated with that behavior. Add a test case for merging from a
non-notes ref.

Signed-off-by: Jacob Keller 
---
I do not remember what version this was since it has been an age ago
that I sent the previous code. This is mostly just a rebase onto current
next. I believe I have covered everything previous reviewers noted.

I'm interested in whether this is the right direction, as my longterm
goal is to be able to push/pull notes to a specific namespace (probably
refs/remote-notes/*, since actually modifying to use
refs/remotes/notes/* is difficult to send to users, and remote-notes
makes the most useful sense). The first part of this is allowing merge
to come from an arbitrary reference, as currently it is not really
possible to merge from refs/remote-notes as we'd need it to be.

 builtin/notes.c|  4 ++--
 notes.c| 14 ++
 notes.h|  8 
 t/t3308-notes-merge.sh | 22 +++---
 4 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index e0f5d308d206..4a86cc90ee92 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -809,7 +809,7 @@ static int merge(int argc, const char **argv, const char 
*prefix)
 
o.local_ref = default_notes_ref();
strbuf_addstr(&remote_ref, argv[0]);
-   expand_notes_ref(&remote_ref);
+   expand_loose_notes_ref(&remote_ref);
o.remote_ref = remote_ref.buf;
 
t = init_notes_check("merge", NOTES_INIT_WRITABLE);
@@ -836,7 +836,7 @@ static int merge(int argc, const char **argv, const char 
*prefix)
}
 
strbuf_addf(&msg, "notes: Merged notes from %s into %s",
-   remote_ref.buf, default_notes_ref());
+   argv[0], default_notes_ref());
strbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); /* skip "notes: 
" */
 
result = notes_merge(&o, t, result_sha1);
diff --git a/notes.c b/notes.c
index 358e2fdb74eb..c92c22aa217a 100644
--- a/notes.c
+++ b/notes.c
@@ -1306,3 +1306,17 @@ void expand_notes_ref(struct strbuf *sb)
else
strbuf_insert(sb, 0, "refs/notes/", 11);
 }
+
+void expand_loose_notes_ref(struct strbuf *sb)
+{
+   unsigned char object[20];
+
+   if (get_sha1(sb->buf, object)) {
+   /* fallback to expand_notes_ref */
+   expand_notes_ref(sb);
+   } else {
+   /* we got an object, so replace the strbuf with the hex string 
*/
+   strbuf_reset(sb);
+   strbuf_addstr(sb, sha1_to_hex(object));
+   }
+}
diff --git a/notes.h b/notes.h
index e5d67fd3754a..658caf7d6e99 100644
--- a/notes.h
+++ b/notes.h
@@ -302,4 +302,12 @@ void string_list_add_refs_from_colon_sep(struct 
string_list *list,
 /* Expand inplace a note ref like "foo" or "notes/foo" into "refs/notes/foo" */
 void expand_notes_ref(struct strbuf *sb);
 
+/*
+ * Similar to expand_notes_ref, but allows arbitrary refs to be expanded via
+ * get_sha1 first. If get_sha1 fails to find a ref, fall back to traditional
+ * expand_notes_ref. The contents of the strbuf will be suitable to attempt
+ * passing to get_sha1 again inside the notes machinery.
+ */
+void expand_loose_notes_ref(struct strbuf *sb);
+
 #endif
diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 24d82b49bbea..19aed7ec953b 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -18,7 +18,9 @@ test_expect_success setup '
git notes add -m "Notes on 1st commit" 1st &&
git notes add -m "Notes on 2nd commit" 2nd &&
git notes add -m "Notes on 3rd commit" 3rd &&
-   git notes add -m "Notes on 4th commit" 4th
+   git notes add -m "Notes on 4th commit" 4th &&
+   # Copy notes to remote-notes
+   git fetch . refs/notes/*:refs/remote-notes/origin/*
 '
 
 commit_sha1=$(git rev-parse 1st^{commit})
@@ -66,7 +68,9 @@ test_expect_success 'verify initial notes (x)' '
 '
 
 cp expect_notes_x expect_notes_y
+cp expect_notes_x expect_notes_v
 cp expect_log_x expect_log_y
+cp expect_log_x expect_log_v
 
 test_expect_success 'fail to merge empty notes ref into empty notes ref (z => 
y)' '
test_must_fail git -c "core.notesRef=refs/notes/y" notes merge z
@@ -84,16 +88,12 @@ test_expect_success 'fail to merge into various non-notes 
refs' '
test_must_fail git -c "core.notesRef=refs/notes/foo^{bar" notes merge x
 '
 
-test_expect_success 'fail to merge various non-note-trees' '
-   git config core.notesRef refs/notes/y &&
-   te

[RFC] rename detection: allow more renames

[Andreas Krey]

Hi all,

we also ran into the maximum rename limit
in the rename detection. (Yes, we get a lot
of rename candidates when cherry-picking
between two specific releases.)

The code talks about limiting the size
of the rename matrix, but as far as I
can see, the matrix itself never exists
as such, and the only thing that could
actually overflow is the computation
for the progress indication. This
can be fixed by reporting just the
destinations checked instead of the
combinations, since we only update
the progress once per destination
anyway.

If the direction is good, I will
shape it up (and obtain the
proper signoffs).


diff --git a/diffcore-rename.c b/diffcore-rename.c
index 749a35d..279f24e 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -368,7 +368,7 @@ static int too_many_rename_candidates(int num_create,
int rename_limit = options->rename_limit;
int num_src = rename_src_nr;
int i;
-
+   static int max_rename_limit = 10;
options->needed_rename_limit = 0;
 
/*
@@ -380,8 +380,8 @@ static int too_many_rename_candidates(int num_create,
 * but handles the potential overflow case specially (and we
 * assume at least 32-bit integers)
 */
-   if (rename_limit <= 0 || rename_limit > 32767)
-   rename_limit = 32767;
+   if (rename_limit <= 0 || rename_limit > max_rename_limit)
+   rename_limit = max_rename_limit;
if ((num_create <= rename_limit || num_src <= rename_limit) &&
(num_create * num_src <= rename_limit * rename_limit))
return 0;
@@ -515,7 +515,7 @@ void diffcore_rename(struct diff_options *options)
if (options->show_rename_progress) {
progress = start_progress_delay(
_("Performing inexact rename detection"),
-   rename_dst_nr * rename_src_nr, 50, 1);
+   rename_dst_nr, 50, 1);
}
 
mx = xcalloc(num_create * NUM_CANDIDATE_PER_DST, sizeof(*mx));
@@ -552,7 +552,7 @@ void diffcore_rename(struct diff_options *options)
diff_free_filespec_blob(two);
}
dst_cnt++;
-   display_progress(progress, (i+1)*rename_src_nr);
+   display_progress(progress, (i+1));
}
stop_progress(&progress);


And we would also like to see progress when doing
a cherry pick - in our case this takes a few minutes:

 
diff --git a/sequencer.c b/sequencer.c
index 3c060e0..8fce028 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -282,6 +282,7 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
 
for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
parse_merge_opt(&o, *xopt);
+   o.show_rename_progress = isatty(2);
 
clean = merge_trees(&o,
head_tree,


Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800
--
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

git log --merges doesn't show commits as expected

[Dominik Rauch]

Hello!
 
(This is my first post to this mailing list and I couldn't find a FAQ section - 
please excuse me, if I haven't followed all the established posting guidelines 
yet.)

I have the following repository tree:

C
|\
| B
| /
A

Commit A: Parents=(). Initial commit. Contains file foo with content "ABC".
Commit B: Parents=(A). Represents a commit on some feature branch. Contains 
file foo with content "XYZ".
Commit C: Parents=(A, B). Represents a merge commit of a feature branch back to 
the main branch. Contains file foo with content "XYZ".

I expected "git log --merges foo" to show C, however, the log is empty! 
Specifying "--full-history" results in the correct history, therefore I assume, 
I misunderstand Git's default history simplification algorithm. Unfortunately, 
the example in the Git docs at [1] does not contain the very same situation 
(although it is probably one of the most common situations...).

Does anybody know why I don't see the log output I expect? I'm confused...even 
if the log output is correct, I don't think it follows the principle of least 
surprise...

Side note: specifying "--first-parent" also results in commit C being shown.

Best regards,
Dominik

PS: This is a cross post of [2], somebody noted it could be a bug, which is why 
I decided to post to this mailing list.

[1] https://git-scm.com/docs/git-log#_history_simplification 
[2] http://stackoverflow.com/q/33695763/

--
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] rebase-i-exec: Allow space in SHELL_PATH

[Matthieu Moy]

Fredrik Medley  writes:

> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -610,7 +610,7 @@ do_next () {
>   read -r command rest < "$todo"
>   mark_action_done
>   printf 'Executing: %s\n' "$rest"
> - ${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
> + "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution

Looks good to me. Don't know why I didn't add these double quotes when I
introduced this line. Thanks for fixing it.

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

[PATCH v2] contrib/subtree: unwrap tag refs

[Rob Mayoff]

If a subtree was added using a tag ref, the tag ref is stored in
the subtree commit message instead of the underlying commit's ref.
To split or push subsequent changes to the subtree, the subtree
command needs to unwrap the tag ref.  This patch makes it do so.

The problem was described in a message to the mailing list from
Junio C Hamano dated 29 Apr 2014, with the subject "Re: git subtree
issue in more recent versions". The archived message can be found
at .

Signed-off-by: Rob Mayoff 
---

changes since v1:

* remove obsolete sub assignments
* wrap lines

 contrib/subtree/git-subtree.sh | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 9f06571..5ed0ea5 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -245,7 +245,10 @@ find_latest_squash()
case "$a" in
START) sq="$b" ;;
git-subtree-mainline:) main="$b" ;;
-   git-subtree-split:) sub="$b" ;;
+   git-subtree-split:)
+   sub="$(git rev-parse "$b^0")" ||
+   die "could not rev-parse split hash $b from 
commit $sq"
+   ;;
END)
if [ -n "$sub" ]; then
if [ -n "$main" ]; then
@@ -278,7 +281,10 @@ find_existing_splits()
case "$a" in
START) sq="$b" ;;
git-subtree-mainline:) main="$b" ;;
-   git-subtree-split:) sub="$b" ;;
+   git-subtree-split:)
+   sub="$(git rev-parse "$b^0")" ||
+   die "could not rev-parse split hash $b from 
commit $sq"
+   ;;
END)
debug "  Main is: '$main'"
if [ -z "$main" -a -n "$sub" ]; then
-- 
2.4.3

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

[PATCH RFC] completion: add support for completing email aliases

[Jacob Keller]

From: Jacob Keller 

Extract email aliases from the sendemail.aliasesfile according to the
known types. Implementation only extracts the alias name and does not
attempt to complete email addresses.

Add a few tests for simple layouts of the currently supported alias
filetypes.

Signed-off-by: Jacob Keller 
---

Labeled this RFC because I have only been able to test the mutt format
as this is what I use locally. I have a few (probably brittle) test
cases for the files, but they are not "real" configuration files as per
the upstream tools, so they are essentially made to work with the simple
extractors that I have now. I'd like some review on this to see if it's
valuable, but it definitely helps me type out aliases and see what is
available by just using TAB.

 contrib/completion/git-completion.bash | 69 ++
 t/t9902-completion.sh  | 90 ++
 2 files changed, 159 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 482ca84b451b..9b786bb390ba 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -10,6 +10,7 @@
 #*) local and remote tag names
 #*) .git/remotes file names
 #*) git 'subcommands'
+#*) git email aliases for git-send-email
 #*) tree paths within 'ref:path/to/file' expressions
 #*) file paths within current working directory and index
 #*) common --long-options
@@ -785,6 +786,56 @@ __git_aliased_command ()
done
 }
 
+# Print aliases for email addresses from sendemail.aliasesfile
+__git_email_aliases ()
+{
+   local file="$(git --git-dir="$(__gitdir)" config --path 
sendemail.aliasesfile)"
+   local filetype="$(git --git-dir="$(__gitdir)" config 
sendemail.aliasfiletype)"
+
+   # Only run awk if we find an actual file
+   if ! [ -f $file ]; then
+   return
+   fi
+
+   case "$filetype" in
+   # Each file type needs to be parsed differently.
+   mutt|mailrc)
+   # Mutt and mailrc are simple and just put the alias in
+   # the 2nd field of the file.
+   awk '{print $2}' $file
+   return
+   ;;
+   sendmail)
+   # Skip new lines, lines without fields, and lines
+   # ending in '\' then print the name minus the final :
+   awk 'NF && $1!~/^#/ && !/\\$/ {sub(/:$/, "", $1); print 
$1 }' $file
+   return
+   ;;
+   pine)
+   # According to spec, line continuations are any line
+   # which starts with whitespace, otherwise we can just
+   # use the normal separator and print the first field.
+   awk '/^\S/ {print $1}' "$file"
+   return
+   ;;
+   elm)
+   # Elm doesn't appear to allow newlines, and
+   # git-send-email only accepts one alias per line, so
+   # just print the first field.
+   awk '{print $1}' "$file"
+   return
+   ;;
+   gnus)
+   # The gnus format has the alias quoted, so we just use
+   # gsub to extract the alias from the quotes
+   awk '/define-mail-alias/ {gsub(/"/, "", $2); print $2}' 
$file
+   return
+   ;;
+   *)
+   return;;
+   esac
+}
+
 # __git_find_on_cmdline requires 1 argument
 __git_find_on_cmdline ()
 {
@@ -1735,6 +1786,24 @@ _git_send_email ()
" "" "${cur##--thread=}"
return
;;
+   --to=*)
+   __gitcomp "
+   $(__git_email_aliases)
+   " "" "${cur##--to=}"
+   return
+   ;;
+   --cc=*)
+   __gitcomp "
+   $(__git_email_aliases)
+   " "" "${cur##--cc=}"
+   return
+   ;;
+   --bcc=*)
+   __gitcomp "
+   $(__git_email_aliases)
+   " "" "${cur##--bcc=}"
+   return
+   ;;
--*)
__gitcomp "--annotate --bcc --cc --cc-cmd --chain-reply-to
--compose --confirm= --dry-run --envelope-sender
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2ba62fbc178e..0549f75e6e7c 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -404,6 +404,96 @@ test_expect_success '__git_aliases' '
test_cmp expect actual
 '
 
+test_expect_success '__git_email_aliases (mutt)' '
+   cat >aliases <<-EOF &&
+   alias user1 Some User 
+   alias user2 random-user-foo@foo.garbage
+   EOF
+  

Re: [PATCH v2] add test to demonstrate that shallow recursive clones fail

[Stefan Beller]

On Thu, Nov 12, 2015 at 9:35 PM, Jeff King  wrote:
>
> Hrm. Do we want to make these workarounds work correctly? Or is the
> final solution going to be that the first command you gave simply works,
> and no workarounds are needed.  If the latter, I wonder if we want to be
> adding tests for the workarounds in the first place.

I think we want to make the final solution just work. I dug into that and it is
harder than expected. I may even call it a bug. The bug doesn't occur often as
it is only triggered by things like rewriting history (forced pushes)
or a short dpeth
argument.

So if you invoke "git clone --recursive", it will internally just
delegate the submodule
handling to "git submodule update --init --recursive", which then (as
the submodule
doesn't exist yet) will delegate the cloning to "git submodule--helper
clone", which
will then call git clone for the actual cloning.

However in this whole chain of commands we never pass around the actual sha1
we need. The strategy is to clone first and then checkout the sha1, which the
superprojects wants to see. The desired sha1 was hopefully included in
the cloning,
so we can check it out.

But the sha1 may not be present if we have a very short depth argument, or if we
rewrote history. In case of a short depth argument, consider the
following history:

... <- A <- B

A is the recorded sha1 in the superproject, whereas B is the HEAD in the
remote you're cloning from. If cloning with depth=1, the most naive way
would have been to pass on the depth argument down the command chain,
but then we would end up cloning B with no further depth, and upon checkout
we cannot find A.

In case of the rewritten history, consider:

.. < - C <- B
 \
  A

whereas A is the recorded sha1 in the superproject, but on a different branch
(or even just a dangling commit. but used to be on master).
B is the master branch. In case we pass on --depth to cloning the submodule,
--single-branch is implied by --depth, so we would not clone A. In case of
A being a dangling commit, we wouldn't even clone it without the depth argument.

So I propose:
 * similar to fetch, we enable clone to obtain a specific sha1 from remote.
 * we explicitly pass the submodule sha1 as recorded in the superproject
   to the submodule fetch/clone in case we follow the exact sha1. In case of
   --remote or the branch field present in the superprojects .gitmodule file,
   we can just pass the branch name.

Thanks,
Stefan
--
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] http: fix some printf format warnings on 32-bit builds

[Ramsay Jones]

On 13/11/15 08:57, Eric Sunshine wrote:
> On Fri, Nov 13, 2015 at 3:46 AM, Lars Schneider
>  wrote:
>> On 11 Nov 2015, at 18:49, Ramsay Jones  wrote:
>>> On 11/11/15 02:00, Stefan Beller wrote:
 On Tue, Nov 10, 2015 at 5:22 PM, Eric Sunshine  
 wrote:
> On Tue, Nov 10, 2015 at 7:23 PM, Ramsay Jones
>  wrote:
>> Commit f8117f55 ("http: use off_t to store partial file size",
>> 02-11-2015) changed the type of some variables from long to off_t.
>> The 32-bit build, which enables the large filesystem interface
>> (_FILE_OFFSET_BITS == 64), defines the off_t type as a 64-bit
>> integer, whereas long is a 32-bit integer. This results in a couple
>> of printf format warnings.
>
> My machine is 64-bit, though, so perhaps it's misleading to
> characterize this as a fix for 32-bit builds. In particular, off_t is
> 'long long' on this machine, so it complains about the "long" format
> specifier.

 I wonder if 32 bit compilation can be part of travis.
>>>
>>> Did this warning show up on the OS X build?
>>
>> Yes, I added CFLAGS="-Werror=format" to the my experimental TravisCI
>> build and it breaks the build on OS X.
>> See here (you need to scroll all the way down):
>> https://travis-ci.org/larsxschneider/git/jobs/90899656
>>
>> BTW: I tried to set "-Werror" but then I got a bunch of macro redefined 
>> errors like this:
>> ./git-compat-util.h:614:9: error: 'strlcpy' macro redefined [-Werror]
>>
>> Is this a known issue? Is this an issue at all?
> 
> Odd. I don't experience anything like that on my Mac.
> 

Hmm, from the output, it looks like the configure script is
not detecting that 'strlcpy' is available (so setting
NO_STRLCPY=YesPlease in the config.mak.autogen file).
However, it seems to be a 'macro redirect' set in the
/usr/include/secure/_string.h header file (presumably it
redirects between a more or less secure version ;-)

Unfortunately, I don't have access to a mac - so I can't
help you with the debugging. :(

ATB,
Ramsay Jones


--
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: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Jens Lehmann]

Am 12.11.2015 um 00:34 schrieb Stefan Beller:

On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann  wrote:


TL;DR: checkout is serial, network-related stuff only will be using
submodule.jobs



My point being: isn't "jobs" a bit too generic for a config option that
is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
or similar would be better, as you are already thinking about adding
other parallelisms with different constraints later?


Actually I don't think that far ahead.


Maybe I've been bitten once too often by too generic names that became
a problem later on ... ;-)


(I assume network to be the bottleneck for clone/fetch operations)
All I want is a saturated network all the time, and as the native git protocol
doesn't provide that (tcp startup takes time until full band witdth is reached,
local operations both on client and server) I added the parallel stuff
to 'smear' different submodule network traffics along the timeline,
such that we have a better approximation of an always fully saturated link
for the whole operation. So in the long term future, we maybe want to
reuse an http/ssh session for a different submodule, possibly interleaving
the different submodules on the wire to make it even faster. Though that
may not be helping much.

So we're back at bike shedding about the name. submodule.fetchJobs
sounds like it only applies to fetching, do you think it's sufficient for clone
as well?


Hmm, to me fetching is a part of cloning, so I don't have a problem with
that. And documenting it accordingly should make it clear to everyone.


Once upon a time, Junio used  'submodule.fetchParallel' or  'submodule.paralle'
in a discussion[1] for the distinction of the local and networked things.
[1] Discussing "[PATCH] Add fetch.recurseSubmoduleParallelism config option"

How about submodules.parallelNetwork for the networking part and
submodules.parallelLocal for the local part? (I don't implement parallelLocal in
the next few weeks I'd estimate).


If 'submodules.parallelNetwork' will be used for submodule push too as
soon as that learns parallel operation, I'm ok with that. But if we don't
have good reason to believe the number of jobs for fetch can simply be
reused for push, me thinks we should have one config option containing the
term "fetch" now and another that contains "push" when we need it later,
just to be on the safe side. Otherwise it might be hard to explain to
users why 'submodules.parallelNetwork' is only used for fetch and clone
and why they have to set 'submodules.parallelPush' for pushing ...

So either 'submodule.fetchParallel' or 'submodule.fetchJobs' is fine for
me, and 'submodules.parallelNetwork' is ok too as long as we have reason
to believe this value can be used for push later too.
--
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: [PATCHv3 08/11] fetching submodules: respect `submodule.jobs` config option

[Stefan Beller]

On Fri, Nov 13, 2015 at 12:47 PM, Jens Lehmann  wrote:
> Am 12.11.2015 um 00:34 schrieb Stefan Beller:
>>
>> On Wed, Nov 11, 2015 at 11:55 AM, Jens Lehmann 
>> wrote:


 TL;DR: checkout is serial, network-related stuff only will be using
 submodule.jobs
>>>
>>>
>>>
>>> My point being: isn't "jobs" a bit too generic for a config option that
>>> is only relevant for network-related stuff? Maybe "submodule.fetchJobs"
>>> or similar would be better, as you are already thinking about adding
>>> other parallelisms with different constraints later?
>>
>>
>> Actually I don't think that far ahead.
>
>
> Maybe I've been bitten once too often by too generic names that became
> a problem later on ... ;-)
>
>> (I assume network to be the bottleneck for clone/fetch operations)
>> All I want is a saturated network all the time, and as the native git
>> protocol
>> doesn't provide that (tcp startup takes time until full band witdth is
>> reached,
>> local operations both on client and server) I added the parallel stuff
>> to 'smear' different submodule network traffics along the timeline,
>> such that we have a better approximation of an always fully saturated link
>> for the whole operation. So in the long term future, we maybe want to
>> reuse an http/ssh session for a different submodule, possibly interleaving
>> the different submodules on the wire to make it even faster. Though that
>> may not be helping much.
>>
>> So we're back at bike shedding about the name. submodule.fetchJobs
>> sounds like it only applies to fetching, do you think it's sufficient for
>> clone
>> as well?
>
>
> Hmm, to me fetching is a part of cloning, so I don't have a problem with
> that. And documenting it accordingly should make it clear to everyone.
>
>> Once upon a time, Junio used  'submodule.fetchParallel' or
>> 'submodule.paralle'
>> in a discussion[1] for the distinction of the local and networked things.
>> [1] Discussing "[PATCH] Add fetch.recurseSubmoduleParallelism config
>> option"
>>
>> How about submodules.parallelNetwork for the networking part and
>> submodules.parallelLocal for the local part? (I don't implement
>> parallelLocal in
>> the next few weeks I'd estimate).
>
>
> If 'submodules.parallelNetwork' will be used for submodule push too as
> soon as that learns parallel operation, I'm ok with that. But if we don't
> have good reason to believe the number of jobs for fetch can simply be
> reused for push, me thinks we should have one config option containing the
> term "fetch" now and another that contains "push" when we need it later,
> just to be on the safe side. Otherwise it might be hard to explain to
> users why 'submodules.parallelNetwork' is only used for fetch and clone
> and why they have to set 'submodules.parallelPush' for pushing ...
>
> So either 'submodule.fetchParallel' or 'submodule.fetchJobs' is fine for
> me, and 'submodules.parallelNetwork' is ok too as long as we have reason
> to believe this value can be used for push later too.

Ok, got it. So fetchJobs is fine with me.
Mind the difference in the first part, submodule[s] in singular/plural.
I thought submodule as a prefix for any individual submodule, but any
settings applying to all of the submodules, you'd take the plural submodules.*
settings.
--
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: Replicating the default "git log" format with a format string.

[Jacob Keller]

On Thu, Nov 12, 2015 at 9:38 PM, Jeff King  wrote:
>> Is it possible to exactly replicate the default "git log" format with a
>> format string?
>
> Sadly, no, I don't think it is possible with the current format
> specifiers. It would be nice if it was, though.
>
> -Peff
> --

Isn't this something that might be obtainable using the new ref-filter
work being done?

Regards,
Jake
--
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: Replicating the default "git log" format with a format string.

[Jeff King]

On Fri, Nov 13, 2015 at 01:41:23PM -0800, Jacob Keller wrote:

> On Thu, Nov 12, 2015 at 9:38 PM, Jeff King  wrote:
> >> Is it possible to exactly replicate the default "git log" format with a
> >> format string?
> >
> > Sadly, no, I don't think it is possible with the current format
> > specifiers. It would be nice if it was, though.
> 
> Isn't this something that might be obtainable using the new ref-filter
> work being done?

It moves in the right direction, but it isn't enough. I don't recall the
specifics, but I think it has conditional placeholders for things like
"if this is non-empty print it". But there are still some problems:

  - the ref-filter formatting isn't unified with the commit
pretty-printer formatting. So this wouldn't help for a traversal,
anyway.

  - the decoration thing needs a new placeholder. "%d" means "always
show the decoration", but the OP really wants "show the decoration
if the user asked for decorations, and empty otherwise".

-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] rebase-i-exec: Allow space in SHELL_PATH

[Jeff King]

On Fri, Nov 13, 2015 at 04:25:18PM +0100, Fredrik Medley wrote:

> >> - ${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
> >> + "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
> >
> > I think this is the right thing to do (at least I could not think of a
> > case that would be harmed by it, and it certainly fixes your case). It
> > looks like filter-branch would need a similar fix?
> >
> > I think this still isn't resilient to weird meta-characters in the
> > @SHELL_PATH@, but as this is a build-time option, I think it's OK to let
> > people who do
> >
> >   make SHELL_PATH='}"; rm -rf /'
> >
> > hang themselves.
> >
> > -Peff
> 
> Okay, that's what @SHELL_PATH@ stands for. I just read the result
> in the Windows installation that is something like ${SHELL:-/bin/sh}.
> The shell script processor then replaces /bin/sh with
> C:\Program Files\...\bin\sh.

Hmm. Now I'm a bit puzzled. It sounds like the installed file does have
@SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting
containing space is coming from the $SHELL environment variable.

My original "I could not think of a case harmed by it" was because
@SHELL_PATH@ also gets used on the shebang line at the beginning of the
script. And that does not really handle command names with arguments
well (you get one argument, and it better not have spaces in it). Of
course, it _also_ does not handle command names with spaces in them,
either (and there's no room for quoting there).

So anything exotic pretty much has to be coming in from $SHELL, and my
mention of filter-branch is not interesting; it just uses @SHELL_PATH@.

So what are people putting in $SHELL? Obviously a shell path with a
space in it wants the quotes. Do people do more exotic things like:

  SHELL="my-shell --options"

(I think a plausible option might be "--posix" for some shells).  That
would break with your patch.

I dunno. We usually try to err on the side of the status quo, so as to
avoid breaking existing users. But I find the idea of $SHELL with
options reasonably unlikely, so I think your patch is the right
direction. I wish I understood better who was setting $SHELL and why,
though.

-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: git log --merges doesn't show commits as expected

[Jeff King]

On Fri, Nov 13, 2015 at 04:21:36PM +, Dominik Rauch wrote:

> (This is my first post to this mailing list and I couldn't find a FAQ
> section - please excuse me, if I haven't followed all the established
> posting guidelines yet.)

This is the right place. Welcome. :)

> I have the following repository tree:
> 
> C
> |\
> | B
> | /
> A
> 
> Commit A: Parents=(). Initial commit. Contains file foo with content "ABC".
> Commit B: Parents=(A). Represents a commit on some feature branch. Contains 
> file foo with content "XYZ".
> Commit C: Parents=(A, B). Represents a merge commit of a feature branch back 
> to the main branch. Contains file foo with content "XYZ".
> 
> I expected "git log --merges foo" to show C, however, the log is
> empty! Specifying "--full-history" results in the correct history,
> therefore I assume, I misunderstand Git's default history
> simplification algorithm. Unfortunately, the example in the Git docs
> at [1] does not contain the very same situation (although it is
> probably one of the most common situations...).

I don't think "--merges" is relevant to the simplification here. By
asking for "foo", that turns on history simplification. Since the merge
at C took one side directly, that means we can cull the parent link that
leads to A (its foo=ABC did not contribute to the final outcome). And
then C is TREESAME to its only remaining parent (they both have
foo=XYZ), so it can be removed, leaving only commit B to be passed to
the next stage.

And then we apply "--merges", and see that B is not a merge, and so
do not show it (but we still traverse it, of course).

In theory we could apply "--merges" first (by simplifying history to
shrink any chains of non-merges to a single point). But I don't think
there's any way to do that currently with git.

-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] rebase-i-exec: Allow space in SHELL_PATH

[Fredrik Medley]

2015-11-13 23:27 GMT+01:00 Jeff King :
> On Fri, Nov 13, 2015 at 04:25:18PM +0100, Fredrik Medley wrote:
>
>> >> - ${SHELL:-@SHELL_PATH@} -c "$rest" # Actual execution
>> >> + "${SHELL:-@SHELL_PATH@}" -c "$rest" # Actual execution
>> >
>> > I think this is the right thing to do (at least I could not think of a
>> > case that would be harmed by it, and it certainly fixes your case). It
>> > looks like filter-branch would need a similar fix?
>> >
>> > I think this still isn't resilient to weird meta-characters in the
>> > @SHELL_PATH@, but as this is a build-time option, I think it's OK to let
>> > people who do
>> >
>> >   make SHELL_PATH='}"; rm -rf /'
>> >
>> > hang themselves.
>> >
>> > -Peff
>>
>> Okay, that's what @SHELL_PATH@ stands for. I just read the result
>> in the Windows installation that is something like ${SHELL:-/bin/sh}.
>> The shell script processor then replaces /bin/sh with
>> C:\Program Files\...\bin\sh.
>
> Hmm. Now I'm a bit puzzled. It sounds like the installed file does have
> @SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting
> containing space is coming from the $SHELL environment variable.
I wrote the email on another computer. Checking the system where I reinstalled
Git for Windows yesterday shows ${SHELL:-/bin/sh} and SHELL=/usr/bin/bash
When running "git rebase --interactive HEAD^ --exec 'echo $SHELL'", I get
mingw64/libexec/git-core/git-rebase--interactive: line 613:
C:/Program: No such file or directory

Fixing the double quotes, "git rebase --interactive HEAD^ --exec 'echo
$SHELL'" shows
C:/Program Files/Git/usr/bin/bash

>
> My original "I could not think of a case harmed by it" was because
> @SHELL_PATH@ also gets used on the shebang line at the beginning of the
> script. And that does not really handle command names with arguments
> well (you get one argument, and it better not have spaces in it). Of
> course, it _also_ does not handle command names with spaces in them,
> either (and there's no room for quoting there).
>
> So anything exotic pretty much has to be coming in from $SHELL, and my
> mention of filter-branch is not interesting; it just uses @SHELL_PATH@.
>
> So what are people putting in $SHELL? Obviously a shell path with a
> space in it wants the quotes. Do people do more exotic things like:
>
>   SHELL="my-shell --options"
>
> (I think a plausible option might be "--posix" for some shells).  That
> would break with your patch.
I agree, tricky case. Just to accept the fact that it won't work.

>
> I dunno. We usually try to err on the side of the status quo, so as to
> avoid breaking existing users. But I find the idea of $SHELL with
> options reasonably unlikely, so I think your patch is the right
> direction. I wish I understood better who was setting $SHELL and why,
> though.
I don't understand where $SHELL is being set neither.

>
> -Peff

/Fredrik
--
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] rebase-i-exec: Allow space in SHELL_PATH

[Jeff King]

On Fri, Nov 13, 2015 at 11:47:40PM +0100, Fredrik Medley wrote:

> > Hmm. Now I'm a bit puzzled. It sounds like the installed file does have
> > @SHELL_PATH@ set to /bin/sh, which is normal. And presumably the setting
> > containing space is coming from the $SHELL environment variable.
> I wrote the email on another computer. Checking the system where I reinstalled
> Git for Windows yesterday shows ${SHELL:-/bin/sh} and SHELL=/usr/bin/bash
> When running "git rebase --interactive HEAD^ --exec 'echo $SHELL'", I get
> mingw64/libexec/git-core/git-rebase--interactive: line 613:
> C:/Program: No such file or directory
> 
> Fixing the double quotes, "git rebase --interactive HEAD^ --exec 'echo
> $SHELL'" shows
> C:/Program Files/Git/usr/bin/bash

It looks like bash on your system may simply be filling in its absolute
path into the $SHELL variable. This seems like an interesting bash-ism:

  $ unset SHELL
  $ dash -c 'echo $SHELL'

  $ bash -c 'echo $SHELL'
  /bin/bash

If $SHELL is already set and exported, it will not override it, but in
your case $SHELL is probably local to each individual bash invocation.

So that at least kind-of makes sense. It's possible somebody could be
doing something clever with $SHELL, but I kind of doubt it. If they want
to do something clever, it is much easier to put it directly on the exec
line, and have the normal $SHELL run their clever thing.

-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: Re: git log --merges doesn't show commits as expected

[Dominik Rauch]

Thank you Jeff for your elaboration! The algorithm is now clear to me and I can 
see why the log has been empty.

However, I don't think the default simplification algorithm is a good default 
when used in combination with "--merges". "git log --merges file" looks very 
natural if I want to answer the question "When has file been merged into my 
master/develop/whatsoever branch?" and it just doesn't work. Is there a simpler 
way to answer that question? What is the primary use of "--merges" if not 
combined with "--full-history" or at least "--first-parent"? I would only see 
merges where file has been a "merge conflict". What do you think about implying 
"--full-history" when using "--merges"?

Best regards,
Dominik

> > (This is my first post to this mailing list and I couldn't find a FAQ 
> > section - please excuse me, if I haven't followed all the established 
> > posting guidelines yet.)
> 
> This is the right place. Welcome. :)
> 
> > I have the following repository tree:
> > 
> > C
> > |\
> > | B
> > | /
> > A
> > 
> > Commit A: Parents=(). Initial commit. Contains file foo with content "ABC".
> > Commit B: Parents=(A). Represents a commit on some feature branch. Contains 
> > file foo with content "XYZ".
> > Commit C: Parents=(A, B). Represents a merge commit of a feature branch 
> > back to the main branch. Contains file foo with content "XYZ".
> > 
> > I expected "git log --merges foo" to show C, however, the log is 
> > empty! Specifying "--full-history" results in the correct history, 
> > therefore I assume, I misunderstand Git's default history 
> > simplification algorithm. Unfortunately, the example in the Git docs 
> > at [1] does not contain the very same situation (although it is 
> > probably one of the most common situations...).
> 
> I don't think "--merges" is relevant to the simplification here. By asking 
> for "foo", that turns on history simplification. Since the merge at C took 
> one side directly, that means we can cull the parent link that leads to A 
> (its foo=ABC did not contribute to the final outcome). And then C is TREESAME 
> to its only remaining parent (they both have foo=XYZ), so it can be removed, 
> leaving only commit B to be passed to the next stage.
> 
> And then we apply "--merges", and see that B is not a merge, and so do not 
> show it (but we still traverse it, of course).
> 
> In theory we could apply "--merges" first (by simplifying history to shrink 
> any chains of non-merges to a single point). But I don't think there's any 
> way to do that currently with git.
> 
> -Peff

Re: [PATCH v2] add test to demonstrate that shallow recursive clones fail

[Stefan Beller]

On Fri, Nov 13, 2015 at 10:41 AM, Stefan Beller  wrote:
> On Thu, Nov 12, 2015 at 9:35 PM, Jeff King  wrote:
>>
>> Hrm. Do we want to make these workarounds work correctly? Or is the
>> final solution going to be that the first command you gave simply works,
>> and no workarounds are needed.  If the latter, I wonder if we want to be
>> adding tests for the workarounds in the first place.
>
> I think we want to make the final solution just work. I dug into that and it 
> is
> harder than expected. I may even call it a bug. The bug doesn't occur often as
> it is only triggered by things like rewriting history (forced pushes)
> or a short dpeth
> argument.
>
> So if you invoke "git clone --recursive", it will internally just
> delegate the submodule
> handling to "git submodule update --init --recursive", which then (as
> the submodule
> doesn't exist yet) will delegate the cloning to "git submodule--helper
> clone", which
> will then call git clone for the actual cloning.
>
> However in this whole chain of commands we never pass around the actual sha1
> we need. The strategy is to clone first and then checkout the sha1, which the
> superprojects wants to see. The desired sha1 was hopefully included in
> the cloning,
> so we can check it out.
>
> But the sha1 may not be present if we have a very short depth argument, or if 
> we
> rewrote history. In case of a short depth argument, consider the
> following history:
>
> ... <- A <- B
>
> A is the recorded sha1 in the superproject, whereas B is the HEAD in the
> remote you're cloning from. If cloning with depth=1, the most naive way
> would have been to pass on the depth argument down the command chain,
> but then we would end up cloning B with no further depth, and upon checkout
> we cannot find A.
>
> In case of the rewritten history, consider:
>
> .. < - C <- B
>  \
>   A
>
> whereas A is the recorded sha1 in the superproject, but on a different branch
> (or even just a dangling commit. but used to be on master).
> B is the master branch. In case we pass on --depth to cloning the submodule,
> --single-branch is implied by --depth, so we would not clone A. In case of
> A being a dangling commit, we wouldn't even clone it without the depth 
> argument.
>
> So I propose:
>  * similar to fetch, we enable clone to obtain a specific sha1 from remote.
>  * we explicitly pass the submodule sha1 as recorded in the superproject
>to the submodule fetch/clone in case we follow the exact sha1. In case of
>--remote or the branch field present in the superprojects .gitmodule file,
>we can just pass the branch name.
>
> Thanks,
> Stefan

+cc Junio, Duy

So cloning from an arbitrary SHA1 is not a new thing I just came up with,
but has been discussed before[1].

Junio wrote on Oct 09, 2014:
> This is so non-standard a thing to do that I doubt it is worth
> supporting with "git clone".  "git clone --branch", which is about
"> I want to follow that particular branch", would not mesh well with
> "I want to see the history that leads to this exact commit", either.
> You would not know which branch(es) is that exact commit is on in
> the first place.

I disagree with this. This is the *exact* thing you actually want to do when
dealing with submodules. When fetching/cloning for a submodule, you want
to obtain the exact sha1, instead of a branch (which happens to be supported
too, but is not the original use case with submodules.)

> The "uploadpack.allowtipsha1inwant" is a wrong configuration to tie
> this into.  The intent of the configuration is to allow *ONLY*
> commits at the tip of the (possibly hidden) refs to be asked for.
> Those who want to hide some refs using "uploadpack.hiderefs" may
> want to enable "allowtipsha1inwant" to allow the tips of the hidden
> refs while still disallowing a request to fetch any random reachable
> commit not at the tip.

If the server contains at least one superproject/submodule, there is a legit
use case for fetching an exact sha1, which isn't a tip of a branch, but may
be in any branch  or even in no branch at all. So I wonder how we want
to add that as a non-hacky solution to allow for fetching specific sha1s
as we still need to differentiate between obligerated (forced pushed,
"go away sha1s") and legit submodule pointer sha1s.

As we don't want to lookup in a superproject (we don't even know
which superproject we'd need to look into), we can either go for a
more liberal sha1 fetching attitude or somehow modify the submodule
repository to mark sha1s which are good to fetch as "submodule update"
fetches.

Proposal:
Could we have a refs/submodules/tracking which is tracking all the
sha1s a superproject ever pointed to? That ref would need to be
maintained by the superproject. If there is a forced push to the
superproject, it would also need to obliterate some of the history
in that special ref.
Problems with this proposal:
How do we care about multiple superprojects,
or superprojects wi

Re: [PATCH] allow hooks to ignore their standard input stream

[Jeff King]

On Fri, Nov 13, 2015 at 10:33:03AM +0100, Clemens Buchacher wrote:

> Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input 
> stream)
> the pre-receive and post-receive hooks ignore SIGPIPE. Do the same for the
> remaining hooks pre-push and post-rewrite, which read from standard input. The
> same arguments for ignoring SIGPIPE apply.
> 
> Performance improvements which allow us to enable the test by
> default by Jeff King.

I actually did add a new test. The existing one was basically this:

> +test_expect_success 'filling pipe buffer does not cause failure' '
> + git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
> + test_cmp expected actual
> +'

It actually _does_ read all of the input, but I guess is making sure we
call write() in a loop. I don't know if this is even worth keeping.

Can you think of a good reason that it is checking something
interesting?

> +test_expect_success 'sigpipe does not cause pre-push hook failure' '
> + echo "exit 0" | write_script "$HOOK" &&
> + git push parent1 "refs/heads/b/*:refs/heads/c/*"
> +'

This is the new one which checks your code.

-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: Re: git log --merges doesn't show commits as expected

[Jeff King]

On Fri, Nov 13, 2015 at 11:10:44PM +, Dominik Rauch wrote:

> However, I don't think the default simplification algorithm is a good
> default when used in combination with "--merges". "git log --merges
> file" looks very natural if I want to answer the question "When has
> file been merged into my master/develop/whatsoever branch?" and it
> just doesn't work. Is there a simpler way to answer that question?

I would generally use --first-parent to just walk down the commits that
first-appeared on the master branch. You don't really need --merges
then; you see all direct changes on the branch, and if you have a topic
branch workflow, you'll essentially only see merges anyway.

> What is the primary use of "--merges" if not combined with
> "--full-history" or at least "--first-parent"?

Using "--merges" is orthogonal to --full-history. You might not even be
simplifying history at all (the reason we do simplification in your
example is because of the "foo" pathspec you provide).

I don't use --merges by itself very much (I use --no-merges much more
often).

> What do you think about implying "--full-history" when using "--merges"?

I haven't thought hard about it, but my first instinct is that we should
not. Even if they are _mostly_ used together, they are orthogonal
concepts, and we'd be breaking backwards compatibility.

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

Re: [PATCH v2] add test to demonstrate that shallow recursive clones fail

[Jeff King]

On Fri, Nov 13, 2015 at 03:16:01PM -0800, Stefan Beller wrote:

> Junio wrote on Oct 09, 2014:
> > This is so non-standard a thing to do that I doubt it is worth
> > supporting with "git clone".  "git clone --branch", which is about
> "> I want to follow that particular branch", would not mesh well with
> > "I want to see the history that leads to this exact commit", either.
> > You would not know which branch(es) is that exact commit is on in
> > the first place.
> 
> I disagree with this. This is the *exact* thing you actually want to do when
> dealing with submodules. When fetching/cloning for a submodule, you want
> to obtain the exact sha1, instead of a branch (which happens to be supported
> too, but is not the original use case with submodules.)

I think this is already implemented in 68ee628 (upload-pack: optionally
allow fetching reachable sha1, 2015-05-21), isn't it?

> If the server contains at least one superproject/submodule, there is a legit
> use case for fetching an exact sha1, which isn't a tip of a branch, but may
> be in any branch  or even in no branch at all.

The patch above doesn't handle "no branch at all", but I'm not sure if
we want to (it violates git's usual access model; moreover, a git
repository does not necessarily have all ancestors of an unreachable
object, though these days it usually does).

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

Re: [PATCH v2] add test to demonstrate that shallow recursive clones fail

[Jeff King]

On Fri, Nov 13, 2015 at 06:38:07PM -0500, Jeff King wrote:

> On Fri, Nov 13, 2015 at 03:16:01PM -0800, Stefan Beller wrote:
> 
> > Junio wrote on Oct 09, 2014:
> > > This is so non-standard a thing to do that I doubt it is worth
> > > supporting with "git clone".  "git clone --branch", which is about
> > "> I want to follow that particular branch", would not mesh well with
> > > "I want to see the history that leads to this exact commit", either.
> > > You would not know which branch(es) is that exact commit is on in
> > > the first place.
> > 
> > I disagree with this. This is the *exact* thing you actually want to do when
> > dealing with submodules. When fetching/cloning for a submodule, you want
> > to obtain the exact sha1, instead of a branch (which happens to be supported
> > too, but is not the original use case with submodules.)
> 
> I think this is already implemented in 68ee628 (upload-pack: optionally
> allow fetching reachable sha1, 2015-05-21), isn't it?

Note that this just implements the server side. I think to use this with
submodules right now, you'd have to manually "git init && git fetch" in
the submodule. It might make sense to teach clone to handle this, to
avoid the submodule code duplicating what the clone code does.

-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: git clean performance issues

[Jeff King]

On Fri, Nov 13, 2015 at 03:19:07PM +0100, Andreas Krey wrote:

> On Sat, 04 Apr 2015 15:55:07 +, Jeff King wrote:
> ...
> > I think this is the same issue that was discussed here:
> > 
> >   http://thread.gmane.org/gmane.comp.version-control.git/265560/focus&5585
> > 
> > There is some discussion of a possible fix in that thread. I was hoping
> > that Andreas was going to look further and produce a patch, but I
> > imagine he got busy with other things.
> 
> That about sums it up. However I now have a similar issue;
> git ls-files shows the same behaviour (takes relatively
> forever at 100% CPU), and runs instantly with my patch
> from back then. Nothing seems to have changed, so I
> may have another chance to look into this.

Yeah, I think Erik's patch in 0179ca7 (clean: improve performance when
removing lots of directories, 2015-06-15) handles the git-clean case the
way we want to, but all of the other calls to resolve_gitlink_ref need
to be inspected and fixed similarly.

The one your are hitting with ls-files is probably in dir.c:treat_directory.

-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] refs.c: get_ref_cache: use a bucket hash

[Jeff King]

On Fri, Nov 13, 2015 at 04:29:15PM +0100, Andreas Krey wrote:

> > Likewise, I think dir.c:remove_dir_recurse is in a similar boat.
> > Grepping for resolve_gitlink_ref, it looks like there may be others,
> > too.
> 
> Can't we handle this in resolve_gitlink_ref itself? As I understand it,
> it should resolve a ref (here "HEAD") when path points to a submodule.
> When there isn't one it should return -1, so:

I'm not sure. I think part of the change to git-clean was that
is_git_directory() is a _better_ check than "can we resolve HEAD?"
because it covers empty repos, too.

> diff --git a/refs.c b/refs.c
> index 132eff5..f8648c5 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1553,6 +1553,10 @@ int resolve_gitlink_ref(const char *path, const char 
> *refname, unsigned char *sh
>   if (!len)
>   return -1;
>   submodule = xstrndup(path, len);
> + if (!is_git_directory(submodule)) {
> + free(submodule);
> + return -1;
> + }
>   refs = get_ref_cache(submodule);
>   free(submodule);
> 
> I'm way too little into the code to see what may this may get wrong.

I don't think it produces wrong outcomes, but I think it's sub-optimal.
In cases where we already have a ref cache, we'll hit the filesystem for
each lookup to re-confirm what we already know. That doesn't affect your
case, but it does when we actually _do_ have a submodule.

So if we were to follow this route, I think it would go better in
get_ref_cache itself (right after we determine there is no existing
cache, but before we call create_ref_cache()).

> But this, as well as the old hash-ref-cache patch speeds me
> up considerably, in this case a git ls-files -o from half a
> minute of mostly user CPU to a second.

Right, that makes sense to me.

> > All of these should be using the same test, I think. Doing that with
> > is_git_directory() is probably OK. It is a little more expensive than we
> > might want for mass-use (it actually opens and parses the HEAD file in
> > each directory),
> 
> This happens as well when we let resolve_gitlink_ref run its old course.
> (It (ls-files) even seems to try to open .git and then .git/HEAD, even
> if the former fails with ENOENT.)

Yes, I think my earlier comment that you are quoting was just misguided.
We only do the extra work if the directory actually does look like a
gitdir, and the many-directories case we are optimizing here is the
opposite of that.

So summing up, I think:

  1. We could get by with teaching get_ref_cache not to auto-create ref
 caches for non-git-directories.

  2. But for a little more work, pushing the is_git_directory() check
 out to the call-sites gives us probably saner semantics overall.

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

Re: [PATCH v2] add test to demonstrate that shallow recursive clones fail

[Stefan Beller]

On Fri, Nov 13, 2015 at 3:41 PM, Jeff King  wrote:
> On Fri, Nov 13, 2015 at 06:38:07PM -0500, Jeff King wrote:
>
>> On Fri, Nov 13, 2015 at 03:16:01PM -0800, Stefan Beller wrote:
>>
>> > Junio wrote on Oct 09, 2014:
>> > > This is so non-standard a thing to do that I doubt it is worth
>> > > supporting with "git clone".  "git clone --branch", which is about
>> > "> I want to follow that particular branch", would not mesh well with
>> > > "I want to see the history that leads to this exact commit", either.
>> > > You would not know which branch(es) is that exact commit is on in
>> > > the first place.
>> >
>> > I disagree with this. This is the *exact* thing you actually want to do 
>> > when
>> > dealing with submodules. When fetching/cloning for a submodule, you want
>> > to obtain the exact sha1, instead of a branch (which happens to be 
>> > supported
>> > too, but is not the original use case with submodules.)
>>
>> I think this is already implemented in 68ee628 (upload-pack: optionally
>> allow fetching reachable sha1, 2015-05-21), isn't it?
>
> Note that this just implements the server side. I think to use this with
> submodules right now, you'd have to manually "git init && git fetch" in
> the submodule. It might make sense to teach clone to handle this, to
> avoid the submodule code duplicating what the clone code does.

Yes I want to add it to clone, as that is a prerequisite for making
git clone --recursive --depth 1 to work as you'd expect. (such that
the submodule can be cloned&checkout instead of rewriting that to be
init&fetch.

Thanks for pointing out that we already have some kind of server support.

I wonder if we should add an additional way to make fetching only some
sha1s possible. ("I don't want users to fetch any sha1, but only those
where superprojects point{ed} to", even if you force push a superproject,
you want to want to only allow fetching all sha1s which exist in the current
superprojects branch.)

Maybe our emails crossed, but in the other mail I pointed out we could use
some sort of hidden ref (refs/superprojects/*) for that, which are
allowed to mark
any sort of sha1, which are allowed in the superproject/submodule context
to be fetched.

So whenever you push to a superproject (a project that has a gitlink),
we would need to check serverside if that submodule is at us and mark the
correct sha1s in the submodule. Then you can disallow fetching most of the sha1s
but still could have a correctly working submodule update mechanism.

Thanks,
Stefan


>
> -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 3/3] gc: Clean garbage .bitmap files from pack dir

[Doug Kelly]

Similar to cleaning up excess .idx files, clean any garbage .bitmap
files that are not otherwise associated with any .idx/.pack files.

Signed-off-by: Doug Kelly 
---
 builtin/gc.c | 12 ++--
 t/t5304-prune.sh |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad..7ddf071 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -58,8 +58,16 @@ static void clean_pack_garbage(void)
 
 static void report_pack_garbage(unsigned seen_bits, const char *path)
 {
-   if (seen_bits == PACKDIR_FILE_IDX)
-   string_list_append(&pack_garbage, path);
+   if (seen_bits & PACKDIR_FILE_IDX ||
+   seen_bits & PACKDIR_FILE_BITMAP) {
+   const char *dot = strrchr(path, '.');
+   if (dot) {
+   int baselen = dot - path + 1;
+   if (!strcmp(path+baselen, "idx") ||
+   !strcmp(path+baselen, "bitmap"))
+   string_list_append(&pack_garbage, path);
+   }
+   }
 }
 
 static void git_config_date_string(const char *key, const char **output)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 4fa6e7a..9f9f263 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -257,7 +257,7 @@ EOF
test_cmp expected actual
 '
 
-test_expect_failure 'clean pack garbage with gc' '
+test_expect_success 'clean pack garbage with gc' '
test_when_finished "rm -f .git/objects/pack/fake*" &&
test_when_finished "rm -f .git/objects/pack/foo*" &&
: >.git/objects/pack/foo.keep &&
-- 
2.6.1

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

[PATCH 2/3] t5304: Add test for .bitmap garbage files

[Doug Kelly]

When checking for pack garbage, .bitmap files are now detected as
garbage when not associated with another .pack/.idx file.

Signed-off-by: Doug Kelly 
---
 t/t5304-prune.sh | 24 +---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 1ea8279..4fa6e7a 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '
: >.git/objects/pack/fake.idx &&
: >.git/objects/pack/fake2.keep &&
: >.git/objects/pack/fake3.idx &&
+   : >.git/objects/pack/fake4.bitmap &&
+   : >.git/objects/pack/fake5.bitmap &&
+   : >.git/objects/pack/fake5.idx &&
+   : >.git/objects/pack/fake6.keep &&
+   : >.git/objects/pack/fake6.bitmap &&
+   : >.git/objects/pack/fake6.idx &&
git count-objects -v 2>stderr &&
grep "index file .git/objects/pack/fake.idx is too small" stderr &&
grep "^warning:" stderr | sort >actual &&
@@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar
 warning: garbage found: .git/objects/pack/foo
 warning: garbage found: .git/objects/pack/foo.bar
 warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep
+warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
 warning: no corresponding .pack: .git/objects/pack/fake3.idx
+warning: no corresponding .pack: .git/objects/pack/fake5.bitmap
+warning: no corresponding .pack: .git/objects/pack/fake5.idx
+warning: no corresponding .pack: .git/objects/pack/fake6.bitmap
+warning: no corresponding .pack: .git/objects/pack/fake6.idx
+warning: no corresponding .pack: .git/objects/pack/fake6.keep
 EOF
test_cmp expected actual
 '
 
-test_expect_success 'clean pack garbage with gc' '
+test_expect_failure 'clean pack garbage with gc' '
test_when_finished "rm -f .git/objects/pack/fake*" &&
test_when_finished "rm -f .git/objects/pack/foo*" &&
: >.git/objects/pack/foo.keep &&
@@ -254,15 +266,21 @@ test_expect_success 'clean pack garbage with gc' '
: >.git/objects/pack/fake2.keep &&
: >.git/objects/pack/fake2.idx &&
: >.git/objects/pack/fake3.keep &&
+   : >.git/objects/pack/fake4.bitmap &&
+   : >.git/objects/pack/fake5.bitmap &&
+   : >.git/objects/pack/fake5.idx &&
+   : >.git/objects/pack/fake6.keep &&
+   : >.git/objects/pack/fake6.bitmap &&
+   : >.git/objects/pack/fake6.idx &&
git gc &&
git count-objects -v 2>stderr &&
grep "^warning:" stderr | sort >actual &&
cat >expected <<\EOF &&
+warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep
 warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep
+warning: no corresponding .idx or .pack: .git/objects/pack/fake6.keep
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
-warning: no corresponding .pack: .git/objects/pack/fake2.idx
-warning: no corresponding .pack: .git/objects/pack/fake2.keep
 EOF
test_cmp expected actual
 '
-- 
2.6.1

--
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 0/3] Add cleanup for garbage .bitmap files

[Doug Kelly]

Following Peff and Junio's comments when adding support for cleaning garbage
.idx files left in the pack directory, this patch introduces the ability to
detect garbage .bitmap files.  Additionally, .keep files are still reported,
but no action is taken to clean them.

This includes some refactor around count-objects' report_pack_garbage handler,
to make it more extensible when adding understanding for different file types.
Testing shows this working, but it may be a section to provide extra scrutiny
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

[PATCH 1/3] prepare_packed_git(): find more garbage

[Doug Kelly]

.bitmap and .keep files without .idx/.pack don't make much sense, so
make sure these are reported as garbage as well.  At the same time,
refactoring report_garbage to handle extra bits.

Signed-off-by: Doug Kelly 
---
 builtin/count-objects.c | 16 ++--
 cache.h |  4 +++-
 sha1_file.c | 17 +++--
 t/t5304-prune.sh|  2 ++
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..1637037 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -17,19 +17,15 @@ static off_t loose_size;
 
 static const char *bits_to_msg(unsigned seen_bits)
 {
-   switch (seen_bits) {
-   case 0:
-   return "no corresponding .idx or .pack";
-   case PACKDIR_FILE_GARBAGE:
+   if (seen_bits ==  PACKDIR_FILE_GARBAGE)
return "garbage found";
-   case PACKDIR_FILE_PACK:
+   else if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ ~PACKDIR_FILE_IDX)
return "no corresponding .idx";
-   case PACKDIR_FILE_IDX:
+   else if (seen_bits & PACKDIR_FILE_IDX && seen_bits ^ ~PACKDIR_FILE_PACK)
return "no corresponding .pack";
-   case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:
-   default:
-   return NULL;
-   }
+   else if (seen_bits == 0 || seen_bits ^ 
~(PACKDIR_FILE_IDX|PACKDIR_FILE_PACK))
+   return "no corresponding .idx or .pack";
+   return NULL;
 }
 
 static void real_report_garbage(unsigned seen_bits, const char *path)
diff --git a/cache.h b/cache.h
index 736abc0..5b9d791 100644
--- a/cache.h
+++ b/cache.h
@@ -1292,7 +1292,9 @@ extern struct packed_git *parse_pack_index(unsigned char 
*sha1, const char *idx_
 /* A hook to report invalid files in pack directory */
 #define PACKDIR_FILE_PACK 1
 #define PACKDIR_FILE_IDX 2
-#define PACKDIR_FILE_GARBAGE 4
+#define PACKDIR_FILE_BITMAP 4
+#define PACKDIR_FILE_KEEP 8
+#define PACKDIR_FILE_GARBAGE 16
 extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
 extern void prepare_packed_git(void);
diff --git a/sha1_file.c b/sha1_file.c
index 3d56746..5f939e4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,
if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
return;
 
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
+   return;
+
+   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
+   return;
+
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
+   return;
+
for (; first < last; first++)
report_garbage(seen_bits, list->items[first].string);
 }
@@ -1256,9 +1265,13 @@ static void report_pack_garbage(struct string_list *list)
first = i;
}
if (!strcmp(path + baselen, "pack"))
-   seen_bits |= 1;
+   seen_bits |= PACKDIR_FILE_PACK;
else if (!strcmp(path + baselen, "idx"))
-   seen_bits |= 2;
+   seen_bits |= PACKDIR_FILE_IDX;
+   else if (!strcmp(path + baselen, "bitmap"))
+   seen_bits |= PACKDIR_FILE_BITMAP;
+   else if (!strcmp(path + baselen, "keep"))
+   seen_bits |= PACKDIR_FILE_KEEP;
}
report_helper(list, seen_bits, first, list->nr);
 }
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index def203c..1ea8279 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '
 warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
+warning: no corresponding .pack: .git/objects/pack/fake2.idx
+warning: no corresponding .pack: .git/objects/pack/fake2.keep
 EOF
test_cmp expected actual
 '
-- 
2.6.1

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

Test. Please ignore

[team]

Test message. Please ignore.
--
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/3] prepare_packed_git(): find more garbage

[Stefan Beller]

> +   else if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ 
> ~PACKDIR_FILE_IDX)

as just talked about: did you mention && !(seen_bits & FILE_IDX)
>
> +   if (seen_bits == 
> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
> +   return;
> +
> +   if (seen_bits == 
> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
> +   return;
> +
> +   if (seen_bits == 
> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
> +   return;

I wonder if this should be rewritten as
if (seen_bits & FILE_PACK && seen_bits & FILE_IDX
&& (seen_bits & FILE_KEEP || seen_bits & BITMAP))
return;

to dense it a bit. ;)
--
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 3/3] gc: Clean garbage .bitmap files from pack dir

[Doug Kelly]

Similar to cleaning up excess .idx files, clean any garbage .bitmap
files that are not otherwise associated with any .idx/.pack files.

Signed-off-by: Doug Kelly 
---
 builtin/gc.c | 12 ++--
 t/t5304-prune.sh |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad..7ddf071 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -58,8 +58,16 @@ static void clean_pack_garbage(void)
 
 static void report_pack_garbage(unsigned seen_bits, const char *path)
 {
-   if (seen_bits == PACKDIR_FILE_IDX)
-   string_list_append(&pack_garbage, path);
+   if (seen_bits & PACKDIR_FILE_IDX ||
+   seen_bits & PACKDIR_FILE_BITMAP) {
+   const char *dot = strrchr(path, '.');
+   if (dot) {
+   int baselen = dot - path + 1;
+   if (!strcmp(path+baselen, "idx") ||
+   !strcmp(path+baselen, "bitmap"))
+   string_list_append(&pack_garbage, path);
+   }
+   }
 }
 
 static void git_config_date_string(const char *key, const char **output)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 4fa6e7a..9f9f263 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -257,7 +257,7 @@ EOF
test_cmp expected actual
 '
 
-test_expect_failure 'clean pack garbage with gc' '
+test_expect_success 'clean pack garbage with gc' '
test_when_finished "rm -f .git/objects/pack/fake*" &&
test_when_finished "rm -f .git/objects/pack/foo*" &&
: >.git/objects/pack/foo.keep &&
-- 
2.6.1

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

[PATCH 2/3] t5304: Add test for .bitmap garbage files

[Doug Kelly]

When checking for pack garbage, .bitmap files are now detected as
garbage when not associated with another .pack/.idx file.

Signed-off-by: Doug Kelly 
---
 t/t5304-prune.sh | 24 +---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 1ea8279..4fa6e7a 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '
: >.git/objects/pack/fake.idx &&
: >.git/objects/pack/fake2.keep &&
: >.git/objects/pack/fake3.idx &&
+   : >.git/objects/pack/fake4.bitmap &&
+   : >.git/objects/pack/fake5.bitmap &&
+   : >.git/objects/pack/fake5.idx &&
+   : >.git/objects/pack/fake6.keep &&
+   : >.git/objects/pack/fake6.bitmap &&
+   : >.git/objects/pack/fake6.idx &&
git count-objects -v 2>stderr &&
grep "index file .git/objects/pack/fake.idx is too small" stderr &&
grep "^warning:" stderr | sort >actual &&
@@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar
 warning: garbage found: .git/objects/pack/foo
 warning: garbage found: .git/objects/pack/foo.bar
 warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep
+warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
 warning: no corresponding .pack: .git/objects/pack/fake3.idx
+warning: no corresponding .pack: .git/objects/pack/fake5.bitmap
+warning: no corresponding .pack: .git/objects/pack/fake5.idx
+warning: no corresponding .pack: .git/objects/pack/fake6.bitmap
+warning: no corresponding .pack: .git/objects/pack/fake6.idx
+warning: no corresponding .pack: .git/objects/pack/fake6.keep
 EOF
test_cmp expected actual
 '
 
-test_expect_success 'clean pack garbage with gc' '
+test_expect_failure 'clean pack garbage with gc' '
test_when_finished "rm -f .git/objects/pack/fake*" &&
test_when_finished "rm -f .git/objects/pack/foo*" &&
: >.git/objects/pack/foo.keep &&
@@ -254,15 +266,21 @@ test_expect_success 'clean pack garbage with gc' '
: >.git/objects/pack/fake2.keep &&
: >.git/objects/pack/fake2.idx &&
: >.git/objects/pack/fake3.keep &&
+   : >.git/objects/pack/fake4.bitmap &&
+   : >.git/objects/pack/fake5.bitmap &&
+   : >.git/objects/pack/fake5.idx &&
+   : >.git/objects/pack/fake6.keep &&
+   : >.git/objects/pack/fake6.bitmap &&
+   : >.git/objects/pack/fake6.idx &&
git gc &&
git count-objects -v 2>stderr &&
grep "^warning:" stderr | sort >actual &&
cat >expected <<\EOF &&
+warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep
 warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep
+warning: no corresponding .idx or .pack: .git/objects/pack/fake6.keep
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
-warning: no corresponding .pack: .git/objects/pack/fake2.idx
-warning: no corresponding .pack: .git/objects/pack/fake2.keep
 EOF
test_cmp expected actual
 '
-- 
2.6.1

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

[PATCH 1/3] prepare_packed_git(): find more garbage

[Doug Kelly]

.bitmap and .keep files without .idx/.pack don't make much sense, so
make sure these are reported as garbage as well.  At the same time,
refactoring report_garbage to handle extra bits.

Signed-off-by: Doug Kelly 
---
 builtin/count-objects.c | 16 ++--
 cache.h |  4 +++-
 sha1_file.c | 17 +++--
 t/t5304-prune.sh|  2 ++
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..1637037 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -17,19 +17,15 @@ static off_t loose_size;
 
 static const char *bits_to_msg(unsigned seen_bits)
 {
-   switch (seen_bits) {
-   case 0:
-   return "no corresponding .idx or .pack";
-   case PACKDIR_FILE_GARBAGE:
+   if (seen_bits ==  PACKDIR_FILE_GARBAGE)
return "garbage found";
-   case PACKDIR_FILE_PACK:
+   else if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ ~PACKDIR_FILE_IDX)
return "no corresponding .idx";
-   case PACKDIR_FILE_IDX:
+   else if (seen_bits & PACKDIR_FILE_IDX && seen_bits ^ ~PACKDIR_FILE_PACK)
return "no corresponding .pack";
-   case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:
-   default:
-   return NULL;
-   }
+   else if (seen_bits == 0 || seen_bits ^ 
~(PACKDIR_FILE_IDX|PACKDIR_FILE_PACK))
+   return "no corresponding .idx or .pack";
+   return NULL;
 }
 
 static void real_report_garbage(unsigned seen_bits, const char *path)
diff --git a/cache.h b/cache.h
index 736abc0..5b9d791 100644
--- a/cache.h
+++ b/cache.h
@@ -1292,7 +1292,9 @@ extern struct packed_git *parse_pack_index(unsigned char 
*sha1, const char *idx_
 /* A hook to report invalid files in pack directory */
 #define PACKDIR_FILE_PACK 1
 #define PACKDIR_FILE_IDX 2
-#define PACKDIR_FILE_GARBAGE 4
+#define PACKDIR_FILE_BITMAP 4
+#define PACKDIR_FILE_KEEP 8
+#define PACKDIR_FILE_GARBAGE 16
 extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
 extern void prepare_packed_git(void);
diff --git a/sha1_file.c b/sha1_file.c
index 3d56746..5f939e4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,
if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
return;
 
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
+   return;
+
+   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
+   return;
+
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
+   return;
+
for (; first < last; first++)
report_garbage(seen_bits, list->items[first].string);
 }
@@ -1256,9 +1265,13 @@ static void report_pack_garbage(struct string_list *list)
first = i;
}
if (!strcmp(path + baselen, "pack"))
-   seen_bits |= 1;
+   seen_bits |= PACKDIR_FILE_PACK;
else if (!strcmp(path + baselen, "idx"))
-   seen_bits |= 2;
+   seen_bits |= PACKDIR_FILE_IDX;
+   else if (!strcmp(path + baselen, "bitmap"))
+   seen_bits |= PACKDIR_FILE_BITMAP;
+   else if (!strcmp(path + baselen, "keep"))
+   seen_bits |= PACKDIR_FILE_KEEP;
}
report_helper(list, seen_bits, first, list->nr);
 }
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index def203c..1ea8279 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '
 warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
+warning: no corresponding .pack: .git/objects/pack/fake2.idx
+warning: no corresponding .pack: .git/objects/pack/fake2.keep
 EOF
test_cmp expected actual
 '
-- 
2.6.1

--
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/3] prepare_packed_git(): find more garbage

[Doug Kelly]

Yes, without a doubt.  I think I'm blaming this one on being late on a
Friday afternoon, and really not thinking out the logic clearly. :)

On Fri, Nov 13, 2015 at 4:43 PM, Stefan Beller  wrote:
>> +   else if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ 
>> ~PACKDIR_FILE_IDX)
>
> as just talked about: did you mention && !(seen_bits & FILE_IDX)
>>
>> +   if (seen_bits == 
>> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
>> +   return;
>> +
>> +   if (seen_bits == 
>> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
>> +   return;
>> +
>> +   if (seen_bits == 
>> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
>> +   return;
>
> I wonder if this should be rewritten as
> if (seen_bits & FILE_PACK && seen_bits & FILE_IDX
> && (seen_bits & FILE_KEEP || seen_bits & BITMAP))
> return;
>
> to dense it a bit. ;)
--
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] t5304: Add test for .bitmap garbage files

[Stefan Beller]

On Fri, Nov 13, 2015 at 4:10 PM, Doug Kelly  wrote:
> When checking for pack garbage, .bitmap files are now detected as
> garbage when not associated with another .pack/.idx file.
>
> Signed-off-by: Doug Kelly 
> ---
>  t/t5304-prune.sh | 24 +---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index 1ea8279..4fa6e7a 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' 
> '
> : >.git/objects/pack/fake.idx &&
> : >.git/objects/pack/fake2.keep &&
> : >.git/objects/pack/fake3.idx &&
> +   : >.git/objects/pack/fake4.bitmap &&
> +   : >.git/objects/pack/fake5.bitmap &&
> +   : >.git/objects/pack/fake5.idx &&
> +   : >.git/objects/pack/fake6.keep &&
> +   : >.git/objects/pack/fake6.bitmap &&
> +   : >.git/objects/pack/fake6.idx &&
> git count-objects -v 2>stderr &&
> grep "index file .git/objects/pack/fake.idx is too small" stderr &&
> grep "^warning:" stderr | sort >actual &&
> @@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar
>  warning: garbage found: .git/objects/pack/foo
>  warning: garbage found: .git/objects/pack/foo.bar
>  warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep
> +warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap

Do we want to split that up further, into

no corresponding .idx and .pack:...

to tell that actually both files are missing and we know it?

>  warning: no corresponding .idx: .git/objects/pack/foo.keep
>  warning: no corresponding .idx: .git/objects/pack/foo.pack
>  warning: no corresponding .pack: .git/objects/pack/fake3.idx
> +warning: no corresponding .pack: .git/objects/pack/fake5.bitmap
> +warning: no corresponding .pack: .git/objects/pack/fake5.idx

Wondering if we can condense this into one message (because only
one pack is missing).

> +warning: no corresponding .pack: .git/objects/pack/fake6.bitmap
> +warning: no corresponding .pack: .git/objects/pack/fake6.idx
> +warning: no corresponding .pack: .git/objects/pack/fake6.keep

same here.
no corresponding .pack: .git/objects/pack/fake6.{keep,idx, bitmap}
would look nice and be shell compatible. (rm on that multi path just works,
in case you expect the pack to be gone)
--
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

[PATCHv4 8/9] submodule update: expose parallelism to the user

[Stefan Beller]

Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.jobs" setting.

By having the variable initialized to -1, we make sure 0 can be passed
into the parallel processing machine, which will then pick as many parallel
workers as there are CPUs.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt |  7 ++-
 builtin/submodule--helper.c | 18 ++
 git-submodule.sh|  9 +
 t/t7406-submodule-update.sh | 12 
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index f17687e..a87ff72 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] [--] ...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
  [-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--] [...]
+ [--depth ] [--recursive] [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -374,6 +374,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]
 
+-j ::
+--jobs ::
+   This option is only valid for the update command.
+   Clone new submodules in parallel with as many jobs.
+   Defaults to the `submodule.fetchJobs` option.
 
 ...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 95b45a2..662d329 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -426,6 +426,7 @@ static int update_clone_task_finished(int result,
 
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
+   int max_jobs = -1;
struct string_list_item *item;
struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -446,6 +447,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "depth", &pp.depth, "",
   N_("Create a shallow clone truncated to the "
  "specified number of revisions")),
+   OPT_INTEGER('j', "jobs", &max_jobs,
+   N_("parallel jobs")),
OPT__QUIET(&pp.quiet, N_("do't print cloning progress")),
OPT_END()
};
@@ -467,10 +470,17 @@ static int update_clone(int argc, const char **argv, 
const char *prefix)
gitmodules_config();
/* Overlay the parsed .gitmodules file with .git/config */
git_config(git_submodule_config, NULL);
-   run_processes_parallel(1, update_clone_get_next_task,
- update_clone_start_failure,
- update_clone_task_finished,
- &pp);
+
+   if (max_jobs < 0)
+   max_jobs = config_parallel_submodules();
+   if (max_jobs < 0)
+   max_jobs = 1;
+
+   run_processes_parallel(max_jobs,
+  update_clone_get_next_task,
+  update_clone_start_failure,
+  update_clone_task_finished,
+  &pp);
 
if (pp.print_unmatched) {
printf("#unmatched\n");
diff --git a/git-submodule.sh b/git-submodule.sh
index 9f554fb..10c5af9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
--depth=*)
depth=$1
;;
+   -j|--jobs)
+   case "$2" in '') usage ;; esac
+   jobs="--jobs=$2"
+   shift
+   ;;
+   --jobs=*)
+   jobs=$1
+   ;;
--)
shift
break
@@ -670,6 +678,7 @@ cmd_update()
${update:+--update "$update"} \
${reference:+--reference "$reference"} \
${depth:+--depth "$depth"} \
+   ${jobs:+$jobs} \
"$@" | {
err=
while read mode sha1 stage just_cloned sm_path
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..7fd5142 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops 
module name before recur
 test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked 
out" actual
)
 '
+
+test_expect_success 'submodule update can be run in parallel' '
+   (cd super2 &&
+GIT_TRA

[PATCHv4 9/9] clone: allow an explicit argument for parallel submodule clones

[Stefan Beller]

Just pass it along to "git submodule update", which may pick reasonable
defaults if you don't specify an explicit number.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt |  6 +-
 builtin/clone.c | 19 +--
 t/t7406-submodule-update.sh | 15 +++
 3 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index f1f2a3f..59d8c67 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
  [-o ] [-b ] [-u ] [--reference ]
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--] 
+ [--recursive | --recurse-submodules] [--jobs ] [--] 
  []
 
 DESCRIPTION
@@ -216,6 +216,10 @@ objects from the source repository into a pack in the 
cloned repository.
The result is Git repository can be separated from working
tree.
 
+-j ::
+--jobs ::
+   The number of submodules fetched at the same time.
+   Defaults to the `submodule.fetchJobs` option.
 
 ::
The (possibly remote) repository to clone from.  See the
diff --git a/builtin/clone.c b/builtin/clone.c
index 9eaecd9..ce578d2 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
+static int max_jobs = -1;
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", &option_recursive,
N_("initialize submodules in the clone")),
+   OPT_INTEGER('j', "jobs", &max_jobs,
+   N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", &option_template, N_("template-directory"),
   N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
OPT_END()
 };
 
-static const char *argv_submodule[] = {
-   "submodule", "update", "--init", "--recursive", NULL
-};
-
 static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,16 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive)
-   err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+   if (!err && option_recursive) {
+   struct argv_array args = ARGV_ARRAY_INIT;
+   argv_array_pushl(&args, "submodule", "update", "--init", 
"--recursive", NULL);
+
+   if (max_jobs != -1)
+   argv_array_pushf(&args, "--jobs=%d", max_jobs);
+
+   err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+   argv_array_clear(&args);
+   }
 
return err;
 }
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 7fd5142..090891e 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in 
parallel' '
 grep "9 tasks" trace.out
)
 '
+
+test_expect_success 'git clone passes the parallel jobs config on to 
submodules' '
+   test_when_finished "rm -rf super4" &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . 
super4 &&
+   grep "7 tasks" trace.out &&
+   rm -rf super4 &&
+   git config --global submodule.fetchJobs 8 &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+   grep "8 tasks" trace.out &&
+   rm -rf super4 &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . 
super4 &&
+   grep "9 tasks" trace.out &&
+   rm -rf super4
+'
+
 test_done
-- 
2.6.3.369.gea52ac0

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

[PATCHv4 3/9] submodule-config: drop check against NULL

[Stefan Beller]

Adhere to the common coding style of Git and not check explicitly
for NULL throughout the file. There are still other occurrences in the
code base but that is usually inside of conditions with side effects.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 4239b0e..6d01941 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -265,7 +265,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
if (!strcmp(item.buf, "path")) {
if (!value)
ret = config_error_nonbool(var);
-   else if (!me->overwrite && submodule->path != NULL)
+   else if (!me->overwrite && submodule->path)
warn_multiple_config(me->commit_sha1, submodule->name,
"path");
else {
@@ -289,7 +289,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
} else if (!strcmp(item.buf, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
-   else if (!me->overwrite && submodule->ignore != NULL)
+   else if (!me->overwrite && submodule->ignore)
warn_multiple_config(me->commit_sha1, submodule->name,
"ignore");
else if (strcmp(value, "untracked") &&
@@ -305,7 +305,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
} else if (!strcmp(item.buf, "url")) {
if (!value) {
ret = config_error_nonbool(var);
-   } else if (!me->overwrite && submodule->url != NULL) {
+   } else if (!me->overwrite && submodule->url) {
warn_multiple_config(me->commit_sha1, submodule->name,
"url");
} else {
@@ -315,7 +315,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
} else if (!strcmp(item.buf, "update")) {
if (!value)
ret = config_error_nonbool(var);
-   else if (!me->overwrite && submodule->update != NULL)
+   else if (!me->overwrite && submodule->update)
warn_multiple_config(me->commit_sha1, submodule->name,
 "update");
else {
-- 
2.6.3.369.gea52ac0

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

[PATCHv4 7/9] git submodule update: have a dedicated helper for cloning

[Stefan Beller]

This introduces a new helper function in git submodule--helper
which takes care of cloning all submodules, which we want to
parallelize eventually.

Some tests (such as empty URL, update_mode=none) are required in the
helper to make the decision for cloning. These checks have been
moved into the C function as well (no need to repeat them in the
shell script).

As we can only access the stderr channel from within the parallel
processing engine, we need to reroute the error message for
specified but initialized submodules to stderr. As it is an error
message, this should have gone to stderr in the first place, so it
is a bug fix along the way.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 229 
 git-submodule.sh|  45 +++--
 t/t7400-submodule-basic.sh  |   4 +-
 3 files changed, 242 insertions(+), 36 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..95b45a2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,234 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
return 0;
 }
 
+static int git_submodule_config(const char *var, const char *value, void *cb)
+{
+   return parse_submodule_config_option(var, value);
+}
+
+struct submodule_update_clone {
+   /* states */
+   int count;
+   int print_unmatched;
+   /* configuration */
+   int quiet;
+   const char *reference;
+   const char *depth;
+   const char *update;
+   const char *recursive_prefix;
+   const char *prefix;
+   struct module_list list;
+   struct string_list projectlines;
+   struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, 
MODULE_LIST_INIT, STRING_LIST_INIT_DUP}
+
+static void fill_clone_command(struct child_process *cp, int quiet,
+  const char *prefix, const char *path,
+  const char *name, const char *url,
+  const char *reference, const char *depth)
+{
+   cp->git_cmd = 1;
+   cp->no_stdin = 1;
+   cp->stdout_to_stderr = 1;
+   cp->err = -1;
+   argv_array_push(&cp->args, "submodule--helper");
+   argv_array_push(&cp->args, "clone");
+   if (quiet)
+   argv_array_push(&cp->args, "--quiet");
+
+   if (prefix)
+   argv_array_pushl(&cp->args, "--prefix", prefix, NULL);
+
+   argv_array_pushl(&cp->args, "--path", path, NULL);
+   argv_array_pushl(&cp->args, "--name", name, NULL);
+   argv_array_pushl(&cp->args, "--url", url, NULL);
+   if (reference)
+   argv_array_push(&cp->args, reference);
+   if (depth)
+   argv_array_push(&cp->args, depth);
+}
+
+static int update_clone_get_next_task(void **pp_task_cb,
+ struct child_process *cp,
+ struct strbuf *err,
+ void *pp_cb)
+{
+   struct submodule_update_clone *pp = pp_cb;
+
+   for (; pp->count < pp->list.nr; pp->count++) {
+   const struct submodule *sub = NULL;
+   const char *displaypath = NULL;
+   const struct cache_entry *ce = pp->list.entries[pp->count];
+   struct strbuf sb = STRBUF_INIT;
+   const char *update_module = NULL;
+   char *url = NULL;
+   int needs_cloning = 0;
+
+   if (ce_stage(ce)) {
+   if (pp->recursive_prefix)
+   strbuf_addf(err, "Skipping unmerged submodule 
%s/%s\n",
+   pp->recursive_prefix, ce->name);
+   else
+   strbuf_addf(err, "Skipping unmerged submodule 
%s\n",
+   ce->name);
+   continue;
+   }
+
+   sub = submodule_from_path(null_sha1, ce->name);
+   if (!sub) {
+   strbuf_addf(err, "BUG: internal error managing 
submodules. "
+   "The cache could not locate '%s'", 
ce->name);
+   pp->print_unmatched = 1;
+   continue;
+   }
+
+   if (pp->recursive_prefix)
+   displaypath = relative_path(pp->recursive_prefix, 
ce->name, &sb);
+   else
+   displaypath = ce->name;
+
+   if (pp->update)
+   update_module = pp->update;
+   if (!update_module)
+   update_module = sub->update;
+   if (!update_module)
+   update_module = "checkout";
+   if (!strcmp(update_module, "none")) {
+   strbuf_addf(err, "Skipping submodule '%s'\n", 
d

[PATCHv4 1/9] run_processes_parallel: delimit intermixed task output

[Stefan Beller]

This commit serves 2 purposes. First this may help the user who
tries to diagnose intermixed process calls. Second this may be used
in a later patch for testing. As the output of a command should not
change visibly except for going faster, grepping for the trace output
seems like a viable testing strategy.

Signed-off-by: Stefan Beller 
---
 run-command.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/run-command.c b/run-command.c
index 358429f..b246118 100644
--- a/run-command.c
+++ b/run-command.c
@@ -965,6 +965,9 @@ static struct parallel_processes *pp_init(int n,
n = online_cpus();
 
pp->max_processes = n;
+
+   trace_printf("run_processes_parallel: preparing to run up to %d tasks", 
n);
+
pp->data = data;
if (!get_next_task)
die("BUG: you need to specify a get_next_task function");
@@ -994,6 +997,7 @@ static void pp_cleanup(struct parallel_processes *pp)
 {
int i;
 
+   trace_printf("run_processes_parallel: done");
for (i = 0; i < pp->max_processes; i++) {
strbuf_release(&pp->children[i].err);
child_process_clear(&pp->children[i].process);
-- 
2.6.3.369.gea52ac0

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

[PATCHv4 5/9] submodule-config: introduce parse_generic_submodule_config

[Stefan Beller]

This rewrites parse_config to distinguish between configs specific to
one submodule and configs which apply generically to all submodules.
We do not have generic submodule configs yet, but the next patch will
introduce "submodule.jobs".

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 41 -
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index b826841..29e21b2 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -234,17 +234,22 @@ struct parse_config_parameter {
int overwrite;
 };
 
-static int parse_config(const char *var, const char *value, void *data)
+static int parse_generic_submodule_config(const char *key,
+ const char *var,
+ const char *value,
+ struct parse_config_parameter *me)
 {
-   struct parse_config_parameter *me = data;
-   struct submodule *submodule;
-   int subsection_len, ret = 0;
-   const char *subsection, *key;
-
-   if (parse_config_key(var, "submodule", &subsection,
-&subsection_len, &key) < 0 || !subsection_len)
-   return 0;
+   return 0;
+}
 
+static int parse_specific_submodule_config(const char *subsection, int 
subsection_len,
+  const char *key,
+  const char *var,
+  const char *value,
+  struct parse_config_parameter *me)
+{
+   int ret = 0;
+   struct submodule *submodule;
submodule = lookup_or_create_by_name(me->cache,
 me->gitmodules_sha1,
 subsection, subsection_len);
@@ -314,6 +319,24 @@ static int parse_config(const char *var, const char 
*value, void *data)
return ret;
 }
 
+static int parse_config(const char *var, const char *value, void *data)
+{
+   struct parse_config_parameter *me = data;
+   int subsection_len;
+   const char *subsection, *key;
+
+   if (parse_config_key(var, "submodule", &subsection,
+&subsection_len, &key) < 0)
+   return 0;
+
+   if (!subsection_len)
+   return parse_generic_submodule_config(key, var, value, me);
+   else
+   return parse_specific_submodule_config(subsection,
+  subsection_len, key,
+  var, value, me);
+}
+
 static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
  unsigned char *gitmodules_sha1)
 {
-- 
2.6.3.369.gea52ac0

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

[PATCHv4 6/9] fetching submodules: respect `submodule.jobs` config option

[Stefan Beller]

This allows to configure fetching and updating in parallel
without having the command line option.

This moved the responsibility to determine how many parallel processes
to start from builtin/fetch to submodule.c as we need a way to communicate
"The user did not specify the number of parallel processes in the command
line options" in the builtin fetch. The submodule code takes care of
the precedence (CLI > config > default)

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt|  7 +++
 builtin/fetch.c |  2 +-
 submodule-config.c  | 15 +++
 submodule-config.h  |  2 ++
 submodule.c |  5 +
 t/t5526-fetch-submodules.sh | 14 ++
 6 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..9e7c14c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2643,6 +2643,13 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
 
+submodule.fetchJobs::
+   This is used to determine how many submodules will be
+   fetched/cloned at the same time. Specifying a positive integer
+   allows up to that number of submodules being fetched in parallel.
+   This is used in fetch and clone operations only. A value of 0 will
+   give some reasonable configuration. It defaults to 1.
+
 tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9cc1c9d..60e6797 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
 static int all, append, dry_run, force, keep, multiple, update_head_ok, 
verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule-config.c b/submodule-config.c
index 29e21b2..a32259e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -32,6 +32,7 @@ enum lookup_type {
 
 static struct submodule_cache cache;
 static int is_cache_init;
+static int parallel_jobs = -1;
 
 static int config_path_cmp(const struct submodule_entry *a,
   const struct submodule_entry *b,
@@ -239,6 +240,15 @@ static int parse_generic_submodule_config(const char *key,
  const char *value,
  struct parse_config_parameter *me)
 {
+   if (!strcmp(key, "fetchjobs")) {
+   parallel_jobs = strtol(value, NULL, 10);
+   if (parallel_jobs < 0) {
+   warning("submodule.fetchJobs not allowed to be 
negative.");
+   parallel_jobs = 1;
+   return 1;
+   }
+   }
+
return 0;
 }
 
@@ -482,3 +492,8 @@ void submodule_free(void)
cache_free(&cache);
is_cache_init = 0;
 }
+
+int config_parallel_submodules(void)
+{
+   return parallel_jobs;
+}
diff --git a/submodule-config.h b/submodule-config.h
index f9e2a29..d9bbf9a 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned 
char *commit_sha1,
const char *path);
 void submodule_free(void);
 
+int config_parallel_submodules(void);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index c6350eb..e73f850 100644
--- a/submodule.c
+++ b/submodule.c
@@ -749,6 +749,11 @@ int fetch_populated_submodules(const struct argv_array 
*options,
argv_array_push(&spf.args, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
 
+   if (max_parallel_jobs < 0)
+   max_parallel_jobs = config_parallel_submodules();
+   if (max_parallel_jobs < 0)
+   max_parallel_jobs = 1;
+
calculate_changed_submodule_paths();
run_processes_parallel(max_parallel_jobs,
   get_next_submodule,
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1b4ce69..6671994 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -470,4 +470,18 @@ test_expect_success "don't fetch submodule when newly 
recorded commits are alrea
test_i18ncmp expect.err actual.err
 '
 
+test_expect_success 'fetching submodules respects parallel settings' '
+   git config fetch.recurseSubmodules true &&
+   (
+   cd downstream &&
+   GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
+   grep "7 tasks" trace.out &&
+   git config submodule.fetchJo

[PATCHv4 2/9] submodule-config: keep update strategy around

[Stefan Beller]

We need the submodule update strategies in a later patch.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 submodule-config.c | 11 +++
 submodule-config.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..4239b0e 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
 
submodule->path = NULL;
submodule->url = NULL;
+   submodule->update = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
 
@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char 
*value, void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
+   } else if (!strcmp(item.buf, "update")) {
+   if (!value)
+   ret = config_error_nonbool(var);
+   else if (!me->overwrite && submodule->update != NULL)
+   warn_multiple_config(me->commit_sha1, submodule->name,
+"update");
+   else {
+   free((void *) submodule->update);
+   submodule->update = xstrdup(value);
+   }
}
 
strbuf_release(&name);
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..f9e2a29 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -14,6 +14,7 @@ struct submodule {
const char *url;
int fetch_recurse;
const char *ignore;
+   const char *update;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
 };
-- 
2.6.3.369.gea52ac0

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

[PATCHv4 0/9] Expose submodule parallelism to the user

[Stefan Beller]

This replaces sb/submodule-parallel-update.
It applies on top of d075d2604c0 (Merge branch
'rs/daemon-plug-child-leak' into sb/submodule-parallel-update,
with additionally having merged submodule-parallel-fetch,
which has applied "run-command: detect finished
children by closed pipe rather than waitpid" on top of it.
Alternatively pull from github/stefanbeller/git submodule-parallel-update

* This lets you configure submodule.fetchJobs instead of previously 
submodule.jobs
* no weird NONBLOCK thingies any more as that was handled by 
submodule-parallel-fetch
  (or the patch on top of that) 


Stefan Beller (9):
  run_processes_parallel: delimit intermixed task output
  submodule-config: keep update strategy around
  submodule-config: drop check against NULL
  submodule-config: remove name_and_item_from_var
  submodule-config: introduce parse_generic_submodule_config
  fetching submodules: respect `submodule.jobs` config option
  git submodule update: have a dedicated helper for cloning
  submodule update: expose parallelism to the user
  clone: allow an explicit argument for parallel submodule clones

 Documentation/config.txt|   7 ++
 Documentation/git-clone.txt |   6 +-
 Documentation/git-submodule.txt |   7 +-
 builtin/clone.c |  19 +++-
 builtin/fetch.c |   2 +-
 builtin/submodule--helper.c | 239 
 git-submodule.sh|  54 -
 run-command.c   |   4 +
 submodule-config.c  | 109 +++---
 submodule-config.h  |   3 +
 submodule.c |   5 +
 t/t5526-fetch-submodules.sh |  14 +++
 t/t7400-submodule-basic.sh  |   4 +-
 t/t7406-submodule-update.sh |  27 +
 14 files changed, 417 insertions(+), 83 deletions(-)

-- 
2.6.3.369.gea52ac0

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

[PATCHv4 4/9] submodule-config: remove name_and_item_from_var

[Stefan Beller]

`name_and_item_from_var` does not provide the proper abstraction
we need here in a later patch.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 48 
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 6d01941..b826841 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -161,31 +161,17 @@ static struct submodule *cache_lookup_name(struct 
submodule_cache *cache,
return NULL;
 }
 
-static int name_and_item_from_var(const char *var, struct strbuf *name,
- struct strbuf *item)
-{
-   const char *subsection, *key;
-   int subsection_len, parse;
-   parse = parse_config_key(var, "submodule", &subsection,
-   &subsection_len, &key);
-   if (parse < 0 || !subsection)
-   return 0;
-
-   strbuf_add(name, subsection, subsection_len);
-   strbuf_addstr(item, key);
-
-   return 1;
-}
-
 static struct submodule *lookup_or_create_by_name(struct submodule_cache 
*cache,
-   const unsigned char *gitmodules_sha1, const char *name)
+ const unsigned char 
*gitmodules_sha1,
+ const char *name_ptr, int 
name_len)
 {
struct submodule *submodule;
struct strbuf name_buf = STRBUF_INIT;
+   char *name = xmemdupz(name_ptr, name_len);
 
submodule = cache_lookup_name(cache, gitmodules_sha1, name);
if (submodule)
-   return submodule;
+   goto out;
 
submodule = xmalloc(sizeof(*submodule));
 
@@ -201,7 +187,8 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
hashcpy(submodule->gitmodules_sha1, gitmodules_sha1);
 
cache_add(cache, submodule);
-
+out:
+   free(name);
return submodule;
 }
 
@@ -251,18 +238,18 @@ static int parse_config(const char *var, const char 
*value, void *data)
 {
struct parse_config_parameter *me = data;
struct submodule *submodule;
-   struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
-   int ret = 0;
+   int subsection_len, ret = 0;
+   const char *subsection, *key;
 
-   /* this also ensures that we only parse submodule entries */
-   if (!name_and_item_from_var(var, &name, &item))
+   if (parse_config_key(var, "submodule", &subsection,
+&subsection_len, &key) < 0 || !subsection_len)
return 0;
 
submodule = lookup_or_create_by_name(me->cache,
 me->gitmodules_sha1,
-name.buf);
+subsection, subsection_len);
 
-   if (!strcmp(item.buf, "path")) {
+   if (!strcmp(key, "path")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path)
@@ -275,7 +262,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
submodule->path = xstrdup(value);
cache_put_path(me->cache, submodule);
}
-   } else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
+   } else if (!strcmp(key, "fetchrecursesubmodules")) {
/* when parsing worktree configurations we can die early */
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
@@ -286,7 +273,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
submodule->fetch_recurse = parse_fetch_recurse(
var, value,
die_on_error);
-   } else if (!strcmp(item.buf, "ignore")) {
+   } else if (!strcmp(key, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore)
@@ -302,7 +289,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
free((void *) submodule->ignore);
submodule->ignore = xstrdup(value);
}
-   } else if (!strcmp(item.buf, "url")) {
+   } else if (!strcmp(key, "url")) {
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && submodule->url) {
@@ -312,7 +299,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
-   } else if (!strcmp(item.buf, "update")) {
+   } else if (!strcmp(key, "update")) {
if (!value)
re

Re: [PATCH RFC] completion: add support for completing email aliases

[SZEDER Gábor]

Hi,

Quoting Jacob Keller :


From: Jacob Keller 

Extract email aliases from the sendemail.aliasesfile according to the
known types. Implementation only extracts the alias name and does not
attempt to complete email addresses.

Add a few tests for simple layouts of the currently supported alias
filetypes.

Signed-off-by: Jacob Keller 
---

Labeled this RFC because I have only been able to test the mutt format
as this is what I use locally. I have a few (probably brittle) test
cases for the files, but they are not "real" configuration files as per
the upstream tools, so they are essentially made to work with the simple
extractors that I have now. I'd like some review on this to see if it's
valuable, but it definitely helps me type out aliases and see what is
available by just using TAB.


I think it's definitely valuable, but I'm unsure about parsing the
different alias file formats in shell (well, in awk...), though some
of the parses are much simpler than I expected.
However, 'git send-email' already knows how to parse these files, so
how about letting it do all the work, i.e. teach it a new 'git
send-email --list-aliases' option that would only read the config,
parse the alias file, print all the aliases, i.e. the keys from the
'aliases' associative array and exit.  That way we wouldn't have to
duplicate parts of an already working and tested parsing into a
different language, the completion script would be simpler, because we
wouldn't need __git_email_aliases() helper function, it would
immediately benefit from future bug fixes to 'send-email', and new
alias file formats would Just Work.


contrib/completion/git-completion.bash | 69 ++
t/t9902-completion.sh  | 90
++
2 files changed, 159 insertions(+)

diff --git a/contrib/completion/git-completion.bash
b/contrib/completion/git-completion.bash
index 482ca84b451b..9b786bb390ba 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -10,6 +10,7 @@
#*) local and remote tag names
#*) .git/remotes file names
#*) git 'subcommands'
+#*) git email aliases for git-send-email
#*) tree paths within 'ref:path/to/file' expressions
#*) file paths within current working directory and index
#*) common --long-options
@@ -785,6 +786,56 @@ __git_aliased_command ()
done
}

+# Print aliases for email addresses from sendemail.aliasesfile
+__git_email_aliases ()
+{
+   local file="$(git --git-dir="$(__gitdir)" config --path
sendemail.aliasesfile)"
+   local filetype="$(git --git-dir="$(__gitdir)" config
sendemail.aliasfiletype)"


Using $(__gitdir) is good.  Running it twice not so much.  fork()ing a
subshell and fork()+exec()ing a git command is expensive on some
platforms, most importantly Windows.
Please run it only once, store the returned path in a variable, and
pass that to the 'git config' commands.

Sidenote, just wondering: why are these config keys named
'aliasfiletype' and 'alias*es*file'?!


+   # Only run awk if we find an actual file
+   if ! [ -f $file ]; then
+   return
+   fi


According to the docs multiple alias files can be configured, but this
would only work with one.
That 'git send-email --list-aliases' would take care of this, too.


+
+   case "$filetype" in
+   # Each file type needs to be parsed differently.
+   mutt|mailrc)
+   # Mutt and mailrc are simple and just put the alias in
+   # the 2nd field of the file.
+   awk '{print $2}' $file
+   return
+   ;;
+   sendmail)
+   # Skip new lines, lines without fields, and lines
+   # ending in '\' then print the name minus the final :
+   awk 'NF && $1!~/^#/ && !/\\$/ {sub(/:$/, "", $1); print 
$1 }' $file
+   return
+   ;;
+   pine)
+   # According to spec, line continuations are any line
+   # which starts with whitespace, otherwise we can just
+   # use the normal separator and print the first field.
+   awk '/^\S/ {print $1}' "$file"
+   return
+   ;;
+   elm)
+   # Elm doesn't appear to allow newlines, and
+   # git-send-email only accepts one alias per line, so
+   # just print the first field.
+   awk '{print $1}' "$file"
+   return
+   ;;
+   gnus)
+   # The gnus format has the alias quoted, so we just use
+   # gsub to extract the alias from the quotes
+   awk '/define-mail-alias/ {gsub(/"/, "", $2); print $2}' 
$file
+   ret