Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules

[Stefan Beller]

+ Heiko, who touched the pushing code end of last year.

On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams  wrote:
> There are currently two instances (fetch and push) where we want to
> determine if submodules have changed given some revision specification.
> These two instances don't use the same logic to generate a list of
> changed submodules and as a result there is a fair amount of code
> duplication.
>
> This patch refactors these two code paths such that they both use the
> same logic to generate a list of changed submodules.  This also makes it
> easier for future callers to be able to reuse this logic as they only
> need to create an argv_array with the revision specification to be using
> during the revision walk.

Thanks for doing some refactoring. :)

> -static struct oid_array *submodule_commits(struct string_list *submodules,
> -   const char *path)
> ...

> -static void free_submodules_oids(struct string_list *submodules)
> -{
> ...

These are just moved north, no change in code.
If you want to be extra nice to reviewers this could have been an extra patch,
as it makes reviewing easier if you just have to look at the lines of code with
one goal ("shuffling code around, no change intended" vs "make a change
to improve things with no bad side effects")



> +
> +static void collect_changed_submodules_cb(struct diff_queue_struct *q,
> + struct diff_options *options,
> + void *data)
> +{

This one combines both collect_submodules_from_diff and
submodule_collect_changed_cb ?

> @@ -921,61 +948,6 @@ int push_unpushed_submodules(struct oid_array *commits,
> return ret;
>  }
>
> -static int is_submodule_commit_present(const char *path, unsigned char 
> sha1[20])
> -{
> -   int is_present = 0;
> -   if (!add_submodule_odb(path) && lookup_commit_reference(sha1)) {
> -   /* Even if the submodule is checked out and the commit is
> -* present, make sure it is reachable from a ref. */
> -   struct child_process cp = CHILD_PROCESS_INIT;
> -   const char *argv[] = {"rev-list", "-n", "1", NULL, "--not", 
> "--all", NULL};
> -   struct strbuf buf = STRBUF_INIT;
> -
> -   argv[3] = sha1_to_hex(sha1);
> -   cp.argv = argv;
> -   prepare_submodule_repo_env(_array);
> -   cp.git_cmd = 1;
> -   cp.no_stdin = 1;
> -   cp.dir = path;
> -   if (!capture_command(, , 1024) && !buf.len)
> -   is_present = 1;

Oh, I see where the nit in patch 5/6 is coming from. Another note
on that: The hint is way off. The hint should be on the order of
GIT_SHA1_HEXSZ

>  int find_unpushed_submodules(struct oid_array *commits,
> const char *remotes_name, struct string_list *needs_pushing)
> ...

>  static void calculate_changed_submodule_paths(void)
> ...

These are both nicely clean now.

Thanks,
Stefan

Re: [PATCH 5/6] submodule: improve submodule_has_commits

[Stefan Beller]

On Fri, Apr 28, 2017 at 4:54 PM, Brandon Williams  wrote:
> Teach 'submodule_has_commits()' to ensure that if a commit exists in a
> submodule, that it is also reachable from a ref.
>
> This is a prepritory step prior to merging the logic which checkes for

s/prepritory/preparatory/
s/checkes/checks/

This is the first commit in the series that changes user observable behavior,
I guess there will be tests in a later patch? Can you elaborate in this commit
message more about why it is useful (or at least harmless for performing
this check in the case of fetch/push)?

> diff --git a/submodule.c b/submodule.c
> index 3bcf44521..100d31d39 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, 
> struct oid_array *commits)
> return 0;
>
> oid_array_for_each_unique(commits, check_has_commit, _commit);
> +
> +   if (has_commit) {
> +   /*
> +* Even if the submodule is checked out and the commit is
> +* present, make sure it is reachable from a ref.
> +*/
> +   struct child_process cp = CHILD_PROCESS_INIT;
> +   struct strbuf out = STRBUF_INIT;
> +
> +   argv_array_pushl(, "rev-list", "-n", "1", NULL);
> +   oid_array_for_each_unique(commits, append_oid_to_argv, 
> );
> +   argv_array_pushl(, "--not", "--all", NULL);
> +
> +   prepare_submodule_repo_env(_array);
> +   cp.git_cmd = 1;
> +   cp.no_stdin = 1;
> +   cp.dir = path;
> +
> +   if (capture_command(, , 1024) || out.len)
> +   has_commit = 0;

So if we fail to launch capture_command, we assume we do not
have the commits?

capture_command can fail for reasons that are hard to track down
or even spurious (OOM due to excessive output, disk failure,
corrupt repo, error in executing the process, getting a signal and
so on), some of them are ok to ignore, others should never be ignored.

So I'd rather die on capture_command, and inspect out.len only
in case of successful capturing.

In addition to that we're only interested if there is any output,
such that we can optimize further:
c.f. 
http://public-inbox.org/git/20170324223848.gh31...@aiede.mtv.corp.google.com/

if (start_command())
die("cannot start git-rev-list in submodule'%s', sub->path);

/* read only one character, if any */
if (xread(cp.out, , 1);
has_commit = 0;

/*
 * close cp.out, such that the child may get SIGPIPE, upon which
 * it dies (silently, maybe we need to suppress cp.err ?)
 */
close(cp.out);

/*
 * Though we need to be nitpicky about finish_command IIUC by now:
 * TODO(sbeller): if this turns out to be true, fixup is_submodule_modified
*/
int code = finish_command();
if (!code && code != 128 + SIGPIPE)
die("git rev-list failed in submodule'%s'", sub->path);

Upon rereading the patch, I notice the '-n 1', which would make the
optimized code above useless, so just consider it food for thought.

Thanks,
Stefan

Re: [PATCH] t7400: add BSLASHPSPEC prerequisite to 'add with \\ in path'

[Ramsay Jones]

On 28/04/17 20:54, Johannes Sixt wrote:
> Am 28.04.2017 um 05:09 schrieb Junio C Hamano:
>> Ramsay Jones  writes:
>>
>>> Commit cf9e55f494 ("submodule: prevent backslash expantion in submodule
>>> names", 07-04-2017) added a test which creates a git repository with
>>> some backslash characters in the name. This test cannot work on windows,
>>> since the backslash is used as the directory separator. In order to
>>> suppress this test on cygwin, MinGW and Git for Windows, we add the
>>> BSLASHPSPEC prerequisite. (see also commits 6fd1106aa4 and c51c0da222).
> 
> First, let me say that meaning of BSLASHPSPEC was
> "keeps backslaches in pathspec arguments" originally,
> but it apparently changed meaning since then.

Indeed. I started to give some of the history in the commit message, but
it was nearly 3am, so I punted with the 'see also commits 6fd1106aa4 and
c51c0da222' ... ;-)
 
> t7400.20 does not fail for the MinGW port because the
> test case only operates on the file system, but never
> checks whether an entry 'sub\with\backslash' is present
> in the index.

Ah, OK. I only looked at my (64-bit) MSYS2 build, which fails
exactly the same as cygwin. Hmm, wait, let me just rebuild on
MinGW64 ... indeed it passes (well it passes t7400.20, but it
fails on t7400.11, 61, 63, 87 and 89)!

> 
>>> @@ -273,7 +273,7 @@ test_expect_success 'submodule add with ./, /.. and // 
>>> in path' '
>>>  test_cmp empty untracked
>>>  '
>>>
>>> -test_expect_success 'submodule add with \\ in path' '
>>> +test_expect_success BSLASHPSPEC 'submodule add with \\ in path' '
>>>  test_when_finished "rm -rf parent sub\\with\\backslash" &&
>>>
>>>  # Initialize a repo with a backslash in its name
>>
> 
> I assume the test fails right at 'git init' under Cygwin?

Indeed. Also on MSYS2 (exactly as on cygwin):

ramsay@satellite  MSYS $ ./t7400-submodule-basic.sh -i -v

...

ok 19 - submodule add with ./, /.. and // in path

expecting success:
test_when_finished "rm -rf parent sub\\with\\backslash" &&

# Initialize a repo with a backslash in its name
git init sub\\with\\backslash &&
touch sub\\with\\backslash/empty.file &&
git -C sub\\with\\backslash add empty.file &&
git -C sub\\with\\backslash commit -m "Added empty.file" &&

# Add that repository as a submodule
git init parent &&
git -C parent submodule add ../sub\\with\\backslash

fatal: cannot mkdir sub\with\backslash: No such file or directory
not ok 20 - submodule add with \\ in path
#
#   test_when_finished "rm -rf parent sub\\with\\backslash" &&
#
#   # Initialize a repo with a backslash in its name
#   git init sub\\with\\backslash &&
#   touch sub\\with\\backslash/empty.file &&
#   git -C sub\\with\\backslash add empty.file &&
#   git -C sub\\with\\backslash commit -m "Added empty.file" &&
#
#   # Add that repository as a submodule
#   git init parent &&
#   git -C parent submodule add ../sub\\with\\backslash
#
ramsay@satellite  MSYS $

ramsay@satellite  MSYS $ cd trash\ directory.t7400-submodule-basic/

ramsay@satellite  MSYS $ ls
a   addtest/ empty   expect-head   head   head-sha1  untracked
actual  addtest-ignore/  expect  expect-heads  heads  t  z

ramsay@satellite  MSYS $ git init sub\\with\\backslash
fatal: cannot mkdir sub\with\backslash: No such file or directory

ramsay@satellite  MSYS $ mkdir -p sub\\with

ramsay@satellite  MSYS $ git init sub\\with\\backslash
Initialized empty Git repository in /home/ramsay/git/t/trash 
directory.t7400-submodule-basic/sub/with/backslash/.git/

ramsay@satellite  MSYS $ touch sub\\with\\backslash/empty.file
ramsay@satellite  MSYS $ git -C sub\\with\\backslash add empty.file
ramsay@satellite  MSYS $ git -C sub\\with\\backslash commit -m "Added 
empty.file"
[master (root-commit) 6fde90b] Added empty.file
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 empty.file

ramsay@satellite  MSYS $ git init parent
Initialized empty Git repository in /home/ramsay/git/t/trash 
directory.t7400-submodule-basic/parent/.git/

ramsay@satellite  MSYS $ git -C parent submodule add ../sub\\with\\backslash
Cloning into '/home/ramsay/git/t/trash 
directory.t7400-submodule-basic/parent/sub/with/backslash'...
done.
fatal: Not a git repository: /home/ramsay/git/t/trash 
directory.t7400-submodule-basic/parent/sub\with\backslash/../.git/modules/sub/with/backslash

ramsay@satellite  MSYS $ echo $?
128
ramsay@satellite  MSYS $

Do you not see this on msys?

> 
> BSLASHPSPEC (with its *new* meaning) would be the
> right prerequisite if the check for the index entry
> were added. Until then, !CYGWIN is more appropriate.

OK, I'll re-roll the patch (tomorrow, it's late again).

Thanks.

ATB,
Ramsay Jones

[PATCH 1/6] submodule: rename add_sha1_to_array

[Brandon Williams]

Rename 'add_sha1_to_array()' to 'append_oid_to_array()' to more
accuratly describe what the function does since it handles 'struct
object_id' and not sha1 character arrays.

Signed-off-by: Brandon Williams 
---
 submodule.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index d3299e29c..be0f5d847 100644
--- a/submodule.c
+++ b/submodule.c
@@ -951,17 +951,18 @@ static void submodule_collect_changed_cb(struct 
diff_queue_struct *q,
}
 }
 
-static int add_sha1_to_array(const char *ref, const struct object_id *oid,
-int flags, void *data)
+static int append_oid_to_array(const char *ref, const struct object_id *oid,
+  int flags, void *data)
 {
-   oid_array_append(data, oid);
+   struct oid_array *array = data;
+   oid_array_append(array, oid);
return 0;
 }
 
 void check_for_new_submodule_commits(struct object_id *oid)
 {
if (!initialized_fetch_ref_tips) {
-   for_each_ref(add_sha1_to_array, _tips_before_fetch);
+   for_each_ref(append_oid_to_array, _tips_before_fetch);
initialized_fetch_ref_tips = 1;
}
 
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 6/6] submodule: refactor logic to determine changed submodules

[Brandon Williams]

There are currently two instances (fetch and push) where we want to
determine if submodules have changed given some revision specification.
These two instances don't use the same logic to generate a list of
changed submodules and as a result there is a fair amount of code
duplication.

This patch refactors these two code paths such that they both use the
same logic to generate a list of changed submodules.  This also makes it
easier for future callers to be able to reuse this logic as they only
need to create an argv_array with the revision specification to be using
during the revision walk.

Signed-off-by: Brandon Williams 
---
 submodule.c | 247 ++--
 1 file changed, 105 insertions(+), 142 deletions(-)

diff --git a/submodule.c b/submodule.c
index 100d31d39..88093779e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -617,6 +617,94 @@ const struct submodule *submodule_from_ce(const struct 
cache_entry *ce)
return submodule_from_path(null_sha1, ce->name);
 }
 
+static struct oid_array *submodule_commits(struct string_list *submodules,
+  const char *path)
+{
+   struct string_list_item *item;
+
+   item = string_list_insert(submodules, path);
+   if (item->util)
+   return (struct oid_array *) item->util;
+
+   /* NEEDSWORK: should we have oid_array_init()? */
+   item->util = xcalloc(1, sizeof(struct oid_array));
+   return (struct oid_array *) item->util;
+}
+
+static void collect_changed_submodules_cb(struct diff_queue_struct *q,
+ struct diff_options *options,
+ void *data)
+{
+   int i;
+   struct string_list *changed = data;
+
+   for (i = 0; i < q->nr; i++) {
+   struct diff_filepair *p = q->queue[i];
+   struct oid_array *commits;
+   if (!S_ISGITLINK(p->two->mode))
+   continue;
+
+   if (S_ISGITLINK(p->one->mode)) {
+   /*
+* NEEDSWORK: We should honor the name configured in
+* the .gitmodules file of the commit we are examining
+* here to be able to correctly follow submodules
+* being moved around.
+*/
+   commits = submodule_commits(changed, p->two->path);
+   oid_array_append(commits, >two->oid);
+   } else {
+   /* Submodule is new or was moved here */
+   /*
+* NEEDSWORK: When the .git directories of submodules
+* live inside the superprojects .git directory some
+* day we should fetch new submodules directly into
+* that location too when config or options request
+* that so they can be checked out from there.
+*/
+   continue;
+   }
+   }
+}
+
+/*
+ * Collect the paths of submodules in 'changed' which have changed based on
+ * the revisions as specified in 'argv'.  Each entry in 'changed' will also
+ * have a corresponding 'struct oid_array' (in the 'util' field) which lists
+ * what the submodule pointers were updated to during the change.
+ */
+static void collect_changed_submodules(struct string_list *changed,
+  struct argv_array *argv)
+{
+   struct rev_info rev;
+   const struct commit *commit;
+
+   init_revisions(, NULL);
+   setup_revisions(argv->argc, argv->argv, , NULL);
+   if (prepare_revision_walk())
+   die("revision walk setup failed");
+
+   while ((commit = get_revision())) {
+   struct rev_info diff_rev;
+
+   init_revisions(_rev, NULL);
+   diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+   diff_rev.diffopt.format_callback = 
collect_changed_submodules_cb;
+   diff_rev.diffopt.format_callback_data = changed;
+   diff_tree_combined_merge(commit, 1, _rev);
+   }
+
+   reset_revision_walk();
+}
+
+static void free_submodules_oids(struct string_list *submodules)
+{
+   struct string_list_item *item;
+   for_each_string_list_item(item, submodules)
+   oid_array_clear((struct oid_array *) item->util);
+   string_list_clear(submodules, 1);
+}
+
 static int has_remote(const char *refname, const struct object_id *oid,
  int flags, void *cb_data)
 {
@@ -719,92 +807,31 @@ static int submodule_needs_pushing(const char *path, 
struct oid_array *commits)
return 0;
 }
 
-static struct oid_array *submodule_commits(struct string_list *submodules,
-   const char *path)
-{
-   struct string_list_item *item;
-
-   

[PATCH 3/6] submodule: remove add_oid_to_argv

[Brandon Williams]

The function 'add_oid_to_argv()' provides the same functionality as
'append_oid_to_argv()'.  Remove this duplicate function and instead use
'append_oid_to_argv()' where 'add_oid_to_argv()' was previously used.

Signed-off-by: Brandon Williams 
---
 submodule.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 46abd52b1..7baa28ae0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -970,12 +970,6 @@ void check_for_new_submodule_commits(struct object_id *oid)
oid_array_append(_tips_after_fetch, oid);
 }
 
-static int add_oid_to_argv(const struct object_id *oid, void *data)
-{
-   argv_array_push(data, oid_to_hex(oid));
-   return 0;
-}
-
 static void calculate_changed_submodule_paths(void)
 {
struct rev_info rev;
@@ -989,10 +983,10 @@ static void calculate_changed_submodule_paths(void)
init_revisions(, NULL);
argv_array_push(, "--"); /* argv[0] program name */
oid_array_for_each_unique(_tips_after_fetch,
-  add_oid_to_argv, );
+  append_oid_to_argv, );
argv_array_push(, "--not");
oid_array_for_each_unique(_tips_before_fetch,
-  add_oid_to_argv, );
+  append_oid_to_argv, );
setup_revisions(argv.argc, argv.argv, , NULL);
if (prepare_revision_walk())
die("revision walk setup failed");
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 5/6] submodule: improve submodule_has_commits

[Brandon Williams]

Teach 'submodule_has_commits()' to ensure that if a commit exists in a
submodule, that it is also reachable from a ref.

This is a prepritory step prior to merging the logic which checkes for
changed submodules when fetching or pushing.

Signed-off-by: Brandon Williams 
---
 submodule.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/submodule.c b/submodule.c
index 3bcf44521..100d31d39 100644
--- a/submodule.c
+++ b/submodule.c
@@ -648,6 +648,30 @@ static int submodule_has_commits(const char *path, struct 
oid_array *commits)
return 0;
 
oid_array_for_each_unique(commits, check_has_commit, _commit);
+
+   if (has_commit) {
+   /*
+* Even if the submodule is checked out and the commit is
+* present, make sure it is reachable from a ref.
+*/
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct strbuf out = STRBUF_INIT;
+
+   argv_array_pushl(, "rev-list", "-n", "1", NULL);
+   oid_array_for_each_unique(commits, append_oid_to_argv, 
);
+   argv_array_pushl(, "--not", "--all", NULL);
+
+   prepare_submodule_repo_env(_array);
+   cp.git_cmd = 1;
+   cp.no_stdin = 1;
+   cp.dir = path;
+
+   if (capture_command(, , 1024) || out.len)
+   has_commit = 0;
+
+   strbuf_release();
+   }
+
return has_commit;
 }
 
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 2/6] submodule: rename free_submodules_sha1s

[Brandon Williams]

Rename 'free_submodules_sha1s()' to 'free_submodules_oids()' since the
function frees a 'struct string_list' which has a 'struct oid_array'
stored in the 'util' field.

Signed-off-by: Brandon Williams 
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index be0f5d847..46abd52b1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -738,7 +738,7 @@ static void find_unpushed_submodule_commits(struct commit 
*commit,
diff_tree_combined_merge(commit, 1, );
 }
 
-static void free_submodules_sha1s(struct string_list *submodules)
+static void free_submodules_oids(struct string_list *submodules)
 {
struct string_list_item *item;
for_each_string_list_item(item, submodules)
@@ -779,7 +779,8 @@ int find_unpushed_submodules(struct oid_array *commits,
if (submodule_needs_pushing(submodule->string, commits))
string_list_insert(needs_pushing, submodule->string);
}
-   free_submodules_sha1s();
+
+   free_submodules_oids();
 
return needs_pushing->nr;
 }
-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 0/6] changed submodules

[Brandon Williams]

A little bit ago I was looking for a function in our code base which would be
able to give me a list of all the submodules which changed across a number of
commits.  I stumbled upon some of that logic in both our recurisve fetch and
pull code paths.  Problem is both of these code paths were performing almost
identical tasks, without sharing any code.

This series is meant as a cleanup to submodule.c in order to unify these two
code paths as well as make it easier for future callers to be able to query for
submodules which have chagned across a number of commits.

Brandon Williams (6):
  submodule: rename add_sha1_to_array
  submodule: rename free_submodules_sha1s
  submodule: remove add_oid_to_argv
  submodule: change string_list changed_submodule_paths
  submodule: improve submodule_has_commits
  submodule: refactor logic to determine changed submodules

 submodule.c | 295 
 1 file changed, 139 insertions(+), 156 deletions(-)

-- 
2.13.0.rc0.306.g87b477812d-goog

[PATCH 4/6] submodule: change string_list changed_submodule_paths

[Brandon Williams]

Eliminate a call to 'xstrdup()' by changing the string_list
'changed_submodule_paths' to duplicated strings added to it.

Signed-off-by: Brandon Williams 
---
 submodule.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 7baa28ae0..3bcf44521 100644
--- a/submodule.c
+++ b/submodule.c
@@ -20,7 +20,7 @@
 static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int parallel_jobs = 1;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_NODUP;
