Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Junio C Hamano]

Johannes Sixt  writes:

> Am 11.05.2017 um 03:48 schrieb Junio C Hamano:
>> But perhaps you are right---it may be wrong for the contents of the
>> current index (or any index) to affect how a pathspec element is
>> parsed in the first place.  If the current code (before this series)
>> uses the_index only for error checking, we may want to separate that
>> out of the parse_pathspec() callchain, so that it does not even look
>> at any index (not just the_index).  And that may be a better change
>> overall.
>
> Just a reminder: if core.ignoreCase is set, the variant of a path in
> the index takes precedence over the variant found in the working
> tree. Hence, pathspec must be matched against the index that
> corresponds to the working tree. I guess that's the_index.

Yes, but that is what happens after a path from a working tree is
found to match a pathspec, to coerse its case into the one that is
in the current index.  The parse_pathspec() thing we are discussing
would have finished long time before that actual "matching" is
attempted.

Thanks.

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Johannes Sixt]

Am 11.05.2017 um 03:48 schrieb Junio C Hamano:

But perhaps you are right---it may be wrong for the contents of the
current index (or any index) to affect how a pathspec element is
parsed in the first place.  If the current code (before this series)
uses the_index only for error checking, we may want to separate that
out of the parse_pathspec() callchain, so that it does not even look
at any index (not just the_index).  And that may be a better change
overall.


Just a reminder: if core.ignoreCase is set, the variant of a path in the 
index takes precedence over the variant found in the working tree. 
Hence, pathspec must be matched against the index that corresponds to 
the working tree. I guess that's the_index.


-- Hannes

[PATCH] read-tree: "read-tree -m --empty" does not make sense

[Junio C Hamano]

fb1bb965 ("read-tree: deprecate syntax without tree-ish args",
2010-09-10) wanted to deprecate "git read-tree" without any tree,
which used to be the way to empty the index, and encourage use of
"git read-tree --empty" instead.  

However, when used with "-m", "--empty" does not make any sense,
either, simply because merging 0 trees will result in a different
error anyway.

Omit the deprecation warning and let the code to emit real error
message diagnose the error.

Signed-off-by: Junio C Hamano 
---

Junio C Hamano  writes:

