[PATCH v2 5/5] lock_file: move static locks into functions

[Martin Ågren]

Placing `struct lock_file`s on the stack used to be a bad idea, because
the temp- and lockfile-machinery would keep a pointer into the struct.
But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05), we can safely have lockfiles on the stack. (This applies
even if a user returns early, leaving a locked lock behind.)

Each of these `struct lock_file`s is used from within a single function.
Move them into the respective functions to make the scope clearer and
drop the staticness.

For good measure, I have inspected these sites and come to believe that
they always release the lock, with the possible exception of bailing out
using `die()` or `exit()` or by returning from a `cmd_foo()`.

As pointed out by Jeff King, it would be bad if someone held on to a
`struct lock_file *` for some reason. After some grepping, I agree with
his findings: no-one appears to be doing that.

After this commit, the remaining occurrences of "static struct
lock_file" are locks that are used from within different functions. That
is, they need to remain static. (Short of more intrusive changes like
passing around pointers to non-static locks.)

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 t/helper/test-scrap-cache-tree.c | 4 ++--
 builtin/add.c| 3 +--
 builtin/mv.c | 2 +-
 builtin/read-tree.c  | 3 +--
 builtin/rm.c | 3 +--
 rerere.c | 3 +--
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index d26d3e7c8b..393f1604ff 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -4,10 +4,10 @@
 #include "tree.h"
 #include "cache-tree.h"
 