+static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -939,7 +939,7 @@ static void submodule_collect_changed_cb(struct 
diff_queue_struct *q,
struct string_list_item *path;
path = 
unsorted_string_list_lookup(_submodule_paths, p->two->path);
if (!path && !is_submodule_commit_present(p->two->path, 
p->two->oid.hash))
-   string_list_append(_submodule_paths, 
xstrdup(p->two->path));
+   string_list_append(_submodule_paths, 
p->two->path);
} else {
/* Submodule is new or was moved here */
/* NEEDSWORK: When the .git directories of submodules
-- 
2.13.0.rc0.306.g87b477812d-goog

Bug Report: .gitignore behavior is not matching in git clean and git status

[Chris Johnson]

I am a mailing list noob so I’m sorry if this is the wrong format or
the wrong please.

Here’s the setup for the bug (I will call it a bug but I half expect
somebody to tell me I’m an idiot):

git init
echo "/A/B/" > .gitignore
git add .gitignore && git commit -m 'Add ignore'
mkdir -p A/B
touch A/B/C
git status
git clean -dn

(my client may have screwed up the quotes, please bear with me)

Now, this may just be a case of me misunderstanding the behavior of
.gitignore, and that’s fine, but to me, git clean -dn should roughly
reflect the same behavior as git status as far as ignored files go. As
it is, git status does NOT report A/B/C as an untracked file, but git
clean will remove the entire A dir. It seems to me that one or the
other’s behavior ought to be tweaked. Which one is correct I am not
sure.

This happens on 2.5.1 as well as 2.9.2

Chris

[PATCHv2 2/3] diff: have the diff-* builtins configure diff before initializing revisions

[Marc Branchaud]

This makes the commands respect diff configuration options, such as
indentHeuristic, because init_revisions() calls diff_setup() which fills
in the diff_options struct.

Signed-off-by: Marc Branchaud 
---
 builtin/diff-files.c   |  2 +-
 builtin/diff-index.c   |  2 +-
 builtin/diff-tree.c|  2 +-
 t/t4061-diff-indent.sh | 66 ++
 4 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 15c61fd8d..a572da9d5 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char 
*prefix)
int result;
unsigned options = 0;
 
+   git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
init_revisions(, prefix);
gitmodules_config();
-   git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
rev.abbrev = 0;
precompose_argv(argc, argv);
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index 1af373d00..f084826a2 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char 
*prefix)
int i;
int result;
 
+   git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
init_revisions(, prefix);
gitmodules_config();
-   git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
rev.abbrev = 0;
precompose_argv(argc, argv);
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 326f88b65..36a3a1976 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char 
*prefix)
struct setup_revision_opt s_r_opt;
int read_stdin = 0;
 
+   git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
init_revisions(opt, prefix);
gitmodules_config();
-   git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
opt->abbrev = 0;
opt->diff = 1;
opt->disable_stdin = 1;
diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh
index 556450609..13d3dc96a 100755
--- a/t/t4061-diff-indent.sh
+++ b/t/t4061-diff-indent.sh
@@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic 
overrides config' '
compare_blame spaces-expect out-blame2
 '
 
+test_expect_success 'diff-tree: nice spaces with --indent-heuristic' '
+   git diff-tree --indent-heuristic -p old new -- spaces.txt 
>out-diff-tree-compacted &&
+   compare_diff spaces-compacted-expect out-diff-tree-compacted
+'
+
+test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' '
+   git -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt 
>out-diff-tree-compacted2 &&
+   compare_diff spaces-compacted-expect out-diff-tree-compacted2
+'
+
+test_expect_success 'diff-tree: --no-indent-heuristic overrides config' '
+   git -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old 
new -- spaces.txt >out-diff-tree &&
+   compare_diff spaces-expect out-diff-tree
+'
+
+test_expect_success 'diff-index: nice spaces with --indent-heuristic' '
+   git checkout -B diff-index &&
+   git reset --soft HEAD~ &&
+   git diff-index --indent-heuristic -p old -- spaces.txt 
>out-diff-index-compacted &&
+   compare_diff spaces-compacted-expect out-diff-index-compacted &&
+   git checkout -f master
+'
+
+test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' '
+   git checkout -B diff-index &&
+   git reset --soft HEAD~ &&
+   git -c diff.indentHeuristic=true diff-index -p old -- spaces.txt 
>out-diff-index-compacted2 &&
+   compare_diff spaces-compacted-expect out-diff-index-compacted2 &&
+   git checkout -f master
+'
+
+test_expect_success 'diff-index: --no-indent-heuristic overrides config' '
+   git checkout -B diff-index &&
+   git reset --soft HEAD~ &&
+   git -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p 
old -- spaces.txt >out-diff-index &&
+   compare_diff spaces-expect out-diff-index &&
+   git checkout -f master
+'
+
+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '
+   git checkout -B diff-files &&
+   git reset HEAD~ &&
+   git -c diff.indentHeuristic=true diff-files -p spaces.txt 
>out-diff-files-raw &&
+   grep -v index out-diff-files-raw >out-diff-files-compacted &&
+   compare_diff spaces-compacted-expect out-diff-files-compacted &&
+   git checkout -f master
+'
+
+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '
+   git checkout -B diff-files &&
+   git reset HEAD~ &&
+   git -c diff.indentHeuristic=true diff-files -p spaces.txt 
>out-diff-files-raw2 &&
+   grep -v index out-diff-files-raw2 >out-diff-files-compacted2 &&
+   compare_diff 

[PATCH 3/3] diff: enable indent heuristic by default

[Marc Branchaud]

From: Stefan Beller 

The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got was positive.

Turn it on by default. Users who dislike the feature can turn it off
by setting diff.compactionHeuristic (which includes plumbing commands,
see prior patches).

Signed-off-by: Stefan Beller 
Signed-off-by: Marc Branchaud 
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index da96577ea..2c47ccb4a 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,7 @@
 #endif
 
 static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
+static int diff_indent_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
-- 
2.13.0.rc1.15.gf67d331ad

[PATCHv2 1/3] diff: make the indent heuristic part of diff's basic configuration

[Marc Branchaud]

This heuristic was originally introduced as an experimental feature,
and therefore part of the UI configuration.

But the user often sees diffs generated by plumbing commands like
diff-tree.  Moving the indent heuristic into diff's basic configuration
prepares the way for diff plumbing commands to respect the setting.

The heuristic itself merely makes the diffs more aesthetically
pleasing, without changing their correctness.  Scripts that rely on
the diff plumbing commands should not care whether or not the heuristic
is employed.

Signed-off-by: Marc Branchaud 
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 11eef1c85..da96577ea 100644
--- a/diff.c
+++ b/diff.c
@@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
return 0;
}
 
-   if (git_diff_heuristic_config(var, value, cb) < 0)
-   return -1;
-
if (!strcmp(var, "diff.wserrorhighlight")) {
int val = parse_ws_error_highlight(value);
if (val < 0)
@@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char 
*value, void *cb)
if (starts_with(var, "submodule."))
return parse_submodule_config_option(var, value);
 
+   if (git_diff_heuristic_config(var, value, cb) < 0)
+   return -1;
+
return git_default_config(var, value, cb);
 }
 
-- 
2.13.0.rc1.15.gf67d331ad

[PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.

[Marc Branchaud]

v2: Fixed up the commit messages and added tests.

Marc Branchaud (2):
  diff: make the indent heuristic part of diff's basic configuration
  diff: have the diff-* builtins configure diff before initializing
revisions

Stefan Beller (1):
  diff: enable indent heuristic by default

 builtin/diff-files.c   |  2 +-
 builtin/diff-index.c   |  2 +-
 builtin/diff-tree.c|  2 +-
 diff.c |  8 +++---
 t/t4061-diff-indent.sh | 66 ++
 5 files changed, 73 insertions(+), 7 deletions(-)

-- 
2.13.0.rc1.15.gf67d331ad

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Stefan Beller]

On Fri, Apr 28, 2017 at 3:04 PM, Jeff King  wrote:
> On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:
>
>> > So instead I chose to make the indentHeuristic option part of diff's basic
>> > configuration, and in each of the diff plumbing commands I moved the call 
>> > to
>> > git_config() before the call to init_revisions().
>> [...]
>>
>> The feature was included in v2.11 (released 2016-11-29) and we got no
>> negative feedback. Quite the opposite, all feedback we got, was positive.
>> This could be explained by having the feature as an experimental feature
>> and users who would be broken by it, did not test it yet or did not speak up.
>
> Yeah, if the point you're trying to make is "nobody will be mad if this
> is turned on by default", I don't think shipping it as a config option
> is very conclusive.
>
> I think a more interesting argument is: we turned on renames by default
> a few versions ago, which changes the diff in a much bigger way, and
> nobody complained.
>
>   As a side note, I do happen to know of one program that depends
>   heavily on diffs remaining stable. Imagine you have a Git hosting site
>   which lets people comment on lines of diffs. You need some way to
>   address the lines of the diff so that the annotations appear in the
>   correct position when you regenerate the diff later.
>
>   One way to do it is to just position the comment at the n'th line of
>   the diff. Which obviously breaks if the diff changes. IMHO that is a
>   bug in that program, which should be fixed to use the line numbers
>   from the original blob (which is still not foolproof, because a
>   different diff algorithm may move a change such that the line isn't
>   even part of the diff anymore).
>
>   I'm not worried about this particular program, as I happen to know it
>   has already been fixed. But it's possible others have made a similar
>   mistake.

Thanks for this enlightenment. :)

>> So I'd propose to turn it on by default and anyone negatively impacted by 
>> that
>> could then use the config to turn it off for themselves (including plumbing).
>>
>> Something like this, maybe?
>
> Yeah, as long as this is on top of Marc's patches, I think it is the
> natural conclusion that we had planned.

That is what I was trying to hint at with the commit message given.

> I don't know if we would want to be extra paranoid about patch-ids.
> There is no helping:
>
>   git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable
>
> because diff-tree doesn't know that it's trying for "--stable" output.
> But the diffs we compute internally for patch-id could disable the
> heuristics. I'm not sure if those matter, though. AFAIK those are used
> only for internal comparisons within a single program. I.e., we never
> compare them against input from the user, nor do we output them to the
> user. So they'll change, but I don't think anybody would care.
>
> -Peff

Well, I think as long as we have a reasonable easy way to undo
the default (hence keeping the option), we can proceed with caution
here, too.

Thanks,
Stefan

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Jeff King]

On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:

> > So instead I chose to make the indentHeuristic option part of diff's basic
> > configuration, and in each of the diff plumbing commands I moved the call to
> > git_config() before the call to init_revisions().
> [...]
> 
> The feature was included in v2.11 (released 2016-11-29) and we got no
> negative feedback. Quite the opposite, all feedback we got, was positive.
> This could be explained by having the feature as an experimental feature
> and users who would be broken by it, did not test it yet or did not speak up.

Yeah, if the point you're trying to make is "nobody will be mad if this
is turned on by default", I don't think shipping it as a config option
is very conclusive.

I think a more interesting argument is: we turned on renames by default
a few versions ago, which changes the diff in a much bigger way, and
nobody complained.

  As a side note, I do happen to know of one program that depends
  heavily on diffs remaining stable. Imagine you have a Git hosting site
  which lets people comment on lines of diffs. You need some way to
  address the lines of the diff so that the annotations appear in the
  correct position when you regenerate the diff later.

  One way to do it is to just position the comment at the n'th line of
  the diff. Which obviously breaks if the diff changes. IMHO that is a
  bug in that program, which should be fixed to use the line numbers
  from the original blob (which is still not foolproof, because a
  different diff algorithm may move a change such that the line isn't
  even part of the diff anymore).

  I'm not worried about this particular program, as I happen to know it
  has already been fixed. But it's possible others have made a similar
  mistake.

> So I'd propose to turn it on by default and anyone negatively impacted by that
> could then use the config to turn it off for themselves (including plumbing).
> 
> Something like this, maybe?

Yeah, as long as this is on top of Marc's patches, I think it is the
natural conclusion that we had planned.

I don't know if we would want to be extra paranoid about patch-ids.
There is no helping:

  git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable

because diff-tree doesn't know that it's trying for "--stable" output.
But the diffs we compute internally for patch-id could disable the
heuristics. I'm not sure if those matter, though. AFAIK those are used
only for internal comparisons within a single program. I.e., we never
compare them against input from the user, nor do we output them to the
user. So they'll change, but I don't think anybody would care.

-Peff

Re: [PATCH 1/5] add SWAP macro

[Jeff King]

On Fri, Apr 28, 2017 at 07:04:51PM +0200, René Scharfe wrote:

> > What should:
> > 
> >SWAP(foo[i], foo[j]);
> > 
> > do when i == j? With this code, it ends up calling
> > 
> >memcpy([i], [j], ...);
> > 
> > which can cause valgrind to complain about overlapping memory. I suspect
> > in practice that noop copies are better off than partial overlaps, but I
> > think it does still violate the standard.
> > 
> > Is it worth comparing the pointers and bailing early?
> 
> Hmm, so swapping a value with itself can be a useful thing to do?
> Otherwise an assert would be more appropriate.

No, I doubt that it's useful, and it's probably a sign of a bug
elsewhere. But it's likely a _harmless_ bug, so it may be irritating to
die when we hit it rather than continuing.

I dunno. I could go either way. Or we could leave it as-is, and let
valgrind find the problem. That has zero run-time cost, but of course
nobody bothers to run valgrind outside of the test suite, so the inputs
are not usually very exotic.

> Swapping with *partial* overlap sounds tricky, or even evil.  If
> we want to support that for some reason we'd have to use memmove
> in the middle.  But that would still corrupt at least one of the
> objects, wouldn't it?

Yes, the overlap case is probably an actual bug. Detecting it is a bit
harder, but definitely possible. I hate to pay the run-time cost for it,
but I wonder if a compiler could optimize it out.

> The line in question is this one:
> 
>   for (i = 0; i <= (j = (queue->nr - 1) - i); i++)
> 
> Assignment in the middle?  Hmm.  Why not do it like this?
> 
>   for (i = 0, j = queue->nr - 1; i < j; i++, j--)
> 
> Looks less complicated to me.

Yes, see my other reply. :)

-Peff

[PATCH v4 09/10] t3415: test fixup with wrapped oneline

[Johannes Schindelin]

The `git commit --fixup` command unwraps wrapped onelines when
constructing the commit message, without wrapping the result.

We need to make sure that `git rebase --autosquash` keeps handling such
cases correctly, in particular since we are about to move the autosquash
handling into the rebase--helper.

Signed-off-by: Johannes Schindelin 
---
 t/t3415-rebase-autosquash.sh | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 6c37ebdff87..926bb3da788 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -316,4 +316,18 @@ test_expect_success 'extra spaces after fixup!' '
test $base = $parent
 '
 
+test_expect_success 'wrapped original subject' '
+   if test -d .git/rebase-merge; then git rebase --abort; fi &&
+   base=$(git rev-parse HEAD) &&
+   echo "wrapped subject" >wrapped &&
+   git add wrapped &&
+   test_tick &&
+   git commit --allow-empty -m "$(printf "To\nfixup")" &&
+   test_tick &&
+   git commit --allow-empty -m "fixup! To fixup" &&
+   git rebase -i --autosquash --keep-empty HEAD~2 &&
+   parent=$(git rev-parse HEAD^) &&
+   test $base = $parent
+'
+
 test_done
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v4 10/10] rebase -i: rearrange fixup/squash lines using the rebase--helper

[Johannes Schindelin]

This operation has quadratic complexity, which is especially painful
on Windows, where shell scripts are *already* slow (mainly due to the
overhead of the POSIX emulation layer).

Let's reimplement this with linear complexity (using a hash map to
match the commits' subject lines) for the common case; Sadly, the
fixup/squash feature's design neglected performance considerations,
allowing arbitrary prefixes (read: `fixup! hell` will match the
commit subject `hello world`), which means that we are stuck with
quadratic performance in the worst case.

The reimplemented logic also happens to fix a bug where commented-out
lines (representing empty patches) were dropped by the previous code.

While at it, clarify how the fixup/squash feature works in `git rebase
-i`'s man page.

Signed-off-by: Johannes Schindelin 
---
 Documentation/git-rebase.txt |  16 ++--
 builtin/rebase--helper.c |   6 +-
 git-rebase--interactive.sh   |  90 +---
 sequencer.c  | 195 +++
 sequencer.h  |   1 +
 t/t3415-rebase-autosquash.sh |   2 +-
 6 files changed, 212 insertions(+), 98 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 53f4e14..c5464aa5365 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -430,13 +430,15 @@ without an explicit `--interactive`.
 --autosquash::
 --no-autosquash::
When the commit log message begins with "squash! ..." (or
-   "fixup! ..."), and there is a commit whose title begins with
-   the same ..., automatically modify the todo list of rebase -i
-   so that the commit marked for squashing comes right after the
-   commit to be modified, and change the action of the moved
-   commit from `pick` to `squash` (or `fixup`).  Ignores subsequent
-   "fixup! " or "squash! " after the first, in case you referred to an
-   earlier fixup/squash with `git commit --fixup/--squash`.
+   "fixup! ..."), and there is already a commit in the todo list that
+   matches the same `...`, automatically modify the todo list of rebase
+   -i so that the commit marked for squashing comes right after the
+   commit to be modified, and change the action of the moved commit
+   from `pick` to `squash` (or `fixup`).  A commit matches the `...` if
+   the commit subject matches, or if the `...` refers to the commit's
+   hash. As a fall-back, partial matches of the commit subject work,
+   too.  The recommended way to create fixup/squash commits is by using
+   the `--fixup`/`--squash` options of linkgit:git-commit[1].
 +
 This option is only valid when the `--interactive` option is used.
 +
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index de3ccd9bfbc..e6591f01112 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -33,6 +33,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("check the todo list"), CHECK_TODO_LIST),
OPT_CMDMODE(0, "skip-unnecessary-picks", ,
N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
+   OPT_CMDMODE(0, "rearrange-squash", ,
+   N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_END()
};
 
@@ -59,5 +61,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!check_todo_list();
if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
return !!skip_unnecessary_picks();
+   if (command == REARRANGE_SQUASH && argc == 1)
+   return !!rearrange_squash();
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 92e3ca1de3b..84c6e62518f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -721,94 +721,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-sha1s
 }
 
-# Rearrange the todo list that has both "pick sha1 msg" and
-# "pick sha1 fixup!/squash! msg" appears in it so that the latter
-# comes immediately after the former, and change "pick" to
-# "fixup"/"squash".
-#
-# Note that if the config has specified a custom instruction format
-# each log message will be re-retrieved in order to normalize the
-# autosquash arrangement
-rearrange_squash () {
-   format=$(git config --get 

[PATCH v4 07/10] rebase -i: check for missing commits in the rebase--helper

[Johannes Schindelin]

In particular on Windows, where shell scripts are even more expensive
than on MacOSX or Linux, it makes sense to move a loop that forks
Git at least once for every line in the todo list into a builtin.

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   |   7 +-
 git-rebase--interactive.sh | 164 ++---
 sequencer.c| 122 +
 sequencer.h|   1 +
 4 files changed, 134 insertions(+), 160 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 9444c8d6c60..e706eac710d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,7 +13,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
+   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
+   CHECK_TODO_LIST
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -28,6 +29,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
OPT_CMDMODE(0, "expand-sha1s", ,
N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
+   OPT_CMDMODE(0, "check-todo-list", ,
+   N_("check the todo list"), CHECK_TODO_LIST),
OPT_END()
};
 
@@ -50,5 +53,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!transform_todo_ids(1);
if (command == EXPAND_SHA1S && argc == 1)
return !!transform_todo_ids(0);
+   if (command == CHECK_TODO_LIST && argc == 1)
+   return !!check_todo_list();
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 82a1941c42c..08168a0d46b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -867,96 +867,6 @@ add_exec_commands () {
mv "$1.new" "$1"
 }
 
-# Check if the SHA-1 passed as an argument is a
-# correct one, if not then print $2 in "$todo".badsha
-# $1: the SHA-1 to test
-# $2: the line number of the input
-# $3: the input filename
-check_commit_sha () {
-   badsha=0
-   if test -z "$1"
-   then
-   badsha=1
-   else
-   sha1_verif="$(git rev-parse --verify --quiet $1^{commit})"
-   if test -z "$sha1_verif"
-   then
-   badsha=1
-   fi
-   fi
-
-   if test $badsha -ne 0
-   then
-   line="$(sed -n -e "${2}p" "$3")"
-   warn "$(eval_gettext "\
-Warning: the SHA-1 is missing or isn't a commit in the following line:
- - \$line")"
-   warn
-   fi
-
-   return $badsha
-}
-
-# prints the bad commits and bad commands
-# from the todolist in stdin
-check_bad_cmd_and_sha () {
-   retval=0
-   lineno=0
-   while read -r command rest
-   do
-   lineno=$(( $lineno + 1 ))
-   case $command in
-   "$comment_char"*|''|noop|x|exec)
-   # Doesn't expect a SHA-1
-   ;;
-   "$cr")
-   # Work around CR left by "read" (e.g. with Git for
-   # Windows' Bash).
-   ;;
-   pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
-   if ! check_commit_sha "${rest%%[]*}" "$lineno" 
"$1"
-   then
-   retval=1
-   fi
-   ;;
-   *)
-   line="$(sed -n -e "${lineno}p" "$1")"
-   warn "$(eval_gettext "\
-Warning: the command isn't recognized in the following line:
- - \$line")"
-   warn
-   retval=1
-   ;;
-   esac
-   done <"$1"
-   return $retval
-}
-
-# Print the list of the SHA-1 of the commits
-# from stdin to stdout
-todo_list_to_sha_list () {
-   git stripspace --strip-comments |
-   while read -r command sha1 rest
-   do
-   case $command in
-   "$comment_char"*|''|noop|x|"exec")
-   ;;
-   *)
-   long_sha=$(git rev-list --no-walk "$sha1" 2>/dev/null)
-   printf "%s\n" "$long_sha"
-   ;;
-   esac
-   done
-}
-
-# Use warn for each line in stdin
-warn_lines () {
-   while read -r line
-   do
-   warn " - $line"
-   done
-}
-
 # Switch to the branch in $into and notify it in the 

[PATCH v4 08/10] rebase -i: skip unnecessary picks using the rebase--helper

[Johannes Schindelin]

In particular on Windows, where shell scripts are even more expensive
than on MacOSX or Linux, it makes sense to move a loop that forks
Git at least once for every line in the todo list into a builtin.

Note: The original code did not try to skip unnecessary picks of root
commits but punts instead (probably --root was not considered common
enough of a use case to bother optimizing). We do the same, for now.

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   |   6 ++-
 git-rebase--interactive.sh |  41 ++---
 sequencer.c| 107 +
 sequencer.h|   1 +
 4 files changed, 116 insertions(+), 39 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index e706eac710d..de3ccd9bfbc 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -14,7 +14,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
int keep_empty = 0;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S,
-   CHECK_TODO_LIST
+   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -31,6 +31,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
OPT_CMDMODE(0, "check-todo-list", ,
N_("check the todo list"), CHECK_TODO_LIST),
+   OPT_CMDMODE(0, "skip-unnecessary-picks", ,
+   N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_END()
};
 
@@ -55,5 +57,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!transform_todo_ids(0);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
+   if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
+   return !!skip_unnecessary_picks();
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 08168a0d46b..92e3ca1de3b 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -713,43 +713,6 @@ do_rest () {
done
 }
 
-# skip picking commits whose parents are unchanged
-skip_unnecessary_picks () {
-   fd=3
-   while read -r command rest
-   do
-   # fd=3 means we skip the command
-   case "$fd,$command" in
-   3,pick|3,p)
-   # pick a commit whose parent is current $onto -> skip
-   sha1=${rest%% *}
-   case "$(git rev-parse --verify --quiet "$sha1"^)" in
-   "$onto"*)
-   onto=$sha1
-   ;;
-   *)
-   fd=1
-   ;;
-   esac
-   ;;
-   3,"$comment_char"*|3,)
-   # copy comments
-   ;;
-   *)
-   fd=1
-   ;;
-   esac
-   printf '%s\n' "$command${rest:+ }$rest" >&$fd
-   done <"$todo" >"$todo.new" 3>>"$done" &&
-   mv -f "$todo".new "$todo" &&
-   case "$(peek_next_command)" in
-   squash|s|fixup|f)
-   record_in_rewritten "$onto"
-   ;;
-   esac ||
-   die "$(gettext "Could not skip unnecessary pick commands")"
-}
-
 expand_todo_ids() {
git rebase--helper --expand-sha1s
 }
@@ -1149,7 +1112,9 @@ git rebase--helper --check-todo-list || {
 
 expand_todo_ids
 
-test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
+test -d "$rewritten" || test -n "$force_rebase" ||
+onto="$(git rebase--helper --skip-unnecessary-picks)" ||
+die "Could not skip unnecessary pick commands"
 
 checkout_onto
 if test -z "$rebase_root" && test ! -d "$rewritten"
diff --git a/sequencer.c b/sequencer.c
index 4535ba9d12f..72e3ad8d145 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2616,3 +2616,110 @@ int check_todo_list(void)
 
return res;
 }
+
+/* skip picking commits whose parents are unchanged */
+int skip_unnecessary_picks(void)
+{
+   const char *todo_file = rebase_path_todo();
+   struct strbuf buf = STRBUF_INIT;
+   struct todo_list todo_list = TODO_LIST_INIT;
+   struct object_id onto_oid, *oid = _oid, *parent_oid;
+   int fd, i;
+
+   if (!read_oneliner(, rebase_path_onto(), 0))
+   return error(_("could not read 'onto'"));
+   if (get_sha1(buf.buf, onto_oid.hash)) {
+   strbuf_release();
+   return error(_("need a HEAD to fixup"));
+   }
+   strbuf_release();
+
+   fd = 

[PATCH v4 06/10] t3404: relax rebase.missingCommitsCheck tests

[Johannes Schindelin]

These tests were a bit anal about the *exact* warning/error message
printed by git rebase. But those messages are intended for the *end
user*, therefore it does not make sense to test so rigidly for the
*exact* wording.

In the following, we will reimplement the missing commits check in
the sequencer, with slightly different words.

So let's just test for the parts in the warning/error message that
we *really* care about, nothing more, nothing less.

Signed-off-by: Johannes Schindelin 
---
 t/t3404-rebase-interactive.sh | 22 --
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 33d392ba112..61113be08a4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1242,20 +1242,13 @@ test_expect_success 'rebase -i respects 
rebase.missingCommitsCheck = error' '
test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-cat >expect expect 

[PATCH v4 05/10] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Johannes Schindelin]

This is crucial to improve performance on Windows, as the speed is now
mostly dominated by the SHA-1 transformation (because it spawns a new
rev-parse process for *every* line, and spawning processes is pretty
slow from Git for Windows' MSYS2 Bash).

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   | 10 +++-
 git-rebase--interactive.sh | 27 ++
 sequencer.c| 57 ++
 sequencer.h|  2 ++
 4 files changed, 70 insertions(+), 26 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 821058d452d..9444c8d6c60 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,7 +13,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
struct replay_opts opts = REPLAY_OPTS_INIT;
int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT, MAKE_SCRIPT
+   CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_SHA1S, EXPAND_SHA1S
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
ABORT),
OPT_CMDMODE(0, "make-script", ,
N_("make rebase script"), MAKE_SCRIPT),
+   OPT_CMDMODE(0, "shorten-sha1s", ,
+   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
+   OPT_CMDMODE(0, "expand-sha1s", ,
+   N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
OPT_END()
};
 
@@ -42,5 +46,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_remove_state();
if (command == MAKE_SCRIPT && argc > 1)
return !!sequencer_make_script(keep_empty, stdout, argc, argv);
+   if (command == SHORTEN_SHA1S && argc == 1)
+   return !!transform_todo_ids(1);
+   if (command == EXPAND_SHA1S && argc == 1)
+   return !!transform_todo_ids(0);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 214af0372ba..82a1941c42c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -750,35 +750,12 @@ skip_unnecessary_picks () {
die "$(gettext "Could not skip unnecessary pick commands")"
 }
 
-transform_todo_ids () {
-   while read -r command rest
-   do
-   case "$command" in
-   "$comment_char"* | exec)
-   # Be careful for oddball commands like 'exec'
-   # that do not have a SHA-1 at the beginning of $rest.
-   ;;
-   *)
-   sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[
 ]*}) &&
