Re: [PATCH] t7400: add BSLASHPSPEC prerequisite to 'add with \\ in path'
Am 29.04.2017 um 02:15 schrieb 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)! I don't observe these failures. Are you using a vanila MSYS2 environment? The exact failure modes would be interesting, if you want to hear "Ah, Git for Windows does this and that to make this work". ;) 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 OK: git init calls mkdir("sub\\with\\backslash"), which does not create the missing directories automatically. Therefore, this mkdir helps. In the next call, the OS functions behind mkdir simply take the backslashes as directory separators: 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 MSYS git does not know that the backslash is a directory separator. Hence, it constructs a path with only a single .. component thinking that this walks above sub\with\backslash to end up in parent/; but the underlying OS operation interprets the backslashes as directory separator and ends up in parent/sub\with\. Of coures, no .git directory is at this point, hence the failure. MinGW git, however, knows about the ba
Re: [PATCH 6/6] submodule: refactor logic to determine changed submodules
+ 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(&cp.env_array); > - cp.git_cmd = 1; > - cp.no_stdin = 1; > - cp.dir = path; > - if (!capture_command(&cp, &buf, 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
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, &has_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(&cp.args, "rev-list", "-n", "1", NULL); > + oid_array_for_each_unique(commits, append_oid_to_argv, > &cp.args); > + argv_array_pushl(&cp.args, "--not", "--all", NULL); > + > + prepare_submodule_repo_env(&cp.env_array); > + cp.git_cmd = 1; > + cp.no_stdin = 1; > + cp.dir = path; > + > + if (capture_command(&cp, &out, 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(&cp)) die("cannot start git-rev-list in submodule'%s', sub->path); /* read only one character, if any */ if (xread(cp.out, &tmp, 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(&cp); 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'
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
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, &ref_tips_before_fetch); + for_each_ref(append_oid_to_array, &ref_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
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, &p->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(&rev, NULL); + setup_revisions(argv->argc, argv->argv, &rev, NULL); + if (prepare_revision_walk(&rev)) + die("revision walk setup failed"); + + while ((commit = get_revision(&rev))) { + struct rev_info diff_rev; + + init_revisions(&diff_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, &diff_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
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(&ref_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(&rev, NULL); argv_array_push(&argv, "--"); /* argv[0] program name */ oid_array_for_each_unique(&ref_tips_after_fetch, - add_oid_to_argv, &argv); + append_oid_to_argv, &argv); argv_array_push(&argv, "--not"); oid_array_for_each_unique(&ref_tips_before_fetch, - add_oid_to_argv, &argv); + append_oid_to_argv, &argv); setup_revisions(argv.argc, argv.argv, &rev, NULL); if (prepare_revision_walk(&rev)) die("revision walk setup failed"); -- 2.13.0.rc0.306.g87b477812d-goog
[PATCH 5/6] submodule: improve submodule_has_commits
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, &has_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(&cp.args, "rev-list", "-n", "1", NULL); + oid_array_for_each_unique(commits, append_oid_to_argv, &cp.args); + argv_array_pushl(&cp.args, "--not", "--all", NULL); + + prepare_submodule_repo_env(&cp.env_array); + cp.git_cmd = 1; + cp.no_stdin = 1; + cp.dir = path; + + if (capture_command(&cp, &out, 1024) || out.len) + has_commit = 0; + + strbuf_release(&out); + } + return has_commit; } -- 2.13.0.rc0.306.g87b477812d-goog
[PATCH 2/6] submodule: rename free_submodules_sha1s
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, &rev); } -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(&submodules); + + free_submodules_oids(&submodules); return needs_pushing->nr; } -- 2.13.0.rc0.306.g87b477812d-goog
[PATCH 0/6] changed submodules
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
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(&changed_submodule_paths, p->two->path); if (!path && !is_submodule_commit_present(p->two->path, p->two->oid.hash)) - string_list_append(&changed_submodule_paths, xstrdup(p->two->path)); + string_list_append(&changed_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
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
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(&rev, 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(&rev, 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 spaces-compacted-
[PATCH 3/3] diff: enable indent heuristic by default
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
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.
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.
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.
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
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(&foo[i], &foo[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
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
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", &opts.allow_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", &command, N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS), + OPT_CMDMODE(0, "rearrange-squash", &command, + 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 rebase.instructionF
[PATCH v4 07/10] rebase -i: check for missing commits in the rebase--helper
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", &opts.allow_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", &command, N_("expand SHA-1s in the todo list"), EXPAND_SHA1S), + OPT_CMDMODE(0, "check-todo-list", &command, + 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 r
[PATCH v4 08/10] rebase -i: skip unnecessary picks using the rebase--helper
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", &opts.allow_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", &command, N_("check the todo list"), CHECK_TODO_LIST), + OPT_CMDMODE(0, "skip-unnecessary-picks", &command, + 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 = &onto_oid, *parent_oid; + int fd, i; + + if (!read_oneliner(&buf, rebase_path_onto(), 0)) + return error(_("could not read 'onto'")); + if (get_sha1(buf.buf, onto_oid.hash)) { + strbuf_release(&buf); + return error(_("need a HEAD to fixup")); + } + strbuf_release(&buf);
[PATCH v4 06/10] t3404: relax rebase.missingCommitsCheck tests
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 >expectexpect
[PATCH v4 05/10] rebase -i: also expand/collapse the SHA-1s via the rebase--helper
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", &opts.allow_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", &command, N_("make rebase script"), MAKE_SCRIPT), + OPT_CMDMODE(0, "shorten-sha1s", &command, + N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S), + OPT_CMDMODE(0, "expand-sha1s", &command, + 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(&opts); 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(&buf); 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(&todo_list.buf); + fd = open(todo_file, O_RDONLY); + if (fd < 0) + return error_errno(_("could not open '%s'"), todo_file); + if (strbuf_read(&todo_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, &todo_list); + if (res) { + todo_list_release(&todo_list); + return error(_("unusable instruction sheet: '%s'"), todo_file); + } + + out = fopen(todo_file, "w"); + if (!out) { + todo_list_release(&todo_list); + return error(_("unable to open '%s' for writing"), todo_file); + } + for (i = 0; i < todo_list.nr; i++) { + struct todo_item *item = todo
[PATCH v4 03/10] rebase -i: remove useless indentation
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
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
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
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", &opts.allow_ff, N_("allow fast-forward")), + OPT_BOOL(0, "keep-empty", &keep_empty, N_("keep empty commits")), OPT_CMDMODE(0, "continue", &command, N_("continue rebase"), CONTINUE), OPT_CMDMODE(0, "abort", &command, N_("abort rebase"), ABORT), + OPT_CMDMODE(0, "make-script", &command, + 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(&opts); if (command == ABORT && argc == 1) return !!sequencer_remove_state(&opts); + 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 +
[PATCH v4 00/10] The final building block for a faster rebase -i
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", &format); + if (!format || !*format) { + free(format); + format = xstrdup("%s"); + } get_commit_format(format, &revs); 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(&item->commit->object.oid); + int len; - if (!eoc) { - p += strspn(p, " \t"); - eoc = strc
Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity
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'
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
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
> > 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
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
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
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
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.
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
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, &len); } +#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(&foo[i], &foo[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
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
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
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
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
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
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
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(&sob); > >> > } > >> > + > >> > +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(&revs, 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", &format); > >> > +get_commit_format(format, &revs); > >> > +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
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(&todo_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(&todo_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, &todo_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(&todo_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(&todo_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(&todo_list.buf, fd, 0) < 0) { > > + close(fd); > > + res = error(_("could not read '%s'."), todo_file.buf); > > + goto leave_check; > > + } > > + close(fd); > > + strbuf_release(&todo_file); > > + res = !!parse_insn_buffer(todo_list.buf.buf, &todo_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(&todo_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, &missing); > > + 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
Re: push fails with return code 22
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
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) >> + if (option_single_branch && !option_no_tags
[PATCH v2 21/25] add_reflog_for_walk: avoid memory leak
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, &b) == 1) { + int ret = dwim_log(branch, strlen(branch), + oid.hash, &b); + 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(&info->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
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(&sb, 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(&sb); - free(submodule_gitdir); if (!dir) return 0; -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v2 24/25] show_worktree(): plug memory leak
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(&sb, "(detached HEAD)"); - else if (wt->head_ref) - strbuf_addf(&sb, "[%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(&sb, "[%s]", ref); + free(ref); + } else strbuf_addstr(&sb, "(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
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()
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(&matched_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
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, &head); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v2 18/25] receive-pack: plug memory leak in update()
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 = &cmd->old_oid; struct object_id *new_oid = &cmd->new_oid; @@ -997,6 +998,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) } strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name); + free(namespaced_name); namespaced_name = strbuf_detach(&namespaced_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
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(&parent_range, rev, &queue, 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()
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(&tag->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
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(&p_uq, path, NULL)) die("invalid quoting"); - path = strbuf_detach(&p_uq, NULL); + path = p = strbuf_detach(&p_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 14/25] setup_discovered_git_dir(): help static analysis
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
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
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 12/25] split_commit_in_progress(): fix memory leak
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 10/25] cat-file: fix memory leak
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
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
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(&buf, 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(&continuation, 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(&line, mi->input)) check_header(mi, &line, 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
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(&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
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
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 07/25] difftool: address a couple of resource/memory leaks
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(&diff_files)) die("diff-files did not exit properly"); strbuf_release(&index_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, &loid, src_path, &lstate)) - return error("could not write '%s'", src_path); + if (checkout_path(lmode, &loid, src_path, &lstate)) { + 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(&working_tree_dups, entry); if (!use_wt_file(workdir, dst_path, &roid)) { - if (checkout_path(rmode, &roid, dst_path, &rstate)) - return error("could not write '%s'", -dst_path); + if (checkout_path(rmode, &roid, dst_path, + &rstate)) { + ret = error("could not write '%s'", + dst_path); + goto finish; + } } else if (!is_null_oid(&roid)) { /* * 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, 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, 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(&child)) { 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(&ldir); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v2 06/25] get_mail_commit_oid(): avoid resource leak
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(&sb, fp)) - return -1; - - if (!skip_prefix(sb.buf, "From ", &x)) - return -1; - - if (get_oid_hex(x, commit_id) < 0) - return -1; + if (strbuf_getline_lf(&sb, fp) || + !skip_prefix(sb.buf, "From ", &x) || + get_oid_hex(x, commit_id) < 0) + ret = -1; strbuf_release(&sb); fclose(fp); - return 0; + return ret; } /** -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v2 03/25] winansi: avoid buffer overrun
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), &dummy, NULL); + return; + } /* write directly to console */ WriteConsoleW(console, wbuf, wlen, &dummy, NULL); -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v2 02/25] winansi: avoid use of uninitialized value
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, &mode)) return 0; + sbi.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN | + FOREGROUND_RED; } else if (!GetConsoleScreenBufferInfo(hcon, &sbi)) return 0; -- 2.12.2.windows.2.800.gede8f145e06
[PATCH v2 00/25] Address a couple of issues identified by Coverity
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: 027a3b9
[PATCH v2 01/25] mingw: avoid memory leak when splitting PATH
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
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
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
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: [PATCH 10/26] Check for EOF while parsing mails
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: Git and Active directory ldap Authentication
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,
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
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 &c on my local installation if that would be valuable. Adam
Re: [PATCH 00/26] Address a couple of issues identified by Coverity
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
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, &b) == > > 1) { > > + int ret = dwim_log(branch, strlen(branch), > > + oid.hash, &b); > > + 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
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
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
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(&parent_range, rev, &queue, 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()
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
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
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
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(&buf, 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(&buf, 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(&continuation, 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(&line, mi->input)) check_header(mi, &line, 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 al
Re: [PATCH v3 1/9] rebase -i: generate the script via rebase--helper
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(&sob); > } > + > +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(&revs, 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", &format); 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, &revs); > + free(format); > + pp.fmt = revs.commit_format; > + pp.output_encoding = get_log_output_encoding(); > + > + if (setup_revisions(argc, argv, &revs, NULL) > 1) > + return error(_("make_script: unhandled options")); > + > + if (prepare_revision_walk(&revs) < 0) > + return error(_("make_script: error preparing revisions")); > + > + while ((commit = get_revision(&revs))) { > + strbuf_reset(&buf); > + if (!keep_empty && is_original_commit_empty(commit)) > + strbuf_addf(&buf, "%c ", comment_line_char); > + strbuf_addf(&buf, "pick %s ", oid_to_hex(&commit->object.oid)); > + pretty_print_commit(&pp, commit, &buf); > + strbuf_addch(&buf, '\n'); > + fputs(buf.buf, out); > + } > + strbuf_release(&buf); > + 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
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
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(&sb, fp)) > > - return -1; > > + ret = -1; > > > > - if (!skip_prefix(sb.buf, "From ", &x)) > > - return -1; > > + if (!ret && !skip_prefix(sb.buf, "From ", &x)) > > + 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(&sb); > > fclose(fp); > > > > You forgot to 'return ret;', didn't you? I sure did! Thanks, Dscho
Re: [PATCH 11/26] cat-file: fix memory leak
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
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
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
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(&re, dir, 1, out, 0)) { > > size_t n; > > > > - if (strcmp(method, c->method)) > > + if (strcmp(method, c->method)) { > > + free(dir); > > return bad_request(&hdr, 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(&hdr, 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
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