Re: [PATCH v2 11/20] refs: record the ref_store in ref_cache, not ref_dir

[Michael Haggerty]

On 04/07/2017 01:38 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  
> wrote:
>> Instead of keeping a pointer to the ref_store in every ref_dir entry,
>> store it once in `struct ref_cache`, and change `struct ref_dir` to
>> include a pointer to its containing `ref_cache` instead. This makes it
>> easier to add to the information that is accessible from a `ref_dir`
>> without increasing the size of every `ref_dir` instance.
> ...
>> @@ -526,7 +526,7 @@ static struct ref_dir *get_loose_refs(struct 
>> files_ref_store *refs)
>>  * lazily):
>>  */
>> add_entry_to_dir(get_ref_dir(refs->loose->root),
>> -create_dir_entry(refs, "refs/", 5, 1));
>> +create_dir_entry(refs->loose, "refs/", 5, 
>> 1));
> 
> The commit message mentions nothing about this change. Is it intended?

The old `create_dir_entry()` took a `files_ref_store` as its first
parameter, because that is what needed to be stored into the old
`dir_entry` struct. The new version takes a `ref_cache`, because that is
what the new `dir_entry` struct requires. This hunk is a logical
consequence of that change.

I'll improve the commit message to explain this better.

Thanks,
Michael

Re: [PATCH v2 10/20] ref-cache: introduce a new type, ref_cache

[Michael Haggerty]

On 04/07/2017 01:32 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  
> wrote:
>> +void free_ref_cache(struct ref_cache *cache)
>> +{
>> +   free_ref_entry(cache->root);
>> +   free(cache);
>> +}
> 
> free(NULL) is no-op (and safe). Maybe we should follow the same
> pattern for free_ref_cache(). It's really up to you.

If I look at other `free_*()` functions in our codebase, it doesn't seem
that many of them handle NULL, and that feature wouldn't help the
callers of this function. So I think I'll leave it the way that it is.

Michael

Re: includeIf breaks calling dashed externals

[Jeff King]

On Sat, Apr 15, 2017 at 06:49:01PM +0700, Duy Nguyen wrote:

> > Probably this fixes it:
> > 
> > diff --git a/config.c b/config.c
> > index b6e4a57b9..8d66bdf56 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t 
> > cond_len, int icase)
> > struct strbuf pattern = STRBUF_INIT;
> > int ret = 0, prefix;
> >  
> > +   if (!have_git_dir())
> > +   return 0;
> > +
> > strbuf_add_absolute_path(, get_git_dir());
> > strbuf_add(, cond, cond_len);
> > prefix = prepare_include_condition_pattern();
> > 
> > But it does raise a question of reading config before/after repository
> > setup, since those will give different answers. I guess they do anyway
> > because of $GIT_DIR/config.
> 
> This happens in execv_dased_external() -> check_pager_config() ->
> read_early_config(). We probably could use the same discover_git_directory
> trick to get .git dir (because we should find it). Maybe something
> like this instead?

I know that we only kick in discover_git_directory() in certain cases
when we're reading config (for the "early config"). Would it makes sense
to respect the same rules here?

I.e., if we have a program that wants to look at config but explicitly
_doesn't_ setup the git repository, is it right to say that we are
inside that repository?

So instead of lazily doing the discovery here:

> diff --git a/config.c b/config.c
> index 1a4d85537b..4f540ae578 100644
> --- a/config.c
> +++ b/config.c
> @@ -212,8 +212,14 @@ static int include_by_gitdir(const char *cond, size_t 
> cond_len, int icase)
>   struct strbuf text = STRBUF_INIT;
>   struct strbuf pattern = STRBUF_INIT;
>   int ret = 0, prefix;
> + struct strbuf gitdir = STRBUF_INIT;
>  
> - strbuf_add_absolute_path(, get_git_dir());
> + if (have_git_dir())
> + strbuf_addstr(, get_git_dir());
> + else if (!discover_git_directory())
> + goto done;

