Re: [PATCH] replace --edit: respect core.editor

[Christian Couder]

On Wed, Apr 20, 2016 at 5:53 AM, Jeff King  wrote:
> On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:
>
>> I can understand "we only know edit mode needs config, and we know
>> it will never affect other modes to have the new call here", and it
>> would be good for an emergency patch for ancient maintenance track
>> that will not get any other changes or enhancements.  I do not think
>> it is a sound reasoning to maintain the codefor the longer term,
>> though.
>
> Yeah. I agree the patch here is not wrong, but I would prefer to just
> have git-replace load the config when it starts. It's _possible_ that
> something might break or misbehave, but IMHO any program which breaks
> when git_default_config() is run is probably in need of fixing.

I agree.

> And I cannot recall any reason we did not read config when "--edit"
> was added; we just didn't think of it.

Yeah I think so.

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

Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

[Stefan Beller]

On Tue, Apr 19, 2016 at 9:18 PM, Jeff King  wrote:
> [your original probably didn't make it to the list because of its 5MB
>  attachment; the list has a 100K limit; I'll try to quote liberally]
>
> On Tue, Apr 19, 2016 at 04:17:50PM -0700, Jacob Keller wrote:
>
>> I ran this version of the patch against the entire Linux kernel
>> history, as I figured this has a large batch of C code to try and spot
>> any issues.
>>
>> I ran something like the following command in bash
>>
>> $git rev-list HEAD | while read -r rev; do diff -F ^commit -u <(git
>> show --format="commit %H" --no-compaction-heuristic $rev) <(git show
>> --format="commit %H" --compaction-heuristic $rev); done >
>> heuristic.patch
>
> My earlier tests with the perl script were all done with "git log -p",
> which will not show anything at all for merges (and my script wouldn't
> know how to deal with combined diffs anyway). But I think this new patch
> _will_ kick in for combined diffs (because it is built on individual
> diffs). It will be interesting to see if this has any effect there, and
> what it looks like.
>
> We should be able to see it (on a small enough repository) with:
>
>   git log --format='commit %H' --cc --merges
>
> and comparing the before/after.
>
>> I've attached the file that I generated for the Linux history, it's
>> rather large so hopefully I can get some help to spot any differences.
>> The above approach will work for pretty much any repository, and works
>> better than trying to generate the entire thing first and then diff
>> (since that runs out of memory pretty fast).
>
> I don't think there is much point in generating a complete diff between
> the patches for every commit, when nobody can look at the whole thing.
> Unless we have automated tooling to find "interesting" bits (and
> certainly a tool to remove the boring "a comment got shifted by one"
> lines would help; those are all known improvements, but it's the _other_
> stuff we want to look).
>
> But if we are not using automated tooling to find the needle in the
> haystack, we might as well using sampling to make the dataset more
> manageable. Adding "--since=1.year.ago" is one way, though we may want
> to sample more randomly across time.
>
>> So far, I haven't spotted anything that would want me to disable it,
>> while I've spotted several cases where I felt that readability was
>> improved. It's somewhat difficult to spot though.
>
> I did find one case that I think is worse. Look at 857942fd1a in the
> kernel. It has a pattern like this:
>
>   ... surrounding code ...
>
>   function_one();
>   ... more surrounding code ...
>
> which becomes:
>
>   ... surrounding code ...
>
>   function_two();
>
>   ... more surrounding code
>
> Without the new heuristic, that looks like:
>
>   -function_one();
>   +function_two();
>   +
>
> but with it, it becomes:
>
>   +
>   +function_two();
>
>   -function_one();
>
> which is kind of weird. Having the two directly next to each other reads
> better to me. This is a pretty unusual diff, though, in that it did
> change the surrounding whitespace (and if you look further in the diff,
> the identical change is made elsewhere _without_ touching the
> whitespace). So this is kind of an anomaly. And IMHO the weirdness here
> is outweighed by the vast number of improvements elsewhere.

The new implementation supports the flags for ignoring white space, too.

>
> -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 2/2] xdiff: implement empty line chunk heuristic

[Jeff King]

On Wed, Apr 20, 2016 at 12:18:27AM -0400, Jeff King wrote:

> My earlier tests with the perl script were all done with "git log -p",
> which will not show anything at all for merges (and my script wouldn't
> know how to deal with combined diffs anyway). But I think this new patch
> _will_ kick in for combined diffs (because it is built on individual
> diffs). It will be interesting to see if this has any effect there, and
> what it looks like.
> 
> We should be able to see it (on a small enough repository) with:
> 
>   git log --format='commit %H' --cc --merges
> 
> and comparing the before/after.

Add in "-p" if you are testing the tip of jk/diff-compact-heuristic. It
is based on the older maintenance track in which "--cc" does not imply
"-p".

Looking over the results, it's about what you'd expect (comment blocks
shifted by one as we want, and then there happens to be a one-line
conflict resolved later in the hunk).

The most interesting thing I found was db65f0fc3b1e. There we have two
functions being added in the same spot, and the resolution obviously is
to put one after the other. So both sides do the usual comment-block
thing, and the resulting combined diff carries through that improvement
as you'd expect.

-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 2/2] xdiff: implement empty line chunk heuristic

[Jeff King]

[your original probably didn't make it to the list because of its 5MB
 attachment; the list has a 100K limit; I'll try to quote liberally]

On Tue, Apr 19, 2016 at 04:17:50PM -0700, Jacob Keller wrote:

> I ran this version of the patch against the entire Linux kernel
> history, as I figured this has a large batch of C code to try and spot
> any issues.
> 
> I ran something like the following command in bash
> 
> $git rev-list HEAD | while read -r rev; do diff -F ^commit -u <(git
> show --format="commit %H" --no-compaction-heuristic $rev) <(git show
> --format="commit %H" --compaction-heuristic $rev); done >
> heuristic.patch

My earlier tests with the perl script were all done with "git log -p",
which will not show anything at all for merges (and my script wouldn't
know how to deal with combined diffs anyway). But I think this new patch
_will_ kick in for combined diffs (because it is built on individual
diffs). It will be interesting to see if this has any effect there, and
what it looks like.

We should be able to see it (on a small enough repository) with:

  git log --format='commit %H' --cc --merges

and comparing the before/after.

> I've attached the file that I generated for the Linux history, it's
> rather large so hopefully I can get some help to spot any differences.
> The above approach will work for pretty much any repository, and works
> better than trying to generate the entire thing first and then diff
> (since that runs out of memory pretty fast).

I don't think there is much point in generating a complete diff between
the patches for every commit, when nobody can look at the whole thing.
Unless we have automated tooling to find "interesting" bits (and
certainly a tool to remove the boring "a comment got shifted by one"
lines would help; those are all known improvements, but it's the _other_
stuff we want to look).

But if we are not using automated tooling to find the needle in the
haystack, we might as well using sampling to make the dataset more
manageable. Adding "--since=1.year.ago" is one way, though we may want
to sample more randomly across time.

> So far, I haven't spotted anything that would want me to disable it,
> while I've spotted several cases where I felt that readability was
> improved. It's somewhat difficult to spot though.

I did find one case that I think is worse. Look at 857942fd1a in the
kernel. It has a pattern like this:

  ... surrounding code ...

  function_one();
  ... more surrounding code ...

which becomes:

  ... surrounding code ...

  function_two();

  ... more surrounding code

Without the new heuristic, that looks like:

  -function_one();
  +function_two();
  +

but with it, it becomes:

  +
  +function_two();

  -function_one();

which is kind of weird. Having the two directly next to each other reads
better to me. This is a pretty unusual diff, though, in that it did
change the surrounding whitespace (and if you look further in the diff,
the identical change is made elsewhere _without_ touching the
whitespace). So this is kind of an anomaly. And IMHO the weirdness here
is outweighed by the vast number of improvements elsewhere.

-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] replace --edit: respect core.editor

[Jeff King]

On Tue, Apr 19, 2016 at 09:22:37AM -0700, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > We simply need to read the config, is all.
> >
> > This fixes https://github.com/git-for-windows/git/issues/733
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/replace.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/builtin/replace.c b/builtin/replace.c
> > index 748c6ca..02b13f6 100644
> > --- a/builtin/replace.c
> > +++ b/builtin/replace.c
> > @@ -475,6 +475,7 @@ int cmd_replace(int argc, const char **argv, const char 
> > *prefix)
> > return replace_object(argv[0], argv[1], force);
> >  
> > case MODE_EDIT:
> > +   git_config(git_default_config, NULL);
> > if (argc != 1)
> > usage_msg_opt("-e needs exactly one argument",
> >   git_replace_usage, options);
> 
> The placement of git_config() makes me wonder why.
> 
> I can understand "we only know edit mode needs config, and we know
> it will never affect other modes to have the new call here", and it
> would be good for an emergency patch for ancient maintenance track
> that will not get any other changes or enhancements.  I do not think
> it is a sound reasoning to maintain the codefor the longer term,
> though.

Yeah. I agree the patch here is not wrong, but I would prefer to just
have git-replace load the config when it starts. It's _possible_ that
something might break or misbehave, but IMHO any program which breaks
when git_default_config() is run is probably in need of fixing.

And I cannot recall any reason we did not read config when "--edit"
was added; we just didn't think of it.

-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/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Tue, Apr 19, 2016 at 07:43:11PM -0400, David Turner wrote:

> On Tue, 2016-04-19 at 19:22 -0400, Jeff King wrote:
> > You can find previous discussion on the list, but I think the options
> > basically are:
> > 
> >   1. Something like v2, where the client gets a chance to speak
> > before
> >  the advertisement.
> > 
> >   2. Some out-of-band way of getting values from the client to the
> >  server (so maybe extra command-line arguments for git-over-ssh,
> > and
> >  maybe shoving something after the "\0" for git-daemon, and of
> >  course extra parameters for HTTP).
> > 
> >   3. The client saying "stop spewing refs at me, I want to give you a
> >  ref filter" asynchronously, and accepting a little spew at the
> >  beginning of each conversation. That obviously only works for
> > the
> >  full-duplex transports, so you'd probably fall back to (2) for
> >  http.
> 
> OK, so (2) seems like what I'm doing -- it just happens that I only
> implemented it for one protocol.

Right. And I don't mind that approach _if_ we can figure out a way to do
it for all protocols. But I think there are some complications with the
other ones, which means that HTTP will have the ability to grow features
the other protocols do not.

-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 v5 06/15] index-helper: add --detach

[David Turner]

On Wed, 2016-04-20 at 06:50 +0700, Duy Nguyen wrote:
> On Wed, Apr 20, 2016 at 6:28 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > @@ -317,6 +320,8 @@ int main(int argc, char **argv)
> > if (fd < 0)
> > die_errno(_("could not set up index-helper
> > socket"));
> > 
> > +   if (detach && daemonize())
> > +   die_errno(_("unable to detach"));
> 
> At the least, I think we need to redirect both stdout and stderr to a
> file, so we can catch errors. The watchman patch uses warning() to
> report errors, for example. And there is always a chance of die().
> 
> Then we need to report the errors back. I faced the same problem with
> daemonizing git-gc, but I'm not sure if we can do exactly the same
> here like in commit 329e6e8 (gc: save log from daemonized gc --auto
> and print it next time - 2015-09-19)

I'll add in code to log errors.  I'm not sure where it would make sense
to report the errors.  Generally, for errors during a client operation,
we would like to report them to the client, but the client might have
already disconnected.  I guess in that case it's OK if they just go to
the log?  The client could warn on a timeout while waiting for index
-helper and direct people to the log.
--
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 v5 09/15] index-helper: use watchman to avoid refreshing index with lstat()

[David Turner]

On Wed, 2016-04-20 at 07:15 +0700, Duy Nguyen wrote:
> Continuing my comment from the --use-watchman patch about watchman
> not
> being supported...
> 
> On Wed, Apr 20, 2016 at 6:28 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > +static int poke_and_wait_for_reply(int fd)
> > +{
> > +   struct strbuf buf = STRBUF_INIT;
> > +   struct strbuf reply = STRBUF_INIT;
> > +   int ret = -1;
> > +   fd_set fds;
> > +   struct timeval timeout;
> > +
> > +   timeout.tv_usec = 0;
> > +   timeout.tv_sec = 1;
> > +
> > +   if (fd < 0)
> > +   return -1;
> > +
> > +   strbuf_addf(, "poke %d", getpid());
> > +   if (write_in_full(fd, buf.buf, buf.len + 1) != buf.len + 1)
> > +   goto done_poke;
> > +
> > +   /* Now wait for a reply */
> > +   FD_ZERO();
> > +   FD_SET(fd, );
> > +   if (select(fd + 1, , NULL, NULL, ) == 0)
> > +   /* No reply, giving up */
> > +   goto done_poke;
> > +
> > +   if (strbuf_getwholeline_fd(, fd, 0))
> > +   goto done_poke;
> > +
> > +   if (!starts_with(reply.buf, "OK"))
> > +   goto done_poke;
> 
> ... while we could simply check USE_WATCHMAN macro and reject in
> update-index, a better solution is sending "poke %d watchman" and
> returning "OK watchman" (vs "OK") when watchman is supported and
> active. If the user already requests watchman and index-helper
> returns
> just "OK" then we can warn the user the reason of possible
> performance
> degradation. It's related to the error reporting, but I don't think
> you can send straight errors over unix socket. It's possible but it's
> a bit more complicated.

Do you mean that we should do this here?  Or in update-index -
-watchman?  If the former, I agree.  If the latter, I'm not sure; maybe
you'll be setting up your index before you've started the index helper?

> > +static void refresh_by_watchman(struct index_state *istate)
> > +{
> > +   void *shm = NULL;
> > +   int length;
> > +   int i;
> > +   struct stat st;
> > +   int fd = -1;
> > +   const char *path = git_path("shm-watchman-%s-%"PRIuMAX,
> > +   sha1_to_hex(istate->sha1),
> > +   (uintmax_t)getpid());
> > +
> > +   fd = open(path, O_RDONLY);
> > +   if (fd < 0)
> > +   return;
> > +
> > +   /*
> > +* This watchman data is just for us -- no need to keep it
> > +* around once we've got it open.
> > +*/
> > +   unlink(path);
> 
> This will not play well when multiple processes read and refresh the
> index at the same time. 

Multiple processes will have different pids, right?  And the pid is
included in the filename.  Am I missing something?

> This is really extra. But if we know in advance that git does not 
> need refresh(), then we should be able to tell index-helper not to 
> waste cycles contacting watchman and preparing shm-watchman-%s-%d 
> (the poke line gets more parameters). Either that, or we decouple 
> watchman requests from read_cache() requests. Only when 
> refresh_index() is called that we send something to request shm-
> watchman-%s-%d. The same for read_directory() (i.e. untracked cache 
> stuff). Hmm?

It's true that we could decouple watchman requests.  I'll look and see
if that's reasonable.

> Now that I think of it, with watchman backing us, we probably should
> just do nothing in update_index_if_able() (or write_locked_index()
> when we know only stat info is changed) when watchman is active. The
> purpose of update_index_if_able() is to avoid costly refresh, but we
> can already avoid that with watchman. And updating big index files is
> always costly (even though it should cost less with split-index). 

That sounds like a change we could make in a separate series.  It's not
a bad idea, but if our goal is to get the basic version out, we should
start there.

> Of
> course this can only be done if watchman (inotify to be precise) can
> cover whole worktree. I'm not sure how watchman behaves when there's
> not enough inotify resource to cover full worktree.

It will detect this case and will either manually recrawl (in the event
of a max_queued_events overflow) or return an error (in the event of
too many watched directories).


--
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 v5 03/15] index-helper: new daemon for caching index and related stuff

[David Turner]

On Wed, 2016-04-20 at 07:31 +0700, Duy Nguyen wrote:
> On Wed, Apr 20, 2016 at 6:27 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > Shared memory is done by storing files in a per-repository
> > temporary
> > directory.  This is more portable than shm (which requires
> > posix-realtime and has various quirks on OS X).  It might even work
> > on
> > Windows, although this has not been tested.
> 
> There's another option, but I'm not sure if it's too clever/tricky to
> do. Anyway, on *nix we can send file descriptors over unix socket
> [2],
> then mmap them back to access content. On Windows, it looks like
> DuplicateHandle [1] can do nearly the same thing. This keeps
> everything in memory and we don't have to worry about cleaning up
> shm-* files.
> 
> [1] http://lackingrhoticity.blogspot.com/2015/05/passing-fds-handles-
> between-processes.html
> [2] http://www.normalesup.org/~george/comp/libancillary/

It's possibly a bit simpler for the index, although more complex
overall since we still need to write temp files for the watchman data.

Will consider/try.
--
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 v5 03/15] index-helper: new daemon for caching index and related stuff

[Duy Nguyen]

On Wed, Apr 20, 2016 at 6:27 AM, David Turner  wrote:
> Shared memory is done by storing files in a per-repository temporary
> directory.  This is more portable than shm (which requires
> posix-realtime and has various quirks on OS X).  It might even work on
> Windows, although this has not been tested.

There's another option, but I'm not sure if it's too clever/tricky to
do. Anyway, on *nix we can send file descriptors over unix socket [2],
then mmap them back to access content. On Windows, it looks like
DuplicateHandle [1] can do nearly the same thing. This keeps
everything in memory and we don't have to worry about cleaning up
shm-* files.

[1] 
http://lackingrhoticity.blogspot.com/2015/05/passing-fds-handles-between-processes.html
[2] http://www.normalesup.org/~george/comp/libancillary/
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] Convert struct name_entry to use struct object_id.

[Junio C Hamano]

"brian m. carlson"  writes:

