date:20180103

On Thu, Jan 04, 2018 at 01:08:28AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Hopefully this is clearer, and depending on how the rest of the
> discussion goes I'll submit v2 with something like this in the commit
> message:
> 
> SSH keys A and B are known to the remote service, and used to identify
> two different users.
> 
> A can only push to repository X, and B can only fetch from repository Y.
> 
> Thus, if you have a script that does:
> 
> GIT_SSH_COMMAND="ssh -i A -i B" git ...
> 
> It'll always fail for pulling from X, and pushing to Y. Supply:
> 
> GIT_SSH_COMMAND="ssh -i B -i A" git ...
> 
> And now pulling will work, but pushing won't.

I get that you may have two different keys to go with two different
identities on a remote system. But I'm not sure I understand why
"sending" or "receiving" is the right way to split those up. Wouldn't
you also sometimes want to fetch from repository X? IOW, wouldn't you
want to tie identity "A" to repository "X", and "B" to repository "Y?

> So now I just have a GIT_SSH_COMMAND that dispatches to different keys
> depending on the operation, as noted in the commit message, and I can
> assure you that without that logic it doesn't work.

You mentioned host aliases later, which is the solution I've seen in the
wild. And then you can map each remote to a different host alias.

-Peff

Re: Bug report: git clone with dest

On Wed, Jan 03, 2018 at 02:42:51PM -0800, Isaac Shabtay wrote:

> Indeed interesting... this one's for the books...
> Thanks for the patches. Any idea when these are going to make it to the
> official Git client builds? (specifically the Windows one)

They haven't even been reviewed yet. If they get good feedback, then the
maintainer will pick them up, then merge them to 'next', and then
eventually to 'master', after which they'd become part of the next
major release. For a pure bug-fix, it may instead go to 'maint' and
become part of the next minor release.

Right now we're entering release freeze for v2.16.0. We'd still take
fixes for recent breakages there, but given the age of the problem I
doubt it will make the cutoff. But as this is a bug-fix, it might make
it into v2.16.1.

-Peff

Re: [PATCH] git-archive: accept --owner and --group like GNU tar

2018-01-03 Thread suzuki toshiya


Hi,


Hmm, it could be reasonable to assume that --append-file
would serve more cases than --uid --gid options. There
might be many people who don't care multiple UID/GID in
the source tarball, but want to append some files to the
archive generated by git-archive. I would take a look how
to do that. A point I'm afraid is that some people may
request to pass the file listing the pathnames instead of
giving many --append-file options (and a few people could
want to have a built-in default list specified by GNU
convention :-)).


Taking a glance on parse-options.h, I could not find the
existing class collecting the operands as an array (or
linked list) from multiple "--xxx=yyy" options. Similar
things might be the collecting the pathnames to pathspec
structure. Should I write something with OPTION_CALLBACK?

Regards,
mpsuzuki

suzuki toshiya wrote:

Dear René ,

By overlooking your response, I was writing a patch to add
uid/gid into zip archive X-D (not finished yet)
https://github.com/mpsuzuki/git/tree/add-zip-uid-gid
However, I found that most unix platforms use infozip's
extension to store uid/gid instead of pkzip's extension...


So this is in the context of generating release tarballs that contain
untracked files as well.  That's done in Git's own Makefile, too:


Oh, I should check other software's tarball :-)


The generated archive leaks the IDs of the user preparing the archive in
the appended entries for untracked files.  I think that's more of a
concern.  Publishing a valid non-root username on your build system may
invite attackers.


Hmm, I was not aware of such security concern about the
tarball including the developers username.


So how about making it possible to append untracked files using git
archive?  This could simplify the dist target for Git as well.  It's
orthogonal to adding the ability to explicitly specify owner and group,
but might suffice in most (all?) cases.


Hmm, it could be reasonable to assume that --append-file
would serve more cases than --uid --gid options. There
might be many people who don't care multiple UID/GID in
the source tarball, but want to append some files to the
archive generated by git-archive. I would take a look how
to do that. A point I'm afraid is that some people may
request to pass the file listing the pathnames instead of
giving many --append-file options (and a few people could
want to have a built-in default list specified by GNU
convention :-)).

I want to hear other experts' comment; no need for me to
work "--uid" "--gid" anymore, and should I switch to
"--append-file" options?

Regards,
mpsuzuki

René Scharfe wrote:

Am 02.01.2018 um 07:58 schrieb suzuki toshiya:

Dear René ,

René Scharfe wrote:

Am 29.12.2017 um 15:05 schrieb suzuki toshiya:

The ownership of files created by git-archive is always
root:root. Add --owner and --group options which work
like the GNU tar equivalent to allow overriding these
defaults.

In which situations do you use the new options?

(The sender would need to know the names and/or IDs on the receiving
end.  And the receiver would need to be root to set both IDs, or be a
group member to set the group ID; I guess the latter is more common.)

Thank you for asking the background.

In the case that additional contents are appended to the tar file
generated by git-archive, the part by git-archive and the part
appended by common tar would have different UID/GID, because common
tar preserves the UID/GID of the original files.

Of cource, both of GNU tar and bsdtar have the options to set
UID/GID manually, but their syntax are different.

In the recent source package of poppler (poppler.freedesktop.org),
there are 2 sets of UID/GIDs are found:
https://poppler.freedesktop.org/poppler-0.62.0.tar.xz

I've discussed with the maintainers of poppler, and there was a
suggestion to propose a feature to git.
https://lists.freedesktop.org/archives/poppler/2017-December/012739.html

So this is in the context of generating release tarballs that contain
untracked files as well.  That's done in Git's own Makefile, too:

  dist: git-archive$(X) configure
  ./git-archive --format=tar \
  --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
  @mkdir -p $(GIT_TARNAME)
  @cp configure $(GIT_TARNAME)
  @echo $(GIT_VERSION) > $(GIT_TARNAME)/version
  @$(MAKE) -C git-gui TARDIR=../$(GIT_TARNAME)/git-gui dist-version
  $(TAR) rf $(GIT_TARNAME).tar \
  $(GIT_TARNAME)/configure \
  $(GIT_TARNAME)/version \
  $(GIT_TARNAME)/git-gui/version
  @$(RM) -r $(GIT_TARNAME)
  gzip -f -9 $(GIT_TARNAME).tar

Having files with different owners and groups is a non-issue when
extracting with --no-same-owner, which is the default for regular users.
I assume this covers most use cases in the wild.

The generated archive leaks the IDs of the user preparing the archive in
the appended entries for untracked files.  I think that's more

Re: [PATCH] git-archive: accept --owner and --group like GNU tar

2018-01-03 Thread suzuki toshiya


Dear René ,

By overlooking your response, I was writing a patch to add
uid/gid into zip archive X-D (not finished yet)
https://github.com/mpsuzuki/git/tree/add-zip-uid-gid
However, I found that most unix platforms use infozip's
extension to store uid/gid instead of pkzip's extension...


So this is in the context of generating release tarballs that contain
untracked files as well.  That's done in Git's own Makefile, too:


Oh, I should check other software's tarball :-)


The generated archive leaks the IDs of the user preparing the archive in
the appended entries for untracked files.  I think that's more of a
concern.  Publishing a valid non-root username on your build system may
invite attackers.


Hmm, I was not aware of such security concern about the
tarball including the developers username.


So how about making it possible to append untracked files using git
archive?  This could simplify the dist target for Git as well.  It's
orthogonal to adding the ability to explicitly specify owner and group,
but might suffice in most (all?) cases.


Hmm, it could be reasonable to assume that --append-file
would serve more cases than --uid --gid options. There
might be many people who don't care multiple UID/GID in
the source tarball, but want to append some files to the
archive generated by git-archive. I would take a look how
to do that. A point I'm afraid is that some people may
request to pass the file listing the pathnames instead of
giving many --append-file options (and a few people could
want to have a built-in default list specified by GNU
convention :-)).

I want to hear other experts' comment; no need for me to
work "--uid" "--gid" anymore, and should I switch to
"--append-file" options?

Regards,
mpsuzuki

René Scharfe wrote:

Am 02.01.2018 um 07:58 schrieb suzuki toshiya:

Dear René ,

René Scharfe wrote:

Am 29.12.2017 um 15:05 schrieb suzuki toshiya:

The ownership of files created by git-archive is always
root:root. Add --owner and --group options which work
like the GNU tar equivalent to allow overriding these
defaults.

In which situations do you use the new options?

(The sender would need to know the names and/or IDs on the receiving
end.  And the receiver would need to be root to set both IDs, or be a
group member to set the group ID; I guess the latter is more common.)

Thank you for asking the background.

In the case that additional contents are appended to the tar file
generated by git-archive, the part by git-archive and the part
appended by common tar would have different UID/GID, because common
tar preserves the UID/GID of the original files.

Of cource, both of GNU tar and bsdtar have the options to set
UID/GID manually, but their syntax are different.

In the recent source package of poppler (poppler.freedesktop.org),
there are 2 sets of UID/GIDs are found:
https://poppler.freedesktop.org/poppler-0.62.0.tar.xz

I've discussed with the maintainers of poppler, and there was a
suggestion to propose a feature to git.
https://lists.freedesktop.org/archives/poppler/2017-December/012739.html


So this is in the context of generating release tarballs that contain
untracked files as well.  That's done in Git's own Makefile, too:

  dist: git-archive$(X) configure
  ./git-archive --format=tar \
  --prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
  @mkdir -p $(GIT_TARNAME)
  @cp configure $(GIT_TARNAME)
  @echo $(GIT_VERSION) > $(GIT_TARNAME)/version
  @$(MAKE) -C git-gui TARDIR=../$(GIT_TARNAME)/git-gui dist-version
  $(TAR) rf $(GIT_TARNAME).tar \
  $(GIT_TARNAME)/configure \
  $(GIT_TARNAME)/version \
  $(GIT_TARNAME)/git-gui/version
  @$(RM) -r $(GIT_TARNAME)
  gzip -f -9 $(GIT_TARNAME).tar

Having files with different owners and groups is a non-issue when
extracting with --no-same-owner, which is the default for regular users.
I assume this covers most use cases in the wild.

The generated archive leaks the IDs of the user preparing the archive in
the appended entries for untracked files.  I think that's more of a
concern.  Publishing a valid non-root username on your build system may
invite attackers.

Changing the build procedure to set owner and group to root as well as
UID and GID to zero seems like a better idea.  This is complicated by
the inconsistent command line options for GNU tar and bsdtar, as you
mentioned.

So how about making it possible to append untracked files using git
archive?  This could simplify the dist target for Git as well.  It's
orthogonal to adding the ability to explicitly specify owner and group,
but might suffice in most (all?) cases.

Not sure what kind of file name transformation abilities would be
needed and how to package them nicely.  The --transform option of GNU
tar with its sed replace expressions seems quite heavy for me.  With
poppler it's only used to add the --prefix string; I'd expect that to
be done for all