..should we record the discovered git dir in read_early_config(), and
use it only if available? And if not, then we know the program is
explicitly trying not to be in a repo (or we know we tried to discover
one already and it didn't exist).

-Peff

Re: [PATCH v2 04/20] refs_verify_refname_available(): implement once for all backends

[Michael Haggerty]

On 04/07/2017 01:20 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  
> wrote:
>> It turns out that we can now implement
>> `refs_verify_refname_available()` based on the other virtual
>> functions, so there is no need for it to be defined at the backend
>> level. Instead, define it once in `refs.c` and remove the
>> `files_backend` definition.
>>
>> Signed-off-by: Michael Haggerty 
>> ---
>>  refs.c   | 85 
>> ++--
>>  refs.h   |  2 +-
>>  refs/files-backend.c | 39 +---
> 
> Much appreciated. This will make future backends simpler to implement as well.
> 
>> +   iter = refs_ref_iterator_begin(refs, dirname.buf, 0,
>> +  DO_FOR_EACH_INCLUDE_BROKEN);
>> +   while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
>> +   if (skip &&
>> +   string_list_has_string(skip, iter->refname))
>> +   continue;
>> +
>> +   strbuf_addf(err, "'%s' exists; cannot create '%s'",
>> +   iter->refname, refname);
>> +   ok = ref_iterator_abort(iter);
> 
> Saving the return code in "ok" seems redundant because you don't use
> it. Or did you want to check that ok == ITER_DONE or die() too?

True, setting `ok` here is redundant. I don't think there's much point
worrying about whether `ref_iterator_abort()` fails here, since we've
already gotten the information that we require.

I'll remove it.

Thanks,
Michael

Re: [REQ] Allow alternatives to gpg

[Jeff King]

On Sat, Apr 15, 2017 at 08:10:41PM -0700, Nathan McSween wrote:

> I would like to try to make git signing pluggable, this would allow for
> using tools such as signify[1].
> Now I'm wondering if this endeavor is worth taking and what would need to be
> changed besides
> gpg-interface?
> 
> [1] http://man.openbsd.org/signify

I haven't used signify, but I have played around a bit with using gpgsm
with git. You can actually get pretty far without writing any code by
tweaking gpg.program, as long as:

  - your tool can generate and verify detached signatures

  - it follows the gpg command-line convention (or you wrap it in a
script which converts the two)

There are a few quirks around detecting the "BEGIN PGP MESSAGE" block.
It's not necessary for tag signatures, but is for commit signatures
(IIRC). There's some discussion in this thread:

  http://public-inbox.org/git/1459432304-35779-1-git-send-email-...@dwim.me/T/#u

Which isn't to say we shouldn't teach Git natively to understand more
encryption types. But it may be useful to prototype and get experience
first by plugging the tool in via the config.

(I don't have opinions on signify itself as a tool for general purpose
signatures).

-Peff

Re: [PATCH v2 03/20] refs_ref_iterator_begin(): new function

[Michael Haggerty]

On 04/07/2017 12:57 PM, Duy Nguyen wrote:
> On Fri, Mar 31, 2017 at 9:11 PM, Michael Haggerty  
> wrote:
>> Extract a new function from `do_for_each_ref()`. It will be useful
>> elsewhere.
>>
>> Signed-off-by: Michael Haggerty 
>> ---
>>  refs.c   | 15 +--
>>  refs/refs-internal.h | 11 +++
>>  2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 0ed6c3c7a4..aeb704ab92 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -1230,6 +1230,18 @@ int head_ref(each_ref_fn fn, void *cb_data)
>> return head_ref_submodule(NULL, fn, cb_data);
>>  }
>>
>> +struct ref_iterator *refs_ref_iterator_begin(
>> +   struct ref_store *refs,
>> +   const char *prefix, int trim, int flags)
>> +{
>> +   struct ref_iterator *iter;
>> +
>> +   iter = refs->be->iterator_begin(refs, prefix, flags);
>> +   iter = prefix_ref_iterator_begin(iter, prefix, trim);
> 
> Off topic. This code made me wonder if we really need the prefix
> iterator if prefix is NULL. And in fact we don't since
> prefix_ref_iterator_begin() will return the old iter in that case. But
> it's probably better to move that optimization outside. I think it's
> easier to understand that way, calling prefix_ref_ will always give
> you a new iterator. Don't call it unless you want to have it.

I like this aspect of the design because it is more powerful. Consider
for example that some reference backend might (e.g., a future
`packed_ref_store`) need to use a `prefix_ref_iterator` to implement
`iterator_begin()`, while another (e.g., one based on `ref_cache`) might
not. It would be easy to make `prefix_ref_iterator_begin()` check
whether its argument is already a `prefix_ref_iterator`, and if so, swap
it out with a new one with different arguments (to avoid having to stack
them up). It would be inappropriate for the caller to make such an
optimization, because it (a) shouldn't have to care what type of
reference iterator it is dealing with, and (b) shouldn't have to know
enough about `prefix_ref_iterator`s to know that the optimization makes
sense.

>> +/*
>> + * Return an iterator that goes over each reference in `refs` for
>> + * which the refname begins with prefix. If trim is non-zero, then
>> + * trim that many characters off the beginning of each refname. flags
>> + * can be DO_FOR_EACH_INCLUDE_BROKEN to include broken references in
>> + * the iteration.
>> + */
> 
> Do we need a separate docstring here? I think we document more or less
> the same for ref_iterator_begin_fn (except the include-broken flag).

There is a subtle difference: currently, `ref_iterator_begin_fn()`
doesn't use its full `prefix` argument as prefix, but rather
`find_containing_dir(prefix)`. (This is basically an implementation
detail of `ref_cache` leaking out into the virtual function interface.)

`refs_ref_iterator_begin()`, on the other hand, treats the prefix
literally, using `starts_with()`.

I don't like this design and plan to improve it sometime, but for now
that's an important difference. The fix, incidentally, would motivate
the `prefix_ref_iterator_begin()` optimization mentioned above.

Michael

Re: [PATCH] doc/revisions: remove brackets from rev^-n shorthand

[Jeff King]

On Sun, Apr 16, 2017 at 12:07:57AM -0400, Kyle Meyer wrote:

> Given that other instances of "{...}" in the revision documentation
> represent literal characters of revision specifications, describing
> the rev^-n shorthand as "^-{}" incorrectly suggests that
> something like "master^-{1}" is an acceptable form.

I wondered at first if this was some weird asciidoc quoting thing. But
no, the curly braces make it through to the rendered version. I agree
they are confusing.

> -The '{caret}-{}' notation includes '' but excludes the th
> +The '{caret}-' notation includes '' but excludes the th
>  parent (i.e. a shorthand for '{caret}..'), with '' = 1 if

This _could_ be:

  ^-[]

to show that the  parameter is optional. I think the extra
punctuation in a situation like this just makes things harder to read,
though. The text already mentions the default for  and gives an
example that omits it, so I think the paragraph is clear as-is (well,
after your patch removes the confusing "{}").

-Peff

Re: [PATCH] t1400: use consistent style for test_expect_success calls

[Jeff King]

On Sat, Apr 15, 2017 at 10:31:02PM -0400, Kyle Meyer wrote:

> Structure calls as
> 
> test_expect_success 'description' '
>   body
> '
> 
> Use double quotes for the description if it requires parameter
> expansion or contains a single quote.
> 
> Signed-off-by: Kyle Meyer 

Looks good to me.

> -test_expect_success \
> -'creating initial files' \
> -'test_when_finished rm -f M &&
> - echo TEST >F &&
> - git add F &&
> -  GIT_AUTHOR_DATE="2005-05-26 23:30" \
> -  GIT_COMMITTER_DATE="2005-05-26 23:30" git commit -m add -a &&

Not even sure what's going on with the indentation here in the original.
I don't see any reason this "git add" should start an indented block.
This and the other whitespace fixes all look improvements to me.

-Peff

Re: [PATCH 3/3] reset.c: update files when using sparse to avoid data loss.

[Duy Nguyen]

On Wed, Apr 12, 2017 at 10:37 PM, Kevin Willford  wrote:
>
>> -Original Message-
>> From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On
>> Behalf Of Duy Nguyen
>> Sent: Wednesday, April 12, 2017 7:21 AM
>> To: Kevin Willford 
>> Cc: Kevin Willford ; git@vger.kernel.org;
>> gits...@pobox.com; p...@peff.net
>> Subject: Re: [PATCH 3/3] reset.c: update files when using sparse to avoid
>> data loss.
>>
>> On Wed, Apr 12, 2017 at 5:30 AM, Kevin Willford 
>> wrote:
>> > The loss of the skip-worktree bits is part of the problem if you are
>> > talking about modified files.  The other issue that I was having is
>> > when running a reset and there were files added in the commit that is
>> > being reset, there will not be an entry in the index and not a file on
>> > disk so the data for that file is completely lost at that point.
>> > "status" also doesn't include anything about this loss of data.  On
>> > modified files status will at least have the file as deleted since
>> > there is still an index entry but again the previous version of the file 
>> > and it's
>> data is lost.
>>
>> Well, we could have "deleted" index entries, those marked with
>> CE_REMOVE. They are never written down to file though, so 'status'
>> won't benefit from that. Hopefully we can restore the file before the index
>> file is written down and we really lose skip-worktree bits?
>
> Because this is a reset --mixed it will never run through unpack_trees and
> The entries are never marked with CE_REMOVE.

I know. But in my view, it should. All updates from a tree object to
the index should happen through unpack_trees().

>> > To me this is totally unexpected behavior, for example if I am on a
>> > commit where there are only files that where added and run a reset
>> > HEAD~1 and then a status, it will show a clean working directory.
>> > Regardless of skip-worktree bits the user needs to have the data in
>> > the working directory after the reset or data is lost which is always bad.
>>
>> I agree we no longer have a place to save the skip-worktree bit, we have to
>> restore the state back as if skip-worktree bit does not exist.
>> It would be good if we could keep the logic in unpack_trees() though.
>> For example, if the file is present on disk even if skip-worktree bit is on,
>> unpack_trees() would abort instead of silently overwriting it.
>> This is a difference between skip-worktree and assume-unchanged bits.
>> If you do explicit checkout_entry() you might have to add more checks to
>> keep behavior consistent.
>> --
>> Duy
>
> Because this is a reset --mixed it will follow the code path calling 
> read_from_tree
> and ends up calling update_index_from_diff in the format_callback of the diff,
> so unpack_trees() is never called in the --mixed case.  This code change also 
> only applies
> when the file does not exist and the skip-worktree bit is on and the updated
> index entry either will be missing (covers the added scenario) or was not 
> missing
> before (covers the modified scenario).  If there is a better way to get the 
> previous
> index entry to disk than what I am doing, I am happy to implement it 
> correctly.

I think it's ok to just look at the diff (from update_index_from_diff)
and restore the on-disk version for now. I'd like to make --mixed use
unpack_trees() too but I haven't studied  this code long enough to see
why it went with "diff" instead of "read-tree" (which translates
directly to unpack_trees). Maybe there is some subtle reason for that.
Though it looks like it was more convenient to do "diff" in the
git-reset.sh version, and that got translated literally to C when the
command was rewritten.
-- 
Duy

[PATCH] doc/revisions: remove brackets from rev^-n shorthand

[Kyle Meyer]

Given that other instances of "{...}" in the revision documentation
represent literal characters of revision specifications, describing
the rev^-n shorthand as "^-{}" incorrectly suggests that
something like "master^-{1}" is an acceptable form.

Signed-off-by: Kyle Meyer 
---
 Documentation/revisions.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 75d211f1a..61277469c 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -295,7 +295,7 @@ The 'r1{caret}@' notation means all parents of 'r1'.
 The 'r1{caret}!' notation includes commit 'r1' but excludes all of its parents.
 By itself, this notation denotes the single commit 'r1'.
 
-The '{caret}-{}' notation includes '' but excludes the th
+The '{caret}-' notation includes '' but excludes the th
 parent (i.e. a shorthand for '{caret}..'), with '' = 1 if
 not given. This is typically useful for merge commits where you
 can just pass '{caret}-' to get all the commits in the branch
@@ -337,7 +337,7 @@ Revision Range Summary
   as giving commit '' and then all its parents prefixed with
   '{caret}' to exclude them (and their ancestors).
 
-'{caret}-{}', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
+'{caret}-', e.g. 'HEAD{caret}-, HEAD{caret}-2'::
Equivalent to '{caret}..', with '' = 1 if not
given.
 
-- 
2.12.2

[REQ] Allow alternatives to gpg

[Nathan McSween]

I would like to try to make git signing pluggable, this would allow for 
using tools such as signify[1].
Now I'm wondering if this endeavor is worth taking and what would need 
to be changed besides

gpg-interface?

[1] http://man.openbsd.org/signify

[PATCH] t1400: use consistent style for test_expect_success calls

[Kyle Meyer]

Structure calls as

test_expect_success 'description' '
body
'

Use double quotes for the description if it requires parameter
expansion or contains a single quote.

Signed-off-by: Kyle Meyer 
---
This patch follows up on a recent t1400 series:
https://public-inbox.org/git/xmqq8tnysekm@gitster.mtv.corp.google.com/

 t/t1400-update-ref.sh | 335 +-
 1 file changed, 167 insertions(+), 168 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index a23cd7b57..dc98b4bc6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -35,14 +35,14 @@ test_expect_success setup '
cd -
 '
 
-test_expect_success \
-   "create $m" \
-   "git update-ref $m $A &&
-test $A"' = $(cat .git/'"$m"')'
-test_expect_success \
-   "create $m with oldvalue verification" \
-   "git update-ref $m $B $A &&
-test $B"' = $(cat .git/'"$m"')'
+test_expect_success "create $m" '
+   git update-ref $m $A &&
+   test $A = $(cat .git/$m)
+'
+test_expect_success "create $m with oldvalue verification" '
+   git update-ref $m $B $A &&
+   test $B = $(cat .git/$m)
+'
 test_expect_success "fail to delete $m with stale ref" '
test_must_fail git update-ref -d $m $A &&
test $B = "$(cat .git/$m)"
@@ -67,14 +67,14 @@ test_expect_success "fail to create $n" '
test_must_fail git update-ref $n $A
 '
 
-test_expect_success \
-   "create $m (by HEAD)" \
-   "git update-ref HEAD $A &&
-test $A"' = $(cat .git/'"$m"')'
-test_expect_success \
-   "create $m (by HEAD) with oldvalue verification" \
-   "git update-ref HEAD $B $A &&
-test $B"' = $(cat .git/'"$m"')'
+test_expect_success "create $m (by HEAD)" '
+   git update-ref HEAD $A &&
+   test $A = $(cat .git/$m)
+'
+test_expect_success "create $m (by HEAD) with oldvalue verification" '
+   git update-ref HEAD $B $A &&
+   test $B = $(cat .git/$m)
+'
 test_expect_success "fail to delete $m (by HEAD) with stale ref" '
test_must_fail git update-ref -d HEAD $A &&
test $B = $(cat .git/$m)
@@ -176,17 +176,17 @@ test_expect_success '--no-create-reflog overrides 
core.logAllRefUpdates=always'
test_must_fail git reflog exists $outside
 '
 
-test_expect_success \
-   "create $m (by HEAD)" \
-   "git update-ref HEAD $A &&
-test $A"' = $(cat .git/'"$m"')'
-test_expect_success \
-   "pack refs" \
-   "git pack-refs --all"
-test_expect_success \
-   "move $m (by HEAD)" \
-   "git update-ref HEAD $B $A &&
-test $B"' = $(cat .git/'"$m"')'
+test_expect_success "create $m (by HEAD)" '
+   git update-ref HEAD $A &&
+   test $A = $(cat .git/$m)
+'
+test_expect_success 'pack refs' '
+   git pack-refs --all
+'
+test_expect_success "move $m (by HEAD)" '
+   git update-ref HEAD $B $A &&
+   test $B = $(cat .git/$m)
+'
 test_expect_success "delete $m (by HEAD) should remove both packed and loose 
$m" '
test_when_finished "rm -f .git/$m" &&
git update-ref -d HEAD $B &&
@@ -195,13 +195,13 @@ test_expect_success "delete $m (by HEAD) should remove 
both packed and loose $m"
 '
 
 cp -f .git/HEAD .git/HEAD.orig
-test_expect_success "delete symref without dereference" '
+test_expect_success 'delete symref without dereference' '
test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
git update-ref --no-deref -d HEAD &&
test_path_is_missing .git/HEAD
 '
 
-test_expect_success "delete symref without dereference when the referred ref 
is packed" '
+test_expect_success 'delete symref without dereference when the referred ref 
is packed' '
test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
echo foo >foo.c &&
git add foo.c &&
@@ -239,46 +239,46 @@ test_expect_success 'update-ref --no-deref -d can delete 
reference to bad ref' '
test_path_is_missing .git/refs/heads/ref-to-bad
 '
 
-test_expect_success '(not) create HEAD with old sha1' "
+test_expect_success '(not) create HEAD with old sha1' '
test_must_fail git update-ref HEAD $A $B
-"
+'
 test_expect_success "(not) prior created .git/$m" '
test_when_finished "rm -f .git/$m" &&
test_path_is_missing .git/$m
 '
 
-test_expect_success \
-   "create HEAD" \
-   "git update-ref HEAD $A"
-test_expect_success '(not) change HEAD with wrong SHA1' "
+test_expect_success 'create HEAD' '
+   git update-ref HEAD $A
+'
+test_expect_success '(not) change HEAD with wrong SHA1' '
test_must_fail git update-ref HEAD $B $Z
-"
+'
 test_expect_success "(not) changed .git/$m" '
test_when_finished "rm -f .git/$m" &&
! test $B = $(cat .git/$m)
 '
 
 rm -f .git/logs/refs/heads/master
-test_expect_success \
-   "create $m (logged by touch)" \
-   'test_config core.logAllRefUpdates false &&
-GIT_COMMITTER_DATE="2005-05-26 23:30" \
-git update-ref 

Re: Index files autocompletion too slow in big repositories (w / suggestion for improvement)

[Jacob Keller]

On Sat, Apr 15, 2017 at 4:59 AM, Johannes Sixt  wrote:
> Cc Gábor.
>
> Am 15.04.2017 um 00:33 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Sat, Apr 15, 2017 at 12:08 AM, Carlos Pita 
>> wrote:
>>>
>>> This is much faster (below 0.1s):
>>>
>>> __git_index_files ()
>>> {
>>> local dir="$(__gitdir)" root="${2-.}" file;
>>> if [ -d "$dir" ]; then
>>> __git_ls_files_helper "$root" "$1" | \
>>> sed -r 's@/.*@@' | uniq | sort | uniq
>>> fi
>>> }
>>>
>>> time __git_index_files
>>>
>>> real0m0.075s
>>> user0m0.083s
>>> sys0m0.010s
>>>
>>> Most of the improvement is due to the simpler, non-grouping, regex.
>>> Since I expect most of the common prefixes to arrive consecutively,
>>> running uniq before sort also improves things a bit. I'm not removing
>>> leading double quotes anymore (this isn't being done by the current
>>> version, anyway) but this doesn't seem to hurt.
>>>
>>> Despite the dependence on sed this is ten times faster than the
>>> original, maybe an option to enable fast index completion or something
>>> like that might be desirable.
>>
>>
>> It's fine to depend on sed, these shell-scripts are POSIX compatible,
>> and so is sed, we use sed in a lot of the built-in shellscripts.
>
>
> This is about command line completion. We go a long way to avoid forking
> processes there. What is 10x faster on Linux despite of forking a process
> may not be so on Windows.
>
> (I'm not using bash command line completion on Windows, so I can't tell what
> the effect of your suggested change is on Windows. I hope Gábor can comment
> on it.)
>
> -- Hannes
>

In cases like this, might it be worth somehow splitting it so Linux
can use the best thing, and Windows can continue using what's best for
it, since it is a pretty significant advantage on Linux.

Thanks,
Jake

Re: [PATCH] Documentation/git-checkout: make doc. of checkout clearer

[Philip Oakley]

From: "Christoph Michelbach" 

While technically in the documentation, the fact that changes
introduced by a checkout  are staged automatically, was
not obvious when reading its documentation. It is now specifically
pointed out.

Signed-off-by: Christoph Michelbach 
---
Documentation/git-checkout.txt | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt 
b/Documentation/git-checkout.txt

index 8e2c066..cfd7b18 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -85,9 +85,10 @@ Omitting  detaches HEAD at the tip of the 
current branch.

from the index file or from a named  (most often a
commit). In this case, the `-b` and `--track` options are
meaningless and giving either of them results in an error. The
-  argument can be used to specify a specific tree-ish
- (i.e. commit, tag or tree) to update the index for the given


Do these lines above actually need reflowing? Their content hasn't changed 
making it more difficult to spot the significant changes below here.



- paths before updating the working tree.
+  argument can be used to specify the tree-ish (i.e.
+ commit, tag, or tree) to update the index to for the given paths
+ before updating the working tree accordingly.



+   Note that this means
+ that the changes this command introduces are staged automatically.


Does this actually capture the intent of the user confusion it's meant to 
cover? I may have missed the original discussions.


For a clean commit checkout my mental model is not one of anything new being 
actively staged i.e. different from what was in the commit. I can see that 
if a particular tree is checkout then it's implicit staging could well be a 
surprise.


I am in favour of improving the documentation to avoid user surprise


+
'git checkout' with  or `--patch` is used to restore modified or
deleted paths to their original contents from the index or replace paths
--
2.7.4


--
Philip 

[PATCH] Documentation/git-checkout: make doc. of checkout clearer

[Christoph Michelbach]

While technically in the documentation, the fact that changes
introduced by a checkout  are staged automatically, was
not obvious when reading its documentation. It is now specifically
pointed out.

Signed-off-by: Christoph Michelbach 
---
 Documentation/git-checkout.txt | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8e2c066..cfd7b18 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -85,9 +85,10 @@ Omitting  detaches HEAD at the tip of the current 
branch.
    from the index file or from a named  (most often a
    commit).  In this case, the `-b` and `--track` options are
    meaningless and giving either of them results in an error.  The
-argument can be used to specify a specific tree-ish
-   (i.e.  commit, tag or tree) to update the index for the given
-   paths before updating the working tree.
+argument can be used to specify the tree-ish (i.e.
+   commit, tag, or tree) to update the index to for the given paths
+   before updating the working tree accordingly.  Note that this means
+   that the changes this command introduces are staged automatically.
 +
 'git checkout' with  or `--patch` is used to restore modified or
 deleted paths to their original contents from the index or replace paths
-- 
2.7.4

Feature request: Configurable branch colors in git status --short --branch

[Stephen Kent]

It would be nice if the branch, remote tracking branch, and branch 
commit comparison count colors in git status --short --branch were 
configurable like the other git status colors.


Example command output:

$ git status --short --branch
## master...origin/master [ahead 1]
M  README.md
M  wrapper.c
M ws.c
M wt-status.c

In the above example, "master", "origin/master", and "1" (in "ahead 1") 
use git's standard red and green colors. I have configured the staged 
and unstaged modification markers (the M's) to use brighter green and 
red, and I would like to configure the branch info to also use those 
brighter colors. However, there is currently no option (color slot for 
git status in git config) to change the branch colors in 
git status --short --branch.


(See the attached screenshot or http://i.imgur.com/evqgaRd.png to see 
the colors I am talking about.)


Stephen


signature.asc
Description: Digital signature

Re: [PATCH v10 3/3] read-cache: speed up add_index_entry during checkout

[René Scharfe]

Am 14.04.2017 um 21:12 schrieb g...@jeffhostetler.com:

From: Jeff Hostetler 

Teach add_index_entry_with_check() and has_dir_name()
to see if the path of the new item is greater than the
last path in the index array before attempting to search
for it.

During checkout, merge_working_tree() populates the new
index in sorted order, so this change will save at least 2
binary lookups per file.  This preserves the original
behavior but simply checks the last element before starting
the search.

This helps performance on very large repositories.

This can be seen using p0006-read-tree-checkout.sh and the
artificial repository created by t/perf/repos/many-files.sh
with parameters (5, 10, 9).   (1M files in index.)

TestHEAD^   
   HEAD
--
0006.2: read-tree br_base br_ballast (101)  4.15(2.72+1.41) 
   3.21(1.97+1.22) -22.7%
0006.3: switch between br_base br_ballast (101) 8.11(5.57+2.28) 
   6.77(4.36+2.14) -16.5%
0006.4: switch between br_ballast br_ballast_plus_1 (101)   
13.52(8.68+4.35)   11.80(7.38+3.96) -12.7%
0006.5: switch between aliases (101)
13.59(8.75+4.43)   11.85(7.49+3.87) -12.8%

On linux.git, results are:
Test  HEAD^ 
HEAD
--
0006.2: read-tree br_base br_ballast (57994)  0.24(0.22+0.01)   
0.20(0.17+0.02) -16.7%
0006.3: switch between br_base br_ballast (57994) 9.91(5.82+2.79)   
10.26(5.92+2.77) +3.5%
0006.4: switch between br_ballast br_ballast_plus_1 (57994)   0.59(0.44+0.16)   
0.50(0.34+0.18) -15.3%
0006.5: switch between aliases (57994)0.62(0.48+0.16)   
0.50(0.35+0.16) -19.4%

Signed-off-by: Jeff Hostetler 
---
  read-cache.c | 118 ++-
  1 file changed, 116 insertions(+), 2 deletions(-)


Very nice, especially the perf test!  But can we unbundle the different
optimizations into separate patches with their own performance numbers?
Candidates IMHO: The change in add_index_entry_with_check(), the first
hunk in has_dir_name(), the loop shortcuts.  That might also help find
the reason for the slight slowdown of 0006.3 for the kernel repository.


diff --git a/read-cache.c b/read-cache.c
index 97f13a1..ba95fbb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -910,6 +910,9 @@ int strcmp_offset(const char *s1, const char *s2, size_t 
*first_change)
  /*
   * Do we have another file with a pathname that is a proper
   * subset of the name we're trying to add?
+ *
+ * That is, is there another file in the index with a path
+ * that matches a sub-directory in the given entry?
   */
  static int has_dir_name(struct index_state *istate,
const struct cache_entry *ce, int pos, int 
ok_to_replace)
@@ -918,9 +921,51 @@ static int has_dir_name(struct index_state *istate,
int stage = ce_stage(ce);
const char *name = ce->name;
const char *slash = name + ce_namelen(ce);
+   size_t len_eq_last;
+   int cmp_last = 0;
+
+   /*
+* We are frequently called during an iteration on a sorted
+* list of pathnames and while building a new index.  Therefore,
+* there is a high probability that this entry will eventually
+* be appended to the index, rather than inserted in the middle.
+* If we can confirm that, we can avoid binary searches on the
+* components of the pathname.
+*
+* Compare the entry's full path with the last path in the index.
+*/
+   if (istate->cache_nr > 0) {
+   cmp_last = strcmp_offset(name,
+   istate->cache[istate->cache_nr - 1]->name,
+   _eq_last);
+   if (cmp_last > 0) {
+   if (len_eq_last == 0) {
+   /*
+* The entry sorts AFTER the last one in the
+* index and their paths have no common prefix,
+* so there cannot be a F/D conflict.
+*/
+   return retval;
+   } else {
+   /*
+* The entry sorts AFTER the last one in the
+* index, but has a common prefix.  Fall through
+* to the loop below to disect the entry's path
+* and see where the difference is.
+*/
+   }
+   } else if (cmp_last == 0) {
+  

Re: [PATCH 3/3] rebase: pass --[no-]signoff option to git am

[Giuseppe Bilotta]

Damnit! I just realized that I forgot to amend before the format-patch:

On Sat, Apr 15, 2017 at 4:41 PM, Giuseppe Bilotta
 wrote:

> +signoff!   passed to 'git am'

This should be without the ! or --no-signoff is not accepted. Do I
need to resend or ... ?

[PATCH 3/3] rebase: pass --[no-]signoff option to git am

[Giuseppe Bilotta]

This makes it easy to sign off a whole patchset before submission.

Signed-off-by: Giuseppe Bilotta 
---
 Documentation/git-rebase.txt |  5 +
 git-rebase.sh|  3 ++-
 t/t3428-rebase-signoff.sh| 46 
 3 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100755 t/t3428-rebase-signoff.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 67d48e6883..e6f0b93337 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -385,6 +385,11 @@ have the long commit hash prepended to the format.
Recreate merge commits instead of flattening the history by replaying
commits a merge commit introduces. Merge conflict resolutions or manual
amendments to merge commits are not preserved.
+
+--signoff::
+   This flag is passed to 'git am' to sign off all the rebased
+   commits (see linkgit:git-am[1]).
+
 +
 This uses the `--interactive` machinery internally, but combining it
 with the `--interactive` option explicitly is generally not a good
diff --git a/git-rebase.sh b/git-rebase.sh
index 48d7c5ded4..6889fd19f3 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -34,6 +34,7 @@ root!  rebase all reachable commits up to the 
root(s)
 autosquash move commits that begin with squash!/fixup! under -i
 committer-date-is-author-date! passed to 'git am'
 ignore-date!   passed to 'git am'
+signoff!   passed to 'git am'
 whitespace=!   passed to 'git apply'
 ignore-whitespace! passed to 'git apply'
 C=!passed to 'git apply'
@@ -321,7 +322,7 @@ do
--ignore-whitespace)
git_am_opt="$git_am_opt $1"
;;
-   --committer-date-is-author-date|--ignore-date)
+   --committer-date-is-author-date|--ignore-date|--signoff|--no-signoff)
git_am_opt="$git_am_opt $1"
force_rebase=t
;;
diff --git a/t/t3428-rebase-signoff.sh b/t/t3428-rebase-signoff.sh
new file mode 100755
index 00..2afb564701
--- /dev/null
+++ b/t/t3428-rebase-signoff.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='git rebase --signoff
+
+This test runs git rebase --signoff and make sure that it works.
+'
+
+. ./test-lib.sh
+
+# A simple file to commit
+cat >file /")
+EOF
+
+# Expected commit message after rebase without --signoff (or with --no-signoff)
+cat >expected-unsigned < actual &&
+   test_cmp expected-signed actual
+'
+
+test_expect_success 'rebase --no-signoff does not add a sign-off line' '
+   git commit --amend -m "first" &&
+   git rbs --no-signoff HEAD^ &&
+   git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+   test_cmp expected-unsigned actual
+'
+
+test_done
-- 
2.12.2.765.g2bf946761b

[PATCH 1/3] builtin/am: obey --signoff also when --rebasing

[Giuseppe Bilotta]

Signoff is handled in parse_mail(), but not in parse_mail_rebasing(),
since the latter is only used when git-rebase calls git-am with the
--rebasing option, and --signoff is never passed in this case.

In order to introduce (in the upcoming commits) support for `git-rebase
--signoff`, we must make gi-am obey it also in the rebase case. This is
trivially fixed by moving the conditional addition of the signoff from
parse_mail() to the caller am_run(), after either of the parse_mail*()
functions were called.

Signed-off-by: Giuseppe Bilotta 
---
 builtin/am.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index f7a7a971fb..d072027b5a 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const char 
*mail)
strbuf_addbuf(, _message);
strbuf_stripspace(, 0);
 
-   if (state->signoff)
-   am_signoff();
-
assert(!state->author_name);
state->author_name = strbuf_detach(_name, NULL);
 
@@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)
if (skip)
goto next; /* mail should be skipped */
 