-static struct lock_file index_lock;
-
 int cmd__scrap_cache_tree(int ac, const char **av)
 {
+   struct lock_file index_lock = LOCK_INIT;
+
setup_git_directory();
hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
if (read_cache() < 0)
diff --git a/builtin/add.c b/builtin/add.c
index c9e2619a9a..8a155dd41e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -265,8 +265,6 @@ static int edit_patch(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static struct lock_file lock_file;
-
 static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
@@ -393,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+   struct lock_file lock_file = LOCK_INIT;
 
git_config(add_config, NULL);
 
diff --git a/builtin/mv.c b/builtin/mv.c
index 6d141f7a53..b4692409e3 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -72,7 +72,6 @@ static const char *add_slash(const char *path)
return path;
 }
 
-static struct lock_file lock_file;
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
 static void prepare_move_submodule(const char *src, int first,
@@ -131,6 +130,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
struct stat st;
struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
+   struct lock_file lock_file = LOCK_INIT;
 
git_config(git_default_config, NULL);
 
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index bf87a2710b..ebc43eb805 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -107,8 +107,6 @@ static int git_read_tree_config(const char *var, const char 
*value, void *cb)
return git_default_config(var, value, cb);
 }
 
-static struct lock_file lock_file;
-
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 {
int i, stage = 0;
@@ -116,6 +114,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
struct tree_desc t[MAX_UNPACK_TREES];
struct unpack_trees_options opts;
int prefix_set = 0;
+   struct lock_file lock_file = LOCK_INIT;
const struct option read_tree_options[] = {
{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
  N_("write resulting index to "),
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee81..65b448ef8e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int 
index_only)
return errs;
 }
 
-static struct lock_file lock_file;
-
 static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
 static int ignore_unmatch = 0;
 
@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {
 
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
+   struct lock_file lock_file = LOCK_INIT;
int i;
struct pathspec pathspec;
char *seen;
diff --git a/rerere.c b/rerere.c
inde

[PATCH v2 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

[Martin Ågren]

If we could not take the lock, we add an error to the `strbuf err` and
return. However, this code is dead. The reason is that we take the lock
using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
error-handling to actually kick in.

We could instead just drop the dead code and die here. But everything is
prepared for gently propagating the error, so let's do that instead.

There is similar dead code in `delete_pseudoref()`, but let's save that
for the next patch.

While at it, make the lock non-static. (Placing `struct lock_file`s on
the stack used to be a bad idea, because the temp- and
lockfile-machinery would keep a pointer into the struct. But after
076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we
can safely have lockfiles on the stack.)

Reviewed-by: Stefan Beller <sbel...@google.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 refs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 8b7a77fe5e..8c50b8b139 100644
--- a/refs.c
+++ b/refs.c
@@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
 {
const char *filename;
int fd;
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
struct strbuf buf = STRBUF_INIT;
int ret = -1;
 
@@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
strbuf_addf(, "%s\n", oid_to_hex(oid));
 
filename = git_path("%s", pseudoref);
-   fd = hold_lock_file_for_update_timeout(, filename,
-  LOCK_DIE_ON_ERROR,
+   fd = hold_lock_file_for_update_timeout(, filename, 0,
   get_files_ref_lock_timeout_ms());
if (fd < 0) {
strbuf_addf(err, "could not open '%s' for writing: %s",
-- 
2.17.0.411.g9fd64c8e46

[PATCH v2 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

Placing `struct lock_file`s on the stack used to be a bad idea, because
the temp- and lockfile-machinery would keep a pointer into the struct.
But after 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05), we can safely have lockfiles on the stack. (This applies
even if a user returns early, leaving a locked lock behind.)

These `struct lock_file`s are local to their respective functions and we
can drop their staticness.

For good measure, I have inspected these sites and come to believe that
they always release the lock, with the possible exception of bailing out
using `die()` or `exit()` or by returning from a `cmd_foo()`.

As pointed out by Jeff King, it would be bad if someone held on to a
`struct lock_file *` for some reason. After some grepping, I agree with
his findings: no-one appears to be doing that.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 apply.c| 2 +-
 builtin/describe.c | 2 +-
 builtin/difftool.c | 2 +-
 builtin/gc.c   | 2 +-
 builtin/merge.c| 4 ++--
 builtin/receive-pack.c | 2 +-
 bundle.c   | 2 +-
 fast-import.c  | 2 +-
 refs/files-backend.c   | 2 +-
 shallow.c  | 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996..07b14d1127 100644
--- a/apply.c
+++ b/apply.c
@@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, 
struct patch *list)
 {
struct patch *patch;
struct index_state result = { NULL };
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
int res;
 
/* Once we start supporting the reverse patch, it may be
diff --git a/builtin/describe.c b/builtin/describe.c
index b5afc45846..8bd95cf371 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -612,7 +612,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
suffix = broken;
}
} else if (dirty) {
-   static struct lock_file index_lock;
+   struct lock_file index_lock = LOCK_INIT;
struct rev_info revs;
struct argv_array args = ARGV_ARRAY_INIT;
int fd, result;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index aad0e073ee..162806f238 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -610,7 +610,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
continue;
 
if (!indices_loaded) {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
strbuf_reset();
strbuf_addf(, "%s/wtindex", tmpdir);
if (hold_lock_file_for_update(, buf.buf, 0) < 0 ||
diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124eaa..274660d9dc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -234,7 +234,7 @@ static int need_to_gc(void)
 /* return NULL on success, else hostname running the gc */
 static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
char my_host[HOST_NAME_MAX + 1];
struct strbuf sb = STRBUF_INIT;
struct stat st;
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..de62b2c5c6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -647,7 +647,7 @@ static int try_merge_strategy(const char *strategy, struct 
commit_list *common,
  struct commit_list *remoteheads,
  struct commit *head)
 {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
const char *head_arg = "HEAD";
 
hold_locked_index(, LOCK_DIE_ON_ERROR);
@@ -805,7 +805,7 @@ static int merge_trivial(struct commit *head, struct 
commit_list *remoteheads)
 {
struct object_id result_tree, result_commit;
struct commit_list *parents, **pptr = 
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
 
hold_locked_index(, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4b68a28e92..d3cf36cef5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -876,7 +876,7 @@ static void refuse_unconfigured_deny_delete_current(void)
 static int command_singleton_iterator(void *cb_data, struct object_id *oid);
 static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 {
-   static struct lock_file shallow_lock;
+   struct lock_file shallow_lock = LOCK_INIT;
struct oid_array extra = OID_ARRAY_INIT;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
uint32_t mask = 1 << (cmd->index % 32);
diff --git a/bundle.c b/bundle

Re: [PATCH v2 03/12] commit-graph: test that 'verify' finds corruption

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

> +test_expect_success 'detect bad signature' '
> +   cd "$TRASH_DIRECTORY/full" &&

I was a bit surprised at the "cd outside subshell", but then realized
that this file already does that. It will only be a problem if later
tests think they're somewhere else. Let's read on.

> +   cp $objdir/info/commit-graph commit-graph-backup &&
> +   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +   corrupt_data $objdir/info/commit-graph 0 "\0" &&
> +   test_must_fail git commit-graph verify 2>err &&
> +   grep -v "^\+" err > verify-errors &&
> +   test_line_count = 1 verify-errors &&
> +   grep "graph signature" verify-errors
> +'
> +
> +test_expect_success 'detect bad version number' '
> +   cd "$TRASH_DIRECTORY/full" &&
> +   cp $objdir/info/commit-graph commit-graph-backup &&
> +   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +   corrupt_data $objdir/info/commit-graph 4 "\02" &&
> +   test_must_fail git commit-graph verify 2>err &&
> +   grep -v "^\+" err > verify-errors &&
> +   test_line_count = 1 verify-errors &&
> +   grep "graph version" verify-errors
> +'
> +
> +test_expect_success 'detect bad hash version' '
> +   cd "$TRASH_DIRECTORY/full" &&
> +   cp $objdir/info/commit-graph commit-graph-backup &&
> +   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +   corrupt_data $objdir/info/commit-graph 5 "\02" &&
> +   test_must_fail git commit-graph verify 2>err &&
> +   grep -v "^\+" err > verify-errors &&
> +   test_line_count = 1 verify-errors &&
> +   grep "hash version" verify-errors
> +'

These look a bit boiler-platey. Maybe not too bad though.

> +test_expect_success 'detect too small chunk-count' '

s/too small/bad/?

To be honest, I wrote this title without thinking too hard about the
problem. In general, it would be quite hard for `git commit-graph
verify` to say "*this* is wrong in your file" ("the number of chunks is
too small") -- it should be much easier to say "*something* is wrong".

> +   cd "$TRASH_DIRECTORY/full" &&
> +   cp $objdir/info/commit-graph commit-graph-backup &&
> +   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> +   corrupt_data $objdir/info/commit-graph 6 "\01" &&
> +   test_must_fail git commit-graph verify 2>err &&
> +   grep -v "^\+" err > verify-errors &&
> +   test_line_count = 2 verify-errors &&
> +   grep "missing the OID Lookup chunk" verify-errors &&
> +   grep "missing the Commit Data chunk" verify-errors

Maybe these tests could go with the previous patch(es). IMVHO I would
prefer reading the test with the implementation. A separate commit for
the tests might make sense if they're really tricky and need some
explaining, but I don't think that's the case here.

All of these comments are just minor nits, or not even that. I will
continue with the other patches at another time.

Thank you, I'm really looking forward to Git with commit-graph magic.

Martin

Re: [PATCH v2 01/12] commit-graph: add 'verify' subcommand

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

> graph_name = get_commit_graph_filename(opts.obj_dir);
> graph = load_commit_graph_one(graph_name);
> +   FREE_AND_NULL(graph_name);
>
> if (!graph)
> die("graph file %s does not exist", graph_name);
> -   FREE_AND_NULL(graph_name);

This is probably because of something I said, but this does not look
correct. The `die()` would typically print "(null)" or segfault. If the
`die()` means we don't free `graph_name`, that should be fine.

You're still leaking `graph` here (possibly nothing this patch should
worry about) and in `graph_verify()`. UNLEAK-ing it immediately before
calling `verify_commit_graph()` should be ok. I also think punting on
this UNLEAK-business entirely would be ok; I was just a bit surprised to
see one variable getting freed and the other one ignored.

Martin

Re: [PATCH v2 02/12] commit-graph: verify file header information

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:
> During a run of 'git commit-graph verify', list the issues with the
> header information in the commit-graph file. Some of this information
> is inferred from the loaded 'struct commit_graph'. Some header
> information is checked as part of load_commit_graph_one().
>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 32 +++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index b25aaed128..d2db20e49a 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -818,7 +818,37 @@ void write_commit_graph(const char *obj_dir,
> oids.nr = 0;
>  }
>
> +static int verify_commit_graph_error;
> +
> +static void graph_report(const char *fmt, ...)
> +{
> +   va_list ap;
> +   struct strbuf sb = STRBUF_INIT;
> +   verify_commit_graph_error = 1;
> +
> +   va_start(ap, fmt);
> +   strbuf_vaddf(, fmt, ap);
> +
> +   fprintf(stderr, "%s\n", sb.buf);
> +   strbuf_release();
> +   va_end(ap);
> +}

Right, so this replaces the macro-trickery from v1, and we print a
newline after each error.

>  int verify_commit_graph(struct commit_graph *g)
>  {
> -   return !g;
> +   if (!g) {
> +   graph_report("no commit-graph file loaded");
> +   return 1;
> +   }

> +
> +   return verify_commit_graph_error;
>  }

Not sure it matters much: I suppose you could introduce the parts that I
have quoted here in the previous patch. Or maybe not.

Martin

Re: [PATCH v2 04/12] commit-graph: parse commit from chosen graph

[Martin Ågren]

> -int parse_commit_in_graph(struct commit *item)
> +int parse_commit_in_graph_one(struct commit_graph *g, struct commit *item)

I think this function should be static.

Re: [PATCH v2 07/12] commit-graph: load a root tree from specific graph

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

> -struct tree *get_commit_tree_in_graph(const struct commit *c)
> +static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
> +const struct commit *c)
>  {
> if (c->maybe_tree)
> return c->maybe_tree;
> if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
> BUG("get_commit_tree_in_graph called from non-commit-graph 
> commit");

Update the function name in the BUG? Not that it will ever matter. ;-)

(This one is now static, ok.)

Re: [PATCH v2 08/12] commit-graph: verify commit contents against odb

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:
> When running 'git commit-graph verify', compare the contents of the
> commits that are loaded from the commit-graph file with commits that are
> loaded directly from the object database. This includes checking the
> root tree object ID, commit date, and parents.
>
> Parse the commit from the graph during the initial loop through the
> object IDs to guarantee we parse from the commit-graph file.
>
> In addition, verify the generation number calculation is correct for all
> commits in the commit-graph file.
>
> While testing, we discovered that mutating the integer value for a
> parent to be outside the accepted range causes a segmentation fault. Add
> a new check in insert_parent_or_die() that prevents this fault. Check
> for that error during the test, both in the typical parents and in the
> list of parents for octopus merges.

This paragraph and the corresponding fix and test feel like a separate
patch to me. (The commit message of it could be "To test the next patch,
we threw invalid data at `git commit-graph verify, and it crashed in
pre-existing code, so let's fix that first" -- there is definitely a
connection.) Is this important enough to fast-track to master in time
for 2.18? My guess would be no.

> +
> +   if (pos >= g->num_commits)
> +   die("invalide parent position %"PRIu64, pos);

s/invalide/invalid/

> @@ -875,6 +879,8 @@ int verify_commit_graph(struct commit_graph *g)
> return 1;
>
> for (i = 0; i < g->num_commits; i++) {
> +   struct commit *graph_commit;
> +
> hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
>
> if (i && oidcmp(_oid, _oid) >= 0)
> @@ -892,6 +898,10 @@ int verify_commit_graph(struct commit_graph *g)
>
> cur_fanout_pos++;
> }
> +
> +   graph_commit = lookup_commit(_oid);
> +   if (!parse_commit_in_graph_one(g, graph_commit))
> +   graph_report("failed to parse %s from commit-graph", 
> oid_to_hex(_oid));
> }

Could this end up giving ridiculous amounts of output? It would depend
on the input, I guess.

> while (cur_fanout_pos < 256) {
> @@ -904,5 +914,95 @@ int verify_commit_graph(struct commit_graph *g)
> cur_fanout_pos++;
> }
>
> +   if (verify_commit_graph_error)
> +   return 1;

Well, here we give up before running into *too* much problem.

> +   for (i = 0; i < g->num_commits; i++) {
> +   struct commit *graph_commit, *odb_commit;
> +   struct commit_list *graph_parents, *odb_parents;
> +   int num_parents = 0;
> +
> +   hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);
> +
> +   graph_commit = lookup_commit(_oid);
> +   odb_commit = (struct commit *)create_object(cur_oid.hash, 
> alloc_commit_node());
> +   if (parse_commit_internal(odb_commit, 0, 0)) {
> +   graph_report("failed to parse %s from object 
> database", oid_to_hex(_oid));
> +   continue;
> +   }
> +
> +   if (oidcmp(_commit_tree_in_graph_one(g, 
> graph_commit)->object.oid,
> +  get_commit_tree_oid(odb_commit)))
> +   graph_report("root tree object ID for commit %s in 
> commit-graph is %s != %s",
> +oid_to_hex(_oid),
> +
> oid_to_hex(get_commit_tree_oid(graph_commit)),
> +
> oid_to_hex(get_commit_tree_oid(odb_commit)));
> +
> +   if (graph_commit->date != odb_commit->date)
> +   graph_report("commit date for commit %s in 
> commit-graph is %"PRItime" != %"PRItime"",
> +oid_to_hex(_oid),
> +graph_commit->date,
> +odb_commit->date);
> +
> +
> +   graph_parents = graph_commit->parents;
> +   odb_parents = odb_commit->parents;
> +
> +   while (graph_parents) {
> +   num_parents++;
> +
> +   if (odb_parents == NULL)
> +   graph_report("commit-graph parent list for 
> commit %s is too long (%d)",
> +oid_to_hex(_oid),
> +num_parents);
> +
> +   if (oidcmp(_parents->item->object.oid, 
> _parents->item->object.oid))
> +   graph_report("commit-graph parent for %s is 
> %s != %s",
> +oid_to_hex(_oid),
> +
> oid_to_hex(_parents->item->object.oid),
> +
> oid_to_hex(_parents->item->object.oid));
> +

Re: [PATCH v2 06/12] commit: force commit to parse from object database

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

> -int parse_commit_gently(struct commit *item, int quiet_on_missing)
> +int parse_commit_internal(struct commit *item, int quiet_on_missing, int 
> use_commit_graph)
>  {
> enum object_type type;
> void *buffer;
> @@ -403,17 +403,17 @@ int parse_commit_gently(struct commit *item, int 
> quiet_on_missing)
> return -1;
> if (item->object.parsed)
> return 0;
> -   if (parse_commit_in_graph(item))
> +   if (use_commit_graph && parse_commit_in_graph(item))
> return 0;

Right, this is where we check the graph. It's the only place we need to
consider the new flag.

> buffer = read_sha1_file(item->object.oid.hash, , );
> if (!buffer)
> return quiet_on_missing ? -1 :
> error("Could not read %s",
> -oid_to_hex(>object.oid));
> +   oid_to_hex(>object.oid));
> if (type != OBJ_COMMIT) {
> free(buffer);
> return error("Object %s not a commit",
> -oid_to_hex(>object.oid));
> +   oid_to_hex(>object.oid));

Some spurious indentation reshuffling going on in two lines here.

> --- a/commit.h
> +++ b/commit.h
> @@ -73,6 +73,7 @@ struct commit *lookup_commit_reference_by_name(const char 
> *name);
>  struct commit *lookup_commit_or_die(const struct object_id *oid, const char 
> *ref_name);
>
>  int parse_commit_buffer(struct commit *item, const void *buffer, unsigned 
> long size, int check_graph);
> +int parse_commit_internal(struct commit *item, int quiet_on_missing, int 
> use_commit_graph);

Unlike my comment on a previous patch, this one is meant for external
use. That's why it's not marked as static above. Ok.

Martin

[PATCH 3/1] config: let `config_store_data_clear()` handle `key`

[Martin Ågren]

Instead of carefully clearing up `key` in each code path, let
`config_store_data_clear()` handle that.

We still need to free it before replacing it, though. Move that freeing
closer to the replacing to be safe. Note that in that same part of the
code, we can no longer set `key` to the original pointer, but need to
`xstrdup()` it.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 config.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index 2e3c6c94e9..963edacf10 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,7 @@ struct config_store_data {
 
 void config_store_data_clear(struct config_store_data *store)
 {
+   free(store->key);
if (store->value_regex != NULL &&
store->value_regex != CONFIG_REGEX_NONE) {
regfree(store->value_regex);
@@ -2679,7 +2680,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
fd = hold_lock_file_for_update(, config_filename, 0);
if (fd < 0) {
error_errno("could not lock config file %s", config_filename);
-   free(store.key);
ret = CONFIG_NO_LOCK;
goto out_free;
}
@@ -2689,8 +2689,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 */
in_fd = open(config_filename, O_RDONLY);
if ( in_fd < 0 ) {
-   free(store.key);
-
if ( ENOENT != errno ) {
error_errno("opening %s", config_filename);
ret = CONFIG_INVALID_FILE; /* same as "invalid config 
file" */
@@ -2702,7 +2700,8 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
goto out_free;
}
 
-   store.key = (char *)key;
+   free(store.key);
+   store.key = xstrdup(key);
if (write_section(fd, key, ) < 0 ||
write_pair(fd, key, value, ) < 0)
goto write_err_out;
@@ -2751,13 +2750,10 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  config_filename,
  , )) {
error("invalid config file %s", config_filename);
-   free(store.key);
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
-   free(store.key);
-
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
(store.seen_nr > 1 && multi_replace == 0)) {
-- 
2.17.0.583.g9a75a153ac

[PATCH 2/1] config: let `config_store_data_clear()` handle `value_regex`

[Martin Ågren]

Instead of carefully clearing up `value_regex` in each code path, let
`config_store_data_clear()` handle that.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
I *think* that it should be ok to `regfree()` after `regcomp()` failed,
but I'll need to look into that some more (and say something about it in
the commit message).

 config.c | 16 +---
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 83d7d0851a..2e3c6c94e9 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,11 @@ struct config_store_data {
 
 void config_store_data_clear(struct config_store_data *store)
 {
+   if (store->value_regex != NULL &&
+   store->value_regex != CONFIG_REGEX_NONE) {
+   regfree(store->value_regex);
+   free(store->value_regex);
+   }
free(store->parsed);
free(store->seen);
memset(store, 0, sizeof(*store));
@@ -2722,7 +2727,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
if (regcomp(store.value_regex, value_regex,
REG_EXTENDED)) {
error("invalid pattern: %s", value_regex);
-   free(store.value_regex);
ret = CONFIG_INVALID_PATTERN;
goto out_free;
}
@@ -2748,21 +2752,11 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  , )) {
error("invalid config file %s", config_filename);
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
 
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
-- 
2.17.0.583.g9a75a153ac

Re: [PATCH] config: free resources of `struct config_store_data`

[Martin Ågren]

On 13 May 2018 at 10:59, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Sun, May 13, 2018 at 4:23 AM Martin Ågren <martin.ag...@gmail.com> wrote:
>
>> Introduce and use a small helper function `config_store_data_clear()` to
>> plug these leaks. This should be safe. The memory tracked here is config
>> parser events. Once the users (`git_config_set_multivar_in_file_gently()`
>> and `git_config_copy_or_rename_section_in_file()` at the moment) are
>> done, no-one should be holding on to a pointer into this memory.
>
> A newcomer to this code (such as myself) might legitimately wonder why
> store->key and store->value_regex are not also being cleaned up by this

Good point. I was only concerned by the members that no-one took
responsibility for.

> function. An examination of the relevant code reveals that those structure
> members are manually (and perhaps tediously) freed already by
> git_config_set_multivar_in_file_gently(), however, if
> config_store_data_clear() was responsible for freeing them, then
> git_config_set_multivar_in_file_gently() could be made a bit less noisy.

Yes, that's true.

> On the other hand, if there's actually a good reason for
>   config_store_data_clear() not freeing those members, then perhaps it
> deserves an in-code comment explaining why they are exempt.

Not any good reason that I can think of, other than "history". But to be
clear, a day ago I was as much of a newcomer in this part of the code as
you are. Johannes is the one who might have the most up-to-date
understanding of this.

How about the following two patches as patches 2/3 and 3/3? I would also
need to mention in the commit message of this patch (1/3) that the
function will soon learn to clean up more members.

I could of course squash the three patches into one, but there is some
minor trickery involved, like we can't reuse a pointer in one place, but
need to xstrdup it.

Thank you for your comments. I'd be very interested in your thoughts on
this.

Martin

Martin Ågren (2):
  config: let `config_store_data_clear()` handle `value_regex`
  config: let `config_store_data_clear()` handle `key`

 config.c | 26 --
 1 file changed, 8 insertions(+), 18 deletions(-)

-- 
2.17.0.583.g9a75a153ac

[PATCH] config: free resources of `struct config_store_data`

[Martin Ågren]

Commit fee8572c6d (config: avoid using the global variable `store`,
2018-04-09) dropped the staticness of a certain struct, instead letting
the users create an instance on the stack and pass around a pointer.

We do not free the memory that the struct tracks. When the struct was
static, the memory would always be reachable. Now that we keep the
struct on the stack, though, as soon as we return, it goes out of scope
and we leak the memory it points to.

Introduce and use a small helper function `config_store_data_clear()` to
plug these leaks. This should be safe. The memory tracked here is config
parser events. Once the users (`git_config_set_multivar_in_file_gently()`
and `git_config_copy_or_rename_section_in_file()` at the moment) are
done, no-one should be holding on to a pointer into this memory.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 config.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/config.c b/config.c
index 6f8f1d8c11..83d7d0851a 100644
--- a/config.c
+++ b/config.c
@@ -2333,6 +2333,13 @@ struct config_store_data {
unsigned int key_seen:1, section_seen:1, is_keys_section:1;
 };
 
+void config_store_data_clear(struct config_store_data *store)
+{
+   free(store->parsed);
+   free(store->seen);
+   memset(store, 0, sizeof(*store));
+}
+
 static int matches(const char *key, const char *value,
   const struct config_store_data *store)
 {
@@ -2887,6 +2894,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
munmap(contents, contents_sz);
if (in_fd >= 0)
close(in_fd);
+   config_store_data_clear();
return ret;
 
 write_err_out:
@@ -3127,6 +3135,7 @@ static int 
git_config_copy_or_rename_section_in_file(const char *config_filename
rollback_lock_file();
 out_no_rollback:
free(filename_buf);
+   config_store_data_clear();
return ret;
 }
 
-- 
2.17.0.583.g9a75a153ac

Re: [PATCH] config: free resources of `struct config_store_data`

[Martin Ågren]

On 13 May 2018 at 10:23, Martin Ågren <martin.ag...@gmail.com> wrote:

> +void config_store_data_clear(struct config_store_data *store)

I will do s/void/static void/ here...

Re: [PATCH 00/12] Integrate commit-graph into fsck, gc, and fetch

[Martin Ågren]

On 11 May 2018 at 19:23, Derrick Stolee  wrote:

> Martin's initial test cases are wonderful. I've adapted them to test the
> other conditions in the verify_commit_graph() method and caught some
> interesting behavior in the process. I'm preparing v2 so we can investigate
> the direction of the tests.

Cool, I'm glad you found them useful. One thought I had was that you
could possibly write the tests such that you introduce errors from the
back of the file. That might enable you to do less of the "backup
commit-graph file and restore it"-dance. Just a thought.

Martin

Re: [PATCH 00/12] Integrate commit-graph into fsck, gc, and fetch

[Martin Ågren]

On 10 May 2018 at 21:22, Stefan Beller <sbel...@google.com> wrote:
> On Thu, May 10, 2018 at 12:05 PM, Martin Ågren <martin.ag...@gmail.com> wrote:

>> I hope to find time to do some more hands-on testing of this to see that
>> errors actually do get caught.

> Packfiles and loose objects are primary data, which means that those
> need a more advanced way to diagnose and repair them, so I would imagine
> the commit graph fsck is closer to bitmaps fsck, which I would have suspected
> to be found in t5310, but a quick read doesn't reveal many tests that are
> checking for integrity. So I guess the test coverage here is ok, (although we
> should always ask for more)

Since I'm wrapping up for today, I'm posting some simple tests that I
assembled. The last of these showcases one or two problems with the
current error-reporting. Depending on the error, there can be *lots* of
errors reported and there are no new-lines, so the result on stdout can
be a wall of not-very-legible text.

Some of these might not make sense. I just started going through the
documentation on the format, causing some sort of corruption in each
field. Maybe this can be helpful somehow.

Martin

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 82f95eb11f..a7e48db2de 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -255,4 +255,49 @@ test_expect_success 'git fsck (checks commit-graph)' '
git fsck
 '
 
+# usage: corrupt_data   []
+corrupt_data() {
+   file=$1
+   pos=$2
+   data="${3:-\0}"
+   printf "$data" | dd of="$file" bs=1 seek="$pos" conv=notrunc
+}
+
+test_expect_success 'detect bad signature' '
+   cd "$TRASH_DIRECTORY/full" &&
+   cp $objdir/info/commit-graph commit-graph-backup &&
+   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+   corrupt_data $objdir/info/commit-graph 0 "\0" &&
+   test_must_fail git commit-graph verify 2>err &&
+   grep "graph signature" err
+'
+
+test_expect_success 'detect bad version number' '
+   cd "$TRASH_DIRECTORY/full" &&
+   cp $objdir/info/commit-graph commit-graph-backup &&
+   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+   corrupt_data $objdir/info/commit-graph 4 "\02" &&
+   test_must_fail git commit-graph verify 2>err &&
+   grep "graph version" err
+'
+
+test_expect_success 'detect bad hash version' '
+   cd "$TRASH_DIRECTORY/full" &&
+   cp $objdir/info/commit-graph commit-graph-backup &&
+   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+   corrupt_data $objdir/info/commit-graph 5 "\02" &&
+   test_must_fail git commit-graph verify 2>err &&
+   grep "hash version" err
+'
+
+test_expect_success 'detect too small chunk-count' '
+   cd "$TRASH_DIRECTORY/full" &&
+   cp $objdir/info/commit-graph commit-graph-backup &&
+   test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
+   corrupt_data $objdir/info/commit-graph 6 "\01" &&
+   test_must_fail git commit-graph verify 2>err &&
+   cat err
+'
+
+
 test_done
-- 
2.17.0

Re: [PATCH 01/28] t/test-lib: add an SHA1 prerequisite

[Martin Ågren]

On 8 May 2018 at 01:30, brian m. carlson <sand...@crustytoothpaste.net> wrote:
> On Mon, May 07, 2018 at 12:10:39PM +0200, Martin Ågren wrote:
>> On 7 May 2018 at 01:17, brian m. carlson <sand...@crustytoothpaste.net> 
>> wrote:
>> > Add an SHA1 prerequisite to annotate both of these types of tests and
>> > disable them when we're using a different hash.  In the future, we can
>> > create versions of these tests which handle both SHA-1 and NewHash.
>>
>> Minor nit: s/can/can and should/
>
> I agree with that sentiment.  I can change that.

To be clear, this was an "if you have independent reasons for rerolling
this, then maybe consider possibly doing this".

>> So SHA1 means roughly "git hash-object uses SHA-1, so supposedly
>> everything on disk is SHA-1." I could imagine one or two different
>> meanings: "Git was compiled with support for SHA-1 [oids]."
>
> Currently it means that, yes.  It may specialize to mean, "git emits
> SHA-1 output, regardless of the format on disk."  See cases 1 and 2
> below.
>
>> Do we actually need more SHA-1-related prereqs, at least long-term, in
>> which case we would want to find a more specific name for this one now?
>> Is this SHA1_STORAGE, or some much better name than that?
>
> We may.  The transition plan anticipates several states:

"We may" as in, "we may need more SHA1-FOO prereqs later", or "we may
want this to be SHA1-BAR"?

> 1. Store data in NewHash, but input and output are SHA-1.
> 2. Store data in NewHash; output is SHA-1; input can be either.
> 3. Store data and output in NewHash; input can be either.
> 4. All NewHash.
>
> At this point, I'm working on getting the tests to handle case 4, as
> that's actually the easiest to fix (because if things are wrong, the
> code tends to segfault).  Case 1 will be next, in which case, we may
> need SHA1_STORAGE or such.

> I plan to make the SHA1 prerequisite go away or at least be far less
> used in a few releases.  Once we know what NewHash is going to be, it's
> pretty easy to write tooling and translation tables that do the
> conversion for most tests, and a test helper can simply emit the right
> output for whichever algorithm we're using in that situation, whether
> that's the on-disk algorithm, the input algorithm, or the output
> algorithm.

I do not feel entirely relaxed about a reasoning such as "this prereq
will soon go away again, so we do not need to think too much about its
name and meaning" (heavily paraphrased and possibly a bit pointed, but
hopefully not too dishonest).

I guess a counter-argument might be "sure, if only we knew which
SHA1-FOOs we will need. Only time and experience will tell." You've
certainly spent way more brain-cycles on this than I have, and most
likely more than anyone else on this list.

Maybe we want to document the transition-ness of this in the code and/or
the commit message. Not only "transition" in the sense of the big
transition, but in the sense of "this will probably go away long before
the transition is completed."

>> I am thinking for example about a repo with NewHash that gets pushed to
>> and fetched from a SHA-1 server, see hash-function-transition.txt, goal
>> 1b. We'd want to always test that SHA-1-related functionality in git.
>> (But only until the day when someone defines a prereq such as "SHA1" to
>> be able to test a git that was compiled without any traces of SHA-1
>> whatsoever.)
>
> I anticipate that by the time we get to that point, the SHA1
> prerequisite will be long gone and can be reused for that purpose,
> should we need it.

Re: [PATCH 06/28] t0000: annotate with SHA1 prerequisite

[Martin Ågren]

On 8 May 2018 at 01:40, brian m. carlson <sand...@crustytoothpaste.net> wrote:
> On Mon, May 07, 2018 at 12:24:13PM +0200, Martin Ågren wrote:
>> This could be more centralized at the top of the file, more likely to be
>> noticed during a `make test` and easier to adapt in the future. Maybe
>> something like this at the top of the file:
>>
>> if hash for storage is SHA-1 then
>> knowntree=7bb943559a305bdd6bdee2cef6e5df2413c3d30a
>> path0=f87290f8eb2cbbea7857214459a0739927eab154
>> 
>> else
>> skip_all='unknown hash, but you really need to expand this test'
>> # or even error out!
>> fi
>
> As I mentioned in an earlier email, I plan to set an environment
> variable for the algorithms in use and then do something like:
>
>   test "$tree" = "$(test-tool hash-helper --output known-tree)"
>
> where "known-tree" is some key we can use to look up the SHA-1 or
> NewHash value, and we've specified we want the output format (as opposed
> to input format or on-disk format).

My first thought was, can't we introduce such a helper already now? It
would not check with the environment, but would always output SHA-1
values. Thinking about it some more, maybe the exact usage/interface
would be a bit volatile in the beginning, making it just as good an idea
to gain some more experience and feeling first.

>> When we add NewHash, we get to add an "else if". Otherwise, we'd be
>> plugging in NewHash without testing some very basic stuff.
>>
>> Ok, so `skip_all` is quite hard, but skipping certain basic tests also
>> feels really shaky. Adding a new hash algo without adapting this test
>> should be a no-go, IMHO.
>
> I agree that this test needs to be updated for NewHash, but since we
> haven't decided what that's going to be, it makes sense during
> development to still test the rest of the code if possible so that we
> know what does and doesn't work.
>
> This is a first step in making it obvious what doesn't work, and when we
> know what the data is supposed to be, we can adjust it by fixing the
> tests so that it works in all cases.

A lingering feeling is, how do we find all these tests that we "hide"
(for lack of a better word)? Maybe introduce some standardized comment
keyword. Or use a more grep-friendly prereq-name. Or, if all occurances
of "SHA1" (outside of t-*sha*) are expected to go away real soon
now, maybe this is not an issue.

Anyway, thanks for putting up with me, and thanks a big lot for driving
this major undertaking forward.

Martin

Re: [PATCH 2/2] Documentation: render revisions correctly under Asciidoctor

[Martin Ågren]

On 8 May 2018 at 03:13, brian m. carlson <sand...@crustytoothpaste.net> wrote:
> On Mon, May 07, 2018 at 06:11:43AM +0200, Martin Ågren wrote:
>> Excellent. These two patches fix my original problem and seem like the
>> obviously correct approach (in hindsight ;-) ). I wonder if the diagrams
>> earlier in the file should be tackled also. Right now, one has a huge
>> number of dots instead of the four you added; the other has none. They
>> do render fine though, so that'd only be about consistency in the
>> original .txt-file.
>
> Yeah, the number of dots has to be at least four, but can be any
> matching number.
>
> Since this patch fixes the present issue, I'd like to leave it as it is
> and run through a cleanup series a bit later that catches all the
> literal indented blocks and marks them explicitly to avoid this issue in
> the future.

Sounds like a plan. :-) As noted elsewhere, I have no immediate idea how
to identify which of the 13000 tab-indented lines are really part of
diagrams and ascii-art. Counting the number of spaces? Anyway, thanks
for this mini-series.

Martin

[PATCH] refs: handle null-oid for pseudorefs

[Martin Ågren]

According to the documentation on `git update-ref`, it is possible to
"specify 40 '0' or an empty string as  to make sure that the
ref you are creating does not exist." But in the code for pseudorefs, we
do not implement this. If we fail to read the old ref, we immediately
die. A failure to read would actually be a good thing if we have been
given the null-oid.

With the null-oid, allow -- and even require -- the ref-reading to fail.
This implements the "make sure that the ref ... does not exist" part of
the documentation.

Since we have a `strbuf err` for collecting errors, let's use it and
signal an error to the caller instead of dying hard.

Reported-by: Rafael Ascensão <rafa.al...@gmail.com>
Helped-by: Rafael Ascensão <rafa.al...@gmail.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
(David's twopensource-address bounced, so I'm trying instead the one he
most recently posted from.)

 t/t1400-update-ref.sh |  7 +++
 refs.c| 19 +++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 664a3a4e4e..bd41f86f22 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -457,6 +457,13 @@ test_expect_success 'git cat-file blob master@{2005-05-26 
23:42}:F (expect OTHER
test OTHER = $(git cat-file blob "master@{2005-05-26 23:42}:F")
 '
 
+test_expect_success 'create pseudoref with old oid null, but do not overwrite' 
'
+   git update-ref PSEUDOREF $A $Z &&
+   test_when_finished "git update-ref -d PSEUDOREF" &&
+   test $A = $(cat .git/PSEUDOREF) &&
+   test_must_fail git update-ref PSEUDOREF $A $Z
+'
+
 a=refs/heads/a
 b=refs/heads/b
 c=refs/heads/c
diff --git a/refs.c b/refs.c
index 8b7a77fe5e..3669190499 100644
--- a/refs.c
+++ b/refs.c
@@ -666,10 +666,21 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
if (old_oid) {
struct object_id actual_old_oid;
 
-   if (read_ref(pseudoref, _old_oid))
-   die("could not read ref '%s'", pseudoref);
-   if (oidcmp(_old_oid, old_oid)) {
-   strbuf_addf(err, "unexpected sha1 when writing '%s'", 
pseudoref);
+   if (read_ref(pseudoref, _old_oid)) {
+   if (!is_null_oid(old_oid)) {
+   strbuf_addf(err, "could not read ref '%s'",
+   pseudoref);
+   rollback_lock_file();
+   goto done;
+   }
+   } else if (is_null_oid(old_oid)) {
+   strbuf_addf(err, "ref '%s' already exists",
+   pseudoref);
+   rollback_lock_file();
+   goto done;
+   } else if (oidcmp(_old_oid, old_oid)) {
+   strbuf_addf(err, "unexpected sha1 when writing '%s'",
+   pseudoref);
rollback_lock_file();
goto done;
}
-- 
2.17.0.411.g9fd64c8e46

[PATCH 2/5] refs.c: do not die if locking fails in `write_pseudoref()`

[Martin Ågren]

If we could not take the lock, we add an error to the `strbuf err` and
return. However, this code is dead. The reason is that we take the lock
using `LOCK_DIE_ON_ERROR`. Drop the flag to allow our more gentle
error-handling to actually kick in.

We could instead just drop the dead code and die here. But everything is
prepared for gently propagating the error, so let's do that instead.

There is similar dead code in `delete_pseudoref()`, but let's save that
for the next patch.

While at it, make the lock non-static.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 refs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 8b7a77fe5e..8c50b8b139 100644
--- a/refs.c
+++ b/refs.c
@@ -644,7 +644,7 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
 {
const char *filename;
int fd;
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
struct strbuf buf = STRBUF_INIT;
int ret = -1;
 
@@ -654,8 +654,7 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
strbuf_addf(, "%s\n", oid_to_hex(oid));
 
filename = git_path("%s", pseudoref);
-   fd = hold_lock_file_for_update_timeout(, filename,
-  LOCK_DIE_ON_ERROR,
+   fd = hold_lock_file_for_update_timeout(, filename, 0,
   get_files_ref_lock_timeout_ms());
if (fd < 0) {
strbuf_addf(err, "could not open '%s' for writing: %s",
-- 
2.17.0.411.g9fd64c8e46

[PATCH 4/5] lock_file: make function-local locks non-static

[Martin Ågren]

These `struct lock_file`s are local to their respective functions and we
can drop their staticness.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 apply.c| 2 +-
 builtin/describe.c | 2 +-
 builtin/difftool.c | 2 +-
 builtin/gc.c   | 2 +-
 builtin/merge.c| 4 ++--
 builtin/receive-pack.c | 2 +-
 bundle.c   | 2 +-
 fast-import.c  | 2 +-
 refs/files-backend.c   | 2 +-
 shallow.c  | 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/apply.c b/apply.c
index 7e5792c996..07b14d1127 100644
--- a/apply.c
+++ b/apply.c
@@ -4058,7 +4058,7 @@ static int build_fake_ancestor(struct apply_state *state, 
struct patch *list)
 {
struct patch *patch;
struct index_state result = { NULL };
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
int res;
 
/* Once we start supporting the reverse patch, it may be
diff --git a/builtin/describe.c b/builtin/describe.c
index b5afc45846..8bd95cf371 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -612,7 +612,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
suffix = broken;
}
} else if (dirty) {
-   static struct lock_file index_lock;
+   struct lock_file index_lock = LOCK_INIT;
struct rev_info revs;
struct argv_array args = ARGV_ARRAY_INIT;
int fd, result;
diff --git a/builtin/difftool.c b/builtin/difftool.c
index aad0e073ee..162806f238 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -610,7 +610,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
continue;
 
if (!indices_loaded) {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
strbuf_reset();
strbuf_addf(, "%s/wtindex", tmpdir);
if (hold_lock_file_for_update(, buf.buf, 0) < 0 ||
diff --git a/builtin/gc.c b/builtin/gc.c
index 3e67124eaa..274660d9dc 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -234,7 +234,7 @@ static int need_to_gc(void)
 /* return NULL on success, else hostname running the gc */
 static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
char my_host[HOST_NAME_MAX + 1];
struct strbuf sb = STRBUF_INIT;
struct stat st;
diff --git a/builtin/merge.c b/builtin/merge.c
index 9db5a2cf16..de62b2c5c6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -647,7 +647,7 @@ static int try_merge_strategy(const char *strategy, struct 
commit_list *common,
  struct commit_list *remoteheads,
  struct commit *head)
 {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
const char *head_arg = "HEAD";
 
hold_locked_index(, LOCK_DIE_ON_ERROR);
@@ -805,7 +805,7 @@ static int merge_trivial(struct commit *head, struct 
commit_list *remoteheads)
 {
struct object_id result_tree, result_commit;
struct commit_list *parents, **pptr = 
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
 
hold_locked_index(, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4b68a28e92..d3cf36cef5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -876,7 +876,7 @@ static void refuse_unconfigured_deny_delete_current(void)
 static int command_singleton_iterator(void *cb_data, struct object_id *oid);
 static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 {
-   static struct lock_file shallow_lock;
+   struct lock_file shallow_lock = LOCK_INIT;
struct oid_array extra = OID_ARRAY_INIT;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
uint32_t mask = 1 << (cmd->index % 32);
diff --git a/bundle.c b/bundle.c
index 902c9b5448..160bbfdc64 100644
--- a/bundle.c
+++ b/bundle.c
@@ -409,7 +409,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info 
*revs)
 int create_bundle(struct bundle_header *header, const char *path,
  int argc, const char **argv)
 {
-   static struct lock_file lock;
+   struct lock_file lock = LOCK_INIT;
int bundle_fd = -1;
int bundle_to_stdout;
int ref_count = 0;
diff --git a/fast-import.c b/fast-import.c
index 05d1079d23..09443c1a9d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1817,7 +1817,7 @@ static void dump_marks_helper(FILE *f,
 
 static void dump_marks(void)
 {
-   static struct lock_file mark_lock;
+   struct lock_file 

[PATCH 5/5] lock_file: move static locks into functions

[Martin Ågren]

Each of these `struct lock_file`s is used from within a single function.
Move them into the respective functions to make the scope clearer. While
doing so, we can drop the staticness.

After this commit, the remaing occurences of "static struct lock_file"
are locks that are used from within different functions. That is, they
need to remain static. (Short of more intrusive changes like passing
around pointers to non-static locks.)

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 t/helper/test-scrap-cache-tree.c | 4 ++--
 builtin/add.c| 3 +--
 builtin/mv.c | 2 +-
 builtin/read-tree.c  | 3 +--
 builtin/rm.c | 3 +--
 rerere.c | 3 +--
 6 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index d26d3e7c8b..393f1604ff 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -4,10 +4,10 @@
 #include "tree.h"
 #include "cache-tree.h"
 
-static struct lock_file index_lock;
-
 int cmd__scrap_cache_tree(int ac, const char **av)
 {
+   struct lock_file index_lock = LOCK_INIT;
+
setup_git_directory();
hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
if (read_cache() < 0)
diff --git a/builtin/add.c b/builtin/add.c
index c9e2619a9a..8a155dd41e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -265,8 +265,6 @@ static int edit_patch(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static struct lock_file lock_file;
-
 static const char ignore_error[] =
 N_("The following paths are ignored by one of your .gitignore files:\n");
 
@@ -393,6 +391,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
int add_new_files;
int require_pathspec;
char *seen = NULL;
+   struct lock_file lock_file = LOCK_INIT;
 
git_config(add_config, NULL);
 
diff --git a/builtin/mv.c b/builtin/mv.c
index 6d141f7a53..b4692409e3 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -72,7 +72,6 @@ static const char *add_slash(const char *path)
return path;
 }
 
-static struct lock_file lock_file;
 #define SUBMODULE_WITH_GITDIR ((const char *)1)
 
 static void prepare_move_submodule(const char *src, int first,
@@ -131,6 +130,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
enum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;
struct stat st;
struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
+   struct lock_file lock_file = LOCK_INIT;
 
git_config(git_default_config, NULL);
 
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index bf87a2710b..ebc43eb805 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -107,8 +107,6 @@ static int git_read_tree_config(const char *var, const char 
*value, void *cb)
return git_default_config(var, value, cb);
 }
 
-static struct lock_file lock_file;
-
 int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 {
int i, stage = 0;
@@ -116,6 +114,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
struct tree_desc t[MAX_UNPACK_TREES];
struct unpack_trees_options opts;
int prefix_set = 0;
+   struct lock_file lock_file = LOCK_INIT;
const struct option read_tree_options[] = {
{ OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
  N_("write resulting index to "),
diff --git a/builtin/rm.c b/builtin/rm.c
index 5b6fc7ee81..65b448ef8e 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -233,8 +233,6 @@ static int check_local_mod(struct object_id *head, int 
index_only)
return errs;
 }
 
-static struct lock_file lock_file;
-
 static int show_only = 0, force = 0, index_only = 0, recursive = 0, quiet = 0;
 static int ignore_unmatch = 0;
 
@@ -251,6 +249,7 @@ static struct option builtin_rm_options[] = {
 
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
+   struct lock_file lock_file = LOCK_INIT;
int i;
struct pathspec pathspec;
char *seen;
diff --git a/rerere.c b/rerere.c
index 18cae2d11c..e0862e2778 100644
--- a/rerere.c
+++ b/rerere.c
@@ -703,10 +703,9 @@ static int merge(const struct rerere_id *id, const char 
*path)
return ret;
 }
 
-static struct lock_file index_lock;
-
 static void update_paths(struct string_list *update)
 {
+   struct lock_file index_lock = LOCK_INIT;
int i;
 
hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
-- 
2.17.0.411.g9fd64c8e46

[PATCH 1/5] t/helper/test-write-cache: clean up lock-handling

[Martin Ågren]

Die in case writing the index fails, so that the caller can notice
(instead of, say, being impressed by how performant the writing is).

While at it, note that after opening a lock with `LOCK_DIE_ON_ERROR`, we
do not need to worry about whether we succeeded. Also, we can move the
`struct lock_file` into the function and drop the staticness.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 t/helper/test-write-cache.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/t/helper/test-write-cache.c b/t/helper/test-write-cache.c
index 017dc30380..8837717d36 100644
--- a/t/helper/test-write-cache.c
+++ b/t/helper/test-write-cache.c
@@ -2,22 +2,18 @@
 #include "cache.h"
 #include "lockfile.h"
 
-static struct lock_file index_lock;
-
 int cmd__write_cache(int argc, const char **argv)
 {
-   int i, cnt = 1, lockfd;
+   struct lock_file index_lock = LOCK_INIT;
+   int i, cnt = 1;
if (argc == 2)
cnt = strtol(argv[1], NULL, 0);
setup_git_directory();
read_cache();
for (i = 0; i < cnt; i++) {
-   lockfd = hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
-   if (0 <= lockfd) {
-   write_locked_index(_index, _lock, 
COMMIT_LOCK);
-   } else {
-   rollback_lock_file(_lock);
-   }
+   hold_locked_index(_lock, LOCK_DIE_ON_ERROR);
+   if (write_locked_index(_index, _lock, COMMIT_LOCK))
+   die("unable to write index file");
}
 
return 0;
-- 
2.17.0.411.g9fd64c8e46

[PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`

[Martin Ågren]

After taking the lock we check whether we got it and die otherwise. But
since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have
died.

Unlike in the previous patch, this function is not prepared for
indicating errors via a `strbuf err`, so let's just drop the dead code.
Any improved error-handling can be added later.

While at it, make the lock non-static and reduce its scope.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 refs.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 8c50b8b139..7abd30dfe1 100644
--- a/refs.c
+++ b/refs.c
@@ -689,20 +689,17 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
 
 static int delete_pseudoref(const char *pseudoref, const struct object_id 
*old_oid)
 {
-   static struct lock_file lock;
const char *filename;
 
filename = git_path("%s", pseudoref);
 
if (old_oid && !is_null_oid(old_oid)) {
-   int fd;
+   struct lock_file lock = LOCK_INIT;
struct object_id actual_old_oid;
 
-   fd = hold_lock_file_for_update_timeout(
+   hold_lock_file_for_update_timeout(
, filename, LOCK_DIE_ON_ERROR,
get_files_ref_lock_timeout_ms());
-   if (fd < 0)
-   die_errno(_("Could not open '%s' for writing"), 
filename);
if (read_ref(pseudoref, _old_oid))
die("could not read ref '%s'", pseudoref);
if (oidcmp(_old_oid, old_oid)) {
-- 
2.17.0.411.g9fd64c8e46

[PATCH 0/5] getting rid of most "static struct lock_file"s

[Martin Ågren]

This series addresses two classes of "static struct lock_file", removing
the staticness: Those locks that already live inside a function, and
those that can simply be moved into the function they are used from.

The first three patches are some cleanups I noticed along the way, where
we first take a lock using LOCK_DIE_ON_ERROR, then check whether we got
it.

After this series, we have a small number of "static struct lock_file"
left, namely those locks that are used from within more than one
function. Thus, after this series, when one stumbles upon a static
lock_file, it should be a clear warning that the lock is being
used by more than one function.

Martin

Martin Ågren (5):
  t/helper/test-write-cache: clean up lock-handling
  refs.c: do not die if locking fails in `write_pseudoref()`
  refs.c: drop dead code checking lock status in `delete_pseudoref()`
  lock_file: make function-local locks non-static
  lock_file: move static locks into functions

 t/helper/test-scrap-cache-tree.c |  4 ++--
 t/helper/test-write-cache.c  | 14 +-
 apply.c  |  2 +-
 builtin/add.c|  3 +--
 builtin/describe.c   |  2 +-
 builtin/difftool.c   |  2 +-
 builtin/gc.c |  2 +-
 builtin/merge.c  |  4 ++--
 builtin/mv.c |  2 +-
 builtin/read-tree.c  |  3 +--
 builtin/receive-pack.c   |  2 +-
 builtin/rm.c |  3 +--
 bundle.c |  2 +-
 fast-import.c|  2 +-
 refs.c   | 12 
 refs/files-backend.c |  2 +-
 rerere.c |  3 +--
 shallow.c|  2 +-
 18 files changed, 27 insertions(+), 39 deletions(-)

-- 
2.17.0.411.g9fd64c8e46

Re: [PATCH v2 3/3] unpack_trees_options: free messages when done

[Martin Ågren]

On 18 May 2018 at 00:10, Junio C Hamano <gits...@pobox.com> wrote:
> Martin Ågren <martin.ag...@gmail.com> writes:
>
>> The `opts` string array contains multiple copies of the same pointers.
>> Be careful to only free each pointer once, then zeroize the whole array
>> so that we do not leave any dangling pointers.

> I wonder if an approach that is longer-term a bit more maintainable
> is to add a new string-list instance to opts, save these xstrfmt()'ed
> messages to it when setup_unpack_trees_porcelain() create them, and
> then make clear_unpack_trees_porcelain() pay *no* attention to msg[]
> array and the positions of these allocated messages and duplicates
> but just reclaim the resources held in that string-list, or
> something like that.

Thank you for thoughts and this suggestion. I will try this out,
hopefully later today.

Martin

Re: error(?) in "man git-stash" regarding "--keep-index"

[Martin Ågren]

On 18 May 2018 at 12:51, Robert P. J. Day <rpj...@crashcourse.ca> wrote:
> On Fri, 18 May 2018, Martin Ågren wrote:
>
>> On 18 May 2018 at 11:37, Robert P. J. Day <rpj...@crashcourse.ca> wrote:
>> >
>> >   toward the bottom of "man git-stash", one reads part of an example:
>> >
>> >   # ... hack hack hack ...
>> >   $ git add --patch foo# add just first part to the index
>> >   $ git stash push --keep-index# save all other changes to the stash
>> >   ^ ???
>> >
>> > i thought that, even if "--keep-index" left staged changes in the
>> > index, it still included those staged changes in the stash. that's
>> > not the impression one gets from the above.

If I try to follow the example, it does exactly what it purports to do
and I understand why I would want to use this exact technique with
`--keep-index` in the situation outlined: "you want to make two or more
commits out of the changes in the work tree, and you want to test each
change before committing"

So I claim that the example is correct in the sense that it describes
what happens when one uses git in the real world. Another question is
whether the example and the real-world behavior match the impression
you have from elsewhere, e.g., from earlier in the man-page.

That's why I asked this:

>> So would the error be in the part of the man-page quoted below?
>>
>>   If the --keep-index option is used, all changes already added to
>>   the index are left intact.
>
>   no, that part is correct, it clearly(?) states that staged changes
> are left where they are, in the index. i submit that the misleading
> part is in the example i quoted, which suggests that only the "other"
> changes are saved to the stash -- that is, the changes other than what
> is already in the index.

Could you try the example? I think it is important that we find whether
the source of confusion is the example or the earlier explanation
quoted just here.

>> That is, this doesn't say *where* things are left intact (in the
>> index? in the working tree?).
>
>   in that case, that's something that could obviously be clarified.

Would you suggest adding something like "both in the index and in the
working tree" to what I quoted above?

>> The man-page does start with
>>
>>   git-stash - Stash the changes in a dirty working directory away
>>
>> which to me suggests that "leaving something intact" refers to
>> precisely this -- the working directory.
>>
>> Or is it the name of the option that trips you up? That is, you read
>> the name as `--keep-the-index-as-is-but-stash-as-usual`, as opposed
>> to `--keep-what-is-already-in-the-index-around`?
>>
>> While I'm sure that some clarification could be provided, I'm tempted to
>> argue that is exactly what the example provides that you quoted from.
>
>   i guess we can agree to disagree, since i think the snippet of the
> example i provided gives a misleading explanation.

We can disagree on many things, but I would very much like us to agree
on whether the example correctly describes what the command does, or
not.

Then, if the error or source of confusion is not there, then let's
identify it and see if we can find out how to address it.

I suggest that the source of confusion is here:

  If the --keep-index option is used, all changes already added to
  the index are left intact.

Martin

Re: error(?) in "man git-stash" regarding "--keep-index"

[Martin Ågren]

On 18 May 2018 at 11:37, Robert P. J. Day  wrote:
>
>   toward the bottom of "man git-stash", one reads part of an example:
>
>   # ... hack hack hack ...
>   $ git add --patch foo# add just first part to the index
>   $ git stash push --keep-index# save all other changes to the stash
>   ^ ???
>
> i thought that, even if "--keep-index" left staged changes in the
> index, it still included those staged changes in the stash. that's not
> the impression one gets from the above.

So would the error be in the part of the man-page quoted below?

  If the --keep-index option is used, all changes already added to
  the index are left intact.

That is, this doesn't say *where* things are left intact (in the index?
in the working tree?). The man-page does start with

  git-stash - Stash the changes in a dirty working directory away

which to me suggests that "leaving something intact" refers to precisely
this -- the working directory.

Or is it the name of the option that trips you up? That is, you read the
name as `--keep-the-index-as-is-but-stash-as-usual`, as opposed to
`--keep-what-is-already-in-the-index-around`?

While I'm sure that some clarification could be provided, I'm tempted to
argue that is exactly what the example provides that you quoted from.

Martin

Re: [PATCH v3 3/3] unpack_trees_options: free messages when done

[Martin Ågren]

On 19 May 2018 at 03:02, Jeff King  wrote:
> On Fri, May 18, 2018 at 03:30:44PM -0700, Elijah Newren wrote:
>
>> > would become:
>> >
>> >   msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOUPTODATE_FILE] =
>> > string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
>> >
>> > I don't know if that's worth it or not (I suspect that there are other
>> > places where appendf would be handy, but I didn't poke around).

This does poke at the `string` member, but there is precedent for doing
that. That also feels much closer to the purpose of a string list than
the fiddling with `strdup_strings` that I do in my patch.

I'll look into this over the weekend. Thanks for the suggestion.

>> The strdup_strings=1 immediately before calling string_list_clear()
>> has been used in one other place in merge-recursive.c, and tripped up
>> the reviewer requiring a big code comment to explain it. (See the very
>> end of 
>> https://public-inbox.org/git/cabpp-bgh7qttfu3kgh4ko5drrxiqjtrnhx_uaqsb6fhxt+9...@mail.gmail.com/
>> ).  So there's already one other place in merge-recursive.c that might
>> benefit from such a change.
>
> Thanks. I knew I had seen such hackery before, but it's nice to have a
> specific site that would benefit.
>
> IMHO the "nodup" variant of string_list is quite often a sign that
> things are more complicated than they need to be. Even in cases that are
> truly pointing to existing strings, is the complication really worth
> saving a few strdups? Perhaps sometimes, but I have a suspicion it's
> mostly premature optimization.
>
>> Maybe someone wants to tackle that as a separate patch series?  (Maybe
>> we make it a micro-project for future GSoC'ers?)
>
> Yeah, I'm fine with these patches if somebody wants to do it separately.
> It would be a good micro-project, but I'd also be just as happy if
> somebody did it before next year. :)

Obviously, I won't be tackling all of that now. I'll just look into
making this final patch better and leave any further cleaning up for
later.

Martin

[PATCH v4 2/4] merge-recursive: provide pair of `unpack_trees_{start,finish}()`

[Martin Ågren]

From: Elijah Newren <new...@gmail.com>

Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
call to `discard_index()` into a new function `unpack_trees_finish()`.
As a result, these are called early resp. late in `merge_trees()`,
making the resource handling clearer. A later commit will expand on
that, teaching `..._finish()` to free more memory. (So rather than
moving the FIXME-comment, just drop it, since it will be addressed soon
enough.)

Also call `..._finish()` when `merge_trees()` returns early.

Signed-off-by: Elijah Newren <new...@gmail.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 merge-recursive.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 680e01226b..ddb0fa7369 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,10 +337,10 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(struct merge_options *o,
-  struct tree *common,
-  struct tree *head,
-  struct tree *merge)
+static int unpack_trees_start(struct merge_options *o,
+ struct tree *common,
+ struct tree *head,
+ struct tree *merge)
 {
int rc;
struct tree_desc t[3];
@@ -379,6 +379,11 @@ static int git_merge_trees(struct merge_options *o,
return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+   discard_index(>orig_index);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
struct tree *result = NULL;
@@ -3088,13 +3093,14 @@ int merge_trees(struct merge_options *o,
return 1;
}
 
-   code = git_merge_trees(o, common, head, merge);
+   code = unpack_trees_start(o, common, head, merge);
 
if (code != 0) {
if (show(o, 4) || o->call_depth)
err(o, _("merging of trees %s and %s failed"),
oid_to_hex(>object.oid),
oid_to_hex(>object.oid));
+   unpack_trees_finish(o);
return -1;
}
 
@@ -3147,20 +3153,15 @@ int merge_trees(struct merge_options *o,
 
hashmap_free(>current_file_dir_set, 1);
 
-   if (clean < 0)
+   if (clean < 0) {
+   unpack_trees_finish(o);
return clean;
+   }
}
else
clean = 1;
 
-   /* Free the extra index left from git_merge_trees() */
-   /*
-* FIXME: Need to also free data allocated by
-* setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs,
-* but the problem is that only half of it refers to dynamically
-* allocated data, while the other half points at static strings.
-*/
-   discard_index(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0.840.g5d83f92caf

[PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Martin Ågren]

Add a function `string_list_appendf(list, fmt, ...)` to the string-list
API. The next commit will add a user.

This function naturally ignores the `strdup_strings`-setting and always
appends a freshly allocated string. Thus, using this function with
`strdup_strings = 0` risks making ownership unclear and leaking memory.
With `strdup_strings = 1` on the other hand, we can easily add formatted
strings without going through `string_list_append_nodup()` or playing
with `strdup_strings`.

Suggested-by: Jeff King <p...@peff.net>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 string-list.h |  9 +
 string-list.c | 13 +
 2 files changed, 22 insertions(+)

diff --git a/string-list.h b/string-list.h
index ff8f6094a3..3a73b86ffa 100644
--- a/string-list.h
+++ b/string-list.h
@@ -208,6 +208,15 @@ void string_list_remove_duplicates(struct string_list 
*sorted_list, int free_uti
  */
 struct string_list_item *string_list_append(struct string_list *list, const 
char *string);
 
+/**
+ * Add formatted string to the end of `list`. This function ignores
+ * the value of `list->strdup_strings` and always appends a freshly
+ * allocated string, so you will probably not want to use it with
+ * `strdup_strings = 0`.
+ */
+struct string_list_item *string_list_appendf(struct string_list *list,
+const char *fmt, ...);
+
 /**
  * Like string_list_append(), except string is never copied.  When
  * list->strdup_strings is set, this function can be used to hand
diff --git a/string-list.c b/string-list.c
index a0cf0cfe88..b54d31c1cf 100644
--- a/string-list.c
+++ b/string-list.c
@@ -224,6 +224,19 @@ struct string_list_item *string_list_append(struct 
string_list *list,
list->strdup_strings ? xstrdup(string) : (char 
*)string);
 }
 
+struct string_list_item *string_list_appendf(struct string_list *list,
+const char *fmt, ...)
+{
+   struct string_list_item *retval;
+   va_list ap;
+
+   va_start(ap, fmt);
+   retval = string_list_append_nodup(list, xstrvfmt(fmt, ap));
+   va_end(ap);
+
+   return retval;
+}
+
 static int cmp_items(const void *a, const void *b, void *ctx)
 {
compare_strings_fn cmp = ctx;
-- 
2.17.0.840.g5d83f92caf

[PATCH v2 1/3] config: free resources of `struct config_store_data`

[Martin Ågren]

Commit fee8572c6d (config: avoid using the global variable `store`,
2018-04-09) dropped the staticness of a certain struct, instead letting
the users create an instance on the stack and pass around a pointer.

We do not free all the memory that the struct tracks. When the struct
was static, the memory would always be reachable. Now that we keep the
struct on the stack, though, as soon as we return, it goes out of scope
and we leak the memory it points to. In particular, we leak the memory
pointed to by the `parsed` and `seen` fields.

Introduce and use a helper function `config_store_data_clear()` to plug
these leaks. The memory tracked here is config parser events. Once the
users (`git_config_set_multivar_in_file_gently()` and
`git_config_copy_or_rename_section_in_file()` at the moment) are done,
no-one should be holding on to a pointer into this memory.

There are two more members of the struct that are candidates for freeing
in this new function (`key` and `value_regex`). Those are actually
already being taken care of. The next couple of patches will move their
freeing into the function we are adding here.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 config.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/config.c b/config.c
index 6f8f1d8c11..b3282f7193 100644
--- a/config.c
+++ b/config.c
@@ -2333,6 +2333,13 @@ struct config_store_data {
unsigned int key_seen:1, section_seen:1, is_keys_section:1;
 };
 
+static void config_store_data_clear(struct config_store_data *store)
+{
+   free(store->parsed);
+   free(store->seen);
+   memset(store, 0, sizeof(*store));
+}
+
 static int matches(const char *key, const char *value,
   const struct config_store_data *store)
 {
@@ -2887,6 +2894,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
munmap(contents, contents_sz);
if (in_fd >= 0)
close(in_fd);
+   config_store_data_clear();
return ret;
 
 write_err_out:
@@ -3127,6 +3135,7 @@ static int 
git_config_copy_or_rename_section_in_file(const char *config_filename
rollback_lock_file();
 out_no_rollback:
free(filename_buf);
+   config_store_data_clear();
return ret;
 }
 
-- 
2.17.0.840.g5d83f92caf

[PATCH v2 0/3] config: free resources of `struct config_store_data`

[Martin Ågren]

On 14 May 2018 at 05:03, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Sun, May 13, 2018 at 5:58 AM, Martin Ågren <martin.ag...@gmail.com> wrote:
>>
>> How about the following two patches as patches 2/3 and 3/3? I would also
>> need to mention in the commit message of this patch (1/3) that the
>> function will soon learn to clean up more members.
>>
>
> Yep, making this a multi-part patch series and updating the commit
> message of the patch which introduces config_store_data_clear(), as
> you suggest, makes sense. The patch series could be organized
> differently -- such as first moving freeing of 'value_regex' into new
> config_store_data_clear(), then freeing additional fields in later
> patches -- but I don't think it matters much in practice, so the
> current organization is likely good enough.

I tried such a re-ordering but wasn't entirely happy about the result
(maybe I didn't try hard enough), so here are these patches again, as a
proper series and with improved commit messages.

Martin

Martin Ågren (3):
  config: free resources of `struct config_store_data`
  config: let `config_store_data_clear()` handle `value_regex`
  config: let `config_store_data_clear()` handle `key`

 config.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

-- 
2.17.0.840.g5d83f92caf

[PATCH v2 3/3] config: let `config_store_data_clear()` handle `key`

[Martin Ågren]

Instead of remembering to free `key` in each code path, let
`config_store_data_clear()` handle that.

We still need to free it before replacing it, though. Move that freeing
closer to the replacing to be safe. Note that in that same part of the
code, we can no longer set `key` to the original pointer, but need to
`xstrdup()` it.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 config.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/config.c b/config.c
index ac71f3f2e1..339a92235d 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,7 @@ struct config_store_data {
 
 static void config_store_data_clear(struct config_store_data *store)
 {
+   free(store->key);
if (store->value_regex != NULL &&
store->value_regex != CONFIG_REGEX_NONE) {
regfree(store->value_regex);
@@ -2679,7 +2680,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
fd = hold_lock_file_for_update(, config_filename, 0);
if (fd < 0) {
error_errno("could not lock config file %s", config_filename);
-   free(store.key);
ret = CONFIG_NO_LOCK;
goto out_free;
}
@@ -2689,8 +2689,6 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
 */
in_fd = open(config_filename, O_RDONLY);
if ( in_fd < 0 ) {
-   free(store.key);
-
if ( ENOENT != errno ) {
error_errno("opening %s", config_filename);
ret = CONFIG_INVALID_FILE; /* same as "invalid config 
file" */
@@ -2702,7 +2700,8 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
goto out_free;
}
 
-   store.key = (char *)key;
+   free(store.key);
+   store.key = xstrdup(key);
if (write_section(fd, key, ) < 0 ||
write_pair(fd, key, value, ) < 0)
goto write_err_out;
@@ -2752,13 +2751,10 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  config_filename,
  , )) {
error("invalid config file %s", config_filename);
-   free(store.key);
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
-   free(store.key);
-
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
(store.seen_nr > 1 && multi_replace == 0)) {
-- 
2.17.0.840.g5d83f92caf

[PATCH v2 2/3] config: let `config_store_data_clear()` handle `value_regex`

[Martin Ågren]

Instead of duplicating the logic for clearing up `value_regex`, let
`config_store_data_clear()` handle that.

When `regcomp()` fails, the current code does not call `regfree()`. Make
sure we do the same by immediately invalidating `value_regex`. Some
implementations are able to handle such an extra `regfree()`-call [1],
but from the example in [2], we should not do so. (The language itself
in [2] is not super-clear on this.)

[1] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html

[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html

Researched-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 config.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index b3282f7193..ac71f3f2e1 100644
--- a/config.c
+++ b/config.c
@@ -2335,6 +2335,11 @@ struct config_store_data {
 
 static void config_store_data_clear(struct config_store_data *store)
 {
+   if (store->value_regex != NULL &&
+   store->value_regex != CONFIG_REGEX_NONE) {
+   regfree(store->value_regex);
+   free(store->value_regex);
+   }
free(store->parsed);
free(store->seen);
memset(store, 0, sizeof(*store));
@@ -2722,7 +2727,7 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
if (regcomp(store.value_regex, value_regex,
REG_EXTENDED)) {
error("invalid pattern: %s", value_regex);
-   free(store.value_regex);
+   FREE_AND_NULL(store.value_regex);
ret = CONFIG_INVALID_PATTERN;
goto out_free;
}
@@ -2748,21 +2753,11 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
  , )) {
error("invalid config file %s", config_filename);
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
ret = CONFIG_INVALID_FILE;
goto out_free;
}
 
free(store.key);
-   if (store.value_regex != NULL &&
-   store.value_regex != CONFIG_REGEX_NONE) {
-   regfree(store.value_regex);
-   free(store.value_regex);
-   }
 
/* if nothing to unset, or too many matches, error out */
if ((store.seen_nr == 0 && value == NULL) ||
-- 
2.17.0.840.g5d83f92caf

[PATCH v4 1/4] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

After we initialize the various fields in `opts` but before we actually
use them, we might return early. Move the initialization further down,
to immediately before we use `opts`.

This limits the scope of `opts` and will help a later commit fix a
memory leak without having to worry about those early returns.

This patch is best viewed using something like this (note the tab!):
--color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge.c | 34 ++
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..f123658e58 100644
--- a/merge.c
+++ b/merge.c
@@ -94,23 +94,7 @@ int checkout_fast_forward(const struct object_id *head,
return -1;
 
memset(, 0, sizeof(trees));
-   memset(, 0, sizeof(opts));
memset(, 0, sizeof(t));
-   if (overwrite_ignore) {
-   memset(, 0, sizeof(dir));
-   dir.flags |= DIR_SHOW_IGNORED;
-   setup_standard_excludes();
-   opts.dir = 
-   }
-
-   opts.head_idx = 1;
-   opts.src_index = _index;
-   opts.dst_index = _index;
-   opts.update = 1;
-   opts.verbose_update = 1;
-   opts.merge = 1;
-   opts.fn = twoway_merge;
-   setup_unpack_trees_porcelain(, "merge");
 
trees[nr_trees] = parse_tree_indirect(head);
if (!trees[nr_trees++]) {
@@ -126,6 +110,24 @@ int checkout_fast_forward(const struct object_id *head,
parse_tree(trees[i]);
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
+
+   memset(, 0, sizeof(opts));
+   if (overwrite_ignore) {
+   memset(, 0, sizeof(dir));
+   dir.flags |= DIR_SHOW_IGNORED;
+   setup_standard_excludes();
+   opts.dir = 
+   }
+
+   opts.head_idx = 1;
+   opts.src_index = _index;
+   opts.dst_index = _index;
+   opts.update = 1;
+   opts.verbose_update = 1;
+   opts.merge = 1;
+   opts.fn = twoway_merge;
+   setup_unpack_trees_porcelain(, "merge");
+
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
return -1;
-- 
2.17.0.840.g5d83f92caf

[PATCH v4 4/4] unpack_trees_options: free messages when done

[Martin Ågren]

The strings allocated in `setup_unpack_trees_porcelain()` are never
freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
call it where we use `setup_unpack_trees_porcelain()`. The only
non-trivial user is `unpack_trees_start()`, where we should place the
new call in `unpack_trees_finish()`.

We keep the string pointers in an array, mixing pointers to static
memory and memory that we allocate on the heap. We also keep several
copies of the individual pointers. So we need to make sure that we do
not free what we must not free and that we do not double-free. Keep the
unique, heap-allocated pointers in a separate string list, to make the
freeing safe and future-proof.

Zero the whole array of string pointers to make sure that we do not
leave any dangling pointers.

Note that we only take responsibility for the memory allocated in
`setup_unpack_trees_porcelain()` and not any other members of the
`struct unpack_trees_options`.

Helped-by: Junio C Hamano <gits...@pobox.com>
Helped-by: Jeff King <p...@peff.net>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 unpack-trees.h |  6 ++
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c|  3 +++
 unpack-trees.c | 20 +---
 5 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.h b/unpack-trees.h
index 41178ada94..5a84123a40 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -33,6 +33,11 @@ enum unpack_trees_error_types {
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  const char *cmd);
 
+/*
+ * Frees resources allocated by setup_unpack_trees_porcelain().
+ */
+void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+
 struct unpack_trees_options {
unsigned int reset,
 merge,
@@ -57,6 +62,7 @@ struct unpack_trees_options {
struct pathspec *pathspec;
merge_fn_t fn;
const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
+   struct string_list msgs_to_free;
/*
 * Store error messages in an array, each case
 * corresponding to a error message type
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..5cebe170fc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
init_tree_desc([1], tree->buffer, tree->size);
 
ret = unpack_trees(2, trees, );
+   clear_unpack_trees_porcelain();
if (ret == -1) {
/*
 * Unpack couldn't do a trivial merge; either
diff --git a/merge-recursive.c b/merge-recursive.c
index ddb0fa7369..338f63a952 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -382,6 +382,7 @@ static int unpack_trees_start(struct merge_options *o,
 static void unpack_trees_finish(struct merge_options *o)
 {
discard_index(>orig_index);
+   clear_unpack_trees_porcelain(>unpack_opts);
 }
 
 struct tree *write_tree_from_memory(struct merge_options *o)
diff --git a/merge.c b/merge.c
index f123658e58..b433291d0c 100644
--- a/merge.c
+++ b/merge.c
@@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head,
 
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
+   clear_unpack_trees_porcelain();
return -1;
}
+   clear_unpack_trees_porcelain();
+
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index 79fd97074e..86046b987a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -103,6 +103,12 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
const char **msgs = opts->msgs;
const char *msg;
 
+   /*
+* As we add strings using `...appendf()`, this does not matter,
+* but when we clear the string list, we want them to be freed.
+*/
+   opts->msgs_to_free.strdup_strings = 1;
+
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
  ? _("Your local changes to the following files would be 
overwritten by checkout:\n%%s"
@@ -119,7 +125,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
  "Please commit your changes or stash them before you 
%s.")
  : _("Your local changes to the following files would be 
overwritten by %s:\n%%s");
msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
-   xstrfmt(msg, cmd, cmd);
+   string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
 
msgs[ERROR_NOT_UPTODATE_DIR] =
_("Updating the following directories would l

[PATCH v4 0/4] unpack_trees_options: free messages when done

[Martin Ågren]

This is v4 of my series for taking care of the memory allocated by
`setup_unpack_trees_porcelain()`. As before, this is based on
bp/merge-rename-config.

On 19 May 2018 at 08:13, Martin Ågren <martin.ag...@gmail.com> wrote:
> On 19 May 2018 at 03:02, Jeff King <p...@peff.net> wrote:
>>
>>> > would become:
>>> >
>>> >   msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOUPTODATE_FILE] =
>>> > string_list_appendf(>msgs_to_free, msg, cmd, cmd)->string;
>>> >
>>> > I don't know if that's worth it or not (I suspect that there are other
>>> > places where appendf would be handy, but I didn't poke around).
>
> I'll look into this over the weekend. Thanks for the suggestion.

The difference to v3 is indeed the new patch 3/4, which introduces
`string_list_appendf()`. I think that makes patch 4/4 clearer and the
resulting code less surprising.

There is an obvious candidate for using this new function in bisect.c,
but I refrained from doing that conversion in this series. While
converting that user to use this new function would be trivial and safe,
such a change might not look entirely sane on its own. The reason is
that the user does the whole `strdup_strings`-dance that I did in v3.

I think it would be much better to do that conversion as a part of a
"let's not play with strdup_strings"-patch. I have one prepared and it
looks quite ok to me. I should be able to be able to collect more
`strdup_string`-cleanups soonish and submit a series later (say, when/if
this here series has matured).

Elijah Newren (1):
  merge-recursive: provide pair of `unpack_trees_{start,finish}()`

Martin Ågren (3):
  merge: setup `opts` later in `checkout_fast_forward()`
  string-list: provide `string_list_appendf()`
  unpack_trees_options: free messages when done

 string-list.h  |  9 +
 unpack-trees.h |  6 ++
 builtin/checkout.c |  1 +
 merge-recursive.c  | 30 --
 merge.c| 35 ---
 string-list.c  | 13 +
 unpack-trees.c | 20 +---
 7 files changed, 82 insertions(+), 32 deletions(-)

-- 
2.17.0.840.g5d83f92caf

Re: [PATCH] Use OPT_SET_INT_F() for cmdline option specification

[Martin Ågren]

On 20 May 2018 at 10:12, Nguyễn Thái Ngọc Duy  wrote:
> The only thing these commands need is extra parseopt flags which can be
> passed in by OPT_SET_INT_F() and it is a bit more compact than full
> struct initialization.

> diff --git a/archive.c b/archive.c
> index 93ab175b0b..4fe7bec60c 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -411,11 +411,9 @@ static void parse_treeish_arg(const char **argv,
>  }
>
>  #define OPT__COMPR(s, v, h, p) \
> -   { OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
> +   OPT_SET_INT_F(s, NULL, v, h, p, PARSE_OPT_NONEG)
>  #define OPT__COMPR_HIDDEN(s, v, p) \
> -   { OPTION_SET_INT, (s), NULL, (v), NULL, "", \
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN, NULL, (p) }
> +   OPT_SET_INT_F(s, NULL, v, "", p, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN)

Right. We have NULLs in the fifth and the second-to-last positions, and
we use PARSE_OPT_NOARG.  By switching to OPT_SET_INT_F we get those for
free.

Do we want to keep "(s)" instead of "s", just to be safe? And same for
"(v)", "(p)". Macro expansion always makes me paranoid.

> diff --git a/builtin/am.c b/builtin/am.c
> index d834f9e62b..666287b497 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2231,12 +2231,12 @@ int cmd_am(int argc, const char **argv, const char 
> *prefix)
> N_("pass -b flag to git-mailinfo"), KEEP_NON_PATCH),
> OPT_BOOL('m', "message-id", _id,
> N_("pass -m flag to git-mailinfo")),
> -   { OPTION_SET_INT, 0, "keep-cr", _cr, NULL,
> - N_("pass --keep-cr flag to git-mailsplit for mbox format"),
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},
> -   { OPTION_SET_INT, 0, "no-keep-cr", _cr, NULL,
> - N_("do not pass --keep-cr flag to git-mailsplit independent 
> of am.keepcr"),
> - PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
> +   OPT_SET_INT_F(0, "keep-cr", _cr,
> +   N_("pass --keep-cr flag to git-mailsplit for mbox 
> format"),
> +   1, PARSE_OPT_NONEG),
> +   OPT_SET_INT_F(0, "no-keep-cr", _cr,
> +   N_("do not pass --keep-cr flag to git-mailsplit for 
> mbox format"),
> +   0, PARSE_OPT_NONEG),

I found `-w` and `--word-diff` useful. You actually change the N_("...")
for `--no-keep-cr` here: [-independent of am.keepcr-]{+for mbox format+}
Copy-paste mistake?

Other than that, `--word-diff` has a very structured appearance and
nothing stood out. The ordering is different (f goes at the end in the
post-image), which makes the diff busier than it would have had to be.
(That's obviously nothing this patch can do anything about.)

Martin

[PATCH] regex: do not call `regfree()` if compilation fails

[Martin Ågren]

It is apparently undefined behavior to call `regfree()` on a regex where
`regcomp()` failed. The language in [1] is a bit muddy, at least to me,
but the clearest hint is this (`preg` is the `regex_t *`):

Upon successful completion, the regcomp() function shall return 0.
Otherwise, it shall return an integer value indicating an error as
described in , and the content of preg is undefined.

Funnily enough, there is also the `regerror()` function which should be
given a pointer to such a "failed" `regex_t` -- the content of which
would supposedly be undefined -- and which may investigate it to come up
with a detailed error message.

In any case, the example in that document shows how `regfree()` is not
called after `regcomp()` fails.

We have quite a few users of this API and most get this right. These
three users do not.

Several implementations can handle this just fine [2] and these code paths
supposedly have not wreaked havoc or we'd have heard about it. (These
are all in code paths where git got bad input and is just about to die
anyway.) But let's just avoid the issue altogether.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/regcomp.html

[2] https://www.redhat.com/archives/libvir-list/2013-September/msg00262.html

Researched-by: Eric Sunshine <sunsh...@sunshineco.com>
Signed-off-byi Martin Ågren <martin.ag...@gmail.com>
---

On 14 May 2018 at 05:05, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> My research (for instance [1,2]) seems to indicate that it would be
> better to avoid regfree() upon failed regcomp(), even though such a
> situation is handled sanely in some implementations.
>
> [1]: https://www.redhat.com/archives/libvir-list/2013-September/msg00276.html
> [2]: https://www.redhat.com/archives/libvir-list/2013-September/msg00273.html

Thank you for researching this. I think it would make sense to get rid
of the few places we have where we get this wrong (if our understanding
of this being undefined is right).

 diffcore-pickaxe.c | 1 -
 grep.c | 2 --
 2 files changed, 3 deletions(-)

diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 239ce5122b..800a899c86 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char 
*needle, int cflags)
/* The POSIX.2 people are surely sick */
char errbuf[1024];
regerror(err, regex, errbuf, 1024);
-   regfree(regex);
die("invalid regex: %s", errbuf);
}
 }
diff --git a/grep.c b/grep.c
index 65b90c10a3..5e4f3f9a9d 100644
--- a/grep.c
+++ b/grep.c
@@ -636,7 +636,6 @@ static void compile_fixed_regexp(struct grep_pat *p, struct 
grep_opt *opt)
if (err) {
char errbuf[1024];
regerror(err, >regexp, errbuf, sizeof(errbuf));
-   regfree(>regexp);
compile_regexp_failed(p, errbuf);
}
 }
@@ -701,7 +700,6 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
if (err) {
char errbuf[1024];
regerror(err, >regexp, errbuf, 1024);
-   regfree(>regexp);
compile_regexp_failed(p, errbuf);
}
 }
-- 
2.17.0.840.g5d83f92caf

Re: [PATCH v2 10/12] commit-graph: add '--reachable' option

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:
> When writing commit-graph files, it can be convenient to ask for all
> reachable commits (starting at the ref set) in the resulting file. This
> is particularly helpful when writing to stdin is complicated, such as a
> future integration with 'git gc' which will call
> 'git commit-graph write --reachable' after performing cleanup of the
> object database.
>
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt |  8 ++--
>  builtin/commit-graph.c | 41 
> ++
>  2 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index a222cfab08..cc1715a823 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -38,12 +38,16 @@ Write a commit graph file based on the commits found in 
> packfiles.
>  +
>  With the `--stdin-packs` option, generate the new commit graph by
>  walking objects only in the specified pack-indexes. (Cannot be combined
> -with --stdin-commits.)
> +with --stdin-commits or --reachable.)

You could enclose --reachable in `...` for nicer rendering and fix
--stdin-commits as well while you're here.

>  With the `--stdin-commits` option, generate the new commit graph by
>  walking commits starting at the commits specified in stdin as a list
>  of OIDs in hex, one OID per line. (Cannot be combined with
> ---stdin-packs.)
> +--stdin-packs or --reachable.)

Ditto.

> +With the `--reachable` option, generate the new commit graph by walking
> +commits starting at all refs. (Cannot be combined with --stdin-commits
> +or --stind-packs.)

Ditto. Also, s/stind/stdin/.

Martin

Re: [PATCH v2 11/12] gc: automatically write commit-graph files

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:
> The commit-graph file is a very helpful feature for speeding up git
> operations. In order to make it more useful, write the commit-graph file
> by default during standard garbage collection operations.

So does it really write by default...

> Add a 'gc.commitGraph' config setting that triggers writing a
> commit-graph file after any non-trivial 'git gc' command. Defaults to
> false while the commit-graph feature matures. We specifically do not

or not...? I guess the first paragraph has simply been there since
before you changed your mind about the default?

> want to turn this on by default until the commit-graph feature is fully
> integrated with history-modifying features like shallow clones.

So if someone would turn this on with a shallow clone, ... Do we want
some note (warning?) around that in the user documentation?

Martin

Re: [PATCH v2 09/12] fsck: verify commit-graph

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:
> If core.commitGraph is true, verify the contents of the commit-graph
> during 'git fsck' using the 'git commit-graph verify' subcommand. Run
> this check on all alternates, as well.

> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 5ab268a024..91c8406d97 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -205,6 +205,16 @@ test_expect_success 'build graph from commits with 
> append' '
>  graph_git_behavior 'append graph, commit 8 vs merge 1' full commits/8 merge/1
>  graph_git_behavior 'append graph, commit 8 vs merge 2' full commits/8 merge/2
>
> +test_expect_success 'build graph using --reachable' '
> +   cd "$TRASH_DIRECTORY/full" &&
> +   git commit-graph write --reachable &&
> +   test_path_is_file $objdir/info/commit-graph &&
> +   graph_read_expect "11" "large_edges"
> +'

This should be in the next patch.

> +graph_git_behavior 'append graph, commit 8 vs merge 1' full commits/8 merge/1
> +graph_git_behavior 'append graph, commit 8 vs merge 2' full commits/8 merge/2

(Possibly the same here.)

>  test_expect_success 'setup bare repo' '
> cd "$TRASH_DIRECTORY" &&
> git clone --bare --no-local full bare &&
> @@ -335,7 +345,7 @@ test_expect_success 'detect OID not in object database' '
> cd "$TRASH_DIRECTORY/full" &&
> cp $objdir/info/commit-graph commit-graph-backup &&
> test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> -   corrupt_data $objdir/info/commit-graph 1134 "\01" &&
> +   corrupt_data $objdir/info/commit-graph 1134 "\00" &&

This and two similar ones as well, I guess.

Actually, I can drop them altogether and the tests still pass. Rebase
mishap?

> +test_expect_success 'git fsck (checks commit-graph)' '
> +   cd "$TRASH_DIRECTORY/full" &&
> +   git fsck
> +'

Maybe inject an error and verify that `git fsck` does indeed catch it,
i.e., it does call out to check the commit-graph.

Maybe also a run with `-c core.commitGraph=no` where the error should
not be found because the commit-graph should not be checked?

Martin

[PATCH v2 3/3] unpack_trees_options: free messages when done

[Martin Ågren]

The strings allocated in `setup_unpack_trees_porcelain()` are never
freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
call it where we use `setup_unpack_trees_porcelain()`. The only
non-trivial user is `unpack_trees_start()`, where we should place the
new call in `unpack_trees_finish()`.

The `opts` string array contains multiple copies of the same pointers.
Be careful to only free each pointer once, then zeroize the whole array
so that we do not leave any dangling pointers.

Note that we only take responsibility for the memory allocated in
`setup_unpack_trees_porcelain()` and not any other members of the
`struct unpack_trees_options`.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 unpack-trees.h |  5 +
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c|  3 +++
 unpack-trees.c | 11 +++
 5 files changed, 21 insertions(+)

diff --git a/unpack-trees.h b/unpack-trees.h
index 41178ada94..70053cb3ff 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -33,6 +33,11 @@ enum unpack_trees_error_types {
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  const char *cmd);
 
+/*
+ * Frees resources allocated by setup_unpack_trees_porcelain().
+ */
+extern void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+
 struct unpack_trees_options {
unsigned int reset,
 merge,
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..5cebe170fc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
init_tree_desc([1], tree->buffer, tree->size);
 
ret = unpack_trees(2, trees, );
+   clear_unpack_trees_porcelain();
if (ret == -1) {
/*
 * Unpack couldn't do a trivial merge; either
diff --git a/merge-recursive.c b/merge-recursive.c
index ddb0fa7369..338f63a952 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -382,6 +382,7 @@ static int unpack_trees_start(struct merge_options *o,
 static void unpack_trees_finish(struct merge_options *o)
 {
discard_index(>orig_index);
+   clear_unpack_trees_porcelain(>unpack_opts);
 }
 
 struct tree *write_tree_from_memory(struct merge_options *o)
diff --git a/merge.c b/merge.c
index f123658e58..b433291d0c 100644
--- a/merge.c
+++ b/merge.c
@@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head,
 
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
+   clear_unpack_trees_porcelain();
return -1;
}
+   clear_unpack_trees_porcelain();
+
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index 79fd97074e..25e766d30e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -179,6 +179,17 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
opts->unpack_rejects[i].strdup_strings = 1;
 }
 
+void clear_unpack_trees_porcelain(struct unpack_trees_options *opts)
+{
+   char **msgs = (char **)opts->msgs;
+
+   free(msgs[ERROR_WOULD_OVERWRITE]);
+   free(msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED]);
+   free(msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN]);
+
+   memset(opts->msgs, 0, sizeof(opts->msgs));
+}
+
 static int do_add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 unsigned int set, unsigned int clear)
 {
-- 
2.17.0.583.g9a75a153ac

[PATCH v2 2/3] merge-recursive: provide pair of `unpack_trees_{start,finish}()`

[Martin Ågren]

From: Elijah Newren <new...@gmail.com>

Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
call to `discard_index()` into a new function `unpack_trees_finish()`.
As a result, these are called early resp. late in `merge_trees()`,
making the resource handling clearer. The next commit will expand on
that, teaching `..._finish()` to free more memory. (So rather than
moving the FIXME-comment, just drop it, since it will be addressed soon
enough.)

Also call `..._finish()` when `merge_trees()` returns early.

Signed-off-by: Elijah Newren <new...@gmail.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 merge-recursive.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 680e01226b..ddb0fa7369 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,10 +337,10 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(struct merge_options *o,
-  struct tree *common,
-  struct tree *head,
-  struct tree *merge)
+static int unpack_trees_start(struct merge_options *o,
+ struct tree *common,
+ struct tree *head,
+ struct tree *merge)
 {
int rc;
struct tree_desc t[3];
@@ -379,6 +379,11 @@ static int git_merge_trees(struct merge_options *o,
return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+   discard_index(>orig_index);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
struct tree *result = NULL;
@@ -3088,13 +3093,14 @@ int merge_trees(struct merge_options *o,
return 1;
}
 
-   code = git_merge_trees(o, common, head, merge);
+   code = unpack_trees_start(o, common, head, merge);
 
if (code != 0) {
if (show(o, 4) || o->call_depth)
err(o, _("merging of trees %s and %s failed"),
oid_to_hex(>object.oid),
oid_to_hex(>object.oid));
+   unpack_trees_finish(o);
return -1;
}
 
@@ -3147,20 +3153,15 @@ int merge_trees(struct merge_options *o,
 
hashmap_free(>current_file_dir_set, 1);
 
-   if (clean < 0)
+   if (clean < 0) {
+   unpack_trees_finish(o);
return clean;
+   }
}
else
clean = 1;
 
-   /* Free the extra index left from git_merge_trees() */
-   /*
-* FIXME: Need to also free data allocated by
-* setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs,
-* but the problem is that only half of it refers to dynamically
-* allocated data, while the other half points at static strings.
-*/
-   discard_index(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0.583.g9a75a153ac

[PATCH v2 1/3] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

After we initialize the various fields in `opts` but before we actually
use them, we might return early. Move the initialization further down,
to immediately before we use `opts`.

This limits the scope of `opts` and will help a later commit fix a
memory leak without having to worry about those early returns.

This patch is best viewed using something like this (note the tab!):
--color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 merge.c | 34 ++
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..f123658e58 100644
--- a/merge.c
+++ b/merge.c
@@ -94,23 +94,7 @@ int checkout_fast_forward(const struct object_id *head,
return -1;
 
memset(, 0, sizeof(trees));
-   memset(, 0, sizeof(opts));
memset(, 0, sizeof(t));
-   if (overwrite_ignore) {
-   memset(, 0, sizeof(dir));
-   dir.flags |= DIR_SHOW_IGNORED;
-   setup_standard_excludes();
-   opts.dir = 
-   }
-
-   opts.head_idx = 1;
-   opts.src_index = _index;
-   opts.dst_index = _index;
-   opts.update = 1;
-   opts.verbose_update = 1;
-   opts.merge = 1;
-   opts.fn = twoway_merge;
-   setup_unpack_trees_porcelain(, "merge");
 
trees[nr_trees] = parse_tree_indirect(head);
if (!trees[nr_trees++]) {
@@ -126,6 +110,24 @@ int checkout_fast_forward(const struct object_id *head,
parse_tree(trees[i]);
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
+
+   memset(, 0, sizeof(opts));
+   if (overwrite_ignore) {
+   memset(, 0, sizeof(dir));
+   dir.flags |= DIR_SHOW_IGNORED;
+   setup_standard_excludes();
+   opts.dir = 
+   }
+
+   opts.head_idx = 1;
+   opts.src_index = _index;
+   opts.dst_index = _index;
+   opts.update = 1;
+   opts.verbose_update = 1;
+   opts.merge = 1;
+   opts.fn = twoway_merge;
+   setup_unpack_trees_porcelain(, "merge");
+
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
return -1;
-- 
2.17.0.583.g9a75a153ac

[PATCH v2 0/3] unpack_trees_options: free messages when done

[Martin Ågren]

Hi Elijah

On 16 May 2018 at 16:32, Elijah Newren <new...@gmail.com> wrote:
> On Sat, Apr 28, 2018 at 4:32 AM, Martin Ågren <martin.ag...@gmail.com> wrote:
>> As you noted elsewhere [1], Ben is also working in this area. I'd be
>> perfectly happy to sit on these patches until both of your contributions
>> come through to master.
>>
>> [1] 
>> https://public-inbox.org/git/CABPp-BFh=gl6rnbst2bgtynkij1z5tmgar1via5_vytef5e...@mail.gmail.com/
>
> Instead of waiting for these to come through to master, could you just
> submit based on the top of bp/merge-rename-config? 

Sure, here goes. This is based on bp/merge-rename-config, gets rid of
all leaks of memory allocated in `setup_unpack_trees_porcelain()` and
cuts the number of leaks in the test-suite (i.e., the subset of the 
tests that I run) by around 10%.

Martin

Elijah Newren (1):
  merge-recursive: provide pair of `unpack_trees_{start,finish}()`

Martin Ågren (2):
  merge: setup `opts` later in `checkout_fast_forward()`
  unpack_trees_options: free messages when done

 unpack-trees.h |  5 +
 builtin/checkout.c |  1 +
 merge-recursive.c  | 30 --
 merge.c| 37 +
 unpack-trees.c | 11 +++
 5 files changed, 54 insertions(+), 30 deletions(-)

-- 
2.17.0.583.g9a75a153ac

[PATCH v5 2/4] merge-recursive: provide pair of `unpack_trees_{start,finish}()`

[Martin Ågren]

From: Elijah Newren <new...@gmail.com>

Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
call to `discard_index()` into a new function `unpack_trees_finish()`.
As a result, these are called early resp. late in `merge_trees()`,
making the resource handling clearer. A later commit will expand on
that, teaching `..._finish()` to free more memory. (So rather than
moving the FIXME-comment, just drop it, since it will be addressed soon
enough.)

Also call `..._finish()` when `merge_trees()` returns early.

Signed-off-by: Elijah Newren <new...@gmail.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge-recursive.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 680e01226b..ddb0fa7369 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,10 +337,10 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(struct merge_options *o,
-  struct tree *common,
-  struct tree *head,
-  struct tree *merge)
+static int unpack_trees_start(struct merge_options *o,
+ struct tree *common,
+ struct tree *head,
+ struct tree *merge)
 {
int rc;
struct tree_desc t[3];
@@ -379,6 +379,11 @@ static int git_merge_trees(struct merge_options *o,
return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+   discard_index(>orig_index);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
struct tree *result = NULL;
@@ -3088,13 +3093,14 @@ int merge_trees(struct merge_options *o,
return 1;
}
 
-   code = git_merge_trees(o, common, head, merge);
+   code = unpack_trees_start(o, common, head, merge);
 
if (code != 0) {
if (show(o, 4) || o->call_depth)
err(o, _("merging of trees %s and %s failed"),
oid_to_hex(>object.oid),
oid_to_hex(>object.oid));
+   unpack_trees_finish(o);
return -1;
}
 
@@ -3147,20 +3153,15 @@ int merge_trees(struct merge_options *o,
 
hashmap_free(>current_file_dir_set, 1);
 
-   if (clean < 0)
+   if (clean < 0) {
+   unpack_trees_finish(o);
return clean;
+   }
}
else
clean = 1;
 
-   /* Free the extra index left from git_merge_trees() */
-   /*
-* FIXME: Need to also free data allocated by
-* setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs,
-* but the problem is that only half of it refers to dynamically
-* allocated data, while the other half points at static strings.
-*/
-   discard_index(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0.840.g5d83f92caf

[PATCH v5 3/4] argv-array: return the pushed string from argv_push*()

[Martin Ågren]

From: Junio C Hamano <gits...@pobox.com>

Such an API change allows us to use an argv_array this way:

struct argv_array to_free = ARGV_ARRAY_INIT;
const char *msg;

if (some condition) {
msg = "constant string message";
... other logic ...
} else {
msg = argv_pushf(_free, "format %s", var);
}
... use "msg" ...
... do other things ...
argv_clear(_free);

Note that argv_array_pushl() and argv_array_pushv() are used to push
one or more strings with a single call, so we do not return any one
of these strings from these two functions in order to reduce the
chance to misuse the API.

Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 argv-array.h | 4 ++--
 argv-array.c | 6 --
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/argv-array.h b/argv-array.h
index 29056e49a1..715c93b246 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -12,9 +12,9 @@ struct argv_array {
 #define ARGV_ARRAY_INIT { empty_argv, 0, 0 }
 
 void argv_array_init(struct argv_array *);
-void argv_array_push(struct argv_array *, const char *);
+const char *argv_array_push(struct argv_array *, const char *);
 __attribute__((format (printf,2,3)))
-void argv_array_pushf(struct argv_array *, const char *fmt, ...);
+const char *argv_array_pushf(struct argv_array *, const char *fmt, ...);
 LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pushv(struct argv_array *, const char **);
diff --git a/argv-array.c b/argv-array.c
index 5d370fa336..449dfc105a 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -21,12 +21,13 @@ static void argv_array_push_nodup(struct argv_array *array, 
const char *value)
array->argv[array->argc] = NULL;
 }
 
-void argv_array_push(struct argv_array *array, const char *value)
+const char *argv_array_push(struct argv_array *array, const char *value)
 {
argv_array_push_nodup(array, xstrdup(value));
+   return array->argv[array->argc - 1];
 }
 
-void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
+const char *argv_array_pushf(struct argv_array *array, const char *fmt, ...)
 {
va_list ap;
struct strbuf v = STRBUF_INIT;
@@ -36,6 +37,7 @@ void argv_array_pushf(struct argv_array *array, const char 
*fmt, ...)
va_end(ap);
 
argv_array_push_nodup(array, strbuf_detach(, NULL));
+   return array->argv[array->argc - 1];
 }
 
 void argv_array_pushl(struct argv_array *array, ...)
-- 
2.17.0.840.g5d83f92caf

[PATCH v5 4/4] unpack_trees_options: free messages when done

[Martin Ågren]

The strings allocated in `setup_unpack_trees_porcelain()` are never
freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
call it where we use `setup_unpack_trees_porcelain()`. The only
non-trivial user is `unpack_trees_start()`, where we should place the
new call in `unpack_trees_finish()`.

We keep the string pointers in an array, mixing pointers to static
memory and memory that we allocate on the heap. We also keep several
copies of the individual pointers. So we need to make sure that we do
not free what we must not free and that we do not double-free. Let a
separate argv_array take ownership of all the strings we create so that
we can easily free them.

Zero the whole array of string pointers to make sure that we do not
leave any dangling pointers.

Note that we only take responsibility for the memory allocated in
`setup_unpack_trees_porcelain()` and not any other members of the
`struct unpack_trees_options`.

Helped-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 unpack-trees.h |  8 +++-
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c|  3 +++
 unpack-trees.c | 17 ++---
 5 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/unpack-trees.h b/unpack-trees.h
index 41178ada94..c2b434c606 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -2,7 +2,7 @@
 #define UNPACK_TREES_H
 
 #include "tree-walk.h"
-#include "string-list.h"
+#include "argv-array.h"
 
 #define MAX_UNPACK_TREES 8
 
@@ -33,6 +33,11 @@ enum unpack_trees_error_types {
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  const char *cmd);
 
+/*
+ * Frees resources allocated by setup_unpack_trees_porcelain().
+ */
+void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+
 struct unpack_trees_options {
unsigned int reset,
 merge,
@@ -57,6 +62,7 @@ struct unpack_trees_options {
struct pathspec *pathspec;
merge_fn_t fn;
const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
+   struct argv_array msgs_to_free;
/*
 * Store error messages in an array, each case
 * corresponding to a error message type
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..5cebe170fc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
init_tree_desc([1], tree->buffer, tree->size);
 
ret = unpack_trees(2, trees, );
+   clear_unpack_trees_porcelain();
if (ret == -1) {
/*
 * Unpack couldn't do a trivial merge; either
diff --git a/merge-recursive.c b/merge-recursive.c
index ddb0fa7369..338f63a952 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -382,6 +382,7 @@ static int unpack_trees_start(struct merge_options *o,
 static void unpack_trees_finish(struct merge_options *o)
 {
discard_index(>orig_index);
+   clear_unpack_trees_porcelain(>unpack_opts);
 }
 
 struct tree *write_tree_from_memory(struct merge_options *o)
diff --git a/merge.c b/merge.c
index f123658e58..b433291d0c 100644
--- a/merge.c
+++ b/merge.c
@@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head,
 
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
+   clear_unpack_trees_porcelain();
return -1;
}
+   clear_unpack_trees_porcelain();
+
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index 79fd97074e..73a6dc1701 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1,5 +1,6 @@
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
+#include "argv-array.h"
 #include "repository.h"
 #include "config.h"
 #include "dir.h"
@@ -103,6 +104,8 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
const char **msgs = opts->msgs;
const char *msg;
 
+   argv_array_init(>msgs_to_free);
+
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
  ? _("Your local changes to the following files would be 
overwritten by checkout:\n%%s"
@@ -119,7 +122,7 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
  "Please commit your changes or stash them before you 
%s.")
  : _("Your local changes to the following files would be 
overwritten by %s:\n%%s");
msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
- 

[PATCH v5 0/4] unpack_trees_options: free messages when done

[Martin Ågren]

On 21 May 2018 at 02:25, Junio C Hamano <gits...@pobox.com> wrote:
> Junio C Hamano <gits...@pobox.com> writes:
>
>> I have a feeling that argv_array might be a better fit for the
>> purpose of keeping track of to_free[] strings in the context of this
>> series.  Moving away from string_list would allow us to sidestep the
>> storage ownership issues the API has, and we do not need the .util
>> thing string_list gives us (which is one distinct advantage string_list
>> has over argv_array, if the application needs that feature).
>>
>> We would need to make _pushf() and friends return "const char *" if
>> we go that route to make the resulting API more useful, though.
>
> ... and redoing the 4/4 patch using argv_array_pushf() makes the
> result look like this, which does not look too bad.

Thanks to Jacob, Junio and Peff for comments on the previous iteration.

I've taken the six patches that Junio has queued and rebuilt the series
to get rid of the new and possibly bug-prone function that no-one uses
once the series is over.

That is, I've replaced the `string_list_appendf()`-patch with Junio's
`argv_push*()`-patch, then squashed Junio's "redoing the 4/4"-patch into
patch 4/4 -- with the exception of keeping the `memset(opts->msgs, ...)`
which I suspect was mistakenly dropped.

Again, thanks for all the helpful comments and patches pointing me in
the right direction.

Martin

Elijah Newren (1):
  merge-recursive: provide pair of `unpack_trees_{start,finish}()`

Junio C Hamano (1):
  argv-array: return the pushed string from argv_push*()

Martin Ågren (2):
  merge: setup `opts` later in `checkout_fast_forward()`
  unpack_trees_options: free messages when done

 argv-array.h   |  4 ++--
 unpack-trees.h |  8 +++-
 argv-array.c   |  6 --
 builtin/checkout.c |  1 +
 merge-recursive.c  | 30 --
 merge.c| 35 ---
 unpack-trees.c | 17 ++---
 7 files changed, 64 insertions(+), 37 deletions(-)

-- 
2.17.0.840.g5d83f92caf

[PATCH v5 1/4] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

After we initialize the various fields in `opts` but before we actually
use them, we might return early. Move the initialization further down,
to immediately before we use `opts`.

This limits the scope of `opts` and will help a later commit fix a
memory leak without having to worry about those early returns.

This patch is best viewed using something like this (note the tab!):
--color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge.c | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..f123658e58 100644
--- a/merge.c
+++ b/merge.c
@@ -94,8 +94,24 @@ int checkout_fast_forward(const struct object_id *head,
return -1;
 
memset(, 0, sizeof(trees));
-   memset(, 0, sizeof(opts));
memset(, 0, sizeof(t));
+
+   trees[nr_trees] = parse_tree_indirect(head);
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
+   return -1;
+   }
+   trees[nr_trees] = parse_tree_indirect(remote);
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
+   return -1;
+   }
+   for (i = 0; i < nr_trees; i++) {
+   parse_tree(trees[i]);
+   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
+   }
+
+   memset(, 0, sizeof(opts));
if (overwrite_ignore) {
memset(, 0, sizeof(dir));
dir.flags |= DIR_SHOW_IGNORED;
@@ -112,20 +128,6 @@ int checkout_fast_forward(const struct object_id *head,
opts.fn = twoway_merge;
setup_unpack_trees_porcelain(, "merge");
 
-   trees[nr_trees] = parse_tree_indirect(head);
-   if (!trees[nr_trees++]) {
-   rollback_lock_file(_file);
-   return -1;
-   }
-   trees[nr_trees] = parse_tree_indirect(remote);
-   if (!trees[nr_trees++]) {
-   rollback_lock_file(_file);
-   return -1;
-   }
-   for (i = 0; i < nr_trees; i++) {
-   parse_tree(trees[i]);
-   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
-   }
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
return -1;
-- 
2.17.0.840.g5d83f92caf

Re: symbol string_list_appendf() unused

[Martin Ågren]

Hi Ramsay

On 22 May 2018 at 02:08, Ramsay Jones  wrote:
> On 22/05/18 00:59, Junio C Hamano wrote:

>> There is a reroll by Martin that ties all the loose ends.
>
> Ah, OK, sorry for the noise.

No worry. Thanks for pointing out the unused function to me. I
appreciate it.

Martin

Re: [PATCH] regex: do not call `regfree()` if compilation fails

[Martin Ågren]

On 22 May 2018 at 04:20, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Mon, May 21, 2018 at 2:43 PM, Stefan Beller <sbel...@google.com> wrote:
>> On Sun, May 20, 2018 at 3:50 AM, Martin Ågren <martin.ag...@gmail.com> wrote:
>>> It is apparently undefined behavior to call `regfree()` on a regex where
>>> `regcomp()` failed. [...]
>>>
>>> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
>>> @@ -215,7 +215,6 @@ static void regcomp_or_die(regex_t *regex, const char 
>>> *needle, int cflags)
>>> /* The POSIX.2 people are surely sick */
>>> char errbuf[1024];
>>> regerror(err, regex, errbuf, 1024);
>>> -   regfree(regex);
>>> die("invalid regex: %s", errbuf);
>>
>> While the commit message is very clear why we supposedly introduce a leak 
>> here,
>> it is hard to be found from the source code (as we only delete code
>> there, so digging
>> for history is not obvious), so maybe
>>
>>  /* regfree(regex) is invalid here */
>>
>> instead?
>
> The commit message doesn't say that we are supposedly introducing a
> leak (and, indeed, no resources should have been allocated to the
> 'regex' upon failed compile). It's saying that removing this call
> potentially avoids a crash under some implementations.
>
> Given that the very next line is die(), and that the function name has
> "_or_die" in it, I'm not sure that an in-code comment about regfree()
> being invalid upon failed compile would be all that helpful; indeed,
> it could be confusing, causing the reader to wonder why that is
> significant if we're just dying anyhow. I find that the patch, as is,
> clarifies rather than muddles the situation.

Like Eric, I feel that the possible leak here is somewhat uninteresting,
since the next line will die. That said, I seem to recall from my
grepping around earlier that we have other users where we return
with a failure instead of dying.

Any clarifying comments in such code would be a separate patch to me. I
also do not immediately see the need for adding such a comment in those
places. We can do that once we verify that we actually do leak (I would
expect that to happen only in some implementations, and I think there is
a fair chance that we will never encounter such an implementation.)

Martin

Re: [PATCH v5 0/4] unpack_trees_options: free messages when done

[Martin Ågren]

On 22 May 2018 at 04:54, Junio C Hamano  wrote:
> Junio C Hamano  writes:

>> Hmph, this unfortunately depends on 'next', which means we cannot
>> merge it down to 'maint' later to fix these leaks.  I guess it is
>> not a huge deal, though.  We've lived with these message leaks for
>> quite some time now and earth still kept rotating ;-)
>
> Oh, what was I thinking.  This, just like its previous rounds, is on
> top of bp/merge-rename-config^0 and it is expected *not* to be
> mergeable to 'maint' (or 'master', for that matter, at least not
> yet).

Right. The reason it depends on that topic is the user in
merge-recursive.c. Other than patch 2 and a small part of patch 4, this
should be mergeable to 'master' (as I recall) and probably also to
'maint'. I suppose this series could have been done as three patches to
fix all users except one, then one or two patches to fix
merge-recursive.c.

That would have allowed merging the first part of the series to 'maint'.
(Maybe not to fix the leaking as such, but to keep 'maint' more up to
date with 'master' for easier merging of other topics?) If you'd prefer
an ordering like that (now and/or in the future), just let me know.

Martin

[RFC PATCH 0/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 25 May 2018 at 11:14, Junio C Hamano <gits...@pobox.com> wrote:
> Junio C Hamano <gits...@pobox.com> writes:
>
>>> +warning("the '-l' option is an alias for 
>>> '--create-reflog' and");
>>> +warning("has no effect in list mode. This option will 
>>> soon be");
>>> +warning("removed and you should omit it (or use 
>>> '--list' instead).");
>
> By the way, this is one of these times when I feel that we should
> have a better multi-line message support in die/error/warning/info
> functions.  Ideally, I should be able to write
[...]
> warning(_("the '-l' option is an alias for '--create-reflog' and\n"
>   "has no effect in list mode, This option will soon be\n"
>   "removed and you should omit it (or use '--list' 
> instead)."));
[...]
> and warning() would:
>
>  - do the sprintf formatting thing as necessary to prepare a long multi-line
>message;
>
>  - chomp that into lines at '\n' boundary; and
>
>  - give each of these lines with _("warning: ") prefixed.
>
> That way, translators can choose to make the resulting message to
> different number of lines from the original easily.

How about something like this? The first two patches implement the
above three points, except for the translation of "warning: ".

The third patch is the main reason this is marked RFC. It translates
"warning: " and similar, and breaks quite a few tests under
GETTEXT_POISON since we grep for, e.g., "warning" on stderr. I could
annotate those tests, but since I'm running out of time anyway, I
thought I'd post this as a heads-up of "I'm looking into this".

I do wonder: If our tests rely on grepping for "warning" (for pretty
good reasons), how many scripts out there do something similar (for
maybe-not-so-good reasons, but still)? Do we want to avoid breaking
them?

Also left to do is to convert any existing lego-ing users to the
single-string form that Junio wished for above.

Martin

Martin Ågren (3):
  usage: extract `prefix_suffix_lines()` from `advise()`
  usage: prefix all lines in `vreportf()`, not just the first
  usage: translate the "error: "-prefix and others

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh   |  2 +-
 t/t1600-index.sh |  6 ++---
 t/t3600-rm.sh| 36 -
 t/t5512-ls-remote.sh |  6 ++---
 t/t7607-merge-overwrite.sh   |  6 ++---
 t/t7609-merge-co-error-msgs.sh   | 39 ++--
 git-compat-util.h|  8 ++
 advice.c | 18 ++---
 usage.c  | 28 
 10 files changed, 89 insertions(+), 66 deletions(-)

-- 
2.17.0.1181.g093e983b05

[RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

Teach `vreportf()` to prefix all lines with the given prefix, not only
the first line. This matches how "hint: " is being shown, and affects
"error: ", "fatal: ", "usage: ", "warning: " and "BUG: " (as well as any
out-of-tree and future users).

Note that we need to adjust quite a few tests as a result of this
change. All of these changes are because we obviously need to prefix
more lines in various "expect"-files -- except for one instance of a
trailing empty line that disappears with this commit (see t7609). This
is a very minor change, and arguably a good one.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
Looking back at this, I wonder if the opposite approach would be better,
that is, making `advise()` use `vreportf()` after teaching the latter
the multi-line trick.

 t/t1011-read-tree-sparse-checkout.sh |  6 ++---
 t/t1506-rev-parse-diagnosis.sh   |  2 +-
 t/t1600-index.sh |  6 ++---
 t/t3600-rm.sh| 36 -
 t/t5512-ls-remote.sh |  6 ++---
 t/t7607-merge-overwrite.sh   |  6 ++---
 t/t7609-merge-co-error-msgs.sh   | 39 ++--
 usage.c  |  2 +-
 8 files changed, 51 insertions(+), 52 deletions(-)

diff --git a/t/t1011-read-tree-sparse-checkout.sh 
b/t/t1011-read-tree-sparse-checkout.sh
index 0c6f48f302..31b0702e6c 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
worktree' '
test_must_fail git checkout top 2>actual &&
cat >expected <<\EOF &&
 error: The following untracked working tree files would be overwritten by 
checkout:
-   sub/added
-   sub/addedtoo
-Please move or remove them before you switch branches.
+error: sub/added
+error: sub/addedtoo
+error: Please move or remove them before you switch branches.
 Aborting
 EOF
test_i18ncmp expected actual
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 4ee009da66..80d35087b7 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -11,7 +11,7 @@ test_did_you_mean ()
sq="'" &&
cat >expected <<-EOF &&
fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
-   Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
+   fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
EOF
test_cmp expected error
 }
diff --git a/t/t1600-index.sh b/t/t1600-index.sh
index c4422312f4..39a707c94a 100755
--- a/t/t1600-index.sh
+++ b/t/t1600-index.sh
@@ -16,7 +16,7 @@ test_expect_success 'bogus GIT_INDEX_VERSION issues warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -30,7 +30,7 @@ test_expect_success 'out of bounds GIT_INDEX_VERSION issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: GIT_INDEX_VERSION set, but the value is 
invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
@@ -54,7 +54,7 @@ test_expect_success 'out of bounds index.version issues 
warning' '
git add a 2>&1 | sed "s/[0-9]//" >actual.err &&
sed -e "s/ Z$/ /" <<-\EOF >expect.err &&
warning: index.version set, but the value is invalid.
-   Using version Z
+   warning: Using version Z
EOF
test_i18ncmp expect.err actual.err
)
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..cd4a10df2d 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -771,10 +771,10 @@ test_expect_success 'setup for testing rm messages' '
 test_expect_success 'rm files with different staged content' '
cat >expect <<-\EOF &&
error: the following files have staged content different from both the
-   file and the HEAD:
-   bar.txt
-   foo.txt
-   (use -f to force removal)
+   error: file and the HEAD:
+   error: bar.txt
+   error: foo.txt
+   error: (use -f to force removal)
EOF
echo content1 >foo.txt &&

[RFC PATCH 3/3] usage: translate the "error: "-prefix and others

[Martin Ågren]

Translate the "error: ", "fatal: ", "usage: " and "warning: " prefixes
that we use for reporting that kind of information.

Do not translate "BUG: ". We tend to prefer the messages themselves to
be non-translated (and they're not supposed to ever appear anyway) so it
makes sense to let the prefix be nontranslated, too.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
This change breaks several tests under GETTEXT_POISON, e.g. t6030 which
does this:

git bisect skip > my_bisect_log.txt 2>&1 &&
grep "warning" my_bisect_log.txt

 usage.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/usage.c b/usage.c
index 6a5669922f..7709cb22e7 100644
--- a/usage.c
+++ b/usage.c
@@ -39,24 +39,24 @@ void vreportf(const char *prefix, const char *err, va_list 
params)
 
 static NORETURN void usage_builtin(const char *err, va_list params)
 {
-   vreportf("usage: ", err, params);
+   vreportf(_("usage: "), err, params);
exit(129);
 }
 
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-   vreportf("fatal: ", err, params);
+   vreportf(_("fatal: "), err, params);
exit(128);
 }
 
 static void error_builtin(const char *err, va_list params)
 {
-   vreportf("error: ", err, params);
+   vreportf(_("error: "), err, params);
 }
 
 static void warn_builtin(const char *warn, va_list params)
 {
-   vreportf("warning: ", warn, params);
+   vreportf(_("warning: "), warn, params);
 }
 
 static int die_is_recursing_builtin(void)
-- 
2.17.0.1181.g093e983b05

[RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Martin Ågren]

advice.c contains a useful code snippet which takes a multi-line string
and prints the lines, prefixing and suffixing each line with two
constant strings. This was originally added in 23cb5bf3b3 (i18n of
multi-line advice messages, 2011-12-22) to produce such output:

hint: some multi-line advice
hint: prefixed with "hint: "

The prefix is actually colored after 960786e761 (push: colorize errors,
2018-04-21) and each line has a suffix for resetting the color.

The next commit will teach the same "prefix all the lines"-trick to the
code that produces, e.g., "warning: "-messages. In preparation for that,
extract the code for printing the individual lines and expose it through
git-compat-util.h.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
I'm open for suggestions on the naming of `prefix_suffix_lines()`...

 git-compat-util.h |  8 
 advice.c  | 18 --
 usage.c   | 18 ++
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f9e4c5f9bc..23445f7ab9 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -415,6 +415,14 @@ static inline char *git_find_last_dir_sep(const char *path)
 struct strbuf;
 
 /* General helper functions */
+
+/*
+ * Write the message to the file, prefixing and suffixing
+ * each line with `prefix` resp. `suffix`.
+ */
+void prefix_suffix_lines(FILE *f, const char *prefix,
+const char *message, const char *suffix);
+
 extern void vreportf(const char *prefix, const char *err, va_list params);
 extern NORETURN void usage(const char *err);
 extern NORETURN void usagef(const char *err, ...) __attribute__((format 
(printf, 1, 2)));
diff --git a/advice.c b/advice.c
index 370a56d054..ffb29e7ef4 100644
--- a/advice.c
+++ b/advice.c
@@ -79,24 +79,22 @@ static struct {
 
 void advise(const char *advice, ...)
 {
+   struct strbuf prefix = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
va_list params;
-   const char *cp, *np;
+
+   strbuf_addf(, _("%shint: "),
+   advise_get_color(ADVICE_COLOR_HINT));
 
va_start(params, advice);
strbuf_vaddf(, advice, params);
va_end(params);
 
-   for (cp = buf.buf; *cp; cp = np) {
-   np = strchrnul(cp, '\n');
-   fprintf(stderr, _("%shint: %.*s%s\n"),
-   advise_get_color(ADVICE_COLOR_HINT),
-   (int)(np - cp), cp,
-   advise_get_color(ADVICE_COLOR_RESET));
-   if (*np)
-   np++;
-   }
+   prefix_suffix_lines(stderr, prefix.buf, buf.buf,
+   advise_get_color(ADVICE_COLOR_RESET));
+
strbuf_release();
+   strbuf_release();
 }
 
 int git_default_advice_config(const char *var, const char *value)
diff --git a/usage.c b/usage.c
index cdd534c9df..80f9c1d14b 100644
--- a/usage.c
+++ b/usage.c
@@ -6,6 +6,24 @@
 #include "git-compat-util.h"
 #include "cache.h"
 
+void prefix_suffix_lines(FILE *f,
+const char *prefix,
+const char *message,
+const char *suffix)
+{
+   const char *cp, *np;
+
+   for (cp = message; *cp; cp = np) {
+   np = strchrnul(cp, '\n');
+   fprintf(f, "%s%.*s%s\n",
+   prefix,
+   (int)(np - cp), cp,
+   suffix);
+   if (*np)
+   np++;
+   }
+}
+
 void vreportf(const char *prefix, const char *err, va_list params)
 {
char msg[4096];
-- 
2.17.0.1181.g093e983b05

[PATCH v3 1/3] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

After we initialize the various fields in `opts` but before we actually
use them, we might return early. Move the initialization further down,
to immediately before we use `opts`.

This limits the scope of `opts` and will help a later commit fix a
memory leak without having to worry about those early returns.

This patch is best viewed using something like this (note the tab!):
--color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge.c | 32 +---
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/merge.c b/merge.c
index f06a4773d4..f123658e58 100644
--- a/merge.c
+++ b/merge.c
@@ -94,8 +94,24 @@ int checkout_fast_forward(const struct object_id *head,
return -1;
 
memset(, 0, sizeof(trees));
-   memset(, 0, sizeof(opts));
memset(, 0, sizeof(t));
+
+   trees[nr_trees] = parse_tree_indirect(head);
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
+   return -1;
+   }
+   trees[nr_trees] = parse_tree_indirect(remote);
+   if (!trees[nr_trees++]) {
+   rollback_lock_file(_file);
+   return -1;
+   }
+   for (i = 0; i < nr_trees; i++) {
+   parse_tree(trees[i]);
+   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
+   }
+
+   memset(, 0, sizeof(opts));
if (overwrite_ignore) {
memset(, 0, sizeof(dir));
dir.flags |= DIR_SHOW_IGNORED;
@@ -112,20 +128,6 @@ int checkout_fast_forward(const struct object_id *head,
opts.fn = twoway_merge;
setup_unpack_trees_porcelain(, "merge");
 
-   trees[nr_trees] = parse_tree_indirect(head);
-   if (!trees[nr_trees++]) {
-   rollback_lock_file(_file);
-   return -1;
-   }
-   trees[nr_trees] = parse_tree_indirect(remote);
-   if (!trees[nr_trees++]) {
-   rollback_lock_file(_file);
-   return -1;
-   }
-   for (i = 0; i < nr_trees; i++) {
-   parse_tree(trees[i]);
-   init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
-   }
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
return -1;
-- 
2.17.0.840.g5d83f92caf

[PATCH v3 3/3] unpack_trees_options: free messages when done

[Martin Ågren]

The strings allocated in `setup_unpack_trees_porcelain()` are never
freed. Provide a function `clear_unpack_trees_porcelain()` to do so and
call it where we use `setup_unpack_trees_porcelain()`. The only
non-trivial user is `unpack_trees_start()`, where we should place the
new call in `unpack_trees_finish()`.

We keep the string pointers in an array, mixing pointers to static
memory and memory that we allocate on the heap. We also keep several
copies of the individual pointers. So we need to make sure that we do
not free what we must not free and that we do not double-free. Keep the
unique, heap-allocated pointers in a separate string list, to make the
freeing safe and future-proof.

Zero the whole array of string pointers to make sure that we do not
leave any dangling pointers.

Note that we only take responsibility for the memory allocated in
`setup_unpack_trees_porcelain()` and not any other members of the
`struct unpack_trees_options`.

Helped-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 unpack-trees.h |  6 ++
 builtin/checkout.c |  1 +
 merge-recursive.c  |  1 +
 merge.c|  3 +++
 unpack-trees.c | 23 +++
 5 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/unpack-trees.h b/unpack-trees.h
index 41178ada94..5a84123a40 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -33,6 +33,11 @@ enum unpack_trees_error_types {
 void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
  const char *cmd);
 
+/*
+ * Frees resources allocated by setup_unpack_trees_porcelain().
+ */
+void clear_unpack_trees_porcelain(struct unpack_trees_options *opts);
+
 struct unpack_trees_options {
unsigned int reset,
 merge,
@@ -57,6 +62,7 @@ struct unpack_trees_options {
struct pathspec *pathspec;
merge_fn_t fn;
const char *msgs[NB_UNPACK_TREES_ERROR_TYPES];
+   struct string_list msgs_to_free;
/*
 * Store error messages in an array, each case
 * corresponding to a error message type
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b49b582071..5cebe170fc 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -526,6 +526,7 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
init_tree_desc([1], tree->buffer, tree->size);
 
ret = unpack_trees(2, trees, );
+   clear_unpack_trees_porcelain();
if (ret == -1) {
/*
 * Unpack couldn't do a trivial merge; either
diff --git a/merge-recursive.c b/merge-recursive.c
index ddb0fa7369..338f63a952 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -382,6 +382,7 @@ static int unpack_trees_start(struct merge_options *o,
 static void unpack_trees_finish(struct merge_options *o)
 {
discard_index(>orig_index);
+   clear_unpack_trees_porcelain(>unpack_opts);
 }
 
 struct tree *write_tree_from_memory(struct merge_options *o)
diff --git a/merge.c b/merge.c
index f123658e58..b433291d0c 100644
--- a/merge.c
+++ b/merge.c
@@ -130,8 +130,11 @@ int checkout_fast_forward(const struct object_id *head,
 
if (unpack_trees(nr_trees, t, )) {
rollback_lock_file(_file);
+   clear_unpack_trees_porcelain();
return -1;
}
+   clear_unpack_trees_porcelain();
+
if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
return 0;
diff --git a/unpack-trees.c b/unpack-trees.c
index 79fd97074e..60293ff536 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -103,6 +103,8 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
const char **msgs = opts->msgs;
const char *msg;
 
+   opts->msgs_to_free.strdup_strings = 0;
+
if (!strcmp(cmd, "checkout"))
msg = advice_commit_before_merge
  ? _("Your local changes to the following files would be 
overwritten by checkout:\n%%s"
@@ -118,8 +120,9 @@ void setup_unpack_trees_porcelain(struct 
unpack_trees_options *opts,
  ? _("Your local changes to the following files would be 
overwritten by %s:\n%%s"
  "Please commit your changes or stash them before you 
%s.")
  : _("Your local changes to the following files would be 
overwritten by %s:\n%%s");
-   msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] =
-   xstrfmt(msg, cmd, cmd);
+   msg = xstrfmt(msg, cmd, cmd);
+   msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] = msg;
+   string_list_append(>msgs_to_free, msg);
 
msgs[ERROR_NOT_UPTODATE_DIR] =
_("Updating the following directories would 

[PATCH v3 2/3] merge-recursive: provide pair of `unpack_trees_{start,finish}()`

[Martin Ågren]

From: Elijah Newren <new...@gmail.com>

Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
call to `discard_index()` into a new function `unpack_trees_finish()`.
As a result, these are called early resp. late in `merge_trees()`,
making the resource handling clearer. The next commit will expand on
that, teaching `..._finish()` to free more memory. (So rather than
moving the FIXME-comment, just drop it, since it will be addressed soon
enough.)

Also call `..._finish()` when `merge_trees()` returns early.

Signed-off-by: Elijah Newren <new...@gmail.com>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
 merge-recursive.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 680e01226b..ddb0fa7369 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,10 +337,10 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(struct merge_options *o,
-  struct tree *common,
-  struct tree *head,
-  struct tree *merge)
+static int unpack_trees_start(struct merge_options *o,
+ struct tree *common,
+ struct tree *head,
+ struct tree *merge)
 {
int rc;
struct tree_desc t[3];
@@ -379,6 +379,11 @@ static int git_merge_trees(struct merge_options *o,
return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+   discard_index(>orig_index);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
struct tree *result = NULL;
@@ -3088,13 +3093,14 @@ int merge_trees(struct merge_options *o,
return 1;
}
 
-   code = git_merge_trees(o, common, head, merge);
+   code = unpack_trees_start(o, common, head, merge);
 
if (code != 0) {
if (show(o, 4) || o->call_depth)
err(o, _("merging of trees %s and %s failed"),
oid_to_hex(>object.oid),
oid_to_hex(>object.oid));
+   unpack_trees_finish(o);
return -1;
}
 
@@ -3147,20 +3153,15 @@ int merge_trees(struct merge_options *o,
 
hashmap_free(>current_file_dir_set, 1);
 
-   if (clean < 0)
+   if (clean < 0) {
+   unpack_trees_finish(o);
return clean;
+   }
}
else
clean = 1;
 
-   /* Free the extra index left from git_merge_trees() */
-   /*
-* FIXME: Need to also free data allocated by
-* setup_unpack_trees_porcelain() tucked away in o->unpack_opts.msgs,
-* but the problem is that only half of it refers to dynamically
-* allocated data, while the other half points at static strings.
-*/
-   discard_index(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0.840.g5d83f92caf

[PATCH v3 0/3] unpack_trees_options: free messages when done

[Martin Ågren]

This is a reroll of my attempt at freeing the memory allocated by
`setup_unpack_trees_porcelain()`. The first two patches are identical to
v2. The third patch no longer relies on rather intimate knowledge of
which strings are on the heap and which pointers are duplicates.
Instead, as suggested by Junio, I keep a separate string-list of strings
to free. That should make things more future-proof.

v2: https://public-inbox.org/git/cover.1526488122.git.martin.ag...@gmail.com/

Martin

Elijah Newren (1):
  merge-recursive: provide pair of `unpack_trees_{start,finish}()`

Martin Ågren (2):
  merge: setup `opts` later in `checkout_fast_forward()`
  unpack_trees_options: free messages when done

 unpack-trees.h |  6 ++
 builtin/checkout.c |  1 +
 merge-recursive.c  | 30 --
 merge.c| 35 ---
 unpack-trees.c | 23 +++
 5 files changed, 62 insertions(+), 33 deletions(-)

-- 
2.17.0.840.g5d83f92caf

Re: error(?) in "man git-stash" regarding "--keep-index"

[Martin Ågren]

On 18 May 2018 at 17:43, Robert P. J. Day  wrote:
> On Fri, 18 May 2018, Sybille Peters wrote:
>
>> My 2c on this:
>>
>> 1) "If the --keep-index option is used, all changes already added to
>>the index are left intact" (manpage git stash)
>>
>> That appears to be correct and clear
>
>   yup, that's not the issue.
>
>> 2) "$ git stash push --keep-index # save *all other* changes to the
>>stash"  (manpage git stash)
>>
>> That is either not correct or misleading. "All other" implies in my
>> opinion all changes except the ones that were already added. *"All
>> changes including already staged changes"* might be a better choice.

Thank you Sybille for formulating these two points.

>   yup, that is *exactly* the point i was trying to make.

Ah, this is about saving to the stash vs stashing away. The latter is
what `git stash` is all about -- stashing changes *away*. At least
according to my mental model and the top of the man-page.

But testing this -- not only from the point of view of the example, by
pushing and popping, but by actually investigating what is in the stash
-- finally makes me see what you mean. Yes, the whole lot gets saved to
the stash.

So there is a difference between what gets saved to the stash and what
gets removed from the working directory. The comment in the example
places itself somewhere in the middle by using the word "save" but
really talking about what gets dropped from the working tree.

The proposed wording does not really address that. It could be taken to
mean "all changes are saved to the stash; none are removed from the
working tree".

The work flow in the example is about temporarily stashing a few changes
(changes B) to test a couple of others (changes A). Whether the stash
entry contains changes A or not is practically irrelevant to the use
case. At pop-time, auto-merging will do the correct thing.

So how about "All changes are saved to the stash. Those that have been
added to the index are left intact in the working tree, all others are
removed from the working tree."? That's quite a lot of text. Maybe
"save all changes to the stash, make the working tree match the index"?

Or more to the point: "make the working directory match the index" or
"keep only what is in the index"?

Martin

Re: [PATCH v2 1/3] merge: setup `opts` later in `checkout_fast_forward()`

[Martin Ågren]

On 16 May 2018 at 18:41, Stefan Beller <sbel...@google.com> wrote:
> On Wed, May 16, 2018 at 9:30 AM, Martin Ågren <martin.ag...@gmail.com> wrote:
>>
>> This patch is best viewed using something like this (note the tab!):
>> --color-moved --anchored="  trees[nr_trees] = parse_tree_indirect"
>
> Heh! Having a "is best viewed" paragraph is the new shiny thing in
> commit messages as 'git log origin/pu --grep "is best viewed"' tells me.

:-)

> Regarding the anchoring, I wonder if we can improve it by ignoring
> whitespaces or just looking for substrings, or by allowing regexes or ...

FWIW, because my first naive attempt failed (for some reason I did not
consider the leading tab part of the "line" so I did not provide it), I
had the same thought. Ignoring leading whitespace seemed easy enough in
the implementation.

Then I started thinking about all the ways in which whitespace can be
ignored. My reaction in the end was to not try and open that can right
there and then. I did not think about regexes.

I guess this boils down to the usage. Copying the line to anchor on from
an editor could run into these kind of whitespace-issues, and shell
escaping. Typing an anchor could become easier with regexes since one
could skip typing common substrings and just anchor on /unique-part/.

Martin

Re: [PATCH v2 08/12] commit-graph: verify commit contents against odb

[Martin Ågren]

On 11 May 2018 at 23:15, Derrick Stolee  wrote:

I finally sat down today and familiarized myself with the commit-graph
code a little. My biggest surprise was when I noticed that there is a
hash checksum at the end of the commit-graph-file. That in combination
with the tests where you flip some bytes...

It turns out, if my reading is right, that the hash value is written as
the commit-graph is generated, but that it is not verified as the
commit-graph is later read. I could not find any mention of your plans
here -- I understand why you would not want to verify the hash in
`load_commit_graph_one()`, at least not in every run. Anyway, this is
just an observation. Verifying the hash would affect the tests this
series adds. They might need to rewrite the hash or set some magic
environment variable. :-/ But that's for another day.

> +   for (i = 0; i < g->num_commits; i++) {
> +   struct commit *graph_commit, *odb_commit;
> +   struct commit_list *graph_parents, *odb_parents;
> +   int num_parents = 0;
> +   hashcpy(cur_oid.hash, g->chunk_oid_lookup + g->hash_len * i);

`num_commits` was derived as the commit-graph was loaded. It was derived
from offsets which were verified to be in the mmap-ed memory. So this
source address is guaranteed to be so, as well. Ok.

(Once brian's latest series hits master, this could use `oidread(...)`.)

> +   graph_commit = lookup_commit(_oid);

Do we know this comes from the graph? Even with a more-or-less-messed-up
commit graph? See below.

> +   odb_commit = (struct commit *)create_object(cur_oid.hash, 
> alloc_commit_node());
> +   if (parse_commit_internal(odb_commit, 0, 0)) {
> +   graph_report("failed to parse %s from object 
> database", oid_to_hex(_oid));
> +   continue;
> +   }
> +
> +   if (oidcmp(_commit_tree_in_graph_one(g, 
> graph_commit)->object.oid,
> +  get_commit_tree_oid(odb_commit)))

`get_commit_tree_in_graph_one()` will BUG rather than return NULL. So
this will not dereference NULL. Good. But might it hit the BUG? That is,
can we trust the commit coming out of `lookup_commit()` not to have
`graph_pos == COMMIT_NOT_FROM_GRAPH`?

> +   graph_report("root tree object ID for commit %s in 
> commit-graph is %s != %s",
> +oid_to_hex(_oid),
> +
> oid_to_hex(get_commit_tree_oid(graph_commit)),
> +
> oid_to_hex(get_commit_tree_oid(odb_commit)));
> +
> +   if (graph_commit->date != odb_commit->date)
> +   graph_report("commit date for commit %s in 
> commit-graph is %"PRItime" != %"PRItime"",
> +oid_to_hex(_oid),
> +graph_commit->date,
> +odb_commit->date);
> +
> +

(Extra blank line?)

> +   graph_parents = graph_commit->parents;
> +   odb_parents = odb_commit->parents;
> +
> +   while (graph_parents) {
> +   num_parents++;
> +
> +   if (odb_parents == NULL)
> +   graph_report("commit-graph parent list for 
> commit %s is too long (%d)",
> +oid_to_hex(_oid),
> +num_parents);
> +
> +   if (oidcmp(_parents->item->object.oid, 
> _parents->item->object.oid))
> +   graph_report("commit-graph parent for %s is 
> %s != %s",
> +oid_to_hex(_oid),
> +
> oid_to_hex(_parents->item->object.oid),
> +
> oid_to_hex(_parents->item->object.oid));
> +
> +   graph_parents = graph_parents->next;
> +   odb_parents = odb_parents->next;
> +   }
> +
> +   if (odb_parents != NULL)
> +   graph_report("commit-graph parent list for commit %s 
> terminates early",
> +oid_to_hex(_oid));

Ok, ensure the lists are equally long and compare the entries.

> +
> +   if (graph_commit->generation) {

If the commit has a generation number (not an old commit graph)...

> +   uint32_t max_generation = 0;
> +   graph_parents = graph_commit->parents;
> +
> +   while (graph_parents) {
> +   if (graph_parents->item->generation == 
> GENERATION_NUMBER_ZERO ||
> +   graph_parents->item->generation == 
> GENERATION_NUMBER_INFINITY)
> +   graph_report("commit-graph has valid 
> generation for %s but not 

Re: [PATCH 09/20] abbrev tests: test for "git-log" behavior

[Martin Ågren]

On 9 June 2018 at 11:56, Ævar Arnfjörð Bjarmason  wrote:
>
> On Sat, Jun 09 2018, Martin Ågren wrote:
>
>> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
>>> The "log" family of commands does its own parsing for --abbrev in
>>> revision.c, so having dedicated tests for it makes sense.
>>
>>> +for i in $(test_seq 4 40)
>>
>> I've just been skimming so might have missed something, but I see
>> several instances of this construct, and I wonder what this brute-force
>> approach really buys us. An alternative would be, e.g., "for i in 4 23
>> 40". That is, min/max and some arbitrary number in between (odd because
>> the others are even).
>>
>> Of course, we might have a bug which magically happens for the number 9,
>> but I'd expect us to test for that only if we have some reason to
>> believe that number 9 is indeed magical.
>
> Good point, I'll change this in v2, or at least guard it with
> EXPENSIVE. I hacked it up like this while exhaustively testing things
> during development, and discovered some edge cases (e.g. "0" is special
> sometimes).

Ah, "useful during hacking" explains why you did it like this. Of your
two approaches, I'd probably favour "make it cheaper" over "mark it as
EXPENSIVE". Nothing I feel strongly about.

>> Also, 40 is of course tied to SHA-1. You could perhaps define a variable
>> at the top of this file to simplify a future generalization. (Same for
>> 39/41 which are related to 40.)
>
> I forgot to note this in the commit message, but I intentionally didn't
> guard this test with the SHA1 prereq, there's nothing per-se specific to
> SHA-1 here, it's not a given that whatever our NewHash is that we won't
> use 40 characters, and the rest of the magic constants like 4 and 7 is
> something we're likely to retain with NewHash.

I'd tend to agree about not marking this SHA1.

> Although maybe we should expose GIT_SHA1_HEXSZ to the test suite.

It seems like brian's "test_translate"-approach [1] would be a good
choice of tool for this. That is, you'd just define something at the top
of this file for now, then once that tool is in place, a one-line change
could get "hexsz" from `test_translate` instead.

[1] 
https://public-inbox.org/git/20180604235229.279814-2-sand...@crustytoothpaste.net/

Martin

Re: [PATCH 17/20] abbrev: unify the handling of empty values

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
> For no good reason the --abbrev= command-line option was less strict
> than the core.abbrev config option, which came down to the latter
> using git_config_int() which rejects an empty string, but the rest of
> the parsing using strtoul() which will convert it to 0.

It will still be less strict in that it accepts trailing garbage, e.g.,
`--abbrev=7a`. Probably ok to leave it at that in this series, but
possibly useful to mention here that this only makes these options "less
differently strict".

I also notice that the documentation of `--abbrev` starts with "Instead
of showing the full 40-byte hexadecimal object name" which doesn't seem
right. I get much shorter IDs and I can't see that I'd have any
configuration causing this. Anyway, that might not be anything this
series needs to do anything about.

> +   if (!strcmp(arg, ""))
> +   die("--abbrev expects a value, got '%s'", arg);

> +   if (!strcmp(arg, ""))
> +   return opterror(opt, "expects a value", 0);

> +   if (!strcmp(optarg, ""))
> +   die("--abbrev expects a value, got '%s'", optarg);

> +   test_must_fail git branch -v --abbrev= 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

> +   test_must_fail git log --abbrev= -1 --pretty=format:%h 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

> +   test_must_fail git diff --raw --abbrev= HEAD~ 2>stderr &&
> +   test_i18ngrep "expects a value" stderr &&

Mismatch re i18n-ed or not between implementations and tests?

Martin

Re: [PATCH 20/20] abbrev: add a core.validateAbbrev setting

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:

> Instead of trying really hard to find an unambiguous SHA-1 we can with
> core.validateAbbrev=false, and preferably combined with the new
> support for relative core.abbrev values (such as +4) unconditionally
> print a short SHA-1 without doing any disambiguation check. I.e. it

This first paragraph read weirdly the first time. On the second attempt
I knew how to structure it and got it right. It might be easier to read
if the part about core.appreb=+4 were in a separate second sentence.

That last "it" is "the combination of these two configs", right?

> allows for picking a trade-off between performance, and the odds that
> future or remote (or current and local) short SHA-1 will be ambiguous.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index abf07be7b6..df31d1351f 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -925,6 +925,49 @@ means to add or subtract N characters from the SHA-1 
> that Git would
>  otherwise print, this allows for producing more future-proof SHA-1s
>  for use within a given project, while adjusting the value for the
>  current approximate number of objects.
> ++
> +This is especially useful in combination with the
> +`core.validateAbbrev` setting, or to get more future-proof hashes to
> +reference in the future in a repository whose number of objects is
> +expected to grow.

Maybe s/validateAbbrev/validateAbbrev = false/?

> +
> +core.validateAbbrev::
> +   If set to false (true by default) don't do any validation when
> +   printing abbreviated object names to see if they're really
> +   unique. This makes printing objects more performant at the
> +   cost of potentially printing object names that aren't unique
> +   within the current repository.

Good. I understand why I'd want to use it, and why not.

> ++
> +When printing abbreviated object names Git needs to look through the
> +local object store. This is an `O(log N)` operation assuming all the
> +objects are in a single pack file, but `X * O(log N)` given `X` pack
> +files, which can get expensive on some larger repositories.

This might be very close to too much information.

> ++
> +This setting changes that to `O(1)`, but with the trade-off that
> +depending on the value of `core.abbrev` we may be printing abbreviated
> +hashes that collide. Too see how likely this is, try running:
> ++
> +---
> +git log --all --pretty=format:%h --abbrev=4 | perl -nE 'chomp; say length' | 
> sort | uniq -c | sort -nr
> +---
> ++
> +This shows how many commits were found at each abbreviation length. On
> +linux.git in June 2018 this shows a bit more than 750,000 commits,
> +with just 4 needing 11 characters to be fully abbreviated, and the
> +default heuristic picks a length of 12.

These last few paragraphs seem like too much to me.

> ++
> +Even without `core.validateAbbrev=false` the results abbreviation
> +already a bit of a probability game. They're guaranteed at the moment
> +of generation, but as more objects are added, ambiguities may be
> +introduced. Likewise, what's unambiguous for you may not be for
> +somebody else you're communicating with, if they have their own clone.

This seems more useful.

> ++
> +Therefore the default of `core.validateAbbrev=true` may not save you
> +in practice if you're sharing the SHA-1 or noting it now to use after
> +a `git fetch`. You may be better off setting `core.abbrev` to
> +e.g. `+2` to add 2 extra characters to the SHA-1, and possibly combine
> +that with `core.validateAbbrev=false` to get a reasonable trade-off
> +between safety and performance.

Makes sense. As before, I'd suggest s/SHA-1/object ID/.

I do wonder if this documentation could be a bit less verbose without
sacrificing too much correctness and clarity. No brilliant suggestions
on how to do that, sorry.

Martin

Re: [PATCH 17/20] abbrev: unify the handling of empty values

[Martin Ågren]

On 9 June 2018 at 16:24, Martin Ågren  wrote:
> On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
>> For no good reason the --abbrev= command-line option was less strict
>> than the core.abbrev config option, which came down to the latter
>> using git_config_int() which rejects an empty string, but the rest of
>> the parsing using strtoul() which will convert it to 0.
>
> It will still be less strict in that it accepts trailing garbage, e.g.,
> `--abbrev=7a`. Probably ok to leave it at that in this series, but
> possibly useful to mention here that this only makes these options "less
> differently strict".

Hmpf, please ignore. That's what I get for looking at a few patches,
taking a break, picking it up again and completely forgetting what's
going on...

Re: [PATCH 19/20] abbrev: support relative abbrev values

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index ab641bf5a9..abf07be7b6 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -919,6 +919,12 @@ core.abbrev::
> in your repository, which hopefully is enough for
> abbreviated object names to stay unique for some time.
> The minimum length is 4.
> ++
> +This can also be set to relative values such as `+2` or `-2`, which
> +means to add or subtract N characters from the SHA-1 that Git would
> +otherwise print, this allows for producing more future-proof SHA-1s
> +for use within a given project, while adjusting the value for the
> +current approximate number of objects.

How about s/, this/. This/ to break it up a little? Also, you write "+2"
and "-2" but then "N". Unify it?

Also, I'd suggest s/SHA-1/object ID/ to be future-proof.

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index f466600972..f1114a7b8d 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -384,6 +384,9 @@ endif::git-format-patch[]
> independent of the `--full-index` option above, which controls
> the diff-patch output format.  Non default number of
> digits can be specified with `--abbrev=`.
> ++
> +Can also be set to a relative value, see `core.abbrev` in
> +linkgit:git-diff[1].

Good. You then add this paragraph to lots of other places...

> diff --git a/config.c b/config.c
> index 12f762ad92..cd95c6bdfb 100644
> --- a/config.c
> +++ b/config.c
> @@ -1151,6 +1151,17 @@ static int git_default_core_config(const char *var, 
> const char *value)
> return config_error_nonbool(var);
> if (!strcasecmp(value, "auto")) {
> default_abbrev = -1;
> +   } else if (*value == '+' || *value == '-') {
> +   int relative = git_config_int(var, value);
> +   if (relative == 0)
> +   die(_("bad core.abbrev value %s. "

Trailing period? Same below.

> + "relative values must be non-zero"),
> +   value);
> +   if (abs(relative) > GIT_SHA1_HEXSZ)
> +   die(_("bad core.abbrev value %s. "
> + "impossibly out of range"),
> +   value);
> +   default_abbrev_relative = relative;
> } else {
> int abbrev = git_config_int(var, value);
> if (abbrev < minimum_abbrev || abbrev > 40)
> diff --git a/diff.c b/diff.c
> index e0141cfbc0..f7861b8472 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4801,16 +4801,28 @@ int diff_opt_parse(struct diff_options *options,
> else if (!strcmp(arg, "--abbrev"))
> options->abbrev = DEFAULT_ABBREV;
> else if (skip_prefix(arg, "--abbrev=", )) {
> +   int v;
> char *end;
> if (!strcmp(arg, ""))
> die("--abbrev expects a value, got '%s'", arg);
> -   options->abbrev = strtoul(arg, , 10);
> +   v = strtoul(arg, , 10);
> if (*end)
> die("--abbrev expects a numerical value, got '%s'", 
> arg);
> -   if (options->abbrev < MINIMUM_ABBREV) {
> +   if (*arg == '+' || *arg == '-') {
> +   if (v == 0) {
> +   die("relative abbrev must be non-zero");
> +   } else if (abs(v) > the_hash_algo->hexsz) {
> +   die("relative abbrev impossibly out of 
> range");
> +   } else {
> +   default_abbrev_relative = v;
> +   options->abbrev = DEFAULT_ABBREV;
> +   }
> +   } else if (v < MINIMUM_ABBREV) {
> options->abbrev = MINIMUM_ABBREV;
> -   } else if (the_hash_algo->hexsz < options->abbrev) {
> +   } else if (the_hash_algo->hexsz < v) {
> options->abbrev = the_hash_algo->hexsz;
> +   } else {
> +   options->abbrev = v;

I've cut out a few instances of more-or-less repeated code. Any
possibility of extracting this into a helper? Maybe after you've done
the preparatory work of unifying these sites. Or as part of it, i.e.,
"let's switch this spot to use the helper; that makes it stricter in
this-and-that sense".

These can't all be entirely unified, I guess, but maybe "mostly"?

Martin

Re: [PATCH] fsck: avoid looking at NULL blob->object

[Martin Ågren]

On 9 June 2018 at 10:32, Jeff King  wrote:
> Except it _does_ do one non-trivial thing, which is call the
> report() function, which wants us to pass a pointer to a
> "struct object". Which we don't have (we have only a "struct
> object_id"). So we erroneously passed the NULL object, which

s/passed/dereferenced/? Probably doesn't affect the fix though.

> ends up segfaulting.

> blob = lookup_blob(oid);
> if (!blob) {
> -   ret |= report(options, >object,
> +   struct object *obj = lookup_unknown_object(oid->hash);
> +   ret |= report(options, obj,
>   FSCK_MSG_GITMODULES_BLOB,
>   "non-blob found at .gitmodules");

Re: [PATCH 09/20] abbrev tests: test for "git-log" behavior

[Martin Ågren]

On 9 June 2018 at 00:41, Ævar Arnfjörð Bjarmason  wrote:
> The "log" family of commands does its own parsing for --abbrev in
> revision.c, so having dedicated tests for it makes sense.

> +for i in $(test_seq 4 40)

I've just been skimming so might have missed something, but I see
several instances of this construct, and I wonder what this brute-force
approach really buys us. An alternative would be, e.g., "for i in 4 23
40". That is, min/max and some arbitrary number in between (odd because
the others are even).

Of course, we might have a bug which magically happens for the number 9,
but I'd expect us to test for that only if we have some reason to
believe that number 9 is indeed magical.

Also, 40 is of course tied to SHA-1. You could perhaps define a variable
at the top of this file to simplify a future generalization. (Same for
39/41 which are related to 40.)

Martin

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Martin Ågren]

On 30 May 2018 at 08:00, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Jeff King  writes:
>>
>>> But most importantly, it means we could eventually colorize errors, too,
>>> where we are not allowed to allocate.
>>>
>>> So perhaps:
>>>
>>>   void report_lines(FILE *out,
>>> const char *color, const char *color_reset,
>>>  const char *prefix, const char *msg);
>>>
>>> or something?
>>
>> Sounds good to me.  And if you hate the repeated "error:" prefix
>> that makes the prefix on the second and subsequent lines included in
>> cutting and pasting, we could use the two-prefix idea elsewhere in
>> the thread, too.

(That also gets rid of the minor strangeness of my series to introduce an
overly broad `suffix` and use it only for resetting color, not actually
for giving any textual suffix.)

> If we do not want duplicate prefix, another approach is to leave
> everything including alignment up to the translators.  For example,
>
> error(_("the first line of error.\n"
> "   ... and the second."));
>
[...]
> with these entries for that hypothetical "shout in caps" language.
>
> msgid "error: "
> msgstr "ERRORX: "
> msgid "the first line of error.\n   ... and the second."
> msgstr"THE FIRST LINE OF ERROR.\n... AND THE SECOND."
>
> So I do not think the two-prefix idea is necessary (and if people
> prefer to have repeated prefix, that can also be done perfectly well
> within this scheme---the second and subsequent message needs to
> duplicate "error:" at the beginning, which is sort of ugly, but the
> leading spaces for alignment we see above already knows how wide
> "error:" and its translation is, so it is not that much worse
> anyway).

Thanks for lots of good input. Doing the indenting in the error does
mean that to indent properly, the translator needs to know that it is an
error, and not a warning. Or one could say that they would need to know
the amount of indentation anyway so that they can know where to wrap
optimally.

Another consequence is that if we can emit a certain string as either,
e.g., a warning or an error depending on the context, we need to address
that. Using contexts, of course. Thank you for the hint about that. (I
have not checked if we actually have such "this is a warning or an
error"-strings currently.)

Somehow it feels slightly cleaner to me, at least on first thought, to
try to decouple the indenting from the translating and line-wrapping.
But as noted above, the indenting does affect how the line-wrapping
should/may be done.

I won't have as much time as I'd like for tinkering with this the next
week, or even weeks. Hopefully when I do get around to it, I will have
processed all the very good input I have received and turn that into
something good.

Thanks,
Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 17:50, Duy Nguyen  wrote:
> On Tue, May 29, 2018 at 6:49 AM, Martin Ågren  wrote:
>> On 28 May 2018 at 23:45, Junio C Hamano  wrote:
>>> Duy Nguyen  writes:
>>>
>>>>>> +error:   sub/added
>>>>>> +error:   sub/addedtoo
>>>>>> +error: Please move or remove them before you switch branches.
>>>>>>  Aborting
>>>>>>  EOF
>>>>>
>>>>> This shows the typical effect of this series, which (I subjectively
>>>>> think) gives us a more pleasant end-user experience.
>>>>
>>>> Also, very subjectively, I'm torn about this. To me, just one
>>>> "error/warning/fatal" at the start of the first paragraph feels much
>>>> better. If we have to somehow mark the second paragraph that "this is
>>>> also part of the error message" then it's probably better to rephrase.
>>
>> Would you feel the same about "hint: "? We already do prefix all the
>> lines there. It seems to we we should probably do the same for "hint: "
>> as for "warning: ", whatever we decide is right.
>
> It may depend on context. Let's look at the commit that introduces
> this "hint:" prefix, 38ef61cfde (advice: Introduce
> error_resolve_conflict - 2011-08-04). The example in the commit
> message shows the hint paragraph sandwiched by an error and a fatal
> one:
>
>   error: 'commit' is not possible because you have unmerged files.
>   hint: Fix them up in the work tree ...
>   hint: ...
>   fatal: Exiting because of an unresolved conflict.
>
> I think in this case (dense paragraphs of different message types) yes
> it might make sense to prefix lines with "hint:". But when there's
> only one type of message like the "error" part quoted at the top, it
> feels too verbose to have error: prefix everywhere.

Hmm, that's interesting. Deciding based on what has already been output
seems feasible, although it sounds like the potential target of infinite
tweaking of some central logic for deciding which way to go. Or, lots of
various places to try and make consistent. I am tempted towards
indenting with spaces in v2, and to leave "hint: " alone as an outlier.
(It always was one. :-/ ) I'll keep your feedback in mind.

Thanks,
Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 23:32, Jeff King  wrote:
> On Mon, May 28, 2018 at 06:25:18PM +0900, Junio C Hamano wrote:
>
>> This shows the typical effect of this series, which (I subjectively
>> think) gives us a more pleasant end-user experience.
>
> Heh, that is one of the cases that I found most ugly when I looked into
> this earlier (and in particular, because I think it makes cut-and-paste
> a little harder).
>
> More discussion in:
>
>   
> https://public-inbox.org/git/2017040758.yyfsc3r3spqpi...@sigill.intra.peff.net/
>
> and down-thread.

Thanks for the pointer. I had missed that thread entirely.

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 29 May 2018 at 07:50, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>>>  - allow callers to align 1st prefix (e.g. "error: ") with the
>>>leading indent for the second and subsequent lines by passing the
>>>second prefix with appropriate display width.
>>
>> I suspect this second prefix would always consist of
>> strlen(first_prefix) spaces? We should be able to construct it on the
>> fly, without any need for manual counting and human mistakes.
>
> I wouldn't be so bold to claim that, given especially that we are
> talking about i18n/l10n where byte count, character count and
> display width are all different even on a terminal with fixed-width
> font.

You are of course correct. I should have my morning tea before typing.

About the _("\t")-approach that you mentioned up-thread. It would allow
a translator to adjust all the indentations for a particular language.
To be clear, what you mean is _(" " /* 9 spaces */) to align
nicely with "warning: ", which is the longest English string. Then the
translator would translate the nine spaces and all of "fatal:   " and
others to padded strings, all of the same length (not necessarily nine).
Correct?

That approach seems a bit shaky, if nothing else because we may one day
similarly want to use nine "translated" spaces in some other context. Or
maybe this is actually i18n-best-practices.

It also means that shorter prefixes are somewhat arbitrarily padded,
just because there exists a longer prefix that some other code path may
want to use. OTOH, if a "warning: " is followed by an "error:   ", both
lines/blocks would have the same indentation, which might perhaps be
(slightly) preferable.

Martin

Re: [PATCH v2 05/36] refspec: convert valid_fetch_refspec to use parse_refspec

[Martin Ågren]

Hi Brandon,

On 17 May 2018 at 00:57, Brandon Williams  wrote:
> Convert 'valid_fetch_refspec()' to use the new 'parse_refspec()'
> function to only parse a single refspec and eliminate an allocation.
>
> Signed-off-by: Brandon Williams 
> ---
>  refspec.c | 17 -
>  refspec.h |  3 ++-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/refspec.c b/refspec.c
> index af9d0d4b3..ab37b5ba1 100644
> --- a/refspec.c
> +++ b/refspec.c
> @@ -146,15 +146,6 @@ static struct refspec_item *parse_refspec_internal(int 
> nr_refspec, const char **
> die("Invalid refspec '%s'", refspec[i]);
>  }
>
> -int valid_fetch_refspec(const char *fetch_refspec_str)
> -{
> -   struct refspec_item *refspec;
> -
> -   refspec = parse_refspec_internal(1, _refspec_str, 1, 1);
> -   free_refspec(1, refspec);
> -   return !!refspec;
> -}
> -
>  struct refspec_item *parse_fetch_refspec(int nr_refspec, const char 
> **refspec)
>  {
> return parse_refspec_internal(nr_refspec, refspec, 1, 0);
> @@ -242,3 +233,11 @@ void refspec_clear(struct refspec *rs)
>
> rs->fetch = 0;
>  }
> +
> +int valid_fetch_refspec(const char *fetch_refspec_str)
> +{
> +   struct refspec_item refspec;
> +   int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
> +   refspec_item_clear();
> +   return ret;
> +}

My compiler warned about this function. The `dst` and `src` pointers
will equal some random data on the stack, then they may or may not be
assigned to, then we will call `free()` on them.

At least I *think* that we "may or may not" assign to them. I don't have
much or any time to really dig into this right now unfortunately.

I suppose this could use a REFSPEC_ITEM_INIT, or a memset inside
`parse_refspec()`, but I am very unfamiliar with this code.

Martin

[PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`

[Martin Ågren]

We allocate a `struct refspec_item` on the stack without initializing
it. In particular, its `dst` and `src` members will contain some random
data from the stack. When we later call `refspec_item_clear()`, it will
call `free()` on those pointers. So if the call to `parse_refspec()` did
not assign to them, we will be freeing some random "pointers". This is
undefined behavior.

To the best of my understanding, this cannot currently be triggered by
user-provided data. And for what it's worth, the test-suite does not
trigger this with SANITIZE=address. It can be provoked by calling
`valid_fetch_refspec(":*")`.

Zero the struct, as is done in other users of `struct refspec_item`.

Signed-off-by: Martin Ågren 
---
I found some time to look into this. It does not seem to be a
user-visible bug, so not particularly critical.

 refspec.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/refspec.c b/refspec.c
index ada7854f7a..7dd7e361e5 100644
--- a/refspec.c
+++ b/refspec.c
@@ -189,7 +189,10 @@ void refspec_clear(struct refspec *rs)
 int valid_fetch_refspec(const char *fetch_refspec_str)
 {
struct refspec_item refspec;
-   int ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
+   int ret;
+
+   memset(, 0, sizeof(refspec));
+   ret = parse_refspec(, fetch_refspec_str, REFSPEC_FETCH);
refspec_item_clear();
return ret;
 }
-- 
2.18.0.rc0.43.gb85e7bcbff

Re: [PATCH] refspec: initalize `refspec_item` in `valid_fetch_refspec()`

[Martin Ågren]

On 4 June 2018 at 23:55, Ævar Arnfjörð Bjarmason  wrote:

> I think this makes more sense instead of this fix:
[...]
> -void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch)
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch)
>  {
> memset(item, 0, sizeof(*item));
> +   int ret = parse_refspec(item, refspec, fetch);
> +   return ret;
> +}

Nit: Declaration after statement. Intriguingly, you do use a `ret`
variable, so I suspect you sort of thought about it but left the final
cleaning up for later. ;-)

> -void refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch);
> +int refspec_item_init(struct refspec_item *item, const char *refspec, int 
> fetch);
> +void refspec_item_init_or_die(struct refspec_item *item, const char 
> *refspec, int fetch);
>  void refspec_item_clear(struct refspec_item *item);
>  void refspec_init(struct refspec *rs, int fetch);
>  void refspec_append(struct refspec *rs, const char *refspec);
>
> I.e. let's fix the bug, but with this admittedly more verbose fix we're
> left with exactly two memset() in refspec.c, one for each type of struct
> that's initialized by the API.
>
> The reason this is difficult now is because the current API conflates
> the init function with an init_or_die, which is what most callers want,
> so let's just split those concerns up. Then we're left with one init
> function that does the memset.

I didn't test this, but it looks sane in general IMHO, and should fix
the issue I saw. Should we be worried that someone might already depend
on `refspec_item_init()` to die, and we'll silently break that
assumption?

Martin

Re: [PATCH 0/3] refspec: refactor & fix free() behavior

[Martin Ågren]

On 5 June 2018 at 21:58, Brandon Williams  wrote:
> On 06/05, Ævar Arnfjörð Bjarmason wrote:
>> Since Martin & Brandon both liked this direction I've fixed it
>> up.
>>
>> Martin: I didn't want to be the author of the actual fix for the bug
>> you found, so I rewrote your commit in 3/3. The diff is different, and
>> I slightly modified the 3rd paragraph of the commit message & added my
>> sign-off, but otherwise it's the same.
>
> Thanks for writing up a proper patch series for this fix.  I liked
> breaking up your diff into two different patches to make it clear that
> all callers of refpsec_item_init relying on dieing.

Me too.

>> Martin Ågren (1):
>>   refspec: initalize `refspec_item` in `valid_fetch_refspec()`

I was a bit surprised at first that this wasn't a "while at it" in the
second patch, but on second thought, it does make sense to keep this
separate. Thanks for picking this up and polishing it.

Just noticed: s/initalize/initialize/. That would be my fault...

Martin

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 28 May 2018 at 23:45, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
 +error:   sub/added
 +error:   sub/addedtoo
 +error: Please move or remove them before you switch branches.
  Aborting
  EOF
>>>
>>> This shows the typical effect of this series, which (I subjectively
>>> think) gives us a more pleasant end-user experience.
>>
>> Also, very subjectively, I'm torn about this. To me, just one
>> "error/warning/fatal" at the start of the first paragraph feels much
>> better. If we have to somehow mark the second paragraph that "this is
>> also part of the error message" then it's probably better to rephrase.

Would you feel the same about "hint: "? We already do prefix all the
lines there. It seems to we we should probably do the same for "hint: "
as for "warning: ", whatever we decide is right.

> I personally can go either way.  If you prefer less noisy route, we
> could change the function signature of vreportf() to take a prefix
> for the first line and another prefix for the remaining lines and
> pass that through down to the "split and print with prefix" helper.
>
> That way, we can
>
>  - allow callers to align 1st prefix (e.g. "error: ") with the
>leading indent for the second and subsequent lines by passing the
>second prefix with appropriate display width.

I suspect this second prefix would always consist of
strlen(first_prefix) spaces? We should be able to construct it on the
fly, without any need for manual counting and human mistakes.

>
>  - allow translators to grow or shrink number of lines a given
>message takes, and to decide where in the translated string to
>wrap lines.
>
> Even though step 3/3 may become a bit awkward (the second prefix
> would most likely be only whitespace, and you'd need to write
> something silly like _("\t")), we can still keep the alignment if we
> wanted to.

Thanks both for your comments. I'll see what I can come up with.

Martin

[PATCH] revisions.txt: expand tabs to spaces in diagram

[Martin Ågren]

The diagram renders fine in AsciiDoc before and after this patch.
Asciidoctor, on the other hand, ignores the tabs entirely, which results
in different indentation for different lines. The graph illustration
earlier in the document already uses spaces instead of a tab.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
Can be seen at the end of https://git-scm.com/docs/gitrevisions

 Documentation/revisions.txt | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index dfcc49c72c..011746b74f 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -355,15 +355,15 @@ spelt out:
B..C   = ^B CC
B...C  = B ^F C  G H D E B C
B^-= B^..B
- = ^B^1 B  E I J F B
+  = ^B^1 B  E I J F B
C^@= C^1
- = F   I J F
+  = F   I J F
B^@= B^1 B^2 B^3
- = D E F   D G H E F I J
+  = D E F   D G H E F I J
C^!= C ^C^@
- = C ^C^1
- = C ^FC
+  = C ^C^1
+  = C ^FC
B^!= B ^B^@
- = B ^B^1 ^B^2 ^B^3
- = B ^D ^E ^F  B
+  = B ^B^1 ^B^2 ^B^3
+  = B ^D ^E ^F  B
F^! D  = F ^I ^J D   G H D F
-- 
2.17.0

Re: [PATCH] revisions.txt: expand tabs to spaces in diagram

[Martin Ågren]

On 2 May 2018 at 06:50, Junio C Hamano <gits...@pobox.com> wrote:
> Martin Ågren <martin.ag...@gmail.com> writes:
>
>> The diagram renders fine in AsciiDoc before and after this patch.
>> Asciidoctor, on the other hand, ignores the tabs entirely, which results
>> in different indentation for different lines. The graph illustration
>> earlier in the document already uses spaces instead of a tab.
>
> Ouch.  We might want to teach Documentation/.gitattributes that
> indent-with-tab is unwelcome in that directory, after making this
> fix (and possibly similar ones to other files).

I actually grepped around a little for a leading tab, to see if I could
immediately spot any similar issues. But there are tons of tabs here
(about 13000). Most of them work out just fine, e.g., in the OPTIONS,
where we tab-indent the descriptions.

So while we could try to move away from leading tabs in Documentation/,
it would be a huge undertaking. Any kind of "do it while we're nearby"
approach would take a long time to complete. And a one-off conversion
would probably be a horrible idea. ;-)

I just noticed another difference in how the tabs are handled. In
git-add.txt, which I just picked at random, the three continuation lines
in the synopsis indent differently in AsciiDoc (which indents them more
than in the .txt) and Asciidoctor (which indents them less than in the
.txt). To me, this is more of a "if I didn't sit down and compare the
two outputs, I would never think about these indentations -- they're
both fine".

So it might be that the only places where leading tabs really matter is
this kind of diagrams. Maybe we have a handful such bugs lingering among
the 13000 tab-indented lines...

Martin

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Martin Ågren]

On 27 October 2017 at 17:06, Pranit Bauva <pranit.ba...@gmail.com> wrote:
> +static void free_terms(struct bisect_terms *terms)
> +{
> +   if (!terms->term_good)
> +   free((void *) terms->term_good);
> +   if (!terms->term_bad)
> +   free((void *) terms->term_bad);
> +}

These look like no-ops. Remove `!` for correctness, or `if (...)` for
simplicity, since `free()` can handle NULL.

You leave the pointers dangling, but you're ok for now since this is the
last thing that happens in `cmd_bisect__helper()`. Your later patches
add more users, but they're also ok, since they immediately assign new
values.

In case you (and others) find it useful, the below is a patch I've been
sitting on for a while as part of a series to plug various memory-leaks.
`FREE_AND_NULL_CONST()` would be useful in precisely these situations.

-- >8 --
Subject: git-compat-util: add FREE_AND_NULL_CONST() wrapper

Commit 481df65 ("git-compat-util: add a FREE_AND_NULL() wrapper around
free(ptr); ptr = NULL", 2017-06-15) added `FREE_AND_NULL()`. One
advantage of this macro is that it reduces risk of copy-paste errors and
reviewer-fatigue, especially when cleaning up more than just a single
pointer.

However, `FREE_AND_NULL()` can not be used with a const-pointer:

  struct foo { const char *bar; }
  ...
  FREE_AND_NULL(baz.bar);

This causes the compiler to barf as `free()` is called: "error: passing
argument 1 of ‘free’ discards ‘const’ qualifier from pointer target
type". A naive attempt to remedy this might look like this:

  FREE_AND_NULL((void *)baz.bar);

Now the assignment is problematic: "error: lvalue required as left
operand of assignment".

Add a `FREE_AND_NULL_CONST()` wrapper macro which acts as
`FREE_AND_NULL()`, except it casts to `(void *)` when freeing. As a
demonstration, use this in two existing code paths that were identified
by some grepping. Future patches will use it to clean up struct-fields:

  FREE_AND_NULL_CONST(baz.bar);

This macro is a slightly bigger hammer than `FREE_AND_NULL` and has a
slightly larger potential for misuse. Document that `FREE_AND_NULL`
should be preferred.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 argv-array.c  | 3 +--
 git-compat-util.h | 8 
 submodule.c   | 3 +--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index 5d370fa33..433a5997a 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -59,8 +59,7 @@ void argv_array_pop(struct argv_array *array)
 {
if (!array->argc)
return;
-   free((char *)array->argv[array->argc - 1]);
-   array->argv[array->argc - 1] = NULL;
+   FREE_AND_NULL_CONST(array->argv[array->argc - 1]);
array->argc--;
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d58..ca3dcbc58 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -815,6 +815,14 @@ extern FILE *fopen_or_warn(const char *path, const char 
*mode);
  */
 #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
 
+/*
+ * FREE_AND_NULL_CONST(ptr) is like FREE_AND_NULL(ptr) except it casts
+ * to (void *) when calling free. This is useful when handling, e.g., a
+ * `const char *`, but it can also be misused. Prefer FREE_AND_NULL, and
+ * use this only when you need to and it is safe to do so.
+ */
+#define FREE_AND_NULL_CONST(p) do { free((void *)(p)); (p) = NULL; } while (0)
+
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), 
(alloc)))
 
diff --git a/submodule.c b/submodule.c
index 63e7094e1..7759f9050 100644
--- a/submodule.c
+++ b/submodule.c
@@ -364,8 +364,7 @@ int parse_submodule_update_strategy(const char *value,
 {
enum submodule_update_type type;
 
-   free((void*)dst->command);
-   dst->command = NULL;
+   FREE_AND_NULL_CONST(dst->command);
 
type = parse_submodule_update_type(value);
if (type == SM_UPDATE_UNSPECIFIED)
-- 
2.15.0.rc2.5.g97dd00bb7

Re: [PATCH v16 Part II 5/8] bisect--helper: `bisect_next_check` shell function in C

[Martin Ågren]

On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> +   /*
> +* have bad (or new) but not good (or old). We could bisect
> +* although this is less optimum.
> +*/
> +   fprintf(stderr, _("Warning: bisecting only with a %s 
> commit\n"),
> +   terms->term_bad);

Maybe this should use `warning()`?

> -   # have bad (or new) but not good (or old).  we could bisect 
> although
> -   # this is less optimum.
> -   eval_gettextln "Warning: bisecting only with a \$TERM_BAD 
> commit." >&2

I wonder if we can somehow pick up the existing translation? It would
now be fuzzy, in some sense, but since the string was originally in a
different file, maybe the po-tools won't be able to discover the
fuzzyness? We could add a TRANSLATORS-comment, so that the translators
know that this string matches an old one. There are more strings like
that in this patch, and maybe in some others as well, I haven't looked.

(Adding Jiang to cc.)

Re: [PATCH v16 Part II 6/8] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Martin Ågren]

On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> +   for (i = 0; i < argc; i++) {
> +   if (!strcmp(argv[i], "--term-good"))
> +   printf(_("%s\n"), terms->term_good);
> +   else if (!strcmp(argv[i], "--term-bad"))
> +   printf(_("%s\n"), terms->term_bad);

You seem to have lost --term-old and --term-new. I also wonder why these
would need translating. You break GETTEXT_POISON here, then fix it in
patch 8/8.

I'm not even sure you need patch 8/8. If I drop these two `_()`, I can
run `git rebase -ix "make GETTEXT_POISON=Yes test"` on the entire series
without failure. Patch 8/8 also switches to `test_i18ngrep` for some
usages of `git branch` and for some checks on `.git/BISECT_START`. I'm
not sure that's needed.

> +   else
> +   error(_("BUG: invalid argument %s for 'git bisect 
> terms'.\n"
> + "Supported options are: "
> + "--term-good|--term-old and "
> + "--term-bad|--term-new."), argv[i]);
> +   }

Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip

[Martin Ågren]

On 6 January 2018 at 15:27, Yasushi SHOJI  wrote:
> best_bisection_sorted() seems to do
>
>  - get the commit list along with the number of elements in the list
>  - walk the list one by one to check whether a element have TREESAME or not
>  - if TREESAME, skip
>  - if not, add it to array
>  - sort the array by distance
>  - put elements back to the list
>
> so, if you find TREESAME, you get less elements than given, right?

All of this matches my understanding.

> Also, if you sort, the last commit, which has NULL in the ->next,
> might get in the middle of the array??

This is a bit tricky. The elements of the linked list are not sorted.
Instead, the items of the linked list are copied into an array and
sorted. The result is then put into a linked list.

Or actually, into the *same* list. That's an optimization to avoid
deallocating all objects in the list, then allocate (roughly) the same
number of objects again. It means all the `next`-pointers are already
set up, and we just need to set the final one to NULL. (It will already
be NULL if and only if the new list has the same length as the old,
i.e., if we didn't skip any TREESAME-commit.)

> # BTW, is it really fast to use QSORT even if you have to convert to
> # an array from list?

You'll find some QSORT/qsort-stuff in git-compat-util.h. We may or may
not end up doing an actual "quick sort". That would depend on, e.g., how
the C-library implements `qsort()`. Also, I'd guess that even if we have
room for an improvement, those cases (small `cnt`!) are already fast
enough that no-one really cares. That is, maybe we could measure a
speed-up, but would anyone really *notice* it?

>> Thank you for providing a script for reproducing this. It helped me come
>> up with the attached patch. The patch is based on ma/bisect-leakfix,
>> which includes Ævar's patch.
>>
>> I think this patch could be useful, either as a final "let's test
>> something previously non-tested; this would have caught the segfault",
>> or simply squashed into Ævar's patch as a "let's add a test that would
>> have caught this, and which also tests a previously non-tested code
>> path."
>
> Do we really need that?  What is a practical use of a commit having
> both good and bad?

There is not much practical use for *having* it. But there might be some
use in *detecting* it. Linus wrote in 670f5fe34f: "Also, add a safety
net: if somebody marks as good something that includes the known-bad
point, we now notice and complain, instead of writing an empty revision
to the new bisection branch."

So there might be some value in verifying that the safety-net works and
tells the user something useful (as opposed to Git segfaulting ;-) ).

Regards,
Martin

Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip

[Martin Ågren]

> On Fri, Jan 5, 2018 at 11:45 AM, Yasushi SHOJI <yasushi.sh...@gmail.com> 
> wrote:
>> When does the list allowed to contain NULLs?

Short answer: there are no commits left to test.

The list is built in the for-loop in `find_bisection()`. So the
technical answer is: if all commits in the initial list `commit_list`
are UNINTERESTING (including if `commit_list` is empty to begin with).

It's also helpful to study where we should end up from there. We should
take the `if (!revs.commits)` branch in `bisect_next_all()`. That is, we
should print either "There are only 'skip'ped commits left to test. The
first bad commit could be any of:" or " was both good and bad".

>> Since nobody noticed it since 7c117184d7, it must be a rare case, right?

Right, you marked a commit both good and bad. That's probably not very
common. But it obviously happens. :-)

On 5 January 2018 at 06:28, Yasushi SHOJI <yasushi.sh...@gmail.com> wrote:
> OK, here is the step to reproduce on git.git

Thank you for providing a script for reproducing this. It helped me come
up with the attached patch. The patch is based on ma/bisect-leakfix,
which includes Ævar's patch.

I think this patch could be useful, either as a final "let's test
something previously non-tested; this would have caught the segfault",
or simply squashed into Ævar's patch as a "let's add a test that would
have caught this, and which also tests a previously non-tested code
path."

Thanks for digging and finding a reproduction recipe.

Martin

-- >8 --
Subject: [PATCH] bisect: add test for marking commit both good and bad

Since 670f5fe34f ([PATCH] Fix bisection terminating condition,
2005-08-30), we have noticed and complained when a commit was marked
both good and bad. But we had no tests for this behavior.

Test the behavior when we mark a commit first bad, then good, but also
when we are already in the bad state, where `git bisect skip` should
notice it.

This test would have caught the segfault which was recently fixed in
2e9fdc795c (bisect: fix a regression causing a segfault, 2018-01-03).

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 8c2c6eaef8..190f0ce0ab 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,13 @@ test_expect_success 'bisect start takes options and revs 
in any order' '
test_cmp expected actual
 '
 
+test_expect_success 'marking commit both good and bad gets reported' '
+   git bisect reset &&
+   git bisect start HEAD &&
+   test_must_fail git bisect good HEAD >out &&
+   test_i18ngrep "both good and bad" out &&
+   test_must_fail git bisect skip >out &&
+   test_i18ngrep "both good and bad" out
+'
+
 test_done
-- 
2.16.0.rc1

Re: [PATCH 3/7] worktree move: new command

[Martin Ågren]

On 6 February 2018 at 03:13, Jeff King <p...@peff.net> wrote:
> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
>> I learned SANITIZE=leak today! It not only catches this but also "dst".
>>
>> Jeff is there any ongoing effort to make the test suite pass with
>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
>> suite and saw so many failures. I did see in your original mails that
>> you focused on t and t0001 only..
>
> Yeah, I did those two scripts to try to prove to myself that the
> approach was good. But I haven't really pushed it any further.
>
> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
> long way to go.

Agreed. :-)

> My hope is that people who are interested in
> leak-checking their new code can run some specific script they're
> interested in, and maybe fix up one or two nearby bits while they're
> there (either by fixing a leak, or just annotating via UNLEAK). Then we
> can slowly converge on correctness. :)

Yeah. My experience is that it's easy -- or was for me, anyway -- to
get carried away trying to fix all "related" leaks to the one you
started with, since there are so many dimensions to search in. Two
obvious aspects are "leaks nearby" and "leaks using the same API". This
is not to suggest that the situation is horrible. It's just that if you
pick a leak at random and there is a fair number of "clusters" of
leaks, chances are good you'll find yourself in such a cluster.

Addressing a leak without worrying too much about how it generalizes
would still help. ;-)

Martin

Re: [PATCH 3/7] worktree move: new command

[Martin Ågren]

On 12 February 2018 at 10:56, Duy Nguyen <pclo...@gmail.com> wrote:
> On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren <martin.ag...@gmail.com> wrote:
>> On 6 February 2018 at 03:13, Jeff King <p...@peff.net> wrote:
>>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
>>>> I learned SANITIZE=leak today! It not only catches this but also "dst".
>>>>
>>>> Jeff is there any ongoing effort to make the test suite pass with
>>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
>>>> suite and saw so many failures. I did see in your original mails that
>>>> you focused on t and t0001 only..
>>>
>>> Yeah, I did those two scripts to try to prove to myself that the
>>> approach was good. But I haven't really pushed it any further.
>>>
>>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
>>> long way to go.
>>
>> Agreed. :-)
>
> Should we mark the test files that pass SANITIZE=leak and run as a sub
> testsuite? This way we can start growing it until it covers everything
> (unlikely) and make sure people don't break what's already passed.
>
> Of course I don't expect everybody to run this new make target with
> SANITIZE=leak. Travis can do that for us and hopefully have some way
> to tell git@vger about new breakages.

Makes sense to try to make sure that we don't regress leak-free tests. I
don't know what our Travis-budget looks like, but I would volunteer to
run something like this periodically using my own cycles.

My experience with the innards of our Makefiles and test-lib.sh is
non-existant, but from a very cursory look it seems like something as
simple as loading GIT_SKIP_TESTS from a blacklist-file might do the
trick. I could try to look into it in the next couple of days.

Martin

Re: [PATCH 1/3] t7006: add tests for how git config paginates

[Martin Ågren]

On 12 February 2018 at 23:17, Junio C Hamano <gits...@pobox.com> wrote:
> Martin Ågren <martin.ag...@gmail.com> writes:
>
>> +test_expect_success TTY 'git config respects pager.config when setting' '
>> + rm -f paginated.out &&
>> + test_terminal git -c pager.config config foo.bar bar &&
>> + test -e paginated.out
>> +'
>
> I am debating myself if this test should instead spell out what we
> eventually want from the above test and make it expect_failure, just
> like the next one.

That's a valid point. I was coming at it from the point of view of "the
current behavior is well-defined and non-broken -- we'll soon change it,
but that's more a redefinition, not a bugfix (as with --edit)". But I
could go either way.

There is some prior art in ma/branch-list-paginate, where I handled `git
branch --set-upstream-to` similar to here, cf. d74b541e0b (branch:
respect `pager.branch` in list-mode only, 2017-11-19).

> In addition to setting (which will start ignoring pager in later
> steps), unsetting, replacing of a variable and renaming/removing a
> section in a configuration should not page, I would suspect.  Should
> we test them all?

I actually had several more tests in an early draft, including --unset.
Similarly, testing all the --get-* would be possible but feels like
overkill.  From the implementation, it's "obvious enough" (famous last
words) that there are two classes of arguments, and by testing a few
from each class we should be home free.

This now comes to `git config` after `git tag` and `git branch`, where
the "complexity" of the problem has been steadily increasing. (Off the
top of my head, tag has two modes, branch has three, config has lots.)
That the tests don't get more complex might be bad, or good. But all of
these use the same basic API (DELAY_PAGER_CONFIG) in the same rather
simple way. I actually almost had the feeling that these tests here were
too much, considering that DELAY_PAGER_CONFIG gets tested quite a few
times by now.

Thanks for your comments. I'll ponder this, and see what I come up with.
Maybe a changed approach, or maybe an extended commit message. Any
further input welcome, as always.

Martin

[PATCH 1/3] t7006: add tests for how git config paginates

[Martin Ågren]

The next couple of commits will change how `git config` handles
`pager.config`, similar to how de121ffe5 (tag: respect `pager.tag` in
list-mode only, 2017-08-02) and ff1e72483 (tag: change default of
`pager.tag` to "on", 2017-08-02) changed `git tag`. Similar work has
also been done to `git branch`.

Add tests in this area to make sure that we don't regress and so that
the upcoming commits can be made clearer by adapting the tests. Add some
tests for `--list` and `--get`, one for `--edit`, and one for simple
config-setting.

In particular, use `test_expect_failure` to document that we currently
respect the pager-configuration with `--edit`. The current behavior is
buggy since the pager interferes with the editor and makes the end
result completely broken. See also b3ee740c8 (t7006: add tests for how
git tag paginates, 2017-08-02).

Remove the test added in commit 3ba7e6e29a (config: run
setup_git_directory_gently() sooner, 2010-08-05) since it has some
overlap with these. We could leave it or tweak it, or place new tests
like these next to it, but let's instead make the tests for `git config`
similar to the ones for `git tag` and `git branch`, and place them after
those.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 t/t7006-pager.sh | 42 +++---
 1 file changed, 35 insertions(+), 7 deletions(-)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f5f46a95b4..5a7b757c94 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -110,13 +110,6 @@ test_expect_success TTY 'configuration can disable pager' '
! test -e paginated.out
 '
 
-test_expect_success TTY 'git config uses a pager if configured to' '
-   rm -f paginated.out &&
-   test_config pager.config true &&
-   test_terminal git config --list &&
-   test -e paginated.out
-'
-
 test_expect_success TTY 'configuration can enable pager (from subdir)' '
rm -f paginated.out &&
mkdir -p subdir &&
@@ -252,6 +245,41 @@ test_expect_success TTY 'git branch --set-upstream-to 
ignores pager.branch' '
! test -e paginated.out
 '
 
+test_expect_success TTY 'git config respects pager.config when setting' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.config config foo.bar bar &&
+   test -e paginated.out
+'
+
+test_expect_failure TTY 'git config --edit ignores pager.config' '
+   rm -f paginated.out editor.used &&
+   write_script editor <<-\EOF &&
+   touch editor.used
+   EOF
+   EDITOR=./editor test_terminal git -c pager.config config --edit &&
+   ! test -e paginated.out &&
+   test -e editor.used
+'
+
+test_expect_success TTY 'git config --get defaults to not paging' '
+   rm -f paginated.out &&
+   test_terminal git config --get foo.bar &&
+   ! test -e paginated.out
+'
+
+test_expect_success TTY 'git config --get respects pager.config' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.config config --get foo.bar &&
+   test -e paginated.out
+'
+
+test_expect_success TTY 'git config --list defaults to not paging' '
+   rm -f paginated.out &&
+   test_terminal git config --list &&
+   ! test -e paginated.out
+'
+
+
 # A colored commit log will begin with an appropriate ANSI escape
 # for the first color; the text "commit" comes later.
 colorful() {
-- 
2.16.1.72.g5be1f00a9a

[PATCH 2/3] config: respect `pager.config` in list/get-mode only

[Martin Ågren]

Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,
2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect
`pager.config` when we are listing or "get"ing config.

Some getters give at most one line of output, but it is much easier to
document and understand that we page all of --get[-*] and --list, than
to divide the (current and future) getters into "pages" and "doesn't".

This fixes the failing test added in the previous commit. Also adapt the
test for whether `git config foo.bar bar` respects `pager.config`.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/git-config.txt | 5 +
 t/t7006-pager.sh | 6 +++---
 builtin/config.c | 8 
 git.c| 2 +-
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 14da5fc157..ccc8f0bcff 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -233,6 +233,11 @@ See also <>.
using `--file`, `--global`, etc) and `on` when searching all
config files.
 
+CONFIGURATION
+-
+`pager.config` is only respected when listing configuration, i.e., when
+`--list`, `--get` or any of `--get-*` is used.
+
 [[FILES]]
 FILES
 -
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 5a7b757c94..869a0359a8 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -245,13 +245,13 @@ test_expect_success TTY 'git branch --set-upstream-to 
ignores pager.branch' '
! test -e paginated.out
 '
 
-test_expect_success TTY 'git config respects pager.config when setting' '
+test_expect_success TTY 'git config ignores pager.config when setting' '
rm -f paginated.out &&
test_terminal git -c pager.config config foo.bar bar &&
-   test -e paginated.out
+   ! test -e paginated.out
 '
 
-test_expect_failure TTY 'git config --edit ignores pager.config' '
+test_expect_success TTY 'git config --edit ignores pager.config' '
rm -f paginated.out editor.used &&
write_script editor <<-\EOF &&
touch editor.used
diff --git a/builtin/config.c b/builtin/config.c
index ab5f95476e..9a57a0caff 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -48,6 +48,11 @@ static int show_origin;
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
 
+/* convenience macro for "ACTION_LIST | ACTION_GET_*" */
+#define ACTION_LIST_OR_GET (ACTION_LIST | ACTION_GET | ACTION_GET_ALL | \
+   ACTION_GET_REGEXP | ACTION_GET_COLOR | \
+   ACTION_GET_COLORBOOL | ACTION_GET_URLMATCH)
+
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
@@ -594,6 +599,9 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
 
+   if (actions & ACTION_LIST_OR_GET)
+   setup_auto_pager("config", 0);
+
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
if (config_with_options(show_all_config, NULL,
diff --git a/git.c b/git.c
index c870b9719c..e5c9b8729d 100644
--- a/git.c
+++ b/git.c
@@ -389,7 +389,7 @@ static struct cmd_struct commands[] = {
{ "column", cmd_column, RUN_SETUP_GENTLY },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-   { "config", cmd_config, RUN_SETUP_GENTLY },
+   { "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
{ "count-objects", cmd_count_objects, RUN_SETUP },
{ "credential", cmd_credential, RUN_SETUP_GENTLY },
{ "describe", cmd_describe, RUN_SETUP },
-- 
2.16.1.72.g5be1f00a9a

[PATCH 3/3] config: change default of `pager.config` to "on"

[Martin Ågren]

This is similar to ff1e72483 (tag: change default of `pager.tag` to
"on", 2017-08-02) and is safe now that we do not consider `pager.config`
at all when we are not listing or getting configuration. This change
will help with listing large configurations, but will not hurt users of
`git config --edit` as it would have before the previous commit.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/git-config.txt |  2 +-
 t/t7006-pager.sh | 12 ++--
 builtin/config.c |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index ccc8f0bcff..78074cf3b2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -236,7 +236,7 @@ See also <>.
 CONFIGURATION
 -
 `pager.config` is only respected when listing configuration, i.e., when
-`--list`, `--get` or any of `--get-*` is used.
+`--list`, `--get` or any of `--get-*` is used. The default is to use a pager.
 
 [[FILES]]
 FILES
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 869a0359a8..47f7830eb1 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -261,22 +261,22 @@ test_expect_success TTY 'git config --edit ignores 
pager.config' '
test -e editor.used
 '
 
-test_expect_success TTY 'git config --get defaults to not paging' '
+test_expect_success TTY 'git config --get defaults to paging' '
rm -f paginated.out &&
test_terminal git config --get foo.bar &&
-   ! test -e paginated.out
+   test -e paginated.out
 '
 
 test_expect_success TTY 'git config --get respects pager.config' '
rm -f paginated.out &&
-   test_terminal git -c pager.config config --get foo.bar &&
-   test -e paginated.out
+   test_terminal git -c pager.config=false config --get foo.bar &&
+   ! test -e paginated.out
 '
 
-test_expect_success TTY 'git config --list defaults to not paging' '
+test_expect_success TTY 'git config --list defaults to paging' '
rm -f paginated.out &&
test_terminal git config --list &&
-   ! test -e paginated.out
+   test -e paginated.out
 '
 
 
diff --git a/builtin/config.c b/builtin/config.c
index 9a57a0caff..61808a93cb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -600,7 +600,7 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
}
 
if (actions & ACTION_LIST_OR_GET)
-   setup_auto_pager("config", 0);
+   setup_auto_pager("config", 1);
 
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
-- 
2.16.1.72.g5be1f00a9a

Re: [PATCH 2/3] config: respect `pager.config` in list/get-mode only

[Martin Ågren]

On 13 February 2018 at 11:25, Duy Nguyen <pclo...@gmail.com> wrote:
> On Sun, Feb 11, 2018 at 11:40 PM, Martin Ågren <martin.ag...@gmail.com> wrote:
>> Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,
>> 2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect
>> `pager.config` when we are listing or "get"ing config.
>>
>> Some getters give at most one line of output, but it is much easier to
>> document and understand that we page all of --get[-*] and --list, than
>> to divide the (current and future) getters into "pages" and "doesn't".
>
> I realize modern pagers like 'less' can automatically exit if the
> output is less than a screen. But are we sure it's true for all
> pagers? It would be annoying to have a pager waiting for me to exit
> when I only want to check one config item out (which prints one line).
> Trading one-time convenience at reading the manual with constantly
> pressing 'q' does not seem justified.

Well, there was one recent instance of a misconfigured LESS causing the
pager not to quit automatically [1]. Your "Trading"-sentence does argue
nicely for rethinking my approach here.

A tweaked behavior could be documented as something like:

`pager.config` is only respected when listing configuration, i.e.,
when using `--list` or any of the `--get-*` which may return
multiple results.

Maybe it doesn't look to complicated after all. I'd rather not give any
ideas about how we only page if there *are* more than one line of
result, i.e., that we'd examine the result before turning on the pager.
I think I've avoided that misconception here.

Thanks
Martin

[1] 
https://public-inbox.org/git/2412a603-4382-4af5-97d0-d16d5faaf...@eluvio.com/

[PATCH] t/known-leaky: add list of known-leaky test scripts

[Martin Ågren]

Here's what a list of known leaks might look like. It feels a bit
awkward to post a known-incomplete list (I don't run all tests). Duy
offered to pick up the ball if I gave up, maybe you could complete and
post this as your own? :-? Even if I (or others) can't reproduce the
complete list locally, regressions will be trivial to find, and newly
leak-free tests fairly easy to notice.

I'm not sure if the shamelessly stolen shell snippets warrant their own
scripts, or how make targets overriding various variables would be
received. At least they're in the commit message.

-- >8 --
We have quite a lot of leaks in the code base. Using SANITIZE=leak, it
is easy to find them, and every now and then we simply stumble upon one.
Still, we can expect it to take some time to get to the point where
`make SANITIZE=leak test` succeeds.

Until that happens, it would be nice if we could at least try not to
regress in the sense that a test t which was at one point leak-free
turns leaky. Such a regression would indicate that leak-free code
turned leaky, that a new feature is leaky, or that we simply happen to
trigger an existing leak as part of a newly added/modified test.

To that end, introduce a list of known-leaky tests, i.e., a list of
t-values. Now this will be able to find such regressions:

make SANITIZE=leak test GIT_SKIP_TESTS="$(cat t/known-leaky)"

The list was generated, and can be updated, as follows:

# Assume we're using prove, which will keep running after failure,
# and will record the results for us to parse (using "--state=").
# Otherwise use "make -k" and grep in t/test-results.
GIT_TEST_OPTS=-i make SANITIZE=leak test
cd t
prove --dry --state=failed |
perl -lne '/^(t[0-9]{4})-.*\.sh$/ and print $1' |
sort >known-leaky

The list added in this commit might be incomplete, since I do not run
all tests (I'm missing svn, cvs, p4, Windows-only and
filesystem-dependent tests, as well as "writeable /"). The majority of
these do not primarily test our C code, but all of them might trigger
leaks, in which case they would belong in this list.

Suggested-by: Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
Helped-by: Jeff King <p...@peff.net>
Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 t/known-leaky | 539 ++
 1 file changed, 539 insertions(+)
 create mode 100644 t/known-leaky

diff --git a/t/known-leaky b/t/known-leaky
new file mode 100644
index 00..80a0af4d09
--- /dev/null
+++ b/t/known-leaky
@@ -0,0 +1,539 @@
+t0002
+t0003
+t0006
+t0008
+t0009
+t0012
+t0020
+t0021
+t0023
+t0040
+t0050
+t0056
+t0060
+t0062
+t0064
+t0070
+t0090
+t0100
+t0101
+t0110
+t0203
+t0300
+t0301
+t0302
+t1001
+t1002
+t1004
+t1005
+t1006
+t1007
+t1008
+t1011
+t1012
+t1013
+t1020
+t1021
+t1050
+t1051
+t1090
+t1301
+t1302
+t1304
+t1306
+t1308
+t1350
+t1400
+t1401
+t1402
+t1403
+t1404
+t1405
+t1406
+t1407
+t1408
+t1409
+t1410
+t1411
+t1412
+t1413
+t1414
+t1430
+t1450
+t1500
+t1502
+t1505
+t1507
+t1508
+t1510
+t1511
+t1512
+t1514
+t1700
+t2007
+t2008
+t2009
+t2010
+t2011
+t2012
+t2013
+t2014
+t2015
+t2016
+t2017
+t2018
+t2019
+t2020
+t2021
+t2022
+t2023
+t2024
+t2025
+t2026
+t2027
+t2030
+t2103
+t2106
+t2200
+t2203
+t2204
+t3001
+t3004
+t3005
+t3007
+t3010
+t3020
+t3030
+t3031
+t3032
+t3033
+t3034
+t3040
+t3050
+t3060
+t3200
+t3201
+t3202
+t3203
+t3204
+t3205
+t3210
+t3301
+t3302
+t3303
+t3304
+t3306
+t3307
+t3308
+t3309
+t3310
+t3311
+t3400
+t3402
+t3403
+t3404
+t3405
+t3406
+t3407
+t3408
+t3409
+t3410
+t3411
+t3412
+t3413
+t3414
+t3415
+t3416
+t3417
+t3418
+t3419
+t3420
+t3421
+t3425
+t3426
+t3427
+t3428
+t3429
+t3500
+t3501
+t3502
+t3503
+t3504
+t3505
+t3506
+t3507
+t3508
+t3509
+t3510
+t3511
+t3512
+t3513
+t3600
+t3700
+t3701
+t3800
+t3900
+t3901
+t3903
+t3904
+t3905
+t3906
+t4001
+t4008
+t4010
+t4013
+t4014
+t4015
+t4016
+t4017
+t4018
+t4020
+t4021
+t4022
+t4023
+t4026
+t4027
+t4028
+t4030
+t4031
+t4036
+t4038
+t4039
+t4041
+t4042
+t4043
+t4044
+t4045
+t4047
+t4048
+t4049
+t4051
+t4052
+t4053
+t4055
+t4056
+t4057
+t4059
+t4060
+t4061
+t4064
+t4065
+t4103
+t4107
+t4108
+t4111
+t4114
+t4115
+t4117
+t4118
+t4120
+t4121
+t4122
+t4124
+t4125
+t4127
+t4131
+t4135
+t4137
+t4138
+t4150
+t4151
+t4152
+t4153
+t4200
+t4201
+t4202
+t4203
+t4204
+t4205
+t4206
+t4207
+t4208
+t4209
+t4210
+t4211
+t4212
+t4213
+t4252
+t4253
+t4254
+t4255
+t4300
+t5000
+t5001
+t5002
+t5003
+t5004
+t5100
+t5150
+t5300
+t5301
+t5302
+t5303
+t5304
+t5305
+t5306
+t5310
+t5311
+t5312
+t5313
+t5314
+t5315
+t5316
+t5317
+t5400
+t5401
+t5402
+t5403
+t5404
+t5405
+t5406
+t5407
+t5408
+t5500
+t5501
+t5502
+t5503
+t5504
+t5505
+t5506
+t5509
+t5510
+t5512
+t5513
+t5514
+t5515
+t5516
+t5517
+t5518
+t5519
+t5520
+t5521
+t5522
+t5523
+t5524
+t5525
+t5526
+t5527
+t5528
+t5531
+t5532
+t5533
+t5534
+t5535
+t5536
+t5537
+t5538
+t5539
+t5540
+t5541
+t5542
+t5543
+t5544
+t5545
+t5546
+t5547
+t5550
+t5551
+t5560
+t556

[PATCH v2 2/3] config: respect `pager.config` in list/get-mode only

[Martin Ågren]

Similar to de121ffe5 (tag: respect `pager.tag` in list-mode only,
2017-08-02), use the DELAY_PAGER_CONFIG-mechanism to only respect
`pager.config` when we are listing or "get"ing config.

We have several getters and some are guaranteed to give at most one line
of output. Paging all getters including those could be convenient from a
documentation point-of-view. The downside would be that a misconfigured
or not so modern pager might wait for user interaction before
terminating. Let's instead respect the config for precisely those
getters which may produce more than one line of output.

`--get-urlmatch` may or may not produce multiple lines of output,
depending on the exact usage. Let's not try to recognize the two modes,
but instead make `--get-urlmatch` always respect the config. Analyzing
the detailed usage might be trivial enough here, but could establish a
precedent that we will never be able to enforce throughout the codebase
and that will just open a can of worms.

This fixes the failing test added in the previous commit. Also adapt the
test for whether `git config foo.bar bar` and `git config --get foo.bar`
respects `pager.config`.

Signed-off-by: Martin Ågren <martin.ag...@gmail.com>
---
 Documentation/git-config.txt |  5 +
 t/t7006-pager.sh | 10 +-
 builtin/config.c | 10 ++
 git.c|  2 +-
 4 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 14da5fc157..249090ac84 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -233,6 +233,11 @@ See also <>.
using `--file`, `--global`, etc) and `on` when searching all
config files.
 
+CONFIGURATION
+-
+`pager.config` is only respected when listing configuration, i.e., when
+using `--list` or any of the `--get-*` which may return multiple results.
+
 [[FILES]]
 FILES
 -
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index a46a079339..95bd26f0b2 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -245,13 +245,13 @@ test_expect_success TTY 'git branch --set-upstream-to 
ignores pager.branch' '
! test -e paginated.out
 '
 
-test_expect_success TTY 'git config respects pager.config when setting' '
+test_expect_success TTY 'git config ignores pager.config when setting' '
rm -f paginated.out &&
test_terminal git -c pager.config config foo.bar bar &&
-   test -e paginated.out
+   ! test -e paginated.out
 '
 
-test_expect_failure TTY 'git config --edit ignores pager.config' '
+test_expect_success TTY 'git config --edit ignores pager.config' '
rm -f paginated.out editor.used &&
write_script editor <<-\EOF &&
touch editor.used
@@ -261,10 +261,10 @@ test_expect_failure TTY 'git config --edit ignores 
pager.config' '
test -e editor.used
 '
 
-test_expect_success TTY 'git config --get respects pager.config' '
+test_expect_success TTY 'git config --get ignores pager.config' '
rm -f paginated.out &&
test_terminal git -c pager.config config --get foo.bar &&
-   test -e paginated.out
+   ! test -e paginated.out
 '
 
 test_expect_success TTY 'git config --get-urlmatch defaults to not paging' '
diff --git a/builtin/config.c b/builtin/config.c
index ab5f95476e..a732d9b56c 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -48,6 +48,13 @@ static int show_origin;
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
 
+/*
+ * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
+ * one line of output and which should therefore be paged.
+ */
+#define PAGING_ACTIONS (ACTION_LIST | ACTION_GET_ALL | \
+   ACTION_GET_REGEXP | ACTION_GET_URLMATCH)
+
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
 #define TYPE_BOOL_OR_INT (1<<2)
@@ -594,6 +601,9 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
 
+   if (actions & PAGING_ACTIONS)
+   setup_auto_pager("config", 0);
+
if (actions == ACTION_LIST) {
check_argc(argc, 0, 0);
if (config_with_options(show_all_config, NULL,
diff --git a/git.c b/git.c
index c870b9719c..e5c9b8729d 100644
--- a/git.c
+++ b/git.c
@@ -389,7 +389,7 @@ static struct cmd_struct commands[] = {
{ "column", cmd_column, RUN_SETUP_GENTLY },
{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-   { "config", cmd_config, RUN_SETUP_GENTLY },
+   { "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
{ "count-objects", cmd_count_objects, RUN_SETUP },
{ "credential", cmd_credential, RUN_SETUP_GENTLY },
{ "describe", cmd_describe, RUN_SETUP },
-- 
2.16.2.246.ga4ee8f