-   if test "a$rest" = "a${rest#*[   ]}"
-   then
-   rest=$sha1
-   else
-   rest="$sha1 ${rest#*[]}"
-   fi
-   ;;
-   esac
-   printf '%s\n' "$command${rest:+ }$rest"
-   done <"$todo" >"$todo.new" &&
-   mv -f "$todo.new" "$todo"
-}
-
 expand_todo_ids() {
-   transform_todo_ids
+   git rebase--helper --expand-sha1s
 }
 
 collapse_todo_ids() {
-   transform_todo_ids --short
+   git rebase--helper --shorten-sha1s
 }
 
 # Rearrange the todo list that has both "pick sha1 msg" and
diff --git a/sequencer.c b/sequencer.c
index 88819a1a2a9..201d45b1677 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2437,3 +2437,60 @@ int sequencer_make_script(int keep_empty, FILE *out,
strbuf_release();
return 0;
 }
+
+
+int transform_todo_ids(int shorten_sha1s)
+{
+   const char *todo_file = rebase_path_todo();
+   struct todo_list todo_list = TODO_LIST_INIT;
+   int fd, res, i;
+   FILE *out;
+
+   strbuf_reset(_list.buf);
+   fd = open(todo_file, O_RDONLY);
+   if (fd < 0)
+   return error_errno(_("could not open '%s'"), todo_file);
+   if (strbuf_read(_list.buf, fd, 0) < 0) {
+   close(fd);
+   return error(_("could not read '%s'."), todo_file);
+   }
+   close(fd);
+
+   res = parse_insn_buffer(todo_list.buf.buf, _list);
+   if (res) {
+   todo_list_release(_list);
+   return error(_("unusable instruction sheet: '%s'"), todo_file);
+   }
+
+   out = fopen(todo_file, "w");
+   if (!out) {
+   todo_list_release(_list);
+   return error(_("unable to open '%s' for writing"), todo_file);
+   }
+   for (i = 0; i < todo_list.nr; i++) {
+   struct todo_item *item = todo_list.items + i;
+   int bol 

[PATCH v4 03/10] rebase -i: remove useless indentation

[Johannes Schindelin]

The commands used to be indented, and it is nice to look at, but when we
transform the SHA-1s, the indentation is removed. So let's do away with it.

For the moment, at least: when we will use the upcoming rebase--helper
to transform the SHA-1s, we *will* keep the indentation and can
reintroduce it. Yet, to be able to validate the rebase--helper against
the output of the current shell script version, we need to remove the
extra indentation.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 609e150d38f..c40b1fd1d2e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -155,13 +155,13 @@ reschedule_last_action () {
 append_todo_help () {
gettext "
 Commands:
- p, pick = use commit
- r, reword = use commit, but edit the commit message
- e, edit = use commit, but stop for amending
- s, squash = use commit, but meld into previous commit
- f, fixup = like \"squash\", but discard this commit's log message
- x, exec = run command (the rest of the line) using shell
- d, drop = remove commit
+p, pick = use commit
+r, reword = use commit, but edit the commit message
+e, edit = use commit, but stop for amending
+s, squash = use commit, but meld into previous commit
+f, fixup = like \"squash\", but discard this commit's log message
+x, exec = run command (the rest of the line) using shell
+d, drop = remove commit
 
 These lines can be re-ordered; they are executed from top to bottom.
 " | git stripspace --comment-lines >>"$todo"
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v4 04/10] rebase -i: do not invent onelines when expanding/collapsing SHA-1s

[Johannes Schindelin]

To avoid problems with short SHA-1s that become non-unique during the
rebase, we rewrite the todo script with short/long SHA-1s before and
after letting the user edit the script. Since SHA-1s are not intuitive
for humans, rebase -i also provides the onelines (commit message
subjects) in the script, purely for the user's convenience.

It is very possible to generate a todo script via different means than
rebase -i and then to let rebase -i run with it; In this case, these
onelines are not required.

And this is where the expand/collapse machinery has a bug: it *expects*
that oneline, and failing to find one reuses the previous SHA-1 as
"oneline".

It was most likely an oversight, and made implementation in the (quite
limiting) shell script language less convoluted. However, we are about
to reimplement performance-critical parts in C (and due to spawning a
git.exe process for every single line of the todo script, the
expansion/collapsing of the SHA-1s *is* performance-hampering on
Windows), therefore let's fix this bug to make cross-validation with the
C version of that functionality possible.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c40b1fd1d2e..214af0372ba 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -760,7 +760,12 @@ transform_todo_ids () {
;;
*)
sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[
 ]*}) &&
-   rest="$sha1 ${rest#*[]}"
+   if test "a$rest" = "a${rest#*[   ]}"
+   then
+   rest=$sha1
+   else
+   rest="$sha1 ${rest#*[]}"
+   fi
;;
esac
printf '%s\n' "$command${rest:+ }$rest"
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v4 01/10] t3415: verify that an empty instructionFormat is handled as before

[Johannes Schindelin]

An upcoming patch will move the todo list generation into the
rebase--helper. An early version of that patch regressed on an empty
rebase.instructionFormat value (the shell version could not discern
between an empty one and a non-existing one, but the C version used the
empty one as if that was intended to skip the oneline from the `pick
` lines).

Let's verify that this still works as before.

Signed-off-by: Johannes Schindelin 
---
 t/t3415-rebase-autosquash.sh | 12 
 1 file changed, 12 insertions(+)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 48346f1cc0c..6c37ebdff87 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -271,6 +271,18 @@ test_expect_success 'autosquash with custom inst format' '
test 2 = $(git cat-file commit HEAD^ | grep squash | wc -l)
 '
 
+test_expect_success 'autosquash with empty custom instructionFormat' '
+   git reset --hard base &&
+   test_commit empty-instructionFormat-test &&
+   (
+   set_cat_todo_editor &&
+   test_must_fail git -c rebase.instructionFormat= \
+   rebase --autosquash  --force -i HEAD^ >actual &&
+   git log -1 --format="pick %h %s" >expect &&
+   test_cmp expect actual
+   )
+'
+
 set_backup_editor () {
write_script backup-editor.sh <<-\EOF
cp "$1" .git/backup-"$(basename "$1")"
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

The first step of an interactive rebase is to generate the so-called "todo
script", to be stored in the state directory as "git-rebase-todo" and to
be edited by the user.

Originally, we adjusted the output of `git log ` using a simple
sed script. Over the course of the years, the code became more
complicated. We now use shell scripting to edit the output of `git log`
conditionally, depending whether to keep "empty" commits (i.e. commits
that do not change any files).

On platforms where shell scripting is not native, this can be a serious
drag. And it opens the door for incompatibilities between platforms when
it comes to shell scripting or to Unix-y commands.

Let's just re-implement the todo script generation in plain C, using the
revision machinery directly.

This is substantially faster, improving the speed relative to the
shell script version of the interactive rebase from 2x to 3x on Windows.

Note that the rearrange_squash() function in git-rebase--interactive
relied on the fact that we set the "format" variable to the config setting
rebase.instructionFormat. Relying on a side effect like this is no good,
hence we explicitly perform that assignment (possibly again) in
rearrange_squash().

Signed-off-by: Johannes Schindelin 
---
 builtin/rebase--helper.c   |  8 +++-
 git-rebase--interactive.sh | 44 +
 sequencer.c| 49 ++
 sequencer.h|  3 +++
 4 files changed, 82 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index ca1ebb2fa18..821058d452d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -11,15 +11,19 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
+   int keep_empty = 0;
enum {
-   CONTINUE = 1, ABORT
+   CONTINUE = 1, ABORT, MAKE_SCRIPT
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
+   OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
commits")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
ABORT),
+   OPT_CMDMODE(0, "make-script", ,
+   N_("make rebase script"), MAKE_SCRIPT),
OPT_END()
};
 