+   if (state->signoff)
+   am_append_signoff(state);
+
write_author_script(state);
write_commit_msg(state);
}
-- 
2.12.2.765.g2bf946761b

[PATCH 2/3] builtin/am: fold am_signoff() into am_append_signoff()

[Giuseppe Bilotta]

There are no more direct calls to am_signoff(), so we can fold its
logic  in am_append_signoff().

(This is done in a separate commit rather than in the previous one, to
make it easier to revert this specific change if additional calls are
ever introduced.)

Signed-off-by: Giuseppe Bilotta 
---
 builtin/am.c | 33 +++--
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d072027b5a..b29f885e41 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1181,42 +1181,39 @@ static void NORETURN die_user_resolve(const struct 
am_state *state)
exit(128);
 }
 
-static void am_signoff(struct strbuf *sb)
+/**
+ * Appends signoff to the "msg" field of the am_state.
+ */
+static void am_append_signoff(struct am_state *state)
 {
char *cp;
struct strbuf mine = STRBUF_INIT;
+   struct strbuf sb = STRBUF_INIT;
 
-   /* Does it end with our own sign-off? */
+   strbuf_attach(, state->msg, state->msg_len, state->msg_len);
+
+   /* our sign-off */
strbuf_addf(, "\n%s%s\n",
sign_off_header,
fmt_name(getenv("GIT_COMMITTER_NAME"),
 getenv("GIT_COMMITTER_EMAIL")));
-   if (mine.len < sb->len &&
-   !strcmp(mine.buf, sb->buf + sb->len - mine.len))
+
+   /* Does sb end with it already? */
+   if (mine.len < sb.len &&
+   !strcmp(mine.buf, sb.buf + sb.len - mine.len))
goto exit; /* no need to duplicate */
 
/* Does it have any Signed-off-by: in the text */
-   for (cp = sb->buf;
+   for (cp = sb.buf;
 cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
 cp = strchr(cp, '\n')) {
-   if (sb->buf == cp || cp[-1] == '\n')
+   if (sb.buf == cp || cp[-1] == '\n')
break;
}
 
-   strbuf_addstr(sb, mine.buf + !!cp);
+   strbuf_addstr(, mine.buf + !!cp);
 exit:
strbuf_release();
-}
-
-/**
- * Appends signoff to the "msg" field of the am_state.
- */
-static void am_append_signoff(struct am_state *state)
-{
-   struct strbuf sb = STRBUF_INIT;
-
-   strbuf_attach(, state->msg, state->msg_len, state->msg_len);
-   am_signoff();
state->msg = strbuf_detach(, >msg_len);
 }
 
-- 
2.12.2.765.g2bf946761b

[PATCH 0/3] rebase --signoff

[Giuseppe Bilotta]

Allow signing off a whole patchset by rebasing it with the --signoff
option, which is simply passed through to git am.

Compared to previous incarnations, I've split the am massaging to
separate commits (for cleanliness and easier reverts if needed),
and introduced a test case for both --signoff and its negation.

Giuseppe Bilotta (3):
  builtin/am: obey --signoff also when --rebasing
  builtin/am: fold am_signoff() into am_append_signoff()
  rebase: pass --[no-]signoff option to git am

 Documentation/git-rebase.txt |  5 +
 builtin/am.c | 39 +
 git-rebase.sh|  3 ++-
 t/t3428-rebase-signoff.sh| 46 
 4 files changed, 71 insertions(+), 22 deletions(-)
 create mode 100755 t/t3428-rebase-signoff.sh

-- 
2.12.2.765.g2bf946761b

Re: Git allow to unconditionaly remove files on other developer host

[Konstantin Khomoutov]

On Sat, 15 Apr 2017 14:27:00 +0200
Johannes Sixt  wrote:

> > That curious, but git allow to unconditionally delete files on
> > other developer host when he do `git pull`
[...]
> Know that Git regards everything mentioned in .gitignore as
> dispensible; IOW, by mentioning a file in .gitignore you actually
> give permission to remove the file if necessary. Git does not have a
> feature to say "ignore this file, but it is precious".

KES, you might also be interested in this recent thread [1].

1. 
http://public-inbox.org/git/capuvn2u0uos2mt5+4ejj8m0oknk6xwerl6ce2mihfhtues-...@mail.gmail.com/

Re: Index files autocompletion too slow in big repositories (w / suggestion for improvement)

[Johannes Sixt]

Cc Gábor, resent with working email (hopefully); please follow-up on 
this mail.


Am 15.04.2017 um 00:33 schrieb Ævar Arnfjörð Bjarmason:

On Sat, Apr 15, 2017 at 12:08 AM, Carlos Pita  wrote:

This is much faster (below 0.1s):

__git_index_files ()
{
local dir="$(__gitdir)" root="${2-.}" file;
if [ -d "$dir" ]; then
__git_ls_files_helper "$root" "$1" | \
sed -r 's@/.*@@' | uniq | sort | uniq
fi
}

time __git_index_files

real0m0.075s
user0m0.083s
sys0m0.010s

Most of the improvement is due to the simpler, non-grouping, regex.
Since I expect most of the common prefixes to arrive consecutively,
running uniq before sort also improves things a bit. I'm not removing
leading double quotes anymore (this isn't being done by the current
version, anyway) but this doesn't seem to hurt.

Despite the dependence on sed this is ten times faster than the
original, maybe an option to enable fast index completion or something
like that might be desirable.


It's fine to depend on sed, these shell-scripts are POSIX compatible,
and so is sed, we use sed in a lot of the built-in shellscripts.


This is about command line completion. We go a long way to avoid forking 
processes there. What is 10x faster on Linux despite of forking a 
process may not be so on Windows.


(I'm not using bash command line completion on Windows, so I can't tell 
what the effect of your suggested change is on Windows. I hope Gábor can 
comment on it.)


-- Hannes

Re: Git allow to unconditionaly remove files on other developer host

[Johannes Sixt]

Am 15.04.2017 um 13:36 schrieb KES:

That curious, but git allow to unconditionally delete files on other developer 
host when he do `git pull`

How to reproduce:

1. File should be ignored:
echo "somefile" >> .gitignore

2. Add this ignored file into repository
git add -f somefile

3. Push changes to origin
git push

4. When other developer has also 'somefile' on his host and when he does
git pull

Content of hist local `somefile` file will be replaced by content pushed by 
first developer


This happens *only* if the other developers also have somefile mentioned 
in their .gitignore.




EXPECTED: git should warn about that content will be replaced and do not 
pull/checkout until we force pull/checkout


If somefile is *not* mentioned in their .gitignore, the file is not 
removed and there is a warning.


Know that Git regards everything mentioned in .gitignore as dispensible; 
IOW, by mentioning a file in .gitignore you actually give permission to 
remove the file if necessary. Git does not have a feature to say "ignore 
this file, but it is precious".


-- Hannes

Re: [PATCH 03/12] Makefile & configure: reword outdated comment about PCRE

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 11, 2017 at 12:14 PM, Jeff King  wrote:
> On Sat, Apr 08, 2017 at 01:24:57PM +, Ævar Arnfjörð Bjarmason wrote:
>
>> Reword an outdated comment which suggests that only git-grep can use
>> PCRE.
>
> Makes sense.
>
>> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be
>> -# able to use Perl-compatible regular expressions.
>> +# Define USE_LIBPCRE if you have and want to use libpcre. Various
>> +# commands such as like log, grep offer runtime options to use
>> +# Perl-compatible regular expressions instead of standard or extended
>> +# POSIX regular expressions.
>
> s/such as like log, grep/such as log and grep/ ?
Thanks.

>> diff --git a/configure.ac b/configure.ac
>> [...]
>> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be
>> -# able to use Perl-compatible regular expressions.
>> +# Define USE_LIBPCRE if you have and want to use libpcre. Various
>> +# commands such as like log, grep offer runtime options to use
>> +# Perl-compatible regular expressions instead of standard or extended
>> +# POSIX regular expressions.
>
> Ditto.
>
>> @@ -499,8 +501,10 @@ GIT_CONF_SUBST([NEEDS_SSL_WITH_CRYPTO])
>>  GIT_CONF_SUBST([NO_OPENSSL])
>>
>>  #
>> -# Define USE_LIBPCRE if you have and want to use libpcre. git-grep will be
>> -# able to use Perl-compatible regular expressions.
>> +# Define USE_LIBPCRE if you have and want to use libpcre. Various
>> +# commands such as like log, grep offer runtime options to use
>> +# Perl-compatible regular expressions instead of standard or extended
>> +# POSIX regular expressions.
>
> And again. It's weird that the text appears twice in configure.ac.
> Apparently you can use --with-libpcre or USE_LIBPCRE in the environment?
> Configure is weird.

You can't, I've added this to the commit message:

Copy/pasting this so much in configure.ac is lame, these Makefile-like
flags aren't even used by autoconf, just the corresponding
--with[out]-* options. But copy/pasting the comments that make sense
for the Makefile to configure.ac where they make less sence is the
pattern everything else follows in that file. I'm not going to war
against that as part of this change, just following the existing
pattern.

Re: Index files autocompletion too slow in big repositories (w / suggestion for improvement)

[Johannes Sixt]

Cc Gábor.

Am 15.04.2017 um 00:33 schrieb Ævar Arnfjörð Bjarmason:

On Sat, Apr 15, 2017 at 12:08 AM, Carlos Pita  wrote:

This is much faster (below 0.1s):

__git_index_files ()
{
local dir="$(__gitdir)" root="${2-.}" file;
if [ -d "$dir" ]; then
__git_ls_files_helper "$root" "$1" | \
sed -r 's@/.*@@' | uniq | sort | uniq
fi
}

time __git_index_files

real0m0.075s
user0m0.083s
sys0m0.010s

Most of the improvement is due to the simpler, non-grouping, regex.
Since I expect most of the common prefixes to arrive consecutively,
running uniq before sort also improves things a bit. I'm not removing
leading double quotes anymore (this isn't being done by the current
version, anyway) but this doesn't seem to hurt.

Despite the dependence on sed this is ten times faster than the
original, maybe an option to enable fast index completion or something
like that might be desirable.


It's fine to depend on sed, these shell-scripts are POSIX compatible,
and so is sed, we use sed in a lot of the built-in shellscripts.


This is about command line completion. We go a long way to avoid forking 
processes there. What is 10x faster on Linux despite of forking a 
process may not be so on Windows.


(I'm not using bash command line completion on Windows, so I can't tell 
what the effect of your suggested change is on Windows. I hope Gábor can 
comment on it.)


-- Hannes

Re: includeIf breaks calling dashed externals

[Duy Nguyen]

On Fri, Apr 14, 2017 at 01:43:37PM -0400, Jeff King wrote:
> On Fri, Apr 14, 2017 at 07:04:23PM +0200, Bert Wesarg wrote:
> 
> > Dear Duy,
> > 
> > heaving an includeIf in a git config file breaks calling external git
> > commands, most prominently git-gui.
> > 
> > $ git --version
> > git version 2.12.2.599.gcf11a6797
> > $ git rev-parse --is-inside-work-tree
> > true
> > $ git echo
> > git: 'echo' is not a git command. See 'git --help'.
> > 
> > Did you mean this?
> > fetch
> > $ echo '[includeIf "gitdir:does-not-exists"]path = does-not-exists'
> > >>.git/config
> > $ git rev-parse --is-inside-work-tree
> > true
> > $ git echo
> > fatal: BUG: setup_git_env called without repository
> 
> Probably this fixes it:
> 
> diff --git a/config.c b/config.c
> index b6e4a57b9..8d66bdf56 100644
> --- a/config.c
> +++ b/config.c
> @@ -213,6 +213,9 @@ static int include_by_gitdir(const char *cond, size_t 
> cond_len, int icase)
>   struct strbuf pattern = STRBUF_INIT;
>   int ret = 0, prefix;
>  
> + if (!have_git_dir())
> + return 0;
> +
>   strbuf_add_absolute_path(, get_git_dir());
>   strbuf_add(, cond, cond_len);
>   prefix = prepare_include_condition_pattern();
> 
> But it does raise a question of reading config before/after repository
> setup, since those will give different answers. I guess they do anyway
> because of $GIT_DIR/config.

This happens in execv_dased_external() -> check_pager_config() ->
read_early_config(). We probably could use the same discover_git_directory
trick to get .git dir (because we should find it). Maybe something
like this instead?

diff --git a/config.c b/config.c
index 1a4d85537b..4f540ae578 100644
--- a/config.c
+++ b/config.c
@@ -212,8 +212,14 @@ static int include_by_gitdir(const char *cond, size_t 
cond_len, int icase)
struct strbuf text = STRBUF_INIT;
struct strbuf pattern = STRBUF_INIT;
int ret = 0, prefix;
+   struct strbuf gitdir = STRBUF_INIT;
 
-   strbuf_add_absolute_path(, get_git_dir());
+   if (have_git_dir())
+   strbuf_addstr(, get_git_dir());
+   else if (!discover_git_directory())
+   goto done;
+
+   strbuf_add_absolute_path(, gitdir.buf);
strbuf_add(, cond, cond_len);
prefix = prepare_include_condition_pattern();
 
@@ -237,6 +243,7 @@ static int include_by_gitdir(const char *cond, size_t 
cond_len, int icase)
 icase ? WM_CASEFOLD : 0, NULL);
 
 done:
+   strbuf_release();
strbuf_release();
strbuf_release();
return ret;

--
Duy

Git allow to unconditionaly remove files on other developer host

[KES]

Hi.

That curious, but git allow to unconditionally delete files on other developer 
host when he do `git pull`

How to reproduce:

1. File should be ignored:
echo "somefile" >> .gitignore

2. Add this ignored file into repository
git add -f somefile

3. Push changes to origin
git push

4. When other developer has also 'somefile' on his host and when he does
git pull

Content of hist local `somefile` file will be replaced by content pushed by 
first developer

EXPECTED: git should warn about that content will be replaced and do not 
pull/checkout until we force pull/checkout

Re: [PATCH] worktree add: add --lock option

[Duy Nguyen]

On Sat, Apr 15, 2017 at 3:07 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Nguyễn Thái Ngọc Duy   writes:
>>
>>> -unlink_or_warn(sb.buf);
>>> +if (!ret && opts->keep_locked) {
>>> +/*
>>> + * Don't keep the confusing "initializing" message
>>> + * after it's already over.
>>> + */
>>> +truncate(sb.buf, 0);
>>> +} else {
>>> +unlink_or_warn(sb.buf);
>>> +}
>>
>> builtin/worktree.c: In function 'add_worktree':
>> builtin/worktree.c:314:11: error: ignoring return value of 'truncate', 
>> declared with attribute warn_unused_result [-Werror=unused-result]
>>truncate(sb.buf, 0);
>>^