Re: [PATCH 20/26] fetch-pack: perform a fetch using v2

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> When communicating with a v2 server, perform a fetch by requesting the
> 'fetch' command.
>
> Signed-off-by: Brandon Williams 
> ---
>  builtin/fetch-pack.c   |   2 +-
>  fetch-pack.c   | 267 
> -
>  fetch-pack.h   |   4 +-
>  t/t5701-protocol-v2.sh |  40 
>  transport.c|   8 +-
>  5 files changed, 314 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index f492e8abd..867dd3cc7 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -213,7 +213,7 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
> }
>
> ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
> -, pack_lockfile_ptr);
> +, pack_lockfile_ptr, protocol_v0);
> if (pack_lockfile) {
> printf("lock %s\n", pack_lockfile);
> fflush(stdout);
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 9f6b07ad9..c26fdc539 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1008,6 +1008,262 @@ static struct ref *do_fetch_pack(struct 
> fetch_pack_args *args,
> return ref;
>  }
>
> +static void add_wants(const struct ref *wants, struct strbuf *req_buf)
> +{
> +   for ( ; wants ; wants = wants->next) {
> +   const struct object_id *remote = >old_oid;
> +   const char *remote_hex;
> +   struct object *o;
> +
> +   /*
> +* If that object is complete (i.e. it is an ancestor of a
> +* local ref), we tell them we have it but do not have to
> +* tell them about its ancestors, which they already know
> +* about.
> +*
> +* We use lookup_object here because we are only
> +* interested in the case we *know* the object is
> +* reachable and we have already scanned it.
> +*/
> +   if (((o = lookup_object(remote->hash)) != NULL) &&
> +   (o->flags & COMPLETE)) {
> +   continue;
> +   }
> +
> +   remote_hex = oid_to_hex(remote);
> +   packet_buf_write(req_buf, "want %s\n", remote_hex);
> +   }
> +}
> +
> +static int add_haves(struct strbuf *req_buf, int *in_vain)
> +{
> +   int ret = 0;
> +   int haves_added = 0;
> +   const struct object_id *oid;
> +
> +   while ((oid = get_rev())) {
> +   packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
> +   if (++haves_added >= INITIAL_FLUSH)
> +   break;
> +   };
> +
> +   *in_vain += haves_added;
> +   if (!haves_added || *in_vain >= MAX_IN_VAIN) {
> +   /* Send Done */
> +   packet_buf_write(req_buf, "done\n");
> +   ret = 1;
> +   }
> +
> +   return ret;
> +}
> +
> +static int send_haves(int fd_out, int *in_vain)
> +{
> +   int ret = 0;
> +   struct strbuf req_buf = STRBUF_INIT;
> +
> +   ret = add_haves(_buf, in_vain);
> +
> +   /* Send request */
> +   packet_buf_flush(_buf);
> +   write_or_die(fd_out, req_buf.buf, req_buf.len);
> +
> +   strbuf_release(_buf);
> +   return ret;
> +}
> +
> +static int send_fetch_request(int fd_out, const struct fetch_pack_args *args,
> + const struct ref *wants, struct oidset *common,
> + int *in_vain)
> +{
> +   int ret = 0;
> +   struct strbuf req_buf = STRBUF_INIT;
> +
> +   packet_buf_write(_buf, "command=fetch");
> +   packet_buf_write(_buf, "agent=%s", git_user_agent_sanitized());
> +   if (args->stateless_rpc)
> +   packet_buf_write(_buf, "stateless-rpc=true");
> +
> +   packet_buf_delim(_buf);
> +   if (args->use_thin_pack)
> +   packet_buf_write(_buf, "thin-pack");
> +   if (args->no_progress)
> +   packet_buf_write(_buf, "no-progress");
> +   if (args->include_tag)
> +   packet_buf_write(_buf, "include-tag");
> +   if (prefer_ofs_delta)
> +   packet_buf_write(_buf, "ofs-delta");
> +
> +   /* add wants */
> +   add_wants(wants, _buf);
> +
> +   /*
> +* If we are running stateless-rpc we need to add all the common
> +* commits we've found in previous rounds
> +*/
> +   if (args->stateless_rpc) {
> +   struct oidset_iter iter;
> +   const struct object_id *oid;
> +   oidset_iter_init(common, );
> +
> +   while ((oid = oidset_iter_next())) {
> +   packet_buf_write(_buf, "have %s\n", 
> oid_to_hex(oid));
> +   }
> +   }
> +
> +   /* Add initial haves */
> +   ret = add_haves(_buf, in_vain);
> +
> +   /*

Re: [PATCH 19/26] upload-pack: introduce fetch server command

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> Introduce the 'fetch' server command.
>
> Signed-off-by: Brandon Williams 
> ---
>  Documentation/technical/protocol-v2.txt |  14 ++
>  serve.c |   2 +
>  upload-pack.c   | 290 
> 
>  upload-pack.h   |   9 +
>  4 files changed, 315 insertions(+)
>  create mode 100644 upload-pack.h
>
> diff --git a/Documentation/technical/protocol-v2.txt 
> b/Documentation/technical/protocol-v2.txt
> index 5f4d0e719..2a8e2f226 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -115,3 +115,17 @@ The output of ls-refs is as follows:
>
>  symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
>  shallow = PKT-LINE("shallow" SP obj-id LF)
> +
> + Fetch
> +---
> +
> +Fetch will need to be a modified version of the v1 fetch protocol.  Some
> +potential areas for improvement are: Ref-in-want, CDN offloading,
> +Fetch-options.
> +
> +Since we'll have an 'ls-ref' service we can eliminate the need of fetch
> +to perform a ref-advertisement, instead a client can run the 'ls-refs'
> +service first, in order to find out what refs the server has, and then
> +request those refs directly using the fetch service.
> +
> +//TODO Flesh out the design

TODO: actually do it. ;)

a couple notes from the discussion in office:
* Could we split fetch into multiple phases
  (negotiation + getting the pack)
* negotiation could be reused in forced push to
  minimize the pack to be sent
* negotiation in a half duplex is actually better
  called 'discovery', which discovers about the set
  of objects available on the remote side.
  (the opposite would be reveal, or 'ask-for-discovery', which
  is could be used for a symmetric design of fetch and push)


> diff --git a/serve.c b/serve.c
> index 88d548410..ca3bb7190 100644
> --- a/serve.c
> +++ b/serve.c
> @@ -6,6 +6,7 @@
>  #include "argv-array.h"
>  #include "ls-refs.h"
>  #include "serve.h"
> +#include "upload-pack.h"
>
>  static int always_advertise(struct repository *r,
> struct strbuf *value)
> @@ -46,6 +47,7 @@ static struct protocol_capability capabilities[] = {
> { "agent", agent_advertise, NULL },
> { "stateless-rpc", always_advertise, NULL },
> { "ls-refs", always_advertise, ls_refs },
> +   { "fetch", always_advertise, upload_pack_v2 },
>  };
>
>  static void advertise_capabilities(void)
> diff --git a/upload-pack.c b/upload-pack.c
> index 2ca60d27c..c41f6f528 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -20,6 +20,7 @@
>  #include "prio-queue.h"
>  #include "protocol.h"
>  #include "serve.h"
> +#include "upload-pack.h"
>
>  static const char * const upload_pack_usage[] = {
> N_("git upload-pack [] "),
> @@ -1040,6 +1041,295 @@ static void upload_pack(void)
> }
>  }
>
> +struct upload_pack_data {
> +   struct object_array wants;
> +   struct oid_array haves;
> +
> +   unsigned stateless_rpc : 1;
> +
> +   unsigned use_thin_pack : 1;
> +   unsigned use_ofs_delta : 1;
> +   unsigned no_progress : 1;
> +   unsigned use_include_tag : 1;
> +   unsigned done : 1;
> +};
> +
> +#define UPLOAD_PACK_DATA_INIT { OBJECT_ARRAY_INIT, OID_ARRAY_INIT, 0, 0, 0, 
> 0, 0, 0 }
> +
> +static void upload_pack_data_clear(struct upload_pack_data *data)
> +{
> +   object_array_clear(>wants);
> +   oid_array_clear(>haves);
> +}
> +
> +static int parse_want(const char *line)
> +{
> +   const char *arg;
> +   if (skip_prefix(line, "want ", )) {
> +   struct object_id oid;
> +   struct object *o;
> +
> +   if (get_oid_hex(arg, ))
> +   die("git upload-pack: protocol error, "
> +   "expected to get oid, not '%s'", line);
> +
> +   o = parse_object();
> +   if (!o) {
> +   packet_write_fmt(1,
> +"ERR upload-pack: not our ref %s",
> +oid_to_hex());
> +   die("git upload-pack: not our ref %s",
> +   oid_to_hex());
> +   }
> +
> +   if (!(o->flags & WANTED)) {
> +   o->flags |= WANTED;
> +   add_object_array(o, NULL, _obj);
> +   }
> +
> +   return 1;
> +   }
> +
> +   return 0;
> +}
> +
> +static int parse_have(const char *line, struct oid_array *haves)
> +{
> +   const char *arg;
> +   if (skip_prefix(line, "have ", )) {
> +   struct object_id oid;
> +
> +   if (get_oid_hex(arg, ))
> +   die("git upload-pack: expected SHA1 object, got 
> '%s'", arg);
> +   oid_array_append(haves, );
> +   return 1;
> +

Re: [PATCH v5 13/34] directory rename detection: tests for handling overwriting untracked files

2018-01-03 Thread SZEDER Gábor


> ---
>  t/t6043-merge-rename-directories.sh | 337 
> 
>  1 file changed, 337 insertions(+)

> +test_expect_failure '10b-check: Overwrite untracked with dir rename + 
> delete' '
> + (
> + cd 10b &&
> +
> + git checkout A^0 &&
> + echo very >y/c &&
> + echo important >y/d &&
> + echo contents >y/e &&
> +
> + test_must_fail git merge -s recursive B^0 >out 2>err &&
> + test_i18ngrep "CONFLICT (rename/delete).*Version B^0 of y/d 
> left in tree at y/d~B^0" out &&
> + test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
> writing to y/e~B^0 instead" out &&
> +
> + test 3 -eq $(git ls-files -s | wc -l) &&
> + test 2 -eq $(git ls-files -u | wc -l) &&
> + test 5 -eq $(git ls-files -o | wc -l) &&
> +
> + test $(git rev-parse :0:y/b) = $(git rev-parse O:z/b) &&

There is a test helper for that :)

  test_cmp_rev :0:y/b O:z/b

Note, that this is not only a matter of useful output on failure, but
also that of correctness and robustness.  A failing command inside
those command substitutions won't cause the whole command above to
fail, and if both 'git rev-parse' were to fail without writing
anything to stdout, the whole condition would still be fulfilled:

  $ test $(false) = $(false) && echo true
  true

I noticed that this patch series adds several similar

  test $(git hash-object this) = $(git rev-parse that)

conditions.  Well, for that we don't have a test helper
function.  Similar 'hash-object = rev-parse' comparisons are already
present in two other test scripts, so perhaps it's worth adding a
helper function.  Or you could perhaps

  git cat-file -p that >out &&
  test_cmp this out

I also noticed that all existing 'hash-object = rev-parse' conditions
came from you, so I would leave it up to you to decide which is easier
to work with and whether it's worth it.


> + test "very" = "$(cat y/c)" &&
> +
> + test "important" = "$(cat y/d)" &&

The 'verbose' helper could make conditions like these more, well,
verbose about their failure:

  verbose test "very" = "$(cat y/c)" &&

> + test "important" != "$(git rev-parse :3:y/d)" &&

I'm not sure what this condition is supposed to check.

I'm not particularly well versed in the intricacies of 'git rev-parse'
operating on different stages of the index, but to my understanding
'git rev-parse rev' either outputs the object name pointed by 'rev',
or 'rev' verbatim if that doesn't resolve to a valid object.  IOW, it
would never output "important" and the condition would always be
fulfilled.
What am I missing?

> + test $(git rev-parse :3:y/d) = $(git rev-parse O:z/c) &&
> +
> + test "contents" = "$(cat y/e)" &&
> + test "contents" != "$(git rev-parse :3:y/e)" &&
> + test $(git rev-parse :3:y/e) = $(git rev-parse B:z/e)
> + )
> +'

Re: [PATCH] git-archive: accept --owner and --group like GNU tar

2018-01-03 Thread René Scharfe

[replying only to the list because emails to per...@pluto.rain.com
 are rejected by my mail server with the following error message:
 "Requested action not taken: mailbox unavailable
  invalid DNS MX or A/ resource record."]

Am 02.01.2018 um 01:32 schrieb Perry Hutchison:
> Ren?? Scharfe  wrote:
>> Am 29.12.2017 um 15:05 schrieb suzuki toshiya:
>>> The ownership of files created by git-archive is always
>>> root:root. Add --owner and --group options which work
>>> like the GNU tar equivalent to allow overriding these
>>> defaults.
>> ... the receiver would need to be root to set both IDs, or be a
>> group member to set the group ID; I guess the latter is more common.
> 
> If the received files are owned by root:root as stated, I guess the
> receiver must be running as root, no?

That depends on what you mean with "must".  Users who want the files
they extract to be owned by root need root permissions on Unix and
Linux.  If they are OK with owning the files themselves then regular
user accounts suffice.  I assume the latter is much more common.

René

Re: [PATCH 0/2] Several fixes for the test suite related to spaces in filenames

Jeff King  writes:

> On Wed, Jan 03, 2018 at 05:54:44PM +0100, Johannes Schindelin wrote:
>
>> The second issue was found long ago, and the patch carried in Git for
>> Windows, although nothing about it is specific to Windows.
>> 
>> The first patch was developed today, when I tried to verify that Git's
>> test suite passes if Git is cloned to a directory called `with spaces/`.
>
> Heh, the whole point of the space in the trash directory was to find
> these issues early, but obviously it is not foolproof. :)

Exactly.

> The patches themselves look good to me from inspection. Thanks.

Yes, these changes look good.  Thanks both.

Re: [PATCH 12/26] ls-refs: introduce ls-refs server command

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> Introduce the ls-refs server command.  In protocol v2, the ls-refs
> command is used to request the ref advertisement from the server.  Since
> it is a command which can be requested (as opposed to mandatory in v1),
> a client can sent a number of parameters in its request to limit the ref
> advertisement based on provided ref-patterns.
>
> Signed-off-by: Brandon Williams 
> ---
>  Documentation/technical/protocol-v2.txt | 26 +
>  Makefile|  1 +
>  ls-refs.c   | 97 
> +
>  ls-refs.h   |  9 +++

Maybe consider putting any served command into a sub directory?

For example the code in builtin/ has laxer rules w.r.t. die()ing
as it is a user facing command, whereas some devs want to see
code at the root of the repo to not die() at all as the eventual goal
is to have a library there.
All this code is on the remote side, which also has different traits than
the code at the root of the git.git repo; non-localisation comes to mind,
but there might be other aspects as well (security?).


>  serve.c |  2 +
>  5 files changed, 135 insertions(+)
>  create mode 100644 ls-refs.c
>  create mode 100644 ls-refs.h
>
> diff --git a/Documentation/technical/protocol-v2.txt 
> b/Documentation/technical/protocol-v2.txt
> index b87ba3816..5f4d0e719 100644
> --- a/Documentation/technical/protocol-v2.txt
> +++ b/Documentation/technical/protocol-v2.txt
> @@ -89,3 +89,29 @@ terminate the connection.
>  Commands are the core actions that a client wants to perform (fetch, push,
>  etc).  Each command will be provided with a list capabilities and
>  arguments as requested by a client.
> +
> + Ls-refs

So is it ls-refs or Ls-refs or is any capitalization valid?

> +-
> +
> +Ls-refs is the command used to request a reference advertisement in v2.
> +Unlike the current reference advertisement, ls-refs takes in parameters
> +which can be used to limit the refs sent from the server.
> +
> +Ls-ref takes in the following parameters wraped in packet-lines:
> +
> +  symrefs: In addition to the object pointed by it, show the underlying
> +  ref pointed by it when showing a symbolic ref.
> +  peel: Show peeled tags.
> +  ref-pattern : When specified, only references matching the
> +given patterns are displayed.

What kind of pattern matching is allowed here?
strictly prefix only, or globbing, regexes?
Is there a given grammar to follow? Maybe a link to the git
glossary is or somewhere else might be fine.

Seeing that we do wildmatch() down there (as opposed to regexes),
I wonder if it provides an entry for a denial of service attack, by crafting
a pattern that is very expensive for the server to compute but cheap to
ask for from a client. (c.f. 94da9193a6 (grep: add support for PCRE v2,
2017-06-01, but that is regexes!)

> +The output of ls-refs is as follows:
> +
> +output = *ref
> +flush-pkt
> +ref = PKT-LINE((tip | peeled) LF)
> +tip = obj-id SP refname (SP symref-target)
> +peeled = obj-id SP refname "^{}"
> +
> +symref = PKT-LINE("symref" SP symbolic-ref SP resolved-ref LF)
> +shallow = PKT-LINE("shallow" SP obj-id LF)
> diff --git a/Makefile b/Makefile
> index 5f3b5fe8b..152a73bec 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -820,6 +820,7 @@ LIB_OBJS += list-objects-filter-options.o
>  LIB_OBJS += ll-merge.o
>  LIB_OBJS += lockfile.o
>  LIB_OBJS += log-tree.o
> +LIB_OBJS += ls-refs.o
>  LIB_OBJS += mailinfo.o
>  LIB_OBJS += mailmap.o
>  LIB_OBJS += match-trees.o
> diff --git a/ls-refs.c b/ls-refs.c
> new file mode 100644
> index 0..ac4904a40
> --- /dev/null
> +++ b/ls-refs.c
> @@ -0,0 +1,97 @@
> +#include "cache.h"
> +#include "repository.h"
> +#include "refs.h"
> +#include "remote.h"
> +#include "argv-array.h"
> +#include "ls-refs.h"
> +#include "pkt-line.h"
> +
> +struct ls_refs_data {
> +   unsigned peel;
> +   unsigned symrefs;
> +   struct argv_array patterns;
> +};
> +
> +/*
> + * Check if one of the patterns matches the tail part of the ref.
> + * If no patterns were provided, all refs match.
> + */
> +static int ref_match(const struct argv_array *patterns, const char *refname)
> +{
> +   char *pathbuf;
> +   int i;
> +
> +   if (!patterns->argc)
> +   return 1; /* no restriction */
> +
> +   pathbuf = xstrfmt("/%s", refname);
> +   for (i = 0; i < patterns->argc; i++) {
> +   if (!wildmatch(patterns->argv[i], pathbuf, 0)) {
> +   free(pathbuf);
> +   return 1;
> +   }
> +   }
> +   free(pathbuf);
> +   return 0;
> +}
> +
> +static int send_ref(const char *refname, const struct object_id *oid,
> +   int flag, void *cb_data)
> +{
> +   struct ls_refs_data *data =

Re: [RFC/PATCH] connect: add GIT_SSH_{SEND,RECEIVE}{,_COMMAND} env variables

On Wed, Jan 03 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason   writes:
>
>> This is useful for talking to systems such as Github or Gitlab that
>> identify user accounts (or deploy keys) by ssh keys. Normally, ssh
>> could do this itself by supplying multiple keys via -i, but that trick
>> doesn't work on these systems as the connection will have already been
>> accepted when the "wrong" key gets rejected.
>
> You need to explain this a lot better than the above.
>
> I am sure systems such as Github have more than dozens of users who
> push over ssh and these users identify themselves by which key to
> use when establishing connection just fine (presumably by using a
> "Host" entry for the github URL in ~/.ssh/config), and presumably we
> are not sending "wrong" keys over there.  So there needs to be a lot
> more clear description of the problem you are trying to solve in the
> first place.

Hopefully this is clearer, and depending on how the rest of the
discussion goes I'll submit v2 with something like this in the commit
message:

SSH keys A and B are known to the remote service, and used to identify
two different users.

A can only push to repository X, and B can only fetch from repository Y.

Thus, if you have a script that does:

GIT_SSH_COMMAND="ssh -i A -i B" git ...

It'll always fail for pulling from X, and pushing to Y. Supply:

GIT_SSH_COMMAND="ssh -i B -i A" git ...

And now pulling will work, but pushing won't.

If you were to do, where C is a completly unknown key:

GIT_SSH_COMMAND="ssh -i C -i A" git push X ...

It would work, since ssh wouldn't get far enough in the key negotiation
to drop you into a shell. This is the case you had in mind, but is
unrelated to the problem I'm trying to address.

I tested this on a Gitlab instance, but as far as I know this property
is going to be intrinsic to anything that uses ssh in this way,
i.e. once you get past the step where the server says "this key is OK"
and drops you into a shell, it's not going to retry the whole
negotiation with another key just because the command you ran exited
with non-zero.

So now I just have a GIT_SSH_COMMAND that dispatches to different keys
depending on the operation, as noted in the commit message, and I can
assure you that without that logic it doesn't work.

I thought that use-case might be useful enough to be natively supported,
since right now you either need to hack it up like that, or perform
similar hacks with url/pushurl and ssh host aliases in your config.

[no subject]

2018-01-03 Thread Jalus Bilieyich

Re: [PATCH] doc/SubmittingPatches: improve text formatting

2018-01-03 Thread brian m. carlson

On Tue, Jan 02, 2018 at 10:33:50AM -0500, Todd Zullinger wrote:
> 049e64aa50 ("Documentation: convert SubmittingPatches to AsciiDoc",
> 2017-11-12) changed the `git blame` and `git shortlog` examples given in
> the section on sending your patches.
> 
> In order to italicize the `$path` argument the commands are enclosed in
> plus characters as opposed to backticks.  The difference between the
> quoting methods is that backtick enclosed text is not subject to further
> expansion.  This formatting makes reading SubmittingPatches in a git
> clone a little more difficult.  In addition to the underscores around
> `$path` the `--` chars in `git shortlog --no-merges` must be replaced
> with `{litdd}`.
> 
> Use backticks to quote these commands.  The italicized `$path` is lost
> from the html version but the commands can be read (and copied) more
> easily by users reading the text version.  These readers are more likely
> to use the commands while submitting patches.  Make it easier for them.

I think this change is fine.  I don't have a strong opinion either way
and if others think the change makes the plain text more readable, I'm
all for it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: Misleading documentation for git-diff-files (diff-filter)

John Cheng  writes:

> I wanted to know if git diff-files shows files that are not in the
> index but are in the working tree.

At least in the original design of Git, that would fundamentally be
impossible, as Git _only_ cares about paths that are in the index,
so a new file won't be in the picture until it is added.  Because a
change is shown as "A"dded by the diff family of commands only when
the old side lacks a path that appears in the new side, there is no
way "diff-files" that compares the index and the working tree would
see a path that is missing from the old (i.e. the index) side.

Re: [RFC/PATCH] connect: add GIT_SSH_{SEND,RECEIVE}{,_COMMAND} env variables

Ævar Arnfjörð Bjarmason   writes:

> This is useful for talking to systems such as Github or Gitlab that
> identify user accounts (or deploy keys) by ssh keys. Normally, ssh
> could do this itself by supplying multiple keys via -i, but that trick
> doesn't work on these systems as the connection will have already been
> accepted when the "wrong" key gets rejected.

You need to explain this a lot better than the above.  

I am sure systems such as Github have more than dozens of users who
push over ssh and these users identify themselves by which key to
use when establishing connection just fine (presumably by using a
"Host" entry for the github URL in ~/.ssh/config), and presumably we
are not sending "wrong" keys over there.  So there needs to be a lot
more clear description of the problem you are trying to solve in the
first place.

Re: Bug report: git clone with dest

2018-01-03 Thread Isaac Shabtay

Indeed interesting... this one's for the books...
Thanks for the patches. Any idea when these are going to make it to
the official Git client builds? (specifically the Windows one)

On 3 January 2018 at 14:28, Jeff King  wrote:
> On Wed, Jan 03, 2018 at 12:59:48PM -0800, Isaac Shabtay wrote:
>
>> Target directory is deleted on clone failures.
>>
>> Steps to reproduce, for example on Windows:
>>
>> cd /d %TEMP%
>> mkdir dest
>> git clone https://some-fake-url/whatever-makes-git-clone-fail dest
>>
>> Of course, the clone will fail as it should. But looks like the Git
>> client also ends up deleting the "dest" directory.
>
> Interesting. AFAICT Git has behaved this way for almost 9 years, and now
> we have two reports in two days. Serendipity, or did something else
> change? :)
>
> Anyway, you might be interested in the patch series I posted yesterday:
>
>   https://public-inbox.org/git/20180102210753.ga10...@sigill.intra.peff.net/
>
> -Peff

Re: Bug report: git clone with dest

On Wed, Jan 03, 2018 at 12:59:48PM -0800, Isaac Shabtay wrote:

> Target directory is deleted on clone failures.
> 
> Steps to reproduce, for example on Windows:
> 
> cd /d %TEMP%
> mkdir dest
> git clone https://some-fake-url/whatever-makes-git-clone-fail dest
> 
> Of course, the clone will fail as it should. But looks like the Git
> client also ends up deleting the "dest" directory.

Interesting. AFAICT Git has behaved this way for almost 9 years, and now
we have two reports in two days. Serendipity, or did something else
change? :)

Anyway, you might be interested in the patch series I posted yesterday:

  https://public-inbox.org/git/20180102210753.ga10...@sigill.intra.peff.net/

-Peff

Re: [PATCH v5 00/34] Add directory rename detection to git

2018-01-03 Thread Johannes Sixt

Am 03.01.2018 um 22:02 schrieb Elijah Newren:

On Wed, Jan 3, 2018 at 2:57 AM, Johannes Sixt wrote:

I tested the series on Windows recently. It requires the patch below.
I don't know whether this is indicating some portability issues of grep
(^ being used in the middle of a RE instead of at the very beginning) or
just a quirk in my setup.

Thanks for testing it out. What version of Windows were you running
on? With cygwin or without? I tested previously on cygwin (I think
on Windows Server 2012??) and got all the tests passing there,
eventually[1]. I'm not sure I can find access to any other Windows
systems, but I'd be happy to take a look if I can.

[1]
https://public-inbox.org/git/cabpp-bej6-mry0ocz1wwetrtg_iehkzodcuon_puukvywau...@mail.gmail.com/

I have an ancient MinGW setup, where I build "vanilla" Git (not exactly
vanilla, but also not with the many patches that Git for Windows carries).

The need to backslash escape a caret for a literal match when it
appears in the middle of the string makes sense. Thanks for sending
along the patch. Would you prefer I squashed it into the series
(still sitting in 'pu'), or keep your patch separate? I'm fine with
either, I'm just unsure the protocol here.

Please squash into the relevant commits so that the series is bisectable
if the need arises.

But it still does not pass the test suite because the system does not
like file names such as y/c~HEAD:

++ grep 'Refusing to lose dirty file at z/c' out
Refusing to lose dirty file at z/c
++ grep -q stuff x/b y/a y/c y/c~HEAD z/c
grep: y/c: Invalid request code
error: last command exited with $?=2
not ok 94 - 11d-check: Avoid losing not-uptodate with rename + D/F conflict

This is exceptionally odd. The actual line from the testsuite was
grep -q stuff */*

which suggests your shell is both doing the pathname expansion and
treating the resulting filename not as a string but as something to be
interpreted that happens to have some kind of special
characters/commands, and then choking on the result. Super weird. I
could probably work around this by just running
grep -q stuff z/c

I think I had the asterisks in there because I was thinking in terms
of directory rename detection potentially moving the file, but that's
probably just overkill. Does the test pass for you with that change?

I can test on Monday at the earliest. If it's that easy to fix my
failures, I'd appreciate to go this route. But otherwise, I can deal
with the situation, so we don't need to complicate things just to please
my exotic setup.

(If so, there are also two similar tests that I'd need to make similar
changes to.)

However, although that might fix this particular case, it suggests
some fragility of the tests and filenames for whatever system you
happen to be using. merge-recursive.c's unique_path has created
filenames with tilde's in them for many years, it may just be that I'm
the first to use the resulting file in combination with grep to ensure
the contents are as we expect. There may be other issues lurking
(even if not yet appearing in the testsuite) for your system when
dealing with merge conflicts.

I can't recall having seen issues around tildas in file names, either.
It may be a new situation. I'll investigate.

-- Hannes

Re: [PATCH 5/5] diff: properly error out when combining multiple pickaxe options

On Wed, Jan 3, 2018 at 2:08 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
> ;
>> + count = 0;
>> +
>> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
>> + count++;
>> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
>> + count++;
>> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
>> + count++;
>> + if (count > 1)
>> + die(_("-G, -S, --find-object are mutually exclusive"));
>
> I thought the reason you defined pickaxe-kind bitmask was so that
> you can mask this field to grab these (and only these) bits.

Originally I only wanted to mask out the 'case independency'
bit and keep it future proof for any similar bits.

> Once you have that mask, you should be able to use HAS_MULTI_BITS()
> on the masked result without counting, no?

Oh, what a nice macro! Thanks for pointing at it.

As soon as I figured out the right place where to put this check,
I saw the lines above, whose style I imitated.
(I guess there is just no mask defined for "--name-only, --name-status,
--check and -s", nor would it make sense to do so; though that given
macro should work just fine even for non-continuous masks)

Re: [PATCH v2 2/3] prune: fix pruning with multiple worktrees and split index

2018-01-03 Thread Thomas Gummerer

[sorry for the late reply.  I was on Christmas holidays until today
and am still catching up on the mailing list.  It will probably take
me untill the weekend to send a re-roll]

On 12/18, Brandon Williams wrote:
> On 12/17, Thomas Gummerer wrote:
> > be489d02d2 ("revision.c: --indexed-objects add objects from all
> > worktrees", 2017-08-23) made sure that pruning takes objects from all
> > worktrees into account.
> > 
> > It did that by reading the index of every worktree and adding the
> > necessary index objects to the set of pending objects.  The index is
> > read by read_index_from.  As mentioned in the previous commit,
> > read_index_from depends on the CWD for the location of the split index,
> 
> As I mentioned before this doesn't actually depend on the CWD but
> rather the per-worktree gitdir.

Right, will fix.

> > and add_index_objects_to_pending doesn't set that before using
> > read_index_from.
> > 
> > Instead of using read_index_from, use repo_read_index, which is aware of
> > the proper paths for the worktree.
> > 
> > This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.
> > 
> > Signed-off-by: Thomas Gummerer 
> > ---
> >  repository.c | 11 +++
> >  repository.h |  2 ++
> >  revision.c   | 14 +-
> >  3 files changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/repository.c b/repository.c
> > index 928b1f553d..3c9bfbd1b8 100644
> > --- a/repository.c
> > +++ b/repository.c
> > @@ -2,6 +2,7 @@
> >  #include "repository.h"
> >  #include "config.h"
> >  #include "submodule-config.h"
> > +#include "worktree.h"
> >  
> >  /* The main repository */
> >  static struct repository the_repo = {
> > @@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char 
> > *gitdir, const char *worktree)
> > return -1;
> >  }
> >  
> > +/*
> > + * Initialize 'repo' based on the provided worktree
> > + * Return 0 upon success and a non-zero value upon failure.
> > + */
> > +int repo_worktree_init(struct repository *repo, struct worktree *worktree)
> > +{
> > +   return repo_init(repo, get_worktree_git_dir(worktree),
> > +worktree->path);
> 
> I still feel very unsettled about this and don't think its a good idea.
> get_worktree_git_dir depends implicitly on the global the_repository
> object and I would like to avoid relying on it for an initialization
> function like this.
> 
> > +}
> > +
> >  /*
> >   * Initialize 'submodule' as the submodule given by 'path' in parent 
> > repository
> >   * 'superproject'.
> > diff --git a/repository.h b/repository.h
> > index 7f5e24a0a2..2adeb05bf4 100644
> > --- a/repository.h
> > +++ b/repository.h
> > @@ -4,6 +4,7 @@
> >  struct config_set;
> >  struct index_state;
> >  struct submodule_cache;
> > +struct worktree;
> >  
> >  struct repository {
> > /* Environment */
> > @@ -87,6 +88,7 @@ extern struct repository *the_repository;
> >  extern void repo_set_gitdir(struct repository *repo, const char *path);
> >  extern void repo_set_worktree(struct repository *repo, const char *path);
> >  extern int repo_init(struct repository *repo, const char *gitdir, const 
> > char *worktree);
> > +extern int repo_worktree_init(struct repository *repo, struct worktree 
> > *worktree);
> >  extern int repo_submodule_init(struct repository *submodule,
> >struct repository *superproject,
> >const char *path);
> > diff --git a/revision.c b/revision.c
> > index e2e691dd5a..34e1e4b799 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -22,6 +22,7 @@
> >  #include "packfile.h"
> >  #include "worktree.h"
> >  #include "argv-array.h"
> > +#include "repository.h"
> >  
> >  volatile show_early_output_fn_t show_early_output;
> >  
> > @@ -1346,15 +1347,18 @@ void add_index_objects_to_pending(struct rev_info 
> > *revs, unsigned int flags)
> > worktrees = get_worktrees(0);
> > for (p = worktrees; *p; p++) {
> > struct worktree *wt = *p;
> > -   struct index_state istate = { NULL };
> > +   struct repository *repo;
> >  
> > +   repo = xmalloc(sizeof(struct repository));
> > if (wt->is_current)
> > continue; /* current index already taken care of */
> > +   if (repo_worktree_init(repo, wt))
> > +   BUG("couldn't initialize repository object from 
> > worktree");
> >  
> > -   if (read_index_from(,
> > -   worktree_git_path(wt, "index")) > 0)
> 
> Ok, after thinking this through a bit more I think a better approach may
> be to restructure the call to read_index_from to take in both an index
> file as well as the explicit gitdir to use when constructing a path to
> the sharedindex file.  That way you can fix this for worktrees and
> submodules without having to pass in a repository object to the logic
> which is reading an index file as well as avoiding needing to init a
> repository object for every

Re: [PATCH 5/5] diff: properly error out when combining multiple pickaxe options

Stefan Beller  writes:

;
> + count = 0;
> +
> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_S)
> + count++;
> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_G)
> + count++;
> + if (options->pickaxe_opts & DIFF_PICKAXE_KIND_OBJFIND)
> + count++;
> + if (count > 1)
> + die(_("-G, -S, --find-object are mutually exclusive"));

