Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor

[Eric Wong]

Jeff King  wrote:
> On Thu, Jan 26, 2017 at 12:13:44AM +, brian m. carlson wrote:
> > +
> > +  def process(parent, target, attrs)
> > +if parent.document.basebackend? 'html'
> > +  prefix = parent.document.attr('git-relative-html-prefix')
> > +  %(#{target}(#{attrs[1]})\n)
> > +elsif parent.document.basebackend? 'docbook'
> > +  %(
> > +#{target}#{attrs[1]}
> > +
> > +)



> The multi-line string is kind of ugly because of the indentation.
> Apparently Ruby has here-docs that will eat leading whitespace, but the
> syntax was not introduce until Ruby 2.3, which is probably more recent
> than we should count on.

You can use '\' to continue long lines with any Ruby version:

"" \
  "#{target}" \
  "#{attrs[1]}" \
""

The above happens during the parse phase, so there's no garbage
or method call overhead compared to the more-frequently seen '+'
or '<<' method calls to combine strings.

> I think you could write:
> 
>   %(
> 
> #{target}#{attrs[1]}
> 
> ).gsub(/^\s*/, "")
> 
> I don't know if that's too clever or not.

Ick...

> But either way, I like this better than introducing an extra dependency.

Agreed.

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Johannes Sixt]

Am 25.01.2017 um 23:01 schrieb Jeff King:

+#pragma GCC diagnostic ignored "-Wformat-zero-length"


Last time I used #pragma GCC in a cross-platform project, it triggered 
an "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't 
know if the C compiler would also warn.) It would have to be spelled 
like this:


#pragma warning(disable: 4068)   /* MSVC: unknown pragma */
#pragma GCC diagnostic ignored "-Wformat-zero-length"

Dscho mentioned that he's compiling with MSVC. It would do him a favor.

-- Hannes

Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.

[Jeff King]

On Thu, Jan 26, 2017 at 08:54:10AM +0900, Mike Hommey wrote:

> > Implementation-wise, I'd be happier if we do not add any new
> > callsites of strbuf_split(), which is a horrible interface.  But
> > that is a minor detail.
> 
> What would you suggest otherwise?

Try string_list_split() (or its in_place() variant, since it is probably
OK to hack up the string for your use case). Like this:

diff --git a/gpg-interface.c b/gpg-interface.c
index 2768bb307..051bb7d3e 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -158,14 +158,16 @@ static int pipe_gpg_command(struct child_process *cmd,
/* Print out any line that doesn't start with [GNUPG:] if the gpg
 * command failed. */
if (ret) {
-   struct strbuf **err_lines = strbuf_split(err, '\n');
-   for (struct strbuf **line = err_lines; *line; line++) {
-   if (memcmp((*line)->buf, "[GNUPG:]", 8)) {
-   strbuf_rtrim(*line);
-   fprintf(stderr, "%s\n", (*line)->buf);
-   }
+   struct string_list lines = STRING_LIST_INIT_NODUP;
+   int i;
+
+   string_list_split_in_place(, err->buf, '\n', 0);
+   for (i = 0; i < lines.nr; i++) {
+   const char *line = lines.items[i].string;
+   if (!starts_with(line, "[GNUPG:]"))
+   fprintf(stderr, "%s\n", line);
}
-   strbuf_list_free(err_lines);
+   string_list_clear(, 0);
}
return ret;
 }

Note that I also replaced the memcmp with starts_with(). That avoids the
magic number "8". I also suspect your original can read off the end of
the buffer when the line is shorter than 8 characters (e.g., if memcmp
does 64-bit loads).

-Peff

Re: Force Confirmation for Dropping Changed Lines

[Jacob Keller]

On Wed, Jan 25, 2017 at 6:32 PM, Junio C Hamano  wrote:
> Where did you get that "unset" from?  If that is this paragraph in
> Documentation/gitattributes.txt:
>

Ok so that whole section of documentation is very confusing to me.
Maybe it could be improved for more readability. I'll see if I can't
help clear up some of my confusion.

Thanks,
Jake

Re: sparse checkout - weird behavior

[Jeff King]

On Wed, Jan 25, 2017 at 10:21:19PM -0500, Paul Hammant wrote:

> Related bug (maybe the same). Reproduction:
> 
>   $ git clone g...@github.com:jekyll/jekyll.git --no-checkout
>   Cloning into 'jekyll'...
>   remote: Counting objects: 41331, done.
>   remote: Compressing objects: 100% (5/5), done.
>   remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
>   Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
>   Resolving deltas: 100% (26530/26530), done.
>   $ cd jekyll
>   $ git config core.sparsecheckout true
>   $ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
>   $ echo 'Gemfile' >> .git/info/sparse-checkout
>   $ echo 'Rakefile' >> .git/info/sparse-checkout
>   $ echo 'appveyor.yml' >> .git/info/sparse-checkout
>   $ git checkout --
>   Your branch is up-to-date with 'origin/master'.
>   $ ls
>   CONDUCT.markdown Gemfile Rakefile appveyor.yml lib
> 
> I was not expecting to see 'lib' in the resulting file list

Yep, I think this is the same problem. Inside lib, you get only
"lib/theme_template/Gemfile", because it matches your unanchored
pattern. Using "/Gemfile" in the sparse-checkout file fixes it.

-Peff

Re: sparse checkout - weird behavior

[Jeff King]

On Wed, Jan 25, 2017 at 09:59:38PM -0500, Paul Hammant wrote:

> Here's a simple reproducible bug - something unexpected in sparse-checkout 
> mode:
> 
>   $ git clone g...@github.com:jekyll/jekyll.git --no-checkout
>   Cloning into 'jekyll'...
>   remote: Counting objects: 41331, done.
>   remote: Compressing objects: 100% (5/5), done.
>   remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
>   Receiving objects: 100% (41331/41331), 11.91 MiB | 7.98 MiB/s, done.
>   Resolving deltas: 100% (26530/26530), done.
>   $ cd jekyll
>   $ ls
>   $ git config core.sparsecheckout true
>   $ echo 'docs*' > .git/info/sparse-checkout
>   $ git read-tree -mu HEAD
>   $ ls
>   docs rake
> 
> I didn't expect to see 'rake' amongst the results.

If you look inside the rake/ directory, you should see that only
"docs.rake" was checked out.

The sparse-checkout file uses the same parser as .git/info/exclude. One
important aspect of that file is that entries are _not_ left-anchored
unless they start with "/". So you asked Git to include files named
"docs*" anywhere in the tree.

You probably wanted just:

  echo /docs >.git/info/sparse-checkout

-Peff

[PATCH 2/2] fsck: lazily load types under --connectivity-only

[Jeff King]

The recent fixes to "fsck --connectivity-only" load all of
the objects with their correct types. This keeps the
connectivity-only code path close to the regular one, but it
also introduces some unnecessary inefficiency. While getting
the type of an object is cheap compared to actually opening
and parsing the object (as the non-connectivity-only case
would do), it's still not free.

For reachable non-blob objects, we end up having to parse
them later anyway (to see what they point to), making our
type lookup here redundant.

For unreachable objects, we might never hit them at all in
the reachability traversal, making the lookup completely
wasted. And in some cases, we might have quite a few
unreachable objects (e.g., when alternates are used for
shared object storage between repositories, it's normal for
there to be objects reachable from other repositories but
not the one running fsck).

The comment in mark_object_for_connectivity() claims two
benefits to getting the type up front:

  1. We need to know the types during fsck_walk(). (And not
 explicitly mentioned, but we also need them when
 printing the types of broken or dangling commits).

 We can address this by lazy-loading the types as
 necessary. Most objects never need this lazy-load at
 all, because they fall into one of these categories:

   a. Reachable from our tips, and are coerced into the
  correct type as we traverse (e.g., a parent link
  will call lookup_commit(), which converts OBJ_NONE
  to OBJ_COMMIT).

   b. Unreachable, but not at the tip of a chunk of
  unreachable history. We only mention the tips as
  "dangling", so an unreachable commit which links
  to hundreds of other objects needs only report the
  type of the tip commit.

  2. It serves as a cross-check that the coercion in (1a) is
 correct (i.e., we'll complain about a parent link that
 points to a blob). But we get most of this for free
 already, because right after coercing, we'll parse any
 non-blob objects. So we'd notice then if we expected a
 commit and got a blob.

 The one exception is when we expect a blob, in which
 case we never actually read the object contents.

 So this is a slight weakening, but given that the whole
 point of --connectivity-only is to sacrifice some data
 integrity checks for speed, this seems like an
 acceptable tradeoff.

Here are before and after timings for an extreme case with
~5M reachable objects and another ~12M unreachable (it's the
torvalds/linux repository on GitHub, connected to shared
storage for all of the other kernel forks):

  [before]
  $ time git fsck --no-dangling --connectivity-only
  real  3m4.323s
  user  1m25.121s
  sys   1m38.710s

  [after]
  $ time git fsck --no-dangling --connectivity-only
  real  0m51.497s
  user  0m49.575s
  sys   0m1.776s

Signed-off-by: Jeff King 
---
 builtin/fsck.c | 58 +++---
 fsck.c |  4 
 2 files changed, 11 insertions(+), 51 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 3d5ced2d3..140357b6f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -60,6 +60,12 @@ static const char *printable_type(struct object *obj)
 {
const char *ret;
 
+   if (obj->type == OBJ_NONE) {
+   enum object_type type = sha1_object_info(obj->oid.hash, NULL);
+   if (type > 0)
+   object_as_type(obj, type, 0);
+   }
+
ret = typename(obj->type);
if (!ret)
ret = "unknown";
@@ -595,57 +601,7 @@ static int fsck_cache_tree(struct cache_tree *it)
 
 static void mark_object_for_connectivity(const unsigned char *sha1)
 {
-   struct object *obj = lookup_object(sha1);
-
-   /*
-* Setting the object type here isn't strictly necessary for a
-* connectivity check. In most cases, our walk will expect a certain
-* type (e.g., a tree referencing a blob) and will use lookup_blob() to
-* assign the type. But doing it here has two advantages:
-*
-*   1. When the fsck_walk code looks at objects that _don't_ come from
-*  links (e.g., the tip of a ref), it may complain about the
-*  "unknown object type".
-*
-*   2. This serves as a nice cross-check that the graph links are
-*  sane. So --connectivity-only does not check that the bits of
-*  blobs are not corrupted, but it _does_ check that 100644 tree
-*  entries point to blobs, and so forth.
-*
-* Unfortunately we can't just use parse_object() here, because the
-* whole point of --connectivity-only is to avoid reading the object
-* data more than necessary.
-*/
-   if (!obj || obj->type == OBJ_NONE) {
-   enum object_type type = sha1_object_info(sha1, NULL);
-   switch (type) {

[PATCH 0/2] (re-)optimizing fsck --connectivity-only

[Jeff King]

I've been playing around with the newly-fixed `fsck --connectivity-only`.
While it's much faster than it was, I was sad to find a case that is
still much slower than "git rev-list --objects --all".

This goes on top of my origin/jk/fsck-connectivity-check-fix, and gives
a noticeable speedup. IMHO it's worth considering as part of the same
topic (which is currently in 'next').

  [1/2]: fsck: move typename() printing to its own function
  [2/2]: fsck: lazily load types under --connectivity-only

 builtin/fsck.c | 87 ++
 fsck.c |  4 +++
 2 files changed, 31 insertions(+), 60 deletions(-)

-Peff

[PATCH 1/2] fsck: move typename() printing to its own function

[Jeff King]

When an object has a problem, we mention its type. But we do
so by feeding the result of typename() directly to
fprintf(). This is potentially dangerous because typename()
can return NULL for some type values (like OBJ_NONE).

It's doubtful that this can be triggered in practice with
the current code, so this is probably not fixing a bug. But
it future-proofs us against modifications that make things
like OBJ_NONE more likely (and gives future patches a
central point to handle them).

Signed-off-by: Jeff King 
---
 builtin/fsck.c | 29 -
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 57f529b41..3d5ced2d3 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -56,6 +56,17 @@ static const char *describe_object(struct object *obj)
return buf.buf;
 }
 
+static const char *printable_type(struct object *obj)
+{
+   const char *ret;
+
+   ret = typename(obj->type);
+   if (!ret)
+   ret = "unknown";
+
+   return ret;
+}
+
 static int fsck_config(const char *var, const char *value, void *cb)
 {
if (strcmp(var, "fsck.skiplist") == 0) {
@@ -83,7 +94,7 @@ static void objreport(struct object *obj, const char 
*msg_type,
const char *err)
 {
fprintf(stderr, "%s in %s %s: %s\n",
-   msg_type, typename(obj->type), describe_object(obj), err);
+   msg_type, printable_type(obj), describe_object(obj), err);
 }
 
 static int objerror(struct object *obj, const char *err)
@@ -114,7 +125,7 @@ static int mark_object(struct object *obj, int type, void 
*data, struct fsck_opt
if (!obj) {
/* ... these references to parent->fld are safe here */
printf("broken link from %7s %s\n",
-  typename(parent->type), describe_object(parent));
+  printable_type(parent), describe_object(parent));
printf("broken link from %7s %s\n",
   (type == OBJ_ANY ? "unknown" : typename(type)), 
"unknown");
errors_found |= ERROR_REACHABLE;
@@ -131,9 +142,9 @@ static int mark_object(struct object *obj, int type, void 
*data, struct fsck_opt
if (!(obj->flags & HAS_OBJ)) {
if (parent && !has_object_file(>oid)) {
printf("broken link from %7s %s\n",
-typename(parent->type), 
describe_object(parent));
+printable_type(parent), 
describe_object(parent));
printf("  to %7s %s\n",
-typename(obj->type), describe_object(obj));
+printable_type(obj), describe_object(obj));
errors_found |= ERROR_REACHABLE;
}
return 1;
@@ -205,7 +216,7 @@ static void check_reachable_object(struct object *obj)
if (!(obj->flags & HAS_OBJ)) {
if (has_sha1_pack(obj->oid.hash))
return; /* it is in pack - forget about it */
-   printf("missing %s %s\n", typename(obj->type),
+   printf("missing %s %s\n", printable_type(obj),
describe_object(obj));
errors_found |= ERROR_REACHABLE;
return;
@@ -231,7 +242,7 @@ static void check_unreachable_object(struct object *obj)
 * since this is something that is prunable.
 */
if (show_unreachable) {
-   printf("unreachable %s %s\n", typename(obj->type),
+   printf("unreachable %s %s\n", printable_type(obj),
describe_object(obj));
return;
}
@@ -250,7 +261,7 @@ static void check_unreachable_object(struct object *obj)
 */
if (!obj->used) {
if (show_dangling)
-   printf("dangling %s %s\n", typename(obj->type),
+   printf("dangling %s %s\n", printable_type(obj),
   describe_object(obj));
if (write_lost_and_found) {
char *filename = git_pathdup("lost-found/%s/%s",
@@ -324,7 +335,7 @@ static int fsck_obj(struct object *obj)
 
if (verbose)
fprintf(stderr, "Checking %s %s\n",
-   typename(obj->type), describe_object(obj));
+   printable_type(obj), describe_object(obj));
 
if (fsck_walk(obj, NULL, _obj_options))
objerror(obj, "broken links");
@@ -350,7 +361,7 @@ static int fsck_obj(struct object *obj)
struct tag *tag = (struct tag *) obj;
 
if (show_tags && tag->tagged) {
-   printf("tagged %s %s", typename(tag->tagged->type),
+   printf("tagged %s %s", printable_type(tag->tagged),
describe_object(tag->tagged));
   

Re: [PATCH] Documentation: implement linkgit macro for Asciidoctor

[Jeff King]

On Thu, Jan 26, 2017 at 12:13:44AM +, brian m. carlson wrote:

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 19c42eb60..d1b7a6865 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -179,10 +179,7 @@ ASCIIDOC = asciidoctor
>  ASCIIDOC_CONF =
>  ASCIIDOC_HTML = xhtml5
>  ASCIIDOC_DOCBOOK = docbook45
> -ifdef ASCIIDOCTOR_EXTENSIONS_LAB
> -ASCIIDOC_EXTRA = -I$(ASCIIDOCTOR_EXTENSIONS_LAB) -rasciidoctor/extensions 
> -rman-inline-macro
> -endif
> -ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
> +ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions -alitdd='&\#x2d;&\#x2d;'

Might be more readable to just leave the litdd part on its own line.

> diff --git a/Documentation/asciidoctor-extensions.rb 
> b/Documentation/asciidoctor-extensions.rb
> new file mode 100644
> index 0..09f7088ee
> --- /dev/null
> +++ b/Documentation/asciidoctor-extensions.rb
> @@ -0,0 +1,28 @@
> +require 'asciidoctor'
> +require 'asciidoctor/extensions'
> +
> +module Git
> +  module Documentation
> +class LinkGitProcessor < Asciidoctor::Extensions::InlineMacroProcessor
> +  use_dsl
> +
> +  named :chrome
> +
> +  def process(parent, target, attrs)
> +if parent.document.basebackend? 'html'
> +  prefix = parent.document.attr('git-relative-html-prefix')
> +  %(#{target}(#{attrs[1]})\n)
> +elsif parent.document.basebackend? 'docbook'
> +  %(
> +#{target}#{attrs[1]}
> +
> +)
> +end
> +  end
> +end
> +  end
> +end

I think this looks reasonable. There's some boilerplate, but even as
somebody not familiar with asciidoctor, it's all quite obvious.

The multi-line string is kind of ugly because of the indentation.
Apparently Ruby has here-docs that will eat leading whitespace, but the
syntax was not introduce until Ruby 2.3, which is probably more recent
than we should count on.

I think you could write:

  %(

#{target}#{attrs[1]}

  ).gsub(/^\s*/, "")

I don't know if that's too clever or not.

But either way, I like this better than introducing an extra dependency.

-Peff

Re: [PATCH] refs: add option core.logAllRefUpdates = always

[Jeff King]

On Thu, Jan 26, 2017 at 02:16:54AM +0100, cornelius.w...@tngtech.com wrote:

> From: Cornelius Weig 
> 
> When core.logallrefupdates is true, we only create a new reflog for refs
> that are under certain well-known hierarchies. The reason is that we
> know that some hierarchies (like refs/tags) do not typically change, and
> that unknown hierarchies might not want reflogs at all (e.g., a
> hypothetical refs/foo might be meant to change often and drop old
> history immediately).

I tried to read this patch with fresh eyes. But given the history, you
may take my review with a grain of salt. :)

Overall it looks OK to me. A few comments below.

> This patch introduces a new "always" mode for the core.logallrefupdates
> option which will log updates to everything under refs/, regardless
> where in the hierarchy it is (we still will not log things like
> ORIG_HEAD and FETCH_HEAD, which are known to be transient).

I don't think my original had tests for this, but it might be worth
adding a test for this last bit (i.e., that an update of ORIG_HEAD does
not write a reflog when logallrefupdates is set to "always").

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index af2ae4c..2117616 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -517,10 +517,13 @@ core.logAllRefUpdates::
>   "`$GIT_DIR/logs/`", by appending the new and old
>   SHA-1, the date/time and the reason of the update, but
>   only when the file exists.  If this configuration
> - variable is set to true, missing "`$GIT_DIR/logs/`"
> + variable is set to `true`, missing "`$GIT_DIR/logs/`"
>   file is automatically created for branch heads (i.e. under
>   refs/heads/), remote refs (i.e. under refs/remotes/),
> - note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
> + `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
> + note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
> + If it is set to `always`, then a missing reflog is automatically
> + created for any ref under `refs/`.

I guess the backtick fixups came from my original. It might be easier to
see the change if they were pulled into their own patch, but it's
probably not that big a deal.

> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -150,7 +150,8 @@ This option is only applicable when listing tags without 
> annotation lines.
>   'strip' removes both whitespace and commentary.
>  
>  --create-reflog::
> - Create a reflog for the tag.
> + Create a reflog for the tag. To globally enable reflogs for tags, see
> + `core.logAllRefUpdates` in linkgit:git-config[1].

This documentation tweak makes sense to me.

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 76d68fa..1d4d6a0 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,
>   const char *work_tree = get_git_work_tree();
>   git_config_set("core.bare", "false");
>   /* allow template config file to override the default */
> - if (log_all_ref_updates == -1)
> + if (log_all_ref_updates == LOG_REFS_UNSET)
>   git_config_set("core.logallrefupdates", "true");
>   if (needs_work_tree_config(original_git_dir, work_tree))
>   git_config_set("core.worktree", work_tree);

I expected that this hunk would need tweaked due to refactoring around
init-db that happened earlier this year. But it seems fine.

> diff --git a/refs.c b/refs.c
> index 9bd0bc1..cd36b64 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)
>  
>  int should_autocreate_reflog(const char *refname)
>  {
> - if (!log_all_ref_updates)
> + switch (log_all_ref_updates) {
> + case LOG_REFS_ALWAYS:
> + return 1;
> + case LOG_REFS_NORMAL:
> + return starts_with(refname, "refs/heads/") ||
> + starts_with(refname, "refs/remotes/") ||
> + starts_with(refname, "refs/notes/") ||
> + !strcmp(refname, "HEAD");
> + default:
>   return 0;
> - return starts_with(refname, "refs/heads/") ||
> - starts_with(refname, "refs/remotes/") ||
> - starts_with(refname, "refs/notes/") ||
> - !strcmp(refname, "HEAD");
> + }
>  }

And this function got broken out already by David in an earlier patch.
Looks good.

> @@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const 
> unsigned char *old_sha1,
>  {
>   int logfd, result, oflags = O_APPEND | O_WRONLY;
>  
> - if (log_all_ref_updates < 0)
> - log_all_ref_updates = !is_bare_repository();
> + if (log_all_ref_updates == LOG_REFS_UNSET)
> + log_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : 

Re: sparse checkout - weird behavior

[Paul Hammant]

Related bug (maybe the same). Reproduction:

  $ git clone g...@github.com:jekyll/jekyll.git --no-checkout
  Cloning into 'jekyll'...
  remote: Counting objects: 41331, done.
  remote: Compressing objects: 100% (5/5), done.
  remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
  Receiving objects: 100% (41331/41331), 11.91 MiB | 9.15 MiB/s, done.
  Resolving deltas: 100% (26530/26530), done.
  $ cd jekyll
  $ git config core.sparsecheckout true
  $ echo 'CONDUCT.markdown' > .git/info/sparse-checkout
  $ echo 'Gemfile' >> .git/info/sparse-checkout
  $ echo 'Rakefile' >> .git/info/sparse-checkout
  $ echo 'appveyor.yml' >> .git/info/sparse-checkout
  $ git checkout --
  Your branch is up-to-date with 'origin/master'.
  $ ls
  CONDUCT.markdown Gemfile Rakefile appveyor.yml lib

I was not expecting to see 'lib' in the resulting file list

I didn't say so before - I'm on a Mac, with a homebrew installed Git 2.11.0

- Paul

sparse checkout - weird behavior

[Paul Hammant]

Here's a simple reproducible bug - something unexpected in sparse-checkout mode:

  $ git clone g...@github.com:jekyll/jekyll.git --no-checkout
  Cloning into 'jekyll'...
  remote: Counting objects: 41331, done.
  remote: Compressing objects: 100% (5/5), done.
  remote: Total 41331 (delta 0), reused 0 (delta 0), pack-reused 41326
  Receiving objects: 100% (41331/41331), 11.91 MiB | 7.98 MiB/s, done.
  Resolving deltas: 100% (26530/26530), done.
  $ cd jekyll
  $ ls
  $ git config core.sparsecheckout true
  $ echo 'docs*' > .git/info/sparse-checkout
  $ git read-tree -mu HEAD
  $ ls
  docs rake

I didn't expect to see 'rake' amongst the results.

If I take out the asterisk on the echo line, only 'docs' is checked
out after the read-tree operation. Even if asterisk isn't allowed, I'd
not expect it to fail in that way. Sparse checkout documentation is a
little . .. . sparse it has to be said (sorry).

As it happens, I have some more complicated cases where
sparse-checkout is doing the wrong thing in the working copy - and
I'll work towards reproduction of that/those after this. Best to start
with a simple reproducible case in case there's a simple fix for more
that one similar bug.

Regards,

- Paul

mergetool and difftool inconsistency?

[Denton Liu]

Hello all,

I was wondering if there is any reason why 'git difftool' accepts the
'-g|--gui' whereas 'git mergetool' does not have an option to accept
that flag. Please let me know if this is intentional, otherwise I can
write up a patch to add the GUI flag to 'mergetool'.

Thanks!

Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.

[Mike Hommey]

On Wed, Jan 25, 2017 at 06:37:55PM -0800, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> > On Wed, Jan 25, 2017 at 03:04:38PM -0800, Junio C Hamano wrote:
> > ...
> >> Overall I think this is a good thing to do.  Instead of eating the
> >> status output, showing what we got, especially when we know the
> >> command failed, would make the bug-hunting of user's environment
> >> easier, like you showed above.
> >> 
> >> The only thing in the design that makes me wonder is the filtering
> >> out based on "[GNUPG:]" prefix.  Why do we need to filter them out?
> >
> > The [GNUPG:] lines are part of the status-fd protocol. They show details
> > that don't really seem interesting to the user. In fact, they even
> > contain the signed message (yes, in my case, it turns out gpg was
> > actually still signing, but returned an error code...).
> 
> OK, that detail was what was missing in the proposed log message.
> Without that "why do we filter?", readers cannot correctly assess
> why it is a good idea.  I wasn't arguing against filtering it; I
> just wanted to make sure "git log" readers (and "git show" user
> after "git blame" identifies this change in the history) will know
> how we decided to filter lines with the prefix.  
> 
> With that information recorded in the log (or in-code comment, or
> both), if it turns out that some lines with the prefix are useful
> (or some other lines without the prefix are not very useful), they
> can tweak the filtering criteria as appropriate, with confidence
> that they _know_ for what purpose the initial "filter lines with the
> prefix" was trying to serve, and their update is still in the same
> spirit as the original, only executed better.

Come to think of it, and considering that mutt happily signs emails in
the same conditions, maybe it would make sense to just ignore gpg return
code as long as there is a SIG_CREATED message...

Mike

Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.

[Junio C Hamano]

Mike Hommey  writes:

> On Wed, Jan 25, 2017 at 03:04:38PM -0800, Junio C Hamano wrote:
> ...
>> Overall I think this is a good thing to do.  Instead of eating the
>> status output, showing what we got, especially when we know the
>> command failed, would make the bug-hunting of user's environment
>> easier, like you showed above.
>> 
>> The only thing in the design that makes me wonder is the filtering
>> out based on "[GNUPG:]" prefix.  Why do we need to filter them out?
>
> The [GNUPG:] lines are part of the status-fd protocol. They show details
> that don't really seem interesting to the user. In fact, they even
> contain the signed message (yes, in my case, it turns out gpg was
> actually still signing, but returned an error code...).

OK, that detail was what was missing in the proposed log message.
Without that "why do we filter?", readers cannot correctly assess
why it is a good idea.  I wasn't arguing against filtering it; I
just wanted to make sure "git log" readers (and "git show" user
after "git blame" identifies this change in the history) will know
how we decided to filter lines with the prefix.  

With that information recorded in the log (or in-code comment, or
both), if it turns out that some lines with the prefix are useful
(or some other lines without the prefix are not very useful), they
can tweak the filtering criteria as appropriate, with confidence
that they _know_ for what purpose the initial "filter lines with the
prefix" was trying to serve, and their update is still in the same
spirit as the original, only executed better.

Thanks.

Re: Force Confirmation for Dropping Changed Lines

[Junio C Hamano]

Jacob Keller  writes:

> Setting the merge driver to "unset" will do what you want, but it
> would leave the current branch as the tentative answer and doesn't
> actually make it easy to resolve properly. That would only require
> putting "pom.xml merge=unset" in the .gitattributes file.

Where did you get that "unset" from?  If that is this paragraph in
Documentation/gitattributes.txt:

Unset::

Take the version from the current branch as the
tentative merge result, and declare that the merge has
conflicts.  This is suitable for binary files that do
not have a well-defined merge semantics.

you'd need to read the beginning of the file to learn how to declare
that an attribute is Unset for the path.  merge=unset is setting it
to a string value, i.e. you are doing

String::

3-way merge is performed using the specified custom
merge driver.  The built-in 3-way merge driver can be
explicitly specified by asking for "text" driver; the
built-in "take the current branch" driver can be
requested with "binary".

instead, specifying a custom merge driver "unset", which would
require something like

[merge "unset"]
name = feel-free merge driver
driver = filfre %O %A %B %L %P
recursive = binary

in your configuration file.

> That might be what you want, but it doesn't actually try to update the
> file during the merge so you'd have to hand-fix it yourself.

I think you should be able to do something like

$ cat >$HOME/bin/fail-3way <<\EOF
#!/bin/sh
git merge-file "$@"
exit 1
EOF
$ chmod +x $HOME/bin/fail-3way
$ cat >>$HOME/.gitconfig <<\EOF
[merge "fail"]
name = always fail 3-way merge
driver = $HOME/bin/fail-3way %A %O %B
recursive = text
EOF
$ echo pom.xml merge=fail >>.gitattributes

to define a custom merge driver whose name is "fail", that runs the
fail-3way program, which runs the bog standard 3-way merge we use
(so that it will do the best-effort textual merge) but always return
with a non-zero status to signal that the result is conflicting and
needs manual resolution.

[PATCH] refs: add option core.logAllRefUpdates = always

[cornelius . weig]

Hi peff,

 you made it easy for me. Most of your patch still applied, only the tests
didn't quite fit. Maybe you can have a look if I've overlooked something, since
you know the changes best?

Thanks for supporting this with your patch!

[PATCH] refs: add option core.logAllRefUpdates = always

[cornelius . weig]

From: Cornelius Weig 

When core.logallrefupdates is true, we only create a new reflog for refs
that are under certain well-known hierarchies. The reason is that we
know that some hierarchies (like refs/tags) do not typically change, and
that unknown hierarchies might not want reflogs at all (e.g., a
hypothetical refs/foo might be meant to change often and drop old
history immediately).

However, sometimes it is useful to override this decision and simply log
for all refs, because the safety and audit trail is more important than
the performance implications of keeping the log around.

This patch introduces a new "always" mode for the core.logallrefupdates
option which will log updates to everything under refs/, regardless
where in the hierarchy it is (we still will not log things like
ORIG_HEAD and FETCH_HEAD, which are known to be transient).

Based-on-patch-by: Jeff King 
Signed-off-by: Cornelius Weig 
---
 Documentation/config.txt  |  7 +--
 Documentation/git-tag.txt |  3 ++-
 branch.c  |  2 +-
 builtin/init-db.c |  2 +-
 cache.h   |  9 +++-
 config.c  |  7 ++-
 environment.c |  2 +-
 refs.c| 15 +-
 refs/files-backend.c  |  6 +++---
 t/t1400-update-ref.sh | 53 +++
 10 files changed, 90 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4c..2117616 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -517,10 +517,13 @@ core.logAllRefUpdates::
"`$GIT_DIR/logs/`", by appending the new and old
SHA-1, the date/time and the reason of the update, but
only when the file exists.  If this configuration
-   variable is set to true, missing "`$GIT_DIR/logs/`"
+   variable is set to `true`, missing "`$GIT_DIR/logs/`"
file is automatically created for branch heads (i.e. under
refs/heads/), remote refs (i.e. under refs/remotes/),
-   note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+   `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+   note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
+   If it is set to `always`, then a missing reflog is automatically
+   created for any ref under `refs/`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 5055a96..2ac25a9 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -150,7 +150,8 @@ This option is only applicable when listing tags without 
annotation lines.
'strip' removes both whitespace and commentary.
 
 --create-reflog::
-   Create a reflog for the tag.
+   Create a reflog for the tag. To globally enable reflogs for tags, see
+   `core.logAllRefUpdates` in linkgit:git-config[1].
 
 ::
The name of the tag to create, delete, or describe.
diff --git a/branch.c b/branch.c
index c431cbf..b955d4f 100644
--- a/branch.c
+++ b/branch.c
@@ -298,7 +298,7 @@ void create_branch(const char *name, const char *start_name,
 start_name);
 
if (reflog)
-   log_all_ref_updates = 1;
+   log_all_ref_updates = LOG_REFS_NORMAL;
 
if (!dont_change_ref) {
struct ref_transaction *transaction;
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 76d68fa..1d4d6a0 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,
const char *work_tree = get_git_work_tree();
git_config_set("core.bare", "false");
/* allow template config file to override the default */
-   if (log_all_ref_updates == -1)
+   if (log_all_ref_updates == LOG_REFS_UNSET)
git_config_set("core.logallrefupdates", "true");
if (needs_work_tree_config(original_git_dir, work_tree))
git_config_set("core.worktree", work_tree);
diff --git a/cache.h b/cache.h
index 00a029a..96eeaaf 100644
--- a/cache.h
+++ b/cache.h
@@ -660,7 +660,6 @@ extern int minimum_abbrev, default_abbrev;
 extern int ignore_case;
 extern int assume_unchanged;
 extern int prefer_symlink_refs;
-extern int log_all_ref_updates;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
 extern const char *apply_default_whitespace;
@@ -728,6 +727,14 @@ enum hide_dotfiles_type {
 };
 extern enum hide_dotfiles_type hide_dotfiles;
 
+enum log_refs_config {
+   LOG_REFS_UNSET = -1,
+   LOG_REFS_NONE = 0,
+   LOG_REFS_NORMAL,
+   LOG_REFS_ALWAYS
+};
+extern enum log_refs_config log_all_ref_updates;
+
 enum branch_track {
BRANCH_TRACK_UNSPECIFIED = -1,

Re: [RFC 12/14] fetch-pack: do not printf after closing stdout

[Stefan Beller]

On Wed, Jan 25, 2017 at 2:03 PM, Jonathan Tan  wrote:
> In fetch-pack, during a stateless RPC, printf is invoked after stdout is
> closed. Update the code to not do this, preserving the existing
> behavior.

This seems to me as if it could go as an independent
bugfix(?) or refactoring as this seems to be unclear from the code?

>
> Signed-off-by: Jonathan Tan 
> ---
>  builtin/fetch-pack.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index ae073ab24..24af3b7c5 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -191,10 +191,10 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
> printf("connectivity-ok\n");
> fflush(stdout);
> }
> -   close(fd[0]);
> -   close(fd[1]);
> -   if (finish_connect(conn))
> -   return 1;
> +   if (finish_connect(conn)) {
> +   ret = 1;
> +   goto cleanup;
> +   }
>
> ret = !ref;
>
> @@ -218,11 +218,17 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
> ret = 1;
> }
>
> +   if (args.stateless_rpc)
> +   goto cleanup;
> +
> while (ref) {
> printf("%s %s\n",
>oid_to_hex(>old_oid), ref->name);
> ref = ref->next;
> }
>
> +cleanup:
> +   close(fd[0]);
> +   close(fd[1]);
> return ret;
>  }
> --
> 2.11.0.483.g087da7b7c-goog
>

Re: merge maintaining history

[Jacob Keller]

On Wed, Jan 25, 2017 at 4:31 PM, David J. Bakeman  wrote:
> OK so what I've done so far is to clone the original then I added
> another remote connected to new repo.  Then I did git merge newrepo.  It
> did a bunch of stuff that flashed by really fast and then reported a
> conflict.  Now if I do a git st there are a bunch of files that seem to
> be already added to a commit and all the files with conflicts which it's
> says need to be fixed and added.
> I'm still learning git even after using it for several years.  I've
> never really seen this before.  So the already added files are the ones
> that git was able to merge mechanically?  If so can I diff those changes
> some way?  Would I have to un add (reset HEAD) all those files to see
> the diffs?  Would it have assumed that my changes are to be preferred?
>
> Thanks again for all the great help!

Try "git diff --cached" to show all the current differences saved in the index.

Thanks,
Jake

Re: Force Confirmation for Dropping Changed Lines

[Jacob Keller]

On Wed, Jan 25, 2017 at 3:57 PM, Hilco Wijbenga
 wrote:
> On 25 January 2017 at 15:46, Junio C Hamano  wrote:
>> Jacob Keller  writes:
>>
 Mmm, that sounds complex. The "my-code.x" is made up so I could keep
 my example as simple as possible. In reality, it's Maven's POM files
 (pom.xml).

 So there is no setting for any of this? There is no way to switch off
 auto merging for certain files?
>>>
>>> Not really sure, but a quick google search revealed
>>> https://github.com/ralfth/pom-merge-driver
>>>
>>> Maybe that will help you?
>>
>> Its readme seems to indicate that it is either solving a different
>> problem, or solving the same problem with the opposite goal in mind,
>> in that its goal seems to be to forcibly resolve what textually does
>> not resolve cleanly by choosing sides with an arbitrary policy, so
>> that humans do not have to get involved when they ordinarily would
>> have to.
>>
>> Hilco's goal sounded to me the opposite---to force conflict even
>> when the two histories did what textually does resolve cleanly and
>> require humans to get involved even when they ordinarily wouldn't
>> have to.
>
> Yes, unfortunately, you are correct. This seems to do the exact
> opposite of what I want.
>
> Before I start learning about custom merge drivers, is what I want
> even possible? If yes, would you happen to know some good examples of
> (custom) merge drivers? (Python, Ruby, Java are all okay.)

Setting the merge driver to "unset" will do what you want, but it
would leave the current branch as the tentative answer and doesn't
actually make it easy to resolve properly. That would only require
putting "pom.xml merge=unset" in the .gitattributes file.

That might be what you want, but it doesn't actually try to update the
file during the merge so you'd have to hand-fix it yourself.

Thanks,
Jake

[PATCH] Revert "push: change submodule default to check when submodules exist"

[Stefan Beller]

This reverts commit 04a1d8b1ae5eeecb90675cfaca2850bf26269485.

In the previous commit we set push.recurseSubmodules to "check"
by default. This check is done by walking all remote refs that are known.

For remotes we only store the refs/heads/* (and tags), which doesn't
include all commits. In e.g. Gerrit commits often end up at refs/changes/*
(that we do not store) when pushing to refs/for/master (which we also do
not store). So a workflow such as the following still fails:

$ git -C  push origin HEAD:refs/for/master
$ git push origin HEAD:refs/for/master
The following submodule paths contain changes that can
not be found on any remote:
  submodule

Please try

git push --recurse-submodules=on-demand

or cd to the path and use

git push

to push them to a remote.

Trying to push with --recurse-submodules={on-demand,check}
would run into the same problem, yielding the same error message.

So changing the default to check for submodules is clearly not working
as intended for Gerrit users. We need another option that works for them.
For now just revert the change of the default.

A future patch to address this problem would be add another option that
treats the submodules differently:

  1) check if the submodule needs pushing (as currently done in "check")
  2) for those submodules that need pushing we run a command, e.g.
 git push with the refspec passed down exactly as is. This would
 work for the Gerrit case, as HEAD:refs/for/master is correct
 for both the superproject and the submodule.
  3) Unlike in "on-demand", we would not check again after performing
 step 2), as we would not find the newly pushed things at Gerrit.

Once we have such an option, we can default to "check" again, and the
error message would mention both on-demand as well as this new option.
I'd imagine "on-demand" is what works in branch driven code review
systems such as github; for Gerrit which is based on changes the option
outlined above would work.

Signed-off-by: Stefan Beller 
---

  After some thought, my opinion on
  sb/push-make-submodule-check-the-default
  change. We should not merge it down to master
  until we found a good solution.
  
  This applies on top of sb/push-make-submodule-check-the-default
  unbreaking existing Gerrit users, explaining why this was a bad idea.
  
  Feel free to either apply this patch or just eject said branch from
  next.
  
  Thanks,
  Stefan  

 builtin/push.c | 16 +---
 t/t5531-deep-submodule-push.sh |  6 +-
 2 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 9e0b8dba9a..3bb9d6b7e6 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -3,7 +3,6 @@
  */
 #include "cache.h"
 #include "refs.h"
-#include "dir.h"
 #include "run-command.h"
 #include "builtin.h"
 #include "remote.h"
@@ -23,7 +22,6 @@ static int deleterefs;
 static const char *receivepack;
 static int verbosity;
 static int progress = -1;
-static int has_submodules_configured;
 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static enum transport_family family;
 
@@ -33,15 +31,6 @@ static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
 
-static void preset_submodule_default(void)
-{
-   if (has_submodules_configured || file_exists(git_path("modules")) ||
-   (!is_bare_repository() && file_exists(".gitmodules")))
-   recurse_submodules = RECURSE_SUBMODULES_CHECK;
-   else
-   recurse_submodules = RECURSE_SUBMODULES_OFF;
-}
-
 static void add_refspec(const char *ref)
 {
refspec_nr++;
@@ -506,9 +495,7 @@ static int git_push_config(const char *k, const char *v, 
void *cb)
const char *value;
if (!git_config_get_value("push.recursesubmodules", ))
recurse_submodules = 
parse_push_recurse_submodules_arg(k, value);
-   } else if (starts_with(k, "submodule.") && ends_with(k, ".url"))
-   /* The submodule..url is used as a bit to indicate 
existence */
-   has_submodules_configured = 1;
+   }
 
return git_default_config(k, v, NULL);
 }
@@ -565,7 +552,6 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
};
 
packet_trace_identity("push");
-   preset_submodule_default();
git_config(git_push_config, );
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
set_push_cert_flags(, push_cert);
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index e690749e8a..198ce84754 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -65,11 +65,7 @@ test_expect_success 'push fails if submodule commit not on 
remote' '
git add gar/bage &&
git commit -m "Third commit for gar/bage" &&
# the push should fail with --recurse-submodules=check
-   

Re: merge maintaining history

[David J. Bakeman]

On 01/20/2017 03:37 AM, Jakub Narębski wrote:
> W dniu 19.01.2017 o 22:42, Junio C Hamano pisze:
>> "David J. Bakeman"  writes:
>  
> [...]
>>> Thanks I think that's close but it's a little more complicated I think
>>> :<(  I don't know if this diagram will work but lets try.
>>>
>>> original A->B->C->D->E->F
>>>  \
>>> first branch  b->c->d->e
>>>
>>> new repo e->f->g->h
>>>
>>> Now I need to merge h to F without loosing b through h hopefully.  Yes e
>>> was never merged back to the original repo and it's essentially gone now
>>> so I can't just merge to F or can I?
>> With the picture, I think you mean 'b' is forked from 'B' and the
>> first branch built 3 more commits on top, leading to 'e'.
>>
>> You say "new repo" has 'e' thru 'h', and I take it to mean you
>> started developing on top of the history that leads to 'e' you built
>> in the first branch, and "new repo" has the resulting history that
>> leads to 'h'.
>>
>> Unless you did something exotic and non-standard, commit 'e' in "new
>> repo" would be exactly the same as 'e' sitting on the tip of the
>> "first branch", so the picture would be more like:
>>
>>> original A->B->C->D->E->F
>>>  \
>>> first branch  b->c->d->e
>>> \
>>> new repo f->g->h
>> no?
> On the other hand Git has you covered even if you did something 
> non-standard, like starting new repo from the _state_ of 'e', that
> is you have just copied files and created new repository, having
> 'e' (or actually 'e*') as an initial commit.
>
>original A<-B<-C<-D<-E<-F
> \
>first branch  b<-c<-d<-e
>
>new repo   e*<-f<-g<-h
>
> Note that arrows are in reverse direction, as it is newer commit
> pointing to its parents, not vice versa.
>
> Assuming that you have everything in a single repository, by adding
> both original and new repo as "remotes", you can use 'git replace'
> command to replace 'e*' with 'e'.
>
>original A<-B<-C<-D<-E<-F
> \
>first branch  b<-c<-d<-e
>\
>new repo \-f<-g<-h
>(with refs/replace)
>
>> Then merging 'h' into 'F' will pull everything you did since
>> you diverged from the history that leads to 'F', resulting in a
>> history of this shape:
>>
>>> original A->B->C->D->E->F--M
>>>  \/
>>> first branch  b->c->d->e /
>>> \   /
>>> new repo f->g->h
> Then you would have the above history in repositories that fetched
> refs/replace/*, and the one below if replacement info is absent:
>
>original A<-B<-C<-D<-E<-F<---M
> \  /
>first branch  b<-c<-d<-e   /
>  /
>new repo   e*<-f->g->h
>
> But as Junio said it is highly unlikely that you are in this situation.
>
> HTH
OK so what I've done so far is to clone the original then I added
another remote connected to new repo.  Then I did git merge newrepo.  It
did a bunch of stuff that flashed by really fast and then reported a
conflict.  Now if I do a git st there are a bunch of files that seem to
be already added to a commit and all the files with conflicts which it's
says need to be fixed and added.
I'm still learning git even after using it for several years.  I've
never really seen this before.  So the already added files are the ones
that git was able to merge mechanically?  If so can I diff those changes
some way?  Would I have to un add (reset HEAD) all those files to see
the diffs?  Would it have assumed that my changes are to be preferred?

Thanks again for all the great help!
<>

[PATCH] Documentation: implement linkgit macro for Asciidoctor

[brian m. carlson]

AsciiDoc uses a configuration file to implement macros like linkgit,
while Asciidoctor uses Ruby extensions.  Implement a Ruby extension that
implements the linkgit macro for Asciidoctor in the same way that
asciidoc.conf does for AsciiDoc.  Adjust the Makefile to use it by
default.

Signed-off-by: brian m. carlson 
---
 Documentation/Makefile  |  5 +
 Documentation/asciidoctor-extensions.rb | 28 
 2 files changed, 29 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/asciidoctor-extensions.rb

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 19c42eb60..d1b7a6865 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -179,10 +179,7 @@ ASCIIDOC = asciidoctor
 ASCIIDOC_CONF =
 ASCIIDOC_HTML = xhtml5
 ASCIIDOC_DOCBOOK = docbook45
-ifdef ASCIIDOCTOR_EXTENSIONS_LAB
-ASCIIDOC_EXTRA = -I$(ASCIIDOCTOR_EXTENSIONS_LAB) -rasciidoctor/extensions 
-rman-inline-macro
-endif
-ASCIIDOC_EXTRA += -alitdd='&\#x2d;&\#x2d;'
+ASCIIDOC_EXTRA += -I. -rasciidoctor-extensions -alitdd='&\#x2d;&\#x2d;'
 DBLATEX_COMMON =
 endif
 
diff --git a/Documentation/asciidoctor-extensions.rb 
b/Documentation/asciidoctor-extensions.rb
new file mode 100644
index 0..09f7088ee
--- /dev/null
+++ b/Documentation/asciidoctor-extensions.rb
@@ -0,0 +1,28 @@
+require 'asciidoctor'
+require 'asciidoctor/extensions'
+
+module Git
+  module Documentation
+class LinkGitProcessor < Asciidoctor::Extensions::InlineMacroProcessor
+  use_dsl
+
+  named :chrome
+
+  def process(parent, target, attrs)
+if parent.document.basebackend? 'html'
+  prefix = parent.document.attr('git-relative-html-prefix')
+  %(#{target}(#{attrs[1]})\n)
+elsif parent.document.basebackend? 'docbook'
+  %(
+#{target}#{attrs[1]}
+
+)
+end
+  end
+end
+  end
+end
+
+Asciidoctor::Extensions.register do
+  inline_macro Git::Documentation::LinkGitProcessor, :linkgit
+end
-- 
2.11.0

Re: Force Confirmation for Dropping Changed Lines

[Hilco Wijbenga]

On 25 January 2017 at 15:46, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>>> Mmm, that sounds complex. The "my-code.x" is made up so I could keep
>>> my example as simple as possible. In reality, it's Maven's POM files
>>> (pom.xml).
>>>
>>> So there is no setting for any of this? There is no way to switch off
>>> auto merging for certain files?
>>
>> Not really sure, but a quick google search revealed
>> https://github.com/ralfth/pom-merge-driver
>>
>> Maybe that will help you?
>
> Its readme seems to indicate that it is either solving a different
> problem, or solving the same problem with the opposite goal in mind,
> in that its goal seems to be to forcibly resolve what textually does
> not resolve cleanly by choosing sides with an arbitrary policy, so
> that humans do not have to get involved when they ordinarily would
> have to.
>
> Hilco's goal sounded to me the opposite---to force conflict even
> when the two histories did what textually does resolve cleanly and
> require humans to get involved even when they ordinarily wouldn't
> have to.

Yes, unfortunately, you are correct. This seems to do the exact
opposite of what I want.

Before I start learning about custom merge drivers, is what I want
even possible? If yes, would you happen to know some good examples of
(custom) merge drivers? (Python, Ruby, Java are all okay.)

Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.

[Mike Hommey]

On Wed, Jan 25, 2017 at 03:04:38PM -0800, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> > For instance, after changing my laptop for a new one, I copied my
> > configs, but had some environment differences that broke gpg.
> > With this change applied, the output becomes, on this new machine:
> >   gpg: keyblock resource '/usr/share/keyrings/debian-keyring.gpg': No
> > such file or directory
> >   error: gpg failed to sign the data
> >   error: unable to sign the tag
> >
> > which makes it clearer what's wrong.
> 
> Overall I think this is a good thing to do.  Instead of eating the
> status output, showing what we got, especially when we know the
> command failed, would make the bug-hunting of user's environment
> easier, like you showed above.
> 
> The only thing in the design that makes me wonder is the filtering
> out based on "[GNUPG:]" prefix.  Why do we need to filter them out?

The [GNUPG:] lines are part of the status-fd protocol. They show details
that don't really seem interesting to the user. In fact, they even
contain the signed message (yes, in my case, it turns out gpg was
actually still signing, but returned an error code...).

For instance, in that failed run above, the entire output looks like:

gpg: keyblock resource '/usr/share/keyrings/debian-keyring.gpg': No
such file or directory
[GNUPG:] ERROR add_keyblock_resource ...
[GNUPG:] KEY_CONSIDERED ...
[GNUPG:] BEGIN_SIGNING H2
[GNUPG:] SIG_CREATED ...

All the [GNUPG:] lines are implementation details that don't seem
particularly useful. A case could be made for the ERROR one, though,
although it's also implementation detail-y.

> Implementation-wise, I'd be happier if we do not add any new
> callsites of strbuf_split(), which is a horrible interface.  But
> that is a minor detail.

What would you suggest otherwise?

Mike

[PATCHv3] submodule update: run custom update script for initial populating as well

[Stefan Beller]

In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
2011-02-17), all actions were defaulted to checkout for populating
a submodule initially, because merging or rebasing makes no sense
in that situation.

Other commands however do make sense, such as the custom command
that was added later (6cb5728c43, submodule update: allow custom
command to update submodule working tree, 2013-07-03).

I am unsure about the "none" command, as I can see an initial
checkout there as a useful thing. On the other hand going strictly
by our own documentation, we should do nothing in case of "none"
as well, because the user asked for it.

Reported-by: Han-Wen Nienhuys 
Signed-off-by: Stefan Beller 
---

 Thanks, Brandon, for spotting the unneeded subshell.
 I also fixed the && chaining along the way.
 
 Thanks,
 Stefan

 git-submodule.sh|  5 -
 t/t7406-submodule-update.sh | 12 
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9788175979..63fc4fe9bc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -607,7 +607,10 @@ cmd_update()
if test $just_cloned -eq 1
then
subsha1=
-   update_module=checkout
+   case "$update_module" in
+   merge | rebase | none)
+   update_module=checkout ;;
+   esac
else
subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
git rev-parse --verify HEAD) ||
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 725bbed1f8..347857fa7c 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -441,6 +441,16 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
test_i18ncmp actual expect
 '
 
+test_expect_success 'submodule update - command run for initial population of 
submodule' '
+   cat <<-\ EOF >expect
+   Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
+   EOF &&
+   rm -rf super/submodule &&
+   test_must_fail git -C super submodule update >../actual &&
+   test_cmp expect actual &&
+   git -C super submodule update --checkout
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path 
'../super/submodule'
 Failed to recurse into submodule path '../super'
@@ -493,6 +503,7 @@ test_expect_success 'submodule init picks up merge' '
 '
 
 test_expect_success 'submodule update --merge  - ignores --merge  for new 
submodules' '
+   test_config -C super submodule.submodule.update checkout &&
(cd super &&
 rm -rf submodule &&
 git submodule update submodule &&
@@ -505,6 +516,7 @@ test_expect_success 'submodule update --merge  - ignores 
--merge  for new submod
 '
 
 test_expect_success 'submodule update --rebase - ignores --rebase for new 
submodules' '
+   test_config -C super submodule.submodule.update checkout &&
(cd super &&
 rm -rf submodule &&
 git submodule update submodule &&
-- 
2.11.0.495.g04f60290a0.dirty

Re: Force Confirmation for Dropping Changed Lines

[Junio C Hamano]

Jacob Keller  writes:

>> Mmm, that sounds complex. The "my-code.x" is made up so I could keep
>> my example as simple as possible. In reality, it's Maven's POM files
>> (pom.xml).
>>
>> So there is no setting for any of this? There is no way to switch off
>> auto merging for certain files?
>
> Not really sure, but a quick google search revealed
> https://github.com/ralfth/pom-merge-driver
>
> Maybe that will help you?

Its readme seems to indicate that it is either solving a different
problem, or solving the same problem with the opposite goal in mind,
in that its goal seems to be to forcibly resolve what textually does
not resolve cleanly by choosing sides with an arbitrary policy, so
that humans do not have to get involved when they ordinarily would
have to.

Hilco's goal sounded to me the opposite---to force conflict even
when the two histories did what textually does resolve cleanly and
require humans to get involved even when they ordinarily wouldn't
have to.

Re: [PATCHv2] submodule update: run custom update script for initial populating as well

[Brandon Williams]

On 01/25, Stefan Beller wrote:
> In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
> 2011-02-17), all actions were defaulted to checkout for populating
> a submodule initially, because merging or rebasing makes no sense
> in that situation.
> 
> Other commands however do make sense, such as the custom command
> that was added later (6cb5728c43, submodule update: allow custom
> command to update submodule working tree, 2013-07-03).
> 
> I am unsure about the "none" command, as I can see an initial
> checkout there as a useful thing. On the other hand going strictly
> by our own documentation, we should do nothing in case of "none"
> as well, because the user asked for it.
> 
> Reported-by: Han-Wen Nienhuys 
> Signed-off-by: Stefan Beller 
> ---
>  git-submodule.sh|  5 -
>  t/t7406-submodule-update.sh | 15 +++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9788175979..63fc4fe9bc 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -607,7 +607,10 @@ cmd_update()
>   if test $just_cloned -eq 1
>   then
>   subsha1=
> - update_module=checkout
> + case "$update_module" in
> + merge | rebase | none)
> + update_module=checkout ;;
> + esac
>   else
>   subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
>   git rev-parse --verify HEAD) ||
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 725bbed1f8..2f83243c7d 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -441,6 +441,19 @@ test_expect_success 'submodule update - command in 
> .git/config catches failure -
>   test_i18ncmp actual expect
>  '
>  
> +test_expect_success 'submodule update - command run for initial population 
> of submodule' '
> + cat <<-\ EOF >expect
> + Execution of '\''false $submodulesha1'\'' failed in submodule path 
> '\''submodule'\''
> + EOF
> + (
> + cd super &&
> + rm -rf submodule
> + test_must_fail git submodule update >../actual
> + )
> + test_cmp expect actual
> + git -C super submodule update --checkout
> +'

You can probably get away without the subshell:

rm -rf super/submodule
test_must_fail git -C super submodule upsate >actual

> +
>  cat << EOF >expect
>  Execution of 'false $submodulesha1' failed in submodule path 
> '../super/submodule'
>  Failed to recurse into submodule path '../super'
> @@ -493,6 +506,7 @@ test_expect_success 'submodule init picks up merge' '
>  '
>  
>  test_expect_success 'submodule update --merge  - ignores --merge  for new 
> submodules' '
> + test_config -C super submodule.submodule.update checkout &&
>   (cd super &&
>rm -rf submodule &&
>git submodule update submodule &&
> @@ -505,6 +519,7 @@ test_expect_success 'submodule update --merge  - ignores 
> --merge  for new submod
>  '
>  
>  test_expect_success 'submodule update --rebase - ignores --rebase for new 
> submodules' '
> + test_config -C super submodule.submodule.update checkout &&
>   (cd super &&
>rm -rf submodule &&
>git submodule update submodule &&
> -- 
> 2.11.0.495.g04f60290a0.dirty
> 

-- 
Brandon Williams

Re: [PATCH v2 0/7] Macros for Asciidoctor support

[brian m. carlson]

On Wed, Jan 25, 2017 at 06:30:00PM -0500, Jeff King wrote:
> On Wed, Jan 25, 2017 at 11:19:26PM +, brian m. carlson wrote:
> 
> > On Wed, Jan 25, 2017 at 04:35:44PM -0500, Jeff King wrote:
> > > On Wed, Jan 25, 2017 at 02:28:55PM +0100, Johannes Schindelin wrote:
> > > 
> > > > > The need for the extensions could be replaced with a small amount of
> > > > > Ruby code, if that's considered desirable.  Previous opinions on doing
> > > > > so were negative, however.
> > > > 
> > > > Quite frankly, it is annoying to be forced to install the extensions. I
> > > > would much rather have the small amount of Ruby code in Git's 
> > > > repository.
> > > 
> > > Me too. Dependencies can be a big annoyance. I'd reserve judgement until
> > > I saw the actual Ruby code, though. :)
> > 
> > I've sent the patch before, but I can send it again.  It's relatively
> > small and self-contained.  I'm also happy to be responsible for
> > maintaining it.
> 
> Ah, it's:
> 
>   
> http://public-inbox.org/git/1413070656-241955-5-git-send-email-sand...@crustytoothpaste.net/
> 
> (and note there is some surrounding discussion there).
> 
> The code is not _too_ bad. The main thing is that it would have to be
> kept up to date with changes to asciidoc.conf's version of the linkgit
> macro. But it's not like that changes frequently.

Yes.  I think I can actually simplify it some more, since we always seem
to use the argument to linkgit, so I'll send out a simplified patch in a
few minutes.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] tag: add tag.createReflog option

[Cornelius Weig]

> 
> It may have been obvious, but to be explicit for somebody new,
> !prefixcmp() corresponds to starts_with().  IOW, we changed the
> meaning of the return value when moving from cmp-lookalike (where 0
> means "equal") to "does it start with this string?" bool (where 1
> means "yes").
> 

I see. It reads much better that way!

I re-did all the changes from Jeff's patch, but some tests are breaking
now. I will have to mend that tomorrow, because it's already too late in
my timezone.

Thanks a lot for your support m(_ _)m

Re: [PATCHv3 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves

[Brandon Williams]

On 01/25, Stefan Beller wrote:
> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
> When nested is already absorbed into sub, but sub is not absorbed into
> its superproject, then we need to fixup the gitfile and core.worktree
> setting for 'nested' when absorbing 'sub', but we do not need to move
> its git dir around.
> 
> Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
> it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".
> 
> An alternative I considered to do this work lazily, i.e. when resolving
> "../.git/modules/nested", we would notice the ".git" being a gitfile
> linking to another path.  That seemed to be robuster by design, but harder
> to get the implementation right.  Maybe we have to do that anyway once we
> try to have submodules and worktrees working nicely together, but for now
> just produce 'correct' (i.e. direct) pointers.
> 
> Signed-off-by: Stefan Beller 

Looks good to me!

-- 
Brandon Williams

[PATCHv2] submodule update: run custom update script for initial populating as well

[Stefan Beller]

In 1b4735d9f3 (submodule: no [--merge|--rebase] when newly cloned,
2011-02-17), all actions were defaulted to checkout for populating
a submodule initially, because merging or rebasing makes no sense
in that situation.

Other commands however do make sense, such as the custom command
that was added later (6cb5728c43, submodule update: allow custom
command to update submodule working tree, 2013-07-03).

I am unsure about the "none" command, as I can see an initial
checkout there as a useful thing. On the other hand going strictly
by our own documentation, we should do nothing in case of "none"
as well, because the user asked for it.

Reported-by: Han-Wen Nienhuys 
Signed-off-by: Stefan Beller 
---
 git-submodule.sh|  5 -
 t/t7406-submodule-update.sh | 15 +++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9788175979..63fc4fe9bc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -607,7 +607,10 @@ cmd_update()
if test $just_cloned -eq 1
then
subsha1=
-   update_module=checkout
+   case "$update_module" in
+   merge | rebase | none)
+   update_module=checkout ;;
+   esac
else
subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
git rev-parse --verify HEAD) ||
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 725bbed1f8..2f83243c7d 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -441,6 +441,19 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
test_i18ncmp actual expect
 '
 
+test_expect_success 'submodule update - command run for initial population of 
submodule' '
+   cat <<-\ EOF >expect
+   Execution of '\''false $submodulesha1'\'' failed in submodule path 
'\''submodule'\''
+   EOF
+   (
+   cd super &&
+   rm -rf submodule
+   test_must_fail git submodule update >../actual
+   )
+   test_cmp expect actual
+   git -C super submodule update --checkout
+'
+
 cat << EOF >expect
 Execution of 'false $submodulesha1' failed in submodule path 
'../super/submodule'
 Failed to recurse into submodule path '../super'
@@ -493,6 +506,7 @@ test_expect_success 'submodule init picks up merge' '
 '
 
 test_expect_success 'submodule update --merge  - ignores --merge  for new 
submodules' '
+   test_config -C super submodule.submodule.update checkout &&
(cd super &&
 rm -rf submodule &&
 git submodule update submodule &&
@@ -505,6 +519,7 @@ test_expect_success 'submodule update --merge  - ignores 
--merge  for new submod
 '
 
 test_expect_success 'submodule update --rebase - ignores --rebase for new 
submodules' '
+   test_config -C super submodule.submodule.update checkout &&
(cd super &&
 rm -rf submodule &&
 git submodule update submodule &&
-- 
2.11.0.495.g04f60290a0.dirty

Re: [PATCH v2 0/7] Macros for Asciidoctor support

[Jeff King]

On Wed, Jan 25, 2017 at 11:19:26PM +, brian m. carlson wrote:

> On Wed, Jan 25, 2017 at 04:35:44PM -0500, Jeff King wrote:
> > On Wed, Jan 25, 2017 at 02:28:55PM +0100, Johannes Schindelin wrote:
> > 
> > > > The need for the extensions could be replaced with a small amount of
> > > > Ruby code, if that's considered desirable.  Previous opinions on doing
> > > > so were negative, however.
> > > 
> > > Quite frankly, it is annoying to be forced to install the extensions. I
> > > would much rather have the small amount of Ruby code in Git's repository.
> > 
> > Me too. Dependencies can be a big annoyance. I'd reserve judgement until
> > I saw the actual Ruby code, though. :)
> 
> I've sent the patch before, but I can send it again.  It's relatively
> small and self-contained.  I'm also happy to be responsible for
> maintaining it.

Ah, it's:

  
http://public-inbox.org/git/1413070656-241955-5-git-send-email-sand...@crustytoothpaste.net/

(and note there is some surrounding discussion there).

The code is not _too_ bad. The main thing is that it would have to be
kept up to date with changes to asciidoc.conf's version of the linkgit
macro. But it's not like that changes frequently.

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Jacob Keller  writes:

> On Wed, Jan 25, 2017 at 1:27 PM, Jeff King  wrote:
>> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>>
>>> IOW, the ref-selector options build up until a group option is given,
>>> which acts on the built-up options (over that group) and then resets the
>>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>>> think in practice nobody would do that, because it's hard to read).
>>
>> So here's what I would have expected your series to look more like (with
>> probably one patch adding clear_ref_selection_options, and the other
>> adding the decorate stuff):
>>
>
> I agree that this is how I would have expected it to work as well.

That makes three of us ;-)

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:
>
>> -- >8 --
>> Subject: [PATCH] connect: core.sshvariant to correct misidentification
>
> I have been watching this discussion from the sidelines, and I agree
> that this direction is a big improvement.
> ...
> IIRC, the "const" in git_config_get_string_const is only about avoiding
> an annoying cast. The result is still allocated and needs freed. Since
> you are not keeping the value after the function returns, I think you
> could just use git_config_get_value().

It is too late for today's pushout (I have this topic near the tip
of 'pu', so it is possible to tweak and redo only 'pu' branch, but I
generally hate tweaking things after 15:00 my time), but I'll fix
that before the topic hits 'jch' (which is a bit more than 'next'
and that is what I use for everyday work).

Thanks.  

Re: [PATCH] Retire the `relink` command

[Eric Wong]

Johannes Schindelin  wrote:
> Back in the olden days, when all objects were loose and rubber boots were
> made out of wood, it made sense to try to share (immutable) objects
> between repositories.
> 
> Ever since the arrival of pack files, it is but an anachronism.
> 
> Let's move the script to the contrib/examples/ directory and no longer
> offer it.

On the other hand, we have no idea if there are still people
using it for whatever reason...

I suggest we have a deprecation period where:

1) there is warning message when it's run
2) a note in the manpage indicating it's to-be-removed
3) ...then wait a few distro LTS cycles to remove it entirely

Re: diff --color-words breaks spacing

[Jeff King]

On Tue, Jan 24, 2017 at 09:39:31AM -0800, Phil Hord wrote:

> I noticed some weird spacing when comparing files with git diff
> --color-words.  The space before a colored word disappears sometimes.

I _think_ this is working as designed, though it is a bit tricky (and it
may be possible to make it better).

> echo "FOO foo; foo = bar" > a
> echo "FOO foo = baz" > b
> git diff --color-words --no-index a b
> FOOfoo; foo = barbaz

It might be easier to see with --word-diff, which uses the same code but
marks it with punctuation:

  $ git diff --word-diff
  FOO[-foo;-] foo = [-bar-]{+baz+}

The key thing is that what you are seeing is the post-image, plus
word-based annotations. And the post-image does not have that space in
its context, so we would not expect to see:

  FOO [-foo;-] foo = [-bar-]{+baz+}

(you would if we showed the pre-image plus annotations; but then I think
you'd probably end up with the opposite problem when text was added).

However, we also don't see the space in the removed part. I.e., I think:

  FOO[- foo;-] foo = [-bar-]{+baz+}

would be correct. But I think because of the way word-diff is
implemented, a non-word character will never be part of the annotation.

The way it works is basically to break the string down on word
boundaries, so we have:

  pre: FOO, foo;, foo, =, bar
  post: FOO, foo, =, baz

as a set of tokens. We stick each of those on their own line and feed
them back to xdiff, so we get a diff like:

  @@ -1,5 +1,4 @@
   FOO
  -foo;
   foo
   =
  -bar
  +baz

and then walk through the post-image buffer, either inserting chunks of
context from the original buffer, or the changed lines. But the changed
lines themselves do not include the non-word characters, so we have no
idea that a space went away.

-Peff

Re: [PATCH v2 0/7] Macros for Asciidoctor support

[brian m. carlson]

On Wed, Jan 25, 2017 at 04:35:44PM -0500, Jeff King wrote:
> On Wed, Jan 25, 2017 at 02:28:55PM +0100, Johannes Schindelin wrote:
> 
> > > The need for the extensions could be replaced with a small amount of
> > > Ruby code, if that's considered desirable.  Previous opinions on doing
> > > so were negative, however.
> > 
> > Quite frankly, it is annoying to be forced to install the extensions. I
> > would much rather have the small amount of Ruby code in Git's repository.
> 
> Me too. Dependencies can be a big annoyance. I'd reserve judgement until
> I saw the actual Ruby code, though. :)

I've sent the patch before, but I can send it again.  It's relatively
small and self-contained.  I'm also happy to be responsible for
maintaining it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure

[Brandon Williams]

On 01/25, Brandon Williams wrote:
> On 01/25, Junio C Hamano wrote:
> > Brandon Williams  writes:
> > 
> > >> In my mind, the value of having a constant check_attr is primarily
> > >> that it gives us a stable pointer to serve as a hashmap key,
> > >> i.e. the identifier for each call site, in a later iteration.
> > >
> > > We didn't really discuss this notion of having the pointer be a key into
> > > a hashmap, what sort of information are you envisioning being stored in
> > > this sort of hashmap?
> > 
> > The "entries relevant to this attr_check() call, that is specific to
> > the  tuple" (aka "what used to be
> > called the global attr_stack") we discussed would be the primary
> > example.  A thread is likely be looping in a caller that has many
> > paths inside a directory, calling a function that has a call to
> > attr_check() for each path.  Having something that can use to
> > identify the check_attr instance in a stable way, even when the
> > inner function is called and returns many times, would allow us to
> > populate the "attr stack" just once for the thread when it enters a
> > directory for the first time (remember, another thread may be
> > executing the same codepath, checking for paths in a different
> > directory) and keep using it.  There may be other mechanisms you can
> > come up with, so I wouldn't say it is the only valid way, but it is
> > a way.  That is why I said:
> > 
> > >> But all of the above comes from my intuition, and I'll very much
> > >> welcome to be proven wrong with an alternative design, or better
> > >> yet, a working code based on an alternative design ;-).
> > 
> > near the end of my message.
> > 
> > > One issue I can see with this is that the
> > > functions which have a static attr_check struct would still not be thread
> > > safe if the initialization of the structure isn't surrounded by a mutex
> > > itself. ie
> > 
> > Yes, that goes without saying.  That is why I suggested Stefan to do
> > not this:
> > 
> > > static struct attr_check *check;
> > >
> > > if (!check)
> > >   init(check);
> > >
> > > would need to be:
> > >
> > > lock()
> > > if (!check)
> > >   init(check);
> > > unlock();
> > 
> > but this:
> > 
> > static struct attr_check *check;
> > init();
> > 
> > and hide the lock/unlock gymnastics inside the API.  I thought that
> > already was in what you inherited from him and started your work
> > on top of?
> 
> I essentially built off of the series you had while using Stefan's
> patches as inspiration, but I don't believe the kind of mechanism you
> are describing existed in Stefan's series.  His series had a single lock
> for the entire system, only allowing a single caller to be in it at any
> given time.  This definitely isn't ideal, hence why I picked it up.
> 
> Implementation aside I want to try and nail down what the purpose of
> this refactor is.  There are roughly two notions of being "thread-safe".
> 
> 1. The first is that the subsystem itself is thread safe, that is
>multiple threads can be executing inside the subsystem without stepping
>on each others work.
> 
> 2. The second is that the object itself is thread safe or that multiple
>threads can use the same object.
> 
> I thought that the main purpose of this was to achieve (1) since
> currently that is not the case.

Ok, so I discovered a very good reason why we should do as Stefan
originally did and split the question and answer (beyond the reasoning
for using the reference as a hashkey).

One motivation behind making this API thread-safe is that we can use it
in pathspec code to match against attributes.  This means that a
pathspec structure will contain an attr_check member describing the
attributes that a pathspec item is interested in.  Then the pathspec
structure is passed to match_pathspec() as a const pointer.  To me, when
passing something as 'const' I expect none of the members should change
at all.  The struct should remain exactly in the same form as before I
invoked the function.

Requiring the attr_check structure to be modified in the process of a
check_attrs() call would violate this "contract" when calling
match_pathspec() as the attr_check structure would have modified state.
The compiler wouldn't catch this as the "const" modifier isn't passed on
to struct members.

-- 
Brandon Williams

[PATCHv3 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves

[Stefan Beller]

Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
When nested is already absorbed into sub, but sub is not absorbed into
its superproject, then we need to fixup the gitfile and core.worktree
setting for 'nested' when absorbing 'sub', but we do not need to move
its git dir around.

Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".

An alternative I considered to do this work lazily, i.e. when resolving
"../.git/modules/nested", we would notice the ".git" being a gitfile
linking to another path.  That seemed to be robuster by design, but harder
to get the implementation right.  Maybe we have to do that anyway once we
try to have submodules and worktrees working nicely together, but for now
just produce 'correct' (i.e. direct) pointers.

Signed-off-by: Stefan Beller 
---

 This replaces the last patch of the series, containing Brandons SQUASH proposal
 as well as the removal of the goto.
 Thanks,
 Stefan

 submodule.c| 62 --
 t/t7412-submodule-absorbgitdirs.sh | 27 +
 2 files changed, 73 insertions(+), 16 deletions(-)

diff --git a/submodule.c b/submodule.c
index 4c4f033e8a..3b98766a6b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1437,22 +1437,57 @@ void absorb_git_dir_into_superproject(const char 
*prefix,
  const char *path,
  unsigned flags)
 {
-   const char *sub_git_dir, *v;
-   char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
+   int err_code;
+   const char *sub_git_dir;
struct strbuf gitdir = STRBUF_INIT;
-
strbuf_addf(, "%s/.git", path);
-   sub_git_dir = resolve_gitdir(gitdir.buf);
+   sub_git_dir = resolve_gitdir_gently(gitdir.buf, _code);
 
/* Not populated? */
-   if (!sub_git_dir)
-   goto out;
+   if (!sub_git_dir) {
+   char *real_new_git_dir;
+   const char *new_git_dir;
+   const struct submodule *sub;
+
+   if (err_code == READ_GITFILE_ERR_STAT_FAILED) {
+   /* unpopulated as expected */
+   strbuf_release();
+   return;
+   }
+
+   if (err_code != READ_GITFILE_ERR_NOT_A_REPO)
+   /* We don't know what broke here. */
+   read_gitfile_error_die(err_code, path, NULL);
+
+   /*
+   * Maybe populated, but no git directory was found?
+   * This can happen if the superproject is a submodule
+   * itself and was just absorbed. The absorption of the
+   * superproject did not rewrite the git file links yet,
+   * fix it now.
+   */
+   sub = submodule_from_path(null_sha1, path);
+   if (!sub)
+   die(_("could not lookup name for submodule '%s'"), 
path);
+   new_git_dir = git_path("modules/%s", sub->name);
+   if (safe_create_leading_directories_const(new_git_dir) < 0)
+   die(_("could not create directory '%s'"), new_git_dir);
+   real_new_git_dir = real_pathdup(new_git_dir);
+   connect_work_tree_and_git_dir(path, real_new_git_dir);
+
+   free(real_new_git_dir);
+   } else {
+   /* Is it already absorbed into the superprojects git dir? */
+   char *real_sub_git_dir = real_pathdup(sub_git_dir);
+   char *real_common_git_dir = real_pathdup(get_git_common_dir());
 
-   /* Is it already absorbed into the superprojects git dir? */
-   real_sub_git_dir = real_pathdup(sub_git_dir);
-   real_common_git_dir = real_pathdup(get_git_common_dir());
-   if (!skip_prefix(real_sub_git_dir, real_common_git_dir, ))
-   relocate_single_git_dir_into_superproject(prefix, path);
+   if (!starts_with(real_sub_git_dir, real_common_git_dir))
+   relocate_single_git_dir_into_superproject(prefix, path);
+
+   free(real_sub_git_dir);
+   free(real_common_git_dir);
+   }
+   strbuf_release();
 
if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
struct child_process cp = CHILD_PROCESS_INIT;
@@ -1478,9 +1513,4 @@ void absorb_git_dir_into_superproject(const char *prefix,
 
strbuf_release();
}
-
-out:
-   strbuf_release();
-   free(real_sub_git_dir);
-   free(real_common_git_dir);
 }
diff --git a/t/t7412-submodule-absorbgitdirs.sh 
b/t/t7412-submodule-absorbgitdirs.sh
index 1c47780e2b..e2bbb449b6 100755
--- a/t/t7412-submodule-absorbgitdirs.sh
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -64,6 +64,33 @@ test_expect_success 'absorb the git dir in a nested 
submodule' '
test_cmp expect.2 actual.2
 '
 

Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.

[Junio C Hamano]

Mike Hommey  writes:

> For instance, after changing my laptop for a new one, I copied my
> configs, but had some environment differences that broke gpg.
> With this change applied, the output becomes, on this new machine:
>   gpg: keyblock resource '/usr/share/keyrings/debian-keyring.gpg': No
> such file or directory
>   error: gpg failed to sign the data
>   error: unable to sign the tag
>
> which makes it clearer what's wrong.

Overall I think this is a good thing to do.  Instead of eating the
status output, showing what we got, especially when we know the
command failed, would make the bug-hunting of user's environment
easier, like you showed above.

The only thing in the design that makes me wonder is the filtering
out based on "[GNUPG:]" prefix.  Why do we need to filter them out?

Implementation-wise, I'd be happier if we do not add any new
callsites of strbuf_split(), which is a horrible interface.  But
that is a minor detail.

Thanks.

Re: [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves

[Stefan Beller]

On Tue, Jan 24, 2017 at 4:46 PM, Brandon Williams  wrote:
> On 01/24, Stefan Beller wrote:
>> + connect_work_tree_and_git_dir(path, real_new_git_dir);
>
> Memory leak with 'real_new_git_dir'

yup. :(


> The scope of 'real_sub_git_dir' and 'real_common_git_dir' variable can
> be narrowed.  Move their declaration here and free it prior to exiting
> the else block.

ok.

> 'v' isn't ever used, just use starts_with() and get rid of 'v'

makes sense.

>
>> + relocate_single_git_dir_into_superproject(prefix, 
>> path);
>> + }
>>
>
> There's a label 'out' at the end of the function which can be removed as
> there is no 'goto' statement using it.

We do use it in

if (err_code == READ_GITFILE_ERR_STAT_FAILED)
goto out; /* unpopulated as expected */

I took your proposed SQUASH and removed the goto, see the followup email.

Re: [PATCH] tag: add tag.createReflog option

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> +enum log_refs_config {
>> +LOG_REFS_UNSET = -1,
>> +LOG_REFS_NONE = 0,
>> +LOG_REFS_NORMAL, /* see should_create_reflog for rules */
>> +LOG_REFS_ALWAYS
>> +};
>> +extern enum log_refs_config log_all_ref_updates;
>> +...
>> +int should_create_reflog(const char *refname)
>> +{
>> +switch (log_all_ref_updates) {
>> +case LOG_REFS_ALWAYS:
>> +return 1;
>> +case LOG_REFS_NORMAL:
>> +return !prefixcmp(refname, "refs/heads/") ||
>> +   !prefixcmp(refname, "refs/remotes/") ||
>> +   !prefixcmp(refname, "refs/notes/") ||
>> +   !strcmp(refname, "HEAD");
>> +default:
>> +return 0;
>> +}
>> +}
>
> Yup, this is how I expected for the feature to be done.
>
> Just a hint for Cornelius; prefixcmp() is an old name for what is
> called starts_with() these days.

It may have been obvious, but to be explicit for somebody new,
!prefixcmp() corresponds to starts_with().  IOW, we changed the
meaning of the return value when moving from cmp-lookalike (where 0
means "equal") to "does it start with this string?" bool (where 1
means "yes").

Re: [PATCHv2 3/3] submodule absorbing: fix worktree/gitdir pointers recursively for non-moves

[Junio C Hamano]

Stefan Beller  writes:

> Consider having a submodule 'sub' and a nested submodule at 'sub/nested'.
> When nested is already absorbed into sub, but sub is not absorbed into
> its superproject, then we need to fixup the gitfile and core.worktree
> setting for 'nested' when absorbing 'sub', but we do not need to move
> its git dir around.
>
> Previously 'nested's gitfile contained "gitdir: ../.git/modules/nested";
> it has to be corrected to "gitdir: ../../.git/modules/sub1/modules/nested".

That sounds like a sensible way to make things consistent.

Re: Force Confirmation for Dropping Changed Lines

[Jacob Keller]

On Wed, Jan 25, 2017 at 2:51 PM, Hilco Wijbenga
 wrote:
> On 25 January 2017 at 14:24, Jacob Keller  wrote:
>> On Wed, Jan 25, 2017 at 2:16 PM, Hilco Wijbenga
>>  wrote:
>>> How can I force Git to not assume my change to the first line is 
>>> "redundant"?
>>>
>>
>> My guess is that you probably want a custom merge driver for your file
>> types. That's where I would look initially.
>
> Mmm, that sounds complex. The "my-code.x" is made up so I could keep
> my example as simple as possible. In reality, it's Maven's POM files
> (pom.xml).
>
> So there is no setting for any of this? There is no way to switch off
> auto merging for certain files?

Not really sure, but a quick google search revealed
https://github.com/ralfth/pom-merge-driver

Maybe that will help you?

Thanks,
Jake

Re: Force Confirmation for Dropping Changed Lines

[Hilco Wijbenga]

On 25 January 2017 at 14:24, Jacob Keller  wrote:
> On Wed, Jan 25, 2017 at 2:16 PM, Hilco Wijbenga
>  wrote:
>> How can I force Git to not assume my change to the first line is "redundant"?
>>
>
> My guess is that you probably want a custom merge driver for your file
> types. That's where I would look initially.

Mmm, that sounds complex. The "my-code.x" is made up so I could keep
my example as simple as possible. In reality, it's Maven's POM files
(pom.xml).

So there is no setting for any of this? There is no way to switch off
auto merging for certain files?

Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)

[Junio C Hamano]

Jeff King  writes:

> I guess the way to dig would be to add a test that looks at the output
> of "type mv" or something, push it to a Travis-hooked branch, and then
> wait for the output

Sounds tempting ;-)

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Jeff King]

On Wed, Jan 25, 2017 at 02:35:36PM -0800, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] connect: core.sshvariant to correct misidentification

I have been watching this discussion from the sidelines, and I agree
that this direction is a big improvement.

> +static void override_ssh_variant(int *port_option, int *needs_batch)
> +{
> + const char *variant;
> +
> + if (git_config_get_string_const("core.sshvariant", ))
> + return;
> + if (!strcmp(variant, "tortoiseplink")) {
> + *port_option = 'P';
> + *needs_batch = 1;
> + } else if (!strcmp(variant, "putty")) {
> + *port_option = 'P';
> + *needs_batch = 0;
> + } else {
> + /* default */
> + if (strcmp(variant, "ssh")) {
> + warning(_("core.sshvariant: unknown value '%s'"), 
> variant);
> + warning(_("using OpenSSH compatible behaviour"));
> + }
> + *port_option = 'p';
> + *needs_batch = 0;
> + }
> +}

IIRC, the "const" in git_config_get_string_const is only about avoiding
an annoying cast. The result is still allocated and needs freed. Since
you are not keeping the value after the function returns, I think you
could just use git_config_get_value().

(Grepping around, I see a few other places that seem to make the same
mistake. I think this is a confusing interface that should probably be
fixed).

-Peff

Re: SubmittingPatches: drop temporal reference for PGP signing

[Stefan Beller]

On Tue, Jan 24, 2017 at 10:54 PM, Philip Oakley  wrote:
>> -Do not PGP sign your patch, at least for now.  Most likely, your
>> -maintainer or other people on the list would not have your PGP
>> -key and would not bother obtaining it anyway.  Your patch is not
>> -judged by who you are; a good patch from an unknown origin has a
>> -far better chance of being accepted than a patch from a known,
>> -respected origin that is done poorly or does incorrect things.
>> +Do not PGP sign your patch. Most likely, your maintainer or other
>> +people on the list would not have your PGP key and would not bother
>> +obtaining it anyway. Your patch is not judged by who you are; a good
>> +patch from an unknown origin has a far better chance of being accepted
>> +than a patch from a known, respected origin that is done poorly or
>> +does incorrect things.
>
>
> Wouldn't this also benefit from a forward reference to the section 5 on the
> DOC signining? This would avoid Cornelius's case where he felt that section
> 5 no longer applied.

Yeah I agree. My patch was not the best shot by far.

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Junio C Hamano  writes:

> Yes.  Here is what comes on an obvious clean-up patch (which will be
> sent as a follow-up to this message).

... and this is the "obvious clean-up patch" to apply directly on
top of Segev's GIT_SSH_COMMAND support, which comes before the
previous one.

-- >8 --
Subject: [PATCH] connect: rename tortoiseplink and putty variables

One of these two may have originally been named after "what exact
SSH implementation do we have" so that we can tweak the command line
options, but these days "putty=1" no longer means "We are using the
plink SSH implementation that comes with PuTTY".  It is set when we
guess that either PuTTY plink or Tortoiseplink is in use.

Rename them after what effect is desired.  The current "putty"
option is about using "-P " when OpenSSH would use "-p ",
so rename it to port_option whose value is either 'p' or 'P".  The
other one is about passing an extra command line option "-batch",
so rename it needs_batch.

Signed-off-by: Junio C Hamano 
---
 connect.c | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/connect.c b/connect.c
index c81f77001b..aa51b33bfc 100644
--- a/connect.c
+++ b/connect.c
@@ -769,7 +769,8 @@ struct child_process *git_connect(int fd[2], const char 
*url,
conn->in = conn->out = -1;
if (protocol == PROTO_SSH) {
const char *ssh;
-   int putty = 0, tortoiseplink = 0;
+   int needs_batch = 0;
+   int port_option = 'p';
char *ssh_host = hostandport;
const char *port = NULL;
char *ssh_argv0 = NULL;
@@ -819,12 +820,14 @@ struct child_process *git_connect(int fd[2], const char 
*url,
if (ssh_argv0) {
const char *base = basename(ssh_argv0);
 
-   tortoiseplink = !strcasecmp(base, 
"tortoiseplink") ||
-   !strcasecmp(base, "tortoiseplink.exe");
-   putty = tortoiseplink ||
-   !strcasecmp(base, "plink") ||
-   !strcasecmp(base, "plink.exe");
-
+   if (!strcasecmp(base, "tortoiseplink") ||
+   !strcasecmp(base, "tortoiseplink.exe")) {
+   port_option = 'P';
+   needs_batch = 1;
+   } else if (!strcasecmp(base, "plink") ||
+  !strcasecmp(base, "plink.exe")) {
+   port_option = 'P';
+   }
free(ssh_argv0);
}
 
@@ -833,11 +836,10 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)
argv_array_push(>args, "-6");
-   if (tortoiseplink)
+   if (needs_batch)
argv_array_push(>args, "-batch");
if (port) {
-   /* P is for PuTTY, p is for OpenSSH */
-   argv_array_push(>args, putty ? "-P" : 
"-p");
+   argv_array_pushf(>args, "-%c", 
port_option);
argv_array_push(>args, port);
}
argv_array_push(>args, ssh_host);
-- 
2.11.0-699-ga1f1475476

Re: [PATCH] connect: handle putty/plink also in GIT_SSH_COMMAND

[Junio C Hamano]

Johannes Schindelin  writes:

> Now, with the patch in question (without the follow-up, which I would like
> to ask you to ignore, just like you did so far), Git would not figure out
> that your script calls PuTTY eventually. The work-around? Easy:
>
>   DUMMY=/plink.exe /path/to/junio-is-a-superstar.sh

Think about how you would explain that to an end-user in our
document?  You'll need to explain how exactly the auto-detection
works, so that the user can "exploit" the loophole to do that.  And
what maintenance burden does it add when auto-detection is updated?

I think I know you well enough that you know well enough that it is
too ugly to live, and I suspect that the above is a tongue-in-cheek
"arguing for the sake of argument" and would not need a serious
response, but just in case...

> ...
> Of course, given our discussion I think all of this should be documented
> in the commit message as well as in the man page.

Yes.  Here is what comes on an obvious clean-up patch (which will be
sent as a follow-up to this message).

-- >8 --
Subject: [PATCH] connect: core.sshvariant to correct misidentification

While the logic we have been using to guess which variant of SSH
implementation is in use by looking at the name of the program has
been robust enough for GIT_SSH (which does not go through the shell,
hence it can only spell the name the SSH program and nothing else),
extending it to GIT_SSH_COMMAND and core.sshcommand opens it for
larger chance of misidentification, because these can specify
arbitrary shell command, and a simple "the first word on the line
must be the command name" heuristic may not even look at the command
name at all.

As any effort to improve the heuristic will give us only diminishing
returns, and especially given that most of the users are expected to
have a command line for which the heuristic would work correctly,
give an explicit escape hatch to override a misidentification, just
in case one happens.

It is arguably bad to add this new variable to the core.* set, in
the sense that you'll never see this variable if you do not interact
with other repositories over ssh, but core.sshcommand is already
there for some reason, so let's match it.

Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt | 13 +
 connect.c| 26 ++
 2 files changed, 39 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af2ae4cc02..8ea1db49c6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -455,6 +455,19 @@ core.sshCommand::
the `GIT_SSH_COMMAND` environment variable and is overridden
when the environment variable is set.
 
+core.sshVariant::
+   SSH implementations used by Git when running `git fetch`,
+   `git clone`, and `git push` use slightly different command
+   line options (e.g. PuTTY and TortoisePlink use `-P `
+   while OpenSSH uses `-p ` to specify the port number.
+   TortoisePlink in addition requires `-batch` option to be
+   passed).  Git guesses which variant is in use correctly from
+   the name of the ssh command used (see `core.sshCommand`
+   configuration variable and also `GIT_SSH_COMMAND`
+   environment variable) most of the time.  You can set this
+   variable to 'putty', 'tortoiseplink', or 'ssh' to correct it
+   when Git makes an incorrect guess.
+
 core.ignoreStat::
If true, Git will avoid using lstat() calls to detect if files have
changed by setting the "assume-unchanged" bit for those tracked files
diff --git a/connect.c b/connect.c
index aa51b33bfc..358e9c06f0 100644
--- a/connect.c
+++ b/connect.c
@@ -691,6 +691,29 @@ static const char *get_ssh_command(void)
return NULL;
 }
 
+static void override_ssh_variant(int *port_option, int *needs_batch)
+{
+   const char *variant;
+
+   if (git_config_get_string_const("core.sshvariant", ))
+   return;
+   if (!strcmp(variant, "tortoiseplink")) {
+   *port_option = 'P';
+   *needs_batch = 1;
+   } else if (!strcmp(variant, "putty")) {
+   *port_option = 'P';
+   *needs_batch = 0;
+   } else {
+   /* default */
+   if (strcmp(variant, "ssh")) {
+   warning(_("core.sshvariant: unknown value '%s'"), 
variant);
+   warning(_("using OpenSSH compatible behaviour"));
+   }
+   *port_option = 'p';
+   *needs_batch = 0;
+   }
+}
+
 /*
  * This returns a dummy child_process if the transport protocol does not
  * need fork(2), or a struct child_process object if it does.  Once done,
@@ -836,6 +859,9 @@ struct child_process *git_connect(int fd[2], const char 
*url,
argv_array_push(>args, "-4");
else if (flags & CONNECT_IPV6)

Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)

[Stefan Beller]

On Wed, Jan 25, 2017 at 2:26 PM, Jeff King  wrote:
> On Wed, Jan 25, 2017 at 02:11:15PM -0800, Stefan Beller wrote:
>
>> > Thanks. I've applied and pushed, though I'll admit I didn't really read
>> > it carefully for content. A few of the ideas look like they would need
>> > to be aggregated to be big enough for a SoC project, but that can be
>> > fleshed out in future patches (i.e., I don't think we care enough about
>> > history to have people polish and re-roll what are essentially wiki
>> > edits).
>>
>> Yeah, I wasn't sure if the ideas were meant to also contain microprojects
>> so I wrote down everything that came to mind, that I do not intend to work on
>> myself over the next couple month.
>
> Microprojects go on their own page. But make sure that they really are
> "micro" sized for an applicant.

Yeah micro as in real micro.
e.g. fix the SubmittingPatches doc, after confusion about "signing",
start reading here
https://public-inbox.org/git/923cd4e4-5c9c-4eaf-0fea-6deff6875...@tngtech.com/

(I did not push it, as I'd need to copy over the micro projects page from 2016,
and then find out where to put links to have the web page not look broken)

Thanks,
Stefan

Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)

[Jeff King]

On Wed, Jan 25, 2017 at 02:11:15PM -0800, Stefan Beller wrote:

> > Thanks. I've applied and pushed, though I'll admit I didn't really read
> > it carefully for content. A few of the ideas look like they would need
> > to be aggregated to be big enough for a SoC project, but that can be
> > fleshed out in future patches (i.e., I don't think we care enough about
> > history to have people polish and re-roll what are essentially wiki
> > edits).
> 
> Yeah, I wasn't sure if the ideas were meant to also contain microprojects
> so I wrote down everything that came to mind, that I do not intend to work on
> myself over the next couple month.

Microprojects go on their own page. But make sure that they really are
"micro" sized for an applicant.

> > If you (or anybody interested in working on this) would prefer direct
> > push access to the git.github.io repo, I'm happy to set that up.
> 
> Yeah I wouldn't mind direct push access there, then I could fixup
> what I just sent you, e.g. adding myself as a possible mentor or
> re-shuffling these ideas. :)

OK, done. For anybody else interested, I just need to know your GitHub
username.

-Peff

Re: Force Confirmation for Dropping Changed Lines

[Jacob Keller]

On Wed, Jan 25, 2017 at 2:16 PM, Hilco Wijbenga
 wrote:
> How can I force Git to not assume my change to the first line is "redundant"?
>

My guess is that you probably want a custom merge driver for your file
types. That's where I would look initially.

Thanks,
Jake

> Cheers,
> Hilco
>
> [1] Note that this someone is (correctly) using the same, new version of 1.3.
> [2] If my example is actually incorrect, then please just pretend
> there are no conflicts.

Re: [PATCH v1 1/3] blame: add --aggregate option

[Jeff King]

On Tue, Jan 24, 2017 at 11:27:32PM -0600, Edmundo Carmona Antoranz wrote:

> To avoid taking up so much space on normal output printing duplicate
> information on consecutive lines that "belong" to the same revision,
> this option allows to print a single line with the information about
> the revision before the lines of content themselves.

I admit I have not followed the preceding discussion closely, so ignore
me if my suggestion is way off base.

But it really seems like the various outputs you are looking for are
really all about customizing git-blame's human-readable output.

Would it be more productive to move towards a "--format" option that
shows custom items for each line? It could build on the custom formats
in the pretty-print code (though I think you would want to add some
custom ones, like filename, line number, line content, etc).

I know that only does half of what you want, which is making some output
not just per-line, but per-block. But that can come easily on top, if we
allow separate per-line and per-block formats (which would default to
the current output and the empty string, respectively).

Then you could do something like:

  git blame --format-block='%h %an <%ae>%n  %s' \
--format-line='\t%(filename):%(linenumber) %(linecontent)'

and get something like:

1234abcd A U Thor 
  initial commit
foo.c:1 #include 
foo.c:2 #include 
5678abcd A U Thor 
  add third include
foo.c:3 #include 

and so on. But people can mix and match what they want to see on each
line, and what they'd rather push to other lines.

I don't have a huge interest in the feature myself. I switched to "tig
blame" years ago and never looked back. But it just seems like your
options touch no this kind of flexibility, but will limit somebody as
soon as they want to switch around which information goes where.

-Peff

Force Confirmation for Dropping Changed Lines

[Hilco Wijbenga]

Hi all,

Most of the time, when a later commit changes a line in an identical
fashion during, say, a rebase, you want Git to silently continue by
dropping the duplicate change from the later commit. I have a common
(for me) scenario where I want Git to specifically ask me to resolve
this "conflict" myself instead of simply assuming that the change has
already been applied.

Let's say I have a file my-code.x that starts with a line indicating
its version:

= my-code.x =
VERSION=1.2
line 1
line 2
line 3
=

In my branch my-branch off of master, I make a change:

= my-code.x =
VERSION=1.3
line 1
line 2
line 2a
line 3
=

Now someone else makes a different change on master and pushes it ([1]):

= my-code.x =
VERSION=1.3
line 1
line 2
line 3
line 4
=

When I rebase my-branch onto origin/master, I get no conflicts and
everything seems fine ([2]):

= my-code.x =
VERSION=1.3
line 1
line 2
line 2a
line 3
line 4
=

Except that I should have used VERSION=1.4, not VERSION=1.3 because I
made a change to my-code.x. Obviously, for a single file this is easy
to remember/check but when it's hidden among lots of files and commits
it becomes very hard to find these types of errors.

How can I force Git to not assume my change to the first line is "redundant"?

Cheers,
Hilco

[1] Note that this someone is (correctly) using the same, new version of 1.3.
[2] If my example is actually incorrect, then please just pretend
there are no conflicts.

Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)

[Stefan Beller]

On Wed, Jan 25, 2017 at 2:06 PM, Jeff King  wrote:
> On Wed, Jan 25, 2017 at 01:59:31PM -0800, Stefan Beller wrote:
>
>>   This applies to the repo at https://github.com/git/git.github.io
>
> Thanks. I've applied and pushed, though I'll admit I didn't really read
> it carefully for content. A few of the ideas look like they would need
> to be aggregated to be big enough for a SoC project, but that can be
> fleshed out in future patches (i.e., I don't think we care enough about
> history to have people polish and re-roll what are essentially wiki
> edits).

Yeah, I wasn't sure if the ideas were meant to also contain microprojects
so I wrote down everything that came to mind, that I do not intend to work on
myself over the next couple month.

>
> If you (or anybody interested in working on this) would prefer direct
> push access to the git.github.io repo, I'm happy to set that up.

Yeah I wouldn't mind direct push access there, then I could fixup
what I just sent you, e.g. adding myself as a possible mentor or
re-shuffling these ideas. :)

>
>>   If you're looking for a co-admin/mentors, I can help out.
>
> I may take you up on the co-admin thing. I think it's good to have a
> backup, just in case.
>
> Anything you put on the Ideas page, you should probably be willing to
> mentor. We definitely _don't_ want Ideas that don't have a mentor.

agreed.

> I think in general that each idea should have the possible mentors
> listed below it.

ok, I can make a patch for that; but pushing seems (slightly) easier than
mailing a patch.

Thanks,
Stefan

Re: [PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)

[Jeff King]

On Wed, Jan 25, 2017 at 01:59:31PM -0800, Stefan Beller wrote:

>   This applies to the repo at https://github.com/git/git.github.io

Thanks. I've applied and pushed, though I'll admit I didn't really read
it carefully for content. A few of the ideas look like they would need
to be aggregated to be big enough for a SoC project, but that can be
fleshed out in future patches (i.e., I don't think we care enough about
history to have people polish and re-roll what are essentially wiki
edits).

If you (or anybody interested in working on this) would prefer direct
push access to the git.github.io repo, I'm happy to set that up.

>   If you're looking for a co-admin/mentors, I can help out.

I may take you up on the co-admin thing. I think it's good to have a
backup, just in case.

Anything you put on the Ideas page, you should probably be willing to
mentor. We definitely _don't_ want Ideas that don't have a mentor.
I think in general that each idea should have the possible mentors
listed below it.

-Peff

[RFC 06/14] fetch: refactor to make function args narrower

[Jonathan Tan]

Refactor find_non_local_tags and get_ref_map to only take the
information they need instead of the entire transport struct. Besides
improving code clarity, this also improves their flexibility, which will
be needed in a future patch.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 43e35c494..ae7c6daa1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -220,7 +220,7 @@ static int will_fetch(struct ref **head, const unsigned 
char *sha1)
return 0;
 }
 
-static void find_non_local_tags(struct transport *transport,
+static void find_non_local_tags(const struct ref *refs,
struct ref **head,
struct ref ***tail)
 {
@@ -230,7 +230,7 @@ static void find_non_local_tags(struct transport *transport,
struct string_list_item *item = NULL;
 
for_each_ref(add_existing, _refs);
-   for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
+   for (ref = refs; ref; ref = ref->next) {
if (!starts_with(ref->name, "refs/tags/"))
continue;
 
@@ -302,7 +302,8 @@ static void find_non_local_tags(struct transport *transport,
string_list_clear(_refs, 0);
 }
 
-static struct ref *get_ref_map(struct transport *transport,
+static struct ref *get_ref_map(const struct remote *remote,
+  const struct ref *remote_refs,
   struct refspec *refspecs, int refspec_count,
   int tags, int *autotags)
 {
@@ -314,8 +315,6 @@ static struct ref *get_ref_map(struct transport *transport,
/* opportunistically-updated references: */
struct ref *orefs = NULL, **oref_tail = 
 
-   const struct ref *remote_refs = transport_get_remote_refs(transport);
-
struct string_list existing_refs = STRING_LIST_INIT_DUP;
 
if (refspec_count) {
@@ -355,8 +354,8 @@ static struct ref *get_ref_map(struct transport *transport,
fetch_refspec = parse_fetch_refspec(refmap_nr, 
refmap_array);
fetch_refspec_nr = refmap_nr;
} else {
-   fetch_refspec = transport->remote->fetch;
-   fetch_refspec_nr = transport->remote->fetch_refspec_nr;
+   fetch_refspec = remote->fetch;
+   fetch_refspec_nr = remote->fetch_refspec_nr;
}
 
for (i = 0; i < fetch_refspec_nr; i++)
@@ -365,7 +364,6 @@ static struct ref *get_ref_map(struct transport *transport,
die("--refmap option is only meaningful with command-line 
refspec(s).");
} else {
/* Use the defaults */
-   struct remote *remote = transport->remote;
struct branch *branch = branch_get(NULL);
int has_merge = branch_has_merge_config(branch);
if (remote &&
@@ -404,7 +402,7 @@ static struct ref *get_ref_map(struct transport *transport,
/* also fetch all tags */
get_fetch_map(remote_refs, tag_refspec, , 0);
else if (tags == TAGS_DEFAULT && *autotags)
-   find_non_local_tags(transport, _map, );
+   find_non_local_tags(remote_refs, _map, );
 
/* Now append any refs to be updated opportunistically: */
*tail = orefs;
@@ -1083,6 +1081,7 @@ static int do_fetch(struct transport *transport,
struct ref *ref_map;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
+   const struct ref *remote_refs;
 
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1101,7 +1100,9 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
 
-   ref_map = get_ref_map(transport, refs, ref_count, tags, );
+   remote_refs = transport_get_remote_refs(transport);
+   ref_map = get_ref_map(transport->remote, remote_refs, refs, ref_count,
+ tags, );
if (!update_head_ok)
check_not_current_branch(ref_map);
 
@@ -1134,7 +1135,7 @@ static int do_fetch(struct transport *transport,
if (tags == TAGS_DEFAULT && autotags) {
struct ref **tail = _map;
ref_map = NULL;
-   find_non_local_tags(transport, _map, );
+   find_non_local_tags(remote_refs, _map, );
if (ref_map)
backfill_tags(transport, ref_map);
free_refs(ref_map);
-- 
2.11.0.483.g087da7b7c-goog

[RFC 10/14] fetch-pack: support partial names and globs

[Jonathan Tan]

Teach fetch-pack to support partial ref names and ref patterns as input.

This does not use "want-ref" yet - support for that will be added in a
future patch.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch-pack.c  | 40 -
 remote.c  | 55 +++
 remote.h  | 16 +++
 t/t5500-fetch-pack.sh | 38 +++
 4 files changed, 122 insertions(+), 27 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index a18fd0c44..5f14242ae 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -11,32 +11,12 @@ static const char fetch_pack_usage[] =
 "[--include-tag] [--upload-pack=] [--depth=] "
 "[--no-progress] [--diag-url] [-v] [:] [...]";
 
-static void add_sought_entry(const struct ref ***sought, int *nr, int *alloc,
+static void add_sought_entry(struct refspec **sought, int *nr, int *alloc,
 const char *name)
 {
-   struct ref *ref;
-   struct object_id oid;
-
-   if (!get_oid_hex(name, )) {
-   if (name[GIT_SHA1_HEXSZ] == ' ') {
-   /*  , find refname */
-   name += GIT_SHA1_HEXSZ + 1;
-   } else if (name[GIT_SHA1_HEXSZ] == '\0') {
-   ; /* , leave sha1 as name */
-   } else {
-   /* , clear cruft from oid */
-   oidclr();
-   }
-   } else {
-   /* , clear cruft from get_oid_hex */
-   oidclr();
-   }
-
-   ref = alloc_ref(name);
-   oidcpy(>old_oid, );
(*nr)++;
ALLOC_GROW(*sought, *nr, *alloc);
-   (*sought)[*nr - 1] = ref;
+   parse_ref_or_pattern(&(*sought)[*nr - 1], name);
 }
 
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
@@ -44,8 +24,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
int i, ret;
struct ref *ref = NULL;
const char *dest = NULL;
-   const struct ref **sought = NULL;
+   struct refspec *sought = NULL;
int nr_sought = 0, alloc_sought = 0;
+   const struct ref **sought_refs;
+   int nr_sought_refs;
int fd[2];
char *pack_lockfile = NULL;
char **pack_lockfile_ptr = NULL;
@@ -195,8 +177,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
return args.diag_url ? 0 : 1;
}
get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
+   get_ref_array(_refs, _sought_refs, ref, sought, nr_sought);
 
-   ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
+   ref = fetch_pack(, fd, conn, ref, dest, sought_refs, 
nr_sought_refs,
 , pack_lockfile_ptr);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
@@ -222,12 +205,15 @@ int cmd_fetch_pack(int argc, const char **argv, const 
char *prefix)
 */
for (i = 0; i < nr_sought; i++) {
struct ref *r;
-   for (r = ref; r; r = r->next)
-   if (!sought[i] || refname_match(sought[i]->name, 
r->name))
+   if (sought[i].pattern)
+   continue; /* patterns do not need to match anything */
+   for (r = ref; r; r = r->next) {
+   if (refname_match(sought[i].src, r->name))
break;
+   }
if (r)
continue;
-   error("no such remote ref %s", sought[i]->name);
+   error("no such remote ref %s", sought[i].src);
ret = 1;
}
 
diff --git a/remote.c b/remote.c
index 725a2d39a..08f3c910e 100644
--- a/remote.c
+++ b/remote.c
@@ -612,6 +612,39 @@ static struct refspec *parse_refspec_internal(int 
nr_refspec, const char **refsp
die("Invalid refspec '%s'", refspec[i]);
 }
 
+void parse_ref_or_pattern(struct refspec *refspec, const char *str)
+{
+   struct object_id oid;
+   memset(refspec, 0, sizeof(*refspec));
+
+   if (!get_oid_hex(str, )) {
+   if (str[GIT_SHA1_HEXSZ] == ' ') {
+   struct object_id oid2;
+   /*  , find refname */
+   refspec->src = xstrdup(str + GIT_SHA1_HEXSZ + 1);
+   if (!get_oid_hex(refspec->src, )
+   && !oidcmp(, ))
+   /* The name is actually a SHA-1 */
+   refspec->exact_sha1 = 1;
+   } else if (str[GIT_SHA1_HEXSZ] == '\0') {
+   ; /* , leave sha1 as name */
+   refspec->src = xstrdup(str);
+   refspec->exact_sha1 = 1;
+   } else {
+   /*  */
+   refspec->src = xstrdup(str);
+   }
+   } else {
+   /*  

[RFC 08/14] fetch-pack: check returned refs for matches

[Jonathan Tan]

Instead of setting "matched" on matched refs, detect matches by checking
the sought refs against the returned refs.  Also, since the "matched"
field in struct ref is now no longer used, remove it.

This is the 2nd of 3 patches to eliminate using input refs to
communicate information obtained by the fetch mechanism.

(There are possibly ways more efficient than a nested for loop to
accomplish this. However, since a subsequent patch will compare the
user-provided refspecs against the fetched refs directly, and thus
necessitating the nested for loop anyway, I decided to use the nested
for loop in this patch.)

Signed-off-by: Jonathan Tan 
---
 builtin/fetch-pack.c | 7 ++-
 fetch-pack.c | 9 ++---
 fetch-pack.h | 2 --
 remote.h | 3 +--
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cfe9e447c..f31f874a0 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -4,6 +4,7 @@
 #include "remote.h"
 #include "connect.h"
 #include "sha1-array.h"
+#include "refs.h"
 
 static const char fetch_pack_usage[] =
 "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
@@ -220,7 +221,11 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
 * an error.
 */
for (i = 0; i < nr_sought; i++) {
-   if (!sought[i] || sought[i]->matched)
+   struct ref *r;
+   for (r = ref; r; r = r->next)
+   if (!sought[i] || refname_match(sought[i]->name, 
r->name))
+   break;
+   if (r)
continue;
error("no such remote ref %s", sought[i]->name);
ret = 1;
diff --git a/fetch-pack.c b/fetch-pack.c
index 9a87ddd3d..d4642b05c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -562,6 +562,7 @@ static void filter_refs(struct fetch_pack_args *args,
struct ref **newtail = 
struct ref *ref, *next;
int i;
+   int *matched = xcalloc(nr_sought, sizeof(*matched));
 
i = 0;
for (ref = *refs; ref; ref = next) {
@@ -578,7 +579,7 @@ static void filter_refs(struct fetch_pack_args *args,
break; /* definitely do not have it */
else if (cmp == 0) {
keep = 1; /* definitely have it */
-   sought[i]->matched = 1;
+   matched[i] = 1;
}
i++;
}
@@ -604,19 +605,21 @@ static void filter_refs(struct fetch_pack_args *args,
unsigned char sha1[20];
 
ref = sought[i];
-   if (ref->matched)
+   if (matched[i])
continue;
if (get_sha1_hex(ref->name, sha1) ||
ref->name[40] != '\0' ||
hashcmp(sha1, ref->old_oid.hash))
continue;
 
-   ref->matched = 1;
+   matched[i] = 1;
*newtail = copy_ref(ref);
newtail = &(*newtail)->next;
}
}
*refs = newlist;
+
+   free(matched);
 }
 
 static void mark_alternate_complete(const struct ref *ref, void *unused)
diff --git a/fetch-pack.h b/fetch-pack.h
index c912e3d32..76f7c719c 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -33,8 +33,6 @@ struct fetch_pack_args {
 
 /*
  * sought represents remote references that should be updated from.
- * On return, the names that were found on the remote will have been
- * marked as such.
  */
 struct ref *fetch_pack(struct fetch_pack_args *args,
   int fd[], struct child_process *conn,
diff --git a/remote.h b/remote.h
index 57d059431..2f7f23d47 100644
--- a/remote.h
+++ b/remote.h
@@ -89,8 +89,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   deletion:1,
-   matched:1;
+   deletion:1;
 
/*
 * Order is important here, as we write to FETCH_HEAD
-- 
2.11.0.483.g087da7b7c-goog

[RFC 09/14] transport: put ref oid in out param

[Jonathan Tan]

Return new OID information obtained through fetching in new structs
instead of reusing the existing ones. With this change, the input
structs are no longer used for output, and are thus marked const.

This is the 3rd of 3 patches to eliminate using input refs to
communicate information obtained by the fetch mechanism.

Signed-off-by: Jonathan Tan 
---
 builtin/clone.c  | 14 --
 builtin/fetch-pack.c |  4 ++--
 fetch-pack.c | 26 +++---
 fetch-pack.h |  2 +-
 transport-helper.c   | 34 +++---
 transport.c  |  6 +++---
 transport.h  | 13 +
 7 files changed, 61 insertions(+), 38 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 0135c5f1c..3191da391 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -858,6 +858,8 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
struct refspec *refspec;
const char *fetch_pattern;
 
+   struct ref *new_remote_refs = NULL;
+
packet_trace_identity("clone");
argc = parse_options(argc, argv, prefix, builtin_clone_options,
 builtin_clone_usage, 0);
@@ -1075,8 +1077,15 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
break;
}
 
-   if (!is_local && !complete_refs_before_fetch)
-   transport_fetch_refs(transport, mapped_refs, NULL);
+   if (!is_local && !complete_refs_before_fetch) {
+   transport_fetch_refs(transport, mapped_refs,
+_remote_refs);
+   if (new_remote_refs) {
+   refs = new_remote_refs;
+   free_refs(mapped_refs);
+   mapped_refs = wanted_peer_refs(refs, refspec);
+   }
+   }
 
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1148,5 +1157,6 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
junk_mode = JUNK_LEAVE_ALL;
 
free(refspec);
+   free_refs(new_remote_refs);
return err;
 }
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index f31f874a0..a18fd0c44 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -11,7 +11,7 @@ static const char fetch_pack_usage[] =
 "[--include-tag] [--upload-pack=] [--depth=] "
 "[--no-progress] [--diag-url] [-v] [:] [...]";
 
-static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
+static void add_sought_entry(const struct ref ***sought, int *nr, int *alloc,
 const char *name)
 {
struct ref *ref;
@@ -44,7 +44,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
int i, ret;
struct ref *ref = NULL;
const char *dest = NULL;
-   struct ref **sought = NULL;
+   const struct ref **sought = NULL;
int nr_sought = 0, alloc_sought = 0;
int fd[2];
char *pack_lockfile = NULL;
diff --git a/fetch-pack.c b/fetch-pack.c
index d4642b05c..8cc85c19f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -52,6 +52,10 @@ static int non_common_revs, multi_ack, use_sideband;
 #define ALLOW_REACHABLE_SHA1   02
 static unsigned int allow_unadvertised_object_request;
 
+/* An arbitrary non-NULL non-const pointer to assign to the util field of
+ * string list items when we need one. */
+#define ARBITRARY (_unpack_limit)
+
 __attribute__((format (printf, 2, 3)))
 static inline void print_verbose(const struct fetch_pack_args *args,
 const char *fmt, ...)
@@ -556,7 +560,7 @@ static void mark_recent_complete_commits(struct 
fetch_pack_args *args,
 
 static void filter_refs(struct fetch_pack_args *args,
struct ref **refs,
-   struct ref **sought, int nr_sought)
+   const struct ref **sought, int nr_sought)
 {
struct ref *newlist = NULL;
struct ref **newtail = 
@@ -604,16 +608,16 @@ static void filter_refs(struct fetch_pack_args *args,
for (i = 0; i < nr_sought; i++) {
unsigned char sha1[20];
 
-   ref = sought[i];
+   const struct ref *sref = sought[i];
if (matched[i])
continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
+   if (get_sha1_hex(sref->name, sha1) ||
+   sref->name[40] != '\0' ||
+   hashcmp(sha1, sref->old_oid.hash))
continue;
 
matched[i] = 1;
-   *newtail = 

[RFC 14/14] DONT USE advertise_ref_in_want=1

[Jonathan Tan]

---
 t/t5500-fetch-pack.sh | 2 ++
 upload-pack.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 18fe23c97..f39dbcab8 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -551,6 +551,7 @@ test_expect_success 'fetch-pack can fetch refs using a 
partial name' '
git init server &&
(
cd server &&
+   git config uploadpack.advertiseRefInWant false
test_commit 1 &&
test_commit 2 &&
git checkout -b one
@@ -587,6 +588,7 @@ test_expect_success 'fetch-pack can fetch refs using a 
glob' '
git init server &&
(
cd server &&
+   git config uploadpack.advertiseRefInWant false
test_commit 1 &&
test_commit 2 &&
git checkout -b ona &&
diff --git a/upload-pack.c b/upload-pack.c
index 0678c53d6..4998a8c7e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -62,7 +62,7 @@ static int use_sideband;
 static int advertise_refs;
 static int stateless_rpc;
 static const char *pack_objects_hook;
-static int advertise_ref_in_want;
+static int advertise_ref_in_want = 1;
 
 static void reset_timeout(void)
 {
-- 
2.11.0.483.g087da7b7c-goog

[RFC 07/14] fetch-pack: put shallow info in out param

[Jonathan Tan]

Expand the transport fetch method signature, by adding an out parameter,
to allow transports to return information about the refs they have
fetched. Then communicate shallow status information through this
mechanism instead of by modifying the input list of refs.

This does require clients to sometimes generate the ref map twice: one
generation from the list of refs provided by the remote (as is currently
done) and potentially one regeneration from the new list of refs that
the fetch mechanism provides (added in this patch). The double
generation may result in duplicate error messages when a remote-tracking
branch seems to track more than one remote branch.

This is the 1st of 3 patches to eliminate using input refs to
communicate information obtained by the fetch mechanism.

Signed-off-by: Jonathan Tan 
---
 builtin/clone.c|  4 ++--
 builtin/fetch.c| 23 +++
 fetch-pack.c   | 18 +++---
 remote.c   | 12 +++-
 remote.h   |  1 +
 t/t5536-fetch-conflicts.sh |  2 ++
 transport-helper.c |  5 +++--
 transport.c| 32 ++--
 transport.h| 12 ++--
 9 files changed, 85 insertions(+), 24 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 5ef81927a..0135c5f1c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1076,7 +1076,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
}
 
if (!is_local && !complete_refs_before_fetch)
-   transport_fetch_refs(transport, mapped_refs);
+   transport_fetch_refs(transport, mapped_refs, NULL);
 
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
@@ -1115,7 +1115,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
-   transport_fetch_refs(transport, mapped_refs);
+   transport_fetch_refs(transport, mapped_refs, NULL);
 
update_remote_refs(refs, mapped_refs, remote_head_points_at,
   branch_top.buf, reflog_msg.buf, transport, 
!is_local);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ae7c6daa1..19e3f40a0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -911,11 +911,13 @@ static int quickfetch(struct ref *ref_map)
return check_connected(iterate_ref_map, , );
 }
 
-static int fetch_refs(struct transport *transport, struct ref *ref_map)
+static int fetch_refs(struct transport *transport, struct ref *ref_map,
+ struct ref **updated_remote_refs)
 {
int ret = quickfetch(ref_map);
if (ret)
-   ret = transport_fetch_refs(transport, ref_map);
+   ret = transport_fetch_refs(transport, ref_map,
+  updated_remote_refs);
if (ret)
transport_unlock_pack(transport);
return ret;
@@ -1066,7 +1068,7 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-   if (!fetch_refs(transport, ref_map))
+   if (!fetch_refs(transport, ref_map, NULL))
consume_refs(transport, ref_map);
 
if (gsecondary) {
@@ -1082,6 +1084,7 @@ static int do_fetch(struct transport *transport,
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
const struct ref *remote_refs;
+   struct ref *new_remote_refs = NULL;
 
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1123,7 +1126,19 @@ static int do_fetch(struct transport *transport,
   transport->url);
}
}
-   if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) 
{
+   if (fetch_refs(transport, ref_map, _remote_refs)) {
+   free_refs(ref_map);
+   retcode = 1;
+   goto cleanup;
+   }
+   if (new_remote_refs) {
+   free_refs(ref_map);
+   ref_map = get_ref_map(transport->remote, new_remote_refs,
+ refs, ref_count, tags, );
+   free_refs(new_remote_refs);
+   }
+
+   if (consume_refs(transport, ref_map)) {
free_refs(ref_map);
retcode = 1;
goto cleanup;
diff --git a/fetch-pack.c b/fetch-pack.c
index 601f0779a..9a87ddd3d 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -974,12 +974,13 @@ static int remove_duplicates_in_refs(struct ref **ref, 
int nr)
 }
 
 static void update_shallow(struct 

[RFC 11/14] fetch-pack: support want-ref

[Jonathan Tan]

Teach fetch-pack to use the want-ref mechanism whenever the server
advertises it.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch-pack.c  |   5 +-
 fetch-pack.c  | 173 --
 fetch-pack.h  |   2 +
 t/t5500-fetch-pack.sh |  42 
 transport.c   |   2 +-
 5 files changed, 175 insertions(+), 49 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 5f14242ae..ae073ab24 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -179,8 +179,9 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
get_ref_array(_refs, _sought_refs, ref, sought, nr_sought);
 
-   ref = fetch_pack(, fd, conn, ref, dest, sought_refs, 
nr_sought_refs,
-, pack_lockfile_ptr);
+   ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
+sought_refs, nr_sought_refs, ,
+pack_lockfile_ptr);
if (pack_lockfile) {
printf("lock %s\n", pack_lockfile);
fflush(stdout);
diff --git a/fetch-pack.c b/fetch-pack.c
index 8cc85c19f..02149c930 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -219,11 +219,19 @@ static void consume_shallow_list(struct fetch_pack_args 
*args, int fd)
}
 }
 
-static enum ack_type get_ack(int fd, unsigned char *result_sha1)
+/*
+ * Reads an ACK or NAK from fd. If wanted_ref_tail is not NULL, also accepts
+ * any "wanted-ref" lines before that ACK or NAK, writing them to
+ * wanted_ref_tail.
+ */
+static enum ack_type get_ack(int fd, unsigned char *result_sha1,
+struct ref ***wanted_ref_tail)
 {
int len;
-   char *line = packet_read_line(fd, );
+   char *line;
const char *arg;
+start:
+   line = packet_read_line(fd, );
 
if (!len)
die(_("git fetch-pack: expected ACK/NAK, got EOF"));
@@ -244,7 +252,19 @@ static enum ack_type get_ack(int fd, unsigned char 
*result_sha1)
return ACK;
}
}
-   die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
+   if (wanted_ref_tail) {
+   struct object_id oid;
+   if (skip_prefix(line, "wanted-ref ", ) &&
+   !get_sha1_hex(arg, oid.hash) && arg[40] == ' ' && arg[41]) {
+   struct ref *ref = alloc_ref(arg + 41);
+   oidcpy(>old_oid, );
+   **wanted_ref_tail = ref;
+   *wanted_ref_tail = >next;
+   goto start;
+   }
+   die(_("git fetch_pack: expected ACK/NAK or wanted-ref, got 
'%s'"), line);
+   }
+   die(_("git fetch_pack: expected ACK/NAK, got '%s'"), line);
 }
 
 static void send_request(struct fetch_pack_args *args,
@@ -282,29 +302,55 @@ static int next_flush(struct fetch_pack_args *args, int 
count)
return count;
 }
 
-static int find_common(struct fetch_pack_args *args,
-  int fd[2], unsigned char *result_sha1,
-  struct ref *refs)
+static void write_capabilities(struct strbuf *sb,
+  const struct fetch_pack_args *args)
 {
-   int fetching;
-   int count = 0, flushes = 0, flush_at = INITIAL_FLUSH, retval;
-   const unsigned char *sha1;
-   unsigned in_vain = 0;
-   int got_continue = 0;
-   int got_ready = 0;
-   struct strbuf req_buf = STRBUF_INIT;
-   size_t state_len = 0;
+   if (multi_ack == 2) strbuf_addstr(sb, " multi_ack_detailed");
+   if (multi_ack == 1) strbuf_addstr(sb, " multi_ack");
+   if (no_done)strbuf_addstr(sb, " no-done");
+   if (use_sideband == 2)  strbuf_addstr(sb, " side-band-64k");
+   if (use_sideband == 1)  strbuf_addstr(sb, " side-band");
+   if (args->deepen_relative) strbuf_addstr(sb, " deepen-relative");
+   if (args->use_thin_pack) strbuf_addstr(sb, " thin-pack");
+   if (args->no_progress)   strbuf_addstr(sb, " no-progress");
+   if (args->include_tag)   strbuf_addstr(sb, " include-tag");
+   if (prefer_ofs_delta)   strbuf_addstr(sb, " ofs-delta");
+   if (deepen_since_ok)strbuf_addstr(sb, " deepen-since");
+   if (deepen_not_ok)  strbuf_addstr(sb, " deepen-not");
+   if (agent_supported)strbuf_addf(sb, " agent=%s",
+   git_user_agent_sanitized());
+}
 
-   if (args->stateless_rpc && multi_ack == 1)
-   die(_("--stateless-rpc requires multi_ack_detailed"));
-   if (marked)
-   for_each_ref(clear_marks, NULL);
-   marked = 1;
+static void write_wants(struct strbuf *sb, const struct fetch_pack_args *args,
+   const struct refspec *refspecs, int nr_refspec,
+   struct ref *refs)
+{
+   int 

[RFC 03/14] upload-pack: test negotiation with changing repo

[Jonathan Tan]

Make upload-pack report "not our ref" errors to the client. (If not, the
client would be left waiting for a response when the server is already
dead.)

Add tests to check the behavior of upload-pack and fetch-pack when
upload-pack is served from a changing repository (for example, when
different servers in a load-balancing agreement participate in the same
stateless RPC negotiation). This forms a baseline of comparison to the
ref-in-want functionality (which will be introduced in subsequent
commits), and ensures that subsequent commits do not change existing
behavior.

As part of this effort, a mechanism to substitute strings in an HTTP
response only on the first invocation is added.

Signed-off-by: Jonathan Tan 
---
 t/lib-httpd.sh |  1 +
 t/lib-httpd/apache.conf|  8 
 t/lib-httpd/one-time-sed.sh|  8 
 t/t5552-upload-pack-ref-in-want.sh | 91 ++
 upload-pack.c  |  6 ++-
 5 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 t/lib-httpd/one-time-sed.sh

diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 435a37465..84f8efdd4 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -132,6 +132,7 @@ prepare_httpd() {
cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
install_script broken-smart-http.sh
install_script error.sh
+   install_script one-time-sed.sh
 
ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
 
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 69174c6e3..ef218ff15 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -106,9 +106,14 @@ Alias /auth/dumb/ www/auth/dumb/
SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
SetEnv GIT_HTTP_EXPORT_ALL
 
+
+   SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+   SetEnv GIT_HTTP_EXPORT_ALL
+
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
+ScriptAliasMatch /one_time_sed/(.*) one-time-sed.sh/$1
 
Options FollowSymlinks
 
@@ -118,6 +123,9 @@ ScriptAlias /error/ error.sh/
 
   Options ExecCGI
 
+
+   Options ExecCGI
+
 
Options ExecCGI
 
diff --git a/t/lib-httpd/one-time-sed.sh b/t/lib-httpd/one-time-sed.sh
new file mode 100644
index 0..060ec0300
--- /dev/null
+++ b/t/lib-httpd/one-time-sed.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+if [ -e one-time-sed ]; then
+   "$GIT_EXEC_PATH/git-http-backend" | sed "$(cat one-time-sed)"
+   rm one-time-sed
+else
+   "$GIT_EXEC_PATH/git-http-backend"
+fi
diff --git a/t/t5552-upload-pack-ref-in-want.sh 
b/t/t5552-upload-pack-ref-in-want.sh
index 3496af641..80cf2263a 100755
--- a/t/t5552-upload-pack-ref-in-want.sh
+++ b/t/t5552-upload-pack-ref-in-want.sh
@@ -292,4 +292,95 @@ test_expect_success 'hideRefs with namespaces' '
check_output
 '
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+REPO="$HTTPD_DOCUMENT_ROOT_PATH/repo"
+LOCAL_PRISTINE="$(pwd)/local_pristine"
+
+test_expect_success 'setup repos for change-while-negotiating test' '
+   (
+   git init "$REPO" &&
+   cd "$REPO" &&
+   >.git/git-daemon-export-ok &&
+   test_commit m1 &&
+   git tag -d m1 &&
+
+   # Local repo with many commits (so that negotiation will take
+   # more than 1 request/response pair)
+   git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo; 
"$LOCAL_PRISTINE" &&
+   cd "$LOCAL_PRISTINE" &&
+   git checkout -b side &&
+   for i in $(seq 1 33); do test_commit s$i; done &&
+
+   # Add novel commits to upstream
+   git checkout master &&
+   cd "$REPO" &&
+   test_commit m2 &&
+   test_commit m3 &&
+   git tag -d m2 m3
+   ) &&
+   git -C "$LOCAL_PRISTINE" remote set-url origin 
"http://127.0.0.1:$LIB_HTTPD_PORT/one_time_sed/repo;
+'
+
+inconsistency() {
+   # Simulate that the server initially reports $2 as the ref
+   # corresponding to $1, and after that, $1 as the ref corresponding to
+   # $1. This corresponds to the real-life situation where the server's
+   # repository appears to change during negotiation, for example, when
+   # different servers in a load-balancing arrangement serve (stateless)
+   # RPCs during a single negotiation.
+   printf "s/%s/%s/" \
+  $(git -C "$REPO" rev-parse $1 | tr -d "\n") \
+  $(git -C "$REPO" rev-parse $2 | tr -d "\n") \
+  >"$HTTPD_ROOT_PATH/one-time-sed"
+}
+
+test_expect_success 'server is initially ahead - no ref in want' '
+   git -C "$REPO" config uploadpack.advertiseRefInWant false &&
+   rm -rf local &&
+   cp -r "$LOCAL_PRISTINE" local &&
+   inconsistency master 1234567890123456789012345678901234567890 &&
+   test_must_fail git -C local 

[RFC 05/14] fetch: refactor fetch_refs into two functions

[Jonathan Tan]

Refactor the fetch_refs function into a function that does the fetching
of refs and another function that stores them. This prepares for a
future patch where some processing may be done between those tasks.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c71d5eb9b..43e35c494 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -918,10 +918,16 @@ static int fetch_refs(struct transport *transport, struct 
ref *ref_map)
int ret = quickfetch(ref_map);
if (ret)
ret = transport_fetch_refs(transport, ref_map);
-   if (!ret)
-   ret |= store_updated_refs(transport->url,
-   transport->remote->name,
-   ref_map);
+   if (ret)
+   transport_unlock_pack(transport);
+   return ret;
+}
+
+static int consume_refs(struct transport *transport, struct ref *ref_map)
+{
+   int ret = store_updated_refs(transport->url,
+transport->remote->name,
+ref_map);
transport_unlock_pack(transport);
return ret;
 }
@@ -1062,7 +1068,8 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
-   fetch_refs(transport, ref_map);
+   if (!fetch_refs(transport, ref_map))
+   consume_refs(transport, ref_map);
 
if (gsecondary) {
transport_disconnect(gsecondary);
@@ -1115,7 +1122,7 @@ static int do_fetch(struct transport *transport,
   transport->url);
}
}
-   if (fetch_refs(transport, ref_map)) {
+   if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) 
{
free_refs(ref_map);
retcode = 1;
goto cleanup;
-- 
2.11.0.483.g087da7b7c-goog

[RFC 12/14] fetch-pack: do not printf after closing stdout

[Jonathan Tan]

In fetch-pack, during a stateless RPC, printf is invoked after stdout is
closed. Update the code to not do this, preserving the existing
behavior.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch-pack.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index ae073ab24..24af3b7c5 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -191,10 +191,10 @@ int cmd_fetch_pack(int argc, const char **argv, const 
char *prefix)
printf("connectivity-ok\n");
fflush(stdout);
}
-   close(fd[0]);
-   close(fd[1]);
-   if (finish_connect(conn))
-   return 1;
+   if (finish_connect(conn)) {
+   ret = 1;
+   goto cleanup;
+   }
 
ret = !ref;
 
@@ -218,11 +218,17 @@ int cmd_fetch_pack(int argc, const char **argv, const 
char *prefix)
ret = 1;
}
 
+   if (args.stateless_rpc)
+   goto cleanup;
+
while (ref) {
printf("%s %s\n",
   oid_to_hex(>old_oid), ref->name);
ref = ref->next;
}
 
+cleanup:
+   close(fd[0]);
+   close(fd[1]);
return ret;
 }
-- 
2.11.0.483.g087da7b7c-goog

[RFC 13/14] fetch: send want-ref and receive fetched refs

[Jonathan Tan]

Teach fetch to send refspecs to the underlying transport, and teach all
components used by the HTTP transport (remote-curl, transport-helper,
fetch-pack) to understand and propagate the names and SHA-1s of the refs
fetched.

Signed-off-by: Jonathan Tan 
---
 builtin/clone.c|   4 +-
 builtin/fetch-pack.c   |   8 ++-
 builtin/fetch.c| 100 ++---
 remote-curl.c  |  91 -
 t/t5552-upload-pack-ref-in-want.sh |   4 +-
 transport-helper.c |  74 +--
 transport.c|  10 ++--
 transport.h|  20 +---
 8 files changed, 229 insertions(+), 82 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 3191da391..765a3a3b6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -1078,7 +1078,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
}
 
if (!is_local && !complete_refs_before_fetch) {
-   transport_fetch_refs(transport, mapped_refs,
+   transport_fetch_refs(transport, NULL, 0, mapped_refs,
 _remote_refs);
if (new_remote_refs) {
refs = new_remote_refs;
@@ -1124,7 +1124,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
if (is_local)
clone_local(path, git_dir);
else if (refs && complete_refs_before_fetch)
-   transport_fetch_refs(transport, mapped_refs, NULL);
+   transport_fetch_refs(transport, NULL, 0, mapped_refs, NULL);
 
update_remote_refs(refs, mapped_refs, remote_head_points_at,
   branch_top.buf, reflog_msg.buf, transport, 
!is_local);
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 24af3b7c5..ed1608c12 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -35,6 +35,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct fetch_pack_args args;
struct sha1_array shallow = SHA1_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
+   int always_print_refs = 0;
 
packet_trace_identity("fetch-pack");
 
@@ -126,6 +127,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
args.update_shallow = 1;
continue;
}
+   if (!strcmp("--always-print-refs", arg)) {
+   always_print_refs = 1;
+   continue;
+   }
usage(fetch_pack_usage);
}
if (deepen_not.nr)
@@ -218,7 +223,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
ret = 1;
}
 
-   if (args.stateless_rpc)
+   if (args.stateless_rpc && !always_print_refs)
goto cleanup;
 
while (ref) {
@@ -226,6 +231,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
   oid_to_hex(>old_oid), ref->name);
ref = ref->next;
}
+   fflush(stdout);
 
 cleanup:
close(fd[0]);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 19e3f40a0..87de00e49 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -302,10 +302,75 @@ static void find_non_local_tags(const struct ref *refs,
string_list_clear(_refs, 0);
 }
 
+static void get_effective_refspecs(struct refspec **e_rs, int *e_rs_nr,
+  const struct remote *remote,
+  const struct refspec *cli_rs, int cli_rs_nr,
+  int tags, int *autotags)
+{
+   static struct refspec head_refspec;
+
+   const struct refspec *base_rs;
+   int base_rs_nr;
+   struct branch *merge_branch = NULL;
+   int i;
+
+   struct refspec *rs;
+   int nr, alloc;
+
+   if (cli_rs_nr) {
+   base_rs = cli_rs;
+   base_rs_nr = cli_rs_nr;
+   } else if (refmap_array) {
+   die("--refmap option is only meaningful with command-line 
refspec(s).");
+   } else {
+   /* Use the defaults */
+   struct branch *branch = branch_get(NULL);
+   int has_merge = branch_has_merge_config(branch);
+   /* Note: has_merge implies non-NULL branch->remote_name */
+   if (has_merge && !strcmp(branch->remote_name, remote->name))
+   /*
+* if the remote we're fetching from is the same
+* as given in branch..remote, we add the
+* ref given in branch..merge, too.
+*/
+   merge_branch = branch;
+   if (remote &&
+   

[RFC 00/14] Allow fetch-pack to send ref names (globs allowed)

[Jonathan Tan]

Hello everyone - this is a proposal for a protocol change to allow the
fetch-pack/upload-pack to converse in terms of ref names (globs
allowed), and also an implementation of the server (upload-pack) and
fetch-from-HTTP client (fetch-pack invoked through fetch).

Negotiation currently happens by upload-pack initially sending a list of
refs with names and SHA-1 hashes, and then several request/response
pairs in which the request from fetch-pack consists of SHA-1 hashes
(selected from the initial list). Allowing the request to consist of
names instead of SHA-1 hashes increases tolerance to refs changing
(due to time, and due to having load-balanced servers without strong
consistency), and is a step towards eliminating the need for the server
to send the list of refs first (possibly improving performance).

The protocol is extended by allowing fetch-pack to send
"want-ref ", where  is a full name (refs/...) [1], possibly
including glob characters. Upload-pack will inform the client of the
refs actually matched by sending "wanted-ref  " before
sending the last ACK or NAK.

This patch set is laid out in this way:
 1-2:
  Upload-pack, protocol documentation, tests that test upload-pack
  independently. A configuration option is added to control if the
  "ref-in-want" capability is advertised. (It is always supported even
  if not advertised - this is so that this feature in multiple
  load-balanced servers can be switched on or off without needing any
  atomic switching.)
 3:
  Mechanism to test a repo that changes over the negotiation (currently,
  only with the existing mechanism).
 4-9:
  The current transport mechanism takes in an array of refs which is
  used as both input and output. Since we are planning to extend the
  transport mechanism to also allow the specification of ref names
  (which may include globs, and thus do not have a 1-1 correspondence to
  refs), refactor to make this parameter to be solely an input
  parameter.
 10-11:
  Changes to fetch-pack (which is used by remote-curl) to support
  "want-ref".
 12-13:
  Changes to the rest (fetch -> transport -> transport-helper ->
  remote-curl) to support "want-ref" when fetching from HTTP. The
  transport fetch function signature has been widened to allow passing
  in ref names - transports may use those ref names instead of or in
  addition to refs if they support it. (I chose to preserve refs in the
  function signature because many parts of Git, including remote
  helpers, only understand the old functionality anyway, and because
  precalculating the refs allows some optimizations.)
 14:
  This is not meant for submission - this is just to show that the tests
  pass if ref-in-want was advertised by default (except for some newly
  added tests that explicitly check for the old behavior).

[1] There has been some discussion about whether the server should
accept partial ref names, e.g. [2]. In this patch set, I have made the
server only accept full names, and it is the responsibility of the
client to send the multiple patterns which it wants to match. Quoting
from the commit message of the second patch:

  For example, a client could reasonably expand an abbreviated
  name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref
  refs/tags/foo", among others, and ensure that at least one such ref has
  been fetched.

[2] <20161024132932.i42rqn2vlpocq...@sigill.intra.peff.net>

Jonathan Tan (14):
  upload-pack: move parsing of "want" line
  upload-pack: allow ref name and glob requests
  upload-pack: test negotiation with changing repo
  fetch: refactor the population of hashes
  fetch: refactor fetch_refs into two functions
  fetch: refactor to make function args narrower
  fetch-pack: put shallow info in out param
  fetch-pack: check returned refs for matches
  transport: put ref oid in out param
  fetch-pack: support partial names and globs
  fetch-pack: support want-ref
  fetch-pack: do not printf after closing stdout
  fetch: send want-ref and receive fetched refs
  DONT USE advertise_ref_in_want=1

 Documentation/technical/http-protocol.txt |  20 +-
 Documentation/technical/pack-protocol.txt |  24 +-
 Documentation/technical/protocol-capabilities.txt |   6 +
 builtin/clone.c   |  16 +-
 builtin/fetch-pack.c  |  64 ++--
 builtin/fetch.c   | 178 +++---
 fetch-pack.c  | 226 +
 fetch-pack.h  |   6 +-
 remote-curl.c |  91 +++--
 remote.c  |  67 +++-
 remote.h  |  20 +-
 t/lib-httpd.sh|   1 +
 t/lib-httpd/apache.conf   |   8 +
 t/lib-httpd/one-time-sed.sh   |   8 +
 t/t5500-fetch-pack.sh |  82 +
 t/t5536-fetch-conflicts.sh  

[RFC 04/14] fetch: refactor the population of hashes

[Jonathan Tan]

Populate SHA-1 ref hashes in get_ref_map instead of do_fetch. Besides
tightening scopes of variables in the code, this also prepares for a
future patch where get_ref_map is called multiple times within do_fetch.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch.c | 37 +++--
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f1570e346..c71d5eb9b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -316,6 +316,8 @@ static struct ref *get_ref_map(struct transport *transport,
 
const struct ref *remote_refs = transport_get_remote_refs(transport);
 
+   struct string_list existing_refs = STRING_LIST_INIT_DUP;
+
if (refspec_count) {
struct refspec *fetch_refspec;
int fetch_refspec_nr;
@@ -411,7 +413,23 @@ static struct ref *get_ref_map(struct transport *transport,
tail = >next;
}
 
-   return ref_remove_duplicates(ref_map);
+   ref_map = ref_remove_duplicates(ref_map);
+
+   for_each_ref(add_existing, _refs);
+   for (rm = ref_map; rm; rm = rm->next) {
+   if (rm->peer_ref) {
+   struct string_list_item *peer_item =
+   string_list_lookup(_refs,
+  rm->peer_ref->name);
+   if (peer_item) {
+   struct object_id *old_oid = peer_item->util;
+   oidcpy(>peer_ref->old_oid, old_oid);
+   }
+   }
+   }
+   string_list_clear(_refs, 1);
+
+   return ref_map;
 }
 
 #define STORE_REF_ERROR_OTHER 1
@@ -1055,14 +1073,10 @@ static void backfill_tags(struct transport *transport, 
struct ref *ref_map)
 static int do_fetch(struct transport *transport,
struct refspec *refs, int ref_count)
 {
-   struct string_list existing_refs = STRING_LIST_INIT_DUP;
struct ref *ref_map;
-   struct ref *rm;
int autotags = (transport->remote->fetch_tags == 1);
int retcode = 0;
 
-   for_each_ref(add_existing, _refs);
-
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
tags = TAGS_SET;
@@ -1084,18 +1098,6 @@ static int do_fetch(struct transport *transport,
if (!update_head_ok)
check_not_current_branch(ref_map);
 
-   for (rm = ref_map; rm; rm = rm->next) {
-   if (rm->peer_ref) {
-   struct string_list_item *peer_item =
-   string_list_lookup(_refs,
-  rm->peer_ref->name);
-   if (peer_item) {
-   struct object_id *old_oid = peer_item->util;
-   oidcpy(>peer_ref->old_oid, old_oid);
-   }
-   }
-   }
-
if (tags == TAGS_DEFAULT && autotags)
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, "1");
if (prune) {
@@ -1132,7 +1134,6 @@ static int do_fetch(struct transport *transport,
}
 
  cleanup:
-   string_list_clear(_refs, 1);
return retcode;
 }
 
-- 
2.11.0.483.g087da7b7c-goog

[RFC 02/14] upload-pack: allow ref name and glob requests

[Jonathan Tan]

Currently, while performing packfile negotiation [1], upload-pack allows
clients to specify their desired objects only as SHA-1s. This causes:
(a) vulnerability to failure when an object turns non-existent during
negotiation, which may happen if, for example, upload-pack is
provided by multiple Git servers in a load-balancing arrangement,
and
(b) dependence on the server first publishing a list of refs with
associated objects.

To eliminate (a) and take a step towards eliminating (b), teach
upload-pack to support requests in the form of ref names and globs (in
addition to the existing support for SHA-1s) through a new line of the
form "want-ref " where ref is the full name of a ref, a glob
pattern, or a SHA-1. At the conclusion of negotiation, the server will
write "wanted-ref  " for all requests that have been
specified this way.

The server indicates that it supports this feature by advertising the
capability "ref-in-want". Advertisement of this capability is by default
disabled, but can be enabled through a configuration option.

To be flexible with respect to client needs, the server does not
indicate an error if a "want-ref" line corresponds to no refs, but
instead relies on the client to ensure that what the user needs has been
fetched. For example, a client could reasonably expand an abbreviated
name "foo" to "want-ref foo", "want-ref refs/heads/foo", "want-ref
refs/tags/foo", among others, and ensure that at least one such ref has
been fetched.

[1] pack-protocol.txt

Signed-off-by: Jonathan Tan 
---
 Documentation/technical/http-protocol.txt |  20 +-
 Documentation/technical/pack-protocol.txt |  24 +-
 Documentation/technical/protocol-capabilities.txt |   6 +
 t/t5552-upload-pack-ref-in-want.sh| 295 ++
 upload-pack.c |  89 ++-
 5 files changed, 411 insertions(+), 23 deletions(-)
 create mode 100755 t/t5552-upload-pack-ref-in-want.sh

diff --git a/Documentation/technical/http-protocol.txt 
b/Documentation/technical/http-protocol.txt
index 1c561bdd9..162d6bc07 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -316,7 +316,8 @@ to prevent caching of the response.
 
 Servers SHOULD support all capabilities defined here.
 
-Clients MUST send at least one "want" command in the request body.
+Clients MUST send at least one "want" or "want-ref" command in the
+request body.
 Clients MUST NOT reference an id in a "want" command which did not
 appear in the response obtained through ref discovery unless the
 server advertises capability `allow-tip-sha1-in-want` or
@@ -330,7 +331,7 @@ server advertises capability `allow-tip-sha1-in-want` or
   want_list =  PKT-LINE(want NUL cap_list LF)
   *(want_pkt)
   want_pkt  =  PKT-LINE(want LF)
-  want  =  "want" SP id
+  want  =  "want" SP id / "want-ref" SP name
   cap_list  =  *(SP capability) SP
 
   have_list =  *PKT-LINE("have" SP id LF)
@@ -352,7 +353,8 @@ C: Build an empty set, `common`, to hold the objects that 
are later
determined to be on both ends.
 
 C: Build a set, `want`, of the objects from `advertised` the client
-   wants to fetch, based on what it saw during ref discovery.
+   wants to fetch, based on what it saw during ref discovery. This is to
+   be used if the server does not support the ref-in-want capability.
 
 C: Start a queue, `c_pending`, ordered by commit time (popping newest
first).  Add all client refs.  When a commit is popped from
@@ -363,8 +365,8 @@ C: Start a queue, `c_pending`, ordered by commit time 
(popping newest
 
 C: Send one `$GIT_URL/git-upload-pack` request:
 
-   C: 0032want ...
-   C: 0032want ...
+   C: want-ref ...
+   C: want-ref ...

C: 0032have .
C: 0032have .
@@ -384,7 +386,7 @@ the pkt-line value.
 Commands MUST appear in the following order, if they appear
 at all in the request stream:
 
-* "want"
+* "want" or "want-ref"
 * "have"
 
 The stream is terminated by a pkt-line flush (``).
@@ -393,6 +395,9 @@ A single "want" or "have" command MUST have one hex 
formatted
 SHA-1 as its value.  Multiple SHA-1s MUST be sent by sending
 multiple commands.
 
+A "want-ref" command MUST be followed by a ref name (which may include
+shell glob characters) or a hex formatted SHA-1.
+
 The `have` list is created by popping the first 32 commits
 from `c_pending`.  Less can be supplied if `c_pending` empties.
 
@@ -410,6 +415,9 @@ Verify all objects in `want` are directly reachable from 
refs.
 The server MAY walk backwards through history or through
 the reflog to permit slightly stale requests.
 
+Treat "want-ref" requests as if the equivalent 0 or more "want" commands

[RFC 01/14] upload-pack: move parsing of "want" line

[Jonathan Tan]

Refactor to parse "want" lines when the prefix is found. This makes it
easier to add support for another prefix, which will be done in a
subsequent commit.

Signed-off-by: Jonathan Tan 
---
 upload-pack.c | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 7597ba340..15c60a204 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -793,8 +793,20 @@ static void receive_needs(void)
deepen_rev_list = 1;
continue;
}
-   if (!skip_prefix(line, "want ", ) ||
-   get_sha1_hex(arg, sha1_buf))
+   if (skip_prefix(line, "want ", ) &&
+   !get_sha1_hex(arg, sha1_buf)) {
+   o = parse_object(sha1_buf);
+   if (!o)
+   die("git upload-pack: not our ref %s",
+   sha1_to_hex(sha1_buf));
+   if (!(o->flags & WANTED)) {
+   o->flags |= WANTED;
+   if (!((allow_unadvertised_object_request & 
ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
+ || is_our_ref(o)))
+   has_non_tip = 1;
+   add_object_array(o, NULL, _obj);
+   }
+   } else
die("git upload-pack: protocol error, "
"expected to get sha, not '%s'", line);
 
@@ -820,18 +832,6 @@ static void receive_needs(void)
no_progress = 1;
if (parse_feature_request(features, "include-tag"))
use_include_tag = 1;
-
-   o = parse_object(sha1_buf);
-   if (!o)
-   die("git upload-pack: not our ref %s",
-   sha1_to_hex(sha1_buf));
-   if (!(o->flags & WANTED)) {
-   o->flags |= WANTED;
-   if (!((allow_unadvertised_object_request & 
ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
- || is_our_ref(o)))
-   has_non_tip = 1;
-   add_object_array(o, NULL, _obj);
-   }
}
 
/*
-- 
2.11.0.483.g087da7b7c-goog

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Jeff King]

On Wed, Jan 25, 2017 at 01:28:27PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The only advantage is that it is self-documenting, so somebody does not
> > come through later and convert ("%s", "") back to (""). We could also
> > write a comment. But I am happy if we simply catch it in review (or
> > preferably the person is clueful enough to read the output of git-blame
> > and see why it is that way in the first place).
> 
> And the last sentence unfortunatly does not reflect reality.  
> 
> I would prefer something self-documenting, like your wrapper with a
> comment.  Then somebody who is looking at the implementation of
> warning_blank_line() will not get tempted to turn "%s", "" into ""
> because of the comment.  And somebody who is looking at the callsite
> of warning_blank_line() will think twice before suggesting to turn
> it into warning("").

I am OK with it either way. I was mostly responding to Dscho's
complaint, and I would just like to get this resolved so we never have
to revisit it again. :)

> In any case, the patch is a minimum effort band-aid that lets us
> punt on the whole issue for now, so I'll queue it as-is.

Here's one other option that I came across. Pragmas feel gross, but I
think it will behave as we want, and it doesn't require cooperation from
the callsites at all.

-- >8 --
Subject: [PATCH] disable -Wzero-length-format via #pragma

Building with "gcc -Wall" will complain that the format in:

  warning("")

is empty. Which is true, but the warning is over-eager. We
are calling the function for its side effect of printing
"warning:", even with an empty string.

We disable this warning already with the DEVELOPER Makefile
knob. But we can't unconditionally add -Wno-format-zero-length
to the normal CFLAGS variable, because not all compilers will
understand it. So we may get reports about the warning from
non-developer users who compile with our default of -Wall.

Instead, we can disable the warning using a gcc-specific
#pragma. This should be ignored by non-gcc compilers, and do
what we want for gcc.

I tested also with clang, which often implements gcc
compatible extensions like this. Clang does not generate the
warning in the first place, but also does not complain about
our pragma.

Signed-off-by: Jeff King 
---
 git-compat-util.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 325950426..a6558930d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -413,6 +413,8 @@ extern int error_errno(const char *err, ...) 
__attribute__((format (printf, 1, 2
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 
2)));
 extern void warning_errno(const char *err, ...) __attribute__((format (printf, 
1, 2)));
 
+#pragma GCC diagnostic ignored "-Wformat-zero-length"
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
-- 
2.11.0.840.gd37c5973a

[PATCH] Add more proposals to SoC 2017 ideas page. (lots of submodule stuff)

[Stefan Beller]

Signed-off-by: Stefan Beller 
---

  Peff,
  
  This applies to the repo at https://github.com/git/git.github.io
  
  If you're looking for a co-admin/mentors, I can help out.
  
  Thanks,
  Stefan

 SoC-2017-Ideas.md | 57 +++
 1 file changed, 57 insertions(+)

diff --git a/SoC-2017-Ideas.md b/SoC-2017-Ideas.md
index df98919..6d554b8 100644
--- a/SoC-2017-Ideas.md
+++ b/SoC-2017-Ideas.md
@@ -162,3 +162,60 @@ See discussion in:
 The goal is to better format object related information as discussed in:
 
 
[https://public-inbox.org/git/CA+P7+xr4ZNCCJkS0=yR-FNu+MrL60YX-+Wsz9L_5LCNhnY_d=a...@mail.gmail.com/](https://public-inbox.org/git/CA+P7+xr4ZNCCJkS0=yR-FNu+MrL60YX-+Wsz9L_5LCNhnY_d=a...@mail.gmail.com/)
+
+### Submodule related work:
+
+* Cleanup our test suite.  Do not use a repo itself as a submodule for itself
+  (Search for "git submodule add ./. ")
+
+* Fix the ./ bug for submodule URL handling.
+  (c.f. 
https://public-inbox.org/git/20161021235939.20792-4-sbel...@google.com/)
+
+* Teach "git -C  status" in an un-populated submodule
+  to report the submodule being un-populated, do not fall back to the
+  superproject.
+
+* "git -C sub add ." might behave just like "git add sub"
+
+* Teach "git log -- " to behave
+  like "git -C  log -- 
+
+* git archive(/bundle) to have a --recurse-submodules flag to
+  include the submodule contents.
+
+* Convert a submodule subcommand to C (c.f. 3604242f080a8,
+  submodule: port init from shell to C, 2016-04-15)
+  I'd propose to go for "foreach" first, as that will
+  have most performance impact and is one of the shortest
+
+* (Advanced datastructure knowledge required?)
+  Protect submodule from gc-ing interesting HEADS.
+  Given that the the modules file has a ‘branch’ field, we may want checkout
+  to have the ability to checkout the branch specified in this ‘branch’ field.
+  This can be especially useful when making a brand new branch in the
+  superproject which can then make corresponding branches in the submodules.
+  Or if we are tracking a particular branch, we can checkout that branch
+  (given HEAD of that branch is pointing to the same SHA1 that is checked
+  into the superproject).  This may be needed to avoid unintended garbage
+  collection of commits in the submodules which aren’t reachable by the
+  standard refs/branches.
+
+* (Advanced understanding of usability:)
+  Design and implement an "overlay" for .gitmodules as a ref.
+  To get submodules to usable state, you need to configure a lot. To aid with
+  this the file ".gitmodules" in the repository provides some defaults that
+  are copied to the actual config e.g. in "git submodule init".
+  These defaults are not always the right choice (e.g. when working in a
+  large organisation, you may have an internal git mirror site, that
+  you rather want to clone/fetch from; This can be helped with by configuring
+  e.g. url."".insteadOf; But generally this is a pain for users; this
+  large organisation could provide such a configuration as a ref as well,
+  which has higher priority than the .gitmodules file, but lower priority
+  than the .git/config file.)
+
+### Discourage pushing annotated tag to a branch ref
+  If I run
+
+git push origin v1.0:refs/heads/master
+
+  and v1.0 is an annotated tag, then I probably meant v1.0^{commit}, not 
^{tag}.
-- 
2.11.0.495.g04f60290a0.dirty

Re: [PATCH v2 25/27] attr: store attribute stack in attr_check structure

[Brandon Williams]

On 01/25, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> >> In my mind, the value of having a constant check_attr is primarily
> >> that it gives us a stable pointer to serve as a hashmap key,
> >> i.e. the identifier for each call site, in a later iteration.
> >
> > We didn't really discuss this notion of having the pointer be a key into
> > a hashmap, what sort of information are you envisioning being stored in
> > this sort of hashmap?
> 
> The "entries relevant to this attr_check() call, that is specific to
> the  tuple" (aka "what used to be
> called the global attr_stack") we discussed would be the primary
> example.  A thread is likely be looping in a caller that has many
> paths inside a directory, calling a function that has a call to
> attr_check() for each path.  Having something that can use to
> identify the check_attr instance in a stable way, even when the
> inner function is called and returns many times, would allow us to
> populate the "attr stack" just once for the thread when it enters a
> directory for the first time (remember, another thread may be
> executing the same codepath, checking for paths in a different
> directory) and keep using it.  There may be other mechanisms you can
> come up with, so I wouldn't say it is the only valid way, but it is
> a way.  That is why I said:
> 
> >> But all of the above comes from my intuition, and I'll very much
> >> welcome to be proven wrong with an alternative design, or better
> >> yet, a working code based on an alternative design ;-).
> 
> near the end of my message.
> 
> > One issue I can see with this is that the
> > functions which have a static attr_check struct would still not be thread
> > safe if the initialization of the structure isn't surrounded by a mutex
> > itself. ie
> 
> Yes, that goes without saying.  That is why I suggested Stefan to do
> not this:
> 
> > static struct attr_check *check;
> >
> > if (!check)
> >   init(check);
> >
> > would need to be:
> >
> > lock()
> > if (!check)
> >   init(check);
> > unlock();
> 
> but this:
> 
>   static struct attr_check *check;
>   init();
> 
> and hide the lock/unlock gymnastics inside the API.  I thought that
> already was in what you inherited from him and started your work
> on top of?

I essentially built off of the series you had while using Stefan's
patches as inspiration, but I don't believe the kind of mechanism you
are describing existed in Stefan's series.  His series had a single lock
for the entire system, only allowing a single caller to be in it at any
given time.  This definitely isn't ideal, hence why I picked it up.

Implementation aside I want to try and nail down what the purpose of
this refactor is.  There are roughly two notions of being "thread-safe".

1. The first is that the subsystem itself is thread safe, that is
   multiple threads can be executing inside the subsystem without stepping
   on each others work.

2. The second is that the object itself is thread safe or that multiple
   threads can use the same object.

I thought that the main purpose of this was to achieve (1) since
currently that is not the case.

-- 
Brandon Williams

Re: [PATCH] tag: add tag.createReflog option

[Junio C Hamano]

Jeff King  writes:

> +enum log_refs_config {
> + LOG_REFS_UNSET = -1,
> + LOG_REFS_NONE = 0,
> + LOG_REFS_NORMAL, /* see should_create_reflog for rules */
> + LOG_REFS_ALWAYS
> +};
> +extern enum log_refs_config log_all_ref_updates;
> +...
> +int should_create_reflog(const char *refname)
> +{
> + switch (log_all_ref_updates) {
> + case LOG_REFS_ALWAYS:
> + return 1;
> + case LOG_REFS_NORMAL:
> + return !prefixcmp(refname, "refs/heads/") ||
> +!prefixcmp(refname, "refs/remotes/") ||
> +!prefixcmp(refname, "refs/notes/") ||
> +!strcmp(refname, "HEAD");
> + default:
> + return 0;
> + }
> +}

Yup, this is how I expected for the feature to be done.

Just a hint for Cornelius; prefixcmp() is an old name for what is
called starts_with() these days.

Re: [PATCH] Retire the `relink` command

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jan 25, 2017 at 05:58:57PM +0100, Johannes Schindelin wrote:
>
>> Back in the olden days, when all objects were loose and rubber boots were
>> made out of wood, it made sense to try to share (immutable) objects
>> between repositories.
>> 
>> Ever since the arrival of pack files, it is but an anachronism.
>
> Yes, this is a good idea. I could _almost_ see its utility if it linked
> packfiles, too, but then it is very unlikely that two repos have the
> exact same packfile.
>
>> Let's move the script to the contrib/examples/ directory and no longer
>> offer it.
>
> I am OK with this, but perhaps we should go even further and just delete
> it entirely. The point of contrib/examples is to show people "this is
> how you could script plumbing to implement a porcelain". But this script
> does not call a single plumbing script. It just looks directly in
> objects/, which is probably not something we would want to encourage. :)

Yeah.  I am OK with this patch as the first step of "first move to
contrib and then remove" two step process, but I suspect we may
forget to follow through.

Re: [PATCH] mingw: allow hooks to be .exe files

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>
>> This change is necessary to allow the files in .git/hooks/ to optionally
>> have the file extension `.exe` on Windows, as the file names are
>> hardcoded otherwise.
>
> Make sense as a goal.
>
>> -if (access(path.buf, X_OK) < 0)
>> +if (access(path.buf, X_OK) < 0) {
>> +#ifdef STRIP_EXTENSION
>> +strbuf_addstr(, ".exe");
>
> I think STRIP_EXTENSION is a string.  Should this line be:
>
>   strbuf_addstr(, STRIP_EXTENSION);
>
> ?

Yup, I think that is more in line with how git.c (the other user of
the macro) uses it.

Re: [PATCH] mingw: allow hooks to be .exe files

[Junio C Hamano]

Johannes Schindelin  writes:

> This change is necessary to allow the files in .git/hooks/ to optionally
> have the file extension `.exe` on Windows, as the file names are
> hardcoded otherwise.

Hmph.  There is no longer ".com"?

I briefly wondered if hooks/post-receive.{py,rb,...} would be good
things to support, but I do not think we want to go there, primarily
because we do not want to deal with "what happens when there are
many?"

As Peff pointed out while I was typing this message, ".exe" would be
better spelled as STRIP_EXTENSION, I think.  The resulting code
would read naturally when you read the macro not as "please do strip
extensions" boolean, but as "this extension is to be stripped"
string, which is how it is used in the other site the macro is used
(namely, strip_extension() called by handle_builtin()).

> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v1
>
>  run-command.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index 73bfba7ef9..45229ef052 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -871,8 +871,14 @@ const char *find_hook(const char *name)
>  
>   strbuf_reset();
>   strbuf_git_path(, "hooks/%s", name);
> - if (access(path.buf, X_OK) < 0)
> + if (access(path.buf, X_OK) < 0) {
> +#ifdef STRIP_EXTENSION
> + strbuf_addstr(, ".exe");
> + if (access(path.buf, X_OK) >= 0)
> + return path.buf;
> +#endif
>   return NULL;
> + }
>   return path.buf;
>  }
>  
>
> base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe

Re: [PATCH v2 0/7] Macros for Asciidoctor support

[Jeff King]

On Wed, Jan 25, 2017 at 02:28:55PM +0100, Johannes Schindelin wrote:

> > The need for the extensions could be replaced with a small amount of
> > Ruby code, if that's considered desirable.  Previous opinions on doing
> > so were negative, however.
> 
> Quite frankly, it is annoying to be forced to install the extensions. I
> would much rather have the small amount of Ruby code in Git's repository.

Me too. Dependencies can be a big annoyance. I'd reserve judgement until
I saw the actual Ruby code, though. :)

-Peff

Re: [PATCH] tag: add tag.createReflog option

[Jeff King]

On Wed, Jan 25, 2017 at 10:21:48PM +0100, Cornelius Weig wrote:

> On 01/25/2017 07:00 PM, Jeff King wrote:
> 
> >   - Is that the end of it, or is the desire really "I want reflogs for
> > _everything_"? That seems like a sane thing to want.
> > 
> > If so, then the update to core.logallrefupdates should turn it into
> > a tri-state:
> > 
> >   - false; no reflogs
> > 
> >   - true; reflogs for branches, remotes, notes, as now
> > 
> >   - always; reflogs for all refs under "refs/"
> > 
> 
> I think you nailed it. This is much more useful than what I suggested.
> I'll see if I can code it up.

I cheated a little. I actually wrote the "always" patch 3 years ago as
part of another thing I was working on. But in the end I didn't need it,
and never submitted it.

The patch is below for reference. I have no idea whether it even applies
now, let alone runs and does the right thing. But perhaps you can
salvage bits of it (but feel free to ignore it if it makes things
harder).

-- >8 --
From: Jeff King 
Date: Mon, 15 Apr 2013 23:31:05 -0400
Subject: [PATCH] teach core.logallrefupdates an "always" mode

When core.logallrefupdates is true, we only create a new
reflog for refs that are under certain well-known
hierarchies. The reason is that we know that some
hierarchies (like refs/tags) do not typically change, and
that unknown hierarchies might not want reflogs at all
(e.g., a hypothetical refs/foo might be meant to change
often and drop old history immediately).

However, sometimes it is useful to override this decision
and simply log for all refs, because the safety and audit
trail is more important than the performance implications of
keeping the log around.

This patch introduces a new "always" mode for the
core.logallrefupdates option which will log updates to
everything under refs/, regardless where in the hierarchy it
is (we still will not log things like ORIG_HEAD and
FETCH_HEAD, which are known to be transient).

Signed-off-by: Jeff King 
---
 Documentation/config.txt |  8 +---
 branch.c |  2 +-
 builtin/checkout.c   |  2 +-
 builtin/init-db.c|  2 +-
 cache.h  |  9 -
 config.c |  7 ++-
 environment.c|  2 +-
 refs.c   | 23 +--
 t/t1400-update-ref.sh| 32 
 9 files changed, 72 insertions(+), 15 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e37ba94a72..cb72e559ec 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -390,10 +390,12 @@ core.logAllRefUpdates::
"$GIT_DIR/logs/", by appending the new and old
SHA1, the date/time and the reason of the update, but
only when the file exists.  If this configuration
-   variable is set to true, missing "$GIT_DIR/logs/"
+   variable is set to `true`, a missing "$GIT_DIR/logs/"
file is automatically created for branch heads (i.e. under
-   refs/heads/), remote refs (i.e. under refs/remotes/),
-   note refs (i.e. under refs/notes/), and the symbolic ref HEAD.
+   `refs/heads/`), remote refs (i.e. under `refs/remotes/`),
+   note refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.
+   If it is set to `always`, then a missing reflog is automatically
+   created for any ref under `refs/`.
 +
 This information can be used to determine what commit
 was the tip of a branch "2 days ago".
diff --git a/branch.c b/branch.c
index 2bef1e7e71..c11880b181 100644
--- a/branch.c
+++ b/branch.c
@@ -259,7 +259,7 @@ void create_branch(const char *head,
}
 
if (reflog)
-   log_all_ref_updates = 1;
+   log_all_ref_updates = LOG_REFS_NORMAL;
 
if (forcing)
snprintf(msg, sizeof msg, "branch: Reset to %s",
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a9c1b5a95f..00e231d83b 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -564,7 +564,7 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
char *ref_name = mkpath("refs/heads/%s", 
opts->new_orphan_branch);
 
temp = log_all_ref_updates;
-   log_all_ref_updates = 1;
+   log_all_ref_updates = LOG_REFS_NORMAL;
if (log_ref_setup(ref_name, log_file, 
sizeof(log_file))) {
fprintf(stderr, _("Can not do reflog 
for '%s'\n"),
opts->new_orphan_branch);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index 78aa3872dd..0ebad0b37d 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -264,7 +264,7 @@ static int create_default_files(const char *template_path)
const char *work_tree = get_git_work_tree();
git_config_set("core.bare", "false");
   

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jacob Keller]

On Wed, Jan 25, 2017 at 1:27 PM, Jeff King  wrote:
> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>
>> IOW, the ref-selector options build up until a group option is given,
>> which acts on the built-up options (over that group) and then resets the
>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>> think in practice nobody would do that, because it's hard to read).
>
> So here's what I would have expected your series to look more like (with
> probably one patch adding clear_ref_selection_options, and the other
> adding the decorate stuff):
>

I agree that this is how I would have expected it to work as well.

Thanks,
Jake

> diff --git a/revision.c b/revision.c
> index b37dbec37..2f67707c7 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const 
> struct object_id *oid,
>
> if (ref_excluded(cb->all_revs->ref_excludes, path))
> return 0;
> +   if (cb->all_revs->decorate_reflog) {
> +   /* TODO actually do it for real */
> +   warning("would decorate %s", path);
> +   return 0; /* do not add it as a tip */
> +   }
>
> object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
> add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, 
> cb->all_flags);
> @@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list 
> **ref_excludes_p, const char *exclude)
> string_list_append(*ref_excludes_p, exclude);
>  }
>
> +static void clear_ref_selection_options(struct rev_info *revs)
> +{
> +   clear_ref_exclusion(>ref_excludes);
> +   revs->decorate_reflog = 0;
> +}
> +
>  static void handle_refs(const char *submodule, struct rev_info *revs, 
> unsigned flags,
> int (*for_each)(const char *, each_ref_fn, void *))
>  {
> @@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
> if (!strcmp(arg, "--all")) {
> handle_refs(submodule, revs, *flags, for_each_ref_submodule);
> handle_refs(submodule, revs, *flags, head_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--branches")) {
> handle_refs(submodule, revs, *flags, 
> for_each_branch_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--bisect")) {
> read_bisect_terms(_bad, _good);
> handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
> @@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
> revs->bisect = 1;
> } else if (!strcmp(arg, "--tags")) {
> handle_refs(submodule, revs, *flags, 
> for_each_tag_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--remotes")) {
> handle_refs(submodule, revs, *flags, 
> for_each_remote_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if ((argcount = parse_long_opt("glob", argv, ))) {
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref(handle_one_ref, optarg, );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> return argcount;
> } else if ((argcount = parse_long_opt("exclude", argv, ))) {
> add_ref_exclusion(>ref_excludes, optarg);
> @@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", 
> );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (starts_with(arg, "--tags=")) {
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", 
> );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (starts_with(arg, "--remotes=")) {
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 10, 
> "refs/remotes/", );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> +   } else if (!strcmp(arg, "--decorate-reflog")) {
> +   revs->decorate_reflog = 1;
> } else if 

Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Junio C Hamano]

Jeff King  writes:

> The only advantage is that it is self-documenting, so somebody does not
> come through later and convert ("%s", "") back to (""). We could also
> write a comment. But I am happy if we simply catch it in review (or
> preferably the person is clueful enough to read the output of git-blame
> and see why it is that way in the first place).

And the last sentence unfortunatly does not reflect reality.  

I would prefer something self-documenting, like your wrapper with a
comment.  Then somebody who is looking at the implementation of
warning_blank_line() will not get tempted to turn "%s", "" into ""
because of the comment.  And somebody who is looking at the callsite
of warning_blank_line() will think twice before suggesting to turn
it into warning("").

That does not make it unnecessary to review; we still need to catch
those who wants to add new calls to warning("") without even knowing
the presence of warning_blank_line(), if the original codepath being
touched does not have any call to it.

> So maybe:

In any case, the patch is a minimum effort band-aid that lets us
punt on the whole issue for now, so I'll queue it as-is.

Thanks.


> -- >8 --
> Subject: [PATCH] difftool: hack around -Wzero-length-format warning
>
> Building with "gcc -Wall" will complain that the format in:
>
>   warning("")
>
> is empty. Which is true, but the warning is over-eager. We
> are calling the function for its side effect of printing
> "warning:", even with an empty string.
>
> Our DEVELOPER Makefile knob disables the warning, but not
> everybody uses it. Let's silence the warning in the code so
> that nobody reports it or tries to "fix" it.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/difftool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 42ad9e804..b5e85ab07 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
> const char *prefix,
>   warning(_("both files modified: '%s' and 
> '%s'."),
>   wtdir.buf, rdir.buf);
>   warning(_("working tree file has been left."));
> - warning("");
> + warning("%s", "");
>   err = 1;
>   } else if (unlink(wtdir.buf) ||
>  copy_file(wtdir.buf, rdir.buf, st.st_mode))

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:

> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).

So here's what I would have expected your series to look more like (with
probably one patch adding clear_ref_selection_options, and the other
adding the decorate stuff):

diff --git a/revision.c b/revision.c
index b37dbec37..2f67707c7 100644
--- a/revision.c
+++ b/revision.c
@@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct 
object_id *oid,
 
if (ref_excluded(cb->all_revs->ref_excludes, path))
return 0;
+   if (cb->all_revs->decorate_reflog) {
+   /* TODO actually do it for real */
+   warning("would decorate %s", path);
+   return 0; /* do not add it as a tip */
+   }
 
object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
@@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list 
**ref_excludes_p, const char *exclude)
string_list_append(*ref_excludes_p, exclude);
 }
 
+static void clear_ref_selection_options(struct rev_info *revs)
+{
+   clear_ref_exclusion(>ref_excludes);
+   revs->decorate_reflog = 0;
+}
+
 static void handle_refs(const char *submodule, struct rev_info *revs, unsigned 
flags,
int (*for_each)(const char *, each_ref_fn, void *))
 {
@@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
if (!strcmp(arg, "--all")) {
handle_refs(submodule, revs, *flags, for_each_ref_submodule);
handle_refs(submodule, revs, *flags, head_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--branches")) {
handle_refs(submodule, revs, *flags, 
for_each_branch_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--bisect")) {
read_bisect_terms(_bad, _good);
handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
@@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
revs->bisect = 1;
} else if (!strcmp(arg, "--tags")) {
handle_refs(submodule, revs, *flags, 
for_each_tag_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--remotes")) {
handle_refs(submodule, revs, *flags, 
for_each_remote_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if ((argcount = parse_long_opt("glob", argv, ))) {
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref(handle_one_ref, optarg, );
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
return argcount;
} else if ((argcount = parse_long_opt("exclude", argv, ))) {
add_ref_exclusion(>ref_excludes, optarg);
@@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", 
);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (starts_with(arg, "--tags=")) {
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", 
);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (starts_with(arg, "--remotes=")) {
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", 
);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
+   } else if (!strcmp(arg, "--decorate-reflog")) {
+   revs->decorate_reflog = 1;
} else if (!strcmp(arg, "--reflog")) {
add_reflogs_to_pending(revs, *flags);
} else if (!strcmp(arg, "--indexed-objects")) {
diff --git a/revision.h b/revision.h
index 9fac1a607..c74879829 100644
--- a/revision.h
+++ b/revision.h
@@ -66,6 +66,8 @@ struct rev_info {
/* excluding from --branches, --refs, etc. expansion */

Re: [PATCH] tag: add tag.createReflog option

[Cornelius Weig]

On 01/25/2017 07:00 PM, Jeff King wrote:

>   - Is that the end of it, or is the desire really "I want reflogs for
> _everything_"? That seems like a sane thing to want.
> 
> If so, then the update to core.logallrefupdates should turn it into
> a tri-state:
> 
>   - false; no reflogs
> 
>   - true; reflogs for branches, remotes, notes, as now
> 
>   - always; reflogs for all refs under "refs/"
> 

I think you nailed it. This is much more useful than what I suggested.
I'll see if I can code it up.

Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard

[Junio C Hamano]

Jakub Narębski  writes:

>>> -   Save your local modifications to a new 'stash', and run `git reset
>>> -   --hard` to revert them.  The  part is optional and gives
>>> +   Save your local modifications to a new 'stash', and revert the
>>> +   the changes in the working tree to match the index.
>>> +   The  part is optional and gives
>> 
>> Hrm. "git reset --hard" doesn't just make the working tree match the
>> index. It also resets the index to HEAD.  So either the original or your
>> new description is wrong.
>> 
>> I think it's the latter. We really do reset the index unless
>> --keep-index is specified.
>
> I wonder if it is worth mentioning that "saving local modifications"
> in 'git stash' means storing both the state of the worktree (of tracked
> files in it) _and_ the state of the index, before both get set to
> state of HEAD.

Yes, it is, I would say.

Re: [PATCH 1/3] Documentation/stash: remove mention of git reset --hard

[Junio C Hamano]

Jeff King  writes:

> On Sat, Jan 21, 2017 at 08:08:02PM +, Thomas Gummerer wrote:
>
>> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
>> index 2e9cef06e6..0ad5335a3e 100644
>> --- a/Documentation/git-stash.txt
>> +++ b/Documentation/git-stash.txt
>> @@ -47,8 +47,9 @@ OPTIONS
>>  
>>  save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] 
>> [-a|--all] [-q|--quiet] []::
>>  
>> -Save your local modifications to a new 'stash', and run `git reset
>> ---hard` to revert them.  The  part is optional and gives
>> +Save your local modifications to a new 'stash', and revert the
>> +the changes in the working tree to match the index.
>> +The  part is optional and gives
>
> Hrm. "git reset --hard" doesn't just make the working tree match the
> index. It also resets the index to HEAD.  So either the original or your
> new description is wrong.
>
> I think it's the latter. We really do reset the index unless
> --keep-index is specified.

Correct.  So the patch is a net loss.  Perhaps not requiring readers
to know "reset --hard" might be an improvement (I personally do not
think so), but this loses crucial information from the description.

Save your local modifications (both in the working tree and
to the index) to a new 'stash', and resets the index to HEAD
and makes the working tree match the index (i.e. runs "git
reset --hard").

That's one-and-a-half lines longer than the original, though.

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

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

> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
>
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
>
> --exclude .. --decorate-reflog ... --remotes
>
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?

I would expect that the effect of exclude, decorate-reflog and
friends will extend until the next occurrence of --remotes (or --all
or whatever you catch in parse_ref_selector_option() function).

So, I'd read it as "add all remote tracking refs, but (1) exclude
exclude the refs matching pattern .. and (2) use reflog of them if
they match pattern ...".

Or did you mean by "..." something other than a single argument that
is a pattern?

Re: [PATCH] mingw: allow hooks to be .exe files

[Jeff King]

On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:

> This change is necessary to allow the files in .git/hooks/ to optionally
> have the file extension `.exe` on Windows, as the file names are
> hardcoded otherwise.

Make sense as a goal.

> - if (access(path.buf, X_OK) < 0)
> + if (access(path.buf, X_OK) < 0) {
> +#ifdef STRIP_EXTENSION
> + strbuf_addstr(, ".exe");

I think STRIP_EXTENSION is a string.  Should this line be:

  strbuf_addstr(, STRIP_EXTENSION);

?

-Peff

Re: [PATCH] Retire the `relink` command

[Jeff King]

On Wed, Jan 25, 2017 at 05:58:57PM +0100, Johannes Schindelin wrote:

> Back in the olden days, when all objects were loose and rubber boots were
> made out of wood, it made sense to try to share (immutable) objects
> between repositories.
> 
> Ever since the arrival of pack files, it is but an anachronism.

Yes, this is a good idea. I could _almost_ see its utility if it linked
packfiles, too, but then it is very unlikely that two repos have the
exact same packfile.

> Let's move the script to the contrib/examples/ directory and no longer
> offer it.

I am OK with this, but perhaps we should go even further and just delete
it entirely. The point of contrib/examples is to show people "this is
how you could script plumbing to implement a porcelain". But this script
does not call a single plumbing script. It just looks directly in
objects/, which is probably not something we would want to encourage. :)

-Peff

Re: [PATCH 2/5] revision.c: group ref selection options together

[Junio C Hamano]

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

> These options have on thing in common: when specified right after

one thing.

> --exclude, they will de-select refs instead of selecting them by
> default.
>
> This change makes it possible to introduce new options that use these
> options in the same way as --exclude. Such an option would just
> implement something like handle_refs_pseudo_opt().
>
> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
> that similar functions like handle_refs_pseudo_opt() are forced to
> handle all ref selector options, not skipping some by mistake, which may
> revert the option back to default behavior (rev selection).

I am not sure about these two refactorings, for at least two reasons.

 * Naming.  The function is all about handling "refs options" and I
   do not see anything "pseudo" about the options handled by the
   handle_refs_pseudo_opt() function.  This is minor.

 * I am expecting that the new one yet to be introduced will not
   share the huge "switch (selector)" part, but does its own things
   in a separate function with a similar structure.  The only thing
   common between these two functions would be the structure
   (i.e. it has a big "switch(selector)" that does different things
   depending on REF_SELECT_*) and a call to clear_* function.

   If we were to add a new kind of REF_SELECT_* (say
   REF_SELECT_NOTES just for the sake of being concrete), what
   changes will be needed to the code if the addition of "use reflog
   from this class of refs for decoration" feature was done with or
   without this step?  I have a suspicion that the change will be
   simpler without this step.

   The above comments will be invalid if the new "use reflog for
   decoration" feature will be done by extending this pseudo_opt()
   function by extending each of the switch/case arms.  If that is
   the case, please disregard the above.  I just didn't get that
   impression from the above paragraph.

Re: [PATCH 06/12] clone: disable save_commit_buffer

[Jeff King]

On Wed, Jan 25, 2017 at 02:35:41PM -0500, Jeff King wrote:

> On Wed, Jan 25, 2017 at 02:27:40PM -0500, Jeff King wrote:
> 
> > > > For fetching operations like clone, we already disable
> > > 
> > > s/clone/fetch/ you meant?
> > 
> > Well, no, because this patch deals with clone.
> > 
> > It's likely that builtin/fetch.c would want the same treatment. It
> > didn't come up for me because I've disabled the alternates check for
> > that case (but you can't do that with stock git), and I didn't dig
> > further.
> > 
> > Possibly this should just go into fetch-pack.c, right before we walk
> > over all of the refs and call parse_object() and deref_tag() on all of
> > them. It feels a little funny to tweak the global save_commit_buffer
> > flag there, but it already do so in everything_local(), so I don't think
> > we're really losing much.
> 
> Hrm. So I thought it might be useful to write a patch that just tweaks
> save_commit_buffer at the start of fetch_pack(). But looking it over,
> we call everything_local() _before_ we walk over all the refs. So
> save_commit_buffer should already be turned off for my case.
> 
> I wonder if I made a mistake while measuring the peak RSS. Or if clone
> parses some commits before it calls into fetch_pack() (but which
> objects? It doesn't have any until it does the fetch).
> 
> Perhaps we should just drop this patch (though I think it is logically
> consistent and wouldn't hurt anything).

OK, I just repeated my heap measurements with valgrind's massif tool,
specifically checking builds of this patch and its parent. I couldn't
find any improvement. So I must have screwed something up in my earlier
measurements.

Sorry for the noise. I think we should probably just drop this patch.

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Wed, Jan 25, 2017 at 07:50:53PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
> 
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
> 
> --exclude .. --decorate-reflog ... --remotes
> 
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?

I don't think it means either. It means to include remotes in the
selected revisions, but excluding the entries mentioned by --exclude.

IOW:

  --exclude=foo --remotes
include all remotes except refs/remotes/foo

  --exclude=foo --unrelated --remotes
same

  --exclude=foo --decorate-reflog --remotes
decorate reflogs of all remotes except "foo". Do _not_ use them
as traversal tips.

  --decorate-reflog --exclude=foo --remotes
same

IOW, the ref-selector options build up until a group option is given,
which acts on the built-up options (over that group) and then resets the
built-up options. Doing "--unrelated" as above is orthogonal (though I
think in practice nobody would do that, because it's hard to read).

> Granted, there may be valid use cases for such a combination (e.g.
> "decorate all reflogs except remote ones") but I feel option order is
> not a good fit to express them.

That would be spelled:

  --exclude=refs/remotes --decorate-reflogs --all

(or you could swap the first two options).

Again, I'm not sure if I'm missing something subtle, or if you are
confused about how --exclude works. :)

-Peff

Re: [PATCH v3 14/21] read-cache: touch shared index files when used

[Junio C Hamano]

Christian Couder  writes:

> Well, when we cannot freshen a loose file (with
> freshen_loose_object()), we don't warn or die, we just write the loose
> file. But here we cannot write the shared index file.

I think that is an excellent point.  Let me make sure I got you
right.  For loose object files, we may attempt to freshen and when
it fails we stay silent and do not talk about the failure.  Instead,
we write the same file again.  That will have two potential outcomes:

 1. write fails and we fail the whole thing.  It is very clear to
the user that there is something wrong going on.

 2. write succeeds, and because we just wrote it, we know that the
file is fresh and is protected from gc.

So the "freshen and if fails just write" is sufficient.  It is
absolutely the right thing to do for loose object files.

When we are forking off a new split index file based on an old
shared index file, we may attempt to "touch" the old shared one, but
we cannot write the old shared one again, because other split index
may be based on that, and we do not have enough information to
recreate the old one [*1*].  The fallback safety is not available.

> And this doesn't lead to a catastrophic failure right away. 

Exactly.

> There
> could be a catastrophic failure if the shared index file is needed
> again later, but we are not sure that it's going to be needed later.
> In fact it may have just been removed because it won't be needed
> later.

You are listing only the irrelevant cases.  The shared one may be
used immediately, and the user can keep using it for a while without
"touching".  Perhaps the shared one was chowned to "root" while the
user is looking the other way, and its timestamp is now frozen to
the time of chown.  It is a ticking time-bomb that will go off when
your expiry logic kicks in.

> So I am not sure it's a good idea to anticipate a catastrophic failure
> that may not happen. Perhaps we could just warn, but I am not sure it
> will help the user. If the catastrophic failure doesn't happen because
> the shared index file is not needed, I can't see how the warning could
> help. And if there is a catastrophic failure, the following will be
> displayed:
>
> fatal: .git/sharedindex.cbfe41352a4623d4d3e9757861fed53f3094e0ac:
> index file open failed: No such file or directory

That "fatal" is given _ONLY_ after time passes and our failure to
"touch" the file that is still-in-use left it subject to "gc".  Of
course, when "fatal" is given, it is too late to warn about ticking
time bomb.

At the time we notice a ticking time bomb is the only sensible time
to warn.  Or better yet take a corrective action.

As I expect (and you seem to agree) that a failure to "touch" would
be a very rare event (like, sysadmin chowning it to "root" by
mistake), I do not mind if the "corrective action" were "immediately
unsplit the index, so that at least the current and the latest index
contents will be written out safely to a new single unsplit index
file".  That won't help _other_ split index files that are based on
the same "untouchable" shared index, but I do not think that is a
problem we need to solve---if they are still in use, the code that
use them will notice it, and otherwise they are not in use and can
be aged away safely.


[Footnote]

*1* My understanding is that we lose information on stale entries in
the shared file that are covered by the split index overlay
after read_index() returns, so we _might_ be able to write the
"old" one that is sufficient for _our_ split index, but we do
not have good enough information to recreate "old" one usable by
other split index files.

Re: [PATCH 2/5] revision.c: group ref selection options together

[Jeff King]

On Wed, Jan 25, 2017 at 07:50:51PM +0700, Nguyễn Thái Ngọc Duy wrote:

> These options have on thing in common: when specified right after
> --exclude, they will de-select refs instead of selecting them by
> default.
> 
> This change makes it possible to introduce new options that use these
> options in the same way as --exclude. Such an option would just
> implement something like handle_refs_pseudo_opt().
> 
> parse_ref_selector_option() is taken out of handle_refs_pseudo_opt() so
> that similar functions like handle_refs_pseudo_opt() are forced to
> handle all ref selector options, not skipping some by mistake, which may
> revert the option back to default behavior (rev selection).
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  revision.c | 134 
> +
>  1 file changed, 100 insertions(+), 34 deletions(-)

Hmm. I see what you're trying to do here, and abstract the repeated
bits. But I'm not sure the line-count reflects a real simplification.
Everything ends up converted to an enum, and then that enum just expands
to similar C code.

I kind of expected that clear_ref_exclusion() would just become a more
abstract clear_ref_selection(). For now it would clear exclusions, and
then later learn to clear the decoration flags.

Maybe I am missing something in the later patches, though.

-Peff

Re: GSoC 2017: application open, deadline = February 9, 2017

[Jeff King]

On Mon, Jan 23, 2017 at 04:02:02PM +0100, Matthieu Moy wrote:

> If the answer to one of the above question is yes, then:
> 
> * Who's willing to mentor? and to admin?

I did co-admin last year and the year before, but I made Matthieu do all
the work. :)

I do not mind doing the administrative stuff.  But the real work is in
polishing up the ideas list and microprojects page. So I think the first
step, if people are interested in GSoC, is not just to say "yes, let's
do it", but to actually flesh out these pages:

> * We need to write the application, i.e. essentially polish and update
>   the text here: https://git.github.io/SoC-2016-Org-Application/ and
>   update the list of project ideas and microprojects :
>   https://git.github.io/SoC-2017-Ideas/
>   https://git.github.io/SoC-2016-Microprojects/

That can be done incrementally by people who care (especially mentors)
over the next week or so, and doesn't require any real admin
coordination. If it happens and the result looks good, then the
application process is pretty straightforward.

If it doesn't, then we probably ought not to participate in GSoC.

-Peff

PS If we do participate, we should consider Outreachy as well, which can
   expand our base of possible applicants. Though note that ones who are
   not eligible for GSoC would need to be funded by us. Last year GitHub
   offered to cover the stipend for an Outreachy intern for Git. If
   there's interest I can see if that offer can be extended for this
   year.

Re: What's cooking in git.git (Jan 2017, #04; Mon, 23)

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Jan 23, 2017 at 4:18 PM, Junio C Hamano  wrote:
>>
>> * sb/unpack-trees-super-prefix (2017-01-12) 5 commits
>>  - SQUASH
>>  - unpack-trees: support super-prefix option
>>  - t1001: modernize style
>>  - t1000: modernize style
>>  - read-tree: use OPT_BOOL instead of OPT_SET_INT
>>
>>  "git read-tree" and its underlying unpack_trees() machinery learned
>>  to report problematic paths prefixed with the --super-prefix option.
>>
>>  Expecting a reroll.
>>  The first three are in good shape.  The last one needs a better
>>  explanation and possibly an update to its test.
>>  cf.