Yes it's supposed to be safe to ignore the error in this case. I
thought of adding (void) last minute but didn't do it.


>> cc1: all warnings being treated as errors
>> make: *** [builtin/worktree.o] Error 1
>
> I wonder why we need to have "initializing" and then remove the
> string.  Wouldn't it be simpler to do something like this instead?
> Does an empty lockfile have some special meaning?

It's mostly for troubleshooting. If "git worktree add" fails in a
later phase with a valid/locked worktree remains, this gives us a
clue.

>  builtin/worktree.c  | 16 +++-
>  t/t2025-worktree-add.sh |  3 +--
>  2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 3dab07c829..5ebdcce793 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -243,7 +243,10 @@ static int add_worktree(const char *path, const char 
> *refname,
>  * after the preparation is over.
>  */
> strbuf_addf(, "%s/locked", sb_repo.buf);
> -   write_file(sb.buf, "initializing");
> +   if (!opts->keep_locked)
> +   write_file(sb.buf, "initializing");
> +   else
> +   write_file(sb.buf, "added with --lock");
>
> strbuf_addf(_git, "%s/.git", path);
> if (safe_create_leading_directories_const(sb_git.buf))
> @@ -306,15 +309,10 @@ static int add_worktree(const char *path, const char 
> *refname,
>  done:
> strbuf_reset();
> strbuf_addf(, "%s/locked", sb_repo.buf);
> -   if (!ret && opts->keep_locked) {
> -   /*
> -* Don't keep the confusing "initializing" message
> -* after it's already over.
> -*/
> -   truncate(sb.buf, 0);
> -   } else {
> +   if (!ret && opts->keep_locked)
> +   ;
> +   else
> unlink_or_warn(sb.buf);
> -   }
> argv_array_clear(_env);
> strbuf_release();
> strbuf_release();
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 6dce920c03..304f25fcd1 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -65,8 +65,7 @@ test_expect_success '"add" worktree' '
>
>  test_expect_success '"add" worktree with lock' '
> git rev-parse HEAD >expect &&
> -   git worktree add --detach --lock here-with-lock master &&
> -   test_must_be_empty .git/worktrees/here-with-lock/locked
> +   git worktree add --detach --lock here-with-lock master

I think you still need to check for the presence of "locked" file.

>  '
>
>  test_expect_success '"add" worktree from a subdir' '
-- 
Duy

Fwd: Errors running gitk on OS X

[Uxío Prego]

I have a similar one

bash-4.4$ gitk
2017-04-15 13:09:52.514 Wish[3344:197352] *** Terminating app due to uncaught 
exception 'CALayerInvalidGeometry', reason: 'CALayer position contains NaN: 
[nan 0]'
*** First throw call stack:
(
0   CoreFoundation  0x7fffd119048b 
__exceptionPreprocess + 171
1   libobjc.A.dylib 0x7fffe58f2cad 
objc_exception_throw + 48
2   CoreFoundation  0x7fffd120e96d 
+[NSException raise:format:] + 205
3   QuartzCore  0x7fffd6d35520 
_ZN2CA5Layer12set_positionERKNS_4Vec2IdEEb + 152
4   QuartzCore  0x7fffd6d35695 -[CALayer 
setPosition:] + 44
5   QuartzCore  0x7fffd6d35ceb -[CALayer 
setFrame:] + 644
6   CoreUI  0x7fffdcafd4ce 
_ZN20CUICoreThemeRenderer26MakeOrUpdateScrollBarLayerEPK13CUIDescriptoraPP7CALayer
 + 1284
7   CoreUI  0x7fffdcaf94c3 
_ZN20CUICoreThemeRenderer19CreateOrUpdateLayerEPK13CUIDescriptorPP7CALayer + 
1595
8   CoreUI  0x7fffdca7a58d 
_ZN11CUIRenderer19CreateOrUpdateLayerEPK14__CFDictionaryPP7CALayer + 175
9   CoreUI  0x7fffdca7d241 
CUICreateOrUpdateLayer + 221
10  AppKit  0x7fffcf7bc21f 
-[NSCompositeAppearance _callCoreUIWithBlock:options:] + 226
11  AppKit  0x7fffcee620b3 
-[NSAppearance _createOrUpdateLayer:options:] + 76
12  AppKit  0x7fffcf0da71f 
-[NSScrollerImp _animateToRolloverState] + 274
13  AppKit  0x7fffcf09a0f9 
__49-[NSScrollerImp _installDelayedRolloverAnimation]_block_invoke + 673
14  AppKit  0x7fffcef602c5 
-[NSScrollerImp _doWork:] + 15
15  Foundation  0x7fffd2b6ec88 
__NSFireDelayedPerform + 417
16  CoreFoundation  0x7fffd110fd74 
__CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
17  CoreFoundation  0x7fffd110f9ff 
__CFRunLoopDoTimer + 1071
18  CoreFoundation  0x7fffd110f55a 
__CFRunLoopDoTimers + 298
19  CoreFoundation  0x7fffd1106f81 
__CFRunLoopRun + 2065
20  CoreFoundation  0x7fffd1106514 
CFRunLoopRunSpecific + 420
21  Tcl 0x000106fe5b43 
Tcl_WaitForEvent + 314
22  Tcl 0x000106fb55cd 
Tcl_DoOneEvent + 274
23  Tk  0x000106e1ef4f Tk_MainLoop 
+ 33
24  Tk  0x000106e2aa5b Tk_MainEx + 
1566
25  Wish0x000106dfe55a Wish + 9562
26  libdyld.dylib   0x7fffe61d1255 start + 1
)
libc++abi.dylib: terminating with uncaught exception of type NSException
/usr/bin/wish: line 2:  3344 Abort trap: 6   "$(dirname 
$0)/../../System/Library/Frameworks/Tk.framework/Versions/8.5/Resources/Wish.app/Contents/MacOS/Wish"
 "$@"
bash-4.4$

I think it is new - I do not really care as long as is working mint on Linux.
Please pardon the 80th column limit exceed.

> Begin forwarded message:
> 
> From: Evan Aad 
> Subject: Errors running gitk on OS X
> Date: 13 April 2017 at 19:03:45 GMT+2
> To: git@vger.kernel.org
> 
> Running gitk from the command line, while at the top level of a Git
> working directory, produces the following error message, and gitk
> fails to open:
> 
>> objc[1031]: Objective-C garbage collection is no longer supported.
> 
>> /usr/local/bin/wish: line 2:  1031 Abort trap: 6   "$(dirname 
>> $0)/../../../Library/Frameworks/Tk.framework/Versions/8.5/Resources/Wish.app/Contents/MacOS/Wish"
>>  "$@"
> 
> Additionally, the following error message pops up in a window:
> 
>> Wish quit unexpectedly.
> 
>> Click Reopen to open the application again. Click Report to see more 
>> detailed information and send a report to Apple.
> 
>> Ignore | Report... | Reopen
> 
> How can this be fixed?
> 
> ***
> 
> OS: macOS Sierra, Version 10.12.4
> Git version: 2.11.0 (Apple Git-81)