@@ -36,5 +40,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
+   if (command == MAKE_SCRIPT && argc > 1)
+   return !!sequencer_make_script(keep_empty, stdout, argc, argv);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2c9c0165b5a..609e150d38f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -785,6 +785,7 @@ collapse_todo_ids() {
 # each log message will be re-retrieved in order to normalize the
 # autosquash arrangement
 rearrange_squash () {
+   format=$(git config --get rebase.instructionFormat)
# extract fixup!/squash! lines and resolve any referenced sha1's
while read -r pick sha1 message
do
@@ -1210,26 +1211,27 @@ else
revisions=$onto...$orig_head
shortrevisions=$shorthead
 fi
-format=$(git config --get rebase.instructionFormat)
-# the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to 
parse
-git rev-list $merges_option --format="%m%H ${format:-%s}" \
-   --reverse --left-right --topo-order \
-   $revisions ${restrict_revision+^$restrict_revision} | \
-   sed -n "s/^>//p" |
-while read -r sha1 rest
-do
-
-   if test -z "$keep_empty" && is_empty_commit $sha1 && ! is_merge_commit 
$sha1
-   then
-   comment_out="$comment_char "
-   else
-   comment_out=
-   fi
+if test t != "$preserve_merges"
+then
+   git rebase--helper --make-script ${keep_empty:+--keep-empty} \
+   $revisions ${restrict_revision+^$restrict_revision} >"$todo"
+else
+   format=$(git config --get rebase.instructionFormat)
+   # the 'rev-list .. | sed' requires %m to parse; the instruction 
requires %H to parse
+   git rev-list $merges_option --format="%m%H ${format:-%s}" \
+   --reverse --left-right --topo-order \
+   $revisions ${restrict_revision+^$restrict_revision} | \
+   sed -n "s/^>//p" |
+   while read -r sha1 rest
+   do
+
+   if test -z "$keep_empty" && is_empty_commit $sha1 && ! 
is_merge_commit $sha1
+   then
+ 

[PATCH v4 00/10] The final building block for a faster rebase -i

[Johannes Schindelin]

This patch series reimplements the expensive pre- and post-processing of
the todo script in C.

And it concludes the work I did to accelerate rebase -i.

Changes since v3:

- removed the no-longer-used transform_todo_ids shell function

- simplified transform_todo_ids()'s command parsing

- fixed two commits in check_todo_list(), renamed the unclear
  `raise_error` variable to `advise_to_edit_todo`, build the message
  about missing commits directly (without the detour to building a
  commit_list) and instead of assigning an unused pointer to commit->util
  the code now uses (void *)1.

- return early from check_todo_list() when parsing failed, even if the
  check level is something else than CHECK_IGNORE

- the todo list is generated is again generated in the same way as
  before when rebase.instructionFormat is empty: it was interpreted as
  if it had not been set

- added a test for empty rebase.instructionFormat settings


Johannes Schindelin (10):
  t3415: verify that an empty instructionFormat is handled as before
  rebase -i: generate the script via rebase--helper
  rebase -i: remove useless indentation
  rebase -i: do not invent onelines when expanding/collapsing SHA-1s
  rebase -i: also expand/collapse the SHA-1s via the rebase--helper
  t3404: relax rebase.missingCommitsCheck tests
  rebase -i: check for missing commits in the rebase--helper
  rebase -i: skip unnecessary picks using the rebase--helper
  t3415: test fixup with wrapped oneline
  rebase -i: rearrange fixup/squash lines using the rebase--helper

 Documentation/git-rebase.txt  |  16 +-
 builtin/rebase--helper.c  |  29 ++-
 git-rebase--interactive.sh| 373 -
 sequencer.c   | 530 ++
 sequencer.h   |   8 +
 t/t3404-rebase-interactive.sh |  22 +-
 t/t3415-rebase-autosquash.sh  |  28 ++-
 7 files changed, 646 insertions(+), 360 deletions(-)


base-commit: 027a3b943b444a3e3a76f9a89803fc10245b858f
Based-On: rebase--helper at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git rebase--helper
Published-As: https://github.com/dscho/git/releases/tag/rebase-i-extra-v4
Fetch-It-Via: git fetch https://github.com/dscho/git rebase-i-extra-v4

Interdiff vs v3:

 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
 index d39fe4f5fb7..84c6e62518f 100644
 --- a/git-rebase--interactive.sh
 +++ b/git-rebase--interactive.sh
 @@ -713,29 +713,6 @@ do_rest () {
done
  }
  
 -transform_todo_ids () {
 -  while read -r command rest
 -  do
 -  case "$command" in
 -  "$comment_char"* | exec)
 -  # Be careful for oddball commands like 'exec'
 -  # that do not have a SHA-1 at the beginning of $rest.
 -  ;;
 -  *)
 -  sha1=$(git rev-parse --verify --quiet "$@" ${rest%%[
 ]*}) &&
 -  if test "a$rest" = "a${rest#*[   ]}"
 -  then
 -  rest=$sha1
 -  else
 -  rest="$sha1 ${rest#*[]}"
 -  fi
 -  ;;
 -  esac
 -  printf '%s\n' "$command${rest:+ }$rest"
 -  done <"$todo" >"$todo.new" &&
 -  mv -f "$todo.new" "$todo"
 -}
 -
  expand_todo_ids() {
git rebase--helper --expand-sha1s
  }
 diff --git a/sequencer.c b/sequencer.c
 index 84f8e366761..63a588f0916 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -2393,7 +2393,7 @@ void append_signoff(struct strbuf *msgbuf, int 
ignore_footer, unsigned flag)
  int sequencer_make_script(int keep_empty, FILE *out,
int argc, const char **argv)
  {
 -  char *format = xstrdup("%s");
 +  char *format = NULL;
struct pretty_print_context pp = {0};
struct strbuf buf = STRBUF_INIT;
struct rev_info revs;
 @@ -2411,6 +2411,10 @@ int sequencer_make_script(int keep_empty, FILE *out,
  
revs.pretty_given = 1;
git_config_get_string("rebase.instructionFormat", );
 +  if (!format || !*format) {
 +  free(format);
 +  format = xstrdup("%s");
 +  }
get_commit_format(format, );
free(format);
pp.fmt = revs.commit_format;
 @@ -2475,18 +2479,16 @@ int transform_todo_ids(int shorten_sha1s)
if (item->command >= TODO_EXEC && item->command != TODO_DROP)
fwrite(p, eol - bol, 1, out);
else {
 -  int eoc = strcspn(p, " \t");
const char *sha1 = shorten_sha1s ?
short_commit_name(item->commit) :
oid_to_hex(>commit->object.oid);
 +  int len;
  
 -  if (!eoc) {
 -  p += strspn(p, " \t");
 -  eoc = strcspn(p, " \t");
 - 

Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity

[Johannes Schindelin]

Hi Stefan,

On Fri, 28 Apr 2017, Stefan Beller wrote:

> On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin
>  wrote:
> 
> > I still have to find the time to figure out one more detail: how to
> > download and extract the Coverity tool (the .zip archive has a
> > variable name for the top-level directory), and doing that only every
> > once in a while, say, only when there is no previously unpacked tool,
> > or it is already 4 weeks old.
> 
> That is an interesting problem, which I ignored as the older versions of
> their tools still works once they release new versions. So I just
> manually check every once in a while if they have new versions out
> there.
> 
> So if you find a nice solution to that problem, let me know, please.

I think I have a working idea (jotting it down in the editor, untested):

init_or_update_coverity_tool () {
# check once per week whether there is a new version
coverity_tool=.git/coverity-tool/
test ! -d $coverity_tool ||
test $(($(date +%s)-$(stat -c %Y $coverity_tool))) -gt
$((7*24*60*60)) ||
return

curl --form "token=$(COVERITY.TOKEN)" \
--form "project=git-for-windows" \
--time-cond .git/coverity_tool.zip \
-o .git/coverity_tool.zip.new \
https://scan.coverity.com/download/win64 &&
test -f .git/coverity_tool.zip.new || {
# Try again in a week
touch $coverity_tool
return
}

mv -f .git/coverity_tool.zip.new .git/coverity_tool.zip ||
die "Could not overwrite coverity_tool.zip"

mkdir $coverity_tool.new &&
(cd $coverity_tool.new &&
 unzip ../coverity_tool.zip) ||
die "Could not unpack coverity_tool.zip"

rm -rf $coverity_tool &&
mv $coverity_tool.new $coverity_tool ||
die "Could not switch to new Coverity tool version"
}

init_or_update_coverity_tool
PATH=$(echo $coverity_tool/*/bin):$PATH

I guess I will start from that snippet once I have time to work on that
Coverity automation.

BTW I stumbled over an interesting tidbit today: if you define FLEX_ARRAY
outside of git-compat-util.h, it will not be overridden by Git. That is,
if you want to use 64kB flex arrays by default, you can call

make CPPFLAGS=-DFLEX_ARRAY=65536

No need to patch the source code.

Ciao,
Dscho

Re: [PATCH] t7400: add BSLASHPSPEC prerequisite to 'add with \\ in path'

[Johannes Sixt]

Am 28.04.2017 um 05:09 schrieb Junio C Hamano:

Ramsay Jones  writes:


Commit cf9e55f494 ("submodule: prevent backslash expantion in submodule
names", 07-04-2017) added a test which creates a git repository with
some backslash characters in the name. This test cannot work on windows,
since the backslash is used as the directory separator. In order to
suppress this test on cygwin, MinGW and Git for Windows, we add the
BSLASHPSPEC prerequisite. (see also commits 6fd1106aa4 and c51c0da222).


First, let me say that meaning of BSLASHPSPEC was "keeps backslaches in 
pathspec arguments" originally, but it apparently changed meaning since 
then.


The prerequisite was introduced in 6fd1106aa4 because the MinGW port 
rewrites backslashes in command arguments that undergo the '\'->"/" 
transformation introduced for prefix_filename() in 25fe217b86. It 
destroys the backslashes that could otherwise be used to escape globbing 
characters. t3700 does just that.


Since Cygwin does not do this rewriting, the original CYGWIN section in 
test-lib.sh had the prerequisite, just like the default (POSIX). But it 
was removed by 5b5d53cbe5 (t4135-*.sh: Skip the "backslash" tests on 
cygwin), and that is where BSLASHPSPEC changed meaning. That commit even 
carries my Acked-by (!), even though I can't recall giving it and I 
don't find it in the conversation about the patch:


https://public-inbox.org/git/4d07b977.9010...@ramsay1.demon.co.uk/


I built v2.13.0-rc1 and ran the test-suite on cygwin this evening and
had an additional failure, over and above the failures reported for
v2.13.0-rc0, namely t7400.20. This patch elides that test for cygwin
(and MinGW and GfW, so it would be good to hear success reports from
both Johannes).


t7400.20 does not fail for the MinGW port because the test case only 
operates on the file system, but never checks whether an entry 
'sub\with\backslash' is present in the index.



@@ -273,7 +273,7 @@ test_expect_success 'submodule add with ./, /.. and // in 
path' '
test_cmp empty untracked
 '

-test_expect_success 'submodule add with \\ in path' '
+test_expect_success BSLASHPSPEC 'submodule add with \\ in path' '
test_when_finished "rm -rf parent sub\\with\\backslash" &&

# Initialize a repo with a backslash in its name




I assume the test fails right at 'git init' under Cygwin?

BSLASHPSPEC (with its *new* meaning) would be the right prerequisite if 
the check for the index entry were added. Until then, !CYGWIN is more 
appropriate.


-- Hannes

Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Philip,

On Fri, 28 Apr 2017, Phillip Wood wrote:

> On 26/04/17 12:59, Johannes Schindelin wrote:
>
> > The first step of an interactive rebase is to generate the so-called
> > "todo script", to be stored in the state directory as
> > "git-rebase-todo" and to be edited by the user.
> > 
> > Originally, we adjusted the output of `git log ` using a
> > simple sed script. Over the course of the years, the code became more
> > complicated. We now use shell scripting to edit the output of `git
> > log` conditionally, depending whether to keep "empty" commits (i.e.
> > commits that do not change any files).
> > 
> > On platforms where shell scripting is not native, this can be a
> > serious drag. And it opens the door for incompatibilities between
> > platforms when it comes to shell scripting or to Unix-y commands.
> > 
> > Let's just re-implement the todo script generation in plain C, using
> > the revision machinery directly.
> > 
> > This is substantially faster, improving the speed relative to the
> > shell script version of the interactive rebase from 2x to 3x on
> > Windows.
> 
> This changes the behaviour of git -c rebase.instructionFormat= rebase -i
> The shell version treats the rebase.instructionFormat being unset or set
> to the empty string as equivalent. This version generates a todo list
> with lines like 'pick ' rather than 'pick 
> '
> 
> I only picked this up because I have a script that does 'git -c
> rebase.instructionFormat= rebase -i' with a custom sequence editor. I
> can easily add '%s' in the appropriate place but I thought I'd point it
> out in case other people are affected by the change.

While I would argue that the C version is more correct, it would be
backwards-incompatible.

So I changed it.

BTW in the future you could help me a *lot* by providing a patch that adds
a test case to our test suite that not only demonstrates what exactly goes
wrong, but also will help prevent future regressions.

Ciao,
Johannes

Re: git v2.13.0-rc0 test failures on cygwin

[Devin Lehmacher]

> > Test Summary Report
> > ---
> > t0301-credential-cache.sh(Wstat: 256 Tests: 29 
> > Failed: 6)
> >   Failed tests:  12, 24-28
> >   Non-zero exit status: 1
>
> Confirmed I'm seeing this on v2.13.0-rc1, and this passed in v2.12.2.
> `git bisect` tells me this failure was introduced by commit
> v2.12.0-267-g612c49e94, added by Devin Lehmacher (added to the CC
> list).

Can someone with cygwin check that `test -S` works on cygwin?

The only that tests that failed are those that use `test -S` to check
for the existence of a socket.

I'll also take a look at verbose test output (maybe with a trace too)
for t0301 if someone sends me that.

-Devin

Re: [PATCH v3 0/5] clone: --no-tags option

[Stefan Beller]

On Fri, Apr 28, 2017 at 12:11 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Thu, Apr 27, 2017 at 1:12 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>> This is an expansion of the previously solo 02/05 "clone: add a
>> --no-tags option to clone without tags" patch (see
>> <20170418191553.15464-1-ava...@gmail.com>).
>>
>> This addresses the comments by Junio & Jonathan Nieder on v2 (thanks a
>> lot), and in addition implements a --no-tags-submodules option. That
>> code was implemented by Brandon & sent to me privately after I'd
>> failed to come up with it, but I added tests, a commit message & bash
>> completion to it.
>>
>> The WIP 5/5 patch implements a submodule.NAME.tags config facility for
>> the option, but is broken currently & floats along in this submission
>> as an RFC patch. AFAICT it *should* work and it goes through all the
>> motions the similar existing *.shallow config does, but for some
>> reason the tags=false option isn't picked up & propagated in a freshly
>> cloned submodule.
>>
>> I'm probably missing something trivial, but I can't see what it is,
>> I'm hoping thath either Stefan or Brandon will see what that is.
>
> Junio, can you please just take patch 1-3 in this series, i.e.:
>
> DROP:
>
>> Brandon Williams (1):
>>   clone: add a --no-tags-submodules to pass --no-tags to submodules
>> [...]
>>   WIP clone: add a --[no-]recommend-tags & submodule.NAME.tags config
>
> KEEP:
>
>> Ævar Arnfjörð Bjarmason (4):
>>   tests: change "cd ... && git fetch" to "cd &&\n\tgit fetch"
>>   clone: add a --no-tags option to clone without tags
>>   tests: rename a test having to do with shallow submodules
>
> I think a fair summary of the discussion so far is that the submodule
> interaction I copy/pasted from --shallow-submodules isn't how we want
> to do things at all, I defer to Stefan & Brandon on that.

ok. In that case we'd want to discuss what we actually want with no-tags
in submodules.

>
> The only other objection to the patches marked as KEEP is from Stefan
> saying it should be --tags on by default not --no-tags off by default.
> As noted in 
> 
> I don't disagree, but a lot of flags in git now use that pattern, and
> this change is consistent with those. Makes sense to discuss changing
> that as another series.

Ok. I assumed with that next series on the radar, we'd not want to intentionally
add more of the no-OPTIONs as that would produce more work for that series.

Re: [PATCH v3 0/5] clone: --no-tags option

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 27, 2017 at 1:12 AM, Ævar Arnfjörð Bjarmason
 wrote:
> This is an expansion of the previously solo 02/05 "clone: add a
> --no-tags option to clone without tags" patch (see
> <20170418191553.15464-1-ava...@gmail.com>).
>
> This addresses the comments by Junio & Jonathan Nieder on v2 (thanks a
> lot), and in addition implements a --no-tags-submodules option. That
> code was implemented by Brandon & sent to me privately after I'd
> failed to come up with it, but I added tests, a commit message & bash
> completion to it.
>
> The WIP 5/5 patch implements a submodule.NAME.tags config facility for
> the option, but is broken currently & floats along in this submission
> as an RFC patch. AFAICT it *should* work and it goes through all the
> motions the similar existing *.shallow config does, but for some
> reason the tags=false option isn't picked up & propagated in a freshly
> cloned submodule.
>
> I'm probably missing something trivial, but I can't see what it is,
> I'm hoping thath either Stefan or Brandon will see what that is.

Junio, can you please just take patch 1-3 in this series, i.e.:

DROP:

> Brandon Williams (1):
>   clone: add a --no-tags-submodules to pass --no-tags to submodules
> [...]
>   WIP clone: add a --[no-]recommend-tags & submodule.NAME.tags config

KEEP:

> Ævar Arnfjörð Bjarmason (4):
>   tests: change "cd ... && git fetch" to "cd &&\n\tgit fetch"
>   clone: add a --no-tags option to clone without tags
>   tests: rename a test having to do with shallow submodules

I think a fair summary of the discussion so far is that the submodule
interaction I copy/pasted from --shallow-submodules isn't how we want
to do things at all, I defer to Stefan & Brandon on that.

The only other objection to the patches marked as KEEP is from Stefan
saying it should be --tags on by default not --no-tags off by default.
As noted in 
I don't disagree, but a lot of flags in git now use that pattern, and
this change is consistent with those. Makes sense to discuss changing
that as another series.

Re: [PATCH 00/26] Address a couple of issues identified by Coverity

[Stefan Beller]

Hi Johannes,

thanks for the reply.

On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin
 wrote:

> I still have to find the time to figure out one more detail: how to
> download and extract the Coverity tool (the .zip archive has a variable
> name for the top-level directory), and doing that only every once
> in a while, say, only when there is no previously unpacked tool, or it is
> already 4 weeks old.

That is an interesting problem, which I ignored as the older versions of their
tools still works once they release new versions. So I just manually check
every once in a while if they have new versions out there.

So if you find a nice solution to that problem, let me know, please.

Thanks,
Stefan

Re: [PATCH v3 3/5] tests: rename a test having to do with shallow submodules

[Stefan Beller]

On Fri, Apr 28, 2017 at 1:59 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Fri, Apr 28, 2017 at 12:13 AM, Brandon Williams  wrote:
>> On 04/27, Stefan Beller wrote:
>>> On Wed, Apr 26, 2017 at 4:12 PM, Ævar Arnfjörð Bjarmason
>>>  wrote:
>>> > Rename the t5614-clone-submodules.sh test to
>>> > t5614-clone-submodules-shallow.sh. It's not a general test of
>>> > submodules, but of shallow cloning in relation to submodules. Move it
>>> > to create another similar t56*-clone-submodules-*.sh test.
>>> >
>>> > Signed-off-by: Ævar Arnfjörð Bjarmason 
>>> > ---
>>> >  t/{t5614-clone-submodules.sh => t5614-clone-submodules-shallow.sh} | 0
>>> >  1 file changed, 0 insertions(+), 0 deletions(-)
>>> >  rename t/{t5614-clone-submodules.sh => 
>>> > t5614-clone-submodules-shallow.sh} (100%)
>>> >
>>> > diff --git a/t/t5614-clone-submodules.sh 
>>> > b/t/t5614-clone-submodules-shallow.sh
>>> > similarity index 100%
>>> > rename from t/t5614-clone-submodules.sh
>>> > rename to t/t5614-clone-submodules-shallow.sh
>>>
>>> Thanks for formatting the patches with rename detection. :)
>>> The rename looks good.
>>
>> Do you have to turn that on or is that on by default?
>
> Looks like it just uses the diff.renames setting which I don't tweak,
> I didn't do anything special, but maybe it picked up some part of my
> .gitconfig that doesn't look like it has anything to do with
> renames...

After reading the man page of format-patch, I do not think you'd need
to configure anything special as the default settings easily pickup
a rename with 100% similarity.  It is only hairy when changes are in
there as well, then you may want to play around with -B/-M.

Thanks,
Stefan

Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

[Stefan Beller]

On Thu, Apr 27, 2017 at 1:50 PM, Marc Branchaud  wrote:
> So here's my attempt at fixing this.
>
> The thing I was missing is that init_revisions() calls diff_setup(), which
> sets the xdl options.  It's therefore necessary to have the
> diff_indent_heuristic flag set before calling init_revisions().
>
> A naive way to get the indentHeuristic config option respected in the
> diff-* plumbing commands is to make them use the git_diff_heuristic_config()
> callback right at the start of their main cmd functions.
>
> But I did not like that for two reasons:
>
> * It would make these commands invoke git_config() twice.
>
> * It doesn't avoid the problem if/when someone creates a new diff-something
>   plumbing command, and forgets to set the diff_indent_heuristic flag before
>   calling init_revisions().
>
> So instead I chose to make the indentHeuristic option part of diff's basic
> configuration, and in each of the diff plumbing commands I moved the call to
> git_config() before the call to init_revisions().
>
> This still doesn't really future-proof things for possible new diff plumbing
> commands, because someone could still invoke init_revisions() before setting
> up diff's basic configuration.  But I don't see an obvious way of ensuring
> that the diff_indent_heuristic flag is respected regardless of when
> diff_setup() is invoked.
>

The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got, was positive.
This could be explained by having the feature as an experimental feature
and users who would be broken by it, did not test it yet or did not speak up.

So I'd propose to turn it on by default and anyone negatively impacted by that
could then use the config to turn it off for themselves (including plumbing).

Something like this, maybe?

---8<---
>From 58d9a1ef135eff9f85b165986e4bc13479914f8e Mon Sep 17 00:00:00 2001
From: Stefan Beller 
Date: Thu, 27 Apr 2017 14:26:59 -0700
Subject: [PATCH] diff.c: enable indent heuristic by default

The feature was included in v2.11 (released 2016-11-29) and we got no
negative feedback. Quite the opposite, all feedback we got was positive.

Turn it on by default. users who dislike the feature can turn it off
using by setting diff.compactionHeuristic (which includes plumbing
commands, see prior patches)

Signed-off-by: Stefan Beller 
---
diff --git a/diff.c b/diff.c
index 11eef1c85d..7f301c1b62 100644
--- a/diff.c
+++ b/diff.c
@@ -27,7 +27,7 @@
 #endif

 static int diff_detect_rename_default;
-static int diff_indent_heuristic; /* experimental */
+static int diff_indent_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;

Re: [PATCH 1/5] add SWAP macro

[René Scharfe]

Am 24.04.2017 um 13:29 schrieb Jeff King:

On Sat, Jan 28, 2017 at 10:38:21PM +0100, René Scharfe wrote:


diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092b..66cd466eea 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -527,6 +527,16 @@ static inline int ends_with(const char *str, const char 
*suffix)
return strip_suffix(str, suffix, );
  }
  
+#define SWAP(a, b) do {		\

+   void *_swap_a_ptr = &(a);   \
+   void *_swap_b_ptr = &(b);   \
+   unsigned char _swap_buffer[sizeof(a)];  \
+   memcpy(_swap_buffer, _swap_a_ptr, sizeof(a));   \
+   memcpy(_swap_a_ptr, _swap_b_ptr, sizeof(a) +\
+  BUILD_ASSERT_OR_ZERO(sizeof(a) == sizeof(b)));   \
+   memcpy(_swap_b_ptr, _swap_buffer, sizeof(a));   \
+} while (0)


What should:

   SWAP(foo[i], foo[j]);

do when i == j? With this code, it ends up calling

   memcpy([i], [j], ...);

which can cause valgrind to complain about overlapping memory. I suspect
in practice that noop copies are better off than partial overlaps, but I
think it does still violate the standard.

Is it worth comparing the pointers and bailing early?


Hmm, so swapping a value with itself can be a useful thing to do?
Otherwise an assert would be more appropriate.

Swapping with *partial* overlap sounds tricky, or even evil.  If
we want to support that for some reason we'd have to use memmove
in the middle.  But that would still corrupt at least one of the
objects, wouldn't it?


A related question is whether the caller should ever be asking to swap
something with itself. This particular case[1] comes from
prio_queue_reverse(). I suspect its "<=" could become a "<", but I
haven't thought it through carefully.


The line in question is this one:

for (i = 0; i <= (j = (queue->nr - 1) - i); i++)

Assignment in the middle?  Hmm.  Why not do it like this?

for (i = 0, j = queue->nr - 1; i < j; i++, j--)

Looks less complicated to me.

René

Re: push fails with return code 22

[Andrew Watson]

You are the best Peff.

It was indeed the hierarchy. So just had to change document root.

Thanks a bunch.

On Fri, Apr 28, 2017 at 11:34 AM, Jeff King  wrote:
> On Fri, Apr 28, 2017 at 11:28:14AM -0400, Andrew Watson wrote:
>
>> $ GIT_CURL_VERBOSE=1 git clone http://git.site.domain.com/foo/gitrepo.git
>> Cloning into 'gitrepo'...
>> * Couldn't find host git.site.domain.com in the _netrc file; using defaults
>> * timeout on name lookup is not supported
>> *   Trying 192.168.16.138...
>> * TCP_NODELAY set
>> * Connected to git.site.domain.com (192.168.16.138) port 80 (#0)
>> > GET /foo/gitrepo.git/info/refs?service=git-upload-pack HTTP/1.1
>> Host: git.site.domain.com
>> User-Agent: git/2.12.2.windows.2
>> Accept: */*
>> Accept-Encoding: gzip
>> Pragma: no-cache
>>
>> < HTTP/1.1 200 OK
>> < Date: Fri, 28 Apr 2017 15:25:02 GMT
>> < Server: Apache/2.4.6 (CentOS) PHP/5.4.16
>> < Last-Modified: Tue, 25 Apr 2017 18:11:35 GMT
>> < ETag: "0-54e01a77ac500"
>> < Accept-Ranges: bytes
>> < Content-Length: 0
>> < Content-Type: text/plain; charset=UTF-8
>
> OK, so this is not doing smart-http either (the content-type should be
> application/x-git-upload-pack-advertisement when the CGI generates it).
>
> Looking at your config again, I see:
>
>   ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/
>
> but your example output shows:
>
>   GET /gitrepo.git/info/refs?service=git-receive-pack
>
> I.e., not in the /git/ hierarchy. So that might explain why the CGI is
> not kicking in.
>
> -Peff

Re: git with ssh won't pull submodule

[Chris Packham]

Hi Erik,

On Fri, Apr 28, 2017 at 11:25 AM, Erik Haller  wrote:
> Getting the following error for a submodule when using git/ssh:
>
> $ git clone --recursive ssh://incense:/home/erik/git/nacl.git
> Cloning into 'nacl'...
> remote: Counting objects: 32, done.
> remote: Compressing objects: 100% (25/25), done.
> remote: Total 32 (delta 5), reused 0 (delta 0)
> Receiving objects: 100% (32/32), 16.50 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (5/5), done.
> Submodule 'vendor/golang.org/x/crypto'
> (file:///home/erik/git/github.com/golang/crypto.git) registered for
> path 'vendor/golang.org/x/crypto'

This is the problem. The .gitmodules entry in nacl.git uses an
absolute path (or URI in this case) for the submodule. Git does
exactly what it should and tries to clone it.

The solution to this is to use a relative path[1] in .gitmodules
(either edit it by hand or do git rm & git submodule add). Note that
by using a relative path it assumes that the parent and submodule
repositories are hosted in the same location (which may or may not be
true for your use-case).

--
[1] - see the 3rd paragraph for the add command in
https://git-scm.com/docs/git-submodule

> Cloning into '/home/erik/go/src/nacl/vendor/golang.org/x/crypto'...
> fatal: '/home/erik/git/github.com/golang/crypto.git' does not appear
> to be a git repository
> fatal: Could not read from remote repository.
>
> Please make sure you have the correct access rights
> and the repository exists.
> fatal: clone of 'file:///home/erik/git/github.com/golang/crypto.git'
> into submodule path
> '/home/erik/go/src/nacl/vendor/golang.org/x/crypto' failed
> Failed to clone 'vendor/golang.org/x/crypto'. Retry scheduled
> Cloning into '/home/erik/go/src/nacl/vendor/golang.org/x/crypto'...
> fatal: '/home/erik/git/github.com/golang/crypto.git' does not appear
> to be a git repository
> fatal: Could not read from remote repository.
>
> Please make sure you have the correct access rights
> and the repository exists.
> fatal: clone of 'file:///home/erik/git/github.com/golang/crypto.git'
> into submodule path
> '/home/erik/go/src/nacl/vendor/golang.org/x/crypto' failed
> Failed to clone 'vendor/golang.org/x/crypto' a second time, aborting
>
>
> The git clone --recursive file:///home/erik/git/nacl.git works fine
> and pulls the submodule "crypto.git". Any ideas?
>
> - The crypto.git is a valid repo.
> - I have the correct permissions.
> - The crypto.git repo is a git --mirror repo.
>
>
> git version: 2.11.0
> system: linux debian/testing

Re: push fails with return code 22

[Jeff King]

On Fri, Apr 28, 2017 at 11:28:14AM -0400, Andrew Watson wrote:

> $ GIT_CURL_VERBOSE=1 git clone http://git.site.domain.com/foo/gitrepo.git
> Cloning into 'gitrepo'...
> * Couldn't find host git.site.domain.com in the _netrc file; using defaults
> * timeout on name lookup is not supported
> *   Trying 192.168.16.138...
> * TCP_NODELAY set
> * Connected to git.site.domain.com (192.168.16.138) port 80 (#0)
> > GET /foo/gitrepo.git/info/refs?service=git-upload-pack HTTP/1.1
> Host: git.site.domain.com
> User-Agent: git/2.12.2.windows.2
> Accept: */*
> Accept-Encoding: gzip
> Pragma: no-cache
> 
> < HTTP/1.1 200 OK
> < Date: Fri, 28 Apr 2017 15:25:02 GMT
> < Server: Apache/2.4.6 (CentOS) PHP/5.4.16
> < Last-Modified: Tue, 25 Apr 2017 18:11:35 GMT
> < ETag: "0-54e01a77ac500"
> < Accept-Ranges: bytes
> < Content-Length: 0
> < Content-Type: text/plain; charset=UTF-8

OK, so this is not doing smart-http either (the content-type should be
application/x-git-upload-pack-advertisement when the CGI generates it).

Looking at your config again, I see:

  ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/

but your example output shows:

  GET /gitrepo.git/info/refs?service=git-receive-pack

I.e., not in the /git/ hierarchy. So that might explain why the CGI is
not kicking in.

-Peff

Re: push fails with return code 22

[Andrew Watson]

Hi Peff,

Thanks for the help.

$ GIT_CURL_VERBOSE=1 git clone http://git.site.domain.com/foo/gitrepo.git
Cloning into 'gitrepo'...
* Couldn't find host git.site.domain.com in the _netrc file; using defaults
* timeout on name lookup is not supported
*   Trying 192.168.16.138...
* TCP_NODELAY set
* Connected to git.site.domain.com (192.168.16.138) port 80 (#0)
> GET /foo/gitrepo.git/info/refs?service=git-upload-pack HTTP/1.1
Host: git.site.domain.com
User-Agent: git/2.12.2.windows.2
Accept: */*
Accept-Encoding: gzip
Pragma: no-cache

< HTTP/1.1 200 OK
< Date: Fri, 28 Apr 2017 15:25:02 GMT
< Server: Apache/2.4.6 (CentOS) PHP/5.4.16
< Last-Modified: Tue, 25 Apr 2017 18:11:35 GMT
< ETag: "0-54e01a77ac500"
< Accept-Ranges: bytes
< Content-Length: 0
< Content-Type: text/plain; charset=UTF-8
<
* Connection #0 to host git.site.domain.com left intact
* Couldn't find host git.site.domain.com in the _netrc file; using defaults
* Found bundle for host git.site.domain.com: 0x1cc9fc0 [can pipeline]
* Re-using existing connection! (#0) with host git.site.domain.com
* Connected to git.site.domain.com (192.168.16.138) port 80 (#0)
> GET /foo/gitrepo.git/HEAD HTTP/1.1
Host: git.site.domain.com
User-Agent: git/2.12.2.windows.2
Accept: */*
Accept-Encoding: gzip
Pragma: no-cache

< HTTP/1.1 200 OK
< Date: Fri, 28 Apr 2017 15:25:02 GMT
< Server: Apache/2.4.6 (CentOS) PHP/5.4.16
< Last-Modified: Mon, 24 Apr 2017 20:51:42 GMT
< ETag: "17-54defc6469818"
< Accept-Ranges: bytes
< Content-Length: 23
<
* Connection #0 to host git.site.domain.com left intact
warning: You appear to have cloned an empty repository.

Content length is again 0.

On Fri, Apr 28, 2017 at 11:20 AM, Jeff King  wrote:
> On Fri, Apr 28, 2017 at 11:09:55AM -0400, Andrew Watson wrote:
>
>> Thanks for pointing me to git help http-backend. I confirmed the
>> modules are loaded and the CGI environment variables. I've added
>> "AcceptPathInfo On" to my httpd.conf just to be safe.
>>
>> I'm not sure what /info/refs is supposed to look like, but it is
>> empty. Could that be the issue?
>
> No, that shouldn't matter. The on-disk file is used only for dumb-http
> requests. In a working smart-http system, the info/refs request should
> go to the CGI, which will generate the ref advertisement dynamically.
>
>> Do you see anything in my apache configuration that looks wrong?
>
> It looks reasonable to me, but I'm far from an expert on Apache config.
>
> When you clone, is it using smart-http there? Try using GIT_CURL_VERBOSE
> to see what the response is to the initial /info/refs fetch when you
> clone.
>
> -Peff

Re: push fails with return code 22

[Jeff King]

On Fri, Apr 28, 2017 at 11:09:55AM -0400, Andrew Watson wrote:

> Thanks for pointing me to git help http-backend. I confirmed the
> modules are loaded and the CGI environment variables. I've added
> "AcceptPathInfo On" to my httpd.conf just to be safe.
> 
> I'm not sure what /info/refs is supposed to look like, but it is
> empty. Could that be the issue?

No, that shouldn't matter. The on-disk file is used only for dumb-http
requests. In a working smart-http system, the info/refs request should
go to the CGI, which will generate the ref advertisement dynamically.

> Do you see anything in my apache configuration that looks wrong?

It looks reasonable to me, but I'm far from an expert on Apache config.

When you clone, is it using smart-http there? Try using GIT_CURL_VERBOSE
to see what the response is to the initial /info/refs fetch when you
clone.

-Peff

Re: [PATCH v3 4/9] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Johannes Schindelin]

Hi Junio,

On Thu, 27 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> > +out = fopen(todo_file, "w");
> >> 
> >> The usual "open lockfile, write to it and then rename" dance is not
> >> necessary for the purpose of preventing other people from reading
> >> this file while we are writing to it.  But if we fail inside this
> >> function before we fclose(3) "out", the user will lose the todo
> >> list.  It probably is not a big deal, though.
> >
> > I guess you're right. It is bug-for-bug equivalent to the previous shell
> > function, though.
> 
> I think the scripted version uses the "write to $todo.new and mv
> $todo.new to $todo" pattern so you'd at least have something to go
> back to when the loopfails.

My mistake.

Sorry,
Dscho

Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Junio,

On Thu, 27 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Wed, 26 Apr 2017, Junio C Hamano wrote:
> >
> >> Johannes Schindelin  writes:
> >> 
> >> > diff --git a/sequencer.c b/sequencer.c
> >> > index 77afecaebf0..e858a976279 100644
> >> > --- a/sequencer.c
> >> > +++ b/sequencer.c
> >> > @@ -2388,3 +2388,48 @@ void append_signoff(struct strbuf *msgbuf, int 
> >> > ignore_footer, unsigned flag)
> >> >  
> >> >  strbuf_release();
> >> >  }
> >> > +
> >> > +int sequencer_make_script(int keep_empty, FILE *out,
> >> > +int argc, const char **argv)
> >> > +{
> >> > +char *format = xstrdup("%s");
> >> > +struct pretty_print_context pp = {0};
> >> > +struct strbuf buf = STRBUF_INIT;
> >> > +struct rev_info revs;
> >> > +struct commit *commit;
> >> > +
> >> > +init_revisions(, NULL);
> >> > +revs.verbose_header = 1;
> >> > +revs.max_parents = 1;
> >> > +revs.cherry_pick = 1;
> >> > +revs.limited = 1;
> >> > +revs.reverse = 1;
> >> > +revs.right_only = 1;
> >> > +revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> >> > +revs.topo_order = 1;
> >> > +
> >> > +revs.pretty_given = 1;
> >> > +git_config_get_string("rebase.instructionFormat", );
> >> > +get_commit_format(format, );
> >> > +free(format);
> >> > +pp.fmt = revs.commit_format;
> >> > +pp.output_encoding = get_log_output_encoding();
> >> 
> >> All of the above feels like inviting unnecessary future breakages by
> >> knowing too much about the implementation the current version of
> >> revision.c happens to use.
> >
> > You mean that the `--reverse` option gets translated into the `reverse`
> > bit, and the other settings?
> 
> Yes.  The "pretty_given" trick is one example that the underlying
> implementation can change over time.  If you wrote this patch before
> 66b2ed09 ("Fix "log" family not to be too agressive about showing
> notes", 2010-01-20) happened, you wouldn't have known to flip this
> bit on to emulate the command line parsing of "--pretty" and
> friends, and you would have required the author of that change to
> know that you have this cut & pasted duplicated code here when the
> commit is primarily about updating revision.c
> 
> So I am very serious when I say that this is adding an unnecessary
> maintenance burden.

In that case, I would strongly advise to consider redesigning the API. It
is just no good to ask for a change in stringent code that would delay
compile errors to runtime errors, that's just poor form.

And if the API allows settings that do something unintentional without at
least a runtime warning, the API is no good.

Ciao,
Dscho

Re: [PATCH v3 6/9] rebase -i: check for missing commits in the rebase--helper

[Johannes Schindelin]

Hi Junio,


On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > -check_todo_list
> > +git rebase--helper --check-todo-list || {
> > +   ret=$?
> > +   checkout_onto
> > +   exit $ret
> > +}
> 
> I find this a better division of labor between "check_todo_list" and
> its caller.  Compared to the original that did the "recover and exit
> with failure" inside the helper, this is much easier to see what is
> going on.

Yes. My first attempt did not even checkout , and it was
surprisingly difficult to pin that one down. I would never have expected
check_todo_list to have that side effect.

> > +/*
> > + * Check if the user dropped some commits by mistake
> > + * Behaviour determined by rebase.missingCommitsCheck.
> > + * Check if there is an unrecognized command or a
> > + * bad SHA-1 in a command.
> > + */
> > +int check_todo_list(void)
> > +{
> > +   enum check_level check_level = get_missing_commit_check_level();
> > +   struct strbuf todo_file = STRBUF_INIT;
> > +   struct todo_list todo_list = TODO_LIST_INIT;
> > +   struct commit_list *missing = NULL;
> > +   int raise_error = 0, res = 0, fd, i;
> > +
> > +   strbuf_addstr(_file, rebase_path_todo());
> > +   fd = open(todo_file.buf, O_RDONLY);
> > +   if (fd < 0) {
> > +   res = error_errno(_("could not open '%s'"), todo_file.buf);
> > +   goto leave_check;
> > +   }
> > +   if (strbuf_read(_list.buf, fd, 0) < 0) {
> > +   close(fd);
> > +   res = error(_("could not read '%s'."), todo_file.buf);
> > +   goto leave_check;
> > +   }
> > +   close(fd);
> > +   raise_error = res =
> > +   parse_insn_buffer(todo_list.buf.buf, _list);
> > +
> > +   if (check_level == CHECK_IGNORE)
> > +   goto leave_check;
> 
> OK, so even it is set to ignore, unreadable todo list will be shown
> with a loud error message that tells the user to use --edit-todo.
> 
> What should happen when it is not set to ignore and we found the
> todo list unacceptable, I wonder?

Whoops. In case of a parse error, it does not make sense to check, does
it. Fixed.

> > +   /* Get the SHA-1 of the commits */
> > +   for (i = 0; i < todo_list.nr; i++) {
> > +   struct commit *commit = todo_list.items[i].commit;
> > +   if (commit)
> > +   commit->util = todo_list.items + i;
> > +   }
> 
> It does not look like this loop is "Get(ting) the SHA-1 of the commits"
> to me, though.  It is setting up ->util to be usable as a back-pointer
> into the list.

Right, and that is not even necessary. It is even incorrect, as we release
the todo_list and read git-rebase-todo.backup into the same data
structure, possibly reallocating the array, therefore the pointers may
become stale. So I went with your suggestion further down to use (void *)1
instead.

Also, the comment is actively wrong, I agree. I changed it to

/* Mark the commits in git-rebase-todo as seen */

> > +   todo_list_release(_list);
> 
> But then the todo-list is released?  The util field we have set, if
> any, in the previous loop are now dangling, no?

Right.

> > +   strbuf_addstr(_file, ".backup");
> > +   fd = open(todo_file.buf, O_RDONLY);
> > +   if (fd < 0) {
> > +   res = error_errno(_("could not open '%s'"), todo_file.buf);
> > +   goto leave_check;
> > +   }
> > +   if (strbuf_read(_list.buf, fd, 0) < 0) {
> > +   close(fd);
> > +   res = error(_("could not read '%s'."), todo_file.buf);
> > +   goto leave_check;
> > +   }
> > +   close(fd);
> > +   strbuf_release(_file);
> > +   res = !!parse_insn_buffer(todo_list.buf.buf, _list);
> 
> Then we read from .backup; failure to do so does not result in the
> "you need to --edit-todo" warning.

Correct. At this point, we could even add

if (res)
die("BUG: cannot read '%s'", todo_file.buf);

(moving the strbuf_release(_file) below, of course), as the .backup
file is not intended to be edited by the user, i.e. it is the original
todo which should *never* be unparseable.

> > +   /* Find commits that are missing after editing */
> > +   for (i = 0; i < todo_list.nr; i++) {
> > +   struct commit *commit = todo_list.items[i].commit;
> > +   if (commit && !commit->util) {
> > +   commit_list_insert(commit, );
> > +   commit->util = todo_list.items + i;
> > +   }
> > +   }
> 
> And we check the commits mentioned in the backup; any commit whose
> util is not marked in the previous loop is noticed and thrown into
> the missing list.
> 
> The loop we later have does "while (missing)" and does not look at
> commit->util for commits that are *not* missing, i.e. the ones that
> are marked in the previous loop, so it does not matter that their
> util field have dangling pointers.  In that sense, it may not be
> buggy, but it is misleading.  The only thing these two loops care
> about is that the commits found in the earlier loop get 

Re: push fails with return code 22

[Andrew Watson]

Hi Peff,

I agree that it does look like the client isn't using smart-http. I am
using the Windows client from git-scm.com.

Thanks for pointing me to git help http-backend. I confirmed the
modules are loaded and the CGI environment variables. I've added
"AcceptPathInfo On" to my httpd.conf just to be safe.

I'm not sure what /info/refs is supposed to look like, but it is
empty. Could that be the issue?

Do you see anything in my apache configuration that looks wrong?

Andrew

On Thu, Apr 27, 2017 at 4:18 PM, Jeff King  wrote:
> On Thu, Apr 27, 2017 at 02:37:19PM -0400, Andrew Watson wrote:
>
>> I'm trying to setup git with Smart HTTP so we can move off of SVN.
>>
>> I've used the blog post: https://git-scm.com/blog/2010/03/04/smart-http.html
>
> I'm not sure how that post will have aged. You might check your setup
> against the documentation in "git help http-backend", which is kept more
> up to date.
>
>> My system is CentOS 7 which reports git version 1.8.3.1 and Apache
>> 2.4.6. I also tried on Ubuntu 16.04 with git 2.7.4 and Apache 2.4.18.
>>
>> Using GIT_CURL_VERBOSE I can see it fail after a PROPFIND.
>
> That means the client isn't using smart-http. PROPFIND is part of the
> "dumb" http push-over-webdav.
>
> So the problem is likely in the very first request Git makes to
> /info/refs/?service=git-receive-pack. The response there is what the
> client uses to decide whether the server understands smart-http or not.
>
>> My stackoverflow post with all the debug info I could think of is
>> here: 
>> http://stackoverflow.com/questions/43643152/git-push-results-in-return-code-22
>
> I notice the response for that first request has:
>
>   Content-Length: 0
>   Content-Type: text/plain; charset=UTF-8
>
> which implies to me that the git-http-backend CGI isn't being run.
>
> -Peff

Re: [PATCH v3 2/5] clone: add a --no-tags option to clone without tags

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 27, 2017 at 7:54 PM, Stefan Beller  wrote:
> On Wed, Apr 26, 2017 at 4:12 PM, Ævar Arnfjörð Bjarmason
>  wrote:
>> Add a --no-tags option to clone without fetching any tags.
>>
>> Without this change there's no easy way to clone a repository without
>> also fetching its tags.
>>
>> When supplying --single-branch the primary remote branch will be
>> cloned, but in addition tags will be followed & retrieved. Now
>> --no-tags can be added --single-branch to clone a repository without
>> tags, and which only tracks a single upstream branch.
>>
>> This option works without --single-branch as well, and will do a
>> normal clone but not fetch any tags.
>>
>> Many git commands pay some fixed overhead as a function of the number
>> of references. E.g. creating ~40k tags in linux.git will cause a
>> command like `git log -1 >/dev/null` to run in over a second instead
>> of in a matter of milliseconds, in addition numerous other things will
>> slow down, e.g. "git log " with the bash completion will slowly
>> show ~40k references instead of 1.
>>
>> The user might want to avoid all of that overhead to simply use a
>> repository like that to browse the "master" branch, or something like
>> a CI tool might want to keep that one branch up-to-date without caring
>> about any other references.
>>
>> Without this change the only way of accomplishing this was either by
>> manually tweaking the config in a fresh repository:
>>
>> git init git &&
>> cat >git/.git/config <> [remote "origin"]
>> url = g...@github.com:git/git.git
>> tagOpt = --no-tags
>> fetch = +refs/heads/master:refs/remotes/origin/master
>> [branch "master"]
>> remote = origin
>> merge = refs/heads/master
>> EOF
>> cd git &&
>> git pull
>>
>> Which requires hardcoding the "master" name, which may not be the main
>> --single-branch would have retrieved, or alternatively by setting
>> tagOpt=--no-tags right after cloning & deleting any existing tags:
>>
>> git clone --single-branch g...@github.com:git/git.git &&
>> cd git &&
>> git config remote.origin.tagOpt --no-tags &&
>> git tag -l | xargs git tag -d
>>
>> Which of course was also subtly buggy if --branch was pointed at a
>> tag, leaving the user in a detached head:
>>
>> git clone --single-branch --branch v2.12.0 g...@github.com:git/git.git &&
>> cd git &&
>> git config remote.origin.tagOpt --no-tags &&
>> git tag -l | xargs git tag -d
>>
>> Now all this complexity becomes the much simpler:
>>
>> git clone --single-branch --no-tags g...@github.com:git/git.git
>>
>> Or in the case of cloning a single tag "branch":
>>
>> git clone --single-branch --branch v2.12.0 --no-tags 
>> g...@github.com:git/git.git
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>
> I like the option, though I dislike the implementation, specifically as you
> brought up e.g. "[PATCH] various: disallow --no-no-OPT for --no-opt options".
>
> Can we have an option "--tags" instead, which is on by default
> and then you can negate it to --no-tags, without having to worry
> about the no-no case.
>
> The problem with tags is that they are in a shared name-space
> and not part of the remote refspec. If they were, the documentation
> would be way easier, too going this way.

I don't mind doing it this way, but this really should be part of an
unrelated topic to eliminate --no-OPT in favor of just --OPT which is
supplied by default, if we want to go that route.

Right now it makes more sense to have a --no-tags, because this along
with e.g. --no-hardlinks and numerous other options[1] is for an
obscure use-case most users won't care about, and then we have &
document --no-BEHAVIOR.

Perhaps we shouldn't have that at all, go through all these --no-OPT
and replace them with --OPT in both the docs & code & get rid of the
--no-no-OPT & some confusion that way.

1. git grep -- '\[--no-' Documentation/git-*txt

>> +--no-tags::
>> +   Don't clone any tags, and set
>> +   `remote..tagOpt=--no-tags` in the config, ensuring
>> +   that future `git pull` and `git fetch` operations won't follow
>> +   any tags. Subsequent explicit tag fetches will still work,
>> +   (see linkgit:git-fetch[1]).
>> ++
>> +Can be used in conjunction with `--single-branch` to clone and
>> +maintain a branch with no references other than a single cloned
>> +branch. This is useful e.g. to maintain minimal clones of the default
>> +branch of some repository for search indexing.
>
> In the future we may want to have '--depth' also imply --no-tags,
> just as it implies --single-branch.

Are there some cases where we'd like to also somehow import & rewrite
tags covering the given --depth?

>> @@ -652,7 +655,7 @@ static void update_remote_refs(const struct ref *refs,
>>
>> if (refs) {
>> write_remote_refs(mapped_refs);
>> -   if (option_single_branch)
>> +  

[PATCH v2 21/25] add_reflog_for_walk: avoid memory leak

[Johannes Schindelin]

We free()d the `log` buffer when dwim_log() returned 1, but not when it
returned a larger value (which meant that it still allocated the buffer
but we simply ignored it).

While in the vicinity, make sure that the `reflogs` structure as well as
the `branch` variable are released properly, too.

Identified by Coverity.

Signed-off-by: Johannes Schindelin 
---
 reflog-walk.c | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 99679f58255..c63eb1a3fd7 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -183,7 +183,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
if (!reflogs || reflogs->nr == 0) {
struct object_id oid;
char *b;
-   if (dwim_log(branch, strlen(branch), oid.hash, ) == 
1) {
+   int ret = dwim_log(branch, strlen(branch),
+  oid.hash, );
+   if (ret > 1)
+   free(b);
+   else if (ret == 1) {
if (reflogs) {
free(reflogs->ref);
free(reflogs);
@@ -193,17 +197,27 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
reflogs = read_complete_reflog(branch);
}
}
-   if (!reflogs || reflogs->nr == 0)
+   if (!reflogs || reflogs->nr == 0) {
+   if (reflogs) {
+   free(reflogs->ref);
+   free(reflogs);
+   }
+   free(branch);
return -1;
+   }
string_list_insert(>complete_reflogs, branch)->util
= reflogs;
}
+   free(branch);
 
commit_reflog = xcalloc(1, sizeof(struct commit_reflog));
if (recno < 0) {
commit_reflog->recno = get_reflog_recno_by_time(reflogs, 
timestamp);
if (commit_reflog->recno < 0) {
-   free(branch);
+   if (reflogs) {
+   free(reflogs->ref);
+   free(reflogs);
+   }
free(commit_reflog);
return -1;
}
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 25/25] submodule_uses_worktrees(): plug memory leak

[Johannes Schindelin]

There is really no reason why we would need to hold onto the allocated
string longer than necessary.

Reported by Coverity.

Signed-off-by: Johannes Schindelin 
---
 worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/worktree.c b/worktree.c
index bae787cf8d7..89a81b13de3 100644
--- a/worktree.c
+++ b/worktree.c
@@ -399,6 +399,7 @@ int submodule_uses_worktrees(const char *path)
 
/* The env would be set for the superproject. */
get_common_dir_noenv(, submodule_gitdir);
+   free(submodule_gitdir);
 
/*
 * The check below is only known to be good for repository format
@@ -418,7 +419,6 @@ int submodule_uses_worktrees(const char *path)
/* See if there is any file inside the worktrees directory. */
dir = opendir(sb.buf);
strbuf_release();
-   free(submodule_gitdir);
 
if (!dir)
return 0;
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 24/25] show_worktree(): plug memory leak

[Johannes Schindelin]

The buffer allocated by shorten_unambiguous_ref() needs to be released.

Discovered by Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/worktree.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 1722a9bdc2a..ff5dfd2b102 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -414,9 +414,11 @@ static void show_worktree(struct worktree *wt, int 
path_maxlen, int abbrev_len)
find_unique_abbrev(wt->head_sha1, 
DEFAULT_ABBREV));
if (wt->is_detached)
strbuf_addstr(, "(detached HEAD)");
-   else if (wt->head_ref)
-   strbuf_addf(, "[%s]", 
shorten_unambiguous_ref(wt->head_ref, 0));
-   else
+   else if (wt->head_ref) {
+   char *ref = shorten_unambiguous_ref(wt->head_ref, 0);
+   strbuf_addf(, "[%s]", ref);
+   free(ref);
+   } else
strbuf_addstr(, "(error)");
}
printf("%s\n", sb.buf);
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 23/25] name-rev: avoid leaking memory in the `deref` case

[Johannes Schindelin]

When the `name_rev()` function is asked to dereference the tip name, it
allocates memory. But when it turns out that another tip already
described the commit better than the current one, we forgot to release
the memory.

Pointed out by Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/name-rev.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 92a5d8a5d26..a4ce73fb1e9 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -28,6 +28,7 @@ static void name_rev(struct commit *commit,
struct rev_name *name = (struct rev_name *)commit->util;
struct commit_list *parents;
int parent_number = 1;
+   char *p = NULL;
 
parse_commit(commit);
 
@@ -35,7 +36,7 @@ static void name_rev(struct commit *commit,
return;
 
if (deref) {
-   tip_name = xstrfmt("%s^0", tip_name);
+   tip_name = p = xstrfmt("%s^0", tip_name);
 
if (generation)
die("generation: %d, but deref?", generation);
@@ -53,8 +54,10 @@ static void name_rev(struct commit *commit,
name->taggerdate = taggerdate;
name->generation = generation;
name->distance = distance;
-   } else
+   } else {
+   free(p);
return;
+   }
 
for (parents = commit->parents;
parents;
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 22/25] remote: plug memory leak in match_explicit()

[Johannes Schindelin]

The `guess_ref()` returns an allocated buffer of which `make_linked_ref()`
does not take custody (`alloc_ref()` makes a copy), therefore we need to
release the buffer afterwards.

Noticed via Coverity.

Signed-off-by: Johannes Schindelin 
---
 remote.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 801137c72eb..72b4591b983 100644
--- a/remote.c
+++ b/remote.c
@@ -1191,9 +1191,10 @@ static int match_explicit(struct ref *src, struct ref 
*dst,
else if (is_null_oid(_src->new_oid))
error("unable to delete '%s': remote ref does not 
exist",
  dst_value);
-   else if ((dst_guess = guess_ref(dst_value, matched_src)))
+   else if ((dst_guess = guess_ref(dst_value, matched_src))) {
matched_dst = make_linked_ref(dst_guess, dst_tail);
-   else
+   free(dst_guess);
+   } else
error("unable to push to unqualified destination: %s\n"
  "The destination refspec neither matches an "
  "existing ref on the remote nor\n"
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 20/25] shallow: avoid memory leak

[Johannes Schindelin]

Reported by Coverity.

Signed-off-by: Johannes Schindelin 
---
 shallow.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/shallow.c b/shallow.c
index 25b6db989bf..f9370961f99 100644
--- a/shallow.c
+++ b/shallow.c
@@ -473,11 +473,15 @@ static void paint_down(struct paint_info *info, const 
unsigned char *sha1,
struct commit_list *head = NULL;
int bitmap_nr = (info->nr_bits + 31) / 32;
size_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr);
-   uint32_t *tmp = xmalloc(bitmap_size); /* to be freed before return */
-   uint32_t *bitmap = paint_alloc(info);
struct commit *c = lookup_commit_reference_gently(sha1, 1);
+   uint32_t *tmp; /* to be freed before return */
+   uint32_t *bitmap;
+
if (!c)
return;
+
+   tmp = xmalloc(bitmap_size);
+   bitmap = paint_alloc(info);
memset(bitmap, 0, bitmap_size);
bitmap[id / 32] |= (1U << (id % 32));
commit_list_insert(c, );
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 18/25] receive-pack: plug memory leak in update()

[Johannes Schindelin]

Reported via Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/receive-pack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f96834f42c9..48c07ddb659 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -986,7 +986,8 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
 {
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
-   const char *namespaced_name, *ret;
+   static char *namespaced_name;
+   const char *ret;
struct object_id *old_oid = >old_oid;
struct object_id *new_oid = >new_oid;
 
@@ -997,6 +998,7 @@ static const char *update(struct command *cmd, struct 
shallow_info *si)
}
 
strbuf_addf(_name_buf, "%s%s", get_git_namespace(), name);
+   free(namespaced_name);
namespaced_name = strbuf_detach(_name_buf, NULL);
 
if (is_ref_checked_out(namespaced_name)) {
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 19/25] line-log: avoid memory leak

[Johannes Schindelin]

Discovered by Coverity.

Signed-off-by: Johannes Schindelin 
---
 line-log.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/line-log.c b/line-log.c
index a23b910471b..b9087814b8c 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1125,6 +1125,7 @@ static int process_ranges_ordinary_commit(struct rev_info 
*rev, struct commit *c
changed = process_all_files(_range, rev, , range);
if (parent)
add_line_range(rev, parent, parent_range);
+   free_line_log_data(parent_range);
return changed;
 }
 
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 17/25] fast-export: avoid leaking memory in handle_tag()

[Johannes Schindelin]

Reported by, you guessed it, Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/fast-export.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index e0220630d00..64617ad8e36 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -734,6 +734,7 @@ static void handle_tag(const char *name, struct tag *tag)
 oid_to_hex(>object.oid));
case DROP:
/* Ignore this tag altogether */
+   free(buf);
return;
case REWRITE:
if (tagged->type != OBJ_COMMIT) {
@@ -765,6 +766,7 @@ static void handle_tag(const char *name, struct tag *tag)
   (int)(tagger_end - tagger), tagger,
   tagger == tagger_end ? "" : "\n",
   (int)message_size, (int)message_size, message ? message : "");
+   free(buf);
 }
 
 static struct commit *get_commit(struct rev_cmdline_entry *e, char *full_name)
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 16/25] mktree: plug memory leaks reported by Coverity

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 builtin/mktree.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/mktree.c b/builtin/mktree.c
index de9b40fc63b..f0354bc9718 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -72,7 +72,7 @@ static void mktree_line(char *buf, size_t len, int 
nul_term_line, int allow_miss
unsigned mode;
enum object_type mode_type; /* object type derived from mode */
enum object_type obj_type; /* object type derived from sha */
-   char *path;
+   char *path, *p = NULL;
unsigned char sha1[20];
 
ptr = buf;
@@ -102,7 +102,7 @@ static void mktree_line(char *buf, size_t len, int 
nul_term_line, int allow_miss
struct strbuf p_uq = STRBUF_INIT;
if (unquote_c_style(_uq, path, NULL))
die("invalid quoting");
-   path = strbuf_detach(_uq, NULL);
+   path = p = strbuf_detach(_uq, NULL);
}
 
/*
@@ -136,6 +136,7 @@ static void mktree_line(char *buf, size_t len, int 
nul_term_line, int allow_miss
}
 
append_to_tree(mode, sha1, path);
+   free(p);
 }
 
 int cmd_mktree(int ac, const char **av, const char *prefix)
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 12/25] split_commit_in_progress(): fix memory leak

[Johannes Schindelin]

Reported via Coverity.

Signed-off-by: Johannes Schindelin 
---
 wt-status.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 0a6e16dbe0f..1f3f6bcb980 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1088,8 +1088,13 @@ static int split_commit_in_progress(struct wt_status *s)
char *rebase_orig_head = 
read_line_from_git_path("rebase-merge/orig-head");
 
if (!head || !orig_head || !rebase_amend || !rebase_orig_head ||
-   !s->branch || strcmp(s->branch, "HEAD"))
+   !s->branch || strcmp(s->branch, "HEAD")) {
+   free(head);
+   free(orig_head);
+   free(rebase_amend);
+   free(rebase_orig_head);
return split_in_progress;
+   }
 
if (!strcmp(rebase_amend, rebase_orig_head)) {
if (strcmp(head, rebase_amend))
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 14/25] setup_discovered_git_dir(): help static analysis

[Johannes Schindelin]

Coverity reported a memory leak in this function. However, it can only
be called once, as setup_git_directory() changes global state and hence
is not reentrant.

Mark the variable as static to indicate that this is a singleton.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index 0320a9ad14c..12efca85a41 100644
--- a/setup.c
+++ b/setup.c
@@ -703,11 +703,16 @@ static const char *setup_discovered_git_dir(const char 
*gitdir,
 
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
+   char *p = NULL;
+   const char *ret;
+
if (offset != cwd->len && !is_absolute_path(gitdir))
-   gitdir = real_pathdup(gitdir, 1);
+   gitdir = p = real_pathdup(gitdir, 1);
if (chdir(cwd->buf))
die_errno("Could not come back to cwd");
-   return setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+   ret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);
+   free(p);
+   return ret;
}
 
/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 13/25] setup_bare_git_dir(): help static analysis

[Johannes Schindelin]

Coverity reported a memory leak in this function. However, it can only
be called once, as setup_git_directory() changes global state and hence
is not reentrant.

Mark the variable as static to indicate that this is a singleton.

Signed-off-by: Johannes Schindelin 
---
 setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 0309c278218..0320a9ad14c 100644
--- a/setup.c
+++ b/setup.c
@@ -748,7 +748,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, 
int offset,
 
/* --work-tree is set without --git-dir; use discovered one */
if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
-   const char *gitdir;
+   static const char *gitdir;
 
gitdir = offset == cwd->len ? "." : xmemdupz(cwd->buf, offset);
if (chdir(cwd->buf))
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 15/25] pack-redundant: plug memory leak

[Johannes Schindelin]

Identified via Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/pack-redundant.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844dd..cb1df1c7614 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -442,6 +442,7 @@ static void minimize(struct pack_list **min)
/* return if there are no objects missing from the unique set */
if (missing->size == 0) {
*min = unique;
+   free(missing);
return;
}
 
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 10/25] cat-file: fix memory leak

[Johannes Schindelin]

Discovered by Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/cat-file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 1890d7a6390..9af863e7915 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, 
const char *obj_name,
die("git cat-file %s: bad file", obj_name);
 
write_or_die(1, buf, size);
+   free(buf);
return 0;
 }
 
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 11/25] checkout: fix memory leak