> On Tue, Apr 19, 2016 at 04:02:22PM -0700, Junio C Hamano wrote:
>> "brian m. carlson"  writes:
>> 
>> > @@ -314,7 +314,7 @@ static int threeway_callback(int n, unsigned long 
>> > mask, unsigned long dirmask, s
>> >}
>> >  
>> >if (same_entry(entry+0, entry+1)) {
>> > -  if (entry[2].sha1 && !S_ISDIR(entry[2].mode)) {
>> > +  if (entry[2].oid->hash && !S_ISDIR(entry[2].mode)) {
>> 
>> Thanks for a warning in the cover letter.
>> 
>> "if (entry[2].oid && !S_ISDIR(entry[2].mode)" would be a faithful
>> conversion, wouldn't it?
>
> Yes, I think that would be a better conversion.  I'll reroll after
> waiting for further comments.

Thanks.  A simple general rule to follow is that anything that
assumed entry.sha1 is non-NULL should become entry.oid->hash, while
anyting that checked if entry.sha1 is NULL or not should become a
check on entry.oid, I think.  Even though offsetof(oid.hash) might
happen to be zero, compiler writers are crazy and "optimize" the
above entry[2].oid->hash saying "you are dereerencing into the hash
field without first checking if oid pointer is NULL, so we'd assume
that you know oid is not NULL here--which makes this always true so
we won't even do any check and only check entry[2].mode"..

--
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 v5 09/15] index-helper: use watchman to avoid refreshing index with lstat()

[Duy Nguyen]

Continuing my comment from the --use-watchman patch about watchman not
being supported...

On Wed, Apr 20, 2016 at 6:28 AM, David Turner  wrote:
> +static int poke_and_wait_for_reply(int fd)
> +{
> +   struct strbuf buf = STRBUF_INIT;
> +   struct strbuf reply = STRBUF_INIT;
> +   int ret = -1;
> +   fd_set fds;
> +   struct timeval timeout;
> +
> +   timeout.tv_usec = 0;
> +   timeout.tv_sec = 1;
> +
> +   if (fd < 0)
> +   return -1;
> +
> +   strbuf_addf(, "poke %d", getpid());
> +   if (write_in_full(fd, buf.buf, buf.len + 1) != buf.len + 1)
> +   goto done_poke;
> +
> +   /* Now wait for a reply */
> +   FD_ZERO();
> +   FD_SET(fd, );
> +   if (select(fd + 1, , NULL, NULL, ) == 0)
> +   /* No reply, giving up */
> +   goto done_poke;
> +
> +   if (strbuf_getwholeline_fd(, fd, 0))
> +   goto done_poke;
> +
> +   if (!starts_with(reply.buf, "OK"))
> +   goto done_poke;

... while we could simply check USE_WATCHMAN macro and reject in
update-index, a better solution is sending "poke %d watchman" and
returning "OK watchman" (vs "OK") when watchman is supported and
active. If the user already requests watchman and index-helper returns
just "OK" then we can warn the user the reason of possible performance
degradation. It's related to the error reporting, but I don't think
you can send straight errors over unix socket. It's possible but it's
a bit more complicated.

> +static void refresh_by_watchman(struct index_state *istate)
> +{
> +   void *shm = NULL;
> +   int length;
> +   int i;
> +   struct stat st;
> +   int fd = -1;
> +   const char *path = git_path("shm-watchman-%s-%"PRIuMAX,
> +   sha1_to_hex(istate->sha1),
> +   (uintmax_t)getpid());
> +
> +   fd = open(path, O_RDONLY);
> +   if (fd < 0)
> +   return;
> +
> +   /*
> +* This watchman data is just for us -- no need to keep it
> +* around once we've got it open.
> +*/
> +   unlink(path);

This will not play well when multiple processes read and refresh the
index at the same time. They could refresh non-overlapping
subdirectories, and I think it's perfectly ok for them to do so
(writing index down is a different matter). I don't have a good answer
for this. Perhaps if shm-watchman-%s-%d file is small enough (and it
should be, we store it in the index), then we can just send the
content straight over unix socket. I didn't have this option with my
signal-based communication model.

This is really extra. But if we know in advance that git does not need
refresh(), then we should be able to tell index-helper not to waste
cycles contacting watchman and preparing shm-watchman-%s-%d (the poke
line gets more parameters). Either that, or we decouple watchman
requests from read_cache() requests. Only when refresh_index() is
called that we send something to request shm-watchman-%s-%d. The same
for read_directory() (i.e. untracked cache stuff). Hmm?

Now that I think of it, with watchman backing us, we probably should
just do nothing in update_index_if_able() (or write_locked_index()
when we know only stat info is changed) when watchman is active. The
purpose of update_index_if_able() is to avoid costly refresh, but we
can already avoid that with watchman. And updating big index files is
always costly (even though it should cost less with split-index). Of
course this can only be done if watchman (inotify to be precise) can
cover whole worktree. I'm not sure how watchman behaves when there's
not enough inotify resource to cover full worktree.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 06/15] index-helper: add --detach

[Duy Nguyen]

On Wed, Apr 20, 2016 at 6:28 AM, David Turner  wrote:
> @@ -317,6 +320,8 @@ int main(int argc, char **argv)
> if (fd < 0)
> die_errno(_("could not set up index-helper socket"));
>
> +   if (detach && daemonize())
> +   die_errno(_("unable to detach"));

At the least, I think we need to redirect both stdout and stderr to a
file, so we can catch errors. The watchman patch uses warning() to
report errors, for example. And there is always a chance of die().

Then we need to report the errors back. I faced the same problem with
daemonizing git-gc, but I'm not sure if we can do exactly the same
here like in commit 329e6e8 (gc: save log from daemonized gc --auto
and print it next time - 2015-09-19)
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 10/15] update-index: enable/disable watchman support

[Duy Nguyen]

On Wed, Apr 20, 2016 at 6:28 AM, David Turner  wrote:
> +   if (use_watchman > 0) {
> +   the_index.last_update= xstrdup("");
> +   the_index.cache_changed |= WATCHMAN_CHANGED;
> +   } else if (!use_watchman) {
> +   the_index.last_update= NULL;
> +   the_index.cache_changed |= WATCHMAN_CHANGED;
> +   }
> +

We probably should warn people if index-helper is not built with
watchman support, which makes this knob completely useless. If
watchman fails to start, that's a separate problem..
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Tue, 2016-04-19 at 19:22 -0400, Jeff King wrote:
> You can find previous discussion on the list, but I think the options
> basically are:
> 
>   1. Something like v2, where the client gets a chance to speak
> before
>  the advertisement.
> 
>   2. Some out-of-band way of getting values from the client to the
>  server (so maybe extra command-line arguments for git-over-ssh,
> and
>  maybe shoving something after the "\0" for git-daemon, and of
>  course extra parameters for HTTP).
> 
>   3. The client saying "stop spewing refs at me, I want to give you a
>  ref filter" asynchronously, and accepting a little spew at the
>  beginning of each conversation. That obviously only works for
> the
>  full-duplex transports, so you'd probably fall back to (2) for
>  http.

OK, so (2) seems like what I'm doing -- it just happens that I only
implemented it for one protocol. 
--
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 v5 14/15] index-helper: autorun mode

[David Turner]

Soon, we'll want to automatically start index-helper, so we need
a mode that silently exits if it can't start up (either because
it's not in a git dir, or because another one is already running).

Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  4 
 index-helper.c | 29 +++--
 t/t7900-index-helper.sh|  8 
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index 7d80638..91d7a36 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -40,6 +40,10 @@ OPTIONS
 --kill::
Kill any running index-helper and clean up the socket
 
+--autorun::
+   If index-helper is not already running, start it.  Else, do
+   nothing.
+
 NOTES
 -
 
diff --git a/index-helper.c b/index-helper.c
index 8fcb76e..3d884da 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -381,8 +381,9 @@ static void request_kill(void)
 int main(int argc, char **argv)
 {
const char *prefix;
-   int idle_in_seconds = 600, detach = 0, kill = 0;
+   int idle_in_seconds = 600, detach = 0, kill = 0, autorun = 0;
int fd;
+   int nongit;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
OPT_INTEGER(0, "exit-after", _in_seconds,
@@ -391,6 +392,7 @@ int main(int argc, char **argv)
 "verify shared memory after creating"),
OPT_BOOL(0, "detach", , "detach the process"),
OPT_BOOL(0, "kill", , "request that existing index helper 
processes exit"),
+   OPT_BOOL(0, "autorun", , "this is an automatic run of 
git index-helper, so certain errors can be solved by silently exiting"),
OPT_END()
};
 
@@ -400,7 +402,14 @@ int main(int argc, char **argv)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(usage_text, options);
 
-   prefix = setup_git_directory();
+   prefix = setup_git_directory_gently();
+   if (nongit) {
+   if (autorun)
+   exit(0);
+   else
+   die(_("not a git repository"));
+   }
+
if (parse_options(argc, (const char **)argv, prefix,
  options, usage_text, 0))
die(_("too many arguments"));
@@ -414,10 +423,18 @@ int main(int argc, char **argv)
 
/* check that no other copy is running */
fd = unix_stream_connect(git_path("index-helper.sock"));
-   if (fd > 0)
-   die(_("Already running"));
-   if (errno != ECONNREFUSED && errno != ENOENT)
-   die_errno(_("Unexpected error checking socket"));
+   if (fd > 0) {
+   if (autorun)
+   exit(0);
+   else
+   die(_("Already running"));
+   }
+   if (errno != ECONNREFUSED && errno != ENOENT) {
+   if (autorun)
+   return 0;
+   else
+   die_errno(_("Unexpected error checking socket"));
+   }
 
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 7159971..aa6e5fc 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -38,4 +38,12 @@ test_expect_success 'index-helper will not start if already 
running' '
grep "Already running" err
 '
 
+test_expect_success 'index-helper is quiet with --autorun' '
+   test_when_finished "git index-helper --kill" &&
+   git index-helper --kill &&
+   git index-helper --detach &&
+   test -S .git/index-helper.sock &&
+   git index-helper --autorun
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 06/15] index-helper: add --detach

[David Turner]

From: Nguyễn Thái Ngọc Duy 

We detach after creating and opening the socket, because otherwise
we might return control to the shell before index-helper is ready to
accept commands.  This might lead to flaky tests.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt | 3 +++
 index-helper.c | 9 +++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index a5c058f..f6aa525 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -31,6 +31,9 @@ OPTIONS
for reading an index, but because it will happen in the
background, it's not noticable. `--strict` is enabled by default.
 
+--detach::
+   Detach from the shell.
+
 NOTES
 -
 
diff --git a/index-helper.c b/index-helper.c
index e8d6b64..80b1dbe 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -16,7 +16,7 @@ struct shm {
 
 static struct shm shm_index;
 static struct shm shm_base_index;
-static int to_verify = 1;
+static int daemonized, to_verify = 1;
 
 static void release_index_shm(struct shm *is)
 {
@@ -35,6 +35,8 @@ static void cleanup_shm(void)
 
 static void cleanup(void)
 {
+   if (daemonized)
+   return;
unlink(git_path("index-helper.sock"));
cleanup_shm();
 }
@@ -286,7 +288,7 @@ static const char * const usage_text[] = {
 int main(int argc, char **argv)
 {
const char *prefix;
-   int idle_in_seconds = 600;
+   int idle_in_seconds = 600, detach = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -294,6 +296,7 @@ int main(int argc, char **argv)
N_("exit if not used after some seconds")),
OPT_BOOL(0, "strict", _verify,
 "verify shared memory after creating"),
+   OPT_BOOL(0, "detach", , "detach the process"),
OPT_END()
};
 
@@ -317,6 +320,8 @@ int main(int argc, char **argv)
if (fd < 0)
die_errno(_("could not set up index-helper socket"));
 
+   if (detach && daemonize())
+   die_errno(_("unable to detach"));
loop(fd, idle_in_seconds);
 
close(fd);
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 10/15] update-index: enable/disable watchman support

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  3 +++
 Documentation/git-update-index.txt |  6 ++
 builtin/update-index.c | 11 +++
 3 files changed, 20 insertions(+)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index b1fa49e..dd039a1 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -15,6 +15,9 @@ DESCRIPTION
 Keep the index file in memory for faster access. This daemon is per
 repository.
 
+If you want the index-helper to accelerate untracked file checking,
+run git update-index --watchman before using it.
+
 OPTIONS
 ---
 
diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index c6cbed1..6736487 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -19,6 +19,7 @@ SYNOPSIS
 [--ignore-submodules]
 [--[no-]split-index]
 [--[no-|test-|force-]untracked-cache]
+[--[no-]watchman]
 [--really-refresh] [--unresolve] [--again | -g]
 [--info-only] [--index-info]
 [-z] [--stdin] [--index-version ]
@@ -176,6 +177,11 @@ may not support it yet.
 --no-untracked-cache::
Enable or disable untracked cache feature. Please use
`--test-untracked-cache` before enabling it.
+
+--watchman::
+--no-watchman::
+   Enable or disable watchman support. This is, at present,
+   only useful with git index-helper.
 +
 These options take effect whatever the value of the `core.untrackedCache`
 configuration variable (see linkgit:git-config[1]). But a warning is
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 1c94ca5..7c08e1c 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -914,6 +914,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
 {
int newfd, entries, has_errors = 0, nul_term_line = 0;
enum uc_mode untracked_cache = UC_UNSPECIFIED;
+   int use_watchman = -1;
int read_from_stdin = 0;
int prefix_length = prefix ? strlen(prefix) : 0;
int preferred_index_format = 0;
@@ -1012,6 +1013,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
N_("test if the filesystem supports untracked 
cache"), UC_TEST),
OPT_SET_INT(0, "force-untracked-cache", _cache,
N_("enable untracked cache without testing the 
filesystem"), UC_FORCE),
+   OPT_BOOL(0, "watchman", _watchman,
+   N_("use or not use watchman to reduce refresh cost")),
OPT_END()
};
 
@@ -1149,6 +1152,14 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
die("Bug: bad untracked_cache value: %d", untracked_cache);
}
 
+   if (use_watchman > 0) {
+   the_index.last_update= xstrdup("");
+   the_index.cache_changed |= WATCHMAN_CHANGED;
+   } else if (!use_watchman) {
+   the_index.last_update= NULL;
+   the_index.cache_changed |= WATCHMAN_CHANGED;
+   }
+
if (active_cache_changed) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 03/15] index-helper: new daemon for caching index and related stuff

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Instead of reading the index from disk and worrying about disk
corruption, the index is cached in memory (memory bit-flips happen
too, but hopefully less often). The result is faster read. Read time
is reduced by 70%.

The biggest gain is not having to verify the trailing SHA-1, which
takes lots of time especially on large index files. But this also
opens doors for further optimiztions:

 - we could create an in-memory format that's essentially the memory
   dump of the index to eliminate most of parsing/allocation
   overhead. The mmap'd memory can be used straight away. Experiment
   [1] shows we could reduce read time by 88%.

 - we could cache non-index info such as name hash

Shared memory is done by storing files in a per-repository temporary
directory.  This is more portable than shm (which requires
posix-realtime and has various quirks on OS X).  It might even work on
Windows, although this has not been tested. The shared memory file's
name folows the template "shm--" where  is the
trailing SHA-1 of the index file.  is "index" for cached index
files (and might later be "name-hash" for name-hash cache). If such
shared memory exists, it contains the same index content as on
disk. The content is already validated by the daemon and git won't
validate it again (except comparing the trailing SHA-1s).

We keep this daemon's logic as thin as possible. The "brain" stays in
git. So the daemon can read and validate stuff, but that's all it's
allowed to do. It does not add/create new information. It doesn't even
accept direct updates from git.

Git can poke the daemon via unix domain sockets to tell it to refresh
the index cache, or to keep it alive some more minutes. It can't give
any real index data directly to the daemon. Real data goes to disk
first, then the daemon reads and verifies it from there. Poking only
happens for $GIT_DIR/index, not temporary index files.

$GIT_DIR/index-helper.sock is the socket for the daemon process. The
daemon reads from the socket and executes commands.

Named pipes were considered for portability reasons, but then commands
that need replies from the daemon would have open their own pipes,
since a named pipe should only have one reader.  Unix domain sockets
don't have this problem.

On webkit.git with index format v2, duplicating 8 times to 1.5m
entries and 236MB in size:

(vanilla)  0.50 s: read_index_from .git/index
(index-helper) 0.18 s: read_index_from .git/index

Interestingly with index v4, we get less out of index-helper. It makes
sense as v4 requires more processing after loading the index:

(vanilla)  0.37 s: read_index_from .git/index
(index-helper) 0.22 s: read_index_from .git/index

[1] http://thread.gmane.org/gmane.comp.version-control.git/247268/focus=248771

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
Signed-off-by: Ramsay Jones 
---
 .gitignore |   1 +
 Documentation/git-index-helper.txt |  47 +++
 Makefile   |   5 +
 cache.h|   2 +
 git-compat-util.h  |   1 +
 index-helper.c | 276 +
 read-cache.c   | 120 ++--
 t/t7900-index-helper.sh|  23 
 8 files changed, 466 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/git-index-helper.txt
 create mode 100644 index-helper.c
 create mode 100755 t/t7900-index-helper.sh

diff --git a/.gitignore b/.gitignore
index 5087ce1..b92f122 100644
--- a/.gitignore
+++ b/.gitignore
@@ -71,6 +71,7 @@
 /git-http-fetch
 /git-http-push
 /git-imap-send
+/git-index-helper
 /git-index-pack
 /git-init
 /git-init-db
diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
new file mode 100644
index 000..77687c0
--- /dev/null
+++ b/Documentation/git-index-helper.txt
@@ -0,0 +1,47 @@
+git-index-helper(1)
+===
+
+NAME
+
+git-index-helper - A simple cache daemon for speeding up index file access
+
+SYNOPSIS
+
+[verse]
+'git index-helper' [options]
+
+DESCRIPTION
+---
+Keep the index file in memory for faster access. This daemon is per
+repository.
+
+OPTIONS
+---
+
+--exit-after=::
+   Exit if the cached index is not accessed for ``
+   seconds. Specify 0 to wait forever. Default is 600.
+
+NOTES
+-
+
+$GIT_DIR/index-helper.sock a Unix domain socket that the daemon reads
+commands from.  The directory will also contain files named
+"shm-index-".  These are used as backing stores for shared
+memory.  Normally the daemon will clean up these files when it exits
+or when they are no longer relevant.  But if it crashes, some objects
+could remain there and they can be safely deleted with "rm"
+command. The following commands are used to control the daemon:
+
+"refresh"::
+   Reread the index.
+

[PATCH v5 07/15] read-cache: add watchman 'WAMA' extension

[David Turner]

From: Nguyễn Thái Ngọc Duy 

The extension contains a bitmap, one bit for each entry in the
index. If the n-th bit is zero, the n-th entry is considered
unchanged, we can ce_mark_uptodate() it without refreshing. If the bit
is non-zero and we found out the corresponding file is clean after
refresh, we can clear the bit.

In addition, there's a list of directories in the untracked-cache
to invalidate (because they have new or modified entries).

The 'skipping refresh' bit is not in this patch yet as we would need
watchman. More details in later patches.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/technical/index-format.txt |  22 ++
 cache.h  |   4 ++
 dir.h|   3 +
 read-cache.c | 117 ++-
 4 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/Documentation/technical/index-format.txt 
b/Documentation/technical/index-format.txt
index ade0b0c..86ed3a6 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -295,3 +295,25 @@ The remaining data of each directory block is grouped by 
type:
 in the previous ewah bitmap.
 
   - One NUL.
+
+== Watchman cache
+
+  The watchman cache tracks files for which watchman has told us about
+  changes.  The signature for this extension is { 'W', 'A', 'M', 'A' }.
+
+  The extension starts with
+
+  - A NUL-terminated string: the watchman vector clock at the last
+time we heard from watchman.
+
+  - 32-bit bitmap size: the size of the CE_WATCHMAN_DIRTY bitmap
+
+  - 32-bit untracked cache entry count: the number of dirty untracked
+cache entries
+
+  - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+is CE_WATCHMAN_DIRTY.
+
+  - a list of N NUL-terminated strings.  Each is a directory that should
+be marked dirty in the untracked cache because watchman has told us
+about an update to a file in it.
diff --git a/cache.h b/cache.h
index 0aeb994..f4f7eef 100644
--- a/cache.h
+++ b/cache.h
@@ -182,6 +182,8 @@ struct cache_entry {
 #define CE_VALID (0x8000)
 #define CE_STAGESHIFT 12
 
+#define CE_WATCHMAN_DIRTY  (0x0001)
+
 /*
  * Range 0x0FFF in ce_flags is divided into
  * two parts: in-memory flags and on-disk ones.
@@ -320,6 +322,7 @@ static inline unsigned int canon_mode(unsigned int mode)
 #define CACHE_TREE_CHANGED (1 << 5)
 #define SPLIT_INDEX_ORDERED(1 << 6)
 #define UNTRACKED_CHANGED  (1 << 7)
+#define WATCHMAN_CHANGED   (1 << 8)
 
 struct split_index;
 struct untracked_cache;
@@ -344,6 +347,7 @@ struct index_state {
struct untracked_cache *untracked;
void *mmap;
size_t mmap_size;
+   char *last_update;
 };
 
 extern struct index_state the_index;
diff --git a/dir.h b/dir.h
index 3ec3fb0..3d540de 100644
--- a/dir.h
+++ b/dir.h
@@ -142,6 +142,9 @@ struct untracked_cache {
int gitignore_invalidated;
int dir_invalidated;
int dir_opened;
+   /* watchman invalidation data */
+   unsigned int use_watchman : 1;
+   struct string_list invalid_untracked;
 };
 
 struct dir_struct {
diff --git a/read-cache.c b/read-cache.c
index 5df5b21..050668f 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -19,6 +19,7 @@
 #include "split-index.h"
 #include "utf8.h"
 #include "unix-socket.h"
+#include "ewah/ewok.h"
 
 static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
   unsigned int options);
@@ -41,11 +42,13 @@ static struct cache_entry *refresh_cache_entry(struct 
cache_entry *ce,
 #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
 #define CACHE_EXT_LINK 0x6c696e6b/* "link" */
 #define CACHE_EXT_UNTRACKED 0x554E5452   /* "UNTR" */
+#define CACHE_EXT_WATCHMAN 0x57414D41/* "WAMA" */
 
 /* changes that can be kept in $GIT_DIR/index (basically all extensions) */
 #define EXTMASK (RESOLVE_UNDO_CHANGED | CACHE_TREE_CHANGED | \
 CE_ENTRY_ADDED | CE_ENTRY_REMOVED | CE_ENTRY_CHANGED | \
-SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED)
+SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED | \
+WATCHMAN_CHANGED)
 
 struct index_state the_index;
 static const char *alternate_index_output;
@@ -1220,8 +1223,13 @@ int refresh_index(struct index_state *istate, unsigned 
int flags,
continue;
 
new = refresh_cache_ent(istate, ce, options, _errno, 
);
-   if (new == ce)
+   if (new == ce) {
+   if (ce->ce_flags & CE_WATCHMAN_DIRTY) {
+   ce->ce_flags  &= ~CE_WATCHMAN_DIRTY;
+   istate->cache_changed |= WATCHMAN_CHANGED;
+   }
continue;
+   }
if (!new) {

[PATCH v5 02/15] read-cache: allow to keep mmap'd memory after reading

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Later, we will introduce git index-helper to share this memory with
other git processes.

Since the memory will be shared, it will never be unmapped (although
the kernel may of course choose to page it out).

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 cache.h  |  3 +++
 read-cache.c | 13 -
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index b829410..4180e2b 100644
--- a/cache.h
+++ b/cache.h
@@ -333,11 +333,14 @@ struct index_state {
struct split_index *split_index;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+keep_mmap : 1,
 initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
unsigned char sha1[20];
struct untracked_cache *untracked;
+   void *mmap;
+   size_t mmap_size;
 };
 
 extern struct index_state the_index;
diff --git a/read-cache.c b/read-cache.c
index 16cc487..7e387e9 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1574,6 +1574,10 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
mmap = xmmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE, fd, 0);
if (mmap == MAP_FAILED)
die_errno("unable to map index file");
+   if (istate->keep_mmap) {
+   istate->mmap = mmap;
+   istate->mmap_size = mmap_size;
+   }
close(fd);
 
hdr = mmap;
@@ -1626,10 +1630,12 @@ int do_read_index(struct index_state *istate, const 
char *path, int must_exist)
src_offset += 8;
src_offset += extsize;
}
-   munmap(mmap, mmap_size);
+   if (!istate->keep_mmap)
+   munmap(mmap, mmap_size);
return istate->cache_nr;
 
 unmap:
+   istate->mmap = NULL;
munmap(mmap, mmap_size);
die("index file corrupt");
 }
@@ -1655,6 +1661,7 @@ int read_index_from(struct index_state *istate, const 
char *path)
discard_index(split_index->base);
else
split_index->base = xcalloc(1, sizeof(*split_index->base));
+   split_index->base->keep_mmap = istate->keep_mmap;
ret = do_read_index(split_index->base,
git_path("sharedindex.%s",
 sha1_to_hex(split_index->base_sha1)), 1);
@@ -1698,6 +1705,10 @@ int discard_index(struct index_state *istate)
free(istate->cache);
istate->cache = NULL;
istate->cache_alloc = 0;
+   if (istate->keep_mmap && istate->mmap) {
+   munmap(istate->mmap, istate->mmap_size);
+   istate->mmap = NULL;
+   }
discard_split_index(istate);
free_untracked_cache(istate->untracked);
istate->untracked = NULL;
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 08/15] Add watchman support to reduce index refresh cost

[David Turner]

From: Nguyễn Thái Ngọc Duy 

The previous patch has the logic to clear bits in 'WAMA' bitmap. This
patch has logic to set bits as told by watchman. The missing bit,
_using_ these bits, are not here yet.

A lot of this code is written by David Turner originally, mostly from
[1]. I'm just copying and polishing it a bit.

[1] http://article.gmane.org/gmane.comp.version-control.git/248006

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Makefile   |  12 +
 cache.h|   1 +
 config.c   |   5 ++
 configure.ac   |   8 
 environment.c  |   3 ++
 watchman-support.c | 135 +
 watchman-support.h |   7 +++
 7 files changed, 171 insertions(+)
 create mode 100644 watchman-support.c
 create mode 100644 watchman-support.h

diff --git a/Makefile b/Makefile
index c8be0e7..65ab0f4 100644
--- a/Makefile
+++ b/Makefile
@@ -451,6 +451,7 @@ MSGFMT = msgfmt
 CURL_CONFIG = curl-config
 PTHREAD_LIBS = -lpthread
 PTHREAD_CFLAGS =
+WATCHMAN_LIBS =
 GCOV = gcov
 
 export TCL_PATH TCLTK_PATH
@@ -1416,6 +1417,13 @@ else
LIB_OBJS += thread-utils.o
 endif
 
+ifdef USE_WATCHMAN
+   LIB_H += watchman-support.h
+   LIB_OBJS += watchman-support.o
+   WATCHMAN_LIBS = -lwatchman
+   BASIC_CFLAGS += -DUSE_WATCHMAN
+endif
+
 ifdef HAVE_PATHS_H
BASIC_CFLAGS += -DHAVE_PATHS_H
 endif
@@ -2025,6 +2033,9 @@ git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS 
$(GITLIBS) $(VCSSVN_LIB)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) 
$(LIBS) \
$(VCSSVN_LIB)
 
+git-index-helper$X: index-helper.o GIT-LDFLAGS $(GITLIBS)
+   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) 
$(LIBS) $(WATCHMAN_LIBS)
+
 $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY)
$(QUIET_LNCP)$(RM) $@ && \
ln $< $@ 2>/dev/null || \
@@ -2164,6 +2175,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
@echo NO_MMAP=\''$(subst ','\'',$(subst ','\'',$(NO_MMAP)))'\' >>$@+
+   @echo USE_WATCHMAN=\''$(subst ','\'',$(subst 
','\'',$(USE_WATCHMAN)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/cache.h b/cache.h
index f4f7eef..37f211b 100644
--- a/cache.h
+++ b/cache.h
@@ -687,6 +687,7 @@ extern char *git_replace_ref_base;
 
 extern int fsync_object_files;
 extern int core_preload_index;
+extern int core_watchman_sync_timeout;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
 extern int protect_hfs;
diff --git a/config.c b/config.c
index 9ba40bc..e6dc141 100644
--- a/config.c
+++ b/config.c
@@ -882,6 +882,11 @@ static int git_default_core_config(const char *var, const 
char *value)
return 0;
}
 
+   if (!strcmp(var, "core.watchmansynctimeout")) {
+   core_watchman_sync_timeout = git_config_int(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "core.createobject")) {
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
diff --git a/configure.ac b/configure.ac
index 0cd9f46..334d63b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1099,6 +1099,14 @@ AC_COMPILE_IFELSE([BSD_SYSCTL_SRC],
HAVE_BSD_SYSCTL=])
 GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
 
+#
+# Check for watchman client library
+
+AC_CHECK_LIB([watchman], [watchman_connect],
+   [USE_WATCHMAN=YesPlease],
+   [USE_WATCHMAN=])
+GIT_CONF_SUBST([USE_WATCHMAN])
+
 ## Other checks.
 # Define USE_PIC if you need the main git objects to be built with -fPIC
 # in order to build and link perl/Git.so.  x86-64 seems to need this.
diff --git a/environment.c b/environment.c
index 6dec9d0..35e03c7 100644
--- a/environment.c
+++ b/environment.c
@@ -94,6 +94,9 @@ int core_preload_index = 1;
  */
 int ignore_untracked_cache_config;
 
+int core_watchman_sync_timeout = 300;
+
+
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
 static char *work_tree;
diff --git a/watchman-support.c b/watchman-support.c
new file mode 100644
index 000..b168e88
--- /dev/null
+++ b/watchman-support.c
@@ -0,0 +1,135 @@
+#include "cache.h"
+#include "watchman-support.h"
+#include "strbuf.h"
+#include "dir.h"
+#include 
+
+static struct watchman_query *make_query(const char *last_update)
+{
+   struct watchman_query *query = watchman_query();
+   watchman_query_set_fields(query, WATCHMAN_FIELD_NAME |
+WATCHMAN_FIELD_EXISTS |
+WATCHMAN_FIELD_NEWER);
+   watchman_query_set_empty_on_fresh(query, 1);
+   

[PATCH v5 13/15] index-helper: don't run if already running

[David Turner]

Signed-off-by: David Turner 
---
 index-helper.c  | 7 +++
 t/t7900-index-helper.sh | 9 +
 2 files changed, 16 insertions(+)

diff --git a/index-helper.c b/index-helper.c
index 6af01c9..8fcb76e 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -412,6 +412,13 @@ int main(int argc, char **argv)
return 0;
}
 
+   /* check that no other copy is running */
+   fd = unix_stream_connect(git_path("index-helper.sock"));
+   if (fd > 0)
+   die(_("Already running"));
+   if (errno != ECONNREFUSED && errno != ENOENT)
+   die_errno(_("Unexpected error checking socket"));
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
 
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index e71b5af..7159971 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -29,4 +29,13 @@ test_expect_success 'index-helper creates usable path file 
and can be killed' '
test_path_is_missing .git/index-helper.sock
 '
 
+test_expect_success 'index-helper will not start if already running' '
+   test_when_finished "git index-helper --kill" &&
+   git index-helper --detach &&
+   test -S .git/index-helper.sock &&
+   test_must_fail git index-helper 2>err &&
+   test -S .git/index-helper.sock &&
+   grep "Already running" err
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 15/15] index-helper: optionally automatically run