[PATCH 2/3] git-p4: add read_pipe_text() internal function

[Luke Diamand]

The existing read_pipe() function returns an empty string on
error, but also returns an empty string if the command returns
an empty string.

This leads to ugly constructions trying to detect error cases.

Add read_pipe_text() which just returns None on error.

Signed-off-by: Luke Diamand 
---
 git-p4.py | 31 ---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index eab319d76..584b81775 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -160,17 +160,42 @@ def p4_write_pipe(c, stdin):
 real_cmd = p4_build_cmd(c)
 return write_pipe(real_cmd, stdin)
 
-def read_pipe(c, ignore_error=False):
+def read_pipe_full(c):
+""" Read output from  command. Returns a tuple
+of the return status, stdout text and stderr
+text.
+"""
 if verbose:
 sys.stderr.write('Reading pipe: %s\n' % str(c))
 
 expand = isinstance(c,basestring)
 p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, 
shell=expand)
 (out, err) = p.communicate()
-if p.returncode != 0 and not ignore_error:
-die('Command failed: %s\nError: %s' % (str(c), err))
+return (p.returncode, out, err)
+
+def read_pipe(c, ignore_error=False):
+""" Read output from  command. Returns the output text on
+success. On failure, terminates execution, unless
+ignore_error is True, when it returns an empty string.
+"""
+(retcode, out, err) = read_pipe_full(c)
+if retcode != 0:
+if ignore_error:
+out = ""
+else:
+die('Command failed: %s\nError: %s' % (str(c), err))
 return out
 
+def read_pipe_text(c):
+""" Read output from a command with trailing whitespace stripped.
+On error, returns None.
+"""
+(retcode, out, err) = read_pipe_full(c)
+if retcode != 0:
+return None
+else:
+return out.rstrip()
+
 def p4_read_pipe(c, ignore_error=False):
 real_cmd = p4_build_cmd(c)
 return read_pipe(real_cmd, ignore_error)
-- 
2.12.2.719.gcbd162c

[PATCH 0/3] git-p4: use symbolic-ref instead of name-rev

[Luke Diamand]

Followup to earlier discussion about use of name-rev in git-p4.

http://marc.info/?l=git=148979063421355

Luke Diamand (3):
  git-p4: add failing test for name-rev rather than symbolic-ref
  git-p4: add read_pipe_text() internal function
  git-p4: don't use name-rev to get current branch

 git-p4.py| 38 +-
 t/t9807-git-p4-submit.sh | 16 
 2 files changed, 45 insertions(+), 9 deletions(-)