[Johannes Schindelin]

This change addresses part of the NEEDSWORK comment above the code,
therefore the comment needs to be adjusted, too.

Discovered via Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/checkout.c | 17 +
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f335..5faea3a05fa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout 
*state)
/*
 * NEEDSWORK:
 * There is absolutely no reason to write this as a blob object
-* and create a phony cache entry just to leak.  This hack is
-* primarily to get to the write_entry() machinery that massages
-* the contents to work-tree format and writes out which only
-* allows it for a cache entry.  The code in write_entry() needs
-* to be refactored to allow us to feed a 
-* instead of a cache entry.  Such a refactoring would help
-* merge_recursive as well (it also writes the merge result to the
-* object database even when it may contain conflicts).
+* and create a phony cache entry.  This hack is primarily to get
+* to the write_entry() machinery that massages the contents to
+* work-tree format and writes out which only allows it for a
+* cache entry.  The code in write_entry() needs to be refactored
+* to allow us to feed a  instead of a cache
+* entry.  Such a refactoring would help merge_recursive as well
+* (it also writes the merge result to the object database even
+* when it may contain conflicts).
 */
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, oid.hash))
@@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout 
*state)
if (!ce)
die(_("make_cache_entry failed for path '%s'"), path);
status = checkout_entry(ce, state, NULL);
+   free(ce);
return status;
 }
 
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 09/25] mailinfo & mailsplit: check for EOF while parsing