> Jean-Noel Avila  writes:
>
>> "git read-tree -m" requires a tree argument to name the tree to be
>> merged in.  Git uses a cutesy error message to say so and why:
>>
>> $ git read-tree -m
>> warning: read-tree: emptying the index with no arguments is
>> deprecated; use --empty
>> fatal: just how do you expect me to merge 0 trees?
>> $ git read-tree -m --empty
>> fatal: just how do you expect me to merge 0 trees?
>
> This shows another issue.  The "emptying ... is deprecated" message
> shouldn't be given when -m is present.
>
> I am not saying that that needs to be fixed by you and/or as a part
> of this patch.  Just something I noticed while reviewing the patch.

 builtin/read-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 23e212ee8c..284de743c3 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -210,7 +210,7 @@ int cmd_read_tree(int argc, const char **argv, const char 
*unused_prefix)
die("failed to unpack tree object %s", arg);
stage++;
}
-   if (nr_trees == 0 && !read_empty)
+   if (!nr_trees && !read_empty && !opts.merge)
warning("read-tree: emptying the index with no arguments is 
deprecated; use --empty");
else if (nr_trees > 0 && read_empty)
die("passing trees as arguments contradicts --empty");
-- 
2.13.0-336-gf73534b083

Re: [PATCH v2 3/3] git-filter-branch: make the error msg when missing branch more open

[Junio C Hamano]

Jean-Noel Avila  writes:

> git-filter-branch requires the specification of a branch by one way or
> another. If no branch appears to have been specified, we know the user
> got the usage wrong but we don't know what they were trying to do ---
> e.g. maybe they specified the ref to rewrite but in the wrong place.
>
> The safest solution is to just print the usage in this case.
>
> Signed-off-by: Jean-Noel Avila 
> ---
>  git-filter-branch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-filter-branch.sh b/git-filter-branch.sh
> index 2b8cdba15..bda2bae23 100755
> --- a/git-filter-branch.sh
> +++ b/git-filter-branch.sh
> @@ -239,7 +239,7 @@ git rev-parse --no-flags --revs-only --symbolic-full-name 
> \
>  sed -e '/^^/d' "$tempdir"/raw-heads >"$tempdir"/heads
>  
>  test -s "$tempdir"/heads ||
> - die "Which ref do you want to rewrite?"
> + usage
>  
>  GIT_INDEX_FILE="$(pwd)/../index"
>  export GIT_INDEX_FILE

I tend to agree with Ævar on this one.  It is not apparent to the
end user after this change what exactly was wrong in the input; for
that matter, it is not even clear that the command is refusing to
run because it found problem with the input.  

Trying to move away from asking "I didn't get that, what did you
mean?" is one thing, and that can be done by saying "no ref to
rewrite given" or something.  We may want to make it into a more
"positive" nudge, telling the user what to do, e.g. "give me the
refs to rewrite."

Thanks.

Re: [PATCH v2 2/3] read-tree -m: make error message for merging 0 trees less smart aleck

[Junio C Hamano]

Jean-Noel Avila  writes:

> "git read-tree -m" requires a tree argument to name the tree to be
> merged in.  Git uses a cutesy error message to say so and why:
>
> $ git read-tree -m
> warning: read-tree: emptying the index with no arguments is
> deprecated; use --empty
> fatal: just how do you expect me to merge 0 trees?
> $ git read-tree -m --empty
> fatal: just how do you expect me to merge 0 trees?

This shows another issue.  The "emptying ... is deprecated" message
shouldn't be given when -m is present.

I am not saying that that needs to be fixed by you and/or as a part
of this patch.  Just something I noticed while reviewing the patch.

>  Merging
>  ---
> -If `-m` is specified, 'git read-tree' can perform 3 kinds of
> -merge, a single tree merge if only 1 tree is given, a
> -fast-forward merge with 2 trees, or a 3-way merge if 3 trees are
> -provided.
> +If `-m` is specified, at least one tree must be given on the command
> +line. 'git read-tree' can perform 3 kinds of merge, a single tree
> +merge if only 1 tree is given, a fast-forward merge with 2 trees, or a
> +3-way merge if 3 trees are provided.

It may not incorrect per-se, but the existing enumeration already
say 1, 2 and 3 are the valid choices, so "at least one" may be
redundant.

One incorrectness that needs to be changed is "if 3 trees are
provided"; it is "if 3 or more trees".  Again, not the topic of your
change, but this one you may want to address while you are at it.

>   if (opts.merge) {
> - if (stage < 2)
> - die("just how do you expect me to merge %d trees?", 
> stage-1);
>   switch (stage - 1) {
> + case 0:

Could "stage" be 0 (or negative) when we come here?  If so, this rewrite
may no longer diagnose the error correctly in such a case.

... goes and looks ...

I think it begins with either 0 or 1 and then only counts up, so we
should be safe.  Rolling it in the switch() like this patch does
makes it easier to follow what is going on, I think.

> + die("you must specify at least one tree to merge");
> + break;
>   case 1:
>   opts.fn = opts.prefix ? bind_merge : oneway_merge;
>   break;

Thanks.

Re: [PATCH v2 1/3] usability: don't ask questions if no reply is required

[Junio C Hamano]

Jean-Noel Avila  writes:

> diff --git a/builtin/am.c b/builtin/am.c
> index a95dd8b4e..f5afa438d 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -1312,7 +1312,7 @@ static int parse_mail(struct am_state *state, const 
> char *mail)
>   }
>  
>   if (is_empty_file(am_path(state, "patch"))) {
> - printf_ln(_("Patch is empty. Was it split wrong?"));
> + printf_ln(_("Patch is empty. It may have been split wrong."));
>   die_user_resolve(state);
>   }

While I do not belong to "we should feel free to ask rhetorical
questions" camp, I do not mind this particular rewrite.  An obvious
alternative is just to stop the sentence with "Patch is empty."

At this point in the code, we do not even know why we are seeing an
empty patch, and "perhaps it was incorrectly split" is not a
particularly useful idle speculation that would help the user who
sees it.

> @@ -1940,7 +1940,7 @@ static void am_resolve(struct am_state *state)
>  
>   if (unmerged_cache()) {
>   printf_ln(_("You still have unmerged paths in your index.\n"
> - "Did you forget to use 'git add'?"));
> + "You might want to use 'git add' on them."));

This case is *not* an "rhetorical question is the most succinct way
to convey the information" situation; I think this rewrite is a
definite improvement.  "You might want to 'git add' them" may be
more succinct, though.

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index bfa5419f3..05037b9b6 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1287,7 +1287,7 @@ int cmd_checkout(int argc, const char **argv, const 
> char *prefix)
>*/
>   if (opts.new_branch && argc == 1)
>   die(_("Cannot update paths and switch to branch '%s' at 
> the same time.\n"
> -   "Did you intend to checkout '%s' which can not be 
> resolved as commit?"),
> +   "'%s' can not be resolved as commit, but it 
> should."),

I am not sure a firm statement "but it should" is an improvement.
This message is given when the user says:

$ git checkout -b newone naster

And "but it should" is appropriate when it is a mistyped "I want to
create and checkout 'newone' branch at the same commit as 'master'
branch", i.e.

$ git checkout -b newone master

The reason why the message begins with "Cannot update paths and ..."
is because it could be a mistyped "I want to grab the file 'naster'
out of 'newone' branch", i.e. the user meant to say this:

$ git checkout newone naster

IOW, the current error message is hedging its bets, because it does
not want to exclude the possibility that "-b" is there by mistake
(as opposed to 'naster' is the typo).

If we ignore that possibility and assume that 'naster' is the typo
(iow, the user did mean "-b"), then your updated message makes
sense.  But if we commit to "the user meant -b", we could make the
message even more helpful by being more direct, e.g.

die("'%s' is not a commit and a branch '%s' cannot be created from it",
argv[0], opts.new_branch);

> diff --git a/help.c b/help.c
> index bc6cd19cf..4658a55c6 100644
> --- a/help.c
> +++ b/help.c
> @@ -411,8 +411,8 @@ const char *help_unknown_cmd(const char *cmd)
>  
>   if (SIMILAR_ENOUGH(best_similarity)) {
>   fprintf_ln(stderr,
> -Q_("\nDid you mean this?",
> -   "\nDid you mean one of these?",
> +Q_("\nThe most approaching command is",
> +   "\nThe most approaching commands are",
>  n));

With "closest" or "most similar", as others pointed out, I think
this may be an improvement.

Thanks.

Re: What's cooking in git.git (May 2017, #03; Wed, 10)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> * ab/grep-pcre-v2 (2017-05-02) 19 commits
> ...
> This can just be ejected. ...
> 
> * ab/grep-threading-cleanup (2017-04-16) 8 commits
> ...
> Can also be discarded, will re-send in a different form soon.

Will do and wait for replacements.  Thanks.

Re: [PATCH] status: show in-progress info for short status

[Junio C Hamano]

Jacob Keller  writes:

> On Wed, Apr 12, 2017 at 11:09 PM, Junio C Hamano  wrote:
>> Jacob Keller  writes:
>>
>>> Personally, I would want this to become the default and not have a new
>>> option to trigger it. I think we could also extend the porcelain
>>> format to include this information as well, but I'm not too familiar
>>> with how the v2 format extends or not.

I started to do a simple s/inprogress/in-progress/ while waiting,
but Ævar reminded me of this discussion---and I agree with you that
this probably should be on.  Moreover, I think this should not be
optional (which makes s/inprogress/in-progress/ a moot point).  

Michael went the cautious route to make it optional just like the
"-b/--branch" option, but I think "-b/--branch" was a design mistake
and not something we want to mimic.  The long output format gives
the same information without "--branch", and giving "--no-branch"
does not even disable the information in the long format; "--branch"
is only effective in the short format, even though giving it to long
format does not diagnose any error.  

We should have just enhanced the feature unconditionally, I would
think.  But fixing that past mistake is a separate issue.

>> I think the general rule of thumb for --porcelain is that we can
>> freely introduce new record types without version bump, and expect
>> the reading scripts to ignore unrecognised records (we may need to
>> describe this a bit more strongly in our document, though), while
>> changes to the format of existing records must require a command
>> line option that cannot be turned on by default with configuration
>> (or a version bump, if you want to change the output format by
>> default).
>>
>> I am getting the impression that this "we are doing X" is a new and
>> discinct record type that existing readers can safely ignore?  If
>> that is the case, it may be better to add it without making it
>> optional.
>
> Correct. But either way we should be free to change and extend the
> non-porcelain format without worry I thought?

While I think we should update "short" and "porcelain" at the same
time when it is clear that we can (e.g. we are adding a new record
type and we make sure that the current readers of "porcelain" output
would ignore the new record type), an update to "porcelain" can come
later, as long as we don't forget.  Otherwise people will script
around "short", ignoring "porcelain", no?

Thanks.

Re: What's cooking in git.git (May 2017, #03; Wed, 10)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Wed, May 10, 2017 at 7:18 AM, Junio C Hamano  wrote:
>> [New Topics]
>>
>> * ab/compat-regex-update (2017-05-09) 2 commits
>>  - compat/regex: update the gawk regex engine from upstream
>>  - compat/regex: add a README with a maintenance guide
>>
>>  Will merge to 'next'.
>
> [Sent last E-Mail too soon]
>
> There's a tiny typo in "compat/regex: update the gawk regex engine
> from upstream", one of the two bullet points says "Git project.f",
> should say "Git project.", i.e. without the stray ".f".

Yeah, I recall seeing it and feeling puzzled when I queued it.  Will
remove and requeue.

Thanks.

> If you don't mind amending that before merging it down that would be
> great, due to the whole multi-patch !fixup -> you needing to squash
> that sounds easier

Re: [PATCH v3 1/1] t0027: tests are not expensive; remove t0025

[Junio C Hamano]

Torstem Bögershausen  writes:

>> Am 10.05.2017 um 17:52 schrieb Johannes Schindelin 
>> :
>> 
>>  ...
>> While at it, retire t0025: Recent "stress" tests show that t0025 if
>> flaky, reported by Lars Schneider , but all
>> tests in t0025 are covered by t0027 already.
>> 
>> Signed-off-by: Torsten Bögershausen 
>> Acked-by: Johannes Schindelin 
>
> Ok for me, thanks for helping out.

I'll do a s/if flaky/is flaky/ and queue it.

Thanks, both.  

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Junio C Hamano]

Brandon Williams  writes:

> ls-files is the only command (that I know of) which does cache pruning
> based on the common prefix of all pathspecs given.  If there is a
> submodule named 'sub' and you provided a pathspec 'sub/', the matching
> logic can handle this but the cache pruning logic would prune all
> entries from the index which don't have a leading 'sub/' which is the
> common prefix of all pathspecs (if you didn't strip off the slash).
> Meaning you'd prune the submodule 'sub' when  you really didn't want to.
> This could probably be fixed to have the cache pruning logic to prune by
> ignoring the trailing slash always.
>
> So there's another option here if you don't feel quite right about
> piping through an index into parse_pathspec just yet.

Oh, don't get me wrong---I do not think passing an index_state
instance throughout the callchain (and perhaps later we may pass an
instance of "repository" instead) is a wrong move in the longer term
for various APIs.  I was just wondering if we have callers that can
benefit from this change immediately---manipulators like "commit" do
already use multiple instances of index_state structure.

But perhaps you are right---it may be wrong for the contents of the
current index (or any index) to affect how a pathspec element is
parsed in the first place.  If the current code (before this series)
uses the_index only for error checking, we may want to separate that
out of the parse_pathspec() callchain, so that it does not even look
at any index (not just the_index).  And that may be a better change
overall.

Thanks.

Re: What's cooking in git.git (May 2017, #03; Wed, 10)

[Junio C Hamano]

Jean-Noël AVILA  writes:

> Le mercredi 10 mai 2017, 10:08:14 CEST Ævar Arnfjörð Bjarmason a écrit :
>> > * ja/doc-l10n (2017-03-20) 3 commits
>> >  . SQUASH???
>> >  . l10n: add git-add.txt to localized man pages
>> >  . l10n: introduce framework for localizing man pages
>> >
>> >  A proposal to use po4a to localize our manual pages.
>> >
>> >  Will discard.
>> >  Has been stalled for too long.
>> 
>> Jean-Noel: Need any help with this?
>
> I restarted on this yesterday

OK, then I'll discard what is listed there, expecting that we will
see an updated attempt that we can queue in the future.  No rush.

Thanks.

Re: [PATCH v2 1/2] perf: add function to setup a fresh test repo

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Thu, May 11, 2017 at 1:30 AM, Jonathan Nieder  wrote:
>> Hi,
>>
>> Ęvar Arnfjörš Bjarmason wrote:
>>
>> [...]
>>>  # call at least one of these to establish an appropriately-sized repository
>>> +test_perf_fresh_repo () {
>>> + repo="${1:-$TRASH_DIRECTORY}"
>>> + "$MODERN_GIT" init -q "$repo" &&
>>> + cd "$repo" &&
>>> + test_perf_do_repo_symlink_config_
>>> +}
>>
>> Unlike the other two variants, wouldn't this leave the cwd inside the
>> new repo?
>>
>> In other words, I wonder if the commands starting with the 'cd' should
>> be in a subshell.
>>
>> Thanks and hope that helps,
>
> Yup, I'll fix that. Thanks for the review & also on the other patch.
> Will send a v3 with these issues fixed.

Thanks.  FWIW, I didn't see anything questionable other than
Jonathan spotted.

Re: t6134 test failure in 'pu'

[Junio C Hamano]

Thanks for a quick report.  Yes, that was a stupid mismerge.

Will correct in my rerere database X-<.

Re: [PATCH v2 1/2] perf: add function to setup a fresh test repo

[Ævar Arnfjörð Bjarmason]

On Thu, May 11, 2017 at 1:30 AM, Jonathan Nieder  wrote:
> Hi,
>
> Ęvar Arnfjörš Bjarmason wrote:
>
> [...]
>>  # call at least one of these to establish an appropriately-sized repository
>> +test_perf_fresh_repo () {
>> + repo="${1:-$TRASH_DIRECTORY}"
>> + "$MODERN_GIT" init -q "$repo" &&
>> + cd "$repo" &&
>> + test_perf_do_repo_symlink_config_
>> +}
>
> Unlike the other two variants, wouldn't this leave the cwd inside the
> new repo?
>
> In other words, I wonder if the commands starting with the 'cd' should
> be in a subshell.
>
> Thanks and hope that helps,

Yup, I'll fix that. Thanks for the review & also on the other patch.
Will send a v3 with these issues fixed.

Re: [PATCH v2 2/2] perf: add test showing exponential growth in path globbing

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Add a test showing that ls-files times grow exponentially in the face
> of some pathological globs, whereas refglobs via for-each-ref don't in
> practice suffer from the same issue.

Cool.

[...]
> --- /dev/null
> +++ b/t/perf/p0100-globbing.sh
> @@ -0,0 +1,48 @@
> +#!/bin/sh
> +
> +test_description="Tests pathalogical globbing performance
> +
> +Shows how Git's globbing performance performs when given the sort of
> +pathalogical patterns described in at https://research.swtch.com/glob

s/pathalogical/pathological/

> +"
> +
> +. ./perf-lib.sh
> +
> +test_globs_big='10 25 50 75 100'
> +test_globs_small='1 2 3 4 5 6'
> +
> +test_perf_fresh_repo
> +
> +test_expect_success 'setup' '
> + for i in $(test_seq 1 100)
> + do
> + printf "a" >>refname &&
> + for j in $(test_seq 1 $i)
> + do
> + printf "a*" >>refglob.$i
> + done &&
> + echo b >>refglob.$i
> + done &&
> + test_commit $(cat refname) &&
> + for i in $(test_seq 1 100)
> + do
> + echogit tag $(cat refname)-$i

Leftover echo from debugging?

> + done &&
> + test_commit hello
> +'
> +
> +for i in $test_globs_big
> +do
> + test_perf "refglob((a*)^nb) against tag a^100; n = $i" '
> + git for-each-ref "refs/tags/$(cat refglob.'$i')b"
> + '
> +done
> +
> +for i in $test_globs_small
> +do
> + test_perf "fileglob((a*)^nb) against file (a^100).t; n = $i" '
> + git ls-files "$(cat refglob.'$i')b"
> + '
> +done

The rest looks sensible.

Thanks,
Jonathan

Re: [PATCH v2 1/2] perf: add function to setup a fresh test repo

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

[...]
>  # call at least one of these to establish an appropriately-sized repository
> +test_perf_fresh_repo () {
> + repo="${1:-$TRASH_DIRECTORY}"
> + "$MODERN_GIT" init -q "$repo" &&
> + cd "$repo" &&
> + test_perf_do_repo_symlink_config_
> +}

Unlike the other two variants, wouldn't this leave the cwd inside the
new repo?

In other words, I wonder if the commands starting with the 'cd' should
be in a subshell.

Thanks and hope that helps,
Jonathan

Re: [PATCH v3] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> fetch-pack, when fetching a literal SHA-1 from a server that is not
> configured with uploadpack.allowtipsha1inwant (or similar), always
> returns an error message of the form "Server does not allow request for
> unadvertised object %s". However, it is sometimes the case that such
> object is advertised. This situation would occur, for example, if a user
> or a script was provided a SHA-1 instead of a branch or tag name for
> fetching, and wanted to invoke "git fetch" or "git fetch-pack" using
> that SHA-1.

Yay, thanks again.

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -598,6 +598,7 @@ static void filter_refs(struct fetch_pack_args *args,
>  {
>   struct ref *newlist = NULL;
>   struct ref **newtail = 
> + struct ref *unmatched = NULL;
>   struct ref *ref, *next;
>   int i;
>  
> @@ -631,13 +632,15 @@ static void filter_refs(struct fetch_pack_args *args,
>   ref->next = NULL;
>   newtail = >next;
>   } else {
> - free(ref);
> + ref->next = unmatched;
> + unmatched = ref;

Interesting.  Makes sense.

[...]
>   /* Append unmatched requests to the list */
>   for (i = 0; i < nr_sought; i++) {
>   unsigned char sha1[20];
> + int can_append = 0;

Can this be simplified by factoring out a function?  I.e. something
like

if ((... ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)
|| find_oid_among_refs(unmatched, oid)
|| find_oid_among_refs(newlist, oid)) {
ref->match_status = REF_MATCHED;
...
} else {
ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
}

[...]
> @@ -657,6 +679,11 @@ static void filter_refs(struct fetch_pack_args *args,
>   }
>   }
>   *refs = newlist;
> +
> + for (ref = unmatched; ref; ref = next) {
> + next = ref->next;
> + free(ref);
> + }
>  }

optional nit: keeping the "*refs =" line as the last line of the
function would make it easier to see the contract at a glance.  (That
said, a doc comment at the top would make it even clearer.)

> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '

Yay, thanks much for these.

[...]
> +test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named 
> ref' '

Ha, you read my mind. :)

Except for the search-ref-list-for-oid function needing to be factored out,
Reviewed-by: Jonathan Nieder 

Thanks.

[PATCH v2 0/2] perf: show that wildmatch() regressed for pathological cases in v2.0

[Ævar Arnfjörð Bjarmason]

This is a re-send of <20170424211249.28553-1-ava...@gmail.com>
(https://public-inbox.org/git/20170424211249.28553-1-ava...@gmail.com/)
which fell through the cracks at the time.

The only change is a typo fix in the description of the test in 2/2.

Ævar Arnfjörð Bjarmason (2):
  perf: add function to setup a fresh test repo
  perf: add test showing exponential growth in path globbing

 t/perf/README|  1 +
 t/perf/p0100-globbing.sh | 48 
 t/perf/perf-lib.sh   | 17 +
 3 files changed, 62 insertions(+), 4 deletions(-)
 create mode 100755 t/perf/p0100-globbing.sh

-- 
2.11.0

[PATCH v2 1/2] perf: add function to setup a fresh test repo

[Ævar Arnfjörð Bjarmason]

Add a function to setup a fresh test repo via 'git init' to compliment
the existing functions to copy over a normal & large repo.

Some performance tests don't need any existing repository data at all
to be significant, e.g. tests which stress glob matches against
revisions or files, which I'm about to add in a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/perf/README  |  1 +
 t/perf/perf-lib.sh | 17 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/t/perf/README b/t/perf/README
index 49ea4349be..de2fe15696 100644
--- a/t/perf/README
+++ b/t/perf/README
@@ -106,6 +106,7 @@ sources perf-lib.sh:
 
 After that you will want to use some of the following:
 
+   test_perf_fresh_repo# sets up an empty repository
test_perf_default_repo  # sets up a "normal" repository
test_perf_large_repo# sets up a "large" repository
 
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index ab4b8b06ae..f51fc773e8 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -78,6 +78,10 @@ if test -z "$GIT_PERF_LARGE_REPO"; then
GIT_PERF_LARGE_REPO=$TEST_DIRECTORY/..
 fi
 
+test_perf_do_repo_symlink_config_ () {
+   test_have_prereq SYMLINKS || git config core.symlinks false
+}
+
 test_perf_create_repo_from () {
test "$#" = 2 ||
error "bug in the test script: not 2 parameters to test-create-repo"
@@ -102,15 +106,20 @@ test_perf_create_repo_from () {
) &&
(
cd "$repo" &&
-   "$MODERN_GIT" init -q && {
-   test_have_prereq SYMLINKS ||
-   git config core.symlinks false
-   } &&
+   "$MODERN_GIT" init -q &&
+   test_perf_do_repo_symlink_config_ &&
mv .git/hooks .git/hooks-disabled 2>/dev/null
) || error "failed to copy repository '$source' to '$repo'"
 }
 
 # call at least one of these to establish an appropriately-sized repository
+test_perf_fresh_repo () {
+   repo="${1:-$TRASH_DIRECTORY}"
+   "$MODERN_GIT" init -q "$repo" &&
+   cd "$repo" &&
+   test_perf_do_repo_symlink_config_
+}
+
 test_perf_default_repo () {
test_perf_create_repo_from "${1:-$TRASH_DIRECTORY}" "$GIT_PERF_REPO"
 }
-- 
2.11.0

[PATCH v2 2/2] perf: add test showing exponential growth in path globbing

[Ævar Arnfjörð Bjarmason]

Add a test showing that ls-files times grow exponentially in the face
of some pathological globs, whereas refglobs via for-each-ref don't in
practice suffer from the same issue.

As noted in the test description this is a test to see whether Git
suffers from the issue noted in an article Russ Cox posted today about
common bugs in various glob implementations:
https://research.swtch.com/glob

The pathological git-ls-files globbing is done by wildmatch() in
wildmatch.c. The for-each-ref codepath also uses wildmatch(), but will
always match against e.g. "refs/tags/aaa...", not "aaa.." as
git-ls-files will.

I'm unsure why the pathological case isn't triggered by for-each-ref,
but in any case, now we have a performance test for it.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/perf/p0100-globbing.sh | 48 
 1 file changed, 48 insertions(+)
 create mode 100755 t/perf/p0100-globbing.sh

diff --git a/t/perf/p0100-globbing.sh b/t/perf/p0100-globbing.sh
new file mode 100755
index 00..e98fd7ce4b
--- /dev/null
+++ b/t/perf/p0100-globbing.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description="Tests pathalogical globbing performance
+
+Shows how Git's globbing performance performs when given the sort of
+pathalogical patterns described in at https://research.swtch.com/glob
+"
+
+. ./perf-lib.sh
+
+test_globs_big='10 25 50 75 100'
+test_globs_small='1 2 3 4 5 6'
+
+test_perf_fresh_repo
+
+test_expect_success 'setup' '
+   for i in $(test_seq 1 100)
+   do
+   printf "a" >>refname &&
+   for j in $(test_seq 1 $i)
+   do
+   printf "a*" >>refglob.$i
+   done &&
+   echo b >>refglob.$i
+   done &&
+   test_commit $(cat refname) &&
+   for i in $(test_seq 1 100)
+   do
+   echogit tag $(cat refname)-$i
+   done &&
+   test_commit hello
+'
+
+for i in $test_globs_big
+do
+   test_perf "refglob((a*)^nb) against tag a^100; n = $i" '
+   git for-each-ref "refs/tags/$(cat refglob.'$i')b"
+   '
+done
+
+for i in $test_globs_small
+do
+   test_perf "fileglob((a*)^nb) against file (a^100).t; n = $i" '
+   git ls-files "$(cat refglob.'$i')b"
+   '
+done
+
+test_done
-- 
2.11.0

[PATCH v3] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Tan]

fetch-pack, when fetching a literal SHA-1 from a server that is not
configured with uploadpack.allowtipsha1inwant (or similar), always
returns an error message of the form "Server does not allow request for
unadvertised object %s". However, it is sometimes the case that such
object is advertised. This situation would occur, for example, if a user
or a script was provided a SHA-1 instead of a branch or tag name for
fetching, and wanted to invoke "git fetch" or "git fetch-pack" using
that SHA-1.

Teach fetch-pack to also check the SHA-1s of the refs in the received
ref advertisement if a literal SHA-1 was given by the user.

Signed-off-by: Jonathan Tan 
---

Reviewers, note that the patch has been substantially rewritten.

After looking at ways to solve jrnieder's performance concerns, if we're
going to need to manage one more item of state within the function, I
might as well use my earlier idea of storing unmatched refs in its own
list instead of immediately freeing them. This version of the patch
should have much better performance characteristics.

I have also added the test requested by jrnieder (to show that
fetch-pack cannot fetch a SHA-1 if it is not advertised as a ref).

 fetch-pack.c  | 29 -
 t/t5500-fetch-pack.sh | 35 +++
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index afb8b0502..5cace7458 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -598,6 +598,7 @@ static void filter_refs(struct fetch_pack_args *args,
 {
struct ref *newlist = NULL;
struct ref **newtail = 
+   struct ref *unmatched = NULL;
struct ref *ref, *next;
int i;
 
@@ -631,13 +632,15 @@ static void filter_refs(struct fetch_pack_args *args,
ref->next = NULL;
newtail = >next;
} else {
-   free(ref);
+   ref->next = unmatched;
+   unmatched = ref;
}
}
 
/* Append unmatched requests to the list */
for (i = 0; i < nr_sought; i++) {
unsigned char sha1[20];
+   int can_append = 0;
 
ref = sought[i];
if (ref->match_status != REF_NOT_MATCHED)
@@ -649,6 +652,25 @@ static void filter_refs(struct fetch_pack_args *args,
 
if ((allow_unadvertised_object_request &
(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
+   can_append = 1;
+   } else {
+   struct ref *u;
+   /* Check all refs, including those already matched */
+   for (u = unmatched; u; u = u->next) {
+   if (!oidcmp(>old_oid, >old_oid)) {
+   can_append = 1;
+   goto can_append;
+   }
+   }
+   for (u = newlist; u; u = u->next) {
+   if (!oidcmp(>old_oid, >old_oid)) {
+   can_append = 1;
+   break;
+   }
+   }
+   }
+can_append:
+   if (can_append) {
ref->match_status = REF_MATCHED;
*newtail = copy_ref(ref);
newtail = &(*newtail)->next;
@@ -657,6 +679,11 @@ static void filter_refs(struct fetch_pack_args *args,
}
}
*refs = newlist;
+
+   for (ref = unmatched; ref; ref = next) {
+   next = ref->next;
+   free(ref);
+   }
 }
 
 static void mark_alternate_complete(struct object *obj)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index b5865b385..80a1a3239 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -547,6 +547,41 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
 '
 
+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a 
ref' '
+   rm -rf server client &&
+   git init server &&
+   test_commit -C server 1 &&
+
+   git init client &&
+   git -C client fetch-pack ../server \
+   $(git -C server rev-parse refs/heads/master)
+'
+
+test_expect_success 'fetch-pack can fetch a raw sha1 overlapping a named ref' '
+   rm -rf server client &&
+   git init server &&
+   test_commit -C server 1 &&
+   test_commit -C server 2 &&
+
+   git init client &&
+   git -C client fetch-pack ../server \
+   $(git -C server rev-parse refs/tags/1) refs/tags/1
+'
+
+test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised 
as a ref' '
+   rm -rf server &&
+
+   git init server &&
+   test_commit 

Re: Possible bug in includeIf / conditional includes

[Raphael Stolt]

> Am 10.05.2017 um 23:21 schrieb Raphael Stolt :
> 
>> 
>> Am 10.05.2017 um 21:48 schrieb Ævar Arnfjörð Bjarmason :
>> 
>> On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth
>>  wrote:
>>> On 2017-05-10 19:00, raphael.st...@gmail.com wrote:
>>> 
 Current configuration which finds the conditional configuration.
 
> 
> a)
> 
 ~/.gitconfig
 [includeIf "gitdir:~/Work/git-repos/oss/"]
  path = ~/Work/git-repos/oss/.oss-gitconfig
 
 Expected configuration which doesn't find the conditional configuration:
 
> 
> b)
> 
 ~/.gitconfig
 [includeIf "gitdir:~/Work/git-repos/oss/"]
  path = .oss-gitconfig
>>> 
>>> 
>>> My guess is, because includeIf might contain other conditionals than
>>> "gitdir", the generic convention is to always use an absolute path for
>>> "path".
>> 
>> [CC'd OP Raphael Stolt, please reply-all]
>> 
>> In both cases the conditional is the same, but the path is relative
>> v.s. absolute.
>> 
>> Raphael: Does the config get included if you cd to
>> ~/Work/git-repos/oss/? From git-config(1):
> 
> Given I’m in a repo in ~/Work/git-repos/oss/ e.g. 
> ~/Work/git-repos/oss/project-repo-a and I’m using config a) 
> the config is used from ~/Work/git-repos/oss/.oss-gitconfig
> 
> Given I’m in a repo in ~/Work/git-repos/oss/ e.g. 
> ~/Work/git-repos/oss/project-repo-a and I’m using config b) 
> the global config is used because there is no .oss-gitconfig in $HOME.

Given I’m in ~/Work/git-repos/oss and I’m using config b)
also the global config (~/.gitconfig) is used.

> 
> I guess it’s an intended behavior since conditional configuration files 
> __SHOULD__ reside in $HOME rather than 
> in my case in ~/Work/git-repos/oss.
> 
>> 
>> ---cut---
>> The included file is expanded immediately, as if its contents had been
>> found at the location of the include directive. If the value of the
>> `include.path` variable is a relative path, the path is considered to
>> be relative to the configuration file in which the include directive
>> was found.  See below for examples.
>> ---cut---
>> 
>> The commit that added IncludeIf (3efd0bedc6) does something with
>> relative path (just skimming, need to get to other things), but unlike
>> [Include] the docs don't explicitly mention what it's supposed to do
>> with that, and all the examples show absolute paths.
>> 
>> So whether this is a bug in the code or not it seems to definitely be
>> a doc bug, whatever it does with relative files should be in the docs.
> + 1

Re: Possible bug in includeIf / conditional includes

[Raphael Stolt]

> 
> Am 10.05.2017 um 21:48 schrieb Ævar Arnfjörð Bjarmason :
> 
> On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth
>  wrote:
>> On 2017-05-10 19:00, raphael.st...@gmail.com wrote:
>> 
>>> Current configuration which finds the conditional configuration.
>>> 

a)

>>> ~/.gitconfig
>>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>>   path = ~/Work/git-repos/oss/.oss-gitconfig
>>> 
>>> Expected configuration which doesn't find the conditional configuration:
>>> 

b)

>>> ~/.gitconfig
>>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>>   path = .oss-gitconfig
>> 
>> 
>> My guess is, because includeIf might contain other conditionals than
>> "gitdir", the generic convention is to always use an absolute path for
>> "path".
> 
> [CC'd OP Raphael Stolt, please reply-all]
> 
> In both cases the conditional is the same, but the path is relative
> v.s. absolute.
> 
> Raphael: Does the config get included if you cd to
> ~/Work/git-repos/oss/? From git-config(1):

Given I’m in a repo in ~/Work/git-repos/oss/ e.g. 
~/Work/git-repos/oss/project-repo-a and I’m using config a) 
the config is used from ~/Work/git-repos/oss/.oss-gitconfig

Given I’m in a repo in ~/Work/git-repos/oss/ e.g. 
~/Work/git-repos/oss/project-repo-a and I’m using config b) 
the global config is used because there is no .oss-gitconfig in $HOME.

I guess it’s an intended behavior since conditional configuration files 
__SHOULD__ reside in $HOME rather than 
in my case in ~/Work/git-repos/oss.

> 
> ---cut---
> The included file is expanded immediately, as if its contents had been
> found at the location of the include directive. If the value of the
> `include.path` variable is a relative path, the path is considered to
> be relative to the configuration file in which the include directive
> was found.  See below for examples.
> ---cut---
> 
> The commit that added IncludeIf (3efd0bedc6) does something with
> relative path (just skimming, need to get to other things), but unlike
> [Include] the docs don't explicitly mention what it's supposed to do
> with that, and all the examples show absolute paths.
> 
> So whether this is a bug in the code or not it seems to definitely be
> a doc bug, whatever it does with relative files should be in the docs.
+ 1

git worktrees must exist even if locked

[taylor, david]

The Git documentation in describing worktrees says that one reason
why you might want to lock a worktree is to prevent it from being pruned
if it is on a removable media that isn't currently mounted.

So, my expectation was that if the worktree is inaccessible (and locked), Git
would pretend that there is no worktree by that name.

In reality, if you have such a worktree, Git gets an error.

 On local systems, /home is local to a machine; home directories are elsewhere.
Home directories are NFS mounted; /home is not.

. create a repository in /my/home/dir/my-repo.git with

git clone --bare 

. create an empty directory /home/somedir/worktree-tests

. use 'git worktree add' to add /home/somedir/worktree-tests/
  as a worktree on branch .  It gets populated with the correct
  content.

. lock it using'git worktree lock'

So far, so good.  Now, go to a different computer -- one on which
/home/somedir/worktree-tests does not exist (and therefore
/home/somedir/worktree-tests/ does not exist).

. cd /my/home/dir/my-repo.git

Now, try issuing Git commands.  Many will fail.

  git fetch ==> fails:

  fatal: Invalid path '/home/somedir/worktree-tests': No such file or directory

  git status ==> fails -- same error as above
  git help worktree ==> fails -- same error as above

Re: [PATCH v3 1/1] t0027: tests are not expensive; remove t0025

[Torstem Bögershausen]

Ok for me, thanks for helping out.



> Am 10.05.2017 um 17:52 schrieb Johannes Schindelin 
> :
> 
> Hi,
> 
>> On Wed, 10 May 2017, tbo...@web.de wrote:
>> 
>> From: Torsten Bögershausen 
>> 
>> The purpose of t0027 is to test all CRLF related conversions at "git 
>> checkout"
>> and "git add".
>> 
>> Running t0027 under Git for Windows takes 3-4 minutes, so the whole script 
>> had
>> been marked as "EXPENSIVE".
>> 
>> The source code for "Git for Windows" overrides this since 2014:
>> "t0027 is marked expensive, but really, for MinGW we want to run these
>> tests always."
>> 
>> Recent "stress" tests show that t0025 if flaky, reported by Lars Schneider,
>> larsxschnei...@gmail.com
>> 
>> All tests in t0025 are covered by t0027 already, so that t0025 can be 
>> retired.
>> t0027 takes less than 14 seconds under Linux, and 63 seconds under Mac Os X,
>> and this is more or less the same with a SSD or a spinning disk.
>> 
>> Acked-by: Johannes Schindelin 
>> Signed-off-by: Torsten Bögershausen 
> 
> This is still formatted very awkwardly. How about this instead (I fixed
> the formatting, reworded a little here and there, and fixed the order of
> the footers)?
> 
> -- snipsnap --
> From: Torsten Bögershausen 
> 
> The purpose of t0027 is to test all CRLF related conversions at "git
> checkout" and "git add".  Running t0027 under Git for Windows takes 3-4
> minutes, so the whole script had been marked as "EXPENSIVE".
> 
> However, the "Git for Windows" fork overrides this since 2014: "t0027
> is marked expensive, but really, for MinGW we want to run these tests
> always."
> 
> The test seems not to be expensive on other platforms at all: it takes
> less than 14 seconds under Linux, and 63 seconds under Mac Os X, and
> this is more or less the same with a SSD or a spinning disk.
> 
> So let's drop the "EXPENSIVE" prereq.
> 
> While at it, retire t0025: Recent "stress" tests show that t0025 if
> flaky, reported by Lars Schneider , but all
> tests in t0025 are covered by t0027 already.
> 
> Signed-off-by: Torsten Bögershausen 
> Acked-by: Johannes Schindelin 

Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity

[Stefan Beller]

On Wed, May 10, 2017 at 12:48 PM, Johannes Schindelin
 wrote:
> Hi Stefan,
>
> On Fri, 5 May 2017, Johannes Schindelin wrote:
>
>> On Fri, 28 Apr 2017, Johannes Schindelin wrote:
>>
>> > On Fri, 28 Apr 2017, Stefan Beller wrote:
>> >
>> > > On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin
>> > >  wrote:
>> > >
>> > > > I still have to find the time to figure out one more detail: how
>> > > > to download and extract the Coverity tool (the .zip archive has a
>> > > > variable name for the top-level directory), and doing that only
>> > > > every once in a while, say, only when there is no previously
>> > > > unpacked tool, or it is already 4 weeks old.
>> > >
>> > > That is an interesting problem, which I ignored as the older
>> > > versions of their tools still works once they release new versions.
>> > > So I just manually check every once in a while if they have new
>> > > versions out there.
>> > >
>> > > So if you find a nice solution to that problem, let me know, please.
>> >
>> > I think I have a working idea (jotting it down in the editor,
>> > untested):
>> >
>> > [... totally untested snippet ...]
>>
>> And now I edited it and tested it. The code is now part of the script I
>> use for pretty much all administrative (i.e. recurring and boring) tasks
>> in the Git for Windows project:
>>
>>   https://github.com/git-for-windows/build-extra/commit/05b5342128
>
> Oh, I completely forgot to mention that I tried to set the FLEX_ARRAY
> constant to something quite large (I used 64k), but apparently that does
> not work as expected, Coverity still insists on complaining about strbufs.
>
> On a second thought, it is actually quite obvious why it does not fix
> those reports: STRBUF_INIT has nothing to do with FLEX_ARRAY. D'oh.

D'oh. I must have been living in an alternate universe for quite some time
as I seemed to have confused different things here.

Checkout this commit,
https://github.com/stefanbeller/git/commit/977f81d6dec4461a1a12da2df6c5c919b41129b4
that is cherry-picked for the coverity build. That fixes it.

That is about the only patch I apply before sending it off to coverity.
I am still contemplating a nice solution for FLEX_ARRAYs in other cases.

Thanks,
Stefan

Re: Automating Coverity, was Re: [PATCH 00/26] Address a couple of issues identified by Coverity

[Johannes Schindelin]

Hi Stefan,

On Fri, 5 May 2017, Johannes Schindelin wrote:

> On Fri, 28 Apr 2017, Johannes Schindelin wrote:
> 
> > On Fri, 28 Apr 2017, Stefan Beller wrote:
> > 
> > > On Thu, Apr 27, 2017 at 3:50 PM, Johannes Schindelin
> > >  wrote:
> > > 
> > > > I still have to find the time to figure out one more detail: how
> > > > to download and extract the Coverity tool (the .zip archive has a
> > > > variable name for the top-level directory), and doing that only
> > > > every once in a while, say, only when there is no previously
> > > > unpacked tool, or it is already 4 weeks old.
> > > 
> > > That is an interesting problem, which I ignored as the older
> > > versions of their tools still works once they release new versions.
> > > So I just manually check every once in a while if they have new
> > > versions out there.
> > > 
> > > So if you find a nice solution to that problem, let me know, please.
> > 
> > I think I have a working idea (jotting it down in the editor,
> > untested):
> > 
> > [... totally untested snippet ...]
> 
> And now I edited it and tested it. The code is now part of the script I
> use for pretty much all administrative (i.e. recurring and boring) tasks
> in the Git for Windows project:
> 
>   https://github.com/git-for-windows/build-extra/commit/05b5342128

Oh, I completely forgot to mention that I tried to set the FLEX_ARRAY
constant to something quite large (I used 64k), but apparently that does
not work as expected, Coverity still insists on complaining about strbufs.

On a second thought, it is actually quite obvious why it does not fix
those reports: STRBUF_INIT has nothing to do with FLEX_ARRAY. D'oh.

My next attempt to work around these bogus claims was to modify the "model
file" by adding a line saying "char strbuf_slopbuf[64];", but that was
sadly not picked up by Coverity either.

My current thinking is that I will simply patch strbuf.c via `sed
's/^\(char struct_slopbuf\[\)1\[/&64[/'`.

Ciao,
Dscho

Re: Possible bug in includeIf / conditional includes

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 8:58 PM, Sebastian Schuberth
 wrote:
> On 2017-05-10 19:00, raphael.st...@gmail.com wrote:
>
>> Current configuration which finds the conditional configuration.
>>
>> ~/.gitconfig
>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>path = ~/Work/git-repos/oss/.oss-gitconfig
>>
>> Expected configuration which doesn't find the conditional configuration:
>>
>> ~/.gitconfig
>> [includeIf "gitdir:~/Work/git-repos/oss/"]
>>path = .oss-gitconfig
>
>
> My guess is, because includeIf might contain other conditionals than
> "gitdir", the generic convention is to always use an absolute path for
> "path".

[CC'd OP Raphael Stolt, please reply-all]

In both cases the conditional is the same, but the path is relative
v.s. absolute.

Raphael: Does the config get included if you cd to
~/Work/git-repos/oss/? From git-config(1):

---cut---
The included file is expanded immediately, as if its contents had been
found at the location of the include directive. If the value of the
`include.path` variable is a relative path, the path is considered to
be relative to the configuration file in which the include directive
was found.  See below for examples.
---cut---

The commit that added IncludeIf (3efd0bedc6) does something with
relative path (just skimming, need to get to other things), but unlike
[Include] the docs don't explicitly mention what it's supposed to do
with that, and all the examples show absolute paths.

So whether this is a bug in the code or not it seems to definitely be
a doc bug, whatever it does with relative files should be in the docs.

Re: git log --follow after subtree merge

[Samuel Lijin]

On Wed, May 10, 2017 at 9:46 AM, Jonny Gilchrist
 wrote:
> Hi,
>
> After doing a subtree merge, using 'git log' and 'git log --follow' on
> files in the subtree show only the merge commit in which they were
> added.
>
> After reading around I understand that the issue is that git log
> --follow doesn't track renames that occur during a merge.

Try git log --follow -M. (You may also want to combine this with -l and/or -C).

> Has there been any work (or are there any plans) to allow git log
> --follow to work in this case? I couldn't find anything in the mailing
> list archives aside from a couple of threads from 2011 explaining the
> issue.
>
> Thanks,
> J.

Re: Possible bug in includeIf / conditional includes

[Sebastian Schuberth]

On 2017-05-10 19:00, raphael.st...@gmail.com wrote:


Current configuration which finds the conditional configuration.

~/.gitconfig
[includeIf "gitdir:~/Work/git-repos/oss/"]
   path = ~/Work/git-repos/oss/.oss-gitconfig

Expected configuration which doesn't find the conditional configuration:

~/.gitconfig
[includeIf "gitdir:~/Work/git-repos/oss/"]
   path = .oss-gitconfig


My guess is, because includeIf might contain other conditionals than 
"gitdir", the generic convention is to always use an absolute path for 
"path".


--
Sebastian Schuberth

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Sebastian Schuberth]

On 2017-05-10 19:00, Jonathan Nieder wrote:


Right, makes sense.  I wondered if GitHub should be turning on
allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide
some internal refs[1], and they're things that users wouldn't want to
fetch. The problem for your case really is just on the client side, and
this patch fixes it.

[...]

[1] The reachability checks from upload-pack don't actually do much on
 GitHub, because you can generally access the objects via the API or
 the web site anyway. So I'm not really opposed to turning on
 allowTipSHA1InWant if it would be useful for users, but after
 Jonathan's patch I don't see how it would be.


Given that, what would make me really happy is if github enables
uploadpack.allowAnySHA1InWant.  That would be useful for me, at least.


Me too, BTW. If you're only interested in building / analyzing a 
specific SHA1 it's really a waste of network resources to fetch all of 
the history.


--
Sebastian Schuberth

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Nieder]

On Wed, May 10, 2017 at 12:48:37PM -0600, Martin Fick wrote:
>> Ævar Arnfjörð Bjarmason wrote:

>>> Just a side question, what are the people who use this
>>> feature using it for? The only thing I can think of
>>> myself is some out of band ref advertisement because
>>> you've got squillions of refs as a hack around git's
>>> limitations in that area.
[...]
> Perhaps another use case is submodules and repo(android
> tool) subprojects since they can be "pinned" to sha1s,

Yes, thanks for mentioning that.  We've talked a little about making
'git fetch --recurse-submodules' use this feature, either always, or
if after the usual fetch the desired commit is not available in the
submodule.

Jonathan

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Martin Fick]

On Wednesday, May 10, 2017 11:20:49 AM Jonathan Nieder 
wrote:
> Hi,
> 
> Ævar Arnfjörð Bjarmason wrote:
> > Just a side question, what are the people who use this
> > feature using it for? The only thing I can think of
> > myself is some out of band ref advertisement because
> > you've got squillions of refs as a hack around git's
> > limitations in that area.
> 
> That's one use case.
> 
> Another is when you really care about the exact sha1 (for
> example because you are an automated build system and
> this is the specific sha1 you have already decided you
> want to build).
> > Are there other use-cases for this? All the commits[1]
> > that touched this feature just explain what, not why.
> 
> Similar to the build system case I described above is when
> a human has a sha1 (from a mailing list, or source
> browser, or whatever) and wants to fetch just that
> revision, with --depth=1.  You could use "git archive
> --remote", but (1) github doesn't support that and (2)
> that doesn't give you all the usual git-ish goodness.


Perhaps another use case is submodules and repo(android 
tool) subprojects since they can be "pinned" to sha1s,

-Martin
-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> Just a side question, what are the people who use this feature using
> it for? The only thing I can think of myself is some out of band ref
> advertisement because you've got squillions of refs as a hack around
> git's limitations in that area.

That's one use case.

Another is when you really care about the exact sha1 (for example
because you are an automated build system and this is the specific
sha1 you have already decided you want to build).

> Are there other use-cases for this? All the commits[1] that touched
> this feature just explain what, not why.

Similar to the build system case I described above is when a human has
a sha1 (from a mailing list, or source browser, or whatever) and wants
to fetch just that revision, with --depth=1.  You could use "git
archive --remote", but (1) github doesn't support that and (2) that
doesn't give you all the usual git-ish goodness.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Nieder]

Hi,

Jonathan Tan wrote:

> fetch-pack, when fetching a literal SHA-1 from a server that is not
> configured with uploadpack.allowtipsha1inwant (or similar), always
> returns an error message of the form "Server does not allow request for
> unadvertised object %s". However, it is sometimes the case that such
> object is advertised.

Thanks for fixing it.

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -592,6 +592,15 @@ static void mark_recent_complete_commits(struct 
> fetch_pack_args *args,
>   }
>  }
>  
> +static int is_literal_sha1(const struct ref *ref)
> +{
> + struct object_id oid;
> + const char *end;
> + return !parse_oid_hex(ref->name, , ) &&
> +!*end &&
> +!oidcmp(, >old_oid);

When would a ref have its name be a oid and its value be a different
oid?

Ah, this check comes from

commit b7916422c741b50925d454819b1977449fccc111
Author: Jeff King 
Date:   Thu Mar 19 16:34:51 2015 -0400

filter_ref: avoid overwriting ref->old_sha1 with garbage

which explains that the answer is "never" (leaving me even more
confused).  Anyway, that's orthogonal to this patch.

> +}
> +
>  static void filter_refs(struct fetch_pack_args *args,
>   struct ref **refs,
>   struct ref **sought, int nr_sought)
> @@ -601,7 +610,6 @@ static void filter_refs(struct fetch_pack_args *args,
>   struct ref *ref, *next;
>   int i;
>  
> - i = 0;
>   for (ref = *refs; ref; ref = next) {
>   int keep = 0;
>   next = ref->next;
> @@ -610,15 +618,14 @@ static void filter_refs(struct fetch_pack_args *args,
>   check_refname_format(ref->name, 0))
>   ; /* trash */
>   else {
> - while (i < nr_sought) {
> - int cmp = strcmp(ref->name, sought[i]->name);
> - if (cmp < 0)
> - break; /* definitely do not have it */
> - else if (cmp == 0) {
> - keep = 1; /* definitely have it */
> - sought[i]->match_status = REF_MATCHED;
> + for (i = 0; i < nr_sought; i++) {

This resets i each time this code path is encountered.  Doesn't that
make this more expensive e.g. in the case where there are no sha1s
among the refs to be searched for?

Should this be conditional on whether there are any sha1-shaped refs in
'sought' to avoid that performance regression?

It's tempting to split 'sought' into two lists --- sha1-shaped refs
that have to be processed in full every time and the others that can
be processed in a single pass because they are in sorted order.

[...]
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
>   git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
>  '
>  
> +test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a 
> ref' '
> + git init server &&
> + test_commit -C server 4 &&
> + git fetch-pack server $(git -C server rev-parse refs/heads/master)
> +'

Is there test for the error case, too? (I.e. the server does not advertise
fetch-by-sha1 and the sha1 is not among the advertised refs)

Thanks,
Jonathan

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 6:46 AM, Mike Hommey  wrote:
> On Wed, May 10, 2017 at 12:33:44AM -0400, Jeff King wrote:
>> On Tue, May 09, 2017 at 09:22:11PM -0700, Shawn Pearce wrote:
>>
>> > > Hmm. That makes sense generally, as the request should succeed. But it
>> > > seems like we're creating a client that will sometimes succeed and
>> > > sometimes fail, and the reasoning will be somewhat opaque to the user.
>> > > I have a feeling I'm missing some context on when you'd expect this to
>> > > kick in.
>> >
>> > Specifically, someone I know was looking at building an application
>> > that is passed only a SHA-1 for the tip of a ref on a popular hosting
>> > site[1]. They wanted to run `git fetch URL SHA1`, but this failed
>> > because the site doesn't have upload.allowtipsha1inwant enabled.
>> > However the SHA1 was clearly in the output of git ls-remote.
>>
>> OK. So this is basically a case where we expect that the user knows what
>> they're doing.
>>
>> > For various reasons they expected this to work, because it works
>> > against other sites that do have upload.allowtipsha1inwant enabled.
>> > And I personally just expected it to work because the fetch client
>> > accepts SHA-1s, and the wire protocol uses "want SHA1" not "want ref",
>> > and the SHA-1 passed on the command line was currently in the
>> > advertisement when the connection opened, so its certainly something
>> > the server is currently willing to serve.
>>
>> Right, makes sense.  I wondered if GitHub should be turning on
>> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide
>> some internal refs[1], and they're things that users wouldn't want to
>> fetch. The problem for your case really is just on the client side, and
>> this patch fixes it.
>
> More broadly, I think it is desirable that any commit that can be
> reached from public refs can be fetched by an explicit sha1 without
> allowTipSHA1InWant.

Just a side question, what are the people who use this feature using
it for? The only thing I can think of myself is some out of band ref
advertisement because you've got squillions of refs as a hack around
git's limitations in that area.

Are there other use-cases for this? All the commits[1] that touched
this feature just explain what, not why.

1. git log --reverse -p -i -Gallowtipsha1inwant

Re: [noob] is this normal behavior

[Harry Putnam]

Igor Djordjevic  writes:

[...] snipped detailed reply
 [To Anyone who lands here on a search: Please read Igor D's full
 answer in this thread... ]

> Just pay attention that untracked files are not affected, you still 
> need to add them first to tell Git to start tracking them, including 
> them in the next commit, but that seems to align nicely with your 
> expectations already.
>
> I personally find the staging area to be one of the greatest Git 
> possibilities, but I do understand beginners getting confused by it, 
> as admittedly I once was myself.

Thank you sir for such a full and complete answer.  Adds to by
understanding in a big way.

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Brandon Williams]

On 05/10, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > Convert 'parse_pathspec()' to take an index parameter.
> >
> > Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH
> > and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check
> > requiring a non-NULL index when either of these flags are given.
> > Convert callers which use these two flags to pass in an index while
> > having other callers pass in NULL.
> >
> > Now that pathspec.c does not use any cache macros and has no references
> > to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS.
> >
> > Signed-off-by: Brandon Williams 
> 
> The same comment as 5/8 applies to this change, but it is a bit
> easier to judge, because it has so many callers, and for some
> builtins, especially manipulator commands like add, checkout, and
> commit, there may be a good reason why they want to keep the primary
> index while playing with an additional in-core index in a distant
> future.
> 
> Does a pathspec parsed using one instance of index_state expected to
> work when matching against a path in another instance of index_state?
> Otherwise, passing a non-NULL istate to parse_pathspec() would tie
> the resulting pathspec to a particular index_state in some way and
> there may need a mechanism to catch an attempt to match paths in
> another index_state with such a pathspec as an error.  Just
> speculating out loud...
> 

Currently I don't believe this links a pathspec with a particular
index_state since an index is really just used to do some pre-processing
on the paths (check if the path goes into a submodule and die, or strip
a slash if the path matches a submodule), though I can see a future where
this is true.

I did have another version of this series where I completely removed the
two flags related to submodules.  Since builtin/add is the only caller
which needs to die if a path descends into a submodule, I had a function
which did this after the parse_pathspec() call.  Also, since (ae8d08242
pathspec: pass directory indicator to match_pathspec_item()) stripping
off the slash from a submodule path really is no longer needed for the
path matching logic, this means that we could potentially just drop the
strip slash flag.  The only caveat is ls-files.

ls-files is the only command (that I know of) which does cache pruning
based on the common prefix of all pathspecs given.  If there is a
submodule named 'sub' and you provided a pathspec 'sub/', the matching
logic can handle this but the cache pruning logic would prune all
entries from the index which don't have a leading 'sub/' which is the
common prefix of all pathspecs (if you didn't strip off the slash).
Meaning you'd prune the submodule 'sub' when  you really didn't want to.
This could probably be fixed to have the cache pruning logic to prune by
ignoring the trailing slash always.

So there's another option here if you don't feel quite right about
piping through an index into parse_pathspec just yet.

-- 
Brandon Williams

Re: [PATCH] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Nieder]

Hi,

Jeff King wrote:

> Right, makes sense.  I wondered if GitHub should be turning on
> allowTipSHA1InWant, but it really doesn't make sense to. We _do_ hide
> some internal refs[1], and they're things that users wouldn't want to
> fetch. The problem for your case really is just on the client side, and
> this patch fixes it.
[...]
> [1] The reachability checks from upload-pack don't actually do much on
> GitHub, because you can generally access the objects via the API or
> the web site anyway. So I'm not really opposed to turning on
> allowTipSHA1InWant if it would be useful for users, but after
> Jonathan's patch I don't see how it would be.

Given that, what would make me really happy is if github enables
uploadpack.allowAnySHA1InWant.  That would be useful for me, at least.

Thanks,
Jonathan

Possible bug in includeIf / conditional includes

[raphael . stolt]

Hi there,

I might have stumbled over a bug in includeIf / conditional includes or maybe 
it's just as intended.

Current configuration which finds the conditional configuration.

~/.gitconfig
[includeIf "gitdir:~/Work/git-repos/oss/"]
  path = ~/Work/git-repos/oss/.oss-gitconfig

Expected configuration which doesn't find the conditional configuration:

~/.gitconfig
[includeIf "gitdir:~/Work/git-repos/oss/"]
  path = .oss-gitconfig

Best regards,

Raphael Stolt

[PATCH v2] fetch-pack: always allow fetching of literal SHA1s

[Jonathan Tan]

fetch-pack, when fetching a literal SHA-1 from a server that is not
configured with uploadpack.allowtipsha1inwant (or similar), always
returns an error message of the form "Server does not allow request for
unadvertised object %s". However, it is sometimes the case that such
object is advertised. This situation would occur, for example, if a user
or a script was provided a SHA-1 instead of a branch or tag name for
fetching, and wanted to invoke "git fetch" or "git fetch-pack" using
that SHA-1.

Teach fetch-pack to also check the SHA-1s of the refs in the received
ref advertisement if a literal SHA-1 was given by the user.

Signed-off-by: Jonathan Tan 
---

Thanks, peff, for your suggestions. Changes from v1:
 - updated commit message to explain a bit of the context
 - use parse_oid_hex instead of get_oid_hex to avoid the constant 40

peff also suggested a possible optimization in computing is_literal_sha1
outside the loop. After some thought, I don't think that it takes that
much time in the general case where the ref name starts with "refs/",
because parse_oid_hex will see the "r" (which is not in "[0-9a-f]") and
immediately return. So I left it as-is.

 fetch-pack.c  | 31 +--
 t/t5500-fetch-pack.sh |  6 ++
 2 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index afb8b0502..d4b57b9ba 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -592,6 +592,15 @@ static void mark_recent_complete_commits(struct 
fetch_pack_args *args,
}
 }
 
+static int is_literal_sha1(const struct ref *ref)
+{
+   struct object_id oid;
+   const char *end;
+   return !parse_oid_hex(ref->name, , ) &&
+  !*end &&
+  !oidcmp(, >old_oid);
+}
+
 static void filter_refs(struct fetch_pack_args *args,
struct ref **refs,
struct ref **sought, int nr_sought)
@@ -601,7 +610,6 @@ static void filter_refs(struct fetch_pack_args *args,
struct ref *ref, *next;
int i;
 
-   i = 0;
for (ref = *refs; ref; ref = next) {
int keep = 0;
next = ref->next;
@@ -610,15 +618,14 @@ static void filter_refs(struct fetch_pack_args *args,
check_refname_format(ref->name, 0))
; /* trash */
else {
-   while (i < nr_sought) {
-   int cmp = strcmp(ref->name, sought[i]->name);
-   if (cmp < 0)
-   break; /* definitely do not have it */
-   else if (cmp == 0) {
-   keep = 1; /* definitely have it */
-   sought[i]->match_status = REF_MATCHED;
+   for (i = 0; i < nr_sought; i++) {
+   struct ref *s = sought[i];
+   if (!strcmp(ref->name, s->name) ||
+   (is_literal_sha1(s) &&
+!oidcmp(>old_oid, >old_oid))) {
+   keep = 1;
+   s->match_status = REF_MATCHED;
}
-   i++;
}
}
 
@@ -637,14 +644,10 @@ static void filter_refs(struct fetch_pack_args *args,
 
/* Append unmatched requests to the list */
for (i = 0; i < nr_sought; i++) {
-   unsigned char sha1[20];
-
ref = sought[i];
if (ref->match_status != REF_NOT_MATCHED)
continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
+   if (!is_literal_sha1(ref))
continue;
 
if ((allow_unadvertised_object_request &
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index b5865b385..d87983bc2 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -547,6 +547,12 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
 '
 
+test_expect_success 'fetch-pack can fetch a raw sha1 that is advertised as a 
ref' '
+   git init server &&
+   test_commit -C server 4 &&
+   git fetch-pack server $(git -C server rev-parse refs/heads/master)
+'
+
 check_prot_path () {
cat >expected <<-EOF &&
Diag: url=$1
-- 
2.13.0.rc2.291.g57267f2277-goog

Good day

[FDF]

--
Did you receive my previous message about my donation to you?

Re: [PATCH 3/8] pathspec: change PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag

[Brandon Williams]

On 05/10, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > It's confusing to have two different 'strip submodule slash' flags which
> > do subtly different things.  Both
> > PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and
> > PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP will accomplish the same task of
> > striping a slash from a pathspec which matches a submodule entry in the
> > index.  The only difference is that
> > PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE will perform additional checks
> > and die if a pathspec has a leading path component which corresponds to
> > a submodule.  This additional functionality should be split out into its
> > own flag.
> >
> > To this end, rename the PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE flag to
> > PATHSPEC_SUBMODULE_LEADING_PATH and change its behavior to only die if a
> > path descends into a submodule.  In addition add the
> > PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP flag to callers which relied on the
> > old slash stripping functionality.
> 
> "PATHSPEC_SUBMODULE_LEADING_PATH" feels like an unfinished sentence
> to me.  Do I understand your description correctly if I say it is
> about "checking" the leading path to see if it overlaps with a
> submodule path?  IOW, I am wondering if the name should have the
> word CHECK somewhere in it.
> 

You're probably right, the name has something left to be desired.  I
chose it simply because it conforms to another flag
"PATHSPEC_SYMLINK_LEADING_PATH" which dies if there is a symlink in the
leading path.

And you are correct, it checks if the leading path overlaps with a
submodule path.  If it strictly matches a submodule path that's alright
though.  The point of this flag is to disallow paths which descend into
submodules.  One such use case is to prevent a user from trying to 'git
add' a file from a submodule.

-- 
Brandon Williams

t6134 test failure in 'pu'

[Ramsay Jones]

Hi Junio,

t6134 fails for me in 'pu', but I think it is just a question
of merge 641f3ad90a3 dropping the last line of the test from
the change introduced by commit bdab972153.

In other words, this fixes the test for me:

$ git diff
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
index 6d5df294a..c67066840 100755
--- a/t/t6134-pathspec-in-submodule.sh
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -26,6 +26,7 @@ test_expect_success 'error message for path inside submodule' 
'
 
 test_expect_success 'error message for path inside submodule from within 
submodule' '
test_must_fail git -C sub add . 2>actual &&
+   test_i18ngrep "in unpopulated submodule" actual
 '
 
 test_done
$ 

Thanks!

ATB,
Ramsay Jones

Re: [PATCH v3 1/1] t0027: tests are not expensive; remove t0025

[Johannes Schindelin]

Hi,

On Wed, 10 May 2017, tbo...@web.de wrote:

> From: Torsten Bögershausen 
> 
> The purpose of t0027 is to test all CRLF related conversions at "git checkout"
> and "git add".
> 
> Running t0027 under Git for Windows takes 3-4 minutes, so the whole script had
> been marked as "EXPENSIVE".
> 
> The source code for "Git for Windows" overrides this since 2014:
> "t0027 is marked expensive, but really, for MinGW we want to run these
> tests always."
> 
> Recent "stress" tests show that t0025 if flaky, reported by Lars Schneider,
> larsxschnei...@gmail.com
> 
> All tests in t0025 are covered by t0027 already, so that t0025 can be retired.
> t0027 takes less than 14 seconds under Linux, and 63 seconds under Mac Os X,
> and this is more or less the same with a SSD or a spinning disk.
> 
> Acked-by: Johannes Schindelin 
> Signed-off-by: Torsten Bögershausen 

This is still formatted very awkwardly. How about this instead (I fixed
the formatting, reworded a little here and there, and fixed the order of
the footers)?

-- snipsnap --
From: Torsten Bögershausen 

The purpose of t0027 is to test all CRLF related conversions at "git
checkout" and "git add".  Running t0027 under Git for Windows takes 3-4
minutes, so the whole script had been marked as "EXPENSIVE".

However, the "Git for Windows" fork overrides this since 2014: "t0027
is marked expensive, but really, for MinGW we want to run these tests
always."

The test seems not to be expensive on other platforms at all: it takes
less than 14 seconds under Linux, and 63 seconds under Mac Os X, and
this is more or less the same with a SSD or a spinning disk.

So let's drop the "EXPENSIVE" prereq.

While at it, retire t0025: Recent "stress" tests show that t0025 if
flaky, reported by Lars Schneider , but all
tests in t0025 are covered by t0027 already.

Signed-off-by: Torsten Bögershausen 
Acked-by: Johannes Schindelin 

git log --follow after subtree merge

[Jonny Gilchrist]

Hi,

After doing a subtree merge, using 'git log' and 'git log --follow' on
files in the subtree show only the merge commit in which they were
added.

After reading around I understand that the issue is that git log
--follow doesn't track renames that occur during a merge.

Has there been any work (or are there any plans) to allow git log
--follow to work in this case? I couldn't find anything in the mailing
list archives aside from a couple of threads from 2011 explaining the
issue.

Thanks,
J.

[PATCH v3 1/1] t0027: tests are not expensive; remove t0025

[tboegi]

From: Torsten Bögershausen 

The purpose of t0027 is to test all CRLF related conversions at "git checkout"
and "git add".

Running t0027 under Git for Windows takes 3-4 minutes, so the whole script had
been marked as "EXPENSIVE".

The source code for "Git for Windows" overrides this since 2014:
"t0027 is marked expensive, but really, for MinGW we want to run these
tests always."

Recent "stress" tests show that t0025 if flaky, reported by Lars Schneider,
larsxschnei...@gmail.com

All tests in t0025 are covered by t0027 already, so that t0025 can be retired.
t0027 takes less than 14 seconds under Linux, and 63 seconds under Mac Os X,
and this is more or less the same with a SSD or a spinning disk.

Acked-by: Johannes Schindelin 
Signed-off-by: Torsten Bögershausen 
---
 t/t0025-crlf-auto.sh | 181 ---
 t/t0027-auto-crlf.sh |   6 --
 2 files changed, 187 deletions(-)
 delete mode 100755 t/t0025-crlf-auto.sh

diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
deleted file mode 100755
index 89826c5..000
--- a/t/t0025-crlf-auto.sh
+++ /dev/null
@@ -1,181 +0,0 @@
-#!/bin/sh
-
-test_description='CRLF conversion'
-
-. ./test-lib.sh
-
-has_cr() {
-   tr '\015' Q <"$1" | grep Q >/dev/null
-}
-
-test_expect_success setup '
-
-   git config core.autocrlf false &&
-
-   for w in Hello world how are you; do echo $w; done >LFonly &&
-   for w in I am very very fine thank you; do echo ${w}Q; done | q_to_cr 
>CRLFonly &&
-   for w in Oh here is a QNUL byte how alarming; do echo ${w}; done | 
q_to_nul >LFwithNUL &&
-   git add . &&
-
-   git commit -m initial &&
-
-   LFonly=$(git rev-parse HEAD:LFonly) &&
-   CRLFonly=$(git rev-parse HEAD:CRLFonly) &&
-   LFwithNUL=$(git rev-parse HEAD:LFwithNUL) &&
-
-   echo happy.
-'
-
-test_expect_success 'default settings cause no changes' '
-
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   git read-tree --reset -u HEAD &&
-
-   ! has_cr LFonly &&
-   has_cr CRLFonly &&
-   LFonlydiff=$(git diff LFonly) &&
-   CRLFonlydiff=$(git diff CRLFonly) &&
-   LFwithNULdiff=$(git diff LFwithNUL) &&
-   test -z "$LFonlydiff" -a -z "$CRLFonlydiff" -a -z "$LFwithNULdiff"
-'
-
-test_expect_success 'crlf=true causes a CRLF file to be normalized' '
-
-   # Backwards compatibility check
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   echo "CRLFonly crlf" > .gitattributes &&
-   git read-tree --reset -u HEAD &&
-
-   # Note, "normalized" means that git will normalize it if added
-   has_cr CRLFonly &&
-   CRLFonlydiff=$(git diff CRLFonly) &&
-   test -n "$CRLFonlydiff"
-'
-
-test_expect_success 'text=true causes a CRLF file to be normalized' '
-
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   echo "CRLFonly text" > .gitattributes &&
-   git read-tree --reset -u HEAD &&
-
-   # Note, "normalized" means that git will normalize it if added
-   has_cr CRLFonly &&
-   CRLFonlydiff=$(git diff CRLFonly) &&
-   test -n "$CRLFonlydiff"
-'
-
-test_expect_success 'eol=crlf gives a normalized file CRLFs with 
autocrlf=false' '
-
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   git config core.autocrlf false &&
-   echo "LFonly eol=crlf" > .gitattributes &&
-   git read-tree --reset -u HEAD &&
-
-   has_cr LFonly &&
-   LFonlydiff=$(git diff LFonly) &&
-   test -z "$LFonlydiff"
-'
-
-test_expect_success 'eol=crlf gives a normalized file CRLFs with 
autocrlf=input' '
-
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   git config core.autocrlf input &&
-   echo "LFonly eol=crlf" > .gitattributes &&
-   git read-tree --reset -u HEAD &&
-
-   has_cr LFonly &&
-   LFonlydiff=$(git diff LFonly) &&
-   test -z "$LFonlydiff"
-'
-
-test_expect_success 'eol=lf gives a normalized file LFs with autocrlf=true' '
-
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   git config core.autocrlf true &&
-   echo "LFonly eol=lf" > .gitattributes &&
-   git read-tree --reset -u HEAD &&
-
-   ! has_cr LFonly &&
-   LFonlydiff=$(git diff LFonly) &&
-   test -z "$LFonlydiff"
-'
-
-test_expect_success 'autocrlf=true does not normalize CRLF files' '
-
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   git config core.autocrlf true &&
-   git read-tree --reset -u HEAD &&
-
-   has_cr LFonly &&
-   has_cr CRLFonly &&
-   LFonlydiff=$(git diff LFonly) &&
-   CRLFonlydiff=$(git diff CRLFonly) &&
-   LFwithNULdiff=$(git diff LFwithNUL) &&
-   test -z "$LFonlydiff" -a -z "$CRLFonlydiff" -a -z "$LFwithNULdiff"
-'
-
-test_expect_success 'text=auto, autocrlf=true does not normalize CRLF files' '
-
-   rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   git config core.autocrlf true 

bug with git add and .gitignore

[Orgad Shaneh]

Hi,

When a not-ignored file inside an ignore directory is added along with
other files, a false alarm is shown:

git init
echo /d/ > .gitignore
mkdir d
touch d/file foo
git add -f d/file foo
git add d/file
# fine
git add d/file foo
# The following paths are ignored by one of your .gitignore files:
# d
# Use -f if you really want to add them.

I did not try to add a new file in d. It's the same file that is
already indexed.

- Orgad

Re: [Query] Override sendemail.tocmd

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 12:43 PM, Viresh Kumar  wrote:
> On 10-05-17, 16:04, Viresh Kumar wrote:
>> On 10-05-17, 12:18, Ćvar Arnfjörđ Bjarmason wrote:
>> > On Wed, May 10, 2017 at 9:23 AM, Viresh Kumar  
>> > wrote:
>> > > Hi,
>> > >
>> > > I have a use case and wanted help from you guys..
>> > >
>> > > I have set the sendemail.tocmd option in my kernel's .git/config
>> > > file and that works very well. But sometimes I want to avoid that
>> > > (for example sending the patch to someone without the list and
>> > > maintainers).
>> > >
>> > > The suppress-cc=all option works quite well to avoid almost
>> > > everything (including running --cc-cmd), but it doesn't suppress
>> > > --to-cmd.
>> > >
>> > > Are there any specific reasons why it is being done that way?
>> > >
>> > > Currently I need to go edit my .git/config file and remove tocmd
>> > > before sending any such patches and it sucks :)
>> >
>> > Does something like this work:
>> >
>> > git -c sendemail.tocmd=/bin/false send-email ...
>> >
>> > ?
>>
>> That failed. I tried with 2.12 version first and then on the next branch with
>> version: 2.13.0.303.g4ebf3021692d
>>
>> $ git -c sendemail.tocmd=/bin/false send-email --to vingu 0001-abc.patch
>> 0001-abc.patch
>> (mbox) Adding cc: Viresh Kumar  from line 'From: 
>> Viresh Kumar '
>> (body) Adding cc: Viresh Kumar  from line 
>> 'Signed-off-by: Viresh Kumar '
>> (to-cmd) failed to close pipe to '/bin/false' at 
>> /usr/libexec/git-core/git-send-email line 1687.
>
> And even if this worked, I believe that the right solution would be to fix
> suppress-cc to include tocmd also. That's the whole purpose of suppress-cc
> option.

Indeed, it looks like either there should be a new option or the
existing option amended to work with tocmd as you suggest. I was just
wondering if there was a way to do this now, even if with some
non-obvious hack.

Re: [PATCH v2] archive-tar: fix a sparse 'constant too large' warning

[Johannes Schindelin]

Hi Junio,

On Wed, 10 May 2017, Junio C Hamano wrote:

> Ramsay Jones  writes:
> 
> > Yeah, I had a similar comment in the commit message (but much more
> > verbose than your concise addition above), but I edited it several
> > times, without finding a wording that I liked. I eventually removed
> > it, because it didn't really add any value. :(
> 
> I tend to agree that the proposed additional comment does not add much
> value.  It assures the readers that we (at the time of applying this
> patch) know that the earlier use of ULL was not done with a good reason
> but was merely an accident, and strengthens the claim that this is a
> good change, but the correctness of the change is already obvious, and
> the readers would understand without being explained where the
> incorrectness we have to fix with this patch came from, I would think.

Future me would find that comment in the commit message very clarifying,
though: why was that code there? Ah, that's why.

Now I have to dig through the mailing list to find out.

Ciao,
Dscho

Re: [Query] Override sendemail.tocmd

[Viresh Kumar]

On 10-05-17, 16:04, Viresh Kumar wrote:
> On 10-05-17, 12:18, Ævar Arnfjörð Bjarmason wrote:
> > On Wed, May 10, 2017 at 9:23 AM, Viresh Kumar  
> > wrote:
> > > Hi,
> > >
> > > I have a use case and wanted help from you guys..
> > >
> > > I have set the sendemail.tocmd option in my kernel's .git/config
> > > file and that works very well. But sometimes I want to avoid that
> > > (for example sending the patch to someone without the list and
> > > maintainers).
> > >
> > > The suppress-cc=all option works quite well to avoid almost
> > > everything (including running --cc-cmd), but it doesn't suppress
> > > --to-cmd.
> > >
> > > Are there any specific reasons why it is being done that way?
> > >
> > > Currently I need to go edit my .git/config file and remove tocmd
> > > before sending any such patches and it sucks :)
> > 
> > Does something like this work:
> > 
> > git -c sendemail.tocmd=/bin/false send-email ...
> > 
> > ?
> 
> That failed. I tried with 2.12 version first and then on the next branch with
> version: 2.13.0.303.g4ebf3021692d
> 
> $ git -c sendemail.tocmd=/bin/false send-email --to vingu 0001-abc.patch
> 0001-abc.patch
> (mbox) Adding cc: Viresh Kumar  from line 'From: 
> Viresh Kumar '
> (body) Adding cc: Viresh Kumar  from line 
> 'Signed-off-by: Viresh Kumar '
> (to-cmd) failed to close pipe to '/bin/false' at 
> /usr/libexec/git-core/git-send-email line 1687.

And even if this worked, I believe that the right solution would be to fix
suppress-cc to include tocmd also. That's the whole purpose of suppress-cc
option.

-- 
viresh

Re: [Query] Override sendemail.tocmd

[Viresh Kumar]

On 10 May 2017 at 12:59, Viresh Kumar  wrote:
> On 10 May 2017 at 12:53, Viresh Kumar  wrote:
>> Hi,
>>
>> I have a use case and wanted help from you guys..
>>
>> I have set the sendemail.tocmd option in my kernel's .git/config
>> file and that works very well. But sometimes I want to avoid that
>> (for example sending the patch to someone without the list and
>> maintainers).
>>
>> The suppress-cc=all option works quite well to avoid almost
>> everything (including running --cc-cmd), but it doesn't suppress
>> --to-cmd.
>>
>> Are there any specific reasons why it is being done that way?
>>
>> Currently I need to go edit my .git/config file and remove tocmd
>> before sending any such patches and it sucks :)
>
> And it would be *really* useful if we can add the "review" (or whatever
> it is called) screen with suppress-cc command as well. Currently the mail
> goes away directly without giving us a chance to look at the final list of
> recipients the mail is going to :)

Fixed that with: sendemail.confirm = always

Snapping git

[Alan Pope]

Hello!

I work at Canonical as part of the engineering team around Ubuntu and
Snapcraft [1].

We’re working on snaps, a platform to enable ISVs to directly control
delivery of software updates to their users, and make their software
available on many Linux distributions to a wide audience via our
snappy store.

Snaps enable you to have the latest build of git available directly
from github master to adventurous users in minutes, and your stable
tagged releases to more conservative users automatically.

Ideally I'm looking for someone in the community who can spend a
little bit of time over the next week or so creating a snap package
out of the latest source from git master. We have documentation [2]
and a forum [3] where the snap developers and users hang out. We're
keen to find out where the rough edges are in our docs / process, and
improve them, so feedback (+ve/-ve) from this process is essential to
us.

Ultimately the outcome would be a single package in the snap store
'edge' channel which has the latest git master build that adventurous
developers on various Linux [4] distributions can install with a
simple 'snap install git --edge'.

Anyone fancy taking on this challenge? :)

[1] http://snapcraft.io/
[2] http://snapcraft.io/docs
[3] http://forum.snapcraft.io/
[4] https://snapcraft.io/docs/core/install

Best regards
-- 
Alan Pope
Snap Advocate

Canonical - Ubuntu Engineering and Services
+44 (0) 7973 620 164
alan.p...@canonical.com
http://ubuntu.com/

Re: [Query] Override sendemail.tocmd

[Viresh Kumar]

On 10-05-17, 12:18, Ævar Arnfjörð Bjarmason wrote:
> On Wed, May 10, 2017 at 9:23 AM, Viresh Kumar  wrote:
> > Hi,
> >
> > I have a use case and wanted help from you guys..
> >
> > I have set the sendemail.tocmd option in my kernel's .git/config
> > file and that works very well. But sometimes I want to avoid that
> > (for example sending the patch to someone without the list and
> > maintainers).
> >
> > The suppress-cc=all option works quite well to avoid almost
> > everything (including running --cc-cmd), but it doesn't suppress
> > --to-cmd.
> >
> > Are there any specific reasons why it is being done that way?
> >
> > Currently I need to go edit my .git/config file and remove tocmd
> > before sending any such patches and it sucks :)
> 
> Does something like this work:
> 
> git -c sendemail.tocmd=/bin/false send-email ...
> 
> ?

That failed. I tried with 2.12 version first and then on the next branch with
version: 2.13.0.303.g4ebf3021692d

$ git -c sendemail.tocmd=/bin/false send-email --to vingu 0001-abc.patch
0001-abc.patch
(mbox) Adding cc: Viresh Kumar  from line 'From: 
Viresh Kumar '
(body) Adding cc: Viresh Kumar  from line 
'Signed-off-by: Viresh Kumar '
(to-cmd) failed to close pipe to '/bin/false' at 
/usr/libexec/git-core/git-send-email line 1687.

-- 
viresh

Re: [Query] Override sendemail.tocmd

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 9:23 AM, Viresh Kumar  wrote:
> Hi,
>
> I have a use case and wanted help from you guys..
>
> I have set the sendemail.tocmd option in my kernel's .git/config
> file and that works very well. But sometimes I want to avoid that
> (for example sending the patch to someone without the list and
> maintainers).
>
> The suppress-cc=all option works quite well to avoid almost
> everything (including running --cc-cmd), but it doesn't suppress
> --to-cmd.
>
> Are there any specific reasons why it is being done that way?
>
> Currently I need to go edit my .git/config file and remove tocmd
> before sending any such patches and it sucks :)

Does something like this work:

git -c sendemail.tocmd=/bin/false send-email ...

?

Schönen Tag

[Santander Consumer Finance]

Schönen Tag,

Ich bin Frau Ana Botin, die Vorsitzende von Santander Consumer Finance. Incase 
Sie haben eine schlechte Kredit oder brauchen Geld, um Ihre Rechnungen und 
Investitionen zu bezahlen, hier ist der richtige Ort, um finanziert werden. 
Unser Zinssatz beträgt 3%.

Füllen Sie das untenstehende Formular aus.

Vollständiger Name:
Geschlecht:
Benötigte Menge:
Dauer:

Danke vielmals,
Frau Ana Botin

Re: What's cooking in git.git (May 2017, #03; Wed, 10)

[Jean-Noël AVILA]

Le mercredi 10 mai 2017, 10:08:14 CEST Ævar Arnfjörð Bjarmason a écrit :
> > * ja/doc-l10n (2017-03-20) 3 commits
> >  . SQUASH???
> >  . l10n: add git-add.txt to localized man pages
> >  . l10n: introduce framework for localizing man pages
> >
> >  A proposal to use po4a to localize our manual pages.
> >
> >  Will discard.
> >  Has been stalled for too long.
> 
> Jean-Noel: Need any help with this?
> 

I restarted on this yesterday. The change for git will be reduced to what Junio 
proposed, that is embedding a repo into /Documentation, with some targets added 
to Makefile if the directory is present. In the doc l10n repo, a Makefile's 
target will copy the asciidoc source files from /Documentation into the repo, 
in order to create a stable base for all the po4a stuff.

This is quite slow as my Makefile-fu is very basic.

[ANNOUNCE] Git for Windows 2.13.0

[Johannes Schindelin]

Dear Git users,

It is my pleasure to announce that Git for Windows 2.13.0 is available from:

https://git-for-windows.github.io/

Changes since Git for Windows v2.12.2(2) (April 5th 2017)

New Features

  * Comes with Git v2.13.0.
  * Comes with cURL v7.54.0.
  * Comes with Git LFS v2.1.0.

Bug Fixes

  * As per Git LFS' convention, it is installed into the bin/ directory
again.
  * Calling git add with an absolute path using different upper/lower
case than recorded on disk will now work as expected instead of
claiming that the paths are outside the repository.
  * Git for Windows no longer tries to determine the default printer.
  * When writing the Git index file, Git for Windows no longer has the
wrong idea about the file's timestamp.
  * On Windows, absolute paths can start with a backslash (implicitly
referring to the same drive as the current directory), and now git
clone can use those paths, too.

Filename | SHA-256
 | ---
Git-2.13.0-64-bit.exe | 
6b298c597ffe960bec84d52fc15438d0ff61a24ac9de1c17433878ef656d50c8
Git-2.13.0-32-bit.exe | 
a1b45272f2e30a3545b96086ccf5a56a4154a47898a7d1d6803d51440236b3b5
PortableGit-2.13.0-64-bit.7z.exe | 
1d6008cb6fec817a302d4b59921423ba1f6db2677c6b519c20e9ca2cacc7c220
PortableGit-2.13.0-32-bit.7z.exe | 
119c95b4416ca1100a4feee0d75ceaf3d41962b0038620c04a62dd787e80e5f2
MinGit-2.13.0-64-bit.zip | 
20acda973eca1df056ad08bec6e05c3136f40a1b90e2a290260dfc36e9c2c800
MinGit-2.13.0-32-bit.zip | 
64600a4158eb6a74087f901b6d56d4284f14c43a5075b8f7c947cad7cc667d1f
Git-2.13.0-64-bit.tar.bz2 | 
a1090222306dddaad076b431021be7ad99fce682823e38f15177e1ac230aea91
Git-2.13.0-32-bit.tar.bz2 | 
7a90913ad80fc6a9c1591bb7829ce84b9c2c9800ea0d5abf96c642815ba1a193

Ciao,
Johannes

Re: What's cooking in git.git (May 2017, #03; Wed, 10)

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 7:18 AM, Junio C Hamano  wrote:
> [New Topics]
>
> * ab/compat-regex-update (2017-05-09) 2 commits
>  - compat/regex: update the gawk regex engine from upstream
>  - compat/regex: add a README with a maintenance guide
>
>  Will merge to 'next'.

[Sent last E-Mail too soon]

There's a tiny typo in "compat/regex: update the gawk regex engine
from upstream", one of the two bullet points says "Git project.f",
should say "Git project.", i.e. without the stray ".f".

If you don't mind amending that before merging it down that would be
great, due to the whole multi-patch !fixup -> you needing to squash
that sounds easier

Re: What's cooking in git.git (May 2017, #03; Wed, 10)

[Ævar Arnfjörð Bjarmason]

On Wed, May 10, 2017 at 7:18 AM, Junio C Hamano  wrote:

> * mg/status-in-progress-info (2017-04-14) 1 commit
>  - status: show in-progress info for short status
>
>  "git status" learns an option to report various operations
>  (e.g. "merging") that the user is in the middle of.
>
>  It is still unclear if the participants decided that it is OK to
>  spell "--inprogress" as a single word.

I agree with your mention in
 that it should be
--in-progress & status.inProgress, or alternatively just turned on by
default as mentioned upthread from that.

> * jc/bundle (2016-03-03) 6 commits
>  - index-pack: --clone-bundle option
>  - Merge branch 'jc/index-pack' into jc/bundle
>  - bundle v3: the beginning
>  - bundle: keep a copy of bundle file name in the in-core bundle header
>  - bundle: plug resource leak
>  - bundle doc: 'verify' is not about verifying the bundle
>
>  The beginning of "split bundle", which could be one of the
>  ingredients to allow "git clone" traffic off of the core server
>  network to CDN.
>
>  This was surrected from a "to be discarded" pile, as from time to
>  time people wonder about resumable clone that can be primed without
>  bothering Git servers with dynamic packfile creation, and some
>  people seem to think that the topic could serve as a useful
>  building block for that goal.  But nothing seem to have happend.
>  Unless people really want it, I am inclined to discard this topic.
>  Opinions?

I think it would be awesome to have CDN distribution, but I'm not
familiar enough with that area to say if the implementation is good.
Would be nice if this were pushed forward somehow.

> * ja/doc-l10n (2017-03-20) 3 commits
>  . SQUASH???
>  . l10n: add git-add.txt to localized man pages
>  . l10n: introduce framework for localizing man pages
>
>  A proposal to use po4a to localize our manual pages.
>
>  Will discard.
>  Has been stalled for too long.

Jean-Noel: Need any help with this?

> * jk/parse-options-no-no-no (2017-04-25) 2 commits
>  . SQUASH???
>  . parse-options: disallow double-negations of options starting with no-
>
>  Command line options that begin with "--no-" (e.g. "--no-checkout"
>  option of "git clone") can be negated by removing "--no-"; we
>  historically also allowed prefixing an extra "no" to the option
>  (e.g. "--no-no-checkout"), which made the command line look ugly
>  and unusual.  This proposes to forbid it.
>
>  While I agree there is no need to support "--no-no-checkout", this
>  looks more like "if it looks ugly and unusual, you do not have to
>  use it".  Perhaps we can drop it?
>
>  Will discard.

Having started this whole saga with another patch: Sound sensible, if
something's to be salvaged from this Stefan Beller's suggestion that
we don't have any no-* options but instead just have * on by default
was the most viable way forward I thought.

> * sh/grep-tree-obj-tweak-output (2017-01-20) 2 commits
>  - grep: use '/' delimiter for paths
>  - grep: only add delimiter if there isn't one already
>
>  "git grep", when fed a tree-ish as an input, shows each hit
>  prefixed with ":::".  As  is
>  almost always either a commit or a tag that points at a commit, the
>  early part of the output ":" can be used as the
>  name of the blob and given to "git show".  When  is a
>  tree given in the extended SHA-1 syntax (e.g. ":", or
>  ":"), however, this results in a string that does not
>  name a blob (e.g. "::" or "::").
>  "git grep" has been taught to be a bit more intelligent about these
>  cases and omit a colon (in the former case) or use slash (in the
>  latter case) to produce ":" and
>  ":/" that can be used as the name of a blob.
>
>  Will discard.
>  Nobody seems to be championing this change, after asked in all
>  recent "What's cooking" report:
>  "Is this good enough with known limitations?"

Stefan Hajnoczi: After skimming this for the Nth time it's not obvious
to me from the commit messages why this is better, and if it's
different from how this grep-commit feature is usually used.

Also I've read the grep manpage 2-3 times trying to find out how to
grep a commit/tree, but always end up not getting it and just brute
forcing something. Maybe fixing the docs would be a good starting
point for a series that changes the semantics of how the revisions are
specified?

> * ja/do-not-ask-needless-questions (2017-05-04) 3 commits
>  - git-filter-branch: make the error msg when missing branch more open
>  - read-tree -m: make error message for merging 0 trees less smart aleck
>  - usability: don't ask questions if no reply is required
>
>  Git sometimes gives an advice in a rhetorical question that does
>  not require an answer, which can confuse new users and non native
>  speakers.  Attempt to rephrase them.
>
>  I lost track of the discussion on this topic.  Did we decide that
>  this is not a good idea?

I tried to summarize the status in

Re: [Query] Override sendemail.tocmd

[Viresh Kumar]

On 10 May 2017 at 12:53, Viresh Kumar  wrote:
> Hi,
>
> I have a use case and wanted help from you guys..
>
> I have set the sendemail.tocmd option in my kernel's .git/config
> file and that works very well. But sometimes I want to avoid that
> (for example sending the patch to someone without the list and
> maintainers).
>
> The suppress-cc=all option works quite well to avoid almost
> everything (including running --cc-cmd), but it doesn't suppress
> --to-cmd.
>
> Are there any specific reasons why it is being done that way?
>
> Currently I need to go edit my .git/config file and remove tocmd
> before sending any such patches and it sucks :)

And it would be *really* useful if we can add the "review" (or whatever
it is called) screen with suppress-cc command as well. Currently the mail
goes away directly without giving us a chance to look at the final list of
recipients the mail is going to :)

--
viresh

[Query] Override sendemail.tocmd

[Viresh Kumar]

Hi,

I have a use case and wanted help from you guys..

I have set the sendemail.tocmd option in my kernel's .git/config
file and that works very well. But sometimes I want to avoid that
(for example sending the patch to someone without the list and
maintainers).

The suppress-cc=all option works quite well to avoid almost
everything (including running --cc-cmd), but it doesn't suppress
--to-cmd.

Are there any specific reasons why it is being done that way?

Currently I need to go edit my .git/config file and remove tocmd
before sending any such patches and it sucks :)

--
viresh

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Junio C Hamano]

Brandon Williams  writes:

> Convert 'parse_pathspec()' to take an index parameter.
>
> Since the index is only needed when the PATHSPEC_SUBMODULE_LEADING_PATH
> and PATHSPEC_STRIP_SUBMODULE_SLASH flags are given, add a check
> requiring a non-NULL index when either of these flags are given.
> Convert callers which use these two flags to pass in an index while
> having other callers pass in NULL.
>
> Now that pathspec.c does not use any cache macros and has no references
> to 'the_index', mark it by defining NO_THE_INDEX_COMPATIBILITY_MACROS.
>
> Signed-off-by: Brandon Williams 

The same comment as 5/8 applies to this change, but it is a bit
easier to judge, because it has so many callers, and for some
builtins, especially manipulator commands like add, checkout, and
commit, there may be a good reason why they want to keep the primary
index while playing with an additional in-core index in a distant
future.

Does a pathspec parsed using one instance of index_state expected to
work when matching against a path in another instance of index_state?
Otherwise, passing a non-NULL istate to parse_pathspec() would tie
the resulting pathspec to a particular index_state in some way and
there may need a mechanism to catch an attempt to match paths in
another index_state with such a pathspec as an error.  Just
speculating out loud...