[David Turner]

Introduce a new config option, indexhelper.autorun, to automatically
run git index-helper before starting up a builtin git command.  This
enables users to keep index-helper running without manual
intervention.

Signed-off-by: David Turner 
---
 Documentation/config.txt |  4 
 read-cache.c | 37 +++--
 t/t7900-index-helper.sh  | 19 +++
 3 files changed, 58 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..8ec8824 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1852,6 +1852,10 @@ index.version::
Specify the version with which new index files should be
initialized.  This does not affect existing repositories.
 
+indexhelper.autorun::
+   Automatically run git index-helper when any builtin git
+   command is run inside a repository.
+
 init.templateDir::
Specify the directory from which templates will be copied.
(See the "TEMPLATE DIRECTORY" section of linkgit:git-init[1].)
diff --git a/read-cache.c b/read-cache.c
index 65f22f9..a73487e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -20,6 +20,7 @@
 #include "utf8.h"
 #include "unix-socket.h"
 #include "ewah/ewok.h"
+#include "run-command.h"
 
 static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
   unsigned int options);
@@ -1722,6 +1723,33 @@ static void post_read_index_from(struct index_state 
*istate)
tweak_untracked_cache(istate);
 }
 
+static int want_auto_index_helper(void)
+{
+   int want = -1;
+   const char *value = NULL;
+   const char *key = "indexhelper.autorun";
+
+   if (git_config_key_is_valid(key) &&
+   !git_config_get_value(key, )) {
+   int b = git_config_maybe_bool(key, value);
+   want = b >= 0 ? b : 0;
+   }
+   return want;
+}
+
+static void autorun_index_helper(void)
+{
+   const char *argv[] = {"git-index-helper", "--detach", "--autorun", 
NULL};
+   if (want_auto_index_helper() <= 0)
+   return;
+
+   trace_argv_printf(argv, "trace: auto index-helper:");
+
+   if (run_command_v_opt(argv,
+ RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT))
+   warning(_("You specified indexhelper.autorun, but running 
git-index-helper failed."));
+}
+
 static int poke_and_wait_for_reply(int fd)
 {
struct strbuf buf = STRBUF_INIT;
@@ -1776,6 +1804,7 @@ static int poke_daemon(struct index_state *istate,
if (fd < 0) {
warning("Failed to connect to index-helper socket");
unlink(git_path("index-helper.sock"));
+   autorun_index_helper();
return -1;
}
 
@@ -1811,9 +1840,13 @@ static int try_shm(struct index_state *istate)
int fd = -1;
 
if (!is_main_index(istate) ||
-   old_size <= 20 ||
-   stat(git_path("index-helper.sock"), ))
+   old_size <= 20)
return -1;
+
+   if (stat(git_path("index-helper.sock"), )) {
+   autorun_index_helper();
+   return -1;
+   }
if (poke_daemon(istate, , 0))
return -1;
sha1 = (unsigned char *)istate->mmap + old_size - 20;
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index aa6e5fc..2d3ce3c 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -16,6 +16,9 @@ test -n "$NO_MMAP" && {
 }
 
 test_expect_success 'index-helper smoke test' '
+   # We need an existing commit so that the index exists (otherwise,
+   # the index-helper will not be autostarted)
+   test_commit x &&
git index-helper --exit-after 1 &&
test_path_is_missing .git/index-helper.sock
 '
@@ -46,4 +49,20 @@ test_expect_success 'index-helper is quiet with --autorun' '
git index-helper --autorun
 '
 
+test_expect_success 'index-helper autorun works' '
+   rm -f .git/index-helper.sock &&
+   git status &&
+   test_path_is_missing .git/index-helper.sock &&
+   test_config indexhelper.autorun true &&
+   git status &&
+   test -S .git/index-helper.sock &&
+   git status 2>err &&
+   test -S .git/index-helper.sock &&
+   ! grep -q . err &&
+   git index-helper --kill &&
+   test_config indexhelper.autorun false &&
+   git status &&
+   test_path_is_missing .git/index-helper.sock
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 11/15] unpack-trees: preserve index extensions

[David Turner]

Make git checkout (and other unpack_tree operations) preserve the
untracked cache and watchman status. This is valuable for two reasons:

1. Often, an unpack_tree operation will not touch large parts of the
working tree, and thus most of the untracked cache will continue to be
valid.

2. Even if the untracked cache were entirely invalidated by such an
operation, the user has signaled their intention to have such a cache,
and we don't want to throw it away.

The same logic applies to the watchman state.

Signed-off-by: David Turner 
---
 cache.h   |  1 +
 read-cache.c  |  8 
 t/t7063-status-untracked-cache.sh | 22 ++
 t/test-lib-functions.sh   |  4 
 unpack-trees.c|  1 +
 5 files changed, 36 insertions(+)

diff --git a/cache.h b/cache.h
index 4cc89bb..49fa128 100644
--- a/cache.h
+++ b/cache.h
@@ -571,6 +571,7 @@ extern void write_watchman_ext(struct strbuf *sb, struct 
index_state* istate);
 #define CLOSE_LOCK (1 << 1)
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
 extern int discard_index(struct index_state *);
+extern void move_index_extensions(struct index_state *dst, struct index_state 
*src);
 extern int unmerged_index(const struct index_state *);
 extern int verify_path(const char *path);
 extern int index_dir_exists(struct index_state *istate, const char *name, int 
namelen);
diff --git a/read-cache.c b/read-cache.c
index fb0168e..65f22f9 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2745,3 +2745,11 @@ void stat_validity_update(struct stat_validity *sv, int 
fd)
fill_stat_data(sv->sd, );
}
 }
+
+void move_index_extensions(struct index_state *dst, struct index_state *src)
+{
+   dst->untracked = src->untracked;
+   src->untracked = NULL;
+   dst->last_update = src->last_update;
+   src->last_update = NULL;
+}
diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index a971884..083516d 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -646,4 +646,26 @@ test_expect_success 'test ident field is working' '
test_cmp ../expect ../err
 '
 
+test_expect_success 'untracked cache survives a checkout' '
+   git commit --allow-empty -m empty &&
+   test-dump-untracked-cache >../before &&
+   test_when_finished  "git checkout master" &&
+   git checkout -b other_branch &&
+   test-dump-untracked-cache >../after &&
+   test_cmp ../before ../after &&
+   test_commit test &&
+   test-dump-untracked-cache >../before &&
+   git checkout master &&
+   test-dump-untracked-cache >../after &&
+   test_cmp ../before ../after
+'
+
+test_expect_success 'untracked cache survives a commit' '
+   test-dump-untracked-cache >../before &&
+   git add done/two &&
+   git commit -m commit &&
+   test-dump-untracked-cache >../after &&
+   test_cmp ../before ../after
+'
+
 test_done
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d99eb3..e974b5b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -186,6 +186,10 @@ test_commit () {
test_tick
fi &&
git commit $signoff -m "$1" &&
+   if [ "$(git config core.bare)" = false ]
+   then
+   git update-index --force-untracked-cache
+   fi
git tag "${4:-$1}"
 }
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 9f55cc2..fc90eb3 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1215,6 +1215,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
  WRITE_TREE_SILENT |
  WRITE_TREE_REPAIR);
}
+   move_index_extensions(>result, o->dst_index);
discard_index(o->dst_index);
*o->dst_index = o->result;
} else {
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 04/15] index-helper: add --strict

[David Turner]

From: Nguyễn Thái Ngọc Duy 

There are "holes" in the index-helper approach because the shared
memory is not verified again by git. If $USER is compromised, shared
memory could be modified. But anyone who could do this could already
modify $GIT_DIR/index. A more realistic risk is some bugs in
index-helper that produce corrupt shared memory. --strict is added to
avoid that.

Strictly speaking there's still a very small gap where corrupt shared
memory could still be read by git: after we write the trailing SHA-1 in
the shared memory (thus signaling "this shm is ready") and before
verify_shm() detects an error.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  9 +++
 cache.h|  1 +
 index-helper.c | 48 ++
 read-cache.c   |  9 ---
 4 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index 77687c0..a5c058f 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -22,6 +22,15 @@ OPTIONS
Exit if the cached index is not accessed for ``
seconds. Specify 0 to wait forever. Default is 600.
 
+--strict::
+--no-strict::
+   Strict mode makes index-helper verify the shared memory after
+   it's created. If the result does not match what's read from
+   $GIT_DIR/index, the shared memory is destroyed. This makes
+   index-helper take more than double the amount of time required
+   for reading an index, but because it will happen in the
+   background, it's not noticable. `--strict` is enabled by default.
+
 NOTES
 -
 
diff --git a/cache.h b/cache.h
index 43fb314..4b678e9 100644
--- a/cache.h
+++ b/cache.h
@@ -336,6 +336,7 @@ struct index_state {
 keep_mmap : 1,
 from_shm : 1,
 to_shm : 1,
+always_verify_trailing_sha1 : 1,
 initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
diff --git a/index-helper.c b/index-helper.c
index d64e49b..e8d6b64 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -16,6 +16,7 @@ struct shm {
 
 static struct shm shm_index;
 static struct shm shm_base_index;
+static int to_verify = 1;
 
 static void release_index_shm(struct shm *is)
 {
@@ -110,11 +111,56 @@ static void share_index(struct index_state *istate, 
struct shm *is)
hashcpy((unsigned char *)new_mmap + istate->mmap_size - 20, is->sha1);
 }
 
+static int verify_shm(void)
+{
+   int i;
+   struct index_state istate;
+   memset(, 0, sizeof(istate));
+   istate.always_verify_trailing_sha1 = 1;
+   istate.to_shm = 1;
+   i = read_index();
+   if (i != the_index.cache_nr)
+   goto done;
+   for (; i < the_index.cache_nr; i++) {
+   struct cache_entry *base, *ce;
+   /* namelen is checked separately */
+   const unsigned int ondisk_flags =
+   CE_STAGEMASK | CE_VALID | CE_EXTENDED_FLAGS;
+   unsigned int ce_flags, base_flags, ret;
+   base = the_index.cache[i];
+   ce = istate.cache[i];
+   if (ce->ce_namelen != base->ce_namelen ||
+   strcmp(ce->name, base->name)) {
+   warning("mismatch at entry %d", i);
+   break;
+   }
+   ce_flags = ce->ce_flags;
+   base_flags = base->ce_flags;
+   /* only on-disk flags matter */
+   ce->ce_flags   &= ondisk_flags;
+   base->ce_flags &= ondisk_flags;
+   ret = memcmp(>ce_stat_data, >ce_stat_data,
+offsetof(struct cache_entry, name) -
+offsetof(struct cache_entry, ce_stat_data));
+   ce->ce_flags = ce_flags;
+   base->ce_flags = base_flags;
+   if (ret) {
+   warning("mismatch at entry %d", i);
+   break;
+   }
+   }
+done:
+   discard_index();
+   return i == the_index.cache_nr;
+}
+
 static void share_the_index(void)
 {
if (the_index.split_index && the_index.split_index->base)
share_index(the_index.split_index->base, _base_index);
share_index(_index, _index);
+   if (to_verify && !verify_shm())
+   cleanup_shm();
discard_index(_index);
 }
 
@@ -246,6 +292,8 @@ int main(int argc, char **argv)
struct option options[] = {
OPT_INTEGER(0, "exit-after", _in_seconds,
N_("exit if not used after some seconds")),
+   OPT_BOOL(0, "strict", _verify,
+"verify shared memory after creating"),
OPT_END()
};
 

[PATCH v5 05/15] daemonize(): set a flag before exiting the main process

[David Turner]

From: Nguyễn Thái Ngọc Duy 

This allows signal handlers and atexit functions to realize this
situation and not clean up.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 builtin/gc.c | 2 +-
 cache.h  | 2 +-
 daemon.c | 2 +-
 setup.c  | 4 +++-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad..37180de 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -385,7 +385,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 * failure to daemonize is ok, we'll continue
 * in foreground
 */
-   daemonized = !daemonize();
+   daemonized = !daemonize(NULL);
}
} else
add_repack_all_option();
diff --git a/cache.h b/cache.h
index 4b678e9..0aeb994 100644
--- a/cache.h
+++ b/cache.h
@@ -530,7 +530,7 @@ extern int set_git_dir_init(const char *git_dir, const char 
*real_git_dir, int);
 extern int init_db(const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
-extern int daemonize(void);
+extern int daemonize(int *);
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
diff --git a/daemon.c b/daemon.c
index 8d45c33..a5cf954 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1365,7 +1365,7 @@ int main(int argc, char **argv)
return execute();
 
if (detach) {
-   if (daemonize())
+   if (daemonize(NULL))
die("--detach not supported on this platform");
} else
sanitize_stdfds();
diff --git a/setup.c b/setup.c
index de1a2a7..9adf13f 100644
--- a/setup.c
+++ b/setup.c
@@ -1017,7 +1017,7 @@ void sanitize_stdfds(void)
close(fd);
 }
 
-int daemonize(void)
+int daemonize(int *daemonized)
 {
 #ifdef NO_POSIX_GOODIES
errno = ENOSYS;
@@ -1029,6 +1029,8 @@ int daemonize(void)
case -1:
die_errno("fork failed");
default:
+   if (daemonized)
+   *daemonized = 1;
exit(0);
}
if (setsid() == -1)
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 12/15] index-helper: kill mode

[David Turner]

Add a new command (and command-line arg) to allow index-helpers to
exit cleanly.

This is mainly useful for tests.

Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |  3 +++
 index-helper.c | 31 ++-
 t/t7900-index-helper.sh|  9 +
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index dd039a1..7d80638 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -37,6 +37,9 @@ OPTIONS
 --detach::
Detach from the shell.
 
+--kill::
+   Kill any running index-helper and clean up the socket
+
 NOTES
 -
 
diff --git a/index-helper.c b/index-helper.c
index 9d85a7c..6af01c9 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -327,6 +327,8 @@ static void loop(int fd, int idle_in_seconds)
 * alive, nothing to do.
 */
}
+   } else if (!strcmp(command.buf, "die")) {
+   break;
} else {
warning("BUG: Bogus command %s", command.buf);
}
@@ -357,10 +359,29 @@ static const char * const usage_text[] = {
NULL
 };
 
+static void request_kill(void)
+{
+   int fd = unix_stream_connect(git_path("index-helper.sock"));
+
+   if (fd >= 0) {
+   write_in_full(fd, "die", 4);
+   close(fd);
+   }
+
+   /*
+* The child will try to do this anyway, but we want to be
+* ready to launch a new daemon immediately after this command
+* returns.
+*/
+
+   unlink(git_path("index-helper.sock"));
+   return;
+}
+
 int main(int argc, char **argv)
 {
const char *prefix;
-   int idle_in_seconds = 600, detach = 0;
+   int idle_in_seconds = 600, detach = 0, kill = 0;
int fd;
struct strbuf socket_path = STRBUF_INIT;
struct option options[] = {
@@ -369,6 +390,7 @@ int main(int argc, char **argv)
OPT_BOOL(0, "strict", _verify,
 "verify shared memory after creating"),
OPT_BOOL(0, "detach", , "detach the process"),
+   OPT_BOOL(0, "kill", , "request that existing index helper 
processes exit"),
OPT_END()
};
 
@@ -383,6 +405,13 @@ int main(int argc, char **argv)
  options, usage_text, 0))
die(_("too many arguments"));
 
+   if (kill) {
+   if (detach)
+   die(_("--kill doesn't want any other options"));
+   request_kill();
+   return 0;
+   }
+
atexit(cleanup);
sigchain_push_common(cleanup_on_signal);
 
diff --git a/t/t7900-index-helper.sh b/t/t7900-index-helper.sh
index 114c112..e71b5af 100755
--- a/t/t7900-index-helper.sh
+++ b/t/t7900-index-helper.sh
@@ -20,4 +20,13 @@ test_expect_success 'index-helper smoke test' '
test_path_is_missing .git/index-helper.sock
 '
 
+test_expect_success 'index-helper creates usable path file and can be killed' '
+   test_when_finished "git index-helper --kill" &&
+   test_path_is_missing .git/index-helper.sock &&
+   git index-helper --detach &&
+   test -S .git/index-helper.sock &&
+   git index-helper --kill &&
+   test_path_is_missing .git/index-helper.sock
+'
+
 test_done
-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 09/15] index-helper: use watchman to avoid refreshing index with lstat()

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Watchman is hidden behind index-helper. Before git tries to read the
index from shm, it notifies index-helper through the socket and waits
for index-helper to prepare a file for sharing memory (with
MAP_SHARED). index-helper then contacts watchman, updates 'WAMA'
extension and put it in a separate file and wakes git up with a reply
to git's socket.

Git uses this extension to not lstat unchanged entries. Git only
trusts the 'WAMA' extension when it's received from the separate file,
not from disk. Unmarked entries are "clean". Marked entries are dirty
from watchman point of view. If it finds out some entries are
'watchman-dirty', but are really unchanged (e.g. the file was changed,
then reverted back), then Git will clear the marking in 'WAMA' before
writing it down.

Hiding watchman behind index-helper means you need both daemons. You
can't run watchman alone. Not so good. But on the other hand, 'git'
binary is not linked to watchman/json libraries, which is good for
packaging. Core git package will run fine without watchman-related
packages. If they need watchman, they can install git-index-helper and
dependencies.

This also lets us trust anything in the untracked cache that we haven't
marked invalid, saving those stat() calls.

Another reason for tying watchman to index-helper is, when used with
untracked cache, we need to keep track of $GIT_WORK_TREE file
listing. That kind of list can be kept in index-helper.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 Documentation/git-index-helper.txt |   6 +
 cache.h|   2 +
 dir.c  |  23 +++-
 dir.h  |   3 +
 index-helper.c |  86 +--
 read-cache.c   | 220 ++---
 6 files changed, 315 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index f6aa525..b1fa49e 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -52,6 +52,12 @@ command. The following commands are used to control the 
daemon:
Let the daemon know the index is to be read. It keeps the
daemon alive longer, unless `--exit-after=0` is used.
 
+"poke ":
+   Like "poke", but replies with "OK".  If the index has the
+   watchman extension, index-helper queries watchman, then
+   prepares a shared memory object with the watchman index
+   extension before replying.
+
 All commands and replies are terminated by a 0 byte.
 
 GIT
diff --git a/cache.h b/cache.h
index 37f211b..4cc89bb 100644
--- a/cache.h
+++ b/cache.h
@@ -558,6 +558,7 @@ extern int daemonize(int *);
 
 /* Initialize and use the cache information */
 struct lock_file;
