[PATCH RFC v2 25/25] cat-file: update of docs

[Olga Telezhnaya]

Update the docs for cat-file command. Some new formatting atoms added
because of reusing ref-filter code.
We do not support cat-file atoms in general formatting logic
(there is just the support for cat-file), that is why some of the atoms
are still explained in cat-file docs.
We need to move these explanations when atoms will be supported
by other commands.

Signed-off-by: Olga Telezhnaia <olyatelezhn...@gmail.com>
Mentored-by: Christian Couder <christian.cou...@gmail.com>
Mentored by: Jeff King <p...@peff.net>
---
 Documentation/git-cat-file.txt | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index f90f09b03fae5..90639ac21d0e8 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -187,17 +187,8 @@ linkgit:git-rev-parse[1].
 You can specify the information shown for each object by using a custom
 ``. The `` is copied literally to stdout for each
 object, with placeholders of the form `%(atom)` expanded, followed by a
-newline. The available atoms are:
-
-`objectname`::
-   The 40-hex object name of the object.
-
-`objecttype`::
-   The type of the object (the same as `cat-file -t` reports).
-
-`objectsize`::
-   The size, in bytes, of the object (the same as `cat-file -s`
-   reports).
+newline. The available atoms are the same as that of
+linkgit:git-for-each-ref[1], but there are some additional ones:
 
 `objectsize:disk`::
The size, in bytes, that the object takes up on disk. See the

--
https://github.com/git/git/pull/452

Re: [PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head

[SZEDER Gábor]

> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index da565624e3..d1a23bcdaf 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh


> @@ -107,6 +112,9 @@ test_expect_success 'setup bare repo' \
>  test_expect_success 'write graph in bare repo' \
>  'graphbare=$(git commit-graph --write) &&
>   test_path_is_file ${baredir}/graph-${graphbare}.graph &&
> + test_path_is_file ${baredir}/graph-head &&

This test and the one preceeding it are wrong.

Note that 'git commit-graph --write' above is missing the
'--update-head' option, so there should be no graph-head file written,
yet this 'this test_path_is_file' doesn't fail the test.

The devil lies in the previous test 'setup bare repo', where this bare
repo is created by cloning from a local remote: a simple 'git clone
--bare full bare' hardlinks all files under .git/objects, including
all graph and graph-head files that exist in the remote repo.

The previous test should run 'git clone --bare --no-local full bare'
instead, and then this test would fail because of the missing
graph-head file, as it should.  Specifying '--update-head' will make
it work again.


> + echo ${graphbare} >expect &&
> + cmp -n 40 expect ${baredir}/graph-head &&
>   git commit-graph --read --graph-hash=${graphbare} >output &&
>   _graph_read_expect "18" "${baredir}" &&
>   cmp expect output'

Re: [PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head

[SZEDER Gábor]

> It is possible to have multiple commit graph files in a pack directory,
> but only one is important at a time. Use a 'graph_head' file to point
> to the important file.

This implies that all those other files are ignored, right?

> Teach git-commit-graph to write 'graph_head' upon
> writing a new commit graph file.
> 
> Signed-off-by: Derrick Stolee <dsto...@microsoft.com>
> ---
>  Documentation/git-commit-graph.txt | 34 ++
>  builtin/commit-graph.c | 38 
> +++---
>  commit-graph.c | 25 +
>  commit-graph.h |  2 ++
>  t/t5318-commit-graph.sh| 12 ++--
>  5 files changed, 106 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 09aeaf6c82..99ced16ddc 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -12,15 +12,49 @@ SYNOPSIS
>  'git commit-graph' --write  [--pack-dir ]
>  'git commit-graph' --read  [--pack-dir ]
>  
> +OPTIONS
> +---

Oh, look, the 'OPTIONS' section I missed in the previous patches! ;)

This should be split up and squashed into the previous patches where
the individual --options are first mentioned.

> +--pack-dir::
> + Use given directory for the location of packfiles, graph-head,
> + and graph files.
> +
> +--read::
> + Read a graph file given by the graph-head file and output basic
> + details about the graph file. (Cannot be combined with --write.)

>From the output of 'git commit-graph --read' it seems that it's not a
generally useful option to the user.  Perhaps it should be mentioned
that it's only intended as a debugging aid?  Or maybe it doesn't
really matter, because eventually this command will become irrelevant,
as other commands (clone, fetch, gc) will invoke it automagically...

> +--graph-id::
> + When used with --read, consider the graph file graph-.graph.
> +
> +--write::
> + Write a new graph file to the pack directory. (Cannot be combined
> + with --read.)

I think this should also mention that it prints the generated graph
file's checksum.

> +
> +--update-head::
> + When used with --write, update the graph-head file to point to
> + the written graph file.

So it should be used with '--write', noted.

>  EXAMPLES
>  
>  
> +* Output the hash of the graph file pointed to by /graph-head.
> ++
> +
> +$ git commit-graph --pack-dir=
> +
> +
>  * Write a commit graph file for the packed commits in your local .git folder.
>  +
>  ----
>  $ git commit-graph --write
>  
>  
> +* Write a graph file for the packed commits in your local .git folder,
> +* and update graph-head.
> ++
> +
> +$ git commit-graph --write --update-head
> +
> +
>  * Read basic information from a graph file.
>  +
>  
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 218740b1f8..d73cbc907d 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -11,7 +11,7 @@
>  static char const * const builtin_commit_graph_usage[] = {
>   N_("git commit-graph [--pack-dir ]"),
>   N_("git commit-graph --read [--graph-hash=]"),
> - N_("git commit-graph --write [--pack-dir ]"),
> + N_("git commit-graph --write [--pack-dir ] [--update-head]"),
>   NULL
>  };
>  
> @@ -20,6 +20,9 @@ static struct opts_commit_graph {
>   int read;
>   const char *graph_hash;
>   int write;
> + int update_head;
> + int has_existing;
> + struct object_id old_graph_hash;
>  } opts;
>  
>  static int graph_read(void)
> @@ -30,8 +33,8 @@ static int graph_read(void)
>  
>   if (opts.graph_hash && strlen(opts.graph_hash) == GIT_MAX_HEXSZ)
>   get_oid_hex(opts.graph_hash, _hash);
> - else
> - die("no graph hash specified");
> + else if (!get_graph_head_hash(opts.pack_dir, _hash))
> + die("no graph-head exists");
>  
>   graph_file = get_commit_graph_filename_hash(opts.pack_dir, _hash);
>   graph = load_commit_graph_one(graph_file, opts.pack_dir);
> @@ -62,10 +65,33 @@ static int graph_read(void)
>   return 0;
>  }
>  
> +static void update_head_file(const char

[PATCH v2 07/14] commit-graph: implement git-commit-graph --update-head

[Derrick Stolee]

It is possible to have multiple commit graph files in a pack directory,
but only one is important at a time. Use a 'graph_head' file to point
to the important file. Teach git-commit-graph to write 'graph_head' upon
writing a new commit graph file.

Signed-off-by: Derrick Stolee <dsto...@microsoft.com>
---
 Documentation/git-commit-graph.txt | 34 ++
 builtin/commit-graph.c | 38 +++---
 commit-graph.c | 25 +
 commit-graph.h |  2 ++
 t/t5318-commit-graph.sh| 12 ++--
 5 files changed, 106 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 09aeaf6c82..99ced16ddc 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -12,15 +12,49 @@ SYNOPSIS
 'git commit-graph' --write  [--pack-dir ]
 'git commit-graph' --read  [--pack-dir ]
 
+OPTIONS
+---
+--pack-dir::
+   Use given directory for the location of packfiles, graph-head,
+   and graph files.
+
+--read::
+   Read a graph file given by the graph-head file and output basic
+   details about the graph file. (Cannot be combined with --write.)
+
+--graph-id::
+   When used with --read, consider the graph file graph-.graph.
+
+--write::
+   Write a new graph file to the pack directory. (Cannot be combined
+   with --read.)
+
+--update-head::
+   When used with --write, update the graph-head file to point to
+   the written graph file.
+
 EXAMPLES
 
 
+* Output the hash of the graph file pointed to by /graph-head.
++
+
+$ git commit-graph --pack-dir=
+
+
 * Write a commit graph file for the packed commits in your local .git folder.
 +
 
 $ git commit-graph --write
 
 
+* Write a graph file for the packed commits in your local .git folder,
+* and update graph-head.
++
+
+$ git commit-graph --write --update-head
+
+
 * Read basic information from a graph file.
 +
 
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 218740b1f8..d73cbc907d 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -11,7 +11,7 @@
 static char const * const builtin_commit_graph_usage[] = {
N_("git commit-graph [--pack-dir ]"),
N_("git commit-graph --read [--graph-hash=]"),
-   N_("git commit-graph --write [--pack-dir ]"),
+   N_("git commit-graph --write [--pack-dir ] [--update-head]"),
NULL
 };
 
@@ -20,6 +20,9 @@ static struct opts_commit_graph {
int read;
const char *graph_hash;
int write;
+   int update_head;
+   int has_existing;
+   struct object_id old_graph_hash;
 } opts;
 
 static int graph_read(void)
@@ -30,8 +33,8 @@ static int graph_read(void)
 
if (opts.graph_hash && strlen(opts.graph_hash) == GIT_MAX_HEXSZ)
get_oid_hex(opts.graph_hash, _hash);
-   else
-   die("no graph hash specified");
+   else if (!get_graph_head_hash(opts.pack_dir, _hash))
+   die("no graph-head exists");
 
graph_file = get_commit_graph_filename_hash(opts.pack_dir, _hash);
graph = load_commit_graph_one(graph_file, opts.pack_dir);
@@ -62,10 +65,33 @@ static int graph_read(void)
return 0;
 }
 
+static void update_head_file(const char *pack_dir, const struct object_id 
*graph_hash)
+{
+   struct strbuf head_path = STRBUF_INIT;
+   int fd;
+   struct lock_file lk = LOCK_INIT;
+
+   strbuf_addstr(_path, pack_dir);
+   strbuf_addstr(_path, "/");
+   strbuf_addstr(_path, "graph-head");
+
+   fd = hold_lock_file_for_update(, head_path.buf, LOCK_DIE_ON_ERROR);
+   strbuf_release(_path);
+
+   if (fd < 0)
+   die_errno("unable to open graph-head");
+
+   write_in_full(fd, oid_to_hex(graph_hash), GIT_MAX_HEXSZ);
+   commit_lock_file();
+}
+
 static int graph_write(void)
 {
struct object_id *graph_hash = construct_commit_graph(opts.pack_dir);
 
+   if (opts.update_head)
+   update_head_file(opts.pack_dir, graph_hash);
+
if (graph_hash)
printf("%s\n", oid_to_hex(graph_hash));
 
@@ -83,6 +109,8 @@ int cmd_commit_graph(int argc, const char **argv, const char 
*prefix)
N_("read graph file")),
    OPT_BOOL('w', "write", ,
    N_("write commit graph file")),
+   OPT_BOOL('u', &quo

[PATCH RFC 24/24] cat-file: update of docs

[Olga Telezhnaya]

Update the docs for cat-file command. Some new formatting atoms added
because of reusing ref-filter code.
We do not support cat-file atoms in general formatting logic
(there is just the support for cat-file), that is why some of the atoms
are still explained in cat-file docs.
We need to move these explanations when atoms will be supported
by other commands.

Signed-off-by: Olga Telezhnaia <olyatelezhn...@gmail.com>
Mentored-by: Christian Couder <christian.cou...@gmail.com>
Mentored by: Jeff King <p...@peff.net>
---
 Documentation/git-cat-file.txt | 13 ++---
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index f90f09b03fae5..90639ac21d0e8 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -187,17 +187,8 @@ linkgit:git-rev-parse[1].
 You can specify the information shown for each object by using a custom
 ``. The `` is copied literally to stdout for each
 object, with placeholders of the form `%(atom)` expanded, followed by a
-newline. The available atoms are:
-
-`objectname`::
-   The 40-hex object name of the object.
-
-`objecttype`::
-   The type of the object (the same as `cat-file -t` reports).
-
-`objectsize`::
-   The size, in bytes, of the object (the same as `cat-file -s`
-   reports).
+newline. The available atoms are the same as that of
+linkgit:git-for-each-ref[1], but there are some additional ones:
 
 `objectsize:disk`::
The size, in bytes, that the object takes up on disk. See the

--
https://github.com/git/git/pull/452

[PATCH 08/14] graph: implement git-graph --update-head

[Derrick Stolee]

It is possible to have multiple packed graph files in a pack directory,
but only one is important at a time. Use a 'graph_head' file to point
to the important file. Teach git-graph to write 'graph_head' upon
writing a new packed graph file.

Signed-off-by: Derrick Stolee <dsto...@microsoft.com>
---
 Documentation/git-graph.txt | 38 --
 builtin/graph.c | 38 +++---
 packed-graph.c  | 25 +
 packed-graph.h  |  1 +
 t/t5319-graph.sh| 12 ++--
 5 files changed, 107 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-graph.txt b/Documentation/git-graph.txt
index 0939c3f1be..ac20aa67a9 100644
--- a/Documentation/git-graph.txt
+++ b/Documentation/git-graph.txt
@@ -12,19 +12,53 @@ SYNOPSIS
 'git graph' --write  [--pack-dir ]
 'git graph' --read  [--pack-dir ]
 
+OPTIONS
+---
+--pack-dir::
+   Use given directory for the location of packfiles, graph-head,
+   and graph files.
+
+--read::
+   Read a graph file given by the graph-head file and output basic
+   details about the graph file. (Cannot be combined with --write.)
+
+--graph-id::
+   When used with --read, consider the graph file graph-.graph.
+
+--write::
+   Write a new graph file to the pack directory. (Cannot be combined
+   with --read.)
+
+--update-head::
+   When used with --write, update the graph-head file to point to
+   the written graph file.
+
 EXAMPLES
 
 
+* Output the OID of the graph file pointed to by /graph-head.
++
+
+$ git graph --pack-dir=
+
+
 * Write a graph file for the packed commits in your local .git folder.
 +
 
-$ git midx --write
+$ git graph --write
+
+
+* Write a graph file for the packed commits in your local .git folder,
+* and update graph-head.
++
+
+$ git graph --write --update-head
 
 
 * Read basic information from a graph file.
 +
 
-$ git midx --read --graph-id=
+$ git graph --read --graph-id=
 
 
 CONFIGURATION
diff --git a/builtin/graph.c b/builtin/graph.c
index bc66722924..0760d99f43 100644
--- a/builtin/graph.c
+++ b/builtin/graph.c
@@ -11,7 +11,7 @@
 static char const * const builtin_graph_usage[] ={
N_("git graph [--pack-dir ]"),
N_("git graph --read [--graph-id=]"),
-   N_("git graph --write [--pack-dir ]"),
+   N_("git graph --write [--pack-dir ] [--update-head]"),
NULL
 };
 
@@ -20,6 +20,9 @@ static struct opts_graph {
int read;
const char *graph_id;
int write;
+   int update_head;
+   int has_existing;
+   struct object_id old_graph_oid;
 } opts;
 
 static int graph_read(void)
@@ -30,8 +33,8 @@ static int graph_read(void)
 
if (opts.graph_id && strlen(opts.graph_id) == GIT_MAX_HEXSZ)
get_oid_hex(opts.graph_id, _oid);
-   else
-   die("no graph id specified");
+   else if (!get_graph_head_oid(opts.pack_dir, _oid))
+   die("no graph-head exists.");
 
graph_file = get_graph_filename_oid(opts.pack_dir, _oid);
graph = load_packed_graph_one(graph_file, opts.pack_dir);
@@ -62,10 +65,33 @@ static int graph_read(void)
return 0;
 }
 
+static void update_head_file(const char *pack_dir, const struct object_id 
*graph_id)
+{
+   struct strbuf head_path = STRBUF_INIT;
+   int fd;
+   struct lock_file lk = LOCK_INIT;
+
+   strbuf_addstr(_path, pack_dir);
+   strbuf_addstr(_path, "/");
+   strbuf_addstr(_path, "graph-head");
+
+   fd = hold_lock_file_for_update(, head_path.buf, LOCK_DIE_ON_ERROR);
+   strbuf_release(_path);
+
+   if (fd < 0)
+   die_errno("unable to open graph-head");
+
+   write_in_full(fd, oid_to_hex(graph_id), GIT_MAX_HEXSZ);
+   commit_lock_file();
+}
+
 static int graph_write(void)
 {
struct object_id *graph_id = construct_graph(opts.pack_dir);
 
+   if (opts.update_head)
+   update_head_file(opts.pack_dir, graph_id);
+
if (graph_id)
printf("%s\n", oid_to_hex(graph_id));
 
@@ -83,6 +109,8 @@ int cmd_graph(int argc, const char **argv, const char 
*prefix)
N_("read graph file")),
OPT_BOOL('w', "write", ,
N_("write graph file")),
+   OPT_BOOL('u', "update-head", _head,
+   N_("update graph-head to written graph file")),

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

[Nguyễn Thái Ngọc Duy]

From: Ævar Arnfjörð Bjarmason <ava...@gmail.com>

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

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

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

Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
---
 Documentation/git-update-index.txt | 16 
 1 file changed, 16 insertions(+)

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

[PATCH 0/3] nd/shared-index-fix update

[Nguyễn Thái Ngọc Duy]

This only changes the last patch to correct the test prerequisite and
a couple minor refinements. Interdiff:

diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 5494389dbb..d2a8e0312a 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -401,7 +401,7 @@ done <<\EOF
 0642 -rw-r---w-
 EOF
 
-test_expect_success POSIXPERM 'graceful handling splitting index is not 
allowed' '
+test_expect_success SANITY 'graceful handling when splitting index is not 
allowed' '
test_create_repo ro &&
(
cd ro &&
@@ -409,11 +409,11 @@ test_expect_success POSIXPERM 'graceful handling 
splitting index is not allowed'
git update-index --split-index &&
test -f .git/sharedindex.*
) &&
-   test_when_finished "chmod -R u+w ro" &&
-   chmod -R u-w ro &&
cp ro/.git/index new-index &&
+   test_when_finished "chmod u+w ro/.git" &&
+   chmod u-w ro/.git &&
GIT_INDEX_FILE="$(pwd)/new-index" git -C ro update-index --split-index 
&&
-   chmod -R u+w ro &&
+   chmod u+w ro/.git &&
rm ro/.git/sharedindex.* &&
GIT_INDEX_FILE=new-index git ls-files >actual &&
echo initial.t >expected &&

Nguyễn Thái Ngọc Duy (3):
  read-cache.c: change type of "temp" in write_shared_index()
  read-cache.c: move tempfile creation/cleanup out of write_shared_index
  read-cache: don't write index twice if we can't write shared index

 read-cache.c   | 40 ++--
 t/t1700-split-index.sh | 19 +++
 2 files changed, 41 insertions(+), 18 deletions(-)

-- 
2.16.0.47.g3d9b0fac3a

[PATCH v5 3/4] status: update short status to respect --no-ahead-behind

[Jeff Hostetler]

From: Jeff Hostetler 

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

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

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

[PATCH v4 3/4] status: update short status to respect --no-ahead-behind

[Jeff Hostetler]

From: Jeff Hostetler 

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

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

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

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

[RFC PATCH 07/18] midx: teach midx --write to update midx-head

[Derrick Stolee]

There may be multiple MIDX files in a single pack directory. The primary
file is pointed to by a pointer file "midx-head" that contains an OID.
The MIDX file to load is then given by "midx-.midx".

This head file will be especially important when the MIDX files are
extended to be incremental and we expect multiple MIDX files at any
point.

Signed-off-by: Derrick Stolee <dsto...@microsoft.com>
---
 Documentation/git-midx.txt | 19 ++-
 builtin/midx.c | 32 ++--
 midx.c | 31 +++
 midx.h |  3 +++
 t/t5318-midx.sh| 33 ++---
 5 files changed, 104 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-midx.txt b/Documentation/git-midx.txt
index 17464222c1..01f79cbba5 100644
--- a/Documentation/git-midx.txt
+++ b/Documentation/git-midx.txt
@@ -9,7 +9,7 @@ git-midx - Write and verify multi-pack-indexes (MIDX files).
 SYNOPSIS
 
 [verse]
-'git midx' --write [--pack-dir ]
+'git midx' --write  [--pack-dir ]
 
 DESCRIPTION
 ---
@@ -26,15 +26,32 @@ OPTIONS
If specified, write a new midx file to the pack directory using
the packfiles present. Outputs the hash of the result midx file.
 
+--update-head::
+   If specified with --write, update the midx-head file to point to
+   the written midx file.
+
 EXAMPLES
 
 
+* Read the midx-head file and output the OID of the head MIDX file.
++
+
+$ git midx
+
+
 * Write a MIDX file for the packfiles in your local .git folder.
 +
 
 $ git midx --write
 
 
+* Write a MIDX file for the packfiles in your local .git folder and
+* update the midx-head file.
++
+----
+$ git midx --write --update-head
+
+
 * Write a MIDX file for the packfiles in a different folder
 +
 -
diff --git a/builtin/midx.c b/builtin/midx.c
index 4aae14cf8e..84ce6588a2 100644
--- a/builtin/midx.c
+++ b/builtin/midx.c
@@ -9,13 +9,17 @@
 #include "midx.h"
 
 static char const * const builtin_midx_usage[] = {
-   N_("git midx --write [--pack-dir ]"),
+   N_("git midx [--pack-dir ]"),
+   N_("git midx --write [--update-head] [--pack-dir ]"),
NULL
 };
 
 static struct opts_midx {
const char *pack_dir;
int write;
+   int update_head;
+   int has_existing;
+   struct object_id old_midx_oid;
 } opts;
 
 static int build_midx_from_packs(
@@ -109,6 +113,22 @@ static int build_midx_from_packs(
return 0;
 }
 
+static void update_head_file(const char *pack_dir, const char *midx_id)
+{
+   int fd;
+   struct lock_file lk = LOCK_INIT;
+   char *head_path = get_midx_head_filename(pack_dir);
+
+   fd = hold_lock_file_for_update(, head_path, LOCK_DIE_ON_ERROR);
+   FREE_AND_NULL(head_path);
+
+   if (fd < 0)
+   die_errno("unable to open midx-head");
+
+   write_in_full(fd, midx_id, GIT_MAX_HEXSZ);
+   commit_lock_file();
+}
+
 static int midx_write(void)
 {
const char **pack_names = NULL;
@@ -152,6 +172,9 @@ static int midx_write(void)
 
printf("%s\n", midx_id);
 
+   if (opts.update_head)
+   update_head_file(opts.pack_dir, midx_id);
+
 cleanup:
if (pack_names)
FREE_AND_NULL(pack_names);
@@ -166,6 +189,8 @@ int cmd_midx(int argc, const char **argv, const char 
*prefix)
N_("The pack directory containing set of packfile and 
pack-index pairs.") },
OPT_BOOL('w', "write", ,
    N_("write midx file")),
+   OPT_BOOL('u', "update-head", _head,
+   N_("update midx-head to written midx file")),
OPT_END(),
};
 
@@ -187,9 +212,12 @@ int cmd_midx(int argc, const char **argv, const char 
*prefix)
opts.pack_dir = strbuf_detach(, NULL);
}
 
+   opts.has_existing = !!get_midx_head_oid(opts.pack_dir, 
_midx_oid);
+
if (opts.write)
return midx_write();
 
-   usage_with_options(builtin_midx_usage, builtin_midx_options);
+   if (opts.has_existing)
+   printf("%s\n", oid_to_hex(_midx_oid));
return 0;
 }
diff --git a/midx.c b/midx.c
index 5c320726ed..f4178c1b81 100644
--- a/midx.c
+++ b/midx.c
@@ -34,6 +34,37 @@ char* get_midx_filename_oid(const char *pack_dir,
return strbuf_detach(_path, NULL);
 }
 
+char *get_midx_head_filename(const char *pack_dir)
+{
+   struct strbuf head_filename = 

Documentation for update-index

[Rasmus Villemoes]

The man page for update-index says

   -q
   Quiet. If --refresh finds that the index needs an update,
the default behavior is to error out. This
   option makes git update-index continue anyway.

   --ignore-submodules
   Do not try to update submodules. This option is only
respected when passed before --refresh.


However, it seems that the "This option is only respected when passed
before --refresh." also applies to -q (and --unmerged); at least I get
different results from

  git update-index -q --refresh
  git update-index --refresh -q

>From the documentation, that doesn't seem to be intentional, but the
code in update-index.c seems to handle -q, --ignore-submodules,
--ignore-missing and --unmerged the same way.

Rasmus

[PATCHv4 3/4] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller <sbel...@google.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..96c3327f19 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
ie_match_stat(o->src_index, old, , 
CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
    update |= CE_UPDATE;
}
+       if (o->update && S_ISGITLINK(old->ce_mode) &&
+   should_update_submodules() && !verify_uptodate(old, o))
+   update |= CE_UPDATE;
add_entry(o, old, update, 0);
return 0;
}
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: Recommendation for consistent update on invoke of "sha1_to_hex()"

[Jeff King]

On Fri, Jan 05, 2018 at 11:26:01AM +0800, 牛旭 wrote:

> By mining historical patches, we suggest that invokes of sha1_to_hex()
> should be replaced with that of oid_to_hex(). One example for
> recommendation and corresponding patch are listed as follows. 