-- 
2.12.2.719.gcbd162c

[PATCH 3/3] git-p4: don't use name-rev to get current branch

[Luke Diamand]

git-p4 was using "git name-rev" to find out the current branch.

That is not safe, since if multiple branches or tags point at
the same revision, the result obtained might not be what is
expected.

Instead use "git symbolic-ref".

Signed-off-by: Luke Diamand 
---
 git-p4.py| 7 +--
 t/t9807-git-p4-submit.sh | 2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 584b81775..8d151da91 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -602,12 +602,7 @@ def p4Where(depotPath):
 return clientPath
 
 def currentGitBranch():
-retcode = system(["git", "symbolic-ref", "-q", "HEAD"], ignore_error=True)
-if retcode != 0:
-# on a detached head
-return None
-else:
-return read_pipe(["git", "name-rev", "HEAD"]).split(" ")[1].strip()
+return read_pipe_text(["git", "symbolic-ref", "--short", "-q", "HEAD"])
 
 def isValidGitDir(path):
 return git_dir(path) != None
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index ae05816e0..3457d5db6 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -139,7 +139,7 @@ test_expect_success 'submit with master branch name from 
argv' '
)
 '
 
-test_expect_failure 'allow submit from branch with same revision but different 
name' '
+test_expect_success 'allow submit from branch with same revision but different 
name' '
test_when_finished cleanup_git &&
git p4 clone --dest="$git" //depot &&
(
-- 
2.12.2.719.gcbd162c

[PATCH 1/3] git-p4: add failing test for name-rev rather than symbolic-ref

[Luke Diamand]

Using name-rev to find the current git branch means that git-p4
does not correctly get the current branch name if there are
multiple branches pointing at HEAD, or a tag.

This change adds a test case which demonstrates the problem.
Configuring which branches are allowed to be submitted from goes
wrong, as git-p4 gets confused about which branch is in use.

This appears to be the only place that git-p4 actually cares
about the current branch.

Signed-off-by: Luke Diamand 
Signed-off-by: Junio C Hamano 
---
 t/t9807-git-p4-submit.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index e37239e65..ae05816e0 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -139,6 +139,22 @@ test_expect_success 'submit with master branch name from 
argv' '
)
 '
 
+test_expect_failure 'allow submit from branch with same revision but different 
name' '
+   test_when_finished cleanup_git &&
+   git p4 clone --dest="$git" //depot &&
+   (
+   cd "$git" &&
+   test_commit "file8" &&
+   git checkout -b branch1 &&
+   git checkout -b branch2 &&
+   git config git-p4.skipSubmitEdit true &&
+   git config git-p4.allowSubmit "branch1" &&
+   test_must_fail git p4 submit &&
+   git checkout branch1 &&
+   git p4 submit
+   )
+'
+
 #
 # Basic submit tests, the five handled cases
 #
-- 
2.12.2.719.gcbd162c

What's cooking in git.git (Apr 2017, #02; draft as of Sat, 15)

[Junio C Hamano]

This is not yet the second issue of this month, but is a draft.  I
haven't caught up with the list traffic yet, but have skimmed most
of the discussion and even managed to pick up some new topics.  

Two requests to topic owners:

 - You'd notice that the topics in the New Topics section below do
   not have any usual two/three-line one-paragraph summary.  If you
   can supply one for me, that would be helpful.

 - Please check what is queued from you (listed below in New topics
   section) to see if I picked up a stale one and you have a newer
   version that I should replace them with, in which case let me
   know the Message-Id of the latest.

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

--
[New Topics]

* ah/diff-files-ours-theirs-doc (2017-04-13) 1 commit
 - diff-files: document --ours etc.


* dt/xgethostname-nul-termination (2017-04-13) 1 commit
 - xgethostname: handle long hostnames


* jh/string-list-micro-optim (2017-04-15) 1 commit
 - string-list: use ALLOC_GROW macro when reallocing string_list


* jh/unpack-trees-micro-optim (2017-04-15) 1 commit
 - unpack-trees: avoid duplicate ODB lookups during checkout


* jh/verify-index-checksum-only-in-fsck (2017-04-15) 1 commit
 - read-cache: force_verify_index_checksum


* jk/no-looking-at-dotgit-outside-repo (2017-04-13) 1 commit
 - has_sha1_file: don't bother if we are not in a repository


* ls/travis-doc-asciidoctor (2017-04-13) 3 commits
 - travis-ci: unset compiler for jobs that do not need one
 - travis-ci: parallelize documentation build
 - travis-ci: build documentation with AsciiDoc and Asciidoctor


* mg/status-in-progress-info (2017-04-14) 1 commit
 - status: show in-progress info for short status


* nd/conditional-config-include (2017-04-14) 2 commits
 - config: resolve symlinks in conditional include's patterns
 - path.c: and an option to call real_path() in expand_user_path()


* nd/worktree-add-lock (2017-04-15) 2 commits
 - SQUASH???
 - worktree add: add --lock option


* ps/pathspec-empty-prefix-origin (2017-04-14) 1 commit
 - pathspec: honor `PATHSPEC_PREFIX_ORIGIN` with empty prefix


* sb/submodule-rm-absorb (2017-04-13) 1 commit
 - submodule.c: add missing ' in error messages


* sr/http-proxy-configuration-fix (2017-04-13) 2 commits
 - http: fix the silent ignoring of proxy misconfiguraion
 - http: honor empty http.proxy option to bypass proxy


* tb/doc-eol-normalization (2017-04-13) 1 commit
 - gitattributes.txt: document how to normalize the line endings


* va/i18n-perl-scripts (2017-04-13) 1 commit
 - git-add--interactive.perl: add missing dot in a message


* bw/submodule-is-active (2017-04-13) 1 commit
 - submodule--helper: fix typo in is_active error message


* gp/rebase-signoff (2017-04-15) 1 commit
 - rebase: pass --[no-]signoff option to git am


* jh/add-index-entry-optim (2017-04-15) 3 commits
 - read-cache: speed up add_index_entry during checkout
 - p0006-read-tree-checkout: perf test to time read-tree
 - read-cache: add strcmp_offset function

--
[Stalled]

* sg/clone-refspec-from-command-line-config (2017-02-27) 1 commit
 - clone: respect configured fetch respecs during initial fetch

 Expecting a reroll.
 cf. <20170227211217.73gydlxb2qu2s...@sigill.intra.peff.net>
 cf. 

Re: [PATCHv3] rebase: pass --[no-]signoff option to git am

[Junio C Hamano]

Giuseppe Bilotta  writes:

>> We need new tests for "git rebase --signoff" that makes sure this
>> works as expected and only when it should.
>
> Would the norm in this case be to introduce the test in the same
> commit, or in a previous commit (as in: this is the feature we want to
> implement, it obviously doesn't work now, but the next commit will fix
> that), or in a subsequent one?

For a new feature (especially with this small implementation), it is
best to have the test in the same commit.  

We often use the "start with expect_failure, update the code while
flipping _failure to _success" pattern but that is primarily
suitable for bugfixes.

Re: [PATCH 00/12] PCREv2 & more

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 15, 2017 at 10:11 AM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Sat, Apr 08, 2017 at 01:24:54PM +, Ævar Arnfjörð Bjarmason wrote:
>>
>>> This adds PCRE v2 support, but as I was adding that I kept noticing
>>> other related problems to fix. It's all bundled up into the same
>>> series because much of it conflicts because it modifies the same test
>>> or other code. Notes on each patch below.
>>
>> Overall, the series looks OK to me.
>>
>> I'm not sure if it is worth all the complexity to carry pcre1/pcre2 as
>> run-time options. That does make it easier to do back-to-back
>> comparisons, but it makes the code a lot more complicated. In particular
>> I'm worried about subtle cases where we pcre1 turns into pcre2 (or vice
>> versa) by use of the aliases. That shouldn't matter to a user for
>> correctness, but it would throw off the benchmarking.
>>
>> If we literally just added USE_LIBPCRE2 and built against one or the
>> other, then all the complexity would be limited to a few #ifdefs. The
>> big drawback AFAICT is that anybody doing timing tests would have to
>> recompile in between.
>
> Yeah, having to dl two libs at runtime, even when you would ever use
> just one in a single run, is less than ideal.  A small downside
> inflicted on everybody will add up to million times more than a
> larger downside only suffered by developers, so I tend to agree with
> you that we probably should simplify to choose just one (or zero) at
> compile time.

I'll document & clarify this in v2, but I don't expect / want anyone
who's distributing git to link to both v1 & v2, more details in my
.

It's just something we already have 95% of the code to support anyway,
and doing the remaining 5% makes it easier to test & benchmark it for
us devs without incurring any real maintenance or tech burden.

But as noted elsewhere in that message I'll include a patch to only
add the ability to use one PCRE version. So we can just review &
discuss the tradeoffs of doing that then.

Re: [PATCHv3] rebase: pass --[no-]signoff option to git am

[Giuseppe Bilotta]

On Sat, Apr 15, 2017 at 11:17 AM, Junio C Hamano  wrote:
> Giuseppe Bilotta  writes:
>
>> This makes it easy to sign off a whole patchset before submission.
>>
>> To make things work, we also fix a design issue in git-am that made it
>> ignore the signoff option during rebase (specifically, signoff was
>> handled in parse_mail(), but not in parse_mail_rebasing()).
>
> I doubt that the above implementation detail in the code is "a
> design issue"; it is a logical consequence of a design whose
> "rebase" never passes "--signoff" down to underlying "am", so it is
> understandable that whoever wants to pass "--signoff" thru during
> the rebase needs to update the implementation, but I do not think it
> is fair to call that "an issue".

Good point. It's an issue now that we want to be able to pass signoff,
but when the split was introduced it most definitely wasn't 8-)