+extern int verify_index(const struct index_state *);
 extern int read_index(struct index_state *);
 extern int read_index_preload(struct index_state *, const struct pathspec 
*pathspec);
 extern int do_read_index(struct index_state *istate, const char *path,
@@ -565,6 +566,7 @@ extern int do_read_index(struct index_state *istate, const 
char *path,
 extern int read_index_from(struct index_state *, const char *path);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
+extern void write_watchman_ext(struct strbuf *sb, struct index_state* istate);
 #define COMMIT_LOCK(1 << 0)
 #define CLOSE_LOCK (1 << 1)
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
diff --git a/dir.c b/dir.c
index 69e0be6..5058b29 100644
--- a/dir.c
+++ b/dir.c
@@ -597,9 +597,9 @@ static void trim_trailing_spaces(char *buf)
  *
  * If "name" has the trailing slash, it'll be excluded in the search.
  */
-static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
-   struct untracked_cache_dir 
*dir,
-   const char *name, int len)
+struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
+struct untracked_cache_dir *dir,
+const char *name, int len)
 {
int first, last;
struct untracked_cache_dir *d;
@@ -1726,6 +1726,17 @@ static int valid_cached_dir(struct dir_struct *dir,
if (!untracked)
return 0;
 
+   if (dir->untracked->use_watchman) {
+   /*
+* With watchman, we can trust the untracked cache's
+* valid field.
+*/
+   if (untracked->valid)
+   goto skip_stat;
+   else
+   invalidate_directory(dir->untracked, untracked);
+   }
+
if (stat(path->len ? path->buf : ".", )) {

[PATCH v5 00/15] index-helper/watchman

[David Turner]

This version includes the following changes since v4:
1. The last patch has been removed; it's pretty much always a good
idea to wait for the index-helper

2. Documentation for index-helper --kill and --autorun.  Documentation
for update-index --watchman.  Documentation for index-format for WAMA.
Documentation for index-helper suggesting running update-index --watchman.

3. index-helper autorun moved to read-cache so it's only run on relevant
commands.

4. Tests: Broken && chain fixed; removed a subshell

5. A couple of fd leaks fixed.

6. Cruft removed.

7. New perf numbers.

David Turner (5):
  unpack-trees: preserve index extensions
  index-helper: kill mode
  index-helper: don't run if already running
  index-helper: autorun mode
  index-helper: optionally automatically run

Nguyễn Thái Ngọc Duy (10):
  read-cache.c: fix constness of verify_hdr()
  read-cache: allow to keep mmap'd memory after reading
  index-helper: new daemon for caching index and related stuff
  index-helper: add --strict
  daemonize(): set a flag before exiting the main process
  index-helper: add --detach
  read-cache: add watchman 'WAMA' extension
  Add watchman support to reduce index refresh cost
  index-helper: use watchman to avoid refreshing index with lstat()
  update-index: enable/disable watchman support

 .gitignore   |   1 +
 Documentation/config.txt |   4 +
 Documentation/git-index-helper.txt   |  75 +
 Documentation/git-update-index.txt   |   6 +
 Documentation/technical/index-format.txt |  22 ++
 Makefile |  17 ++
 builtin/gc.c |   2 +-
 builtin/update-index.c   |  11 +
 cache.h  |  16 +-
 config.c |   5 +
 configure.ac |   8 +
 daemon.c |   2 +-
 dir.c|  23 +-
 dir.h|   6 +
 environment.c|   3 +
 git-compat-util.h|   1 +
 index-helper.c   | 454 
 read-cache.c | 488 ++-
 setup.c  |   4 +-
 t/t7063-status-untracked-cache.sh|  22 ++
 t/t7900-index-helper.sh  |  68 +
 t/test-lib-functions.sh  |   4 +
 unpack-trees.c   |   1 +
 watchman-support.c   | 135 +
 watchman-support.h   |   7 +
 25 files changed, 1364 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/git-index-helper.txt
 create mode 100644 index-helper.c
 create mode 100755 t/t7900-index-helper.sh
 create mode 100644 watchman-support.c
 create mode 100644 watchman-support.h

-- 
2.4.2.767.g62658d5-twtrsrc

--
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 v5 01/15] read-cache.c: fix constness of verify_hdr()

[David Turner]

From: Nguyễn Thái Ngọc Duy 

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: David Turner 
---
 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index d9fb78b..16cc487 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1345,7 +1345,7 @@ struct ondisk_cache_entry_extended {
ondisk_cache_entry_extended_size(ce_namelen(ce)) : \
ondisk_cache_entry_size(ce_namelen(ce)))
 
-static int verify_hdr(struct cache_header *hdr, unsigned long size)
+static int verify_hdr(const struct cache_header *hdr, unsigned long size)
 {
git_SHA_CTX c;
unsigned char sha1[20];
-- 
2.4.2.767.g62658d5-twtrsrc

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

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Jeff King]

On Tue, Apr 19, 2016 at 05:40:01PM -0400, David Turner wrote:

> > I dunno, I am a bit negative on bringing new features to Git-over
> > -HTTP
> > (which is already less efficient than the other protocols!) without
> > any
> > plan for supporting them in the other protocols.
> 
> Interesting -- can you expand on git-over-http being less efficient?
> This is the first I'd heard of it.  Is it documented somewhere?

I don't know offhand of thorough discussion I can link to. But
basically, the issue is the tip negotiation that happens during a fetch.
In the normal git-over-ssh and git-over-tcp protocols, we're
full-duplex, and both sides remember all of the state. So the client is
spewing "want" and "have" lines at the server, which is responding
asynchronously with acks or naks until they reach a shared point to
generate the pack from.

In the HTTP protocol, this negotiation has to happen via synchronous
request/response pairs. So the client says "here are some haves; what do
you think?" and gets back the response. Then it prepares another of
haves, and so on, until the server says "OK, I've seen enough; here's
the pack". But because the server is stateless, each request has to
summarize the findings of the prior request. And so each request gets
slightly bigger as we iterate.

There are some tunable parameters there (e.g., how many haves to send in
the first batch?), and the current settings are meant to be a mix of not
wasting too much time preparing a request, but also putting enough into
it that common requests can complete with only a single round trip.

I don't have numbers on how often we have to fall back multiple
requests, or how big they can grow. I know I have very occasionally seen
pathological cases where we outgrew the HTTP buffer sizes, and re-trying
the fetch via ssh just worked.

I'm cc-ing Shawn, who designed all of this, and can probably give more
details (and may also have opinions on new http-only protocol features,
as he'd probably end up implementing them in JGit, too).

It would be nice if we could do a true full-duplex conversation over
HTTP. I looked into Websockets at one point, but IIRC there wasn't
libcurl support for them.

> > So I'd rather see something like:
> > 
> >   1. Support for v2 "capabilities only" initial negotiation, followed
> >  by ref advertisement.
> > 
> >   2. Support for refspec-limiting capability.
> > 
> >   3. HTTP-only option from client to trigger v2 on the server.
> > 
> > That's still HTTP-specific, but it has a clear path for converging
> > with
> > the ssh and git protocols eventually, rather than having to support
> > magic out-of-band capabilities forever.
> > 
> > It does require an extra round of HTTP request/response, though.
> 
> This seems way more complicated to me, and not necessarily super
> -efficient.  That is, it seems like rather a lot of work to add a whole
> round of negotiation and a new protocol, when all we really need is one
> little tweak.

It is less efficient because of the extra round. If the new protocol
were truly client-speaks-first, we could drop that round (which is
essentially what your proposal is doing; you're just sticking the
first-speak part into HTTP parameters).

I don't know how much that round costs if it's part of the same TCP
session, or part of the same pipelined HTTP connection.

> I wonder if it would be possible to just add these tweaks to v1, and
> save the v2 work for when someone has the time to implement it?

I don't think it's possible for the non-HTTP protocols. The single
change in v2 is to add a phase before the ref advertisement starts.
Without that, the server is going to start spewing advertisements.

You can find previous discussion on the list, but I think the options
basically are:

  1. Something like v2, where the client gets a chance to speak before
 the advertisement.

  2. Some out-of-band way of getting values from the client to the
 server (so maybe extra command-line arguments for git-over-ssh, and
 maybe shoving something after the "\0" for git-daemon, and of
 course extra parameters for HTTP).

  3. The client saying "stop spewing refs at me, I want to give you a
 ref filter" asynchronously, and accepting a little spew at the
 beginning of each conversation. That obviously only works for the
 full-duplex transports, so you'd probably fall back to (2) for
 http.

-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 4/6] Convert struct name_entry to use struct object_id.

[brian m. carlson]

On Tue, Apr 19, 2016 at 04:02:22PM -0700, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> > @@ -314,7 +314,7 @@ static int threeway_callback(int n, unsigned long mask, 
> > unsigned long dirmask, s
> > }
> >  
> > if (same_entry(entry+0, entry+1)) {
> > -   if (entry[2].sha1 && !S_ISDIR(entry[2].mode)) {
> > +   if (entry[2].oid->hash && !S_ISDIR(entry[2].mode)) {
> 
> Thanks for a warning in the cover letter.
> 
> "if (entry[2].oid && !S_ISDIR(entry[2].mode)" would be a faithful
> conversion, wouldn't it?

Yes, I think that would be a better conversion.  I'll reroll after
waiting for further comments.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

[Junio C Hamano]

Jacob Keller  writes:

> On Tue, Apr 19, 2016 at 10:06 AM, Jeff King  wrote:
>> On Tue, Apr 19, 2016 at 08:17:38AM -0700, Stefan Beller wrote:
>>
>>> On Mon, Apr 18, 2016 at 10:03 PM, Jeff King  wrote:
>>>
>>> > I guess this will invalidate old patch-ids, but there's not much to be
>>> > done about that.
>>>
>>> What do you mean by that? (What consequences do you imagine?)
>>> I think diffs with any kind of heuristic can still be applied, no?
>>
>> I mean that if you save any old patch-ids from "git patch-id", they
>> won't match up when compared with new versions of git. We can probably
>> ignore it, though. This isn't the first time that patch-ids might have
>> changed, and I think the advice is already that one should not count on
>> them to be stable in the long term.
>>
>> -Peff
>
> Plus they'll be stable within a version of Git, it's only recorded
> patch ids that change, which hopefully isn't done very much if at all.
>
> Thanks,
> Jake

Some people, like those who did things like 30e12b92 (patch-id: make
it stable against hunk reordering, 2014-04-27), _may_ care.


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

Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

[Jacob Keller]

On Tue, Apr 19, 2016 at 10:06 AM, Jeff King  wrote:
> On Tue, Apr 19, 2016 at 08:17:38AM -0700, Stefan Beller wrote:
>
>> On Mon, Apr 18, 2016 at 10:03 PM, Jeff King  wrote:
>>
>> > I guess this will invalidate old patch-ids, but there's not much to be
>> > done about that.
>>
>> What do you mean by that? (What consequences do you imagine?)
>> I think diffs with any kind of heuristic can still be applied, no?
>
> I mean that if you save any old patch-ids from "git patch-id", they
> won't match up when compared with new versions of git. We can probably
> ignore it, though. This isn't the first time that patch-ids might have
> changed, and I think the advice is already that one should not count on
> them to be stable in the long term.
>
> -Peff

Plus they'll be stable within a version of Git, it's only recorded
patch ids that change, which hopefully isn't done very much if at all.

Thanks,
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: [PATCH 4/6] Convert struct name_entry to use struct object_id.

[Junio C Hamano]

"brian m. carlson"  writes:

> @@ -314,7 +314,7 @@ static int threeway_callback(int n, unsigned long mask, 
> unsigned long dirmask, s
>   }
>  
>   if (same_entry(entry+0, entry+1)) {
> - if (entry[2].sha1 && !S_ISDIR(entry[2].mode)) {
> + if (entry[2].oid->hash && !S_ISDIR(entry[2].mode)) {

Thanks for a warning in the cover letter.

"if (entry[2].oid && !S_ISDIR(entry[2].mode)" would be a faithful
conversion, wouldn't it?

threeway-callback is called from the unpack-trees codepath and
tree-walk.c::traverse_trees() is what prepared these entries.

An entry[] which is an N-element array represents the traversal
state of walking N trees in parallel, and the callback function like
this one gets an entry that has been tree-walk.c::entry_clear()ed
(i.e. memset to full of '\0's) for a tree that lacks the corresponding
path.

Try to do merge-tree of two trees, where there is a path that we did
not change since common ancestor, while the other side removed it,
and you would hit this if () statement, I would think.
--
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 5/5] t5504: drop sigpipe=ok from push tests

[Jeff King]

These were added by 8bf4bec (add "ok=sigpipe" to
test_must_fail and use it to fix flaky tests, 2015-11-27)
because we would racily die via SIGPIPE when the pack was
rejected by the other side.

But since we have recently de-flaked send-pack, we should be
able to tighten up these tests (including re-adding the
expected output checks).

Signed-off-by: Jeff King 
---
Would be nice to reference HEAD^2 by name here, but of
course I don't know its final commit sha1 yet.

 t/t5504-fetch-receive-strict.sh | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index a3e12d2..44f3d5f 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -100,11 +100,8 @@ test_expect_success 'push with receive.fsckobjects' '
git config receive.fsckobjects true &&
git config transfer.fsckobjects false
) &&
-   test_must_fail ok=sigpipe git push --porcelain dst 
master:refs/heads/test >act &&
-   {
-   test_cmp exp act ||
-   ! test -s act
-   }
+   test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+   test_cmp exp act
 '
 
 test_expect_success 'push with transfer.fsckobjects' '
@@ -114,7 +111,8 @@ test_expect_success 'push with transfer.fsckobjects' '
cd dst &&
git config transfer.fsckobjects true
) &&
-   test_must_fail ok=sigpipe git push --porcelain dst 
master:refs/heads/test >act
+   test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+   test_cmp exp act
 '
 
 cat >bogus-commit <<\EOF
-- 
2.8.1.512.g4e0a533
--
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 4/5] fetch-pack: isolate sigpipe in demuxer thread

[Jeff King]

In commit 9ff18fa (fetch-pack: ignore SIGPIPE in sideband
demuxer, 2016-02-24), we started using sigchain_push() to
ignore SIGPIPE in the async demuxer thread. However, this is
rather clumsy, as it ignores SIGPIPE for the entire process,
including the main thread. At the time we didn't have any
per-thread signal support, but we now we do. Let's use it.

Signed-off-by: Jeff King 
---
 fetch-pack.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index f96f6df..b501d5c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -15,7 +15,6 @@
 #include "version.h"
 #include "prio-queue.h"
 #include "sha1-array.h"
-#include "sigchain.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -674,10 +673,8 @@ static int sideband_demux(int in, int out, void *data)
int *xd = data;
int ret;
 
-   sigchain_push(SIGPIPE, SIG_IGN);
ret = recv_sideband("fetch-pack", xd[0], out);
close(out);
-   sigchain_pop(SIGPIPE);
return ret;
 }
 
@@ -701,6 +698,7 @@ static int get_pack(struct fetch_pack_args *args,
demux.proc = sideband_demux;
demux.data = xd;
demux.out = -1;
+   demux.isolate_sigpipe = 1;
if (start_async())
die("fetch-pack: unable to fork off sideband"
" demultiplexer");
-- 
2.8.1.512.g4e0a533

--
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/5] send-pack: isolate sigpipe in demuxer thread

[Jeff King]

If we get an error from pack-objects, we may exit
send_pack() early, before reading the server's status
response. In such a case, we may racily see SIGPIPE from our
async demuxer (which is trying to write that status back to
us), and we'd prefer to continue pushing the error up the
call stack, rather than taking down the whole process with
signal death.

This is safe to do because our demuxer just calls
recv_sideband, whose data writes are all done with
write_or_die(), which will notice SIGPIPE.

We do also write sideband 2 to stderr, and we would no
longer die on SIGPIPE there (if it were piped in the first
place, and if the piped program went away). But that's
probably a good thing, as it likewise should not abort the
push process at all (neither immediately by signal, nor
eventually by reporting failure back to the main thread).

Signed-off-by: Jeff King 
---
 send-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/send-pack.c b/send-pack.c
index fc27db6..37ee04e 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -518,6 +518,7 @@ int send_pack(struct send_pack_args *args,
demux.proc = sideband_demux;
demux.data = fd;
demux.out = -1;
+   demux.isolate_sigpipe = 1;
if (start_async())
die("send-pack: unable to fork off sideband 
demultiplexer");
in = demux.out;
-- 
2.8.1.512.g4e0a533

--
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/5] run-command: teach async threads to ignore SIGPIPE

[Jeff King]

Async processes can be implemented as separate forked
processes, or as threads (depending on the NO_PTHREADS
setting). In the latter case, if an async thread gets
SIGPIPE, it takes down the whole process. This is obviously
bad if the main process was not otherwise going to die, but
even if we were going to die, it means the main process does
not have a chance to report a useful error message.

There's also the small matter that forked async processes
will not take the main process down on a signal, meaning git
will behave differently depending on the NO_PTHREADS
setting.

This patch fixes it by adding a new flag to "struct async"
to block SIGPIPE just in the async thread. In theory, this
should always be on (which makes async threads behave more
like async processes), but we would first want to make sure
that each async process we spawn is careful about checking
return codes from write() and would not spew endlessly into
a dead pipe. So let's start with it as optional, and we can
enable it for specific sites in future patches.

The natural name for this option would be "ignore_sigpipe",
since that's what it does for the threaded case. But since
that name might imply that we are ignoring it in all cases
(including the separate-process one), let's call it
"isolate_sigpipe". What we are really asking for is
isolation. I.e., not to have our main process taken down by
signals spawned by the async process. How that is
implemented is up to the run-command code.

Signed-off-by: Jeff King 
---
This is our first use of pthread_sigmask, and I think Windows will have
to come up with something for this in compat/. I don't know how SIGPIPE
works there at all, so it's possible that we can just turn this into a
noop. Worst case it could probably block SIGPIPE for the whole process.

Note also that by blocking, we may end up with a pending SIGPIPE signal
(so if were ever to unblock, it would get delivered). We can unqueue it
with sigwaitinfo(), but I _think_ it shouldn't matter. We never unblock,
but just kill the thread, and the pending signal should be killed with
it.

At least that's my understanding. This is the first time I've worked
with signals and threads.

 run-command.c | 10 ++
 run-command.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/run-command.c b/run-command.c
index 8c7115a..e4593cd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -590,6 +590,16 @@ static void *run_thread(void *data)
struct async *async = data;
intptr_t ret;
 
+   if (async->isolate_sigpipe) {
+   sigset_t mask;
+   sigemptyset();
+   sigaddset(, SIGPIPE);
+   if (pthread_sigmask(SIG_BLOCK, , NULL) < 0) {
+   ret = error("unable to block SIGPIPE in async thread");
+   return (void *)ret;
+   }
+   }
+
pthread_setspecific(async_key, async);
ret = async->proc(async->proc_in, async->proc_out, async->data);
return (void *)ret;
diff --git a/run-command.h b/run-command.h
index de1727e..11f76b0 100644
--- a/run-command.h
+++ b/run-command.h
@@ -116,6 +116,7 @@ struct async {
int proc_in;
int proc_out;
 #endif
+   int isolate_sigpipe;
 };
 
 int start_async(struct async *async);
-- 
2.8.1.512.g4e0a533

--
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/5] send-pack: close demux pipe before finishing async process

[Jeff King]

This fixes a deadlock on the client side when pushing a
large number of refs from a corrupted repo.  There's a
reproduction script below, but let's start with a
human-readable explanation.

The client side of a push goes something like this:

  1. Start an async process to demux sideband coming from
 the server.

  2. Run pack-objects to send the actual pack, and wait for
 its status via finish_command().

  3. If pack-objects failed, abort immediately.

  4. If pack-objects succeeded, read the per-ref status from
 the server, which is actually coming over a pipe from
 the demux process started in step 1.

We run finish_async() to wait for and clean up the demux
process in two places. In step 3, if we see an error, we
want it to end early. And after step 4, it should be done
writing any data and we are just cleaning it up.

Let's focus on the error case first. We hand the output
descriptor to the server over to pack-objects. So by the
time it has returned an error to us, it has closed the
descriptor and the server has gotten EOF. The server will
mark all refs as failed with "unpacker error" and send us
back the status for each (followed by EOF).

This status goes to the demuxer thread, which relays it over
a pipe to the main thread. But the main thread never even
tries reading the status. It's trying to bail because of the
pack-objects error, and is waiting for the demuxer thread to
finish. If there are a small number of refs, that's OK; the
demuxer thread writes into the pipe buffer, sees EOF from
the server, and quits. But if there are a large number of
refs, it may block on write() back to the main thread,
leading to a deadlock (the main thread is waiting for the
demuxer to finish, the demuxer is waiting for the main
thread to read).

We can break this deadlock by closing the pipe between the
demuxer and the main thread before calling finish_async().
Then the demuxer gets a write() error and exits.

The non-error case usually just works, because we will have
read all of the data from the other side. We do close
demux.out already, but we only do so _after_ calling
finish_async(). This is OK because there shouldn't be any
more data coming from the server. But technically we've only
read to a flush packet, and a broken or malicious server
could be sending more cruft. In such a case, we would hit
the same deadlock. Closing the pipe first doesn't affect the
normal case, and means that for a cruft-sending server,
we'll notice a write() error rather than deadlocking.

Note that when write() sees this error, we'll actually
deliver SIGPIPE to the thread, which will take down the
whole process (unless we're compiled with NO_PTHREADS). This
isn't ideal, but it's an improvement over the status quo,
which is deadlocking. And SIGPIPE handling in async threads
is a bigger problem that we can deal with separately.

A simple reproduction for the error case is below. It's
technically racy (we could exit the main process and take
down the async thread with us before it even reads the
status), though in practice it seems to fail pretty
consistently.

git init repo &&
cd repo &&

# make some commits; we need two so we can simulate corruption
# in the history later.
git commit --allow-empty -m one &&
one=$(git rev-parse HEAD) &&
git commit --allow-empty -m two &&
two=$(git rev-parse HEAD) &&

# now make a ton of refs; our goal here is to overflow the pipe buffer
# when reporting the ref status, which will cause the demuxer to block
# on write()
for i in $(seq 2); do
echo "create refs/heads/this-is-a-really-long-branch-name-$i $two"
done |
git update-ref --stdin &&

# now make a corruption in the history such that pack-objects will fail
rm -vf .git/objects/$(echo $one | sed 's}..}&/}') &&

# and then push the result
git init --bare dst.git &&
git push --mirror dst.git

Signed-off-by: Jeff King 
---
Even though the SIGPIPE isn't great, it's certainly better
than a deadlock.  So if we had to take just this one for
"maint" (e.g., because the SIGPIPE stuff causes problems on
Windows), I think it's still worth doing (and is why I stuck
this patch at the front of the series).

One alternative would be to add async_cancel() which would
run pthread_cancel() for the threaded case, and kill() for
the non-threaded. That helps the error case, but makes the
non-error case a bit weird (if the thread _isn't_ blocking
on write to us, we want to get the real status code, rather
than canceling it. But we don't have any way of knowing
which of _cancel() or finish() to call).

 send-pack.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/send-pack.c b/send-pack.c
index 047bd18..fc27db6 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -531,8 +531,10 @@ int send_pack(struct send_pack_args *args,
close(out);
if (git_connection_is_socket(conn))
   

[PATCH 0/5] fix deadlock in git-push

[Jeff King]

I ran across a deadlock today while pushing from a corrupted repository
where pack-objects fails. Obviously I don't expect this to succeed, but
it should not hang indefinitely.

The first patch below fixes the deadlock. Unfortunately, it turns it
into a likely SIGPIPE death. Which is an improvement, but not ideal.

Patches 2 and 3 address that by fixing the way we handle SIGPIPE in
async threads.

Patches 4 and 5 are cleanups to earlier topics that are enabled by the
new SIGPIPE handling.

  [1/5]: send-pack: close demux pipe before finishing async process
  [2/5]: run-command: teach async threads to ignore SIGPIPE
  [3/5]: send-pack: isolate sigpipe in demuxer thread
  [4/5]: fetch-pack: isolate sigpipe in demuxer thread
  [5/5]: t5504: drop sigpipe=ok from push tests

I confirmed that after this series we still pass the stress test I
outlined in

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

So hopefully I haven't made anything worse (and that the tightening in
5/5 isn't just bringing back some test flakiness).

-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 v4 15/16] index-helper: optionally automatically run

[David Turner]

On Sun, 2016-04-17 at 12:19 +0700, Duy Nguyen wrote:
> On Wed, Apr 13, 2016 at 7:33 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > @@ -536,8 +567,10 @@ static void handle_builtin(int argc, const
> > char **argv)
> > }
> > 
> > builtin = get_builtin(cmd);
> > -   if (builtin)
> > +   if (builtin) {
> > +   maybe_run_index_helper(builtin);
> > exit(run_builtin(builtin, argc, argv));
> > +   }
> >  }
> 
> Isn't it too early to start index-helper here? Unrelated commands
> like
> git-log are affected. And config handling this early could be tricky.
> A better place may be in the socket connection code. When we fail to
> connect, we can check config key and run index-helper then.

Will move.
--
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/4] http.h: Add debug callback and helper routine for implementing the GIT_TRACE_CURL environment variable in http.c

[Ramsay Jones]

On 19/04/16 22:48, Ramsay Jones wrote:
> 
[snip]

> I think the minimal fixup (including Junio's comment on patch #2, which also
> triggered for me) is given in the patch below.

BTW, if you want to have a single static instance of the 'struct trace_key',
then the following patch on top should work. (Also, I have compiled and run
the testsuite on these patches, but _not_ tested that the trace functionality
actually works!) ;-)

HTH

ATB,
Ramsay Jones
-- >8 --
Subject: [PATCH] curl-trace: reduce visibility of the trace key struct

Signed-off-by: Ramsay Jones 
---
 http.c  | 9 +++--
 http.h  | 2 +-
 imap-send.c | 2 +-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index ce91421..eed6dba 100644
--- a/http.c
+++ b/http.c
@@ -11,7 +11,7 @@
 #include "gettext.h"
 #include "transport.h"
 
-struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
+static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 #if LIBCURL_VERSION_NUM >= 0x070a08
 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
 #else
@@ -516,6 +516,11 @@ static void curl_dump(const char *text, unsigned char 
*ptr, size_t size, char no
}
 }
 
+int curl_trace_want(void)
+{
+   return trace_want(_curl);
+}
+
 int curl_trace(CURL *handle, curl_infotype type,
 char *data, size_t size, void *userp)
 {
@@ -652,7 +657,7 @@ static CURL *get_curl_handle(void)
"your curl version is too old (>= 7.19.4)");
 #endif
 
-   if (trace_want(_curl)) {
+   if (curl_trace_want()) {
curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);
diff --git a/http.h b/http.h
index 00e4ad7..153ff17 100644
--- a/http.h
+++ b/http.h
@@ -225,7 +225,7 @@ extern void abort_http_object_request(struct 
http_object_request *freq);
 extern void release_http_object_request(struct http_object_request *freq);
 
 /* Debug callback and helper routine for curl_easy_setopt 
CURLOPT_DEBUGFUNCTION */
-extern struct trace_key trace_curl;
+int curl_trace_want(void);
 int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void 
*userp);
 
 
diff --git a/imap-send.c b/imap-send.c
index 36ff843..627ffa4 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1444,7 +1444,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
 
-   if (trace_want(_curl)) {
+   if (curl_trace_want()) {
curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
curl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);
-- 
2.8.0

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

Re: [PATCH v4 12/16] index-helper: kill mode

[David Turner]

On Sat, 2016-04-16 at 18:08 +0200, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Apr 13, 2016 at 2:33 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > Add a new command (and command-line arg) to allow index-helpers to
> > exit cleanly.
> > 
> > This is mainly useful for tests.
> 
> Both --kill and --autorun are missing documentation in
> Documentation/git-index-helper.txt.

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

Re: [PATCH 1/4] http.h: Add debug callback and helper routine for implementing the GIT_TRACE_CURL environment variable in http.c

[Ramsay Jones]

On 19/04/16 16:10, Elia Pinto wrote:
> Add the debug callback and helper routine prototype used by
> curl_easy_setopt CURLOPT_DEBUGFUNCTION in http.c
> for implementing the GIT_TRACE_CURL environment variable
> 
> 
> Helped-by: Torsten Bögershausen 
> Helped-by: Ramsay Jones 
> Helped-by: Junio C Hamano 
> Helped-by: Eric Sunshine  
> Helped-by: Jeff King 
> Signed-off-by: Elia Pinto 
> ---
>  http.h | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/http.h b/http.h
> index 4ef4bbd..a2d10bc 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,4 +224,10 @@ extern int finish_http_object_request(struct 
> http_object_request *freq);
>  extern void abort_http_object_request(struct http_object_request *freq);
>  extern void release_http_object_request(struct http_object_request *freq);
>  
> +/* Debug callback and helper routine for curl_easy_setopt 
> CURLOPT_DEBUGFUNCTION */
> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);

Ah no, this would add 6 instances of the 'trace_curl' key in http-fetch.c,
http-push.c, http-walker.c, http.c, imap-send.c and remote-curl.c. Hmm ...
since these would end up in different executables (by and large) it might
work OK, ... but is simply not necessary.

Also, patches #1 and #2 should be squashed into one patch and, since the
curl_dump() function is only called from http.c, it can be a static symbol.

