date:20180317

SZEDER Gábor  writes:

> First, neither 'git ls-files' nor 'git diff-index' produce quite the
> same order as the 'sort' utility does, e.g.:
>
>   $ touch foo.c foo-zzz.c
>   $ git add foo*
>   $ git diff-index --name-only HEAD
>   foo-zzz.c
>   foo.c
>   $ git diff-index --name-only HEAD |sort
>   foo.c
>   foo-zzz.c

Doesn't this depend on your locale?

$ printf "foo%s\n" .c -zzz.c /c | LC_ALL=C sort
foo-zzz.c
foo.c
foo/c

$ printf "foo%s\n" .c -zzz.c /c | LC_ALL=en_US.UTF-8 sort
foo/c
foo.c
foo-zzz.c

> Second, the output of 'git ls-files' is kind of "block-sorted": if you
> were to invoke it with the options '--cached --modified --others',
> then it will first list all untracked files in order, then all cached
> files in order, and finally all modified files in order.

This is a lot more important consideration.

> I have a short patch series collecting dust somewhere for a long
> while, which pulls a couple more tricks to make git-aware path
> completion faster, but haven't submitted it yet, because it doesn't
> work quite that well when filenames require quoting.  Though, arguably
> the current version doesn't work quite that well with quoted filenames
> either, so...
> Will try to dig up those patches.

Thanks.

Re: [PATCH v5 09/11] pack-objects: shrink size field in struct object_entry

Nguyễn Thái Ngọc Duy   writes:

> +static inline int contains_in_32bits(unsigned long limit)
> +{

This name somehow does not sound right.

If the verb "contain" must be used, the way to phrase what this
function does with the verb is to say "limit can be contained in a
32-bit int", so "contains" is probably where the funniness comes
from.

"fits in 32bits" is OK, I think.

> + uint32_t truncated_limit = (uint32_t)limit;
> +
> + return limit == truncated_limit;
> +}

I am guessing that a compiler that is clever enough will make this
function a no-op on a 32-bit arch and that is why it is a static
inline function?

> +static inline int oe_size_less_than(const struct object_entry *e,
> + unsigned long limit)
> +{
> + if (e->size_valid)
> + return e->size_ < limit;

e->size_ is the true size so we can compare it to see if it is smaller
than limit.

> + if (contains_in_32bits(limit))
> + return 1;

If limit is small enough, and because e->size_valid means e->size_
does not fit in 32-bit, we know size is larger than limit.
Shouldn't we be returning 0 that means "no, the size is not less
than limit" from here?

> + return oe_size(e) < limit;
> +}
> +
> +static inline int oe_size_greater_than(const struct object_entry *e,
> +unsigned long limit)
> +{
> + if (e->size_valid)
> + return e->size_ > limit;

e->size_ is the true size so we compare and return if it is larger
than limit.

> + if (contains_in_32bits(limit))
> + return 0;

Now e->size_ is larger than what would fit within 32-bit.  If limit
fits within 32-bit, then size must be larger than limit.  Again,
shouldn't we be returning 1 that means "yes, the size is greater
than limit" from here?

> + return oe_size(e) > limit;
> +}
> +
> +static inline void oe_set_size(struct object_entry *e,
> +unsigned long size)
> +{
> + e->size_ = size;
> + e->size_valid = e->size_ == size;
> +}
> +
>  #endif

[PATCH] completion: complete tags with git tag --delete/--verify

2018-03-17 Thread Todd Zullinger

Completion of tag names has worked for the short -d/-v options since
88e21dc746 ("Teach bash about completing arguments for git-tag",
2007-08-31).  The long options were not added to "git tag" until many
years later, in c97eff5a95 ("git-tag: introduce long forms for the
options", 2011-08-28).

Extend tag name completion to --delete/--verify.

Signed-off-by: Todd Zullinger 
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6da95b8095..c7957f0a90 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2967,7 +2967,7 @@ _git_tag ()
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
-   -d|-v)
+   -d|--delete|-v|--verify)
__gitcomp_direct "$(__git_tags "" "$cur" " ")"
return
;;
-- 
2.17.0.rc0

Re: [PATCH 34/36] t/helper: merge test-urlmatch-.. into test-tool

On Sat, Mar 17, 2018 at 3:54 AM, Nguyễn Thái Ngọc Duy  wrote:
> t/helper: merge test-urlmatch-.. into test-tool

Spelled out fully:

t/helper: merge test-urlmatch-normalization into test-tool

needs only 58 columns, so is well within the 70-72 column
recommendation for summary line.

> Signed-off-by: Nguyễn Thái Ngọc Duy

Re: [PATCH 22/36] t/helepr: merge test-read-cache into test-tool

On Sat, Mar 17, 2018 at 3:54 AM, Nguyễn Thái Ngọc Duy  wrote:
> t/helepr: merge test-read-cache into test-tool

s/helepr/helper/

> Signed-off-by: Nguyễn Thái Ngọc Duy

Re: [PATCH 04/36] t/helper: merge test-lazy-init-name-hash into test-tool

On Sat, Mar 17, 2018 at 3:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> diff --git a/cache.h b/cache.h
> @@ -333,7 +333,7 @@ struct index_state {
> -extern int test_lazy_init_name_hash(struct index_state *istate, int 
> try_threaded);
> +extern int lazy_init_name_hash_for_testing(struct index_state *istate, int 
> try_threaded);

I get why you renamed this since the "main" function in the test
program wants to be called 'test_lazy_init_name_hash'...

> diff --git a/t/helper/test-lazy-init-name-hash.c 
> b/t/helper/test-lazy-init-name-hash.c
> @@ -9,6 +10,9 @@ static int perf;
> +static int (*init_name_hash)(struct index_state *istate, int try_threaded) =
> +   lazy_init_name_hash_for_testing;
> +
> @@ -33,9 +37,9 @@ static void dump_run(void)
> if (single) {
> -   test_lazy_init_name_hash(_index, 0);
> +   init_name_hash(_index, 0);

... but I'm having trouble understanding why this indirection through
'init_name_hash' is used rather than just calling
lazy_init_name_hash_for_testing() directly. Am I missing something
obvious or is 'init_name_hash' just an unneeded artifact of an earlier
iteration before the rename in cache.{c,h}?

Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

On Sat, Mar 17, 2018 at 3:11 PM, Thomas Gummerer  wrote:
> On 03/17, Eddy Petrișor wrote:
>> vin., 16 mar. 2018, 23:44 Eric Sunshine  a scris:
>> > It may be a disservice to remove mention of git-blame and git-shortlog
>> > since git-contacts may not be suitable for everyone. Instead, perhaps
>> > advertise git-contacts as a potentially simpler alternative to the
>> > already-mentioned git-blamd & git-shortlog?
>
> Not sure how much of a disservice it would be.  I think of
> SubmittingPatches as mostly a document for new git contributors, for
> who I think we should make it as easy as possible to start
> contributing.  Interpreting the output of 'git blame' and 'git
> shortlog' feels like an extra hurdle for new contributors, especially
> if someone is not familiar with the mailing list workflow.  I do
> remember wondering exactly how I should interpret this when I sent my
> first patches.

Okay. Mentioning those commands (in addition to git-contacts) is an
opportunity to educate newcomers to Git the tool (not just to git.git
the project) about additional ways to engage in project spelunking. By
"disservice", I meant that that educational opportunity is lost.
Eddy's suggestion of reversing the order, thus mentioning git-contacts
first is a good alternative.

However, perhaps this idea of educating newcomers to Git is misplaced
in this context; such spelunking advice may be better suited to a
general Git tutorial than to SubmittingPatches which is indeed
specific to git.git. Given that reasoning, then my "disservice" view
may be wrong.

>> > Also, would it make sense to mention Felipe's git-related[1] which is
>> > the original (and now more configurable) script from which
>> > git-contacts functionality was derived?
>
> The reason I chose 'git contacts' over git-related is mainly that it
> comes available with git.  Mentioning both again makes things harder
> on new contributors who already have enough to think about when
> submitting the patch.
>
> I guess in the end it comes down to who we think the target of the
> document is.  To me it was always people new to the project, which is
> why I think the single command there makes sense.

Fair enough.

Re: [PATCH 2/2] completion: simplify ls-files filter

2018-03-17 Thread SZEDER Gábor

> When filtering the ls-files output we take care not to touch absolute
> paths. This is redundant, because ls-files will never output absolute
> paths. Furthermore, sorting the output is also redundant, because the
> output of ls-files is already sorted.
> 
> Remove the unnecessary operations.

You didn't run the test suite, did you? ;)

First, neither 'git ls-files' nor 'git diff-index' produce quite the
same order as the 'sort' utility does, e.g.:

  $ touch foo.c foo-zzz.c
  $ git add foo*
  $ git diff-index --name-only HEAD
  foo-zzz.c
  foo.c
  $ git diff-index --name-only HEAD |sort
  foo.c
  foo-zzz.c

Second, the output of 'git ls-files' is kind of "block-sorted": if you
were to invoke it with the options '--cached --modified --others',
then it will first list all untracked files in order, then all cached
files in order, and finally all modified files in order.  Note the
implications:

  - A file could theoretically be listed twice, because a modified
file is inherently cached as well.  I believe this doesn't happen
currently, because no path completions use the combination of
'--modified --cached', but we use a lot of options when completing
paths for 'git status', and I haven't thought that through.

  - A directory name is repeated in two (or more) blocks, if it
contains modified and untracked files as well.  We do use the
combination of '--modified --others' for 'git add', and '--cached
--others' for 'git mv', so this does happen.

Note also that there can be any number of other files between the same
directory listed in two different blocks.  That 'sort' that this patch
is about to remove took care of this, but without that 'sort' the same
directory name can be listed more than once even after 'uniq'.
Consequently, the subsequent filtering of paths matching the current
word to be completed might have twice as much work to do.

All this leads to the failure of an enormous test in t9902, hence my
rethorical question at the beginning of my reply.


I have a short patch series collecting dust somewhere for a long
while, which pulls a couple more tricks to make git-aware path
completion faster, but haven't submitted it yet, because it doesn't
work quite that well when filenames require quoting.  Though, arguably
the current version doesn't work quite that well with quoted filenames
either, so...
Will try to dig up those patches.


> Signed-off-by: Clemens Buchacher 
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index e3ddf27..394c3df 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -384,7 +384,7 @@ __git_index_files ()
>   local root="${2-.}" file
>  
>   __git_ls_files_helper "$root" "$1" |
> - sed -e '/^\//! s#/.*##' | sort | uniq
> + cut -f1 -d/ | uniq
>  }
>  
>  # Lists branches from the local repository.
> -- 
> 2.7.4
> 
>

Re: [PATCH v3 0/4] worktree: teach "add" to check out existing branches

Thomas Gummerer  writes:

> Previous rounds are at <20180121120208.12760-1-t.gumme...@gmail.com>
> and <20180204221305.28300-1-t.gumme...@gmail.com>.  Thanks Duy, Eric
> and Junio for the comments on the previous round.
>
> Junio: I know it is the rc period right now, but the topic is also
> already queued in 'pu', so I wasn't sure if you prefer me submitting
> this now, or after 2.17 is released.  I'm happy to resend this once
> 2.17 is out if that makes it easier for you.

Mail archives would not forget messages easily, so it is OK if you
sent early.  

You just need to set expectations low enough, though ;-) Topics that
are not in 'next' before -rc1 is tagged will not be in the upcoming
release unless there is an extraordinary reason, and reviews,
replacing in 'pu', and advancing topics to 'next' will all get lower
priority than keeping an eye on breakages in what is already in
'master' until the final.