[Johannes Schindelin]

While POSIX states that it is okay to pass EOF to isspace() (and it seems
to be implied that EOF should *not* be treated as whitespace), and also to
pass EOF to ungetc() (which seems to be intended to fail without buffering
the character), it is much better to handle these cases explicitly. Not
only does it reduce head-scratching (and helps static analysis avoid
reporting false positives), it also lets us handle files containing
nothing but whitespace by erroring out.

Reported via Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/mailsplit.c |  3 ++-
 mailinfo.c  | 15 +++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 30681681c13..9b3efc8e987 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -233,7 +233,8 @@ static int split_mbox(const char *file, const char *dir, 
int allow_bare,
do {
peek = fgetc(f);
} while (isspace(peek));
-   ungetc(peek, f);
+   if (peek != EOF)
+   ungetc(peek, f);
 
if (strbuf_getwholeline(, f, '\n')) {
/* empty stdin is OK */
diff --git a/mailinfo.c b/mailinfo.c
index 68037758f2f..a319911b510 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE 
*in)
for (;;) {
int peek;
 
-   peek = fgetc(in); ungetc(peek, in);
+   peek = fgetc(in);
+   if (peek == EOF)
+   break;
+   ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(, in))
@@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, 
const char *patch)
return -1;
}
 
-   mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
-   mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
-
do {
peek = fgetc(mi->input);
+   if (peek == EOF) {
+   fclose(cmitmsg);
+   return error("empty patch: '%s'", patch);
+   }
} while (isspace(peek));
ungetc(peek, mi->input);
 
+   mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
+   mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
+
/* process the email header */
while (read_one_header_line(, mi->input))
check_header(mi, , mi->p_hdr_data, 1);
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 08/25] status: close file descriptor after reading git-rebase-todo

[Johannes Schindelin]

Reported via Coverity.

Signed-off-by: Johannes Schindelin 
---
 wt-status.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/wt-status.c b/wt-status.c
index 03754849626..0a6e16dbe0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1168,6 +1168,7 @@ static int read_rebase_todolist(const char *fname, struct 
string_list *lines)
abbrev_sha1_in_line();
string_list_append(lines, line.buf);
}
+   fclose(f);
return 0;
 }
 
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 04/25] add_commit_patch_id(): avoid allocating memory unnecessarily

[Johannes Schindelin]

It would appear that we allocate (and forget to release) memory if the
patch ID is not even defined.

Reported by the Coverity tool.

Signed-off-by: Johannes Schindelin 
---
 patch-ids.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/patch-ids.c b/patch-ids.c
index fa8f11de826..92eba7a059e 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -99,11 +99,12 @@ struct patch_id *has_commit_patch_id(struct commit *commit,
 struct patch_id *add_commit_patch_id(struct commit *commit,
 struct patch_ids *ids)
 {
-   struct patch_id *key = xcalloc(1, sizeof(*key));
+   struct patch_id *key;
 
if (!patch_id_defined(commit))
return NULL;
 
+   key = xcalloc(1, sizeof(*key));
if (init_patch_id_entry(key, commit, ids)) {
free(key);
return NULL;
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 05/25] git_config_rename_section_in_file(): avoid resource leak

[Johannes Schindelin]

In case of errors, we really want the file descriptor to be closed.

Discovered by a Coverity scan.

Signed-off-by: Johannes Schindelin 
---
 config.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index b4a3205da32..a30056ec7e9 100644
--- a/config.c
+++ b/config.c
@@ -2621,7 +2621,7 @@ int git_config_rename_section_in_file(const char 
*config_filename,
struct lock_file *lock;
int out_fd;
char buf[1024];
-   FILE *config_file;
+   FILE *config_file = NULL;
struct stat st;
 
if (new_name && !section_name_is_ok(new_name)) {
@@ -2703,11 +2703,14 @@ int git_config_rename_section_in_file(const char 
*config_filename,
}
}
fclose(config_file);
+   config_file = NULL;
 commit_and_out:
if (commit_lock_file(lock) < 0)
ret = error_errno("could not write config file %s",
  config_filename);
 out:
+   if (config_file)
+   fclose(config_file);
rollback_lock_file(lock);
 out_no_rollback:
free(filename_buf);
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 06/25] get_mail_commit_oid(): avoid resource leak

[Johannes Schindelin]

When we fail to read, or parse, the file, we still want to close the file
descriptor and release the strbuf.

Reported via Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/am.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index a95dd8b4e6c..9c5c2c778e2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1351,19 +1351,16 @@ static int get_mail_commit_oid(struct object_id 
*commit_id, const char *mail)
struct strbuf sb = STRBUF_INIT;
FILE *fp = xfopen(mail, "r");
const char *x;
+   int ret = 0;
 
-   if (strbuf_getline_lf(, fp))
-   return -1;
-
-   if (!skip_prefix(sb.buf, "From ", ))
-   return -1;
-
-   if (get_oid_hex(x, commit_id) < 0)
-   return -1;
+   if (strbuf_getline_lf(, fp) ||
+   !skip_prefix(sb.buf, "From ", ) ||
+   get_oid_hex(x, commit_id) < 0)
+   ret = -1;
 
strbuf_release();
fclose(fp);
-   return 0;
+   return ret;
 }
 
 /**
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 07/25] difftool: address a couple of resource/memory leaks

[Johannes Schindelin]

This change plugs a couple of memory leaks and makes sure that the file
descriptor is closed in run_dir_diff().

Spotted by Coverity.

Signed-off-by: Johannes Schindelin 
---
 builtin/difftool.c | 33 +++--
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 1354d0e4625..b9a892f2693 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -226,6 +226,7 @@ static void changed_files(struct hashmap *result, const 
char *index_path,
hashmap_entry_init(entry, strhash(buf.buf));
hashmap_add(result, entry);
}
+   fclose(fp);
if (finish_command(_files))
die("diff-files did not exit properly");
strbuf_release(_env);
@@ -439,8 +440,10 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
}
 
if (lmode && status != 'C') {
-   if (checkout_path(lmode, , src_path, ))
-   return error("could not write '%s'", src_path);
+   if (checkout_path(lmode, , src_path, )) {
+   ret = error("could not write '%s'", src_path);
+   goto finish;
+   }
}
 
if (rmode && !S_ISLNK(rmode)) {
@@ -456,9 +459,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
hashmap_add(_tree_dups, entry);
 
if (!use_wt_file(workdir, dst_path, )) {
-   if (checkout_path(rmode, , dst_path, 
))
-   return error("could not write '%s'",
-dst_path);
+   if (checkout_path(rmode, , dst_path,
+ )) {
+   ret = error("could not write '%s'",
+   dst_path);
+   goto finish;
+   }
} else if (!is_null_oid()) {
/*
 * Changes in the working tree need special
@@ -473,10 +479,12 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
ADD_CACHE_JUST_APPEND);
 
add_path(, rdir_len, dst_path);
-   if (ensure_leading_directories(rdir.buf))
-   return error("could not create "
-"directory for '%s'",
-dst_path);
+   if (ensure_leading_directories(rdir.buf)) {
+   ret = error("could not create "
+   "directory for '%s'",
+   dst_path);
+   goto finish;
+   }
add_path(, wtdir_len, dst_path);
if (symlinks) {
if (symlink(wtdir.buf, rdir.buf)) {
@@ -497,13 +505,15 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
}
}
 
+   fclose(fp);
+   fp = NULL;
if (finish_command()) {
ret = error("error occurred running diff --raw");
goto finish;
}
 
if (!i)
-   return 0;
+   goto finish;
 
/*
 * Changes to submodules require special treatment.This loop writes a
@@ -626,6 +636,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
exit_cleanup(tmpdir, rc);
 
 finish:
+   if (fp)
+   fclose(fp);
+
free(lbase_dir);
free(rbase_dir);
strbuf_release();
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 03/25] winansi: avoid buffer overrun

[Johannes Schindelin]

When we could not convert the UTF-8 sequence into Unicode for writing to
the Console, we should not try to write an insanely-long sequence of
invalid wide characters (mistaking the negative return value for an
unsigned length).

Reported by Coverity.

Signed-off-by: Johannes Schindelin 
---
 compat/winansi.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/compat/winansi.c b/compat/winansi.c
index fd6910746c8..861b79d8c31 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -135,6 +135,11 @@ static void write_console(unsigned char *str, size_t len)
 
/* convert utf-8 to utf-16 */
int wlen = xutftowcsn(wbuf, (char*) str, ARRAY_SIZE(wbuf), len);
+   if (wlen < 0) {
+   wchar_t *err = L"[invalid]";
+   WriteConsoleW(console, err, wcslen(err), , NULL);
+   return;
+   }
 
/* write directly to console */
WriteConsoleW(console, wbuf, wlen, , NULL);
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 02/25] winansi: avoid use of uninitialized value

[Johannes Schindelin]

When stdout is not connected to a Win32 console, we incorrectly used an
uninitialized value for the "plain" character attributes.

Detected by Coverity.

Signed-off-by: Johannes Schindelin 
---
 compat/winansi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/compat/winansi.c b/compat/winansi.c
index 793420f9d0d..fd6910746c8 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -105,6 +105,8 @@ static int is_console(int fd)
if (!fd) {
if (!GetConsoleMode(hcon, ))
return 0;
+   sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN |
+   FOREGROUND_RED;
} else if (!GetConsoleScreenBufferInfo(hcon, ))
return 0;
 
-- 
2.12.2.windows.2.800.gede8f145e06

[PATCH v2 00/25] Address a couple of issues identified by Coverity

[Johannes Schindelin]

I recently registered the git-for-windows fork with Coverity to ensure
that even the Windows-specific patches get some static analysis love.

While at it, I squashed a couple of obvious issues in the part that is
not Windows-specific.

Note: while this patch series squashes some of those issues, the
remaining issues are not necessarily all false positives (e.g. Coverity
getting fooled by our FLEX_ARRAY trick into believing that the last
member of the struct is indeed a 0 or 1 size array) or intentional (e.g.
builtins not releasing memory because exit() is called right after
returning from the function that leaks memory).

Notable examples of the remaining issues are:

- a couple of callers of shorten_unambiguous_ref() assume that they do
  not have to release the memory returned from that function, often
  assigning the pointer to a `const` variable that typically does not
  hold a pointer that needs to be free()d. My hunch is that we will want
  to convert that function to have a fixed number of static buffers and
  use those in a round robin fashion à la sha1_to_hex().