I thought the reason you defined pickaxe-kind bitmask was so that
you can mask this field to grab these (and only these) bits.
Once you have that mask, you should be able to use HAS_MULTI_BITS()
on the masked result without counting, no?

Re: [PATCH 3/5] diff: introduce DIFF_PICKAXE_KINDS_MASK

Stefan Beller  writes:

> Currently the check whether to perform pickaxing is done via checking
> `diffopt->pickaxe`, which contains the command line argument that we
> want to pickaxe for. Soon we'll introduce a new type of pickaxing, that
> will not store anything in the `.pickaxe` field, so let's migrate the
> check to be dependent on pickaxe_opts.
>
> It is not enough to just replace the check for pickaxe by pickaxe_opts,
> because flags might be set, but pickaxing was not requested ('-i').
> To cope with that, introduce a mask to check only for the bits indicating
> the modes of operation.

The resulting code after this series would leave a few "huh?" if it
were new code, but the series is not making anything worse, so take
this as just something noticed, not as something needs further work.

Because we do not allow "log -S -G", there is
no legitimate reason why they have to be a bit in the pickaxe_opts
flag word.  A single enum that says "We are doing pickaxe search and
_this_ is the kind of pickaxe search we are doing" would suffice,
i.e. the NULL-ness check of rev->diffopt.pickaxe string can be
replaced with a check of that enum field against PICKAXE_NONE or
something that signals us that no pickaxe is in effect.