Note that these two functions take different types. If you only have a
"const unsigned char *", then you must use sha1_to_hex(). If you have a
"struct object_id", then you should be using oid_to_hex(). If there are
sites which do:

  sha1_to_hex(oid.hash)

those should be converted to use oid_to_hex(). I think there's a
coccinelle rule for this, though, so there shouldn't be any lingering
calls like that.

Of course the ultimate goal is for every function to use oid_to_hex().
But that's much bigger than a single-line change, since groups of
dependent functions need to be converted (try "git log
--author=carlson" to see example patches).

> One example of missed spot:
> 1  void assert_sha1_type(const unsigned char *sha1, enum 
>   object_type expect)
> 2 {
> 3  enum object_type type = sha1_object_info(sha1, NULL);
> 4  if (type < 0)
> 5   die("%s is not a valid object",sha1_to_hex(sha1));
> 6  if (type != expect)
> 7   die("%s is not a valid '%s' object", sha1_to_hex(sha1),
> 8  typename(expect));
> 9 }

So this is an example that doesn't convert easily. The function has only
the bare pointer, so you'd have to change its parameter list (and
therefore all of its callers).

-Peff

Recommendation for consistent update on invoke of "sha1_to_hex()"

[牛旭]

Our team researches on consistent update of Git during evolution. And we have 
figured out several spots that may be missed. 


By mining historical patches, we suggest that invokes of sha1_to_hex() should 
be replaced with that of oid_to_hex(). One example for recommendation and 
corresponding patch are listed as follows. 

One example of missed spot:
1  void assert_sha1_type(const unsigned char *sha1, enum 
  object_type expect)
2 {
3  enum object_type type = sha1_object_info(sha1, NULL);
4  if (type < 0)
5   die("%s is not a valid object",sha1_to_hex(sha1));
6  if (type != expect)
7   die("%s is not a valid '%s' object", sha1_to_hex(sha1),
8  typename(expect));
9 }

One example of historical patch:
1  * We do not want to call parse_object here, because
2  * inflating blobs and trees could be very expensive.
3  * However, we do need to know the correct type for
4  * later processing, and the revision machinery expects
5  * commits and tags to have been parsed.
6  */
7  - type = sha1_object_info(sha1, NULL);
8  + type = sha1_object_info(oid->hash, NULL);
9  if (type < 0)
10 - die("unable to get object info for %s", sha1_to_hex(sha1));
11 + die("unable to get object info for %s", oid_to_hex(oid));
12
More recommendations and supporting patches are saved in attachments. It is so 
kind of you to reply me about the correctness of our suggestions. And thank you 
for your reading. 
<>
<>

Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later

[Johannes Schindelin]

Hi Alex,

On Tue, 2 Jan 2018, Alex Vandiver wrote:

> diff --git a/config.c b/config.c
> index e617c2018..7c6ed888e 100644
> --- a/config.c
> +++ b/config.c
> @@ -2174,8 +2174,13 @@ int git_config_get_fsmonitor(void)
>   if (core_fsmonitor && !*core_fsmonitor)
>   core_fsmonitor = NULL;
>  
> - if (core_fsmonitor)
> - return 1;
> +
> + if (core_fsmonitor) {
> + if (!strcasecmp(core_fsmonitor, "keep"))
> + return -1;
> + else
> + return 1;
> + }

It took me a while to reason about this:

- there is no existing code path that can return -1 from
  git_config_get_fsmonitor(),

- the callers in builtin/update-index.c (testing explicitly for 0 and 1)
  do not matter because they only trigger warnings.

- the remaining two callers are in fsmonitor.c:

  - tweak_fsmonitor() (which handles -1 specifically), and

  - inflate_fsmonitor_ewah(), which only tests whether
git_config_get_fsmonitor() returned a non-zero value, but that test is
inside a code block that is only triggered if the index has an
fsmonitor_dirty array, meaning: it already had fsmonitor enabled.
Therefore the test is legitimate.

This would take the next reader as much time, I would wager a bet. So
maybe you can include this information (or at least the information about
inflate_fsmonitor_ewah()) in the commit message?

> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
> index ad452707e..48c4bab0b 100644
> --- a/t/helper/test-dump-fsmonitor.c
> +++ b/t/helper/test-dump-fsmonitor.c
> @@ -1,12 +1,14 @@
>  #include "cache.h"
> +#include "config.h"
>  
>  int cmd_main(int ac, const char **av)
>  {
>   struct index_state *istate = _index;
>   int i;
>  
> + git_config_push_parameter("core.fsmonitor=keep");

The alternative would be to use an environment variable. We already use
GIT_FSMONITOR_TEST.

However, I wonder why we need this. Do we really update the index anywhere
in the tests, then *toggle* the core.fsmonitor setting, and *then* call
test-dump-fsmonitor?

And if we do, can't we simply avoid it?

Ciao,
Johannes

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

[Jeff Hostetler]

From: Jeff Hostetler 

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

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

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

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

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

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

[PATCH 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller <sbel...@google.com>
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..7657d6ecdd 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
ie_match_stat(o->src_index, old, , 
CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
        update |= CE_UPDATE;
}
+   if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
+   o->update && !verify_uptodate(old, o))
+   update |= CE_UPDATE;
    add_entry(o, old, update, 0);
return 0;
}
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

On Tue, Jan 2, 2018 at 11:43 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@google.com> writes:
>
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index bf8b602901..ac59524a7f 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>>   ie_match_stat(o->src_index, old, , 
>> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>>   update |= CE_UPDATE;
>>   }
>> + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
>> + !verify_uptodate(old, o))
>> + update |= CE_UPDATE;
>>   add_entry(o, old, update, 0);
>>   return 0;
>>   }
>
> This is more sensible than the previous one that broke same() by
> unconditionally checking the working tree state, even when the
> machinery is told not to look at the working tree.
>
> The code in the pre-context of this hunk, (i.e. the original one you
> are half-way mimicking), we realize that the original cache entry
> and the cache entry we are checking out are exactly the same and we
> overwrite when we know that the path in the working tree is dirty,
> and we are not told to "skip" that path in the working tree
> (i.e. sparse checkout).  That happens only when we are told to
> o->update and o->reset.
>
> Shouldn't we be setting the update bit only when o->update is in
> effect,

Yes, we should.

> just like we can see in the pre-context of this hunk?  I do
> not know if we need to require o->reset and/or need to pay attention
> to the skip worktree bit in this new case.

verify_uptodate takes care of the worktree bits
("o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE))")

o->reset is taken care of inside the nested verify_uptodate_1 function via

...
    else if (o->reset || ce_uptodate(ce))
return 0;

So I'd think we only need to toss in the o->update check.

And that additional check would only be a performance optimization
as it would omit the check in case we do not want to update.
(verify_uptodate is expensive for submodules)

Re: [PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Junio C Hamano]

Stefan Beller <sbel...@google.com> writes:

> diff --git a/unpack-trees.c b/unpack-trees.c
> index bf8b602901..ac59524a7f 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
>   ie_match_stat(o->src_index, old, , 
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
>   update |= CE_UPDATE;
>   }
> + if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
> +     !verify_uptodate(old, o))
> +     update |= CE_UPDATE;
>   add_entry(o, old, update, 0);
>   return 0;
>   }

This is more sensible than the previous one that broke same() by
unconditionally checking the working tree state, even when the
machinery is told not to look at the working tree.

The code in the pre-context of this hunk, (i.e. the original one you
are half-way mimicking), we realize that the original cache entry
and the cache entry we are checking out are exactly the same and we
overwrite when we know that the path in the working tree is dirty,
and we are not told to "skip" that path in the working tree
(i.e. sparse checkout).  That happens only when we are told to
o->update and o->reset.

Shouldn't we be setting the update bit only when o->update is in
effect, just like we can see in the pre-context of this hunk?  I do
not know if we need to require o->reset and/or need to pay attention
to the skip worktree bit in this new case.

[PATCHv2 4/5] unpack-trees: oneway_merge to update submodules

[Stefan Beller]

When there is a one way merge, each submodule needs to be one way merged
as well, if we're asked to recurse into submodules.

In case of a submodule, check if it is up-to-date, otherwise set the
flag CE_UPDATE, which will trigger an update of it in the phase updating
the tree later.

Signed-off-by: Stefan Beller <sbel...@google.com>
---
 unpack-trees.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/unpack-trees.c b/unpack-trees.c
index bf8b602901..ac59524a7f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2139,6 +2139,9 @@ int oneway_merge(const struct cache_entry * const *src,
ie_match_stat(o->src_index, old, , 
CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))
        update |= CE_UPDATE;
}
+   if (S_ISGITLINK(old->ce_mode) && should_update_submodules() &&
+   !verify_uptodate(old, o))
+   update |= CE_UPDATE;
    add_entry(o, old, update, 0);
return 0;
}
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH 3/1] update-index doc: note a fixed bug in the untracked cache

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <ava...@gmail.com> writes:

> Document the bug tested for in my "status: add a failing test showing
> a core.untrackedCache bug" and fixed in Duy's "dir.c: fix missing dir
> invalidation in untracked code".
>
> Since this is very likely something others will encounter in the
> future on older versions, and it's not obvious how to fix it let's
> document both that it exists, and how to "fix" it with a one-off
> command.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
> ---
>
>> On Tue, Dec 26, 2017 at 05:47:19PM +0700, Duy Nguyen wrote:
>>> Strangely, root dir is not cached (no valid flag). I don't know why
>>> but that's ok we'll roll with it.
>>
>> I figured this out. Which is good because now I know how/why the bug
>> happens.
>
> Thanks a lot. I think it's probably good to apply something like this
> on top of this now 3-patch series.

Thanks both for working well together.  Now that the problem turns
out not about symbolic links, can we update the test in 1/1 not to
depend on symbolic links?

>
>  Documentation/git-update-index.txt | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/git-update-index.txt 
> b/Documentation/git-update-index.txt
> index bdb0342593..bc6c32002f 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -464,6 +464,17 @@ command reads the index; while when 
> `--[no-|force-]untracked-cache`
>  are used, the untracked cache is immediately added to or removed from
>  the index.
>  
> +Before 2.16, the untracked cache had a bug where replacing a directory
> +with a symlink to another directory could cause it to incorrectly show
> +files tracked by git as untracked. See the "status: add a failing test
> +showing a core.untrackedCache bug" commit to git.git. A workaround for
> +that was (and this might work for other undiscoverd bugs in the
> +future):
> +
> +
> +$ git -c core.untrackedCache=false status
> +
> +
>  File System Monitor
>  ---

[PATCH 3/1] update-index doc: note a fixed bug in the untracked cache

[Ævar Arnfjörð Bjarmason]

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

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

Signed-off-by: Ævar Arnfjörð Bjarmason <ava...@gmail.com>
---

> On Tue, Dec 26, 2017 at 05:47:19PM +0700, Duy Nguyen wrote:
>> Strangely, root dir is not cached (no valid flag). I don't know why
>> but that's ok we'll roll with it.
>
> I figured this out. Which is good because now I know how/why the bug
> happens.

Thanks a lot. I think it's probably good to apply something like this
on top of this now 3-patch series.

 Documentation/git-update-index.txt | 11 +++
 1 file changed, 11 insertions(+)

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

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Stefan Beller]

On Thu, Dec 21, 2017 at 2:17 PM, Andreas Urke  wrote:
> Thanks for the detailed explanation. Although I am not too git savvy,
> I believe I got the gist of it.
>
> Regarding your question,
> I would say the term "name" in an IT context makes me primarily think
> of something that is specified by a user (as opposed to e.g. an "id"),

ID sounds great.

> and can be altered by a user. Also, the manual mention it as a
> "logical name", which would further strengthen my belief that it is an
> abstraction which can be changed (as opposed to something "physical").
> I would be much more reluctant to change the id of something than its
> name.

If you changed the name in all places, it would have been no problem. ;)
So once you did the "git config -f .gitmodules --rename-section ...",
you'd also have to "mv .git/modules/ .git/modules/"
and then it would work fine for you. (You would also need to instruct
any collaborators to run the move command when they pull your changes.
So it is more of a "in theory it works", rather than practical advice here)

> In terms of the commands...In an ideal world I would obviously ask for
> a --rename or mv command which would achieve what I wanted to do.
> Other than that, maybe a word about this in the man for "git mv"? Or

git-mv moves the path, not the name of the submodule, though.

> perhaps "git submodule sync" could give me some kind of warning that I
> did something strange/illegal.

That is harder than thought, because it is normal that a submodule
worktree doesn't exist at the given path (not initialized).
Also it is totally valid to have outdated entries in the .gitmodules file. :/

> Can I ask you, now that I have made this mess, do you have any
> suggestion on how to rectify it or some other way to achieve my goal?

Looking at the script you had above, I would think you need to init
the renamed submodule again, not sure if anything else needs
to be done, as now you'd have essentially two repositories inside
the superprojects .git/modules/ for just one logical submodule.

So I would assume if you were to re-clone the superproject
everything works out fine again (except for bisection/checkout
before that rename. ;) But for that you can symlink the
oldname to the newname inside the .git/modules/ of the superproject
I would assume.

Stefan

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Stefan Beller]

On Thu, Dec 21, 2017 at 2:08 PM, Junio C Hamano  wrote:
> That sounds like a bit of revisionist history, but you weren't around
> back then, so...
>
> https://public-inbox.org/git/11793556371774-git-send-email-jun...@cox.net/#r
>
> is my summarization of discussions before that time. There is a
> mention of "three-level"
> thing by Steven Grimm in
> https://public-inbox.org/git/464e4c94.5070...@midwinter.com/
> in the thread, and together with messages like
> https://public-inbox.org/git/7vejle6p96@assigned-by-dhcp.cox.net/
> you can see that we already knew that submodule identity (name), path
> in the superproject (path) and where it
> comes from (url) need to be separate things that need to be tied
> together by .gitmodules.

Taking this private reply back to the list, I hope you don't mind.
I think linking into the archives (hence making the huge unstructured
archive a bit more discoverable by these links) is a good idea.

Yes, I was not around when these discussions happened, hence I came
up with a narrative that I can rationalize best.

This message https://public-inbox.org/git/464cf435.1010...@midwinter.com/
helps (me) most as that states the problem that need to be solved and I agree
with the issues and how to solve it. The "symbolic names", however, are the
crux there. They must not be changed, so as Andreas says, maybe "ID" is
a better notion than "submodule name".

I am of the opinion that we'd even want to go as far as to not expose
this symbolic to the user.

So if we redesign from scratch, I would not have "gitlink entries"
but rather "special blobs"[1], that contain the submodule sha1,
as well as their ID.

A special blob could look like:

  git cat-file "kernel/"
  version 
  id 

Additionally you'd have slight guidance of id -> URL in
an extra ref, versioned independently of the main history:

   -> git://kernel.org/...

(A)
So if linux moves away from kernel.org, you can change that ref
independently of your history at that time, such that even old
versions of your main history can obtain the kernel from the correct
URL. Of course a porcelain command could do the double lookup
such that the user can say "kernel/ -> newURL" without needing
to have knowledge of the internal ID.

(B)
Now if you want to move the submodule locally, you can do so by
having that special blob at a different path in your tree.
git-mv "just works" even for nested submodules. No need to
rewrite the .gitmodules file.

(C)
What if you want to drop linux and use some BSD?
Then you change the ID such that a new submodule repo
is created inside .git/modules/. Of course it is also pinned
to a different sha1. A porcelain command such as
"git submodule replace" would sound like a good porcelain-ish.

I think this so far is a clean design, but it is incompatible with history.

The really big difference is that the ID is the core thing that everything
revolves around (similar to the submodule name), but is tied to the path
and is hidden as much as possible from the user, who can use given
commands for each of the scenarios.

The big flaw of this that the ID is sort of random and not based
on content, which is counter to Gits philosophy as a content
addressable FS. Maybe the ID can be set to
HASH( +), such that when two people
independently of each other make a submodule pointing at the
same commit at the same path, they'd have the same tree-id.

Thanks,
Stefan

[1] An in office discussion hinted at that "special blobs" are not
that special. git-LFS uses this mechanism, but of course their
design flaw is to keep it out of main-Git. But one could posit
"submodules can be implemented using smudge filters alone".

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Andreas Urke]

Thanks for the detailed explanation. Although I am not too git savvy,
I believe I got the gist of it.

Regarding your question,
I would say the term "name" in an IT context makes me primarily think
of something that is specified by a user (as opposed to e.g. an "id"),
and can be altered by a user. Also, the manual mention it as a
"logical name", which would further strengthen my belief that it is an
abstraction which can be changed (as opposed to something "physical").
I would be much more reluctant to change the id of something than its
name.

In terms of the commands...In an ideal world I would obviously ask for
a --rename or mv command which would achieve what I wanted to do.
Other than that, maybe a word about this in the man for "git mv"? Or
perhaps "git submodule sync" could give me some kind of warning that I
did something strange/illegal.

Can I ask you, now that I have made this mess, do you have any
suggestion on how to rectify it or some other way to achieve my goal?
The only side-effect I have seen is this update problem (been running
this for a few months), but after your explanations I would like to
get back to solid ground. My use-case is that I need to follow a
specific folder-naming (i.e. subrepo path) convention, but I do not
want to use that naming as the repo name in our gitlab.

Regards,
Andreas

On 21 December 2017 at 19:55, Stefan Beller <sbel...@google.com> wrote:
> On Thu, Dec 21, 2017 at 8:21 AM, Andreas Urke <aru...@gmail.com> wrote:
>> If I am entering into undefined behavior territory with the renaming
>> then there might not be any point to pursue this further, but in any
>> case, script below:
>>
>> Apologies up-front for the verbosity, there are probably a lot of
>> unnecessary steps and git shortcuts missed - I just wanted it to
>> exactly match my scenario. Note that it is divided into two parts as
>> it requires you to edit .gitmodules.
>>
>> Part 1:
>>
>> cd ~/
>> # Make sub repos and add a commit to each
>> mkdir super && cd super
>> mkdir sub1 && mkdir sub2
>> cd sub1
>> git init && touch first && git add . && git commit -m "first"
>> cd ../sub2
>> git init && touch first && git add . && git commit -m "first"
>> cd ..
>>
>> # Make super repo, add subrepos, and commit
>> git init
>> git submodule add ./sub1
>> git submodule add ./sub2
>> git add .
>> git commit -m "first"
>>
>> # Edit .gitmodules, change sub2 name and path to sub2_newname:
>> # $ cat .gitmodules
>> # [submodule "sub1"]
>> # path = sub1
>> # url = ./sub1
>> # [submodule "sub2_newname"]
>> # path = sub2
>> # url = ./sub2_newname
>>
>
> A couple of things here:
> (a) In a script you could do this via
> git config -f .gitmodules --rename-section  
> (b) This is not just undefined, but rather Git explicitly assumes
> the user does not rename the section themselves.
>
> The reason for this is found in gits history:
> 1. submodules were invented
> 2. submodule configuration such as where to obtain
> the submodule from needs some place, and at the time
> submodule..url inside the .gitmodules file seemed
> like a good idea.
> 3. "What if we want git-mv, git-rm support for submodules?"
> Or for example when bisecting and jumping between revisions
> where the submodule exists or does not exists. To solve
> this problem, the concept of a "submodule name" was invented,
> such that the submodules git dir can be put into the
> superprojects .git/modules/ dir, and the working tree
> only needs to have a ".git" file pointing at that gitdir. The the
> working tree of the submodule can be removed while keeping
> the submodules git dir (with potentially important information
> such as local-only commits) around! Success!
>
> The transition path is also easy:
> These newly "named" submodules have an additional entry of
> 'submodule..path ='. Note how the subsection
> part of the config changed its meaning, but that is ok, as the
> name is allowed to be equal to the  value, and keeps
> all config related to one submodule in one section. Easy but
> confusing as now we have to adhere to a couple assumptions:
> The  value must match where it is in the working tree,
> the  however must not be changed because that is
> where we'll find its repository inside the superproject.
> And confusingly these two keys have often the same value,
> for this historic reason.
>
&

[PATCH v2 4/5] status: update short status to use --no-ahead-behind

[Jeff Hostetler]

From: Jeff Hostetler 

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

Signed-off-by: Jeff Hostetler 
---
 t/t6040-tracking-info.sh | 13 +
 wt-status.c  |  9 ++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 8f17fd9..0190220 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -147,6 +147,19 @@ test_expect_success 'status -s -b (diverged from 
upstream)' '
 '
 
 cat >expect <<\EOF
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status -s -b --no-ahead-behind | head -1
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index d03b47a..3235ec2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1765,7 +1765,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
const char *base;
char *short_base;
const char *branch_name;
-   int num_ours, num_theirs;
+   int num_ours, num_theirs, sti;
int upstream_is_gone = 0;
 
color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## ");
@@ -1791,7 +1791,8 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
 
color_fprintf(s->fp, branch_color_local, "%s", branch_name);
 
-   if (stat_tracking_info(branch, _ours, _theirs, , ABF_FULL) 
< 0) {
+   sti = stat_tracking_info(branch, _ours, _theirs, , 
s->ab_flags);
+   if (sti < 0) {
if (!base)
goto conclude;
 
@@ -1803,12 +1804,14 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
color_fprintf(s->fp, branch_color_remote, "%s", short_base);
free(short_base);
 
-   if (!upstream_is_gone && !num_ours && !num_theirs)
+   if (!upstream_is_gone && !sti)
goto conclude;
 
color_fprintf(s->fp, header_color, " [");
if (upstream_is_gone) {
color_fprintf(s->fp, header_color, LABEL(N_("gone")));
+   } else if (s->ab_flags == ABF_QUICK) {
+   color_fprintf(s->fp, header_color, LABEL(N_("different")));
} else if (!num_ours) {
color_fprintf(s->fp, header_color, LABEL(N_("behind ")));
color_fprintf(s->fp, branch_color_remote, "%d", num_theirs);
-- 
2.9.3

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Stefan Beller]

On Thu, Dec 21, 2017 at 8:21 AM, Andreas Urke  wrote:
> If I am entering into undefined behavior territory with the renaming
> then there might not be any point to pursue this further, but in any
> case, script below:
>
> Apologies up-front for the verbosity, there are probably a lot of
> unnecessary steps and git shortcuts missed - I just wanted it to
> exactly match my scenario. Note that it is divided into two parts as
> it requires you to edit .gitmodules.
>
> Part 1:
>
> cd ~/
> # Make sub repos and add a commit to each
> mkdir super && cd super
> mkdir sub1 && mkdir sub2
> cd sub1
> git init && touch first && git add . && git commit -m "first"
> cd ../sub2
> git init && touch first && git add . && git commit -m "first"
> cd ..
>
> # Make super repo, add subrepos, and commit
> git init
> git submodule add ./sub1
> git submodule add ./sub2
> git add .
> git commit -m "first"
>
> # Edit .gitmodules, change sub2 name and path to sub2_newname:
> # $ cat .gitmodules
> # [submodule "sub1"]
> # path = sub1
> # url = ./sub1
> # [submodule "sub2_newname"]
> # path = sub2
> # url = ./sub2_newname
>

A couple of things here:
(a) In a script you could do this via
git config -f .gitmodules --rename-section  
(b) This is not just undefined, but rather Git explicitly assumes
the user does not rename the section themselves.

The reason for this is found in gits history:
1. submodules were invented
2. submodule configuration such as where to obtain
the submodule from needs some place, and at the time
submodule..url inside the .gitmodules file seemed
like a good idea.
3. "What if we want git-mv, git-rm support for submodules?"
Or for example when bisecting and jumping between revisions
where the submodule exists or does not exists. To solve
this problem, the concept of a "submodule name" was invented,
such that the submodules git dir can be put into the
superprojects .git/modules/ dir, and the working tree
only needs to have a ".git" file pointing at that gitdir. The the
working tree of the submodule can be removed while keeping
the submodules git dir (with potentially important information
such as local-only commits) around! Success!

The transition path is also easy:
These newly "named" submodules have an additional entry of
'submodule..path ='. Note how the subsection
part of the config changed its meaning, but that is ok, as the
name is allowed to be equal to the  value, and keeps
all config related to one submodule in one section. Easy but
confusing as now we have to adhere to a couple assumptions:
The  value must match where it is in the working tree,
the  however must not be changed because that is
where we'll find its repository inside the superproject.
And confusingly these two keys have often the same value,
for this historic reason.

4. (rather lately): We need to fix the submodule mess.
While in (3) we were unclear about name/path issues,
and tried to be super backwards compatible, now we'd
rather want the submodules to be well-defined. So we'll
try to write some docs, such as gitsubmodules [1] in addition
to the usage manual of "git submodule"[2]. Of course there is
still the description of the .gitmodules file[3].

[1] https://www.kernel.org/pub/software/scm/git/docs/gitsubmodules.html
[2] https://www.kernel.org/pub/software/scm/git/docs/git-submodule.html
[3] https://www.kernel.org/pub/software/scm/git/docs/gitmodules.html

I guess in [1] we'd want to add a discussion on what the  and
what the  is for and why one can change but not the other.

I think you ran into trouble here because you and Git had
differing assumptions on some part of the submodule model.
Is there any part of the documentation, configuration or some
commands that would have helped explaining some of these things?
(i.e. What do we best patch to help others from running into the
same issue?)

Thanks,
Stefan

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff Hostetler]

On 12/21/2017 10:39 AM, Jeff King wrote:

On Thu, Dec 21, 2017 at 09:18:17AM -0500, Jeff Hostetler wrote:


This patch will affect "git status --porcelain", too. That's not
supposed to change in incompatible ways. I guess it's up for debate
whether callers are meant to handle any arbitrary string inside the []
(we already show "[gone]" for some cases), since AFAICT the format of
the tracking info is left completely vague in the documentation.

(I'd also hope that everybody is using --porcelain=v2 if they can, but
we should still avoid breaking v1).


I hadn't intended to alter V1 output.  I'll disable the new feature
when V1 is selected.


To be clear, I am on the fence regarding the "is it a breaking change"
line. Certainly if the caller says "--no-ahead-behind", I don't see any
harm in doing what they asked.

But one further complication is that this may be triggered by config.
And that goes for --porcelain=v2, as well. Even though the v2
documentation specifically says "ignore headers you don't recognize",
would any callers be confused if a header is omitted due to a user's
config?

I guess for "branch.ab", the answer is "probably not", since it is
already documented to appear only if certain conditions are met. So
probably "omit branch.ab" is an OK change, as is "add a new header".
But I just wonder if it would be simpler to ignore the config entirely
for porcelain outputs (and require the explicit command-line option).


I like that actually.  Have the porcelain versions only use the
command line option and do what is asked.  And let the short/long
forms use both the command line option and the config setting.
That makes it explicit for scripts that parse the output.

Thanks!



Personally, I am not a purist when it comes to config and plumbing, and
I'd be fine with having the config impact v2 (it is a hint from the user
that they do not want to spend time on the computation, so having
scripts respect that would be what the user wants). If you really have a
script that is unhappy missing "branch.ab", then you can either choose
not to set that config, or you can fix the script to use "--ahead-behind"
to override the config. But I'm not sure everybody in the community
would necessarily agree with me.