I think the minimal fixup (including Junio's comment on patch #2, which also
triggered for me) is given in the patch below.

Hope that helps.

ATB,
Ramsay Jones

> +int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, 
> void *userp);
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char 
> nohex);
> +
> +
>  #endif /* HTTP_H */

-- >8 --
Subject: [PATCH] curl-trace: fix scope/visibility of various symbols

Signed-off-by: Ramsay Jones 
---
 http.c | 9 +++--
 http.h | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/http.c b/http.c
index 64dd975..ce91421 100644
--- a/http.c
+++ b/http.c
@@ -11,9 +11,7 @@
 #include "gettext.h"
 #include "transport.h"
 
-/*
-tatic struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
-*/
+struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 #if LIBCURL_VERSION_NUM >= 0x070a08
 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
 #else
@@ -468,12 +466,11 @@ static void set_curl_keepalive(CURL *c)
 #endif
 
 
-void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
+static void curl_dump(const char *text, unsigned char *ptr, size_t size, char 
nohex)
 {
size_t i;
size_t w;
-   struct strbuf out = STRBUF_INIT;;
-
+   struct strbuf out = STRBUF_INIT;
unsigned int width = 0x10;
 
if (nohex)
diff --git a/http.h b/http.h
index a2d10bc..00e4ad7 100644
--- a/http.h
+++ b/http.h
@@ -225,9 +225,8 @@ extern void abort_http_object_request(struct 
http_object_request *freq);
 extern void release_http_object_request(struct http_object_request *freq);
 
 /* Debug callback and helper routine for curl_easy_setopt 
CURLOPT_DEBUGFUNCTION */
-static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
+extern struct trace_key trace_curl;
 int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void 
*userp);
-void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex);
 
 
 #endif /* HTTP_H */
-- 
2.8.0

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

Re: [PATCH v4 03/16] index-helper: new daemon for caching index and related stuff

[David Turner]

On Fri, 2016-04-15 at 17:04 -0700, Stefan Beller wrote:
> > +static int try_shm(struct index_state *istate)
> > +{
> > +   void *new_mmap = NULL;
> > +   size_t old_size = istate->mmap_size;
> > +   ssize_t new_size;
> > +   const unsigned char *sha1;
> > +   struct stat st;
> > +   int fd;
> > +
> > +   if (!is_main_index(istate) ||
> > +   old_size <= 20 ||
> > +   stat(git_path("index-helper.path"), ))
> > +   return -1;
> > +   if (poke_daemon(istate, , 0))
> > +   return -1;
> > +   sha1 = (unsigned char *)istate->mmap + old_size - 20;
> > +
> > +   fd = open(index_helper_path("git-index-%s",
> > sha1_to_hex(sha1)),
> > + O_RDONLY);
> > +   if (fd < 0)
> > +   goto fail;
> > +
> > +   if (fstat(fd, ))
> > +   goto fail;
> > +
> > +   new_size = st.st_size;
> > +   new_mmap = mmap(NULL, new_size, PROT_READ, MAP_SHARED, fd,
> > 0);
> > +   if (new_size <= 20 ||
> > +   hashcmp((unsigned char *)istate->mmap + old_size - 20,
> > +   (unsigned char *)new_mmap + new_size - 20)) {
> > +   if (new_mmap)
> > +   munmap(new_mmap, new_size);
> > +   goto fail;
> 
> coming from here
> 
> > +   }
> > +
> > +   /* The memory barrier here matches index
> > -helper.c:share_index. */
> > +   __sync_synchronize();
> > +
> > +   munmap(istate->mmap, istate->mmap_size);
> > +   istate->mmap = new_mmap;
> > +   istate->mmap_size = new_size;
> > +   istate->from_shm = 1;
> > +   return 0;
> > +
> > +fail:
> 
> fd may be leaking here?
> 
> > +   poke_daemon(istate, , 1);
> > +   return -1;
> > +}
> > +


Good point.  (It's also leaking on the happy path -- will fix all of
those)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 03/16] index-helper: new daemon for caching index and related stuff

[David Turner]

On Fri, 2016-04-15 at 17:04 -0700, Stefan Beller wrote:
> > +static int try_shm(struct index_state *istate)
> > +{
> > +   void *new_mmap = NULL;
> > +   size_t old_size = istate->mmap_size;
> > +   ssize_t new_size;
> > +   const unsigned char *sha1;
> > +   struct stat st;
> > +   int fd;
> > +
> > +   if (!is_main_index(istate) ||
> > +   old_size <= 20 ||
> > +   stat(git_path("index-helper.path"), ))
> > +   return -1;
> > +   if (poke_daemon(istate, , 0))
> > +   return -1;
> > +   sha1 = (unsigned char *)istate->mmap + old_size - 20;
> > +
> > +   fd = open(index_helper_path("git-index-%s",
> > sha1_to_hex(sha1)),
> > + O_RDONLY);
> > +   if (fd < 0)
> > +   goto fail;
> > +
> > +   if (fstat(fd, ))
> > +   goto fail;
> > +
> > +   new_size = st.st_size;
> > +   new_mmap = mmap(NULL, new_size, PROT_READ, MAP_SHARED, fd,
> > 0);
> > +   if (new_size <= 20 ||
> > +   hashcmp((unsigned char *)istate->mmap + old_size - 20,
> > +   (unsigned char *)new_mmap + new_size - 20)) {
> > +   if (new_mmap)
> > +   munmap(new_mmap, new_size);
> > +   goto fail;
> 
> coming from here
> 
> > +   }
> > +
> > +   /* The memory barrier here matches index
> > -helper.c:share_index. */
> > +   __sync_synchronize();
> > +
> > +   munmap(istate->mmap, istate->mmap_size);
> > +   istate->mmap = new_mmap;
> > +   istate->mmap_size = new_size;
> > +   istate->from_shm = 1;
> > +   return 0;
> > +
> > +fail:
> 
> fd may be leaking here?
> 
> > +   poke_daemon(istate, , 1);
> > +   return -1;
> > +}
> > +


Good point.  (It's also leaking on the happy path -- will fix all of
those)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Jan Durovec]

> By the way, you may or may not have noticed that I've been
> reordering the lines of your message quoted in my responses; around
> here, top-posting is frowned upon.

I haven't noticed. Thanks for pointing out.

As for the submitGit cover letter I wanted to raise at least an issue
(if not create a fix itself) but it seems to be raised already as
https://github.com/rtyley/submitgit/issues/9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Tue, 2016-04-19 at 03:14 -0400, Jeff King wrote:
> On Mon, Apr 18, 2016 at 11:45:54AM -0700, Junio C Hamano wrote:
> 
> > David Turner  writes:
> > 
> > > Add parameters for a list of refspecs to
> > > transport_get_remote_refs and
> > > get_refs_list.  These parameters are presently unused -- soon, we
> > > will
> > > use them to implement fetches which only learn about a subset of
> > > refs.
> > > 
> > > Signed-off-by: David Turner 
> > > ---
> > 
> > What the code tries to do I am more than halfway happy.  It is
> > unfortunate that we cannot do this natively without upgrading the
> > protocol in a fundamental way, but this is a nice way to work it
> > around only for Git-over-HTTP transport without having to break the
> > protocol.
> 
> I dunno, I am a bit negative on bringing new features to Git-over
> -HTTP
> (which is already less efficient than the other protocols!) without
> any
> plan for supporting them in the other protocols.

Interesting -- can you expand on git-over-http being less efficient?
This is the first I'd heard of it.  Is it documented somewhere?

> I thought Stefan's v2 protocol work looked quite good, but it seems
> to
> have stalled. The hardest part of that topic is figuring out the
> upgrade
> path. But for git-over-http, we can solve that in the same way that
> David is passing in the extra refspecs.
> 
> So I'd rather see something like:
> 
>   1. Support for v2 "capabilities only" initial negotiation, followed
>  by ref advertisement.
> 
>   2. Support for refspec-limiting capability.
> 
>   3. HTTP-only option from client to trigger v2 on the server.
> 
> That's still HTTP-specific, but it has a clear path for converging
> with
> the ssh and git protocols eventually, rather than having to support
> magic out-of-band capabilities forever.
> 
> It does require an extra round of HTTP request/response, though.

This seems way more complicated to me, and not necessarily super
-efficient.  That is, it seems like rather a lot of work to add a whole
round of negotiation and a new protocol, when all we really need is one
little tweak.

I do think a fetch v2 protocol is potentially interesting for other
reasons, but I don't know that I have the time to fully implement it.  

I wonder if it would be possible to just add these tweaks to v1, and
save the v2 work for when someone has the time to implement it?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Junio C Hamano]

Jan Durovec  writes:

> On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano  wrote:
>
>> For a series this small it does not matter, but anything longer it
>> would be easier to review with a cover letter (i.e. [PATCH 0/N]).  I
>> do not know if submitGit lets us do that, though.
>
> There's a comment on PR itself (in addition to individual commits) so
> theoretically it could.
>
> It seems that for [PATCH ... n/m] e-mails the commit messages are used,
> so there's no reason why the PR comment couldn't be used for a cover
> letter.
>
> In this case the PR comment was the same as for one of the commits.
> Maybe submitGit tried to be smart there? Or maybe it just really
> doesn't support it (yet?).

In any case, I've queued the updated ones as they looked good.
Thanks.

By the way, you may or may not have noticed that I've been
reordering the lines of your message quoted in my responses; around
here, top-posting is frowned upon.


--
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 v5 3/4] t0027: test cases for combined attributes

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> Add more test cases for the not normalized files ("NNO").  The
> "text" attribute is most important, use it as the first parameter.
> "ident", if set, is the second paramater followed by the eol
> attribute.  The eol attribute overrides core.autocrlf, which
> overrides core.eol.
> indent is not yet uses, this will be done in the next commit.
>
> Use loops to test more combinations of attributes, like
> "* text eol=crlf" or especially "*text=auto eol=crlf".
>
> Currently "* text=auto eol=lf" does the same as "* text eol=lf",
> as the eol attribute overrides "text=auto", this will change in
> future.

Will change in future in what way?  In patch 5/4?

>
> Signed-off-by: Torsten Bögershausen 
> ---
>  t/t0027-auto-crlf.sh | 298 
> ++-
>  1 file changed, 129 insertions(+), 169 deletions(-)
>
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> index 9fe539b..fd5e326 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -52,14 +52,17 @@ create_gitattributes () {
>  create_NNO_files () {
>   for crlf in false true input
>   do
> - for attr in "" auto text -text lf crlf
> + for attr in "" auto text -text
>   do
> - pfx=NNO_${crlf}_attr_${attr} &&
> - cp CRLF_mix_LF ${pfx}_LF.txt &&
> - cp CRLF_mix_LF ${pfx}_CRLF.txt &&
> - cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
> - cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
> - cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
> + for aeol in "" lf crlf
> + do
> + pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
> + cp CRLF_mix_LF ${pfx}_LF.txt &&
> + cp CRLF_mix_LF ${pfx}_CRLF.txt &&
> + cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
> + cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
> + cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
> + done
>   done
>   done
>  }
> @@ -100,16 +103,17 @@ commit_check_warn () {
>  }
>  
>  commit_chk_wrnNNO () {
> - crlf=$1
> - attr=$2
> - lfwarn=$3
> - crlfwarn=$4
> - lfmixcrlf=$5
> - lfmixcr=$6
> - crlfnul=$7
> - pfx=NNO_${crlf}_attr_${attr}
> + attr=$1 ; shift
> + aeol=$1 ; shift
> + crlf=$1 ; shift
> + lfwarn=$1 ; shift
> + crlfwarn=$1 ; shift
> + lfmixcrlf=$1 ; shift
> + lfmixcr=$1 ; shift
> + crlfnul=$1 ; shift
> + pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
>   #Commit files on top of existing file
> - create_gitattributes "$attr" &&
> + create_gitattributes "$attr" $aeol &&
>   for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
>   do
>   fname=${pfx}_$f.txt &&
> @@ -122,19 +126,19 @@ commit_chk_wrnNNO () {
>   test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
>   check_warning "$lfwarn" ${pfx}_LF.err
>   '
> - test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF" '
> + test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
> CRLF" '
>   check_warning "$crlfwarn" ${pfx}_CRLF.err
>   '
>  
> - test_expect_success "commit NNO files crlf=$crlf attr=$attr 
> CRLF_mix_LF" '
> + test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
> CRLF_mix_LF" '
>   check_warning "$lfmixcrlf" ${pfx}_CRLF_mix_LF.err
>   '
>  
> - test_expect_success "commit NNO files crlf=$crlf attr=$attr LF_mix_cr" '
> + test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
> LF_mix_cr" '
>   check_warning "$lfmixcr" ${pfx}_LF_mix_CR.err
>   '
>  
> - test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF_nul" '
> + test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
> CRLF_nul" '
>   check_warning "$crlfnul" ${pfx}_CRLF_nul.err
>   '
>  }
> @@ -163,6 +167,7 @@ stats_ascii () {
>  
>  # contruct the attr/ returned by git ls-files --eol
>  # Take none (=empty), one or two args
> +# convert.c: eol=XX overrides text=auto
>  attr_ascii () {
>   case $1,$2 in
>   -text,*)   echo "-text" ;;
> @@ -170,8 +175,8 @@ attr_ascii () {
>   text,lf)   echo "text eol=lf" ;;
>   text,crlf) echo "text eol=crlf" ;;
>   auto,) echo "text=auto" ;;
> - auto,lf)   echo "text=auto eol=lf" ;;
> - auto,crlf) echo "text=auto eol=crlf" ;;
> + auto,lf)   echo "text eol=lf" ;;
> + auto,crlf) echo "text eol=crlf" ;;
>   lf,)   echo "text eol=lf" ;;
>   crlf,) echo "text eol=crlf" ;;
>   ,) echo "" ;;
> @@ -196,28 +201,29 @@ check_files_in_repo () {
>  }
>  
>  check_in_repo_NNO () {
> - crlf=$1
> - attr=$2
> - 

Re: [PATCH/RFC 5/6] fetch: pass refspec to http server

[David Turner]

On Sat, 2016-04-16 at 22:33 -0400, Eric Sunshine wrote:
> On Fri, Apr 15, 2016 at 3:19 PM, David Turner <
> dtur...@twopensource.com> wrote:
> > When fetching over http, send the requested refspec to the server.
> > The server will then only send refs matching that refspec.  It is
> > permitted for old servers to ignore that parameter, and the client
> > will automatically handle this.
> > 
> > When the server has many refs, and the client only wants a few,
> > this
> > can save bandwidth and reduce fetch latency.
> > 
> > Signed-off-by: David Turner 
> > ---
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > @@ -302,9 +302,27 @@ static struct ref *get_ref_map(struct
> > transport *transport,
> > -   remote_refs = transport_get_remote_refs(transport, NULL,
> > 0);
> > +   qualified_refspecs = xcalloc(refspec_count,
> > sizeof(*qualified_refspecs));
> > +   for (i = 0; i < refspec_count; i++) {
> > +   if (starts_with(refspecs[i].src, "refs/")) {
> > +   qualified_refspecs[i].src =
> > xstrdup(refspecs[i].src);
> > +   } else {
> > +   struct strbuf buf = STRBUF_INIT;
> > +   strbuf_addf(, "refs/heads/%s",
> > refspecs[i].src);
> > +   qualified_refspecs[i].src =
> > strbuf_detach(, NULL);
> 
> Alternately, replace these three lines with:
> 
> qualified_refspecs[i].src = xstrfmt("refs/heads/%s",
> refspecs[i].src);
> 
> and drop the braces.

Yep, good point.

> > +   }
> > +   }
> > +
> > +   remote_refs = transport_get_remote_refs(transport,
> > qualified_refspecs,
> > +   refspec_count);
> > +
> > +   for (i = 0; i < refspec_count; i++) {
> > +   free(qualified_refspecs[i].src);
> > +   }
> > +   free(qualified_refspecs);
> > diff --git a/t/t5552-http-fetch-branch.sh b/t/t5552-http-fetch
> > -branch.sh
> > @@ -0,0 +1,42 @@
> > +test_expect_success 'make some more commits' '
> > +   (
> > +   cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > +   test_commit 2 &&
> > +   git checkout -b another_branch &&
> > +   test_commit 3
> 
> Broken &&-chain.

Thanks.

> > +   git checkout -b a_third_branch &&
> > +   test_commit 4
> > +   )
> > +'
> > +
> > +test_expect_success 'fetch with refspec only fetches requested
> > branch' '
> > +   test_when_finished "rm trace" &&
> > +   (
> > +   cd clone &&
> > +   GIT_TRACE_PACKET="$TRASH_DIRECTORY/trace" git fetch
> > origin another_branch &&
> > +   ! grep "refs/heads/master" ../trace
> > +   )
> > +'
> 
> This could be done without the subshell, perhaps?
> 
> GIT_TRACE_PACKET=blah git -C clone fetch ...


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

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Jan Durovec]

There's a comment on PR itself (in addition to individual commits) so
theoretically it could.

It seems that for [PATCH ... n/m] e-mails the commit messages are used,
so there's no reason why the PR comment couldn't be used for a cover
letter.

In this case the PR comment was the same as for one of the commits.
Maybe submitGit tried to be smart there? Or maybe it just really
doesn't support it (yet?).

On Tue, Apr 19, 2016 at 11:09 PM, Junio C Hamano  wrote:
> Jan Durovec  writes:
>
>> On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec  wrote:
 Any submitGit users?  I think it lets you throw multiple-patch
 series just fine.  In this case, you'd prepare a two patch series on
 a branch, 1/2 being the clean-up and 2/2 being the new feature, and
 if you give that branch to submitGit as a whole it should do the
 right thing, I'd imagine.
>>>
>>> Hm... I'll see what it does with a pull request that has 2 commits.
>>
>> Huh... seems that it works :)
>>
>> v3 sent in 2 parts
>
> Thanks.
>
> For a series this small it does not matter, but anything longer it
> would be easier to review with a cover letter (i.e. [PATCH 0/N]).  I
> do not know if submitGit lets us do that, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 6/6] clone: send refspec for single-branch clones

[David Turner]

On Sat, 2016-04-16 at 22:36 -0400, Eric Sunshine wrote:
> On Fri, Apr 15, 2016 at 3:19 PM, David Turner <
> dtur...@twopensource.com> wrote:
> > For single-branch clones (when we know in advance what the remote
> > branch name will be), send a refspec so that the server doesn't
> > tell us about any other refs.
> > 
> > Signed-off-by: David Turner 
> > ---
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > @@ -1010,7 +1010,21 @@ int cmd_clone(int argc, const char **argv,
> > const char *prefix)
> > +   if (option_single_branch && option_branch) {
> > +   struct refspec branch_refspec = {0};
> > +
> > +   if (starts_with(option_branch, "refs/")) {
> > +   branch_refspec.src =
> > xstrdup(option_branch);
> > +   } else {
> > +   struct strbuf buf = STRBUF_INIT;
> > +   strbuf_addf(, "refs/heads/%s",
> > option_branch);
> > +   branch_refspec.src = strbuf_detach(,
> > NULL);
> 
> branch_refspec.src = xstrfmt("refs/heads/%s", option_branch);


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

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Junio C Hamano]

Jan Durovec  writes:

> On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec  wrote:
>>> Any submitGit users?  I think it lets you throw multiple-patch
>>> series just fine.  In this case, you'd prepare a two patch series on
>>> a branch, 1/2 being the clean-up and 2/2 being the new feature, and
>>> if you give that branch to submitGit as a whole it should do the
>>> right thing, I'd imagine.
>>
>> Hm... I'll see what it does with a pull request that has 2 commits.
>
> Huh... seems that it works :)
>
> v3 sent in 2 parts

Thanks.

For a series this small it does not matter, but anything longer it
would be easier to review with a cover letter (i.e. [PATCH 0/N]).  I
do not know if submitGit lets us do that, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/2] git-p4: fix Git LFS pointer parsing

[Junio C Hamano]

Lars Schneider  writes:

>> On 19 Apr 2016, at 22:30, Junio C Hamano  wrote:
>> 
>> larsxschnei...@gmail.com writes:
>> 
>>> From: Lars Schneider 
>>> 
>>> Git LFS 1.2.0 removed a line from the output of the 'git lfs pointer'
>>> command [1] which broke the parsing of this output. Adjust the parser
>>> to the new output and add minimum Git LFS version to the docs.
>> 
>> Hmph, adjust to operate with both, or drop the support for the old
>> one?

> I dropped the support for the older version to keep the code as
> simple as possible (plus it would be cumbersome to test with an
> outdated Git LFS version). Since it is probably a niche feature I
> thought that might be acceptable.

It is bad enough that clients need to be adjusted at all in the
first place, but I would have found it very troubling if the
problematic change to LFS thing were made in such a way that it
makes backward compatible adjustment on the client code impossible.

But it seems that you could read their output and strip a line that
begins with a known substring to make it compatible with both?

"git P4" itself may be niche and using it with the LFS thing may
even be more so, but in Git land, traditionally we take the backward
compatibility seriously.  If it is not too much work, I'd prefer to
see this done the right way.

Thanks.

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

Re: [PATCH v1 2/2] git-p4: fix Git LFS pointer parsing

[Lars Schneider]

> On 19 Apr 2016, at 22:30, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> Git LFS 1.2.0 removed a line from the output of the 'git lfs pointer'
>> command [1] which broke the parsing of this output. Adjust the parser
>> to the new output and add minimum Git LFS version to the docs.
> 
> Hmph, adjust to operate with both, or drop the support for the old
> one?
I dropped the support for the older version to keep the code as simple as 
possible (plus it would be cumbersome to test with an outdated Git LFS 
version). Since it is probably a niche feature I thought that might be 
acceptable.

Thanks,
Lars