>>  Documentation/git-rebase.txt | 5 +
>>  builtin/am.c | 6 +++---
>>  git-rebase.sh| 3 ++-
>>  3 files changed, 10 insertions(+), 4 deletions(-)
>
> We need new tests for "git rebase --signoff" that makes sure this
> works as expected and only when it should.

Would the norm in this case be to introduce the test in the same
commit, or in a previous commit (as in: this is the feature we want to
implement, it obviously doesn't work now, but the next commit will fix
that), or in a subsequent one?

>> diff --git a/builtin/am.c b/builtin/am.c
>> index f7a7a971fb..d072027b5a 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const 
>> char *mail)
>>   strbuf_addbuf(, _message);
>>   strbuf_stripspace(, 0);
>>
>> - if (state->signoff)
>> - am_signoff();
>> -
>>   assert(!state->author_name);
>>   state->author_name = strbuf_detach(_name, NULL);
>>
>> @@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)
>>   if (skip)
>>   goto next; /* mail should be skipped */
>>
>> + if (state->signoff)
>> + am_append_signoff(state);
>> +
>>   write_author_script(state);
>>   write_commit_msg(state);
>>   }
>
> This removes the last direct caller to am_signoff().  It may be
> worth considering to remove the function and move its body to its
> only internal caller am_append_signoff().

Good point. It becomes a bit bigger change though, so I'll probably
split it off in a separate commit now.


-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCHv3] rebase: pass --[no-]signoff option to git am

[Junio C Hamano]

Giuseppe Bilotta  writes:

> This makes it easy to sign off a whole patchset before submission.
>
> To make things work, we also fix a design issue in git-am that made it
> ignore the signoff option during rebase (specifically, signoff was
> handled in parse_mail(), but not in parse_mail_rebasing()).

I doubt that the above implementation detail in the code is "a
design issue"; it is a logical consequence of a design whose
"rebase" never passes "--signoff" down to underlying "am", so it is
understandable that whoever wants to pass "--signoff" thru during
the rebase needs to update the implementation, but I do not think it
is fair to call that "an issue".

>  Documentation/git-rebase.txt | 5 +
>  builtin/am.c | 6 +++---
>  git-rebase.sh| 3 ++-
>  3 files changed, 10 insertions(+), 4 deletions(-)

We need new tests for "git rebase --signoff" that makes sure this
works as expected and only when it should.

> diff --git a/builtin/am.c b/builtin/am.c
> index f7a7a971fb..d072027b5a 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1321,9 +1321,6 @@ static int parse_mail(struct am_state *state, const 
> char *mail)
>   strbuf_addbuf(, _message);
>   strbuf_stripspace(, 0);
>  
> - if (state->signoff)
> - am_signoff();
> -
>   assert(!state->author_name);
>   state->author_name = strbuf_detach(_name, NULL);
>  
> @@ -1848,6 +1845,9 @@ static void am_run(struct am_state *state, int resume)
>   if (skip)
>   goto next; /* mail should be skipped */
>  
> + if (state->signoff)
> + am_append_signoff(state);
> +
>   write_author_script(state);
>   write_commit_msg(state);
>   }

This removes the last direct caller to am_signoff().  It may be
worth considering to remove the function and move its body to its
only internal caller am_append_signoff().

Re: [PATCH 00/12] PCREv2 & more

[Junio C Hamano]

Jeff King  writes:

> On Sat, Apr 08, 2017 at 01:24:54PM +, Ævar Arnfjörð Bjarmason wrote:
>
>> This adds PCRE v2 support, but as I was adding that I kept noticing
>> other related problems to fix. It's all bundled up into the same
>> series because much of it conflicts because it modifies the same test
>> or other code. Notes on each patch below.
>
> Overall, the series looks OK to me.
>
> I'm not sure if it is worth all the complexity to carry pcre1/pcre2 as
> run-time options. That does make it easier to do back-to-back
> comparisons, but it makes the code a lot more complicated. In particular
> I'm worried about subtle cases where we pcre1 turns into pcre2 (or vice
> versa) by use of the aliases. That shouldn't matter to a user for
> correctness, but it would throw off the benchmarking.
>
> If we literally just added USE_LIBPCRE2 and built against one or the
> other, then all the complexity would be limited to a few #ifdefs. The
> big drawback AFAICT is that anybody doing timing tests would have to
> recompile in between.

Yeah, having to dl two libs at runtime, even when you would ever use
just one in a single run, is less than ideal.  A small downside
inflicted on everybody will add up to million times more than a
larger downside only suffered by developers, so I tend to agree with
you that we probably should simplify to choose just one (or zero) at
compile time.

> So I dunno. I had hoped libpcre2 would be a hands-down win on timing,
> but it sounds like there's a little back-and-forth depending on the
> context. Is there a ticking clock on pcre1 going away? I suspect it will
> be around for many years to come, but if they'll continue optimizing
> pcre2 but not pcre1, then it may make sense to hitch our wagon to the
> one that upstream is actually working on.

Re: [PATCH] worktree add: add --lock option

[Junio C Hamano]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
>
>> -unlink_or_warn(sb.buf);
>> +if (!ret && opts->keep_locked) {
>> +/*
>> + * Don't keep the confusing "initializing" message
>> + * after it's already over.
>> + */
>> +truncate(sb.buf, 0);
>> +} else {
>> +unlink_or_warn(sb.buf);
>> +}
>
> builtin/worktree.c: In function 'add_worktree':
> builtin/worktree.c:314:11: error: ignoring return value of 'truncate', 
> declared with attribute warn_unused_result [-Werror=unused-result]
>truncate(sb.buf, 0);
>^
> cc1: all warnings being treated as errors
> make: *** [builtin/worktree.o] Error 1

I wonder why we need to have "initializing" and then remove the
string.  Wouldn't it be simpler to do something like this instead?
Does an empty lockfile have some special meaning?

 builtin/worktree.c  | 16 +++-
 t/t2025-worktree-add.sh |  3 +--
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 3dab07c829..5ebdcce793 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -243,7 +243,10 @@ static int add_worktree(const char *path, const char 
*refname,
 * after the preparation is over.
 */
strbuf_addf(, "%s/locked", sb_repo.buf);
-   write_file(sb.buf, "initializing");
+   if (!opts->keep_locked)
+   write_file(sb.buf, "initializing");
+   else
+   write_file(sb.buf, "added with --lock");
 
strbuf_addf(_git, "%s/.git", path);
if (safe_create_leading_directories_const(sb_git.buf))
@@ -306,15 +309,10 @@ static int add_worktree(const char *path, const char 
*refname,
 done:
strbuf_reset();
strbuf_addf(, "%s/locked", sb_repo.buf);
-   if (!ret && opts->keep_locked) {
-   /*
-* Don't keep the confusing "initializing" message
-* after it's already over.
-*/
-   truncate(sb.buf, 0);
-   } else {
+   if (!ret && opts->keep_locked)
+   ;
+   else
unlink_or_warn(sb.buf);
-   }
argv_array_clear(_env);
strbuf_release();
strbuf_release();
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 6dce920c03..304f25fcd1 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -65,8 +65,7 @@ test_expect_success '"add" worktree' '
 
 test_expect_success '"add" worktree with lock' '
git rev-parse HEAD >expect &&
-   git worktree add --detach --lock here-with-lock master &&
-   test_must_be_empty .git/worktrees/here-with-lock/locked
+   git worktree add --detach --lock here-with-lock master
 '
 
 test_expect_success '"add" worktree from a subdir' '

Re: Index files autocompletion too slow in big repositories (w / suggestion for improvement)

[Junio C Hamano]

Jacob Keller  writes:

> On Fri, Apr 14, 2017 at 3:33 PM, Ævar Arnfjörð Bjarmason
>  wrote:
>> On Sat, Apr 15, 2017 at 12:08 AM, Carlos Pita  
>> wrote:
>>> This is much faster (below 0.1s):
>>>
>>> __git_index_files ()
>>> {
>>> local dir="$(__gitdir)" root="${2-.}" file;
>>> if [ -d "$dir" ]; then
>>> __git_ls_files_helper "$root" "$1" | \
>>> sed -r 's@/.*@@' | uniq | sort | uniq
>>> fi
>>> }
>>>
>>> time __git_index_files
>>>
>>> real0m0.075s
>>> user0m0.083s
>>> sys0m0.010s
>>>
>>> Most of the improvement is due to the simpler, non-grouping, regex.
>>> Since I expect most of the common prefixes to arrive consecutively,
>>> running uniq before sort also improves things a bit. I'm not removing
>>> leading double quotes anymore (this isn't being done by the current
>>> version, anyway) but this doesn't seem to hurt.
>>>
>>> Despite the dependence on sed this is ten times faster than the
>>> original, maybe an option to enable fast index completion or something
>>> like that might be desirable.
>>>
>>> Best regards
>>
>> It's fine to depend on sed, these shell-scripts are POSIX compatible,
>> and so is sed, we use sed in a lot of the built-in shellscripts.
>>
>> I think you should submit this as a patch, see 
>> Documentation/SubmittingPatches.
>
> Yea it should be fine to use sed.

As long as the use of "sed" is in line with POSIX.1; I do not think
you need the non-portable "-r" merely to strip out everything that
follow the first slash, so perhaps "s|-r|-e|" with the above (and do
not write backslash after pipe at the end of the line---shell knows
you haven't finished talking to it yet if you end a line with a
pipe, and there is no need for backslash), you'd be golden.

Re: [PATCH] worktree add: add --lock option

[Junio C Hamano]

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

> - unlink_or_warn(sb.buf);
> + if (!ret && opts->keep_locked) {
> + /*
> +  * Don't keep the confusing "initializing" message
> +  * after it's already over.
> +  */
> + truncate(sb.buf, 0);
> + } else {
> + unlink_or_warn(sb.buf);
> + }

builtin/worktree.c: In function 'add_worktree':
builtin/worktree.c:314:11: error: ignoring return value of 'truncate', declared 
with attribute warn_unused_result [-Werror=unused-result]
   truncate(sb.buf, 0);
   ^
cc1: all warnings being treated as errors
make: *** [builtin/worktree.o] Error 1