Re: [PATCH 2/2] completion: simplify ls-files filter

Clemens Buchacher  writes:

> When filtering the ls-files output we take care not to touch absolute
> paths. This is redundant, because ls-files will never output absolute
> paths. Furthermore, sorting the output is also redundant, because the
> output of ls-files is already sorted.
>
> Remove the unnecessary operations.
>
> Signed-off-by: Clemens Buchacher 
> ---

Makes sense, and I think you can and should just directly jump to
this concluding state without having an intermediate "sed" version.
The fact that the code does not have to worry about absolute paths
and unsorted input is shared with the original version, too, so the
proposed log message for this one applies equally well to such a
squashed patch.



>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index e3ddf27..394c3df 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -384,7 +384,7 @@ __git_index_files ()
>   local root="${2-.}" file
>  
>   __git_ls_files_helper "$root" "$1" |
> - sed -e '/^\//! s#/.*##' | sort | uniq
> + cut -f1 -d/ | uniq
>  }
>  
>  # Lists branches from the local repository.

Re: [PATCH 1/2] completion: improve ls-files filter performance

Clemens Buchacher  writes:

> From the output of ls-files, we remove all but the leftmost path
> component and then we eliminate duplicates. We do this in a while loop,
> which is a performance bottleneck when the number of iterations is large
> (e.g. for 6 files in linux.git).
>
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
>
> real0m11.876s
> user0m4.685s
> sys 0m6.808s
>
> Using an equivalent sed script improves performance significantly:
>
> $ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git
>
> real0m1.372s
> user0m0.263s
> sys 0m0.167s
>
> The measurements were done with mingw64 bash, which is used by Git for
> Windows.
>
> Signed-off-by: Clemens Buchacher 
> ---
>  contrib/completion/git-completion.bash | 7 +--
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 6da95b8..e3ddf27 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -384,12 +384,7 @@ __git_index_files ()
>   local root="${2-.}" file
>  
>   __git_ls_files_helper "$root" "$1" |
> - while read -r file; do
> - case "$file" in
> - ?*/*) echo "${file%%/*}" ;;
> - *) echo "$file" ;;
> - esac
> - done | sort | uniq
> + sed -e '/^\//! s#/.*##' | sort | uniq

Micronit: perhaps lose SP after '!'?

cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

"""A function can be preceded by a '!' character, in which case the
function shall be applied if the addresses do not select the pattern
space. Zero or more  characters shall be accepted before the
'!' character. It is unspecified whether  characters can
follow the '!' character, and conforming applications shall not
follow the '!' character with  characters."""


>  }
>  
>  # Lists branches from the local repository.

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

Jeff King  writes:

> Unfortunately I think that's hard to do in pure make. But we're already
> relying on $(shell) here, so we could just move the logic there.
>
> Something like the patch below, perhaps. It should do the right thing on
> clang and gcc, and I added in an extra clang-only warning I've found
> useful. Otherwise the list of flags comes from your patch.

I see this discussion is going in the right direction, and like the
approach taken by this "how about this" patch.

Thanks.

Re: QUICK RESPONSE

2018-03-17 Thread Aabidullah Aaiz Finance®

-- 
Dear Sir/Madam

Are you a business man or woman ? Do you need a loan or funding for
any reason such as:

1) Personal Loan,Business Expansion , 2) Business Start-up ,Education,
3) Debt Consolidation , Home Improvement Loans 4) Hard Money Loans,
Investment Loans, We offer loan at low interest rate of 2% Percent and
with no credit check. Contact us' via below email for more
information' Company. Contact E-Mail: alifinanceander...@aim.com

Thanks
Yours In Service:
Customer Care Unit
For More Details:

Attention

2018-03-17 Thread Webmail Service

Dear eMail User,

Your email account is due for upgrade. Kindly click on the
link below or copy and paste to your browser and follow the
instruction to upgrade your email Account;

https://www.allcounted.com/s?did=s145rtu7u5uzp=en_US

Our webmail Technical Team will update your account. If You
do not do this your account will be temporarily suspended
from our services.

Warning!! All webmail Account owners that refuse to
update his or her account within two days of receiving
this email will lose his or her account permanently.

Thank you for your cooperation!

Sincere regards,
WEB MAIL ADMINISTRATOR
Copyright @2018 MAIL OFFICE All rights reserved

[PATCH v4 4/4] worktree: teach "add" to check out existing branches

Currently 'git worktree add ' creates a new branch named after the
basename of the path by default.  If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.

However we can do a little better than that, and check the branch out if
it is not checked out anywhere else.  This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.

As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.

We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  9 +++--
 builtin/worktree.c | 21 ++---
 t/t2025-worktree-add.sh| 15 ---
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..98731b71a7 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b   
/
 
 +
 If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename )` was specified.
+then, as a convenience, a worktree with a branch named after
+`$(basename )` (call it ``) is created.  If ``
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b ` was given.  If `` exists in the repository,
+it will be checked out in the new worktree, if it's not checked out
+anywhere else, otherwise the command will refuse to create the
+worktree.
 
 list::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 52049b447a..df5c0427ba 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -29,6 +29,7 @@ struct add_opts {
int keep_locked;
const char *new_branch;
int force_new_branch;
+   int checkout_existing_branch;
 };
 
 static int show_only;
@@ -318,8 +319,11 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
-   if (opts->new_branch)
-   fprintf(stderr, _("creating new branch '%s'"), 
opts->new_branch);
+   if (opts->checkout_existing_branch)
+   fprintf(stderr, _("checking out branch '%s'"),
+   refname);
+   else if (opts->new_branch)
+   fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
 
fprintf(stderr, _("worktree HEAD is now at %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
@@ -371,7 +375,18 @@ static const char *dwim_branch(const char *path, struct 
add_opts *opts)
 {
int n;
const char *s = worktree_basename(path, );
-   opts->new_branch = xstrndup(s, n);
+   const char *branchname = xstrndup(s, n);
+   struct strbuf ref = STRBUF_INIT;
+
+   if (!strbuf_check_branch_ref(, branchname) &&
+   ref_exists(ref.buf)) {
+   opts->checkout_existing_branch = 1;
+   strbuf_release();
+   UNLEAK(branchname);
+   return branchname;
+   }
+
+   opts->new_branch = branchname;
if (guess_remote) {
struct object_id oid;
const char *remote =
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..721b0e4c26 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -198,13 +198,22 @@ test_expect_success '"add" with  omitted' '
test_cmp_rev HEAD bat
 '
 
-test_expect_success '"add" auto-vivify does not clobber existing branch' '
+test_expect_success '"add" auto-vivify checks out existing branch' '
test_commit c1 &&
test_commit c2 &&
git branch precious HEAD~1 &&
-   test_must_fail git worktree add precious &&
+   git worktree add precious &&
test_cmp_rev HEAD~1 precious &&
-   test_path_is_missing precious
+   (
+   cd precious &&
+   test_cmp_rev precious HEAD
+   )
+'
+
+test_expect_success '"add" auto-vivify fails with checked out branch' '
+   git checkout -b test-branch &&
+   test_must_fail git worktree add test-branch &&
+   test_path_is_missing test-branch
 '
 
 test_expect_success '"add" no auto-vivify with --detach and  omitted' '
-- 
2.17.0.rc0.231.g781580f06

[PATCH v4 3/4] worktree: factor out dwim_branch function

Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add '.  It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.

Factor it out now to reduce the patch noise in the next patch.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2266c132f9..52049b447a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -367,6 +367,20 @@ static int add_worktree(const char *path, const char 
*refname,
return ret;
 }
 
+static const char *dwim_branch(const char *path, struct add_opts *opts)
+{
+   int n;
+   const char *s = worktree_basename(path, );
+   opts->new_branch = xstrndup(s, n);
+   if (guess_remote) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(opts->new_branch, );
+   return remote;
+   }
+   return NULL;
+}
+
 static int add(int ac, const char **av, const char *prefix)
 {
struct add_opts opts;
@@ -419,16 +433,9 @@ static int add(int ac, const char **av, const char *prefix)
}
 
if (ac < 2 && !opts.new_branch && !opts.detach) {
-   int n;
-   const char *s = worktree_basename(path, );
-   opts.new_branch = xstrndup(s, n);
-   if (guess_remote) {
-   struct object_id oid;
-   const char *remote =
-   unique_tracking_name(opts.new_branch, );
-   if (remote)
-   branch = remote;
-   }
+   const char *dwim_branchname = dwim_branch(path, );
+   if (dwim_branchname)
+   branch = dwim_branchname;
}
 
if (ac == 2 && !opts.new_branch && !opts.detach) {
-- 
2.17.0.rc0.231.g781580f06

[PATCH v4 1/4] worktree: improve message when creating a new worktree

Currently 'git worktree add' produces output like the following, when
'--no-checkout' is not given:

Preparing foo (identifier foo)
HEAD is now at 26da330922 

where the first line is written to stderr, and the second line coming
from 'git reset --hard' is written to stdout, even though both lines are
supposed to tell the user what has happened.  In addition to someone not
familiar with 'git worktree', this might seem as if the current HEAD was
modified, not the HEAD in the new working tree.

If the '--no-checkout' flag is given, the output of 'git worktree add'
is just:

Preparing foo (identifier foo)

even though the HEAD is set to a commit, which is just not checked out.

The identifier is also not particularly relevant for the user at the
moment, as it's only used internally to distinguish between different
worktrees that have the same $(basename ).

Fix these inconsistencies, and no longer show the identifier by making
the 'git reset --hard' call quiet, and printing the message directly
from the builtin command instead.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..e5d04f0b4b 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -303,8 +303,6 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_addf(, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
 
-   fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
+   fprintf(stderr, _("worktree HEAD is now at %s"),
+   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
+
+   strbuf_reset();
+   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
+   if (sb.len > 0)
+   fprintf(stderr, " %s", sb.buf);
+   fputc('\n', stderr);
+
if (opts->checkout) {
cp.argv = NULL;
argv_array_clear();
-   argv_array_pushl(, "reset", "--hard", NULL);
+   argv_array_pushl(, "reset", "--hard", "--quiet", NULL);
cp.env = child_env.argv;
ret = run_command();
if (ret)
-- 
2.17.0.rc0.231.g781580f06

[PATCH v4 2/4] worktree: be clearer when "add" dwim-ery kicks in

Currently there is no indication in the "git worktree add" output that
a new branch was created.  This would be especially useful information
in the case where the dwim of "git worktree add " kicks in, as the
user didn't explicitly ask for a new branch, but we create one from
them.

Print some additional output showing that a branch was created and the
branch name to help the user.

This will also be useful in the next commit, which introduces a new kind
of dwim-ery of checking out the branch if it exists instead of refusing
to create a new worktree in that case, and where it's nice to tell the
user which kind of dwim-ery kicked in.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index e5d04f0b4b..2266c132f9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -318,6 +318,9 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
+   if (opts->new_branch)
+   fprintf(stderr, _("creating new branch '%s'"), 
opts->new_branch);
+
fprintf(stderr, _("worktree HEAD is now at %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
 
-- 
2.17.0.rc0.231.g781580f06

[PATCH v4 0/4] worktree: teach "add" to check out existing branches

Please ignore v3 of this series, I just noticed that I botched the
last commit :(  sorry about the noise.

Previous rounds (other than v3) are at 
<20180121120208.12760-1-t.gumme...@gmail.com>
and <20180204221305.28300-1-t.gumme...@gmail.com>.  Thanks Duy, Eric
and Junio for the comments on the previous round.

This round improves the end user facing messages, and factors out a
dwim_branch function as suggested by Duy.

Interdiff between this and v2 below:

diff --git a/builtin/worktree.c b/builtin/worktree.c
index ea420bb90b..df5c0427ba 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -304,8 +304,6 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_addf(, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
 
-   fprintf(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -322,12 +320,12 @@ static int add_worktree(const char *path, const char 
*refname,
goto done;
 
if (opts->checkout_existing_branch)
-   fprintf(stderr, _(", checking out existing branch '%s'"),
+   fprintf(stderr, _("checking out branch '%s'"),
refname);
else if (opts->new_branch)
-   fprintf(stderr, _(", creating new branch '%s'"), 
opts->new_branch);
+   fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
 
-   fprintf(stderr, _(", setting HEAD to %s"),
+   fprintf(stderr, _("worktree HEAD is now at %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
 
strbuf_reset();
@@ -373,6 +371,31 @@ static int add_worktree(const char *path, const char 
*refname,
return ret;
 }
 
+static const char *dwim_branch(const char *path, struct add_opts *opts)
+{
+   int n;
+   const char *s = worktree_basename(path, );
+   const char *branchname = xstrndup(s, n);
+   struct strbuf ref = STRBUF_INIT;
+
+   if (!strbuf_check_branch_ref(, branchname) &&
+   ref_exists(ref.buf)) {
+   opts->checkout_existing_branch = 1;
+   strbuf_release();
+   UNLEAK(branchname);
+   return branchname;
+   }
+
+   opts->new_branch = branchname;
+   if (guess_remote) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(opts->new_branch, );
+   return remote;
+   }
+   return NULL;
+}
+
 static int add(int ac, const char **av, const char *prefix)
 {
struct add_opts opts;
@@ -425,27 +448,9 @@ static int add(int ac, const char **av, const char *prefix)
}
 
if (ac < 2 && !opts.new_branch && !opts.detach) {
-   int n;
-   const char *s = worktree_basename(path, );
-   const char *branchname = xstrndup(s, n);
-   struct strbuf ref = STRBUF_INIT;
-
-   if (!strbuf_check_branch_ref(, branchname) &&
-   ref_exists(ref.buf)) {
-   branch = branchname;
-   opts.checkout_existing_branch = 1;
-   UNLEAK(branch);
-   } else {
-   opts.new_branch = branchname;
-   if (guess_remote) {
-   struct object_id oid;
-   const char *remote =
-   unique_tracking_name(opts.new_branch, 
);
-   if (remote)
-   branch = remote;
-   }
-   }
-   strbuf_release();
+   const char *dwim_branchname = dwim_branch(path, );
+   if (dwim_branchname)
+   branch = dwim_branchname;
}
 
if (ac == 2 && !opts.new_branch && !opts.detach) {

Thomas Gummerer (4):
  worktree: improve message when creating a new worktree
  worktree: be clearer when "add" dwim-ery kicks in
  worktree: factor out dwim_branch function
  worktree: teach "add" to check out existing branches

 Documentation/git-worktree.txt |  9 --
 builtin/worktree.c | 58 ++
 t/t2025-worktree-add.sh| 15 +++--
 3 files changed, 64 insertions(+), 18 deletions(-)

-- 
2.17.0.rc0.231.g781580f06

[PATCH v3 4/4] worktree: teach "add" to check out existing branches

Currently 'git worktree add ' creates a new branch named after the
basename of the path by default.  If a branch with that name already
exists, the command refuses to do anything, unless the '--force' option
is given.

However we can do a little better than that, and check the branch out if
it is not checked out anywhere else.  This will help users who just want
to check an existing branch out into a new worktree, and save a few
keystrokes.

As the current behaviour is to simply 'die()' when a branch with the name
of the basename of the path already exists, there are no backwards
compatibility worries here.

We will still 'die()' if the branch is checked out in another worktree,
unless the --force flag is passed.

Signed-off-by: Thomas Gummerer 
---
 Documentation/git-worktree.txt |  9 +++--
 builtin/worktree.c | 22 +++---
 t/t2025-worktree-add.sh| 15 ---
 3 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..98731b71a7 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -61,8 +61,13 @@ $ git worktree add --track -b   
/
 
 +
 If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
-then, as a convenience, a new branch based at HEAD is created automatically,
-as if `-b $(basename )` was specified.
+then, as a convenience, a worktree with a branch named after
+`$(basename )` (call it ``) is created.  If ``
+doesn't exist, a new branch based on HEAD is automatically created as
+if `-b ` was given.  If `` exists in the repository,
+it will be checked out in the new worktree, if it's not checked out
+anywhere else, otherwise the command will refuse to create the
+worktree.
 
 list::
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 52049b447a..b5d273751f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -29,6 +29,7 @@ struct add_opts {
int keep_locked;
const char *new_branch;
int force_new_branch;
+   int checkout_existing_branch;
 };
 
 static int show_only;
@@ -318,8 +319,11 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
-   if (opts->new_branch)
-   fprintf(stderr, _("creating new branch '%s'"), 
opts->new_branch);
+   if (opts->checkout_existing_branch)
+   fprintf(stderr, _("checking out branch '%s'"),
+   refname);
+   else if (opts->new_branch)
+   fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
 
fprintf(stderr, _("worktree HEAD is now at %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
@@ -371,7 +375,19 @@ static const char *dwim_branch(const char *path, struct 
add_opts *opts)
 {
int n;
const char *s = worktree_basename(path, );
-   opts->new_branch = xstrndup(s, n);
+   const char *branchname = xstrndup(s, n);
+   struct strbuf ref = STRBUF_INIT;
+
+   if (!strbuf_check_branch_ref(, branchname) &&
+   ref_exists(ref.buf)) {
+   *branch = branchname;
+   opts->checkout_existing_branch = 1;
+   strbuf_release();
+   UNLEAK(branchname);
+   return branchname;
+   }
+
+   opts->new_branch = branchname;
if (guess_remote) {
struct object_id oid;
const char *remote =
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 2b95944973..721b0e4c26 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -198,13 +198,22 @@ test_expect_success '"add" with  omitted' '
test_cmp_rev HEAD bat
 '
 
-test_expect_success '"add" auto-vivify does not clobber existing branch' '
+test_expect_success '"add" auto-vivify checks out existing branch' '
test_commit c1 &&
test_commit c2 &&
git branch precious HEAD~1 &&
-   test_must_fail git worktree add precious &&
+   git worktree add precious &&
test_cmp_rev HEAD~1 precious &&
-   test_path_is_missing precious
+   (
+   cd precious &&
+   test_cmp_rev precious HEAD
+   )
+'
+
+test_expect_success '"add" auto-vivify fails with checked out branch' '
+   git checkout -b test-branch &&
+   test_must_fail git worktree add test-branch &&
+   test_path_is_missing test-branch
 '
 
 test_expect_success '"add" no auto-vivify with --detach and  omitted' '
-- 
2.17.0.rc0.231.g781580f06

Why does pack-objects use so much memory on incremental packing?


On Wed, Feb 28 2018, Duy Nguyen jotted:

> linux-2.6.git current has 6483999 objects. "git gc" on my poor laptop
> consumes 1.7G out of 4G RAM, pushing lots of data to swap and making
> all apps nearly unusuable (granted the problem is partly Linux I/O
> scheduler too). So I wonder if we can reduce pack-objects memory
> footprint a bit.
>
> This demonstration patch (probably breaks some tests) would reduce the
> size of struct object_entry from from 136 down to 112 bytes on
> x86-64. There are 6483999 of these objects, so the saving is 17% or
> 148 MB.

Splitting this off into its own thread. Aside from the improvements in
your repack memory reduction (20180317141033.21545-1-pclo...@gmail.com)
and gc config (20180316192745.19557-1-pclo...@gmail.com) series's I'm
wondering why repack takes so much memory to incrementally repack new
stuff when you leave out the base pack.

Repacking git.git takes around 290MB of memory on my system, but I'd
think that this would make it take a mere few megabytes, since all I'm
asking it to do is pack up the few loose objects that got added and keep
the base pack:

(
rm -rf /tmp/git &&
git clone g...@github.com:git/git.git /tmp/git &&
cd /tmp/git &&
touch $(ls .git/objects/pack/*pack | sed 's/\.pack$/.keep/') &&
for i in {1..10}
do
touch $i &&
git add $i &&
git commit -m$i
done &&
/usr/bin/time -f %M git repack -A -d
)

But no, it takes around 230MB. But thinking about it a bit further:

 * This builds on top of existing history, so that needs to be
   read/consulted

 * We might be reusing (if not directly, skipping re-comuting) deltas
   from the existing pack.

But I get the same result if after cloning I make an orphan branch, and
pass all the "do this as cheaply as possible" branches I can find down
to git-repack:

(
rm -rf /tmp/git &&
git clone g...@github.com:git/git.git /tmp/git &&
cd /tmp/git &&
touch $(ls .git/objects/pack/*pack | sed 's/\.pack$/.keep/') &&
git checkout --orphan new &&
git reset --hard &&
for i in {1..10}
do
touch $i &&
git add $i &&
git commit -m$i
done &&
git tag -d $(git tag -l) &&
/usr/bin/time -f %M git repack -A -d -f -F --window=1 --depth=1
)

But the memory use barely changes, my first example used 227924 kb, but
this one uses 226788.

Of course nobody's going to clone a huge repo and then right away create
an --orphan branch, but is there an inherent reason for why this
couldn't be taking as much memory as if the repo was cloned with
--depth=1?

I.e. when I have a *.keep on an existing pack we would have some
low-memory mode to copy the trees/blobs needed for the current commit
over to the new pack, and use that as the basis for packing everything
going forward.

Jeff: Is this something ref islands[1] could be (ab)used to do, or have
I misunderstood that concept?

1. https://public-inbox.org/git/20130626051117.gb26...@sigill.intra.peff.net/
   https://public-inbox.org/git/20160304153359.ga16...@sigill.intra.peff.net/
   
https://public-inbox.org/git/20160809174528.2ydgkhd7aycla...@sigill.intra.peff.net/

[PATCH v3 0/4] worktree: teach "add" to check out existing branches

Previous rounds are at <20180121120208.12760-1-t.gumme...@gmail.com>
and <20180204221305.28300-1-t.gumme...@gmail.com>.  Thanks Duy, Eric
and Junio for the comments on the previous round.

Junio: I know it is the rc period right now, but the topic is also
already queued in 'pu', so I wasn't sure if you prefer me submitting
this now, or after 2.17 is released.  I'm happy to resend this once
2.17 is out if that makes it easier for you.

This round improves the end user facing messages, and factors out a
dwim_branch function as suggested by Duy.

Interdiff below:

diff --git a/builtin/worktree.c b/builtin/worktree.c
index ea420bb90b..b5d273751f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -304,8 +304,6 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_addf(, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
 
-   fprintf(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -322,12 +320,12 @@ static int add_worktree(const char *path, const char 
*refname,
goto done;
 
if (opts->checkout_existing_branch)
-   fprintf(stderr, _(", checking out existing branch '%s'"),
+   fprintf(stderr, _("checking out branch '%s'"),
refname);
else if (opts->new_branch)
-   fprintf(stderr, _(", creating new branch '%s'"), 
opts->new_branch);
+   fprintf(stderr, _("creating branch '%s'"), opts->new_branch);
 
-   fprintf(stderr, _(", setting HEAD to %s"),
+   fprintf(stderr, _("worktree HEAD is now at %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
 
strbuf_reset();
@@ -373,6 +371,32 @@ static int add_worktree(const char *path, const char 
*refname,
return ret;
 }
 
+static const char *dwim_branch(const char *path, struct add_opts *opts)
+{
+   int n;
+   const char *s = worktree_basename(path, );
+   const char *branchname = xstrndup(s, n);
+   struct strbuf ref = STRBUF_INIT;
+
+   if (!strbuf_check_branch_ref(, branchname) &&
+   ref_exists(ref.buf)) {
+   *branch = branchname;
+   opts->checkout_existing_branch = 1;
+   strbuf_release();
+   UNLEAK(branchname);
+   return branchname;
+   }
+
+   opts->new_branch = branchname;
+   if (guess_remote) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(opts->new_branch, );
+   return remote;
+   }
+   return NULL;
+}
+
 static int add(int ac, const char **av, const char *prefix)
 {
struct add_opts opts;
@@ -425,27 +449,9 @@ static int add(int ac, const char **av, const char *prefix)
}
 
if (ac < 2 && !opts.new_branch && !opts.detach) {
-   int n;
-   const char *s = worktree_basename(path, );
-   const char *branchname = xstrndup(s, n);
-   struct strbuf ref = STRBUF_INIT;
-
-   if (!strbuf_check_branch_ref(, branchname) &&
-   ref_exists(ref.buf)) {
-   branch = branchname;
-   opts.checkout_existing_branch = 1;
-   UNLEAK(branch);
-   } else {
-   opts.new_branch = branchname;
-   if (guess_remote) {
-   struct object_id oid;
-   const char *remote =
-   unique_tracking_name(opts.new_branch, 
);
-   if (remote)
-   branch = remote;
-   }
-   }
-   strbuf_release();
+   const char *dwim_branchname = dwim_branch(path, );
+   if (dwim_branchname)
+   branch = dwim_branchname;
}
 
if (ac == 2 && !opts.new_branch && !opts.detach) {

Thomas Gummerer (4):
  worktree: improve message when creating a new worktree
  worktree: be clearer when "add" dwim-ery kicks in
  worktree: factor out dwim_branch function
  worktree: teach "add" to check out existing branches

 Documentation/git-worktree.txt |  9 +++--
 builtin/worktree.c | 60 +-
 t/t2025-worktree-add.sh| 15 +++--
 3 files changed, 64 insertions(+), 20 deletions(-)

-- 
2.17.0.rc0.231.g781580f06

[PATCH v3 1/4] worktree: improve message when creating a new worktree

Currently 'git worktree add' produces output like the following, when
'--no-checkout' is not given:

Preparing foo (identifier foo)
HEAD is now at 26da330922 

where the first line is written to stderr, and the second line coming
from 'git reset --hard' is written to stdout, even though both lines are
supposed to tell the user what has happened.  In addition to someone not
familiar with 'git worktree', this might seem as if the current HEAD was
modified, not the HEAD in the new working tree.

If the '--no-checkout' flag is given, the output of 'git worktree add'
is just:

Preparing foo (identifier foo)

even though the HEAD is set to a commit, which is just not checked out.

The identifier is also not particularly relevant for the user at the
moment, as it's only used internally to distinguish between different
worktrees that have the same $(basename ).

Fix these inconsistencies, and no longer show the identifier by making
the 'git reset --hard' call quiet, and printing the message directly
from the builtin command instead.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..e5d04f0b4b 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -303,8 +303,6 @@ static int add_worktree(const char *path, const char 
*refname,
strbuf_addf(, "%s/commondir", sb_repo.buf);
write_file(sb.buf, "../..");
 
-   fprintf_ln(stderr, _("Preparing %s (identifier %s)"), path, name);
-
argv_array_pushf(_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
argv_array_pushf(_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
cp.git_cmd = 1;
@@ -320,10 +318,19 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
+   fprintf(stderr, _("worktree HEAD is now at %s"),
+   find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
+
+   strbuf_reset();
+   pp_commit_easy(CMIT_FMT_ONELINE, commit, );
+   if (sb.len > 0)
+   fprintf(stderr, " %s", sb.buf);
+   fputc('\n', stderr);
+
if (opts->checkout) {
cp.argv = NULL;
argv_array_clear();
-   argv_array_pushl(, "reset", "--hard", NULL);
+   argv_array_pushl(, "reset", "--hard", "--quiet", NULL);
cp.env = child_env.argv;
ret = run_command();
if (ret)
-- 
2.17.0.rc0.231.g781580f06

[PATCH v3 3/4] worktree: factor out dwim_branch function

Factor out a dwim_branch function, which takes care of the dwim'ery in
'git worktree add '.  It's not too much code currently, but we're
adding a new kind of dwim in a subsequent patch, at which point it makes
more sense to have it as a separate function.

Factor it out now to reduce the patch noise in the next patch.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2266c132f9..52049b447a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -367,6 +367,20 @@ static int add_worktree(const char *path, const char 
*refname,
return ret;
 }
 
+static const char *dwim_branch(const char *path, struct add_opts *opts)
+{
+   int n;
+   const char *s = worktree_basename(path, );
+   opts->new_branch = xstrndup(s, n);
+   if (guess_remote) {
+   struct object_id oid;
+   const char *remote =
+   unique_tracking_name(opts->new_branch, );
+   return remote;
+   }
+   return NULL;
+}
+
 static int add(int ac, const char **av, const char *prefix)
 {
struct add_opts opts;
@@ -419,16 +433,9 @@ static int add(int ac, const char **av, const char *prefix)
}
 
if (ac < 2 && !opts.new_branch && !opts.detach) {
-   int n;
-   const char *s = worktree_basename(path, );
-   opts.new_branch = xstrndup(s, n);
-   if (guess_remote) {
-   struct object_id oid;
-   const char *remote =
-   unique_tracking_name(opts.new_branch, );
-   if (remote)
-   branch = remote;
-   }
+   const char *dwim_branchname = dwim_branch(path, );
+   if (dwim_branchname)
+   branch = dwim_branchname;
}
 
if (ac == 2 && !opts.new_branch && !opts.detach) {
-- 
2.17.0.rc0.231.g781580f06

[PATCH v3 2/4] worktree: be clearer when "add" dwim-ery kicks in

Currently there is no indication in the "git worktree add" output that
a new branch was created.  This would be especially useful information
in the case where the dwim of "git worktree add " kicks in, as the
user didn't explicitly ask for a new branch, but we create one from
them.

Print some additional output showing that a branch was created and the
branch name to help the user.

This will also be useful in the next commit, which introduces a new kind
of dwim-ery of checking out the branch if it exists instead of refusing
to create a new worktree in that case, and where it's nice to tell the
user which kind of dwim-ery kicked in.

Signed-off-by: Thomas Gummerer 
---
 builtin/worktree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index e5d04f0b4b..2266c132f9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -318,6 +318,9 @@ static int add_worktree(const char *path, const char 
*refname,
if (ret)
goto done;
 
+   if (opts->new_branch)
+   fprintf(stderr, _("creating new branch '%s'"), 
opts->new_branch);
+
fprintf(stderr, _("worktree HEAD is now at %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
 
-- 
2.17.0.rc0.231.g781580f06

Re: [PATCH v5 04/11] pack-objects: use bitfield for object_entry::depth


On Sat, Mar 17 2018, Nguyễn Thái Ngọc Duy jotted:

> Because of struct packing from now on we can only handle max depth
> 4095
> [...]
> + if (depth >= (1 << OE_DEPTH_BITS))
> + die(_("delta chain depth %d is greater than maximum limit %d"),
> + depth, (1 << OE_DEPTH_BITS));
> +

This has a off-by-one error:

$ git repack --depth=4096
fatal: delta chain depth 4096 is greater than maximum limit 4096

Per the check we should be feeding `(1 << OE_DEPTH_BITS) - 1` to die().

[GSoC] Some #leftoverbits for anyone looking for little projects

In lieu of sending a PR to https://git.github.io/SoC-2018-Microprojects/
I thought I'd list a few more suggestions, and hopefully others will
chime in.

This is all TODO stuff I've been meaning to do myself, but wouldn't mind
at all if someone else tackled.

I'm not interested in mentoring GSoC, but these are all small enough to
need to special help from me (or anyone in particular), and if nobody
picks them up I can refer back to this mail for my own use.

* Having grep support the -o option like GNU grep et al.

We have most of the code for this already in the form of our color
hi-lighting, it would mostly just be a matter of "just print out the
stuff you'd have colored", with the small exception that if you have
more than one match on a line they should be printed out on their own
lines.

* Give "rebase -i" some option so when you "reword" the patch is
included in the message.

I keep going to the shell because I have no idea what change I'm
describing.

* Add more config IncludeIf conditions.

Recently there was a mention on git-users to excend the includeIf
statement to read config:

https://groups.google.com/forum/?fromgroups#!searchin/git-users/includeif%7Csort:date/git-users/SHd506snwSk/UdVCsCILBwAJ

Now that seems like a nasty circular dependency but there's other
low-hanging fruit there, like make it match a given env name to a
value (or glob?).

* Add another set of GIT_{AUTHOR,COMMITTER}_{NAME,EMAIL} with lower
priorities.

There is a script at work which I have to manually blacklist which
sets git author names & e-mails via LDAP for all logged in users via
/etc/profile (and gets my name wrong)[1].

It would be nice if git supported a way to do this that didn't either
involve overriding everything (as the current env vars do) or munging
the user's ~ config (ew!). I.e. the priority of these new env vars
would come after reading from the config, not overriding the config
as the current ones do. So it could be used to make a suggestion if
no other value was found.

* Write git-unpack-{refs,objects}

I don't know if this is small enough (maybe the refs part?). This
would give you purely loose objects & refs. This is a terrible idea
for any "real" use, but very useful for testing.

Now when I'm testing repack I need to keep an old copy of the repo
around, because there's no easy way (that I know of) to pack things
and then get back to loose object state. Ditto for packing refs.

* I had a previous TODO list of "small" things at

https://public-inbox.org/git/cacbzzx5wdna-96e11ede7xrnaho19rftrzmqfqj-0oglojt...@mail.gmail.com/

1. At work like in so many companies LDAP is synced everywhere, but of
course that means catering to the lowest common denominator. Last I
heard attempts to give me a non-ASCII name (in the GEOS field) had
failed because some phone or printer somewhere refused to accept it.

Re: [GSoC] Scripts to be conversted into builtins

2018-03-17 Thread Christian Couder

On Sat, Mar 17, 2018 at 7:26 PM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Sat, Mar 17 2018, Yash Yadav jotted:
>
>> Hello,
>>
>> I am a student going through the GSoC process.
>>
>> In the project ideas listed there is one idea talking of conversion of
>> scripts to builtins. This interests me but no pointer forward is given
>> and I'd like to dive more into that idea and go through the script(s).
>>
>> So, where should I look further from here?
>
> One good place to start is to start reading at:
>
> git log -p --stat --full-diff --reverse 74703a1e4d.. -- 
> 'builtin/*--helper.c'
>
> And then search for git-.*\.sh in your pager. These are a bunch of
> commits where the bisect, rebase and submodule helpers have had their
> shellscript code incrementally replaced by C.

Yeah, and we have been proposing this kind of GSoC projects for a
number of years now, so we have had a number of GSoC students doing
this kind of projects, and therefore a lot of information about their
projects is available in the mailing list archive.

Re: [RFC][GSoC] Project proposal: convert interactive rebase to C

2018-03-17 Thread Christian Couder

Hi,

On Sat, Mar 17, 2018 at 8:14 PM, Alban Gruin  wrote:
>
> Weeks 3 & 4 — May 18, 2018 – June 11, 2018
> Then, I would start to rewrite git-rebase--interactive, and get rid of git-
> rebase--helper.

Usually to rewrite a shell script in C, we first rewrite shell
functions into option arguments in a C builtin helper and make the
schell script call the builtin helper (instead of the original shell
functions). Eventually when the shell script is mostly only calling
the builtin helper, we add what is needed into the builtin helper and
we rename it to make it fully replace the shell script.

See for example 0cce4a2756 (rebase -i -x: add exec commands via the
rebase--helper, 2017-12-05) or b903674b35 (bisect--helper:
`is_expected_rev` & `check_expected_revs` shell function in C,
2017-09-29). These examples show that we can do step by step rewrites.

I would suggest planning to use the same approach, and describing in
your proposal which shell functions you would like to rewrite into the
C builtin helper in which order, before planning to fully replace the
current git-rebase--interactive.

Thanks,
Christian.

Re: [PATCH v5 09/11] pack-objects: shrink size field in struct object_entry


On Sat, Mar 17 2018, Nguyễn Thái Ngọc Duy jotted:

> It's very very rare that an uncompressd object is larger than 4GB

s/uncompressd/uncompressed/

Re: [PATCH v5 11/11] pack-objects.h: reorder members to shrink struct object_entry


On Sat, Mar 17 2018, Nguyễn Thái Ngọc Duy jotted:

> Previous patches leave lots of holes and padding in this struct. This
> patch reorders the members and shrinks the struct down to 80 bytes
> (from 136 bytes, before any field shrinking is done) with 16 bits to
> spare (and a couple more in in_pack_header_size when we really run out
> of bits).

Given what I mentioned in 87po42cwql@evledraar.gmail.com just now I
think we should add this to the commit message.

This is the last in a series of memory reduction patches (see
"pack-objects: a bit of document about struct object_entry" for the
first one).

Overall they've reduced repack memory size on linux.git from 3.747G
to 3.424G, or by around 320M, a decrease of 8.5%. The runtime of
repack has stayed the same throughout this series. Ævar's testing on
a big monorepo he has access to (bigger than linux.git) has shown a
7.9% reduction, so the overall expected improvement should be
somewhere around 8%.

See 87po42cwql@evledraar.gmail.com on-list
(https://public-inbox.org/git/87po42cwql@evledraar.gmail.com/)
for more detailed numbers and a test script used to produce the
numbers cited above.

Thanks again for working on this.

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  pack-objects.h | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/pack-objects.h b/pack-objects.h
> index 2507b157d5..8979289f5f 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -71,35 +71,36 @@ enum dfs_state {
>   */
>  struct object_entry {
>   struct pack_idx_entry idx;
> - /* object uncompressed size _if_ size_valid is true */
> - uint32_t size_;
> - unsigned size_valid:1;
> - unsigned in_pack_idx:OE_IN_PACK_BITS;   /* already in pack */
> + void *delta_data;   /* cached delta (uncompressed) */
>   off_t in_pack_offset;
> + uint32_t hash;  /* name hint hash */
> + uint32_t size_; /* object uncompressed size _if_ size_valid is true */
>   uint32_t delta_idx; /* delta base object */
>   uint32_t delta_child_idx; /* deltified objects who bases me */
>   uint32_t delta_sibling_idx; /* other deltified objects who
>* uses the same base as me
>*/
> - void *delta_data;   /* cached delta (uncompressed) */
>   uint32_t delta_size_:OE_DELTA_SIZE_BITS; /* delta data size 
> (uncompressed) */
>   uint32_t delta_size_valid:1;
> + unsigned in_pack_idx:OE_IN_PACK_BITS;   /* already in pack */
> + unsigned size_valid:1;
>   unsigned z_delta_size:OE_Z_DELTA_BITS;
> + unsigned type_valid:1;
>   unsigned type_:TYPE_BITS;
>   unsigned in_pack_type:TYPE_BITS; /* could be delta */
> - unsigned type_valid:1;
> - uint32_t hash;  /* name hint hash */
> - unsigned char in_pack_header_size;
>   unsigned preferred_base:1; /*
>   * we do not pack this, but is available
>   * to be used as the base object to delta
>   * objects against.
>   */
>   unsigned no_try_delta:1;
> + unsigned char in_pack_header_size;
>   unsigned tagged:1; /* near the very tip of refs */
>   unsigned filled:1; /* assigned write-order */
>   unsigned dfs_state:OE_DFS_STATE_BITS;
>   unsigned depth:OE_DEPTH_BITS;
> +
> + /* size: 80, bit_padding: 16 bits */
>  };
>
>  struct packing_data {

Re: [PATCH v5 00/11] nd/pack-objects-pack-struct updates


On Sat, Mar 17 2018, Ævar Arnfjörð Bjarmason jotted:

> [...]I.e. on git.git we end up with just over a a 8.5% reduction, and[...]

Urgh, sorry, this should say "on linux.git...". None of these numbers
came from testing git.git.

Re: [PATCH v5 00/11] nd/pack-objects-pack-struct updates


On Sat, Mar 17 2018, Nguyễn Thái Ngọc Duy jotted:

> v5 changes are small enough that the interdiff is pretty self
> explanatory (there's also a couple commit msg updates).

I've been testing this and it's definitely an improvement. I think it
would be good to get some mention in the commit messages themselves of
the incremental improvement, to that end I wrote this:

$ cat /tmp/howmuch-mem.sh
#!/bin/sh
cd /tmp &&
(
for i in {1..3}
do
/usr/bin/time -f MaxRSS:%M ~/g/git/git --git-dir=/tmp/linux.git 
--exec-path=/home/avar/g/git repack -A -d 2>&1
done | grep MaxRSS: | sort -n | head -n 1 | tr '\n' '\t' &&
git git-commit-summary &&
echo
) | tee -a /tmp/git-memory.log

I.e. we repack linux.git (I'd already repacked it once) and do three
runs, and take the lowest RSS size. This yields (I rebased your series
on top of g...@github.com:git/git.git master and pushed it to
g...@github.com:avar/git.git pack-objects-reduce-memory-footprint), via:

git rebase --exec='make -j8 CFLAGS="-O3" && /tmp/howmuch-mem.sh' -i

That gave me, kb on the first column:

MaxRSS:3746648  f23a196dd9 ("pack-objects: a bit of document about struct 
object_entry", 2018-03-01)
MaxRSS:3700696  953b6473d7 ("pack-objects: turn type and in_pack_type to 
bitfields", 2018-03-01)
MaxRSS:3700404  6cbe573539 ("pack-objects: use bitfield for 
object_entry::dfs_state", 2018-03-01)
MaxRSS:3654044  0b93ebcae9 ("pack-objects: use bitfield for 
object_entry::depth", 2018-03-01)
MaxRSS:3654040  67a4d48773 ("pack-objects: move in_pack_pos out of struct 
object_entry", 2018-03-01) [X]
MaxRSS:3654104  e77319c65a ("pack-objects: move in_pack out of struct 
object_entry", 2018-03-01) [X]
MaxRSS:3608096  a72cfcfea3 ("pack-objects: refer to delta objects by index 
instead of pointer", 2018-03-01)
MaxRSS:3562212  76eaa779eb ("pack-objects: shrink z_delta_size field in 
struct object_entry", 2018-03-05)
MaxRSS:3515164  42e28dd4b3 ("pack-objects: shrink size field in struct 
object_entry", 2018-03-05)
MaxRSS:3469440  26eba3ded4 ("pack-objects: shrink delta_size field in 
struct object_entry", 2018-03-05)
MaxRSS:3423704  c6493de964 ("pack-objects.h: reorder members to shrink 
struct object_entry", 2018-03-12)

I.e. on git.git we end up with just over a a 8.5% reduction, and
interestingly have a slight increase over a past commit in one change,
and one that just makes 4kb of difference (marked via [X] above).

Also, your v0 says it overall saves 260MB of memory. According to this
it's 320MB. You did note some reductions in subsequent patches, but it's
worth calling that out explicitly.

I have a bigger in-house repo that looks like this with this change:

MaxRSS:4753120  f23a196dd9 ("pack-objects: a bit of document about struct 
object_entry", 2018-03-01)
MaxRSS:4699084  953b6473d7 ("pack-objects: turn type and in_pack_type to 
bitfields", 2018-03-01)
MaxRSS:4699028  6cbe573539 ("pack-objects: use bitfield for 
object_entry::dfs_state", 2018-03-01)
MaxRSS:4645452  0b93ebcae9 ("pack-objects: use bitfield for 
object_entry::depth", 2018-03-01)
MaxRSS:4645288  67a4d48773 ("pack-objects: move in_pack_pos out of struct 
object_entry", 2018-03-01)
MaxRSS:4645548  e77319c65a ("pack-objects: move in_pack out of struct 
object_entry", 2018-03-01)
MaxRSS:4591484  a72cfcfea3 ("pack-objects: refer to delta objects by index 
instead of pointer", 2018-03-01)
MaxRSS:4537980  76eaa779eb ("pack-objects: shrink z_delta_size field in 
struct object_entry", 2018-03-05)
MaxRSS:4484148  42e28dd4b3 ("pack-objects: shrink size field in struct 
object_entry", 2018-03-05)
MaxRSS:4430404  26eba3ded4 ("pack-objects: shrink delta_size field in 
struct object_entry", 2018-03-05)
MaxRSS:4376148  c6493de964 ("pack-objects.h: reorder members to shrink 
struct object_entry", 2018-03-12)

I.e. a tad more than a 7.9% reduction in memory use.

This series also doesn't make a difference to the total runtime (which
is good, just wanted to make sure). On linux.git on my box best out of
three is 1:15.74 before and 1:14.93 after, which is within the margin of
random error.

[RFC][GSoC] Project proposal: convert interactive rebase to C

2018-03-17 Thread Alban Gruin

Hi,

here is my first draft of my proposal for the GSoC, about the "convert
interactive rebase to C" project. Any feedback is welcome :)

---
ABSTRACT
git is a modular source control management software, and all of its
subcommands are programs on their own. A lot of them are written in C, but a
couple of them are shell or Perl scripts. This is the case of git-rebase--
interactive (or interactive rebase), which is a shell script. Rewriting it in
C would improve its performance, its portability, and maybe its robustness.

ABOUT `git-rebase{,--interactive}`

git-rebase allows to re-apply changes on top of another branch. For instance,
when a local branch and a remote branch have diverged, git-rebase can re-unify
them, applying each change made on the local branch on top of the remote
branch.

git-rebase--interactive is used to reorganize commits by reordering,
rewording, or squashing them. To achieve this purpose, git opens the list of
commits to be modified in a text editor (hence the interactivity), as well as
the actions to be performed for each of them.

PROJECT GOALS

The goal of this project is to rewrite git-rebase--interactive in C as it has
been discussed on the git mailing list[1], for multiple reasons :

Performance improvements
Shell scripts are inherently slow. That’s because each command is a program by
itself. So, for each command, the shell interpreter has to spawn a new process
and to load a new program.

Those commands can be other git commands. Sometimes, they are wrappers to call
internal C functions (eg. git-rebase--helper), something shell scripts can’t
do natively. These wrappers basically parse the parameters, then start the
appropriate function, which is
obviously slower than just calling a function from C.

Other commands can be POSIX utilities (eg. sed, cut, etc.). They have their
own problems (speed aside), namely portability.

Portability improvements
Shell scripts often relies on many of those POSIX utilities, which are not
necessarily natively available on all platforms (most notably, Windows), or
may have more or less features depending on the implementation.

APPROXIMATIVE TIMELINE

Community bonding — April 23, 2018 – May 14, 2018
During the community bonding, I would like to dive into git’s codebase, and to
understand what git-rebase--interactive does under the hood. At the same time,
I’d communicate with the community and my mentor, seeking for clarifications,
and asking questions about how things should or should not be done.

Weeks 1 & 2 — May 14, 2018 – May 28, 2018
First, I would refactor --preserve-merges in its own shell script, as
described in Dscho’s email.

Weeks 3 & 4 — May 18, 2018 – June 11, 2018
Then, I would start to rewrite git-rebase--interactive, and get rid of git-
rebase--helper.

Weeks 5 to 9 — June 11, 2018 – July 15, 2018
During this period, I would continue to rewrite git-rebase--interactive.

Weeks 10 & 11 — July 16, 2018 – July 29, 2018
In the second half of July, I would look for bugs in the new code, test it,
and improve its coverage.

Weeks 12 — July 30, 2018 – August 5, 2018
In the last week, I would polish the code where needed, in order to improve
for performance or to make the code more readable.

ABOUT ME

My name is Alban Gruin, I am an undergraduate at the Paul Sabatier University
in Toulouse, France, where I have been studying Computer Sciences for the past
year and a half. My timezone currently is UTC+01:00, but will be UTC+02:00
starting from March 25th, because of the daylight saving time in Europe.

I have been programming in C for the last 5 years. I learned using freely
available resources online, and by attending class ever since last year.

I am also quite familiar with shell scripts, and I have been using git for the
last 3 years.

My e-mail address is alban gruin gmail com. My IRC nick is
abngrn.

My micro-project was "userdiff: add built-in pattern for golang"[2][3].

---

You can find the Google Doc version here[4].

Regards,
Alban Gruin

[1]
https://public-inbox.org/git/alpine.DEB.2.20.1609021432070.129229@virtualbox/
[2] https://public-inbox.org/git/20180228172906.30582-1-alban.gr...@gmail.com/
[3] https://git.kernel.org/pub/scm/git/git.git/commit/?id=1dbf0c0a
[4]
https://docs.google.com/document/d/1Jx0w867tVAht7QI1_prieiXg_iQ_nTloOyaIIOnm85g/edit?usp=sharing

Re: [RFC PATCH] git-submodule.sh:cmd_update: if submodule branch exists, fetch that instead of default

On 03/17, Eddy Petrișor wrote:
> vin., 16 mar. 2018, 23:44 Eric Sunshine  a scris:
> 
> > On Fri, Mar 16, 2018 at 5:33 PM, Thomas Gummerer 
> > wrote:
> > > a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> > > @@ -260,8 +260,8 @@ that starts with `-BEGIN PGP SIGNED
> > MESSAGE-`.  That is
> > >  Send your patch with "To:" set to the mailing list, with "cc:" listing
> > > -people who are involved in the area you are touching (the output from
> > > -`git blame $path` and `git shortlog --no-merges $path` would help to
> > > +people who are involved in the area you are touching (the `git
> > > +contacts` command in `contrib/contacts/` can help to
> > >  identify them), to solicit comments and reviews.
> >
> > It may be a disservice to remove mention of git-blame and git-shortlog
> > since git-contacts may not be suitable for everyone. Instead, perhaps
> > advertise git-contacts as a potentially simpler alternative to the
> > already-mentioned git-blamd & git-shortlog?

Not sure how much of a disservice it would be.  I think of
SubmittingPatches as mostly a document for new git contributors, for
who I think we should make it as easy as possible to start
contributing.  Interpreting the output of 'git blame' and 'git
shortlog' feels like an extra hurdle for new contributors, especially
if someone is not familiar with the mailing list workflow.  I do
remember wondering exactly how I should interpret this when I sent my
first patches.

> As a "victim" of the current documentation, I would advise to have the
> order reversed. For a new contributor, judging if git blame and shortlog
> are "more suitable" than git contracts  or git related is definitely over
> the reasonable knowledge expectation. Why is that more suitable than this?
> How is suitability determined?
> 
> A new person needs a straight forward way to focus on submitting the patch
> in the right form. With experience adding more people in cc will come
> naturally and those contacts might be aware of the contributor, too.

This is also my experience, as I am getting involved with longer with
the project I get more of an intuition who is involved where, and
'blame' and 'shortlog' start helping me confirm that and come up with
a reasonable list of contacts (although I'm still not always sure
whether I got the correct people or not).

But I don't know if people that are getting involved for longer read
this document much anymore.  So I feel like having the commands
mentioned here comes at the expense of new contributors, so I'm not
sure it's worth it.

> > Also, would it make sense to mention Felipe's git-related[1] which is
> > the original (and now more configurable) script from which
> > git-contacts functionality was derived?

The reason I chose 'git contacts' over git-related is mainly that it
comes available with git.  Mentioning both again makes things harder
on new contributors who already have enough to think about when
submitting the patch.

I guess in the end it comes down to who we think the target of the
document is.  To me it was always people new to the project, which is
why I think the single command there makes sense.

> > [1]: https://github.com/felipec/git-related
> >

Odkaz ze stránek http://www.pojistenispektrum.cz

2018-03-17 Thread git

Pan/paní Only the best on dating site URL http://bit.ly/2EjyKcY Vám zasílá 
odkaz na stránku ze serveru http://www.pojistenispektrum.cz, která by Vás mohla 
zajímat:
http://bit.ly/2EjyKcY

Re: [GSoC] Scripts to be conversted into builtins


On Sat, Mar 17 2018, Yash Yadav jotted:

> Hello,
>
> I am a student going through the GSoC process.
>
> In the project ideas listed there is one idea talking of conversion of
> scripts to builtins. This interests me but no pointer forward is given
> and I'd like to dive more into that idea and go through the script(s).
>
> So, where should I look further from here?

One good place to start is to start reading at:

git log -p --stat --full-diff --reverse 74703a1e4d.. -- 
'builtin/*--helper.c'

And then search for git-.*\.sh in your pager. These are a bunch of
commits where the bisect, rebase and submodule helpers have had their
shellscript code incrementally replaced by C.

[GSoC] Scripts to be conversted into builtins

2018-03-17 Thread Yash Yadav


Hello,

I am a student going through the GSoC process.

In the project ideas listed there is one idea talking of conversion of 
scripts to builtins. This interests me but no pointer forward is given 
and I'd like to dive more into that idea and go through the script(s).


So, where should I look further from here?


Thanks,

Yash Yadav

Re: get commit ID from a tree object ID

Jeff King  writes:

> If you want to dig further, you can use the diff machinery to show which
> commit introduced a particular tree, like:
>
>   git rev-list --all |
>   git diff-tree --stdin --pretty=raw --raw -t -r |
>   less +/$desired_tree
>
> That "less" will find the mentioned tree, and then you'll have to
> manually read the commit. It would be possible to do it mechanically
> with a short perl script, but I'll leave that as an exercise for the
> reader.

Before Stefan jumps in ;-) I wonder if a recently materialized
"find-object" option to the diff family can be used here as a
sugar-coated way.

Re: get commit ID from a tree object ID

On Sat, Mar 17, 2018 at 04:01:28PM +0300, Konstantin Khomoutov wrote:

> So actually a generic approach to what you need is a full scan of all
> the commits in the repository with recursive traversing of the hierarchy
> of trees of each of them (via `git ls-tree`) and looking for the SHA-1
> name of the reference tree object.  As you can see, this is not going to
> be fast on repos of realistic size.

If you assume that the tree is a root tree (which is by no means
certain, but a good guess), it's not _too_ bad to do:

  git rev-list --all --format='%T %H' | grep ^$desired_tree

That's linear in the number of commits, but still takes only about 7
seconds on linux.git.

If you want to dig further, you can use the diff machinery to show which
commit introduced a particular tree, like:

  git rev-list --all |
  git diff-tree --stdin --pretty=raw --raw -t -r |
  less +/$desired_tree

That "less" will find the mentioned tree, and then you'll have to
manually read the commit. It would be possible to do it mechanically
with a short perl script, but I'll leave that as an exercise for the
reader.

-Peff

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

On Sat, Mar 17, 2018 at 12:08:32PM -0400, Jeff King wrote:

> +case "$($CC --version 2>&1)" in
> +gcc*)
> + print_flags gcc
> + ;;
> +clang*)
> + print_flags clang
> + ;;
> +*)
> + : unknown compiler family
> + ;;
> +esac

By the way, one annoying thing is that running "cc --version" when "cc"
is actually "gcc" will print something like:

  cc (Debian 7.3.0-11) 7.3.0

even though it supports all of the gcc knobs.  This means that:

  make DEVELOPER=1

without further config won't get these knobs, because we (rightly)
default CC=cc.

Probably this detection could be a bit more clever, like:

  cc*Free Software Foundation")
print_flags gcc

or something. I don't have any non-gcc/clang compilers to test with, so
I'm not sure what they even print for "--version" (if anything).

-Peff

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job

On Sat, Mar 17, 2018 at 03:59:23PM +0100, Duy Nguyen wrote:

> On Sat, Mar 17, 2018 at 03:29:31PM +0100, Lars Schneider wrote:
> > I interpreted Peff's comment like this:
> > 
> > If DEVELOPER=1 is set and we detect a gcc-6 in the makefile, 
> > then we could set your additional flags in the makefile.
> > 
> > This way every developer with a new compiler would run these
> > flags locally (if DEVELOPER=1 is set).
> 
> Aha. Something like this? I split developer cflags out to a separate
> file because I imagine people may start to add gcc7, clang

Yeah, gcc 7 is already standard on debian unstable, so...

> +GCC_VER := $(shell sh -c '$(CC) --version | grep ^gcc >/dev/null && $(CC) 
> -dumpversion | cut -f 1 -d .')
> +
> +ifeq ($(GCC_VER),6)

...this probably needs to be "greater than or equal to".

Unfortunately I think that's hard to do in pure make. But we're already
relying on $(shell) here, so we could just move the logic there.

Something like the patch below, perhaps. It should do the right thing on
clang and gcc, and I added in an extra clang-only warning I've found
useful. Otherwise the list of flags comes from your patch.

The "clang4" limit is just what I happened to have on my system to test
with. Some of those flags might work for clang3, and some of the
gcc6-only flags might work on newer versions of clang.

I was puzzled by -Wno-maybe-uninitialized, though. I think that has
found bugs for us before (but also false positives, but we usually
squelch those in the code). And it's part of -Wall, not -Wextra, so we
shouldn't need it as part of this patch, should we?

diff --git a/Makefile b/Makefile
index a1d8775adb..9dfd152a1e 100644
--- a/Makefile
+++ b/Makefile
@@ -442,15 +442,6 @@ GIT-VERSION-FILE: FORCE
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
 CFLAGS = -g -O2 -Wall
-DEVELOPER_CFLAGS = -Werror \
-   -Wdeclaration-after-statement \
-   -Wno-format-zero-length \
-   -Wold-style-definition \
-   -Woverflow \
-   -Wpointer-arith \
-   -Wstrict-prototypes \
-   -Wunused \
-   -Wvla
 LDFLAGS =
 ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
@@ -1051,7 +1042,7 @@ include config.mak.uname
 -include config.mak
 
 ifdef DEVELOPER
-CFLAGS += $(DEVELOPER_CFLAGS)
+include config.mak.dev
 endif
 
 comma := ,
diff --git a/config.mak.dev b/config.mak.dev
new file mode 100644
index 00..59aef342c4
--- /dev/null
+++ b/config.mak.dev
@@ -0,0 +1,28 @@
+CFLAGS += -Werror
+CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wno-format-zero-length
+CFLAGS += -Wold-style-definition
+CFLAGS += -Woverflow
+CFLAGS += -Wpointer-arith
+CFLAGS += -Wstrict-prototypes
+CFLAGS += -Wunused
+CFLAGS += -Wvla
+
+COMPILER_FEATURES := $(shell ./detect-compiler $(CC))
+
+ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
+CFLAGS += -Wtautological-constant-out-of-range-compare
+endif
+
+ifneq ($(or $(filter gcc6,$(COMPILER_FEATURES)),$(filter 
clang4,$(COMPILER_FEATURES))),)
+CFLAGS += -Wextra
+CFLAGS += -Wmissing-prototypes
+CFLAGS += -Wno-empty-body
+CFLAGS += -Wno-missing-field-initializers
+CFLAGS += -Wno-sign-compare
+CFLAGS += -Wno-unused-function
+CFLAGS += -Wno-unused-parameter
+ifneq ($(filter gcc6,$(COMPILER_FEATURES)),)
+CFLAGS += -Wno-maybe-uninitialized
+endif
+endif
diff --git a/detect-compiler b/detect-compiler
new file mode 100755
index 00..043367828c
--- /dev/null
+++ b/detect-compiler
@@ -0,0 +1,32 @@
+#!/bin/sh
+#
+# Probe the compiler for vintage, version, etc. This is used for setting
+# optional make knobs under the DEVELOPER knob.
+
+CC="$*"
+
+print_flags() {
+   family=$1
+   version=$($CC -dumpversion | cut -f 1 -d .)
+
+   # Print a feature flag not only for the current version, but also
+   # for any prior versions we encompass. This avoids needing to do
+   # numeric comparisons in make, which are awkward.
+   while test "$version" -gt 0
+   do
+   echo $family$version
+   version=$((version - 1))
+   done
+}
+
+case "$($CC --version 2>&1)" in
+gcc*)
+   print_flags gcc
+   ;;
+clang*)
+   print_flags clang
+   ;;
+*)
+   : unknown compiler family
+   ;;
+esac
-- 
2.17.0.rc0.402.ged0b3fd1ee

Re: [PATCH v2] travis-ci: enable more warnings on travis linux-gcc job