>> 
>> [1] https://github.com/github/git-lfs/pull/1105
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> Documentation/git-p4.txt | 3 ++-
>> git-p4.py| 6 +++---
>> 2 files changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
>> index 88ba42b..b862cb9 100644
>> --- a/Documentation/git-p4.txt
>> +++ b/Documentation/git-p4.txt
>> @@ -522,7 +522,8 @@ git-p4.largeFileSystem::
>>that large file systems do not support the 'git p4 submit' command.
>>Only Git LFS is implemented right now (see https://git-lfs.github.com/
>>for more information). Download and install the Git LFS command line
>> -extension to use this option and configure it like this:
>> +extension (minimum version 1.2.0) to use this option and configure it
>> +like this:
>> +
>> -
>> git config   git-p4.largeFileSystem GitLFS
>> diff --git a/git-p4.py b/git-p4.py
>> index 527d44b..d2be574 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1064,8 +1064,8 @@ class GitLFS(LargeFileSystem):
>> if pointerProcess.wait():
>> os.remove(contentFile)
>> die('git-lfs pointer command failed. Did you install the 
>> extension?')
>> -pointerContents = [i+'\n' for i in pointerFile.split('\n')[2:][:-1]]
>> -oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
>> +oidEntry = [i for i in pointerFile.split('\n') if 
>> i.startswith('oid')]
>> +oid = oidEntry[0].split(' ')[1].split(':')[1]
>> localLargeFile = os.path.join(
>> os.getcwd(),
>> '.git', 'lfs', 'objects', oid[:2], oid[2:4],
>> @@ -1073,7 +1073,7 @@ class GitLFS(LargeFileSystem):
>> )
>> # LFS Spec states that pointer files should not have the executable 
>> bit set.
>> gitMode = '100644'
>> -return (gitMode, pointerContents, localLargeFile)
>> +return (gitMode, pointerFile, localLargeFile)
>> 
>> def pushFile(self, localLargeFile):
>> uploadProcess = subprocess.Popen(
>> --
>> 2.5.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 0/7] fix checking out a being-rebased branch

[Junio C Hamano]

Duy Nguyen  writes:

> On Tue, Apr 19, 2016 at 12:42 AM, Junio C Hamano  wrote:
>>> Another option is leave wt_status_get_state() alone, factor out the
>>> rebase-detection code and use that for worktree/checkout. We save a
>>> few syscalls this way too.
>>>
>>> Comments?
>>>
>>>   [01/07] path.c: add git_common_path() and strbuf_git_common_path()
>>>   [02/07] worktree.c: store "id" instead of "git_dir"
>>>   [03/07] path.c: refactor and add worktree_git_path()
>>>   [04/07] worktree.c: add worktree_git_path_..._head()
>>
>> I actually wondered how ky/branch-[dm]-worktree topics to avoid
>> "branch -d" and "branch -m" from removing or renaming a branch that
>> is checked out in other worktrees from screwing them up.  There may
>> be a missed opportunity to clean them up with using these?
>
> branch-m-worktree uses info populated at get_worktrees() phase.
> branch-d-worktree uses find_shared_symref() which is modified in this
> series in order to detect rebase branch. So both topics have the same
> problem when a branch is being rebased and if I do it right, I'll fix
> them both without extra code.

Yup, that was exactly why I brought it up.

> Actually I may need to check branch-m-worktree again, renaming a
> branch under rebase might cause problems, I need to have a closer
> look at git-rebase*.sh.

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

Re: [PATCH v1 2/2] git-p4: fix Git LFS pointer parsing

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> Git LFS 1.2.0 removed a line from the output of the 'git lfs pointer'
> command [1] which broke the parsing of this output. Adjust the parser
> to the new output and add minimum Git LFS version to the docs.

Hmph, adjust to operate with both, or drop the support for the old
one?



>
> [1] https://github.com/github/git-lfs/pull/1105
>
> Signed-off-by: Lars Schneider 
> ---
>  Documentation/git-p4.txt | 3 ++-
>  git-p4.py| 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> index 88ba42b..b862cb9 100644
> --- a/Documentation/git-p4.txt
> +++ b/Documentation/git-p4.txt
> @@ -522,7 +522,8 @@ git-p4.largeFileSystem::
>   that large file systems do not support the 'git p4 submit' command.
>   Only Git LFS is implemented right now (see https://git-lfs.github.com/
>   for more information). Download and install the Git LFS command line
> - extension to use this option and configure it like this:
> + extension (minimum version 1.2.0) to use this option and configure it
> + like this:
>  +
>  -
>  git config   git-p4.largeFileSystem GitLFS
> diff --git a/git-p4.py b/git-p4.py
> index 527d44b..d2be574 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1064,8 +1064,8 @@ class GitLFS(LargeFileSystem):
>  if pointerProcess.wait():
>  os.remove(contentFile)
>  die('git-lfs pointer command failed. Did you install the 
> extension?')
> -pointerContents = [i+'\n' for i in pointerFile.split('\n')[2:][:-1]]
> -oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
> +oidEntry = [i for i in pointerFile.split('\n') if 
> i.startswith('oid')]
> +oid = oidEntry[0].split(' ')[1].split(':')[1]
>  localLargeFile = os.path.join(
>  os.getcwd(),
>  '.git', 'lfs', 'objects', oid[:2], oid[2:4],
> @@ -1073,7 +1073,7 @@ class GitLFS(LargeFileSystem):
>  )
>  # LFS Spec states that pointer files should not have the executable 
> bit set.
>  gitMode = '100644'
> -return (gitMode, pointerContents, localLargeFile)
> +return (gitMode, pointerFile, localLargeFile)
>
>  def pushFile(self, localLargeFile):
>  uploadProcess = subprocess.Popen(
> --
> 2.5.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 v2] git-p4: add P4 jobs to git commit message

[Jan Durovec]

Huh... seems that it works :)

v3 sent in 2 parts

On Tue, Apr 19, 2016 at 8:50 PM, Jan Durovec  wrote:
>> Any submitGit users?  I think it lets you throw multiple-patch
>> series just fine.  In this case, you'd prepare a two patch series on
>> a branch, 1/2 being the clean-up and 2/2 being the new feature, and
>> if you give that branch to submitGit as a whole it should do the
>> right thing, I'd imagine.
>
> Hm... I'll see what it does with a pull request that has 2 commits.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] http.c: implements the GIT_TRACE_CURL environment variable

[Junio C Hamano]

Elia Pinto  writes:

> Implements the GIT_TRACE_CURL environment variable to allow a
> greater degree of detail of GIT_CURL_VERBOSE, in particular
> the complete transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis
>
>
> Helped-by: Torsten Bögershausen 
> Helped-by: Ramsay Jones 
> Helped-by: Junio C Hamano 
> Helped-by: Eric Sunshine  
> Helped-by: Jeff King 
> Signed-off-by: Elia Pinto 
> ---
>  http.c | 98 
> +-
>  1 file changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 4304b80..278991e 100644
> --- a/http.c
> +++ b/http.c
> @@ -11,6 +11,9 @@
>  #include "gettext.h"
>  #include "transport.h"
>  
> +/*
> +tatic struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
> +*/
>  #if LIBCURL_VERSION_NUM >= 0x070a08
>  long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
>  #else
> @@ -464,6 +467,95 @@ static void set_curl_keepalive(CURL *c)
>  }
>  #endif
>  
> +
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
> +{
> + size_t i;
> + size_t w;
> + struct strbuf out = STRBUF_INIT;;

;; makes the next line decl-after-stmt.

> +
> + unsigned int width = 0x10;




> +
> + if (nohex)
> + /* without the hex output, we can fit more on screen */
> + width = 0x40;
> +
> + strbuf_addf(, "%s, %10.10ld bytes (0x%8.8lx)\n",
> + text, (long)size, (long)size);
> +
> + for (i = 0; i < size; i += width) {
> +
> + strbuf_addf(, "%4.4lx: ", (long)i);
> +
> + if (!nohex) {
> + /* hex not disabled, show it */
> + for (w = 0; w < width; w++)
> + if (i + w < size)
> + strbuf_addf(, "%02x ", ptr[i + w]);
> + else
> + strbuf_addf(,"   ");
> + }
> +
> + for (w = 0; (w < width) && (i + w < size); w++) {
> + /* check for 0D0A; if found, skip past and start a new 
> line of output */
> + if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
> + && ptr[i + w + 1] == '\n') {
> + i += (w + 2 - width);
> + break;
> + }
> + strbuf_addch(, (ptr[i + w] >= 0x20)
> + && (ptr[i + w] < 0x80) ? ptr[i + w] : '.');
> + /* check again for 0D0A, to avoid an extra \n if it's 
> at width */
> + if (nohex && (i + w + 2 < size)
> + && ptr[i + w + 1] == '\r'
> + && ptr[i + w + 2] == '\n') {
> + i += (w + 3 - width);
> + break;
> + }
> + }
> + strbuf_addch(, '\n');
> + trace_strbuf(_curl, );
> + strbuf_release();
> + }
> +}
> +
> +int curl_trace(CURL *handle, curl_infotype type,
> +  char *data, size_t size, void *userp)
> +{
> + const char *text;
> + (void)handle;   /* prevent compiler warning */
> +
> + switch (type) {
> + case CURLINFO_TEXT:
> + trace_printf_key(_curl, "== Info: %s", data);
> + default:/* in case a new one is introduced to shock us 
> */
> + return 0;
> +
> + case CURLINFO_HEADER_OUT:
> + text = "=> Send header";
> + break;
> + case CURLINFO_DATA_OUT:
> + text = "=> Send data";
> + break;
> + case CURLINFO_SSL_DATA_OUT:
> + text = "=> Send SSL data";
> + break;
> + case CURLINFO_HEADER_IN:
> + text = "<= Recv header";
> + break;
> + case CURLINFO_DATA_IN:
> + text = "<= Recv data";
> + break;
> + case CURLINFO_SSL_DATA_IN:
> + text = "<= Recv SSL data";
> + break;
> + }
> +
> + curl_dump(text, (unsigned char *)data, size, 1);
> + return 0;
> +}
> +
> +
>  static CURL *get_curl_handle(void)
>  {
>   CURL *result = curl_easy_init();
> @@ -563,7 +655,11 @@ static CURL *get_curl_handle(void)
>   "your curl version is too old (>= 7.19.4)");
>  #endif
>  
> - if (getenv("GIT_CURL_VERBOSE"))
> + if (trace_want(_curl)) {
> + curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
> + curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
> + curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);
> + } else if (getenv("GIT_CURL_VERBOSE"))
>   curl_easy_setopt(result, 

[PATCH v1 2/2] git-p4: fix Git LFS pointer parsing

[larsxschneider]

From: Lars Schneider 

Git LFS 1.2.0 removed a line from the output of the 'git lfs pointer'
command [1] which broke the parsing of this output. Adjust the parser
to the new output and add minimum Git LFS version to the docs.

[1] https://github.com/github/git-lfs/pull/1105

Signed-off-by: Lars Schneider 
---
 Documentation/git-p4.txt | 3 ++-
 git-p4.py| 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 88ba42b..b862cb9 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -522,7 +522,8 @@ git-p4.largeFileSystem::
that large file systems do not support the 'git p4 submit' command.
Only Git LFS is implemented right now (see https://git-lfs.github.com/
for more information). Download and install the Git LFS command line
-   extension to use this option and configure it like this:
+   extension (minimum version 1.2.0) to use this option and configure it
+   like this:
 +
 -
 git config   git-p4.largeFileSystem GitLFS
diff --git a/git-p4.py b/git-p4.py
index 527d44b..d2be574 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1064,8 +1064,8 @@ class GitLFS(LargeFileSystem):
 if pointerProcess.wait():
 os.remove(contentFile)
 die('git-lfs pointer command failed. Did you install the 
extension?')
-pointerContents = [i+'\n' for i in pointerFile.split('\n')[2:][:-1]]
-oid = pointerContents[1].split(' ')[1].split(':')[1][:-1]
+oidEntry = [i for i in pointerFile.split('\n') if i.startswith('oid')]
+oid = oidEntry[0].split(' ')[1].split(':')[1]
 localLargeFile = os.path.join(
 os.getcwd(),
 '.git', 'lfs', 'objects', oid[:2], oid[2:4],
@@ -1073,7 +1073,7 @@ class GitLFS(LargeFileSystem):
 )
 # LFS Spec states that pointer files should not have the executable 
bit set.
 gitMode = '100644'
-return (gitMode, pointerContents, localLargeFile)
+return (gitMode, pointerFile, localLargeFile)

 def pushFile(self, localLargeFile):
 uploadProcess = subprocess.Popen(
--
2.5.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 v1 0/2] git-p4: fix Git LFS pointer parsing

[larsxschneider]

From: Lars Schneider 

Travis-CI uses 'brew' to always install the latest available version of
Git LFS on the OS X build machines (on Linux the version is sticky).
A change in Git LFS 1.2.0 [1] breaks the git-p4 LFS integration [2].

This mini series updates Travis-CI to the latest Git-LFS and Perforce
versions on Linux and Mac and makes git-p4 work with the latest Git LFS
again.

Thanks,
Lars

[1] https://github.com/github/git-lfs/pull/1105
[2] https://travis-ci.org/git/git/jobs/124047942

Lars Schneider (2):
  travis-ci: update Git-LFS and P4 to the latest version
  git-p4: fix Git LFS pointer parsing

 .travis.yml  | 4 ++--
 Documentation/git-p4.txt | 3 ++-
 git-p4.py| 6 +++---
 3 files changed, 7 insertions(+), 6 deletions(-)

--
2.5.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 v1 1/2] travis-ci: update Git-LFS and P4 to the latest version

[larsxschneider]

From: Lars Schneider 

Signed-off-by: Lars Schneider 
---
 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 78e433b..4acf617 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -22,8 +22,8 @@ addons:
 env:
   global:
 - DEVELOPER=1
-- P4_VERSION="15.2"
-- GIT_LFS_VERSION="1.1.0"
+- P4_VERSION="16.1"
+- GIT_LFS_VERSION="1.2.0"
 - DEFAULT_TEST_TARGET=prove
 - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
 - GIT_TEST_OPTS="--verbose --tee"
-- 
2.5.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 v3 2/2] git-p4: add P4 jobs to git commit message

[Jan Durovec]

When migrating from Perforce to git the information about P4 jobs
associated with P4 changelists is lost.

Having these jobs listed on messages of related git commits enables smooth
migration for projects that take advantage of e.g. JIRA integration
(which uses jobs on Perforce side and parses commit messages on git side).

The jobs are added to the message in the same format as is expected when
migrating in the reverse direction.

Signed-off-by: Jan Durovec 
---
 git-p4.py  | 12 ++
 t/lib-git-p4.sh|  9 +
 t/t9829-git-p4-jobs.sh | 99 ++
 3 files changed, 120 insertions(+)
 create mode 100755 t/t9829-git-p4-jobs.sh

diff --git a/git-p4.py b/git-p4.py
index 527d44b..8f869d7 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2320,6 +2320,15 @@ def extractFilesFromCommit(self, commit):
 fnum = fnum + 1
 return files
 