On the other hand, if somebody comes up with a sensible way to
combine more than one pickaxe queries in a single traversal
(e.g. "log -S -S" might mean "find a
change that loses or gains  and  in the
same commit", or it may mean the same with "... or
"), then a more sensible data structure to represent
the pickaxe request may have been a list of struct, each of which
records the kind and the parameter (e.g. "-S" and ""
would be in a single struct, and "-S" and "" would be
in another, and these two are in the list that is diffopt->pickaxe).
The NULL-ness check of rev->diffopt.pickaxe string would be replaced
with a check of the length of that list.

In any case, this step looks sensible.

>
> Signed-off-by: Stefan Beller 
> ---
>  builtin/log.c  | 4 ++--
>  combine-diff.c | 2 +-
>  diff.c | 4 ++--
>  diff.h | 2 ++
>  revision.c | 2 +-
>  5 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 6c1fa896ad..bd6f2d1efb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -180,8 +180,8 @@ static void cmd_log_init_finish(int argc, const char 
> **argv, const char *prefix,
>   if (rev->show_notes)
>   init_display_notes(>notes_opt);
>  
> - if (rev->diffopt.pickaxe || rev->diffopt.filter ||
> - rev->diffopt.flags.follow_renames)
> + if ((rev->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK) ||
> + rev->diffopt.filter || rev->diffopt.flags.follow_renames)
>   rev->always_show_header = 0;
>  
>   if (source)
> diff --git a/combine-diff.c b/combine-diff.c
> index 2505de119a..bc08c4c5b1 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -1438,7 +1438,7 @@ void diff_tree_combined(const struct object_id *oid,
>   opt->flags.follow_renames   ||
>   opt->break_opt != -1||
>   opt->detect_rename  ||
> - opt->pickaxe||
> + (opt->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)   ||
>   opt->filter;
>  
>  
> diff --git a/diff.c b/diff.c
> index 0763e89263..5508745dc8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4173,7 +4173,7 @@ void diff_setup_done(struct diff_options *options)
>   /*
>* Also pickaxe would not work very well if you do not say recursive
>*/
> - if (options->pickaxe)
> + if (options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)
>   options->flags.recursive = 1;
>   /*
>* When patches are generated, submodules diffed against the work tree
> @@ -5777,7 +5777,7 @@ void diffcore_std(struct diff_options *options)
>   if (options->break_opt != -1)
>   diffcore_merge_broken();
>   }
> - if (options->pickaxe)
> + if (options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK)
>   diffcore_pickaxe(options);
>   if (options->orderfile)
>   diffcore_order(options->orderfile);
> diff --git a/diff.h b/diff.h
> index 8af1213684..9ec4f824fe 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -326,6 +326,8 @@ extern void diff_setup_done(struct diff_options *);
>  #define DIFF_PICKAXE_KIND_S  4 /* traditional plumbing counter */
>  #define DIFF_PICKAXE_KIND_G  8 /* grep in the patch */
>  
> +#define DIFF_PICKAXE_KINDS_MASK (DIFF_PICKAXE_KIND_S | DIFF_PICKAXE_KIND_G)
> +
>  #define DIFF_PICKAXE_IGNORE_CASE 32
>  
>  extern void diffcore_std(struct diff_options *);
> diff --git a/revision.c b/revision.c
> index ccf1d212ce..5d11ecaf27 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2407,7 +2407,7 @@ int setup_revisions(int argc, const char **argv, struct 
> rev_info *revs, struct s
>

Re: [PATCH 1/5] diff.h: Make pickaxe_opts an unsigned bit field

Stefan Beller  writes:

> This variable is used as a bit field[1], and as we are about to add more
> fields, indicate its usage as a bit field by making it unsigned.
>
> [1] containing the bits
>
> #define DIFF_PICKAXE_ALL  1
> #define DIFF_PICKAXE_REGEX2
> #define DIFF_PICKAXE_KIND_S   4
> #define DIFF_PICKAXE_KIND_G   8
>
> Signed-off-by: Stefan Beller 
> ---
>  diff.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Makes perfect sense.

>
> diff --git a/diff.h b/diff.h
> index 0fb18dd735..ea310f76fd 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -146,7 +146,7 @@ struct diff_options {
>   int skip_stat_unmatch;
>   int line_termination;
>   int output_format;
> - int pickaxe_opts;
> + unsigned pickaxe_opts;
>   int rename_score;
>   int rename_limit;
>   int needed_rename_limit;

[PATCH v3 4/5] status: support --no-ahead-behind in long format

From: Jeff Hostetler 

Teach long (normal) status format to respect the --no-ahead-behind
parameter and skip the possibly expensive ahead/behind computation
between the branch and the upstream.

Long status also respects "status.aheadBehind" config setting.

Signed-off-by: Jeff Hostetler 
---
 builtin/checkout.c   |  2 +-
 remote.c | 18 +-
 remote.h |  3 ++-
 t/t6040-tracking-info.sh | 47 +++
 wt-status.c  |  2 +-
 5 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd..655dac2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -605,7 +605,7 @@ static void report_tracking(struct branch_info *new)
struct strbuf sb = STRBUF_INIT;
struct branch *branch = branch_get(new->name);
 
-   if (!format_tracking_info(branch, ))
+   if (!format_tracking_info(branch, , AHEAD_BEHIND_FULL))
return;
fputs(sb.buf, stdout);
strbuf_release();
diff --git a/remote.c b/remote.c
index 32706bc..bbbe3e5 100644
--- a/remote.c
+++ b/remote.c
@@ -2068,15 +2068,16 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
 /*
  * Return true when there is anything to report, otherwise false.
  */
-int format_tracking_info(struct branch *branch, struct strbuf *sb)
+int format_tracking_info(struct branch *branch, struct strbuf *sb,
+enum ahead_behind_flags abf)
 {
-   int ours, theirs;
+   int ours, theirs, sti;
const char *full_base;
char *base;
int upstream_is_gone = 0;
 
-   if (stat_tracking_info(branch, , , _base,
-  AHEAD_BEHIND_FULL) < 0) {
+   sti = stat_tracking_info(branch, , , _base, abf);
+   if (sti < 0) {
if (!full_base)
return 0;
upstream_is_gone = 1;
@@ -2090,10 +2091,17 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
if (advice_status_hints)
strbuf_addstr(sb,
_("  (use \"git branch --unset-upstream\" to 
fixup)\n"));
-   } else if (!ours && !theirs) {
+   } else if (!sti) {
strbuf_addf(sb,
_("Your branch is up to date with '%s'.\n"),
base);
+   } else if (abf == AHEAD_BEHIND_QUICK) {
+   strbuf_addf(sb,
+   _("Your branch and '%s' refer to different 
commits.\n"),
+   base);
+   if (advice_status_hints)
+   strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
+   "git status --ahead-behind");
} else if (!theirs) {
strbuf_addf(sb,
Q_("Your branch is ahead of '%s' by %d commit.\n",
diff --git a/remote.h b/remote.h
index 27feb63..b2fa5cc 100644
--- a/remote.h
+++ b/remote.h
@@ -265,7 +265,8 @@ enum ahead_behind_flags {
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
   const char **upstream_name, enum ahead_behind_flags abf);
-int format_tracking_info(struct branch *branch, struct strbuf *sb);
+int format_tracking_info(struct branch *branch, struct strbuf *sb,
+enum ahead_behind_flags abf);
 
 struct ref *get_local_heads(void);
 /*
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 053dff3..febf63f 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -173,6 +173,53 @@ test_expect_success 'status.aheadbehind=false status -s -b 
(diverged from upstre
 '
 
 cat >expect <<\EOF
+On branch b1
+Your branch and 'origin/master' have diverged,
+and have 1 and 1 different commits each, respectively.
+EOF
+
+test_expect_success 'status --long --branch' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status --long -b | head -3
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+test_expect_success 'status --long --branch' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git -c status.aheadbehind=true status --long -b | head -3
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
+On branch b1
+Your branch and 'origin/master' refer to different commits.
+EOF
+
+test_expect_success 'status --long --branch --no-ahead-behind' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status --long -b --no-ahead-behind | head -2
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+test_expect_success 'status.aheadbehind=false status --long --branch' '
+   (
+   cd test &&
+   git

[PATCH v3 5/5] status: add status.aheadBehind value for porcelain output

From: Jeff Hostetler 

Add status.aheadBehind=2 value to enable --no-ahead-behind
for all formats (both porcelain and non-porcelain).  The
current boolean values only affect non-porcelain formats.

Signed-off-by: Jeff Hostetler 
---
 Documentation/config.txt |  5 +
 builtin/commit.c | 31 +--
 remote.h |  8 
 t/t6040-tracking-info.sh |  9 +
 t/t7064-wtstatus-pv2.sh  |  6 +-
 5 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index affb0d6..eaa1058 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3040,6 +3040,11 @@ status.aheadBehind::
--no-ahead-behind by default in linkgit:git-status[1] for
non-porcelain formats.  This setting is ignored by porcelain
formats for backwards compatibility.
++
+(EXPERIMENTAL) Set to 2 to allow both porcelain and non-porcelain
+formats to inherit --no-ahead-behind.  This may break backward
+compatibility for scripts using porcelain status formats and expecting
+ahead/behind information in the output.
 
 status.displayCommentPrefix::
If set to true, linkgit:git-status[1] will insert a comment
diff --git a/builtin/commit.c b/builtin/commit.c
index 416fe2c..194a6eb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1109,13 +1109,32 @@ static const char *read_commit_message(const char *name)
 static struct status_deferred_config {
enum wt_status_format status_format;
int show_branch;
-   enum ahead_behind_flags ahead_behind;
+   enum ahead_behind_config_flags ahead_behind_config;
 } status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
-1, /* unspecified */
AHEAD_BEHIND_UNSPECIFIED,
 };
 
+static inline enum ahead_behind_flags inherit_deferred_ab_flags(
+   int is_porcelain)
+{
+   switch (status_deferred_config.ahead_behind_config) {
+   case AHEAD_BEHIND_CONFIG_UNSPECIFIED:
+   case AHEAD_BEHIND_CONFIG_FULL:
+   return AHEAD_BEHIND_FULL;
+
+   case AHEAD_BEHIND_CONFIG_QUICK2:
+   return AHEAD_BEHIND_QUICK;
+
+   case AHEAD_BEHIND_CONFIG_QUICK:
+   return is_porcelain ? AHEAD_BEHIND_FULL : AHEAD_BEHIND_QUICK;
+
+   default: /* don't complain about bogus config settings */
+   return AHEAD_BEHIND_FULL;
+   }
+}
+
 static void finalize_deferred_config(struct wt_status *s)
 {
int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
@@ -1140,11 +1159,9 @@ static void finalize_deferred_config(struct wt_status *s)
if (s->show_branch < 0)
s->show_branch = 0;
 
-   if (use_deferred_config &&
-   s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
-   s->ahead_behind_flags = status_deferred_config.ahead_behind;
if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
-   s->ahead_behind_flags = AHEAD_BEHIND_FULL;
+   s->ahead_behind_flags =
+   inherit_deferred_ab_flags(!use_deferred_config);
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
@@ -1306,7 +1323,9 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
return 0;
}
if (!strcmp(k, "status.aheadbehind")) {
-   status_deferred_config.ahead_behind = git_config_bool(k, v);
+   int is_bool;
+   status_deferred_config.ahead_behind_config =
+   git_config_bool_or_int(k, v, _bool);
return 0;
}
if (!strcmp(k, "status.showstash")) {
diff --git a/remote.h b/remote.h
index b2fa5cc..bcf846a 100644
--- a/remote.h
+++ b/remote.h
@@ -262,6 +262,14 @@ enum ahead_behind_flags {
AHEAD_BEHIND_FULL=  1,  /* traditional a/b reporting */
 };
 
+/* Flags for status.aheadBehind values. */
+enum ahead_behind_config_flags {
+   AHEAD_BEHIND_CONFIG_UNSPECIFIED = -1,
+   AHEAD_BEHIND_CONFIG_QUICK   =  0, /* eq/neq for non-porcelain only 
*/
+   AHEAD_BEHIND_CONFIG_FULL=  1, /* a/b reporting for all formats 
*/
+   AHEAD_BEHIND_CONFIG_QUICK2  =  2, /* eq/neq for all formats */
+};
+
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
   const char **upstream_name, enum ahead_behind_flags abf);
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index febf63f..5003366 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -219,6 +219,15 @@ test_expect_success 'status.aheadbehind=false status 
--long --branch' '
test_i18ncmp expect actual
 '
 
+test_expect_success 'status.aheadbehind=2 status --long --branch' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git -c status.aheadbehind=false status

[PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format.

From: Jeff Hostetler 

Teach "git status" and "git commit" to accept "--no-ahead-behind"
and "--ahead-behind" arguments to request quick or full ahead/behind
reporting.

When "--no-ahead-behind" is given, the existing porcelain V2 line
"branch.ab x y" is replaced with a new "branch equal eq|neq" line.
This indicates that the branch and its upstream are or are not equal
without the expense of computing the full ahead/behind values.

Added "status.aheadBehind" config setting.  This is only used by
non-porcelain format for backward-compatibility.

Signed-off-by: Jeff Hostetler 
---
 Documentation/config.txt |  6 
 Documentation/git-status.txt |  5 
 builtin/commit.c | 18 +++-
 remote.c |  2 ++
 remote.h |  5 ++--
 t/t7064-wtstatus-pv2.sh  | 69 
 wt-status.c  | 27 +
 wt-status.h  |  2 ++
 8 files changed, 125 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfa..affb0d6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3035,6 +3035,12 @@ status.branch::
Set to true to enable --branch by default in linkgit:git-status[1].
The option --no-branch takes precedence over this variable.
 
+status.aheadBehind::
+   Set to true to enable --ahead-behind and to false to enable
+   --no-ahead-behind by default in linkgit:git-status[1] for
+   non-porcelain formats.  This setting is ignored by porcelain
+   formats for backwards compatibility.
+
 status.displayCommentPrefix::
If set to true, linkgit:git-status[1] will insert a comment
prefix before each output line (starting with
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a..603bf40 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -111,6 +111,11 @@ configuration variable documented in linkgit:git-config[1].
without options are equivalent to 'always' and 'never'
respectively.
 
+--ahead-behind::
+--no-ahead-behind::
+   Display or do not display detailed ahead/behind counts for the
+   branch relative to its upstream branch.  Defaults to true.
+
 ...::
See the 'pathspec' entry in linkgit:gitglossary[7].
 
diff --git a/builtin/commit.c b/builtin/commit.c
index be370f6..416fe2c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1109,9 +1109,11 @@ static const char *read_commit_message(const char *name)
 static struct status_deferred_config {
enum wt_status_format status_format;
int show_branch;
+   enum ahead_behind_flags ahead_behind;
 } status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
-   -1 /* unspecified */
+   -1, /* unspecified */
+   AHEAD_BEHIND_UNSPECIFIED,
 };
 
 static void finalize_deferred_config(struct wt_status *s)
@@ -1137,6 +1139,12 @@ static void finalize_deferred_config(struct wt_status *s)
s->show_branch = status_deferred_config.show_branch;
if (s->show_branch < 0)
s->show_branch = 0;
+
+   if (use_deferred_config &&
+   s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+   s->ahead_behind_flags = status_deferred_config.ahead_behind;
+   if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+   s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
@@ -1297,6 +1305,10 @@ static int git_status_config(const char *k, const char 
*v, void *cb)
status_deferred_config.show_branch = git_config_bool(k, v);
return 0;
}
+   if (!strcmp(k, "status.aheadbehind")) {
+   status_deferred_config.ahead_behind = git_config_bool(k, v);
+   return 0;
+   }
if (!strcmp(k, "status.showstash")) {
s->show_stash = git_config_bool(k, v);
return 0;
@@ -1351,6 +1363,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 N_("show branch information")),
OPT_BOOL(0, "show-stash", _stash,
 N_("show stash information")),
+   OPT_BOOL(0, "ahead-behind", _behind_flags,
+N_("compute full ahead/behind values")),
{ OPTION_CALLBACK, 0, "porcelain", _format,
  N_("version"), N_("machine-readable output"),
  PARSE_OPT_OPTARG, opt_parse_porcelain },
@@ -1628,6 +1642,8 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
OPT_SET_INT(0, "short", _format, N_("show status 
concisely"),
STATUS_FORMAT_SHORT),
OPT_BOOL(0, "branch", _branch, N_("show branch 
information")),
+   OPT_BOOL(0, "ahead-behind",

[PATCH v3 3/5] status: update short status to respect --no-ahead-behind

From: Jeff Hostetler 

Teach "git status --short --branch" to respect "--no-ahead-behind"
parameter to skip computing ahead/behind counts for the branch and
its upstream and just report '[different]'.

Short status also respect the "status.aheadBehind" config setting.

Signed-off-by: Jeff Hostetler 
---
 t/t6040-tracking-info.sh | 26 ++
 wt-status.c  | 11 +++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 8f17fd9..053dff3 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -147,6 +147,32 @@ test_expect_success 'status -s -b (diverged from 
upstream)' '
 '
 
 cat >expect <<\EOF
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status -s -b --no-ahead-behind | head -1
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status.aheadbehind=false status -s -b (diverged from 
upstream)' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git -c status.aheadbehind=false status -s -b | head -1
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index 3959d31..df6cc33 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1766,7 +1766,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
const char *base;
char *short_base;
const char *branch_name;
-   int num_ours, num_theirs;
+   int num_ours, num_theirs, sti;
int upstream_is_gone = 0;
 
color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## ");
@@ -1792,8 +1792,9 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
 
color_fprintf(s->fp, branch_color_local, "%s", branch_name);
 
-   if (stat_tracking_info(branch, _ours, _theirs, ,
-  AHEAD_BEHIND_FULL) < 0) {
+   sti = stat_tracking_info(branch, _ours, _theirs, ,
+s->ahead_behind_flags);
+   if (sti < 0) {
if (!base)
goto conclude;
 
@@ -1805,12 +1806,14 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
color_fprintf(s->fp, branch_color_remote, "%s", short_base);
free(short_base);
 
-   if (!upstream_is_gone && !num_ours && !num_theirs)
+   if (!upstream_is_gone && !sti)
goto conclude;
 
color_fprintf(s->fp, header_color, " [");
if (upstream_is_gone) {
color_fprintf(s->fp, header_color, LABEL(N_("gone")));
+   } else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK) {
+   color_fprintf(s->fp, header_color, LABEL(N_("different")));
} else if (!num_ours) {
color_fprintf(s->fp, header_color, LABEL(N_("behind ")));
color_fprintf(s->fp, branch_color_remote, "%d", num_theirs);
-- 
2.9.3

[PATCH v3 0/5] Add --no-ahead-behind to status

From: Jeff Hostetler 

This is version 3 of my patch series to avoid expensive
ahead/behind calculations in status.  This version tries
to address most of the comments in V2.

I've switched back to a "status.aheadBehind" config setting
rather than in "core.*".  This has been better integrated
with the existing status_deferred_config mechanism in
builtin/commit.c and lets both status and commit inherit it.

Config values of true and false control non-porcelain formats
for compatibility reasons as previously discussed.  In the
last commit I added a new value of 2 for the config setting
to allow porcelain formats to inherit the new setting.  I've
marked this experimental for now or so that we can discuss
it.

Jeff Hostetler (5):
  stat_tracking_info: return +1 when branches not equal
  status: add --[no-]ahead-behind to status and commit for V2 format.
  status: update short status to respect --no-ahead-behind
  status: support --no-ahead-behind in long format
  status: add status.aheadBehind value for porcelain output

 Documentation/config.txt | 11 ++
 Documentation/git-status.txt |  5 +++
 builtin/checkout.c   |  2 +-
 builtin/commit.c | 37 +++-
 ref-filter.c |  8 ++---
 remote.c | 42 +--
 remote.h | 20 +--
 t/t6040-tracking-info.sh | 82 
 t/t7064-wtstatus-pv2.sh  | 73 +++
 wt-status.c  | 38 +++-
 wt-status.h  |  2 ++
 11 files changed, 292 insertions(+), 28 deletions(-)

-- 
2.9.3

[PATCH v3 1/5] stat_tracking_info: return +1 when branches not equal

From: Jeff Hostetler 

Extend stat_tracking_info() to return +1 when branches are not equal and to
take a new "enum ahead_behind_flags" argument to allow skipping the (possibly
expensive) ahead/behind computation.

This will be used in the next commit to allow "git status" to avoid full
ahead/behind calculations for performance reasons.

Signed-off-by: Jeff Hostetler 
---
 ref-filter.c |  8 
 remote.c | 26 ++
 remote.h |  8 +++-
 wt-status.c  |  6 --
 4 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e728b15..23bcdc4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1238,8 +1238,8 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
if (atom->u.remote_ref.option == RR_REF)
*s = show_ref(>u.remote_ref.refname, refname);
else if (atom->u.remote_ref.option == RR_TRACK) {
-   if (stat_tracking_info(branch, _ours,
-  _theirs, NULL)) {
+   if (stat_tracking_info(branch, _ours, _theirs,
+  NULL, AHEAD_BEHIND_FULL) < 0) {
*s = xstrdup(msgs.gone);
} else if (!num_ours && !num_theirs)
*s = "";
@@ -1256,8 +1256,8 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
free((void *)to_free);
}
} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
-   if (stat_tracking_info(branch, _ours,
-  _theirs, NULL))
+   if (stat_tracking_info(branch, _ours, _theirs,
+  NULL, AHEAD_BEHIND_FULL) < 0)
return;
 
if (!num_ours && !num_theirs)
diff --git a/remote.c b/remote.c
index b220f0d..ca5a416 100644
--- a/remote.c
+++ b/remote.c
@@ -1977,16 +1977,23 @@ int ref_newer(const struct object_id *new_oid, const 
struct object_id *old_oid)
 }
 
 /*
- * Compare a branch with its upstream, and save their differences (number
- * of commits) in *num_ours and *num_theirs. The name of the upstream branch
- * (or NULL if no upstream is defined) is returned via *upstream_name, if it
- * is not itself NULL.
+ * Lookup the upstream branch for the given branch and if present, optionally
+ * compute the commit ahead/behind values for the pair.
+ *
+ * If abf is AHEAD_BEHIND_FULL, compute the full ahead/behind and return the
+ * counts in *num_ours and *num_theirs.  If abf is AHEAD_BEHIND_QUICK, skip
+ * the (potentially expensive) a/b computation (*num_ours and *num_theirs are
+ * left undefined).
+ *
+ * The name of the upstream branch (or NULL if no upstream is defined) is
+ * returned via *upstream_name, if it is not itself NULL.
  *
  * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
- * upstream defined, or ref does not exist), 0 otherwise.
+ * upstream defined, or ref does not exist).  Returns 0 if the commits are
+ * identical.  Returns 1 if commits are different.
  */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
-  const char **upstream_name)
+  const char **upstream_name, enum ahead_behind_flags abf)
 {
struct object_id oid;
struct commit *ours, *theirs;
@@ -2019,6 +2026,8 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
*num_theirs = *num_ours = 0;
return 0;
}
+   if (abf == AHEAD_BEHIND_QUICK)
+   return 1;
 
/* Run "rev-list --left-right ours...theirs" internally... */
argv_array_push(, ""); /* ignored */
@@ -2051,7 +2060,7 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
clear_commit_marks(theirs, ALL_REV_FLAGS);
 
argv_array_clear();
-   return 0;
+   return 1;
 }
 
 /*
@@ -2064,7 +2073,8 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
char *base;
int upstream_is_gone = 0;
 
-   if (stat_tracking_info(branch, , , _base) < 0) {
+   if (stat_tracking_info(branch, , , _base,
+  AHEAD_BEHIND_FULL) < 0) {
if (!full_base)
return 0;
upstream_is_gone = 1;
diff --git a/remote.h b/remote.h
index 2ecf4c8..00932f5 100644
--- a/remote.h
+++ b/remote.h
@@ -255,9 +255,15 @@ enum match_refs_flags {
MATCH_REFS_FOLLOW_TAGS  = (1 << 3)
 };
 
+/* Flags for --ahead-behind option. */
+enum ahead_behind_flags {
+   AHEAD_BEHIND_QUICK = 0,  /* just eq/neq reporting */
+   AHEAD_BEHIND_FULL  = 1,  /* traditional a/b reporting */
+};
+
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
-

Hay

2018-01-03 Thread Financial Services

Loan Offer at 3% Lowest Rate Get Now.

Re: [PATCH 06/26] transport: use get_refs_via_connect to get refs

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> Remove code duplication and use the existing 'get_refs_via_connect()'
> function to retrieve a remote's heads in 'fetch_refs_via_pack()' and
> 'git_transport_push()'.
>
> Signed-off-by: Brandon Williams 

Reviewed-by: Stefan Beller 

> ---
>  transport.c | 18 --
>  1 file changed, 4 insertions(+), 14 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index fc802260f..8e8779096 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -230,12 +230,8 @@ static int fetch_refs_via_pack(struct transport 
> *transport,
> args.cloning = transport->cloning;
> args.update_shallow = data->options.update_shallow;
>
> -   if (!data->got_remote_heads) {
> -   connect_setup(transport, 0);
> -   get_remote_heads(data->fd[0], NULL, 0, _tmp, 0,
> -NULL, >shallow);
> -   data->got_remote_heads = 1;
> -   }
> +   if (!data->got_remote_heads)
> +   refs_tmp = get_refs_via_connect(transport, 0);
>
> refs = fetch_pack(, data->fd, data->conn,
>   refs_tmp ? refs_tmp : transport->remote_refs,
> @@ -541,14 +537,8 @@ static int git_transport_push(struct transport 
> *transport, struct ref *remote_re
> struct send_pack_args args;
> int ret;
>
> -   if (!data->got_remote_heads) {
> -   struct ref *tmp_refs;
> -   connect_setup(transport, 1);
> -
> -   get_remote_heads(data->fd[0], NULL, 0, _refs, REF_NORMAL,
> -NULL, >shallow);
> -   data->got_remote_heads = 1;
> -   }
> +   if (!data->got_remote_heads)
> +   get_refs_via_connect(transport, 1);
>
> memset(, 0, sizeof(args));
> args.send_mirror = !!(flags & TRANSPORT_PUSH_MIRROR);
> --
> 2.15.1.620.gb9897f4670-goog
>

Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes

On Wed, Jan 3, 2018 at 12:49 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Thanks Junio for review of this series!
>> The only change in this version of the series is
>>
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
>> update |= CE_UPDATE;
>> }
>> if (S_ISGITLINK(old->ce_mode) && should_update_submodules() 
>> &&
>> -   !verify_uptodate(old, o))
>> +   o->update && !verify_uptodate(old, o))
>> update |= CE_UPDATE;
>> add_entry(o, old, update, 0);
>>
>
> Sounds OK.
>
> I wonder why o->update is not at the very beginning of the &&-chain,
> though.  After all, the one above this addition begins with o->reset
> && o->update *not* because of the performance concern, but primarily
> due to logic flow.  I.e. "if we are resetting and updating the
> working tree, then..." comes first before saying "we may need to
> flip CE_UPDATE bit in update variable if the file in the working
> tree is not up to date and it is within a narrow checkout area".

It shows that I work too much with submodules. ;)
"If we have a submodule and ..." seemed to be the important
part when writing the patch.

> Of course, because verify_uptodate() is rather expensive, checking
> o->update before that makes sense from micro-optimization's point of
> view, too.

I would think S_ISGITLINK, should_update_submodules as well
as o->update are all on the same order of magnitude of costs
(some couple number of operations)  when
compared to verify_uptodate (spawning processes),
so as long as verify_uptodate goes last we'd be fine.

>
> So after thinking aloud like the above, I am reasonably sure that
> you want to check o->update as the very first thing in this new if
> statement.

Thanks for double checking and thinking about the code base with
a less submodule centric point of view.

Mind to squash it locally or want me to resend?
For a resend I'll wait a couple of days to see if there are more
comments needing to be addressed.


>
>> v2:
>> I dropped the patch to `same()` as I realized we only need to fix the
>> oneway_merge function, the others (two, three way merge) are fine as
>> they have the checks already in place.
>
> This is a bit flawed argument, no?  Checking working tree paths
> unconditionally in same(), which does not even know if we are
> touching the working tree paths, is broken.  Unless "they have the
> checks already in place" refers to checks that bypasses calls to
> same() when we are not touching working tree paths, that is, but
> obviously that is not what is going on.
>
> Will queue.  Thanks for working on this.
>
>

Re: [ANNOUNCE] Git v2.16.0-rc0

Jonathan Nieder  writes:

> It's good you caught this flaw in the detection.  Would something like
> the following make sense?  If so, I can resend with a commit message
> and tests tomorrow or the day after.

So the idea is to keep the 'simple' for implementations that do not
support OpenSSH options, but declare that the auto-detection was
overly conservative and assume that -4/6/p are supported by
everybody?

This change means that those who were meant to be helped by the
original change, that introduced 'simple' and made the (overly
conservative) auto-detection, would now have to explicitly set
ssh.variant to simple, because otherwise they will be passed one of
these three options.  Am I reading the intention of the change
correctly?  If so, I tend to agree that it is lesser of the two
evils to make sure things continue to work for older openssh users
with their current setting like this patch does, even with the cost
of telling users with implementations that do not honor -4/6/p to
set things up manually, I guess.

Thanks, both, for digging this issue through.

>
> diff --git i/Documentation/config.txt w/Documentation/config.txt
> index 64c1dbba94..75eafd8db6 100644
> --- i/Documentation/config.txt
> +++ w/Documentation/config.txt
> @@ -2118,8 +2118,8 @@ ssh.variant::
>   unrecognized, Git will attempt to detect support of OpenSSH
>   options by first invoking the configured SSH command with the
>   `-G` (print configuration) option and will subsequently use
> - OpenSSH options (if that is successful) or no options besides
> - the host and remote command (if it fails).
> + OpenSSH options if that is successful or a conservative set of
> + OpenSSH-style options if it fails.
>  +
>  The config variable `ssh.variant` can be set to override this detection.
>  Valid values are `ssh` (to use OpenSSH options), `plink`, `putty`,
> diff --git i/connect.c w/connect.c
> index c3a014c5ba..3784c2be53 100644
> --- i/connect.c
> +++ w/connect.c
> @@ -941,10 +941,9 @@ static void push_ssh_options(struct argv_array *args, 
> struct argv_array *env,
>  
>   if (flags & CONNECT_IPV4) {
>   switch (variant) {
> - case VARIANT_AUTO:
> - BUG("VARIANT_AUTO passed to push_ssh_options");
>   case VARIANT_SIMPLE:
>   die("ssh variant 'simple' does not support -4");
> + case VARIANT_AUTO:
>   case VARIANT_SSH:
>   case VARIANT_PLINK:
>   case VARIANT_PUTTY:
> @@ -953,10 +952,9 @@ static void push_ssh_options(struct argv_array *args, 
> struct argv_array *env,
>   }
>   } else if (flags & CONNECT_IPV6) {
>   switch (variant) {
> - case VARIANT_AUTO:
> - BUG("VARIANT_AUTO passed to push_ssh_options");
>   case VARIANT_SIMPLE:
>   die("ssh variant 'simple' does not support -6");
> + case VARIANT_AUTO:
>   case VARIANT_SSH:
>   case VARIANT_PLINK:
>   case VARIANT_PUTTY:
> @@ -970,10 +968,9 @@ static void push_ssh_options(struct argv_array *args, 
> struct argv_array *env,
>  
>   if (port) {
>   switch (variant) {
> - case VARIANT_AUTO:
> - BUG("VARIANT_AUTO passed to push_ssh_options");
>   case VARIANT_SIMPLE:
>   die("ssh variant 'simple' does not support setting 
> port");
> + case VARIANT_AUTO:
>   case VARIANT_SSH:
>   argv_array_push(args, "-p");
>   break;
> @@ -1026,7 +1023,7 @@ static void fill_ssh_args(struct child_process *conn, 
> const char *ssh_host,
>VARIANT_SSH, port, flags);
>   argv_array_push(, ssh_host);
>  
> - variant = run_command() ? VARIANT_SIMPLE : VARIANT_SSH;
> + variant = run_command() ? VARIANT_AUTO : VARIANT_SSH;
>   }
>  
>   argv_array_push(>args, ssh);
> diff --git i/t/t5601-clone.sh w/t/t5601-clone.sh
> index 0f895478f0..0224edc85b 100755
> --- i/t/t5601-clone.sh
> +++ w/t/t5601-clone.sh
> @@ -365,6 +365,11 @@ test_expect_success 'OpenSSH variant passes -4' '
>   expect_ssh "-4 -p 123" myhost src
>  '
>  
> +test_expect_success 'OpenSSH passes GIT_PROTOCOL envvar' '
> + git -c protocol.version=1 clone [myhost:123]:src ssh-v1-clone &&
> + expect_ssh "-o SendEnv=GIT_PROTOCOL -p 123" myhost src
> +'
> +
>  test_expect_success 'variant can be overridden' '
>   copy_ssh_wrapper_as "$TRASH_DIRECTORY/putty" &&
>   git -c ssh.variant=putty clone -4 "[myhost:123]:src" ssh-putty-clone &&
> @@ -377,19 +382,32 @@ test_expect_success 'variant=auto picks based on 
> basename' '
>   expect_ssh "-4 -P 123" myhost src
>  '
>  
> -test_expect_success 'simple does not support -4/-6' '
> +test_expect_success 'variant=simple does not support -4/-6' '
>

Re: [PATCH v5 00/34] Add directory rename detection to git

2018-01-03 Thread Elijah Newren

On Wed, Jan 3, 2018 at 2:57 AM, Johannes Sixt  wrote:

> I tested the series on Windows recently. It requires the patch below.
> I don't know whether this is indicating some portability issues of grep
> (^ being used in the middle of a RE instead of at the very beginning) or
> just a quirk in my setup.

Thanks for testing it out.  What version of Windows were you running
on?  With cygwin or without?  I tested previously on cygwin (I think
on Windows Server 2012??) and got all the tests passing there,
eventually[1].  I'm not sure I can find access to any other Windows
systems, but I'd be happy to take a look if I can.

[1] 
https://public-inbox.org/git/cabpp-bej6-mry0ocz1wwetrtg_iehkzodcuon_puukvywau...@mail.gmail.com/

The need to backslash escape a caret for a literal match when it
appears in the middle of the string makes sense.  Thanks for sending
along the patch.  Would you prefer I squashed it into the series
(still sitting in 'pu'), or keep your patch separate?  I'm fine with
either, I'm just unsure the protocol here.

> But it still does not pass the test suite because the system does not
> like file names such as y/c~HEAD:
>
> ++ grep 'Refusing to lose dirty file at z/c' out
> Refusing to lose dirty file at z/c
> ++ grep -q stuff x/b y/a y/c y/c~HEAD z/c
> grep: y/c: Invalid request code
> error: last command exited with $?=2
> not ok 94 - 11d-check: Avoid losing not-uptodate with rename + D/F conflict

This is exceptionally odd.  The actual line from the testsuite was
  grep -q stuff */*

which suggests your shell is both doing the pathname expansion and
treating the resulting filename not as a string but as something to be
interpreted that happens to have some kind of special
characters/commands, and then choking on the result.  Super weird.  I
could probably work around this by just running
  grep -q stuff z/c

I think I had the asterisks in there because I was thinking in terms
of directory rename detection potentially moving the file, but that's
probably just overkill.  Does the test pass for you with that change?
(If so, there are also two similar tests that I'd need to make similar
changes to.)

However, although that might fix this particular case, it suggests
some fragility of the tests and filenames for whatever system you
happen to be using.  merge-recursive.c's unique_path has created
filenames with tilde's in them for many years, it may just be that I'm
the first to use the resulting file in combination with grep to ensure
the contents are as we expect.  There may be other issues lurking
(even if not yet appearing in the testsuite) for your system when
dealing with merge conflicts.

Thanks,
Elijah

>
>  8< 
> From: Johannes Sixt 
> Date: Fri, 22 Dec 2017 09:33:13 +0100
> Subject: [PATCH] fixup directory rename tests
>
> Signed-off-by: Johannes Sixt 
> ---
>  t/t6043-merge-rename-directories.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t6043-merge-rename-directories.sh 
> b/t/t6043-merge-rename-directories.sh
> index f0af66b8a9..b8cd428341 100755
> --- a/t/t6043-merge-rename-directories.sh
> +++ b/t/t6043-merge-rename-directories.sh
> @@ -2940,8 +2940,8 @@ test_expect_success '10b-check: Overwrite untracked 
> with dir rename + delete' '
> echo contents >y/e &&
>
> test_must_fail git merge -s recursive B^0 >out 2>err &&
> -   test_i18ngrep "CONFLICT (rename/delete).*Version B^0 of y/d 
> left in tree at y/d~B^0" out &&
> -   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
> writing to y/e~B^0 instead" out &&
> +   test_i18ngrep "CONFLICT (rename/delete).*Version B\^0 of y/d 
> left in tree at y/d~B\^0" out &&
> +   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
> writing to y/e~B\^0 instead" out &&
>
> test 3 -eq $(git ls-files -s | wc -l) &&
> test 2 -eq $(git ls-files -u | wc -l) &&
> @@ -3010,7 +3010,7 @@ test_expect_success '10c-check: Overwrite untracked 
> with dir rename/rename(1to2)
>
> test_must_fail git merge -s recursive B^0 >out 2>err &&
> test_i18ngrep "CONFLICT (rename/rename)" out &&
> -   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
> as y/c~B^0 instead" out &&
> +   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
> as y/c~B\^0 instead" out &&
>
> test 6 -eq $(git ls-files -s | wc -l) &&
> test 3 -eq $(git ls-files -u | wc -l) &&
> --
> 2.14.2.808.g3bc32f2729

Bug report: git clone with dest

2018-01-03 Thread Isaac Shabtay

Hello,
Target directory is deleted on clone failures.

Steps to reproduce, for example on Windows:

cd /d %TEMP%
mkdir dest
git clone https://some-fake-url/whatever-makes-git-clone-fail dest

Of course, the clone will fail as it should. But looks like the Git
client also ends up deleting the "dest" directory.

This shouldn't happen.

[PATCH v2 4/5] update-index doc: note a fixed bug in the untracked cache

Document the bug tested for in my "status: add a failing test showing
a core.untrackedCache bug" and fixed in Duy's "dir.c: fix missing dir
invalidation in untracked code".

Since this is very likely something others will encounter in the
future on older versions, and it's not obvious how to fix it let's
document both that it exists, and how to "fix" it with a one-off
command.

As noted in that commit, even though this bug gets the untracked cache
into a bad state, we have not yet found a case where this is user
visible, and thus it makes sense for these docs to focus on the
symlink case only.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-update-index.txt | 16 
 1 file changed, 16 insertions(+)

diff --git a/Documentation/git-update-index.txt 
b/Documentation/git-update-index.txt
index bdb0342593..128e0c671f 100644
--- a/Documentation/git-update-index.txt
+++ b/Documentation/git-update-index.txt
@@ -464,6 +464,22 @@ command reads the index; while when 
`--[no-|force-]untracked-cache`
 are used, the untracked cache is immediately added to or removed from
 the index.
 
+Before 2.16, the untracked cache had a bug where replacing a directory
+with a symlink to another directory could cause it to incorrectly show
+files tracked by git as untracked. See the "status: add a failing test
+showing a core.untrackedCache bug" commit to git.git. A workaround for
+that was (and this might work for other undiscoverd bugs in the
+future):
+
+
+$ git -c core.untrackedCache=false status
+
+
+This bug has also been shown to affect non-symlink cases of replacing
+a directory with a file when it comes to the internal structures of
+the untracked cache, but no case has been found where this resulted in
+wrong "git status" output.
+
 File System Monitor
 ---
 
-- 
2.15.1.424.g9478a66081

Re: [PATCHv3 0/5] Fix --recurse-submodules for submodule worktree changes

Stefan Beller  writes:

> Thanks Junio for review of this series!
> The only change in this version of the series is
>
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2140,7 +2140,7 @@ int oneway_merge(const struct cache_entry * const *src,
> update |= CE_UPDATE;
> }
> if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
> -   !verify_uptodate(old, o))
> +   o->update && !verify_uptodate(old, o))
> update |= CE_UPDATE;
> add_entry(o, old, update, 0);
>

Sounds OK.  

I wonder why o->update is not at the very beginning of the &&-chain,
though.  After all, the one above this addition begins with o->reset
&& o->update *not* because of the performance concern, but primarily
due to logic flow.  I.e. "if we are resetting and updating the
working tree, then..." comes first before saying "we may need to
flip CE_UPDATE bit in update variable if the file in the working
tree is not up to date and it is within a narrow checkout area".

Of course, because verify_uptodate() is rather expensive, checking
o->update before that makes sense from micro-optimization's point of
view, too.

So after thinking aloud like the above, I am reasonably sure that
you want to check o->update as the very first thing in this new if
statement.

> v2:
> I dropped the patch to `same()` as I realized we only need to fix the
> oneway_merge function, the others (two, three way merge) are fine as
> they have the checks already in place.

This is a bit flawed argument, no?  Checking working tree paths
unconditionally in same(), which does not even know if we are
touching the working tree paths, is broken.  Unless "they have the
checks already in place" refers to checks that bypasses calls to
same() when we are not touching working tree paths, that is, but
obviously that is not what is going on.

Will queue.  Thanks for working on this.

[PATCH v2 2/5] dir.c: avoid stat() in valid_cached_dir()

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

stat() may follow a symlink and return stat data of the link's target
instead of the link itself. We are concerned about the link itself.

It's kind of hard to demonstrate the bug. I think when path->buf is a
symlink, we most likely find that its target's stat data does not
match our cached one, which means we ignore the cache and fall back to
slow path.

This is performance issue, not correctness (though we could still
catch it by verifying test-dump-untracked-cache. The less unlikely
case is, link target stat data matches the cached version and we
incorrectly go fast path, ignoring real data on disk. A test for this
may involve manipulating stat data, which may be not portable.

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

diff --git a/dir.c b/dir.c
index 7c4b45e30e..edcb7bb462 100644
--- a/dir.c
+++ b/dir.c
@@ -1809,7 +1809,7 @@ static int valid_cached_dir(struct dir_struct *dir,
 */
refresh_fsmonitor(istate);
if (!(dir->untracked->use_fsmonitor && untracked->valid)) {
-   if (stat(path->len ? path->buf : ".", )) {
+   if (lstat(path->len ? path->buf : ".", )) {
invalidate_directory(dir->untracked, untracked);
memset(>stat_data, 0, 
sizeof(untracked->stat_data));
return 0;
-- 
2.15.1.424.g9478a66081

[PATCH v2 5/5] dir.c: stop ignoring opendir() error in open_cached_dir()

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

A follow-up to the recently fixed bugs in the untracked
invalidation. If opendir() fails it should show a warning, perhaps
this should die, but if this ever happens the error is probably
recoverable for the user, and dying would just make things worse.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 dir.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index 163ca69df0..a605e01692 100644
--- a/dir.c
+++ b/dir.c
@@ -1857,17 +1857,22 @@ static int open_cached_dir(struct cached_dir *cdir,
   struct strbuf *path,
   int check_only)
 {
+   const char *c_path;
+
memset(cdir, 0, sizeof(*cdir));
cdir->untracked = untracked;
if (valid_cached_dir(dir, untracked, istate, path, check_only))
return 0;
-   cdir->fdir = opendir(path->len ? path->buf : ".");
+   c_path = path->len ? path->buf : ".";
+   cdir->fdir = opendir(c_path);
if (dir->untracked) {
invalidate_directory(dir->untracked, untracked);
dir->untracked->dir_opened++;
}
-   if (!cdir->fdir)
+   if (!cdir->fdir) {
+   warning_errno(_("could not open directory '%s'"), c_path);
return -1;
+   }
return 0;
 }
 
-- 
2.15.1.424.g9478a66081

[PATCH v2 1/5] status: add a failing test showing a core.untrackedCache bug

The untracked cache gets confused when a directory is swapped out for
a file. It is easiest to reproduce this by swapping out a directory
with a symlink to another directory, and as the tests show the symlink
case is the only case we've found where "git status" will subsequently
report incorrect information, even though it's possible to otherwise
get the untracked cache into a state where its internal data
structures don't reflect reality.

In the symlink case, whatever files are inside the target of the
symlink will be incorrectly shown as untracked. This issue does not
happen if the symlink links to another file, only if it links to
another directory.

A stand-alone testcase for copying into a terminal:

(
rm -rf /tmp/testrepo &&
git init /tmp/testrepo &&
cd /tmp/testrepo &&
mkdir x y &&
touch x/a y/b &&
git add x/a y/b &&
git commit -msnap &&
git rm -rf y &&
ln -s x y &&
git add y &&
git commit -msnap2 &&
git checkout HEAD~ &&
git status &&
git checkout master &&
sleep 1 &&
git status &&
git status
)

This will incorrectly show y/a as an untracked file. Both the "git
status" call right before "git checkout master" and the "sleep 1"
after the "checkout master" are needed to reproduce this, presumably
due to the untracked cache tracking on the basis of cached whole
seconds from stat(2).

When git gets into this state, a workaround to fix it is to issue a
one-off:

git -c core.untrackedCache=false status

For the non-symlink case, the bug is that the output of
test-dump-untracked-cache should not include:

   /one/  recurse valid

It being in the output implies that cached traversal of root includes
the directory "one" which does not exist on disk anymore.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t7063-status-untracked-cache.sh | 87 +++
 1 file changed, 87 insertions(+)

diff --git a/t/t7063-status-untracked-cache.sh 
b/t/t7063-status-untracked-cache.sh
index e5fb892f95..dba7f50bbb 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -22,6 +22,12 @@ avoid_racy() {
sleep 1
 }
 
+status_is_clean() {
+   >../status.expect &&
+   git status --porcelain >../status.actual &&
+   test_cmp ../status.expect ../status.actual
+}
+
 test_lazy_prereq UNTRACKED_CACHE '
{ git update-index --test-untracked-cache; ret=$?; } &&
test $ret -ne 1
@@ -683,4 +689,85 @@ test_expect_success 'untracked cache survives a commit' '
test_cmp ../before ../after
 '
 
+test_expect_success 'teardown worktree' '
+   cd ..
+'
+
+test_expect_success SYMLINKS 'setup worktree for symlink test' '
+   git init worktree-symlink &&
+   cd worktree-symlink &&
+   git config core.untrackedCache true &&
+   mkdir one two &&
+   touch one/file two/file &&
+   git add one/file two/file &&
+   git commit -m"first commit" &&
+   git rm -rf one &&
+   ln -s two one &&
+   git add one &&
+   git commit -m"second commit"
+'
+
+test_expect_failure SYMLINKS '"status" after symlink replacement should be 
clean with UC=true' '
+   git checkout HEAD~ &&
+   status_is_clean &&
+   status_is_clean &&
+   git checkout master &&
+   avoid_racy &&
+   status_is_clean &&
+   status_is_clean
+'
+
+test_expect_success SYMLINKS '"status" after symlink replacement should be 
clean with UC=false' '
+   git config core.untrackedCache false &&
+   git checkout HEAD~ &&
+   status_is_clean &&
+   status_is_clean &&
+   git checkout master &&
+   avoid_racy &&
+   status_is_clean &&
+   status_is_clean
+'
+
+test_expect_success 'setup worktree for non-symlink test' '
+   git init worktree-non-symlink &&
+   cd worktree-non-symlink &&
+   git config core.untrackedCache true &&
+   mkdir one two &&
+   touch one/file two/file &&
+   git add one/file two/file &&
+   git commit -m"first commit" &&
+   git rm -rf one &&
+   cp two/file one &&
+   git add one &&
+   git commit -m"second commit"
+'
+
+test_expect_failure '"status" after file replacement should be clean with 
UC=true' '
+   git checkout HEAD~ &&
+   status_is_clean &&
+   status_is_clean &&
+   git checkout master &&
+   avoid_racy &&
+   status_is_clean &&
+   test-dump-untracked-cache >../actual &&
+   grep -F "recurse valid" ../actual >../actual.grep &&
+   cat >../expect.grep <

[PATCH v2 3/5] dir.c: fix missing dir invalidation in untracked code

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

Let's start with how create a new directory cache after the last one
becomes invalid (e.g. because its dir mtime has changed...). In
open_cached_dir():

1. We start out with valid_cached_dir() returning false, which should
   call invalidate_directory() to put a directory state back to
   initial state, no untracked entries (untracked_nr zero), no sub
   directory traversal (dirs[].recurse zero).

2. Since the cache cannot be used, we go the slow path opendir() and
   go through items one by one via readdir(). All the directories on
   disk will be added back to the cache (if not already exist in
   dirs[]) and its flag "recurse" gets changed to one to note that
   it's part of the cached dir travesal next time.

3. By the time we reach close_cached_dir() we should have a good
   subdir list in dirs[]. Those with "recurse" flag set are the ones
   present in the on-disk directory. The directory is now marked
   "valid".

Next time read_directory() is called, since the directory is marked
valid, it will skip readdir(), go fast path and traverse through
dirs[] array instead.

Steps one and two need some tight cooperation. If a subdir is removed,
readdir() will not find it and of course we cannot examine/invalidate
it. To make sure removed directories on disk are gone from the cache,
step one must make sure recurse flag of all subdirs are zero.

But that's not true. If "valid" flag is already false, there is a
chance we go straight to the end of valid_cached_dir() without calling
invalidate_directory(). Or we fail to meet the "if (untracked-valid)"
condition and skip over the invalidate_directory().

After step 3, we mark the cache valid. Any stale subdir with incorrect
recurse flag becomes a real subdir next time we traverse the directory
using dirs[] array.

We could avoid this by making sure invalidate_directory() is always
called (therefore dirs[].recurse cleared) at the beginning of
open_cached_dir(). Which is what this patch does.

As to how we get into this situation, the key in the test is this
command

git checkout master

where "one/file" is replaced with "one" in the index. This index
update triggers untracked_cache_invalidate_path(), which clears valid
flag of the root directory while keeping "recurse" flag on the subdir
"one" on. On the next git-status, we go through steps 1-3 above and
save an incorrect cache on disk. The second git-status blindly follows
the bad cache data and shows the problem.

This is arguably because of a bad design where "recurse" flag plays
double roles: whether a directory should be saved on disk, and whether
it is part of a directory traversal.

We need to keep recurse flag set at "checkout master" because of the
first role: we need to keep subdir caches (dir "two" for example has
not been touched at all, no reason to throw its cache away).

As long as we make sure to ignore/reset "recurse" flag at the
beginning of a directory traversal, we're good. But maybe eventually
we should separate these two roles.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 dir.c | 22 ++
 t/t7063-status-untracked-cache.sh |  4 ++--
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/dir.c b/dir.c
index edcb7bb462..163ca69df0 100644
--- a/dir.c
+++ b/dir.c
@@ -774,7 +774,16 @@ static void invalidate_directory(struct untracked_cache 
*uc,
 struct untracked_cache_dir *dir)
 {
int i;
-   uc->dir_invalidated++;
+
+   /*
+* Invalidation increment here is just roughly correct. If
+* untracked_nr or any of dirs[].recurse is non-zero, we
+* should increment dir_invalidated too. But that's more
+* expensive to do.
+*/
+   if (dir->valid)
+   uc->dir_invalidated++;
+
dir->valid = 0;
dir->untracked_nr = 0;
for (i = 0; i < dir->dirs_nr; i++)
@@ -1810,23 +1819,18 @@ static int valid_cached_dir(struct dir_struct *dir,
refresh_fsmonitor(istate);
if (!(dir->untracked->use_fsmonitor && untracked->valid)) {
if (lstat(path->len ? path->buf : ".", )) {
-   invalidate_directory(dir->untracked, untracked);
memset(>stat_data, 0, 
sizeof(untracked->stat_data));
return 0;
}
if (!untracked->valid ||
match_stat_data_racy(istate, >stat_data, 
)) {
-   if (untracked->valid)
-   invalidate_directory(dir->untracked, untracked);
fill_stat_data(>stat_data, );
return 0;
}
}
 
-   if (untracked->check_only != !!check_only) {
-   invalidate_directory(dir->untracked, untracked);
+   if (untracked->check_only != !!check_only)

[PATCH v2 0/5] untracked cache bug fixes

Took me a while to get around to this. This is a replacement for the
patches Duy and I have floating around on the list so far related to
the untracked cache bugs raised recently.

Part of this has been me incorporating Duy's work and writing commit
messages etc. for him.

Nguyễn Thái Ngọc Duy (3):
  dir.c: avoid stat() in valid_cached_dir()
  dir.c: fix missing dir invalidation in untracked code
  dir.c: stop ignoring opendir() error in open_cached_dir()

Ævar Arnfjörð Bjarmason (2):
  status: add a failing test showing a core.untrackedCache bug
  update-index doc: note a fixed bug in the untracked cache

 Documentation/git-update-index.txt | 16 +++
 dir.c  | 33 ++-
 t/t7063-status-untracked-cache.sh  | 87 ++
 3 files changed, 125 insertions(+), 11 deletions(-)

-- 
2.15.1.424.g9478a66081

Re: [PATCH v1] convert: add support for 'encoding' attribute

2018-01-03 Thread Lars Schneider


On 03 Jan 2018, at 20:15, Junio C Hamano  wrote:

> Torsten Bögershausen  writes:
> 
>> May be.
>> Originally utf8.c was about encoding and all kind of UTF-8 related stuff.
>> Especially it didn't know anything about strbuf.
>> I don't know why strbuf.h and other functions had been added here,
>> 
>> I once moved them into strbuf.c without any problems, but never send out
>> a patch, because of possible merge conflicts in ongoing patches.
>> 
>> In any case, if it is about strbuf, I would try to put it into strbuf.c
> 
> Please don't.
> 
> A code that happens to use strbuf as a container and about
> manipulating the contents is quite different from a code about
> strbuf.  The latter is to enhance and extend how the strbuf as a
> container behaves.  An operation about character encoding for a
> string that happens to be stored in strbuf is more about the
> encoding, and much much less about strbuf.
> 
> convert.c is about massaging contents coming from the outside world
> into a shape stored in Git and the other way around, and there are
> multiple ways the contents are massaged.  EOL convention may be
> adjusted, characters may be reencoded, end-user defined conversion
> may be applied.  Some of these operations may use helpers specific
> for the task from other more library-ish files, like checking if a
> string looks like encoded in UTF-8 from utf8.[ch].

Agreed. I did that in v2. See these patches:

https://public-inbox.org/git/2017122915.39680-3-lars.schnei...@autodesk.com/
https://public-inbox.org/git/2017122915.39680-4-lars.schnei...@autodesk.com/

- Lars

Re: [PATCH 04/26] upload-pack: convert to a builtin

2018-01-03 Thread Brandon Williams

On 01/03, Stefan Beller wrote:
> On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> > In order to allow for code sharing with the server-side of fetch in
> > protocol-v2 convert upload-pack to be a builtin.
> 
> What is the security aspect of this patch?
> 
> By making upload-pack builtin, it gains additional abilities,
> such as answers to '-h' or '--help' (which would start a pager).
> Is there an easy way to sooth my concerns? (best put into the
> commit message)

receive-pack is already a builtin, so theres that.

> 
> Thanks,
> Stefan
> 

-- 
Brandon Williams

Re: [PATCH 05/26] upload-pack: factor out processing lines

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> Factor out the logic for processing shallow, deepen, deepen_since, and
> deepen_not lines into their own functions to simplify the
> 'receive_needs()' function in addition to making it easier to reuse some
> of this logic when implementing protocol_v2.
>
> Signed-off-by: Brandon Williams 
> ---
>  upload-pack.c | 113 
> ++
>  1 file changed, 74 insertions(+), 39 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 20acaa49d..9a507ae53 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -731,6 +731,75 @@ static void deepen_by_rev_list(int ac, const char **av,
> packet_flush(1);
>  }
>
> +static int process_shallow(const char *line, struct object_array *shallows)
> +{
> +   const char *arg;
> +   if (skip_prefix(line, "shallow ", )) {

stylistic nit:

You could invert the condition in each of the process_* functions
to just have

if (!skip_prefix...))
return 0

/* less indented code goes here */

return 1;

That way we have less indentation as well as easier code.
(The reader doesn't need to keep in mind what the else
part is about; it is a rather local decision to bail out instead
of having the return at the end of the function.)


> +   struct object_id oid;
> +   struct object *object;
> +   if (get_oid_hex(arg, ))
> +   die("invalid shallow line: %s", line);
> +   object = parse_object();
> +   if (!object)
> +   return 1;
> +   if (object->type != OBJ_COMMIT)
> +   die("invalid shallow object %s", oid_to_hex());
> +   if (!(object->flags & CLIENT_SHALLOW)) {
> +   object->flags |= CLIENT_SHALLOW;
> +   add_object_array(object, NULL, shallows);
> +   }
> +   return 1;
> +   }
> +
> +   return 0;
> +}
> +
> +static int process_deepen(const char *line, int *depth)
> +{
> +   const char *arg;
> +   if (skip_prefix(line, "deepen ", )) {
> +   char *end = NULL;
> +   *depth = strtol(arg, , 0);
> +   if (!end || *end || depth <= 0)
> +   die("Invalid deepen: %s", line);
> +   return 1;
> +   }
> +
> +   return 0;
> +}
> +
> +static int process_deepen_since(const char *line, timestamp_t *deepen_since, 
> int *deepen_rev_list)
> +{
> +   const char *arg;
> +   if (skip_prefix(line, "deepen-since ", )) {
> +   char *end = NULL;
> +   *deepen_since = parse_timestamp(arg, , 0);
> +   if (!end || *end || !deepen_since ||
> +   /* revisions.c's max_age -1 is special */
> +   *deepen_since == -1)
> +   die("Invalid deepen-since: %s", line);
> +   *deepen_rev_list = 1;
> +   return 1;
> +   }
> +   return 0;
> +}
> +
> +static int process_deepen_not(const char *line, struct string_list 
> *deepen_not, int *deepen_rev_list)
> +{
> +   const char *arg;
> +   if (skip_prefix(line, "deepen-not ", )) {
> +   char *ref = NULL;
> +   struct object_id oid;
> +   if (expand_ref(arg, strlen(arg), , ) != 1)
> +   die("git upload-pack: ambiguous deepen-not: %s", 
> line);
> +   string_list_append(deepen_not, ref);
> +   free(ref);
> +   *deepen_rev_list = 1;
> +   return 1;
> +   }
> +   return 0;
> +}
> +
>  static void receive_needs(void)
>  {
> struct object_array shallows = OBJECT_ARRAY_INIT;
> @@ -752,49 +821,15 @@ static void receive_needs(void)
> if (!line)
> break;
>
> -   if (skip_prefix(line, "shallow ", )) {
> -   struct object_id oid;
> -   struct object *object;
> -   if (get_oid_hex(arg, ))
> -   die("invalid shallow line: %s", line);
> -   object = parse_object();
> -   if (!object)
> -   continue;
> -   if (object->type != OBJ_COMMIT)
> -   die("invalid shallow object %s", 
> oid_to_hex());
> -   if (!(object->flags & CLIENT_SHALLOW)) {
> -   object->flags |= CLIENT_SHALLOW;
> -   add_object_array(object, NULL, );
> -   }
> +   if (process_shallow(line, ))
> continue;
> -   }
> -   if (skip_prefix(line, "deepen ", )) {
> -   char *end = NULL;
> -   depth = strtol(arg, , 0);
> -

Re: [PATCH 04/26] upload-pack: convert to a builtin

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> In order to allow for code sharing with the server-side of fetch in
> protocol-v2 convert upload-pack to be a builtin.

What is the security aspect of this patch?

By making upload-pack builtin, it gains additional abilities,
such as answers to '-h' or '--help' (which would start a pager).
Is there an easy way to sooth my concerns? (best put into the
commit message)

Thanks,
Stefan

>
> Signed-off-by: Brandon Williams 
> ---
>  Makefile  | 3 ++-
>  builtin.h | 1 +
>  git.c | 1 +
>  upload-pack.c | 2 +-
>  4 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 2a81ae22e..e0740b452 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -636,7 +636,6 @@ PROGRAM_OBJS += imap-send.o
>  PROGRAM_OBJS += sh-i18n--envsubst.o
>  PROGRAM_OBJS += shell.o
>  PROGRAM_OBJS += show-index.o
> -PROGRAM_OBJS += upload-pack.o
>  PROGRAM_OBJS += remote-testsvn.o
>
>  # Binary suffix, set to .exe for Windows builds
> @@ -701,6 +700,7 @@ BUILT_INS += git-merge-subtree$X
>  BUILT_INS += git-show$X
>  BUILT_INS += git-stage$X
>  BUILT_INS += git-status$X
> +BUILT_INS += git-upload-pack$X
>  BUILT_INS += git-whatchanged$X
>
>  # what 'all' will build and 'install' will install in gitexecdir,
> @@ -904,6 +904,7 @@ LIB_OBJS += tree-diff.o
>  LIB_OBJS += tree.o
>  LIB_OBJS += tree-walk.o
>  LIB_OBJS += unpack-trees.o
> +LIB_OBJS += upload-pack.o
>  LIB_OBJS += url.o
>  LIB_OBJS += urlmatch.o
>  LIB_OBJS += usage.o
> diff --git a/builtin.h b/builtin.h
> index 42378f3aa..f332a1257 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -231,6 +231,7 @@ extern int cmd_update_ref(int argc, const char **argv, 
> const char *prefix);
>  extern int cmd_update_server_info(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_upload_archive(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_upload_archive_writer(int argc, const char **argv, const char 
> *prefix);
> +extern int cmd_upload_pack(int argc, const char **argv, const char *prefix);
>  extern int cmd_var(int argc, const char **argv, const char *prefix);
>  extern int cmd_verify_commit(int argc, const char **argv, const char 
> *prefix);
>  extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
> diff --git a/git.c b/git.c
> index c870b9719..f71073dc8 100644
> --- a/git.c
> +++ b/git.c
> @@ -478,6 +478,7 @@ static struct cmd_struct commands[] = {
> { "update-server-info", cmd_update_server_info, RUN_SETUP },
> { "upload-archive", cmd_upload_archive },
> { "upload-archive--writer", cmd_upload_archive_writer },
> +   { "upload-pack", cmd_upload_pack },
> { "var", cmd_var, RUN_SETUP_GENTLY },
> { "verify-commit", cmd_verify_commit, RUN_SETUP },
> { "verify-pack", cmd_verify_pack },
> diff --git a/upload-pack.c b/upload-pack.c
> index d5de18127..20acaa49d 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1032,7 +1032,7 @@ static int upload_pack_config(const char *var, const 
> char *value, void *unused)
> return parse_hide_refs_config(var, value, "uploadpack");
>  }
>
> -int cmd_main(int argc, const char **argv)
> +int cmd_upload_pack(int argc, const char **argv, const char *prefix)
>  {
> const char *dir;
> int strict = 0;
> --
> 2.15.1.620.gb9897f4670-goog
>

Re: [PATCH 01/26] pkt-line: introduce packet_read_with_status

On Tue, Jan 2, 2018 at 4:18 PM, Brandon Williams  wrote:
> The current pkt-line API encodes the status of a pkt-line read in the
> length of the read content.  An error is indicated with '-1', a flush
> with '0' (which can be confusing since a return value of '0' can also
> indicate an empty pkt-line), and a positive integer for the length of
> the read content otherwise.  This doesn't leave much room for allowing
> the addition of additional special packets in the future.
>
> To solve this introduce 'packet_read_with_status()' which reads a packet
> and returns the status of the read encoded as an 'enum packet_status'
> type.  This allows for easily identifying between special and normal
> packets as well as errors.  It also enables easily adding a new special
> packet in the future.
>
> Signed-off-by: Brandon Williams 
> ---
>  pkt-line.c | 55 ++-
>  pkt-line.h | 15 +++
>  2 files changed, 57 insertions(+), 13 deletions(-)
>
> diff --git a/pkt-line.c b/pkt-line.c
> index 2827ca772..8d7cd389f 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
> return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
>  }
>
> -int packet_read(int fd, char **src_buf, size_t *src_len,
> -   char *buffer, unsigned size, int options)
> +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
> size_t *src_len,
> +   char *buffer, unsigned size, 
> int *pktlen,
> +   int options)
>  {
> -   int len, ret;
> +   int len;
> char linelen[4];
>
> -   ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> -   if (ret < 0)
> -   return ret;
> +   if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
> +   return PACKET_READ_EOF;
> +
> len = packet_length(linelen);
> if (len < 0)
> die("protocol error: bad line length character: %.4s", 
> linelen);
> -   if (!len) {
> +
> +   if (len == 0) {
> packet_trace("", 4, 0);
> -   return 0;
> +   return PACKET_READ_FLUSH;
> +   } else if (len >= 1 && len <= 3) {
> +   die("protocol error: bad line length character: %.4s", 
> linelen);

I wonder how much libified code we want here already, maybe we could
have PACKET_READ_ERROR as a return value here instead of die()ing.
There could also be an option that tells this code to die on error, this reminds
me of the repository discovery as well as the refs code, both of which have
this pattern.

Currently this series is only upgrading commands that use the network
anyway, so I guess die()ing in an ls-remote or fetch is no big deal,
but it could
be interesting to keep going once we have more of the partial clone
stuff working
(e.g. remote assisted log/blame would want to gracefully fall back instead of
die()ing without any useful output, I would think.)

> }
> +
> len -= 4;
> -   if (len >= size)
> +   if ((len < 0) || ((unsigned)len >= size))
> die("protocol error: bad line length %d", len);
> -   ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
> -   if (ret < 0)
> -   return ret;
> +
> +   if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 
> 0)
> +   return PACKET_READ_EOF;
>
> if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> len && buffer[len-1] == '\n')
> @@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
>
> buffer[len] = 0;
> packet_trace(buffer, len, 0);
> -   return len;
> +   *pktlen = len;
> +   return PACKET_READ_NORMAL;
> +}
> +
> +int packet_read(int fd, char **src_buffer, size_t *src_len,
> +   char *buffer, unsigned size, int options)
> +{
> +   enum packet_read_status status;
> +   int pktlen;
> +
> +   status = packet_read_with_status(fd, src_buffer, src_len,
> +buffer, size, ,
> +options);
> +   switch (status) {
> +   case PACKET_READ_EOF:
> +   pktlen = -1;
> +   break;
> +   case PACKET_READ_NORMAL:
> +   break;
> +   case PACKET_READ_FLUSH:
> +   pktlen = 0;
> +   break;
> +   }
> +
> +   return pktlen;
>  }
>
>  static char *packet_read_line_generic(int fd,
> diff --git a/pkt-line.h b/pkt-line.h
> index 3dad583e2..06c468927 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t 
> len, int fd_out);
>  int packet_read(int fd, char **src_buffer, size_t *src_len, char
> *buffer, unsigned size, int options);
>
>

Re: [PATCH] Add shell completion for git remote rm

Ævar Arnfjörð Bjarmason  writes:

> On Sat, Dec 30 2017, Todd Zullinger jotted:
>
>> And I think that should also apply to
>> not offering completion for commands/subcommands/options
>> which are only kept for backward compatibility.
>
> Yeah I think it makes sense to at some point stop completing things if
> we're going to remove stuff, if we decide to remove it.
>
>> Here's one way to make 'git remote rm ' work without
>> including it in the output of 'git remote ':
>>
>> diff --git i/contrib/completion/git-completion.bash 
>> w/contrib/completion/git-completion.bash
>> index 3683c772c5..aa63f028ab 100644
>> --- i/contrib/completion/git-completion.bash
>> +++ w/contrib/completion/git-completion.bash
>> @@ -2668,7 +2668,9 @@ _git_remote ()
>>  add rename remove set-head set-branches
>>  get-url set-url show prune update
>>  "
>> -local subcommand="$(__git_find_on_cmdline "$subcommands")"
>> +# Don't advertise rm by including it in subcommands, but complete
>> +# remotes if it is used.
>> +local subcommand="$(__git_find_on_cmdline "$subcommands rm")"
>>  if [ -z "$subcommand" ]; then
>>  case "$cur" in
>>  --*)
>
> Neat!

Yes, indeed it is nice.

Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory


On Wed, Jan 03 2018, Adam Dinwoodie jotted:

> On Wednesday 03 January 2018 at 02:31 pm +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Wed, Jan 03 2018, Adam Dinwoodie jotted:
>>
>> > On Monday 25 December 2017 at 12:28 am +, Ævar Arnfjörð Bjarmason 
>> > wrote:
>> >> There has never been any full roundtrip testing of what git-ls-files
>> >> and other functions that use wildmatch() actually do, rather we've
>> >> been satisfied with just testing the underlying C function.
>> >>
>> >> Due to git-ls-files and friends having their own codepaths before they
>> >> call wildmatch() there's sometimes differences in the behavior between
>> >> the two, and even when we test for those (as with
>> >> 9e4e8a64c2 ("pathspec: die on empty strings as pathspec", 2017-06-06))
>> >> there was no one place where you can review how these two modes
>> >> differ.
>> >>
>> >> Now there is. We now attempt to create a file called $haystack and
>> >> match $needle against it for each pair of $needle and $haystack that
>> >> we were passing to test-wildmatch.
>> >>
>> >> If we can't create the file we skip the test. This ensures that we can
>> >> run this on all platforms and not maintain some infinitely growing
>> >> whitelist of e.g. platforms that don't support certain characters in
>> >> filenames.
>> >>
>> >> As a result of doing this we can now see the cases where these two
>> >> ways of testing wildmatch differ:
>> >>
>> >>  * Creating a file called 'a[]b' and running ls-files 'a[]b' will show
>> >>that file, but wildmatch("a[]b", "a[]b") will not match
>> >>
>> >>  * wildmatch() won't match a file called \ against \, but ls-files
>> >>will.
>> >>
>> >>  * `git --glob-pathspecs ls-files 'foo**'` will match a file
>> >>'foo/bba/arr', but wildmatch won't, however pathmatch will.
>> >>
>> >>This seems like a bug to me, the two are otherwise equivalent as
>> >>these tests show.
>> >>
>> >> This also reveals the case discussed in 9e4e8a64c2 above, where '' is
>> >> now an error as far as ls-files is concerned, but wildmatch() itself
>> >> happily accepts it.
>> >>
>> >> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> >
>> > I'm seeing this test script failing on the pu branch as a result of this
>> > commit when building on Cygwin.  Specifically, the test fails at
>> > 9d45e1ca4 ("Merge branch 'bw/oidmap-autoinit' into pu", 2017-12-28), and
>> > bisecting points the blame at 2ee0c785a ("wildmatch test: create & test
>> > files on disk in addition to in-memory", 2017-12-25).
>> >
>> > I've copied the verbose error output for the first error below, and
>> > uploaded the full output, including verbose and trace output for the
>> > unexpectedly failing tests, at [0].  (With 42 failures among 1512 tests,
>> > there's a lot of it, so I didn't want to include it in an email.)
>>
>> Does the fixup above in <878tdm8k2d@evledraar.gmail.com> work for
>> you, i.e. changing $10 in the script to ${10}?
>
> This fixes some but not all of the failures: I'm now down from 42 to 24
> failures.
>
> Updated verbose test output is at
> https://gist.github.com/me-and/04443bcb00e12436f0eacce079b56d02

Thanks lot, looking through our own commit logs I believe the rest
should be fixed by this (prior art in 6fd1106aa4), it would be great if
you could test it, I don't have access to a Windows machine:

diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
index f985139b6f..5838fcb77d 100755
--- a/t/t3070-wildmatch.sh
+++ b/t/t3070-wildmatch.sh
@@ -23,6 +23,15 @@ create_test_file() {
*//*)
return 1
;;
+   # On Windows, \ in paths is silently converted to /, which
+   # would result in the "touch" below working, but the test
+   # itself failing.
+   *\\*)
+   if ! test_have_prereq BSLASHPSPEC
+   then
+   return 1
+   fi
+   ;;
# When testing the difference between foo/bar and foo/bar/ we
# can't test the latter.
*/)

Re: [PATCH v1] convert: add support for 'encoding' attribute

Torsten Bögershausen  writes:

> May be.
> Originally utf8.c was about encoding and all kind of UTF-8 related stuff.
> Especially it didn't know anything about strbuf.
> I don't know why strbuf.h and other functions had been added here,
>
> I once moved them into strbuf.c without any problems, but never send out
> a patch, because of possible merge conflicts in ongoing patches.
>
> In any case, if it is about strbuf, I would try to put it into strbuf.c

Please don't.

A code that happens to use strbuf as a container and about
manipulating the contents is quite different from a code about
strbuf.  The latter is to enhance and extend how the strbuf as a
container behaves.  An operation about character encoding for a
string that happens to be stored in strbuf is more about the
encoding, and much much less about strbuf.

convert.c is about massaging contents coming from the outside world
into a shape stored in Git and the other way around, and there are
multiple ways the contents are massaged.  EOL convention may be
adjusted, characters may be reencoded, end-user defined conversion
may be applied.  Some of these operations may use helpers specific
for the task from other more library-ish files, like checking if a
string looks like encoded in UTF-8 from utf8.[ch].

Re: [PATCH 0/2] Several fixes for the test suite related to spaces in filenames

On Wed, Jan 03, 2018 at 05:54:44PM +0100, Johannes Schindelin wrote:

> The second issue was found long ago, and the patch carried in Git for
> Windows, although nothing about it is specific to Windows.
> 
> The first patch was developed today, when I tried to verify that Git's
> test suite passes if Git is cloned to a directory called `with spaces/`.

Heh, the whole point of the space in the trash directory was to find
these issues early, but obviously it is not foolproof. :)

The patches themselves look good to me from inspection. Thanks.

-Peff

[PATCH] bisect: fix a regression causing a segfault