Re: [PATCH v2] worktree: add --quiet option

[Martin Ågren]

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

[Hadi Safari]

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

[Jonathan Nieder]

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

[Stefan Beller]

> [...]

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

[Jonathan Nieder]

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)

[Stefan Beller]

>
> * 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

[Stefan Beller]

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

[Stefan Beller]

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

[Stefan Beller]

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

[Stefan Beller]

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

[Stefan Beller]

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

[Stefan Beller]

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

[Stefan Beller]

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

[Stefan Beller]

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

[Aaron Schrab]

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

[Ramsay Jones]

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

[Matthew DeVore]

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

[Matthew DeVore]

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

[Matthew DeVore]

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

[Matthew DeVore]

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

[Matthew DeVore]

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

[Matthew DeVore]

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

[Matthew DeVore]

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)

[Junio C Hamano]

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

[Stefan Beller]

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

[Thomas Gummerer]

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

[Thomas Gummerer]

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

[Thomas Gummerer]

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

[Linus Torvalds]

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

[Linus Torvalds]

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

[Jonathan Nieder]

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

[Ævar Arnfjörð Bjarmason]

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

[Thomas Gummerer]

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

[Stefan Beller]

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

[Michał Górny]

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

[Jonathan Nieder]

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

[Thomas Gummerer]

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

[Bryan Turner]

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 "

[Ben Peart]

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

[Thomas Gummerer]

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

[Elia Pinto]

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

[Hadi Safari]

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

[Jonathan Nieder]

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

[Jonathan Nieder]

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

[Jonathan Nieder]

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

[Jonathan Nieder]

Elijah Newren wrote:

[...]
> Signed-off-by: Elijah Newren 
> ---
[...]
>  55 files changed, 132 insertions(+), 4 deletions(-)

Reviewed-by: Jonathan Nieder 

Thanks.

"less -F" is broken

[Linus Torvalds]

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

[Thomas Gummerer]

> 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

[Thomas Gummerer]

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

[Stefan Beller]

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

[Thomas Gummerer]

> 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

[Junio C Hamano]

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

[Duy Nguyen]

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

[Thomas Gummerer]

> 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

[Stefan Beller]

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

[Duy Nguyen]

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

[Junio C Hamano]

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

[Stefan Beller]

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

[Torsten Bögershausen]

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()

[Stefan Beller]

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 "$("

[Eric Sunshine]

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

[Eric Sunshine]

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

[Eric Sunshine]

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

[Eric Sunshine]

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

[Eric Sunshine]

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

[Eric Sunshine]

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 "cat out".
+# 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

[Eric Sunshine]

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

[Phillip Wood]

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

[Junio C Hamano]

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

[Junio C Hamano]

Æ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

[Junio C Hamano]

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

[Ævar Arnfjörð Bjarmason]

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

[Elijah Newren]

'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

[Elijah Newren]

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

[Elijah Newren]

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

[Matthew DeVore]

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

[Elijah Newren]

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

[Elijah Newren]

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

[Elijah Newren]

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

[Elijah Newren]

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

[Duy Nguyen]

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

[Junio C Hamano]

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

[Junio C Hamano]

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

[Duy Nguyen]

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

[Junio C Hamano]

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

[Duy Nguyen]

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

[Stefan Beller]

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

[Junio C Hamano]

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

[Duy Nguyen]

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

[Duy Nguyen]

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]

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

[Stefan Beller]

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

[Eric Sunshine]

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

[Stefan Beller]

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

[Junio C Hamano]

Æ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

[Duy Nguyen]

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

[Elijah Newren]

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

[Duy Nguyen]

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

[Matthew DeVore]

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

[Duy Nguyen]

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