+def extractJobsFromCommit(self, commit):
+jobs = []
+jnum = 0
+while commit.has_key("job%s" % jnum):
+job = commit["job%s" % jnum]
+jobs.append(job)
+jnum = jnum + 1
+return jobs
+
 def stripRepoPath(self, path, prefixes):
 """When streaming files, this is called to map a p4 depot path
to where it should go in git.  The prefixes are either
@@ -2665,6 +2674,7 @@ def hasBranchPrefix(self, path):
 def commit(self, details, files, branch, parent = ""):
 epoch = details["time"]
 author = details["user"]
+jobs = self.extractJobsFromCommit(details)
 
 if self.verbose:
 print('commit into {0}'.format(branch))
@@ -2692,6 +2702,8 @@ def commit(self, details, files, branch, parent = ""):
 
 self.gitStream.write("data < 0:
+self.gitStream.write("\nJobs: %s" % (' '.join(jobs)))
 self.gitStream.write("\n[git-p4: depot-paths = \"%s\": change = %s" %
  (','.join(self.branchPrefixes), 
details["change"]))
 if len(details['options']) > 0:
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 0e59fd1..ce3536e 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -160,6 +160,15 @@ p4_add_user () {
EOF
 }
 
+p4_add_job () {
+   p4 job -f -i <<-EOF
+   Job: $1
+   Status: open
+   User: dummy
+   Description:
+   EOF
+}
+
 retry_until_success () {
timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
until "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
diff --git a/t/t9829-git-p4-jobs.sh b/t/t9829-git-p4-jobs.sh
new file mode 100755
index 000..971aeee
--- /dev/null
+++ b/t/t9829-git-p4-jobs.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='git p4 retrieve job info'
+
+. ./lib-git-p4.sh
+
+test_expect_success 'start p4d' '
+   start_p4d
+'
+
+test_expect_success 'add p4 jobs' '
+   (
+   p4_add_job TESTJOB-A &&
+   p4_add_job TESTJOB-B
+   )
+'
+
+test_expect_success 'add p4 files' '
+   client_view "//depot/... //client/..." &&
+   (
+   cd "$cli" &&
+   >file1 &&
+   p4 add file1 &&
+   p4 submit -d "Add file 1"
+   )
+'
+
+test_expect_success 'check log message of changelist with no jobs' '
+   client_view "//depot/... //client/..." &&
+   test_when_finished cleanup_git &&
+   (
+   cd "$git" &&
+   git init . &&
+   git p4 clone --use-client-spec --destination="$git" //depot@all 
&&
+   cat >expect <<-\EOF &&
+   Add file 1
+   [git-p4: depot-paths = "//depot/": change = 1]
+
+   EOF
+   git log --format=%B >actual &&
+   test_cmp expect actual
+   )
+'
+
+test_expect_success 'add TESTJOB-A to change 1' '
+   (
+   cd "$cli" &&
+   p4 fix -c 1 TESTJOB-A
+   )
+'
+
+test_expect_success 'check log message of changelist with one job' '
+   client_view "//depot/... //client/..." &&
+   test_when_finished cleanup_git &&
+   (
+   cd "$git" &&
+   git init . &&
+   git p4 clone --use-client-spec --destination="$git" //depot@all 
&&
+   cat >expect <<-\EOF &&
+   Add file 1
+   Jobs: TESTJOB-A
+   [git-p4: depot-paths = "//depot/": change = 1]
+
+   EOF
+   git log --format=%B >actual &&
+   test_cmp expect actual
+   )
+'
+
+test_expect_success 'add TESTJOB-B to change 1' '
+   (
+   cd "$cli" &&
+   p4 fix -c 1 TESTJOB-B
+   )
+'
+
+test_expect_success 'check log message of changelist with more jobs' '
+   client_view "//depot/... //client/..." &&
+   test_when_finished cleanup_git &&
+   (
+   cd "$git" &&
+   git init . 

[PATCH v3 1/2] git-p4: clean-up code style in tests

[Jan Durovec]

Preliminary clean-up of testing libraries for git-p4.

* spaces added to both sides of () in function definitions in lib-git-p4
* tab indentation added to git-p4 tests when <<- redirection is used

Signed-off-by: Jan Durovec 
---
 t/lib-git-p4.sh  | 24 +++
 t/t9826-git-p4-keep-empty-commits.sh | 60 ++--
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index f9ae1d7..0e59fd1 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -33,7 +33,7 @@ fi
 # Older versions of perforce were available compiled natively for
 # cygwin.  Those do not accept native windows paths, so make sure
 # not to convert for them.
-native_path() {
+native_path () {
path="$1" &&
if test_have_prereq CYGWIN && ! p4 -V | grep -q CYGWIN
then
@@ -49,7 +49,7 @@ native_path() {
 # Attention: This function is not safe again against time offset updates
 # at runtime (e.g. via NTP). The 'clock_gettime(CLOCK_MONOTONIC)'
 # function could fix that but it is not in Python until 3.3.
-time_in_seconds() {
+time_in_seconds () {
python -c 'import time; print int(time.time())'
 }
 
@@ -75,7 +75,7 @@ git="$TRASH_DIRECTORY/git"
 pidfile="$TRASH_DIRECTORY/p4d.pid"
 
 # Sometimes "prove" seems to hang on exit because p4d is still running
-cleanup() {
+cleanup () {
if test -f "$pidfile"
then
kill -9 $(cat "$pidfile") 2>/dev/null && exit 255
@@ -89,7 +89,7 @@ trap cleanup EXIT
 TMPDIR="$TRASH_DIRECTORY"
 export TMPDIR
 
-start_p4d() {
+start_p4d () {
mkdir -p "$db" "$cli" "$git" &&
rm -f "$pidfile" &&
(
@@ -151,7 +151,7 @@ start_p4d() {
return 0
 }
 
-p4_add_user() {
+p4_add_user () {
name=$1 &&
p4 user -f -i <<-EOF
User: $name
@@ -160,7 +160,7 @@ p4_add_user() {
EOF
 }
 
-retry_until_success() {
+retry_until_success () {
timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
until "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
do
@@ -168,7 +168,7 @@ retry_until_success() {
done
 }
 
-retry_until_fail() {
+retry_until_fail () {
timeout=$(($(time_in_seconds) + $RETRY_TIMEOUT))
until ! "$@" 2>/dev/null || test $(time_in_seconds) -gt $timeout
do
@@ -176,7 +176,7 @@ retry_until_fail() {
done
 }
 
-kill_p4d() {
+kill_p4d () {
pid=$(cat "$pidfile")
retry_until_fail kill $pid
retry_until_fail kill -9 $pid
@@ -186,13 +186,13 @@ kill_p4d() {
retry_until_fail kill -9 $watchdog_pid
 }
 
-cleanup_git() {
+cleanup_git () {
retry_until_success rm -r "$git"
test_must_fail test -d "$git" &&
retry_until_success mkdir "$git"
 }
 
-marshal_dump() {
+marshal_dump () {
what=$1 &&
line=${2:-1} &&
cat >"$TRASH_DIRECTORY/marshal-dump.py" <<-EOF &&
@@ -208,7 +208,7 @@ marshal_dump() {
 #
 # Construct a client with this list of View lines
 #
-client_view() {
+client_view () {
(
cat <<-EOF &&
Client: $P4CLIENT
@@ -222,7 +222,7 @@ client_view() {
) | p4 client -i
 }
 
-is_cli_file_writeable() {
+is_cli_file_writeable () {
# cygwin version of p4 does not set read-only attr,
# will be marked 444 but -w is true
file="$1" &&
diff --git a/t/t9826-git-p4-keep-empty-commits.sh 
b/t/t9826-git-p4-keep-empty-commits.sh
index be12960..fa8b9da 100755
--- a/t/t9826-git-p4-keep-empty-commits.sh
+++ b/t/t9826-git-p4-keep-empty-commits.sh
@@ -47,23 +47,23 @@ test_expect_success 'Clone repo root path with all history' 
'
git init . &&
git p4 clone --use-client-spec --destination="$git" //depot@all 
&&
cat >expect <<-\EOF &&
-Remove file 4
-[git-p4: depot-paths = "//depot/": change = 6]
+   Remove file 4
+   [git-p4: depot-paths = "//depot/": change = 6]
 
-Remove file 3
-[git-p4: depot-paths = "//depot/": change = 5]
+   Remove file 3
+   [git-p4: depot-paths = "//depot/": change = 5]
 
-Add file 4
-[git-p4: depot-paths = "//depot/": change = 4]
+   Add file 4
+   [git-p4: depot-paths = "//depot/": change = 4]
 
-Add file 3
-[git-p4: depot-paths = "//depot/": change = 3]
+   Add file 3
+   [git-p4: depot-paths = "//depot/": change = 3]
 
-Add file 2
-[git-p4: depot-paths = "//depot/": change = 2]
+   Add file 2
+   [git-p4: depot-paths = "//depot/": change = 2]
 
-Add file 1
-[git-p4: depot-paths = "//depot/": change = 1]
+   Add file 1
+   [git-p4: depot-paths = "//depot/": change = 1]
 
EOF
git log --format=%B >actual &&
@@ -80,23 +80,23 @@ test_expect_success 'Clone repo subdir with all history but 
keep empty commits'
git config git-p4.keepEmptyCommits true &&
git p4 

Re: [PATCH v3 1/7] i18n: index-pack: use plural string instead of normal one

[Junio C Hamano]

Junio C Hamano  writes:

> Hmph, two patches in the previous series seem to be missing.  On
> purpose, or by mistake?  Their net-effect is shown at the end of
> this message, and I thought they made sense.
>
> Puzzled...

Ah, I see.  These two were sent outside the series, but because they
are on the same theme, I chose to queue them together.

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

Re: [PATCH v7 4/6] verify-tag: prepare verify_tag for libification

[Junio C Hamano]

Eric Sunshine  writes:

> I'd have probably called this "display_name", but then I suppose it
> suffers the same issue Junio mentioned previously about it sounding
> like a boolean. Anyhow, as long as Junio is happy with it, that's what
> matters.

No ;-) I am just trying to help people come up with patches in a
better shape.  Somehow "display name" does not bother me as much as
"report name" did.  name_to_report may be a mouthful, but it conveys
what it is clearly, and I think it is good enough.
>
> This version of the patch is nicely improved. One nit below.
>
>> Signed-off-by: Santiago Torres 
>> ---
>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>> @@ -80,6 +83,8 @@ int cmd_verify_tag(int argc, const char **argv, const char 
>> *prefix)
>>  {
>> int i = 1, verbose = 0, had_error = 0;
>> unsigned flags = 0;
>> +   unsigned char sha1[20];
>> +   const char *name;
>
> Mentioned previously[1]: These two declarations could be moved inside
> the while-loop scope (below).
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/291813

Yup.

>
>> const struct option verify_tag_options[] = {
>> OPT__VERBOSE(, N_("print tag contents")),
>> OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
>> GPG_VERIFY_RAW),
>> @@ -96,8 +101,12 @@ int cmd_verify_tag(int argc, const char **argv, const 
>> char *prefix)
>> if (verbose)
>> flags |= GPG_VERIFY_VERBOSE;
>>
>> -   while (i < argc)
>> -   if (verify_tag(argv[i++], flags))
>> +   while (i < argc) {
>> +   name = argv[i++];
>> +   if (get_sha1(name, sha1))
>> +   had_error = !!error("tag '%s' not found.", name);
>> +   else if (verify_tag(sha1, name, flags))
>> had_error = 1;
>> +   }
>> return had_error;
>>  }
>> --
>> 2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[David Turner]

On Mon, 2016-04-18 at 11:45 -0700, Junio C Hamano wrote:
> David Turner  writes:
> 
> > Add parameters for a list of refspecs to transport_get_remote_refs
> > and
> > get_refs_list.  These parameters are presently unused -- soon, we
> > will
> > use them to implement fetches which only learn about a subset of
> > refs.
> > 
> > Signed-off-by: David Turner 
> > ---
> 
> What the code tries to do I am more than halfway happy.  It is
> unfortunate that we cannot do this natively without upgrading the
> protocol in a fundamental way, but this is a nice way to work it
> around only for Git-over-HTTP transport without having to break the
> protocol.
>  
> As a POC it is OK, but I am moderately unhappy with the use of
> "refspec" here.
> 
> At the transport layer, we shouldn't care what the receiving end
> intends to do with the objects that sits at the tip of the refs at
> the other end, so sending "refspecs" down feels somewhat wrong for
> this feature.  At one layer up in the next patch, you do use
> "interesting refs" which makes it clear that only the left-hand-side
> of a refspec, i.e. what they call it, matters, and I think that is a
> much better phrasing of the concept (and the passed data should only
> be the left-hand-side of refspecs).

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

Re: [PATCH/RFC 2/6] remote-curl.c: fix variable shadowing

[David Turner]

On Mon, 2016-04-18 at 11:35 -0700, Junio C Hamano wrote:
> David Turner  writes:
> 
> > The local variable 'options' was shadowing a global of the same
> > name.
> > 
> > Signed-off-by: David Turner 
> > ---
> 
> OK.  In general, giving a longer and more descriptive name to the
> global would be a direction to lead to more readable code, though.

OK, will do that instead.
--
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 v7 4/6] verify-tag: prepare verify_tag for libification

[Eric Sunshine]

On Tue, Apr 19, 2016 at 1:47 PM,   wrote:
> The current interface of verify_tag() resolves reference names to SHA1,
> however, the plan is to make this functionality public and the current
> interface is cumbersome for callers: they are expected to supply the
> textual representation of a sha1/refname. In many cases, this requires
> them to turn the sha1 to hex representation, just to be converted back
> inside verify_tag.
>
> Add a SHA1 parameter to use instead of the name parameter, and rename
> the name parameter to "name_to_report" for reporting purposes only.

I'd have probably called this "display_name", but then I suppose it
suffers the same issue Junio mentioned previously about it sounding
like a boolean. Anyhow, as long as Junio is happy with it, that's what
matters.

This version of the patch is nicely improved. One nit below.

> Signed-off-by: Santiago Torres 
> ---
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> @@ -80,6 +83,8 @@ int cmd_verify_tag(int argc, const char **argv, const char 
> *prefix)
>  {
> int i = 1, verbose = 0, had_error = 0;
> unsigned flags = 0;
> +   unsigned char sha1[20];
> +   const char *name;

Mentioned previously[1]: These two declarations could be moved inside
the while-loop scope (below).

[1]: http://article.gmane.org/gmane.comp.version-control.git/291813

> const struct option verify_tag_options[] = {
> OPT__VERBOSE(, N_("print tag contents")),
> OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
> GPG_VERIFY_RAW),
> @@ -96,8 +101,12 @@ int cmd_verify_tag(int argc, const char **argv, const 
> char *prefix)
> if (verbose)
> flags |= GPG_VERIFY_VERBOSE;
>
> -   while (i < argc)
> -   if (verify_tag(argv[i++], flags))
> +   while (i < argc) {
> +   name = argv[i++];
> +   if (get_sha1(name, sha1))
> +   had_error = !!error("tag '%s' not found.", name);
> +   else if (verify_tag(sha1, name, flags))
> had_error = 1;
> +   }
> return had_error;
>  }
> --
> 2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 1/6] http-backend: use argv_array functions

[David Turner]

On Mon, 2016-04-18 at 11:34 -0700, Junio C Hamano wrote:
> David Turner  writes:
> 
> > Signed-off-by: David Turner 
> > ---
> 
> OK (it might be easier to read if you used the pushl form for the
> "fixed initial segment" like these calls, though).

Good idea.
--
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 v7 0/6] Move PGP verification out of verify-tag

[Eric Sunshine]

On Tue, Apr 19, 2016 at 1:47 PM,   wrote:
> This is a follow up of [1], [2], [3], [4], [5], [6]. patches 1/6, 2/6, are the
> same as the corresponding commits in pu.
>
> v7:
> Mostly style/clarity changes mostly. Thanks Peff, Eric and Junio for the
> feedback! In summary:
>
>  * Eric pointed out issues with 3/6's commit message. It doesn't match the one
>in pu though. I also took the opportunity to update payload_size to a 
> size_t
>as Peff suggested.
>  * 4/6 I updated report_name to name_to_report, I updated the commit message
>and addressed some nits in the code, one of the fixes removed all three 
> nits
>that Eric pointed out. I updated 5/6 to match these changes
>  * I gave the commit message on 6/6 another go.

Thanks, this re-roll looks good. My reviews mention a couple nits, but
I hope they are not worth a re-roll (perhaps Junio can address them
when/if he picks up the series).

As before, the entire series is:

Reviewed-by: Eric Sunshine 
--
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 v7 6/6] tag -v: verfy directly rather than exec-ing verify-tag

[Eric Sunshine]

On Tue, Apr 19, 2016 at 1:47 PM,   wrote:
> tag -v: verfy directly rather than exec-ing verify-tag

s/verfy/verify:

> Instead of having tag -v fork to run verify-tag, use the
> gpg_verify_tag() function directly.

This description is easy enough to understand. Thanks.

> Signed-off-by: Santiago Torres 
> ---
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 1705c94..7b2918e 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,
>  static int verify_tag(const char *name, const char *ref,
> const unsigned char *sha1)
>  {
> -   const char *argv_verify_tag[] = {"verify-tag",
> -   "-v", "SHA1_HEX", NULL};
> -   argv_verify_tag[2] = sha1_to_hex(sha1);
> -
> -   if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
> -   return error(_("could not verify the tag '%s'"), name);
> -   return 0;
> +   return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/7] i18n: index-pack: use plural string instead of normal one

[Junio C Hamano]

Hmph, two patches in the previous series seem to be missing.  On
purpose, or by mistake?  Their net-effect is shown at the end of
this message, and I thought they made sense.

Puzzled...

diff --git a/builtin/branch.c b/builtin/branch.c
index 5ab106b..32be954 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -369,12 +369,14 @@ static char *get_head_description(void)
strbuf_addf(, _("(no branch, bisect started on %s)"),
state.branch);
else if (state.detached_from) {
-   /* TRANSLATORS: make sure these match _("HEAD detached at ")
-  and _("HEAD detached from ") in wt-status.c */
if (state.detached_at)
+   /* TRANSLATORS: make sure this matches
+  "HEAD detached at " in wt-status.c */
strbuf_addf(, _("(HEAD detached at %s)"),
state.detached_from);
else
+   /* TRANSLATORS: make sure this matches
+  "HEAD detached from " in wt-status.c */
strbuf_addf(, _("(HEAD detached from %s)"),
state.detached_from);
}
@@ -828,8 +830,8 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (argc == 1 && track == BRANCH_TRACK_OVERRIDE &&
!branch_existed && remote_tracking) {
fprintf(stderr, _("\nIf you wanted to make '%s' track 
'%s', do this:\n\n"), head, branch->name);
-   fprintf(stderr, _("git branch -d %s\n"), 
branch->name);
-   fprintf(stderr, _("git branch --set-upstream-to 
%s\n"), branch->name);
+   fprintf(stderr, "git branch -d %s\n", branch->name);
+   fprintf(stderr, "git branch --set-upstream-to 
%s\n", branch->name);
}
 
} else
--
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: Binary grep t7008 known breakage vanished on Cygwin

[Ramsay Jones]

On 19/04/16 09:42, Adam Dinwoodie wrote:
> On Mon, Apr 18, 2016 at 06:08:15PM +0100, Ramsay Jones wrote:
>> On 18/04/16 16:21, Adam Dinwoodie wrote:
>>> t7008.12 is marked as an expected failure, but building Git on Cygwin
>>> including a `make configure && ./configure` step has the test
>>> unexpectedly passing.  Building without the configure step has the test
>>> failing as expected.
>>>
>>> This appears to be behaviour specific to Cygwin; at least I get that
>>> test failing on my CentOS box regardless of whether I perform the
>>> configure step.
>>
>> Yes, the configure sets NO_REGEX= whereas the config.mak.uname sets
>> NO_REGEX=UnfortunatelyYes.
>>
>> [Note that the regex bug (see t0070-fundamental.sh test #5) now seems to
>> pass with the 'native' regex library]
> 
> Ah, that makes sense.
> 
> I'm still not quite sure what the "correct" thing to do here is; it
> looks as though the NOREGEX=UnfortunatelyYes can disappear from
> config.mak.uname, but that still leaves t7008.12 passing when it's
> expected to fail.

Yep. ;-)

About two years ago, I went from 32-bit 1.5 to 32-bit 1.7 then 64-bit 1.7.
At that time the 'native' regex library suffered from the 'regex bug' (ie it
failed t0070.5). It also 'unexpectedly passed' t7008.12. However, since the
fix for t0070.5 was to use the compat/regex library, I didn't have to give
t7008.12 any thought. :-D

I am currently running:

$ uname -a
CYGWIN_NT-10.0 satellite 2.4.0(0.293/5/3) 2016-01-15 16:16 x86_64 Cygwin
$ 

Until yesterday, I didn't know that the native regex library no longer suffers
from the t0070.5 bug. I simply don't use the configure script - never have.
It seems that t7008.12 is still an issue, however.

The commit which added that test, commit f96e5673, seems to expect that NUL
characters should not be matched in any way. Now, _if_ you accept that this
is correct behaviour, then the native regex library on Cygwin still has a
problem.

Maybe we need to add a check to test-regex (and/or configure)? dunno.
Alternatively, we could skip the test with a !CYGWIN prerequisite.

[Again, I don't use configure or the native regex library]

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: [PATCH/RFC 3/6] http-backend: handle refspec argument

[David Turner]

On Sat, 2016-04-16 at 21:51 -0400, Eric Sunshine wrote:
> On Fri, Apr 15, 2016 at 3:19 PM, David Turner <
> dtur...@twopensource.com> wrote:
> > +   if (refspec) {
> > +   struct strbuf interesting_refs =
> > STRBUF_INIT;
> > +   strbuf_addstr(_refs, "-
> > -interesting-refs=");
> > +   strbuf_addstr(_refs, refspec);
> > +   argv_array_push(,
> > interesting_refs.buf);
> > +   strbuf_release(_refs);
> > +   }
> 
> if (refspec)
> argv_array_pushf(_refs,
> "--interesting-refs=%s", refspec);


Will fix, thanks.

> > argv_array_push(, ".");
> > run_service(argv.argv, 0);
> > argv_array_clear();
> > @@ -841,6 +905,19 @@ int main(int argc, char **argv)
> > +   if (starts_with(arg, "--interesting-refs=")) {
> > ...
> > +   continue;
> > +   }
> 
> Is this leaking the string list?

Yes, intentionally.  interesting_refspec is a global that we look at
later.  
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 09/16] index-helper: use watchman to avoid refreshing index with lstat()

[David Turner]

On Fri, 2016-04-15 at 17:07 -0700, Stefan Beller wrote:
> > +static void refresh_by_watchman(struct index_state *istate)
> > +{
> > +   void *shm = NULL;
> > +   int length;
> > +   int i;
> > +   struct stat st;
> > +   int fd = -1;
> > +   const char *path = index_helper_path("git-watchman-%s
> > -%"PRIuMAX,
> > +sha1_to_hex(istate
> > ->sha1),
> > +(uintmax_t)getpid());
> > +
> > +   fd = open(path, O_RDONLY);
> > +   if (fd < 0)
> > +   return;
> > +
> > +   /*
> > +* This watchman data is just for us -- no need to keep it
> > +* around once we've got it open.
> > +*/
> > +   unlink(path);
> > +
> > +   if (fstat(fd, ) < 0)
> > +   goto done;
> > +
> > +   length = st.st_size;
> > +   shm = mmap(NULL, length, PROT_READ, MAP_SHARED, fd, 0);
> > +
> > +   if (shm == MAP_FAILED)
> > +   goto done;
> > +
> > +   close(fd);
> > +   fd = -1;
> > +
> > +   if (length <= 20 ||
> > +   hashcmp(istate->sha1, (unsigned char *)shm + length -
> > 20) ||
> > +   /*
> > +* No need to clear CE_WATCHMAN_DIRTY set by 'WAMA' on
> > +* disk. Watchman can only set more, not clear any, so
> > +* this is OR mask.
> > +*/
> > +   read_watchman_ext(istate, shm, length - 20))
> > +   goto done;
> > +
> > +   /*
> > +* Now that we've marked the invalid entries in the
> > +* untracked-cache itself, we can erase them from the list
> > of
> > +* entries to be processed and mark the untracked cache for
> > +* watchman usage.
> > +*/
> > +   if (istate->untracked) {
> > +   string_list_clear(>untracked
> > ->invalid_untracked, 0);
> > +   istate->untracked->use_watchman = 1;
> > +   }
> > +
> > +   for (i = 0; i < istate->cache_nr; i++) {
> > +   struct cache_entry *ce = istate->cache[i];
> > +   if (ce_stage(ce) || (ce->ce_flags &
> > CE_WATCHMAN_DIRTY))
> > +   continue;
> > +   ce_mark_uptodate(ce);
> > +   }
> > +done:
> > +   if (shm)
> > +   munmap(shm, length);
> > +
> > +   if (fd > 0)
> > +   close(fd);
> 
> coverities opinion:
> > off_by_one: Testing whether handle fd is strictly greater than zero
> > is suspicious.
> > fd leaks when it is zero. Did you intend to include equality with
> > zero?
> 
> We can assert that fd is never 0, because that is where stdin is?

You are right: it should be if (fd >= 0).  Will fix. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Jan Durovec]

> Any submitGit users?  I think it lets you throw multiple-patch
> series just fine.  In this case, you'd prepare a two patch series on
> a branch, 1/2 being the clean-up and 2/2 being the new feature, and
> if you give that branch to submitGit as a whole it should do the
> right thing, I'd imagine.

Hm... I'll see what it does with a pull request that has 2 commits.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Junio C Hamano]

Jan Durovec  writes:

> On Tue, Apr 19, 2016 at 7:47 PM, Junio C Hamano  wrote:
>>
>> If you really want to know the preference, we prefer a preliminary
>> clean-up patch to correct existing style issues, followed by a new
>> feature patch that builds on the cleaned up codebase.
>
> Would it be acceptable the other way around? I.e. this patch followed
> by the one that fixes code style (once this gets merged)?

The reason I said "preference" and not "requirement" is because the
answer to that question is "It depends."

When the primary change is something small and urgent (e.g. an
important bugfix that needs to be merged down to 'maint' and older),
we typically do not want to keep the fix waiting for preliminary
clean-up patch to be reviewed.  Instead, we'd say "let's fix it
first on top of a known-to-be-crappy codebase, and postpone the
clean-up until the dust settles".

>From experience, we already know that sometimes clean-up happens,
but often it does not, when we take this route, and it harms the
long-term code health, but we just say an urgent fix is more
important than piling a bit more cruft on the existing technical
debt.

And that experience is why we say "preference is to have preliminary
clean-up first".

> Reason being that I don't know how to use submitGit to generate a patch
> against a state that is not already in git repo (ie. based on another
> patch).

Any submitGit users?  I think it lets you throw multiple-patch
series just fine.  In this case, you'd prepare a two patch series on
a branch, 1/2 being the clean-up and 2/2 being the new feature, and
if you give that branch to submitGit as a whole it should do the
right thing, I'd imagine.
--
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

Hi

[yasser]

Dear Beloved
I am Mrs. Lily KIm from Syria who is dying for a gaseous poisoning exposition 
in Syria who has decided to donate her funds to you for charity project, For
More details contact me.
Via e-mail: mrslilyki...@gmail.com

Thank you and God bless you.
Mrs. Lily Kim
--
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] mv: allow moving nested submodules

[Stefan Beller]

When directories are moved using `git mv` all files in the directory
have been just moved, but no further action was taken on them. This
was done by assigning the mode = WORKING_DIRECTORY to the files
inside a moved directory.

submodules however need to update their link to the git directory as
well as updates to the .gitmodules file. By removing the condition of
`mode != INDEX` (the remaining modes are BOTH and WORKING_DIRECTORY) for
the required submodule actions, we perform these for submodules in a
moved directory.

Signed-off-by: Stefan Beller 
---

> Can you do only the 2/2 on top of maint (or maint-2.6) for now then?

This is developed on maint-2.6.

 builtin/mv.c  | 22 +-
 t/t7001-mv.sh | 16 
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/builtin/mv.c b/builtin/mv.c
index d1d4316..77f3ec5 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -251,15 +251,19 @@ int cmd_mv(int argc, const char **argv, const char 
*prefix)
int pos;
if (show_only || verbose)
printf(_("Renaming %s to %s\n"), src, dst);
-   if (!show_only && mode != INDEX) {
-   if (rename(src, dst) < 0 && !ignore_errors)
-   die_errno(_("renaming '%s' failed"), src);
-   if (submodule_gitfile[i]) {
-   if (submodule_gitfile[i] != 
SUBMODULE_WITH_GITDIR)
-   connect_work_tree_and_git_dir(dst, 
submodule_gitfile[i]);
-   if (!update_path_in_gitmodules(src, dst))
-   gitmodules_modified = 1;
-   }
+   if (show_only)
+   continue;
+   if (mode != INDEX &&
+   rename(src, dst) < 0) {
+   if (ignore_errors)
+   continue;
+   die_errno(_("renaming '%s' failed"), src);
+   }
+   if (submodule_gitfile[i]) {
+   if (submodule_gitfile[i] != SUBMODULE_WITH_GITDIR)
+   connect_work_tree_and_git_dir(dst, 
submodule_gitfile[i]);
+   if (!update_path_in_gitmodules(src, dst))
+   gitmodules_modified = 1;
}
 
if (mode == WORKING_DIRECTORY)
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 7b56081..fcfc953 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -292,6 +292,9 @@ test_expect_success 'setup submodule' '
echo content >file &&
git add file &&
git commit -m "added sub and file" &&
+   mkdir -p deep/directory/hierachy &&
+   git submodule add ./. deep/directory/hierachy/sub &&
+   git commit -m "added another submodule" &&
git branch submodule
 '
 
@@ -475,4 +478,17 @@ test_expect_success 'mv -k does not accidentally destroy 
submodules' '
git checkout .
 '
 
+test_expect_success 'moving a submodule in nested directories' '
+   (
+   cd deep &&
+   git mv directory ../ &&
+   # git status would fail if the update of linking git dir to
+   # work dir of the submodule failed.
+   git status &&
+   git config -f ../.gitmodules 
submodule.deep/directory/hierachy/sub.path >../actual &&
+   echo "directory/hierachy/sub" >../expect
+   ) &&
+   test_cmp actual expect
+'
+
 test_done
-- 
2.6.6.dirty

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

Re: [PATCH 0/2] WAS: [PATCH] mv: allow moving nested submodules

[Junio C Hamano]

Stefan Beller  writes:

> ..., but I am unsure
> if patch 1 is a good idea.

Then let's postpone it for now.  I too would like to hear opinion
from other submodule folks, especially Jens, for what 1/2 does
before committing us to the course.

Can you do only the 2/2 on top of maint (or maint-2.6) for now then?
--
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 4/4] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

[Junio C Hamano]

Elia Pinto  writes:

> Permit the use of the GIT_TRACE_CURL environment variable calling
> the curl_trace and curl_dump http.c helper routine

s/$/./; the patch itself is very concise and the "dump" thing in 3/4
looked sensible.

>
> Helped-by: Torsten Bögershausen 
> Helped-by: Ramsay Jones 
> Helped-by: Junio C Hamano 
> Helped-by: Eric Sunshine  
> Helped-by: Jeff King 
> Signed-off-by: Elia Pinto 
> ---
>  imap-send.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/imap-send.c b/imap-send.c
> index 938c691..b371a78 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1444,6 +1444,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>   if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
>   curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
>  
> + if (trace_want(_curl)) {
> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
> + curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
> + curl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);
> + }
> +
>   return curl;
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 4/6] transport: add refspec list parameters to functions

[Stefan Beller]

On Tue, Apr 19, 2016 at 12:14 AM, Jeff King  wrote:
> On Mon, Apr 18, 2016 at 11:45:54AM -0700, Junio C Hamano wrote:
>
>> David Turner  writes:
>>
>> > Add parameters for a list of refspecs to transport_get_remote_refs and
>> > get_refs_list.  These parameters are presently unused -- soon, we will
>> > use them to implement fetches which only learn about a subset of refs.
>> >
>> > Signed-off-by: David Turner 
>> > ---
>>
>> What the code tries to do I am more than halfway happy.  It is
>> unfortunate that we cannot do this natively without upgrading the
>> protocol in a fundamental way, but this is a nice way to work it
>> around only for Git-over-HTTP transport without having to break the
>> protocol.
>
> I dunno, I am a bit negative on bringing new features to Git-over-HTTP
> (which is already less efficient than the other protocols!) without any
> plan for supporting them in the other protocols.
>
> I thought Stefan's v2 protocol work looked quite good, but it seems to
> have stalled. The hardest part of that topic is figuring out the upgrade
> path. But for git-over-http, we can solve that in the same way that
> David is passing in the extra refspecs.

Yeah it stalled, though I hope to revive it eventually.

I was positive about these changes for that same reason: If http and native
protocol move apart even more, it will be easier to make the native only
v2 protocol without needing to fiddle with http, i.e. this series would reduce
scope of the v2 series drastically?

>
> So I'd rather see something like:
>
>   1. Support for v2 "capabilities only" initial negotiation, followed
>  by ref advertisement.

And the client needs to talk in between capabilities and ref advertisement.
Even if it is just a flush for now. That can be extended later to the actual
desired capabilities, but the server needs to at least wait for a client packet
in here.

Note that the server side for v2 capabilites only first is done, the client side
is missing as I found that to be the hard part.


>
>   2. Support for refspec-limiting capability.
>
>   3. HTTP-only option from client to trigger v2 on the server.
>
> That's still HTTP-specific, but it has a clear path for converging with
> the ssh and git protocols eventually, rather than having to support
> magic out-of-band capabilities forever.
>
> It does require an extra round of HTTP request/response, 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Jan Durovec]

Would it be acceptable the other way around? I.e. this patch followed
by the one that fixes code style (once this gets merged)?

Reason being that I don't know how to use submitGit to generate a patch
against a state that is not already in git repo (ie. based on another
patch).

In the following patch I'll
* add spaces before () for functions in t/lib-git-p4.sh
* remove name local variable in p4_add_job/user in t/lib-git-p4.sh
* fix t/t98* leading tabs where <<- is used

On Tue, Apr 19, 2016 at 7:47 PM, Junio C Hamano  wrote:
> Jan Durovec  writes:
>
>> given the fact that the rest of the code just follows existing
>> source code style, i.e.
>>
>> * using %s not %d to add number to string (see git-p4.py:2301)
>
> This one I do not care too deeply about, as formatting anything that
> can be formatted via '%s' could just be more Pythonic style, in
> which case "%s" is perfectly fine.  It just didn't look kosher to my
> C trained eyes, that's all.
>
>> * no space between function name and parentheses (see all functions
>>   in t/lib-git-p4.sh)
>
> I thought I said "Not a new issue, but..." to this one, and it
> appears that leaving <<- here-doc unindented, which is stupid as
> that shows the person who is writing the here-doc does not know what
> the dash s/he is writing means at all, is also not a new issue.
>
>> * no tab when specifying in-line expected output (see t/t9826...)
>
>> ...is there anything left to fix in this patch or is it good as is?
>>
>> I.e. would you prefer me to change the code mentioned above at the cost
>> of code style consistency?
>
> Not really.
>
> If you really want to know the preference, we prefer a preliminary
> clean-up patch to correct existing style issues, followed by a new
> feature patch that builds on the cleaned up codebase.
>
--
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/4] http.c: implements the GIT_TRACE_CURL environment variable

[Junio C Hamano]

Elia Pinto  writes:

> Implements the GIT_TRACE_CURL environment variable to allow a
> greater degree of detail of GIT_CURL_VERBOSE, in particular
> the complete transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis
>
>
> Helped-by: Torsten Bögershausen 
> Helped-by: Ramsay Jones 
> Helped-by: Junio C Hamano 
> Helped-by: Eric Sunshine  
> Helped-by: Jeff King 
> Signed-off-by: Elia Pinto 
> ---
>  http.c | 98 
> +-
>  1 file changed, 97 insertions(+), 1 deletion(-)
>
> diff --git a/http.c b/http.c
> index 4304b80..278991e 100644
> --- a/http.c
> +++ b/http.c
> @@ -11,6 +11,9 @@
>  #include "gettext.h"
>  #include "transport.h"
>  
> +/*
> +tatic struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
> +*/

Is this a sign that this hasn't been proof-read before sending?

>  #if LIBCURL_VERSION_NUM >= 0x070a08
>  long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
>  #else
> @@ -464,6 +467,95 @@ static void set_curl_keepalive(CURL *c)
>  }
>  #endif
>  
> +
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)
> +{
> + size_t i;
> + size_t w;
> + struct strbuf out = STRBUF_INIT;;
> +
> + unsigned int width = 0x10;
> +
> + if (nohex)
> + /* without the hex output, we can fit more on screen */
> + width = 0x40;
> +
> + strbuf_addf(, "%s, %10.10ld bytes (0x%8.8lx)\n",
> + text, (long)size, (long)size);
> +
> + for (i = 0; i < size; i += width) {
> +
> + strbuf_addf(, "%4.4lx: ", (long)i);
> +
> + if (!nohex) {
> + /* hex not disabled, show it */
> + for (w = 0; w < width; w++)
> + if (i + w < size)
> + strbuf_addf(, "%02x ", ptr[i + w]);
> + else
> + strbuf_addf(,"   ");
> + }
> +
> + for (w = 0; (w < width) && (i + w < size); w++) {
> + /* check for 0D0A; if found, skip past and start a new 
> line of output */
> + if (nohex && (i + w + 1 < size) && ptr[i + w] == '\r'
> + && ptr[i + w + 1] == '\n') {
> + i += (w + 2 - width);
> + break;
> + }
> + strbuf_addch(, (ptr[i + w] >= 0x20)
> + && (ptr[i + w] < 0x80) ? ptr[i + w] : '.');
> + /* check again for 0D0A, to avoid an extra \n if it's 
> at width */
> + if (nohex && (i + w + 2 < size)
> + && ptr[i + w + 1] == '\r'
> + && ptr[i + w + 2] == '\n') {
> + i += (w + 3 - width);
> + break;
> + }
> + }
> + strbuf_addch(, '\n');
> + trace_strbuf(_curl, );
> + strbuf_release();
> + }
> +}
> +
> +int curl_trace(CURL *handle, curl_infotype type,
> +  char *data, size_t size, void *userp)
> +{
> + const char *text;
> + (void)handle;   /* prevent compiler warning */
> +
> + switch (type) {
> + case CURLINFO_TEXT:
> + trace_printf_key(_curl, "== Info: %s", data);
> + default:/* in case a new one is introduced to shock us 
> */
> + return 0;
> +
> + case CURLINFO_HEADER_OUT:
> + text = "=> Send header";
> + break;
> + case CURLINFO_DATA_OUT:
> + text = "=> Send data";
> + break;
> + case CURLINFO_SSL_DATA_OUT:
> + text = "=> Send SSL data";
> + break;
> + case CURLINFO_HEADER_IN:
> + text = "<= Recv header";
> + break;
> + case CURLINFO_DATA_IN:
> + text = "<= Recv data";
> + break;
> + case CURLINFO_SSL_DATA_IN:
> + text = "<= Recv SSL data";
> + break;
> + }
> +
> + curl_dump(text, (unsigned char *)data, size, 1);
> + return 0;
> +}
> +
> +
>  static CURL *get_curl_handle(void)
>  {
>   CURL *result = curl_easy_init();
> @@ -563,7 +655,11 @@ static CURL *get_curl_handle(void)
>   "your curl version is too old (>= 7.19.4)");
>  #endif
>  
> - if (getenv("GIT_CURL_VERBOSE"))
> + if (trace_want(_curl)) {
> + curl_easy_setopt(result, CURLOPT_VERBOSE, 1L);
> + curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
> + curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);
> + } else if (getenv("GIT_CURL_VERBOSE"))
>   

Re: [PATCH 1/4] http.h: Add debug callback and helper routine for implementing the GIT_TRACE_CURL environment variable in http.c

[Junio C Hamano]

Elia Pinto  writes:

> Add the debug callback and helper routine prototype used by
> curl_easy_setopt CURLOPT_DEBUGFUNCTION in http.c
> for implementing the GIT_TRACE_CURL environment variable
>
>
> Helped-by: Torsten Bögershausen 
> Helped-by: Ramsay Jones 
> Helped-by: Junio C Hamano 
> Helped-by: Eric Sunshine  
> Helped-by: Jeff King 
> Signed-off-by: Elia Pinto 
> ---
>  http.h | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/http.h b/http.h
> index 4ef4bbd..a2d10bc 100644
> --- a/http.h
> +++ b/http.h
> @@ -224,4 +224,10 @@ extern int finish_http_object_request(struct 
> http_object_request *freq);
>  extern void abort_http_object_request(struct http_object_request *freq);
>  extern void release_http_object_request(struct http_object_request *freq);
>  
> +/* Debug callback and helper routine for curl_easy_setopt 
> CURLOPT_DEBUGFUNCTION */
> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);

This will be multiply instanciated in every *.c file that includes http.h?

> +int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, 
> void *userp);
> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char 
> nohex);
> +
> +
>  #endif /* HTTP_H */

In any case, you'd want to combine this with 2/4 and write a single
log message for the combined change.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Junio C Hamano]

Junio C Hamano  writes:

> Not a new problem in this script, but we'd prefer to spell this as
>
> p4_add_job () {
>
> i.e. a space on both sides of ().
>
>> +name=$1 &&
>> +p4 job -f -i <<-EOF
>> +Job: $name
>> +Status: open
>> +User: dummy
>> +Description:
>> +EOF
>> +}
>
> It may be better without $name?

Just so that I won't get misunderstood, with this I do not mean
"Job: $name" line does not have to be there.  I meant that there is
no need to use name variable in this function; just writing $1
instead of $name there is better, as $name is not a function local
variable in POSIX shells.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] git-p4: add P4 jobs to git commit message

[Junio C Hamano]

Jan Durovec  writes:

> given the fact that the rest of the code just follows existing
> source code style, i.e.
>
> * using %s not %d to add number to string (see git-p4.py:2301)

This one I do not care too deeply about, as formatting anything that
can be formatted via '%s' could just be more Pythonic style, in
which case "%s" is perfectly fine.  It just didn't look kosher to my
C trained eyes, that's all.

> * no space between function name and parentheses (see all functions
>   in t/lib-git-p4.sh)

I thought I said "Not a new issue, but..." to this one, and it
appears that leaving <<- here-doc unindented, which is stupid as
that shows the person who is writing the here-doc does not know what
the dash s/he is writing means at all, is also not a new issue.

> * no tab when specifying in-line expected output (see t/t9826...)

> ...is there anything left to fix in this patch or is it good as is?
>
> I.e. would you prefer me to change the code mentioned above at the cost
> of code style consistency?

Not really.

If you really want to know the preference, we prefer a preliminary
clean-up patch to correct existing style issues, followed by a new
feature patch that builds on the cleaned up codebase.

--
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 v7 6/6] tag -v: verfy directly rather than exec-ing verify-tag

[santiago]

From: Santiago Torres 

Instead of having tag -v fork to run verify-tag, use the
gpg_verify_tag() function directly.

Helped-by: Eric Sunshine 
Signed-off-by: Santiago Torres 
---
 builtin/tag.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 1705c94..7b2918e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -104,13 +104,7 @@ static int delete_tag(const char *name, const char *ref,
 static int verify_tag(const char *name, const char *ref,
const unsigned char *sha1)
 {
-   const char *argv_verify_tag[] = {"verify-tag",
-   "-v", "SHA1_HEX", NULL};
-   argv_verify_tag[2] = sha1_to_hex(sha1);
-
-   if (run_command_v_opt(argv_verify_tag, RUN_GIT_CMD))
-   return error(_("could not verify the tag '%s'"), name);
-   return 0;
+   return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
 }
 
 static int do_sign(struct strbuf *buffer)
-- 
2.8.0

--
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 v7 3/6] verify-tag: update variable name and type

[santiago]

From: Santiago Torres 

The run_gpg_verify() function has two variables, size and len.

This may come off as confusing when reading the code. Clarify which one
pertains to the length of the tag headers by renaming len to
payload_size. Additionally, change the type of payload_size to size_t to
match the return type of parse_signature.

Signed-off-by: Santiago Torres 
Reviewed-by: Eric Sunshine 
Signed-off-by: Junio C Hamano 
---
 builtin/verify-tag.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 77f070a..fa26e40 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -21,20 +21,21 @@ static const char * const verify_tag_usage[] = {
 static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 {
struct signature_check sigc;
-   int len;
+   size_t payload_size;
int ret;
 
memset(, 0, sizeof(sigc));
 
-   len = parse_signature(buf, size);
+   payload_size = parse_signature(buf, size);
 
-   if (size == len) {
+   if (size == payload_size) {
if (flags & GPG_VERIFY_VERBOSE)
-   write_in_full(1, buf, len);
+   write_in_full(1, buf, payload_size);
return error("no signature found");
}
 
-   ret = check_signature(buf, len, buf + len, size - len, );
+   ret = check_signature(buf, payload_size, buf + payload_size,
+   size - payload_size, );
print_signature_buffer(, flags);
 
signature_check_clear();
-- 
2.8.0

--
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 v7 1/6] builtin/verify-tag.c: ignore SIGPIPE in gpg-interface

[santiago]

From: Santiago Torres 

The verify_signed_buffer() function may trigger a SIGPIPE when the
GPG child process terminates early (due to a bad keyid, for example)
and Git tries to write to it afterwards.  Previously, ignoring
SIGPIPE was done in builtin/verify-tag.c to avoid this issue.

However, any other caller who wants to call verify_signed_buffer()
would have to do the same.

Use sigchain_push(SIGPIPE, SIG_IGN) in verify_signed_buffer(),
pretty much like in sign_buffer(), so that any caller is not
required to perform this task.

This will avoid possible mistakes by further developers using
verify_signed_buffer().

Signed-off-by: Santiago Torres 
Reviewed-by: Eric Sunshine 
Signed-off-by: Junio C Hamano 
---
 builtin/verify-tag.c | 3 ---
 gpg-interface.c  | 2 ++
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 00663f6..77f070a 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -95,9 +95,6 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
 
-   /* sometimes the program was terminated because this signal
-* was received in the process of writing the gpg input: */
-   signal(SIGPIPE, SIG_IGN);
while (i < argc)
if (verify_tag(argv[i++], flags))
had_error = 1;
diff --git a/gpg-interface.c b/gpg-interface.c
index 3dc2fe3..2259938 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -237,6 +237,7 @@ int verify_signed_buffer(const char *payload, size_t 
payload_size,
return error(_("could not run gpg."));
}
 
+   sigchain_push(SIGPIPE, SIG_IGN);
write_in_full(gpg.in, payload, payload_size);
close(gpg.in);
 
@@ -250,6 +251,7 @@ int verify_signed_buffer(const char *payload, size_t 
payload_size,
close(gpg.out);
 
ret = finish_command();
+   sigchain_pop(SIGPIPE);
 
unlink_or_warn(path);
 
-- 
2.8.0

--
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 v7 4/6] verify-tag: prepare verify_tag for libification

[santiago]

From: Santiago Torres 

The current interface of verify_tag() resolves reference names to SHA1,
however, the plan is to make this functionality public and the current
interface is cumbersome for callers: they are expected to supply the
textual representation of a sha1/refname. In many cases, this requires
them to turn the sha1 to hex representation, just to be converted back
inside verify_tag.

Add a SHA1 parameter to use instead of the name parameter, and rename
the name parameter to "name_to_report" for reporting purposes only.

Helped-by: Junio C Hamano 
Signed-off-by: Santiago Torres 
---
 builtin/verify-tag.c | 27 ++-
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index fa26e40..01e956f 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -42,25 +42,28 @@ static int run_gpg_verify(const char *buf, unsigned long 
size, unsigned flags)
return ret;
 }
 
-static int verify_tag(const char *name, unsigned flags)
+static int verify_tag(const unsigned char *sha1, const char *name_to_report,
+   unsigned flags)
 {
enum object_type type;
-   unsigned char sha1[20];
char *buf;
unsigned long size;
int ret;
 
-   if (get_sha1(name, sha1))
-   return error("tag '%s' not found.", name);
-
type = sha1_object_info(sha1, NULL);
if (type != OBJ_TAG)
return error("%s: cannot verify a non-tag object of type %s.",
-   name, typename(type));
+   name_to_report ?
+   name_to_report :
+   find_unique_abbrev(sha1, DEFAULT_ABBREV),
+   typename(type));
 
buf = read_sha1_file(sha1, , );
if (!buf)
-   return error("%s: unable to read file.", name);
+   return error("%s: unable to read file.",
+   name_to_report ?
+   name_to_report :
+   find_unique_abbrev(sha1, DEFAULT_ABBREV));
 
ret = run_gpg_verify(buf, size, flags);
 
@@ -80,6 +83,8 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
 {
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
+   unsigned char sha1[20];
+   const char *name;
const struct option verify_tag_options[] = {
OPT__VERBOSE(, N_("print tag contents")),
OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
GPG_VERIFY_RAW),
@@ -96,8 +101,12 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
 
-   while (i < argc)
-   if (verify_tag(argv[i++], flags))
+   while (i < argc) {
+   name = argv[i++];
+   if (get_sha1(name, sha1))
+   had_error = !!error("tag '%s' not found.", name);
+   else if (verify_tag(sha1, name, flags))
had_error = 1;
+   }
return had_error;
 }
-- 
2.8.0

--
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 v7 5/6] verify-tag: move tag verification code to tag.c

[santiago]

From: Santiago Torres 

The PGP verification routine for tags could be accessed by other modules
that require to do so.

Publish the verify_tag function in tag.c and rename it to gpg_verify_tag
so it does not conflict with builtin/mktag's static function.

Helped-by: Junio C Hamano 
Signed-off-by: Santiago Torres 
---
 builtin/verify-tag.c | 55 +---
 tag.c| 53 ++
 tag.h|  2 ++
 3 files changed, 56 insertions(+), 54 deletions(-)

diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 01e956f..714c899 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -18,59 +18,6 @@ static const char * const verify_tag_usage[] = {
NULL
 };
 
-static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
-{
-   struct signature_check sigc;
-   size_t payload_size;
-   int ret;
-
-   memset(, 0, sizeof(sigc));
-
-   payload_size = parse_signature(buf, size);
-
-   if (size == payload_size) {
-   if (flags & GPG_VERIFY_VERBOSE)
-   write_in_full(1, buf, payload_size);
-   return error("no signature found");
-   }
-
-   ret = check_signature(buf, payload_size, buf + payload_size,
-   size - payload_size, );
-   print_signature_buffer(, flags);
-
-   signature_check_clear();
-   return ret;
-}
-
-static int verify_tag(const unsigned char *sha1, const char *name_to_report,
-   unsigned flags)
-{
-   enum object_type type;
-   char *buf;
-   unsigned long size;
-   int ret;
-
-   type = sha1_object_info(sha1, NULL);
-   if (type != OBJ_TAG)
-   return error("%s: cannot verify a non-tag object of type %s.",
-   name_to_report ?
-   name_to_report :
-   find_unique_abbrev(sha1, DEFAULT_ABBREV),
-   typename(type));
-
-   buf = read_sha1_file(sha1, , );
-   if (!buf)
-   return error("%s: unable to read file.",
-   name_to_report ?
-   name_to_report :
-   find_unique_abbrev(sha1, DEFAULT_ABBREV));
-
-   ret = run_gpg_verify(buf, size, flags);
-
-   free(buf);
-   return ret;
-}
-
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
int status = git_gpg_config(var, value, cb);
@@ -105,7 +52,7 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
name = argv[i++];
if (get_sha1(name, sha1))
had_error = !!error("tag '%s' not found.", name);
-   else if (verify_tag(sha1, name, flags))
+   else if (gpg_verify_tag(sha1, name, flags))
had_error = 1;
}
return had_error;
diff --git a/tag.c b/tag.c
index d72f742..ace619a 100644
--- a/tag.c
+++ b/tag.c
@@ -6,6 +6,59 @@
 
 const char *tag_type = "tag";
 
+static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
+{
+   struct signature_check sigc;
+   size_t payload_size;
+   int ret;
+
+   memset(, 0, sizeof(sigc));
+
+   payload_size = parse_signature(buf, size);
+
+   if (size == payload_size) {
+   if (flags & GPG_VERIFY_VERBOSE)
+   write_in_full(1, buf, payload_size);
+   return error("no signature found");
+   }
+
+   ret = check_signature(buf, payload_size, buf + payload_size,
+   size - payload_size, );
+   print_signature_buffer(, flags);
+
+   signature_check_clear();
+   return ret;
+}
+
+extern int gpg_verify_tag(const unsigned char *sha1,
+   const char *name_to_report, unsigned flags)
+{
+   enum object_type type;
+   char *buf;
+   unsigned long size;
+   int ret;
+
+   type = sha1_object_info(sha1, NULL);
+   if (type != OBJ_TAG)
+   return error("%s: cannot verify a non-tag object of type %s.",
+   name_to_report ?
+   name_to_report :
+   find_unique_abbrev(sha1, DEFAULT_ABBREV),
+   typename(type));
+
+   buf = read_sha1_file(sha1, , );
+   if (!buf)
+   return error("%s: unable to read file.",
+   name_to_report ?
+   name_to_report :
+   find_unique_abbrev(sha1, DEFAULT_ABBREV));
+
+   ret = run_gpg_verify(buf, size, flags);
+
+   free(buf);
+   return ret;
+}
+
 struct object *deref_tag(struct object *o, const char *warn, int warnlen)
 {
while (o && o->type == 

[PATCH v7 0/6] Move PGP verification out of verify-tag

[santiago]

From: Santiago Torres 

This is a follow up of [1], [2], [3], [4], [5], [6]. patches 1/6, 2/6, are the
same as the corresponding commits in pu.

v7: 
Mostly style/clarity changes mostly. Thanks Peff, Eric and Junio for the
feedback! In summary: 

 * Eric pointed out issues with 3/6's commit message. It doesn't match the one 
   in pu though. I also took the opportunity to update payload_size to a size_t
   as Peff suggested.
 * 4/6 I updated report_name to name_to_report, I updated the commit message 
   and addressed some nits in the code, one of the fixes removed all three nits
   that Eric pointed out. I updated 5/6 to match these changes
 * I gave the commit message on 6/6 another go.

v6: 
 * As Junio suggested, updated 4/6, to include the name argument and the
   ternary operator to provide more descriptive error messages. I propagated
   these changes to 5/6 and 6/6 as well. I'm unsure about the 80-column
   on 4/6, the ternary operator is rather long.
 * Updated and reviewed the commit messages based on Eric and Junio's
   feedback

v5:
Added helpful feedback by Eric

 * Reordering of the patches, to avoid temporal inclusion of a regression
 * Fix typos here and there.
 * Review commit messages, as some weren't representative of what the patches
   were doing anymore.
 * Updated t7030 to include Peff's suggestion, and added a helped-by line here
   as it was mostly Peff's code.
 * Updated the error-handling/printing issues that were introduced when.
   libifying the verify_tag function.

v4:

Thanks Eric, Jeff, and Hannes for the feedback.

 * I relocated the sigchain_push call so it comes after the error on
   gpg-interface (thanks Hannnes for catching this).
 * I updated the unit test to match the discussion on [3]. Now it generates
   the expected output of the tag on the fly for comparison. (This is just
   copy and paste from [3], but I verified that it works by breaking the
   while)
 * I split moving the code and renaming the variables into two patches so
   these are easier to review.
 * I used an adapter on builtin/tag.c instead of redefining all the fn*
   declarations everywhere. This introduces an issue with the way git tag -v
   resolves refnames though. I added a new commit to restore the previous
   behavior of git-tag. I'm not sure if I should've split this into two commits
   though.

v3:
Thanks Eric, Jeff, for the feedback.

 * I separated the patch in multiple sub-patches.
 * I compared the behavior of previous git tag -v and git verify-tag 
   invocations to make sure the behavior is the same
 * I dropped the multi-line comment, as suggested.
 * I fixed the issue with the missing brackets in the while (this is 
   now detected by the test).

v2:

 * I moved the pgp-verification code to tag.c 
 * I added extra arguments so git tag -v and git verify-tag both work
   with the same function
 * Relocated the SIGPIPE handling code in verify-tag to gpg-interface

v1:
 
The verify tag function is just a thin wrapper around the verify-tag
command. We can avoid one fork call by doing the verification inside
the tag builtin instead.


This applies on v2.8.0. 
Thanks!
-Santiago

[1] http://thread.gmane.org/gmane.comp.version-control.git/287649
[2] http://thread.gmane.org/gmane.comp.version-control.git/289836
[3] http://thread.gmane.org/gmane.comp.version-control.git/290608
[4] http://thread.gmane.org/gmane.comp.version-control.git/290731
[5] http://thread.gmane.org/gmane.comp.version-control.git/290790
[6] http://thread.gmane.org/gmane.comp.version-control.git/291780

Santiago Torres (6):
  builtin/verify-tag.c: ignore SIGPIPE in gpg-interface
  t7030: test verifying multiple tags
  verify-tag: Update run_gpg_verfy len variable name and type
  verify-tag: prepare verify_tag for libification
  verify-tag: move tag verification code to tag.c
  tag -v: verfy directly rather than exec-ing verify-tag

 builtin/tag.c |  8 +--
 builtin/verify-tag.c  | 62 +++
 gpg-interface.c   |  2 ++
 t/t7030-verify-tag.sh | 13 +++
 tag.c | 53 +++
 tag.h |  2 ++
 6 files changed, 79 insertions(+), 61 deletions(-)

-- 
2.8.0

--
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 v7 2/6] t7030: test verifying multiple tags

[santiago]

From: Santiago Torres 

The verify-tag command supports multiple tag names to verify, but
existing tests only test for invocation with a single tag.

Add a test invoking it with multiple tags.

Helped-by: Jeff King 
Signed-off-by: Santiago Torres 
Reviewed-by: Eric Sunshine 
Signed-off-by: Junio C Hamano 
---
 t/t7030-verify-tag.sh | 13 +
 1 file changed, 13 insertions(+)

diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 4608e71..07079a4 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -112,4 +112,17 @@ test_expect_success GPG 'verify signatures with --raw' '
)
 '
 
+test_expect_success GPG 'verify multiple tags' '
+   tags="fourth-signed sixth-signed seventh-signed" &&
+   for i in $tags
+   do
+   git verify-tag -v --raw $i || return 1
+   done >expect.stdout 2>expect.stderr.1 &&
+   grep "^.GNUPG:." expect.stderr &&
+   git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 &&
+   grep "^.GNUPG:." actual.stderr &&
+   test_cmp expect.stdout actual.stdout &&
+   test_cmp expect.stderr actual.stderr
+'
+
 test_done
-- 
2.8.0

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

Re: [PATCH 2/2] xdiff: implement empty line chunk heuristic

[Jeff King]

On Tue, Apr 19, 2016 at 08:17:38AM -0700, Stefan Beller wrote:

> On Mon, Apr 18, 2016 at 10:03 PM, Jeff King  wrote:
> 
> > I guess this will invalidate old patch-ids, but there's not much to be
> > done about that.
> 
> What do you mean by that? (What consequences do you imagine?)
> I think diffs with any kind of heuristic can still be applied, no?

I mean that if you save any old patch-ids from "git patch-id", they
won't match up when compared with new versions of git. We can probably
ignore it, though. This isn't the first time that patch-ids might have
changed, and I think the advice is already that one should not count on
them to be stable in the long term.

-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 2/2] xdiff: implement empty line chunk heuristic

[Junio C Hamano]

Jeff King  writes:

> I guess this will invalidate old patch-ids, but there's not much to be
> done about that.

If we really cared, we could disable this (and any future) change to
the compaction logic to "patch-id --[un]stable" option.

I am not sure if it is worth the effort, though ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html