-Peff

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Andreas Urke]

If I am entering into undefined behavior territory with the renaming
then there might not be any point to pursue this further, but in any
case, script below:

Apologies up-front for the verbosity, there are probably a lot of
unnecessary steps and git shortcuts missed - I just wanted it to
exactly match my scenario. Note that it is divided into two parts as
it requires you to edit .gitmodules.

Part 1:

cd ~/
# Make sub repos and add a commit to each
mkdir super && cd super
mkdir sub1 && mkdir sub2
cd sub1
git init && touch first && git add . && git commit -m "first"
cd ../sub2
git init && touch first && git add . && git commit -m "first"
cd ..

# Make super repo, add subrepos, and commit
git init
git submodule add ./sub1
git submodule add ./sub2
git add .
git commit -m "first"

# Edit .gitmodules, change sub2 name and path to sub2_newname:
# $ cat .gitmodules
# [submodule "sub1"]
# path = sub1
# url = ./sub1
# [submodule "sub2_newname"]
# path = sub2
# url = ./sub2_newname

Part 2:

cd ~/
# Sync submodule after we edited .gitmodules
cd super
git submodule sync

# Commit in subrepos and commit this to superrepo
cd sub1
touch second && git add . && git commit -m "second"
cd ../sub2
touch second && git add . && git commit -m "second"
cd ..
git add .
git commit -m "second"

# Commit in subrepos and commit this to superrepo
cd sub1
touch third && git add . && git commit -m "third"
cd ../sub2
touch third && git add . && git commit -m "third"
cd ..
git add .
git commit -m "third"

git checkout HEAD^
git status
git submodule update
git status # Observe only sub1 repo was updated

On 20 December 2017 at 18:54, Stefan Beller <sbel...@google.com> wrote:
> On Wed, Dec 20, 2017 at 12:22 AM, Andreas Urke <aru...@gmail.com> wrote:
>> Thanks for looking into this.
>>
>> I was able to reproduce it from scratch, but I followed my earlier
>> workflow where I first created the subrepos, and then later renamed
>> it. At the time I was not able to find any command to rename without
>> changing the path (and I was not able find one now either, is there
>> any?), so I edited name and path in .gitmodules and ran git submodule
>> sync. Am I asking for trouble doing it that way?
>
> "rename without changing the path" sounds like a red flag to me,
> as the submodule name was introduced specifically to be a constant
> as the path may change, whereas the name ought to never change.
>
>
>
>
>>
>> Let me know if you need the exact steps I followed.
>
> Well yes, ideally as a shell script (or even embedded into our test suite).

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff King]

On Thu, Dec 21, 2017 at 09:18:17AM -0500, Jeff Hostetler wrote:

> > This patch will affect "git status --porcelain", too. That's not
> > supposed to change in incompatible ways. I guess it's up for debate
> > whether callers are meant to handle any arbitrary string inside the []
> > (we already show "[gone]" for some cases), since AFAICT the format of
> > the tracking info is left completely vague in the documentation.
> > 
> > (I'd also hope that everybody is using --porcelain=v2 if they can, but
> > we should still avoid breaking v1).
> 
> I hadn't intended to alter V1 output.  I'll disable the new feature
> when V1 is selected.

To be clear, I am on the fence regarding the "is it a breaking change"
line. Certainly if the caller says "--no-ahead-behind", I don't see any
harm in doing what they asked.

But one further complication is that this may be triggered by config.
And that goes for --porcelain=v2, as well. Even though the v2
documentation specifically says "ignore headers you don't recognize",
would any callers be confused if a header is omitted due to a user's
config?

I guess for "branch.ab", the answer is "probably not", since it is
already documented to appear only if certain conditions are met. So
probably "omit branch.ab" is an OK change, as is "add a new header".
But I just wonder if it would be simpler to ignore the config entirely
for porcelain outputs (and require the explicit command-line option).

Personally, I am not a purist when it comes to config and plumbing, and
I'd be fine with having the config impact v2 (it is a hint from the user
that they do not want to spend time on the computation, so having
scripts respect that would be what the user wants). If you really have a
script that is unhappy missing "branch.ab", then you can either choose
not to set that config, or you can fix the script to use "--ahead-behind"
to override the config. But I'm not sure everybody in the community
would necessarily agree with me.

-Peff

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff Hostetler]

On 12/20/2017 11:26 AM, Jeff King wrote:

On Wed, Dec 20, 2017 at 02:42:44PM +, Jeff Hostetler wrote:


From: Jeff Hostetler 

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


How come "--short" and "--long" get this smaller bit of data, but
"--porcelain=v2" just omits the line entirely?

I don't have a real preference for or against the "[different]" message
myself, but if we can get the information cheaply, it seems odd not to
provide it in all cases.


I was only thinking of VS usage.  But you're right, I can include an
alternate line with eq|neq since we already have the data on hand.


[...]

+test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '


This patch will affect "git status --porcelain", too. That's not
supposed to change in incompatible ways. I guess it's up for debate
whether callers are meant to handle any arbitrary string inside the []
(we already show "[gone]" for some cases), since AFAICT the format of
the tracking info is left completely vague in the documentation.

(I'd also hope that everybody is using --porcelain=v2 if they can, but
we should still avoid breaking v1).


I hadn't intended to alter V1 output.  I'll disable the new feature
when V1 is selected.

Thanks
Jeff

[PATCH 1/1] git-p4: update multiple shelved change lists

[Luke Diamand]

--update-shelve can now be specified multiple times on the
command-line, to update multiple shelved changelists in a single
submit.

This then means that a git patch series can be mirrored to a
sequence of shelved changelists, and (relatively easily) kept in
sync as changes are made in git.

Note that Perforce does not really support overlapping shelved
changelists where one change touches the files modified by
another. Trying to do this will result in merge conflicts.

Signed-off-by: Luke Diamand <l...@diamand.org>
---
 Documentation/git-p4.txt |  8 +++-
 git-p4.py| 41 ++---
 t/t9807-git-p4-submit.sh | 24 ++--
 3 files changed, 47 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index 7436c64a9..d8c8f11c9 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -157,6 +157,12 @@ The p4 changes will be created as the user invoking 'git 
p4 submit'. The
 according to the author of the Git commit.  This option requires admin
 privileges in p4, which can be granted using 'p4 protect'.
 
+To shelve changes instead of submitting, use `--shelve` and `--update-shelve`:
+
+
+$ git p4 submit --shelve
+$ git p4 submit --update-shelve 1234 --update-shelve 2345
+
 
 OPTIONS
 ---
@@ -310,7 +316,7 @@ These options can be used to modify 'git p4 submit' 
behavior.
 
 --update-shelve CHANGELIST::
    Update an existing shelved changelist with this commit. Implies
-   --shelve.
+   --shelve. Repeat for multiple shelved changelists.
 
 --conflict=(ask|skip|quit)::
Conflicts can occur when applying a commit to p4.  When this
diff --git a/git-p4.py b/git-p4.py
index 76859b453..7bb9cadc6 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1178,6 +1178,12 @@ class Command:
 self.needsGit = True
 self.verbose = False
 
+# This is required for the "append" cloneExclude action
+def ensure_value(self, attr, value):
+if not hasattr(self, attr) or getattr(self, attr) is None:
+setattr(self, attr, value)
+return getattr(self, attr)
+
 class P4UserMap:
 def __init__(self):
 self.userMapFromPerforceServer = False
