Re: [PATCH v2] worktree: add --quiet option
On Wed, 15 Aug 2018 at 22:56, Elia Pinto wrote: > Add the '--quiet' option to git worktree, > as for the other git commands. 'add' is the > only command affected by it since all other > commands, except 'list', are currently > silent by default. Thanks for a follow-up. The word "currently" means I can't shake the feeling that Eric has a very good point in [1]: It might make sense to say instead that this is adding a --quiet option _in general_, rather than doing so specifically for 'add'. As an example, if `git worktree move` ever learns to produce some sort of output, then Eric's approach would mean that such a hypothetical `move` is buggy until it learns to respect `--quiet`. With your approach, it would mean that we would get feature requests that `move` should also respect `--quiet` (which we would then need to redefine in the documentation) or that it should learn of a `--quiet-move` (which I do not think is a particularly good UI). Doing such a patch instead would mean tweaking the commit message slightly... Add the '--quiet' option to git worktree, as for the other git commands. Currently, 'add' is the only command affected by it since all other commands, except 'list' obviously, are already silent by default. ... and the documentation slightly ... Suppress feedback messages. It might make sense adding a comment to the documentation about how this currently only affects `add`, but such comments do risk going stale. I am on the fence about whether the documentation needs to say something about `list` -- who in their right mind would expect `list --quiet` to actually suppress the list? And if `list` ever learns to give some feedback messages in addition to the list itself, then we would want `--quiet` to suppress *those*, I guess. Others might disagree violently with this, but I wonder whether it makes sense to add a test to verify that, e.g., `git worktree move --quiet` is quiet. Such a test would demonstrate that this is your intention, i.e., anyone digging into a report on "why does git worktree foo not respect --quiet?" would be 100% convinced that your intention back in 2018 really was to respect it everywhere. It seems wasteful to add such a test for all other modes, but maybe you can identify one which you think has a higher risk of learning to output some feedback in the future. To be clear, it is fine for you to disagree with the above! :-) > } > - > - print_preparing_worktree_line(opts.detach, branch, new_branch, > !!new_branch_force); > + if (!opts.quiet) > + print_preparing_worktree_line(opts.detach, branch, > new_branch, !!new_branch_force); I think that empty line should be kept. Probably not worth a reroll. Good work! [1] https://public-inbox.org/git/capig+cs-b2yl2felrzs+jw-o5frd1g8kqak7j1qx5prp0oj...@mail.gmail.com/ Martin
Re: "Changes not staged for commit" after cloning a repo on macOS
I checked line-ending but didn't think to file names. Thank you so much. On 25/5/1397 AP 1:36 AM, Bryan Turner wrote: Taking a look at the repository's file list on GitHub[1], it shows that this is because HFS and APFS by default are case-insensitive. The file listing shows that there is a "nanoc.gitignore" and "Nanoc.gitignore". On APFS and HFS, those are the same file. As a result, one overwrites the other. This is discussed pretty regularly on the list[2], so you can find more details there. [1]: https://github.com/kevinxucs/Sublime-Gitignore/tree/master/boilerplates [2]: https://public-inbox.org/git/24a09b73-b4d4-4c22-bc1b-41b22cb59...@gmail.com/ is a fairly recent (fairly long) thread about this behavior. -- Hadi Safari
Re: [PATCH 2/2] submodule: munge paths to submodule git directories
Stefan Beller wrote: > Jonathan Nieder wrote: >> All at the cost of recording a little configuration somewhere. If we >> want to decrease the configuration, we can avoid recording it there in >> the easy cases (e.g. when name == gitdirname). That's "just" an >> optimization. > > Sounds good, but gerrit for example would not take advantage of such > optimisation as they have slashes in their submodules. :-( > I wonder if we can optimize further and keep slashes if there is > no conflict (as then name == gitdirname, so it can be optimized). One possibility would be to treat gsub("/", "%2f") as another of the easy cases. [...] >> And then we have the ability later to handle all the edge cases we >> haven't handled yet today: >> >> - sharding when the number of submodules is too large >> - case-insensitive filesystems >> - path name length limits >> - different sets of filesystem-special characters >> >> Sane? > > I'll keep thinking about it. Thanks. > FYI: the reduction in configuration was just sent out. https://public-inbox.org/git/20180816023100.161626-1-sbel...@google.com/ for those following along. Ciao, Jonathan
Re: [PATCH 2/2] submodule: munge paths to submodule git directories
> [...] all good reasons; ship it :-) > All at the cost of recording a little configuration somewhere. If we > want to decrease the configuration, we can avoid recording it there in > the easy cases (e.g. when name == gitdirname). That's "just" an > optimization. Sounds good, but gerrit for example would not take advantage of such optimisation as they have slashes in their submodules. :-( I wonder if we can optimize further and keep slashes if there is no conflict (as then name == gitdirname, so it can be optimized). > And then we have the ability later to handle all the edge cases we > haven't handled yet today: > > - sharding when the number of submodules is too large > - case-insensitive filesystems > - path name length limits > - different sets of filesystem-special characters > > Sane? I'll keep thinking about it. FYI: the reduction in configuration was just sent out. Thanks, Stefan
Re: [PATCH 2/2] submodule: munge paths to submodule git directories
Hi again, Stefan Beller wrote: > On Tue, Aug 14, 2018 at 2:12 PM Jonathan Nieder wrote: >> What if we forbid directory separator characters in the gitdirname? > > Fine with me, but ideally we'd want to allow sharding the > submodules. When you have 1000 submodules > we'd want them not all inside the toplevel "modules/" ? That's a good reason to permit slashes in the gitdirname. If I understood the rest of your reply correctly, your worry was about dangerous gitdirname values in .gitmodules. I never had any wish to read them from there anyway, so this worry hopefully goes away. [...] >> In this proposal, it would only be read from config, not from >> .gitmodules. > > Ah good point. That makes sense. > > Stepping back a bit regarding the config: [...] > Now that we have the submodule.active or even > submodule..active flags, we do not need (b) any more. > So the URL turns into a useless piece of cruft that just is unneeded > and might confuse the user. > > So maybe I'd want to propose a patch that removes > submodule..url from the config once it is cloned. > (I just read up on "submodule sync" again, but that might not > even need special care for this new world) > > And with all that said, I think if we can avoid having the submodules > gitdir in the config, the config would look much cleaner, too. Yes, I understand and agree with this. I should further spell out my motivation with this gitdirname suggestion. The issue that some people have mentioned in this thread is that urlencoding might not be perfect --- it's pretty close to perfect, but it's likely we'll come up with some unanticipated needs later (like sharding) that it doesn't solve. Solving those all right now would not necessarily be wise, since the thing about unanticipated needs is that you never know in advance what they will be. ;-) So it would be nice, for future-proofing, if we can change the naming scheme later. As a bonus, that would also make interoperability with other implementations easier. For example, suppose we mess up in JGit and urlencode a different set of characters than Git does. Then a mixed Git + JGit installation would have this subtle bug of the submodule .git directory not being reused when I switch to and from and branch not containing that submodule, in some circumstances. That sounds difficult to support. Whereas if we have a gitdirname configuration variable, then JGit and libgit2 and go-git do not have to match the naming scheme Git chooses. They can try, but if one gets it subtly wrong then that is okay because the submodule's directory name is right there and easy to look up. All at the cost of recording a little configuration somewhere. If we want to decrease the configuration, we can avoid recording it there in the easy cases (e.g. when name == gitdirname). That's "just" an optimization. And then we have the ability later to handle all the edge cases we haven't handled yet today: - sharding when the number of submodules is too large - case-insensitive filesystems - path name length limits - different sets of filesystem-special characters Sane? Thanks, Jonathan
Re: What's cooking in git.git (Aug 2018, #03; Wed, 15)
> > * sb/config-write-fix (2018-08-08) 3 commits > - git-config: document accidental multi-line setting in deprecated syntax > - config: fix case sensitive subsection names on writing > - t1300: document current behavior of setting options > > Recent update to "git config" broke updating variable in a > subsection, which has been corrected. > > Expecting a reroll. > cf. That reroll happened and you picked it up, cf. https://public-inbox.org/git/20180808195020.37374-1-sbel...@google.com/
[PATCH 6/7] submodule--helper, update_clone: store index to update_clone instead of ce
In update_submodules, we use the run_processes_parallel(get_task, finished) API, which allows to pass around a task specific callback cookie from the get_next function to the finish function. That finish function in turn may alter generic callback cookie to have the next call of get_task come up with another new task. Up to now we passed around the index into a list of cache entries, which was stored in the generic callback cookie which is a struct submodule_update_clone. Change this to an index into 'update_clone' array, which is the potential output of this helper. This will allow for a future change to make use of the data the update_clone_data struct. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 31 +++ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 1c9a12781fd..36de64902ec 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1511,8 +1511,10 @@ static int module_update_module_mode(int argc, const char **argv, const char *pr struct update_clone_data { const struct submodule *sub; + const struct cache_entry *ce; struct object_id oid; unsigned just_cloned; + unsigned retried; }; struct submodule_update_clone { @@ -1541,8 +1543,8 @@ struct submodule_update_clone { /* If we want to stop as fast as possible and return an error */ unsigned quickstop : 1; - /* failed clones to be retried again */ - const struct cache_entry **failed_clones; + /* failed clones to be retried again, indexes into update_clone */ + int *failed_clones; int failed_clones_nr, failed_clones_alloc; int max_jobs; @@ -1649,6 +1651,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, oidcpy(>update_clone[suc->update_clone_nr].oid, >oid); suc->update_clone[suc->update_clone_nr].just_cloned = needs_cloning; suc->update_clone[suc->update_clone_nr].sub = sub; + suc->update_clone[suc->update_clone_nr].retried = 0; + suc->update_clone[suc->update_clone_nr].ce = ce; suc->update_clone_nr++; if (!needs_cloning) @@ -1707,7 +1711,8 @@ static int update_clone_get_next_task(struct child_process *child, for (; suc->current < suc->list.nr; suc->current++) { ce = suc->list.entries[suc->current]; if (prepare_to_clone_next_submodule(ce, child, suc, err)) { - *idx_task_cb = update_clone_alloc_cb(suc->current); + *idx_task_cb = update_clone_alloc_cb( + suc->update_clone_nr - 1); suc->current++; return 1; } @@ -1720,7 +1725,9 @@ static int update_clone_get_next_task(struct child_process *child, */ index = suc->current - suc->list.nr; if (index < suc->failed_clones_nr) { - ce = suc->failed_clones[index]; + int ucd_index = suc->failed_clones[index]; + struct update_clone_data *ucd = >update_clone[ucd_index]; + ce = ucd->ce; if (!prepare_to_clone_next_submodule(ce, child, suc, err)) { suc->current ++; strbuf_addstr(err, "BUG: submodule considered for " @@ -1728,7 +1735,7 @@ static int update_clone_get_next_task(struct child_process *child, "any more?\n"); return 0; } - *idx_task_cb = update_clone_alloc_cb(suc->current); + *idx_task_cb = update_clone_alloc_cb(ucd_index); suc->current ++; return 1; } @@ -1750,31 +1757,31 @@ static int update_clone_task_finished(int result, void *suc_cb, void *idx_task_cb) { - const struct cache_entry *ce; struct submodule_update_clone *suc = suc_cb; + struct update_clone_data *ucd; int *idxP = idx_task_cb; int idx = *idxP; + ucd = >update_clone[idx]; free(idxP); if (!result) return 0; - if (idx < suc->list.nr) { - ce = suc->list.entries[idx]; + if (!ucd->retried) { + ucd->retried = 1; strbuf_addf(err, _("Failed to clone '%s'. Retry scheduled"), - ce->name); + ucd->ce->name); strbuf_addch(err, '\n'); ALLOC_GROW(suc->failed_clones, suc->failed_clones_nr + 1, suc->failed_clones_alloc); - suc->failed_clones[suc->failed_clones_nr++] = ce; + suc->failed_clones[suc->failed_clones_nr++] = idx; return 0; } else { idx -=
[RFC PATCH 0/7] Unset the submodule URL in the superproject when no longer needed
This is available as git fetch //github.com/stefanbeller/git unsetsubmoduleurl and was hinted at in https://public-inbox.org/git/cagz79kyfok9hfxm2-vmazlppqbofqyktyyuyjb8twzz6oz5...@mail.gmail.com/ Originally we have had the url in the config, (a) that we can change the URLs after the "git submodule init" and "git submodule update" step that actually clones the submodule if not present and much more importantly (b) to know which submodule "was initialized/active". Now that we have the submodule.active or even submodule..active flags, we do not need (b) any more. So the URL turns into a useless piece of cruft that just is unneeded and might confuse the user. Opinions? Thanks, Stefan Stefan Beller (7): t7410: update to new style builtin/submodule--helper: remove stray new line submodule: is_submodule_active to differentiate between new and old mode submodule sync: omit setting submodule URL in config if possible submodule--helper: factor out allocation of callback cookie submodule--helper, update_clone: store index to update_clone instead of ce builtin/submodule--helper: unset submodule url if possible builtin/submodule--helper.c | 82 ++ submodule.c | 5 +- submodule.h | 6 ++ t/t5526-fetch-submodules.sh | 2 +- t/t7406-submodule-update.sh | 8 +++ t/t7410-submodule-checkout-to.sh | 99 +++- 6 files changed, 131 insertions(+), 71 deletions(-) -- 2.18.0.265.g16de1b435c9.dirty
[PATCH 4/7] submodule sync: omit setting submodule URL in config if possible
We do not need to update the submodule url in the superprojects config if the url is not used to keep the submodule active. Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 2f20bd4abdc..639d0bb20a1 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -922,8 +922,10 @@ static void sync_submodule(const char *path, const char *prefix, struct strbuf sb = STRBUF_INIT; struct child_process cp = CHILD_PROCESS_INIT; char *sub_config_path = NULL; + int active, r; - if (!is_submodule_active(the_repository, path)) + active = is_submodule_active(the_repository, path); + if (!active) return; sub = submodule_from_path(the_repository, _oid, path); @@ -983,13 +985,15 @@ static void sync_submodule(const char *path, const char *prefix, strbuf_strip_suffix(, "\n"); remote_key = xstrfmt("remote.%s.url", sb.buf); + if (active == SUBMODULE_ACTIVE_VIA_URL) + FREE_AND_NULL(sub_origin_url); strbuf_reset(); submodule_to_gitdir(, path); strbuf_addstr(, "/config"); - - if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url)) - die(_("failed to update remote for submodule '%s'"), - path); + if ((r = git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))) + if (sub_origin_url || r != CONFIG_NOTHING_SET) + die(_("failed to update remote for submodule '%s'"), + path); if (flags & OPT_RECURSIVE) { struct child_process cpr = CHILD_PROCESS_INIT; -- 2.18.0.265.g16de1b435c9.dirty
[PATCH 2/7] builtin/submodule--helper: remove stray new line
Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 5c9d1fb496d..2f20bd4abdc 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1024,7 +1024,6 @@ static void sync_submodule_cb(const struct cache_entry *list_item, void *cb_data { struct sync_cb *info = cb_data; sync_submodule(list_item->name, info->prefix, info->flags); - } static int module_sync(int argc, const char **argv, const char *prefix) -- 2.18.0.265.g16de1b435c9.dirty
[PATCH 7/7] builtin/submodule--helper: unset submodule url if possible
Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 24 ++-- t/t5526-fetch-submodules.sh | 2 +- t/t7406-submodule-update.sh | 8 t/t7410-submodule-checkout-to.sh | 2 +- 4 files changed, 32 insertions(+), 4 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 36de64902ec..3aa385bce5c 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1515,6 +1515,7 @@ struct update_clone_data { struct object_id oid; unsigned just_cloned; unsigned retried; + unsigned cleanup_url; }; struct submodule_update_clone { @@ -1590,7 +1591,7 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, struct strbuf displaypath_sb = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; const char *displaypath = NULL; - int needs_cloning = 0; + int needs_cloning = 0, active; if (ce_stage(ce)) { if (suc->recursive_prefix) @@ -1632,7 +1633,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, } /* Check if the submodule has been initialized. */ - if (!is_submodule_active(the_repository, ce->name)) { + active = is_submodule_active(the_repository, ce->name); + if (!active) { next_submodule_warn_missing(suc, out, displaypath); goto cleanup; } @@ -1653,6 +1655,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, suc->update_clone[suc->update_clone_nr].sub = sub; suc->update_clone[suc->update_clone_nr].retried = 0; suc->update_clone[suc->update_clone_nr].ce = ce; + suc->update_clone[suc->update_clone_nr].cleanup_url = + (active != SUBMODULE_ACTIVE_VIA_URL); suc->update_clone_nr++; if (!needs_cloning) @@ -1801,6 +1805,22 @@ static int git_update_clone_config(const char *var, const char *value, static void update_submodule(struct update_clone_data *ucd) { + if (ucd->cleanup_url) { + struct strbuf cfg = STRBUF_INIT; + struct strbuf submodule_url = STRBUF_INIT; + int r; + + strbuf_addf(_url, "submodule.%s.url", ucd->sub->name); + strbuf_repo_git_path(, the_repository, "config"); + + r = git_config_set_in_file_gently(cfg.buf, submodule_url.buf, NULL); + if (r && r != CONFIG_NOTHING_SET) + die(_("failed to remove '%s'"), submodule_url.buf); + + strbuf_release(); + strbuf_release(_url); + } + fprintf(stdout, "dummy %s %d\t%s\n", oid_to_hex(>oid), ucd->just_cloned, diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index 0f730d77815..cd1bd131b59 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -508,7 +508,7 @@ test_expect_success "'fetch.recurseSubmodules=on-demand' works also without .git git config -f .gitmodules submodule.fake.path fake && git config -f .gitmodules submodule.fake.url fakeurl && git add .gitmodules && - git config --unset submodule.submodule.url && + test_might_fail git config --unset submodule.submodule.url && git fetch >../actual.out 2>../actual.err && # cleanup git config --unset fetch.recurseSubmodules && diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh index f604ef7a729..f581fea28e0 100755 --- a/t/t7406-submodule-update.sh +++ b/t/t7406-submodule-update.sh @@ -84,6 +84,14 @@ test_expect_success 'submodule update detaching the HEAD ' ' ) ' +test_expect_success 'active submodule leaves no URL config in superproject' ' + # relies on previous test + ( + cd super && + test_must_fail git config -f .git/config submodule.submodule.url + ) +' + test_expect_success 'submodule update from subdirectory' ' (cd super/submodule && git reset --hard HEAD~1 diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh index f1b492ebc46..683e957934b 100755 --- a/t/t7410-submodule-checkout-to.sh +++ b/t/t7410-submodule-checkout-to.sh @@ -55,7 +55,7 @@ test_expect_failure 'can see submodule diffs just after checkout' ' test_expect_success 'checkout main and initialize independent clones' ' mkdir fully_cloned_submodule && git -C clone/main worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main" && - git -C fully_cloned_submodule/main submodule update + git -C fully_cloned_submodule/main submodule update --init ' test_expect_success 'can see submodule diffs after independent cloning' ' -- 2.18.0.265.g16de1b435c9.dirty
[PATCH 1/7] t7410: update to new style
While at it fix a typo (s/independed/independent) and make sure git is not in a chain of pipes. Signed-off-by: Stefan Beller --- t/t7410-submodule-checkout-to.sh | 99 +++- 1 file changed, 58 insertions(+), 41 deletions(-) diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh index 1acef32647a..f1b492ebc46 100755 --- a/t/t7410-submodule-checkout-to.sh +++ b/t/t7410-submodule-checkout-to.sh @@ -6,55 +6,72 @@ test_description='Combination of submodules and multiple workdirs' base_path=$(pwd -P) -test_expect_success 'setup: make origin' \ -'mkdir -p origin/sub && ( cd origin/sub && git init && - echo file1 >file1 && - git add file1 && - git commit -m file1 ) && -mkdir -p origin/main && ( cd origin/main && git init && - git submodule add ../sub && - git commit -m "add sub" ) && -( cd origin/sub && - echo file1updated >file1 && - git add file1 && - git commit -m "file1 updated" ) && -( cd origin/main/sub && git pull ) && -( cd origin/main && - git add sub && - git commit -m "sub updated" )' - -test_expect_success 'setup: clone' \ -'mkdir clone && ( cd clone && - git clone --recursive "$base_path/origin/main")' +test_expect_success 'setup: make origin' ' + mkdir -p origin/sub && + ( + cd origin/sub && git init && + echo file1 >file1 && + git add file1 && + git commit -m file1 + ) && + mkdir -p origin/main && + ( + cd origin/main && git init && + git submodule add ../sub && + git commit -m "add sub" + ) && + ( + cd origin/sub && + echo file1updated >file1 && + git add file1 && + git commit -m "file1 updated" + ) && + git -C origin/main/sub pull && + ( + cd origin/main && + git add sub && + git commit -m "sub updated" + ) +' + +test_expect_success 'setup: clone' ' + mkdir clone && + git -C clone clone --recursive "$base_path/origin/main" +' rev1_hash_main=$(git --git-dir=origin/main/.git show --pretty=format:%h -q "HEAD~1") rev1_hash_sub=$(git --git-dir=origin/sub/.git show --pretty=format:%h -q "HEAD~1") -test_expect_success 'checkout main' \ -'mkdir default_checkout && -(cd clone/main && - git worktree add "$base_path/default_checkout/main" "$rev1_hash_main")' +test_expect_success 'checkout main' ' + mkdir default_checkout && + git -C clone/main worktree add "$base_path/default_checkout/main" "$rev1_hash_main" +' -test_expect_failure 'can see submodule diffs just after checkout' \ -'(cd default_checkout/main && git diff --submodule master"^!" | grep "file1 updated")' +test_expect_failure 'can see submodule diffs just after checkout' ' + git -C default_checkout/main diff --submodule master"^!" >out && + grep "file1 updated" out +' -test_expect_success 'checkout main and initialize independed clones' \ -'mkdir fully_cloned_submodule && -(cd clone/main && - git worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main") && -(cd fully_cloned_submodule/main && git submodule update)' +test_expect_success 'checkout main and initialize independent clones' ' + mkdir fully_cloned_submodule && + git -C clone/main worktree add "$base_path/fully_cloned_submodule/main" "$rev1_hash_main" && + git -C fully_cloned_submodule/main submodule update +' -test_expect_success 'can see submodule diffs after independed cloning' \ -'(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")' +test_expect_success 'can see submodule diffs after independent cloning' ' + git -C fully_cloned_submodule/main diff --submodule master"^!" >out && + grep "file1 updated" out +' -test_expect_success 'checkout sub manually' \ -'mkdir linked_submodule && -(cd clone/main && - git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") && -(cd clone/main/sub && - git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")' +test_expect_success 'checkout sub manually' ' + mkdir linked_submodule && + git -C clone/main worktree add "$base_path/linked_submodule/main" "$rev1_hash_main" && + git -C clone/main/sub worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub" +' -test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \ -'(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")' +test_expect_success 'can see submodule diffs after manual checkout of linked submodule' ' + git -C linked_submodule/main diff --submodule master"^!" >out && + grep "file1 updated" out +' test_done -- 2.18.0.265.g16de1b435c9.dirty
[PATCH 5/7] submodule--helper: factor out allocation of callback cookie
Signed-off-by: Stefan Beller --- builtin/submodule--helper.c | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 639d0bb20a1..1c9a12781fd 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1688,6 +1688,13 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce, return needs_cloning; } +static void *update_clone_alloc_cb(int i) +{ + int *p = xmalloc(sizeof(*p)); + *p = i; + return p; +} + static int update_clone_get_next_task(struct child_process *child, struct strbuf *err, void *suc_cb, @@ -1700,9 +1707,7 @@ static int update_clone_get_next_task(struct child_process *child, for (; suc->current < suc->list.nr; suc->current++) { ce = suc->list.entries[suc->current]; if (prepare_to_clone_next_submodule(ce, child, suc, err)) { - int *p = xmalloc(sizeof(*p)); - *p = suc->current; - *idx_task_cb = p; + *idx_task_cb = update_clone_alloc_cb(suc->current); suc->current++; return 1; } @@ -1715,7 +1720,6 @@ static int update_clone_get_next_task(struct child_process *child, */ index = suc->current - suc->list.nr; if (index < suc->failed_clones_nr) { - int *p; ce = suc->failed_clones[index]; if (!prepare_to_clone_next_submodule(ce, child, suc, err)) { suc->current ++; @@ -1724,9 +1728,7 @@ static int update_clone_get_next_task(struct child_process *child, "any more?\n"); return 0; } - p = xmalloc(sizeof(*p)); - *p = suc->current; - *idx_task_cb = p; + *idx_task_cb = update_clone_alloc_cb(suc->current); suc->current ++; return 1; } -- 2.18.0.265.g16de1b435c9.dirty
[PATCH 3/7] submodule: is_submodule_active to differentiate between new and old mode
The change a086f921a72 (submodule: decouple url and submodule interest, 2017-03-17) enables us to do more than originally thought. As the url setting was used both to actually set the url where to obtain the submodule from, as well as used as a boolean flag later to see if it was active, we would need to keep the url around. Now that submodules can be activated using the submodule.[.]active setting, we could remove the url if the submodule is activated via that setting. In preparation to do so, pave the way by providing an easy way to see if a submodule is considered active via the new .active setting or via the old .url setting. Signed-off-by: Stefan Beller --- submodule.c | 5 + submodule.h | 6 ++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/submodule.c b/submodule.c index 6e14547e9e0..d56350ed094 100644 --- a/submodule.c +++ b/submodule.c @@ -221,9 +221,6 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt, return 0; } -/* - * Determine if a submodule has been initialized at a given 'path' - */ int is_submodule_active(struct repository *repo, const char *path) { int ret = 0; @@ -267,7 +264,7 @@ int is_submodule_active(struct repository *repo, const char *path) /* fallback to checking if the URL is set */ key = xstrfmt("submodule.%s.url", module->name); - ret = !repo_config_get_string(repo, key, ); + ret = !repo_config_get_string(repo, key, ) ? 2 : 0; free(value); free(key); diff --git a/submodule.h b/submodule.h index 4644683e6cb..bfc070e4629 100644 --- a/submodule.h +++ b/submodule.h @@ -45,6 +45,12 @@ extern int git_default_submodule_config(const char *var, const char *value, void struct option; int option_parse_recurse_submodules_worktree_updater(const struct option *opt, const char *arg, int unset); +/* + * Determine if a submodule has been initialized at a given 'path'. + * Returns 1 if it is considered active via the submodule.[.]active + * setting, or return 2 if it is active via the older submodule.url setting. + */ +#define SUBMODULE_ACTIVE_VIA_URL 2 extern int is_submodule_active(struct repository *repo, const char *path); /* * Determine if a submodule has been populated at a given 'path' by checking if -- 2.18.0.265.g16de1b435c9.dirty
Re: [PATCH 2/2] submodule: munge paths to submodule git directories
At 15:33 -0700 08 Aug 2018, Brandon Williams wrote: Teach "submodule_name_to_gitdir()" to munge a submodule's name (by url encoding it) before using it to build a path to the submodule's gitdir. Seems like this will be a problem if it results in names that exceed NAME_MAX? On common systems that's 255, so it's probably not going to be common; but it certainly could for some repositories.
Re: [PATCHv4 0/6] Add missing includes and forward declares
On 15/08/18 18:54, Elijah Newren wrote: > This series fixes compilation errors when using a simple test.c file that > includes git-compat-util.h and then exactly one other header (and repeating > this for different headers of git). > [snip] > 1: f7d50cef3b ! 1: e6a93208b2 Add missing includes and forward declares > @@ -1,6 +1,13 @@ > Author: Elijah Newren > > -Add missing includes and forward declares > +Add missing includes and forward declarations > + > +I looped over the toplevel header files, creating a temporary > two-line C > +program for each consisting of > + #include "git-compat-util.h" > + #include $HEADER > +This patch is the result of manually fixing errors in compiling those > +tiny programs. As a quick ("just before bedtime") exercise, I tried adding a Makefile target to perform a similar check. The result is given below, but I haven't had time to look too closely at the results: $ make -k hdr-check >zzz 2>&1 $ grep error zzz | wc -l 18 $ grep warning zzz | wc -l 21 $ grep error zzz | cut -d: -f1 | grep -v make | uniq -c | sort -nr 11 refs/refs-internal.h 2 unicode-width.h 2 json-writer.h 1 refs/ref-cache.h 1 commit-slab-impl.h $ grep warning zzz | cut -d: -f1 | grep -v make | uniq -c | sort -nr 7 refs/refs-internal.h 5 delta-islands.h 2 unicode-width.h 2 midx.h 2 commit-reach.h 1 refs/ref-cache.h 1 refs/packed-backend.h 1 pack-bitmap.h $ BTW, I happen to be on the 'pu' branch. I think some of the errors are due to missing compiler flags (-I, -D, etc); which flags did you pass to the compiler? Well, it killed 15min. before bed! ;-) ATB, Ramsay Jones -- >8 -- Subject: [PATCH] Makefile: add a hdr-check target Signed-off-by: Ramsay Jones --- Makefile | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Makefile b/Makefile index 9923db888c..798396c52e 100644 --- a/Makefile +++ b/Makefile @@ -2684,6 +2684,16 @@ $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE .PHONY: sparse $(SP_OBJ) sparse: $(SP_OBJ) +EXCEPT_HDRS := ./compat% ./xdiff% ./ewah% +CHK_HDRS = $(filter-out $(EXCEPT_HDRS),$(LIB_H)) +HCO = $(patsubst %.h,%.hco,$(CHK_HDRS)) + +$(HCO): %.hco: %.h FORCE + $(CC) -Wall -include git-compat-util.h -I. -o /dev/null -c -xc $< + +.PHONY: hdr-check $(HCO) +hdr-check: $(HCO) + .PHONY: style style: git clang-format --style file --diff --extensions c,h -- 2.18.0
[PATCH v6 2/6] list-objects: refactor to process_tree_contents
This will be used in a follow-up patch to reduce indentation needed when invoking the logic conditionally. i.e. rather than: if (foo) { while (...) { /* this is very indented */ } } we will have: if (foo) process_tree_contents(...); Signed-off-by: Matthew DeVore --- list-objects.c | 68 ++ 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/list-objects.c b/list-objects.c index 584518a3f..ccc529e5e 100644 --- a/list-objects.c +++ b/list-objects.c @@ -94,6 +94,46 @@ static void process_gitlink(struct traversal_context *ctx, /* Nothing to do */ } +static void process_tree(struct traversal_context *ctx, +struct tree *tree, +struct strbuf *base, +const char *name); + +static void process_tree_contents(struct traversal_context *ctx, + struct tree *tree, + struct strbuf *base) +{ + struct tree_desc desc; + struct name_entry entry; + enum interesting match = ctx->revs->diffopt.pathspec.nr == 0 ? + all_entries_interesting : entry_not_interesting; + + init_tree_desc(, tree->buffer, tree->size); + + while (tree_entry(, )) { + if (match != all_entries_interesting) { + match = tree_entry_interesting(, base, 0, + >revs->diffopt.pathspec); + if (match == all_entries_not_interesting) + break; + if (match == entry_not_interesting) + continue; + } + + if (S_ISDIR(entry.mode)) + process_tree(ctx, +lookup_tree(the_repository, entry.oid), +base, entry.path); + else if (S_ISGITLINK(entry.mode)) + process_gitlink(ctx, entry.oid->hash, + base, entry.path); + else + process_blob(ctx, +lookup_blob(the_repository, entry.oid), +base, entry.path); + } +} + static void process_tree(struct traversal_context *ctx, struct tree *tree, struct strbuf *base, @@ -101,10 +141,6 @@ static void process_tree(struct traversal_context *ctx, { struct object *obj = >object; struct rev_info *revs = ctx->revs; - struct tree_desc desc; - struct name_entry entry; - enum interesting match = revs->diffopt.pathspec.nr == 0 ? - all_entries_interesting: entry_not_interesting; int baselen = base->len; enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW; int gently = revs->ignore_missing_links || @@ -144,29 +180,7 @@ static void process_tree(struct traversal_context *ctx, if (base->len) strbuf_addch(base, '/'); - init_tree_desc(, tree->buffer, tree->size); - - while (tree_entry(, )) { - if (match != all_entries_interesting) { - match = tree_entry_interesting(, base, 0, - >diffopt.pathspec); - if (match == all_entries_not_interesting) - break; - if (match == entry_not_interesting) - continue; - } - - if (S_ISDIR(entry.mode)) - process_tree(ctx, -lookup_tree(the_repository, entry.oid), -base, entry.path); - else if (S_ISGITLINK(entry.mode)) - process_gitlink(ctx, entry.oid->hash, base, entry.path); - else - process_blob(ctx, -lookup_blob(the_repository, entry.oid), -base, entry.path); - } + process_tree_contents(ctx, tree, base); if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) { r = ctx->filter_fn(LOFS_END_TREE, obj, -- 2.18.0.865.gffc8e1a3cd6-goog
[PATCH v6 3/6] list-objects: always parse trees gently
If parsing fails when revs->ignore_missing_links and revs->exclude_promisor_objects are both false, we print the OID anyway in the die("bad tree object...") call, so any message printed by parse_tree_gently() is superfluous. Signed-off-by: Matthew DeVore --- list-objects.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/list-objects.c b/list-objects.c index ccc529e5e..f9b51db7a 100644 --- a/list-objects.c +++ b/list-objects.c @@ -143,8 +143,6 @@ static void process_tree(struct traversal_context *ctx, struct rev_info *revs = ctx->revs; int baselen = base->len; enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW; - int gently = revs->ignore_missing_links || -revs->exclude_promisor_objects; if (!revs->tree_objects) return; @@ -152,7 +150,7 @@ static void process_tree(struct traversal_context *ctx, die("bad tree object"); if (obj->flags & (UNINTERESTING | SEEN)) return; - if (parse_tree_gently(tree, gently) < 0) { + if (parse_tree_gently(tree, 1) < 0) { if (revs->ignore_missing_links) return; -- 2.18.0.865.gffc8e1a3cd6-goog
[PATCH v6 1/6] list-objects: store common func args in struct
This will make utility functions easier to create, as done by the next patch. Signed-off-by: Matthew DeVore --- list-objects.c | 158 +++-- 1 file changed, 74 insertions(+), 84 deletions(-) diff --git a/list-objects.c b/list-objects.c index c99c47ac1..584518a3f 100644 --- a/list-objects.c +++ b/list-objects.c @@ -12,20 +12,25 @@ #include "packfile.h" #include "object-store.h" -static void process_blob(struct rev_info *revs, +struct traversal_context { + struct rev_info *revs; + show_object_fn show_object; + show_commit_fn show_commit; + void *show_data; + filter_object_fn filter_fn; + void *filter_data; +}; + +static void process_blob(struct traversal_context *ctx, struct blob *blob, -show_object_fn show, struct strbuf *path, -const char *name, -void *cb_data, -filter_object_fn filter_fn, -void *filter_data) +const char *name) { struct object *obj = >object; size_t pathlen; enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW; - if (!revs->blob_objects) + if (!ctx->revs->blob_objects) return; if (!obj) die("bad blob object"); @@ -41,21 +46,21 @@ static void process_blob(struct rev_info *revs, * may cause the actual filter to report an incomplete list * of missing objects. */ - if (revs->exclude_promisor_objects && + if (ctx->revs->exclude_promisor_objects && !has_object_file(>oid) && is_promisor_object(>oid)) return; pathlen = path->len; strbuf_addstr(path, name); - if (!(obj->flags & USER_GIVEN) && filter_fn) - r = filter_fn(LOFS_BLOB, obj, - path->buf, >buf[pathlen], - filter_data); + if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) + r = ctx->filter_fn(LOFS_BLOB, obj, + path->buf, >buf[pathlen], + ctx->filter_data); if (r & LOFR_MARK_SEEN) obj->flags |= SEEN; if (r & LOFR_DO_SHOW) - show(obj, path->buf, cb_data); + ctx->show_object(obj, path->buf, ctx->show_data); strbuf_setlen(path, pathlen); } @@ -81,26 +86,21 @@ static void process_blob(struct rev_info *revs, * the link, and how to do it. Whether it necessarily makes * any sense what-so-ever to ever do that is another issue. */ -static void process_gitlink(struct rev_info *revs, +static void process_gitlink(struct traversal_context *ctx, const unsigned char *sha1, - show_object_fn show, struct strbuf *path, - const char *name, - void *cb_data) + const char *name) { /* Nothing to do */ } -static void process_tree(struct rev_info *revs, +static void process_tree(struct traversal_context *ctx, struct tree *tree, -show_object_fn show, struct strbuf *base, -const char *name, -void *cb_data, -filter_object_fn filter_fn, -void *filter_data) +const char *name) { struct object *obj = >object; + struct rev_info *revs = ctx->revs; struct tree_desc desc; struct name_entry entry; enum interesting match = revs->diffopt.pathspec.nr == 0 ? @@ -133,14 +133,14 @@ static void process_tree(struct rev_info *revs, } strbuf_addstr(base, name); - if (!(obj->flags & USER_GIVEN) && filter_fn) - r = filter_fn(LOFS_BEGIN_TREE, obj, - base->buf, >buf[baselen], - filter_data); + if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) + r = ctx->filter_fn(LOFS_BEGIN_TREE, obj, + base->buf, >buf[baselen], + ctx->filter_data); if (r & LOFR_MARK_SEEN) obj->flags |= SEEN; if (r & LOFR_DO_SHOW) - show(obj, base->buf, cb_data); + ctx->show_object(obj, base->buf, ctx->show_data); if (base->len) strbuf_addch(base, '/'); @@ -157,29 +157,25 @@ static void process_tree(struct rev_info *revs, } if (S_ISDIR(entry.mode)) - process_tree(revs, + process_tree(ctx, lookup_tree(the_repository, entry.oid), -
[PATCH v6 4/6] rev-list: handle missing tree objects properly
Previously, we assumed only blob objects could be missing. This patch makes rev-list handle missing trees like missing blobs. The --missing=* and --exclude-promisor-objects flags now work for trees as they already do for blobs. This is demonstrated in t6112. Signed-off-by: Matthew DeVore --- builtin/rev-list.c | 11 --- list-objects.c | 11 +-- revision.h | 15 + t/t0410-partial-clone.sh | 45 ++ t/t5317-pack-objects-filter-objects.sh | 13 t/t6112-rev-list-filters-objects.sh| 17 ++ 6 files changed, 105 insertions(+), 7 deletions(-) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 5b07f3f4a..49d6deed7 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -6,6 +6,7 @@ #include "list-objects.h" #include "list-objects-filter.h" #include "list-objects-filter-options.h" +#include "object.h" #include "object-store.h" #include "pack.h" #include "pack-bitmap.h" @@ -209,7 +210,8 @@ static inline void finish_object__ma(struct object *obj) */ switch (arg_missing_action) { case MA_ERROR: - die("missing blob object '%s'", oid_to_hex(>oid)); + die("missing %s object '%s'", + type_name(obj->type), oid_to_hex(>oid)); return; case MA_ALLOW_ANY: @@ -222,8 +224,8 @@ static inline void finish_object__ma(struct object *obj) case MA_ALLOW_PROMISOR: if (is_promisor_object(>oid)) return; - die("unexpected missing blob object '%s'", - oid_to_hex(>oid)); + die("unexpected missing %s object '%s'", + type_name(obj->type), oid_to_hex(>oid)); return; default: @@ -235,7 +237,7 @@ static inline void finish_object__ma(struct object *obj) static int finish_object(struct object *obj, const char *name, void *cb_data) { struct rev_list_info *info = cb_data; - if (obj->type == OBJ_BLOB && !has_object_file(>oid)) { + if (!has_object_file(>oid)) { finish_object__ma(obj); return 1; } @@ -373,6 +375,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) init_revisions(, prefix); revs.abbrev = DEFAULT_ABBREV; revs.commit_format = CMIT_FMT_UNSPECIFIED; + revs.do_not_die_on_missing_tree = 1; /* * Scan the argument list before invoking setup_revisions(), so that we diff --git a/list-objects.c b/list-objects.c index f9b51db7a..243192af5 100644 --- a/list-objects.c +++ b/list-objects.c @@ -143,6 +143,7 @@ static void process_tree(struct traversal_context *ctx, struct rev_info *revs = ctx->revs; int baselen = base->len; enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW; + int failed_parse; if (!revs->tree_objects) return; @@ -150,7 +151,9 @@ static void process_tree(struct traversal_context *ctx, die("bad tree object"); if (obj->flags & (UNINTERESTING | SEEN)) return; - if (parse_tree_gently(tree, 1) < 0) { + + failed_parse = parse_tree_gently(tree, 1); + if (failed_parse) { if (revs->ignore_missing_links) return; @@ -163,7 +166,8 @@ static void process_tree(struct traversal_context *ctx, is_promisor_object(>oid)) return; - die("bad tree object %s", oid_to_hex(>oid)); + if (!revs->do_not_die_on_missing_tree) + die("bad tree object %s", oid_to_hex(>oid)); } strbuf_addstr(base, name); @@ -178,7 +182,8 @@ static void process_tree(struct traversal_context *ctx, if (base->len) strbuf_addch(base, '/'); - process_tree_contents(ctx, tree, base); + if (!failed_parse) + process_tree_contents(ctx, tree, base); if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) { r = ctx->filter_fn(LOFS_END_TREE, obj, diff --git a/revision.h b/revision.h index c599c34da..51189 100644 --- a/revision.h +++ b/revision.h @@ -125,6 +125,21 @@ struct rev_info { line_level_traverse:1, tree_blobs_in_commit_order:1, + /* +* Blobs are shown without regard for their existence. +* But not so for trees: unless exclude_promisor_objects +* is set and the tree in question is a promisor object; +* OR ignore_missing_links is set, the revision walker +* dies with a "bad tree object HASH" message when +* encountering a missing tree. For callers that can +* handle missing trees
[PATCH v6 6/6] list-objects-filter: implement filter tree:0
Teach list-objects the "tree:0" filter which allows for filtering out all tree and blob objects (unless other objects are explicitly specified by the user). The purpose of this patch is to allow smaller partial clones. The name of this filter - tree:0 - does not explicitly specify that it also filters out all blobs, but this should not cause much confusion because blobs are not at all useful without the trees that refer to them. I also consider only:commits as a name, but this is inaccurate because it suggests that annotated tags are omitted, but actually they are included. The name "tree:0" allows later filtering based on depth, i.e. "tree:1" would filter out all but the root tree and blobs. In order to avoid confusion between 0 and capital O, the documentation was worded in a somewhat round-about way that also hints at this future improvement to the feature. Signed-off-by: Matthew DeVore --- Documentation/rev-list-options.txt | 5 +++ list-objects-filter-options.c | 4 +++ list-objects-filter-options.h | 1 + list-objects-filter.c | 50 ++ t/t5317-pack-objects-filter-objects.sh | 28 +++ t/t5616-partial-clone.sh | 38 t/t6112-rev-list-filters-objects.sh| 12 +++ 7 files changed, 138 insertions(+) diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 7b273635d..5f1672913 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -731,6 +731,11 @@ the requested refs. + The form '--filter=sparse:path=' similarly uses a sparse-checkout specification contained in . ++ +The form '--filter=tree:' omits all blobs and trees whose depth +from the root tree is >= (minimum depth if an object is located +at multiple depths in the commits traversed). Currently, only =0 +is supported, which omits all blobs and trees. --no-filter:: Turn off any previous `--filter=` argument. diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c index c0e2bd6a0..a28382940 100644 --- a/list-objects-filter-options.c +++ b/list-objects-filter-options.c @@ -50,6 +50,10 @@ static int gently_parse_list_objects_filter( return 0; } + } else if (!strcmp(arg, "tree:0")) { + filter_options->choice = LOFC_TREE_NONE; + return 0; + } else if (skip_prefix(arg, "sparse:oid=", )) { struct object_context oc; struct object_id sparse_oid; diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h index a61f8..af64e5c66 100644 --- a/list-objects-filter-options.h +++ b/list-objects-filter-options.h @@ -10,6 +10,7 @@ enum list_objects_filter_choice { LOFC_DISABLED = 0, LOFC_BLOB_NONE, LOFC_BLOB_LIMIT, + LOFC_TREE_NONE, LOFC_SPARSE_OID, LOFC_SPARSE_PATH, LOFC__COUNT /* must be last */ diff --git a/list-objects-filter.c b/list-objects-filter.c index a0ba78b20..8e3caf5bf 100644 --- a/list-objects-filter.c +++ b/list-objects-filter.c @@ -80,6 +80,55 @@ static void *filter_blobs_none__init( return d; } +/* + * A filter for list-objects to omit ALL trees and blobs from the traversal. + * Can OPTIONALLY collect a list of the omitted OIDs. + */ +struct filter_trees_none_data { + struct oidset *omits; +}; + +static enum list_objects_filter_result filter_trees_none( + enum list_objects_filter_situation filter_situation, + struct object *obj, + const char *pathname, + const char *filename, + void *filter_data_) +{ + struct filter_trees_none_data *filter_data = filter_data_; + + switch (filter_situation) { + default: + die("unknown filter_situation"); + return LOFR_ZERO; + + case LOFS_BEGIN_TREE: + case LOFS_BLOB: + if (filter_data->omits) + oidset_insert(filter_data->omits, >oid); + return LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */ + + case LOFS_END_TREE: + assert(obj->type == OBJ_TREE); + return LOFR_ZERO; + + } +} + +static void* filter_trees_none__init( + struct oidset *omitted, + struct list_objects_filter_options *filter_options, + filter_object_fn *filter_fn, + filter_free_fn *filter_free_fn) +{ + struct filter_trees_none_data *d = xcalloc(1, sizeof(*d)); + d->omits = omitted; + + *filter_fn = filter_trees_none; + *filter_free_fn = free; + return d; +} + /* * A filter for list-objects to omit large blobs. * And to OPTIONALLY collect a list of the omitted OIDs. @@ -374,6 +423,7 @@ static filter_init_fn s_filters[] = { NULL, filter_blobs_none__init, filter_blobs_limit__init, + filter_trees_none__init, filter_sparse_oid__init,
[PATCH v6 0/6] filter: support for excluding all trees and blobs
Applied suggestion from Junio about removing -e flag from awk invocation. Sending an updated patchset now since I haven't heard any other comments for a while, and I don't believe Jonathan, the most active reviewer, has any more concerns. Matthew DeVore (6): list-objects: store common func args in struct list-objects: refactor to process_tree_contents list-objects: always parse trees gently rev-list: handle missing tree objects properly revision: mark non-user-given objects instead list-objects-filter: implement filter tree:0 Documentation/rev-list-options.txt | 5 + builtin/rev-list.c | 11 +- list-objects-filter-options.c | 4 + list-objects-filter-options.h | 1 + list-objects-filter.c | 50 ++ list-objects.c | 232 + revision.c | 1 - revision.h | 25 ++- t/t0410-partial-clone.sh | 45 + t/t5317-pack-objects-filter-objects.sh | 41 + t/t5616-partial-clone.sh | 38 t/t6112-rev-list-filters-objects.sh| 29 12 files changed, 364 insertions(+), 118 deletions(-) -- 2.18.0.865.gffc8e1a3cd6-goog
[PATCH v6 5/6] revision: mark non-user-given objects instead
Currently, list-objects.c incorrectly treats all root trees of commits as USER_GIVEN. Also, it would be easier to mark objects that are non-user-given instead of user-given, since the places in the code where we access an object through a reference are more obvious than the places where we access an object that was given by the user. Resolve these two problems by introducing a flag NOT_USER_GIVEN that marks blobs and trees that are non-user-given, replacing USER_GIVEN. (Only blobs and trees are marked because this mark is only used when filtering objects, and filtering of other types of objects is not supported yet.) Signed-off-by: Matthew DeVore --- list-objects.c | 31 ++- revision.c | 1 - revision.h | 10 +++--- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/list-objects.c b/list-objects.c index 243192af5..7a1a0929d 100644 --- a/list-objects.c +++ b/list-objects.c @@ -53,7 +53,7 @@ static void process_blob(struct traversal_context *ctx, pathlen = path->len; strbuf_addstr(path, name); - if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) + if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) r = ctx->filter_fn(LOFS_BLOB, obj, path->buf, >buf[pathlen], ctx->filter_data); @@ -120,17 +120,19 @@ static void process_tree_contents(struct traversal_context *ctx, continue; } - if (S_ISDIR(entry.mode)) - process_tree(ctx, -lookup_tree(the_repository, entry.oid), -base, entry.path); + if (S_ISDIR(entry.mode)) { + struct tree *t = lookup_tree(the_repository, entry.oid); + t->object.flags |= NOT_USER_GIVEN; + process_tree(ctx, t, base, entry.path); + } else if (S_ISGITLINK(entry.mode)) process_gitlink(ctx, entry.oid->hash, base, entry.path); - else - process_blob(ctx, -lookup_blob(the_repository, entry.oid), -base, entry.path); + else { + struct blob *b = lookup_blob(the_repository, entry.oid); + b->object.flags |= NOT_USER_GIVEN; + process_blob(ctx, b, base, entry.path); + } } } @@ -171,7 +173,7 @@ static void process_tree(struct traversal_context *ctx, } strbuf_addstr(base, name); - if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) + if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) r = ctx->filter_fn(LOFS_BEGIN_TREE, obj, base->buf, >buf[baselen], ctx->filter_data); @@ -185,7 +187,7 @@ static void process_tree(struct traversal_context *ctx, if (!failed_parse) process_tree_contents(ctx, tree, base); - if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) { + if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) { r = ctx->filter_fn(LOFS_END_TREE, obj, base->buf, >buf[baselen], ctx->filter_data); @@ -301,8 +303,11 @@ static void do_traverse(struct traversal_context *ctx) * an uninteresting boundary commit may not have its tree * parsed yet, but we are not going to show them anyway */ - if (get_commit_tree(commit)) - add_pending_tree(ctx->revs, get_commit_tree(commit)); + if (get_commit_tree(commit)) { + struct tree *tree = get_commit_tree(commit); + tree->object.flags |= NOT_USER_GIVEN; + add_pending_tree(ctx->revs, tree); + } ctx->show_commit(commit, ctx->show_data); if (ctx->revs->tree_blobs_in_commit_order) diff --git a/revision.c b/revision.c index 062749437..6d355b43c 100644 --- a/revision.c +++ b/revision.c @@ -175,7 +175,6 @@ static void add_pending_object_with_path(struct rev_info *revs, strbuf_release(); return; /* do not add the commit itself */ } - obj->flags |= USER_GIVEN; add_object_array_with_path(obj, name, >pending, mode, path); } diff --git a/revision.h b/revision.h index 51189..2d381e636 100644 --- a/revision.h +++ b/revision.h @@ -8,7 +8,11 @@ #include "diff.h" #include "commit-slab-decl.h" -/* Remember to update object flag allocation in object.h */ +/* Remember to update object flag allocation in object.h + * NEEDSWORK: NOT_USER_GIVEN doesn't apply to commits because we only
What's cooking in git.git (Aug 2018, #03; Wed, 15)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * bb/make-developer-pedantic (2018-07-25) 1 commit (merged to 'next' on 2018-08-02 at c738a84b7e) + Makefile: add a DEVOPTS flag to get pedantic compilation "make DEVELOPER=1 DEVOPTS=pedantic" allows developers to compile with -pedantic option, which may catch more problematic program constructs and potential bugs. * bb/redecl-enum-fix (2018-07-26) 1 commit (merged to 'next' on 2018-08-06 at 828dc4b156) + packfile: ensure that enum object_type is defined Compilation fix. * bw/clone-ref-prefixes (2018-07-20) 1 commit (merged to 'next' on 2018-08-02 at c8ad140ab0) + clone: send ref-prefixes when using protocol v2 The wire-protocol v2 relies on the client to send "ref prefixes" to limit the bandwidth spent on the initial ref advertisement. "git clone" when learned to speak v2 forgot to do so, which has been corrected. * bw/fetch-pack-i18n (2018-07-23) 1 commit (merged to 'next' on 2018-08-02 at df72001755) + fetch-pack: mark die strings for translation i18n updates. * bw/protocol-v2 (2018-07-24) 1 commit (merged to 'next' on 2018-08-02 at f4076b3e94) + pack-protocol: mention and point to docs for protocol v2 Doc update. * cb/p4-pre-submit-hook (2018-08-01) 1 commit (merged to 'next' on 2018-08-06 at e40ae4af80) + git-p4: add the `p4-pre-submit` hook "git p4 submit" learns to ask its own pre-submit hook if it should continue with submitting. * en/merge-recursive-skip-fix (2018-07-27) 2 commits (merged to 'next' on 2018-08-06 at 9ab321a15c) + merge-recursive: preserve skip_worktree bit when necessary + t3507: add a testcase showing failure with sparse checkout When the sparse checkout feature is in use, "git cherry-pick" and other mergy operations lost the skip_worktree bit when a path that is excluded from checkout requires content level merge, which is resolved as the same as the HEAD version, without materializing the merge result in the working tree, which made the path appear as deleted. This has been corrected by preserving the skip_worktree bit (and not materializing the file in the working tree). * es/diff-color-moved-fix (2018-07-25) 1 commit (merged to 'next' on 2018-08-02 at 233bccfbfb) + diff: --color-moved: rename "dimmed_zebra" to "dimmed-zebra" One of the "diff --color-moved" mode "dimmed_zebra" that was named in an unusual way has been deprecated and replaced by "dimmed-zebra". * es/mw-to-git-chain-fix (2018-07-31) 1 commit (merged to 'next' on 2018-08-06 at c10246e1c8) + mw-to-git/t9360: fix broken &&-chain Test fix. * hs/gpgsm (2018-07-20) 7 commits (merged to 'next' on 2018-08-02 at db28bffe4f) + gpg-interface t: extend the existing GPG tests with GPGSM + gpg-interface: introduce new signature format "x509" using gpgsm + gpg-interface: introduce new config to select per gpg format program + gpg-interface: do not hardcode the key string len anymore + gpg-interface: introduce an abstraction for multiple gpg formats + t/t7510: check the validation of the new config gpg.format + gpg-interface: add new config to select how to sign a commit Teach "git tag -s" etc. a few configuration variables (gpg.format that can be set to "openpgp" or "x509", and gpg..program that is used to specify what program to use to deal with the format) to allow x.509 certs with CMS via "gpgsm" to be used instead of openpgp via "gnupg". * jh/json-writer (2018-07-16) 1 commit (merged to 'next' on 2018-08-02 at d841450c7d) + json_writer: new routines to create JSON data (this branch is used by jh/structured-logging.) Preparatory code to later add json output for telemetry data. * jk/banned-function (2018-07-26) 5 commits (merged to 'next' on 2018-08-06 at 3dcd1999df) + banned.h: mark strncpy() as banned + banned.h: mark sprintf() as banned + banned.h: mark strcat() as banned + automatically ban strcpy() + Merge branch 'sb/blame-color' into jk/banned-function It is too easy to misuse system API functions such as strcat(); these selected functions are now forbidden in this codebase and will cause a compilation failure. * jk/core-use-replace-refs (2018-07-18) 3 commits (merged to 'next' on 2018-08-02 at 90fb6b1056) + add core.usereplacerefs config option + check_replace_refs: rename to read_replace_refs + check_replace_refs: fix outdated comment A new configuration variable core.usereplacerefs has been added, primarily to help server installations that want to ignore the replace mechanism altogether. *
Re: [PATCH] git-submodule.sh: accept verbose flag in cmd_update to be non-quiet
On Tue, Aug 14, 2018 at 11:27 PM "Jochen Kühner" wrote: > > > > > We use git for windows, there I cannot fin the git-submodule.sh! How can I > fix it there? > > It probably doesn't have the .sh extension. I don't know where all git executables are located in GfW. Maybe "dir /s git.exe" can give you a starting point to look for the executables of Git. (courtesy stackoverflow, I am no expert on Windows) As git-submodule is a shell script and doesn't need compilation, you can just edit this in to see if it fixes the problem; mind to use a text editor that doesn't put CRLF, but LF line endings only in that file ... shell script is not the most portable.
Re: [PATCH v2] worktree: add --quiet option
On 08/15, Elia Pinto wrote: > Add the '--quiet' option to git worktree, > as for the other git commands. 'add' is the > only command affected by it since all other > commands, except 'list', are currently > silent by default. > > Helped-by: Martin Ågren > Helped-by: Duy Nguyen > Helped-by: Eric Sunshine > Signed-off-by: Elia Pinto > --- > This is the second version of the patch. > > Changes from the first version > (https://public-inbox.org/git/CACsJy8A=zp7nfbuwyfep4uff3kssiaor3m0mtgvnhceyhsw...@mail.gmail.com/T/): > > - deleted garbage in git-worktree.c and deleted > superfluous blank line in git-worktree.txt. > - when giving "--quiet" to 'add', call git symbolic-ref also with > "--quiet". > - changed the commit message to be more general, but > specifying why the "--quiet" option is meaningful only for > the 'add' command of git-worktree. > - in git-worktree.txt the option > "--quiet" is described near the "--verbose" option. > > Documentation/git-worktree.txt | 4 > builtin/worktree.c | 16 +--- > t/t2025-worktree-add.sh| 5 + > 3 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 9c26be40f..29a5b7e25 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -173,6 +173,10 @@ This can also be set up as the default behaviour by > using the > This format will remain stable across Git versions and regardless of > user > configuration. See below for details. > > +-q:: > +--quiet:: > + With 'add', suppress feedback messages. Very minor nit here, we seem to use backticks everywhere else in this document, maybe we sould do that here as well? Not sure it's worth another iteration though. The rest of the patch looks good to me, thanks! > -v:: > --verbose:: > With `prune`, report all removals. > diff --git a/builtin/worktree.c b/builtin/worktree.c > index a763dbdcc..41e771439 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -27,6 +27,7 @@ static const char * const worktree_usage[] = { > struct add_opts { > int force; > int detach; > + int quiet; > int checkout; > int keep_locked; > }; > @@ -303,9 +304,13 @@ static int add_worktree(const char *path, const char > *refname, > if (!is_branch) > argv_array_pushl(, "update-ref", "HEAD", >oid_to_hex(>object.oid), NULL); > - else > + else { > argv_array_pushl(, "symbolic-ref", "HEAD", >symref.buf, NULL); > + if (opts->quiet) > + argv_array_push(, "--quiet"); > + } > + > cp.env = child_env.argv; > ret = run_command(); > if (ret) > @@ -315,6 +320,8 @@ static int add_worktree(const char *path, const char > *refname, > cp.argv = NULL; > argv_array_clear(); > argv_array_pushl(, "reset", "--hard", NULL); > + if (opts->quiet) > + argv_array_push(, "--quiet"); > cp.env = child_env.argv; > ret = run_command(); > if (ret) > @@ -437,6 +444,7 @@ static int add(int ac, const char **av, const char > *prefix) > OPT_BOOL(0, "detach", , N_("detach HEAD at named > commit")), > OPT_BOOL(0, "checkout", , N_("populate the new > working tree")), > OPT_BOOL(0, "lock", _locked, N_("keep the new working > tree locked")), > + OPT__QUIET(, N_("suppress progress reporting")), > OPT_PASSTHRU(0, "track", _track, NULL, >N_("set up tracking mode (see git-branch(1))"), >PARSE_OPT_NOARG | PARSE_OPT_OPTARG), > @@ -491,8 +499,8 @@ static int add(int ac, const char **av, const char > *prefix) > } > } > } > - > - print_preparing_worktree_line(opts.detach, branch, new_branch, > !!new_branch_force); > + if (!opts.quiet) > + print_preparing_worktree_line(opts.detach, branch, new_branch, > !!new_branch_force); > > if (new_branch) { > struct child_process cp = CHILD_PROCESS_INIT; > @@ -500,6 +508,8 @@ static int add(int ac, const char **av, const char > *prefix) > argv_array_push(, "branch"); > if (new_branch_force) > argv_array_push(, "--force"); > + if (opts.quiet) > + argv_array_push(, "--quiet"); > argv_array_push(, new_branch); > argv_array_push(, branch); > if (opt_track) > diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh > index be6e09314..658647d83 100755 > --- a/t/t2025-worktree-add.sh > +++ b/t/t2025-worktree-add.sh > @@ -252,6 +252,11 @@ test_expect_success 'add -B' ' > test_cmp_rev master^ poodle > ' > > +test_expect_success
Re: [GSoC][PATCH v7 00/26] Convert "git stash" to C builtin
On 08/08, Paul-Sebastian Ungureanu wrote: > Hello, > > Here is the whole `git stash` C version. Some of the previous > patches were already reviewed (up to and including "stash: convert > store to builtin"), but there are some which were not > (starting with "stash: convert create to builtin"). Thanks for this new iteration, and sorry I took a while to find some time to review this. I had another read through the patches up until patch 15, and left some comments, before running out of time again. I hope to find some time in the next few days to go through the rest of the series as well. One more comment in terms of the structure of the series. The patches doing the actual conversion from shell to C seem to be interleaved with cleanup patches and patches that make the C version use more internal APIs. I'd suggest putting all the cleanup patches (e.g. "stash: change `git stash show` usage text and documentation") to the front of the series, as that's more likely to be uncontroversial, and could maybe even be merged by itself. Then I'd put all the conversion from shell to C patches, and only once everything is converted I'd put the patches to use more of the internal APIs rather than using run_command everywhere. A possible alternative would be to squash the patches to replace the run_command calls with patches that use the internal API directly, to save the reviewers some time by reading through less churn. Though I'm kind of on the fence with that, as a faithful conversion using 'run_command' may be easier to review as a first step. Hope this helps! > In order to see the difference between the shell version and > the C version, I ran `time` on: > > * git test suite (t3903-stash.sh, t3904-stash-patch.sh, > t3905-stash-include-untracked.sh and t3906-stash-submodule.sh) > > t3903-stash.sh: > ** SHELL: 12,69s user 9,95s system 109% cpu 20,730 total > ** C: 2,67s user 2,84s system 105% cpu 5,206 total > > t3904-stash-patch.sh: > ** SHELL: 1,43s user 0,94s system 106% cpu 2,242 total > ** C: 1,01s user 0,58s system 104% cpu 1,530 total > > t3905-stash-include-untracked.sh > ** SHELL: 2,22s user 1,73s system 110% cpu 3,569 total > ** C: 0,59s user 0,57s system 106% cpu 1,085 total > > t3906-stash-submodule.sh > ** SHELL: 2,89s user 2,99s system 106% cpu 5,527 total > ** C: 2,21s user 2,61s system 105% cpu 4,568 total > > TOTAL: > ** SHELL: 19.23s user 15.61s system > ** C: 6.48s user 6.60s system Awesome! > * a git repository with 4000 files: 1000 not changed, > 1000 staged files, 1000 unstaged files, 1000 untracked. > In this case I ran some of the most used commands: > > git stash push: > > ** SHELL: 0,12s user 0,21s system 101% cpu 0,329 total > ** C: 0,06s user 0,13s system 105% cpu 0,185 total > > git stash push -u: > > ** SHELL: 0,18s user 0,27s system 108% cpu 0,401 total > ** C: 0,09s user 0,19s system 103% cpu 0,267 total > > git stash pop: > > ** SHELL: 0,16s user 0,26s system 103% cpu 0,399 total > ** C: 0,13s user 0,19s system 102% cpu 0,308 total > > Best regards, > Paul Ungureanu > > > Joel Teichroeb (5): > stash: improve option parsing test coverage > stash: convert apply to builtin > stash: convert drop and clear to builtin > stash: convert branch to builtin > stash: convert pop to builtin > > Paul-Sebastian Ungureanu (21): > sha1-name.c: added 'get_oidf', which acts like 'get_oid' > stash: update test cases conform to coding guidelines > stash: renamed test cases to be more descriptive > stash: implement the "list" command in the builtin > stash: convert show to builtin > stash: change `git stash show` usage text and documentation > stash: refactor `show_stash()` to use the diff API > stash: update `git stash show` documentation > stash: convert store to builtin > stash: convert create to builtin > stash: replace spawning a "read-tree" process > stash: avoid spawning a "diff-index" process > stash: convert push to builtin > stash: make push to be quiet > stash: add tests for `git stash push -q` > stash: replace spawning `git ls-files` child process > stash: convert save to builtin > stash: convert `stash--helper.c` into `stash.c` > stash: optimize `get_untracked_files()` and `check_changes()` > stash: replace all `write-tree` child processes with API calls > stash: replace all "git apply" child processes with API calls > > Documentation/git-stash.txt |7 +- > Makefile|2 +- > builtin.h |1 + > builtin/stash.c | 1562 +++ > cache.h |1 + > git-stash.sh| 752 - > git.c |1 + > sha1-name.c | 19 + >
Re: [GSoC][PATCH v7 15/26] stash: convert create to builtin
On 08/08, Paul-Sebastian Ungureanu wrote: > Add stash create to the helper. > > Signed-off-by: Paul-Sebastian Ungureanu > --- > builtin/stash--helper.c | 406 > git-stash.sh| 2 +- > 2 files changed, 407 insertions(+), 1 deletion(-) > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index 5ff810f8c..a4e57899b 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -21,6 +21,7 @@ static const char * const git_stash_helper_usage[] = { > N_("git stash--helper branch []"), > N_("git stash--helper clear"), > N_("git stash--helper store [-m|--message ] [-q|--quiet] > "), > + N_("git stash--helper create []"), > NULL > }; > > @@ -64,6 +65,11 @@ static const char * const git_stash_helper_store_usage[] = > { > NULL > }; > > +static const char * const git_stash_helper_create_usage[] = { > + N_("git stash--helper create []"), > + NULL > +}; > + > static const char *ref_stash = "refs/stash"; > static int quiet; > static struct strbuf stash_index_path = STRBUF_INIT; > @@ -781,6 +787,404 @@ static int store_stash(int argc, const char **argv, > const char *prefix) > return do_store_stash(argv[0], stash_msg, quiet); > } > > [...] > > + > +static int do_create_stash(int argc, const char **argv, const char *prefix, > +const char **stash_msg, int include_untracked, > +int patch_mode, struct stash_info *info) > +{ > + int untracked_commit_option = 0; > + int ret = 0; > + int subject_len; > + int flags; > + const char *head_short_sha1 = NULL; > + const char *branch_ref = NULL; > + const char *head_subject = NULL; > + const char *branch_name = "(no branch)"; > + struct commit *head_commit = NULL; > + struct commit_list *parents = NULL; > + struct strbuf msg = STRBUF_INIT; > + struct strbuf commit_tree_label = STRBUF_INIT; > + struct strbuf out = STRBUF_INIT; > + struct strbuf final_stash_msg = STRBUF_INIT; > + > + read_cache_preload(NULL); > + refresh_cache(REFRESH_QUIET); > + > + if (!check_changes(argv, include_untracked, prefix)) { > + ret = 1; > + goto done; I wonder if we can just 'exit(0)' here, instead of returning. This whole command is a builtin, and I *think* outside of 'libgit.a' exiting early is fine. It does mean that we're not free'ing the memory though, which means a leak checker would probably complain. So dunno. It would simplify the code a little, but not sure it's worth it. > + } > + > + if (get_oid("HEAD", >b_commit)) { > + fprintf_ln(stderr, "You do not have the initial commit yet"); > + ret = -1; > + goto done; > + } else { > + head_commit = lookup_commit(the_repository, >b_commit); > + } > + > + branch_ref = resolve_ref_unsafe("HEAD", 0, NULL, ); > + if (flags & REF_ISSYMREF) > + branch_name = strrchr(branch_ref, '/') + 1; > + head_short_sha1 = find_unique_abbrev(_commit->object.oid, > + DEFAULT_ABBREV); > + subject_len = find_commit_subject(get_commit_buffer(head_commit, NULL), > + _subject); > + strbuf_addf(, "%s: %s %.*s\n", branch_name, head_short_sha1, > + subject_len, head_subject); I think this can be written in a slightly simpler way: head_short_sha1 = find_unique_abbrev(_commit->object.oid, DEFAULT_ABBREV); strbuf_addf(, "%s: %s", branch_name, head_short_sha1); pp_commit_easy(CMIT_FMT_ONELINE, head_commit, ); strbuf_addch(, '\n'); The other advantage this brings is that it is consistent with other places where we print/use the subject of a commit (e.g. in 'git reset --hard'). > + > + strbuf_addf(_tree_label, "index on %s\n", msg.buf); > + commit_list_insert(head_commit, ); > + if (write_cache_as_tree(>i_tree, 0, NULL) || > + commit_tree(commit_tree_label.buf, commit_tree_label.len, > + >i_tree, parents, >i_commit, NULL, NULL)) { > + fprintf_ln(stderr, "Cannot save the current index state"); Looks like this message is translated in the current 'git stash' implementation, so it should be here as well. Same for the messages below. > + ret = -1; > + goto done; > + } > + > + if (include_untracked && get_untracked_files(argv, 1, > + include_untracked, )) { > + if (save_untracked_files(info, , )) { > + printf_ln("Cannot save the untracked files"); Why does this go to stdout, whereas "Cannot save the current index state" above goes to stderr? In the shell version of git stash these all go to stderr fwiw. There are a few similar cases, it would probably be worth going
Re: "less -F" is broken
On Wed, Aug 15, 2018 at 2:29 PM Ævar Arnfjörð Bjarmason wrote: > > Downloading & trying versions of it locally reveals that it's as of > version 520, not 530. The last version before 520 is 487. Presumably > it's covered by this item in the changelog: > > Don't output terminal init sequence if using -F and file fits on one > screen[1] Side note: that's sad, because we already use X in the default exactly for that reason. So apparently "less" was broken for us to fix something that we already had long solved. The code basically tried to do "automatic X when F is set". And all that line_count stuff (which is what breaks) is pointless when -X is already given. That does give a possible fix: just stop doing the line_count thing if no_init is set. So "-F" would continue to be broken, but "-FX" would work. Something like the attached patch, perhaps? Linus main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/main.c b/main.c index 179bd78..961a9db 100644 --- a/main.c +++ b/main.c @@ -59,6 +59,7 @@ extern int missing_cap; extern int know_dumb; extern int pr_type; extern int quit_if_one_screen; +extern int no_init; /* @@ -274,7 +275,7 @@ main(argc, argv) { if (edit_stdin()) /* Edit standard input */ quit(QUIT_ERROR); - if (quit_if_one_screen) + if (quit_if_one_screen && !no_init) line_count = get_line_count(); } else {
Re: "less -F" is broken
On Wed, Aug 15, 2018 at 2:29 PM Ævar Arnfjörð Bjarmason wrote: > > FWIW they're not linked from > http://www.greenwoodsoftware.com/less/download.html but you can just URL > hack and see releases http://www.greenwoodsoftware.com/less/ and change > links like http://www.greenwoodsoftware.com/less/less-530.tar.gz to > http://www.greenwoodsoftware.com/less/less-520.tar.gz I should have just tried that. I just downloaded the ones linked to, made a git archive of the history, and started bisecting. Which was all pointless extra work, since it was in the last release, but whatever. > > I've reported it as a bug in less, but I'm not sure what the reaction > > will be, the less releases seem to be very random. > > Via bug-l...@gnu.org? Heh. Another thing I didn't actually find. No, I just emailed Mark Nudelman directly, because that's what the FAQ says to do: "There is a list of known bugs here. If you find a bug that is not in the list, please send email to the author. Describe the bug in as much detail as possible, and I'll do what I can to help resolve the problem." and it doesn't mention any mailing list. > Is this report available online somewhere? It was not all that different from the email to the git list - just giving the trivial test-case and my (limited) bisection result. The data you dug up is much more useful. Linus
Re: [PATCH] gpg-interface.c: detect and reject multiple signatures on commits
Michał Górny wrote: > GnuPG supports creating signatures consisting of multiple signature > packets. If such a signature is verified, it outputs all the status > messages for each signature separately. However, git currently does not > account for such scenario and gets terribly confused over getting > multiple *SIG statuses. > > For example, if a malicious party alters a signed commit and appends > a new untrusted signature, git is going to ignore the original bad > signature and report untrusted commit instead. However, %GK and %GS > format strings may still expand to the data corresponding > to the original signature, potentially tricking the scripts into > trusting the malicious commit. > > Given that the use of multiple signatures is quite rare, git does not > support creating them without jumping through a few hoops, and finally > supporting them properly would require extensive API improvement, it > seems reasonable to just reject them at the moment. > --- Thanks for the clear analysis and fix. May we have your sign-off? See https://www.kernel.org/pub/software/scm/git/docs/SubmittingPatches.html#sign-off (or the equivalent section of your local copy of Documentation/SubmittingPatches) for what this means. > gpg-interface.c | 38 ++ > 1 file changed, 30 insertions(+), 8 deletions(-) > > diff --git a/gpg-interface.c b/gpg-interface.c > index 09ddfbc26..4e03aec15 100644 > --- a/gpg-interface.c > +++ b/gpg-interface.c > @@ -24,21 +24,23 @@ void signature_check_clear(struct signature_check *sigc) > static struct { > char result; > const char *check; > + int is_status; > } sigcheck_gpg_status[] = { > - { 'G', "\n[GNUPG:] GOODSIG " }, > - { 'B', "\n[GNUPG:] BADSIG " }, > - { 'U', "\n[GNUPG:] TRUST_NEVER" }, > - { 'U', "\n[GNUPG:] TRUST_UNDEFINED" }, > - { 'E', "\n[GNUPG:] ERRSIG "}, > - { 'X', "\n[GNUPG:] EXPSIG "}, > - { 'Y', "\n[GNUPG:] EXPKEYSIG "}, > - { 'R', "\n[GNUPG:] REVKEYSIG "}, > + { 'G', "\n[GNUPG:] GOODSIG ", 1 }, > + { 'B', "\n[GNUPG:] BADSIG ", 1 }, > + { 'U', "\n[GNUPG:] TRUST_NEVER", 0 }, > + { 'U', "\n[GNUPG:] TRUST_UNDEFINED", 0 }, > + { 'E', "\n[GNUPG:] ERRSIG ", 1}, > + { 'X', "\n[GNUPG:] EXPSIG ", 1}, > + { 'Y', "\n[GNUPG:] EXPKEYSIG ", 1}, > + { 'R', "\n[GNUPG:] REVKEYSIG ", 1}, > }; nit: I wonder if making is_status into a flag field (like 'option' in git.c's cmd_struct) and having an explicit SIGNATURE_STATUS value to put there would make this easier to read. It's not clear to me that the name is_status or SIGNATURE_STATUS captures what this field represents. Aren't these all sigcheck statuses? Can you describe briefly what distinguishes the cases where this should be 0 versus 1? > > static void parse_gpg_output(struct signature_check *sigc) > { > const char *buf = sigc->gpg_status; > int i; > + int had_status = 0; > > /* Iterate over all search strings */ > for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) { > @@ -50,6 +52,10 @@ static void parse_gpg_output(struct signature_check *sigc) > continue; > found += strlen(sigcheck_gpg_status[i].check); > } > + > + if (sigcheck_gpg_status[i].is_status) > + had_status++; > + > sigc->result = sigcheck_gpg_status[i].result; > /* The trust messages are not followed by key/signer > information */ > if (sigc->result != 'U') { > @@ -62,6 +68,22 @@ static void parse_gpg_output(struct signature_check *sigc) > } > } > } > + > + /* > + * GOODSIG, BADSIG etc. can occur only once for each signature. > + * Therefore, if we had more than one then we're dealing with multiple > + * signatures. We don't support them currently, and they're rather > + * hard to create, so something is likely fishy and we should reject > + * them altogether. > + */ > + if (had_status > 1) { > + sigc->result = 'E'; > + /* Clear partial data to avoid confusion */ > + if (sigc->signer) > + FREE_AND_NULL(sigc->signer); > + if (sigc->key) > + FREE_AND_NULL(sigc->key); > + } Makes sense to me. > } > > int check_signature(const char *payload, size_t plen, const char *signature, > -- > 2.18.0 Can we have a test to make sure this behavior doesn't regress? See t/README for an overview of the test framework and "git grep -e gpg t/" for some examples. The result looks good. Thanks again for writing it. Sincerely, Jonathan
Re: "less -F" is broken
On Wed, Aug 15 2018, Linus Torvalds wrote: > Sadly, as of less-530, the behavior of "less -F" is broken enough that > I think git needs to potentially think about changing the defaults for > the pager, or people should at least be aware of it. Downloading & trying versions of it locally reveals that it's as of version 520, not 530. The last version before 520 is 487. Presumably it's covered by this item in the changelog: Don't output terminal init sequence if using -F and file fits on one screen[1] > Older versions of less (at least up to less-487 - March 2017) do not > have this bug. There were apparently 520, 527 and 529 releases in > 2017 too, but I couldn't find their sources to verify if they were > already broken - but 530 (February 2018) has the problem. FWIW they're not linked from http://www.greenwoodsoftware.com/less/download.html but you can just URL hack and see releases http://www.greenwoodsoftware.com/less/ and change links like http://www.greenwoodsoftware.com/less/less-530.tar.gz to http://www.greenwoodsoftware.com/less/less-520.tar.gz > The breakage is easy to see without git: > > (echo "hello"; sleep 5; echo "bye bye") | less -F > > which will result in no output at all for five seconds, and then you > get both lines at once as "less" exits. The relevant change in less is this, cutting out the non-relevant parts: diff --git a/less-487/forwback.c b/less-520/forwback.c index 83ae78e..680fa25 100644 --- a/less-487/forwback.c +++ b/less-520/forwback.c [...] @@ -444,3 +444,21 @@ get_back_scroll() return (sc_height - 2); return (1); /* infinity */ } + +/* + * Return number of displayable lines in the file. + * Stop counting at screen height + 1. + */ + public int +get_line_count() +{ + int nlines; + POSITION pos = ch_zero(); + + for (nlines = 0; nlines <= sc_height; nlines++) + { + pos = forw_line(pos); + if (pos == NULL_POSITION) break; + } + return nlines; +} [...] diff --git a/less-487/main.c b/less-520/main.c index 960d120..6d54851 100644 --- a/less-487/main.c +++ b/less-520/main.c [...] @@ -273,10 +275,19 @@ main(argc, argv) { if (edit_stdin()) /* Edit standard input */ quit(QUIT_ERROR); + if (quit_if_one_screen) + line_count = get_line_count(); } else { if (edit_first()) /* Edit first valid file in cmd line */ quit(QUIT_ERROR); + if (quit_if_one_screen) + { + if (nifile() == 1) + line_count = get_line_count(); + else /* If more than one file, -F can not be used */ + quit_if_one_screen = FALSE; + } } init(); diff --git a/less-487/screen.c b/less-520/screen.c index ad3fca1..2d51bbc 100644 --- a/less-487/screen.c +++ b/less-520/screen.c [...] @@ -1538,7 +1555,9 @@ win32_deinit_term() init() { #if !MSDOS_COMPILER - if (!no_init) + if (quit_if_one_screen && line_count >= sc_height) + quit_if_one_screen = FALSE; + if (!no_init && !quit_if_one_screen) tputs(sc_init, sc_height, putchr); if (!no_keypad) tputs(sc_s_keypad, sc_height, putchr); If you undo that first changed part in main.c your test case prints "hello" to the terminal immediately. > It's not always obvious when using git, because when the terminal > fills up, less also starts outputting, but the default options with -F > are really horrible if you are looking for something uncommon, and > "git log" doesn't respond at all. > > On the kernel tree, this is easy to see with something like > >git log --oneline --grep="The most important one is the mpt3sas fix" > > which takes a bit over 7 seconds before it shows the commit I was looking for. > > In contrast, if you do > >LESS=-RX git log --oneline --grep="The most important one is the mpt3sas > fix" > > that (recent) commit is found and shown immediately. It still takes 7s > for git to go through all history and decide "that was it", but at > least you don't need to wait for the intermediate results. > > I've reported it as a bug in less, but I'm not sure what the reaction > will be, the less releases seem to be very random. Via bug-l...@gnu.org? Is this report available online somewhere? Anyway, CC-ing that address since my digging into this will be useful to them. 1. http://www.greenwoodsoftware.com/less/news.520.html
Re: [GSoC][PATCH v7 14/26] stash: convert store to builtin
On 08/08, Paul-Sebastian Ungureanu wrote: > Add stash store to the helper and delete the store_stash function > from the shell script. > > Add the usage string which was forgotten in the shell script. I think similarly to 'git stash create', which also doesn't appear in the usage, this was intentionally omitted in the shell script. The reason for the omission is that this is only intended to be useful in scripts, and not in interactive usage. As such it doesn't add much value in showing it in 'git stash -h'. Meanwhile it is in the synopsis in the man page. If we want to add it to the help output, I think it would be best to do so in a separate commit, and for 'git stash create' as well. But I'm not sure that's a good change. > Signed-off-by: Paul-Sebastian Ungureanu > --- > builtin/stash--helper.c | 52 + > git-stash.sh| 43 ++ > 2 files changed, 54 insertions(+), 41 deletions(-) > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index ec8c38c6f..5ff810f8c 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -20,6 +20,7 @@ static const char * const git_stash_helper_usage[] = { > N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] > []"), > N_("git stash--helper branch []"), > N_("git stash--helper clear"), > + N_("git stash--helper store [-m|--message ] [-q|--quiet] > "), > NULL > }; > > @@ -58,6 +59,11 @@ static const char * const git_stash_helper_clear_usage[] = > { > NULL > }; > > +static const char * const git_stash_helper_store_usage[] = { > + N_("git stash--helper store [-m|--message ] [-q|--quiet] > "), > + NULL > +}; > + > static const char *ref_stash = "refs/stash"; > static int quiet; > static struct strbuf stash_index_path = STRBUF_INIT; > @@ -731,6 +737,50 @@ static int show_stash(int argc, const char **argv, const > char *prefix) > return 0; > } > > +static int do_store_stash(const char *w_commit, const char *stash_msg, > + int quiet) > +{ > + int ret = 0; > + struct object_id obj; > + > + if (!stash_msg) > + stash_msg = xstrdup("Created via \"git stash--helper > store\"."); I assume we're going to s/--helper// in a later commit? Not sure adding the '--helper' here is necessary, as a user would never invoke 'git stash--helper' directly, so they would expect the stash to be created by 'git stash store'. Anyway that's fairly minor, as I assume this is going to change by the end of the patch series. > + > + ret = get_oid(w_commit, ); > + if (!ret) { > + ret = update_ref(stash_msg, ref_stash, , NULL, > + REF_FORCE_CREATE_REFLOG, > + quiet ? UPDATE_REFS_QUIET_ON_ERR : > + UPDATE_REFS_MSG_ON_ERR); > + } > + if (ret && !quiet) > + fprintf_ln(stderr, _("Cannot update %s with %s"), > +ref_stash, w_commit); > + > + return ret; > +} > + > +static int store_stash(int argc, const char **argv, const char *prefix) > +{ > + const char *stash_msg = NULL; > + struct option options[] = { > + OPT__QUIET(, N_("be quiet, only report errors")), > + OPT_STRING('m', "message", _msg, "message", N_("stash > message")), > + OPT_END() > + }; > + > + argc = parse_options(argc, argv, prefix, options, > + git_stash_helper_store_usage, > + PARSE_OPT_KEEP_UNKNOWN); > + > + if (argc != 1) { > + fprintf(stderr, _("\"git stash--helper store\" requires one > argument\n")); > + return -1; > + } > + > + return do_store_stash(argv[0], stash_msg, quiet); > +} > + > int cmd_stash__helper(int argc, const char **argv, const char *prefix) > { > pid_t pid = getpid(); > @@ -765,6 +815,8 @@ int cmd_stash__helper(int argc, const char **argv, const > char *prefix) > return !!list_stash(argc, argv, prefix); > else if (!strcmp(argv[0], "show")) > return !!show_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "store")) > + return !!store_stash(argc, argv, prefix); > > usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]), > git_stash_helper_usage, options); > diff --git a/git-stash.sh b/git-stash.sh > index 0d05cbc1e..5739c5152 100755 > --- a/git-stash.sh > +++ b/git-stash.sh > @@ -191,45 +191,6 @@ create_stash () { > die "$(gettext "Cannot record working tree state")" > } > > -store_stash () { > - while test $# != 0 > - do > - case "$1" in > - -m|--message) > - shift > - stash_msg="$1" > - ;; > - -m*) > - stash_msg=${1#-m} > - ;; > -
Re: "less -F" is broken
On Wed, Aug 15, 2018 at 1:35 PM Linus Torvalds wrote: > > Sadly, as of less-530, the behavior of "less -F" is broken enough that > I think git needs to potentially think about changing the defaults for > the pager, or people should at least be aware of it. > > Older versions of less (at least up to less-487 - March 2017) do not > have this bug. There were apparently 520, 527 and 529 releases in > 2017 too, but I couldn't find their sources to verify if they were > already broken - but 530 (February 2018) has the problem. http://www.greenwoodsoftware.com/less/news.527.html http://www.greenwoodsoftware.com/less/news.520.html http://www.greenwoodsoftware.com/less/ Release notes for 520 and 527 contains: "Don't output terminal init sequence if using -F and file fits on one screen."
Re: Potential vulnerability: 'mixed up' output when commit has multiple signatures
On Tue, 2018-08-14 at 22:35 -0700, Jonathan Nieder wrote: > Hi, > > Michał Górny wrote: > > > I've been testing the git signature verification a bit and I've > > discovered a troubling behavior when the commit object contains > > multiple signatures. > > Thanks for discovering this. Do you mind if I take this conversation > to the public mailing list? (I'd bounce the existing thread there if > that's okay with you.) > I've already asked somewhere else in the thread if you consider this suitable for disclosure, and haven't received a reply yet. In any case, I don't mind it. I can resend my patch there if necessary too. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: Potential vulnerability: 'mixed up' output when commit has multiple signatures
Michał Górny wrote: > On Tue, 2018-08-14 at 22:35 -0700, Jonathan Nieder wrote: > > Michał Górny wrote: >>> I've been testing the git signature verification a bit and I've >>> discovered a troubling behavior when the commit object contains >>> multiple signatures. >> >> Thanks for discovering this. Do you mind if I take this conversation >> to the public mailing list? (I'd bounce the existing thread there if >> that's okay with you.) > > I've already asked somewhere else in the thread if you consider this > suitable for disclosure, and haven't received a reply yet. In any case, > I don't mind it. Thanks, doing so. Thanks again for the analysis and fix as well.
Re: [GSoC][PATCH v7 13/26] stash: update `git stash show` documentation
On 08/08, Paul-Sebastian Ungureanu wrote: > Add in documentation about the change of behavior regarding > the `--quiet` option, which was introduced in the last commit. > (the `--quiet` option does not exit anymore with erorr if it s/erorr/error/ > is given an empty stash as argument) If we want to keep the change in behaviour here (which I'm not sure about as mentioned in my comment on the previous patch), I think this should be folded into the previous patch. I don't think there's much value in having this as a separate commit, and folding it into the previous commit has the advantage that we can easily see that the new behaviour is documented. > Signed-off-by: Paul-Sebastian Ungureanu > --- > Documentation/git-stash.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt > index e31ea7d30..d60ebdb96 100644 > --- a/Documentation/git-stash.txt > +++ b/Documentation/git-stash.txt > @@ -117,6 +117,9 @@ show [] []:: > You can use stash.showStat and/or stash.showPatch config variables > to change the default behavior. > > + It accepts any option known to `git diff`, but acts different on I notice that we are using single quotes for git commands in some places and backticks in other places in this man page. We may want to clean that up at some point. I wouldn't want to do it in this series though, as this is already long enough, and we've had this inconsistency for a while already. > + `--quiet` option and exit with zero regardless of differences. > + > pop [--index] [-q|--quiet] []:: > > Remove a single stashed state from the stash list and apply it > -- > 2.18.0.573.g56500d98f >
Re: "Changes not staged for commit" after cloning a repo on macOS
On Wed, Aug 15, 2018 at 1:50 PM Hadi Safari wrote: > > Hi everyone! > > I encountered a strange situation on OS X recently. I cloned a > repository (https://github.com/kevinxucs/Sublime-Gitignore.git), went to > folder, and saw "Changes not staged for commit" message for four > specific files. It happened every time I repeated the procedure. I even > was able to commit those to the repo. > At first I thought there's something wrong with repo, but I cloned it on > Ubuntu 16.04 and everything was OK; no "Changes not staged for commit" > message. > > Does anyone have any idea? > > modified: boilerplates/Nanoc.gitignore > modified: boilerplates/OpenCart.gitignore > modified: boilerplates/SASS.gitignore > modified: boilerplates/WordPress.gitignore Taking a look at the repository's file list on GitHub[1], it shows that this is because HFS and APFS by default are case-insensitive. The file listing shows that there is a "nanoc.gitignore" and "Nanoc.gitignore". On APFS and HFS, those are the same file. As a result, one overwrites the other. This is discussed pretty regularly on the list[2], so you can find more details there. [1]: https://github.com/kevinxucs/Sublime-Gitignore/tree/master/boilerplates [2]: https://public-inbox.org/git/24a09b73-b4d4-4c22-bc1b-41b22cb59...@gmail.com/ is a fairly recent (fairly long) thread about this behavior. Hope this helps! Bryan
Re: [PATCH v2] checkout: optimize "git checkout -b "
On 8/6/2018 10:25 AM, Ben Peart wrote: On 8/3/2018 11:58 AM, Duy Nguyen wrote: On Thu, Aug 02, 2018 at 02:02:00PM -0400, Ben Peart wrote: But if you still want to push it further, this is something I have in mind. It probably has bugs, but at least preliminary test shows me that it could skip 99% work inside unpack_trees() and not need to write the index. The main check to determine "checkout -b" is basically the new oidcmp() in merge_working_tree(). Not sure if I miss any condition in that check, I didn't look very closely at checkout code. Thanks Duy. I think this is an interesting idea to pursue but... when I tried running this patch on a virtualized repo it started triggering many object downloads. After taking a quick look, it appears that CE_UPDATE is set on every cache entry so check_updates() ends up calling checkout_entry() which writes out every file to the working tree - even those supposedly skipped by the skip-wortree bit. Oops. Not too surprising (you did say it probably has bugs :)) but it means I can't trivially get performance data on how much this will help. It also fails a lot of tests (see below). It experience does highlight the additional risk of this model of changing the underlying functions (vs the high level optimization of my original patch). In addition, the new special cases in those lower-level functions do add additional complexity and fragility to the codebase. So, like most things, to me it isn't a clear better/worse decision - it's just different. While I like the idea of general optimizations that could apply more broadly to other commands; I do worry about the additional complexity, amount of code churn, and associated risk with the change. When I have cycles, I'll take a look at how to fix this bug and get some performance data. I just wanted to give you a heads up that I'm not ignoring your patch, just that it is going to take additional time and effort before I can properly evaluate how much impact it will have. Now that the unpack-trees and cache-tree optimizations are settling down, I took a look at this proposed patch again with the intent of debugging why so many tests were broken by it. The most obvious first fix was for all the segment faults when dereferencing a NULL pointer. Adding an additional test so that we only perform the optimization when we actually have commit ID's to compare fixed a bunch of the test failures. The next fix was to resolve all the breaks caused by applying this optimization when sparse-checkout is turned on. Since we are skipping the logic to update the skip-worktree bit, I added an additional test so that we only perform the optimization when sparse checkout is not turned on. Of course, this does completely remove the optimization when using sparse checkout so it isn't a workable permanent solution but it let me make progress. There are still test failures with submodules and partial clone. I haven't found/added the necessary tests to prevent those breaks nor the few other remaining breaks. My current set of tests looks like this: if (!core_apply_sparse_checkout && old_branch_info->commit && new_branch_info->commit && !oidcmp(_branch_info->commit->object.oid, _branch_info->commit->object.oid)) { While I'm sure I could find and add additional tests to handle the remaining bugs, the net result is starting to look as fragile as the original patch. Unfortunately it has the additional downsides of 1) being at a much lower level where we risk breaking more code paths and 2) not being nearly as much savings (with the original patch checkout -b takes 0.3 seconds, this patch will make it take >4 seconds.) Net, net - I don't think this particular path is a better path to pursue. I understand the concern with the fragility of the current patch and it's set of tests to determine if the optimization is valid. I also understand the concern with the potential change in behavior (ie not showing the local changes - even though nothing has changed). Other than switching the optimization back to be "opt-in" via a config flag, I don't currently have a great answer. I'll keep thinking and looking but am open to suggestions! Test Summary Report --- ./t1011-read-tree-sparse-checkout.sh (Wstat: 256 Tests: 21 Failed: 1) Failed test: 20 Non-zero exit status: 1 ./t1400-update-ref.sh (Wstat: 256 Tests: 170 Failed: 73) Failed tests: 40, 42-45, 55-59, 70, 72, 82, 85, 87-88 90-100, 103-110, 113-119, 127, 129-130 132-133, 136-137, 140-147, 150-157, 160-166 170 Non-zero exit status: 1 ./t2011-checkout-invalid-head.sh (Wstat: 256 Tests: 10 Failed: 5) Failed tests: 3, 6-7, 9-10 Non-zero exit status: 1 ./t2015-checkout-unborn.sh
Re: [GSoC][PATCH v7 12/26] stash: refactor `show_stash()` to use the diff API
On 08/08, Paul-Sebastian Ungureanu wrote: > Currently, `show_stash()` uses `cmd_diff()` to generate > the output. After this commit, the output will be generated > using the internal API. > > Before this commit, `git stash show --quiet` would act like > `git diff` and error out if the stash is not empty. Now, the > `--quiet` option does not error out given an empty stash. I think this needs a bit more justification. As I mentioned in my comment to a previous patch, I'm not sure '--quiet' makes much sense with 'git stash show' (it will show nothing, and will always exit with an error code, as the stash will always contain something), but if we are supporting the same flags as 'git diff', and essentially just forwarding them, shouldn't they keep the same behaviour as well? > Signed-off-by: Paul-Sebastian Ungureanu > --- > builtin/stash--helper.c | 73 + > 1 file changed, 45 insertions(+), 28 deletions(-) > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index 0c1efca6b..ec8c38c6f 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -10,6 +10,8 @@ > #include "run-command.h" > #include "dir.h" > #include "rerere.h" > +#include "revision.h" > +#include "log-tree.h" > > static const char * const git_stash_helper_usage[] = { > N_("git stash--helper list []"), > @@ -662,56 +664,71 @@ static int git_stash_config(const char *var, const char > *value, void *cb) > > static int show_stash(int argc, const char **argv, const char *prefix) > { > - int i, ret = 0; > - struct child_process cp = CHILD_PROCESS_INIT; > - struct argv_array args_refs = ARGV_ARRAY_INIT; > + int i; > + int flags = 0; > struct stash_info info; > + struct rev_info rev; > + struct argv_array stash_args = ARGV_ARRAY_INIT; > struct option options[] = { > OPT_END() > }; > > - argc = parse_options(argc, argv, prefix, options, > - git_stash_helper_show_usage, > - PARSE_OPT_KEEP_UNKNOWN); > + init_diff_ui_defaults(); > + git_config(git_diff_ui_config, NULL); > > - cp.git_cmd = 1; > - argv_array_push(, "diff"); > + init_revisions(, prefix); > > - /* Push arguments which are not options into args_refs. */ > - for (i = 0; i < argc; ++i) { > - if (argv[i][0] == '-') > - argv_array_push(, argv[i]); > + /* Push arguments which are not options into stash_args. */ > + for (i = 1; i < argc; ++i) { > + if (argv[i][0] != '-') > + argv_array_push(_args, argv[i]); > else > - argv_array_push(_refs, argv[i]); > - } > - > - if (get_stash_info(, args_refs.argc, args_refs.argv)) { > - child_process_clear(); > - argv_array_clear(_refs); > - return -1; > + flags++; > } > > /* >* The config settings are applied only if there are not passed >* any flags. >*/ > - if (cp.args.argc == 1) { > + if (!flags) { > git_config(git_stash_config, NULL); > if (show_stat) > - argv_array_push(, "--stat"); > + rev.diffopt.output_format |= DIFF_FORMAT_DIFFSTAT; > + if (show_patch) { > + rev.diffopt.output_format = ~DIFF_FORMAT_NO_OUTPUT; > + rev.diffopt.output_format |= DIFF_FORMAT_PATCH; > + } I failed to notice this in the previous patch (the problem existed there as well), but this changes the behaviour of 'git -c stash.showStat=false stash show '. Previously doing this would not show anything, which is the correct behaviour, while now still shows the diffstat. I think the show_stat variable is interpreted the wrong way around in the previous patch. Something else I noticed now that I was playing around more with the config options is that the parsing of the config options is not correctly done in the previous patch. It does a 'strcmp(var, "stash.showStat"))', but the config API makes all variables lowercase (config options are case insensitive, and making everything lowercase is the way to ensure that), so it should be 'strcmp(var, "stash.showstat"))', and similar for the 'stash.showpatch' config option. This all sounds like it would be nice to have some tests for these config options, to make sure we get it right, and won't break them in the future. > + } > > - if (show_patch) > - argv_array_push(, "-p"); > + if (get_stash_info(, stash_args.argc, stash_args.argv)) { > + argv_array_clear(_args); > + return -1; > } > > - argv_array_pushl(, oid_to_hex(_commit), > - oid_to_hex(_commit), NULL); > + argc = setup_revisions(argc, argv, , NULL); > + if (!rev.diffopt.output_format) > +
[PATCH v2] worktree: add --quiet option
Add the '--quiet' option to git worktree, as for the other git commands. 'add' is the only command affected by it since all other commands, except 'list', are currently silent by default. Helped-by: Martin Ågren Helped-by: Duy Nguyen Helped-by: Eric Sunshine Signed-off-by: Elia Pinto --- This is the second version of the patch. Changes from the first version (https://public-inbox.org/git/CACsJy8A=zp7nfbuwyfep4uff3kssiaor3m0mtgvnhceyhsw...@mail.gmail.com/T/): - deleted garbage in git-worktree.c and deleted superfluous blank line in git-worktree.txt. - when giving "--quiet" to 'add', call git symbolic-ref also with "--quiet". - changed the commit message to be more general, but specifying why the "--quiet" option is meaningful only for the 'add' command of git-worktree. - in git-worktree.txt the option "--quiet" is described near the "--verbose" option. Documentation/git-worktree.txt | 4 builtin/worktree.c | 16 +--- t/t2025-worktree-add.sh| 5 + 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 9c26be40f..29a5b7e25 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -173,6 +173,10 @@ This can also be set up as the default behaviour by using the This format will remain stable across Git versions and regardless of user configuration. See below for details. +-q:: +--quiet:: + With 'add', suppress feedback messages. + -v:: --verbose:: With `prune`, report all removals. diff --git a/builtin/worktree.c b/builtin/worktree.c index a763dbdcc..41e771439 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -27,6 +27,7 @@ static const char * const worktree_usage[] = { struct add_opts { int force; int detach; + int quiet; int checkout; int keep_locked; }; @@ -303,9 +304,13 @@ static int add_worktree(const char *path, const char *refname, if (!is_branch) argv_array_pushl(, "update-ref", "HEAD", oid_to_hex(>object.oid), NULL); - else + else { argv_array_pushl(, "symbolic-ref", "HEAD", symref.buf, NULL); + if (opts->quiet) + argv_array_push(, "--quiet"); + } + cp.env = child_env.argv; ret = run_command(); if (ret) @@ -315,6 +320,8 @@ static int add_worktree(const char *path, const char *refname, cp.argv = NULL; argv_array_clear(); argv_array_pushl(, "reset", "--hard", NULL); + if (opts->quiet) + argv_array_push(, "--quiet"); cp.env = child_env.argv; ret = run_command(); if (ret) @@ -437,6 +444,7 @@ static int add(int ac, const char **av, const char *prefix) OPT_BOOL(0, "detach", , N_("detach HEAD at named commit")), OPT_BOOL(0, "checkout", , N_("populate the new working tree")), OPT_BOOL(0, "lock", _locked, N_("keep the new working tree locked")), + OPT__QUIET(, N_("suppress progress reporting")), OPT_PASSTHRU(0, "track", _track, NULL, N_("set up tracking mode (see git-branch(1))"), PARSE_OPT_NOARG | PARSE_OPT_OPTARG), @@ -491,8 +499,8 @@ static int add(int ac, const char **av, const char *prefix) } } } - - print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force); + if (!opts.quiet) + print_preparing_worktree_line(opts.detach, branch, new_branch, !!new_branch_force); if (new_branch) { struct child_process cp = CHILD_PROCESS_INIT; @@ -500,6 +508,8 @@ static int add(int ac, const char **av, const char *prefix) argv_array_push(, "branch"); if (new_branch_force) argv_array_push(, "--force"); + if (opts.quiet) + argv_array_push(, "--quiet"); argv_array_push(, new_branch); argv_array_push(, branch); if (opt_track) diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh index be6e09314..658647d83 100755 --- a/t/t2025-worktree-add.sh +++ b/t/t2025-worktree-add.sh @@ -252,6 +252,11 @@ test_expect_success 'add -B' ' test_cmp_rev master^ poodle ' +test_expect_success 'add --quiet' ' + git worktree add --quiet ../foo master >expected 2>&1 && + test_must_be_empty expected +' + test_expect_success 'local clone from linked checkout' ' git clone --local here here-clone && ( cd here-clone && git fsck ) -- 2.18.0.723.g64e6cc43e.dirty
"Changes not staged for commit" after cloning a repo on macOS
Hi everyone! I encountered a strange situation on OS X recently. I cloned a repository (https://github.com/kevinxucs/Sublime-Gitignore.git), went to folder, and saw "Changes not staged for commit" message for four specific files. It happened every time I repeated the procedure. I even was able to commit those to the repo. At first I thought there's something wrong with repo, but I cloned it on Ubuntu 16.04 and everything was OK; no "Changes not staged for commit" message. Does anyone have any idea? Thank you. Log: ``` $ git clone https://github.com/kevinxucs/Sublime-Gitignore.git Cloning into 'Sublime-Gitignore'... remote: Counting objects: 515, done. remote: Total 515 (delta 0), reused 0 (delta 0), pack-reused 515 Receiving objects: 100% (515/515), 102.14 KiB | 48.00 KiB/s, done. Resolving deltas: 100% (143/143), done. $ cd Sublime-Gitignore/ $ git status On branch master Your branch is up to date with 'origin/master'. Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git checkout -- ..." to discard changes in working directory) modified: boilerplates/Nanoc.gitignore modified: boilerplates/OpenCart.gitignore modified: boilerplates/SASS.gitignore modified: boilerplates/WordPress.gitignore no changes added to commit (use "git add" and/or "git commit -a") ``` The rest of the log is available at https://github.com/kevinxucs/Sublime-Gitignore/issues/6. (I don't want this email to become too long.) -- Hadi Safari http://hadisafari.ir/
Re: [PATCHv4 0/6] Add missing includes and forward declares
Elijah Newren wrote: > 62 files changed, 152 insertions(+), 18 deletions(-) All 6 patches in this series are Reviewed-by: Jonathan Nieder Thanks for your patient work. Pointer to previous rounds for the curious: https://public-inbox.org/git/20180811043218.31456-1-new...@gmail.com/
Re: [PATCHv4 6/6] Remove forward declaration of an enum
Elijah Newren wrote: > According to http://c-faq.com/null/machexamp.html, sizeof(char*) != > sizeof(int*) on some platforms. Since an enum could be a char or int > (or long or...), knowing the size of the enum thus is important to > knowing the size of a pointer to an enum, so we cannot just forward > declare an enum the way we can a struct. (Also, modern C++ compilers > apparently define forward declarations of an enum to either be useless > because the enum was defined, or require an explicit size specifier, or > be a compilation error.) Beyond the effect on some obscure platforms, this also makes it possible to build with gcc -pedantic (which can be useful for finding some other problems). Thanks for fixing it. [...] > --- a/packfile.h > +++ b/packfile.h > @@ -1,12 +1,12 @@ > #ifndef PACKFILE_H > #define PACKFILE_H > > +#include "cache.h" > #include "oidset.h" > > /* in object-store.h */ > struct packed_git; > struct object_info; Not about this patch: comments like the above are likely to go stale, since nothing verifies they continue to be true. So we should remove them. #leftoverbits Reviewed-by: Jonathan Nieder
Re: [PATCHv4 2/6] alloc: make allocate_alloc_state and clear_alloc_state more consistent
Elijah Newren wrote: > Since both functions are using the same data type, they should either both > refer to it as void *, or both use the real type (struct alloc_state *). > Opt for the latter. > > Reviewed-by: Jonathan Nieder > Signed-off-by: Elijah Newren Not worth rerolling for this, but these usually go in the opposite order to reflect the chronology: Signed-off-by: Elijah Newren Reviewed-by: Jonathan Nieder The patch still looks good to me. :)
Re: [PATCHv4 1/6] Add missing includes and forward declarations
Elijah Newren wrote: [...] > Signed-off-by: Elijah Newren > --- [...] > 55 files changed, 132 insertions(+), 4 deletions(-) Reviewed-by: Jonathan Nieder Thanks.
"less -F" is broken
Sadly, as of less-530, the behavior of "less -F" is broken enough that I think git needs to potentially think about changing the defaults for the pager, or people should at least be aware of it. Older versions of less (at least up to less-487 - March 2017) do not have this bug. There were apparently 520, 527 and 529 releases in 2017 too, but I couldn't find their sources to verify if they were already broken - but 530 (February 2018) has the problem. The breakage is easy to see without git: (echo "hello"; sleep 5; echo "bye bye") | less -F which will result in no output at all for five seconds, and then you get both lines at once as "less" exits. It's not always obvious when using git, because when the terminal fills up, less also starts outputting, but the default options with -F are really horrible if you are looking for something uncommon, and "git log" doesn't respond at all. On the kernel tree, this is easy to see with something like git log --oneline --grep="The most important one is the mpt3sas fix" which takes a bit over 7 seconds before it shows the commit I was looking for. In contrast, if you do LESS=-RX git log --oneline --grep="The most important one is the mpt3sas fix" that (recent) commit is found and shown immediately. It still takes 7s for git to go through all history and decide "that was it", but at least you don't need to wait for the intermediate results. I've reported it as a bug in less, but I'm not sure what the reaction will be, the less releases seem to be very random. Linus
Re: [GSoC][PATCH v7 11/26] stash: change `git stash show` usage text and documentation
> Subject: stash: change `git stash show` usage text and documentation Another nitpick about commit messages. "change ... usage text and documentation" doesn't say much about what the actual change is. How about something like "stash: mention options in "show" synopsis" instead? The change itself looks good to me, thanks! On 08/08, Paul-Sebastian Ungureanu wrote: > It is already stated in documentation that it will accept any > option known to `git diff`, but not in the usage text and some > parts of the documentation. > > Signed-off-by: Paul-Sebastian Ungureanu > --- > Documentation/git-stash.txt | 4 ++-- > builtin/stash--helper.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt > index 7ef8c4791..e31ea7d30 100644 > --- a/Documentation/git-stash.txt > +++ b/Documentation/git-stash.txt > @@ -9,7 +9,7 @@ SYNOPSIS > > [verse] > 'git stash' list [] > -'git stash' show [] > +'git stash' show [] [] > 'git stash' drop [-q|--quiet] [] > 'git stash' ( pop | apply ) [--index] [-q|--quiet] [] > 'git stash' branch [] > @@ -106,7 +106,7 @@ stash@{1}: On master: 9cc0589... Add git-stash > The command takes options applicable to the 'git log' > command to control what is shown and how. See linkgit:git-log[1]. > > -show []:: > +show [] []:: > > Show the changes recorded in the stash entry as a diff between the > stashed contents and the commit back when the stash entry was first > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index e764cd33e..0c1efca6b 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -13,7 +13,7 @@ > > static const char * const git_stash_helper_usage[] = { > N_("git stash--helper list []"), > - N_("git stash--helper show []"), > + N_("git stash--helper show [] []"), > N_("git stash--helper drop [-q|--quiet] []"), > N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] > []"), > N_("git stash--helper branch []"), > @@ -27,7 +27,7 @@ static const char * const git_stash_helper_list_usage[] = { > }; > > static const char * const git_stash_helper_show_usage[] = { > - N_("git stash--helper show []"), > + N_("git stash--helper show [] []"), > NULL > }; > > -- > 2.18.0.573.g56500d98f >
Re: [GSoC][PATCH v7 10/26] stash: convert show to builtin
On 08/08, Paul-Sebastian Ungureanu wrote: > Add stash show to the helper and delete the show_stash, have_stash, > assert_stash_like, is_stash_like and parse_flags_and_rev functions > from the shell script now that they are no longer needed. > > Before this commit, `git stash show` would ignore `--index` and > `--quiet` options. Now, `git stash show` errors out on `--index` > and does not display any message on `--quiet`, but errors out > if the stash is not empty. I think "errors out" is slightly misleading here. Maybe "but exits with an exit code similar to 'git diff'" instead? Looking at why we ignored them before, it's because we filtered them out in 'parse_flags_and_rev', which looks more accidental than intentional, and I think we could consider a bug, so this change in behaviour here is okay. '--quiet' doesn't make too much sense to use with 'git stash show', so I'm not sure whether or not it makes sense to support it at all. But we do promise to pass all options through to in our documentation, so the new behaviour is what we are documenting. > Signed-off-by: Paul-Sebastian Ungureanu > --- > builtin/stash--helper.c | 78 > git-stash.sh| 132 +--- > 2 files changed, 79 insertions(+), 131 deletions(-) > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index daa4d0034..e764cd33e 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -13,6 +13,7 @@ > > static const char * const git_stash_helper_usage[] = { > N_("git stash--helper list []"), > + N_("git stash--helper show []"), > N_("git stash--helper drop [-q|--quiet] []"), > N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] > []"), > N_("git stash--helper branch []"), > @@ -25,6 +26,11 @@ static const char * const git_stash_helper_list_usage[] = { > NULL > }; > > +static const char * const git_stash_helper_show_usage[] = { > + N_("git stash--helper show []"), > + NULL > +}; > + > static const char * const git_stash_helper_drop_usage[] = { > N_("git stash--helper drop [-q|--quiet] []"), > NULL > @@ -638,6 +644,76 @@ static int list_stash(int argc, const char **argv, const > char *prefix) > return run_command(); > } > > +static int show_stat = 1; > +static int show_patch; > + > +static int git_stash_config(const char *var, const char *value, void *cb) > +{ > + if (!strcmp(var, "stash.showStat")) { > + show_stat = git_config_bool(var, value); > + return 0; > + } > + if (!strcmp(var, "stash.showPatch")) { > + show_patch = git_config_bool(var, value); > + return 0; > + } > + return git_default_config(var, value, cb); > +} > + > +static int show_stash(int argc, const char **argv, const char *prefix) > +{ > + int i, ret = 0; > + struct child_process cp = CHILD_PROCESS_INIT; > + struct argv_array args_refs = ARGV_ARRAY_INIT; > + struct stash_info info; > + struct option options[] = { > + OPT_END() > + }; > + > + argc = parse_options(argc, argv, prefix, options, > + git_stash_helper_show_usage, > + PARSE_OPT_KEEP_UNKNOWN); > + > + cp.git_cmd = 1; > + argv_array_push(, "diff"); > + > + /* Push arguments which are not options into args_refs. */ > + for (i = 0; i < argc; ++i) { > + if (argv[i][0] == '-') > + argv_array_push(, argv[i]); > + else > + argv_array_push(_refs, argv[i]); > + } > + > + if (get_stash_info(, args_refs.argc, args_refs.argv)) { > + child_process_clear(); > + argv_array_clear(_refs); > + return -1; > + } > + > + /* > + * The config settings are applied only if there are not passed > + * any flags. > + */ > + if (cp.args.argc == 1) { > + git_config(git_stash_config, NULL); > + if (show_stat) > + argv_array_push(, "--stat"); > + > + if (show_patch) > + argv_array_push(, "-p"); > + } > + > + argv_array_pushl(, oid_to_hex(_commit), > + oid_to_hex(_commit), NULL); > + > + ret = run_command(); > + > + free_stash_info(); > + argv_array_clear(_refs); > + return ret; > +} > + > int cmd_stash__helper(int argc, const char **argv, const char *prefix) > { > pid_t pid = getpid(); > @@ -670,6 +746,8 @@ int cmd_stash__helper(int argc, const char **argv, const > char *prefix) > return !!branch_stash(argc, argv, prefix); > else if (!strcmp(argv[0], "list")) > return !!list_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "show")) > + return !!show_stash(argc, argv, prefix); > > usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]), >
Re: [PATCH 00/24] Kill the_index part3
On Mon, Aug 13, 2018 at 2:24 PM Junio C Hamano wrote: > > Brandon Williams writes: > > > On 08/13, Nguyễn Thái Ngọc Duy wrote: > >> This is the third part of killing the_index (at least outside > >> builtin/). Part 1 [1] is dropped. Part 2 is nd/no-extern on 'pu'. This > >> part is built on top of nd/no-extern. > >> > >> This series would actually break 'pu' because builtin/stash.c uses > >> three functions that are updated here. So we would need something like > >> the following patch to make it build again. > >> > >> I don't know if that adds too much work on Junio. If it does, I guess > >> I'll hold this off for a while until builtin/stash.c gets merged > >> because reordering these patches, pushing the patches that break > >> stash.c away, really takes a lot of work. > >> > >> [1] https://public-inbox.org/git/20180616054157.32433-1-pclo...@gmail.com/ > > > > I went through and found this to be a pleasant read and hopefully others > > agree with the approach this series took vs what your part 1 did so that > > we can get this change in. > > Yeah, I've only finished my first pass (read: I didn't go through > the patches with fine toothed comb, nor thought about interactions > with other topics), but this round was quite a pleasnt read so far. > I went through this topic with a finer comb and just found nits, none of which I would deem a complete show stopper.
Re: [GSoC][PATCH v7 09/26] stash: implement the "list" command in the builtin
> Subject: stash: implement the "list" command in the builtin Nit: The previous commit messages all have the format "stash: convert to builtin", maybe follow the same pattern here? The rest of the patch looks good to me. On 08/08, Paul-Sebastian Ungureanu wrote: > Add stash list to the helper and delete the list_stash function > from the shell script. > > Signed-off-by: Paul-Sebastian Ungureanu > --- > builtin/stash--helper.c | 31 +++ > git-stash.sh| 7 +-- > 2 files changed, 32 insertions(+), 6 deletions(-) > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c > index d6bd468e0..daa4d0034 100644 > --- a/builtin/stash--helper.c > +++ b/builtin/stash--helper.c > @@ -12,6 +12,7 @@ > #include "rerere.h" > > static const char * const git_stash_helper_usage[] = { > + N_("git stash--helper list []"), > N_("git stash--helper drop [-q|--quiet] []"), > N_("git stash--helper ( pop | apply ) [--index] [-q|--quiet] > []"), > N_("git stash--helper branch []"), > @@ -19,6 +20,11 @@ static const char * const git_stash_helper_usage[] = { > NULL > }; > > +static const char * const git_stash_helper_list_usage[] = { > + N_("git stash--helper list []"), > + NULL > +}; > + > static const char * const git_stash_helper_drop_usage[] = { > N_("git stash--helper drop [-q|--quiet] []"), > NULL > @@ -609,6 +615,29 @@ static int branch_stash(int argc, const char **argv, > const char *prefix) > return ret; > } > > +static int list_stash(int argc, const char **argv, const char *prefix) > +{ > + struct child_process cp = CHILD_PROCESS_INIT; > + struct option options[] = { > + OPT_END() > + }; > + > + argc = parse_options(argc, argv, prefix, options, > + git_stash_helper_list_usage, > + PARSE_OPT_KEEP_UNKNOWN); > + > + if (!ref_exists(ref_stash)) > + return 0; > + > + cp.git_cmd = 1; > + argv_array_pushl(, "log", "--format=%gd: %gs", "-g", > + "--first-parent", "-m", NULL); > + argv_array_pushv(, argv); > + argv_array_push(, ref_stash); > + argv_array_push(, "--"); > + return run_command(); > +} > + > int cmd_stash__helper(int argc, const char **argv, const char *prefix) > { > pid_t pid = getpid(); > @@ -639,6 +668,8 @@ int cmd_stash__helper(int argc, const char **argv, const > char *prefix) > return !!pop_stash(argc, argv, prefix); > else if (!strcmp(argv[0], "branch")) > return !!branch_stash(argc, argv, prefix); > + else if (!strcmp(argv[0], "list")) > + return !!list_stash(argc, argv, prefix); > > usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]), > git_stash_helper_usage, options); > diff --git a/git-stash.sh b/git-stash.sh > index 8f2640fe9..6052441aa 100755 > --- a/git-stash.sh > +++ b/git-stash.sh > @@ -382,11 +382,6 @@ have_stash () { > git rev-parse --verify --quiet $ref_stash >/dev/null > } > > -list_stash () { > - have_stash || return 0 > - git log --format="%gd: %gs" -g --first-parent -m "$@" $ref_stash -- > -} > - > show_stash () { > ALLOW_UNKNOWN_FLAGS=t > assert_stash_like "$@" > @@ -574,7 +569,7 @@ test -n "$seen_non_option" || set "push" "$@" > case "$1" in > list) > shift > - list_stash "$@" > + git stash--helper list "$@" > ;; > show) > shift > -- > 2.18.0.573.g56500d98f >
Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems
Torsten Bögershausen writes: >> + >> +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */ >> +for (i = 0; i < state->istate->cache_nr; i++) { >> +struct cache_entry *dup = state->istate->cache[i]; >> + >> +if (dup == ce) >> +break; >> + >> +if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE)) >> +continue; >> + > > Should the following be protected by core.checkstat ? > if (check_stat) { I do not think such a if statement is strictly necessary. Even if check_stat tells us "when checking if a cached stat information tells us that the path may have modified, use minimum set of fields from the 'struct stat'", we still capture and update the values from the same "full" set of fields when we mark a cache entry up-to-date. So it all depends on why you are limiting with check_stat. Is it because stdev is unusable? Is it because nsec is unusable? Is it because ino is unusable? Only in the last case, paying attention to check_stat will reduce the false positive. But then you made me wonder what value check_stat has on Windows. If it is false, perhaps we do not even need the conditional compilation, which is a huge plus. >> +if (dup->ce_stat_data.sd_ino == st->st_ino) { >> +dup->ce_flags |= CE_MATCHED; >> +break; >> +} >> +} >> +#endif > > Another thing is that we switch of the ASCII case-folding-detection-logic > off for Windows users, even if we otherwise rely on icase. > I think we can use fspathcmp() as a fallback. when inodes fail, > because we may be on a network file system. > > (I don't have a test setup at the moment, but what happens with inodes > when a Windows machine exports a share to Linux or Mac ?) > > Is there a chance to get the fspathcmp() back, like this ? If fspathcmp() never gives false positives, I do not think we would mind using it like your update. False negatives are fine, as that is better than just punting the whole thing when there is no usable inum. And we do not care all that much if it is more expensive; this is an error codepath after all. And from code structure's point of view, I think it makes sense. It would be even better if we can lose the conditional compilation. Another thing we maybe want to see is if we can update the caller of this function so that we do not overwrite the earlier checkout with the data for this path. When two paths collide, we check out one of the paths without reporting (because we cannot notice), then attempt to check out the other path and report (because we do notice the previous one with lstat()). The current code then goes on and overwrites the file with the contents from the "other" path. Even if we had false negative in this loop, if we leave the contents for the earlier path while reporting the "other" path, then the user can get curious, inspect what contents the "other" path has on the filesystem, and can notice that it belongs to the (unreported--due to false negative) earlier path. > static void mark_colliding_entries(const struct checkout *state, > struct cache_entry *ce, struct stat *st) > { > int i; > ce->ce_flags |= CE_MATCHED; > > for (i = 0; i < state->istate->cache_nr; i++) { > struct cache_entry *dup = state->istate->cache[i]; > int folded = 0; > > if (dup == ce) > break; > > if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE)) > continue; > > if (!fspathcmp(dup->name, ce->name)) > folded = 1; > > #if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */ > if (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino)) > folded = 1; > #endif > if (folded) { > dup->ce_flags |= CE_MATCHED; > break; > } > } > }
Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems
On Wed, Aug 15, 2018 at 9:08 PM Torsten Bögershausen wrote: > > +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */ > > + for (i = 0; i < state->istate->cache_nr; i++) { > > + struct cache_entry *dup = state->istate->cache[i]; > > + > > + if (dup == ce) > > + break; > > + > > + if (dup->ce_flags & (CE_MATCHED | CE_VALID | > > CE_SKIP_WORKTREE)) > > + continue; > > + > > Should the following be protected by core.checkstat ? > if (check_stat) { Good catch! st_ino is ignored if core.checkStat is false. I will probably send a separate patch to add more details to config.txt about this key. > > + if (dup->ce_stat_data.sd_ino == st->st_ino) { > > + dup->ce_flags |= CE_MATCHED; > > + break; > > + } > > + } > > +#endif > > Another thing is that we switch of the ASCII case-folding-detection-logic > off for Windows users, even if we otherwise rely on icase. > I think we can use fspathcmp() as a fallback. when inodes fail, > because we may be on a network file system. I admit I did not think about network file system. Will spend some time (and hopefully not on nfs kernel code) on it. For falling back on fspathcmp even on Windows, is it really safe? I'm on Linux and never have to deal with this issue to have any experience. It does sound good though because it should be a subset for any "weird" filesystems out there. > (I don't have a test setup at the moment, but what happens with inodes > when a Windows machine exports a share to Linux or Mac ?) > > Is there a chance to get the fspathcmp() back, like this ? > > static void mark_colliding_entries(const struct checkout *state, >struct cache_entry *ce, struct stat *st) > { > int i; > ce->ce_flags |= CE_MATCHED; > > for (i = 0; i < state->istate->cache_nr; i++) { > struct cache_entry *dup = state->istate->cache[i]; > int folded = 0; > > if (dup == ce) > break; > > if (dup->ce_flags & (CE_MATCHED | CE_VALID | > CE_SKIP_WORKTREE)) > continue; > > if (!fspathcmp(dup->name, ce->name)) > folded = 1; > > #if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */ > if (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino)) > folded = 1; > #endif > if (folded) { > dup->ce_flags |= CE_MATCHED; > break; > } > } > } > -- Duy
Re: [GSoC][PATCH v7 04/26] stash: renamed test cases to be more descriptive
> Subject: Re: [GSoC][PATCH v7 04/26] stash: renamed test cases to be more > descriptive Please use the imperative mood in the title and the commit messages themselves. From Documentation/SubmittingPatches: Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behavior. >From a quick skim over the rest of the series, this also applies to some of the subsequent patches in the series. On 08/08, Paul-Sebastian Ungureanu wrote: > Renamed some test cases' labels to be more descriptive and under 80 > characters per line. > > Signed-off-by: Paul-Sebastian Ungureanu > --- > t/t3903-stash.sh | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index de6cab1fe..8d002a7f2 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -604,7 +604,7 @@ test_expect_success 'stash show -p - no stashes on stack, > stash-like argument' ' > test_cmp expected actual > ' > > -test_expect_success 'stash drop - fail early if specified stash is not a > stash reference' ' > +test_expect_success 'drop: fail early if specified stash is not a stash ref' > ' > git stash clear && > test_when_finished "git reset --hard HEAD && git stash clear" && > git reset --hard && > @@ -618,7 +618,7 @@ test_expect_success 'stash drop - fail early if specified > stash is not a stash r > git reset --hard HEAD > ' > > -test_expect_success 'stash pop - fail early if specified stash is not a > stash reference' ' > +test_expect_success 'pop: fail early if specified stash is not a stash ref' ' > git stash clear && > test_when_finished "git reset --hard HEAD && git stash clear" && > git reset --hard && > @@ -682,7 +682,7 @@ test_expect_success 'invalid ref of the form "n", n >= N' > ' > git stash drop > ' > > -test_expect_success 'stash branch should not drop the stash if the branch > exists' ' > +test_expect_success 'branch: should not drop the stash if the branch exists' > ' Since we're adjusting the titles of the tests here I'll allow myself to nitpick a little :) Maybe "branch: do not drop the stash if the branch exists", which sounds more like an assertion, as the "pop" and "drop" titles above. > git stash clear && > echo foo >file && > git add file && > @@ -693,7 +693,7 @@ test_expect_success 'stash branch should not drop the > stash if the branch exists > git rev-parse stash@{0} -- > ' > > -test_expect_success 'stash branch should not drop the stash if the apply > fails' ' > +test_expect_success 'branch: should not drop the stash if the apply fails' ' > git stash clear && > git reset HEAD~1 --hard && > echo foo >file && > @@ -707,7 +707,7 @@ test_expect_success 'stash branch should not drop the > stash if the apply fails' > git rev-parse stash@{0} -- > ' > > -test_expect_success 'stash apply shows status same as git status (relative > to current directory)' ' > +test_expect_success 'apply: shows same status as git status (relative to > ./)' ' s/shows/show/ above maybe? This used to be a full sentence previously, where 'shows' was appropriate, but I think "show" sounds better after the colon. > git stash clear && > echo 1 >subdir/subfile1 && > echo 2 >subdir/subfile2 && > @@ -1048,7 +1048,7 @@ test_expect_success 'stash push -p with pathspec shows > no changes only once' ' > test_i18ncmp expect actual > ' > > -test_expect_success 'stash push with pathspec shows no changes when there > are none' ' > +test_expect_success 'push: shows no changes when there are none' ' Maybe "push : show no changes when there are none"? "push " would be the rest of the 'git stash' command, having the colon in between them seems a bit odd. > >foo && > git add foo && > git commit -m "tmp" && > @@ -1058,7 +1058,7 @@ test_expect_success 'stash push with pathspec shows no > changes when there are no > test_i18ncmp expect actual > ' > > -test_expect_success 'stash push with pathspec not in the repository errors > out' ' > +test_expect_success 'push: not in the repository errors out' ' This one makes sense to me. > >untracked && > test_must_fail git stash push untracked && > test_path_is_file untracked > -- > 2.18.0.573.g56500d98f >
Re: [PATCH 08/24] unpack-trees: remove 'extern' on function declaration
On Wed, Aug 15, 2018 at 12:21 PM Duy Nguyen wrote: > > On Wed, Aug 15, 2018 at 9:10 PM Stefan Beller wrote: > > > > On Mon, Aug 13, 2018 at 9:15 AM Nguyễn Thái Ngọc Duy > > wrote: > > > > > > Signed-off-by: Nguyễn Thái Ngọc Duy > > > > This removes the only existing extern keyword, which was added by > > Linus in 933bf40a5c6 (Start moving unpack-trees to "struct tree_desc", > > 2007-08-09). All other callers do not have this noise word as it was > > simply never > > present there despite the old age of unpack-trees.h. Interesting history. > > Linus did not add 'extern' though. It was Johannes a year ago in > 16da134b1f (read-trees: refactor the unpack_trees() part - > 2006-07-30). Man this function is _old_. Ah, yes. I stopped at the first blame here but dug down on other functions as I expected some of the recent "remove externs here" and magically overlook this function.
Re: [PATCH 08/24] unpack-trees: remove 'extern' on function declaration
On Wed, Aug 15, 2018 at 9:10 PM Stefan Beller wrote: > > On Mon, Aug 13, 2018 at 9:15 AM Nguyễn Thái Ngọc Duy > wrote: > > > > Signed-off-by: Nguyễn Thái Ngọc Duy > > This removes the only existing extern keyword, which was added by > Linus in 933bf40a5c6 (Start moving unpack-trees to "struct tree_desc", > 2007-08-09). All other callers do not have this noise word as it was > simply never > present there despite the old age of unpack-trees.h. Interesting history. Linus did not add 'extern' though. It was Johannes a year ago in 16da134b1f (read-trees: refactor the unpack_trees() part - 2006-07-30). Man this function is _old_. -- Duy
Re: [PATCH] rebase -i: fix numbering in squash message
Phillip Wood writes: >> I wonder if it makes it easier to read, understand and maintain if >> there were a local variable that gets opts->current_fixup_count+2 at >> the beginning of the function, make these three places refer to that >> variable, and move the increment of opts->current_fixup_count down >> in the function, after the "if we are squashing, do this, if we are >> fixing up, do that, otherwise, we do not know what we are doing" >> cascade. And use the more common post-increment, as we no longer >> depend on the returned value while at it. >> >> IOW, something like this (untested), on top of yours. > > I think you'd need to change commit_staged_changes() as well as it > relies on the current_fixup_count counting the number of fixups, not the > number of fixups plus two. I suspect you misread what I wrote (see below for the patch). The fixup_count is a new local variable in update_squash_messages() that holds N+2; in other words, I am not suggesting to change what opts->current_fixup_count means. > Having said that using 'current_fixup_count + > 2' to create the labels and incrementing the count at the end of > update_squash_messages() would probably be clearer than my version. I'm > about to go away so it'll probably be the second week of September > before I can re-roll this, will that be too late for getting it into 2.19? I actually do not mind to go with what you originally sent, unless a cleaned up version is vastly more readable. After all, a clean-up can be done separately and safely. Thanks. >> sequencer.c | 10 ++ >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index 77d3c2346f..f82c283a89 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -1331,8 +1331,9 @@ static int update_squash_messages(enum todo_command >> command, >> struct strbuf buf = STRBUF_INIT; >> int res; >> const char *message, *body; >> +int fixup_count = opts->current_fixup_count + 2; >> >> -if (opts->current_fixup_count > 0) { >> +if (fixup_count > 2) { >> struct strbuf header = STRBUF_INIT; >> char *eol; >> >> @@ -1345,7 +1346,7 @@ static int update_squash_messages(enum todo_command >> command, >> >> strbuf_addf(, "%c ", comment_line_char); >> strbuf_addf(, _("This is a combination of %d commits."), >> -opts->current_fixup_count + 2); >> +fixup_count); >> strbuf_splice(, 0, eol - buf.buf, header.buf, header.len); >> strbuf_release(); >> } else { >> @@ -1387,18 +1388,19 @@ static int update_squash_messages(enum todo_command >> command, >> unlink(rebase_path_fixup_msg()); >> strbuf_addf(, "\n%c ", comment_line_char); >> strbuf_addf(, _("This is the commit message #%d:"), >> -++opts->current_fixup_count + 1); >> +fixup_count); >> strbuf_addstr(, "\n\n"); >> strbuf_addstr(, body); >> } else if (command == TODO_FIXUP) { >> strbuf_addf(, "\n%c ", comment_line_char); >> strbuf_addf(, _("The commit message #%d will be skipped:"), >> -++opts->current_fixup_count + 1); >> +fixup_count); >> strbuf_addstr(, "\n\n"); >> strbuf_add_commented_lines(, body, strlen(body)); >> } else >> return error(_("unknown command: %d"), command); >> unuse_commit_buffer(commit, message); >> +opts->current_fixup_count++; >> >> res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0); >> strbuf_release(); >> >>
Re: [PATCH 08/24] unpack-trees: remove 'extern' on function declaration
On Mon, Aug 13, 2018 at 9:15 AM Nguyễn Thái Ngọc Duy wrote: > > Signed-off-by: Nguyễn Thái Ngọc Duy This removes the only existing extern keyword, which was added by Linus in 933bf40a5c6 (Start moving unpack-trees to "struct tree_desc", 2007-08-09). All other callers do not have this noise word as it was simply never present there despite the old age of unpack-trees.h. Interesting history. Thanks! Stefan
Re: [PATCH v4] clone: report duplicate entries on case-insensitive filesystems
On Sun, Aug 12, 2018 at 11:07:14AM +0200, Nguyễn Thái Ngọc Duy wrote: > Paths that only differ in case work fine in a case-sensitive > filesystems, but if those repos are cloned in a case-insensitive one, > you'll get problems. The first thing to notice is "git status" will > never be clean with no indication what exactly is "dirty". > > This patch helps the situation a bit by pointing out the problem at > clone time. Even though this patch talks about case sensitivity, the > patch makes no assumption about folding rules by the filesystem. It > simply observes that if an entry has been already checked out at clone > time when we're about to write a new path, some folding rules are > behind this. > > This patch is tested with vim-colorschemes repository on a JFS partition > with case insensitive support on Linux. This repository has two files > darkBlue.vim and darkblue.vim. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > v4 removes nr_duplicates (and fixes that false warning Szeder > reported). It also hints about case insensitivity as a cause of > problem because it's most likely the case when this warning shows up. > > builtin/clone.c | 1 + > cache.h | 1 + > entry.c | 28 > t/t5601-clone.sh | 8 +++- > unpack-trees.c | 28 > unpack-trees.h | 1 + > 6 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index 5c439f1394..0702b0e9d0 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -747,6 +747,7 @@ static int checkout(int submodule_progress) > memset(, 0, sizeof opts); > opts.update = 1; > opts.merge = 1; > + opts.clone = 1; > opts.fn = oneway_merge; > opts.verbose_update = (option_verbosity >= 0); > opts.src_index = _index; > diff --git a/cache.h b/cache.h > index 8b447652a7..6d6138f4f1 100644 > --- a/cache.h > +++ b/cache.h > @@ -1455,6 +1455,7 @@ struct checkout { > unsigned force:1, >quiet:1, >not_new:1, > + clone:1, >refresh_cache:1; > }; > #define CHECKOUT_INIT { NULL, "" } > diff --git a/entry.c b/entry.c > index b5d1d3cf23..c70340df8e 100644 > --- a/entry.c > +++ b/entry.c > @@ -399,6 +399,31 @@ static int check_path(const char *path, int len, struct > stat *st, int skiplen) > return lstat(path, st); > } > > +static void mark_colliding_entries(const struct checkout *state, > +struct cache_entry *ce, struct stat *st) > +{ > + int i; > + > + ce->ce_flags |= CE_MATCHED; > + > +#if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */ > + for (i = 0; i < state->istate->cache_nr; i++) { > + struct cache_entry *dup = state->istate->cache[i]; > + > + if (dup == ce) > + break; > + > + if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE)) > + continue; > + Should the following be protected by core.checkstat ? if (check_stat) { > + if (dup->ce_stat_data.sd_ino == st->st_ino) { > + dup->ce_flags |= CE_MATCHED; > + break; > + } > + } > +#endif Another thing is that we switch of the ASCII case-folding-detection-logic off for Windows users, even if we otherwise rely on icase. I think we can use fspathcmp() as a fallback. when inodes fail, because we may be on a network file system. (I don't have a test setup at the moment, but what happens with inodes when a Windows machine exports a share to Linux or Mac ?) Is there a chance to get the fspathcmp() back, like this ? static void mark_colliding_entries(const struct checkout *state, struct cache_entry *ce, struct stat *st) { int i; ce->ce_flags |= CE_MATCHED; for (i = 0; i < state->istate->cache_nr; i++) { struct cache_entry *dup = state->istate->cache[i]; int folded = 0; if (dup == ce) break; if (dup->ce_flags & (CE_MATCHED | CE_VALID | CE_SKIP_WORKTREE)) continue; if (!fspathcmp(dup->name, ce->name)) folded = 1; #if !defined(GIT_WINDOWS_NATIVE) /* inode is always zero on Windows */ if (check_stat && (dup->ce_stat_data.sd_ino == st->st_ino)) folded = 1; #endif if (folded) { dup->ce_flags |= CE_MATCHED; break; } } }
Re: [PATCH 07/24] ls-files: correct index argument to get_convert_attr_ascii()
On Mon, Aug 13, 2018 at 9:15 AM Nguyễn Thái Ngọc Duy wrote: > > write_eolinfo() does take an istate as function argument and it should > be used instead of the_index. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > builtin/ls-files.c | 17 + > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/builtin/ls-files.c b/builtin/ls-files.c > index 7233b92794..7f9919a362 100644 > --- a/builtin/ls-files.c > +++ b/builtin/ls-files.c > @@ -63,7 +63,7 @@ static void write_eolinfo(const struct index_state *istate, > struct stat st; > const char *i_txt = ""; > const char *w_txt = ""; > - const char *a_txt = get_convert_attr_ascii(_index, path); > + const char *a_txt = get_convert_attr_ascii(istate, path); Going by the commit message this patch should end here? > -static void show_dir_entry(const char *tag, struct dir_entry *ent) > +static void show_dir_entry(const struct index_state *istate, > + const char *tag, struct dir_entry *ent) [...] > - if (!dir_path_match(_index, ent, , len, ps_matched)) > + if (!dir_path_match(istate, ent, , len, ps_matched)) [...] > - write_eolinfo(NULL, NULL, ent->name); > + write_eolinfo(istate, NULL, ent->name); but here we need to pass through the istate, which is why we adjust the dir_path_match while we're here > - show_dir_entry(tag_other, ent); > + show_dir_entry(istate, tag_other, ent); [...] > - show_dir_entry(tag_killed, dir->entries[i]); > + show_dir_entry(istate, tag_killed, dir->entries[i]); and having to adjust more callers here > @@ -228,7 +229,7 @@ static void show_ce(struct repository *repo, struct > dir_struct *dir, > - } else if (match_pathspec(_index, , fullname, > strlen(fullname), > + } else if (match_pathspec(repo->index, , fullname, > strlen(fullname), > @@ -264,7 +265,7 @@ static void show_ru_info(const struct index_state *istate) > - if (!match_pathspec(_index, , path, len, > + if (!match_pathspec(istate, , path, len, These seem more or less unrelated to the commit message or the code changes above. Maybe mention these as a "while at it" or separate them out in their own commit? thanks, Stefan
[PATCH v3 3/6] chainlint: recognize multi-line $(...) when command cuddled with "$("
For multi-line $(...) expressions nested within subshells, chainlint.sed only recognizes: x=$( echo foo && ... but it is not unlikely that test authors may also cuddle the command with the opening "$(", so support that style, as well: x=$(echo foo && ... The closing ")" is already correctly recognized when cuddled or not. Signed-off-by: Eric Sunshine --- t/chainlint.sed | 2 +- .../multi-line-nested-command-substitution.expect | 11 ++- .../multi-line-nested-command-substitution.test | 11 ++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 07c624fe09..a21c4b4d71 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -216,7 +216,7 @@ s/.*\n// # "$(...)" -- command substitution; not closing ")" /\$([^)][^)]*)[^)]*$/bcheckchain # multi-line "$(...\n...)" -- command substitution; treat as nested subshell -/\$([ ]*$/bnest +/\$([^)]*$/bnest # "=(...)" -- Bash array assignment; not closing ")" /=(/bcheckchain # closing "...) &&" diff --git a/t/chainlint/multi-line-nested-command-substitution.expect b/t/chainlint/multi-line-nested-command-substitution.expect index 19c023b1c8..59b6c8b850 100644 --- a/t/chainlint/multi-line-nested-command-substitution.expect +++ b/t/chainlint/multi-line-nested-command-substitution.expect @@ -6,4 +6,13 @@ >> ) && echo ok >) | -sort +sort && +( + bar && + x=$(echo bar | + cat +>> ) && + y=$(echo baz | +>> fip) && + echo fail +>) diff --git a/t/chainlint/multi-line-nested-command-substitution.test b/t/chainlint/multi-line-nested-command-substitution.test index ca0620ab6b..300058341b 100644 --- a/t/chainlint/multi-line-nested-command-substitution.test +++ b/t/chainlint/multi-line-nested-command-substitution.test @@ -6,4 +6,13 @@ ) && echo ok ) | -sort +sort && +( + bar && + x=$(echo bar | + cat + ) && + y=$(echo baz | + fip) && + echo fail +) -- 2.18.0.267.gbc8be36ecb
[PATCH v3 6/6] chainlint: add test of pathological case which triggered false positive
This extract from contrib/subtree/t7900 triggered a false positive due to three chainlint limitations: * recognizing only a "blessed" set of here-doc tag names in a subshell ("EOF", "EOT", "INPUT_END"), of which "TXT" is not a member * inability to recognize multi-line $(...) when the first statement of the body is cuddled with the opening "$(" * inability to recognize multiple constructs on a single line, such as opening a multi-line $(...) and starting a here-doc Now that all of these shortcomings have been addressed, turn this rather pathological bit of shell coding into a chainlint test case. Signed-off-by: Eric Sunshine --- t/chainlint/t7900-subtree.expect | 10 ++ t/chainlint/t7900-subtree.test | 22 ++ 2 files changed, 32 insertions(+) create mode 100644 t/chainlint/t7900-subtree.expect create mode 100644 t/chainlint/t7900-subtree.test diff --git a/t/chainlint/t7900-subtree.expect b/t/chainlint/t7900-subtree.expect new file mode 100644 index 00..c9913429e6 --- /dev/null +++ b/t/chainlint/t7900-subtree.expect @@ -0,0 +1,10 @@ +( + chks="sub1sub2sub3sub4" && + chks_sub=$(cat | sed 's,^,sub dir/,' +>>) && + chkms="main-sub1main-sub2main-sub3main-sub4" && + chkms_sub=$(cat | sed 's,^,sub dir/,' +>>) && + subfiles=$(git ls-files) && + check_equal "$subfiles" "$chkms$chks" +>) diff --git a/t/chainlint/t7900-subtree.test b/t/chainlint/t7900-subtree.test new file mode 100644 index 00..277d8358df --- /dev/null +++ b/t/chainlint/t7900-subtree.test @@ -0,0 +1,22 @@ +( + chks="sub1 +sub2 +sub3 +sub4" && + chks_sub=$(cat <
[PATCH v3 5/6] chainlint: recognize multi-line quoted strings more robustly
chainlint.sed recognizes multi-line quoted strings within subshells: echo "abc def" >out && so it can avoid incorrectly classifying lines internal to the string as breaking the &&-chain. To identify the first line of a multi-line string, it checks if the line contains a single quote. However, this is fragile and can be easily fooled by a line containing multiple strings: echo "xyz" "abc def" >out && Make detection more robust by checking for an odd number of quotes rather than only a single one. (Escaped quotes are not handled, but support may be added later.) The original multi-line string recognizer rather cavalierly threw away all but the final quote, whereas the new one is careful to retain all quotes, so the "expected" output of a couple existing chainlint tests is updated to account for this new behavior. Signed-off-by: Eric Sunshine --- t/chainlint.sed | 32 +-- t/chainlint/here-doc-multi-line-string.expect | 2 +- t/chainlint/multi-line-string.expect | 10 -- t/chainlint/multi-line-string.test| 12 +++ 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 41cb6ef865..1da58b554b 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -151,10 +151,10 @@ s/.*\n// :slurp # incomplete line "...\" /\\$/bincomplete -# multi-line quoted string "...\n..." -/^[^"]*"[^"]*$/bdqstring -# multi-line quoted string '...\n...' (but not contraction in string "it's so") -/^[^']*'[^']*$/{ +# multi-line quoted string "...\n..."? +/"/bdqstring +# multi-line quoted string '...\n...'? (but not contraction in string "it's") +/'/{ /"[^'"]*'[^'"]*"/!bsqstring } :folded @@ -250,20 +250,32 @@ N s/\\\n// bslurp -# found multi-line double-quoted string "...\n..." -- slurp until end of string +# check for multi-line double-quoted string "...\n..." -- fold to one line :dqstring -s/"//g +# remove all quote pairs +s/"\([^"]*\)"/@!\1@!/g +# done if no dangling quote +/"/!bdqdone +# otherwise, slurp next line and try again N s/\n// -/"/!bdqstring +bdqstring +:dqdone +s/@!/"/g bfolded -# found multi-line single-quoted string '...\n...' -- slurp until end of string +# check for multi-line single-quoted string '...\n...' -- fold to one line :sqstring -s/'//g +# remove all quote pairs +s/'\([^']*\)'/@!\1@!/g +# done if no dangling quote +/'/!bsqdone +# otherwise, slurp next line and try again N s/\n// -/'/!bsqstring +bsqstring +:sqdone +s/@!/'/g bfolded # found here-doc -- swallow it to avoid false hits within its body (but keep diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect index 1e5b724b9d..32038a070c 100644 --- a/t/chainlint/here-doc-multi-line-string.expect +++ b/t/chainlint/here-doc-multi-line-string.expect @@ -1,4 +1,4 @@ ( -?!AMP?!cat && echo multi-line string" +?!AMP?!cat && echo "multi-line string" bap >) diff --git a/t/chainlint/multi-line-string.expect b/t/chainlint/multi-line-string.expect index 8334c4cc8e..170cb59993 100644 --- a/t/chainlint/multi-line-string.expect +++ b/t/chainlint/multi-line-string.expect @@ -1,9 +1,15 @@ ( - x=line 1line 2 line 3" && -?!AMP?!y=line 1line2' + x="line 1 line 2 line 3" && +?!AMP?!y='line 1 line2' foobar >) && ( echo "there's nothing to see here" && exit +>) && +( + echo "xyz" "abc def ghi" && + echo 'xyz' 'abc def ghi' && + echo 'xyz' "abc def ghi" && + barfoo >) diff --git a/t/chainlint/multi-line-string.test b/t/chainlint/multi-line-string.test index 14cb44d51c..287ab89705 100644 --- a/t/chainlint/multi-line-string.test +++ b/t/chainlint/multi-line-string.test @@ -12,4 +12,16 @@ # LINT: starting multi-line single-quoted string echo "there's nothing to see here" && exit +) && +( + echo "xyz" "abc + def + ghi" && + echo 'xyz' 'abc + def + ghi' && + echo 'xyz' "abc + def + ghi" && + barfoo ) -- 2.18.0.267.gbc8be36ecb
[PATCH v3 2/6] chainlint: match quoted here-doc tags
A here-doc tag can be quoted ('EOF'/"EOF") or escaped (\EOF) to suppress interpolation within the body. Although, chainlint recognizes escaped tags, it does not know about quoted tags. For completeness, teach it to recognize quoted tags, as well. Signed-off-by: Eric Sunshine --- t/chainlint.sed | 8 t/chainlint/here-doc.expect | 4 t/chainlint/here-doc.test| 14 ++ t/chainlint/subshell-here-doc.expect | 2 ++ t/chainlint/subshell-here-doc.test | 8 5 files changed, 32 insertions(+), 4 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 2af1a687f8..07c624fe09 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -94,8 +94,8 @@ # here-doc -- swallow it to avoid false hits within its body (but keep the # command to which it was attached) -/<<[ ]*[-\\]*[A-Za-z0-9_]/ { - s/^\(.*\)<<[]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1<\1<\1<\1bar && + +cat >boo && + horticulture diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test index 8986eefe74..ad4ce8afd9 100644 --- a/t/chainlint/here-doc.test +++ b/t/chainlint/here-doc.test @@ -14,6 +14,20 @@ boz woz Arbitrary_Tag_42 +# LINT: swallow 'quoted' here-doc +cat <<'FUMP' >bar && +snoz +boz +woz +FUMP + +# LINT: swallow "quoted" here-doc +cat <<"zump" >boo && +snoz +boz +woz +zump + # LINT: swallow here-doc (EOF is last line of test) horticulture <<\EOF gomez diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect index 7c2da63bc7..74723e7340 100644 --- a/t/chainlint/subshell-here-doc.expect +++ b/t/chainlint/subshell-here-doc.expect @@ -5,5 +5,7 @@ >) && ( cat >bup && + cat >bup2 && + cat >bup3 && meep >) diff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test index 05139af0b5..f6b3ba4214 100644 --- a/t/chainlint/subshell-here-doc.test +++ b/t/chainlint/subshell-here-doc.test @@ -27,5 +27,13 @@ glink FIZZ ARBITRARY + cat <<-'ARBITRARY2' >bup2 && + glink + FIZZ + ARBITRARY2 + cat <<-"ARBITRARY3" >bup3 && + glink + FIZZ + ARBITRARY3 meep ) -- 2.18.0.267.gbc8be36ecb
[PATCH v3 4/6] chainlint: let here-doc and multi-line string commence on same line
After swallowing a here-doc, chainlint.sed assumes that no other processing needs to be done on the line aside from checking for &&-chain breakage; likewise, after folding a multi-line quoted string. However, it's conceivable (even if unlikely in practice) that both a here-doc and a multi-line quoted string might commence on the same line: cat <<\EOF && echo "foo bar" data EOF Support this case by sending the line (after swallowing and folding) through the normal processing sequence rather than jumping directly to the check for broken &&-chain. This change also allows other somewhat pathological cases to be handled, such as closing a subshell on the same line starting a here-doc: ( cat <<-\INPUT) data INPUT or, for instance, opening a multi-line $(...) expression on the same line starting a here-doc: x=$(cat <<-\END && data END echo "x") among others. Signed-off-by: Eric Sunshine --- t/chainlint.sed | 7 --- t/chainlint/here-doc-close-subshell.expect | 2 ++ t/chainlint/here-doc-close-subshell.test | 5 + t/chainlint/here-doc-multi-line-command-subst.expect | 5 + t/chainlint/here-doc-multi-line-command-subst.test | 9 + t/chainlint/here-doc-multi-line-string.expect| 4 t/chainlint/here-doc-multi-line-string.test | 8 7 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 t/chainlint/here-doc-close-subshell.expect create mode 100644 t/chainlint/here-doc-close-subshell.test create mode 100644 t/chainlint/here-doc-multi-line-command-subst.expect create mode 100644 t/chainlint/here-doc-multi-line-command-subst.test create mode 100644 t/chainlint/here-doc-multi-line-string.expect create mode 100644 t/chainlint/here-doc-multi-line-string.test diff --git a/t/chainlint.sed b/t/chainlint.sed index a21c4b4d71..41cb6ef865 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -157,6 +157,7 @@ s/.*\n// /^[^']*'[^']*$/{ /"[^'"]*'[^'"]*"/!bsqstring } +:folded # here-doc -- swallow it /<<[ ]*[-\\'"]*[A-Za-z0-9_]/bheredoc # comment or empty line -- discard since final non-comment, non-empty line @@ -255,7 +256,7 @@ s/"//g N s/\n// /"/!bdqstring -bcheckchain +bfolded # found multi-line single-quoted string '...\n...' -- slurp until end of string :sqstring @@ -263,7 +264,7 @@ s/'//g N s/\n// /'/!bsqstring -bcheckchain +bfolded # found here-doc -- swallow it to avoid false hits within its body (but keep # the command to which it was attached) @@ -278,7 +279,7 @@ N } s/^<[^>]*>// s/\n.*$// -bcheckchain +bfolded # found "case ... in" -- pass through untouched :case diff --git a/t/chainlint/here-doc-close-subshell.expect b/t/chainlint/here-doc-close-subshell.expect new file mode 100644 index 00..f011e335e5 --- /dev/null +++ b/t/chainlint/here-doc-close-subshell.expect @@ -0,0 +1,2 @@ +( +> cat) diff --git a/t/chainlint/here-doc-close-subshell.test b/t/chainlint/here-doc-close-subshell.test new file mode 100644 index 00..b857ff5467 --- /dev/null +++ b/t/chainlint/here-doc-close-subshell.test @@ -0,0 +1,5 @@ +( +# LINT: line contains here-doc and closes nested subshell + cat <<-\INPUT) + fizz + INPUT diff --git a/t/chainlint/here-doc-multi-line-command-subst.expect b/t/chainlint/here-doc-multi-line-command-subst.expect new file mode 100644 index 00..e5fb752d2f --- /dev/null +++ b/t/chainlint/here-doc-multi-line-command-subst.expect @@ -0,0 +1,5 @@ +( + x=$(bobble && +?!AMP?!>> wiffle) + echo $x +>) diff --git a/t/chainlint/here-doc-multi-line-command-subst.test b/t/chainlint/here-doc-multi-line-command-subst.test new file mode 100644 index 00..899bc5de8b --- /dev/null +++ b/t/chainlint/here-doc-multi-line-command-subst.test @@ -0,0 +1,9 @@ +( +# LINT: line contains here-doc and opens multi-line $(...) + x=$(bobble <<-\END && + fossil + vegetable + END + wiffle) + echo $x +) diff --git a/t/chainlint/here-doc-multi-line-string.expect b/t/chainlint/here-doc-multi-line-string.expect new file mode 100644 index 00..1e5b724b9d --- /dev/null +++ b/t/chainlint/here-doc-multi-line-string.expect @@ -0,0 +1,4 @@ +( +?!AMP?!cat && echo multi-line string" + bap +>) diff --git a/t/chainlint/here-doc-multi-line-string.test b/t/chainlint/here-doc-multi-line-string.test new file mode 100644 index 00..a53edbcc8d --- /dev/null +++ b/t/chainlint/here-doc-multi-line-string.test @@ -0,0 +1,8 @@ +( +# LINT: line contains here-doc and opens multi-line string + cat <<-\TXT && echo "multi-line + string" + fizzle + TXT + bap +) -- 2.18.0.267.gbc8be36ecb
[PATCH v3 1/6] chainlint: match arbitrary here-docs tags rather than hard-coded names
chainlint.sed swallows top-level here-docs to avoid being fooled by content which might look like start-of-subshell. It likewise swallows here-docs in subshells to avoid marking content lines as breaking the &&-chain, and to avoid being fooled by content which might look like end-of-subshell, start-of-nested-subshell, or other specially-recognized constructs. At the time of implementation, it was believed that it was not possible to support arbitrary here-doc tag names since 'sed' provides no way to stash the opening tag name in a variable for later comparison against a line signaling end-of-here-doc. Consequently, tag names are hard-coded, with "EOF" being the only tag recognized at the top-level, and only "EOF", "EOT", and "INPUT_END" being recognized within subshells. Also, special care was taken to avoid being confused by here-docs nested within other here-docs. In practice, this limited number of hard-coded tag names has been "good enough" for the 13000+ existing Git test, despite many of those tests using tags other than the recognized ones, since the bodies of those here-docs do not contain content which would fool the linter. Nevertheless, the situation is not ideal since someone writing new tests, and choosing a name not in the "blessed" set could potentially trigger a false-positive. To address this shortcoming, upgrade chainlint.sed to handle arbitrary here-doc tag names, both at the top-level and within subshells. Signed-off-by: Eric Sunshine --- t/chainlint.sed | 57 +--- t/chainlint/here-doc.expect | 2 + t/chainlint/here-doc.test| 7 t/chainlint/nested-here-doc.expect | 2 + t/chainlint/nested-here-doc.test | 10 + t/chainlint/subshell-here-doc.expect | 4 ++ t/chainlint/subshell-here-doc.test | 8 7 files changed, 67 insertions(+), 23 deletions(-) diff --git a/t/chainlint.sed b/t/chainlint.sed index 5f0882cb38..2af1a687f8 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -61,6 +61,22 @@ # "else", and "fi" in if-then-else likewise must not end with "&&", thus # receives similar treatment. # +# Swallowing here-docs with arbitrary tags requires a bit of finesse. When a +# line such as "catout". +# As each subsequent line is read, it is appended to the target line and a +# (whitespace-loose) back-reference match /^<(.*)>\n\1$/ is attempted to see if +# the content inside "<...>" matches the entirety of the newly-read line. For +# instance, if the next line read is "some data", when concatenated with the +# target line, it becomes "cat >out\nsome data", and a match is attempted +# to see if "EOF" matches "some data". Since it doesn't, the next line is +# attempted. When a line consisting of only "EOF" (and possible whitespace) is +# encountered, it is appended to the target line giving "cat >out\nEOF", +# in which case the "EOF" inside "<...>" does match the text following the +# newline, thus the closing here-doc tag has been found. The closing tag line +# and the "<...>" prefix on the target line are then discarded, leaving just +# the target line "cat >out". +# # To facilitate regression testing (and manual debugging), a ">" annotation is # applied to the line containing ")" which closes a subshell, ">>" to a line # closing a nested subshell, and ">>>" to a line closing both at once. This @@ -78,14 +94,17 @@ # here-doc -- swallow it to avoid false hits within its body (but keep the # command to which it was attached) -/<<[ ]*[-\\]*EOF[]*/ { - s/[ ]*<<[ ]*[-\\]*EOF// - h +/<<[ ]*[-\\]*[A-Za-z0-9_]/ { + s/^\(.*\)<<[]*[-\\]*\([A-Za-z0-9_][A-Za-z0-9_]*\)/<\2>\1<]*\)>.*\n[ ]*\1[ ]*$/!{ + s/\n.*$// + bhereslurp + } + s/^<[^>]*>// + s/\n.*$// } # one-liner "(...) &&" @@ -139,9 +158,7 @@ s/.*\n// /"[^'"]*'[^'"]*"/!bsqstring } # here-doc -- swallow it -/<<[ ]*[-\\]*EOF/bheredoc -/<<[ ]*[-\\]*EOT/bheredoc -/<<[ ]*[-\\]*INPUT_END/bheredoc +/<<[ ]*[-\\]*[A-Za-z0-9_]/bheredoc # comment or empty line -- discard since final non-comment, non-empty line # before closing ")", "done", "elsif", "else", or "fi" will need to be # re-visited to drop "suspect" marking since final line of those constructs @@ -249,23 +266,17 @@ s/\n// bcheckchain # found here-doc -- swallow it to avoid false hits within its body (but keep -# the command to which it was attached); take care to handle here-docs nested -# within here-docs by only recognizing closing tag matching outer here-doc -# opening tag +# the command to which it was attached) :heredoc -/EOF/{ s/[ ]*<<[ ]*[-\\]*EOF//; s/^/EOF/; } -/EOT/{ s/[ ]*<<[ ]*[-\\]*EOT//; s/^/EOT/; } -/INPUT_END/{ s/[ ]*<<[ ]*[-\\]*INPUT_END//; s/^/INPUT_END/; } +s/^\(.*\)<<[
[PATCH v3 0/6] chainlint: improve robustness against "unusual" shell coding
This is a re-roll of [1] which improves chainlint's robustness in the face of unusual shell coding such as in contrib/subtree/t7900 which triggered a false-positive[2]. Changes since v2: * recognize "quoted" here-doc tag names (in addition to 'quoted' and \escaped) Interdiff below. [1]: https://public-inbox.org/git/20180813084739.16134-1-sunsh...@sunshineco.com/ [2]: https://public-inbox.org/git/20180730181356.ga156...@aiede.svl.corp.google.com/ Eric Sunshine (6): chainlint: match arbitrary here-docs tags rather than hard-coded names chainlint: match quoted here-doc tags chainlint: recognize multi-line $(...) when command cuddled with "$(" chainlint: let here-doc and multi-line string commence on same line chainlint: recognize multi-line quoted strings more robustly chainlint: add test of pathological case which triggered false positive t/chainlint.sed | 98 --- t/chainlint/here-doc-close-subshell.expect| 2 + t/chainlint/here-doc-close-subshell.test | 5 + .../here-doc-multi-line-command-subst.expect | 5 + .../here-doc-multi-line-command-subst.test| 9 ++ t/chainlint/here-doc-multi-line-string.expect | 4 + t/chainlint/here-doc-multi-line-string.test | 8 ++ t/chainlint/here-doc.expect | 6 ++ t/chainlint/here-doc.test | 21 ...ti-line-nested-command-substitution.expect | 11 ++- ...ulti-line-nested-command-substitution.test | 11 ++- t/chainlint/multi-line-string.expect | 10 +- t/chainlint/multi-line-string.test| 12 +++ t/chainlint/nested-here-doc.expect| 2 + t/chainlint/nested-here-doc.test | 10 ++ t/chainlint/subshell-here-doc.expect | 6 ++ t/chainlint/subshell-here-doc.test| 16 +++ t/chainlint/t7900-subtree.expect | 10 ++ t/chainlint/t7900-subtree.test| 22 + 19 files changed, 227 insertions(+), 41 deletions(-) create mode 100644 t/chainlint/here-doc-close-subshell.expect create mode 100644 t/chainlint/here-doc-close-subshell.test create mode 100644 t/chainlint/here-doc-multi-line-command-subst.expect create mode 100644 t/chainlint/here-doc-multi-line-command-subst.test create mode 100644 t/chainlint/here-doc-multi-line-string.expect create mode 100644 t/chainlint/here-doc-multi-line-string.test create mode 100644 t/chainlint/t7900-subtree.expect create mode 100644 t/chainlint/t7900-subtree.test Interdiff against v2: diff --git a/t/chainlint.sed b/t/chainlint.sed index 8544df38df..1da58b554b 100644 --- a/t/chainlint.sed +++ b/t/chainlint.sed @@ -94,8 +94,8 @@ # here-doc -- swallow it to avoid false hits within its body (but keep the # command to which it was attached) -/<<[ ]*[-\\']*[A-Za-z0-9_]/ { - s/^\(.*\)<<[]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<\1<\1<\1bar && +cat >boo && + horticulture diff --git a/t/chainlint/here-doc.test b/t/chainlint/here-doc.test index f2bb14b693..ad4ce8afd9 100644 --- a/t/chainlint/here-doc.test +++ b/t/chainlint/here-doc.test @@ -21,6 +21,13 @@ boz woz FUMP +# LINT: swallow "quoted" here-doc +cat <<"zump" >boo && +snoz +boz +woz +zump + # LINT: swallow here-doc (EOF is last line of test) horticulture <<\EOF gomez diff --git a/t/chainlint/subshell-here-doc.expect b/t/chainlint/subshell-here-doc.expect index 7663ea7fc4..74723e7340 100644 --- a/t/chainlint/subshell-here-doc.expect +++ b/t/chainlint/subshell-here-doc.expect @@ -6,5 +6,6 @@ ( cat >bup && cat >bup2 && + cat >bup3 && meep >) diff --git a/t/chainlint/subshell-here-doc.test b/t/chainlint/subshell-here-doc.test index b6b5a9b33a..f6b3ba4214 100644 --- a/t/chainlint/subshell-here-doc.test +++ b/t/chainlint/subshell-here-doc.test @@ -31,6 +31,9 @@ glink FIZZ ARBITRARY2 + cat <<-"ARBITRARY3" >bup3 && + glink + FIZZ + ARBITRARY3 meep ) -- 2.18.0.267.gbc8be36ecb
Re: [PATCH] rebase -i: fix numbering in squash message
Hi Junio On 15/08/2018 19:05, Junio C Hamano wrote: > > Phillip Wood writes: > >> From: Phillip Wood >> >> Commit e12a7ef597 ("rebase -i: Handle "combination of commits" with >> GETTEXT_POISON", 2018-04-27) changed the way that individual commit >> messages are labelled when squashing commits together. In doing so a >> regression was introduced where the numbering of the messages is off by >> one. This commit fixes that and adds a test for the numbering. >> >> Signed-off-by: Phillip Wood >> --- >> sequencer.c| 4 ++-- >> t/t3418-rebase-continue.sh | 4 +++- >> 2 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/sequencer.c b/sequencer.c >> index 2eb5ec7227..77d3c2346f 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -1387,13 +1387,13 @@ static int update_squash_messages(enum todo_command >> command, >> unlink(rebase_path_fixup_msg()); >> strbuf_addf(, "\n%c ", comment_line_char); >> strbuf_addf(, _("This is the commit message #%d:"), >> -++opts->current_fixup_count); >> +++opts->current_fixup_count + 1); >> strbuf_addstr(, "\n\n"); >> strbuf_addstr(, body); >> } else if (command == TODO_FIXUP) { >> strbuf_addf(, "\n%c ", comment_line_char); >> strbuf_addf(, _("The commit message #%d will be skipped:"), >> -++opts->current_fixup_count); >> +++opts->current_fixup_count + 1); >> strbuf_addstr(, "\n\n"); >> strbuf_add_commented_lines(, body, strlen(body)); >> } else > > Good spotting. When viewed in a wider context (e.g. "git show -W" > after applying this patch), the way opts->current_fixup_count is > used is somewhat incoherent and adding 1 to pre-increment would make > it even more painful to read. Given that there already is > > strbuf_addf(, _("This is a combination of %d commits."), > opts->current_fixup_count + 2); > > before this part, the code should make it clear these three places > refer to the same number for it to be readable. > > I wonder if it makes it easier to read, understand and maintain if > there were a local variable that gets opts->current_fixup_count+2 at > the beginning of the function, make these three places refer to that > variable, and move the increment of opts->current_fixup_count down > in the function, after the "if we are squashing, do this, if we are > fixing up, do that, otherwise, we do not know what we are doing" > cascade. And use the more common post-increment, as we no longer > depend on the returned value while at it. > > IOW, something like this (untested), on top of yours. I think you'd need to change commit_staged_changes() as well as it relies on the current_fixup_count counting the number of fixups, not the number of fixups plus two. Having said that using 'current_fixup_count + 2' to create the labels and incrementing the count at the end of update_squash_messages() would probably be clearer than my version. I'm about to go away so it'll probably be the second week of September before I can re-roll this, will that be too late for getting it into 2.19? Best Wishes Phillip > > sequencer.c | 10 ++ > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 77d3c2346f..f82c283a89 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -1331,8 +1331,9 @@ static int update_squash_messages(enum todo_command > command, > struct strbuf buf = STRBUF_INIT; > int res; > const char *message, *body; > + int fixup_count = opts->current_fixup_count + 2; > > - if (opts->current_fixup_count > 0) { > + if (fixup_count > 2) { > struct strbuf header = STRBUF_INIT; > char *eol; > > @@ -1345,7 +1346,7 @@ static int update_squash_messages(enum todo_command > command, > > strbuf_addf(, "%c ", comment_line_char); > strbuf_addf(, _("This is a combination of %d commits."), > - opts->current_fixup_count + 2); > + fixup_count); > strbuf_splice(, 0, eol - buf.buf, header.buf, header.len); > strbuf_release(); > } else { > @@ -1387,18 +1388,19 @@ static int update_squash_messages(enum todo_command > command, > unlink(rebase_path_fixup_msg()); > strbuf_addf(, "\n%c ", comment_line_char); > strbuf_addf(, _("This is the commit message #%d:"), > - ++opts->current_fixup_count + 1); > + fixup_count); > strbuf_addstr(, "\n\n"); > strbuf_addstr(, body); > } else if (command == TODO_FIXUP) { > strbuf_addf(, "\n%c ", comment_line_char); > strbuf_addf(, _("The commit message #%d will be skipped:"), > -
Re: [PATCH v4 2/5] unpack-trees: add performance tracing
Duy Nguyen writes: > On Tue, Aug 14, 2018 at 10:52 PM Junio C Hamano wrote: >> > It's not just sampling points. There's things like index id being >> > shown in the message for example. I prefer to keep free style format >> > to help me read. There's also things like indentation I do here to >> > help me read. >> >> Yup, I do not think that contradicts with the approach to have a >> single unified "data collection" API; you should also be able to >> specify how that collection of data is to be presented in the trace >> messages meant for humans, which would be discarded when emitting >> json but would be used when showing human-readble trace, no? > > Yes. As Peff also pointed out in another mail, as long as this > structured logging stuff does not stop me from manual trace messages > and don't force more work on me when I add new traces, I don't care if > it exists. I am hoping that we are on the same page, but just to make sure, what I think we would want is to have just a single set of annotations in the codepath, instead of "we can add annotations from these two separate sets, and they do not interfere each other so I do not care about what the other guy is doing". IOW, I found it highly annoying having to resolve merges like 7234f27b ("Merge branch 'nd/unpack-trees-with-cache-tree' into pu", 2018-08-14), taking two topics that try to use different tracing mechanisms in the same codepath.
Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs
Ævar Arnfjörð Bjarmason writes: +ifndef NO_TCLTK +MAN1_TXT_WIP += gitk.txt +MAN1_TXT = $(MAN1_TXT_WIP) +else +TCLTK_FILES += git-gui.txt +TCLTK_FILES += gitk.txt +TCLTK_FILES += git-citool.txt +MAN1_TXT = $(filter-out \ + $(TCLTK_FILES), \ + $(MAN1_TXT_WIP)) +endif >> >> I didn't notice it when I read it for the first time, but asymmetry >> between these two looks somewhat strange. If we are adding gitk.txt >> when we are not declining TCLTK based programs, why can we do >> without adding git-gui and git-citool at the same time? If we know >> we must add gitk.txt when we are not declining TCLTK based programs >> to MAN1_TXT_WIP in this section, it must mean that when we do not >> want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on >> it, so why do we even need it on TCLTK_FILES list to filter it out? > > The only explicitly listed files are those that don't match the wildcard > git-*.txt. Therefore if we want gitk.txt we need to explicitly list only > it, but if we don't want the TCL programs we also need to list the ones > that match git-*.txt. Which means that gitk.txt does not have to be on TCLTK_FILES list, no?
Re: [PATCH] rebase -i: fix numbering in squash message
Phillip Wood writes: > From: Phillip Wood > > Commit e12a7ef597 ("rebase -i: Handle "combination of commits" with > GETTEXT_POISON", 2018-04-27) changed the way that individual commit > messages are labelled when squashing commits together. In doing so a > regression was introduced where the numbering of the messages is off by > one. This commit fixes that and adds a test for the numbering. > > Signed-off-by: Phillip Wood > --- > sequencer.c| 4 ++-- > t/t3418-rebase-continue.sh | 4 +++- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 2eb5ec7227..77d3c2346f 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -1387,13 +1387,13 @@ static int update_squash_messages(enum todo_command > command, > unlink(rebase_path_fixup_msg()); > strbuf_addf(, "\n%c ", comment_line_char); > strbuf_addf(, _("This is the commit message #%d:"), > - ++opts->current_fixup_count); > + ++opts->current_fixup_count + 1); > strbuf_addstr(, "\n\n"); > strbuf_addstr(, body); > } else if (command == TODO_FIXUP) { > strbuf_addf(, "\n%c ", comment_line_char); > strbuf_addf(, _("The commit message #%d will be skipped:"), > - ++opts->current_fixup_count); > + ++opts->current_fixup_count + 1); > strbuf_addstr(, "\n\n"); > strbuf_add_commented_lines(, body, strlen(body)); > } else Good spotting. When viewed in a wider context (e.g. "git show -W" after applying this patch), the way opts->current_fixup_count is used is somewhat incoherent and adding 1 to pre-increment would make it even more painful to read. Given that there already is strbuf_addf(, _("This is a combination of %d commits."), opts->current_fixup_count + 2); before this part, the code should make it clear these three places refer to the same number for it to be readable. I wonder if it makes it easier to read, understand and maintain if there were a local variable that gets opts->current_fixup_count+2 at the beginning of the function, make these three places refer to that variable, and move the increment of opts->current_fixup_count down in the function, after the "if we are squashing, do this, if we are fixing up, do that, otherwise, we do not know what we are doing" cascade. And use the more common post-increment, as we no longer depend on the returned value while at it. IOW, something like this (untested), on top of yours. sequencer.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 77d3c2346f..f82c283a89 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1331,8 +1331,9 @@ static int update_squash_messages(enum todo_command command, struct strbuf buf = STRBUF_INIT; int res; const char *message, *body; + int fixup_count = opts->current_fixup_count + 2; - if (opts->current_fixup_count > 0) { + if (fixup_count > 2) { struct strbuf header = STRBUF_INIT; char *eol; @@ -1345,7 +1346,7 @@ static int update_squash_messages(enum todo_command command, strbuf_addf(, "%c ", comment_line_char); strbuf_addf(, _("This is a combination of %d commits."), - opts->current_fixup_count + 2); + fixup_count); strbuf_splice(, 0, eol - buf.buf, header.buf, header.len); strbuf_release(); } else { @@ -1387,18 +1388,19 @@ static int update_squash_messages(enum todo_command command, unlink(rebase_path_fixup_msg()); strbuf_addf(, "\n%c ", comment_line_char); strbuf_addf(, _("This is the commit message #%d:"), - ++opts->current_fixup_count + 1); + fixup_count); strbuf_addstr(, "\n\n"); strbuf_addstr(, body); } else if (command == TODO_FIXUP) { strbuf_addf(, "\n%c ", comment_line_char); strbuf_addf(, _("The commit message #%d will be skipped:"), - ++opts->current_fixup_count + 1); + fixup_count); strbuf_addstr(, "\n\n"); strbuf_add_commented_lines(, body, strlen(body)); } else return error(_("unknown command: %d"), command); unuse_commit_buffer(commit, message); + opts->current_fixup_count++; res = write_message(buf.buf, buf.len, rebase_path_squash_msg(), 0); strbuf_release();
Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs
On Wed, Aug 15 2018, Junio C Hamano wrote: > Junio C Hamano writes: > >>> # Guard against environment variables >>> MAN1_TXT = >>> +MAN1_TXT_WIP = >>> +TCLTK_FILES = >> >> The latter name loses the fact that it is to hold candidates to be >> on MAN1_TXT that happen to be conditionally included; calling it >> MAN1_TXT_TCLTK or something, perhaps, may be an improvement. >> >> The former name makes it look it is work-in-progress, but in fact >> they are definite and unconditional part of MAN1_TXT. Perhaps >> MAN1_TXT_CORE or something? > > Sorry, I misread the patch. You collect all possible MAN1_TXT > candidates on _WIP, so "this is unconditional core part" is wrong. > Work-in-progress still sounds a bit funny, but now I know what is > going on a bit better, it has become at last understandable ;-) Yeah maybe it should be *_TMP. It's because you can't assign to a make variable twice (or rather, define a variable in terms of its previous value via filter). Otherwise I would just munge it in-place. >>> +ifndef NO_TCLTK >>> +MAN1_TXT_WIP += gitk.txt >>> +MAN1_TXT = $(MAN1_TXT_WIP) >>> +else >>> +TCLTK_FILES += git-gui.txt >>> +TCLTK_FILES += gitk.txt >>> +TCLTK_FILES += git-citool.txt >>> +MAN1_TXT = $(filter-out \ >>> + $(TCLTK_FILES), \ >>> + $(MAN1_TXT_WIP)) >>> +endif > > I didn't notice it when I read it for the first time, but asymmetry > between these two looks somewhat strange. If we are adding gitk.txt > when we are not declining TCLTK based programs, why can we do > without adding git-gui and git-citool at the same time? If we know > we must add gitk.txt when we are not declining TCLTK based programs > to MAN1_TXT_WIP in this section, it must mean that when we do not > want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on > it, so why do we even need it on TCLTK_FILES list to filter it out? The only explicitly listed files are those that don't match the wildcard git-*.txt. Therefore if we want gitk.txt we need to explicitly list only it, but if we don't want the TCL programs we also need to list the ones that match git-*.txt.
[PATCHv4 3/6] Move definition of enum branch_track from cache.h to branch.h
'branch_track' feels more closely related to branching, and it is needed later in branch.h; rather than #include'ing cache.h in branch.h for this small enum, just move the enum and the external declaration for git_branch_track to branch.h. Reviewed-by: Jonathan Nieder Signed-off-by: Elijah Newren --- branch.h | 11 +++ cache.h | 10 -- config.c | 1 + environment.c | 1 + 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/branch.h b/branch.h index 7d9b330eba..5cace4581f 100644 --- a/branch.h +++ b/branch.h @@ -3,6 +3,17 @@ struct strbuf; +enum branch_track { + BRANCH_TRACK_UNSPECIFIED = -1, + BRANCH_TRACK_NEVER = 0, + BRANCH_TRACK_REMOTE, + BRANCH_TRACK_ALWAYS, + BRANCH_TRACK_EXPLICIT, + BRANCH_TRACK_OVERRIDE +}; + +extern enum branch_track git_branch_track; + /* Functions for acting on the information about branches. */ /* diff --git a/cache.h b/cache.h index 8dc7134f00..a1c0c594fb 100644 --- a/cache.h +++ b/cache.h @@ -919,15 +919,6 @@ enum log_refs_config { }; extern enum log_refs_config log_all_ref_updates; -enum branch_track { - BRANCH_TRACK_UNSPECIFIED = -1, - BRANCH_TRACK_NEVER = 0, - BRANCH_TRACK_REMOTE, - BRANCH_TRACK_ALWAYS, - BRANCH_TRACK_EXPLICIT, - BRANCH_TRACK_OVERRIDE -}; - enum rebase_setup_type { AUTOREBASE_NEVER = 0, AUTOREBASE_LOCAL, @@ -944,7 +935,6 @@ enum push_default_type { PUSH_DEFAULT_UNSPECIFIED }; -extern enum branch_track git_branch_track; extern enum rebase_setup_type autorebase; extern enum push_default_type push_default; diff --git a/config.c b/config.c index 66645047eb..66dca7978a 100644 --- a/config.c +++ b/config.c @@ -6,6 +6,7 @@ * */ #include "cache.h" +#include "branch.h" #include "config.h" #include "repository.h" #include "lockfile.h" diff --git a/environment.c b/environment.c index 6cf0079389..0c04a6da7a 100644 --- a/environment.c +++ b/environment.c @@ -8,6 +8,7 @@ * are. */ #include "cache.h" +#include "branch.h" #include "repository.h" #include "config.h" #include "refs.h" -- 2.18.0.553.g74975b7909
[PATCHv4 2/6] alloc: make allocate_alloc_state and clear_alloc_state more consistent
Since both functions are using the same data type, they should either both refer to it as void *, or both use the real type (struct alloc_state *). Opt for the latter. Reviewed-by: Jonathan Nieder Signed-off-by: Elijah Newren --- alloc.c | 2 +- alloc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/alloc.c b/alloc.c index c2fc5d6888..e7aa81b7aa 100644 --- a/alloc.c +++ b/alloc.c @@ -36,7 +36,7 @@ struct alloc_state { int slab_nr, slab_alloc; }; -void *allocate_alloc_state(void) +struct alloc_state *allocate_alloc_state(void) { return xcalloc(1, sizeof(struct alloc_state)); } diff --git a/alloc.h b/alloc.h index 7a41bb9eb3..ba356ed847 100644 --- a/alloc.h +++ b/alloc.h @@ -15,7 +15,7 @@ void *alloc_object_node(struct repository *r); void alloc_report(struct repository *r); unsigned int alloc_commit_index(struct repository *r); -void *allocate_alloc_state(void); +struct alloc_state *allocate_alloc_state(void); void clear_alloc_state(struct alloc_state *s); #endif -- 2.18.0.553.g74975b7909
[PATCHv4 0/6] Add missing includes and forward declares
This series fixes compilation errors when using a simple test.c file that includes git-compat-util.h and then exactly one other header (and repeating this for different headers of git). Changes in this series come from Jonathan Nieder's reviews; full range-diff follows below, but in summary: - Squashed the final patch from the previous series into the first (Junio already applied a previous round and resolved the simple conflicts with next and pu, so making it easy to drop doesn't save effort anymore.) - Added a new patch to the series removing the forward declaration of an enum, for portability reasons. - Added Jonathan's Reviewed-by on the relevant patches - Remove a few includes and forward declares (which were initially added in previous rounds of this series) that are no longer necessary (due to other includes) - Fixed wording in commit message for patch 1 and added some comments about methodology used to come up with the patch. Elijah Newren (6): Add missing includes and forward declarations alloc: make allocate_alloc_state and clear_alloc_state more consistent Move definition of enum branch_track from cache.h to branch.h urlmatch.h: fix include guard compat/precompose_utf8.h: use more common include guard style Remove forward declaration of an enum alloc.c | 2 +- alloc.h | 4 +++- apply.h | 3 +++ archive.h| 1 + attr.h | 1 + bisect.h | 2 ++ branch.h | 13 + bulk-checkin.h | 2 ++ cache.h | 10 -- column.h | 1 + commit-graph.h | 1 + compat/precompose_utf8.h | 3 ++- config.c | 1 + config.h | 5 + connected.h | 1 + convert.h| 2 ++ csum-file.h | 2 ++ diffcore.h | 4 dir-iterator.h | 2 ++ environment.c| 1 + fsck.h | 1 + fsmonitor.h | 3 +++ gpg-interface.h | 2 ++ khash.h | 3 +++ list-objects-filter.h| 4 list-objects.h | 4 ll-merge.h | 2 ++ mailinfo.h | 2 ++ mailmap.h| 2 ++ merge-recursive.h| 4 +++- notes-merge.h| 4 notes-utils.h| 3 +++ notes.h | 3 +++ object-store.h | 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h| 3 +++ pack-objects.h | 1 + packfile.h | 2 +- patch-ids.h | 6 ++ path.h | 1 + pathspec.h | 2 ++ pretty.h | 4 reachable.h | 2 ++ reflog-walk.h| 1 + refs.h | 2 ++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h | 2 ++ revision.h | 1 + send-pack.h | 4 sequencer.h | 5 + shortlog.h | 2 ++ submodule.h | 10 -- tempfile.h | 1 + trailer.h| 2 ++ tree-walk.h | 2 ++ unpack-trees.h | 5 - url.h| 2 ++ urlmatch.h | 2 ++ utf8.h | 2 ++ worktree.h | 1 + 62 files changed, 152 insertions(+), 18 deletions(-) 1: f7d50cef3b ! 1: e6a93208b2 Add missing includes and forward declares @@ -1,6 +1,13 @@ Author: Elijah Newren -Add missing includes and forward declares +Add missing includes and forward declarations + +I looped over the toplevel header files, creating a temporary two-line C +program for each consisting of + #include "git-compat-util.h" + #include $HEADER +This patch is the result of manually fixing errors in compiling those +tiny programs. Signed-off-by: Elijah Newren @@ -38,15 +45,13 @@ --- a/archive.h +++ b/archive.h @@ + #ifndef ARCHIVE_H + #define ARCHIVE_H ++#include "cache.h" #include "pathspec.h" -+struct object_id; -+enum object_type; -+ struct archiver_args { - const char *base; - size_t baselen; diff --git a/attr.h b/attr.h --- a/attr.h @@ -60,6 +65,19 @@ /* * Given a string, return the gitattribute object that +diff --git a/bisect.h b/bisect.h +--- a/bisect.h b/bisect.h +@@ + #ifndef BISECT_H + #define BISECT_H + ++struct commit_list; ++ + /* + * Find bisection. If something is found, `reaches` will be the number of + * commits that the best commit reaches. `all` will be the count of + diff --git a/branch.h b/branch.h --- a/branch.h
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Wed, Aug 15, 2018 at 9:17 AM Junio C Hamano wrote: > > Jeff King writes: > > > Right, I'd agree they probably want the minimum for that traversal. And > > for `rev-list --filter`, that's probably OK. But keep in mind the main > > goal for --filter is using it for fetches, and many servers do not > > perform the traversal at all. Instead they use reachability bitmaps to > > come up with the set of objects to send. The bitmaps have enough > > information to say "remove all trees from the set", but not enough to do > > any kind of depth-based calculation (not even "is this a root tree"). > > If the depth-based cutoff turns out to make sense (on which I > haven't formed an opinion yet), newer version of pack bitmaps could > store that information ;-) > > How are these "fitler" expressions negotiated between the fetcher > and uploader? Does a "fetch-patch" say "am I allowed to ask you to > filter with tree:4?" and refrain from using the option when > "upload-pack" says "no"? I couldn't find a feature like that for the existing features, but adding such a think seems reasonable to me. (thinking in terms of protocol v2,) There could be a filter-check command which takes a single argument: the filter string (like "tree:4"), and responds with a single line: either "ok" or "unsupported".
[PATCHv4 5/6] compat/precompose_utf8.h: use more common include guard style
Reviewed-by: Jonathan Nieder Signed-off-by: Elijah Newren --- compat/precompose_utf8.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/compat/precompose_utf8.h b/compat/precompose_utf8.h index a94e7c4342..6f843d3e1a 100644 --- a/compat/precompose_utf8.h +++ b/compat/precompose_utf8.h @@ -1,4 +1,6 @@ #ifndef PRECOMPOSE_UNICODE_H +#define PRECOMPOSE_UNICODE_H + #include #include #include @@ -41,5 +43,4 @@ int precompose_utf8_closedir(PREC_DIR *dirp); #define DIR PREC_DIR #endif /* PRECOMPOSE_UNICODE_C */ -#define PRECOMPOSE_UNICODE_H #endif /* PRECOMPOSE_UNICODE_H */ -- 2.18.0.553.g74975b7909
[PATCHv4 1/6] Add missing includes and forward declarations
I looped over the toplevel header files, creating a temporary two-line C program for each consisting of #include "git-compat-util.h" #include $HEADER This patch is the result of manually fixing errors in compiling those tiny programs. Signed-off-by: Elijah Newren --- alloc.h | 2 ++ apply.h | 3 +++ archive.h | 1 + attr.h| 1 + bisect.h | 2 ++ branch.h | 2 ++ bulk-checkin.h| 2 ++ column.h | 1 + commit-graph.h| 1 + config.h | 5 + connected.h | 1 + convert.h | 2 ++ csum-file.h | 2 ++ diffcore.h| 4 dir-iterator.h| 2 ++ fsck.h| 1 + fsmonitor.h | 3 +++ gpg-interface.h | 2 ++ khash.h | 3 +++ list-objects-filter.h | 4 list-objects.h| 4 ll-merge.h| 2 ++ mailinfo.h| 2 ++ mailmap.h | 2 ++ merge-recursive.h | 4 +++- notes-merge.h | 4 notes-utils.h | 3 +++ notes.h | 3 +++ object-store.h| 1 + object.h | 2 ++ oidmap.h | 1 + pack-bitmap.h | 3 +++ pack-objects.h| 1 + patch-ids.h | 6 ++ path.h| 1 + pathspec.h| 2 ++ pretty.h | 4 reachable.h | 2 ++ reflog-walk.h | 1 + refs.h| 2 ++ remote.h | 1 + repository.h | 2 ++ resolve-undo.h| 2 ++ revision.h| 1 + send-pack.h | 4 sequencer.h | 5 + shortlog.h| 2 ++ submodule.h | 10 -- tempfile.h| 1 + trailer.h | 2 ++ tree-walk.h | 2 ++ unpack-trees.h| 5 - url.h | 2 ++ utf8.h| 2 ++ worktree.h| 1 + 55 files changed, 132 insertions(+), 4 deletions(-) diff --git a/alloc.h b/alloc.h index 3e4e828db4..7a41bb9eb3 100644 --- a/alloc.h +++ b/alloc.h @@ -1,9 +1,11 @@ #ifndef ALLOC_H #define ALLOC_H +struct alloc_state; struct tree; struct commit; struct tag; +struct repository; void *alloc_blob_node(struct repository *r); void *alloc_tree_node(struct repository *r); diff --git a/apply.h b/apply.h index b80d8ba736..0b70e1f3f5 100644 --- a/apply.h +++ b/apply.h @@ -1,6 +1,9 @@ #ifndef APPLY_H #define APPLY_H +#include "lockfile.h" +#include "string-list.h" + enum apply_ws_error_action { nowarn_ws_error, warn_on_ws_error, diff --git a/archive.h b/archive.h index 1f9954f7cd..7aba133635 100644 --- a/archive.h +++ b/archive.h @@ -1,6 +1,7 @@ #ifndef ARCHIVE_H #define ARCHIVE_H +#include "cache.h" #include "pathspec.h" struct archiver_args { diff --git a/attr.h b/attr.h index 442d464db6..3185538bda 100644 --- a/attr.h +++ b/attr.h @@ -7,6 +7,7 @@ struct git_attr; /* opaque structures used internally for attribute collection */ struct all_attrs_item; struct attr_stack; +struct index_state; /* * Given a string, return the gitattribute object that diff --git a/bisect.h b/bisect.h index a5d9248a47..34df209351 100644 --- a/bisect.h +++ b/bisect.h @@ -1,6 +1,8 @@ #ifndef BISECT_H #define BISECT_H +struct commit_list; + /* * Find bisection. If something is found, `reaches` will be the number of * commits that the best commit reaches. `all` will be the count of diff --git a/branch.h b/branch.h index 473d0a93e9..7d9b330eba 100644 --- a/branch.h +++ b/branch.h @@ -1,6 +1,8 @@ #ifndef BRANCH_H #define BRANCH_H +struct strbuf; + /* Functions for acting on the information about branches. */ /* diff --git a/bulk-checkin.h b/bulk-checkin.h index a85527318b..f438f93811 100644 --- a/bulk-checkin.h +++ b/bulk-checkin.h @@ -4,6 +4,8 @@ #ifndef BULK_CHECKIN_H #define BULK_CHECKIN_H +#include "cache.h" + extern int index_bulk_checkin(struct object_id *oid, int fd, size_t size, enum object_type type, const char *path, unsigned flags); diff --git a/column.h b/column.h index 0a61917fa7..2567a5cf4d 100644 --- a/column.h +++ b/column.h @@ -36,6 +36,7 @@ static inline int column_active(unsigned int colopts) return (colopts & COL_ENABLE_MASK) == COL_ENABLED; } +struct string_list; extern void print_columns(const struct string_list *list, unsigned int colopts, const struct column_options *opts); diff --git a/commit-graph.h b/commit-graph.h index 76e098934a..eea62f8c0e 100644 --- a/commit-graph.h +++ b/commit-graph.h @@ -4,6 +4,7 @@ #include "git-compat-util.h" #include "repository.h" #include "string-list.h" +#include "cache.h" struct commit; diff --git a/config.h b/config.h index bb2f506b27..75ba1d45ff 100644 --- a/config.h +++ b/config.h @@ -1,6 +1,11 @@ #ifndef CONFIG_H #define CONFIG_H
[PATCHv4 4/6] urlmatch.h: fix include guard
Reviewed-by: Jonathan Nieder Signed-off-by: Elijah Newren --- urlmatch.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/urlmatch.h b/urlmatch.h index 37ee5da85e..e482148248 100644 --- a/urlmatch.h +++ b/urlmatch.h @@ -1,4 +1,6 @@ #ifndef URL_MATCH_H +#define URL_MATCH_H + #include "string-list.h" struct url_info { -- 2.18.0.553.g74975b7909
[PATCHv4 6/6] Remove forward declaration of an enum
According to http://c-faq.com/null/machexamp.html, sizeof(char*) != sizeof(int*) on some platforms. Since an enum could be a char or int (or long or...), knowing the size of the enum thus is important to knowing the size of a pointer to an enum, so we cannot just forward declare an enum the way we can a struct. (Also, modern C++ compilers apparently define forward declarations of an enum to either be useless because the enum was defined, or require an explicit size specifier, or be a compilation error.) Helped-by: Jonathan Nieder Signed-off-by: Elijah Newren --- packfile.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packfile.h b/packfile.h index cc7eaffe1b..fa36c473ad 100644 --- a/packfile.h +++ b/packfile.h @@ -1,12 +1,12 @@ #ifndef PACKFILE_H #define PACKFILE_H +#include "cache.h" #include "oidset.h" /* in object-store.h */ struct packed_git; struct object_info; -enum object_type; /* * Generate the filename to be used for a pack file with checksum "sha1" and -- 2.18.0.553.g74975b7909
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 7:38 PM Junio C Hamano wrote: > > Duy Nguyen writes: > > >> 4) eventually (in the very long run), we'd change the signature of > >> all commands from cmd_foo(int argc, char argv, char *prefix) > >> to cmd_foo(int argc, char argv, struct repository *repo) > >> > >> you seem to be interested in removing the_repository from branch.c, > >> but not as much from bultin/ for now, which is roughly step 2 in that plan? > > > > Yes. Though I think step 4 is to make setup_git_directory() and > > enter_repo() return a 'struct repository *'. This means if you have > > not called either function and not take the repo as an argument, you > > do not have access to any repository. > > That part is understandable if somewhat hand-wavy, but it is not > clear how you can lose the prefix and still keep things like > OPT_FILENAME() working correctly. I haven't worked it all out yet, but I think setup_git_directory() could still return it either as a separate argument or inside 'struct repository'. Then parse_options() still takes the prefix like now, or take the struct repository (the latter may be better because there's also other option callbacks that need struct repo). -- Duy
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
Duy Nguyen writes: >> 4) eventually (in the very long run), we'd change the signature of >> all commands from cmd_foo(int argc, char argv, char *prefix) >> to cmd_foo(int argc, char argv, struct repository *repo) >> >> you seem to be interested in removing the_repository from branch.c, >> but not as much from bultin/ for now, which is roughly step 2 in that plan? > > Yes. Though I think step 4 is to make setup_git_directory() and > enter_repo() return a 'struct repository *'. This means if you have > not called either function and not take the repo as an argument, you > do not have access to any repository. That part is understandable if somewhat hand-wavy, but it is not clear how you can lose the prefix and still keep things like OPT_FILENAME() working correctly.
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
Duy Nguyen writes: > On Wed, Aug 15, 2018 at 7:26 PM Junio C Hamano wrote: >> >> Please do not hide this bugfix behind 1/2 that is likely to require >> longer to cook than the fix itself. And more importantly, it makes >> it impossible to merge down the fix to the maintenance track, as I >> do not think we'd want to merge 1/2 there. > > Oh sorry I did not think about that. Will drop 1/2 and send 2/2 as > standalone. But for the record, I think this has been a bug since the > introduction of --quit in this command (back when it was still called > --reset). If this bug has been there longer, it is a reason why we may want to ensure that the fix applies to even older maintenance tracks. Thanks.
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
On Wed, Aug 15, 2018 at 7:26 PM Junio C Hamano wrote: > > Nguyễn Thái Ngọc Duy writes: > > > --quit is supposed to be --abort but without restoring HEAD. Leaving > > CHERRY_PICK_HEAD behind could make other commands mistake that > > cherry-pick is still ongoing (e.g. "git commit --amend" will refuse to > > work). Clean it too. > > > > For abort, this job of deleting CHERRY_PICK_HEAD is on "git reset" so > > we don't need to do anything else. > > > > Signed-off-by: Nguyễn Thái Ngọc Duy > > --- > > Please do not hide this bugfix behind 1/2 that is likely to require > longer to cook than the fix itself. And more importantly, it makes > it impossible to merge down the fix to the maintenance track, as I > do not think we'd want to merge 1/2 there. Oh sorry I did not think about that. Will drop 1/2 and send 2/2 as standalone. But for the record, I think this has been a bug since the introduction of --quit in this command (back when it was still called --reset). -- Duy
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
Nguyễn Thái Ngọc Duy writes: > --quit is supposed to be --abort but without restoring HEAD. Leaving > CHERRY_PICK_HEAD behind could make other commands mistake that > cherry-pick is still ongoing (e.g. "git commit --amend" will refuse to > work). Clean it too. > > For abort, this job of deleting CHERRY_PICK_HEAD is on "git reset" so > we don't need to do anything else. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- Please do not hide this bugfix behind 1/2 that is likely to require longer to cook than the fix itself. And more importantly, it makes it impossible to merge down the fix to the maintenance track, as I do not think we'd want to merge 1/2 there. Thanks. > builtin/revert.c| 9 +++-- > t/t3510-cherry-pick-sequence.sh | 3 ++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/builtin/revert.c b/builtin/revert.c > index 76f0a35b07..e94a4ead2b 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -7,6 +7,7 @@ > #include "rerere.h" > #include "dir.h" > #include "sequencer.h" > +#include "branch.h" > > /* > * This implements the builtins revert and cherry-pick. > @@ -191,8 +192,12 @@ static int run_sequencer(int argc, const char **argv, > struct replay_opts *opts) > opts->gpg_sign = xstrdup_or_null(opts->gpg_sign); > opts->strategy = xstrdup_or_null(opts->strategy); > > - if (cmd == 'q') > - return sequencer_remove_state(opts); > + if (cmd == 'q') { > + int ret = sequencer_remove_state(opts); > + if (!ret) > + remove_branch_state(the_repository); > + return ret; > + } > if (cmd == 'c') > return sequencer_continue(opts); > if (cmd == 'a') > diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh > index 3505b6aa14..9d121f8ce6 100755 > --- a/t/t3510-cherry-pick-sequence.sh > +++ b/t/t3510-cherry-pick-sequence.sh > @@ -103,7 +103,8 @@ test_expect_success '--quit cleans up sequencer state' ' > pristine_detach initial && > test_expect_code 1 git cherry-pick base..picked && > git cherry-pick --quit && > - test_path_is_missing .git/sequencer > + test_path_is_missing .git/sequencer && > + test_path_is_missing .git/CHERRY_PICK_HEAD > ' > > test_expect_success '--quit keeps HEAD and conflicted index intact' '
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 7:20 PM Junio C Hamano wrote: > I also do not think remove_branch_state() function belongs to > branch.c in the first place. The state it is clearing is not even > about a "branch". It is state left by the last command that stopped > in the middle; its only callers are "reset", "am --abort/--skip" and > "checkout ". sequencer.c as its new home then? -- Duy
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
On Wed, Aug 15, 2018 at 10:18 AM Duy Nguyen wrote: > > On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller wrote: > > Technically you would not need patch 1 in this series, as you could call > > remove_branch_state(void) as before that patch to do the same thing here. > > I guess that patch 1 is more of a drive by cleanup, then? > > Yes. > > > It looks a bit interesting as sequencer_remove_state actually > > takes no arguments and assumes the_repository, but I guess that is fine. > > Don't worry. My effort to kill the_index will make sequencer.c take > 'struct repository *' (its operations are so wide that passing just > struct index_state * does not make sense). Cool! I'll give that series a read, then! Thanks for killing the_index! > > I wondered if we need to have this patch for 'a' as well, and it looks like > > which does a sequencer_rollback, which is just some logic before > > attempting sequencer_remove_state. So I'd think remove_branch_state > > could be done in sequencer_remove_state as well? > > sequencer_rollback does not need this remove_branch_state() line > because it calls reset_for_rollback() which does this deletion. Patch > 1/1 kinda hints at that because it touches all remove_branch_state() > ;-) Gah! I am being slow.
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
Duy Nguyen writes: > On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: >> >> On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy >> wrote: >> >> The patch looks good, but since this touches multiple .c files, I >> think I'd s/branch.c/branch/ in the subject line. > > It is about removing the_repository from branch.c though. As much as I > want to completely erase the_repository, that would take a lot more > work. I do not think this is about removing the_repository from branch.c; it is primarily about allowing create_branch() to work on an arbitrary repository instance. I also do not think remove_branch_state() function belongs to branch.c in the first place. The state it is clearing is not even about a "branch". It is state left by the last command that stopped in the middle; its only callers are "reset", "am --abort/--skip" and "checkout ".
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller wrote: > Technically you would not need patch 1 in this series, as you could call > remove_branch_state(void) as before that patch to do the same thing here. > I guess that patch 1 is more of a drive by cleanup, then? Yes. > It looks a bit interesting as sequencer_remove_state actually > takes no arguments and assumes the_repository, but I guess that is fine. Don't worry. My effort to kill the_index will make sequencer.c take 'struct repository *' (its operations are so wide that passing just struct index_state * does not make sense). > I wondered if we need to have this patch for 'a' as well, and it looks like > which does a sequencer_rollback, which is just some logic before > attempting sequencer_remove_state. So I'd think remove_branch_state > could be done in sequencer_remove_state as well? sequencer_rollback does not need this remove_branch_state() line because it calls reset_for_rollback() which does this deletion. Patch 1/1 kinda hints at that because it touches all remove_branch_state() ;-) Another part of the reason I did not put remove_branch_state() in sequencer_remove_state() is I was not sure if it could be used in a different way (I did not study all of its call sites and am not very familiar with sequencer code). > Or are there functions that would definitely not want sequencer_remove_state > after sequencer_remove_state? > > Going down on that I just realize sequencer_remove_state could also > be returning void, as of now it always returns 0, so the condition here > with !ret is just confusing the reader? -- Duy
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 6:58 PM Stefan Beller wrote: > > On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen wrote: > > > > On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: > > > > > > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy > > > wrote: > > > > > > The patch looks good, but since this touches multiple .c files, I > > > think I'd s/branch.c/branch/ in the subject line. > > > > It is about removing the_repository from branch.c though. As much as I > > want to completely erase the_repository, that would take a lot more > > work. > > What is your envisioned end state? > > 1) IMHO we'd first want to put the_repository in place where needed, > 2) then start replacing s/the_repository/a repository/ in / > 3) builtin/ is not critical, but we'd want to do that later > 4) eventually (in the very long run), we'd change the signature of > all commands from cmd_foo(int argc, char argv, char *prefix) > to cmd_foo(int argc, char argv, struct repository *repo) > > you seem to be interested in removing the_repository from branch.c, > but not as much from bultin/ for now, which is roughly step 2 in that plan? Yes. Though I think step 4 is to make setup_git_directory() and enter_repo() return a 'struct repository *'. This means if you have not called either function and not take the repo as an argument, you do not have access to any repository. I've been trying to make setup_git_directory() not touch any global variables, which would be the first step towards that. Unfortunately I'm currently stopped at the one exception called "git init". -- Duy
Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs
Junio C Hamano writes: >> # Guard against environment variables >> MAN1_TXT = >> +MAN1_TXT_WIP = >> +TCLTK_FILES = > > The latter name loses the fact that it is to hold candidates to be > on MAN1_TXT that happen to be conditionally included; calling it > MAN1_TXT_TCLTK or something, perhaps, may be an improvement. > > The former name makes it look it is work-in-progress, but in fact > they are definite and unconditional part of MAN1_TXT. Perhaps > MAN1_TXT_CORE or something? Sorry, I misread the patch. You collect all possible MAN1_TXT candidates on _WIP, so "this is unconditional core part" is wrong. Work-in-progress still sounds a bit funny, but now I know what is going on a bit better, it has become at last understandable ;-) >> +ifndef NO_TCLTK >> +MAN1_TXT_WIP += gitk.txt >> +MAN1_TXT = $(MAN1_TXT_WIP) >> +else >> +TCLTK_FILES += git-gui.txt >> +TCLTK_FILES += gitk.txt >> +TCLTK_FILES += git-citool.txt >> +MAN1_TXT = $(filter-out \ >> +$(TCLTK_FILES), \ >> +$(MAN1_TXT_WIP)) >> +endif I didn't notice it when I read it for the first time, but asymmetry between these two looks somewhat strange. If we are adding gitk.txt when we are not declining TCLTK based programs, why can we do without adding git-gui and git-citool at the same time? If we know we must add gitk.txt when we are not declining TCLTK based programs to MAN1_TXT_WIP in this section, it must mean that when we do not want TCLTK based programs, MAN1_TXT_WIP would not have gitk.txt on it, so why do we even need it on TCLTK_FILES list to filter it out?
Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD
On Wed, Aug 15, 2018 at 9:23 AM Nguyễn Thái Ngọc Duy wrote: > > --quit is supposed to be --abort but without restoring HEAD. Leaving > CHERRY_PICK_HEAD behind could make other commands mistake that > cherry-pick is still ongoing (e.g. "git commit --amend" will refuse to > work). Clean it too. > > For abort, this job of deleting CHERRY_PICK_HEAD is on "git reset" so > we don't need to do anything else. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > builtin/revert.c| 9 +++-- > t/t3510-cherry-pick-sequence.sh | 3 ++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/builtin/revert.c b/builtin/revert.c > index 76f0a35b07..e94a4ead2b 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -7,6 +7,7 @@ > #include "rerere.h" > #include "dir.h" > #include "sequencer.h" > +#include "branch.h" > > /* > * This implements the builtins revert and cherry-pick. > @@ -191,8 +192,12 @@ static int run_sequencer(int argc, const char **argv, > struct replay_opts *opts) > opts->gpg_sign = xstrdup_or_null(opts->gpg_sign); > opts->strategy = xstrdup_or_null(opts->strategy); > > - if (cmd == 'q') > - return sequencer_remove_state(opts); > + if (cmd == 'q') { > + int ret = sequencer_remove_state(opts); > + if (!ret) > + remove_branch_state(the_repository); Technically you would not need patch 1 in this series, as you could call remove_branch_state(void) as before that patch to do the same thing here. I guess that patch 1 is more of a drive by cleanup, then? It looks a bit interesting as sequencer_remove_state actually takes no arguments and assumes the_repository, but I guess that is fine. I wondered if we need to have this patch for 'a' as well, and it looks like which does a sequencer_rollback, which is just some logic before attempting sequencer_remove_state. So I'd think remove_branch_state could be done in sequencer_remove_state as well? Or are there functions that would definitely not want sequencer_remove_state after sequencer_remove_state? Going down on that I just realize sequencer_remove_state could also be returning void, as of now it always returns 0, so the condition here with !ret is just confusing the reader? Thanks, Stefan
Re: [PATCH] branch: support configuring --sort via .gitconfig
On Wed, Aug 15, 2018 at 7:16 AM wrote: > Add support for configuring default sort ordering for git branches. Command > line option will override this configured value, using the exact same > syntax. Your Signed-off-by: is missing. See Documentation/SubmittingPatches. > diff --git a/Documentation/config.txt b/Documentation/config.txt > @@ -1034,6 +1034,11 @@ branch.autoSetupRebase:: > +branch.sort:: > + This variable controls the sort ordering of branches when displayed by > + linkgit:git-branch[1]. Without the "--sort=" option provided, > the > + value of this variable will be used as the default. I realize that you copied this description from 'tag.sort', thus inherited its existing weakness, but as a reader of this, the first question which popped into my head was "what are the possible s? This description gives no clues and leaves the reader hanging. Better would be either to list the values or point the reader (possibly with a linkgit:) at documentation which does list them. > diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt > @@ -272,6 +272,10 @@ start-point is either a local or remote-tracking branch. > full refname (including `refs/...` prefix). This lists > detached HEAD (if present) first, then local branches and > finally remote-tracking branches. > + The keys supported are the same as those in `git for-each-ref`. > + Sort order defaults to the value configured for the `tag.sort` Did you mean s/tag/branch/? > + variable if it exists, or lexicographic order otherwise. See > + linkgit:git-config[1]. Except for the "See linkgit:git-config[1]", isn't this new text mostly duplicating what this item already says? When I look at Documentation/git-branch.txt, I see: Sort based on the key given. Prefix `-` to sort in descending order of the value. You may use the --sort= option multiple times, in which case the last key becomes the primary key. **The keys supported are the same as those in `git for-each-ref`. Sort order defaults to** sorting based on the full refname (including `refs/...` prefix). This lists detached HEAD (if present) first, then local branches and finally remote-tracking branches. I added ** to highlight the existing text which this duplicates. > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > @@ -1305,4 +1305,50 @@ test_expect_success 'tracking with unexpected .fetch > refspec' ' > +test_expect_success 'configured committerdate sort' ' > + git init sort && > + ( > + cd sort && > + git config branch.sort committerdate && > + [...] > + ) > +' > + > +test_expect_success 'option override configured sort' ' > + ( > + cd sort && > + git branch --sort=refname >actual && I would trust this test more if it explicitly configured "branch.sort" rather than inheriting the value from test(s) above it. That way you wouldn't have to worry about someone later inserting a test above this one which changes or removes the value. > + cat >expect <<-\EOF && > + a > + * b > + c > + master > + EOF > + test_cmp expect actual > + ) > +' > + > +test_expect_success 'invalid sort parameter in configuration' ' > + ( > + cd sort && > + git config branch.sort "v:notvalid" && > + test_must_fail git branch > + > + ) > +' Style: Lose the unnecessary blank line. Thanks.
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 9:53 AM Duy Nguyen wrote: > > On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: > > > > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy > > wrote: > > > > The patch looks good, but since this touches multiple .c files, I > > think I'd s/branch.c/branch/ in the subject line. > > It is about removing the_repository from branch.c though. As much as I > want to completely erase the_repository, that would take a lot more > work. What is your envisioned end state? 1) IMHO we'd first want to put the_repository in place where needed, 2) then start replacing s/the_repository/a repository/ in / 3) builtin/ is not critical, but we'd want to do that later 4) eventually (in the very long run), we'd change the signature of all commands from cmd_foo(int argc, char argv, char *prefix) to cmd_foo(int argc, char argv, struct repository *repo) you seem to be interested in removing the_repository from branch.c, but not as much from bultin/ for now, which is roughly step 2 in that plan?
Re: [PATCH] Makefile: extend NO_TCLTK=NoThanks to cover docs
Ævar Arnfjörð Bjarmason writes: > Extend the NO_TCLTK=NoThanks flag to be understood by the > Documentation Makefile. > > Before this change compiling and installing with NO_TCLTK would result > in no git-gui, gitk or git-citool being installed, but their > respective manual pages would still be installed. Thanks, but I personally do not perceive it to be a problem. It is perfectly OK to install programs without installing docs, and also it is OK to install docs without programs. I do not see why gitk.html and the reference to it from the main ToC in git.html must be removed when you are not installing gitk. Lack of an option to skip them from the documentation is something we might want to improve, but you should be able to install the docs for programs you do not happen to have, and I think it should be the default. By the way, to be more explicit than merely to hint, I think this patch needs to also update Documentation/cmd-list.perl so that under NO_TCLTK, the entry for gitk is omitted from cmds-mainporcelain.txt, etc. to be a complete solution to your original problem. > Signed-off-by: Ævar Arnfjörð Bjarmason > --- > Documentation/Makefile | 23 ++- > Makefile | 39 +-- > 2 files changed, 39 insertions(+), 23 deletions(-) > > diff --git a/Documentation/Makefile b/Documentation/Makefile > index d079d7c73a..d53979939e 100644 > --- a/Documentation/Makefile > +++ b/Documentation/Makefile > @@ -1,5 +1,7 @@ > # Guard against environment variables > MAN1_TXT = > +MAN1_TXT_WIP = > +TCLTK_FILES = The latter name loses the fact that it is to hold candidates to be on MAN1_TXT that happen to be conditionally included; calling it MAN1_TXT_TCLTK or something, perhaps, may be an improvement. The former name makes it look it is work-in-progress, but in fact they are definite and unconditional part of MAN1_TXT. Perhaps MAN1_TXT_CORE or something? > ... > +MAN1_TXT_WIP += git.txt > +MAN1_TXT_WIP += gitremote-helpers.txt > +MAN1_TXT_WIP += gitweb.txt > + > +ifndef NO_TCLTK > +MAN1_TXT_WIP += gitk.txt > +MAN1_TXT = $(MAN1_TXT_WIP) > +else > +TCLTK_FILES += git-gui.txt > +TCLTK_FILES += gitk.txt > +TCLTK_FILES += git-citool.txt > +MAN1_TXT = $(filter-out \ > + $(TCLTK_FILES), \ > + $(MAN1_TXT_WIP)) > +endif > > MAN5_TXT += gitattributes.txt > MAN5_TXT += githooks.txt > diff --git a/Makefile b/Makefile > index bc4fc8eeab..8abb23f6ce 100644 > --- a/Makefile > +++ b/Makefile > @@ -2372,21 +2372,21 @@ export DEFAULT_EDITOR DEFAULT_PAGER > > .PHONY: doc man man-perl html info pdf > doc: man-perl > - $(MAKE) -C Documentation all > + $(MAKE) -C Documentation all NO_TCLTK='$(NO_TCLTK)' > ... > pdf: > - $(MAKE) -C Documentation pdf > + $(MAKE) -C Documentation pdf NO_TCLTK='$(NO_TCLTK)' Since we are assuming GNU make anyway, can we just say "export NO_TCLTK" just once, or would it be too magical and create maintenance burden?
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 6:48 PM Elijah Newren wrote: > > On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy > wrote: > > The patch looks good, but since this touches multiple .c files, I > think I'd s/branch.c/branch/ in the subject line. It is about removing the_repository from branch.c though. As much as I want to completely erase the_repository, that would take a lot more work. -- Duy
Re: [PATCH 1/2] branch.c: remove explicit reference to the_repository
On Wed, Aug 15, 2018 at 9:24 AM Nguyễn Thái Ngọc Duy wrote: The patch looks good, but since this touches multiple .c files, I think I'd s/branch.c/branch/ in the subject line. > > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > branch.c | 22 -- > branch.h | 7 +-- > builtin/am.c | 2 +- > builtin/branch.c | 6 -- > builtin/checkout.c | 5 +++-- > builtin/reset.c| 2 +- > 6 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/branch.c b/branch.c > index ecd710d730..0baa1f6877 100644 > --- a/branch.c > +++ b/branch.c > @@ -244,7 +244,8 @@ N_("\n" > "will track its remote counterpart, you may want to use\n" > "\"git push -u\" to set the upstream config as you push."); > > -void create_branch(const char *name, const char *start_name, > +void create_branch(struct repository *repo, > + const char *name, const char *start_name, >int force, int clobber_head_ok, int reflog, >int quiet, enum branch_track track) > { > @@ -302,7 +303,8 @@ void create_branch(const char *name, const char > *start_name, > break; > } > > - if ((commit = lookup_commit_reference(the_repository, )) == NULL) > + commit = lookup_commit_reference(repo, ); > + if (!commit) > die(_("Not a valid branch point: '%s'."), start_name); > oidcpy(, >object.oid); > > @@ -338,15 +340,15 @@ void create_branch(const char *name, const char > *start_name, > free(real_ref); > } > > -void remove_branch_state(void) > +void remove_branch_state(struct repository *repo) > { > - unlink(git_path_cherry_pick_head(the_repository)); > - unlink(git_path_revert_head(the_repository)); > - unlink(git_path_merge_head(the_repository)); > - unlink(git_path_merge_rr(the_repository)); > - unlink(git_path_merge_msg(the_repository)); > - unlink(git_path_merge_mode(the_repository)); > - unlink(git_path_squash_msg(the_repository)); > + unlink(git_path_cherry_pick_head(repo)); > + unlink(git_path_revert_head(repo)); > + unlink(git_path_merge_head(repo)); > + unlink(git_path_merge_rr(repo)); > + unlink(git_path_merge_msg(repo)); > + unlink(git_path_merge_mode(repo)); > + unlink(git_path_squash_msg(repo)); > } > > void die_if_checked_out(const char *branch, int ignore_current_worktree) > diff --git a/branch.h b/branch.h > index 473d0a93e9..14d8282927 100644 > --- a/branch.h > +++ b/branch.h > @@ -3,6 +3,8 @@ > > /* Functions for acting on the information about branches. */ > > +struct repository; > + > /* > * Creates a new branch, where: > * > @@ -24,7 +26,8 @@ > * that start_name is a tracking branch for (if any). > * > */ > -void create_branch(const char *name, const char *start_name, > +void create_branch(struct repository *repo, > + const char *name, const char *start_name, >int force, int clobber_head_ok, >int reflog, int quiet, enum branch_track track); > > @@ -47,7 +50,7 @@ extern int validate_new_branchname(const char *name, struct > strbuf *ref, int for > * Remove information about the state of working on the current > * branch. (E.g., MERGE_HEAD) > */ > -void remove_branch_state(void); > +void remove_branch_state(struct repository *); > > /* > * Configure local branch "local" as downstream to branch "remote" > diff --git a/builtin/am.c b/builtin/am.c > index 2c19e69f58..7abe39939e 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -2017,7 +2017,7 @@ static int clean_index(const struct object_id *head, > const struct object_id *rem > if (merge_tree(remote_tree)) > return -1; > > - remove_branch_state(); > + remove_branch_state(the_repository); > > return 0; > } > diff --git a/builtin/branch.c b/builtin/branch.c > index 4fc55c3508..e04d528ce1 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -795,7 +795,8 @@ int cmd_branch(int argc, const char **argv, const char > *prefix) > * create_branch takes care of setting up the tracking > * info and making sure new_upstream is correct > */ > - create_branch(branch->name, new_upstream, 0, 0, 0, quiet, > BRANCH_TRACK_OVERRIDE); > + create_branch(the_repository, branch->name, new_upstream, > + 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE); > } else if (unset_upstream) { > struct branch *branch = branch_get(argv[0]); > struct strbuf buf = STRBUF_INIT; > @@ -831,7 +832,8 @@ int cmd_branch(int argc, const char **argv, const char > *prefix) > if (track == BRANCH_TRACK_OVERRIDE) > die(_("the '--set-upstream' option is no longer > supported. Please use '--track' or '--set-upstream-to' instead.")); > > -
Re: [PATCH v4 3/5] unpack-trees: optimize walking same trees with cache-tree
On Mon, Aug 13, 2018 at 8:58 PM Ben Peart wrote: > > + * > > + * D/F conflicts and higher stage entries are not a concern > > + * because cache-tree would be invalidated and we would never > > + * get here in the first place. > > + */ > > + for (i = 0; i < nr_entries; i++) { > > + struct cache_entry *tree_ce; > > + int len, rc; > > + > > + src[0] = o->src_index->cache[pos + i]; > > + > > + len = ce_namelen(src[0]); > > + tree_ce = xcalloc(1, cache_entry_size(len)); > > + > > + tree_ce->ce_mode = src[0]->ce_mode; > > + tree_ce->ce_flags = create_ce_flags(0); > > + tree_ce->ce_namelen = len; > > + oidcpy(_ce->oid, [0]->oid); > > + memcpy(tree_ce->name, src[0]->name, len + 1); > > + > > + for (d = 1; d <= nr_names; d++) > > + src[d] = tree_ce; > > + > > + rc = call_unpack_fn((const struct cache_entry * const *)src, > > o); > > I don't fully understand why this is still necessary since "we detect > that all trees are the same as cache-tree at this path." I do know > (because I tried it :)) that if we don't actually call the unpack > function the patch fails a bunch of tests so clearly something important > is being missed. Yeah because removing this line assumes n-way logic, which most likely means "use the index version if all trees are the same as the index" but it's not necessarily true. There could be flags that make n-way behave differently. And even if we make that assumption, we need to copy src[0] to o->result (heh I tried that "skip call_unpack_fn" thing too when I thought this would be the same as the diff-index --cached optimization path, and only realized copying to o->result was needed afterwards). -- Duy
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Wed, Aug 15, 2018 at 9:14 AM Junio C Hamano wrote: > > Matthew DeVore writes: > > > Thank you. I changed it to this: > > awk -e "/tree|blob/{print \$1}" objs >trees_and_blobs > > The "-e" option does not appear in > > http://pubs.opengroup.org/onlinepubs/9699919799/utilities/awk.html > > and I think you can safely drop it from your command line. Fixed it, thank you. It will be in the next patchset version. > > If no -f option is specified, the first operand to awk shall be the > text of the awk program. The application shall supply the program > operand as a single argument to awk. If the text does not end in a > , awk shall interpret the text as if it did. >
Re: [PATCH v4 2/5] unpack-trees: add performance tracing
On Tue, Aug 14, 2018 at 10:52 PM Junio C Hamano wrote: > > It's not just sampling points. There's things like index id being > > shown in the message for example. I prefer to keep free style format > > to help me read. There's also things like indentation I do here to > > help me read. > > Yup, I do not think that contradicts with the approach to have a > single unified "data collection" API; you should also be able to > specify how that collection of data is to be presented in the trace > messages meant for humans, which would be discarded when emitting > json but would be used when showing human-readble trace, no? Yes. As Peff also pointed out in another mail, as long as this structured logging stuff does not stop me from manual trace messages and don't force more work on me when I add new traces, I don't care if it exists. -- Duy