Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/06/2017 07:16 PM, Jeff King wrote:
> On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:
> 
>> I do have one twinge of uneasiness at a deeper level, that I haven't had
>> time to check...
>>
>> Does this patch make it easier to *set* HEAD to an unborn branch that
>> d/f conflicts with an existing reference? If so, that might be a
>> slightly worse UI for users. I'd rather learn about such a problem when
>> setting HEAD (when I am thinking about the new branch name and am in the
>> frame of mind to solve the problem) rather than later, when I try to
>> commit to the new branch.
> 
> Good question. The answer is no, it's allowed both before and after my
> patch. At least via git-symbolic-ref.
> 
> I agree it would be nice to know earlier for such a case. For
> symbolic-ref, we probably should allow it, because it's plumbing that
> may be used for tricky things. For things like "checkout -b", you'd
> generally get a timely warning as we try to create the ref.
> 
> The odd man out is "checkout --orphan", which leaves the branch unborn.
> It might be nice if it did a manual check that the ref is available (and
> also that it's syntactically acceptable, though I think we may do that
> already).
> 
> But all of that is orthogonal to this fix, I think.

Thanks for checking. Yes, I totally agree that this is orthogonal.

Michael

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Junio C Hamano]

Stefan Beller  writes:

> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index ec5f530102..42f584f8b3 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -590,7 +590,7 @@ test_expect_success '%(trailers:unfold) unfolds trailers' 
> '
>  test_expect_success ':only and :unfold work together' '
>   git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
>   git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
> - test_cmp actual reverse &&
> + test_cmp reverse actual &&
>   {
>   grep -v patch.descriptionecho

This test is trying to see that giving the two modifers in any order
produces identical results, so the result of swaping is no more or
no less correct than the original, because what this test calls
"actual" is no less authoritative result than what is in "reverse"
at this point, which is the reason why we prefer "expect actual" in
the typical use of test_cmp.

Renaming  to (rather meaningless)  or
 might make the intention a bit clearer.

THanks.

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Junio C Hamano]

Jeff King  writes:

> Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
> And your patch is obviously an improvement, but I have to wonder if some
> of these make any sense.
>
> If we're expecting some outcome, then it's reasonable to say:
>
>   1. The output should look exactly like this. (test_cmp)
>
>   2. The output should look something like this. (grep)
>
>   3. The output should _not_ mention this (! grep)
>
> But "the output should not look exactly like this" doesn't seem very
> robust. It's likely to give a false success due to small changes (or
> translations), or even bugs in the script.

Yeah, thanks for stepping back and looking at it from higher level.

$ git grep -e 'test_must_fail test_cmp' -e '! test_cmp' t/

gives 21 hits, in addition to the 5 Stefan identified, and it would
be a promising hunt to go through each one of them to see if they
make sense.

Re: Regression in 'git branch -m'?

[Junio C Hamano]

Jeff King  writes:

> Earlier I blamed Duy's 31824d180d. And that is the start of the
> regression in v2.15, but only because it fixed another bug which was
> papering over the one I'm fixing here. :)

I haven't read Michael's reply, but 2/2 is very well explained and
looks correct.

Thanks for digging this through to the root.  

>   [v1 1/2]: t3308: create a real ref directory/file conflict
>   [v1 2/2]: refs_resolve_ref_unsafe: handle d/f conflicts for writes
>
>  refs.c  | 15 ++-
>  t/t1401-symbolic-ref.sh | 26 +-
>  t/t3200-branch.sh   | 10 ++
>  t/t3308-notes-merge.sh  |  2 +-
>  4 files changed, 50 insertions(+), 3 deletions(-)
>
> -Peff

Re: [RFC PATCH v3 0/4] implement fetching of moved submodules

[Junio C Hamano]

Heiko Voigt  writes:

> So the thing here is: If we want to make sure that we stay backwards
> compatible by supporting the setup with gitlinks without configuration.
> Then we also should keep tests around that have the plain manual setup
> without .gitmodules files. Just something, I think, we should keep in
> mind.
>
> Apart from the tests nothing has been added in this iteration. Since I
> finally have a working test now I will continue with reviving the
> fallback to paths.

Thanks for an update.

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Junio C Hamano]

Stefan Beller  writes:

>> How did you find these?  E.g. is there a grep pattern that reviewers
>> can use to repeat your results?
>
> The grunge work was done via scrolling through
>
> git -C t grep test_cmp
>
> However it occurred to me that checking for the completeness could be done
> via
>
>   git -C t grep test_cmp | \
> awk '{$1=""; print }  | \ # remove file name from output
> sort | uniq

Just like grep's GNUism, you can use -h to lose awk, i.e.

git grep -h -e test_cmp t/ | sort -u

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Junio C Hamano]

Paolo Bonzini  writes:

> On 06/10/2017 14:33, Christian Couder wrote:
>> Ok. I think you might want something called for example
>> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
>> trailer with the same (, ) pair above or below the line
>> where the replaced trailer will be put when ignoring trailers with a
>> different ".
>
> So basically "moveIfClosest" (move if last for where=end, move if first
> for where=begin; for where=after and where=before it would just end up
> doing nothing)?  It's not hard to implement, but I'm wondering if it's
> too ad hoc.

Yeah, it makes me wonder exactly that, too.

RE: [Question] Documenting platform implications on CVE to git

[Randall S. Becker]

-Original Message-
On October 6, 2017 7:45 PM  Jonathan Nieder wrote: Cc: git@vger.kernel.org
>Randall S. Becker wrote:
>> The first one, mostly. When looking at CVE-2017-14867, there are 
>> places like
>> https://nvd.nist.gov/vuln/detail/CVE-2017-14867 where the issue is 
>> discussed. It provides hyperlinks to various platform discussions.
>> Unfortunately for me, I am not an HPE employee - and even if I was, 
>> there is no specific site where I can publicly discuss the 
>> vulnerability. I'm looking to the group here for advice on how to get 
>> the word out that it does not appear to apply to the HPE NonStop Git 
>> port. The question of where to best do that for any CVE pertaining to 
>> git as applicable to the NonStop Port is question #1.

>How do people find out about the HPE NonStop Git port?  Where is it 
>distributed? 

It is available at 
http://ituglib.connect-community.org/apps/Ituglib/SrchOpenSrcLib.jsf but we 
have limited abilities to modify anything but that page. There is a brief 
release note but nothing sufficient to have a discussion. 

>Does that distribution point allow you to publish release notes or other 
>documentation?

Not enough for what I think are our needs.
 
> Do you have a web page?  That's another place you can publish information. 
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-14867
> links to lots of resources that are not from the Git project.
> The oss-security list 
> allows anyone to participate.  It is a place that people often collaborate to 
> figure out the impact of a published
> vulnerability, how to mitigate it, etc.  There are other similar mailing 
> lists elsewhere, too.

Thanks, I'll take these to the team.

>> Question #2 - probably more relevant to the specific issue and this 
>> group - is whether the vulnerability is contained to Git's use of Perl 
>> SCM and since NonStop's Perl does not support SCM, the vulnerability 
>> may not be relevant, but I'm not really enough of a Perl guru to make that 
>> determination.

>What is Perl SCM?  I don't know what you're talking about.

Base Perl does not have a lot of capability beyond a simple interpreter. The 
CPAN project,  https://www.cpan.org/, provides implementations of useful 
modules, including Source Code Management (SCM) modules that enable things like 
cvsserver AFAIK, and Mercurial to run. Without it (being the ability to 
arbitrarily add CPAN modules, which is an issue on NonStop), Perl tends to be a 
bit handcuffed and blindfolded. 

Thanks for the suggestions,
Randall

Re: [Question] Documenting platform implications on CVE to git

[Jonathan Nieder]

Hi,

Randall S. Becker wrote:

> The first one, mostly. When looking at CVE-2017-14867, there are places like
> https://nvd.nist.gov/vuln/detail/CVE-2017-14867 where the issue is
> discussed. It provides hyperlinks to various platform discussions.
> Unfortunately for me, I am not an HPE employee - and even if I was, there is
> no specific site where I can publicly discuss the vulnerability. I'm looking
> to the group here for advice on how to get the word out that it does not
> appear to apply to the HPE NonStop Git port. The question of where to best
> do that for any CVE pertaining to git as applicable to the NonStop Port is
> question #1.

How do people find out about the HPE NonStop Git port?  Where is it
distributed?  Does that distribution point allow you to publish
release notes or other documentation?

Do you have a web page?  That's another place you can publish
information.  http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2017-14867
links to lots of resources that are not from the Git project.

The oss-security list 
allows anyone to participate.  It is a place that people often
collaborate to figure out the impact of a published vulnerability, how
to mitigate it, etc.  There are other similar mailing lists elsewhere,
too.

> Question #2 - probably more relevant to the specific issue and this group -
> is whether the vulnerability is contained to Git's use of Perl SCM and since
> NonStop's Perl does not support SCM, the vulnerability may not be relevant,
> but I'm not really enough of a Perl guru to make that determination.

What is Perl SCM?  I don't know what you're talking about.

Thanks,
Jonathan

RE: [Question] Documenting platform implications on CVE to git

[Randall S. Becker]

-Original Message-
On October 6, 2017 6:51 PM, Jonathan Nieder wrote
>Randall S. Becker wrote:
>> I wonder whether there is some mechanism for providing official 
>> responses from platform ports relating to security CVE reports, like
CVE-2017-14867.

>This question is too abstract for me.  Can you say more concretely what you
are trying to do?
>E.g. are you asking how you would communicate to users of your port that
CVE-2017-14867
?does not apply to them?  Or are you asking where to start a conversation
about
>who a bug applies to?  Or something else?

The first one, mostly. When looking at CVE-2017-14867, there are places like
https://nvd.nist.gov/vuln/detail/CVE-2017-14867 where the issue is
discussed. It provides hyperlinks to various platform discussions.
Unfortunately for me, I am not an HPE employee - and even if I was, there is
no specific site where I can publicly discuss the vulnerability. I'm looking
to the group here for advice on how to get the word out that it does not
appear to apply to the HPE NonStop Git port. The question of where to best
do that for any CVE pertaining to git as applicable to the NonStop Port is
question #1.

Question #2 - probably more relevant to the specific issue and this group -
is whether the vulnerability is contained to Git's use of Perl SCM and since
NonStop's Perl does not support SCM, the vulnerability may not be relevant,
but I'm not really enough of a Perl guru to make that determination.

Cheers,
Randall

-- Brief whoami: NonStop developer since approximately
UNIX(421664400)/NonStop(2112884442) 
-- In my real life, I talk too much.

Re: [RFC PATCH v3 0/4] implement fetching of moved submodules

[Stefan Beller]

On Fri, Oct 6, 2017 at 3:25 PM, Heiko Voigt  wrote:
> The last iteration can be found here:
>
> https://public-inbox.org/git/20170817105349.gc52...@book.hvoigt.net/
>
> This is mainly a status update and to let people know that I am still
> working on this.

Cool. :)

> I struggled quite a bit with reviving my original test for the path
> based recursive fetch (first patch). The behavior seems to haved changed
> and simply setting the submodule configuration in .git/config without
> one in .gitmodules does not work anymore. I did not have time to
> investigate whether this was a deliberate change or a maybe a bug?

I think it is deliberate. (We wrote this man page "gitsubmodules"[1] and there
was so much discussion on "What is a submodule?". Key take away is this:
* a gitlink alone is not a submodule.
* a submodule consists of at least
  -> the gitlink in the superproject
  -> a mapping of path -> name via
  $(git config -f .gitmodules submodule..path)
  -> Depending on config in .git/config or the existence of its git directory,
  it may be [in]active or [de]initialized.

[1] not to be confused with "gitmodules" or "git-submodule"

Sometimes we accept a plain git-link without the config in .gitmodules,
(a) due to historic reasons or (b) because it seems sane even for
a repo "that just happens to exist at the gitlinks location"
(example git-diff)

> So the solution for now is that I write my fake configuration (to avoid
> skipping submodule handling altogether) into a .gitmodules file.

I'll try to spot what is fake about the config.

> The second patch (cleanup of a submodule push testcase) was written
> because that currently is the only test failing. It is not meant for
> inclusion but rather as a demonstration of what might be happening when
> we cleanup testcases: Because of the behavioral change above, on first
> sight, it seemed like there was a shortcut in fetch and so on-demand
> fetch without submodule configuration would not be supported anymore.
>
> IIRC there were a lot more tests failing before when I implemented my
> patch without the fallback on paths. So my guess is that some tests have
> been cleaned up to use proper (.gitmodules) submodule setup.

I don't remember any large recent activity for submodule things lately.

> So the thing here is: If we want to make sure that we stay backwards
> compatible by supporting the setup with gitlinks without configuration.
> Then we also should keep tests around that have the plain manual setup
> without .gitmodules files. Just something, I think, we should keep in
> mind.

But do we want this?

Without the name<->path mapping, we can only have the "old style"
submodules, that have their git repo inside its tree instead of inside
the superprojects git dir.
So renaming/moving "old style with no name<->path mapping" will not
work. (That may be an acceptable trade off. But then again, just providing
the mapping, such that the superproject can absorb the git directory
of the submodule for this use case, doesn't seem like a big deal to me.
Something you want to have anyway, for ease of use of the superproject
w.r.t. cloning for example)

So while I do not try to deliberately break these old behaviors, I'd rather
want us to go forward with a saner model than "if we happen to have
enough data around, the operation succeeds", i.e. ignore anything
that is not following the rather strict definition of a submodule.

FYI: Once upon a time I found "fake submodules"
http://debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb
Last time this was discussed on list, this was considered a bug not
worth fixing instead of a feature IIRC. (Personally I think this is
a rather cool hack, which we may want to abuse ourselves for
things like "convert a subtree into a submodule and back again",
but we could also go without this hack)

> Apart from the tests nothing has been added in this iteration. Since I
> finally have a working test now I will continue with reviving the
> fallback to paths.

I'll have a look.

Cheers,
Stefan

Re: [Question] Documenting platform implications on CVE to git

[Jonathan Nieder]

Hi Randall,

Randall S. Becker wrote:

> I wonder whether there is some mechanism for providing official responses
> from platform ports relating to security CVE reports, like CVE-2017-14867.

This question is too abstract for me.  Can you say more concretely
what you are trying to do?

E.g. are you asking how you would communicate to users of your port
that CVE-2017-14867 does not apply to them?  Or are you asking where
to start a conversation about who a bug applies to?  Or something
else?

Thanks,
Jonathan

> For example, the Perl implementation on HPE NonStop does not include the SCM
> module so commands relating cvsserver may not be available - one thing to be
> verified so is a question #1. But the real question (#2) is: where would one
> most appropriately document issues like this to be consistent with other
> platform responses relating to git?
>
> Thanks,
> Randall
>
> -- Brief whoami: NonStop developer since approximately
> UNIX(421664400)/NonStop(2112884442)
> -- In my real life, I talk too much.

[RFC PATCH v3 3/4] implement fetching of moved submodules

[Heiko Voigt]

We store the changed submodules paths to calculate which submodule needs
fetching. This does not work for moved submodules since their paths do
not stay the same in case of a moved submodules. In case of new
submodules we do not have a path in the current checkout, since they
just appeared in this fetch.

It is more general to collect the submodule names for changes instead of
their paths to include the above cases.

With the change described above we implement 'on-demand' fetching of
changes in moved submodules.

Note: This does only work when repositories have a .gitmodules file. In
other words: We now require a name for a submodule repository.

Signed-off-by: Heiko Voigt 
---

No changes from the previous version here.

 submodule.c | 91 +
 t/t5526-fetch-submodules.sh | 35 +
 2 files changed, 85 insertions(+), 41 deletions(-)

diff --git a/submodule.c b/submodule.c
index 63e7094..0c586a0 100644
--- a/submodule.c
+++ b/submodule.c
@@ -21,7 +21,7 @@
 #include "parse-options.h"
 
 static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_paths = STRING_LIST_INIT_DUP;
+static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
 static int initialized_fetch_ref_tips;
 static struct oid_array ref_tips_before_fetch;
 static struct oid_array ref_tips_after_fetch;
@@ -674,11 +674,11 @@ const struct submodule *submodule_from_ce(const struct 
cache_entry *ce)
 }
 
 static struct oid_array *submodule_commits(struct string_list *submodules,
-  const char *path)
+  const char *name)
 {
struct string_list_item *item;
 
-   item = string_list_insert(submodules, path);
+   item = string_list_insert(submodules, name);
if (item->util)
return (struct oid_array *) item->util;
 
@@ -687,39 +687,34 @@ static struct oid_array *submodule_commits(struct 
string_list *submodules,
return (struct oid_array *) item->util;
 }
 
+struct collect_changed_submodules_cb_data {
+   struct string_list *changed;
+   const struct object_id *commit_oid;
+};
+
 static void collect_changed_submodules_cb(struct diff_queue_struct *q,
  struct diff_options *options,
  void *data)
 {
+   struct collect_changed_submodules_cb_data *me = data;
+   struct string_list *changed = me->changed;
+   const struct object_id *commit_oid = me->commit_oid;
int i;
-   struct string_list *changed = data;
 
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
struct oid_array *commits;
+   const struct submodule *submodule;
+
if (!S_ISGITLINK(p->two->mode))
continue;
 
-   if (S_ISGITLINK(p->one->mode)) {
-   /*
-* NEEDSWORK: We should honor the name configured in
-* the .gitmodules file of the commit we are examining
-* here to be able to correctly follow submodules
-* being moved around.
-*/
-   commits = submodule_commits(changed, p->two->path);
-   oid_array_append(commits, >two->oid);
-   } else {
-   /* Submodule is new or was moved here */
-   /*
-* NEEDSWORK: When the .git directories of submodules
-* live inside the superprojects .git directory some
-* day we should fetch new submodules directly into
-* that location too when config or options request
-* that so they can be checked out from there.
-*/
+   submodule = submodule_from_path(commit_oid, p->two->path);
+   if (!submodule)
continue;
-   }
+
+   commits = submodule_commits(changed, submodule->name);
+   oid_array_append(commits, >two->oid);
}
 }
 