- pack-redundant.c seems to have hard-to-reason-about code paths that
  may eventually leak memory. Essentially, the custody of the allocated
  memory is not clear at all.

- fast-import.c calls strbuf_detach() on the command_buf without any
  obvious rationale. Turns out that *some* code paths assign
  command_buf.buf to a `recent_command` which releases the buffer later.
  However, from a cursory look it appears to me as if there are some
  other code paths that skip that assignment, effectively causing a memory
  leak once strbuf_detach() is called.

Sadly, I lack the time to pursue those remaining issues further.

Changes since v1:

- clarified in the commit messages for the setup_*git_dir() functions
  that we are not really plugging a memory leak, but marking singletons
  as `static` to help Coverity not to complain about this again

- dropped the patch to http-backend, as it is supposedly a one-shot
  program using exit() as "garbage collector".

- the difftool patch does a more thorough job of fixing leaks now

- reworded the commit subject of the mailinfo/mailsplit patch to sport
  a prefix indicating the overall area of the fix

- changed the mailinfo/mailsplit patch to *really* handle EOF correctly

- simplified the patch to get_mail_commit_oid(), and now it even returns
  the correct error value!

- adjusted the comment in builtin/checkout.c that talked about leaking
  the cache_entry (which is not leaked anymore)

- add forgotten free(buf) in fast-export.c in an early return

- line-log data is now released properly

- a couple more memory leaks in add_reflog_for_walk() have been found
  and addressed (this one *really* needs a couple more eyeballs, though,
  it is just a much bigger change than I originally anticipated)


Johannes Schindelin (25):
  mingw: avoid memory leak when splitting PATH
  winansi: avoid use of uninitialized value
  winansi: avoid buffer overrun
  add_commit_patch_id(): avoid allocating memory unnecessarily
  git_config_rename_section_in_file(): avoid resource leak
  get_mail_commit_oid(): avoid resource leak
  difftool: address a couple of resource/memory leaks
  status: close file descriptor after reading git-rebase-todo
  mailinfo & mailsplit: check for EOF while parsing
  cat-file: fix memory leak
  checkout: fix memory leak
  split_commit_in_progress(): fix memory leak
  setup_bare_git_dir(): help static analysis
  setup_discovered_git_dir(): help static analysis
  pack-redundant: plug memory leak
  mktree: plug memory leaks reported by Coverity
  fast-export: avoid leaking memory in handle_tag()
  receive-pack: plug memory leak in update()
  line-log: avoid memory leak
  shallow: avoid memory leak
  add_reflog_for_walk: avoid memory leak
  remote: plug memory leak in match_explicit()
  name-rev: avoid leaking memory in the `deref` case
  show_worktree(): plug memory leak
  submodule_uses_worktrees(): plug memory leak

 builtin/am.c | 15 ++-
 builtin/cat-file.c   |  1 +
 builtin/checkout.c   | 17 +
 builtin/difftool.c   | 33 +++--
 builtin/fast-export.c|  2 ++
 builtin/mailsplit.c  |  3 ++-
 builtin/mktree.c |  5 +++--
 builtin/name-rev.c   |  7 +--
 builtin/pack-redundant.c |  1 +
 builtin/receive-pack.c   |  4 +++-
 builtin/worktree.c   |  8 +---
 compat/mingw.c   |  4 +++-
 compat/winansi.c |  7 +++
 config.c |  5 -
 line-log.c   |  1 +
 mailinfo.c   | 15 +++
 patch-ids.c  |  3 ++-
 reflog-walk.c| 20 +---
 remote.c |  5 +++--
 setup.c  | 11 ---
 shallow.c|  8 ++--
 worktree.c   |  2 +-
 wt-status.c  |  8 +++-
 23 files changed, 130 insertions(+), 55 deletions(-)


base-commit: 

[PATCH v2 01/25] mingw: avoid memory leak when splitting PATH

[Johannes Schindelin]

In the (admittedly, concocted) case that PATH consists only of colons, we
would leak the duplicated string.

Signed-off-by: Johannes Schindelin 
---
 compat/mingw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 3fbfda5978b..fe0e3ccd248 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -961,8 +961,10 @@ static char **get_path_split(void)
++n;
}
}
-   if (!n)
+   if (!n) {
+   free(envpath);
return NULL;
+   }
 
ALLOC_ARRAY(path, n + 1);
p = envpath;
-- 
2.12.2.windows.2.800.gede8f145e06

Re: [PATCH 10/26] Check for EOF while parsing mails

[Jeff King]

On Fri, Apr 28, 2017 at 03:33:41PM +0200, Johannes Schindelin wrote:

> As usual, EOF is defined as -1 in Git for Windows' context, meaning that
> we look at the last entry of the sane_ctype array, which returns 0 for any
> sane_istest(x,mask) test for x >= 0x80:
> 
> /* Nothing in the 128.. range */
> 
> So it would appear that it happens to work, but I doubt that it was
> intentional.

Yeah, that was the same analysis I came to. Even though it works, it is
a potential portability problem if a platform defines EOF in a weird
way. The "right" thing in such a case is probably checking explicitly
for EOF, but I'd hate to add an extra conditional to sane_istest(). It's
a fairly hot code path. So I'm fine with punting on that until we see
evidence of such a system.

> Having said that, it is really curious why Coverity should get confused by
> the code and not realize that casting a negative number to (unsigned char)
> will make it valid as an index for the sane_ctype array.

Yeah, that is the part that confuses me, too. It seems like an easy
case to get right.

Oh well. If you are tweaking it to handle EOF separately for other
reasons, then the Coverity question goes away entirely.

-Peff

Re: [PATCH 06/26] get_mail_commit_oid(): avoid resource leak

[Johannes Schindelin]

Hi Junio & Stefan,


On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Stefan Beller  writes:
> 
> >> -   if (get_oid_hex(x, commit_id) < 0)
> >> -   return -1;
> >> +   if (!ret && get_oid_hex(x, commit_id) < 0)
> >> +   ret = -1;
> >>
> >
> > In similar cases of fixing mem leaks, we'd put a label here
> > and make excessive use of goto, instead of setting ret to -1.
> > As "ret" and the commands are short, this is visually just as appealing.
> 
> I wouldn't call that visually appealing.  Fixing resource leaks is
> good, and only because this function is short enough and the funny
> way to skip over various operation need to last for just a few
> instructions, it is tolerable.  If the function were shorter, then
> we may have used
> 
>   if (!strbuf_getline_lf() &&
>   skip_prefix() &&
>   !get_oid_hex())
>   ret = 0; /* happy */
>   else
>   ret = -1;
>   release resources here;
> return ret;

I did almost what you suggested here, but I avoided the explicit ret = 0,
as it is initialized that way.

> and that would have been OK.  If longer, as you said, jumping to a
> label at the end of function to release the acquired resource would
> be a lot more maintainable way than either of the above alternatives
> that are only usable for short functions.
> 
> The patch as posted makes the function fail to return -1 when it
> finds problems, by the way.  It needs to return "ret" not the
> hardcoded "0" at the end.

Oops.

Ciao,
Dscho

Re: [PATCH 10/26] Check for EOF while parsing mails

[Johannes Schindelin]

Hi Peff,

On Fri, 28 Apr 2017, Jeff King wrote:

> On Fri, Apr 28, 2017 at 12:44:52PM +0200, Johannes Schindelin wrote:
> 
> > > Also, what is the behavior of ungetc when we pass it EOF?
> > 
> > According to the documentation, it would cast EOF to an unsigned char and
> > push that back. Definitely incorrect.
> > 
> > > It looks like POSIX does what we want (pushing EOF is a noop, and the
> > > stream retains its feof() status), but I don't know if there are other
> > > implementations to worry about.
> > 
> > That's not what my man page here says:
> > 
> > ungetc()  pushes  c  back to stream, cast to unsigned char, where
> > it is available for subsequent read operations.  Pushed-back
> > characters will be returned in reverse order; only one pushback is
> > guaranteed.
> 
> POSIX covers this this case explicitly:
> 
>   If the value of c equals that of the macro EOF, the operation shall
>   fail and the input stream shall be left unchanged.
> 
> That comes straight from C99, which says:
> 
>   If the value of c equals that of the macro EOF, the operation fails
>   and the input stream is unchanged.
> 
> I don't have a copy of C89 handy, but I didn't see any mention of the
> behavior in the "changes from the previous edition" section of C99.

Yeah. I'd still be more comfortable with being explicit in this case, also
because our documentation states explicitly that we do not necessarily
live by the POSIX standard.

Ciao,
Dscho

Re: [PATCH 10/26] Check for EOF while parsing mails

[Johannes Schindelin]

Hi Peff,

On Fri, 28 Apr 2017, Jeff King wrote:

> On Fri, Apr 28, 2017 at 12:41:02PM +0200, Johannes Schindelin wrote:
> 
> > But then, I guess I misunderstood what Coverity complained about:
> > maybe the problem was not so much the isspace() call but that EOF is
> > not being handled correctly. We pass it, unchecked, to ungetc().
> > 
> > It appears that I (or Coverity, if you will), missed another instance
> > where we simply passed EOF unchecked to ungetc().
> 
> I think that is also fine according to the standard.
> 
> Do you happen to have the exact error from Coverity?

Wow, that was unnecessarily hard. It is a major hassle to get to any
scan other than the latest one.

But I did it. Call me tenatious.

The report says this:

233do {
   2. negative_return_fn: Function mingw_fgetc(f) returns a negative number.
   3. var_assign: Assigning: signed variable peek = mingw_fgetc.
234peek = fgetc(f);
   CID 1049734: Negative array index read (NEGATIVE_RETURNS)
   4.  negative_returns: Using variable peek as an index to array sane_ctype.
235} while (isspace(peek));
236ungetc(peek, f);

So part of the thing is that we use mingw_fgetc() instead of fgetc().
However, the return value is *still* the one from the "real" fgetc(), even
if we intercept what appears to be a Ctrl+C from an interactive console.

> I'm wondering if it is complaining about some aspect of our custom
> isspace() when used with EOF.

That would appear to be the real issue, yes, and I should have
double-checked the claim that POSIX isspace() handles EOF properly: we
override isspace() with our own version, after all:

#define isspace(x) sane_istest(x,GIT_SPACE)

where

#define sane_istest(x,mask) \
((sane_ctype[(unsigned char)(x)] & (mask)) != 0)

(rewrapped for readability)

As usual, EOF is defined as -1 in Git for Windows' context, meaning that
we look at the last entry of the sane_ctype array, which returns 0 for any
sane_istest(x,mask) test for x >= 0x80:

/* Nothing in the 128.. range */

So it would appear that it happens to work, but I doubt that it was
intentional.

Having said that, it is really curious why Coverity should get confused by
the code and not realize that casting a negative number to (unsigned char)
will make it valid as an index for the sane_ctype array.

I double-checked, and there is no override for the isspace() function in
what Coverity calls a "model file" (i.e. pseudo code intended to helping
Coverity realize where it can stop reporting false positives).

> > The next iteration will have it completely reworked: I no longer guard
> > the isspace() behind an `!= EOF` check, but rather handle an early EOF
> > as I think it should be handled. Extra eyes very welcome (this is the
> > fixup!  patch):
> 
> I do think handling EOF explicitly is probably a better strategy anyway,
> as it lets us tell when we have an empty patch.

I agree, I came to the same conclusion independently.

Ciao,
Dscho

RE: Git and Active directory ldap Authentication

[Randall S. Becker]

On April 28, 2017 5:31 AM  Miguel Angel Soriano Morales wrote:
> I would like use git in my Company. We use Active directory for
everything, but I prefer install git in ?
> centos7. I Would like authenticate all my user in Git through Active
Directory. And Every Project had
> ACL permissions .It this possible?

The first thing to remember is that local clones will usually be secured to
the user who did the clone and are not usually subject to enterprise
security rules or ACLs. Security is usually applied when interacting with an
upstream repository from where you clone and push changes and authentication
is important at that time.

This might help:

https://technet.microsoft.com/en-us/library/2008.12.linux.aspx

This discusses SSO for Linux. You should already be covered for Windows.
However please give details on where your upstream repository is and what
server which is likely where you have to authenticate. Typically
authentication to upstream repositories is done through SSH - see git push. 

There are discussions of integrating SSH keys and AD here (and elsewhere):
https://social.technet.microsoft.com/Forums/en-US/8aa28e34-2007-49fe-a689-e2
8e19b2757b/is-there-a-way-to-link-ssh-key-in-ad?forum=winserverDS

You should also consider when, in your environment, to use GPG signing to
definitively identify who did the change even in their local repository. AD
is unlikely to help you there, unless you can use a custom attribute to
store and manage a user's GPG key.

Good luck!

Cheers,
Randall

Hello Beautiful,

[Jack]

Good day dear, i hope this mail meets you well? my name is Jack, from the U.S. 
I know this may seem inappropriate so i ask for your forgiveness but i wish to 
get to know you better, if I may be so bold. I consider myself an easy-going 
man, adventurous, honest and fun loving person but I am currently looking for a 
relationship in which I will feel loved. I promise to answer any question that 
you may want to ask me...all i need is just your attention and the chance to 
know you more.

Please tell me more about yourself, if you do not mind. Hope to hear back from 
you soon.

Jack.

Re: git v2.13.0-rc0 test failures on cygwin

[Adam Dinwoodie]

On 23 April 2017 at 15:44, Ramsay Jones wrote:
> [Adam, if you are no longer the git package maintainer for cygwin, then
> please ignore this email and sorry for the noise!]

I am still the Cygwin Git package maintainer; I've been quiet of late
because of personal health issues, but I'm now picking things back up
again.

> Test Summary Report
> ---
> t0301-credential-cache.sh(Wstat: 256 Tests: 29 
> Failed: 6)
>   Failed tests:  12, 24-28
>   Non-zero exit status: 1

Confirmed I'm seeing this on v2.13.0-rc1, and this passed in v2.12.2.
`git bisect` tells me this failure was introduced by commit
v2.12.0-267-g612c49e94, added by Devin Lehmacher (added to the CC
list).

> t8010-cat-file-filters.sh(Wstat: 256 Tests: 8 Failed: 
> 1)
>   Failed test:  8
>   Non-zero exit status: 1
> Files=780, Tests=14700, 10398 wallclock secs ( 1.27 usr  0.78 sys + 1265.08 
> cusr 4076.38 csys = 5343.50 CPU)
> Result: FAIL
> make[1]: *** [Makefile:45: prove] Error 1
> make[1]: Leaving directory '/home/ramsay/git/t'
> make: *** [Makefile:2313: test] Error 2

I also see this failure; `git bisect` tells me it was introduced by
v2.10.0-rc1-4-g321459439, added by Johannes Schindelin.

> I'm not sure how to proceed from here. (I don't know if it is even possible
> to 'downgrade' my cygwin installation in order to confirm that the current
> git-cat-file would work with the 2.7.0-1 version of the cygwin dll).

I believe (although I've never done it myself) you can get old
versions of Cygwin packages via the Cygwin Time Machine at
http://www.crouchingtigerhiddenfruitbat.org/Cygwin/timemachine.html.

I'm sufficiently over-committed at the moment that I'm unlikely to be
able to spend time investigating these problems myself, but I'm happy
to test patches  on my local installation if that would be valuable.

Adam

Re: [PATCH 00/26] Address a couple of issues identified by Coverity

[Johannes Schindelin]

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

> Am 26.04.2017 um 22:19 schrieb Johannes Schindelin:
> > I recently registered the git-for-windows fork with Coverity to ensure
> > that even the Windows-specific patches get some static analysis love.
> >
> > While at it, I squashed a couple of obvious issues in the part that is
> > not Windows-specific.
> 
> Thanks for the fish. I'd looked at the series and had a few comments.
> 
> Hunting memory leaks is the way to insanity.

I hear you. Loud and clear.

> Never again am I going to help with this.

Awww? And here I thought I had your attention... *sniffle*

;-)

> I prefer to rewrite this codebase to C++ and have leak-free code by
> design.

I had the pleasure of working with some software developers in 2004 who
were experts at introducing memory leaks into C++ code.

The same bunch of people later produced Java code that sorted a string
list in cubic time (carefully avoiding java.util.Collections.sort(), of
course).

For every fool-proof system invented, somebody invents a better fool.

Ciao,
Dscho

Re: [PATCH 22/26] add_reflog_for_walk: avoid memory leak

[Johannes Schindelin]

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

> Am 26.04.2017 um 22:21 schrieb Johannes Schindelin:
> > We free()d the `log` buffer when dwim_log() returned 1, but not when it
> > returned a larger value (which meant that it still allocated the buffer
> > but we simply ignored it).
> >
> > Identified by Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  reflog-walk.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/reflog-walk.c b/reflog-walk.c
> > index 99679f58255..ec66f2b16e6 100644
> > --- a/reflog-walk.c
> > +++ b/reflog-walk.c
> > @@ -183,7 +183,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
> >if (!reflogs || reflogs->nr == 0) {
> > struct object_id oid;
> > char *b;
> > -   if (dwim_log(branch, strlen(branch), oid.hash, ) ==
> > 1) {
> > +   int ret = dwim_log(branch, strlen(branch),
> > +  oid.hash, );
> > +   if (ret > 1)
> > +   free(b);
> > +   else if (ret == 1) {
> >  if (reflogs) {
> >   free(reflogs->ref);
> >   free(reflogs);
> >
> 
> Right after this hunk, there is another conditional that looks like it
> forgets to free reflogs.

Thanks! Seems I got too hung up with the line to which Coverity pointed
and failed to see the bigger picture.

It seems that there are plenty of leaks, even further down. For one, the
`branch` variable is not released at the very end of the function! One
might think that read_complete_reflogs() takes custody of it, but no, it
xstrdup()s the refname right at the beginning.

So there was a lot more memory leaking going on in that function...

Ciao,
Dscho

Re: [PATCH 10/26] Check for EOF while parsing mails

[Jeff King]

On Fri, Apr 28, 2017 at 12:41:02PM +0200, Johannes Schindelin wrote:

> > Why? isspace(EOF) is well-defined.
> 
> So let's look at the man page on Linux:
> 
>   These functions check whether c,  which  must  have  the  value  of  an
>   unsigned char or EOF, [...]
> 
> That is the only mention of it. I find it highly unobvious whether EOF
> should be treated as a space or not. So let's look at the MSDN page
> (https://msdn.microsoft.com/en-us/library/y13z34da.aspx) whether they talk
> more about EOF:
> 
>   The behavior of isspace and _isspace_l is undefined if c is not
>   EOF or in the range 0 through 0xFF, inclusive.
> 
> That's it. So I kind of *guess* that EOF is treated as not being a
> whitespace character (why does this make me think of politics now? Focus,
> Johannes, focus...). But the mathematician in me protests: why would we
> be able to decide the character class of a character that does not exist?

This behavior actually comes from the C standard. From C99, 7.4:

  1 The header  declares several functions useful for
classifying and mapping characters. 166) In all cases the argument
is an int, the value of which shall be representable as an unsigned
char or shall equal the value of the macro EOF. If the argument has
any other value, the behavior is undefined.

It doesn't explicitly say that EOF returns false for these functions,
but it does seem implied. For instance:

  The isspace function tests for any character that is a standard
  white-space character or is one of a locale-specific set of characters
  for which isalnum is false. The standard white-space characters are
  the following: space (' '), form feed ('\f'), new-line ('\n'),
  carriage return ('\r'), horizontal tab ('\t'), and vertical tab
  ('\v'). In the "C" locale, isspace returns true only for the standard
  white-space characters.

So I think it is a reasonable thing to depend on.  But as I said
elsewhere in the thread, we don't actually use the standard isspace()
anyway.

> But then, I guess I misunderstood what Coverity complained about: maybe
> the problem was not so much the isspace() call but that EOF is not being
> handled correctly. We pass it, unchecked, to ungetc().
> 
> It appears that I (or Coverity, if you will), missed another instance
> where we simply passed EOF unchecked to ungetc().

I think that is also fine according to the standard.

Do you happen to have the exact error from Coverity? I'm wondering if it
is complaining about some aspect of our custom isspace() when used with
EOF.

> The next iteration will have it completely reworked: I no longer guard the
> isspace() behind an `!= EOF` check, but rather handle an early EOF as I
> think it should be handled. Extra eyes very welcome (this is the fixup!
> patch):

I do think handling EOF explicitly is probably a better strategy anyway,
as it lets us tell when we have an empty patch.

-Peff

Re: [PATCH 10/26] Check for EOF while parsing mails

[Jeff King]

On Fri, Apr 28, 2017 at 12:44:52PM +0200, Johannes Schindelin wrote:

> > Also, what is the behavior of ungetc when we pass it EOF?
> 
> According to the documentation, it would cast EOF to an unsigned char and
> push that back. Definitely incorrect.
> 
> > It looks like POSIX does what we want (pushing EOF is a noop, and the
> > stream retains its feof() status), but I don't know if there are other
> > implementations to worry about.
> 
> That's not what my man page here says:
> 
>   ungetc()  pushes  c  back to stream, cast to unsigned char, where
>   it is available for subsequent read operations.  Pushed-back
>   characters will be returned in reverse order; only one pushback is
>   guaranteed.

POSIX covers this this case explicitly:

  If the value of c equals that of the macro EOF, the operation shall
  fail and the input stream shall be left unchanged.

That comes straight from C99, which says:

  If the value of c equals that of the macro EOF, the operation fails
  and the input stream is unchanged.

I don't have a copy of C89 handy, but I didn't see any mention of the
behavior in the "changes from the previous edition" section of C99.

So it's possible that there's an implementation that is unhappy with
ungetc(EOF), but unless we know of one specifically, it seems pretty
safe. Given that and the similar explicit rule for EOF via isspace(), I
think the original code actually behaves fine.

Of course, we do not use the standard isspace() anyway. Our
implementation will cast the EOF to an unsigned char. If it's "-1", that
ends up as 255, which matches no classes. But if the platform has an
oddball EOF like 288, that would confuse our isspace().

-Peff

Re: [PATCH 20/26] line-log: avoid memory leak

[Johannes Schindelin]

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

> Am 26.04.2017 um 22:21 schrieb Johannes Schindelin:
> > Discovered by Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  line-log.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/line-log.c b/line-log.c
> > index a23b910471b..19d46e9ea2c 100644
> > --- a/line-log.c
> > +++ b/line-log.c
> > @@ -1125,6 +1125,7 @@ static int process_ranges_ordinary_commit(struct
> > rev_info *rev, struct commit *c
> >   changed = process_all_files(_range, rev, , range);
> >   if (parent)
> > add_line_range(rev, parent, parent_range);
> > +   free(parent_range);
> > return changed;
> >  }
> >
> >
> 
> parent_range is of type struct line_log_data *, which needs more than a mere
> free(). I think it's free_line_log_data(parent_range).

Oh wow, thanks for pointing that out. Will be fixed in the next iteration.

Ciao,
Dscho

Re: [PATCH 18/26] fast-export: avoid leaking memory in handle_tag()

[Johannes Schindelin]

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

> Am 26.04.2017 um 22:21 schrieb Johannes Schindelin:
> > Reported by, you guessed it, Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/fast-export.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> > index e0220630d00..828d41c0c11 100644
> > --- a/builtin/fast-export.c
> > +++ b/builtin/fast-export.c
> > @@ -765,6 +765,7 @@ static void handle_tag(const char *name, struct tag
> > *tag)
> >  (int)(tagger_end - tagger), tagger,
> >  tagger == tagger_end ? "" : "\n",
> >  (int)message_size, (int)message_size, message ? message : "");
> > +   free(buf);
> >  }
> >
> >  static struct commit *get_commit(struct rev_cmdline_entry *e, char
> >  *full_name)
> >
> 
> There is an early return in the function that is not covered by this patch.

Thanks!

> Look for "case DROP".

Or for "return" ;-)

I briefly looked into simply releasing the memory earlier, but the tagger
variable used just before the inserted free(buf) actually points into the
buffer, so I had to repeat the free(buf) for the early return.

Thank you!
Dscho

Re: [PATCH 12/26] checkout: fix memory leak

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Discovered via Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/checkout.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/builtin/checkout.c b/builtin/checkout.c
> > index bfa5419f335..98f98256608 100644
> > --- a/builtin/checkout.c
> > +++ b/builtin/checkout.c
> > @@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct 
> > checkout *state)
> > if (!ce)
> > die(_("make_cache_entry failed for path '%s'"), path);
> > status = checkout_entry(ce, state, NULL);
> > +   free(ce);
> > return status;
> >  }
> 
> Thanks for spotting this one and fixing it.  
> 
> In case anybody is wondering what the "only to leak" comment before
> this part of the code is about (which by the way may need to be
> updated, as the bulk of its reasoning still applies but at least we
> are no longer leaking with this patch), back when this code was
> written in 2008 or so it wasn't kosher to free cache_entry under
> certain conditions, but it has been a long time since it became OK
> to free any cache entries in later 2011---we should have done this a
> long time ago.