@@ -1343,9 +1349,10 @@ class P4Submit(Command, P4UserMap):
 optparse.make_option("--shelve", dest="shelve", 
action="store_true",
  help="Shelve instead of submit. Shelved 
files are reverted, "
  "restoring the workspace to the state 
before the shelve"),
-optparse.make_option("--update-shelve", dest="update_shelve", 
action="store", type="int",
+optparse.make_option("--update-shelve", dest="update_shelve", 
action="append", type="int",
  metavar="CHANGELIST",
- help="update an existing shelved 
changelist, implies --shelve")
+ help="update an existing shelved 
changelist, implies --shelve, "
+   "repeat in-order for multiple 
shelved changelists")
 ]
 self.description = "Submit changes from git to the perforce depot."
 self.usage += " [name of git branch to submit into perforce depot]"
@@ -1354,7 +1361,7 @@ class P4Submit(Command, P4UserMap):
 self.preserveUser = gitConfigBool("git-p4.preserveUser")
 self.dry_run = False
 self.shelve = False
-self.update_shelve = None
+self.update_shelve = list()
 self.prepare_p4_only = False
 self.conflict_behavior = None
 self.isWindows = (platform.system() == "Windows")
@@ -1809,9 +1816,10 @@ class P4Submit(Command, P4UserMap):
 mode = filesToChangeExecBit[f]
 setP4ExecBit(f, mode)
 
-if self.update_shelve:
-print("all_files = %s" % str(all_files))
-p4_reopen_in_change(self.update_shelve, all_files)
+update_shelve = 0
+if len(self.update_shelve) > 0:
+update_shelve = self.update_shelve.pop(0)
+p4_reopen_in_change(update_shelve, all_files)
 
 #
 # Build p4 change description, starting with the contents
@@ -1821,7 +1829,7 @@ class P4Submit(Command, P4UserMap):
 logMessage = logMessage.strip()
 (logMessage, jobs) = self.separate_jobs_from_description(logMessage)
 
-template = self.prepareSubmitTemplate(self.update_shelve)
+template = self.prepareSubmitTemplate(update_shelve)
 submitTemplate = self.prepareLogMessage(template, logMessage, jobs)
 
 if self.preserveUser:
@@ -1894,7 +1902,7 @@ class P4Submit(Command, P4UserMap):
  

[PATCH 0/1] git-p4: update multiple shelved change lists

[Luke Diamand]

This change lets you update several P4 changelists in sequence. Say
you have several git commits which are all somehow related. You would
start by shelving them (e.g. for a review), something like this:

 git p4 submit --origin HEAD^2 --shelve

You then make changes to these commits (in git) and now need to re-shelve
them. Before this change you would need to cherry-pick each change onto
a clean branch and do "git p4 --update-shelve", then remove the tip and
repeat.

With this change, you can just do:

 git p4 submit --origin HEAD^2 --update-shelve $CL1 --update-shelve $CL2

If the shelved changelists overlap (one changelist touches the same line
as another) then this won't work, but that problem already exists with
the --shelve option. Solving that is pretty hard to do as P4 really
only understands files, not changes. Despite this shortcoming, it's
very useful to be able to do update shelved changelists like this.

Luke Diamand (1):
  git-p4: update multiple shelved change lists

 Documentation/git-p4.txt |  8 +++-
 git-p4.py| 41 ++---
 t/t9807-git-p4-submit.sh | 24 ++--
 3 files changed, 47 insertions(+), 26 deletions(-)

-- 
2.15.0.276.g89ea799.dirty

Re: [PATCH 3/6] fsmonitor: Update helper tool, now that flags are filled later

[Alex Vandiver]

On Mon, 18 Dec 2017, Alex Vandiver wrote:
> dd9005a0a ("fsmonitor: delay updating state until after split index is
> merged", 2017-10-27) began deferring the setting of the
> CE_FSMONITOR_VALID flag until later, such that do_read_index() does
> not perform those steps.  This means that t/helper/test-dump-fsmonitor
> showed all bits as always unset.

This isn't the right fix, actually.  With split indexes, this puts us
right back into "only shows a few bits" territory, because
do_read_index doesn't know about split indexes.

Which means we need a way to do the whole index load but _not_
add/remove the fsmonitor cache, even if the config says to do so.

The best I'm coming up with is the below -- but I'm not happy with
it, because 'keep' is meaningless as a configuration value outside of
testing, since it's normally treated as an executable path.  This uses
the fact that fsmonitor.c currently has a:

switch (fsmonitor_enabled) {
case -1: /* keep: do nothing */
break;

...despite get_config_get_fsmonitor() havong no way to return -1 at
present.

Is this sort of testing generally done via environment variables,
rather than magic config values?
 - Alex

-8<
diff --git a/config.c b/config.c
index 6fb06c213..75fcf1a52 100644
--- a/config.c
+++ b/config.c
@@ -2164,8 +2164,13 @@ int git_config_get_fsmonitor(void)
if (core_fsmonitor && !*core_fsmonitor)
core_fsmonitor = NULL;

-   if (core_fsmonitor)
-   return 1;
+
+   if (core_fsmonitor) {
+   if (!strcasecmp(core_fsmonitor, "keep"))
+   return -1;
+   else
+   return 1;
+   }

return 0;
 }
diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c
index ad452707e..12e131530 100644
--- a/t/helper/test-dump-fsmonitor.c
+++ b/t/helper/test-dump-fsmonitor.c
@@ -5,8 +5,9 @@ int cmd_main(int ac, const char **av)
struct index_state *istate = _index;
int i;

+   git_config_push_parameter("core.fsmonitor=keep");
setup_git_directory();
-   if (do_read_index(istate, get_index_file(), 0) < 0)
+   if (read_index_from(istate, get_index_file()) < 0)
die("unable to read index file");
if (!istate->fsmonitor_last_update) {
printf("no fsmonitor\n");
-8<-

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Stefan Beller]

On Wed, Dec 20, 2017 at 12:22 AM, Andreas Urke  wrote:
> Thanks for looking into this.
>
> I was able to reproduce it from scratch, but I followed my earlier
> workflow where I first created the subrepos, and then later renamed
> it. At the time I was not able to find any command to rename without
> changing the path (and I was not able find one now either, is there
> any?), so I edited name and path in .gitmodules and ran git submodule
> sync. Am I asking for trouble doing it that way?

"rename without changing the path" sounds like a red flag to me,
as the submodule name was introduced specifically to be a constant
as the path may change, whereas the name ought to never change.




>
> Let me know if you need the exact steps I followed.

Well yes, ideally as a shell script (or even embedded into our test suite).

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff King]

On Wed, Dec 20, 2017 at 02:42:44PM +, Jeff Hostetler wrote:

> From: Jeff Hostetler 
> 
> Teach "git status --short --branch" to use "--no-ahead-behind"
> flag to skip computing ahead/behind counts for the branch and
> its upstream and just report '[different]'.

How come "--short" and "--long" get this smaller bit of data, but
"--porcelain=v2" just omits the line entirely?

I don't have a real preference for or against the "[different]" message
myself, but if we can get the information cheaply, it seems odd not to
provide it in all cases.

> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 6ce8cf8..ea029ad 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -117,6 +117,9 @@ configuration variable documented in 
> linkgit:git-config[1].
>   expensive computation on extremely large repositories.
>  +
>   In porcelain V2 format, the 'branch.ab' line will not be present.
> ++
> + In short format with --branch, '[different]' will printed rather
> + than detailed ahead/behind counts.

s/will/will be/ ?

> diff --git a/remote.c b/remote.c
> index a38b42e..0a63ac1 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1978,9 +1978,12 @@ int ref_newer(const struct object_id *new_oid, const 
> struct object_id *old_oid)
>  
>  /*
>   * Compare a branch with its upstream, and save their differences (number
> - * of commits) in *num_ours and *num_theirs. The name of the upstream branch
> - * (or NULL if no upstream is defined) is returned via *upstream_name, if it
> - * is not itself NULL.
> + * of commits) in *num_ours and *num_theirs.  If either num_ours or 
> num_theirs
> + * are NULL, we skip counting the commits and just return whether they are
> + * different.

OK, this makes sense. I wondered in the last one why the caller could
not simply check "num_ours != num_theirs" themselves. And this is why:
we want to be able to signal to stat_tracking_info() that we want the
"cheap" version.

> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 8f17fd9..0190220 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -147,6 +147,19 @@ test_expect_success 'status -s -b (diverged from 
> upstream)' '
>  '
>  
>  cat >expect <<\EOF
> +## b1...origin/master [different]
> +EOF
> +
> +test_expect_success 'status -s -b --no-ahead-behind (diverged from 
> upstream)' '

This patch will affect "git status --porcelain", too. That's not
supposed to change in incompatible ways. I guess it's up for debate
whether callers are meant to handle any arbitrary string inside the []
(we already show "[gone]" for some cases), since AFAICT the format of
the tracking info is left completely vague in the documentation.

(I'd also hope that everybody is using --porcelain=v2 if they can, but
we should still avoid breaking v1).

-Peff

[PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff Hostetler]

From: Jeff Hostetler 

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

Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt |  3 +++
 remote.c | 12 +---
 t/t6040-tracking-info.sh | 13 +
 wt-status.c  | 11 +--
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 6ce8cf8..ea029ad 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -117,6 +117,9 @@ configuration variable documented in linkgit:git-config[1].
expensive computation on extremely large repositories.
 +
In porcelain V2 format, the 'branch.ab' line will not be present.
++
+   In short format with --branch, '[different]' will printed rather
+   than detailed ahead/behind counts.
 
 ...::
See the 'pathspec' entry in linkgit:gitglossary[7].
diff --git a/remote.c b/remote.c
index a38b42e..0a63ac1 100644
--- a/remote.c
+++ b/remote.c
@@ -1978,9 +1978,12 @@ int ref_newer(const struct object_id *new_oid, const 
struct object_id *old_oid)
 
 /*
  * Compare a branch with its upstream, and save their differences (number
- * of commits) in *num_ours and *num_theirs. The name of the upstream branch
- * (or NULL if no upstream is defined) is returned via *upstream_name, if it
- * is not itself NULL.
+ * of commits) in *num_ours and *num_theirs.  If either num_ours or num_theirs
+ * are NULL, we skip counting the commits and just return whether they are
+ * different.
+ *
+ * The name of the upstream branch (or NULL if no upstream is defined) is
+ * returned via *upstream_name, if it is not itself NULL.
  *
  * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
  * upstream defined, or ref does not exist).
@@ -2016,6 +2019,9 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
if (!ours)
return -1;
 
+   if (!num_ours || !num_theirs)
+   return theirs != ours;
+
/* are we the same? */
if (theirs == ours) {
*num_theirs = *num_ours = 0;
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 8f17fd9..0190220 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -147,6 +147,19 @@ test_expect_success 'status -s -b (diverged from 
upstream)' '
 '
 
 cat >expect <<\EOF
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status -s -b --no-ahead-behind | head -1
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index 471ba15..6b4f969 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1767,6 +1767,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
const char *branch_name;
int num_ours, num_theirs;
int upstream_is_gone = 0;
+   int sti;
 
color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## ");
 
@@ -1791,7 +1792,11 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
 
color_fprintf(s->fp, branch_color_local, "%s", branch_name);
 
-   if (stat_tracking_info(branch, _ours, _theirs, ) < 0) {
+   if (s->no_ahead_behind)
+   sti = stat_tracking_info(branch, NULL, NULL, );
+   else
+   sti = stat_tracking_info(branch, _ours, _theirs, );
+   if (sti < 0) {
if (!base)
goto conclude;
 
@@ -1803,12 +1808,14 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
color_fprintf(s->fp, branch_color_remote, "%s", short_base);
free(short_base);
 
-   if (!upstream_is_gone && !num_ours && !num_theirs)
+   if (!upstream_is_gone && !sti)
goto conclude;
 
color_fprintf(s->fp, header_color, " [");
if (upstream_is_gone) {
color_fprintf(s->fp, header_color, LABEL(N_("gone")));
+   } else if (s->no_ahead_behind) {
+   color_fprintf(s->fp, header_color, LABEL(N_("different")));
} else if (!num_ours) {
color_fprintf(s->fp, header_color, LABEL(N_("behind ")));
color_fprintf(s->fp, branch_color_remote, "%d", num_theirs);
-- 
2.9.3

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Andreas Urke]

Thanks for looking into this.

I was able to reproduce it from scratch, but I followed my earlier
workflow where I first created the subrepos, and then later renamed
it. At the time I was not able to find any command to rename without
changing the path (and I was not able find one now either, is there
any?), so I edited name and path in .gitmodules and ran git submodule
sync. Am I asking for trouble doing it that way?

Let me know if you need the exact steps I followed.

Andreas


On 19 December 2017 at 23:33, Stefan Beller <sbel...@google.com> wrote:
> On Tue, Dec 19, 2017 at 2:19 PM, Junio C Hamano <gits...@pobox.com> wrote:
>> Stefan Beller <sbel...@google.com> writes:
>>
>>> I tried reproducing the issue (based on the `next` branch, not 2.15,
>>> but I do not recall any changes in the submodule area lately), and
>>> could not come up with a reproduction recipe,...
>>
>> I do not offhand recall anything; the closest I can think of is the
>> three-patch series <20171016135623.ga12...@book.hvoigt.net> that
>> taught the on-demand recursive fetch to pay attention to the location
>> in the superproject tree a submodule is bound to.
>
> I tried the same test on 2.15 and cannot reproduce there either.
>
>>
>> 4b4acedd61 submodule: simplify decision tree whether to or not to fetch
>> c68f837576 implement fetching of moved submodules
>> 01ce12252c fetch: add test to make sure we stay backwards compatible
>>
>> But IIRC, "submodule update" uses a separate codepath?
>
> Yes, any portion of git-submodule.sh that calls out to C is going
> through the submodule--helper. I want to revive the port of that
> shell script to C again.
>
> The "submodule update" uses the submodule helper to obtain
> the list of submodules and then does a "git -C $sub fetch" in there.
>
> Stefan

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Stefan Beller]

On Tue, Dec 19, 2017 at 2:19 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@google.com> writes:
>
>> I tried reproducing the issue (based on the `next` branch, not 2.15,
>> but I do not recall any changes in the submodule area lately), and
>> could not come up with a reproduction recipe,...
>
> I do not offhand recall anything; the closest I can think of is the
> three-patch series <20171016135623.ga12...@book.hvoigt.net> that
> taught the on-demand recursive fetch to pay attention to the location
> in the superproject tree a submodule is bound to.

I tried the same test on 2.15 and cannot reproduce there either.

>
> 4b4acedd61 submodule: simplify decision tree whether to or not to fetch
> c68f837576 implement fetching of moved submodules
> 01ce12252c fetch: add test to make sure we stay backwards compatible
>
> But IIRC, "submodule update" uses a separate codepath?

Yes, any portion of git-submodule.sh that calls out to C is going
through the submodule--helper. I want to revive the port of that
shell script to C again.

The "submodule update" uses the submodule helper to obtain
the list of submodules and then does a "git -C $sub fetch" in there.

Stefan

Re: [PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Junio C Hamano]

Stefan Beller <sbel...@google.com> writes:

> I tried reproducing the issue (based on the `next` branch, not 2.15,
> but I do not recall any changes in the submodule area lately), and
> could not come up with a reproduction recipe,...

I do not offhand recall anything; the closest I can think of is the
three-patch series <20171016135623.ga12...@book.hvoigt.net> that
taught the on-demand recursive fetch to pay attention to the location
in the superproject tree a submodule is bound to.

4b4acedd61 submodule: simplify decision tree whether to or not to fetch
c68f837576 implement fetching of moved submodules
01ce12252c fetch: add test to make sure we stay backwards compatible

But IIRC, "submodule update" uses a separate codepath?

[PATCH] Re: Bug with "git submodule update" + subrepo with differing path/name?

[Stefan Beller]

I tried reproducing the issue (based on the `next` branch, not 2.15,
but I do not recall any changes in the submodule area lately), and
could not come up with a reproduction recipe, but here is what I got so
far, maybe you can take it from here (i.e. either make the test case
more like your environment such that it fails, or rather bisect git
to tell us the offending commit) ?

Signed-off-by: Stefan Beller <sbel...@google.com>
---
 t/t7406-submodule-update.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 6f083c4d68..d957305c38 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -978,4 +978,20 @@ test_expect_success 'git clone passes the parallel jobs 
config on to submodules'
rm -rf super4
 '
 
+test_expect_success 'git submodule update works with submodules with name/path 
difference' '
+   test_create_repo super6 &&
+   (
+   cd super6 &&
+   git submodule add ../submodule sub1 &&
+   git submodule add --name testingname ../submodule sub2 &&
+   git commit -m initial &&
+   git -C sub1 checkout HEAD^ &&
+   git -C sub2 checkout HEAD^ &&
+
+   git submodule update >actual &&
+   grep sub1 actual &&
+   grep sub2 actual
+   )
+'
+
 test_done
-- 
2.15.1.620.gb9897f4670-goog

Bug with "git submodule update" + subrepo with differing path/name?

[Andreas Urke]

Hi,

I have a repo with two submodules. I regularly use "git submodule
update", which works fine for one subrepo, but not for the other. The
only relevant difference I can think of between these two subrepos is
that the latter one does not have a matching path and name:

$ cat .gitmodules
[submodule "subrepo1"]
path = subrepo1
url = g...@gitlab.com:subrepo1.git
[submodule "name_subrepo2"]
path = subrepo2
url = g...@gitlab.com:name_subrepo2.git

And a reproduction looks like this:

$ git --version
git version 2.15.1

$ git status
HEAD detached at 05412d4
Changes not staged for commit:

modified:   subrepo1 (new commits)
modified:   subrepo2 (new commits)

$ git submodule update
Submodule path 'subrepo1': checked out
'e4785498130ec4188bb1745f27e311bff08ed4c8'

$ git status
HEAD detached at 05412d4
Changes not staged for commit:

modified:   subrepo2  (new commits)

Checking out the relevant commit manually in subrepo2 works fine (and
is what I end up doing).

Regards,
Andreas Urke

[PATCH v5 3/9] Add a function to update HEAD after creating a commit

[Phillip Wood]

From: Phillip Wood 

Add update_head_with_reflog() based on the code that updates HEAD
after committing in builtin/commit.c that can be called by 'git
commit' and other commands.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v2:
 - updated commit message to reflect the change in function name
 - style fixes

changes since v1:
 - rename update_head() to update_head_with_reflog()

 builtin/commit.c | 20 ++--
 sequencer.c  | 39 ++-
 sequencer.h  |  4 
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
2175dac8036c465a73c4c782f061e85ae6d1a629..340cc2988ebdb92ef222b677ee12c94fa53aa1ff
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1610,13 +1610,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
const char *index_file, *reflog_msg;
-   char *nl;
struct object_id oid;
struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
-   struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -1739,25 +1737,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(_ident);
free_commit_extra_headers(extra);
 
-   nl = strchr(sb.buf, '\n');
-   if (nl)
-   strbuf_setlen(, nl + 1 - sb.buf);
-   else
-   strbuf_addch(, '\n');
-   strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
-   strbuf_insert(, strlen(reflog_msg), ": ", 2);
-
-   transaction = ref_transaction_begin();
-   if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", ,
-  current_head
-  ? _head->object.oid : _oid,
-  0, sb.buf, ) ||
-   ref_transaction_commit(transaction, )) {
+   if (update_head_with_reflog(current_head, , reflog_msg, ,
+   )) {
rollback_index_files();
die("%s", err.buf);
}
-   ref_transaction_free(transaction);
 
unlink(git_path_cherry_pick_head());
unlink(git_path_revert_head());
diff --git a/sequencer.c b/sequencer.c
index 
168da5093e71f50a4d70af7288cf761110e69e87..5fe6ef3512566622f0423a09cbffe1adf1e65957
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,10 +1,10 @@
 #include "cache.h"
 #include "config.h"
 #include "lockfile.h"
-#include "sequencer.h"
 #include "dir.h"
 #include "object.h"
 #include "commit.h"
+#include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
 #include "exec_cmd.h"
@@ -753,6 +753,43 @@ int template_untouched(const struct strbuf *sb, const char 
*template_file,
return rest_is_empty(sb, start - sb->buf);
 }
 
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char *action, const struct strbuf *msg,
+   struct strbuf *err)
+{
+   struct ref_transaction *transaction;
+   struct strbuf sb = STRBUF_INIT;
+   const char *nl;
+   int ret = 0;
+
+   if (action) {
+   strbuf_addstr(, action);
+   strbuf_addstr(, ": ");
+   }
+
+   nl = strchr(msg->buf, '\n');
+   if (nl) {
+   strbuf_add(, msg->buf, nl + 1 - msg->buf);
+   } else {
+   strbuf_addbuf(, msg);
+   strbuf_addch(, '\n');
+   }
+
+   transaction = ref_transaction_begin(err);
+   if (!transaction ||
+   ref_transaction_update(transaction, "HEAD", new_head,
+  old_head ? _head->object.oid : _oid,
+  0, sb.buf, err) ||
+   ref_transaction_commit(transaction, err)) {
+   ret = -1;
+   }
+   ref_transaction_free(transaction);
+   strbuf_release();
+
+   return ret;
+}
+
 static int is_original_commit_empty(struct commit *commit)
 {
const struct object_id *ptree_oid;
diff --git a/sequencer.h b/sequencer.h
index 
2040773c7b6fd3966d5a1a14f410ea8ff53843c8..3757a7aecb5a7795d7b9b45964f3328ee852e14b
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -68,4 +68,8 @@ int message_is_empty(const struct strbuf *sb,
 enum commit_msg_cleanup_mode cleanup_mode);
 int template_untouched(const struct strbuf *sb, const char *template_file,
   enum commit_msg_cleanup_mode cleanup_mode);
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char 

[PATCH v3 6/9] rebase -i: update functions to use a flags parameter

[Liam Beguin]

Update functions used in the rebase--helper so that they take a generic
'flags' parameter instead of a growing list of options.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c | 13 +++--
 sequencer.c  |  9 +
 sequencer.h  |  8 +---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index c3b8e4d401f8..1102ecb43b67 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   int keep_empty = 0;
+   unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
@@ -48,16 +48,17 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, NULL, options,
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
+   flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+
if (command == CONTINUE && argc == 1)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_OIDS && argc == 1)
-   return !!transform_todos(1);
-   if (command == EXPAND_OIDS && argc == 1)
-   return !!transform_todos(0);
+   return !!sequencer_make_script(stdout, argc, argv, flags);
+   if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
+   return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index c9a661a8c4bd..8b0dd610c881 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2444,14 +2444,15 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
strbuf_release();
 }
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags)
 {
char *format = NULL;
struct pretty_print_context pp = {0};
struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
struct commit *commit;
+   int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2494,7 +2495,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todos(int shorten_ids)
+int transform_todos(unsigned flags)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
@@ -2522,7 +2523,7 @@ int transform_todos(int shorten_ids)
 
/* add commit id */
if (item->commit) {
-   const char *oid = shorten_ids ?
+   const char *oid = flags & TODO_LIST_SHORTEN_IDS ?
  short_commit_name(item->commit) :
  oid_to_hex(>commit->object.oid);
 
diff --git a/sequencer.h b/sequencer.h
index 4f7f2c93f83e..68284e9762c8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv);
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
+#define TODO_LIST_SHORTEN_IDS (1U << 1)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags);
 
-int transform_todos(int shorten_ids);
+int transform_todos(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

[PATCH v6 08/14] fixup: fetch: update --blob-max-bytes to --fitler

[Jeff Hostetler]

From: Jeff Hostetler 

Signed-off-by: Jeff Hostetler 
---
 t/t5500-fetch-pack.sh | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 65fc7bb..ec9ba9b 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -782,7 +782,7 @@ test_expect_success 'filtering by size has no effect if 
support for it is not ad
test_i18ngrep "filtering not recognized by server" err
 '
 
-fetch_blob_max_bytes () {
+fetch_filter_blob_limit_zero () {
SERVER="$1"
URL="$2"
 
@@ -804,15 +804,15 @@ fetch_blob_max_bytes () {
test_must_fail git -C client cat-file -e $(git hash-object 
"$SERVER/two.t")
 }
 
-test_expect_success 'fetch with --blob-max-bytes' '
-   fetch_blob_max_bytes server server
+test_expect_success 'fetch with --filter=blob:limit=0' '
+   fetch_filter_blob_limit_zero server server
 '
 
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-test_expect_success 'fetch with --blob-max-bytes and HTTP' '
-   fetch_blob_max_bytes "$HTTPD_DOCUMENT_ROOT_PATH/server" 
"$HTTPD_URL/smart/server"
+test_expect_success 'fetch with --filter=blob:limit=0 and HTTP' '
+   fetch_filter_blob_limit_zero "$HTTPD_DOCUMENT_ROOT_PATH/server" 
"$HTTPD_URL/smart/server"
 '
 
 stop_httpd
-- 
2.9.3

[PATCH v6 07/14] fixup: fetch: update error messages from --blob-max-bytes to --filter

[Jeff Hostetler]

From: Jeff Hostetler 

Signed-off-by: Jeff Hostetler 
---
 builtin/fetch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 05d0b1a..14aab71 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1372,7 +1372,7 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
deepen = 1;
 
if (filter_options.choice && !repository_format_partial_clone)
-   die("--blob-max-bytes can only be used when 
extensions.partialClone is set");
+   die("--filter can only be used when extensions.partialClone is 
set");
 
if (all) {
if (argc == 1)
@@ -1406,11 +1406,11 @@ int cmd_fetch(int argc, const char **argv, const char 
*prefix)
if (remote) {
if (filter_options.choice &&
strcmp(remote->name, repository_format_partial_clone))
-   die(_("--blob-max-bytes can only be used with the 
remote configured in core.partialClone"));
+   die(_("--filter can only be used with the remote 
configured in core.partialClone"));
result = fetch_one(remote, argc, argv);
} else {
if (filter_options.choice)
-   die(_("--blob-max-bytes can only be used with the 
remote configured in core.partialClone"));
+   die(_("--filter can only be used with the remote 
configured in core.partialClone"));
result = fetch_multiple();
}
 
-- 
2.9.3

RE: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Kerry, Richard]

Indeed so.
In which case it is short for "selling short", or possibly "short selling".

Of course a little searching shows that "shorted" could mean some other things, 
including possibly the meaning originally suggested.
Nevertheless it seems to me that "shortened" is the most appropriate word in 
modern English.

R.


Richard Kerry
BNCS Engineer, SI SOL Telco & Media Vertical Practice

T: +44 (0)20 3618 2669
M: +44 (0)7812 325518
Lync: +44 (0) 20 3618 0778
Room G300, Stadium House, Wood Lane, London, W12 7TA
richard.ke...@atos.net




> -Original Message-
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Tuesday, December 05, 2017 4:06 PM
> To: Kerry, Richard <richard.ke...@atos.net>
> Cc: git@vger.kernel.org; Johannes Schindelin
> <johannes.schinde...@gmx.de>; p...@peff.net; liam Beguin
> <liambeg...@gmail.com>
> Subject: Re: [PATCH v2 6/9] rebase -i: update functions to use a flags
> parameter
>
> "Kerry, Richard" <richard.ke...@atos.net> writes:
>
> > "Shorted" is what happens when you put a piece of wire across the
> terminals of a battery ... (bang, smoke, etc).
> > It's short for "short-circuited".
>
> Or it is what you do to something that you sell and that you yet do not own,
> expecting that you can later buy it cheaper, allowing you to pocket the
> difference ;-).
>

Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading 
names used by the Atos group. The following trading entities are registered in 
England and Wales: Atos IT Services UK Limited (registered number 01245534), 
Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited 
(registered number 08514184) and Canopy The Open Cloud Company Limited 
(registration number 08011902). The registered office for each is at 4 Triton 
Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983.

This e-mail and the documents attached are confidential and intended solely for 
the addressee, and may contain confidential or privileged information. If you 
receive this e-mail in error, you are not authorised to copy, disclose, use or 
retain it. Please notify the sender immediately and delete this email from your 
systems. As emails may be intercepted, amended or lost, they are not secure. 
Atos therefore can accept no liability for any errors or their content. 
Although Atos endeavours to maintain a virus-free network, we do not warrant 
that this transmission is virus-free and can accept no liability for any 
damages resulting from any virus transmitted. The risks are deemed to be 
accepted by everyone who communicates with Atos by email.

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Junio C Hamano]

"Kerry, Richard"  writes:

> "Shorted" is what happens when you put a piece of wire across the terminals 
> of a battery ... (bang, smoke, etc).
> It's short for "short-circuited".

Or it is what you do to something that you sell and that you yet do
not own, expecting that you can later buy it cheaper, allowing you
to pocket the difference ;-).

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[liam Beguin]

Hi,

On 05/12/17 07:41 AM, Kerry, Richard wrote:
> 
> "Shorted" is what happens when you put a piece of wire across the terminals 
> of a battery ... (bang, smoke, etc).
> It's short for "short-circuited".
> Yes, I think you mean "shortened" in this case.
> 

Thanks for the explanation.
Sorry, my eyes stopped at the lowercase 's' in Johannes message.
Will fix.

> Regards,
> Richard.
> 
> 
> 
> Richard Kerry
> BNCS Engineer, SI SOL Telco & Media Vertical Practice
> 
> T: +44 (0)20 3618 2669
> M: +44 (0)7812 325518
> Lync: +44 (0) 20 3618 0778
> Room G300, Stadium House, Wood Lane, London, W12 7TA
> richard.ke...@atos.net
> 
> 

[...]

Thanks,
Liam

RE: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Kerry, Richard]

"Shorted" is what happens when you put a piece of wire across the terminals of 
a battery ... (bang, smoke, etc).
It's short for "short-circuited".
Yes, I think you mean "shortened" in this case.

Regards,
Richard.



Richard Kerry
BNCS Engineer, SI SOL Telco & Media Vertical Practice

T: +44 (0)20 3618 2669
M: +44 (0)7812 325518
Lync: +44 (0) 20 3618 0778
Room G300, Stadium House, Wood Lane, London, W12 7TA
richard.ke...@atos.net




> -Original Message-
> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
> Behalf Of Junio C Hamano
> Sent: Tuesday, December 05, 2017 12:37 PM
> To: liam Beguin <liambeg...@gmail.com>
> Cc: Johannes Schindelin <johannes.schinde...@gmx.de>;
> git@vger.kernel.org; p...@peff.net
> Subject: Re: [PATCH v2 6/9] rebase -i: update functions to use a flags
> parameter
>
> liam Beguin <liambeg...@gmail.com> writes:
>
> > I'll change it to TODO_LIST_SHORTED_IDs. TODO_LIST_SHORTED_INSNS
> would
> > suggest the flag changes both parts of the todo.
>
> I am not a native speaker, but SHORTED does not sound like a right phrase.
> When you make something shorter, that thing is "shortened", not "shorted".

Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading 
names used by the Atos group. The following trading entities are registered in 
England and Wales: Atos IT Services UK Limited (registered number 01245534), 
Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited 
(registered number 08514184) and Canopy The Open Cloud Company Limited 
(registration number 08011902). The registered office for each is at 4 Triton 
Square, Regent’s Place, London, NW1 3HG.The VAT No. for each is: GB232327983.

This e-mail and the documents attached are confidential and intended solely for 
the addressee, and may contain confidential or privileged information. If you 
receive this e-mail in error, you are not authorised to copy, disclose, use or 
retain it. Please notify the sender immediately and delete this email from your 
systems. As emails may be intercepted, amended or lost, they are not secure. 
Atos therefore can accept no liability for any errors or their content. 
Although Atos endeavours to maintain a virus-free network, we do not warrant 
that this transmission is virus-free and can accept no liability for any 
damages resulting from any virus transmitted. The risks are deemed to be 
accepted by everyone who communicates with Atos by email.

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Junio C Hamano]

liam Beguin  writes:

> I'll change it to TODO_LIST_SHORTED_IDs. TODO_LIST_SHORTED_INSNS would
> suggest the flag changes both parts of the todo.

I am not a native speaker, but SHORTED does not sound like a right
phrase.  When you make something shorter, that thing is "shortened",
not "shorted".

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[liam Beguin]

Hi Johannes,

On 04/12/17 10:46 AM, Johannes Schindelin wrote:
> Hi Liam,
> 
> On Sun, 3 Dec 2017, Liam Beguin wrote:
> 
>> diff --git a/sequencer.h b/sequencer.h
>> index 4e444e3bf1c4..3bb6b0658192 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
>>  int sequencer_rollback(struct replay_opts *opts);
>>  int sequencer_remove_state(struct replay_opts *opts);
>>  
>> -int sequencer_make_script(int keep_empty, FILE *out,
>> -int argc, const char **argv);
>> +#define TODO_LIST_KEEP_EMPTY (1U << 0)
>> +#define TODO_LIST_SHORTED_IDS (1U << 1)
> 
> Maybe SHORTEN_IDs? And either revert back to transform_todo_ids() or use
> SHORTEN_INSNS...
> 

I'll change it to TODO_LIST_SHORTED_IDs. TODO_LIST_SHORTED_INSNS would
suggest the flag changes both parts of the todo.

> Maybe also TRANSFORM_TODO_LIST_* and maybe move the #define's above the
> transform_todo_ids() function, i.e. one line further down?
> 
>> +int sequencer_make_script(FILE *out, int argc, const char **argv,
>> +  unsigned flags);
>>  
>> -int transform_todo_insn(int shorten_ids);
>> +int transform_todo_insn(unsigned flags);
>>  int check_todo_list(void);
>>  int skip_unnecessary_picks(void);
>>  int rearrange_squash(void);
> 
> Ciao,
> Johannes
> 

Thanks, 
Liam

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Johannes Schindelin]

Hi Liam,

On Mon, 4 Dec 2017, Johannes Schindelin wrote:

> On Sun, 3 Dec 2017, Liam Beguin wrote:
> 
> > diff --git a/sequencer.h b/sequencer.h
> > index 4e444e3bf1c4..3bb6b0658192 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
> >  int sequencer_rollback(struct replay_opts *opts);
> >  int sequencer_remove_state(struct replay_opts *opts);
> >  
> > -int sequencer_make_script(int keep_empty, FILE *out,
> > -   int argc, const char **argv);
> > +#define TODO_LIST_KEEP_EMPTY (1U << 0)
> > +#define TODO_LIST_SHORTED_IDS (1U << 1)
> 
> Maybe SHORTEN_IDs? And either revert back to transform_todo_ids() or use
> SHORTEN_INSNS...
> 
> Maybe also TRANSFORM_TODO_LIST_* and maybe move the #define's above the
> transform_todo_ids() function, i.e. one line further down?
> 
> > +int sequencer_make_script(FILE *out, int argc, const char **argv,
> > + unsigned flags);

Ah, I just realized that make_script() already takes `flags`. Sorry for
the noise,
Johannes

Re: [PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Johannes Schindelin]

Hi Liam,

On Sun, 3 Dec 2017, Liam Beguin wrote:

> diff --git a/sequencer.h b/sequencer.h
> index 4e444e3bf1c4..3bb6b0658192 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
>  int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
> -int sequencer_make_script(int keep_empty, FILE *out,
> - int argc, const char **argv);
> +#define TODO_LIST_KEEP_EMPTY (1U << 0)
> +#define TODO_LIST_SHORTED_IDS (1U << 1)

Maybe SHORTEN_IDs? And either revert back to transform_todo_ids() or use
SHORTEN_INSNS...

Maybe also TRANSFORM_TODO_LIST_* and maybe move the #define's above the
transform_todo_ids() function, i.e. one line further down?

> +int sequencer_make_script(FILE *out, int argc, const char **argv,
> +   unsigned flags);
>  
> -int transform_todo_insn(int shorten_ids);
> +int transform_todo_insn(unsigned flags);
>  int check_todo_list(void);
>  int skip_unnecessary_picks(void);
>  int rearrange_squash(void);

Ciao,
Johannes

[PATCH v2 6/9] rebase -i: update functions to use a flags parameter

[Liam Beguin]

Update functions used in the rebase--helper so that they take a generic
'flags' parameter instead of a growing list of options.

Signed-off-by: Liam Beguin <liambeg...@gmail.com>
---
 builtin/rebase--helper.c | 13 +++--
 sequencer.c  |  9 +
 sequencer.h  |  8 +---
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index af0f91164fd0..fe814bf7229e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,7 +12,7 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   int keep_empty = 0;
+   unsigned flags = 0, keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
@@ -48,16 +48,17 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, NULL, options,
builtin_rebase_helper_usage, PARSE_OPT_KEEP_ARGV0);
 
+   flags |= keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
+   flags |= command == SHORTEN_OIDS ? TODO_LIST_SHORTED_IDS : 0;
+
if (command == CONTINUE && argc == 1)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
-   if (command == SHORTEN_OIDS && argc == 1)
-   return !!transform_todo_insn(1);
-   if (command == EXPAND_OIDS && argc == 1)
-   return !!transform_todo_insn(0);
+   return !!sequencer_make_script(stdout, argc, argv, flags);
+   if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
+   return !!transform_todo_insn(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
diff --git a/sequencer.c b/sequencer.c
index 0ff3c90e44bf..7d712811e9d1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2444,14 +2444,15 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
strbuf_release();
 }
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags)
 {
char *format = NULL;
struct pretty_print_context pp = {0};
struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
struct commit *commit;
+   int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 
init_revisions(, NULL);
revs.verbose_header = 1;
@@ -2494,7 +2495,7 @@ int sequencer_make_script(int keep_empty, FILE *out,
 }
 
 
-int transform_todo_insn(int shorten_ids)
+int transform_todo_insn(unsigned flags)
 {
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
@@ -2522,7 +2523,7 @@ int transform_todo_insn(int shorten_ids)
 
/* add commit id */
if (item->commit) {
-   const char *oid = shorten_ids ?
+   const char *oid = flags & TODO_LIST_SHORTED_IDS ?
  short_commit_name(item->commit) :
  oid_to_hex(>commit->object.oid);
 
diff --git a/sequencer.h b/sequencer.h
index 4e444e3bf1c4..3bb6b0658192 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -45,10 +45,12 @@ int sequencer_continue(struct replay_opts *opts);
 int sequencer_rollback(struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-int sequencer_make_script(int keep_empty, FILE *out,
-   int argc, const char **argv);
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
+#define TODO_LIST_SHORTED_IDS (1U << 1)
+int sequencer_make_script(FILE *out, int argc, const char **argv,
+ unsigned flags);
 
-int transform_todo_insn(int shorten_ids);
+int transform_todo_insn(unsigned flags);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 
2.15.1.280.g10402c1f5b5c

Re: [PATCH] l10n: update de_DE translation

[Ralf Thielow]

Robert Abel  wrote:
> Der-, die- and dasselbe and their declensions are spelt as one word in German.
> 

Thanks!

> Signed-off-by: Robert Abel 
> ---
>  po/de.po | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/po/de.po b/po/de.po
> index a05aca5f3..400262625 100644
> --- a/po/de.po
> +++ b/po/de.po
> @@ -2925,7 +2925,7 @@ msgstr "  (benutzen Sie \"git branch --unset-upstream\" 
> zum Beheben)\n"
>  #: remote.c:2083
>  #, c-format
>  msgid "Your branch is up to date with '%s'.\n"
> -msgstr "Ihr Branch ist auf dem selben Stand wie '%s'.\n"
> +msgstr "Ihr Branch ist auf demselben Stand wie '%s'.\n"
>  
>  #: remote.c:2087
>  #, c-format
> @@ -10874,7 +10874,7 @@ msgstr "Kann nicht überschreiben"
>  
>  #: builtin/mv.c:239
>  msgid "multiple sources for the same target"
> -msgstr "mehrere Quellen für das selbe Ziel"
> +msgstr "mehrere Quellen für dasselbe Ziel"
>  
>  #: builtin/mv.c:241
>  msgid "destination directory does not exist"
> @@ -11827,7 +11827,7 @@ msgstr ""
>  "\n"
>  "git push %s HEAD:%s\n"
>  "\n"
> -"Um auf den Branch mit dem selben Namen im Remote-Repository zu versenden,\n"
> +"Um auf den Branch mit demselben Namen im Remote-Repository zu versenden,\n"
>  "benutzen Sie:\n"
>  "\n"
>  "git push %s %s\n"
> -- 
> 2.15.1
> 

[PATCH] l10n: update de_DE translation

[Robert Abel]

Der-, die- and dasselbe and their declensions are spelt as one word in German.

Signed-off-by: Robert Abel 
---
 po/de.po | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/po/de.po b/po/de.po
index a05aca5f3..400262625 100644
--- a/po/de.po
+++ b/po/de.po
@@ -2925,7 +2925,7 @@ msgstr "  (benutzen Sie \"git branch --unset-upstream\" 
zum Beheben)\n"
 #: remote.c:2083
 #, c-format
 msgid "Your branch is up to date with '%s'.\n"
-msgstr "Ihr Branch ist auf dem selben Stand wie '%s'.\n"
+msgstr "Ihr Branch ist auf demselben Stand wie '%s'.\n"
 
 #: remote.c:2087
 #, c-format
@@ -10874,7 +10874,7 @@ msgstr "Kann nicht überschreiben"
 
 #: builtin/mv.c:239
 msgid "multiple sources for the same target"
-msgstr "mehrere Quellen für das selbe Ziel"
+msgstr "mehrere Quellen für dasselbe Ziel"
 
 #: builtin/mv.c:241
 msgid "destination directory does not exist"
@@ -11827,7 +11827,7 @@ msgstr ""
 "\n"
 "git push %s HEAD:%s\n"
 "\n"
-"Um auf den Branch mit dem selben Namen im Remote-Repository zu versenden,\n"
+"Um auf den Branch mit demselben Namen im Remote-Repository zu versenden,\n"
 "benutzen Sie:\n"
 "\n"
 "git push %s %s\n"
-- 
2.15.1

[PATCH v4 3/9] Add a function to update HEAD after creating a commit

[Phillip Wood]

From: Phillip Wood 

Add update_head_with_reflog() based on the code that updates HEAD
after committing in builtin/commit.c that can be called by 'git
commit' and other commands.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v2:
 - updated commit message to reflect the change in function name
 - style fixes

changes since v1:
 - rename update_head() to update_head_with_reflog()

 builtin/commit.c | 20 ++--
 sequencer.c  | 39 ++-
 sequencer.h  |  4 
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
d958c2eb2adc9a29dab29340ce9b56daea41fecd..eb144556bf37b7bf357bd976b94305171b4fd159
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1610,13 +1610,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
const char *index_file, *reflog_msg;
-   char *nl;
struct object_id oid;
struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
-   struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -1739,25 +1737,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(_ident);
free_commit_extra_headers(extra);
 
-   nl = strchr(sb.buf, '\n');
-   if (nl)
-   strbuf_setlen(, nl + 1 - sb.buf);
-   else
-   strbuf_addch(, '\n');
-   strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
-   strbuf_insert(, strlen(reflog_msg), ": ", 2);
-
-   transaction = ref_transaction_begin();
-   if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", ,
-  current_head
-  ? _head->object.oid : _oid,
-  0, sb.buf, ) ||
-   ref_transaction_commit(transaction, )) {
+   if (update_head_with_reflog(current_head, , reflog_msg, ,
+   )) {
rollback_index_files();
die("%s", err.buf);
}
-   ref_transaction_free(transaction);
 
unlink(git_path_cherry_pick_head());
unlink(git_path_revert_head());
diff --git a/sequencer.c b/sequencer.c
index 
36e03d041f32bcc0fdd1fddebb33b23c7e4d8a70..ef262980c5255d90ee023c0b29c6c1c628b3c7d2
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,10 +1,10 @@
 #include "cache.h"
 #include "config.h"
 #include "lockfile.h"
-#include "sequencer.h"
 #include "dir.h"
 #include "object.h"
 #include "commit.h"
+#include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
 #include "exec_cmd.h"
@@ -752,6 +752,43 @@ int template_untouched(const struct strbuf *sb, const char 
*template_file,
return rest_is_empty(sb, start - sb->buf);
 }
 
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char *action, const struct strbuf *msg,
+   struct strbuf *err)
+{
+   struct ref_transaction *transaction;
+   struct strbuf sb = STRBUF_INIT;
+   const char *nl;
+   int ret = 0;
+
+   if (action) {
+   strbuf_addstr(, action);
+   strbuf_addstr(, ": ");
+   }
+
+   nl = strchr(msg->buf, '\n');
+   if (nl) {
+   strbuf_add(, msg->buf, nl + 1 - msg->buf);
+   } else {
+   strbuf_addbuf(, msg);
+   strbuf_addch(, '\n');
+   }
+
+   transaction = ref_transaction_begin(err);
+   if (!transaction ||
+   ref_transaction_update(transaction, "HEAD", new_head,
+  old_head ? _head->object.oid : _oid,
+  0, sb.buf, err) ||
+   ref_transaction_commit(transaction, err)) {
+   ret = -1;
+   }
+   ref_transaction_free(transaction);
+   strbuf_release();
+
+   return ret;
+}
+
 static int is_original_commit_empty(struct commit *commit)
 {
const struct object_id *ptree_oid;
diff --git a/sequencer.h b/sequencer.h
index 
82e57713a2940c5d65ccac013c3f42c55cc12baf..81a2098e900f0aca30e45ed7f19ae4bf3ce682f0
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -69,4 +69,8 @@ int message_is_empty(const struct strbuf *sb,
 enum commit_msg_cleanup_mode cleanup_mode);
 int template_untouched(const struct strbuf *sb, const char *template_file,
   enum commit_msg_cleanup_mode cleanup_mode);
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char 

[PATCH 5/6] grep: update boundary variable for pre-context

[René Scharfe]

Function context can be bigger than -A/-B/-C context.  To find the
beginning of the combined context we search backwards.  Currently we
check at each loop iteration what we're looking for and determine the
effective upper boundary based on that.

Simplify this a bit by setting the variable "from" to the lowest unshown
line number up front if we're looking for a function line and set it
back to the required -B/-C context line number when we find one.  This
prepares the ground for the next patch; no functional change intended.

Signed-off-by: Rene Scharfe 
---
 grep.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/grep.c b/grep.c
index d0b9b6cdfa..2c55d10c55 100644
--- a/grep.c
+++ b/grep.c
@@ -1479,20 +1479,21 @@ static void show_funcname_line(struct grep_opt *opt, 
struct grep_source *gs,
 static void show_pre_context(struct grep_opt *opt, struct grep_source *gs,
 char *bol, char *end, unsigned lno)
 {
-   unsigned cur = lno, from = 1, funcname_lno = 0;
+   unsigned cur = lno, from = 1, funcname_lno = 0, orig_from;
int funcname_needed = !!opt->funcname;
 
-   if (opt->funcbody && !match_funcname(opt, gs, bol, end))
-   funcname_needed = 2;
-
if (opt->pre_context < lno)
from = lno - opt->pre_context;
if (from <= opt->last_shown)
from = opt->last_shown + 1;
+   orig_from = from;
+   if (opt->funcbody && !match_funcname(opt, gs, bol, end)) {
+   funcname_needed = 1;
+   from = opt->last_shown + 1;
+   }
 
/* Rewind. */
-   while (bol > gs->buf &&
-  cur > (funcname_needed == 2 ? opt->last_shown + 1 : from)) {
+   while (bol > gs->buf && cur > from) {
char *eol = --bol;
 
while (bol > gs->buf && bol[-1] != '\n')
@@ -1501,6 +1502,7 @@ static void show_pre_context(struct grep_opt *opt, struct 
grep_source *gs,
if (funcname_needed && match_funcname(opt, gs, bol, eol)) {
funcname_lno = cur;
funcname_needed = 0;
+   from = orig_from;
}
}
 
-- 
2.15.0

[PATCH 3/4] branch: update warning message shown when copying a misnamed branch

[Kaartic Sivaraam]

When a user tries to rename a branch that has a "bad name" (e.g.,
starts with a '-') then we warn them that the misnamed branch has
been renamed "away". A similar message is shown when trying to create
a copy of a misnamed branch even though it doesn't remove the misnamed
branch. This is not correct and may confuse the user.

So, update the warning message shown to be more precise that only a copy
of the misnamed branch has been created. It's better to show the warning
message than not showing it at all as it makes the user aware of the
presence of a misnamed branch.

Signed-off-by: Kaartic Sivaraam <kaartic.sivar...@gmail.com>
---
 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4edef5baa..ca9d8abd0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -507,7 +507,7 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 
if (recovery) {
if (copy)
-   warning(_("Copied a misnamed branch '%s' away"),
+   warning(_("Created a copy of a misnamed branch '%s'"),
oldref.buf + 11);
else
warning(_("Renamed a misnamed branch '%s' away"),
-- 
2.15.0.291.g0d8980c5d

[PATCH v3 3/8] Add a function to update HEAD after creating a commit

[Phillip Wood]

From: Phillip Wood 

Add update_head_with_reflog() based on the code that updates HEAD
after committing in builtin/commit.c that can be called by 'git
commit' and other commands.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v2:
 - updated commit message to reflect the change in function name
 - style fixes

changes since v1:
 - rename update_head() to update_head_with_reflog()

 builtin/commit.c | 20 ++--
 sequencer.c  | 39 ++-
 sequencer.h  |  4 
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
d958c2eb2adc9a29dab29340ce9b56daea41fecd..eb144556bf37b7bf357bd976b94305171b4fd159
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1610,13 +1610,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
const char *index_file, *reflog_msg;
-   char *nl;
struct object_id oid;
struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
-   struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -1739,25 +1737,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(_ident);
free_commit_extra_headers(extra);
 
-   nl = strchr(sb.buf, '\n');
-   if (nl)
-   strbuf_setlen(, nl + 1 - sb.buf);
-   else
-   strbuf_addch(, '\n');
-   strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
-   strbuf_insert(, strlen(reflog_msg), ": ", 2);
-
-   transaction = ref_transaction_begin();
-   if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", ,
-  current_head
-  ? _head->object.oid : _oid,
-  0, sb.buf, ) ||
-   ref_transaction_commit(transaction, )) {
+   if (update_head_with_reflog(current_head, , reflog_msg, ,
+   )) {
rollback_index_files();
die("%s", err.buf);
}
-   ref_transaction_free(transaction);
 
unlink(git_path_cherry_pick_head());
unlink(git_path_revert_head());
diff --git a/sequencer.c b/sequencer.c
index 
36e03d041f32bcc0fdd1fddebb33b23c7e4d8a70..ef262980c5255d90ee023c0b29c6c1c628b3c7d2
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,10 +1,10 @@
 #include "cache.h"
 #include "config.h"
 #include "lockfile.h"
-#include "sequencer.h"
 #include "dir.h"
 #include "object.h"
 #include "commit.h"
+#include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
 #include "exec_cmd.h"
@@ -752,6 +752,43 @@ int template_untouched(const struct strbuf *sb, const char 
*template_file,
return rest_is_empty(sb, start - sb->buf);
 }
 
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char *action, const struct strbuf *msg,
+   struct strbuf *err)
+{
+   struct ref_transaction *transaction;
+   struct strbuf sb = STRBUF_INIT;
+   const char *nl;
+   int ret = 0;
+
+   if (action) {
+   strbuf_addstr(, action);
+   strbuf_addstr(, ": ");
+   }
+
+   nl = strchr(msg->buf, '\n');
+   if (nl) {
+   strbuf_add(, msg->buf, nl + 1 - msg->buf);
+   } else {
+   strbuf_addbuf(, msg);
+   strbuf_addch(, '\n');
+   }
+
+   transaction = ref_transaction_begin(err);
+   if (!transaction ||
+   ref_transaction_update(transaction, "HEAD", new_head,
+  old_head ? _head->object.oid : _oid,
+  0, sb.buf, err) ||
+   ref_transaction_commit(transaction, err)) {
+   ret = -1;
+   }
+   ref_transaction_free(transaction);
+   strbuf_release();
+
+   return ret;
+}
+
 static int is_original_commit_empty(struct commit *commit)
 {
const struct object_id *ptree_oid;
diff --git a/sequencer.h b/sequencer.h
index 
82e57713a2940c5d65ccac013c3f42c55cc12baf..81a2098e900f0aca30e45ed7f19ae4bf3ce682f0
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -69,4 +69,8 @@ int message_is_empty(const struct strbuf *sb,
 enum commit_msg_cleanup_mode cleanup_mode);
 int template_untouched(const struct strbuf *sb, const char *template_file,
   enum commit_msg_cleanup_mode cleanup_mode);
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char 

[PATCH] apply: update line lengths for --inaccurate-eof

[René Scharfe]

Some diff implementations don't report missing newlines at the end of
files.  Applying such a patch can cause a newline character to be
added inadvertently.  The option --inaccurate-eof of git apply can be
used to remove trailing newlines if needed.

apply_one_fragment() cuts it off from the buffers for preimage and
postimage.  Before it does, it builds an array with the lengths of each
line for both.  Make sure to update the length of the last line in
these line info structures as well to keep them consistent with their
respective buffer.

Without this fix the added test fails; git apply dies and reports:

   fatal: BUG: caller miscounted postlen: asked 1, orig = 1, used = 2

That sanity check is only called if whitespace changes are ignored.

Reported-by: Mahmoud Al-Qudsi <mqu...@neosmart.net>
Signed-off-by: Rene Scharfe <l@web.de>
---
 apply.c|  2 ++
 t/t4107-apply-ignore-whitespace.sh | 14 ++
 2 files changed, 16 insertions(+)

diff --git a/apply.c b/apply.c
index b8087bd29c..321a9fa68d 100644
--- a/apply.c
+++ b/apply.c
@@ -2953,6 +2953,8 @@ static int apply_one_fragment(struct apply_state *state,
newlines.len > 0 && newlines.buf[newlines.len - 1] == '\n') {
old--;
strbuf_setlen(, newlines.len - 1);
+   preimage.line_allocated[preimage.nr - 1].len--;
+   postimage.line_allocated[postimage.nr - 1].len--;
}
 
leading = frag->leading;
diff --git a/t/t4107-apply-ignore-whitespace.sh 
b/t/t4107-apply-ignore-whitespace.sh
index 9e29b5262d..ac72eeaf27 100755
--- a/t/t4107-apply-ignore-whitespace.sh
+++ b/t/t4107-apply-ignore-whitespace.sh
@@ -178,4 +178,18 @@ test_expect_success 'patch5 fails 
(--no-ignore-whitespace)' '
test_must_fail git apply --no-ignore-whitespace patch5.patch
 '
 
+test_expect_success 'apply --ignore-space-change --inaccurate-eof' '
+   echo 1 >file &&
+   git apply --ignore-space-change --inaccurate-eof <<-\EOF &&
+   diff --git a/file b/file
+   --- a/file
+   +++ b/file
+   @@ -1 +1 @@
+   -1
+   +2
+   EOF
+   printf 2 >expect &&
+   test_cmp expect file
+'
+
 test_done
-- 
2.15.0

Re: [PATCH v2 3/9] Add a function to update HEAD after creating a commit

[Phillip Wood]

On 10/11/17 18:36, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> From: Phillip Wood 
>>
>> Add update_head() based on the code that updates HEAD after committing
>> in builtin/commit.c that can be called by 'git commit' and other
>> commands.
>>
>> Signed-off-by: Phillip Wood 
>> ---
>>
>> Notes:
>> changes since v1:
>>  - rename update_head() to update_head_with_reflog()
> 
> The above proposed log message still calls it with the old name
> though.
> 
Oops, sorry I'll fix that

Re: [PATCH v2 3/9] Add a function to update HEAD after creating a commit

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> Add update_head() based on the code that updates HEAD after committing
> in builtin/commit.c that can be called by 'git commit' and other
> commands.
>
> Signed-off-by: Phillip Wood 
> ---
>
> Notes:
> changes since v1:
>  - rename update_head() to update_head_with_reflog()

The above proposed log message still calls it with the old name
though.

[PATCH v2 3/9] Add a function to update HEAD after creating a commit

[Phillip Wood]

From: Phillip Wood 

Add update_head() based on the code that updates HEAD after committing
in builtin/commit.c that can be called by 'git commit' and other
commands.

Signed-off-by: Phillip Wood 
---

Notes:
changes since v1:
 - rename update_head() to update_head_with_reflog()

 builtin/commit.c | 20 ++--
 sequencer.c  | 39 ++-
 sequencer.h  |  4 
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
dbc160c525e7a9249b7c7df2180495a4c7102296..7c2816644a665f7b7d8f4b6c0a5855aef01b
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1591,13 +1591,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
const char *index_file, *reflog_msg;
-   char *nl;
struct object_id oid;
struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
-   struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -1720,25 +1718,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(_ident);
free_commit_extra_headers(extra);
 
-   nl = strchr(sb.buf, '\n');
-   if (nl)
-   strbuf_setlen(, nl + 1 - sb.buf);
-   else
-   strbuf_addch(, '\n');
-   strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
-   strbuf_insert(, strlen(reflog_msg), ": ", 2);
-
-   transaction = ref_transaction_begin();
-   if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", ,
-  current_head
-  ? _head->object.oid : _oid,
-  0, sb.buf, ) ||
-   ref_transaction_commit(transaction, )) {
+   if (update_head_with_reflog(current_head, , reflog_msg, ,
+   )) {
rollback_index_files();
die("%s", err.buf);
}
-   ref_transaction_free(transaction);
 
unlink(git_path_cherry_pick_head());
unlink(git_path_revert_head());
diff --git a/sequencer.c b/sequencer.c
index 
23c250f16cfb7620215bb9c99b8e12d726dc9191..fcd8e92531a3fb1f0bc1294925e87b3624ada909
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,10 +1,10 @@
 #include "cache.h"
 #include "config.h"
 #include "lockfile.h"
-#include "sequencer.h"
 #include "dir.h"
 #include "object.h"
 #include "commit.h"
+#include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
 #include "exec_cmd.h"
@@ -752,6 +752,43 @@ int template_untouched(const struct strbuf *sb, const char 
*template_file,
return rest_is_empty(sb, start - sb->buf);
 }
 
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char *action, const struct strbuf *msg,
+   struct strbuf *err)
+{
+   struct ref_transaction *transaction;
+   struct strbuf sb = STRBUF_INIT;
+   const char *nl;
+   int ret = 0;
+
+   if (action) {
+   strbuf_addstr(, action);
+   strbuf_addstr(, ": ");
+   }
+
+   nl = strchr(msg->buf, '\n');
+   if (nl) {
+   strbuf_add(, msg->buf, nl + 1 - msg->buf);
+   } else {
+   strbuf_addbuf(, msg);
+   strbuf_addch(, '\n');
+   }
+
+   transaction = ref_transaction_begin(err);
+   if (!transaction ||
+   ref_transaction_update(transaction, "HEAD", new_head,
+  old_head ? _head->object.oid : _oid,
+  0, sb.buf, err) ||
+   ref_transaction_commit(transaction, err)) {
+   ret = -1;
+   }
+   ref_transaction_free(transaction);
+   strbuf_release();
+
+   return ret;
+}
+
 static int is_original_commit_empty(struct commit *commit)
 {
const struct object_id *ptree_oid;
diff --git a/sequencer.h b/sequencer.h
index 
82e57713a2940c5d65ccac013c3f42c55cc12baf..82035ced129d35ff8ba64710477531a9a713b631
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -69,4 +69,8 @@ int message_is_empty(const struct strbuf *sb,
 enum commit_msg_cleanup_mode cleanup_mode);
 int template_untouched(const struct strbuf *sb, const char *template_file,
   enum commit_msg_cleanup_mode cleanup_mode);
+int update_head_with_reflog(const struct commit *old_head,
+   const struct object_id *new_head,
+   const char* action, const struct strbuf *msg,
+   struct strbuf *err);
 #endif
-- 
2.15.0

[PATCH v1 2/4] update-index: add fastindex support to update-index

[Ben Peart]

Add support in update-index to manually add/remove the fastindex
extension via --[no-]fastindex flags.

Signed-off-by: Ben Peart <benpe...@microsoft.com>
---
 builtin/update-index.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index fefbe60167..63b7f18807 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -917,6 +917,7 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
struct refresh_params refresh_args = {0, _errors};
int lock_error = 0;
int split_index = -1;
+   int fast_index = -1;
struct lock_file lock_file = LOCK_INIT;
struct parse_opt_ctx_t ctx;
strbuf_getline_fn getline_fn;
@@ -1008,6 +1009,8 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
N_("test if the filesystem supports untracked 
cache"), UC_TEST),
OPT_SET_INT(0, "force-untracked-cache", _cache,
N_("enable untracked cache without testing the 
filesystem"), UC_FORCE),
+   OPT_BOOL(0, "fastindex", _index,
+   N_("enable or disable fast index parsing")),
OPT_END()
};
 
@@ -1146,6 +1149,25 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
die("BUG: bad untracked_cache value: %d", untracked_cache);
}
 
+   if (fast_index > 0) {
+   if (git_config_get_fast_index() == 0)
+   warning(_("core.fastindex is unset; "
+   "set it if you really want to "
+   "enable fastindex"));
+   core_fast_index = 1;
+   active_cache_changed |= SOMETHING_CHANGED;
+   report(_("fastindex enabled"));
+   }
+   else if (!fast_index) {
+   if (git_config_get_fast_index() == 1)
+   warning(_("core.fastindex is set; "
+   "remove it if you really want to "
+   "disable fastindex"));
+   core_fast_index = 0;
+   active_cache_changed |= SOMETHING_CHANGED;
+   report(_("fastindex disabled"));
+   }
+
if (active_cache_changed) {
if (newfd < 0) {
if (refresh_args.flags & REFRESH_QUIET)
-- 
2.15.0.windows.1

Re: [PATCH v1 2/8] Add a function to update HEAD after creating a commit

[Phillip Wood]

On 07/11/17 03:02, Johannes Schindelin wrote:
> Hi Junio,
> 
> On Tue, 7 Nov 2017, Junio C Hamano wrote:
> 
>> Phillip Wood  writes:
>>
>>> @@ -751,6 +751,42 @@ int template_untouched(const struct strbuf *sb, const 
>>> char *template_file,
>>> return rest_is_empty(sb, start - sb->buf);
>>>  }
>>>  
>>> +int update_head(const struct commit *old_head, const struct object_id 
>>> *new_head,
>>> +   const char *action, const struct strbuf *msg,
>>> +   struct strbuf *err)
>>> +{
>>
>> [...]
>>
>> I however do not think update_head() is such a good name for a
>> helper function in the global scope.  builtin/clone.c has a static
>> one that has quite different semantics with the same name (I am not
>> saying that builtin/clone.c will in the future start including the
>> sequencer.h header file; I am pointing out that update_head() is not
>> a good global name that will be understood by everybody).

Good point, I'll go with the name Dscho suggests if that's OK with you.

> Please try to always accompany a "Don't Do That" by a "How About This
> Instead".
> 
> In this case, I could imagine that `update_head_with_reflog()` would be a
> better name. If you disagree, I invite you to propose an alternative that
> strikes your liking.
> 
> Ciao,
> Dscho
> 

Re: [PATCH v1 2/8] Add a function to update HEAD after creating a commit

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Nov 2017, Junio C Hamano wrote:

> Phillip Wood  writes:
> 
> > @@ -751,6 +751,42 @@ int template_untouched(const struct strbuf *sb, const 
> > char *template_file,
> > return rest_is_empty(sb, start - sb->buf);
> >  }
> >  
> > +int update_head(const struct commit *old_head, const struct object_id 
> > *new_head,
> > +   const char *action, const struct strbuf *msg,
> > +   struct strbuf *err)
> > +{
> 
> [...]
>
> I however do not think update_head() is such a good name for a
> helper function in the global scope.  builtin/clone.c has a static
> one that has quite different semantics with the same name (I am not
> saying that builtin/clone.c will in the future start including the
> sequencer.h header file; I am pointing out that update_head() is not
> a good global name that will be understood by everybody).

Please try to always accompany a "Don't Do That" by a "How About This
Instead".

In this case, I could imagine that `update_head_with_reflog()` would be a
better name. If you disagree, I invite you to propose an alternative that
strikes your liking.

Ciao,
Dscho

Re: [PATCH v1 2/8] Add a function to update HEAD after creating a commit

[Junio C Hamano]

Phillip Wood  writes:

> @@ -1735,25 +1733,10 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>   strbuf_release(_ident);
>   free_commit_extra_headers(extra);
>  
> - nl = strchr(sb.buf, '\n');
> - if (nl)
> - strbuf_setlen(, nl + 1 - sb.buf);
> - else
> - strbuf_addch(, '\n');
> - strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
> - strbuf_insert(, strlen(reflog_msg), ": ", 2);

The old code treated sb (which has the log message we gave to
commit_tree_extended() to create the commit) as expendable at this
point and (1) truncated it to the title line, and (2) prepended the
reflog action prefix, so that it can pass it to the ref transaction
code to use it as the reflog message.

Which was quite ugly X-<.

> - transaction = ref_transaction_begin();
> - if (!transaction ||
> - ref_transaction_update(transaction, "HEAD", ,
> -current_head
> -? _head->object.oid : _oid,
> -0, sb.buf, ) ||
> - ref_transaction_commit(transaction, )) {
> + if (update_head(current_head, , reflog_msg, , )) {
>   rollback_index_files();
>   die("%s", err.buf);
>   }

> @@ -751,6 +751,42 @@ int template_untouched(const struct strbuf *sb, const 
> char *template_file,
>   return rest_is_empty(sb, start - sb->buf);
>  }
>  
> +int update_head(const struct commit *old_head, const struct object_id 
> *new_head,
> + const char *action, const struct strbuf *msg,
> + struct strbuf *err)
> +{
> + struct ref_transaction *transaction;
> + struct strbuf sb = STRBUF_INIT;

It no longer is necessary to call this variable "sb"; the original
had a single instance of strbuf that was reused for different
purposes and could not give it a more specific name, but we can
afford to call this one reflog_message or something.

> + const char *nl;
> + int ret = 0;
> +
> + if (action) {
> + strbuf_addstr(, action);
> + strbuf_addstr(, ": ");
> + }
> +
> + nl = strchr(msg->buf, '\n');
> + if (nl) {
> + strbuf_add(, msg->buf, nl + 1 - msg->buf);
> + } else {
> + strbuf_addbuf(, msg);
> + strbuf_addch(, '\n');
> + }

The updated code is a lot more natural and straight-forward.  I
quite like it.

I however do not think update_head() is such a good name for a
helper function in the global scope.  builtin/clone.c has a static
one that has quite different semantics with the same name (I am not
saying that builtin/clone.c will in the future start including the
sequencer.h header file; I am pointing out that update_head() is not
a good global name that will be understood by everybody).

> diff --git a/sequencer.h b/sequencer.h
> index 
> 65a4b0c25185d7ad5115035abb766d1b95df9a62..1db06caea35bed556dfaabca1c6be8a80857ed5e
>  100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -68,4 +68,7 @@ enum cleanup_mode {
>  int message_is_empty(const struct strbuf *sb, enum cleanup_mode 
> cleanup_mode);
>  int template_untouched(const struct strbuf *sb, const char *template_file,
>  enum cleanup_mode cleanup_mode);
> +int update_head(const struct commit *old_head, const struct object_id 
> *new_head,
> + const char* action, const struct strbuf *msg,
> + struct strbuf *err);
>  #endif

[PATCH v1 2/8] Add a function to update HEAD after creating a commit

[Phillip Wood]

From: Phillip Wood 

Add update_head() based on the code that updates HEAD after committing
in builtin/commit.c that can be called by 'git commit' and other
commands.

Signed-off-by: Phillip Wood 
---
 builtin/commit.c | 19 +--
 sequencer.c  | 38 +-
 sequencer.h  |  3 +++
 3 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 
fab512b668af07a1fa927f713eca71c9f783b422..e82754f1fb9f80b8e5b83546d2b2b010daaba522
 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1606,13 +1606,11 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
const char *index_file, *reflog_msg;
-   char *nl;
struct object_id oid;
struct commit_list *parents = NULL;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
-   struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
if (argc == 2 && !strcmp(argv[1], "-h"))
@@ -1735,25 +1733,10 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
strbuf_release(_ident);
free_commit_extra_headers(extra);
 
-   nl = strchr(sb.buf, '\n');
-   if (nl)
-   strbuf_setlen(, nl + 1 - sb.buf);
-   else
-   strbuf_addch(, '\n');
-   strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
-   strbuf_insert(, strlen(reflog_msg), ": ", 2);
-
-   transaction = ref_transaction_begin();
-   if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", ,
-  current_head
-  ? _head->object.oid : _oid,
-  0, sb.buf, ) ||
-   ref_transaction_commit(transaction, )) {
+   if (update_head(current_head, , reflog_msg, , )) {
rollback_index_files();
die("%s", err.buf);
}
-   ref_transaction_free(transaction);
 
unlink(git_path_cherry_pick_head());
unlink(git_path_revert_head());
diff --git a/sequencer.c b/sequencer.c
index 
f4a04c913c0d60adbf78d68ca87db739c8e3a280..a85ad659114248b9b5215641cd2911bc4d02e4df
 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,10 +1,10 @@
 #include "cache.h"
 #include "config.h"
 #include "lockfile.h"
-#include "sequencer.h"
 #include "dir.h"
 #include "object.h"
 #include "commit.h"
+#include "sequencer.h"
 #include "tag.h"
 #include "run-command.h"
 #include "exec_cmd.h"
@@ -751,6 +751,42 @@ int template_untouched(const struct strbuf *sb, const char 
*template_file,
return rest_is_empty(sb, start - sb->buf);
 }
 
+int update_head(const struct commit *old_head, const struct object_id 
*new_head,
+   const char *action, const struct strbuf *msg,
+   struct strbuf *err)
+{
+   struct ref_transaction *transaction;
+   struct strbuf sb = STRBUF_INIT;
+   const char *nl;
+   int ret = 0;
+
+   if (action) {
+   strbuf_addstr(, action);
+   strbuf_addstr(, ": ");
+   }
+
+   nl = strchr(msg->buf, '\n');
+   if (nl) {
+   strbuf_add(, msg->buf, nl + 1 - msg->buf);
+   } else {
+   strbuf_addbuf(, msg);
+   strbuf_addch(, '\n');
+   }
+
+   transaction = ref_transaction_begin(err);
+   if (!transaction ||
+   ref_transaction_update(transaction, "HEAD", new_head,
+  old_head ? _head->object.oid : _oid,
+  0, sb.buf, err) ||
+   ref_transaction_commit(transaction, err)) {
+   ret = -1;
+   }
+   ref_transaction_free(transaction);
+   strbuf_release();
+
+   return ret;
+}
+
 static int is_original_commit_empty(struct commit *commit)
 {
const struct object_id *ptree_oid;
diff --git a/sequencer.h b/sequencer.h
index 
65a4b0c25185d7ad5115035abb766d1b95df9a62..1db06caea35bed556dfaabca1c6be8a80857ed5e
 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -68,4 +68,7 @@ enum cleanup_mode {
 int message_is_empty(const struct strbuf *sb, enum cleanup_mode cleanup_mode);
 int template_untouched(const struct strbuf *sb, const char *template_file,
   enum cleanup_mode cleanup_mode);
+int update_head(const struct commit *old_head, const struct object_id 
*new_head,
+   const char* action, const struct strbuf *msg,
+   struct strbuf *err);
 #endif
-- 
2.14.3

[PATCH v2 9/9] refs: update some more docs to use "oid" rather than "sha1"

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhag...@alum.mit.edu>
---
 refs.c|  2 +-
 refs.h|  8 
 refs/files-backend.c  | 19 +--
 refs/packed-backend.c |  2 +-
 refs/ref-cache.c  |  4 ++--
 refs/refs-internal.h  | 18 +-
 6 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/refs.c b/refs.c
index 0d9a1348cd..339d4318ee 100644
--- a/refs.c
+++ b/refs.c
@@ -770,7 +770,7 @@ static int read_ref_at_ent(struct object_id *ooid, struct 
object_id *noid,
if (cb->cutoff_cnt)
*cb->cutoff_cnt = cb->reccnt - 1;
/*
-* we have not yet updated cb->[n|o]sha1 so they still
+* we have not yet updated cb->[n|o]oid so they still
 * hold the values for the previous record.
 */
if (!is_null_oid(>ooid)) {
diff --git a/refs.h b/refs.h
index d396012367..18582a408c 100644
--- a/refs.h
+++ b/refs.h
@@ -126,7 +126,7 @@ int peel_ref(const char *refname, struct object_id *oid);
 /**
  * Resolve refname in the nested "gitlink" repository in the specified
  * submodule (which must be non-NULL). If the resolution is
- * successful, return 0 and set sha1 to the name of the object;
+ * successful, return 0 and set oid to the name of the object;
  * otherwise, return a non-zero value.
  */
 int resolve_gitlink_ref(const char *submodule, const char *refname,
@@ -260,7 +260,7 @@ struct ref_transaction;
 
 /*
  * The signature for the callback function for the for_each_*()
- * functions below.  The memory pointed to by the refname and sha1
+ * functions below.  The memory pointed to by the refname and oid
  * arguments is only guaranteed to be valid for the duration of a
  * single callback invocation.
  */
@@ -354,7 +354,7 @@ int reflog_exists(const char *refname);
 
 /*
  * Delete the specified reference. If old_oid is non-NULL, then
- * verify that the current value of the reference is old_sha1 before
+ * verify that the current value of the reference is old_oid before
  * deleting it. If old_oid is NULL, delete the reference if it
  * exists, regardless of its old value. It is an error for old_oid to
  * be null_oid. msg and flags are passed through to
@@ -633,7 +633,7 @@ int ref_transaction_abort(struct ref_transaction 
*transaction,
  * It is a bug to call this function when there might be other
  * processes accessing the repository or if there are existing
  * references that might conflict with the ones being created. All
- * old_sha1 values must either be absent or NULL_SHA1.
+ * old_oid values must either be absent or null_oid.
  */
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bb10b715a8..2298f900dd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -240,7 +240,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
} else if (is_null_oid()) {
/*
 * It is so astronomically unlikely
-* that NULL_SHA1 is the SHA-1 of an
+* that null_oid is the OID of an
 * actual object that we consider its
 * appearance in a loose reference
 * file to be repo corruption
@@ -473,7 +473,7 @@ static void unlock_ref(struct ref_lock *lock)
  * are passed to refs_verify_refname_available() for this check.
  *
  * If mustexist is not set and the reference is not found or is
- * broken, lock the reference anyway but clear sha1.
+ * broken, lock the reference anyway but clear old_oid.
  *
  * Return 0 on success. On failure, write an error message to err and
  * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.
@@ -1648,9 +1648,8 @@ static int files_log_ref_write(struct files_ref_store 
*refs,
 }
 
 /*
- * Write sha1 into the open lockfile, then close the lockfile. On
- * errors, rollback the lockfile, fill in *err and
- * return -1.
+ * Write oid into the open lockfile, then close the lockfile. On
+ * errors, rollback the lockfile, fill in *err and return -1.
  */
 static int write_ref_to_lockfile(struct ref_lock *lock,
 const struct object_id *oid, struct strbuf 
*err)
@@ -2272,7 +2271,7 @@ static int split_symref_update(struct files_ref_store 
*refs,
 
/*
     * Change the symbolic ref update to log only. Also, it
-* doesn't need to check its old SHA-1 value, as that will be
+* doesn't need to check its old OID value, as that will be
 * done when new_update is processed.
 */
update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
@@ -2341,7 +2340,7 @@ static int check_old_oid(struct ref_update *update, 
struct object_

Re: [RFC PATCH 2/8] commit: move code to update HEAD to libgit

[Junio C Hamano]

Phillip Wood  writes:

>> I suspect that it would be sufficient to make update_head() helper
>> function take the reflog action message as another parameter
>> instead to fix the above, but there may be other reasons why you
>> chose to do it this way---I cannot read it in your empty log
>> message, though.
>
> Good point, sorry I should have added some explanation about that. I
> went with using setenv() rather than passing a reflog message to
> update_head() as it meant there were no changes needed on the sequencer
> side as it already sets GIT_REFLOG_ACTION. As the sequencer already sets
> GIT_REFLOG_ACTION, and git-commit does not fork any subprocesses I don't

Doesn't "git commit" run number of hooks?  Is it just the current
code does not run any hooks (by chance) after these new setenv()
calls are made and we happen to be safe?

Re: [RFC PATCH 2/8] commit: move code to update HEAD to libgit

[Phillip Wood]

On 07/10/17 10:54, Junio C Hamano wrote:
> Phillip Wood <phillip.w...@talktalk.net> writes:
> 
>> From: Phillip Wood <phillip.w...@dunelm.org.uk>
>>
>> Signed-off-by: Phillip Wood <phillip.w...@dunelm.org.uk>
>> ---
> 
> This seems to do a lot more than just moving code, most notably, it
> uses setenv() to affect what happens in any subprocesses we may
> spawn, and it is unclear if it was verified that this patch is free
> of unwanted consequences due to that change (and any others I may
> have missed while reading this patch, if any).
>
> I suspect that it would be sufficient to make update_head() helper
> function take the reflog action message as another parameter
> instead to fix the above, but there may be other reasons why you
> chose to do it this way---I cannot read it in your empty log
> message, though.

Good point, sorry I should have added some explanation about that. I
went with using setenv() rather than passing a reflog message to
update_head() as it meant there were no changes needed on the sequencer
side as it already sets GIT_REFLOG_ACTION. As the sequencer already sets
GIT_REFLOG_ACTION, and git-commit does not fork any subprocesses I don't
think this change has any unwanted consequences (I pushed a branch to
github before submitting the patches and the test suite passes on
travis). It would however be clearer to add a parameter to update_head()
for the reflog message as you suggested.

> 
> I will not give line-by-line style nitpick but in general we do not
> leave a SP between function name and the open parenthesis that
> starts its argument list.  New code in this patch seems to use
> mixture of styles.

Sorry I should have spotted those before I posted this series, I go
though all the patches and fix them (this would be a good opportunity
for me to try using git-clang-format from next)

Thanks for looking at this, did you have time to look at the other
changes in this series or did this patch put you off looking further?
I'll update and repost probably towards the end of next week. If I
continue to base these patches on master then I think the patch that
moves the code to print the commit summary will have (trivial) conflicts
with the changes in ao/check-resolve-ref-unsafe-result in pu do you want
the new patches based pu or are you happy with them based on master?

Best Wishes

Phillip

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 
>> 0b8c1ef6f57cfed328d12255e6834adb4bda4137..497778ba2c02afdd4a337969a27ca781e8389040
>>  100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1578,13 +1578,11 @@ int cmd_commit(int argc, const char **argv, const 
>> char *prefix)
>>  struct strbuf sb = STRBUF_INIT;
>>  struct strbuf author_ident = STRBUF_INIT;
>>  const char *index_file, *reflog_msg;
>> -char *nl;
>>  struct object_id oid;
>>  struct commit_list *parents = NULL;
>>  struct stat statbuf;
>>  struct commit *current_head = NULL;
>>  struct commit_extra_header *extra = NULL;
>> -struct ref_transaction *transaction;
>>  struct strbuf err = STRBUF_INIT;
>>  
>>  if (argc == 2 && !strcmp(argv[1], "-h"))
>> @@ -1625,10 +1623,10 @@ int cmd_commit(int argc, const char **argv, const 
>> char *prefix)
>>  reflog_msg = getenv("GIT_REFLOG_ACTION");
>>  if (!current_head) {
>>  if (!reflog_msg)
>> -reflog_msg = "commit (initial)";
>> +setenv ("GIT_REFLOG_ACTION", "commit (initial)", 1);
>>  } else if (amend) {
>>  if (!reflog_msg)
>> -reflog_msg = "commit (amend)";
>> +setenv("GIT_REFLOG_ACTION", "commit (amend)", 1);
>>  parents = copy_commit_list(current_head->parents);
>>  } else if (whence == FROM_MERGE) {
>>  struct strbuf m = STRBUF_INIT;
>> @@ -1637,7 +1635,7 @@ int cmd_commit(int argc, const char **argv, const char 
>> *prefix)
>>  struct commit_list **pptr = 
>>  
>>  if (!reflog_msg)
>> -reflog_msg = "commit (merge)";
>> +setenv("GIT_REFLOG_ACTION", "commit (merge)", 1);
>>  pptr = commit_list_append(current_head, pptr);
>>  fp = xfopen(git_path_merge_head(), "r");
>>  while (strbuf_getline_lf(, fp) != EOF) {
>> @@ -1660,9 +1658,9 @@ int cmd_commit(int argc, const char **argv, const char 
>> *prefix)
>>  parents = red

[PATCH v3 06/25] refs: update ref transactions to use struct object_id

[brian m. carlson]

Update the ref transaction code to use struct object_id.  Remove one
NULL pointer check which was previously inserted around a dereference;
since we now pass a pointer to struct object_id directly through, the
code we're calling handles this for us.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 branch.c   |  2 +-
 builtin/clone.c|  2 +-
 builtin/commit.c   |  4 ++--
 builtin/fetch.c|  4 ++--
 builtin/receive-pack.c |  4 ++--
 builtin/replace.c  |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-ref.c   |  8 
 fast-import.c  |  4 ++--
 refs.c | 50 --
 refs.h | 38 +++---
 refs/files-backend.c   | 12 ++--
 refs/refs-internal.h   |  4 ++--
 sequencer.c|  2 +-
 walker.c   |  2 +-
 15 files changed, 69 insertions(+), 71 deletions(-)

diff --git a/branch.c b/branch.c
index 4377ce2fb1..45029ea142 100644
--- a/branch.c
+++ b/branch.c
@@ -305,7 +305,7 @@ void create_branch(const char *name, const char *start_name,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref.buf,
-  oid.hash, forcing ? NULL : null_sha1,
+  , forcing ? NULL : _oid,
   0, msg, ) ||
ref_transaction_commit(transaction, ))
die("%s", err.buf);
diff --git a/builtin/clone.c b/builtin/clone.c
index 4135621aa3..665a0e2673 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -588,7 +588,7 @@ static void write_remote_refs(const struct ref *local_refs)
for (r = local_refs; r; r = r->next) {
if (!r->peer_ref)
continue;
-   if (ref_transaction_create(t, r->peer_ref->name, 
r->old_oid.hash,
+   if (ref_transaction_create(t, r->peer_ref->name, >old_oid,
   0, NULL, ))
die("%s", err.buf);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index d75b3805ea..f9c9676a3f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1788,9 +1788,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 
transaction = ref_transaction_begin();
if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", oid.hash,
+   ref_transaction_update(transaction, "HEAD", ,
   current_head
-  ? current_head->object.oid.hash : null_sha1,
+  ? _head->object.oid : _oid,
   0, sb.buf, ) ||
ref_transaction_commit(transaction, )) {
rollback_index_files();
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 225c734924..859be91d6c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -457,8 +457,8 @@ static int s_update_ref(const char *action,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref->name,
-  ref->new_oid.hash,
-  check_old ? ref->old_oid.hash : NULL,
+  >new_oid,
+  check_old ? >old_oid : NULL,
   0, msg, ))
goto fail;
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cc48767405..2da3c4cd5c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1139,7 +1139,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
if (ref_transaction_delete(transaction,
   namespaced_name,
-  old_oid ? old_oid->hash : NULL,
+  old_oid,
   0, "push", )) {
rp_error("%s", err.buf);
strbuf_release();
@@ -1156,7 +1156,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
 
if (ref_transaction_update(transaction,
   namespaced_name,
-  new_oid->hash, old_oid->hash,
+  new_oid, old_oid,
   0, "push",
   )) {
rp_error("%s", err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 2854eaa0f3..3099e55307 100644
--- a/builtin/replace.c
+++

Re: [PATCH v2 2/5] Update documentation for new directory and status logic

[Junio C Hamano]

Jameson Miller  writes:

>>> +If this is set, files and directories that explicity match an ignore
>>> +pattern are reported. Implicity ignored directories (directories that
>>> +do not match an ignore pattern, but whose contents are all ignored)
>>> +are not reported, instead all of the contents are reported.
>> Makes me wonder if DIR_SHOW_IGNORED* should be splt out into a short
>> enum.  We have:
>>
>>   - Do not show ignored ones (0)
>>
>>   - Collect ignored ones (DIR_SHOW_IGNORED)
>>
>>   - Collect ignored and untracked ones separately (DIR_SHOW_IGNORED_TOO)
>>
>>   - Collect ignored and duntracked ones separately, but limit them to
>> those mach exclude patterns explicitly 
>> (DIR_SHOW_IGNORED_TOO|...MODE_MATCHING)
>>
>> so we need two bits to fit a 4-possiblity enum.
>>
>> Then we do not have to worry about saying quirky things like A and B
>> are incompatible, and C makes sense only when B is set, etc.
> I could see a potential for other values for the "show ignored
> mode" flags - for example: "NORMAL", "MATCHING", "ALL"... Instead
> of making more change at this point in time, how would you feel
> about waiting until the next change in this area.
>
> If you would prefer for me to change these enums now, I can do
> that.

"Makes me wonder" was just that.  I was made to wonder.  I did not
have strong opinions either way.  You thought about the area of this
code longer than I did, so I do not mind you picking the course of
action that is best for the project, and if you think it is better
to wait until we know more about the vocabulary we want to support
before we restructure these flags, that is fine by me.

Thanks.

Re: [PATCH v2 2/5] Update documentation for new directory and status logic

[Jameson Miller]

On 10/11/2017 10:55 PM, Junio C Hamano wrote:

Jameson Miller  writes:


Signed-off-by: Jameson Miller 
---
  Documentation/git-status.txt  | 21 +-
  Documentation/technical/api-directory-listing.txt | 27 +++
  2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a36c..fc282e0a92 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -97,8 +97,27 @@ configuration variable documented in linkgit:git-config[1].
(and suppresses the output of submodule summaries when the config option
`status.submoduleSummary` is set).
  
---ignored::

+--ignored[=]::
Show ignored files as well.
++
+The mode parameter is used to specify the handling of ignored files.
+It is optional: it defaults to 'traditional'.
++
+The possible options are:
++
+   - 'traditional' - Shows ignored files and directories, unless
+ --untracked-files=all is specifed, in which case
+ individual files in ignored directories are
+ displayed.
+   - 'no'  - Show no ignored files.
+   - 'matching'- Shows ignored files and directories matching an
+ ignore pattern.
++
+When 'matching' mode is specified, paths that explicity match an
+ignored pattern are shown. If a directory matches an ignore pattern,
+then it is shown, but not paths contained in the ignored directory. If
+a directory does not match an ignore pattern, but all contents are
+ignored, then the directory is not shown, but all contents are shown.

Well explained.


diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 6c77b4920c..7fae00f44f 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -22,16 +22,20 @@ The notable options are:
  
  `flags`::
  
-	A bit-field of options (the `*IGNORED*` flags are mutually exclusive):

+   A bit-field of options:
  
  `DIR_SHOW_IGNORED`:::
  
-	Return just ignored files in `entries[]`, not untracked files.

+   Return just ignored files in `entries[]`, not untracked
+   files. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED_TOO`.
  
  `DIR_SHOW_IGNORED_TOO`:::
  
-	Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`

-   in addition to untracked files in `entries[]`.
+   Similar to `DIR_SHOW_IGNORED`, but return ignored files in
+   `ignored[]` in addition to untracked files in
+   `entries[]`. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED`.
  
  `DIR_KEEP_UNTRACKED_CONTENTS`:::
  
@@ -39,6 +43,21 @@ The notable options are:

untracked contents of untracked directories are also returned in
`entries[]`.
  
+`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::

+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
+   this is set, returns ignored files and directories that match
+   an exclude pattern. If a directory matches an exclude pattern,
+   then the directory is returned and the contained paths are
+   not. A directory that does not match an exclude pattern will
+   not be returned even if all of its contents are ignored. In
+   this case, the contents are returned as individual entries.
++
+If this is set, files and directories that explicity match an ignore
+pattern are reported. Implicity ignored directories (directories that
+do not match an ignore pattern, but whose contents are all ignored)
+are not reported, instead all of the contents are reported.

Makes me wonder if DIR_SHOW_IGNORED* should be splt out into a short
enum.  We have:

  - Do not show ignored ones (0)

  - Collect ignored ones (DIR_SHOW_IGNORED)

  - Collect ignored and untracked ones separately (DIR_SHOW_IGNORED_TOO)

  - Collect ignored and duntracked ones separately, but limit them to
those mach exclude patterns explicitly 
(DIR_SHOW_IGNORED_TOO|...MODE_MATCHING)

so we need two bits to fit a 4-possiblity enum.

Then we do not have to worry about saying quirky things like A and B
are incompatible, and C makes sense only when B is set, etc.

I could see a potential for other values for the "show ignored
mode" flags - for example: "NORMAL", "MATCHING", "ALL"... Instead
of making more change at this point in time, how would you feel
about waiting until the next change in this area.

If you would prefer for me to change these enums now, I can do
that.




  `DIR_COLLECT_IGNORED`:::
  
  	Special mode for git-add. Return ignored files in `ignored[]` and

Re: [PATCH v2 2/5] Update documentation for new directory and status logic

[Junio C Hamano]

Jameson Miller  writes:

> Signed-off-by: Jameson Miller 
> ---
>  Documentation/git-status.txt  | 21 +-
>  Documentation/technical/api-directory-listing.txt | 27 
> +++
>  2 files changed, 43 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 9f3a78a36c..fc282e0a92 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -97,8 +97,27 @@ configuration variable documented in linkgit:git-config[1].
>   (and suppresses the output of submodule summaries when the config option
>   `status.submoduleSummary` is set).
>  
> ---ignored::
> +--ignored[=]::
>   Show ignored files as well.
> ++
> +The mode parameter is used to specify the handling of ignored files.
> +It is optional: it defaults to 'traditional'.
> ++
> +The possible options are:
> ++
> + - 'traditional' - Shows ignored files and directories, unless
> +   --untracked-files=all is specifed, in which case
> +   individual files in ignored directories are
> +   displayed.
> + - 'no'  - Show no ignored files.
> + - 'matching'- Shows ignored files and directories matching an
> +   ignore pattern.
> ++
> +When 'matching' mode is specified, paths that explicity match an
> +ignored pattern are shown. If a directory matches an ignore pattern,
> +then it is shown, but not paths contained in the ignored directory. If
> +a directory does not match an ignore pattern, but all contents are
> +ignored, then the directory is not shown, but all contents are shown.

Well explained.

> diff --git a/Documentation/technical/api-directory-listing.txt 
> b/Documentation/technical/api-directory-listing.txt
> index 6c77b4920c..7fae00f44f 100644
> --- a/Documentation/technical/api-directory-listing.txt
> +++ b/Documentation/technical/api-directory-listing.txt
> @@ -22,16 +22,20 @@ The notable options are:
>  
>  `flags`::
>  
> - A bit-field of options (the `*IGNORED*` flags are mutually exclusive):
> + A bit-field of options:
>  
>  `DIR_SHOW_IGNORED`:::
>  
> - Return just ignored files in `entries[]`, not untracked files.
> + Return just ignored files in `entries[]`, not untracked
> + files. This flag is mutually exclusive with
> + `DIR_SHOW_IGNORED_TOO`.
>  
>  `DIR_SHOW_IGNORED_TOO`:::
>  
> - Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
> - in addition to untracked files in `entries[]`.
> + Similar to `DIR_SHOW_IGNORED`, but return ignored files in
> + `ignored[]` in addition to untracked files in
> + `entries[]`. This flag is mutually exclusive with
> + `DIR_SHOW_IGNORED`.
>  
>  `DIR_KEEP_UNTRACKED_CONTENTS`:::
>  
> @@ -39,6 +43,21 @@ The notable options are:
>   untracked contents of untracked directories are also returned in
>   `entries[]`.
>  
> +`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::
> +
> + Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
> + this is set, returns ignored files and directories that match
> + an exclude pattern. If a directory matches an exclude pattern,
> + then the directory is returned and the contained paths are
> + not. A directory that does not match an exclude pattern will
> + not be returned even if all of its contents are ignored. In
> + this case, the contents are returned as individual entries.
> ++
> +If this is set, files and directories that explicity match an ignore
> +pattern are reported. Implicity ignored directories (directories that
> +do not match an ignore pattern, but whose contents are all ignored)
> +are not reported, instead all of the contents are reported.

Makes me wonder if DIR_SHOW_IGNORED* should be splt out into a short
enum.  We have:

 - Do not show ignored ones (0)

 - Collect ignored ones (DIR_SHOW_IGNORED)

 - Collect ignored and untracked ones separately (DIR_SHOW_IGNORED_TOO)

 - Collect ignored and duntracked ones separately, but limit them to
   those mach exclude patterns explicitly 
(DIR_SHOW_IGNORED_TOO|...MODE_MATCHING)

so we need two bits to fit a 4-possiblity enum.

Then we do not have to worry about saying quirky things like A and B
are incompatible, and C makes sense only when B is set, etc.

>  `DIR_COLLECT_IGNORED`:::
>  
>   Special mode for git-add. Return ignored files in `ignored[]` and

[PATCH v2 2/5] Update documentation for new directory and status logic

[Jameson Miller]

Signed-off-by: Jameson Miller 
---
 Documentation/git-status.txt  | 21 +-
 Documentation/technical/api-directory-listing.txt | 27 +++
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a36c..fc282e0a92 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -97,8 +97,27 @@ configuration variable documented in linkgit:git-config[1].
(and suppresses the output of submodule summaries when the config option
`status.submoduleSummary` is set).
 
---ignored::
+--ignored[=]::
Show ignored files as well.
++
+The mode parameter is used to specify the handling of ignored files.
+It is optional: it defaults to 'traditional'.
++
+The possible options are:
++
+   - 'traditional' - Shows ignored files and directories, unless
+ --untracked-files=all is specifed, in which case
+ individual files in ignored directories are
+ displayed.
+   - 'no'  - Show no ignored files.
+   - 'matching'- Shows ignored files and directories matching an
+ ignore pattern.
++
+When 'matching' mode is specified, paths that explicity match an
+ignored pattern are shown. If a directory matches an ignore pattern,
+then it is shown, but not paths contained in the ignored directory. If
+a directory does not match an ignore pattern, but all contents are
+ignored, then the directory is not shown, but all contents are shown.
 
 -z::
Terminate entries with NUL, instead of LF.  This implies
diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 6c77b4920c..7fae00f44f 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -22,16 +22,20 @@ The notable options are:
 
 `flags`::
 
-   A bit-field of options (the `*IGNORED*` flags are mutually exclusive):
+   A bit-field of options:
 
 `DIR_SHOW_IGNORED`:::
 
-   Return just ignored files in `entries[]`, not untracked files.
+   Return just ignored files in `entries[]`, not untracked
+   files. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED_TOO`.
 
 `DIR_SHOW_IGNORED_TOO`:::
 
-   Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
-   in addition to untracked files in `entries[]`.
+   Similar to `DIR_SHOW_IGNORED`, but return ignored files in
+   `ignored[]` in addition to untracked files in
+   `entries[]`. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED`.
 
 `DIR_KEEP_UNTRACKED_CONTENTS`:::
 
@@ -39,6 +43,21 @@ The notable options are:
untracked contents of untracked directories are also returned in
`entries[]`.
 
+`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::
+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
+   this is set, returns ignored files and directories that match
+   an exclude pattern. If a directory matches an exclude pattern,
+   then the directory is returned and the contained paths are
+   not. A directory that does not match an exclude pattern will
+   not be returned even if all of its contents are ignored. In
+   this case, the contents are returned as individual entries.
++
+If this is set, files and directories that explicity match an ignore
+pattern are reported. Implicity ignored directories (directories that
+do not match an ignore pattern, but whose contents are all ignored)
+are not reported, instead all of the contents are reported.
+
 `DIR_COLLECT_IGNORED`:::
 
Special mode for git-add. Return ignored files in `ignored[]` and
-- 
2.13.6

Re: [PATCH v2 05/24] refs: update ref transactions to use struct object_id

[Michael Haggerty]

On 10/09/2017 03:11 AM, brian m. carlson wrote:
> Update the ref transaction code to use struct object_id.  Remove one
> NULL pointer check which was previously inserted around a dereference;
> since we now pass a pointer to struct object_id directly through, the
> code we're calling handles this for us.
> 
> Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
> ---
>  branch.c   |  2 +-
>  builtin/clone.c|  2 +-
>  builtin/commit.c   |  4 ++--
>  builtin/fetch.c|  4 ++--
>  builtin/receive-pack.c |  4 ++--
>  builtin/replace.c  |  2 +-
>  builtin/tag.c  |  2 +-
>  builtin/update-ref.c   |  8 
>  fast-import.c  |  4 ++--
>  refs.c | 44 +---
>  refs.h | 24 
>  refs/files-backend.c   | 12 ++--
>  refs/refs-internal.h   |  4 ++--
>  sequencer.c|  2 +-
>  walker.c   |  2 +-
>  15 files changed, 59 insertions(+), 61 deletions(-)
> 
> [...]
> diff --git a/refs.h b/refs.h
> index 369614d392..543dcc5956 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -519,15 +519,15 @@ struct ref_transaction *ref_transaction_begin(struct 
> strbuf *err);
>   */
>  int ref_transaction_update(struct ref_transaction *transaction,

The docstring for this function needs to be updated.

>  const char *refname,
> -const unsigned char *new_sha1,
> -const unsigned char *old_sha1,
> +const struct object_id *new_oid,
> +const struct object_id *old_oid,
>  unsigned int flags, const char *msg,
>  struct strbuf *err);
>  
> [...]

Michael

Re: [PATCH v2 05/24] refs: update ref transactions to use struct object_id

[Jonathan Nieder]

brian m. carlson wrote:

> Update the ref transaction code to use struct object_id.  Remove one
> NULL pointer check which was previously inserted around a dereference;
> since we now pass a pointer to struct object_id directly through, the
> code we're calling handles this for us.
>
> Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
> ---
>  branch.c   |  2 +-
>  builtin/clone.c|  2 +-
>  builtin/commit.c   |  4 ++--
>  builtin/fetch.c|  4 ++--
>  builtin/receive-pack.c |  4 ++--
>  builtin/replace.c  |  2 +-
>  builtin/tag.c  |  2 +-
>  builtin/update-ref.c   |  8 
>  fast-import.c  |  4 ++--
>  refs.c | 44 +---
>  refs.h | 24 
>  refs/files-backend.c   | 12 ++--
>  refs/refs-internal.h   |  4 ++--
>  sequencer.c|  2 +-
>  walker.c   |  2 +-
>  15 files changed, 59 insertions(+), 61 deletions(-)

Makes sense.

[...]
> +++ b/refs.c
[...]
>  int ref_transaction_create(struct ref_transaction *transaction,
>  const char *refname,
> -const unsigned char *new_sha1,
> +const struct object_id *new_oid,
>  unsigned int flags, const char *msg,
>  struct strbuf *err)
>  {
> - if (!new_sha1 || is_null_sha1(new_sha1))
> + if (!new_oid || is_null_oid(new_oid))
>   die("BUG: create called without valid new_sha1");

The error message still refers to new_sha1.

[...]
>  int ref_transaction_delete(struct ref_transaction *transaction,
>  const char *refname,
> -const unsigned char *old_sha1,
> +const struct object_id *old_oid,
>  unsigned int flags, const char *msg,
>  struct strbuf *err)
>  {
> - if (old_sha1 && is_null_sha1(old_sha1))
> + if (old_oid && is_null_oid(old_oid))
>   die("BUG: delete called with old_sha1 set to zeros");

Likewise.

[...]
>  int ref_transaction_verify(struct ref_transaction *transaction,
>  const char *refname,
> -const unsigned char *old_sha1,
> +const struct object_id *old_oid,
>  unsigned int flags,
>  struct strbuf *err)
>  {
> - if (!old_sha1)
> + if (!old_oid)
>   die("BUG: verify called with old_sha1 set to NULL");

Likewise.

[...]
> +++ b/refs.h
> @@ -519,15 +519,15 @@ struct ref_transaction *ref_transaction_begin(struct 
> strbuf *err);
[...]
>  /*
> - * Add a reference creation to transaction. new_sha1 is the value that
> + * Add a reference creation to transaction. new_oid is the value that
>   * the reference should have after the update; it must not be
> - * null_sha1. It is verified that the reference does not exist
> + * null_oid. It is verified that the reference does not exist
>   * already.
>   *
>   * See the above comment "Reference transaction updates" for more
> @@ -535,35 +535,35 @@ int ref_transaction_update(struct ref_transaction 
> *transaction,
>   */

(Possible diff generation bug: there's exactly one line represented in
that @@ field.  I would have expected the diff generator to combine
the hunks.)

I think this is fine to go in without the error messages being fixed.
A grep for 'sha1' will find them later so that they can be addressed
in a separate patch.

Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>

Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Jeff King]

On Sun, Oct 08, 2017 at 11:37:14PM +0200, Lars Schneider wrote:

> >> Yeah, I think that makes more sense.
> >> 
> >> A patch may look like this on top of these two patches, but I'd
> >> prefer to see Lars's eyeballing and possibly wrapping it up in an
> >> applicable patch after taking the authorship.
> > 
> 
> This looks all good to me. Thank you!
> A few minor style suggestions below.

Thanks, these were all reasonable (I actually avoided unwrapping some of
the lines in the original to make the "-w" diff more readable :) ).

I ended up breaking this into three commits, since I think that makes it
easier to see what each of the changes is doing. Here's what I have (on
top of what Junio has already queued in ls/filter-process-delayed):

  [v2 1/3]: write_entry: fix leak when retrying delayed filter
  [v2 2/3]: write_entry: avoid reading blobs in CE_RETRY case
  [v2 3/3]: write_entry: untangle symlink and regular-file cases

 entry.c | 83 ++---
 1 file changed, 48 insertions(+), 35 deletions(-)

-Peff

[PATCH v2 05/24] refs: update ref transactions to use struct object_id

[brian m. carlson]

Update the ref transaction code to use struct object_id.  Remove one
NULL pointer check which was previously inserted around a dereference;
since we now pass a pointer to struct object_id directly through, the
code we're calling handles this for us.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 branch.c   |  2 +-
 builtin/clone.c|  2 +-
 builtin/commit.c   |  4 ++--
 builtin/fetch.c|  4 ++--
 builtin/receive-pack.c |  4 ++--
 builtin/replace.c  |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-ref.c   |  8 
 fast-import.c  |  4 ++--
 refs.c | 44 +---
 refs.h | 24 
 refs/files-backend.c   | 12 ++--
 refs/refs-internal.h   |  4 ++--
 sequencer.c|  2 +-
 walker.c   |  2 +-
 15 files changed, 59 insertions(+), 61 deletions(-)

diff --git a/branch.c b/branch.c
index 4377ce2fb1..45029ea142 100644
--- a/branch.c
+++ b/branch.c
@@ -305,7 +305,7 @@ void create_branch(const char *name, const char *start_name,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref.buf,
-  oid.hash, forcing ? NULL : null_sha1,
+  , forcing ? NULL : _oid,
   0, msg, ) ||
ref_transaction_commit(transaction, ))
die("%s", err.buf);
diff --git a/builtin/clone.c b/builtin/clone.c
index 4135621aa3..665a0e2673 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -588,7 +588,7 @@ static void write_remote_refs(const struct ref *local_refs)
for (r = local_refs; r; r = r->next) {
if (!r->peer_ref)
continue;
-   if (ref_transaction_create(t, r->peer_ref->name, 
r->old_oid.hash,
+   if (ref_transaction_create(t, r->peer_ref->name, >old_oid,
   0, NULL, ))
die("%s", err.buf);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 0f8ddb6866..d5fbf404f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1785,9 +1785,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 
transaction = ref_transaction_begin();
if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", oid.hash,
+   ref_transaction_update(transaction, "HEAD", ,
   current_head
-  ? current_head->object.oid.hash : null_sha1,
+  ? _head->object.oid : _oid,
   0, sb.buf, ) ||
ref_transaction_commit(transaction, )) {
rollback_index_files();
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 225c734924..859be91d6c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -457,8 +457,8 @@ static int s_update_ref(const char *action,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref->name,
-  ref->new_oid.hash,
-  check_old ? ref->old_oid.hash : NULL,
+  >new_oid,
+  check_old ? >old_oid : NULL,
   0, msg, ))
goto fail;
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 29a0f3b75f..39defd4e3c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1139,7 +1139,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
if (ref_transaction_delete(transaction,
   namespaced_name,
-  old_oid ? old_oid->hash : NULL,
+  old_oid,
   0, "push", )) {
rp_error("%s", err.buf);
strbuf_release();
@@ -1156,7 +1156,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
 
if (ref_transaction_update(transaction,
   namespaced_name,
-  new_oid->hash, old_oid->hash,
+  new_oid, old_oid,
   0, "push",
   )) {
rp_error("%s", err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 2854eaa0f3..3099e55307 100644
--- a/builtin/replace.c
+++

Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Lars Schneider]

> On 06 Oct 2017, at 06:54, Jeff King  wrote:
> 
> On Fri, Oct 06, 2017 at 08:01:48AM +0900, Junio C Hamano wrote:
> 
>>> But
>>> I think we'd want to protect the read_blob_entry() call at the top of
>>> the case with a check for dco->state == CE_RETRY.
>> 
>> Yeah, I think that makes more sense.
>> 
>> A patch may look like this on top of these two patches, but I'd
>> prefer to see Lars's eyeballing and possibly wrapping it up in an
>> applicable patch after taking the authorship.
> 

This looks all good to me. Thank you!
A few minor style suggestions below.


> ...
> 
> The "structured" way, of course, would be to put everything under
> write_out_file into a helper function and just call it from both places
> rather than relying on a spaghetti of gotos and switch-breaks.
> 
> I'm OK with whatever structure we end up with, as long as it fixes the
> leak (and ideally the pessimization).
> 
> Anyway, here's the real patch in case anybody wants to apply it and play
> with it further.
> 
> -- >8 --
> diff --git a/entry.c b/entry.c
> index 1c7e3c11d5..d28b42d82d 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -261,6 +261,7 @@ static int write_entry(struct cache_entry *ce,
>   size_t newsize = 0;
>   struct stat st;
>   const struct submodule *sub;
> + struct delayed_checkout *dco = state->delayed_checkout;
> 
>   if (ce_mode_s_ifmt == S_IFREG) {
>   struct stream_filter *filter = get_stream_filter(ce->name,
> @@ -273,55 +274,61 @@ static int write_entry(struct cache_entry *ce,
>   }
> 
>   switch (ce_mode_s_ifmt) {
> - case S_IFREG:
>   case S_IFLNK:
>   new = read_blob_entry(ce, );
>   if (!new)
>   return error("unable to read sha1 file of %s (%s)",
>   path, oid_to_hex(>oid));
> 
> - if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {
> - ret = symlink(new, path);
> - free(new);
> - if (ret)
> - return error_errno("unable to create symlink 
> %s",
> -path);

Nit: This could go into one line now.


> - break;
> - }
> + /* fallback to handling it like a regular file if we must */
> + if (!has_symlinks || to_tempfile)
> + goto write_out_file;
> 
> + ret = symlink(new, path);
> + free(new);
> + if (ret)
> + return error_errno("unable to create symlink %s",
> +path);
> + break;
> +
> + case S_IFREG:
>   /*
>* Convert from git internal format to working tree format
>*/
> - if (ce_mode_s_ifmt == S_IFREG) {
> - struct delayed_checkout *dco = state->delayed_checkout;
> - if (dco && dco->state != CE_NO_DELAY) {
> - /* Do not send the blob in case of a retry. */
> - if (dco->state == CE_RETRY) {

Maybe we could add here something like:
/* The filer process got the blob already in case of a retry. 
Unnecessary to send it, again! */

> - new = NULL;
> - size = 0;
> - }
> - ret = async_convert_to_working_tree(
> - ce->name, new, size, , dco);

Nit: This could go into one line now.


> - if (ret && string_list_has_string(>paths, 
> ce->name)) {
> - free(new);
> - goto finish;
> - }
> - } else
> - ret = convert_to_working_tree(
> - ce->name, new, size, );

Nit: This could go into one line now.


> 
> - if (ret) {
> + if (dco && dco->state == CE_RETRY) {
> + new = NULL;
> + size = 0;
> + } else {
> + new = read_blob_entry(ce, );
> + if (!new)
> + return error ("unable to read sha1 file of %s 
> (%s)",
> +   path, oid_to_hex(>oid));
> + }
> +
> + if (dco && dco->state != CE_NO_DELAY) {
> + ret = async_convert_to_working_tree(
> + ce->name, new, 
> size, , dco);
> + if (ret && string_list_has_string(>paths, 
> ce->name)) {
>   free(new);
> - new = strbuf_detach(, );
> - size = newsize;
> + goto finish;
> 

Re: [RFC PATCH 2/8] commit: move code to update HEAD to libgit

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> Signed-off-by: Phillip Wood 
> ---

This seems to do a lot more than just moving code, most notably, it
uses setenv() to affect what happens in any subprocesses we may
spawn, and it is unclear if it was verified that this patch is free
of unwanted consequences due to that change (and any others I may
have missed while reading this patch, if any).

I suspect that it would be sufficient to make update_head() helper
function take the reflog action message as another parameter
instead to fix the above, but there may be other reasons why you
chose to do it this way---I cannot read it in your empty log
message, though.

I will not give line-by-line style nitpick but in general we do not
leave a SP between function name and the open parenthesis that
starts its argument list.  New code in this patch seems to use
mixture of styles.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 
> 0b8c1ef6f57cfed328d12255e6834adb4bda4137..497778ba2c02afdd4a337969a27ca781e8389040
>  100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1578,13 +1578,11 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>   struct strbuf sb = STRBUF_INIT;
>   struct strbuf author_ident = STRBUF_INIT;
>   const char *index_file, *reflog_msg;
> - char *nl;
>   struct object_id oid;
>   struct commit_list *parents = NULL;
>   struct stat statbuf;
>   struct commit *current_head = NULL;
>   struct commit_extra_header *extra = NULL;
> - struct ref_transaction *transaction;
>   struct strbuf err = STRBUF_INIT;
>  
>   if (argc == 2 && !strcmp(argv[1], "-h"))
> @@ -1625,10 +1623,10 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>   reflog_msg = getenv("GIT_REFLOG_ACTION");
>   if (!current_head) {
>   if (!reflog_msg)
> - reflog_msg = "commit (initial)";
> + setenv ("GIT_REFLOG_ACTION", "commit (initial)", 1);
>   } else if (amend) {
>   if (!reflog_msg)
> - reflog_msg = "commit (amend)";
> + setenv("GIT_REFLOG_ACTION", "commit (amend)", 1);
>   parents = copy_commit_list(current_head->parents);
>   } else if (whence == FROM_MERGE) {
>   struct strbuf m = STRBUF_INIT;
> @@ -1637,7 +1635,7 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>   struct commit_list **pptr = 
>  
>   if (!reflog_msg)
> - reflog_msg = "commit (merge)";
> + setenv("GIT_REFLOG_ACTION", "commit (merge)", 1);
>   pptr = commit_list_append(current_head, pptr);
>   fp = xfopen(git_path_merge_head(), "r");
>   while (strbuf_getline_lf(, fp) != EOF) {
> @@ -1660,9 +1658,9 @@ int cmd_commit(int argc, const char **argv, const char 
> *prefix)
>   parents = reduce_heads(parents);
>   } else {
>   if (!reflog_msg)
> - reflog_msg = (whence == FROM_CHERRY_PICK)
> - ? "commit (cherry-pick)"
> - : "commit";
> + setenv("GIT_REFLOG_ACTION", (whence == FROM_CHERRY_PICK)
> + ? "commit (cherry-pick)"
> + : "commit", 1);
>   commit_list_insert(current_head, );
>   }
>  
> @@ -1707,25 +1705,10 @@ int cmd_commit(int argc, const char **argv, const 
> char *prefix)
>   strbuf_release(_ident);
>   free_commit_extra_headers(extra);
>  
> - nl = strchr(sb.buf, '\n');
> - if (nl)
> - strbuf_setlen(, nl + 1 - sb.buf);
> - else
> - strbuf_addch(, '\n');
> - strbuf_insert(, 0, reflog_msg, strlen(reflog_msg));
> - strbuf_insert(, strlen(reflog_msg), ": ", 2);
> -
> - transaction = ref_transaction_begin();
> - if (!transaction ||
> - ref_transaction_update(transaction, "HEAD", oid.hash,
> -current_head
> -? current_head->object.oid.hash : null_sha1,
> -0, sb.buf, ) ||
> - ref_transaction_commit(transaction, )) {
> + if (update_head (current_head, , , )) {
>   rollback_index_files();
>   die("%s", err.buf);
>   }
> - ref_transaction_free(transaction);
>  
>   unlink(git_path_cherry_pick_head());
>   unlink(git_path_revert_head());
> diff --git a/sequencer.c b/sequencer.c
> index 
> 319208afb3de36c97b6c62d4ecf6e641245e7a54..917ad4a16216b30adb2c2c9650217926d8db8ba7
>  100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1,10 +1,10 @@
>  #include "cache.h"
>  #include "config.h"
>  #include "lockfile.h"
> -#include "sequencer.h"
>  #include "dir.h"
>  #include 

Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Jeff King]

On Fri, Oct 06, 2017 at 08:01:48AM +0900, Junio C Hamano wrote:

> > But
> > I think we'd want to protect the read_blob_entry() call at the top of
> > the case with a check for dco->state == CE_RETRY.
> 
> Yeah, I think that makes more sense.
> 
> A patch may look like this on top of these two patches, but I'd
> prefer to see Lars's eyeballing and possibly wrapping it up in an
> applicable patch after taking the authorship.

Yeah, agreed.

> I considered initializing new to NULL and size to 0 but decided
> against it, as that would lose the justification to have an if
> statement that marks that "dco->state == CE_RETRY" is a special
> case.  I think explicit if() with clearing these two variables makes
> it clearer to show what is going on.

Also agreed.

> By the way, the S_IFLNK handling seems iffy with or without this
> change (or for that matter, I suspect this iffy-ness existed before
> Lars's delayed filtering change).  On a platform without symlinks,
> we do the same as S_IFREG, but obviously we do not want any content
> conversion that happens to the regular files in such a case.  So we
> may further want to fix that, but I left it outside the scope of
> fixing the leak of NULL and optimizing the blob reading out.

I think the current code is correct because the conversion all happens
in the S_IFREG if-block. We just fall-through down to the actual write
phase for the symlink case.

That said, I found the fall-through here confusing as hell. I actually
think it would be a lot clearer with a goto, which is saying something.
Here's the "diff -w" of what I mean, for readability (the real patch is
at the bottom for reference, but it adjusts the indentation quite a
bit).

diff --git a/entry.c b/entry.c
index 1c7e3c11d5..d28b42d82d 100644
--- a/entry.c
+++ b/entry.c
@@ -261,6 +261,7 @@ static int write_entry(struct cache_entry *ce,
size_t newsize = 0;
struct stat st;
const struct submodule *sub;
+   struct delayed_checkout *dco = state->delayed_checkout;
 
if (ce_mode_s_ifmt == S_IFREG) {
struct stream_filter *filter = get_stream_filter(ce->name,
@@ -273,33 +274,39 @@ static int write_entry(struct cache_entry *ce,
}
 
switch (ce_mode_s_ifmt) {
-   case S_IFREG:
case S_IFLNK:
new = read_blob_entry(ce, );
if (!new)
return error("unable to read sha1 file of %s (%s)",
path, oid_to_hex(>oid));
 
-   if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {
+   /* fallback to handling it like a regular file if we must */
+   if (!has_symlinks || to_tempfile)
+   goto write_out_file;
+
ret = symlink(new, path);
free(new);
if (ret)
return error_errno("unable to create symlink %s",
   path);
break;
-   }
 
+   case S_IFREG:
/*
 * Convert from git internal format to working tree format
 */
-   if (ce_mode_s_ifmt == S_IFREG) {
-   struct delayed_checkout *dco = state->delayed_checkout;
-   if (dco && dco->state != CE_NO_DELAY) {
-   /* Do not send the blob in case of a retry. */
-   if (dco->state == CE_RETRY) {
+
+   if (dco && dco->state == CE_RETRY) {
new = NULL;
size = 0;
+   } else {
+   new = read_blob_entry(ce, );
+   if (!new)
+   return error ("unable to read sha1 file of %s 
(%s)",
+ path, oid_to_hex(>oid));
}
+
+   if (dco && dco->state != CE_NO_DELAY) {
ret = async_convert_to_working_tree(
ce->name, new, 
size, , dco);
if (ret && string_list_has_string(>paths, 
ce->name)) {
@@ -320,8 +327,8 @@ static int write_entry(struct cache_entry *ce,
 * point. If the error would have been fatal (e.g.
 * filter is required), then we would have died already.
 */
-   }
 
+write_out_file:
fd = open_output_fd(path, ce, to_tempfile);
if (fd < 0) {
free(new);

The "structured" way, of course, would be to put everything under
write_out_file into a helper function and just call it from both places
rather than relying on a spaghetti of gotos and switch-breaks.

I'm OK with whatever structure we end up with, as long as it fixes the
leak (and ideally the pessimization).

Anyway, here's the real patch in case anybody wants to apply it and play
with it further.

-- >8 --
diff 

Re: [PATCH] api-argv-array.txt: Update link to string-list API

[Junio C Hamano]

Todd Zullinger  writes:

> I noticed this broken link in the html documentation while building
> 2.15.0-rc0.  I'm not sure whether it's better to point the link to the
> string-list.h file on Git Hub, remove the link, or drop the entire
> paragraph.

Probably removing the link is the right thing to do.  In the longer
term, as we move more and more API documentation to the header file,
we may want to have a mechanism in the documentation build procedure
to extract them back to text.  And at that point:

 - the API doc for argv-array is no longer in api-argv-array.txt in
   the source form;

 - however, it would be extacted from argv-array.h and made into
   manpage or html or whatever human readable format.

 - the API doc for string-list would also be extracted from
   string-list.h and made into manpage or html or whatever human
   readable format.

 - And these two can refer to each other as needed.

But we are not there yet.

> The change I made to remove the link was simply:
>
> -The link:api-string-list.html[string-list API] is similar, but cannot be
> +The string-list API (documented in string-list.h) is similar, but cannot be

This is preferrable for now, I would think.

Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Junio C Hamano]

Jeff King  writes:

> On Thu, Oct 05, 2017 at 08:19:13PM +0900, Junio C Hamano wrote:
>
>> This is unrelated to the main topic of this patch, but we see this
>> just before the precontext of this hunk:
>> 
>>  if (dco && dco->state != CE_NO_DELAY) {
>>  /* Do not send the blob in case of a retry. */
>>  if (dco->state == CE_RETRY) {
>>  new = NULL;
>>  size = 0;
>>  }
>>  ret = async_convert_to_working_tree(
>>  ce->name, new, size, , dco);
>> 
>> Aren't we leaking "new" in that CE_RETRY case?
>
> Yes, it certainly looks like it. Wouldn't we want to avoid reading the
> file from disk entirely in that case?

Probably.  But that is more of a removal of pessimization than a fix ;-)

> I.e., I think free(new) is sufficient to fix the leak you
> mentioned.

In addition to keeping the new = NULL assignment, of course.

> But
> I think we'd want to protect the read_blob_entry() call at the top of
> the case with a check for dco->state == CE_RETRY.

Yeah, I think that makes more sense.

A patch may look like this on top of these two patches, but I'd
prefer to see Lars's eyeballing and possibly wrapping it up in an
applicable patch after taking the authorship.

I considered initializing new to NULL and size to 0 but decided
against it, as that would lose the justification to have an if
statement that marks that "dco->state == CE_RETRY" is a special
case.  I think explicit if() with clearing these two variables makes
it clearer to show what is going on.

By the way, the S_IFLNK handling seems iffy with or without this
change (or for that matter, I suspect this iffy-ness existed before
Lars's delayed filtering change).  On a platform without symlinks,
we do the same as S_IFREG, but obviously we do not want any content
conversion that happens to the regular files in such a case.  So we
may further want to fix that, but I left it outside the scope of
fixing the leak of NULL and optimizing the blob reading out.


 entry.c | 26 +-
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/entry.c b/entry.c
index cac5bf5af2..74e35f942c 100644
--- a/entry.c
+++ b/entry.c
@@ -274,14 +274,12 @@ static int write_entry(struct cache_entry *ce,
}
 
switch (ce_mode_s_ifmt) {
-   case S_IFREG:
case S_IFLNK:
new = read_blob_entry(ce, );
if (!new)
return error("unable to read sha1 file of %s (%s)",
path, oid_to_hex(>oid));
-
-   if (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {
+   if (has_symlinks && !to_tempfile) {
ret = symlink(new, path);
free(new);
if (ret)
@@ -289,18 +287,28 @@ static int write_entry(struct cache_entry *ce,
   path);
break;
}
-
+   /* fallthru */
+   case S_IFREG:
/*
 * Convert from git internal format to working tree format
 */
if (ce_mode_s_ifmt == S_IFREG) {
struct delayed_checkout *dco = state->delayed_checkout;
+
+   /* 
+* In case of a retry, we do not send blob, hence no
+* need to read it, either.
+*/
+   if (dco && dco->state == CE_RETRY) {
+   new = NULL;
+   size = 0;
+   } else {
+   new = read_blob_entry(ce, );
+   if (!new)
+   return error("unable to read sha1 file 
of %s (%s)",
+path, 
oid_to_hex(>oid));
+   }
if (dco && dco->state != CE_NO_DELAY) {
-   /* Do not send the blob in case of a retry. */
-   if (dco->state == CE_RETRY) {
-   new = NULL;
-   size = 0;
-   }
ret = async_convert_to_working_tree(
ce->name, new, size, , dco);
if (ret && string_list_has_string(>paths, 
ce->name)) {

Re: [PATCH 2/6] Update documentation for new directory and status logic

[Stefan Beller]

On Thu, Oct 5, 2017 at 1:54 PM,   wrote:
> From: Jameson Miller 
>
> Signed-off-by: Jameson Miller 
> ---
>  Documentation/git-status.txt  | 22 +-
>  Documentation/technical/api-directory-listing.txt | 28 
> +++
>  2 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 9f3a78a36c..7d1410ff3f 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -97,8 +97,28 @@ configuration variable documented in linkgit:git-config[1].
> (and suppresses the output of submodule summaries when the config 
> option
> `status.submoduleSummary` is set).
>
> ---ignored::
> +--ignored[=]::
> Show ignored files as well.
> ++
> +The mode parameter is used to specify the handling of ignored files.
> +It is optional: it defaults to 'default', and if specified, it must be
> +stuck to the option (e.g. '--ignored=default`, but not `--ignored default`).

Is there a rationale for not accepting `--ignored default`?
(It's just the way OPTION_STRING inputs seem to work,
but in other cases [c.f. man git gc, search "--prune=" ] this is
just implied). Or rather: no need to spell this out explicitly IMHO,
as that draws the users attention to it, which might be confusing.

> ++
> +The possible options are:
> ++
> +   - 'traditional' - Shows ignored files and directories, unless
> + --untracked-files=all is specifed, in which case
> + individual files in ignored directories are
> + displayed.
> +   - 'no'  - Show no ignored files.
> +   - 'matching'- Shows ignored files and directories matching an
> + ignore pattern.
> ++
> +When 'matching' mode is specified, paths that explicity match an
> +ignored pattern are shown. If a directory matches an ignore pattern,
> +then it is shown, but not paths contained in the ignored directory. If
> +a directory does not match an ignore pattern, but all contents are
> +ignored, then the directory is not shown, but all contents are shown.
>
>  -z::
> Terminate entries with NUL, instead of LF.  This implies
> diff --git a/Documentation/technical/api-directory-listing.txt 
> b/Documentation/technical/api-directory-listing.txt
> index 6c77b4920c..86c981c29c 100644
> --- a/Documentation/technical/api-directory-listing.txt
> +++ b/Documentation/technical/api-directory-listing.txt
> @@ -22,16 +22,20 @@ The notable options are:
>
>  `flags`::
>
> -   A bit-field of options (the `*IGNORED*` flags are mutually exclusive):
> +   A bit-field of options:
>
>  `DIR_SHOW_IGNORED`:::
>
> -   Return just ignored files in `entries[]`, not untracked files.
> +   Return just ignored files in `entries[]`, not untracked
> +   files. This is flag is mutually exclusive with

"is flag is" ?


> +   `DIR_SHOW_IGNORED_TOO`.
>
>  `DIR_SHOW_IGNORED_TOO`:::
>
> -   Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
> -   in addition to untracked files in `entries[]`.
> +   Similar to `DIR_SHOW_IGNORED`, but return ignored files in
> +   `ignored[]` in addition to untracked files in
> +   `entries[]`. This flag is mutually exclusive with
> +   `DIR_SHOW_IGNORED`.
>
>  `DIR_KEEP_UNTRACKED_CONTENTS`:::
>
> @@ -39,6 +43,22 @@ The notable options are:
> untracked contents of untracked directories are also returned in
> `entries[]`.
>
> +`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::
> +
> +   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
> +   this is set, returns ignored files and directories that match
> +   an exclude pattern. If a directory matches an exclude pattern,
> +   then the directory is returned and the contained paths are
> +   not. A directory that does not match an exclude pattern will
> +   not be returned even if all of its contents are ignored. In
> +   this case, the contents are returned as individual entries.
> +

I think to make the asciidoc happy, you'd put a '+ LF' as paragraph
delimiter and the subsequent paragraphs are not indented.
C.f. Documentation/git-gc.txt "--auto".
Oh, screw that. It turns out, this place is the only place in our docs
where we use triple colons. So I don't know what I am talking about.

> +If this is set, files and directories
> +   that explicity match an ignore pattern are reported. Implicity
> +   ignored directories (directories that do not match an ignore
> +   pattern, but whose contents are all ignored) are not reported,
> +   instead all of the contents are reported.
> +
>  `DIR_COLLECT_IGNORED`:::
>
> Special mode for git-add. Return ignored files in `ignored[]` and
> --
> 2.13.6
>

[PATCH] api-argv-array.txt: Update link to string-list API

[Todd Zullinger]

In 4f665f2cf3 (string-list.h: move documentation from Documentation/api/
into header, 2017-09-26) the string-list API documentation was moved
into string-list.h.  Fix the link from the argv-array API documentation.

Signed-off-by: Todd Zullinger 
---

Hi,

I noticed this broken link in the html documentation while building
2.15.0-rc0.  I'm not sure whether it's better to point the link to the
string-list.h file on Git Hub, remove the link, or drop the entire
paragraph.

The change I made to remove the link was simply:

-The link:api-string-list.html[string-list API] is similar, but cannot be
+The string-list API (documented in string-list.h) is similar, but cannot be

 Documentation/technical/api-argv-array.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/technical/api-argv-array.txt 
b/Documentation/technical/api-argv-array.txt
index cfc063018c..1603f4a941 100644
--- a/Documentation/technical/api-argv-array.txt
+++ b/Documentation/technical/api-argv-array.txt
@@ -8,7 +8,7 @@ always NULL-terminated at the element pointed to by 
`argv[argc]`. This
 makes the result suitable for passing to functions expecting to receive
 argv from main(), or the link:api-run-command.html[run-command API].
 
-The link:api-string-list.html[string-list API] is similar, but cannot be
+The https://raw.githubusercontent.com/git/git/master/string-list.h[string-list 
API] is similar, but cannot be
 used for these purposes; instead of storing a straight string pointer,
 it contains an item structure with a `util` field that is not compatible
 with the traditional argv interface.
-- 
2.14.2

[PATCH 2/6] Update documentation for new directory and status logic

[jameson . miller81]

From: Jameson Miller 

Signed-off-by: Jameson Miller 
---
 Documentation/git-status.txt  | 22 +-
 Documentation/technical/api-directory-listing.txt | 28 +++
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a36c..7d1410ff3f 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -97,8 +97,28 @@ configuration variable documented in linkgit:git-config[1].
(and suppresses the output of submodule summaries when the config option
`status.submoduleSummary` is set).
 
---ignored::
+--ignored[=]::
Show ignored files as well.
++
+The mode parameter is used to specify the handling of ignored files.
+It is optional: it defaults to 'default', and if specified, it must be
+stuck to the option (e.g. '--ignored=default`, but not `--ignored default`).
++
+The possible options are:
++
+   - 'traditional' - Shows ignored files and directories, unless
+ --untracked-files=all is specifed, in which case
+ individual files in ignored directories are
+ displayed.
+   - 'no'  - Show no ignored files.
+   - 'matching'- Shows ignored files and directories matching an
+ ignore pattern.
++
+When 'matching' mode is specified, paths that explicity match an
+ignored pattern are shown. If a directory matches an ignore pattern,
+then it is shown, but not paths contained in the ignored directory. If
+a directory does not match an ignore pattern, but all contents are
+ignored, then the directory is not shown, but all contents are shown.
 
 -z::
Terminate entries with NUL, instead of LF.  This implies
diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 6c77b4920c..86c981c29c 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -22,16 +22,20 @@ The notable options are:
 
 `flags`::
 
-   A bit-field of options (the `*IGNORED*` flags are mutually exclusive):
+   A bit-field of options:
 
 `DIR_SHOW_IGNORED`:::
 
-   Return just ignored files in `entries[]`, not untracked files.
+   Return just ignored files in `entries[]`, not untracked
+   files. This is flag is mutually exclusive with
+   `DIR_SHOW_IGNORED_TOO`.
 
 `DIR_SHOW_IGNORED_TOO`:::
 
-   Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
-   in addition to untracked files in `entries[]`.
+   Similar to `DIR_SHOW_IGNORED`, but return ignored files in
+   `ignored[]` in addition to untracked files in
+   `entries[]`. This flag is mutually exclusive with
+   `DIR_SHOW_IGNORED`.
 
 `DIR_KEEP_UNTRACKED_CONTENTS`:::
 
@@ -39,6 +43,22 @@ The notable options are:
untracked contents of untracked directories are also returned in
`entries[]`.
 
+`DIR_SHOW_IGNORED_TOO_MODE_MATCHING`:::
+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if
+   this is set, returns ignored files and directories that match
+   an exclude pattern. If a directory matches an exclude pattern,
+   then the directory is returned and the contained paths are
+   not. A directory that does not match an exclude pattern will
+   not be returned even if all of its contents are ignored. In
+   this case, the contents are returned as individual entries.
+
+If this is set, files and directories
+   that explicity match an ignore pattern are reported. Implicity
+   ignored directories (directories that do not match an ignore
+   pattern, but whose contents are all ignored) are not reported,
+   instead all of the contents are reported.
+
 `DIR_COLLECT_IGNORED`:::
 
Special mode for git-add. Return ignored files in `ignored[]` and
-- 
2.13.6

Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Jeff King]

On Thu, Oct 05, 2017 at 08:19:13PM +0900, Junio C Hamano wrote:

> > diff --git a/entry.c b/entry.c
> > index 1c7e3c11d5..5dab656364 100644
> > --- a/entry.c
> > +++ b/entry.c
> > @@ -304,7 +304,7 @@ static int write_entry(struct cache_entry *ce,
> > ce->name, new, size, , dco);
> > if (ret && string_list_has_string(>paths, 
> > ce->name)) {
> > free(new);
> > -   goto finish;
> > +   goto delayed;
> > }
> > } else
> > ret = convert_to_working_tree(
> 
> This is unrelated to the main topic of this patch, but we see this
> just before the precontext of this hunk:
> 
>   if (dco && dco->state != CE_NO_DELAY) {
>   /* Do not send the blob in case of a retry. */
>   if (dco->state == CE_RETRY) {
>   new = NULL;
>   size = 0;
>   }
>   ret = async_convert_to_working_tree(
>   ce->name, new, size, , dco);
> 
> Aren't we leaking "new" in that CE_RETRY case?

Yes, it certainly looks like it. Wouldn't we want to avoid reading the
file from disk entirely in that case?

I.e., I think free(new) is sufficient to fix the leak you mentioned. But
I think we'd want to protect the read_blob_entry() call at the top of
the case with a check for dco->state == CE_RETRY.

-Peff

Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Junio C Hamano]

lars.schnei...@autodesk.com writes:

> diff --git a/entry.c b/entry.c
> index 1c7e3c11d5..5dab656364 100644
> --- a/entry.c
> +++ b/entry.c
> @@ -304,7 +304,7 @@ static int write_entry(struct cache_entry *ce,
>   ce->name, new, size, , dco);
>   if (ret && string_list_has_string(>paths, 
> ce->name)) {
>   free(new);
> - goto finish;
> + goto delayed;
>   }
>   } else
>   ret = convert_to_working_tree(

This is unrelated to the main topic of this patch, but we see this
just before the precontext of this hunk:

if (dco && dco->state != CE_NO_DELAY) {
/* Do not send the blob in case of a retry. */
if (dco->state == CE_RETRY) {
new = NULL;
size = 0;
}
ret = async_convert_to_working_tree(
ce->name, new, size, , dco);

Aren't we leaking "new" in that CE_RETRY case?

Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files

[Jeff King]

On Thu, Oct 05, 2017 at 12:44:06PM +0200, lars.schnei...@autodesk.com wrote:

> From: Lars Schneider 
> 
> In 2841e8f ("convert: add "status=delayed" to filter process protocol",
> 2017-06-30) we taught the filter process protocol to delay responses.
> 
> That means an external filter might answer in the first write_entry()
> call on a file that requires filtering  "I got your request, but I
> can't answer right now. Ask again later!". As Git got no answer, we do
> not write anything to the filesystem. Consequently, the lstat() call in
> the finish block of the function writes garbage to the cache entry.
> The garbage is eventually overwritten when the filter answers with
> the final file content in a subsequent write_entry() call.
> 
> Fix the brief time window of garbage in the cache entry by adding a
> special finish block that does nothing for delayed responses. The cache
> entry is written properly in a subsequent write_entry() call where
> the filter responds with the final file content.

Nicely explained and the patch looks correct. I also verified that MSan
is happy with t0021 after this.

Thanks for the quick turnaround.

-Peff

[PATCH v1 1/2] entry.c: update cache entry only for existing files

[lars . schneider]

From: Lars Schneider 

In 2841e8f ("convert: add "status=delayed" to filter process protocol",
2017-06-30) we taught the filter process protocol to delay responses.

That means an external filter might answer in the first write_entry()
call on a file that requires filtering  "I got your request, but I
can't answer right now. Ask again later!". As Git got no answer, we do
not write anything to the filesystem. Consequently, the lstat() call in
the finish block of the function writes garbage to the cache entry.
The garbage is eventually overwritten when the filter answers with
the final file content in a subsequent write_entry() call.

Fix the brief time window of garbage in the cache entry by adding a
special finish block that does nothing for delayed responses. The cache
entry is written properly in a subsequent write_entry() call where
the filter responds with the final file content.

Reported-by: Jeff King 
Signed-off-by: Lars Schneider 
---
 entry.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/entry.c b/entry.c
index 1c7e3c11d5..5dab656364 100644
--- a/entry.c
+++ b/entry.c
@@ -304,7 +304,7 @@ static int write_entry(struct cache_entry *ce,
ce->name, new, size, , dco);
if (ret && string_list_has_string(>paths, 
ce->name)) {
free(new);
-   goto finish;
+   goto delayed;
}
} else
ret = convert_to_working_tree(
@@ -360,6 +360,7 @@ static int write_entry(struct cache_entry *ce,
ce->ce_flags |= CE_UPDATE_IN_BASE;
state->istate->cache_changed |= CE_ENTRY_CHANGED;
}
+delayed:
return 0;
 }
 
-- 
2.14.2

Re: [PATCH v2] setup: update error message to be more meaningful

[Kaartic Sivaraam]

On Tue, 2017-10-03 at 09:32 +0900, Junio C Hamano wrote:
> 
> As an aside, I wonder if we want to _() the message.  It's outside
> the scope of this fix, obviously.
> 

I'm surprised it isn't already! I think it should.


As an aside, I wanted to know whether we should add
'test_expect_failure's for commands of the following sort,

git log -p --full-diff master --stat
 
git commit -v Makefile --amend

just to keep us reminded that these are accepted not by design but by
accident ?

---
Kaartic

Re: [PATCH v2] setup: update error message to be more meaningful

[Junio C Hamano]

Kaartic Sivaraam  writes:

> Incorrect case,
>
> $ git grep "some random regex" -n
> fatal: bad flag '-n' used after filename
>
> The above case is incorrect as "some random regex" isn't a filename
> in this case.

The command line rule is to have dashed options first and then other
arguments, so I agree that "option '-n' used after non-option
argument(s)" would be a better alternative.

"grep" is an oddball, as it allows you to be lazy and omit the "-e"
option when there is only one pattern, making a perfectly reasonable
"grep -e regex -n" into an invalid "grep regex -n".

As an aside, I wonder if we want to _() the message.  It's outside
the scope of this fix, obviously.

>  setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 860507e1f..09c793282 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -230,7 +230,7 @@ void verify_filename(const char *prefix,
>int diagnose_misspelt_rev)
>  {
>   if (*arg == '-')
> - die("bad flag '%s' used after filename", arg);
> + die("option '%s' must come before non-option arguments", arg);
>   if (looks_like_pathspec(arg) || check_filename(prefix, arg))
>   return;
>   die_verify_filename(prefix, arg, diagnose_misspelt_rev);

[PATCH v2] setup: update error message to be more meaningful

[Kaartic Sivaraam]

From: Kaartic Sivaraam 

The error message shown when a flag is found when expecting a
filename wasn't clear as it didn't communicate what was wrong
using the 'suitable' words in *all* cases.

$ git ls-files
README.md
test-file

Correct case,

$ git rev-parse README.md --flags
README.md
--flags
fatal: bad flag '--flags' used after filename

Incorrect case,

$ git grep "some random regex" -n
fatal: bad flag '-n' used after filename

The above case is incorrect as "some random regex" isn't a filename
in this case.

Change the error message to be general and communicative. This results
in the following output,

$ git rev-parse README.md --flags
README.md
--flags
fatal: option '--flags' must come before non-option arguments

$ git grep "some random regex" -n
fatal: option '-n' must come before non-option arguments

Signed-off-by: Kaartic Sivaraam 
---
 Changes in v2:

Change in error message.

 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 860507e1f..09c793282 100644
--- a/setup.c
+++ b/setup.c
@@ -230,7 +230,7 @@ void verify_filename(const char *prefix,
 int diagnose_misspelt_rev)
 {
if (*arg == '-')
-   die("bad flag '%s' used after filename", arg);
+   die("option '%s' must come before non-option arguments", arg);
if (looks_like_pathspec(arg) || check_filename(prefix, arg))
return;
die_verify_filename(prefix, arg, diagnose_misspelt_rev);
-- 
2.14.1.935.ge2b2bcd8a

[PATCH 05/24] refs: update ref transactions to use struct object_id

[brian m. carlson]

Update the ref transaction code to use struct object_id.  Remove one
NULL pointer check which was previously inserted around a dereference;
since we now pass a pointer to struct object_id directly through, the
code we're calling handles this for us.

Signed-off-by: brian m. carlson <sand...@crustytoothpaste.net>
---
 branch.c   |  2 +-
 builtin/clone.c|  2 +-
 builtin/commit.c   |  4 ++--
 builtin/fetch.c|  4 ++--
 builtin/receive-pack.c |  4 ++--
 builtin/replace.c  |  2 +-
 builtin/tag.c  |  2 +-
 builtin/update-ref.c   |  8 
 fast-import.c  |  4 ++--
 refs.c | 44 +---
 refs.h | 24 
 refs/files-backend.c   | 12 ++--
 refs/refs-internal.h   |  4 ++--
 sequencer.c|  2 +-
 walker.c   |  2 +-
 15 files changed, 59 insertions(+), 61 deletions(-)

diff --git a/branch.c b/branch.c
index 4377ce2fb1..45029ea142 100644
--- a/branch.c
+++ b/branch.c
@@ -305,7 +305,7 @@ void create_branch(const char *name, const char *start_name,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref.buf,
-  oid.hash, forcing ? NULL : null_sha1,
+  , forcing ? NULL : _oid,
   0, msg, ) ||
ref_transaction_commit(transaction, ))
die("%s", err.buf);
diff --git a/builtin/clone.c b/builtin/clone.c
index 4135621aa3..665a0e2673 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -588,7 +588,7 @@ static void write_remote_refs(const struct ref *local_refs)
for (r = local_refs; r; r = r->next) {
if (!r->peer_ref)
continue;
-   if (ref_transaction_create(t, r->peer_ref->name, 
r->old_oid.hash,
+   if (ref_transaction_create(t, r->peer_ref->name, >old_oid,
   0, NULL, ))
die("%s", err.buf);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 0f8ddb6866..d5fbf404f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1785,9 +1785,9 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 
transaction = ref_transaction_begin();
if (!transaction ||
-   ref_transaction_update(transaction, "HEAD", oid.hash,
+   ref_transaction_update(transaction, "HEAD", ,
   current_head
-  ? current_head->object.oid.hash : null_sha1,
+  ? _head->object.oid : _oid,
   0, sb.buf, ) ||
ref_transaction_commit(transaction, )) {
rollback_index_files();
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 225c734924..859be91d6c 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -457,8 +457,8 @@ static int s_update_ref(const char *action,
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref->name,
-  ref->new_oid.hash,
-  check_old ? ref->old_oid.hash : NULL,
+  >new_oid,
+  check_old ? >old_oid : NULL,
   0, msg, ))
goto fail;
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 29a0f3b75f..39defd4e3c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1139,7 +1139,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
if (ref_transaction_delete(transaction,
   namespaced_name,
-  old_oid ? old_oid->hash : NULL,
+  old_oid,
   0, "push", )) {
rp_error("%s", err.buf);
strbuf_release();
@@ -1156,7 +1156,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
 
if (ref_transaction_update(transaction,
   namespaced_name,
-  new_oid->hash, old_oid->hash,
+  new_oid, old_oid,
   0, "push",
   )) {
rp_error("%s", err.buf);
diff --git a/builtin/replace.c b/builtin/replace.c
index 2854eaa0f3..3099e55307 100644
--- a/builtin/replace.c
+++