@@ -742,11 +737,14 @@ static void collect_changed_submodules(struct string_list 
*changed,
 
while ((commit = get_revision())) {
struct rev_info diff_rev;
+   struct collect_changed_submodules_cb_data data;
+   data.changed = changed;
+   data.commit_oid = >object.oid;
 
init_revisions(_rev, NULL);
diff_rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
diff_rev.diffopt.format_callback = 
collect_changed_submodules_cb;
-   diff_rev.diffopt.format_callback_data = changed;
+   diff_rev.diffopt.format_callback_data = 

[RFC PATCH v3 4/4] submodule: simplify decision tree whether to or not to fetch

[Heiko Voigt]

To make extending this logic later easier.

Signed-off-by: Heiko Voigt 
---

This should also be the same as in the previous version.

 submodule.c | 74 ++---
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/submodule.c b/submodule.c
index 0c586a0..c7b32c6 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1142,6 +1142,31 @@ struct submodule_parallel_fetch {
 };
 #define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
 
+static int get_fetch_recurse_config(const struct submodule *submodule,
+   struct submodule_parallel_fetch *spf)
+{
+   if (spf->command_line_option != RECURSE_SUBMODULES_DEFAULT)
+   return spf->command_line_option;
+
+   if (submodule) {
+   char *key;
+   const char *value;
+
+   int fetch_recurse = submodule->fetch_recurse;
+   key = xstrfmt("submodule.%s.fetchRecurseSubmodules", 
submodule->name);
+   if (!repo_config_get_string_const(the_repository, key, )) 
{
+   fetch_recurse = parse_fetch_recurse_submodules_arg(key, 
value);
+   }
+   free(key);
+
+   if (fetch_recurse != RECURSE_SUBMODULES_NONE)
+   /* local config overrules everything except commandline 
*/
+   return fetch_recurse;
+   }
+
+   return spf->default_option;
+}
+
 static int get_next_submodule(struct child_process *cp,
  struct strbuf *err, void *data, void **task_cb)
 {
@@ -1161,46 +1186,21 @@ static int get_next_submodule(struct child_process *cp,
 
submodule = submodule_from_path(_oid, ce->name);
 
-   default_argv = "yes";
-   if (spf->command_line_option == RECURSE_SUBMODULES_DEFAULT) {
-   int fetch_recurse = RECURSE_SUBMODULES_NONE;
-
-   if (submodule) {
-   char *key;
-   const char *value;
-
-   fetch_recurse = submodule->fetch_recurse;
-   key = 
xstrfmt("submodule.%s.fetchRecurseSubmodules", submodule->name);
-   if 
(!repo_config_get_string_const(the_repository, key, )) {
-   fetch_recurse = 
parse_fetch_recurse_submodules_arg(key, value);
-   }
-   free(key);
-   }
-
-   if (fetch_recurse != RECURSE_SUBMODULES_NONE) {
-   if (fetch_recurse == RECURSE_SUBMODULES_OFF)
-   continue;
-   if (fetch_recurse == 
RECURSE_SUBMODULES_ON_DEMAND) {
-   if 
(!unsorted_string_list_lookup(_submodule_names,
-
submodule->name))
-   continue;
-   default_argv = "on-demand";
-   }
-   } else {
-   if (spf->default_option == 
RECURSE_SUBMODULES_OFF)
-   continue;
-   if (spf->default_option == 
RECURSE_SUBMODULES_ON_DEMAND) {
-   if 
(!unsorted_string_list_lookup(_submodule_names,
- 
submodule->name))
-   continue;
-   default_argv = "on-demand";
-   }
-   }
-   } else if (spf->command_line_option == 
RECURSE_SUBMODULES_ON_DEMAND) {
-   if 
(!unsorted_string_list_lookup(_submodule_names,
+   switch (get_fetch_recurse_config(submodule, spf))
+   {
+   default:
+   case RECURSE_SUBMODULES_DEFAULT:
+   case RECURSE_SUBMODULES_ON_DEMAND:
+   if (!submodule || 
!unsorted_string_list_lookup(_submodule_names,
 submodule->name))
continue;
default_argv = "on-demand";
+   break;
+   case RECURSE_SUBMODULES_ON:
+   default_argv = "yes";
+   break;
+   case RECURSE_SUBMODULES_OFF:
+   continue;
}
 
strbuf_addf(_path, "%s/%s", spf->work_tree, ce->name);
-- 
2.10.0.129.g35f6318

[RFC PATCH 2/4] change submodule push test to use proper repository setup

[Heiko Voigt]

NOTE: The argument in this message is not correct, see description in
cover letter.

The setup of the repositories in this test is using gitlinks without the
.gitmodules infrastructure. It is however testing convenience features
like --recurse-submodules=on-demand. These features are already not
supported by fetch without a .gitmodules file. This leads us to the
conclusion that it is not really used here as well.

Let's use the usual submodule commands to setup the repository in a
typical way. This also has the advantage that we are testing with a
repository structure that is more similar to one we could expect on a
users setup.

Signed-off-by: Heiko Voigt 
---

As mentioned in the cover letter. This seems to be the only test that
ensures that we stay compatible with setups without .gitmodules. Maybe
we should add/revive some?

Cheers Heiko

 t/t5531-deep-submodule-push.sh | 29 -
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index 39cb2c1..a4a2c6a 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -8,22 +8,26 @@ test_expect_success setup '
mkdir pub.git &&
GIT_DIR=pub.git git init --bare &&
GIT_DIR=pub.git git config receive.fsckobjects true &&
+   mkdir submodule &&
+   (
+   cd submodule &&
+   git init &&
+   git config push.default matching &&
+   >junk &&
+   git add junk &&
+   git commit -m "Initial junk"
+   ) &&
+   git clone --bare submodule submodule.git &&
mkdir work &&
(
cd work &&
git init &&
git config push.default matching &&
-   mkdir -p gar/bage &&
-   (
-   cd gar/bage &&
-   git init &&
-   git config push.default matching &&
-   >junk &&
-   git add junk &&
-   git commit -m "Initial junk"
-   ) &&
-   git add gar/bage &&
+   mkdir gar &&
+   git submodule add ../submodule.git gar/bage &&
git commit -m "Initial superproject"
+   cd gar/bage &&
+   git remote rm origin
)
 '
 
@@ -51,11 +55,10 @@ test_expect_success 'push if submodule has no remote' '
 
 test_expect_success 'push fails if submodule commit not on remote' '
(
-   cd work/gar &&
-   git clone --bare bage ../../submodule.git &&
-   cd bage &&
+   cd work/gar/bage &&
git remote add origin ../../../submodule.git &&
git fetch &&
+   git push --set-upstream origin master &&
>junk3 &&
git add junk3 &&
git commit -m "Third junk"
-- 
2.10.0.129.g35f6318

[RFC PATCH 1/4] fetch: add test to make sure we stay backwards compatible

[Heiko Voigt]

The current implementation of submodules supports on-demand fetch if
there is no .gitmodules entry for a submodule. Let's add a test to
document this behavior.

Signed-off-by: Heiko Voigt 
---
 t/t5526-fetch-submodules.sh | 42 +-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 42251f7..43a22f6 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -478,7 +478,47 @@ test_expect_success "don't fetch submodule when newly 
recorded commits are alrea
git fetch >../actual.out 2>../actual.err
) &&
! test -s actual.out &&
-   test_i18ncmp expect.err actual.err
+   test_i18ncmp expect.err actual.err &&
+   (
+   cd submodule &&
+   git checkout -q master
+   )
+'
+
+test_expect_success "'fetch.recurseSubmodules=on-demand' works also without 
.gitmodule entry" '
+   (
+   cd downstream &&
+   git fetch --recurse-submodules
+   ) &&
+   add_upstream_commit &&
+   head1=$(git rev-parse --short HEAD) &&
+   git add submodule &&
+   git rm .gitmodules &&
+   git commit -m "new submodule without .gitmodules" &&
+   printf "" >expect.out &&
+   head2=$(git rev-parse --short HEAD) &&
+   echo "From $pwd/." >expect.err.2 &&
+   echo "   $head1..$head2  master -> origin/master" >>expect.err.2 &&
+   head -3 expect.err >>expect.err.2 &&
+   (
+   cd downstream &&
+   rm .gitmodules &&
+   git config fetch.recurseSubmodules on-demand &&
+   # fake submodule configuration to avoid skipping submodule 
handling
+   git config -f .gitmodules submodule.fake.path fake &&
+   git config -f .gitmodules submodule.fake.url fakeurl &&
+   git add .gitmodules &&
+   git config --unset submodule.submodule.url &&
+   git fetch >../actual.out 2>../actual.err &&
+   # cleanup
+   git config --unset fetch.recurseSubmodules &&
+   git reset --hard
+   ) &&
+   test_i18ncmp expect.out actual.out &&
+   test_i18ncmp expect.err.2 actual.err &&
+   git checkout HEAD^ -- .gitmodules &&
+   git add .gitmodules &&
+   git commit -m "new submodule restored .gitmodules"
 '
 
 test_expect_success 'fetching submodules respects parallel settings' '
-- 
2.10.0.129.g35f6318

[RFC PATCH v3 0/4] implement fetching of moved submodules

[Heiko Voigt]

The last iteration can be found here:

https://public-inbox.org/git/20170817105349.gc52...@book.hvoigt.net/

This is mainly a status update and to let people know that I am still
working on this.

I struggled quite a bit with reviving my original test for the path
based recursive fetch (first patch). The behavior seems to haved changed
and simply setting the submodule configuration in .git/config without
one in .gitmodules does not work anymore. I did not have time to
investigate whether this was a deliberate change or a maybe a bug?

So the solution for now is that I write my fake configuration (to avoid
skipping submodule handling altogether) into a .gitmodules file.

The second patch (cleanup of a submodule push testcase) was written
because that currently is the only test failing. It is not meant for
inclusion but rather as a demonstration of what might be happening when
we cleanup testcases: Because of the behavioral change above, on first
sight, it seemed like there was a shortcut in fetch and so on-demand
fetch without submodule configuration would not be supported anymore.

IIRC there were a lot more tests failing before when I implemented my
patch without the fallback on paths. So my guess is that some tests have
been cleaned up to use proper (.gitmodules) submodule setup.

So the thing here is: If we want to make sure that we stay backwards
compatible by supporting the setup with gitlinks without configuration.
Then we also should keep tests around that have the plain manual setup
without .gitmodules files. Just something, I think, we should keep in
mind.

Apart from the tests nothing has been added in this iteration. Since I
finally have a working test now I will continue with reviving the
fallback to paths.

Cheers Heiko

Heiko Voigt (4):
  fetch: add test to make sure we stay backwards compatible
  change submodule push test to use proper repository setup
  implement fetching of moved submodules
  submodule: simplify decision tree whether to or not to fetch

 submodule.c| 155 ++---
 t/t5526-fetch-submodules.sh|  77 +++-
 t/t5531-deep-submodule-push.sh |  29 
 3 files changed, 174 insertions(+), 87 deletions(-)

-- 
2.10.0.129.g35f6318

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Stefan Beller]

On Fri, Oct 6, 2017 at 3:01 PM, Jonathan Nieder  wrote:
> Stefan Beller wrote:
>
>> Fix the argument order for test_cmp. When given the expected
>> result first the diff shows the actual output with '+' and the
>> expectation with '-', which is the convention for our tests.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>
> Yes, this should make the output from failing tests easier to take in
> at a glance.
>
> Reviewed-by: Jonathan Nieder 
>
> How did you find these?  E.g. is there a grep pattern that reviewers
> can use to repeat your results?

The grunge work was done via scrolling through

git -C t grep test_cmp

However it occurred to me that checking for the completeness could be done
via

  git -C t grep test_cmp | \
awk '{$1=""; print }  | \ # remove file name from output
sort | uniq

There are some cases that are still pretty obvious what the
"actual" and the "expect" is, but they are not included in this patch
as the code (and file names) looked hairy.

Re: [PATCH 2/2] tests: fix diff order arguments in test_cmp

[Jonathan Nieder]

Stefan Beller wrote:

> Fix the argument order for test_cmp. When given the expected
> result first the diff shows the actual output with '+' and the
> expectation with '-', which is the convention for our tests.
>
> Signed-off-by: Stefan Beller 
> ---

Yes, this should make the output from failing tests easier to take in
at a glance.

Reviewed-by: Jonathan Nieder 

How did you find these?  E.g. is there a grep pattern that reviewers
can use to repeat your results?

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:

>> The `test_must_fail` should only be used to indicate a git command is
>> failing. `test_cmp` is not a git command, such that it doesn't need the
>> special treatment of `test_must_fail` (which e.g. includes checking for
>> segfault)
>
> Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
> And your patch is obviously an improvement, but I have to wonder if some
> of these make any sense.

Just for the record: I agree with all the above, and my Reviewed-by
still stands.

Thanks for looking closer.  I wonder if there's a systematic way to
avoid this kind of weak test that can bitrot and still pass too easily
--- e.g. should we have a test_must_differ command with an explanation
of why it should usually be avoided?  Would a lint check that bans
this kind of construct completely be going too far?

Jonathan

Re: [PATCH v7 2/3] submodule--helper: introduce for_each_listed_submodule()

[Eric Sunshine]

Same disclaimer as in my review of patch 1/3: I didn't see a URL in
the cover letter pointing at discussions of earlier iterations, so
below comments may be at odds with what went on previously...

On Fri, Oct 6, 2017 at 9:24 AM, Prathamesh Chavan  wrote:
> Introduce function for_each_listed_submodule() and replace a loop
> in module_init() with a call to it.
>
> The new function will also be used in other parts of the
> system in later patches.
>
> Signed-off-by: Prathamesh Chavan 
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -14,6 +14,11 @@
>  #include "refs.h"
>  #include "connect.h"
>
> +#define OPT_QUIET (1 << 0)
> +
> +typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
> + void *cb_data);

What is the reason for having the definition of 'each_submodule_fn' so
far removed textually from its first reference by
for_each_listed_submodule() below?

>  static char *get_default_remote(void)
>  {
> char *dest = NULL, *ret;
> @@ -348,7 +353,23 @@ static int module_list(int argc, const char **argv, 
> const char *prefix)
> return 0;
>  }
>
> -static void init_submodule(const char *path, const char *prefix, int quiet)
> +static void for_each_listed_submodule(const struct module_list *list,
> + each_submodule_fn fn, void *cb_data)
> +{
> +   int i;
> +   for (i = 0; i < list->nr; i++)
> +   fn(list->entries[i], cb_data);
> +}

I'm very curious about the justification for introducing a for-each
function for what amounts to the simplest sort of loop possible: a
canonical for-loop with a one-line body. I could easily understand the
desire for such a function if either the loop conditions or the body
of the loop, or both, were complex, but this does not seem to be the
case. Even the callers of this new function, in this patch and in 3/3,
are as simple as possible: one-liners (simple function calls).

Although this sort of for-each function can, at times, be helpful,
there are costs: extra boilerplate and increased complexity for
clients since it requires callback functions and (optionally) callback
data. The separation of logic into a callback function can make code
more difficult to reason about than when it is simply the body of a
for-loop.

So, unless the plan for the future is that this for-each function will
have considerable additional functionality baked into it, I'm having a
difficult time understanding why this change is desirable.

> +struct init_cb {
> +   const char *prefix;
> +   unsigned int flags;
> +};
> +
> +#define INIT_CB_INIT { NULL, 0 }

Why are these definitions so far removed from init_submodule_cb() below?

> +static void init_submodule(const char *path, const char *prefix,
> +  unsigned int flags)
>  {
> const struct submodule *sub;
> struct strbuf sb = STRBUF_INIT;
> @@ -410,7 +431,7 @@ static void init_submodule(const char *path, const char 
> *prefix, int quiet)
> if (git_config_set_gently(sb.buf, url))
> die(_("Failed to register url for submodule path 
> '%s'"),
> displaypath);
> -   if (!quiet)
> +   if (!(flags & OPT_QUIET))

This change of having init_submodule() accept a 'flags' argument,
rather than a single boolean, increases reviewer burden, since the
reviewer is forced to puzzle out how this change relates to the stated
intention of the patch since it is not mentioned at all by the commit
message.

It's also conceptually unrelated to the introduction of a for-each
function, thus should be instead be done by a separate preparatory
patch.

> fprintf(stderr,
> _("Submodule '%s' (%s) registered for path 
> '%s'\n"),
> sub->name, url, displaypath);
> @@ -437,12 +458,18 @@ static void init_submodule(const char *path, const char 
> *prefix, int quiet)
> free(upd);
>  }
>
> +static void init_submodule_cb(const struct cache_entry *list_item, void 
> *cb_data)
> +{
> +   struct init_cb *info = cb_data;
> +   init_submodule(list_item->name, info->prefix, info->flags);
> +}
> +
>  static int module_init(int argc, const char **argv, const char *prefix)
>  {
> +   struct init_cb info = INIT_CB_INIT;
> struct pathspec pathspec;
> struct module_list list = MODULE_LIST_INIT;
> int quiet = 0;
> -   int i;
>
> struct option module_init_options[] = {
> OPT__QUIET(, N_("Suppress output for initializing a 
> submodule")),
> @@ -467,8 +494,11 @@ static int module_init(int argc, const char **argv, 
> const char *prefix)
> if (!argc && git_config_get_value_multi("submodule.active"))
> module_list_active();
>
> -   for (i = 0; i < list.nr; i++)
> -   

Re: [PATCH v7 1/3] submodule--helper: introduce get_submodule_displaypath()

[Eric Sunshine]

I didn't find a URL in the cover letter pointing at previous
iterations of this patch series and related discussions, so forgive me
if comments below merely repeat what was said earlier...

On Fri, Oct 6, 2017 at 9:24 AM, Prathamesh Chavan  wrote:
> Introduce function get_submodule_displaypath() to replace the code
> occurring in submodule_init() for generating displaypath of the
> submodule with a call to it.
>
> This new function will also be used in other parts of the system
> in later patches.
>
> Signed-off-by: Prathamesh Chavan 
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -219,6 +219,26 @@ static int resolve_relative_url_test(int argc, const 
> char **argv, const char *pr
> +/* the result should be freed by the caller. */
> +static char *get_submodule_displaypath(const char *path, const char *prefix)
> +{
> +   const char *super_prefix = get_super_prefix();
> +
> +   if (prefix && super_prefix) {
> +   BUG("cannot have prefix '%s' and superprefix '%s'",
> +   prefix, super_prefix);
> +   } else if (prefix) {
> +   struct strbuf sb = STRBUF_INIT;
> +   char *displaypath = xstrdup(relative_path(path, prefix, ));
> +   strbuf_release();
> +   return displaypath;
> +   } else if (super_prefix) {
> +   return xstrfmt("%s%s", super_prefix, path);
> +   } else {
> +   return xstrdup(path);
> +   }
> +}

At first glance, this appears to be a simple code-movement patch which
shouldn't require deep inspection by a reviewer, however, upon closer
examination, it turns out that it is doing rather more than that,
which increases reviewer burden, especially since these additional
changes are not mentioned in the commit message. At a minimum, it
includes these changes:

* factors out calls to get_super_prefix()
* adds extra context to the "BUG" message
* changes die("BUG...") to BUG(...)
* allocates/releases a strbuf
* changes assignments to returns

The final two are obvious necessary (or clarifying) changes which a
reviewer would expect to see in a patch which factors code out to its
own function; the others not so.

This isn't to say that the other changes are not reasonable -- they
are -- but if one of your goals is to make the patches easy for
reviewers to digest, then you should make the changes as obvious as
possible for reviewers to spot. One way would be to mention in the
commit message that you're taking the opportunity to also make these
particular cleanups to the code. A more common approach is to place
the various cleanups in preparatory patches before this one, with one
cleanup per patch. I'd prefer to see the latter (if my opinion carries
any weight).

More below...

> @@ -334,15 +354,7 @@ static void init_submodule(const char *path, const char 
> *prefix, int quiet)
> struct strbuf sb = STRBUF_INIT;
> char *upd = NULL, *url = NULL, *displaypath;
>
> -   if (prefix && get_super_prefix())
> -   die("BUG: cannot have prefix and superprefix");
> -   else if (prefix)
> -   displaypath = xstrdup(relative_path(path, prefix, ));
> -   else if (get_super_prefix()) {
> -   strbuf_addf(, "%s%s", get_super_prefix(), path);
> -   displaypath = strbuf_detach(, NULL);
> -   } else
> -   displaypath = xstrdup(path);
> +   displaypath = get_submodule_displaypath(path, prefix);
>
> sub = submodule_from_path(_oid, path);
>
> @@ -357,9 +369,9 @@ static void init_submodule(const char *path, const char 
> *prefix, int quiet)
>  * Set active flag for the submodule being initialized
>  */
> if (!is_submodule_active(the_repository, path)) {
> -   strbuf_reset();
> strbuf_addf(, "submodule.%s.active", sub->name);
> git_config_set_gently(sb.buf, "true");
> +   strbuf_reset();

This strbuf_reset() movement, and those below, are pretty much just
"noise" changes. They add extra burden to the review process without
really improving the code. The reason they add to reviewer burden is
that they do not seem to be related to the intention stated in the
commit message, so the reviewer must spend extra time trying to
understand their purpose and correctness.

More serious, though, is that these strbuf_reset() movements may
actually increase the burden on someone changing the code in the
future. Presumably, your reason for making these changes is that you
reviewed the code after factoring out the get_submodule_displaypath()
logic and discovered that the strbuf was no longer touched before this
point, therefore resetting it before strbuf_addf() is unnecessary.
While this may be true today, it may not be so in the future. If
someone comes along and adds code above this point which does touch
the strbuf, then these code movements either need to be reverted by

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Stefan Beller]

> I might be naive in thinking that protocol.version could be removed or
> redefined at our discretion just because it's marked as "experimental".

Well the redefinition might very well occur, when we now say "set to v1
to test v1 and fallback to v0 otherwise", but long term we want a white
or black list or some other protocol selection strategy encoded in this
configuration (we would not want to introduce yet another config to work
around the initial "failed experiment", would we?)

And hence I would be careful how we define the meaning of
protocol.version now.

For example we could instead now claim "protocol.version is a whitelist
of protocol versions, order is not specified. The only guarantee we're willing
to give is that no protocol is used that is not on the list".

Later we may want to either add another variable '.versionSelectionStrategy'
that helps out there or we'd just say protocol.version tries to select
the leftmost (first) protocol that both ends support.

All I was trying to say initially is that "we may try (one of) protocol.version,
but fall back to whatever (currently v0)" is too broad. We'd need to redefine
it shortly in the foreseeable future already.

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Jonathan Nieder]

Stefan Beller wrote:

> The `test_must_fail` should only be used to indicate a git command is
> failing. `test_cmp` is not a git command, such that it doesn't need the
> special treatment of `test_must_fail` (which e.g. includes checking for
> segfault).
>
> Signed-off-by: Stefan Beller 
> ---
>  t/t3504-cherry-pick-rerere.sh | 2 +-
>  t/t5512-ls-remote.sh  | 2 +-
>  t/t5612-clone-refspec.sh  | 2 +-
>  t/t7508-status.sh | 2 +-
>  t/t9164-git-svn-dcommit-concurrent.sh | 2 +-
>  5 files changed, 5 insertions(+), 5 deletions(-)

Thanks.  I agree that this is more readable, and it matches the advice
in t/README:

   On the other hand, don't use test_must_fail for running regular
   platform commands; just use '! cmd'.  We are not in the business
   of verifying that the world given to us sanely works.

Reviewed-by: Jonathan Nieder 

I wonder if it would be useful to have a nod to that advice in the
docstring in t/test-lib-functions.sh, too.

[PATCH v3 07/12] apply: move lockfile into `apply_state`

[Martin Ågren]

We have two users of `struct apply_state` and the related functionality
in apply.c. Each user sets up its `apply_state` by handing over a
pointer to its static `lock_file`. (Before 076aa2cbd (tempfile:
auto-allocate tempfiles on heap, 2017-09-05), we could never free
lockfiles, so making them static was a reasonable approach.)

Other than that, they never directly access their `lock_file`s, which
are instead handled by the functionality in apply.c.

To make life easier for the caller and to make it less tempting for a
future caller to mess with the lock, make apply.c fully responsible for
setting up the `lock_file`. As mentioned above, it is now safe to free a
`lock_file`, so we can make the `struct apply_state` contain an actual
`struct lock_file` instead of a pointer to one.

The user in builtin/apply.c is rather simple. For builtin/am.c, we might
worry that the lock state is actually meant to be inherited across
calls. But the lock is only taken as `apply_all_patches()` executes, and
code inspection shows that it will always be released.

Alternatively, we can observe that the lock itself is never queried
directly. When we decide whether we should lock, we check a related
variable `newfd`. That variable is not inherited, so from the point of
view of apply.c, the state machine really is reset with each call to
`init_apply_state()`. (It would be a bug if `newfd` and the lock status
were not in sync. The duplication of information in `newfd` and the lock
will be addressed in the next patch.)

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 apply.c | 14 +-
 apply.h |  5 ++---
 builtin/am.c|  3 +--
 builtin/apply.c |  4 +---
 4 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/apply.c b/apply.c
index c022af53a..5a6ca10a7 100644
--- a/apply.c
+++ b/apply.c
@@ -75,12 +75,10 @@ static int parse_ignorewhitespace_option(struct apply_state 
*state,
 }
 
 int init_apply_state(struct apply_state *state,
-const char *prefix,
-struct lock_file *lock_file)
+const char *prefix)
 {
memset(state, 0, sizeof(*state));
state->prefix = prefix;
-   state->lock_file = lock_file;
state->newfd = -1;
state->apply = 1;
state->line_termination = '\n';
@@ -146,8 +144,6 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
}
if (state->check_index)
state->unsafe_paths = 0;
-   if (!state->lock_file)
-   return error("BUG: state->lock_file should not be NULL");
 
if (state->apply_verbosity <= verbosity_silent) {
state->saved_error_routine = get_error_routine();
@@ -4711,11 +4707,11 @@ static int apply_patch(struct apply_state *state,
state->update_index = state->check_index && state->apply;
if (state->update_index && state->newfd < 0) {
if (state->index_file)
-   state->newfd = 
hold_lock_file_for_update(state->lock_file,
+   state->newfd = 
hold_lock_file_for_update(>lock_file,
 
state->index_file,
 
LOCK_DIE_ON_ERROR);
else
-   state->newfd = hold_locked_index(state->lock_file, 
LOCK_DIE_ON_ERROR);
+   state->newfd = hold_locked_index(>lock_file, 
LOCK_DIE_ON_ERROR);
}
 
if (state->check_index && read_apply_cache(state) < 0) {
@@ -4911,7 +4907,7 @@ int apply_all_patches(struct apply_state *state,
}
 
if (state->update_index) {
-   res = write_locked_index(_index, state->lock_file, 
COMMIT_LOCK);
+   res = write_locked_index(_index, >lock_file, 
COMMIT_LOCK);
if (res) {
error(_("Unable to write new index file"));
res = -128;
@@ -4924,7 +4920,7 @@ int apply_all_patches(struct apply_state *state,
 
 end:
if (state->newfd >= 0) {
-   rollback_lock_file(state->lock_file);
+   rollback_lock_file(>lock_file);
state->newfd = -1;
}
 
diff --git a/apply.h b/apply.h
index d9b395770..cf00cda17 100644
--- a/apply.h
+++ b/apply.h
@@ -37,7 +37,7 @@ struct apply_state {
const char *prefix;
 
/* These are lock_file related */
-   struct lock_file *lock_file;
+   struct lock_file lock_file;
int newfd;
 
/* These control what gets looked at and modified */
@@ -116,8 +116,7 @@ extern int apply_parse_options(int argc, const char **argv,
   int *force_apply, int *options,
   const char * const *apply_usage);
 extern int init_apply_state(struct apply_state *state,
-   const char *prefix,
-   struct 

[PATCH v3 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag

[Martin Ågren]

`write_locked_index()` takes two flags: `COMMIT_LOCK` and `CLOSE_LOCK`.
At most one is allowed. But it is also possible to use no flag, i.e.,
`0`. But when `write_locked_index()` calls `do_write_index()`, the
temporary file, a.k.a. the lockfile, will be closed. So passing `0` is
effectively the same as `CLOSE_LOCK`, which seems like a bug.

We might feel tempted to restructure the code in order to close the file
later, or conditionally. It also feels a bit unfortunate that we simply
"happen" to close the lock by way of an implementation detail of
lockfiles. But note that we need to close the temporary file before
`stat`-ing it, at least on Windows. See 9f41c7a6b (read-cache: close
index.lock in do_write_index, 2017-04-26).

Drop `CLOSE_LOCK` and make it explicit that `write_locked_index()`
always closes the lock. Whether it is also committed is governed by the
remaining flag, `COMMIT_LOCK`.

This means we neither have nor suggest that we have a mode to write the
index and leave the file open. Whatever extra contents we might
eventually want to write, we should probably write it from within
`write_locked_index()` itself anyway.

Signed-off-by: Martin Ågren 
---
v3: Dropped the comment in `do_write_locked_index()` and documented how
`do_write_index()` handles the temporary file instead.

 builtin/commit.c | 10 +-
 cache.h  |  5 ++---
 read-cache.c | 14 --
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0f8ddb686..32dc2101f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -355,7 +355,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
 
refresh_cache_or_die(refresh_flags);
 
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to create temporary index"));
 
old_index_env = getenv(INDEX_ENVIRONMENT);
@@ -374,7 +374,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
if (reopen_lock_file(_lock) < 0)
die(_("unable to write index file"));
-   if (write_locked_index(_index, _lock, 
CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to update temporary index"));
} else
warning(_("Failed to update main cache tree"));
@@ -401,7 +401,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
add_files_to_cache(also ? prefix : NULL, , 0);
refresh_cache_or_die(refresh_flags);
update_main_cache_tree(WRITE_TREE_SILENT);
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to write new_index file"));
commit_style = COMMIT_NORMAL;
ret = get_lock_file_path(_lock);
@@ -474,7 +474,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
add_remove_files();
refresh_cache(REFRESH_QUIET);
update_main_cache_tree(WRITE_TREE_SILENT);
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to write new_index file"));
 
hold_lock_file_for_update(_lock,
@@ -486,7 +486,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
add_remove_files();
refresh_cache(REFRESH_QUIET);
 
-   if (write_locked_index(_index, _lock, CLOSE_LOCK))
+   if (write_locked_index(_index, _lock, 0))
die(_("unable to write temporary index file"));
 
discard_cache();
diff --git a/cache.h b/cache.h
index e9d9556e3..21a6856c5 100644
--- a/cache.h
+++ b/cache.h
@@ -604,11 +604,10 @@ extern int read_index_unmerged(struct index_state *);
 
 /* For use with `write_locked_index()`. */
 #define COMMIT_LOCK(1 << 0)
-#define CLOSE_LOCK (1 << 1)
 
 /*
- * Write the index while holding an already-taken lock. The flags may
- * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.
+ * Write the index while holding an already-taken lock. Close the lock,
+ * and if `COMMIT_LOCK` is given, commit it.
  *
  * Unless a split index is in use, write the index into the lockfile.
  *
diff --git a/read-cache.c b/read-cache.c
index 65f4fe837..c7aa3632a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2187,6 +2187,13 @@ void update_index_if_able(struct index_state *istate, 
struct lock_file *lockfile
rollback_lock_file(lockfile);
 }
 
+/*
+ * On success, `tempfile` is closed. If it is the temporary file
+ * of a `struct 

[PATCH v3 08/12] apply: remove `newfd` from `struct apply_state`

[Martin Ågren]

Similar to a previous patch, we do not need to use `newfd` to signal
that we have a lockfile to clean up. We can just unconditionally call
`rollback_lock_file`. If we do not hold the lock, it will be a no-op.

Where we check `newfd` to decide whether we need to take the lock, we
can instead use `is_lock_file_locked()`.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 apply.c | 17 ++---
 apply.h |  3 +--
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/apply.c b/apply.c
index 5a6ca10a7..d676debd5 100644
--- a/apply.c
+++ b/apply.c
@@ -79,7 +79,6 @@ int init_apply_state(struct apply_state *state,
 {
memset(state, 0, sizeof(*state));
state->prefix = prefix;
-   state->newfd = -1;
state->apply = 1;
state->line_termination = '\n';
state->p_value = 1;
@@ -4705,13 +4704,13 @@ static int apply_patch(struct apply_state *state,
state->apply = 0;
 
state->update_index = state->check_index && state->apply;
-   if (state->update_index && state->newfd < 0) {
+   if (state->update_index && !is_lock_file_locked(>lock_file)) {
if (state->index_file)
-   state->newfd = 
hold_lock_file_for_update(>lock_file,
-
state->index_file,
-
LOCK_DIE_ON_ERROR);
+   hold_lock_file_for_update(>lock_file,
+ state->index_file,
+ LOCK_DIE_ON_ERROR);
else
-   state->newfd = hold_locked_index(>lock_file, 
LOCK_DIE_ON_ERROR);
+   hold_locked_index(>lock_file, LOCK_DIE_ON_ERROR);
}
 
if (state->check_index && read_apply_cache(state) < 0) {
@@ -4913,16 +4912,12 @@ int apply_all_patches(struct apply_state *state,
res = -128;
goto end;
}
-   state->newfd = -1;
}
 
res = !!errs;
 
 end:
-   if (state->newfd >= 0) {
-   rollback_lock_file(>lock_file);
-   state->newfd = -1;
-   }
+   rollback_lock_file(>lock_file);
 
if (state->apply_verbosity <= verbosity_silent) {
set_error_routine(state->saved_error_routine);
diff --git a/apply.h b/apply.h
index cf00cda17..dc4a01905 100644
--- a/apply.h
+++ b/apply.h
@@ -36,9 +36,8 @@ enum apply_verbosity {
 struct apply_state {
const char *prefix;
 
-   /* These are lock_file related */
+   /* Lock file */
struct lock_file lock_file;
-   int newfd;
 
/* These control what gets looked at and modified */
int apply; /* this is not a dry-run */
-- 
2.15.0.rc0

[PATCH v3 12/12] read_cache: roll back lock in `update_index_if_able()`

[Martin Ågren]

`update_index_if_able()` used to always commit the lock or roll it back.
Commit 03b866477 (read-cache: new API write_locked_index instead of
write_index/write_cache, 2014-06-13) stopped rolling it back in case a
write was not even attempted. This change in behavior is not motivated
in the commit message and appears to be accidental: the `else`-path was
removed, although that changed the behavior in case the `if` shortcuts.

Reintroduce the rollback and document this behavior. While at it, move
the documentation on this function from the function definition to the
function declaration in cache.h.

If `write_locked_index(..., COMMIT_LOCK)` fails, it will roll back the
lock for us (see the previous commit).

Noticed-by: Junio C Hamano 
Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 cache.h  | 4 
 read-cache.c | 5 ++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 0e26224b9..8c2493766 100644
--- a/cache.h
+++ b/cache.h
@@ -734,6 +734,10 @@ extern void fill_stat_cache_info(struct cache_entry *ce, 
struct stat *st);
 extern int refresh_index(struct index_state *, unsigned int flags, const 
struct pathspec *pathspec, char *seen, const char *header_msg);
 extern struct cache_entry *refresh_cache_entry(struct cache_entry *, unsigned 
int);
 
+/*
+ * Opportunistically update the index but do not complain if we can't.
+ * The lockfile is always committed or rolled back.
+ */
 extern void update_index_if_able(struct index_state *, struct lock_file *);
 
 extern int hold_locked_index(struct lock_file *, int);
diff --git a/read-cache.c b/read-cache.c
index 0d8d2dede..87188d390 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2176,14 +2176,13 @@ static int has_racy_timestamp(struct index_state 
*istate)
return 0;
 }
 
-/*
- * Opportunistically update the index but do not complain if we can't
- */
 void update_index_if_able(struct index_state *istate, struct lock_file 
*lockfile)
 {
if ((istate->cache_changed || has_racy_timestamp(istate)) &&
verify_index(istate))
write_locked_index(istate, lockfile, COMMIT_LOCK);
+   else
+   rollback_lock_file(lockfile);
 }
 
 /*
-- 
2.15.0.rc0

[PATCH v3 09/12] cache.h: document `write_locked_index()`

[Martin Ågren]

The next patches will tweak the behavior of this function. Document it
in order to establish a basis for those patches.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 cache.h | 16 
 1 file changed, 16 insertions(+)

diff --git a/cache.h b/cache.h
index ea6c236e0..e9d9556e3 100644
--- a/cache.h
+++ b/cache.h
@@ -601,9 +601,25 @@ extern int do_read_index(struct index_state *istate, const 
char *path,
 extern int read_index_from(struct index_state *, const char *path);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
+
+/* For use with `write_locked_index()`. */
 #define COMMIT_LOCK(1 << 0)
 #define CLOSE_LOCK (1 << 1)
+
+/*
+ * Write the index while holding an already-taken lock. The flags may
+ * contain at most one of `COMMIT_LOCK` and `CLOSE_LOCK`.
+ *
+ * Unless a split index is in use, write the index into the lockfile.
+ *
+ * With a split index, write the shared index to a temporary file,
+ * adjust its permissions and rename it into place, then write the
+ * split index to the lockfile. If the temporary file for the shared
+ * index cannot be created, fall back to the behavior described in
+ * the previous paragraph.
+ */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
+
 extern int discard_index(struct index_state *);
 extern void move_index_extensions(struct index_state *dst, struct index_state 
*src);
 extern int unmerged_index(const struct index_state *);
-- 
2.15.0.rc0

[PATCH v3 01/12] sha1_file: do not leak `lock_file`

[Martin Ågren]

There is no longer any need to allocate and leak a `struct lock_file`.
Initialize it on the stack instead.

Before this patch, we set `lock = NULL` to signal that we have already
rolled back, and that we should not do any more work. We need to take
another approach now that we cannot assign NULL. We could, e.g., use
`is_lock_file_locked()`. But we already have another variable that we
could use instead, `found`. Its scope is only too small.

Bump `found` to the scope of the whole function and rearrange the "roll
back or write?"-checks to a straightforward if-else on `found`. This
also future-proves the code by making it obvious that we intend to take
exactly one of these paths.

Improved-by: Jeff King 
Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 sha1_file.c | 19 ---
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 5a2014811..1d1747099 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -456,19 +456,19 @@ struct alternate_object_database *alloc_alt_odb(const 
char *dir)
 
 void add_to_alternates_file(const char *reference)
 {
-   struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+   struct lock_file lock = LOCK_INIT;
char *alts = git_pathdup("objects/info/alternates");
FILE *in, *out;
+   int found = 0;
 
-   hold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR);
-   out = fdopen_lock_file(lock, "w");
+   hold_lock_file_for_update(, alts, LOCK_DIE_ON_ERROR);
+   out = fdopen_lock_file(, "w");
if (!out)
die_errno("unable to fdopen alternates lockfile");
 
in = fopen(alts, "r");
if (in) {
struct strbuf line = STRBUF_INIT;
-   int found = 0;
 
while (strbuf_getline(, in) != EOF) {
if (!strcmp(reference, line.buf)) {
@@ -480,18 +480,15 @@ void add_to_alternates_file(const char *reference)
 
strbuf_release();
fclose(in);
-
-   if (found) {
-   rollback_lock_file(lock);
-   lock = NULL;
-   }
}
else if (errno != ENOENT)
die_errno("unable to read alternates file");
 
-   if (lock) {
+   if (found) {
+   rollback_lock_file();
+   } else {
fprintf_or_die(out, "%s\n", reference);
-   if (commit_lock_file(lock))
+   if (commit_lock_file())
die_errno("unable to move new alternates file into 
place");
if (alt_odb_tail)
link_alt_odb_entries(reference, '\n', NULL, 0);
-- 
2.15.0.rc0

[PATCH v3 02/12] treewide: prefer lockfiles on the stack

[Martin Ågren]

There is no longer any need to allocate and leak a `struct lock_file`.
The previous patch addressed an instance where we needed a minor tweak
alongside the trivial changes.

Deal with the remaining instances where we allocate and leak a struct
within a single function. Change them to have the `struct lock_file` on
the stack instead.

These instances were identified by running `git grep "^\s*struct
lock_file\s*\*"`.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 builtin/am.c   | 24 +++-
 builtin/checkout.c | 14 ++
 builtin/clone.c|  7 +++
 builtin/diff.c |  7 +++
 config.c   | 17 -
 merge-recursive.c  |  6 +++---
 merge.c|  8 
 wt-status.c|  8 
 8 files changed, 42 insertions(+), 49 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d7513f537..4e16fd428 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1134,11 +1134,11 @@ static const char *msgnum(const struct am_state *state)
  */
 static void refresh_and_write_cache(void)
 {
-   struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
+   struct lock_file lock_file = LOCK_INIT;
 
-   hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
-   if (write_locked_index(_index, lock_file, COMMIT_LOCK))
+   if (write_locked_index(_index, _file, COMMIT_LOCK))
die(_("unable to write index file"));
 }
 
@@ -1946,15 +1946,14 @@ static void am_resolve(struct am_state *state)
  */
 static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
 {
-   struct lock_file *lock_file;
+   struct lock_file lock_file = LOCK_INIT;
struct unpack_trees_options opts;
struct tree_desc t[2];
 
if (parse_tree(head) || parse_tree(remote))
return -1;
 
-   lock_file = xcalloc(1, sizeof(struct lock_file));
-   hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
 
refresh_cache(REFRESH_QUIET);
 
@@ -1970,11 +1969,11 @@ static int fast_forward_to(struct tree *head, struct 
tree *remote, int reset)
init_tree_desc([1], remote->buffer, remote->size);
 
if (unpack_trees(2, t, )) {
-   rollback_lock_file(lock_file);
+   rollback_lock_file(_file);
return -1;
}
 
-   if (write_locked_index(_index, lock_file, COMMIT_LOCK))
+   if (write_locked_index(_index, _file, COMMIT_LOCK))
die(_("unable to write new index file"));
 
return 0;
@@ -1986,15 +1985,14 @@ static int fast_forward_to(struct tree *head, struct 
tree *remote, int reset)
  */
 static int merge_tree(struct tree *tree)
 {
-   struct lock_file *lock_file;
+   struct lock_file lock_file = LOCK_INIT;
struct unpack_trees_options opts;
struct tree_desc t[1];
 
if (parse_tree(tree))
return -1;
 
-   lock_file = xcalloc(1, sizeof(struct lock_file));
-   hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
 
memset(, 0, sizeof(opts));
opts.head_idx = 1;
@@ -2005,11 +2003,11 @@ static int merge_tree(struct tree *tree)
init_tree_desc([0], tree->buffer, tree->size);
 
if (unpack_trees(1, t, )) {
-   rollback_lock_file(lock_file);
+   rollback_lock_file(_file);
return -1;
}
 
-   if (write_locked_index(_index, lock_file, COMMIT_LOCK))
+   if (write_locked_index(_index, _file, COMMIT_LOCK))
die(_("unable to write new index file"));
 
return 0;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 3345a0d16..34c3c5b61 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -247,7 +247,7 @@ static int checkout_paths(const struct checkout_opts *opts,
struct object_id rev;
struct commit *head;
int errs = 0;
-   struct lock_file *lock_file;
+   struct lock_file lock_file = LOCK_INIT;
 
if (opts->track != BRANCH_TRACK_UNSPECIFIED)
die(_("'%s' cannot be used with updating paths"), "--track");
@@ -275,9 +275,7 @@ static int checkout_paths(const struct checkout_opts *opts,
return run_add_interactive(revision, "--patch=checkout",
   >pathspec);
 
-   lock_file = xcalloc(1, sizeof(struct lock_file));
-
-   hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
if (read_cache_preload(>pathspec) < 0)
return error(_("index file corrupt"));
 
@@ -376,7 +374,7 @@ static int checkout_paths(const struct checkout_opts *opts,
}
errs |= finish_delayed_checkout();
 
-   if (write_locked_index(_index, lock_file, 

[PATCH v3 06/12] cache-tree: simplify locking logic

[Martin Ågren]

After we have taken the lock using `LOCK_DIE_ON_ERROR`, we know that
`newfd` is non-negative. So when we check for exactly that property
before calling `write_locked_index()`, the outcome is guaranteed.

If we write and commit successfully, we set `newfd = -1`, so that we can
later avoid calling `rollback_lock_file` on an already-committed lock.
But we might just as well unconditionally call `rollback_lock_file()` --
it will be a no-op if we have already committed.

All in all, we use `newfd` as a bool and the only benefit we get from it
is that we can avoid calling a no-op. Remove `newfd` so that we have one
variable less to reason about.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 cache-tree.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 71d092ed5..f646f5673 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -602,11 +602,11 @@ static struct cache_tree *cache_tree_find(struct 
cache_tree *it, const char *pat
 
 int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, 
const char *index_path, int flags, const char *prefix)
 {
-   int entries, was_valid, newfd;
+   int entries, was_valid;
struct lock_file lock_file = LOCK_INIT;
int ret = 0;
 
-   newfd = hold_lock_file_for_update(_file, index_path, 
LOCK_DIE_ON_ERROR);
+   hold_lock_file_for_update(_file, index_path, LOCK_DIE_ON_ERROR);
 
entries = read_index_from(index_state, index_path);
if (entries < 0) {
@@ -625,10 +625,7 @@ int write_index_as_tree(unsigned char *sha1, struct 
index_state *index_state, co
ret = WRITE_TREE_UNMERGED_INDEX;
goto out;
}
-   if (0 <= newfd) {
-   if (!write_locked_index(index_state, _file, 
COMMIT_LOCK))
-   newfd = -1;
-   }
+   write_locked_index(index_state, _file, COMMIT_LOCK);
/* Not being able to write is fine -- we are only interested
 * in updating the cache-tree part, and if the next caller
 * ends up using the old index with unupdated cache-tree part
@@ -650,8 +647,7 @@ int write_index_as_tree(unsigned char *sha1, struct 
index_state *index_state, co
hashcpy(sha1, index_state->cache_tree->oid.hash);
 
 out:
-   if (0 <= newfd)
-   rollback_lock_file(_file);
+   rollback_lock_file(_file);
return ret;
 }
 
-- 
2.15.0.rc0

[PATCH v3 05/12] checkout-index: simplify locking logic

[Martin Ågren]

`newfd` starts out negative. If we then take the lock, `newfd` will
become non-negative. We later check for exactly that property before
calling `write_locked_index()`. That is, we are simply using `newfd` as
a boolean to keep track of whether we took the lock or not. (We always
use `newfd` and `lock_file` together, so they really are mirroring each
other.)

Drop `newfd` and check with `is_lock_file_locked()` instead. While at
it, move the `static struct lock_file` into `cmd_checkout_index()` and
make it non-static. It is only used in this function, and after
076aa2cbd (tempfile: auto-allocate tempfiles on heap, 2017-09-05), we
can have lockfiles on the stack.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 builtin/checkout-index.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 39c8be05d..b0e78b819 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -129,8 +129,6 @@ static const char * const builtin_checkout_index_usage[] = {
NULL
 };
 
-static struct lock_file lock_file;
-
 static int option_parse_stage(const struct option *opt,
  const char *arg, int unset)
 {
@@ -150,7 +148,7 @@ static int option_parse_stage(const struct option *opt,
 int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 {
int i;
-   int newfd = -1;
+   struct lock_file lock_file = LOCK_INIT;
int all = 0;
int read_from_stdin = 0;
int prefix_length;
@@ -206,7 +204,7 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
if (index_opt && !state.base_dir_len && !to_tempfile) {
state.refresh_cache = 1;
state.istate = _index;
-   newfd = hold_locked_index(_file, LOCK_DIE_ON_ERROR);
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
}
 
/* Check out named files first */
@@ -251,7 +249,7 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
if (all)
checkout_all(prefix, prefix_length);
 
-   if (0 <= newfd &&
+   if (is_lock_file_locked(_file) &&
write_locked_index(_index, _file, COMMIT_LOCK))
die("Unable to write new index file");
return 0;
-- 
2.15.0.rc0

[PATCH v3 11/12] read-cache: leave lock in right state in `write_locked_index()`

[Martin Ågren]

If the original version of `write_locked_index()` returned with an
error, it didn't roll back the lockfile unless the error occured at the
very end, during closing/committing. See commit 03b866477 (read-cache:
new API write_locked_index instead of write_index/write_cache,
2014-06-13).

In commit 9f41c7a6b (read-cache: close index.lock in do_write_index,
2017-04-26), we learned to close the lock slightly earlier in the
callstack. That was mostly a side-effect of lockfiles being implemented
using temporary files, but didn't cause any real harm.

Recently, commit 076aa2cbd (tempfile: auto-allocate tempfiles on heap,
2017-09-05) introduced a subtle bug. If the temporary file is deleted
(i.e., the lockfile is rolled back), the tempfile-pointer in the `struct
lock_file` will be left dangling. Thus, an attempt to reuse the
lockfile, or even just to roll it back, will induce undefined behavior
-- most likely a crash.

Besides not crashing, we clearly want to make things consistent. The
guarantees which the lockfile-machinery itself provides is A) if we ask
to commit and it fails, roll back, and B) if we ask to close and it
fails, do _not_ roll back. Let's do the same for consistency.

Do not delete the temporary file in `do_write_index()`. One of its
callers, `write_locked_index()` will thereby avoid rolling back the
lock. The other caller, `write_shared_index()`, will delete its
temporary file anyway. Both of these callers will avoid undefined
behavior (crashing).

Teach `write_locked_index(..., COMMIT_LOCK)` to roll back the lock
before returning. If we have already succeeded and committed, it will be
a noop. Simplify the existing callers where we now have a superfluous
call to `rollback_lockfile()`. That should keep future readers from
wondering why the callers are inconsistent.

Signed-off-by: Martin Ågren 
---
v3: Maybe the commit message wasn't too long, but it was loong. For
example, some of it just duplicated stuff from the previous patch.

 builtin/difftool.c |  1 -
 cache.h|  4 
 merge.c|  4 +---
 read-cache.c   | 14 --
 sequencer.c|  1 -
 5 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index b2d3ba753..bcc79d188 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -616,7 +616,6 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
if (hold_lock_file_for_update(, buf.buf, 0) < 0 ||
write_locked_index(, , COMMIT_LOCK)) {
ret = error("could not write %s", buf.buf);
-   rollback_lock_file();
goto finish;
}
changed_files(_modified, buf.buf, workdir);
diff --git a/cache.h b/cache.h
index 21a6856c5..0e26224b9 100644
--- a/cache.h
+++ b/cache.h
@@ -616,6 +616,10 @@ extern int read_index_unmerged(struct index_state *);
  * split index to the lockfile. If the temporary file for the shared
  * index cannot be created, fall back to the behavior described in
  * the previous paragraph.
+ *
+ * With `COMMIT_LOCK`, the lock is always committed or rolled back.
+ * Without it, the lock is closed, but neither committed nor rolled
+ * back.
  */
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
 
diff --git a/merge.c b/merge.c
index a18a452b5..e5d796c9f 100644
--- a/merge.c
+++ b/merge.c
@@ -91,9 +91,7 @@ int checkout_fast_forward(const struct object_id *head,
}
if (unpack_trees(nr_trees, t, ))
return -1;
-   if (write_locked_index(_index, _file, COMMIT_LOCK)) {
-   rollback_lock_file(_file);
+   if (write_locked_index(_index, _file, COMMIT_LOCK))
return error(_("unable to write new index file"));
-   }
return 0;
 }
diff --git a/read-cache.c b/read-cache.c
index c7aa3632a..0d8d2dede 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2182,9 +2182,8 @@ static int has_racy_timestamp(struct index_state *istate)
 void update_index_if_able(struct index_state *istate, struct lock_file 
*lockfile)
 {
if ((istate->cache_changed || has_racy_timestamp(istate)) &&
-   verify_index(istate) &&
-   write_locked_index(istate, lockfile, COMMIT_LOCK))
-   rollback_lock_file(lockfile);
+   verify_index(istate))
+   write_locked_index(istate, lockfile, COMMIT_LOCK);
 }
 
 /*
@@ -2321,7 +2320,6 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
return -1;
if (close_tempfile_gently(tempfile)) {
error(_("could not close '%s'"), tempfile->filename.buf);
-   delete_tempfile();
return -1;
}
if (stat(tempfile->filename.buf, ))
@@ -2501,7 +2499,8 @@ int write_locked_index(struct index_state *istate, 

[PATCH v3 04/12] tempfile: fix documentation on `delete_tempfile()`

[Martin Ågren]

The function has always been documented as returning 0 or -1. It is in
fact `void`. Correct that. As part of the rearrangements we lose the
mention that `delete_tempfile()` might set `errno`. Because there is
no return value, the user can't really know whether it did anyway.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 tempfile.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tempfile.h b/tempfile.h
index b8f4b5e14..450908b2e 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -68,10 +68,10 @@
  * `create_tempfile()` returns an allocated tempfile on success or NULL
  * on failure. On errors, `errno` describes the reason for failure.
  *
- * `delete_tempfile()`, `rename_tempfile()`, and `close_tempfile_gently()`
- * return 0 on success. On failure they set `errno` appropriately and return
- * -1. `delete` and `rename` (but not `close`) do their best to delete the
- * temporary file before returning.
+ * `rename_tempfile()` and `close_tempfile_gently()` return 0 on success.
+ * On failure they set `errno` appropriately and return -1.
+ * `delete_tempfile()` and `rename` (but not `close`) do their best to
+ * delete the temporary file before returning.
  */
 
 struct tempfile {
-- 
2.15.0.rc0

[PATCH v3 00/12] Re: various lockfile-leaks and -fixes

[Martin Ågren]

On 6 October 2017 at 21:44, Martin Ågren  wrote:
> Ok, thanks. I've got a rerolled series running through the final checks
> right now. I did end up making this log message a bit more succinct.

This is v3 of this series. All patches are identical to
ma/lockfile-fixes in Junio's tree, except 10 and 11.

Martin Ågren (12):
  sha1_file: do not leak `lock_file`
  treewide: prefer lockfiles on the stack
  lockfile: fix documentation on `close_lock_file_gently()`
  tempfile: fix documentation on `delete_tempfile()`
  checkout-index: simplify locking logic
  cache-tree: simplify locking logic
  apply: move lockfile into `apply_state`
  apply: remove `newfd` from `struct apply_state`
  cache.h: document `write_locked_index()`
  read-cache: drop explicit `CLOSE_LOCK`-flag
  read-cache: leave lock in right state in `write_locked_index()`
  read_cache: roll back lock in `update_index_if_able()`

 apply.c  | 25 -
 apply.h  |  8 +++-
 builtin/am.c | 27 ---
 builtin/apply.c  |  4 +---
 builtin/checkout-index.c |  8 +++-
 builtin/checkout.c   | 14 ++
 builtin/clone.c  |  7 +++
 builtin/commit.c | 10 +-
 builtin/diff.c   |  7 +++
 builtin/difftool.c   |  1 -
 cache-tree.c | 12 
 cache.h  | 25 -
 config.c | 17 -
 lockfile.h   |  4 ++--
 merge-recursive.c|  6 +++---
 merge.c  |  8 +++-
 read-cache.c | 31 +--
 sequencer.c  |  1 -
 sha1_file.c  | 19 ---
 tempfile.h   |  8 
 wt-status.c  |  8 
 21 files changed, 121 insertions(+), 129 deletions(-)

-- 
2.15.0.rc0

[PATCH v3 03/12] lockfile: fix documentation on `close_lock_file_gently()`

[Martin Ågren]

Commit 83a3069a3 (lockfile: do not rollback lock on failed close,
2017-09-05) forgot to update the documentation by the function definition
to reflect that the lock is not rolled back in case closing fails.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 lockfile.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lockfile.h b/lockfile.h
index 7c1c484d7..f401c979f 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -240,8 +240,8 @@ extern char *get_locked_file_path(struct lock_file *lk);
  * If the lockfile is still open, close it (and the file pointer if it
  * has been opened using `fdopen_lock_file()`) without renaming the
  * lockfile over the file being locked. Return 0 upon success. On
- * failure to `close(2)`, return a negative value and roll back the
- * lock file. Usually `commit_lock_file()`, `commit_lock_file_to()`,
+ * failure to `close(2)`, return a negative value (the lockfile is not
+ * rolled back). Usually `commit_lock_file()`, `commit_lock_file_to()`,
  * or `rollback_lock_file()` should eventually be called.
  */
 static inline int close_lock_file_gently(struct lock_file *lk)
-- 
2.15.0.rc0

Re: [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`

[Martin Ågren]

On 6 October 2017 at 14:02, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> On 6 October 2017 at 04:01, Junio C Hamano  wrote:
>>> Martin Ågren  writes:
>>>
 v2: Except for the slightly different documentation in cache.h, this is
 a squash of the last two patches of v1. I hope the commit message is
 better.
>>>
>>> Yeah, it is long ;-) but readable.
>>
>> "Long but readable"... Yeah. When I rework the previous patch (document
>> the closing-behavior of `do_write_index()`) I could address this. I
>> think there are several interesting details here and I'm not sure which
>> I'd want to leave out, but yeah, they add up...
>
> I didn't mean "long is bad" at all in this case.
>
> Certainly, from time to time we find commits with overlong
> explanation that only states obvious, and they are "long and bad".
> But I do not think this one falls into the same category as those.

Ok, thanks. I've got a rerolled series running through the final checks
right now. I did end up making this log message a bit more succinct.

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Martin Ågren]

On 6 October 2017 at 14:09, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> Maybe I'm missing something Git-specific, but isn't the only thing that
>> needs to be done now, to document/specify that 1) the client should send
>> its list ordered by preference, 2) how preference is signalled, and 3)
>> that the server gets to choose?
>
> I think Simon's reminder of Stefan's was about specifying something
> different from (1) above---it was just a list of good ones (as
> opposed to ones to be avoided).  I was suggesting to tweak that to
> match what you wrote above.

I replied to your mail, but also to the general notion of "we need a
carefully designed configuration and fall-back strategy before we can
include this series" which I sensed (and which I hope I didn't just
misrepresent). I didn't make it very clear exactly what I was replying
to, sorry about that.

Note that my 1-3 above are not about "configuration", which I interpret
as "how does the user tell Git how to select a protocol version?", but
about the protocol, i.e., "how do the client and server agree on which
version to use?".

>> Why would a server operator with only v0 and v1 at their disposal want
>> to choose v0 instead of v1, considering that -- as far as I understand
>> -- they are in fact the same?
>
> Because we may later discover some reason we not yet know that makes
> v$n+1 unsuitable after we introduce it, and we need to avoid it by
> preferring v$n instead?

For n in general, I agree completely. For n=0, and in particular for a
Git which only knows v0 and v1, I'm not so sure. If v1 has a problem
which v0 doesn't, then to the best of my understanding, that problem
would be in this series, i.e., in the version-negotiation. And to
minimize risk in that area, we'd want to make this series more complex?
That might be the correct thing to do -- certainly, if we botch this
series totally, then we might be in big trouble going forward --, but
it's not at all obvious to me. Nor is it obvious to me that such an
escape-hatch needs to be a multi-choice, prioritized configuration.

If a fall-back mechanism to v0 is wanted on the first Git server with
v1/v0, the simplest approach might be to make the server respect
protocol.version (possibly with a default of 1!?).

I might be naive in thinking that protocol.version could be removed or
redefined at our discretion just because it's marked as "experimental".

Martin

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Stefan Beller]

On Fri, Oct 6, 2017 at 12:22 PM, Jeff King  wrote:
> On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:
>
>> The `test_must_fail` should only be used to indicate a git command is
>> failing. `test_cmp` is not a git command, such that it doesn't need the
>> special treatment of `test_must_fail` (which e.g. includes checking for
>> segfault)
>
> Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
> And your patch is obviously an improvement, but I have to wonder if some
> of these make any sense.

Thanks for actually thinking about the problem. I just searched and replaced
the combination of test_must_fail and test_cmp mechanically after I noticed
one such occurrence whilst preparing the next patch (order of test_cmp args).

>
> If we're expecting some outcome, then it's reasonable to say:
>
>   1. The output should look exactly like this. (test_cmp)
>
>   2. The output should look something like this. (grep)
>
>   3. The output should _not_ mention this (! grep)
>
> But "the output should not look exactly like this" doesn't seem very
> robust. It's likely to give a false success due to small changes (or
> translations), or even bugs in the script.

I agree that the case "should not look like exactly this" is most likely
indicating a weak test.


> Running ./t3504 with "-v" (with or without your patch) shows:
>
>   --- expect2017-10-06 19:14:43.677840120 +
>   +++ foo   2017-10-06 19:14:43.705840120 +
>   @@ -1 +1 @@
>   -fatal: cherry-pick: --no-rerere-autoupdate cannot be used with --continue
>   +foo-dev
>
> Which just seems like a bug.  Did the original author mean foo-expect?

This was originally written by a non regular in 2008. I don't think
they remember even if we'd ask.

I think we'd want to not resolve it to foo-dev here,
(so ideally we'd test for


foo

foo-dev


but just testing that we do not blindly resolve to foo seems ok-ish)

Thanks for spotting this!

> It's hard to tell, as we are just reusing expectations from previous
> tests.
>
>> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
>> index 02106c9226..7178b917ce 100755
>> --- a/t/t5512-ls-remote.sh
>> +++ b/t/t5512-ls-remote.sh
>> @@ -56,7 +56,7 @@ test_expect_success 'use "origin" when no remote 
>> specified' '
>>
>>  test_expect_success 'suppress "From " with -q' '
>>   git ls-remote -q 2>actual_err &&
>> - test_must_fail test_cmp exp_err actual_err
>> + ! test_cmp exp_err actual_err
>>  '
>
> This one seems like "test_18ngrep ! ^From" would be more appropriate. Or
> even "test_must_be_empty".

Going by the test title, I agree.

>> diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
>> index fac5a73851..5f9ad51929 100755
>> --- a/t/t5612-clone-refspec.sh
>> +++ b/t/t5612-clone-refspec.sh
>> @@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept 
>> updated' '
>>   git for-each-ref refs/tags >../actual
>>   ) &&
>>   git for-each-ref refs/tags >expect &&
>> - test_must_fail test_cmp expect actual &&
>> + ! test_cmp expect actual &&
>>   test_line_count = 2 actual
>
> Here we check that no updates happened due to a fetch because we see
> that the tags in the fetched repo do not match the tags in the parent
> repo. That actually seems pretty legitimate. But I think:
>
>   git for-each-ref refs/tags >before
>   git fetch
>   git for-each-ref refs/tags >after
>   test_cmp before after
>
> would be more straightforward.
>
>> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
>> index 93f162a4f7..1644866571 100755
>> --- a/t/t7508-status.sh
>> +++ b/t/t7508-status.sh
>> @@ -1532,7 +1532,7 @@ test_expect_success '"status.branch=true" same as 
>> "-b"' '
>>  test_expect_success '"status.branch=true" different from "--no-branch"' '
>>   git status -s --no-branch  >expected_nobranch &&
>>   git -c status.branch=true status -s >actual &&
>> - test_must_fail test_cmp expected_nobranch actual
>> + ! test_cmp expected_nobranch actual
>>  '
>
> Shouldn't this be comparing it positively to the output with "--branch"?
>
>>  test_expect_success '"status.branch=true" weaker than "--no-branch"' '
>> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh 
>> b/t/t9164-git-svn-dcommit-concurrent.sh
>> index d8464d4218..5cd6b40432 100755
>> --- a/t/t9164-git-svn-dcommit-concurrent.sh
>> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
>> @@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a 
>> concurrent commit' '
>>   echo 1 >> file &&
>>   svn_cmd commit -m "changing file" &&
>>   svn_cmd up &&
>> - test_must_fail test_cmp auto_updated_file au_file_saved
>> + ! test_cmp auto_updated_file au_file_saved
>>   )
>>  '
>
> This one looked complicated, so I leave it as an exercise for the
> reader. :)

eh, I was hoping to not stirr up a controversy, but treating this as a
drive-by patch "making the tests a better 

Re: [RFC] Reporting index properties

[Alex Vandiver]

On Fri, 6 Oct 2017, Junio C Hamano wrote:
> Yes, and I am saying that such logic should not live in standard
> install outside developer tools ;-)

Fair enough.  It seems a little odd to me that git provides logic for
_altering_ those bits, but not to report on the state that can be so
altered.

 - Alex

Re: is there a truly compelling rationale for .git/info/exclude?

[Jonathan Nieder]

Hi,

Robert P. J. Day wrote:
> On Fri, 6 Oct 2017, Junio C Hamano wrote:

>> Don't waste time by seeking a "compelling" reason.  A mere "this is
>> the most expedite way to gain convenience" back when something was
>> introduced could be an answer, and it is way too late to complain
>> about such a choice anyway.
>
>   perfectly respectable answer ... it tells me that, between
> .gitignore files and core.excludesFile, there's not much left for
> .git/info/exclude to do, except in weird circumstances.

I use .git/info/exclude in what I don't consider to be weird
circumstances.

But I am not motivated to say more than that without knowing what my
answer is going to be used for.  E.g. is there a part of the
gitignore(5) man page where such an explanation would make it less
confusing and more useful?

Thanks,
Jonathan

Re: [PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Jeff King]

On Fri, Oct 06, 2017 at 12:00:05PM -0700, Stefan Beller wrote:

> The `test_must_fail` should only be used to indicate a git command is
> failing. `test_cmp` is not a git command, such that it doesn't need the
> special treatment of `test_must_fail` (which e.g. includes checking for
> segfault)

Hmph. "test_must_fail test_cmp" is a weird thing for somebody to write.
And your patch is obviously an improvement, but I have to wonder if some
of these make any sense.

If we're expecting some outcome, then it's reasonable to say:

  1. The output should look exactly like this. (test_cmp)

  2. The output should look something like this. (grep)

  3. The output should _not_ mention this (! grep)

But "the output should not look exactly like this" doesn't seem very
robust. It's likely to give a false success due to small changes (or
translations), or even bugs in the script.

For instance:

> diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
> index a267b2d144..c31141c471 100755
> --- a/t/t3504-cherry-pick-rerere.sh
> +++ b/t/t3504-cherry-pick-rerere.sh
> @@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more 
> than once' '
>  test_expect_success 'cherry-pick conflict without rerere' '
>   test_config rerere.enabled false &&
>   test_must_fail git cherry-pick master &&
> - test_must_fail test_cmp expect foo
> + ! test_cmp expect foo
>  '

Running ./t3504 with "-v" (with or without your patch) shows:

  --- expect2017-10-06 19:14:43.677840120 +
  +++ foo   2017-10-06 19:14:43.705840120 +
  @@ -1 +1 @@
  -fatal: cherry-pick: --no-rerere-autoupdate cannot be used with --continue
  +foo-dev

Which just seems like a bug.  Did the original author mean foo-expect?
It's hard to tell, as we are just reusing expectations from previous
tests.

> diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
> index 02106c9226..7178b917ce 100755
> --- a/t/t5512-ls-remote.sh
> +++ b/t/t5512-ls-remote.sh
> @@ -56,7 +56,7 @@ test_expect_success 'use "origin" when no remote specified' 
> '
>  
>  test_expect_success 'suppress "From " with -q' '
>   git ls-remote -q 2>actual_err &&
> - test_must_fail test_cmp exp_err actual_err
> + ! test_cmp exp_err actual_err
>  '

This one seems like "test_18ngrep ! ^From" would be more appropriate. Or
even "test_must_be_empty".

> diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
> index fac5a73851..5f9ad51929 100755
> --- a/t/t5612-clone-refspec.sh
> +++ b/t/t5612-clone-refspec.sh
> @@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept 
> updated' '
>   git for-each-ref refs/tags >../actual
>   ) &&
>   git for-each-ref refs/tags >expect &&
> - test_must_fail test_cmp expect actual &&
> + ! test_cmp expect actual &&
>   test_line_count = 2 actual

Here we check that no updates happened due to a fetch because we see
that the tags in the fetched repo do not match the tags in the parent
repo. That actually seems pretty legitimate. But I think:

  git for-each-ref refs/tags >before
  git fetch
  git for-each-ref refs/tags >after
  test_cmp before after

would be more straightforward.

> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
> index 93f162a4f7..1644866571 100755
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1532,7 +1532,7 @@ test_expect_success '"status.branch=true" same as "-b"' 
> '
>  test_expect_success '"status.branch=true" different from "--no-branch"' '
>   git status -s --no-branch  >expected_nobranch &&
>   git -c status.branch=true status -s >actual &&
> - test_must_fail test_cmp expected_nobranch actual
> + ! test_cmp expected_nobranch actual
>  '

Shouldn't this be comparing it positively to the output with "--branch"?

>  test_expect_success '"status.branch=true" weaker than "--no-branch"' '
> diff --git a/t/t9164-git-svn-dcommit-concurrent.sh 
> b/t/t9164-git-svn-dcommit-concurrent.sh
> index d8464d4218..5cd6b40432 100755
> --- a/t/t9164-git-svn-dcommit-concurrent.sh
> +++ b/t/t9164-git-svn-dcommit-concurrent.sh
> @@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a 
> concurrent commit' '
>   echo 1 >> file &&
>   svn_cmd commit -m "changing file" &&
>   svn_cmd up &&
> - test_must_fail test_cmp auto_updated_file au_file_saved
> + ! test_cmp auto_updated_file au_file_saved
>   )
>  '

This one looked complicated, so I leave it as an exercise for the
reader. :)

-Peff

[PATCH 1/2] tests: use shell negation instead of test_must_fail for test_cmp

[Stefan Beller]

The `test_must_fail` should only be used to indicate a git command is
failing. `test_cmp` is not a git command, such that it doesn't need the
special treatment of `test_must_fail` (which e.g. includes checking for
segfault)

Signed-off-by: Stefan Beller 
---
 t/t3504-cherry-pick-rerere.sh | 2 +-
 t/t5512-ls-remote.sh  | 2 +-
 t/t5612-clone-refspec.sh  | 2 +-
 t/t7508-status.sh | 2 +-
 t/t9164-git-svn-dcommit-concurrent.sh | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index a267b2d144..c31141c471 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -95,7 +95,7 @@ test_expect_success 'cherry-pick --rerere-autoupdate more 
than once' '
 test_expect_success 'cherry-pick conflict without rerere' '
test_config rerere.enabled false &&
test_must_fail git cherry-pick master &&
-   test_must_fail test_cmp expect foo
+   ! test_cmp expect foo
 '
 
 test_done
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 02106c9226..7178b917ce 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -56,7 +56,7 @@ test_expect_success 'use "origin" when no remote specified' '
 
 test_expect_success 'suppress "From " with -q' '
git ls-remote -q 2>actual_err &&
-   test_must_fail test_cmp exp_err actual_err
+   ! test_cmp exp_err actual_err
 '
 
 test_expect_success 'use branch..remote if possible' '
diff --git a/t/t5612-clone-refspec.sh b/t/t5612-clone-refspec.sh
index fac5a73851..5f9ad51929 100755
--- a/t/t5612-clone-refspec.sh
+++ b/t/t5612-clone-refspec.sh
@@ -87,7 +87,7 @@ test_expect_success 'by default no tags will be kept updated' 
'
git for-each-ref refs/tags >../actual
) &&
git for-each-ref refs/tags >expect &&
-   test_must_fail test_cmp expect actual &&
+   ! test_cmp expect actual &&
test_line_count = 2 actual
 '
 
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 93f162a4f7..1644866571 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1532,7 +1532,7 @@ test_expect_success '"status.branch=true" same as "-b"' '
 test_expect_success '"status.branch=true" different from "--no-branch"' '
git status -s --no-branch  >expected_nobranch &&
git -c status.branch=true status -s >actual &&
-   test_must_fail test_cmp expected_nobranch actual
+   ! test_cmp expected_nobranch actual
 '
 
 test_expect_success '"status.branch=true" weaker than "--no-branch"' '
diff --git a/t/t9164-git-svn-dcommit-concurrent.sh 
b/t/t9164-git-svn-dcommit-concurrent.sh
index d8464d4218..5cd6b40432 100755
--- a/t/t9164-git-svn-dcommit-concurrent.sh
+++ b/t/t9164-git-svn-dcommit-concurrent.sh
@@ -92,7 +92,7 @@ test_expect_success 'check if post-commit hook creates a 
concurrent commit' '
echo 1 >> file &&
svn_cmd commit -m "changing file" &&
svn_cmd up &&
-   test_must_fail test_cmp auto_updated_file au_file_saved
+   ! test_cmp auto_updated_file au_file_saved
)
 '
 
-- 
2.14.0.rc0.3.g6c2e499285

[PATCH 2/2] tests: fix diff order arguments in test_cmp

[Stefan Beller]

Fix the argument order for test_cmp. When given the expected
result first the diff shows the actual output with '+' and the
expectation with '-', which is the convention for our tests.

Signed-off-by: Stefan Beller 
---
 t/t1004-read-tree-m-u-wf.sh  |  2 +-
 t/t4015-diff-whitespace.sh   |  4 ++--
 t/t4205-log-pretty-formats.sh|  2 +-
 t/t6007-rev-list-cherry-pick-file.sh | 32 
 t/t6013-rev-list-reverse-parents.sh  |  4 ++--
 t/t7001-mv.sh|  2 +-
 t/t7005-editor.sh|  6 +++---
 t/t7102-reset.sh |  4 ++--
 t/t7201-co.sh|  4 ++--
 t/t7400-submodule-basic.sh   |  2 +-
 t/t7405-submodule-merge.sh   |  2 +-
 t/t7506-status-submodule.sh  |  4 ++--
 t/t7600-merge.sh |  6 +++---
 t/t7610-mergetool.sh |  4 ++--
 t/t9001-send-email.sh|  6 +++---
 15 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh
index c70cf42300..c7ce5d8bb5 100755
--- a/t/t1004-read-tree-m-u-wf.sh
+++ b/t/t1004-read-tree-m-u-wf.sh
@@ -218,7 +218,7 @@ test_expect_success 'D/F' '
echo "100644 $a 2   subdir/file2"
echo "100644 $b 3   subdir/file2/another"
) >expect &&
-   test_cmp actual expect
+   test_cmp expect actual
 
 '
 
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 12d182dc1b..1709e4578b 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -155,7 +155,7 @@ test_expect_success 'ignore-blank-lines: only new lines' '
 " >x &&
git diff --ignore-blank-lines >out &&
>expect &&
-   test_cmp out expect
+   test_cmp expect out
 '
 
 test_expect_success 'ignore-blank-lines: only new lines with space' '
@@ -165,7 +165,7 @@ test_expect_success 'ignore-blank-lines: only new lines 
with space' '
  " >x &&
git diff -w --ignore-blank-lines >out &&
>expect &&
-   test_cmp out expect
+   test_cmp expect out
 '
 
 test_expect_success 'ignore-blank-lines: after change' '
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index ec5f530102..42f584f8b3 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -590,7 +590,7 @@ test_expect_success '%(trailers:unfold) unfolds trailers' '
 test_expect_success ':only and :unfold work together' '
git log --no-walk --pretty="%(trailers:only:unfold)" >actual &&
git log --no-walk --pretty="%(trailers:unfold:only)" >reverse &&
-   test_cmp actual reverse &&
+   test_cmp reverse actual &&
{
grep -v patch.description  actual &&
git name-rev --stdin --name-only --refs="*tags/*" \
< actual > actual.named &&
-   test_cmp actual.named expect
+   test_cmp expect actual.named
 '
 
 test_expect_success '--count' '
@@ -77,14 +77,14 @@ test_expect_success '--cherry-pick bar does not come up 
empty' '
git rev-list --left-right --cherry-pick B...C -- bar > actual &&
git name-rev --stdin --name-only --refs="*tags/*" \
< actual > actual.named &&
-   test_cmp actual.named expect
+   test_cmp expect actual.named
 '
 
 test_expect_success 'bar does not come up empty' '
git rev-list --left-right B...C -- bar > actual &&
git name-rev --stdin --name-only --refs="*tags/*" \
< actual > actual.named &&
-   test_cmp actual.named expect
+   test_cmp expect actual.named
 '
 
 cat >expect < actual &&
git name-rev --stdin --name-only --refs="*tags/*" \
< actual > actual.named &&
-   test_cmp actual.named expect
+   test_cmp expect actual.named
 '
 
 test_expect_success 'name-rev multiple --refs combine inclusive' '
git rev-list --left-right --cherry-pick F...E -- bar >actual &&
git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
actual.named &&
-   test_cmp actual.named expect
+   test_cmp expect actual.named
 '
 
 cat >expect expect expect &&
git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" 
--no-refs --refs="*tags/G" \
actual &&
-   test_cmp actual expect
+   test_cmp expect actual
 '
 
 cat >expect < actual &&
git name-rev --stdin --name-only 

Re: Git config multiple values

[Andreas Heiduk]

Hi,

Am 06.10.2017 um 19:25 schrieb Jonathan Nieder:
> Hi,
> 
> Jeff King wrote:
>> On Fri, Oct 06, 2017 at 01:10:17PM +0200, aleksander.baranowski wrote:
> 
>>> It's just an opinion, but this behaviour is no consistent for me.
>>>
>>> If it's not the bug it's a feature just let me know.
>>
>> It's a feature, though I agree that git-config is rather baroque. We're
>> mostly stuck with it for reasons of backwards compatibility, though.
> 
> This feels like a dodge.  Can we make a list of what is baroque here,
> with an eye to fixing it?  E.g. if we introduce a new --set option,
> then what should its semantics be, to be more intuitive?


My reading of the manual for 

git config --global user.name Foo2 Bar

is this:

   Multiple lines can be added to an option by using the --add option.

Does not apply here - no `--add` option, so no new value should be added.

   If you want to update or unset an option which can occur on multiple
   lines, a POSIX regexp value_regex needs to be given. 

does not apply: First: no "--unset" variant is used here leaving only "update".
Second: before that command there is only one value.

   Only the existing values that match the regexp are updated or unset.

Since "Two" does not match the previous value and `update` is the only 
described case left I'd expect that the command changes nothing.I don't
understand how the description allows `git config` to add a new value,
because the manual talks about "update" twice, nothing about adding.


Confused
--Andreas

Re: git add --patch displays diff and exits

[Jake Bell]

Sorry, please disregard---this is a duplicate of some other messages already 
sent to the list. Setting "color.ui = auto" fixed the issue for me.


-- Jake

> On Oct 6, 2017, at 1:13 PM, Jake Bell  wrote:
> 
> Sorry, please disregard---this is a duplicate of some other messages already 
> sent to the list. Setting "color.ui = auto" fixed the issue for me.
> 
> 
> -- Jake
> 
>> On Oct 6, 2017, at 1:09 PM, Jake Bell  wrote:
>> 
>> Hello,
>> 
>> I'm not sure if this is the right place to report this issue, so apologies 
>> in advance if it isn't.
>> 
>> Recently, I upgraded from git 2.14.1 to 2.14.2, and noticed that "git add 
>> --patch" does not work properly. It just prints a diff of what's changed and 
>> exits, instead of presenting the dialogs to add changes to the index. I did 
>> some bisecting and tracked the bug down to this commit: 
>> https://github.com/git/git/commit/136c8c8b8fa39f1315713248473dececf20f8fe7.
>> 
>> Unfortunately, I'm not familiar with the codebase and don't know C well 
>> enough to debug the issue. Please let me know what additional information I 
>> can provide to help you all figure it out.
>> Thanks!
>> 
>> -- Jake
>> 
> 



signature.asc
Description: Message signed with OpenPGP

Re: git add --patch displays diff and exits

[Stefan Beller]

On Fri, Oct 6, 2017 at 11:09 AM, Jake Bell  wrote:
> Hello,
>
> I'm not sure if this is the right place to report this issue, so apologies in 
> advance if it isn't.
>
> Recently, I upgraded from git 2.14.1 to 2.14.2, and noticed that "git add 
> --patch" does not work properly. It just prints a diff of what's changed and 
> exits, instead of presenting the dialogs to add changes to the index. I did 
> some bisecting and tracked the bug down to this commit: 
> https://github.com/git/git/commit/136c8c8b8fa39f1315713248473dececf20f8fe7.
>
> Unfortunately, I'm not familiar with the codebase and don't know C well 
> enough to debug the issue. Please let me know what additional information I 
> can provide to help you all figure it out.
> Thanks!

Please see
https://public-inbox.org/git/20171003093157.gq7za2fwcqsou...@sigill.intra.peff.net/T/
and replace the color=always by color=auto setting to see if it fixes
your problem.
(If it doesn't we're dealing with a new problem and not the one
reported more than 5 times already :)

Thanks,
Stefan



>
> -- Jake
>

git add --patch displays diff and exits

[Jake Bell]

Hello,

I'm not sure if this is the right place to report this issue, so apologies in 
advance if it isn't.

Recently, I upgraded from git 2.14.1 to 2.14.2, and noticed that "git add 
--patch" does not work properly. It just prints a diff of what's changed and 
exits, instead of presenting the dialogs to add changes to the index. I did 
some bisecting and tracked the bug down to this commit: 
https://github.com/git/git/commit/136c8c8b8fa39f1315713248473dececf20f8fe7.

Unfortunately, I'm not familiar with the codebase and don't know C well enough 
to debug the issue. Please let me know what additional information I can 
provide to help you all figure it out.
Thanks!

-- Jake



signature.asc
Description: Message signed with OpenPGP

[PATCH v2 1/1] completion: add --broken and --dirty to describe

[Thomas Braun]

When the flags for broken and dirty were implemented in
b0176ce6b5 (builtin/describe: introduce --broken flag, 2017-03-21)
and 9f67d2e827 (Teach "git describe" --dirty option, 2009-10-21)
the completion was not updated, although these flags are useful
completions. Add them.

Signed-off-by: Thomas Braun 
Helped-by: Stefan Beller 
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d934417475..0e16f017a4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1385,7 +1385,7 @@ _git_describe ()
__gitcomp "
--all --tags --contains --abbrev= --candidates=
--exact-match --debug --long --match --always 
--first-parent
-   --exclude
+   --exclude --dirty --broken
"
return
esac
-- 
2.14.2.746.g8fb8a945bc.dirty

Re: Line ending normalization doesn't work as expected

[Torsten Bögershausen]

On Fri, Oct 06, 2017 at 09:33:31AM +0900, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
> > Before we put this into stone:
> > Does it make sense to say "renormalize" instead of "rehash" ?
> > (That term does exist already for merge.
> >  And rehash is more a technical term,  rather then a user-point-of-view 
> > explanation)
> 
> I do not mind "renormalize" at all.
> 
> As to the toy patch, I think it needs to (at least by default) turn
> off the add_new_files codepath, and be allowed to work without any
> pathspec (in which case all tracked paths should be renormalized).
> 

OK, then I will pick up your patch in a couple of days/weeks, and push it 
further then
(Documentation, test cases, other ?)

Re: [PATCH 1/1] completion: Add forgotten describe options

[Thomas Braun]

> Stefan Beller  hat am 6. Oktober 2017 um 00:17 
> geschrieben:
> On Thu, Oct 5, 2017 at 2:23 PM, Thomas Braun
>  wrote:

Hi Stefan,

> > completion: Add forgotten describe options
> 
> When/Why was it forgotten? git-blame thinks it was me in b0176ce6b5
> (builtin/describe: introduce --broken flag, 2017-03-21)
> Which do you add? (dirty and broken)
> 
> I had these questions when reading the subject (which is the
> equivalent of reading the output of `git log --oneline` in the future)
> I think a better wording might be
> 
> completion: add --broken and --dirty to describe
> 
> When the flags for broken and dirty were implemented in
> b0176ce6b5 (builtin/describe: introduce --broken flag, 2017-03-21)
> and 9f67d2e827 (Teach "git describe" --dirty option, 2009-10-21)
> the completion was not updated, although these flags are useful
> completions. Add them.

Thanks for the review. Your commit message is (obviously) much better than 
mine. Reroll follows.

> > ---
> 
> The patch looks correct.
> 

Thanks,
Thomas

Re: is there a truly compelling rationale for .git/info/exclude?

[Robert P. J. Day]

On Fri, 6 Oct 2017, Junio C Hamano wrote:

> rpj...@crashcourse.ca writes:
>
> >   at the other end, users are certainly welcome to add extra
> > patterns to be ignored, based purely on the way they work --
> > perhaps based on their choice of editor, they might want to
> > exclude *.swp files, or if working on a Mac, ignore .DS_Store, and
> > so on, using a core.excludesFile setting.
>
> This is primarily why .git/info/exclude exists.  A user who does not
> use the same set of tools to work on different projects may not be
> able to use ~/.gitconfig with core.excludesFile pointing at a single
> place that applies to _all_ repositories the user touches.
>
> Also, core.excludesFile came a lot later than in-project and
> in-repository exclude list, IIRC.
>
> Don't waste time by seeking a "compelling" reason.  A mere "this is
> the most expedite way to gain convenience" back when something was
> introduced could be an answer, and it is way too late to complain
> about such a choice anyway.

  perfectly respectable answer ... it tells me that, between
.gitignore files and core.excludesFile, there's not much left for
.git/info/exclude to do, except in weird circumstances.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: Git config multiple values

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Fri, Oct 06, 2017 at 01:10:17PM +0200, aleksander.baranowski wrote:

>> It's just an opinion, but this behaviour is no consistent for me.
>>
>> If it's not the bug it's a feature just let me know.
>
> It's a feature, though I agree that git-config is rather baroque. We're
> mostly stuck with it for reasons of backwards compatibility, though.

This feels like a dodge.  Can we make a list of what is baroque here,
with an eye to fixing it?  E.g. if we introduce a new --set option,
then what should its semantics be, to be more intuitive?

Thanks,
Jonathan

Re: Git config multiple values

[Jeff King]

On Fri, Oct 06, 2017 at 10:25:30AM -0700, Jonathan Nieder wrote:

> Jeff King wrote:
> > On Fri, Oct 06, 2017 at 01:10:17PM +0200, aleksander.baranowski wrote:
> 
> >> It's just an opinion, but this behaviour is no consistent for me.
> >>
> >> If it's not the bug it's a feature just let me know.
> >
> > It's a feature, though I agree that git-config is rather baroque. We're
> > mostly stuck with it for reasons of backwards compatibility, though.
> 
> This feels like a dodge.  Can we make a list of what is baroque here,
> with an eye to fixing it?  E.g. if we introduce a new --set option,
> then what should its semantics be, to be more intuitive?

Maybe baroque isn't the right word. But changing the function of a
command drastically based on the number of arguments seems like a source
of confusion.

I'm fine if somebody wants to champion a new "--set" option, but frankly
I'm not sure it's worth the pain at this point.

-Peff

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Jeff King]

On Fri, Oct 06, 2017 at 07:09:10PM +0200, Michael Haggerty wrote:

> I do have one twinge of uneasiness at a deeper level, that I haven't had
> time to check...
> 
> Does this patch make it easier to *set* HEAD to an unborn branch that
> d/f conflicts with an existing reference? If so, that might be a
> slightly worse UI for users. I'd rather learn about such a problem when
> setting HEAD (when I am thinking about the new branch name and am in the
> frame of mind to solve the problem) rather than later, when I try to
> commit to the new branch.

Good question. The answer is no, it's allowed both before and after my
patch. At least via git-symbolic-ref.

I agree it would be nice to know earlier for such a case. For
symbolic-ref, we probably should allow it, because it's plumbing that
may be used for tricky things. For things like "checkout -b", you'd
generally get a timely warning as we try to create the ref.

The odd man out is "checkout --orphan", which leaves the branch unborn.
It might be nice if it did a manual check that the ref is available (and
also that it's syntactically acceptable, though I think we may do that
already).

But all of that is orthogonal to this fix, I think.

-Peff

Re: [PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Michael Haggerty]

On 10/06/2017 04:42 PM, Jeff King wrote:
> If our call to refs_read_raw_ref() fails, we check errno to
> see if the ref is simply missing, or if we encountered a
> more serious error. If it's just missing, then in "write"
> mode (i.e., when RESOLVE_REFS_READING is not set), this is
> perfectly fine.
> 
> However, checking for ENOENT isn't sufficient to catch all
> missing-ref cases. In the filesystem backend, we may also
> see EISDIR when we try to resolve "a" and "a/b" exists.
> Likewise, we may see ENOTDIR if we try to resolve "a/b" and
> "a" exists. In both of those cases, we know that our
> resolved ref doesn't exist, but we return an error (rather
> than reporting the refname and returning a null sha1).
> 
> This has been broken for a long time, but nobody really
> noticed because the next step after resolving without the
> READING flag is usually to lock the ref and write it. But in
> both of those cases, the write will fail with the same
> errno due to the directory/file conflict.
> 
> There are two cases where we can notice this, though:
> 
>   1. If we try to write "a" and there's a leftover directory
>  already at "a", even though there is no ref "a/b". The
>  actual write is smart enough to move the empty "a" out
>  of the way.
> 
>  This is reasonably rare, if only because the writing
>  code has to do an independent resolution before trying
>  its write (because the actual update_ref() code handles
>  this case fine). The notes-merge code does this, and
>  before the fix in the prior commit t3308 erroneously
>  expected this case to fail.
> 
>   2. When resolving symbolic refs, we typically do not use
>  the READING flag because we want to resolve even
>  symrefs that point to unborn refs. Even if those unborn
>  refs could not actually be written because of d/f
>  conflicts with existing refs.
> 
>  You can see this by asking "git symbolic-ref" to report
>  the target of a symref pointing past a d/f conflict.
> 
> We can fix the problem by recognizing the other "missing"
> errnos and treating them like ENOENT. This should be safe to
> do even for callers who are then going to actually write the
> ref, because the actual writing process will fail if the d/f
> conflict is a real one (and t1404 checks these cases).
> 
> Arguably this should be the responsibility of the
> files-backend to normalize all "missing ref" errors into
> ENOENT (since something like EISDIR may not be meaningful at
> all to a database backend). However other callers of
> refs_read_raw_ref() may actually care about the distinction;
> putting this into resolve_ref() is the minimal fix for now.
> 
> The new tests in t1401 use git-symbolic-ref, which is the
> most direct way to check the resolution by itself.
> Interestingly we actually had a test that setup this case
> already, but we only used it to verify that the funny state
> could be overwritten, not that it could be resolved.
> 
> We also add a new test in t3200, as "branch -m" was the
> original motivation for looking into this. What happens is
> this:
> 
>   0. HEAD is pointing to branch "a"
> 
>   1. The user asks to rename "a" to "a/b".
> 
>   2. We create "a/b" and delete "a".
> 
>   3. We then try to update any worktree HEADs that point to
>  the renamed ref (including the main repo HEAD). To do
>  that, we have to resolve each HEAD. But now our HEAD is
>  pointing at "a", and we get EISDIR due to the loose
>  "a/b". As a result, we think there is no HEAD, and we
>  do not update it. It now points to the bogus "a".
> 
> Interestingly this case used to work, but only accidentally.
> Before 31824d180d (branch: fix branch renaming not updating
> HEADs correctly, 2017-08-24), we'd update any HEAD which we
> couldn't resolve. That was wrong, but it papered over the
> fact that we were incorrectly failing to resolve HEAD.
> 
> So while the bug demonstrated by the git-symbolic-ref is
> quite old, the regression to "branch -m" is recent.

Thanks for your detailed investigation and analysis and for the new tests.

This makes sense to me at the level of fixing the bug.

I do have one twinge of uneasiness at a deeper level, that I haven't had
time to check...

Does this patch make it easier to *set* HEAD to an unborn branch that
d/f conflicts with an existing reference? If so, that might be a
slightly worse UI for users. I'd rather learn about such a problem when
setting HEAD (when I am thinking about the new branch name and am in the
frame of mind to solve the problem) rather than later, when I try to
commit to the new branch.

Even if so, that wouldn't be a problem with this patch per se, but
rather a possible accidental side-effect of fixing the bug.

Michael

> [...]

Re: Git config multiple values

[aleksander.baranowski]

Hi!,

Thank you very much for descriptive answer.

You are absolutely right. I should read manual more carefully! Still
it's quite odd interface. Thank you for your time.

Bests,
Alex


On 10/06/2017 04:32 PM, Jeff King wrote:
> On Fri, Oct 06, 2017 at 01:10:17PM +0200, aleksander.baranowski wrote:
> 
>> I'm currently using git version 2.14.2. There is possible to put
>> multiple values into same variable with git config.
> 
> Yep, your examples should behave the same even with older versions.
> 
>> Case 1:
>> # git config --global user.name Foo - returns 0
>> # git config --global user.name Bar - returns 0 and replace Foo to Bar
>> # git config --global user.name Foo - returns 0 and replace Bar to Foo
> 
> This is all as expected. You're hitting the first case in the manpage
> synopsis here (I snipped the uninteresting options):
> 
>   git config name [value [value_regex]]
> 
> So you're doing:
> 
>   git config name value
> 
> which replaces any existing key by default. You could also do:
> 
>   git config --add name value
> 
> to add without replacing (if you had a config key that takes multiple
> values).
> 
>> Case 2:
>> # git config --global user.name Foo - returns 0
>> # git config --global user.name Foo2 Bar - returns 0 and put second name
> 
> Here you're doing:
> 
>   git config name value value_regex
> 
> So we're replacing any values which match the regex "Bar" (and there are
> none), and leaving others intact. Hence after this you will have two
> user.name values.
> 
> If you wanted a name with two strings, you'd have to quote the string
> from the shell to leave it as a single argument:
> 
>   git config user.name "Foo2 Bar"
> 
>> # cat ~/.gitconfig
>> [user]
>>  email = aleksander.baranow...@yahoo.pl
>>  name = Foo
>>  name = Foo2
> 
> Right, this is what I'd expect.
> 
>> # git config --global user.name Foo - return 5 and message
>> "warning: user.name has multiple values
>> error: cannot overwrite multiple values with a single value
>>Use a regexp, --add or --replace-all to change user.name."
> 
> And this, too (though I forgot we had such a safety check).
> 
>> It's just an opinion, but this behaviour is no consistent for me.
>>
>> If it's not the bug it's a feature just let me know.
> 
> It's a feature, though I agree that git-config is rather baroque. We're
> mostly stuck with it for reasons of backwards compatibility, though.
> 
> -Peff
> 

Re: [PATCH v2] fsck: handle NULL return of lookup_blob() and lookup_tree()

[René Scharfe]

Am 06.10.2017 um 04:23 schrieb Junio C Hamano:
> René Scharfe  writes:
>> +blob_bin=$(echo $blob | hex2oct) &&
>> +tree=$(
>> +printf "4 dir\0${blob_bin}100644 file\0${blob_bin}" |
> 
> Wow, that's ... cute.
> 
>> +git hash-object -t tree --stdin -w --literally
> 
> Makes me curious why --literally is here.  Even if we let
> check_tree() called from index_mem() by taking the normal path,
> it wouldn't complain the type mismatch, I suspect.  I guess doing it
> this way is a future-proof against check_tree() getting tightened in
> the future, in which case I think it makes sense.
> 
> And for the same reason, hashing "--literally" like this patch does
> is a better solution than using "git mktree", which would have
> allowed us to avoid the hex2oct and instead feed the tree in a bit
> more human-readable way.

git mktree errors out already, complaining about the object type
mismatch.  But I added "--literally" only accidentally, when I copied
the invocation from a few lines up.  The test works fine without that
flag currently.  The flag captures the intent, however, of knowingly
building a flawed tree object.

René

Re: git send-email does not work with Google anymore?!

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 05 2017, Lars Schneider jotted:

> Hi,
>
> I used to use the Google SMTP server to send my patches to the list with
> the following config:
>
> [sendemail]
> smtpencryption = tls
> smtpserver = smtp.gmail.com
> smtpuser = larsxschnei...@gmail.com
> smtpserverport = 587
> from = larsxschnei...@gmail.com
> chainreplyto = false
> suppresscc = self
>
> Apparently that stopped working today. I get this error:
>
> (mbox) Adding cc: Lars Schneider  from line 
> 'From: Lars Schneider '
> Password for 'smtp://larsxschnei...@gmail.com@smtp.gmail.com:587':
> 5.7.14  5.7.14 ...> Please log in via your web browser and
> 5.7.14 then try again.
> 5.7.14  Learn more at
> 5.7.14  https://support.google.com/mail/answer/78754 ... - gsmtp
>
> Of couse I tried to log in via web browser etc. Does anyone else use
> Google as SMTP server? If yes, does it work for you?

It still works for me. Just sent myself an E-Mail now via
git-send-email.

Password for 'smtp://ava...@gmail.com@smtp.gmail.com:465':
OK. Log says:
Server: smtp.gmail.com
[...]
Result: 250

My settings are:

[sendemail]
smtpserver = smtp.gmail.com
smtpEncryption = ssl
smtpuser = ava...@gmail.com
confirm = always

And my https://myaccount.google.com/apppasswords lists an app
password.

If you see this E-Mail from me that means my local Exim instance (which
also has an app password) is able to send E-Mail as well.

I have 2fa turned on on my Google account, I just have these app
passwords for GMail.

Re: [bug] git add -p breaks, if color.ui is set to "always"

[Kevin Daudt]

On Fri, Oct 06, 2017 at 02:47:30PM +0200, Alexander Gehrke wrote:
> After an update to version 2.14.2 from 2.14.1 "git add --patch" stopped 
> working
> for me, just producing the same output as "git diff", but not prompting to 
> stage
> anything.
> 
> I found that unsetting the config key color.ui, which was set to "always" 
> fixed
> the problem.
> 
> From the manpage, color.ui should not have that effect and "always" should be 
> a
> legal value.
> 
> Regards
> Alexander Gehrke

Hello Alexander,

There have been a few mailing-list posts[0] about this already. While
git add -p should probably not have broken by this, setting ui.color to
always itself does not make a lot of sense either.

You are telling git to always output color, even when the target is
something that does not know what to do with the color codes. Setting it
to 'auto' would make more sense.

The thread I posted to discusses some changes that might get introduced
to improve the situation though.

Kevin.

[0}:https://public-inbox.org/git/20171003093157.gq7za2fwcqsou...@sigill.intra.peff.net/T/

Re: git send-email does not work with Google anymore?!

[Christian Couder]

Hi,

On Thu, Oct 5, 2017 at 12:52 PM, Lars Schneider
 wrote:

> Of couse I tried to log in via web browser etc. Does anyone else use
> Google as SMTP server? If yes, does it work for you?

As I wrote at the top of:

https://public-inbox.org/git/cap8ufd2j-ufh+9awz91gtz-jusq7euoexmguro59vpf29jx...@mail.gmail.com/

I also got the same problem.

Re: git send-email does not work with Google anymore?!

[Kaartic Sivaraam]

On Thu, 2017-10-05 at 12:52 +0200, Lars Schneider wrote:
> Apparently that stopped working today. I get this error:
> 
> (mbox) Adding cc: Lars Schneider  from line 
> 'From: Lars Schneider '
> Password for 'smtp://larsxschnei...@gmail.com@smtp.gmail.com:587':
> 5.7.14  5.7.14 ...> Please log in via your web browser and
> 5.7.14 then try again.
> 5.7.14  Learn more at
> 5.7.14  https://support.google.com/mail/answer/78754 ... - gsmtp
> 
> Of couse I tried to log in via web browser etc. Does anyone else use 
> Google as SMTP server? If yes, does it work for you?
> 

I thought I was the only one having this problem. Seems, I'm not alone
;-) Until recently I was using the email address,

kaarticsivaraam91...@gmail.com

to send patches. Last week, I got the same error message you did and
tried to fix it in various ways but failed in my attempt; got fed up
and switched to another mail address I owned (the current one) using
which I'm able to send patches (surprise !). Fortune for me as, it
wasn't a big deal. I think this is an issue with Google.

(I still don't know when I'm going to get the error for this one. I
don't have ideas to change this one.)


-- 
Kaartic

[PATCH 2/2] refs_resolve_ref_unsafe: handle d/f conflicts for writes

[Jeff King]

If our call to refs_read_raw_ref() fails, we check errno to
see if the ref is simply missing, or if we encountered a
more serious error. If it's just missing, then in "write"
mode (i.e., when RESOLVE_REFS_READING is not set), this is
perfectly fine.

However, checking for ENOENT isn't sufficient to catch all
missing-ref cases. In the filesystem backend, we may also
see EISDIR when we try to resolve "a" and "a/b" exists.
Likewise, we may see ENOTDIR if we try to resolve "a/b" and
"a" exists. In both of those cases, we know that our
resolved ref doesn't exist, but we return an error (rather
than reporting the refname and returning a null sha1).

This has been broken for a long time, but nobody really
noticed because the next step after resolving without the
READING flag is usually to lock the ref and write it. But in
both of those cases, the write will fail with the same
errno due to the directory/file conflict.

There are two cases where we can notice this, though:

  1. If we try to write "a" and there's a leftover directory
 already at "a", even though there is no ref "a/b". The
 actual write is smart enough to move the empty "a" out
 of the way.

 This is reasonably rare, if only because the writing
 code has to do an independent resolution before trying
 its write (because the actual update_ref() code handles
 this case fine). The notes-merge code does this, and
 before the fix in the prior commit t3308 erroneously
 expected this case to fail.

  2. When resolving symbolic refs, we typically do not use
 the READING flag because we want to resolve even
 symrefs that point to unborn refs. Even if those unborn
 refs could not actually be written because of d/f
 conflicts with existing refs.

 You can see this by asking "git symbolic-ref" to report
 the target of a symref pointing past a d/f conflict.

We can fix the problem by recognizing the other "missing"
errnos and treating them like ENOENT. This should be safe to
do even for callers who are then going to actually write the
ref, because the actual writing process will fail if the d/f
conflict is a real one (and t1404 checks these cases).

Arguably this should be the responsibility of the
files-backend to normalize all "missing ref" errors into
ENOENT (since something like EISDIR may not be meaningful at
all to a database backend). However other callers of
refs_read_raw_ref() may actually care about the distinction;
putting this into resolve_ref() is the minimal fix for now.

The new tests in t1401 use git-symbolic-ref, which is the
most direct way to check the resolution by itself.
Interestingly we actually had a test that setup this case
already, but we only used it to verify that the funny state
could be overwritten, not that it could be resolved.

We also add a new test in t3200, as "branch -m" was the
original motivation for looking into this. What happens is
this:

  0. HEAD is pointing to branch "a"

  1. The user asks to rename "a" to "a/b".

  2. We create "a/b" and delete "a".

  3. We then try to update any worktree HEADs that point to
 the renamed ref (including the main repo HEAD). To do
 that, we have to resolve each HEAD. But now our HEAD is
 pointing at "a", and we get EISDIR due to the loose
 "a/b". As a result, we think there is no HEAD, and we
 do not update it. It now points to the bogus "a".

Interestingly this case used to work, but only accidentally.
Before 31824d180d (branch: fix branch renaming not updating
HEADs correctly, 2017-08-24), we'd update any HEAD which we
couldn't resolve. That was wrong, but it papered over the
fact that we were incorrectly failing to resolve HEAD.

So while the bug demonstrated by the git-symbolic-ref is
quite old, the regression to "branch -m" is recent.

Signed-off-by: Jeff King 
---
 refs.c  | 15 ++-
 t/t1401-symbolic-ref.sh | 26 +-
 t/t3200-branch.sh   | 10 ++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index df075fcd06..c590a992fb 100644
--- a/refs.c
+++ b/refs.c
@@ -1435,8 +1435,21 @@ const char *refs_resolve_ref_unsafe(struct ref_store 
*refs,
if (refs_read_raw_ref(refs, refname,
  sha1, _refname, _flags)) {
*flags |= read_flags;
-   if (errno != ENOENT || (resolve_flags & 
RESOLVE_REF_READING))
+
+   /* In reading mode, refs must eventually resolve */
+   if (resolve_flags & RESOLVE_REF_READING)
+   return NULL;
+
+   /*
+* Otherwise a missing ref is OK. But the files backend
+* may show errors besides ENOENT if there are
+* similarly-named refs.
+*/
+   if (errno != ENOENT &&
+   

Re: [PATCH] cleanup: fix possible overflow errors in binary search

[Derrick Stolee]

On 10/6/2017 10:18 AM, Jeff King wrote:

On Fri, Oct 06, 2017 at 09:52:31AM -0400, Derrick Stolee wrote:


A common mistake when writing binary search is to allow possible
integer overflow by using the simple average:

mid = (min + max) / 2;

Instead, use the overflow-safe version:

mid = min + (max - min) / 2;

Great, thank you for picking this up!


The included changes were found using the following two greps:

grep "/ 2;" *.c
grep "/ 2;" */*.c
grep "/2;" */*.c

You can use[1]:

   git grep '/ 2;' '*.c'

to have Git expand the wildcard. That catches a few extra cases in
compat/regex/*.c.  Even though it's imported code, it might be
nice to cover those, too (since it's a possible bug, and also as a good
example).

[1] I'd actually write:

   git grep '/ *2;' '*.c'

 to do it all in one grep. :)


Thanks for the grep lesson! I knew there would be a simpler way to do 
what I wanted.



---
  builtin/index-pack.c | 4 ++--
  builtin/pack-objects.c   | 2 +-
  builtin/unpack-objects.c | 2 +-
  cache-tree.c | 2 +-
  packfile.c   | 2 +-
  sha1-lookup.c| 2 +-
  sha1_name.c  | 2 +-
  string-list.c| 2 +-
  utf8.c   | 2 +-
  xdiff/xpatience.c| 2 +-
  10 files changed, 11 insertions(+), 11 deletions(-)

These all look good to me (really the only way the conversion could be
bad is if "min" was higher than "max", and each case is just inside a
loop condition which makes sure that is not the case).

-Peff
I thought this should be simple enough. When we are all happy with the 
idea of this cleanup, I'll re-roll with the missed examples.


Thanks,
-Stolee

[PATCH 1/2] t3308: create a real ref directory/file conflict

[Jeff King]

A test in t3308 wants to make sure that we don't
accidentally merge into "refs/notes/dir" when it exists as a
directory, so it does:

  mkdir .git/refs/notes/dir
  git -c core.notesRef=refs/notes/dir merge ...

and expects the second command to fail. But that
understimates the refs code, which is smart enough to remove
useless directories in the refs hierarchy. The test
succeeded only because of a bug which prevented resolving
refs/notes/dir for writing, even though an actual ref update
would succeed.

In preparation for fixing that bug, let's switch to creating
a real ref in refs/notes/dir, which is a more realistic
situation.

Signed-off-by: Jeff King 
---
 t/t3308-notes-merge.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3308-notes-merge.sh b/t/t3308-notes-merge.sh
index 19aed7ec95..ab946a5153 100755
--- a/t/t3308-notes-merge.sh
+++ b/t/t3308-notes-merge.sh
@@ -79,7 +79,7 @@ test_expect_success 'fail to merge empty notes ref into empty 
notes ref (z => y)
 test_expect_success 'fail to merge into various non-notes refs' '
test_must_fail git -c "core.notesRef=refs/notes" notes merge x &&
test_must_fail git -c "core.notesRef=refs/notes/" notes merge x &&
-   mkdir -p .git/refs/notes/dir &&
+   git update-ref refs/notes/dir/foo HEAD &&
test_must_fail git -c "core.notesRef=refs/notes/dir" notes merge x &&
test_must_fail git -c "core.notesRef=refs/notes/dir/" notes merge x &&
test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x &&
-- 
2.15.0.rc0.413.g9bb4ac64e2

Re: Regression in 'git branch -m'?

[Jeff King]

On Thu, Oct 05, 2017 at 07:25:52PM +0200, Andreas Krey wrote:

> I got something that looks like a regression somewhere since 2.11.
> This script
> 
>   set -xe
>   rm -rf repo
>   git init repo
>   cd repo
>   git commit -m nix --allow-empty
>   git branch -m master/master
>   git rev-parse HEAD
>   git branch
>   git status
> 
> causes .git/HEAD to still contain 'ref: refs/heads/master' and to fail
> in the rev-parse step with
> 
>   + git rev-parse HEAD
>   HEAD
>   fatal: ambiguous argument 'HEAD': unknown revision or path not in the 
> working tree.
>   Use '--' to separate paths from revisions, like this:
>   'git  [...] -- [...]'
> 
> This is with 2.15.0.rc0; with 2.11.0 (and 2.11.0.356.gffac48d09) it still 
> works.

So this turned out to be quite an interesting bug to explore. I think
the solution I ended up with in the second patch is the right thing. I'm
adding Michael to the cc for wisdom on the ref code, though I think the
bug I'm fixing actually goes back to the early days of Git.

Earlier I blamed Duy's 31824d180d. And that is the start of the
regression in v2.15, but only because it fixed another bug which was
papering over the one I'm fixing here. :)

  [v1 1/2]: t3308: create a real ref directory/file conflict
  [v1 2/2]: refs_resolve_ref_unsafe: handle d/f conflicts for writes

 refs.c  | 15 ++-
 t/t1401-symbolic-ref.sh | 26 +-
 t/t3200-branch.sh   | 10 ++
 t/t3308-notes-merge.sh  |  2 +-
 4 files changed, 50 insertions(+), 3 deletions(-)

-Peff

[bug] git add -p breaks, if color.ui is set to "always"

[Alexander Gehrke]

After an update to version 2.14.2 from 2.14.1 "git add --patch" stopped working
for me, just producing the same output as "git diff", but not prompting to stage
anything.

I found that unsetting the config key color.ui, which was set to "always" fixed
the problem.

>From the manpage, color.ui should not have that effect and "always" should be a
legal value.

Regards
Alexander Gehrke

Re: Git config multiple values

[Jeff King]

On Fri, Oct 06, 2017 at 01:10:17PM +0200, aleksander.baranowski wrote:

> I'm currently using git version 2.14.2. There is possible to put
> multiple values into same variable with git config.

Yep, your examples should behave the same even with older versions.

> Case 1:
> # git config --global user.name Foo - returns 0
> # git config --global user.name Bar - returns 0 and replace Foo to Bar
> # git config --global user.name Foo - returns 0 and replace Bar to Foo

This is all as expected. You're hitting the first case in the manpage
synopsis here (I snipped the uninteresting options):

  git config name [value [value_regex]]

So you're doing:

  git config name value

which replaces any existing key by default. You could also do:

  git config --add name value

to add without replacing (if you had a config key that takes multiple
values).

> Case 2:
> # git config --global user.name Foo - returns 0
> # git config --global user.name Foo2 Bar - returns 0 and put second name

Here you're doing:

  git config name value value_regex

So we're replacing any values which match the regex "Bar" (and there are
none), and leaving others intact. Hence after this you will have two
user.name values.

If you wanted a name with two strings, you'd have to quote the string
from the shell to leave it as a single argument:

  git config user.name "Foo2 Bar"

> # cat ~/.gitconfig
> [user]
>   email = aleksander.baranow...@yahoo.pl
>   name = Foo
>   name = Foo2

Right, this is what I'd expect.

> # git config --global user.name Foo - return 5 and message
> "warning: user.name has multiple values
> error: cannot overwrite multiple values with a single value
>Use a regexp, --add or --replace-all to change user.name."

And this, too (though I forgot we had such a safety check).

> It's just an opinion, but this behaviour is no consistent for me.
> 
> If it's not the bug it's a feature just let me know.

It's a feature, though I agree that git-config is rather baroque. We're
mostly stuck with it for reasons of backwards compatibility, though.

-Peff

Re: is there a truly compelling rationale for .git/info/exclude?

[Kaartic Sivaraam]

On Fri, 2017-10-06 at 06:14 -0400, rpj...@crashcourse.ca wrote:
>and in this funny grey area in between, we have .git/info/exclude,
> to be used for ... what, exactly? the one argument i've come up with
> is the situation where you discover that a repo you've cloned has an
> incomplete set of .gitignore patterns, and while you submit a patch
> for that to the maintainer, you can temporarily add that pattern
> to .git/info/exclude, and as soon as the patch is accepted, you can
> toss it.
> 
>but even that isn't a really compelling reason. so what's it for?
> 

Thanks for asking this question. I have long been in the scenario you
just described above except that I didn't know of .git/info/exclude all
these days. I was longing to find if there was a way to ignore files in
 a repo without touching the .gitignore of that repo . Now I have found
one, the ".git/info/exclude".

Thanks, again.

-- 
Kaartic

Re: [PATCH] cleanup: fix possible overflow errors in binary search

[Jeff King]

On Fri, Oct 06, 2017 at 09:52:31AM -0400, Derrick Stolee wrote:

> A common mistake when writing binary search is to allow possible
> integer overflow by using the simple average:
> 
>   mid = (min + max) / 2;
> 
> Instead, use the overflow-safe version:
> 
>   mid = min + (max - min) / 2;

Great, thank you for picking this up!

> The included changes were found using the following two greps:
> 
>   grep "/ 2;" *.c
>   grep "/ 2;" */*.c
>   grep "/2;" */*.c

You can use[1]:

  git grep '/ 2;' '*.c'

to have Git expand the wildcard. That catches a few extra cases in
compat/regex/*.c.  Even though it's imported code, it might be
nice to cover those, too (since it's a possible bug, and also as a good
example).

[1] I'd actually write:

  git grep '/ *2;' '*.c'

to do it all in one grep. :)

> ---
>  builtin/index-pack.c | 4 ++--
>  builtin/pack-objects.c   | 2 +-
>  builtin/unpack-objects.c | 2 +-
>  cache-tree.c | 2 +-
>  packfile.c   | 2 +-
>  sha1-lookup.c| 2 +-
>  sha1_name.c  | 2 +-
>  string-list.c| 2 +-
>  utf8.c   | 2 +-
>  xdiff/xpatience.c| 2 +-
>  10 files changed, 11 insertions(+), 11 deletions(-)

These all look good to me (really the only way the conversion could be
bad is if "min" was higher than "max", and each case is just inside a
loop condition which makes sure that is not the case).

-Peff

Re: [PATCH v2 1/5] test-list-objects: List a subset of object ids

[Jeff King]

On Thu, Oct 05, 2017 at 08:39:42AM -0400, Derrick Stolee wrote:

> > Actually, I'd just as soon see timings for "git log --format=%h" or "git
> > log --raw", as opposed to patches 1 and 2.
> > 
> > You won't see a 90% speedup there, but you will see the actual
> > improvement that real-world users are going to experience, which is way
> > more important, IMHO.
> > 
> Thanks for thinking hard about this.
> 
> For some real-user context: Some engineers using Git for the Windows repo
> were seeing extremely slow commands, such as 'fetch' or 'commit', and when
> we took a trace we saw most of the time spinning in this abbreviation code.

That's surprising to me. I'd expect fetch to generate up to two
abbreviations per fetched ref (for the status table). And for commit to
generate only one for the summary. But maybe it was just so painfully
slow on your repo that it was noticeable.

If there's a case where we're generating a bunch of abbreviated oids,
that might be worth looking into. Do you happen to have a stack trace of
one of the slow cases showing who is calling into the function?

> Our workaround so far has been to set core.abbrev=40.

I actually wondered about this. With the new auto-scaling, we'd
typically jump straight to 12 or 13 hex-chars on a large repo, and that
would be sufficient. So we'd really only need one iteration of the loop.

> I'll run some perf numbers for these commands you recommend, and also see if
> I can replicate some of the pain points that triggered this change using the
> Linux repo.

Thanks!

-Peff

[PATCH] cleanup: fix possible overflow errors in binary search

[Derrick Stolee]

A common mistake when writing binary search is to allow possible
integer overflow by using the simple average:

mid = (min + max) / 2;

Instead, use the overflow-safe version:

mid = min + (max - min) / 2;

The included changes were found using the following two greps:

grep "/ 2;" *.c
grep "/ 2;" */*.c
grep "/2;" */*.c

Making this cleanup will prevent future review friction when a new
binary search is contructed based on existing code.

Signed-off-by: Derrick Stolee 
---
 builtin/index-pack.c | 4 ++--
 builtin/pack-objects.c   | 2 +-
 builtin/unpack-objects.c | 2 +-
 cache-tree.c | 2 +-
 packfile.c   | 2 +-
 sha1-lookup.c| 2 +-
 sha1_name.c  | 2 +-
 string-list.c| 2 +-
 utf8.c   | 2 +-
 xdiff/xpatience.c| 2 +-
 10 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index f2be145e1..8ec459f52 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -633,7 +633,7 @@ static int find_ofs_delta(const off_t offset, enum 
object_type type)
int first = 0, last = nr_ofs_deltas;
 
while (first < last) {
-   int next = (first + last) / 2;
+   int next = first + (last - first) / 2;
struct ofs_delta_entry *delta = _deltas[next];
int cmp;
 
@@ -687,7 +687,7 @@ static int find_ref_delta(const unsigned char *sha1, enum 
object_type type)
int first = 0, last = nr_ref_deltas;
 
while (first < last) {
-   int next = (first + last) / 2;
+   int next = first + (last - first) / 2;
struct ref_delta_entry *delta = _deltas[next];
int cmp;
 
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 5ee2c48ff..6e77dfd44 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1277,7 +1277,7 @@ static int done_pbase_path_pos(unsigned hash)
int lo = 0;
int hi = done_pbase_paths_num;
while (lo < hi) {
-   int mi = (hi + lo) / 2;
+   int mi = lo + (hi - lo) / 2;
if (done_pbase_paths[mi] == hash)
return mi;
if (done_pbase_paths[mi] < hash)
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 689a29fac..62ea264c4 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -394,7 +394,7 @@ static void unpack_delta_entry(enum object_type type, 
unsigned long delta_size,
lo = 0;
hi = nr;
while (lo < hi) {
-   mid = (lo + hi)/2;
+   mid = lo + (hi - lo) / 2;
if (base_offset < obj_list[mid].offset) {
hi = mid;
} else if (base_offset > obj_list[mid].offset) {
diff --git a/cache-tree.c b/cache-tree.c
index 71d092ed5..d3f740127 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -49,7 +49,7 @@ static int subtree_pos(struct cache_tree *it, const char 
*path, int pathlen)
lo = 0;
hi = it->subtree_nr;
while (lo < hi) {
-   int mi = (lo + hi) / 2;
+   int mi = lo + (hi - lo) / 2;
struct cache_tree_sub *mdl = down[mi];
int cmp = subtree_name_cmp(path, pathlen,
   mdl->name, mdl->namelen);
diff --git a/packfile.c b/packfile.c
index eab754248..4a5fe7ab1 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1743,7 +1743,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
   sha1[0], sha1[1], sha1[2], lo, hi, p->num_objects);
 
while (lo < hi) {
-   unsigned mi = (lo + hi) / 2;
+   unsigned mi = lo + (hi - lo) / 2;
int cmp = hashcmp(index + mi * stride, sha1);
 
if (debug_lookup)
diff --git a/sha1-lookup.c b/sha1-lookup.c
index 2552b7902..df08f8355 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -95,7 +95,7 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t 
nr,
hi = mi;
else
lo = mi + 1;
-   mi = (hi + lo) / 2;
+   mi = lo + (hi - lo) / 2;
} while (lo < hi);
return -lo-1;
 }
diff --git a/sha1_name.c b/sha1_name.c
index 134ac9742..c7c5ab376 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -157,7 +157,7 @@ static void unique_in_pack(struct packed_git *p,
num = p->num_objects;
last = num;
while (first < last) {
-   uint32_t mid = (first + last) / 2;
+   uint32_t mid = first + (last - first) / 2;
const unsigned char *current;
int cmp;
 
diff --git a/string-list.c b/string-list.c
index 806b4c872..a0cf0cfe8 100644
--- a/string-list.c
+++ b/string-list.c
@@ -16,7 +16,7 @@ static int get_entry_index(const 

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 15:19, Christian Couder wrote:
> Now it would be strange to have "moveIfClosest" without having "move"
> first and I don't see how "move" would be different from the existing
> "replace".
> Or maybe "move" means "replaceIfIdentical", in this case I think it
> would help users to just call it "replaceIfIdentical".

Well, the effect of "replacing if identical" *is* to move the existing
identical trailer to the new position. :)

Paolo

[Question] Documenting platform implications on CVE to git

[Randall S. Becker]

Hi All,

I wonder whether there is some mechanism for providing official responses
from platform ports relating to security CVE reports, like CVE-2017-14867.
For example, the Perl implementation on HPE NonStop does not include the SCM
module so commands relating cvsserver may not be available - one thing to be
verified so is a question #1. But the real question (#2) is: where would one
most appropriately document issues like this to be consistent with other
platform responses relating to git?

Thanks,
Randall

-- Brief whoami: NonStop developer since approximately
UNIX(421664400)/NonStop(2112884442) 
-- In my real life, I talk too much.

[PATCH v7 1/3] submodule--helper: introduce get_submodule_displaypath()

[Prathamesh Chavan]

Introduce function get_submodule_displaypath() to replace the code
occurring in submodule_init() for generating displaypath of the
submodule with a call to it.

This new function will also be used in other parts of the system
in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 35 +++
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 06ed02f99..56c1c52e2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -219,6 +219,26 @@ static int resolve_relative_url_test(int argc, const char 
**argv, const char *pr
return 0;
 }
 
+/* the result should be freed by the caller. */
+static char *get_submodule_displaypath(const char *path, const char *prefix)
+{
+   const char *super_prefix = get_super_prefix();
+
+   if (prefix && super_prefix) {
+   BUG("cannot have prefix '%s' and superprefix '%s'",
+   prefix, super_prefix);
+   } else if (prefix) {
+   struct strbuf sb = STRBUF_INIT;
+   char *displaypath = xstrdup(relative_path(path, prefix, ));
+   strbuf_release();
+   return displaypath;
+   } else if (super_prefix) {
+   return xstrfmt("%s%s", super_prefix, path);
+   } else {
+   return xstrdup(path);
+   }
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -334,15 +354,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
struct strbuf sb = STRBUF_INIT;
char *upd = NULL, *url = NULL, *displaypath;
 
-   if (prefix && get_super_prefix())
-   die("BUG: cannot have prefix and superprefix");
-   else if (prefix)
-   displaypath = xstrdup(relative_path(path, prefix, ));
-   else if (get_super_prefix()) {
-   strbuf_addf(, "%s%s", get_super_prefix(), path);
-   displaypath = strbuf_detach(, NULL);
-   } else
-   displaypath = xstrdup(path);
+   displaypath = get_submodule_displaypath(path, prefix);
 
sub = submodule_from_path(_oid, path);
 
@@ -357,9 +369,9 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 * Set active flag for the submodule being initialized
 */
if (!is_submodule_active(the_repository, path)) {
-   strbuf_reset();
strbuf_addf(, "submodule.%s.active", sub->name);
git_config_set_gently(sb.buf, "true");
+   strbuf_reset();
}
 
/*
@@ -367,7 +379,6 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
 * To look up the url in .git/config, we must not fall back to
 * .gitmodules, so look it up directly.
 */
-   strbuf_reset();
strbuf_addf(, "submodule.%s.url", sub->name);
if (git_config_get_string(sb.buf, )) {
if (!sub->url)
@@ -404,9 +415,9 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
_("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
}
+   strbuf_reset();
 
/* Copy "update" setting when it is not set yet */
-   strbuf_reset();
strbuf_addf(, "submodule.%s.update", sub->name);
if (git_config_get_string(sb.buf, ) &&
sub->update_strategy.type != SM_UPDATE_UNSPECIFIED) {
-- 
2.14.2

[PATCH v7 0/3] Incremental rewrite of git-submodules

[Prathamesh Chavan]

Changes in v7:

* Instead of using cb_flags in the callback data's struct, 'flags' is used.

* Similar changes were applied to the CB_OPT_QUIET and other bits.

* The function compute_rev_name() was formatted in accordance with the "make
  style", into a compact version.

* Call to precompose_argv() in the function status_submodule() was dropped
  as the call was unnecessary.

As before you can find this series at: 
https://github.com/pratham-pc/git/commits/patch-series-1

And its build report is available at: 
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-1
Build #190

The above changes were based on master branch.

Another branch, similar to the above, was created, but was based
on the 'next' branch.
Complete build report of that is also available at:
https://travis-ci.org/pratham-pc/git/builds
Branch: patch-series-1-next
Build #189
The above changes are also push on github and are available at:
https://github.com/pratham-pc/git/commits/patch-series-1-next

Prathamesh Chavan (3):
  submodule--helper: introduce get_submodule_displaypath()
  submodule--helper: introduce for_each_listed_submodule()
  submodule: port submodule subcommand 'status' from shell to C

 builtin/submodule--helper.c | 273 +---
 git-submodule.sh|  61 +-
 2 files changed, 257 insertions(+), 77 deletions(-)

-- 
2.14.2

[PATCH v7 2/3] submodule--helper: introduce for_each_listed_submodule()

[Prathamesh Chavan]

Introduce function for_each_listed_submodule() and replace a loop
in module_init() with a call to it.

The new function will also be used in other parts of the
system in later patches.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 40 +++-
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 56c1c52e2..29e3fde16 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -14,6 +14,11 @@
 #include "refs.h"
 #include "connect.h"
 
+#define OPT_QUIET (1 << 0)
+
+typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
+ void *cb_data);
+
 static char *get_default_remote(void)
 {
char *dest = NULL, *ret;
@@ -348,7 +353,23 @@ static int module_list(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
-static void init_submodule(const char *path, const char *prefix, int quiet)
+static void for_each_listed_submodule(const struct module_list *list,
+ each_submodule_fn fn, void *cb_data)
+{
+   int i;
+   for (i = 0; i < list->nr; i++)
+   fn(list->entries[i], cb_data);
+}
+
+struct init_cb {
+   const char *prefix;
+   unsigned int flags;
+};
+
+#define INIT_CB_INIT { NULL, 0 }
+
+static void init_submodule(const char *path, const char *prefix,
+  unsigned int flags)
 {
const struct submodule *sub;
struct strbuf sb = STRBUF_INIT;
@@ -410,7 +431,7 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
if (git_config_set_gently(sb.buf, url))
die(_("Failed to register url for submodule path '%s'"),
displaypath);
-   if (!quiet)
+   if (!(flags & OPT_QUIET))
fprintf(stderr,
_("Submodule '%s' (%s) registered for path 
'%s'\n"),
sub->name, url, displaypath);
@@ -437,12 +458,18 @@ static void init_submodule(const char *path, const char 
*prefix, int quiet)
free(upd);
 }
 
+static void init_submodule_cb(const struct cache_entry *list_item, void 
*cb_data)
+{
+   struct init_cb *info = cb_data;
+   init_submodule(list_item->name, info->prefix, info->flags);
+}
+
 static int module_init(int argc, const char **argv, const char *prefix)
 {
+   struct init_cb info = INIT_CB_INIT;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
int quiet = 0;
-   int i;
 
struct option module_init_options[] = {
OPT__QUIET(, N_("Suppress output for initializing a 
submodule")),
@@ -467,8 +494,11 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
if (!argc && git_config_get_value_multi("submodule.active"))
module_list_active();
 
-   for (i = 0; i < list.nr; i++)
-   init_submodule(list.entries[i]->name, prefix, quiet);
+   info.prefix = prefix;
+   if (quiet)
+   info.flags |= OPT_QUIET;
+
+   for_each_listed_submodule(, init_submodule_cb, );
 
return 0;
 }
-- 
2.14.2

[PATCH v7 3/3] submodule: port submodule subcommand 'status' from shell to C

[Prathamesh Chavan]

This aims to make git-submodule 'status' a built-in. Hence, the function
cmd_status() is ported from shell to C. This is done by introducing
four functions: module_status(), submodule_status_cb(),
submodule_status() and print_status().

The function module_status() acts as the front-end of the subcommand.
It parses subcommand's options and then calls the function
module_list_compute() for computing the list of submodules. Then
this functions calls for_each_listed_submodule() looping through the
list obtained.

Then for_each_listed_submodule() calls submodule_status_cb() for each of
the submodule in its list. The function submodule_status_cb() calls
submodule_status() after passing appropriate arguments to the funciton.
Function submodule_status() is responsible for generating the status
each submodule it is called for, and then calls print_status().

Finally, the function print_status() handles the printing of submodule's
status.

Function set_name_rev() is also ported from git-submodule to the
submodule--helper builtin function compute_rev_name(), which now
generates the value of the revision name as required.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 198 
 git-submodule.sh|  61 +-
 2 files changed, 199 insertions(+), 60 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 29e3fde16..d366e8e7b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -13,8 +13,13 @@
 #include "remote.h"
 #include "refs.h"
 #include "connect.h"
+#include "revision.h"
+#include "diffcore.h"
+#include "diff.h"
 
 #define OPT_QUIET (1 << 0)
+#define OPT_CACHED (1 << 1)
+#define OPT_RECURSIVE (1 << 2)
 
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
  void *cb_data);
@@ -244,6 +249,44 @@ static char *get_submodule_displaypath(const char *path, 
const char *prefix)
}
 }
 
+static char *compute_rev_name(const char *sub_path, const char* object_id)
+{
+   struct strbuf sb = STRBUF_INIT;
+   const char ***d;
+
+   static const char *describe_bare[] = { NULL };
+
+   static const char *describe_tags[] = { "--tags", NULL };
+
+   static const char *describe_contains[] = { "--contains", NULL };
+
+   static const char *describe_all_always[] = { "--all", "--always", NULL 
};
+
+   static const char **describe_argv[] = { describe_bare, describe_tags,
+   describe_contains,
+   describe_all_always, NULL };
+
+   for (d = describe_argv; *d; d++) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   prepare_submodule_repo_env(_array);
+   cp.dir = sub_path;
+   cp.git_cmd = 1;
+   cp.no_stderr = 1;
+
+   argv_array_push(, "describe");
+   argv_array_pushv(, *d);
+   argv_array_push(, object_id);
+
+   if (!capture_command(, , 0)) {
+   strbuf_strip_suffix(, "\n");
+   return strbuf_detach(, NULL);
+   }
+   }
+
+   strbuf_release();
+   return NULL;
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -503,6 +546,160 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct status_cb {
+   const char *prefix;
+   unsigned int flags;
+};
+
+#define STATUS_CB_INIT { NULL, 0 }
+
+static void print_status(unsigned int flags, char state, const char *path,
+const struct object_id *oid, const char *displaypath)
+{
+   if (flags & OPT_QUIET)
+   return;
+
+   printf("%c%s %s", state, oid_to_hex(oid), displaypath);
+
+   if (state == ' ' || state == '+')
+   printf(" (%s)", compute_rev_name(path, oid_to_hex(oid)));
+
+   printf("\n");
+}
+
+static int handle_submodule_head_ref(const char *refname,
+const struct object_id *oid, int flags,
+void *cb_data)
+{
+   struct object_id *output = cb_data;
+   if (oid)
+   oidcpy(output, oid);
+
+   return 0;
+}
+
+static void status_submodule(const char *path, const struct object_id *ce_oid,
+unsigned int ce_flags, const char *prefix,
+unsigned int flags)
+{
+   char *displaypath;
+   struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+   struct rev_info rev;
+   int diff_files_result;
+
+   if (!submodule_from_path(_oid, path))
+   die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
+ path);
+
+  

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Christian Couder]

On Fri, Oct 6, 2017 at 2:39 PM, Paolo Bonzini  wrote:
> On 06/10/2017 14:33, Christian Couder wrote:
>> Ok. I think you might want something called for example
>> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
>> trailer with the same (, ) pair above or below the line
>> where the replaced trailer will be put when ignoring trailers with a
>> different ".
>
> So basically "moveIfClosest" (move if last for where=end, move if first
> for where=begin; for where=after and where=before it would just end up
> doing nothing)?

First yeah these would not make sense anyway if where=after or where=before.

Now it would be strange to have "moveIfClosest" without having "move"
first and I don't see how "move" would be different from the existing
"replace".
Or maybe "move" means "replaceIfIdentical", in this case I think it
would help users to just call it "replaceIfIdentical".

Also there is "addIfDifferentNeighbor" so we already have "Neighbor"
which means "just above or below". Then if we use "Closest" I think it
will be harder to distinguish it from "Neighbor" than if we use
"Close".

That's why I think "replaceIfIdenticalClose" is better. It could
enable us to eventually use a regexp like
"(add|replace)(If(Different|Identical)(Close|Neighbor)+)+"  to parse
the add* and replace* options.

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 14:33, Christian Couder wrote:
> Ok. I think you might want something called for example
> "replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
> trailer with the same (, ) pair above or below the line
> where the replaced trailer will be put when ignoring trailers with a
> different ".

So basically "moveIfClosest" (move if last for where=end, move if first
for where=begin; for where=after and where=before it would just end up
doing nothing)?  It's not hard to implement, but I'm wondering if it's
too ad hoc.

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Christian Couder]

On Fri, Oct 6, 2017 at 12:40 PM, Paolo Bonzini  wrote:
> On 06/10/2017 12:30, Christian Couder wrote:

>> Did you try using `--where end --if-exists replace --trailer "$sob"`?
>
> Yes, it's a different behavior; "--if-exists replace" matches on others'
> SoB as well, so it would eat the original author's SoB if I didn't have one.
>
> So "move" does get it wrong for
>
> Signed-off-by: Me
> Signed-off-by: Friend
>
> (Me gets moved last, which may not be what you want) but "replace" gets
> it wrong in the arguably more common case of
>
> Signed-off-by: Friend
>
> which is damaged to just "Signed-off-by: Me".

Ok. I think you might want something called for example
"replaceIfIdenticalClose" where "IdenticalClose" means: "there is a
trailer with the same (, ) pair above or below the line
where the replaced trailer will be put when ignoring trailers with a
different ".

Re: is there a truly compelling rationale for .git/info/exclude?

[Junio C Hamano]

rpj...@crashcourse.ca writes:

>   at the other end, users are certainly welcome to add extra patterns
> to be ignored, based purely on the way they work -- perhaps based on
> their choice of editor, they might want to exclude *.swp files, or
> if working on a Mac, ignore .DS_Store, and so on, using a
> core.excludesFile setting.

This is primarily why .git/info/exclude exists.  A user who does not
use the same set of tools to work on different projects may not be
able to use ~/.gitconfig with core.excludesFile pointing at a single
place that applies to _all_ repositories the user touches.

Also, core.excludesFile came a lot later than in-project and
in-repository exclude list, IIRC.

Don't waste time by seeking a "compelling" reason.  A mere "this is
the most expedite way to gain convenience" back when something was
introduced could be an answer, and it is way too late to complain
about such a choice anyway.

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Junio C Hamano]

Martin Ågren  writes:

> Maybe I'm missing something Git-specific, but isn't the only thing that
> needs to be done now, to document/specify that 1) the client should send
> its list ordered by preference, 2) how preference is signalled, and 3)
> that the server gets to choose?

I think Simon's reminder of Stefan's was about specifying something
different from (1) above---it was just a list of good ones (as
opposed to ones to be avoided).  I was suggesting to tweak that to
match what you wrote above.

> Why would a server operator with only v0 and v1 at their disposal want
> to choose v0 instead of v1, considering that -- as far as I understand
> -- they are in fact the same?

Because we may later discover some reason we not yet know that makes
v$n+1 unsuitable after we introduce it, and we need to avoid it by
preferring v$n instead?

Booking Flight !

[FREDERIC LUCIEN]

Hello Dear,

I would like to make a booking flight with you.

Itinerary:

From : Paris (CDG)- Casablanca (CMN)-Paris (CDG)

Departing Date: 18/10/2017
Returning Date: 31/10/2017
Economic Class

Informations for booking.

Adult: Mr.
First Name: Fréderic Lucien
Last Name: PLAS
Date of Birth: 13/08/1964
Nationality: French
Passport N°: 11 AI48970
Expiration date: 20/02/2021

General Manager
Mr. PLAS
Email: fred.luc...@mail.com

Re: [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`

[Junio C Hamano]

Martin Ågren  writes:

> On 6 October 2017 at 04:01, Junio C Hamano  wrote:
>> Martin Ågren  writes:
>>
>>> v2: Except for the slightly different documentation in cache.h, this is
>>> a squash of the last two patches of v1. I hope the commit message is
>>> better.
>>
>> Yeah, it is long ;-) but readable.
>
> "Long but readable"... Yeah. When I rework the previous patch (document
> the closing-behavior of `do_write_index()`) I could address this. I
> think there are several interesting details here and I'm not sure which
> I'd want to leave out, but yeah, they add up...

I didn't mean "long is bad" at all in this case.  

Certainly, from time to time we find commits with overlong
explanation that only states obvious, and they are "long and bad".
But I do not think this one falls into the same category as those.

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Martin Ågren]

On 6 October 2017 at 11:40, Junio C Hamano  wrote:
> Simon Ruderich  writes:
>
>> Did you consider Stefan Beller's suggestion regarding a
>> (white)list of allowed versions?
>>
>> On Mon, Sep 18, 2017 at 01:06:59PM -0700, Stefan Beller wrote:
>>> Thinking about this, how about:
>>>
>>>   If not configured, we do as we want. (i.e. Git has full control over
>>>   it's decision making process, which for now is "favor v0 over v1 as
>>>   we are experimenting with v1". This strategy may change in the future
>>>   to "prefer highest version number that both client and server can speak".)
>>>
>>>   If it is configured, "use highest configured number from the given set".
>>>
>>> ?
>>
>> It would also allow the server operator to configure only a
>> specific set of versions (to handle the "version x is
>> insecure/slow"-issue raised by Stefan Beller). The current code
>> always uses the latest protocol supported by the git binary.
>
> If we do anything less trivial than "highest supported by both" (and
> I suspect we want to in the final production version), I'd prefer
> the configuration to list versions one side supports in decreasing
> order of preference (e.g. "v3 v0 v2"), and take the earliest from
> this list that both sides know how to talk, so that we can skip
> insecure versions altogether by omitting, and we can express that we
> would rather avoid talking expensive versions unless there is no
> other version that is understood by the other side.

Maybe I'm missing something Git-specific, but isn't the only thing that
needs to be done now, to document/specify that 1) the client should send
its list ordered by preference, 2) how preference is signalled, and 3)
that the server gets to choose?

Why would a server operator with only v0 and v1 at their disposal want
to choose v0 instead of v1, considering that -- as far as I understand
-- they are in fact the same?

Different server implementations and different server configurations
will be able to apply whatever rules they want in order to decide which
version to use. (It's not like the client can verify the choice that the
server makes.) And Brandon's "pick the highest number" will do for now.

There are many possible rules that the server could apply and they
shouldn't affect other servers or what the client does. For example, the
server can go "You seem to know lots of versions, including X and Y.
Those are the two that I really prefer, but between those two I'm not
picky, so I'll use whichever of X and Y that you seem to prefer." Unless
I've missed something, we'll never need to implement -- nor specify --
anything like that before we have learned both v2 and v3.

I guess my thinking is, there's a difference between the protocol and
the implementation.

Martin

Git config multiple values

[aleksander.baranowski]

Hi,

I'm currently using git version 2.14.2. There is possible to put
multiple values into same variable with git config.

Case 1:
# git config --global user.name Foo - returns 0
# git config --global user.name Bar - returns 0 and replace Foo to Bar
# git config --global user.name Foo - returns 0 and replace Bar to Foo

Case 2:
# git config --global user.name Foo - returns 0
# git config --global user.name Foo2 Bar - returns 0 and put second name
# cat ~/.gitconfig
[user]
email = aleksander.baranow...@yahoo.pl
name = Foo
name = Foo2
# git config --global user.name Foo - return 5 and message
"warning: user.name has multiple values
error: cannot overwrite multiple values with a single value
   Use a regexp, --add or --replace-all to change user.name."

It's just an opinion, but this behaviour is no consistent for me.

If it's not the bug it's a feature just let me know.

Bests,
Alex

Re: [PATCH v2 11/12] read-cache: leave lock in right state in `write_locked_index()`

[Martin Ågren]

On 6 October 2017 at 04:01, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> v2: Except for the slightly different documentation in cache.h, this is
>> a squash of the last two patches of v1. I hope the commit message is
>> better.
>
> Yeah, it is long ;-) but readable.

"Long but readable"... Yeah. When I rework the previous patch (document
the closing-behavior of `do_write_index()`) I could address this. I
think there are several interesting details here and I'm not sure which
I'd want to leave out, but yeah, they add up...

Martin

Re: [PATCH v2 10/12] read-cache: drop explicit `CLOSE_LOCK`-flag

[Martin Ågren]

On 6 October 2017 at 03:39, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> diff --git a/read-cache.c b/read-cache.c
>> index 65f4fe837..1c917eba9 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -2343,14 +2343,13 @@ static int do_write_locked_index(struct index_state 
>> *istate, struct lock_file *l
>>   int ret = do_write_index(istate, lock->tempfile, 0);
>>   if (ret)
>>   return ret;
>> - assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
>> -(COMMIT_LOCK | CLOSE_LOCK));
>>   if (flags & COMMIT_LOCK)
>>   return commit_locked_index(lock);
>> - else if (flags & CLOSE_LOCK)
>> - return close_lock_file_gently(lock);
>> - else
>> - return ret;
>> + /*
>> +  * The lockfile already happens to have
>> +  * been closed, but let's be specific.
>> +  */
>> + return close_lock_file_gently(lock);
>
> "already happens to have been" is quite a mouthful, and is not quite
> truthful, as we do not foresee ever wanting to change that (because
> of that stat(2) issue you mentioned).  It might be better to declare
> that do_write_index() closes the lockfile after successfully writing
> the data out to it.  I dunno if that reasoning is strong enough to
> remove this (extra) close, though.
>
> When any of the ce_write() calls in do_write_index() fails, the
> function returns -1 without hitting the close/stat (obviously).
> Somebody very high in the callchain (e.g. write_locked_index())
> would clean it up by calling rollback_lock_file() eventually, so
> that would not be a problem ;-)

When I wrote this, I was too stuck in the "it gets closed accidentally"
world view. It would indeed be cleaner to specify that the close happens
in `do_write_index()`. As you say, because of the stat-ing, we simply
have to close.

It's still an implementation detail that closing the temporary file is
the same as closing the lock. We might want to refactor to hand over the
lock instead of its tempfile. Except the other caller has no suitable
lock, only a temporary file. I guess that caller could use a lock
instead, but it feels like the wrong solution to the wrong problem.

I'm sure that something could be done here to improve the cleanliness.
For this series, I think I'll document better that `do_write_index()`
closes the temporary file on success, that this might mean that it
actually closes a *lock*file, but that the latter should not be relied
upon. I'll get to this later today.

Thanks.

Martin

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Paolo Bonzini]

On 06/10/2017 12:30, Christian Couder wrote:
> On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini  wrote:
>> The purpose of this action is for scripts to be able to keep the
>> user's Signed-off-by at the end.  For example say I have a script
>> that adds a Reviewed-by tag:
>>
>> #! /bin/sh
>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>> trailer="Reviewed-by: $them"
>> git log -1 --pretty=format:%B | \
>>   git interpret-trailers --where end --if-exists doNothing --trailer 
>> "$trailer" | \
>>   git commit --amend -F-
>>
>> Now, this script will leave my Signed-off-by line in a non-canonical
>> place, like
>>
>>Signed-off-by: Paolo Bonzini 
>>Reviewed-by: Junio C Hamano 
>>
>> This new option enables the following improvement:
>>
>> #! /bin/sh
>> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
>> them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
>> trailer="Reviewed-by: $them"
>> sob="Signed-off-by: $me"
>> git log -1 --pretty=format:%B | \
>>   git interpret-trailers --where end --if-exists doNothing --trailer 
>> "$trailer" \
>>  --where end --if-exists move --if-missing 
>> doNothing --trailer "$sob" | \
>>   git commit --amend -F-
>>
>> which lets me keep the SoB line at the end, as it should be.
>> Posting as RFC because it's possible that I'm missing a simpler
>> way to achieve this...
> 
> Did you try using `--where end --if-exists replace --trailer "$sob"`?

Yes, it's a different behavior; "--if-exists replace" matches on others'
SoB as well, so it would eat the original author's SoB if I didn't have one.

So "move" does get it wrong for

Signed-off-by: Me
Signed-off-by: Friend

(Me gets moved last, which may not be what you want) but "replace" gets
it wrong in the arguably more common case of

Signed-off-by: Friend

which is damaged to just "Signed-off-by: Me".

Paolo

Re: [RFC PATCH 0/4] interpret-trailers: introduce "move" action

[Christian Couder]

On Thu, Oct 5, 2017 at 3:22 PM, Paolo Bonzini  wrote:
> The purpose of this action is for scripts to be able to keep the
> user's Signed-off-by at the end.  For example say I have a script
> that adds a Reviewed-by tag:
>
> #! /bin/sh
> them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
> trailer="Reviewed-by: $them"
> git log -1 --pretty=format:%B | \
>   git interpret-trailers --where end --if-exists doNothing --trailer 
> "$trailer" | \
>   git commit --amend -F-
>
> Now, this script will leave my Signed-off-by line in a non-canonical
> place, like
>
>Signed-off-by: Paolo Bonzini 
>Reviewed-by: Junio C Hamano 
>
> This new option enables the following improvement:
>
> #! /bin/sh
> me=$(git var GIT_COMMITTER_IDENT | sed 's,>.*,>,')
> them=$(git log -i -1 --pretty='format:%an <%ae>' --author="$*")
> trailer="Reviewed-by: $them"
> sob="Signed-off-by: $me"
> git log -1 --pretty=format:%B | \
>   git interpret-trailers --where end --if-exists doNothing --trailer 
> "$trailer" \
>  --where end --if-exists move --if-missing 
> doNothing --trailer "$sob" | \
>   git commit --amend -F-
>
> which lets me keep the SoB line at the end, as it should be.
> Posting as RFC because it's possible that I'm missing a simpler
> way to achieve this...

Did you try using `--where end --if-exists replace --trailer "$sob"`?

is there a truly compelling rationale for .git/info/exclude?

[rpjday]

  currently having a discussion with ben straub of "pro git" notoriety,
and he and i seem to agree that there's not much value in registering
ignore patterns in a repo-specific .git/info/exclude file.

  on the one hand, the .gitignore files that come with a repo would
represent (in ben's terminology, which i really like) the "intrinsic"
patterns to be ignored that are related to the basic content of the repo.

  at the other end, users are certainly welcome to add extra patterns
to be ignored, based purely on the way they work -- perhaps based on
their choice of editor, they might want to exclude *.swp files, or
if working on a Mac, ignore .DS_Store, and so on, using a
core.excludesFile setting.

  and in this funny grey area in between, we have .git/info/exclude,
to be used for ... what, exactly? the one argument i've come up with
is the situation where you discover that a repo you've cloned has an
incomplete set of .gitignore patterns, and while you submit a patch
for that to the maintainer, you can temporarily add that pattern
to .git/info/exclude, and as soon as the patch is accepted, you can
toss it.

  but even that isn't a really compelling reason. so what's it for?

rday

Re: Regression in 'git branch -m'?

[Jeff King]

On Fri, Oct 06, 2017 at 06:45:08PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > So this patch fixes the problem:
> >
> > diff --git a/refs.c b/refs.c
> > index df075fcd06..2ba74720c8 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store 
> > *refs,
> > if (refs_read_raw_ref(refs, refname,
> >   sha1, _refname, _flags)) {
> > *flags |= read_flags;
> > -   if (errno != ENOENT || (resolve_flags & 
> > RESOLVE_REF_READING))
> > +   if ((errno != ENOENT && errno != EISDIR) ||
> > +   (resolve_flags & RESOLVE_REF_READING))
> 
> Ooo, good find--is_missing_file_error() strikes back...

Almost. That uses ENOTDIR, so that looking for "foo/bar" handles the
case where "foo" is a regular file.

But this is the opposite: we ask about "foo", but "foo/bar" exists. The
answer isn't "it's not there" in the general case, but "it's not the
thing you were expecting".

But in the case of refs, the filesystem is just a representation of the
abstract namespace. In asking for "refs/heads/foo", if "refs/heads/foo/bar"
exists, then answer is still "no, it's not a ref".

So EISDIR is needed for this case, though I suspect the opposite case
would need ENOTDIR. I actually wonder if the files-backend read_raw_ref
ought to be normalizing all of those to ENOENT.

> > return NULL;
> > hashclr(sha1);
> > if (*flags & REF_BAD_NAME)
> >
> > but seems to stimulate a test failure in t3308. I have a suspicion that
> > I've just uncovered another bug, but I'll dig in that. In the meantime I
> > wanted to post this update in case anybody else was looking into it.

That failure indeed turned out to be a red herring. So I think I'm
definitely onto the right track.

I want to play with the ENOTDIR case, and then I'll write up the whole
thing and send it in later today.

-Peff

Re: Regression in 'git branch -m'?

[Junio C Hamano]

Jeff King  writes:

> So this patch fixes the problem:
>
> diff --git a/refs.c b/refs.c
> index df075fcd06..2ba74720c8 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store 
> *refs,
>   if (refs_read_raw_ref(refs, refname,
> sha1, _refname, _flags)) {
>   *flags |= read_flags;
> - if (errno != ENOENT || (resolve_flags & 
> RESOLVE_REF_READING))
> + if ((errno != ENOENT && errno != EISDIR) ||
> + (resolve_flags & RESOLVE_REF_READING))

Ooo, good find--is_missing_file_error() strikes back...

>   return NULL;
>   hashclr(sha1);
>   if (*flags & REF_BAD_NAME)
>
> but seems to stimulate a test failure in t3308. I have a suspicion that
> I've just uncovered another bug, but I'll dig in that. In the meantime I
> wanted to post this update in case anybody else was looking into it.
>
> -Peff

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Junio C Hamano]

Simon Ruderich  writes:

> Did you consider Stefan Beller's suggestion regarding a
> (white)list of allowed versions?
>
> On Mon, Sep 18, 2017 at 01:06:59PM -0700, Stefan Beller wrote:
>> Thinking about this, how about:
>>
>>   If not configured, we do as we want. (i.e. Git has full control over
>>   it's decision making process, which for now is "favor v0 over v1 as
>>   we are experimenting with v1". This strategy may change in the future
>>   to "prefer highest version number that both client and server can speak".)
>>
>>   If it is configured, "use highest configured number from the given set".
>>
>> ?
>
> It would also allow the server operator to configure only a
> specific set of versions (to handle the "version x is
> insecure/slow"-issue raised by Stefan Beller). The current code
> always uses the latest protocol supported by the git binary.

If we do anything less trivial than "highest supported by both" (and
I suspect we want to in the final production version), I'd prefer
the configuration to list versions one side supports in decreasing
order of preference (e.g. "v3 v0 v2"), and take the earliest from
this list that both sides know how to talk, so that we can skip
insecure versions altogether by omitting, and we can express that we
would rather avoid talking expensive versions unless there is no
other version that is understood by the other side.

Re: [PATCH v3 03/10] protocol: introduce protocol extention mechanisms

[Simon Ruderich]

On Tue, Oct 03, 2017 at 01:15:00PM -0700, Brandon Williams wrote:
> [snip]
>
> +protocol.version::
> + Experimental. If set, clients will attempt to communicate with a
> + server using the specified protocol version.  If unset, no
> + attempt will be made by the client to communicate using a
> + particular protocol version, this results in protocol version 0
> + being used.
> + Supported versions:

Did you consider Stefan Beller's suggestion regarding a
(white)list of allowed versions?

On Mon, Sep 18, 2017 at 01:06:59PM -0700, Stefan Beller wrote:
> Thinking about this, how about:
>
>   If not configured, we do as we want. (i.e. Git has full control over
>   it's decision making process, which for now is "favor v0 over v1 as
>   we are experimenting with v1". This strategy may change in the future
>   to "prefer highest version number that both client and server can speak".)
>
>   If it is configured, "use highest configured number from the given set".
>
> ?

It would also allow the server operator to configure only a
specific set of versions (to handle the "version x is
insecure/slow"-issue raised by Stefan Beller). The current code
always uses the latest protocol supported by the git binary.

Minor nit, s/extention/extension/ in the patch name?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

RE: Enquête auprès de la table d'aide pour incident [INC0903501]

[Tim Rhynes]

From: Tim Rhynes
Sent: Friday, October 06, 2017 3:17 AM
Subject: Enquête auprès de la table d'aide pour incident [INC0903501]


Prenez-vous un moment pour compléter un sondage sur l'incident INC0903501 en ce 
qui concerne «l'enquête sur votre courrier électronique» Votre avis est 
extrêmement précieux.



 Cliquez ici pour l'enquête sur le rendement du service 
d'assistance

Re: Regression in 'git branch -m'?

[Jeff King]

On Fri, Oct 06, 2017 at 03:39:13AM -0400, Jeff King wrote:

> I got a chance to look at this again. I think the root of the problem is
> that resolve_ref() as it is implemented now is just totally unsuitable
> for asking the question "what does this symbolic link point to?".
> 
> Because you end up with either:
> 
>   1. If we pass RESOLVE_REF_READING, then we do not return the target
>  refname for orphaned commits (which is why 31824d180d dropped it).
> 
>   2. If not, then we do not return the target refname for commits with
>  names that are not available for writing. The d/f conflict here is
>  one example, but there may be others.
> 
> So I think we need to teach resolve_ref() a new mode that's like
> "reading", but just follows the symref chain.

This analysis is not _quite_ right. The "not available for writing"
thing actually isn't intentionally enforced by the resolve_ref. It's
just that it's not careful enough about checking errno. We see EISDIR
instead of ENOENT when there's a d/f situation, but both have the same
practical effect: that ref doesn't exist.

I.e., this lookup has _always_ been broken, even in the "reading" case.
It's just that the fix from 31824d180d (correctly) made git-branch more
careful about handling the cases where we couldn't resolve a HEAD.

So this patch fixes the problem:

diff --git a/refs.c b/refs.c
index df075fcd06..2ba74720c8 100644
--- a/refs.c
+++ b/refs.c
@@ -1435,7 +1435,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store 
*refs,
if (refs_read_raw_ref(refs, refname,
  sha1, _refname, _flags)) {
*flags |= read_flags;
-   if (errno != ENOENT || (resolve_flags & 
RESOLVE_REF_READING))
+   if ((errno != ENOENT && errno != EISDIR) ||
+   (resolve_flags & RESOLVE_REF_READING))
return NULL;
hashclr(sha1);
if (*flags & REF_BAD_NAME)

but seems to stimulate a test failure in t3308. I have a suspicion that
I've just uncovered another bug, but I'll dig in that. In the meantime I
wanted to post this update in case anybody else was looking into it.

-Peff

[PATCH v3] run-command: add hint when a hook is ignored

[Damien Marié]

When an hook is present but the file is not set as executable then git will
ignore the hook.
For now this is silent which can be confusing.

This commit adds this warning to improve the situation:

  hint: The 'pre-commit' hook was ignored because it's not set as executable.
  hint: You can disable this warning with `git config advice.ignoredHook false`

To allow the old use-case of enabling/disabling hooks via the executable flag a
new setting is introduced: advice.ignoredHook.

Signed-off-by: Damien Marié 
---
 Documentation/config.txt   |  3 +++
 advice.c   |  2 ++
 advice.h   |  1 +
 contrib/completion/git-completion.bash |  1 +
 run-command.c  | 18 +++
 t/t7520-ignored-hook-warning.sh| 41 ++
 6 files changed, 66 insertions(+)
 create mode 100755 t/t7520-ignored-hook-warning.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb046..9abca499f725c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -351,6 +351,9 @@ advice.*::
addEmbeddedRepo::
Advice on what to do when you've accidentally added one
git repo inside of another.
+   ignoredHook::
+   Advice shown if an hook is ignored because the it's not
+   set as executable.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index d81e1cb7425b0..970bd2b71bf53 100644
--- a/advice.c
+++ b/advice.c
@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
+int advice_ignored_hook = 1;
 
 static struct {
const char *name;
@@ -38,6 +39,7 @@ static struct {
{ "objectnamewarning", _object_name_warning },
{ "rmhints", _rm_hints },
{ "addembeddedrepo", _add_embedded_repo },
+   { "ignoredhook", _ignored_hook},
 
/* make this an alias for backward compatibility */
{ "pushnonfastforward", _push_update_rejected }
diff --git a/advice.h b/advice.h
index c84a44531c7d8..f525d6f89cb44 100644
--- a/advice.h
+++ b/advice.h
@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
+extern int advice_ignored_hook;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d93441747523a..a331ccc556b01 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2350,6 +2350,7 @@ _git_config ()
advice.rmHints
advice.statusHints
advice.statusUoption
+   advice.ignoredHook
alias.
am.keepcr
am.threeWay
diff --git a/run-command.c b/run-command.c
index b5e6eb37c0eb3..288abbba5afd7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "thread-utils.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1169,11 +1170,28 @@ const char *find_hook(const char *name)
strbuf_reset();
strbuf_git_path(, "hooks/%s", name);
if (access(path.buf, X_OK) < 0) {
+   int err = errno;
+
 #ifdef STRIP_EXTENSION
strbuf_addstr(, STRIP_EXTENSION);
if (access(path.buf, X_OK) >= 0)
return path.buf;
+   if (errno == EACCES)
+   err = errno;
 #endif
+
+   if (err == EACCES && advice_ignored_hook) {
+   static struct string_list advise_given = 
STRING_LIST_INIT_DUP;
+   
+   if (!string_list_lookup(_given, name)) {
+   string_list_insert(_given, name);
+   advise(_("The '%s' hook was ignored because "
+   "it's not set as 
executable.\n"
+   "You can disable this 
warning with "
+   "`git config 
advice.ignoredHook false`."),
+   path.buf);
+   }
+   }
return NULL;
}
return path.buf;
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
new file mode 100755
index 0..634fb7f23a040
--- /dev/null
+++ b/t/t7520-ignored-hook-warning.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='ignored hook warning'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   hookdir="$(git rev-parse --git-dir)/hooks" &&
+   hook="$hookdir/pre-commit" &&
+