Thanks for the background. The next iteration will change that comment,
too (simply removing "just to leak" and rewrapping to 74 columns/line.

Ciao,
Dscho

Re: [PATCH 10/26] Check for EOF while parsing mails

[Johannes Schindelin]

Hi Peff,

On Thu, 27 Apr 2017, Jeff King wrote:

> On Wed, Apr 26, 2017 at 10:20:16PM +0200, Johannes Schindelin wrote:
> 
> > diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> > index 30681681c13..c0d88f97512 100644
> > --- a/builtin/mailsplit.c
> > +++ b/builtin/mailsplit.c
> > @@ -232,7 +232,7 @@ static int split_mbox(const char *file, const char 
> > *dir, int allow_bare,
> >  
> > do {
> > peek = fgetc(f);
> > -   } while (isspace(peek));
> > +   } while (peek >= 0 && isspace(peek));
> > ungetc(peek, f);
> 
> Are we guaranteed that EOF is a negative number?

No, you're right.

> Also, what is the behavior of ungetc when we pass it EOF?

According to the documentation, it would cast EOF to an unsigned char and
push that back. Definitely incorrect.

> It looks like POSIX does what we want (pushing EOF is a noop, and the
> stream retains its feof() status), but I don't know if there are other
> implementations to worry about.

That's not what my man page here says:

ungetc()  pushes  c  back to stream, cast to unsigned char, where
it is available for subsequent read operations.  Pushed-back
characters will be returned in reverse order; only one pushback is
guaranteed.

> Perhaps:
> 
>   /* soak up whitespace */
>   while ((peek = fgetc(f)) != EOF) {
>   if (!isspace(peek)) {
>   ungetc(peek, f);
>   break;
>   }
>   }
> 
> would be more portable.

True. I changed it slightly differently, please see my reply to Hannes.

Thanks,
Dscho

Re: [PATCH 10/26] Check for EOF while parsing mails

[Johannes Schindelin]

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

> Am 26.04.2017 um 22:20 schrieb Johannes Schindelin:
>
> > diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
> > index 30681681c13..c0d88f97512 100644
> > --- a/builtin/mailsplit.c
> > +++ b/builtin/mailsplit.c
> > @@ -232,7 +232,7 @@ static int split_mbox(const char *file, const char *dir,
> > int allow_bare,
> >
> >   do {
> > peek = fgetc(f);
> > -   } while (isspace(peek));
> > +   } while (peek >= 0 && isspace(peek));
> >   ungetc(peek, f);
> >
> > if (strbuf_getwholeline(, f, '\n')) {
> > diff --git a/mailinfo.c b/mailinfo.c
> > index 68037758f2f..60dcad7b714 100644
> > --- a/mailinfo.c
> > +++ b/mailinfo.c
> > @@ -1099,7 +1099,7 @@ int mailinfo(struct mailinfo *mi, const char *msg,
> > const char *patch)
> >
> >   do {
> > peek = fgetc(mi->input);
> > -   } while (isspace(peek));
> > +   } while (peek >= 0 && isspace(peek));
> >   ungetc(peek, mi->input);
> >
> >   /* process the email header */
> >
> 
> Why? isspace(EOF) is well-defined.

So let's look at the man page on Linux:

These functions check whether c,  which  must  have  the  value  of  an
unsigned char or EOF, [...]

That is the only mention of it. I find it highly unobvious whether EOF
should be treated as a space or not. So let's look at the MSDN page
(https://msdn.microsoft.com/en-us/library/y13z34da.aspx) whether they talk
more about EOF:

The behavior of isspace and _isspace_l is undefined if c is not
EOF or in the range 0 through 0xFF, inclusive.

That's it. So I kind of *guess* that EOF is treated as not being a
whitespace character (why does this make me think of politics now? Focus,
Johannes, focus...). But the mathematician in me protests: why would we
be able to decide the character class of a character that does not exist?

Technically, you are correct, of course. The specs of fgetc() specify
quite clearly that either an unsigned char cast to an int is returned, or
EOF on end-of-file *or error*. And a quick test verifies that isspace(EOF)
returns 0.

But then, I guess I misunderstood what Coverity complained about: maybe
the problem was not so much the isspace() call but that EOF is not being
handled correctly. We pass it, unchecked, to ungetc().

It appears that I (or Coverity, if you will), missed another instance
where we simply passed EOF unchecked to ungetc().

The next iteration will have it completely reworked: I no longer guard the
isspace() behind an `!= EOF` check, but rather handle an early EOF as I
think it should be handled. Extra eyes very welcome (this is the fixup!
patch):

-- snip --
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index c0d88f97512..9b3efc8e987 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -232,8 +232,9 @@ static int split_mbox(const char *file, const char *dir, 
int allow_bare,
 
do {
peek = fgetc(f);
-   } while (peek >= 0 && isspace(peek));
-   ungetc(peek, f);
+   } while (isspace(peek));
+   if (peek != EOF)
+   ungetc(peek, f);
 
if (strbuf_getwholeline(, f, '\n')) {
/* empty stdin is OK */
diff --git a/mailinfo.c b/mailinfo.c
index 60dcad7b714..a319911b510 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -882,7 +882,10 @@ static int read_one_header_line(struct strbuf *line, FILE 
*in)
for (;;) {
int peek;
 
-   peek = fgetc(in); ungetc(peek, in);
+   peek = fgetc(in);
+   if (peek == EOF)
+   break;
+   ungetc(peek, in);
if (peek != ' ' && peek != '\t')
break;
if (strbuf_getline_lf(, in))
@@ -1094,14 +1097,18 @@ int mailinfo(struct mailinfo *mi, const char *msg, 
const char *patch)
return -1;
}
 
-   mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
-   mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
-
do {
peek = fgetc(mi->input);
-   } while (peek >= 0 && isspace(peek));
+   if (peek == EOF) {
+   fclose(cmitmsg);
+   return error("empty patch: '%s'", patch);
+   }
+   } while (isspace(peek));
ungetc(peek, mi->input);
 
+   mi->p_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->p_hdr_data)));
+   mi->s_hdr_data = xcalloc(MAX_HDR_PARSED, sizeof(*(mi->s_hdr_data)));
+
/* process the email header */
while (read_one_header_line(, mi->input))
check_header(mi, , mi->p_hdr_data, 1);
-- snap --

In the first hunk, I simply rely on the code after ungetc() to figure out
that there are no headers and to handle that case as before.

The second hunk handles the case where looking for a continuation line in
the header section hits EOF; it is still a valid header, but we should
avoid ungetc(EOF) to allow the next read to report 

Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper

[Phillip Wood]

On 26/04/17 12:59, Johannes Schindelin wrote:
> The first step of an interactive rebase is to generate the so-called "todo
> script", to be stored in the state directory as "git-rebase-todo" and to
> be edited by the user.
> 
> Originally, we adjusted the output of `git log ` using a simple
> sed script. Over the course of the years, the code became more
> complicated. We now use shell scripting to edit the output of `git log`
> conditionally, depending whether to keep "empty" commits (i.e. commits
> that do not change any files).
> 
> On platforms where shell scripting is not native, this can be a serious
> drag. And it opens the door for incompatibilities between platforms when
> it comes to shell scripting or to Unix-y commands.
> 
> Let's just re-implement the todo script generation in plain C, using the
> revision machinery directly.
> 
> This is substantially faster, improving the speed relative to the
> shell script version of the interactive rebase from 2x to 3x on Windows.
> 
> Note that the rearrange_squash() function in git-rebase--interactive
> relied on the fact that we set the "format" variable to the config setting
> rebase.instructionFormat. Relying on a side effect like this is no good,
> hence we explicitly perform that assignment (possibly again) in
> rearrange_squash().
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/rebase--helper.c   |  8 +++-
>  git-rebase--interactive.sh | 44 +++-
>  sequencer.c| 45 +
>  sequencer.h|  3 +++
>  4 files changed, 78 insertions(+), 22 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 77afecaebf0..e858a976279 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2388,3 +2388,48 @@ void append_signoff(struct strbuf *msgbuf, int 
> ignore_footer, unsigned flag)
>  
>   strbuf_release();
>  }
> +
> +int sequencer_make_script(int keep_empty, FILE *out,
> + int argc, const char **argv)
> +{
> + char *format = xstrdup("%s");
> + struct pretty_print_context pp = {0};
> + struct strbuf buf = STRBUF_INIT;
> + struct rev_info revs;
> + struct commit *commit;
> +
> + init_revisions(, NULL);
> + revs.verbose_header = 1;
> + revs.max_parents = 1;
> + revs.cherry_pick = 1;
> + revs.limited = 1;
> + revs.reverse = 1;
> + revs.right_only = 1;
> + revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> + revs.topo_order = 1;
> +
> + revs.pretty_given = 1;
> + git_config_get_string("rebase.instructionFormat", );

Firstly thanks for all your work on speeding up rebase -i, it definitely
feels faster.

This changes the behaviour of
git -c rebase.instructionFormat= rebase -i
The shell version treats the rebase.instructionFormat being unset or set
to the empty string as equivalent. This version generates a todo list
with lines like 'pick ' rather than 'pick 
'

I only picked this up because I have a script that does 'git -c
rebase.instructionFormat= rebase -i' with a custom sequence editor. I
can easily add '%s' in the appropriate place but I thought I'd point it
out in case other people are affected by the change.

Please CC me in any replies as I'm not subscribed to this list

Best Wishes

Phillip

> + get_commit_format(format, );
> + free(format);
> + pp.fmt = revs.commit_format;
> + pp.output_encoding = get_log_output_encoding();
> +
> + if (setup_revisions(argc, argv, , NULL) > 1)
> + return error(_("make_script: unhandled options"));
> +
> + if (prepare_revision_walk() < 0)
> + return error(_("make_script: error preparing revisions"));
> +
> + while ((commit = get_revision())) {
> + strbuf_reset();
> + if (!keep_empty && is_original_commit_empty(commit))
> + strbuf_addf(, "%c ", comment_line_char);
> + strbuf_addf(, "pick %s ", oid_to_hex(>object.oid));
> + pretty_print_commit(, commit, );
> + strbuf_addch(, '\n');
> + fputs(buf.buf, out);
> + }
> + strbuf_release();
> + return 0;
> +}
> diff --git a/sequencer.h b/sequencer.h
> index f885b68395f..83f2943b7a9 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -45,6 +45,9 @@ int sequencer_continue(struct replay_opts *opts);
>  int sequencer_rollback(struct replay_opts *opts);
>  int sequencer_remove_state(struct replay_opts *opts);
>  
> +int sequencer_make_script(int keep_empty, FILE *out,
> + int argc, const char **argv);
> +
>  extern const char sign_off_header[];
>  
>  void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
> 

HELLO

[Mr Faisal Samae]

I am Mr. Faisal Samae Director of the company in Burkina Faso, I needed your 
assistant to do business with me and we shall aim a good profit at the end of 
the successful business, please i want you to put all your mind/ trust and 
effort to accomplish this deal, i belief Allah will help us all, Inshal allah.

my good assistance, i am introducing you in this business with trust and not 
betray mind, i want you to help me claim the sum fund that worth about 
$25,000,000.00 USD and this fund is in the securitycompany here in Burkina Faso 
register as a family treasure and also luck with a secret CODE whereby no one 
can open the box except the beneficiary.

I am contacting you to help me and claim the fund, it belong to LATE GHADAFFI 
and i have all information about the saving fund in a security company, please 
get back to me so that i can direct you what to do, the money will be shared in 
ratio of 60% for me and 40% will be for you

Please indicate your wiliness if you are capable to handle this deal with 
sincerity.


with the below information for more details:

Your full names
Your country of origin
Your Mobile N..

i wait your respond.

Best Regards
Mr.Faisal Samae
Former Director of the Company
Burkina Faso

Re: [PATCH 06/26] get_mail_commit_oid(): avoid resource leak

[Johannes Schindelin]

Hi Hannes,

On Thu, 27 Apr 2017, Johannes Sixt wrote:

> Am 26.04.2017 um 22:19 schrieb Johannes Schindelin:
> >
> > diff --git a/builtin/am.c b/builtin/am.c
> > index 805f56cec2f..01b700e5e74 100644
> > --- a/builtin/am.c
> > +++ b/builtin/am.c
> > @@ -1359,15 +1359,16 @@ static int get_mail_commit_oid(struct object_id
> > *commit_id, const char *mail)
> >   struct strbuf sb = STRBUF_INIT;
> >   FILE *fp = xfopen(mail, "r");
> >   const char *x;
> > +   int ret = 0;
> >
> > if (strbuf_getline_lf(, fp))
> > -   return -1;
> > +   ret = -1;
> >
> > -   if (!skip_prefix(sb.buf, "From ", ))
> > -   return -1;
> > +   if (!ret && !skip_prefix(sb.buf, "From ", ))
> > +   ret = -1;
> >
> > -   if (get_oid_hex(x, commit_id) < 0)
> > -   return -1;
> > +   if (!ret && get_oid_hex(x, commit_id) < 0)
> > +   ret = -1;
> >
> >   strbuf_release();
> >   fclose(fp);
> >
> 
> You forgot to 'return ret;', didn't you?

I sure did!

Thanks,
Dscho

Re: [PATCH 11/26] cat-file: fix memory leak

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Discovered by Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/cat-file.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 1890d7a6390..9af863e7915 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, 
> > const char *obj_name,
> > die("git cat-file %s: bad file", obj_name);
> >  
> > write_or_die(1, buf, size);
> > +   free(buf);
> > return 0;
> >  }
> 
> This is a border-line "Meh".  Just like we do not free resources
> immediately before calling die(), we can leave this as-is as the
> only thing that happens after this is a return from cmd_cat_file()
> back to main() that exits.

If you are mostly concerned about the status quo, that is true.

I am a lot more concerned with future changes, where we may easily decide
that it is time to move a file-local function out of its hiding place and
make it more usable.

>From that perspective, it is one thing to have a blatant memory leak in a
cmd_*() function, and it is an entirely different matter to have such a
leak in a function that happens to be called only from cmd_*() functions:
somebody familiar enough with Git's coding conventions (such as myself)
will *expect* cmd_*() to have leaks left and right and pay attention when
libifying that code, but be a lot less concerned about such leaks in other
functions.

And of course this concerns me more than you, as I am still trying to
drive forward the effort to convert more scripts into builtins.

So on my own behalf: thank you for accepting this patch.

Ciao,
Dscho

Re: [PATCH 10/26] Check for EOF while parsing mails

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Reported via Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  builtin/mailsplit.c | 2 +-
> >  mailinfo.c  | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> Good find.  I'd retitle with a prefix
> 
>   mailinfo & mailsplit: check for EOF while parsing
> 
> so that it is clear that this patch is about lower level machinery
> (as oppose to something like "git am").

True. Will be fixed in the next iteration,
Dscho

Re: [PATCH 08/26] difftool: close file descriptors after reading

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> > diff --git a/builtin/difftool.c b/builtin/difftool.c
> > index 1354d0e4625..a4f1d117ef6 100644
> > --- a/builtin/difftool.c
> > +++ b/builtin/difftool.c
> > @@ -497,6 +498,7 @@ static int run_dir_diff(const char *extcmd, int 
> > symlinks, const char *prefix,
> > }
> > }
> >  
> > +   fclose(fp);
> 
> The huge loop we see in the pre-context of this hunk has many
> "return"s and "goto finish"es that can leave fp still open; while
> this patch does not hurt, it is probably somewhat insufficient.

You are absolutely correct, and I am somewhat surprised that Coverity did
not complain more loudly.

TBH I really only tried to address these fixes on the cheap, as my main
aim was to get to the critical bugs in the Windows-specific part (I did
not find anything major, though). Therefore, I only looked at what
Coverity labels as the "high impact" issues. The leaks in the loop that
you pointed out may be labeled as minor by Coverity.

I also noticed that a couple of error messages have not been marked as
translateable, I must have forgotten that before submitting the difftool
patch series :-(

In any case, in the next iteration of this patch series, this patch will
convert all returns to `ret = ...; goto finish;` calls, and fclose(fp)
there unless it has been closed already.

Thanks,
Dscho

Re: [PATCH 07/26] http-backend: avoid memory leaks

[Johannes Schindelin]

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Reported via Coverity.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  http-backend.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/http-backend.c b/http-backend.c
> > index eef0a361f4f..d12572fda10 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -681,8 +681,10 @@ int cmd_main(int argc, const char **argv)
> > if (!regexec(, dir, 1, out, 0)) {
> > size_t n;
> >  
> > -   if (strcmp(method, c->method))
> > +   if (strcmp(method, c->method)) {
> > +   free(dir);
> > return bad_request(, c);
> > +   }
> >  
> > cmd = c;
> > n = out[0].rm_eo - out[0].rm_so;
> > @@ -708,5 +710,7 @@ int cmd_main(int argc, const char **argv)
> >max_request_buffer);
> >  
> > cmd->imp(, cmd_arg);
> > +   free(dir);
> > +   free(cmd_arg);
> > return 0;
> >  }
> 
> Hmph.  I find a "leak" of a resource acquired inside the main
> function and not released when the main function leaves a lot less
> interesting than the other ones this series covers.

Ah, I missed that this falls squarely into the "one-shot programs are
allowed to be sloppy in their memory management, essentially using exit()
as garbage collector" category.

Will drop,
Dscho

Git and Active directory ldap Authentication

[Miguel Angel Soriano Morales]

Hi,

Sorry for my English and Thanks for assisting me. I would like use git in my
Company. We use Active directory for everything, but I prefer install git in
centos7. I Would like authenticate all my user in Git through Active
Directory. And Every Project had ACL permissions .It this posible? Can you
help me?

Thanks a lot and good weekend

Re: [PATCH v3 3/5] tests: rename a test having to do with shallow submodules

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 28, 2017 at 12:13 AM, Brandon Williams  wrote:
> On 04/27, Stefan Beller wrote:
>> On Wed, Apr 26, 2017 at 4:12 PM, Ævar Arnfjörð Bjarmason
>>  wrote:
>> > Rename the t5614-clone-submodules.sh test to
>> > t5614-clone-submodules-shallow.sh. It's not a general test of
>> > submodules, but of shallow cloning in relation to submodules. Move it
>> > to create another similar t56*-clone-submodules-*.sh test.
>> >
>> > Signed-off-by: Ævar Arnfjörð Bjarmason 
>> > ---
>> >  t/{t5614-clone-submodules.sh => t5614-clone-submodules-shallow.sh} | 0
>> >  1 file changed, 0 insertions(+), 0 deletions(-)
>> >  rename t/{t5614-clone-submodules.sh => t5614-clone-submodules-shallow.sh} 
>> > (100%)
>> >
>> > diff --git a/t/t5614-clone-submodules.sh 
>> > b/t/t5614-clone-submodules-shallow.sh
>> > similarity index 100%
>> > rename from t/t5614-clone-submodules.sh
>> > rename to t/t5614-clone-submodules-shallow.sh
>>
>> Thanks for formatting the patches with rename detection. :)
>> The rename looks good.
>
> Do you have to turn that on or is that on by default?

Looks like it just uses the diff.renames setting which I don't tweak,
I didn't do anything special, but maybe it picked up some part of my
.gitconfig that doesn't look like it has anything to do with
renames...