Re: [PATCH v2 4/9] object: add clear_commit_marks_all()

[Jeff King]

On Mon, Dec 25, 2017 at 06:44:58PM +0100, René Scharfe wrote:

> Add a function for clearing the commit marks of all in-core commit
> objects.  It's similar to clear_object_flags(), but more precise, since
> it leaves the other object types alone.  It still has to iterate through
> them, though.

Makes sense.

Is it worth having:

  void clear_object_flags_from_type(int type, unsigned flags);

rather than having two near-identical functions? I guess we'd need some
way of saying "all types" to reimplement clear_object_flags() as a
wrapper (OBJ_NONE, I guess?).

The run-time check is maybe a little bit slower in the middle of a tight
loop, but I'm not sure it would matter much (I'd actually be curious if
this approach is faster than the existing traversal code, too).

-Peff

Re: What's cooking in git.git (Jan 2018, #02; Tue, 9)

[Ævar Arnfjörð Bjarmason]

On Tue, Jan 09 2018, Junio C. Hamano jotted:

> * ab/wildmatch-tests (2018-01-04) 7 commits
>   (merged to 'next' on 2018-01-09 at 09f0b84098)
>  + wildmatch test: create & test files on disk in addition to in-memory
>  + wildmatch test: perform all tests under all wildmatch() modes
>  + wildmatch test: remove dead fnmatch() test code
>  + wildmatch test: use a paranoia pattern from nul_match()
>  + wildmatch test: don't try to vertically align our output
>  + wildmatch test: use more standard shell style
>  + wildmatch test: indent with tabs, not spaces
>
>  More tests for wildmatch functions.
>
>  Will cook in 'next'.

Please don't merge it down for now. I've got a WIP resubmission of this
which rewrites most of the later part of the series & addresses various
issues raised.

> * ab/perf-grep-threads (2018-01-04) 1 commit
>   (merged to 'next' on 2018-01-09 at 8fe1d71894)
>  + perf: amend the grep tests to test grep.threads
>
>  More perf tests for threaded grep
>
>  Will cook in 'next'.

Re: the concern raised in xmqqa7xsaqki@gitster.mtv.corp.google.com I
think it makes sense to just document (and I can do that if you agree)
that:

test_expect_success SOME_PREREQ,$SOME_OTHER_PREREQ,$ANOTHER_ONE [...]

Will work as far as prereqs goes even though the variables might be
empty. It's much less verbose than the proposed alternative, and easy to
support.

>  Will [cook in|merge to] 'next'.

Refresh my memory, that means merge down post-2.16.0 at this point,
right?

Re: [PATCH v2 1/9] commit: avoid allocation in clear_commit_marks_many()

[Jeff King]

On Mon, Dec 25, 2017 at 06:43:37PM +0100, René Scharfe wrote:

> Pass the entries of the commit array directly to clear_commit_marks_1()
> instead of adding them to a commit_list first.  The function clears the
> commit and any first parent without allocation; only higher numbered
> parents are added to a list for later treatment.  This change extends
> that optimization to clear_commit_marks_many().

It took a bit of head-scratching to see that is indeed what
clear_commit_marks_1 does. I suspect this doesn't make all that big a
difference in practice (after all, we're doing an allocation for each
merge anyway, so allocating for the tips is likely to be a subset), but
it doesn't hurt to do so.

-Peff

Re: [PATCH v4 0/4] Add --no-ahead-behind to status

[Jeff King]

On Tue, Jan 09, 2018 at 09:29:31AM -0500, Derrick Stolee wrote:

> > > But even still, finding small answers quickly and accurately and punting
> > > to "really far, I didn't bother to compute it" on the big ones would be
> > > an improvement over always punting.
> > Indeed. The longer I think about it, the more I like the "100+ commits
> > apart" idea.
> > 
> 
> Again, I strongly suggest we drop this approach because it will be more pain
> than it is worth.

To be clear, which approach are we talking about? I think there are
three options:

  1. The user tells us not to bother computing real ahead/behind values.
 We always say "same" or "not the same".

  2. The user tells us not to bother computing ahead/behind values
 with more effort than N. After traversing N commits without getting
 an answer, we say "same" or "not the same". But we may sometimes
 give a real answer if we found it within N.

  3. The user tells us not to spend more effort than N. After traversing
 N commits we try to make some partial statement based on
 generations (or commit timestamps as a proxy for them).

I agree that (3) is probably not going to be useful enough in the
general case to merit the implementation effort and confusion. But is
there anything wrong with (2)?

-Peff

Re: [PATCH v4 0/4] Add --no-ahead-behind to status

[Jeff King]

On Tue, Jan 09, 2018 at 02:15:47PM +0100, Johannes Schindelin wrote:

> > I like this direction a lot. I had hoped we could say "100+ commits
> > ahead",
> 
> How about "100+ commits apart" instead?

Yeah, that is probably more accurate for the general case.

> > but I don't think we can do so accurately without generation numbers.
> 
> Even with generation numbers, it is not possible to say whether two given
> commits reflect diverging branches or have an ancestor-descendant
> relationship (or in graph speak: whether they are comparable).
> 
> It could potentially make it possible to cut off the commit traversal, but
> I do not even see how that would be possible.
> 
> The only thing you could say for sure is that two different commits with
> the same generation number are for sure "uncomparable", i.e. reflect
> diverging branches.

I think sometimes we can say more. E.g., imagine we have two commits, A
and B, with generation numbers N and N+1000. We walk back 100 commits
deep from B and end up at a commit around N+900. We know that there are
at least 100 commits in B that are not in A (the 100 we walked, which we
know cannot be ancestors of A because of their generation numbers).
That's true even if there is no ancestry relationship between the two
commits at all.

But we cannot say in that case how many (if any) commits are in A but
not B. So we can say "100+ commits ahead, unknown behind" (or if the two
generation numbers are reversed, we can say "unknown ahead, 100+ commits
behind).

In the more general case, we could actually walk _past_ generation N,
and end up at N-25 or similar. There we can't say anything intelligent
about the commits with generations <= N. But we could say something like
"there are 75 commits in B that cannot be in A" (note that it's probably
_not_ actually 75; it's however many we walked until we crossed N).

So that was what I was getting at in the earlier mail. We can sometimes
give a partial answer. But I think giving that partial answer is likely
to be more confusing than useful to a user, because there are a lot of
caveats about how much we know for a given situation.

And I think from what you wrote below that you probably agree with that
(that we can make a few claims, but not enough to really be useful).

-Peff

Re: [PATCH 03/20] cat-file: rename variables in ref-filter

[Оля Тележная]

2018-01-10 1:04 GMT+03:00 Junio C Hamano :
> Olga Telezhnaya  writes:
>
>> Rename some variables for easier reading.
>> They point not to values, but to arrays.
>
> Once the code is written and people start to build on top, a change
> like this is not worth the code churn, especially because there are
> two equally valid schools of naming convention.
>
>  - When you have an array, each of whose 20 slots holds a single
>dosh, I would prefer to call the array dosh[20], not doshes[20],
>so that I can refer to the seventh dosh as "dosh[7]".
>
>  - If you more often refer to the array as a whole (than you refer
>to individual elements) and want to stress the fact that the
>array holds multiple elements in it, I can understand that you
>may be tempted to call the whole array "doshes[]".
>
> So please drop this and other "rename variables" patches from the
> series.
>
>

OK, I will revert that. I have done this because it's hard for me to
keep in mind that it's not just a simple pointer to a single value,
and I tried to make the code more intuitive.

[PATCH v2 0/2] Doc/submodules: a few updates

[Kaartic Sivaraam]

Quoting from v1,

These are just a few improvements that I thought would make the 
documentation
related to submodules a little better in various way such as readability,
consistency etc., These were things I noticed while reading thise documents.

Change since v2: 

 I've squashed the fine grained patches into 2 patches that touch two distinct
 documents. This v2 conatins a lot of changes suggested for v1 and few that I
 caught by myself since v1.

This patch is based on 'master' just like v1.

Inter-word-diff v1..v2:

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 5c4d941cc..801d291ca 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -140,7 +140,7 @@ through the `submodule..update` configuration are:
checked out in the submodule on a detached HEAD.
+
If `--force` is specified, the submodule will be checked out (using
`git checkout [---force` if appropriate),-]{+--force`),+} even if the commit 
specified
in the index of the containing repository already matches the commit
checked out in the submodule.

diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index 339fb73db..ce2369c2d 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -36,7 +36,7 @@ The `gitlink` entry contains the object name of the commit 
that the
superproject expects the submodule’s working directory to be at.

The section `submodule.foo.*` in the `.gitmodules` file gives additional
hints to [-Gits-]{+Git's+} porcelain layer. For example, the `submodule.foo.url`
setting specifies where to obtain the submodule.

Submodules can be used for at least two different use cases:
@@ -51,21 +51,21 @@ Submodules can be used for at least two different use cases:

2. Splitting a (logically single) project into multiple
   repositories and tying them back together. This can be used to
   overcome current limitations of [-Gits-]{+Git's+} implementation to have
   finer grained access:

* Size of the [-git-]{+Git+} repository:
  In its current form Git scales up poorly for large repositories containing
  content that is not compressed by delta computation between trees.
  [-Therefore-]{+For example,+} you can use submodules to hold large binary 
assets
  and these repositories [-are then-]{+can be+} shallowly cloned such that 
you do not
  have a large history locally.
* Transfer size:
  In its current form Git requires the whole working tree present. It
  does not allow partial trees to be transferred in fetch or clone.
  If [-you have your-]{+the+} project [-as-]{+you work on consists of+} 
multiple repositories tied
  together as submodules in a superproject, you can avoid fetching the
  working trees of the repositories you are not interested in.
* Access control:
  By restricting user access to submodules, this can be used to implement
  read/write policies for different users.
@@ -76,10 +76,10 @@ The configuration of submodules
Submodule operations can be configured using the following mechanisms
(from highest to lowest precedence):

 * The command line [-arguments of-]{+for+} those commands that support taking 
[-submodule-]
[-   specifications.-]{+submodules+}
{+   as part of their pathspecs.+} Most commands have a boolean flag
   [-'--recurse-submodules'-]{+`--recurse-submodules`+} which specify whether 
[-they should-]{+to+} recurse into submodules.
   Examples are [-`ls-files` or-]{+`grep` and+} `checkout`.
   Some commands take enums, such as `fetch` and `push`, where you can
   specify how submodules are affected.

@@ -101,17 +101,17 @@ remotes are configured in the submodule as usual in the 
`$GIT_DIR/config`
file.

 * The configuration file `$GIT_DIR/config` in the superproject.
   [-Typical configuration at this place is controlling if a submodule-]
[-   is recursed-]{+Git only recurses+} into [-at all via the `active` flag for 
example.-]{+active submodules (see 'ACTIVE SUBMODULES'+}
{+   section below).+}
+
If the submodule is not yet initialized, then the configuration
inside the submodule does not exist yet, so[-configuration-] where to
obtain the submodule from is configured here for example.

 * The `.gitmodules` file inside the superproject. [-Additionally, if mapping-]
[-   is required between a submodule's name and its path, a-]{+A+} project 
usually
   uses this file to suggest defaults for the upstream collection
   of [-repositories.-]{+repositories for the mapping that is required between 
a+}
{+   submodule's name and its path.+}
+
This file mainly serves as the mapping between the name and path of submodules
in the superproject, such that the submodule's Git directory can be
@@ -141,8 +141,8 @@ directory is automatically moved to 
`$GIT_DIR/modules//`
of the superproject.

 * Deinitialized submodule: A `gitlink`, and a `.gitmodules` entry,
but no submodule working directory. The submodule’s [-git-]{+Git+} 

[PATCH v2 2/2] Doc/git-submodule: improve readability and grammar of a sentence

[Kaartic Sivaraam]

While at it, correctly quote important words.

Signed-off-by: Kaartic Sivaraam 
---
 Documentation/git-submodule.txt | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index ff612001d..801d291ca 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -132,15 +132,15 @@ expects by cloning missing submodules and updating the 
working tree of
 the submodules. The "updating" can be done in several ways depending
 on command line options and the value of `submodule..update`
 configuration variable. The command line option takes precedence over
-the configuration variable. if neither is given, a checkout is performed.
-update procedures supported both from the command line as well as setting
-`submodule..update`:
+the configuration variable. If neither is given, a 'checkout' is performed.
+The 'update' procedures supported both from the command line as well as
+through the `submodule..update` configuration are:
 
checkout;; the commit recorded in the superproject will be
checked out in the submodule on a detached HEAD.
 +
 If `--force` is specified, the submodule will be checked out (using
-`git checkout --force` if appropriate), even if the commit specified
+`git checkout --force`), even if the commit specified
 in the index of the containing repository already matches the commit
 checked out in the submodule.
 
@@ -150,8 +150,8 @@ checked out in the submodule.
merge;; the commit recorded in the superproject will be merged
into the current branch in the submodule.
 
-The following procedures are only available via the `submodule..update`
-configuration variable:
+The following 'update' procedures are only available via the
+`submodule..update` configuration variable:
 
custom command;; arbitrary shell command that takes a single
argument (the sha1 of the commit recorded in the
-- 
2.16.0.rc0.223.g4a4ac8367

[PATCH v2 1/2] Doc/gitsubmodules: make some changes to improve readability and syntax

[Kaartic Sivaraam]

* Only mention porcelain commands in examples

* Split a sentence for better readability

* Add missing apostrophes

* Clearly specify the advantages of using submodules

* Avoid abbreviations

* Use "Git" consistently

* Improve readability of certain lines

* Clarify when a submodule is considered active

Helped-by: Eric Sunshine 
Helped-by: Stefan Beller 
Signed-off-by: Kaartic Sivaraam 
---
 Documentation/gitsubmodules.txt | 93 +++--
 1 file changed, 72 insertions(+), 21 deletions(-)

diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
index 46cf120f6..ce2369c2d 100644
--- a/Documentation/gitsubmodules.txt
+++ b/Documentation/gitsubmodules.txt
@@ -36,8 +36,8 @@ The `gitlink` entry contains the object name of the commit 
that the
 superproject expects the submodule’s working directory to be at.
 
 The section `submodule.foo.*` in the `.gitmodules` file gives additional
-hints to Gits porcelain layer such as where to obtain the submodule via
-the `submodule.foo.url` setting.
+hints to Git's porcelain layer. For example, the `submodule.foo.url`
+setting specifies where to obtain the submodule.
 
 Submodules can be used for at least two different use cases:
 
@@ -51,18 +51,21 @@ Submodules can be used for at least two different use cases:
 
 2. Splitting a (logically single) project into multiple
repositories and tying them back together. This can be used to
-   overcome current limitations of Gits implementation to have
+   overcome current limitations of Git's implementation to have
finer grained access:
 
-* Size of the git repository:
+* Size of the Git repository:
   In its current form Git scales up poorly for large repositories 
containing
   content that is not compressed by delta computation between trees.
-  However you can also use submodules to e.g. hold large binary assets
-  and these repositories are then shallowly cloned such that you do not
+  For example, you can use submodules to hold large binary assets
+  and these repositories can be shallowly cloned such that you do not
   have a large history locally.
 * Transfer size:
   In its current form Git requires the whole working tree present. It
   does not allow partial trees to be transferred in fetch or clone.
+  If the project you work on consists of multiple repositories tied
+  together as submodules in a superproject, you can avoid fetching the
+  working trees of the repositories you are not interested in.
 * Access control:
   By restricting user access to submodules, this can be used to implement
   read/write policies for different users.
@@ -73,9 +76,10 @@ The configuration of submodules
 Submodule operations can be configured using the following mechanisms
 (from highest to lowest precedence):
 
- * The command line for those commands that support taking submodule specs.
-   Most commands have a boolean flag '--recurse-submodules' whether to
-   recurse into submodules. Examples are `ls-files` or `checkout`.
+ * The command line for those commands that support taking submodules
+   as part of their pathspecs. Most commands have a boolean flag
+   `--recurse-submodules` which specify whether to recurse into submodules.
+   Examples are `grep` and `checkout`.
Some commands take enums, such as `fetch` and `push`, where you can
specify how submodules are affected.
 
@@ -87,8 +91,8 @@ Submodule operations can be configured using the following 
mechanisms
 For example an effect from the submodule's `.gitignore` file
 would be observed when you run `git status --ignore-submodules=none` in
 the superproject. This collects information from the submodule's working
-directory by running `status` in the submodule, which does pay attention
-to its `.gitignore` file.
+directory by running `status` in the submodule while paying attention
+to the `.gitignore` file of the submodule.
 +
 The submodule's `$GIT_DIR/config` file would come into play when running
 `git push --recurse-submodules=check` in the superproject, as this would
@@ -97,20 +101,20 @@ remotes are configured in the submodule as usual in the 
`$GIT_DIR/config`
 file.
 
  * The configuration file `$GIT_DIR/config` in the superproject.
-   Typical configuration at this place is controlling if a submodule
-   is recursed into at all via the `active` flag for example.
+   Git only recurses into active submodules (see 'ACTIVE SUBMODULES'
+   section below).
 +
 If the submodule is not yet initialized, then the configuration
-inside the submodule does not exist yet, so configuration where to
+inside the submodule does not exist yet, so where to
 obtain the submodule from is configured here for example.
 
- * the `.gitmodules` file inside the superproject. Additionally to the
-   required mapping between submodule's name and path, a project usually
+ * The 

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Junio C Hamano]

Dmitry Torokhov  writes:

>> I had prepare-commit-msg hook that would scrub "Patchwork-ID: " tags
>> form commit messages and would update input mailing list patchwork to
>> mark corresponding patches as "accepted" when I cherry pick form
>> WIP/review queue into branches that I publish, but that recently stopped
>> working if I do a simple cherry-pick.
>
> This seems like a regression, at least for my use case. Unfortunately
> my mail seems to get lost in the mailing list noise...

Possibly.  Can you bisect to see which commit broke things for you?
That would allow people who know what they themselves broke better
than I do to take a look ;-)

Thanks.

Re: prepare-commit-msg hook no longer run for cherry-pick?

[Dmitry Torokhov]

Hi Junio,

On Fri, Jan 5, 2018 at 10:48 AM, Dmitry Torokhov
 wrote:
> Hi,
>
> I had prepare-commit-msg hook that would scrub "Patchwork-ID: " tags
> form commit messages and would update input mailing list patchwork to
> mark corresponding patches as "accepted" when I cherry pick form
> WIP/review queue into branches that I publish, but that recently stopped
> working if I do a simple cherry-pick.

This seems like a regression, at least for my use case. Unfortunately
my mail seems to get lost in the mailing list noise... Please let me
know if this is indeed broken or I need to adjust my workflow.

Thanks!

> If I specify that I want to edit
> the message, then the hook is executed:
>
> dtor@dtor-ws:~/kernel/master (for-linus)$ GIT_TRACE=2 git cherry-pick 
> ff162c1554efe951ba6c7a19a228fc76a91fe1ed
> 10:43:12.832426 git.c:344   trace: built-in: git 'cherry-pick' 
> 'ff162c1554efe951ba6c7a19a228fc76a91fe1ed'
> [for-linus 48bc600a3659] Input: raydium_i2c_ts - include hardware version in 
> firmware name
> Author: Jeffrey Lin 
> Date: Thu Jan 4 21:35:23 2018 -0800
> 1 file changed, 12 insertions(+), 2 deletions(-)
> dtor@dtor-ws:~/kernel/master (for-linus)$ gti reset --hard HEAD^
> HEAD is now at 02a0d9216d4d Input: xen-kbdfront - do not advertise 
> multi-touch pressure support
> dtor@dtor-ws:~/kernel/master (for-linus)$ GIT_TRACE=2 git cherry-pick -e 
> ff162c1554efe951ba6c7a19a228fc76a91fe1ed
> 10:43:24.433162 git.c:344   trace: built-in: git 'cherry-pick' 
> '-e' 'ff162c1554efe951ba6c7a19a228fc76a91fe1ed'
> 10:43:24.782355 run-command.c:627   trace: run_command: 'commit' '-n' '-e'
> 10:43:24.786460 git.c:344   trace: built-in: git 'commit' '-n' 
> '-e'
> 10:43:25.082164 run-command.c:627   trace: run_command: 
> '.git/hooks/prepare-commit-msg' '.git/COMMIT_EDITMSG' 'merge'
> hint: Waiting for your editor to close the file...
> 10:43:31.491551 run-command.c:627   trace: run_command: 'vim' 
> '/usr/local/goo gle/home/dtor/kernel/master/.git/COMMIT_EDITMSG'
> [for-linus 039c57df0ec8] Input: raydium_i2c_ts - include hardware version in 
> firmware name
> Author: Jeffrey Lin 
> Date: Thu Jan 4 21:35:23 2018 -0800
> 1 file changed, 12 insertions(+), 2 deletions(-)
> dtor@dtor-ws:~/kernel/master (for-linus)$
>
> Also note that the argument to the hook is "merge" whereas I think it
> used to be "cherry-pick" earlier.
>
> Is this behavior intentional? dpkg reports version as 2.16.0~rc0+next.
>
> Thanks!
>
> --
> Dmitry

-- 
Dmitry

RE: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

On January 9, 2018 6:01 PM, Johannes Sixt wrote:
> Am 09.01.2018 um 19:12 schrieb Randall S. Becker:
> > This patch create a configuration variable PATH_MAX that corresponds
> > with the value in limits.h. The value of PATH_MAX, if supplied, is
> > added to BASIC_CFLAGS and will validate with limits.h. PATH_MAX is
> > also added to GIT-BUILD-OPTIONS and is available in the git test
> > suite.
> >
> > This patch also creates a test_expected_success_cond, taking a single
> > function as first argument. In the t0001-init.sh case, subtest 34 this
> > function is test_path_max_is_sane, although any
> > 0/1 returning function can be used. The prototype allows the long base
> > path test to be skipped if PATH_MAX is less than 2048 bytes.
> 
> OK, but...

This was suggested in a previous thread, so I prototyped. I'm ok with not going 
ahead with it but still, it might help some platforms. Still, see down.

> > diff --git a/t/t0001-init.sh b/t/t0001-init.sh index c4814d2..58dad87
> > 100755
> > --- a/t/t0001-init.sh
> > +++ b/t/t0001-init.sh
> > @@ -315,7 +315,7 @@ test_expect_success 'init with separate gitdir' '
> > test_path_is_dir realgitdir/refs
> >   '
> >
> > -test_expect_success 'init in long base path' '
> > +test_expect_success_cond 'test_path_max_is_sane' 'init in long base path'
> '
> > # exceed initial buffer size of strbuf_getcwd()
> > component=123456789abcdef &&
> > test_when_finished "chmod 0700 $component; rm -rf $component"
> &&
> 
> ... why would you want to skip this test? If I'm reading the test case 
> correctly,
> it requires only a path length of 127 plus whatever your build directory is 
> plus
> a score for the trash directory. That should pose a problem only if your
> system is even more crippled than Windows with its PATH_MAX of 260.

I'm encountering strange warnings, while looking into the details of what test 
t0001 fails in spots. These include:
#24 warning: templates not found 

Re: [PATCH 26/26] remote-curl: implement connect-half-duplex command

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:28 -0800
Brandon Williams  wrote:

> Teach remote-curl the 'connect-half-duplex' command which is used to
> establish a half-duplex connection with servers which support protocol
> version 2.  This allows remote-curl to act as a proxy, allowing the git
> client to communicate natively with a remote end, simply using
> remote-curl as a pass through to convert requests to http.
> 
> Signed-off-by: Brandon Williams 
> ---
>  remote-curl.c  | 185 
> -
>  t/t5701-protocol-v2.sh |  41 +++
>  2 files changed, 224 insertions(+), 2 deletions(-)

I didn't look at the usage of the curl API in detail, but overall this
looks good. I'm pleasantly surprised that it didn't take so many lines
of code as I expected.

Overall everything looks good, except for the points that I have brought
up in my other e-mails.

> diff --git a/remote-curl.c b/remote-curl.c
> index 4086aa733..b63b06398 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c

[snip]

> +struct proxy_state {
> + char *service_name;
> + char *service_url;
> + char *hdr_content_type;
> + char *hdr_accept;

Maybe document that the above 3 fields (service_url to hdr_accept) are
cached because we need to pass them to curl_easy_setopt() for every
request.

Re: [PATCH 20/26] fetch-pack: perform a fetch using v2

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:22 -0800
Brandon Williams  wrote:

> +static enum ack_type process_ack(const char *line, struct object_id *oid)
> +{
> + const char *arg;
> +
> + if (!strcmp(line, "NAK"))
> + return NAK;
> + if (skip_prefix(line, "ACK ", )) {
> + if (!parse_oid_hex(arg, oid, )) {
> + if (strstr(arg, "continue"))
> + return ACK_continue;

This function seems to be only used for v2, so I don't think we need to
parse "continue".

Also, maybe describe the plan for supporting functionality not supported
yet (e.g. server-side declaration of shallows and client-side "deepen").

It may be possible to delay support for server-side shallows on the
server (that is, only implement support for it in the client) since the
server can just declare that it doesn't support protocol v2 when serving
such repos (although it might just be easier to implement server-side
support in this case).

For "deepen", we need support for it both on the client and the server
now unless we plan to declare a "deepen" capability in the future (then,
as of these patches, clients that require "deepen" will use protocol v1;
when a new server declares "deepen", old clients will ignore it and keep
the status quo, and new clients can then use "deepen").

There may be others that I've missed.

Re: [ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
>> index f1678851de9..470107248eb 100644
>> --- a/t/t0021/rot13-filter.pl
>> +++ b/t/t0021/rot13-filter.pl
>> @@ -31,7 +31,22 @@
>>  #
>>  
>>  use 5.008;
>> -use lib (split(/:/, $ENV{GITPERLLIB}));
>> +sub gitperllib {
>> +...
>> +if ($ENV{GITPERLLIB} =~ /;/) {
>> +return split(/;/, $ENV{GITPERLLIB});
>> +}
>> +return split(/:/, $ENV{GITPERLLIB});
>> +}
>
> This cannot be the whole story for a few reasons.
>
>  - In t/test-lib.sh we see this:
>
>
> GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
>export GITPERLLIB
>
>If this part wants to split with ';', then the joining needs to
>be done with ';' to match, no?
>
>  - In addition to t0021, there are similar split with colon in 0202,
>9000 and 9700, yet I am getting the feeling that you observed the
>issue only in0021, to which I do not think of a good explanation
>why.

This somehow vaguely rang a bell, and I dug this thing up from the
archive, [*1*] which ended like so:

>> In our C code, we have "#define PATH_SEP ';'", and encourage
>> our code to be careful and use it.  Is there something
>> similar for Perl scripts, I wonder.
>>
> We probably should find a better solution to allow this to
> work with windows style paths...? I know that python provides
> os.pathsep, but I haven't seen an equivalent for perl yet.
>
> The Env[1] core modules suggests using
> $Config::Config{path_sep}[2]..  maybe we should be using this?

I was testing this recently on the Perl included with Git for
Windows and it returns : for the path separator even though it's
on Windows, so I don't think that would work. The Perl in Git
for Windows seems to want UNIX-style inputs (something Dscho
seemed to allude to in his response earlier.). I'm not sure why
it's that way, but he probably knows.

Your initial response in this thread made it sound as if -rc1 is the
only thing that changed, but looking at the differences between -rc0
and -rc1, which does not touch t0021 or any other instances of
"split(/:/, $ENV{GITPERLLIB})", I am wondering if it is possible
that perhaps the way Perl is built for GfW has been changed recently
and we can safely and sanely use $Config::Config{path_sep} (contrary
to what was found in late Oct in the message quoted above) now?

In any case, I'd prefer this issue to be resolved properly before
-rc2; a patch to t0021/rot13-filter.pl alone does not smell like a
"proper solution" that is based on the understanding of the root
cause (and that is why I spent time digging the list archive).

Thanks.


[Reference]

*1* 
https://public-inbox.org/git/cagyf7-ejkahgwkn9tro4mfvba9odbwcza9jh0pk6ze6fosk...@mail.gmail.com/

What's cooking in git.git (Jan 2018, #02; Tue, 9)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ab/dc-sha1-loose-ends (2017-12-28) 1 commit
  (merged to 'next' on 2018-01-05 at 7186e81793)
 + Makefile: NO_OPENSSL=1 should no longer imply BLK_SHA1=1

 Tying loose ends for the recent integration work of
 collision-detecting SHA-1 implementation.


* bc/submitting-patches-in-asciidoc (2018-01-03) 1 commit
  (merged to 'next' on 2018-01-05 at 92dd960a56)
 + doc/SubmittingPatches: improve text formatting

 Doc readability update.


* bp/fsmonitor (2017-12-18) 1 commit
  (merged to 'next' on 2017-12-27 at ce216e2978)
 + p7519: improve check for prerequisite WATCHMAN

 Test fix.


* bw/path-doc (2017-12-13) 1 commit
  (merged to 'next' on 2017-12-19 at 2cddee77ca)
 + path: document path functions

 Doc updates.


* bw/protocol-v1 (2018-01-04) 1 commit
  (merged to 'next' on 2018-01-05 at 47a5e0039a)
 + http: fix v1 protocol tests with apache httpd < 2.4

 Test fix for a topic already in 'master'.


* cc/skip-to-optional-val (2017-12-11) 7 commits
  (merged to 'next' on 2017-12-27 at 1b189d8556)
 + t4045: reindent to make helpers readable
 + diff: add tests for --relative without optional prefix value
 + diff: use skip_to_optional_arg_default() in parsing --relative
 + diff: use skip_to_optional_arg_default()
 + diff: use skip_to_optional_arg()
 + index-pack: use skip_to_optional_arg()
 + git-compat-util: introduce skip_to_optional_arg()

 Introduce a helper to simplify code to parse a common pattern that
 expects either "--key" or "--key=".


* db/doc-config-section-names-with-bs (2017-12-22) 1 commit
  (merged to 'next' on 2017-12-28 at e744b99449)
 + config.txt: document behavior of backslashes in subsections

 Doc update.


* ew/svn-crlf (2017-12-14) 2 commits
  (merged to 'next' on 2017-12-27 at 1b81bd634d)
 + Merge branch 'svn-crlf' of git://bogomips.org/git-svn into ew/svn-crlf
 + git-svn: convert CRLF to LF in commit message to SVN

 "git svn" has been updated to strip CRs in the commit messages, as
 recent versions of Subversion rejects them.


* hi/merge-verify-sig-config (2017-12-19) 3 commits
  (merged to 'next' on 2017-12-27 at 34360fb1c1)
 + t5573, t7612: clean up after unexpected success of 'pull' and 'merge'
  (merged to 'next' on 2017-12-14 at cdc511dc36)
 + t: add tests for pull --verify-signatures
 + merge: add config option for verifySignatures

 "git merge" learned to pay attention to merge.verifySignatures
 configuration variable and pretend as if '--verify-signatures'
 option was given from the command line.


* jd/fix-strbuf-add-urlencode-bytes (2017-12-22) 1 commit
  (merged to 'next' on 2017-12-28 at 7f4f291966)
 + strbuf: fix urlencode format string on signed char

 Bytes with high-bit set were encoded incorrectly and made
 credential helper fail.


* jh/memihash-opt (2017-12-22) 1 commit
  (merged to 'next' on 2017-12-28 at bf96e0d849)
 + t/helper/test-lazy-name-hash: fix compilation

 Squelch compiler warning.


* jh/partial-clone-doc (2017-12-14) 1 commit
  (merged to 'next' on 2017-12-27 at 3695847773)
 + partial-clone: design doc


* jk/doc-diff-options (2018-01-04) 1 commit
  (merged to 'next' on 2018-01-05 at 11a42ea807)
 + docs/diff-options: clarify scope of diff-filter types

 Doc update.


* jk/test-suite-tracing (2017-12-08) 4 commits
  (merged to 'next' on 2017-12-27 at 7034a51474)
 + t/Makefile: introduce TEST_SHELL_PATH
 + test-lib: make "-x" work with "--verbose-log"
 + t5615: avoid re-using descriptor 4
 + test-lib: silence "-x" cleanup under bash

 Assorted fixes around running tests with "-x" tracing option.


* js/enhanced-version-info (2017-12-14) 2 commits
  (merged to 'next' on 2017-12-27 at a95dd96a78)
 + version --build-options: report commit, too, if possible
 + version --build-options: also report host CPU

 "git version --build-options" learned to report the host CPU and
 the exact commit object name the binary was built from.


* js/sequencer-cleanups (2017-12-27) 5 commits
  (merged to 'next' on 2017-12-28 at 23c10afb09)
 + sequencer: do not invent whitespace when transforming OIDs
 + sequencer: report when noop has an argument
 + sequencer: remove superfluous conditional
 + sequencer: strip bogus LF at end of error messages
 + rebase: do not continue when the todo list generation failed

 Code cleanup.


* js/test-with-ws-in-path (2018-01-03) 2 commits
  (merged to 'next' on 2018-01-05 at 6438e1c186)
 + t0302 & t3900: add forgotten quotes
 + Allow the test suite to pass in a directory whose name contains spaces

 Test fixes.


* jt/transport-hide-vtable 

Re: [PATCH 07/20] cat-file: reuse parse_ref_filter_atom

[Junio C Hamano]

Olga Telezhnaya  writes:

> +static int is_atom(const char *atom, const char *s, int slen)
> +{
> +...
> +}
> +
> +static void expand_atom_into_fields(const char *atom, int len,
> + struct expand_data *data)
> +{
> +...
> +}
> ...
> -static int is_atom(const char *atom, const char *s, int slen)
> -{
> -...
> -}
> -
> -static void expand_atom_into_fields(const char *atom, int len,
> - struct expand_data *data)
> -{
> -...
> -}
> -
>  /*
>   * Make sure the format string is well formed, and parse out
>   * the used atoms.

There is no need to move these in this step if the previous step
planned ahead well, and that is something you can correct by
pretending to be perfect with "rebase -i" ;-) That also will help
reduce reviewers' load.

> @@ -726,6 +741,7 @@ int verify_ref_format(struct ref_format *format)
>  {
>   const char *cp, *sp;
>  
> + cat_file_info = format->cat_file_data;
>   format->need_color_reset_at_eol = 0;
>   for (cp = format->format; *cp && (sp = find_next(cp)); ) {
>   const char *color, *ep = strchr(sp, ')');
> @@ -736,9 +752,10 @@ int verify_ref_format(struct ref_format *format)
>   /* sp points at "%(" and ep points at the closing ")" */
>  
>   if (format->cat_file_data)
> - expand_atom_into_fields(sp + 2, ep - sp - 2,
> - format->cat_file_data);
> - else
> + {
> + at = parse_ref_filter_atom(format, valid_cat_file_atoms,
> +
> ARRAY_SIZE(valid_cat_file_atoms), sp + 2, ep);
> + } else
>   {
>   at = parse_ref_filter_atom(format, valid_atoms,
>  ARRAY_SIZE(valid_atoms), sp 
> + 2, ep);

if/else with bodies that are compound statements are formatted like
so:

if (condition) {
do this;
} else {
do that;
do that, too;
}

Re: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Johannes Sixt]

Am 09.01.2018 um 19:12 schrieb Randall S. Becker:

This patch create a configuration variable PATH_MAX that
corresponds with the value in limits.h. The value of PATH_MAX,
if supplied, is added to BASIC_CFLAGS and will validate with
limits.h. PATH_MAX is also added to GIT-BUILD-OPTIONS and is
available in the git test suite.

This patch also creates a test_expected_success_cond, taking a
single function as first argument. In the t0001-init.sh case,
subtest 34 this function is test_path_max_is_sane, although any
0/1 returning function can be used. The prototype allows the long base
path test to be skipped if PATH_MAX is less than 2048 bytes.


OK, but...


diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index c4814d2..58dad87 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -315,7 +315,7 @@ test_expect_success 'init with separate gitdir' '
test_path_is_dir realgitdir/refs
  '

-test_expect_success 'init in long base path' '
+test_expect_success_cond 'test_path_max_is_sane' 'init in long base path' '
# exceed initial buffer size of strbuf_getcwd()
component=123456789abcdef &&
test_when_finished "chmod 0700 $component; rm -rf $component" &&


... why would you want to skip this test? If I'm reading the test case 
correctly, it requires only a path length of 127 plus whatever your 
build directory is plus a score for the trash directory. That should 
pose a problem only if your system is even more crippled than Windows 
with its PATH_MAX of 260.



+test_path_max_is_sane() {
+   if test -z "$PATH_MAX"
+   then
+   retval=1
+   elif test $PATH_MAX -ge 2048
+   then
+   retval=1
+   else
+   retval=0
+   fi
+   return "$retval"
+}


This can probably be reduced to

test_path_max_is_sane () {
test "${PATH_MAX:-4000}" -ge 2048
}

(Style note: we have a blank before the () pair in shell scripts.)

-- Hannes

Re: [PATCHv4 0/4] Fix --recurse-submodules for submodule worktree changes

[Junio C Hamano]

Stefan Beller  writes:

> v4:
> Stefan Beller (4):
>   t/lib-submodule-update.sh: clarify test
>   t/lib-submodule-update.sh: Fix test ignoring ignored files in
> submodules
>   unpack-trees: oneway_merge to update submodules
>   submodule: submodule_move_head omits old argument in forced case
>
>  submodule.c   |  4 +++-
>  t/lib-submodule-update.sh | 19 +--
>  unpack-trees.c|  3 +++
>  3 files changed, 23 insertions(+), 3 deletions(-)

Thanks.  This one looks excellent.

Let's merge it to 'next' and cook it there.

Re: WITH ALL DUE RESPECT TO YOUR PERSON

[Lincoln Bah Bah]

Dear Friend,

With due respect to your person and much sincerity of purpose. I have a 
business proposal which I will like to handle with you. $35 million USD is 
involves. But be rest assured that everything is legal and risk free as I have 
concluded all the arrangements and the legal papers that will back the 
transaction up. Kindly indicate your interest as to enable me tell you more 
detail of the proposal.

Waiting for your urgent response.
Yours Faithfully,
Dr.Lincoln Bah Bah

Re: [PATCH 11/26] serve: introduce git-serve

[Brandon Williams]

On 01/09, Jonathan Tan wrote:
> On Tue, 9 Jan 2018 14:16:42 -0800
> Brandon Williams  wrote:
> 
> > All good documentation changes.
> 
> Thanks!
> 
> > > > +   /*
> > > > +* Function called when a client requests the capability as a 
> > > > command.
> > > > +* The command request will be provided to the function via 
> > > > 'keys', the
> > > > +* capabilities requested, and 'args', the command specific 
> > > > parameters.
> > > > +*
> > > > +* This field should be NULL for capabilities which are not 
> > > > commands.
> > > > +*/
> > > > +   int (*command)(struct repository *r,
> > > > +  struct argv_array *keys,
> > > > +  struct argv_array *args);
> > > 
> > > Looking at the code below, I see that the command is not executed unless
> > > advertise returns true - this means that a command cannot be both
> > > supported and unadvertised. Would this be too restrictive? For example,
> > > this would disallow a gradual across-multiple-servers rollout in which
> > > we allow but not advertise a capability, and then after some time,
> > > advertise the capability.
> > 
> > One way to change this would be to just add another function to the
> > struct which is called to check if the command is allowed, instead of
> > relying on the same function to do that for both advertise and
> > allow...though I don't see a big win for allowing a command but not
> > advertising it.
> 
> My rationale for allowing a command but not advertising it is in the
> paragraph above (that you quoted), but if that is insufficient
> rationale, then I agree that we don't need to do this.

I have no issues with adding that functionality, i don't really feel
that strongly one way or another.  Just seemed like additional work for
not much gain right now, key being right now.  It very well may be worth
it for the use case you specified.  If so I can definitely make the
change.

> 
> > > If we change this, then the value parameter of advertise can be
> > > mandatory instead of optional.
> > 
> > I don't see how this fixes the issue you bring up.
> 
> This is a consequence, not a fix - if we were to do as I suggested, then
> we no longer need to invoke advertise to check whether something is
> advertised except when we are advertising them, in which case "value"
> never needs to be NULL.

Oh I understand what you are trying to explain, yes you're right.

-- 
Brandon Williams

Re: [PATCH 11/26] serve: introduce git-serve

[Jonathan Tan]

On Tue, 9 Jan 2018 14:16:42 -0800
Brandon Williams  wrote:

> All good documentation changes.

Thanks!

> > > + /*
> > > +  * Function called when a client requests the capability as a command.
> > > +  * The command request will be provided to the function via 'keys', the
> > > +  * capabilities requested, and 'args', the command specific parameters.
> > > +  *
> > > +  * This field should be NULL for capabilities which are not commands.
> > > +  */
> > > + int (*command)(struct repository *r,
> > > +struct argv_array *keys,
> > > +struct argv_array *args);
> > 
> > Looking at the code below, I see that the command is not executed unless
> > advertise returns true - this means that a command cannot be both
> > supported and unadvertised. Would this be too restrictive? For example,
> > this would disallow a gradual across-multiple-servers rollout in which
> > we allow but not advertise a capability, and then after some time,
> > advertise the capability.
> 
> One way to change this would be to just add another function to the
> struct which is called to check if the command is allowed, instead of
> relying on the same function to do that for both advertise and
> allow...though I don't see a big win for allowing a command but not
> advertising it.

My rationale for allowing a command but not advertising it is in the
paragraph above (that you quoted), but if that is insufficient
rationale, then I agree that we don't need to do this.

> > If we change this, then the value parameter of advertise can be
> > mandatory instead of optional.
> 
> I don't see how this fixes the issue you bring up.

This is a consequence, not a fix - if we were to do as I suggested, then
we no longer need to invoke advertise to check whether something is
advertised except when we are advertising them, in which case "value"
never needs to be NULL.

Re: [PATCH 13/26] connect: request remote refs using v2

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:15 -0800
Brandon Williams  wrote:

> diff --git a/connect.c b/connect.c
> index caa539b75..9badd403f 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -12,9 +12,11 @@
>  #include "sha1-array.h"
>  #include "transport.h"
>  #include "strbuf.h"
> +#include "version.h"
>  #include "protocol.h"
>  
>  static char *server_capabilities;
> +static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;
>  static const char *parse_feature_value(const char *, const char *, int *);
>  
>  static int check_ref(const char *name, unsigned int flags)
> @@ -62,6 +64,33 @@ static void die_initial_contact(int unexpected)
> "and the repository exists."));
>  }
>  
> +static int server_supports_v2(const char *c, int die_on_error)

Document what "c" means.

[snip]

> +static void process_capabilities_v2(struct packet_reader *reader)
> +{
> + while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
> + argv_array_push(_capabilities_v2, reader->line);
> + }

No need for braces on single-line blocks.

> +static int process_ref_v2(const char *line, struct ref ***list)

The "list" is the tail of a linked list, so maybe name it "tail"
instead.

Re: [PATCH 11/26] serve: introduce git-serve

[Brandon Williams]

On 01/09, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:13 -0800
> Brandon Williams  wrote:
> 
> > diff --git a/Documentation/technical/protocol-v2.txt 
> > b/Documentation/technical/protocol-v2.txt
> > new file mode 100644
> > index 0..b87ba3816
> > --- /dev/null
> > +++ b/Documentation/technical/protocol-v2.txt
> 
> I'll review the documentation later, once there is some consensus that
> the overall design is OK. (Or maybe there already is consensus?)
> 
> > diff --git a/builtin/serve.c b/builtin/serve.c
> > new file mode 100644
> > index 0..bb726786a
> > --- /dev/null
> > +++ b/builtin/serve.c
> > @@ -0,0 +1,30 @@
> > +#include "cache.h"
> > +#include "builtin.h"
> > +#include "parse-options.h"
> > +#include "serve.h"
> > +
> > +static char const * const grep_usage[] = {
> 
> Should be serve_usage.
> 
> > diff --git a/serve.c b/serve.c
> > new file mode 100644
> > index 0..da8127775
> > --- /dev/null
> > +++ b/serve.c
> 
> [snip]
> 
> > +struct protocol_capability {
> > +   const char *name; /* capability name */
> 
> Maybe document as:
> 
>   The name of the capability. The server uses this name when advertising
>   this capability, and the client uses this name to invoke the command
>   corresponding to this capability.
> 
> > +   /*
> > +* Function queried to see if a capability should be advertised.
> > +* Optionally a value can be specified by adding it to 'value'.
> > +*/
> > +   int (*advertise)(struct repository *r, struct strbuf *value);
> 
> Document what happens when value is appended to. For example:
> 
>   ... If value is appended to, the server will advertise this capability
>   as = instead of .
> 

All good documentation changes.

> > +   /*
> > +* Function called when a client requests the capability as a command.
> > +* The command request will be provided to the function via 'keys', the
> > +* capabilities requested, and 'args', the command specific parameters.
> > +*
> > +* This field should be NULL for capabilities which are not commands.
> > +*/
> > +   int (*command)(struct repository *r,
> > +  struct argv_array *keys,
> > +  struct argv_array *args);
> 
> Looking at the code below, I see that the command is not executed unless
> advertise returns true - this means that a command cannot be both
> supported and unadvertised. Would this be too restrictive? For example,
> this would disallow a gradual across-multiple-servers rollout in which
> we allow but not advertise a capability, and then after some time,
> advertise the capability.

One way to change this would be to just add another function to the
struct which is called to check if the command is allowed, instead of
relying on the same function to do that for both advertise and
allow...though I don't see a big win for allowing a command but not
advertising it.

> 
> If we change this, then the value parameter of advertise can be
> mandatory instead of optional.

I don't see how this fixes the issue you bring up.

-- 
Brandon Williams

Re: [PATCH 04/20] cat-file: make valid_atoms as a function parameter

[Junio C Hamano]

Olga Telezhnaya  writes:

> Make valid_atoms as a function parameter,
> there could be another variable further.
> Need that for further reusing of formatting logic in cat-file.c.
>
> Signed-off-by: Olga Telezhnaia 
> Mentored-by: Christian Couder 
> Mentored by: Jeff King 
> ---
>  ref-filter.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)

Please be a bit more careful about your title.  This change does not
seem to have anything to do with "cat-file".

I was not sure what you meant by "make X as a function parameter"
after reading the proposed log message twice, but I am guessing that
you are allowing these functions to operate on not just the global
singleton but a different instance of array.  

I also suspect that this step may make the references to the
valid_atom[] array somewhat inconsistent in that the functions that
are touched by this patch would refer to the passed-in array while
the remainder of the existing code still works on the global
singleton.  For example, parse_ref_filter_atom() is called by
verify_ref_format(), but this patch does not make _it_ take the
array as a parameter, and instead uses the global singleton, so
anybody who wants to use verify_ref_format() with different
valid_atom[] are SOL.  I am not saying that this inconsistency will
be a problem, but that the patch (including the proposed log
message) does not explain why it is not---and it should.

Thanks.

> diff --git a/ref-filter.c b/ref-filter.c
> index 1426f0c28bce7..2d6e81fe1ab00 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -396,6 +396,7 @@ struct atom_value {
>   * Used to parse format string and sort specifiers
>   */
>  static int parse_ref_filter_atom(const struct ref_format *format,
> +  const struct valid_atom *valid_atoms, int 
> n_atoms,
>const char *atom, const char *ep)
>  {
>   const char *sp;
> @@ -425,13 +426,13 @@ static int parse_ref_filter_atom(const struct 
> ref_format *format,
>   atom_len = (arg ? arg : ep) - sp;
>  
>   /* Is the atom a valid one? */
> - for (i = 0; i < ARRAY_SIZE(valid_atoms); i++) {
> + for (i = 0; i < n_atoms; i++) {
>   int len = strlen(valid_atoms[i].name);
>   if (len == atom_len && !memcmp(valid_atoms[i].name, sp, len))
>   break;
>   }
>  
> - if (ARRAY_SIZE(valid_atoms) <= i)
> + if (n_atoms <= i)
>   die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
>  
>   /* Add it in, including the deref prefix */
> @@ -708,7 +709,8 @@ int verify_ref_format(struct ref_format *format)
>   if (!ep)
>   return error(_("malformed format string %s"), sp);
>   /* sp points at "%(" and ep points at the closing ")" */
> - at = parse_ref_filter_atom(format, sp + 2, ep);
> + at = parse_ref_filter_atom(format, valid_atoms,
> +ARRAY_SIZE(valid_atoms), sp + 2, ep);
>   cp = ep + 1;
>  
>   if (skip_prefix(used_atoms[at].name, "color:", ))
> @@ -2139,7 +2141,9 @@ void format_ref_array_item(struct ref_array_item *info,
>   if (cp < sp)
>   append_literal(cp, sp, );
>   get_ref_atom_value(info,
> -parse_ref_filter_atom(format, sp + 2, ep),
> +parse_ref_filter_atom(format, valid_atoms,
> +  
> ARRAY_SIZE(valid_atoms),
> +  sp + 2, ep),
>  );
>   atomv->handler(atomv, );
>   }
> @@ -2187,7 +2191,8 @@ static int parse_sorting_atom(const char *atom)
>*/
>   struct ref_format dummy = REF_FORMAT_INIT;
>   const char *end = atom + strlen(atom);
> - return parse_ref_filter_atom(, atom, end);
> + return parse_ref_filter_atom(, valid_atoms,
> +  ARRAY_SIZE(valid_atoms), atom, end);
>  }
>  
>  /*  If no sorting option is given, use refname to sort as default */
>
> --
> https://github.com/git/git/pull/450

Re: [PATCH 03/20] cat-file: rename variables in ref-filter

[Junio C Hamano]

Olga Telezhnaya  writes:

> Rename some variables for easier reading.
> They point not to values, but to arrays.

Once the code is written and people start to build on top, a change
like this is not worth the code churn, especially because there are
two equally valid schools of naming convention.

 - When you have an array, each of whose 20 slots holds a single
   dosh, I would prefer to call the array dosh[20], not doshes[20],
   so that I can refer to the seventh dosh as "dosh[7]".

 - If you more often refer to the array as a whole (than you refer
   to individual elements) and want to stress the fact that the
   array holds multiple elements in it, I can understand that you
   may be tempted to call the whole array "doshes[]".

So please drop this and other "rename variables" patches from the
series.

Re: [ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index f1678851de9..470107248eb 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -31,7 +31,22 @@
>  #
>  
>  use 5.008;
> -use lib (split(/:/, $ENV{GITPERLLIB}));
> +sub gitperllib {
> +...
> + if ($ENV{GITPERLLIB} =~ /;/) {
> + return split(/;/, $ENV{GITPERLLIB});
> + }
> + return split(/:/, $ENV{GITPERLLIB});
> +}

This cannot be the whole story for a few reasons.

 - In t/test-lib.sh we see this:

   
GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
   export GITPERLLIB

   If this part wants to split with ';', then the joining needs to
   be done with ';' to match, no?

 - In addition to t0021, there are similar split with colon in 0202,
   9000 and 9700, yet I am getting the feeling that you observed the
   issue only in0021, to which I do not think of a good explanation
   why.

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Junio C Hamano]

Stefan Beller  writes:

> Johannes wrote:
>> I think a better alternative would be to introduce a new abbreviation mode
>> that is *intended* to stop caring about unique abbreviations.
>>
>> In web interfaces, for example, it makes tons of sense to show, say, 8
>> digits in link texts and have the full name in the actual link URL.
>
> And that is what (b) would solve, as it is shorter than the full hash and
> yet exact.

I still do not get it, even though I fully agree that in Web UI what
Dscho envisions makes tons of sense.  Use some short handle that
does not need to be unique inside repository to display, but have a
full information that can be used by machines.  The shortened ones
need to be unique _within_ a given todo list, to be displayed as
text to be "clicked", where the A element's href attribute that
surrounds that "clickable" text has fully unambiguous information.

And that fully unambiguous information, because it is for machine
consumption, can be a full object name without any shortening.

I do not see a need for REBASE_HEAD~$n to make it less robust
(i.e. we now need to worry about making sure it is not lost or moved
while we need it and clean it up when we are done, etc.)

Re: WITH ALL DUE RESPECT TO YOUR PERSON

[Lincoln Bah Bah]

Dear Friend,

With due respect to your person and much sincerity of purpose. I have a 
business proposal which I will like to handle with you. $35 million USD is 
involves. But be rest assured that everything is legal and risk free as I have 
concluded all the arrangements and the legal papers that will back the 
transaction up. Kindly indicate your interest as to enable me tell you more 
detail of the proposal.

Waiting for your urgent response.
Yours Faithfully,
Dr.Lincoln Bah Bah

Re: [PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Junio C Hamano]

Prathamesh Chavan  writes:

> The same mechanism is used even for porting this submodule
> subcommand, as used in the ported subcommands till now.
> The function cmd_deinit in split up after porting into four
> functions: module_deinit(), for_each_listed_submodule(),
> deinit_submodule() and deinit_submodule_cb().
>
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 153 
> 
>  git-submodule.sh|  55 +---
>  2 files changed, 154 insertions(+), 54 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index dd7737acd..54b0e46fc 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -20,6 +20,7 @@
>  #define OPT_QUIET (1 << 0)
>  #define OPT_CACHED (1 << 1)
>  #define OPT_RECURSIVE (1 << 2)
> +#define OPT_FORCE (1 << 3)
>  
>  typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
> void *cb_data);
> @@ -908,6 +909,157 @@ static int module_sync(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> +struct deinit_cb {
> + const char *prefix;
> + unsigned int flags;
> +};
> +#define DEINIT_CB_INIT { NULL, 0 }
> +
> +static void deinit_submodule(const char *path, const char *prefix,
> +  unsigned int flags)
> +{
> + const struct submodule *sub;
> + char *displaypath = NULL;
> + struct child_process cp_config = CHILD_PROCESS_INIT;
> + struct strbuf sb_config = STRBUF_INIT;
> + char *sub_git_dir = xstrfmt("%s/.git", path);
> + mode_t mode = 0777;
> +
> + sub = submodule_from_path(_oid, path);
> +
> + if (!sub || !sub->name)
> + goto cleanup;
> +
> + displaypath = get_submodule_displaypath(path, prefix);
> +
> + /* remove the submodule work tree (unless the user already did it) */
> + if (is_directory(path)) {
> + struct stat st;
> + /*
> +  * protect submodules containing a .git directory
> +  * NEEDSWORK: automatically call absorbgitdirs before
> +  * warning/die.
> +  */

I guess that you mean "instead of dying, automatically call absorb
and (possibly) warn"?  That sounds like a sensible improvement.

> + if (is_directory(sub_git_dir))
> + die(_("Submodule work tree '%s' contains a .git "
> +   "directory use 'rm -rf' if you really want "
> +   "to remove it including all of its history"),

This changes the message text by removing () around "use ... history",
which I do not think you intended to do.

> +   displaypath);
> +
> + if (!(flags & OPT_FORCE)) {
> + struct child_process cp_rm = CHILD_PROCESS_INIT;
> + cp_rm.git_cmd = 1;
> + argv_array_pushl(_rm.args, "rm", "-qn",
> +  path, NULL);
> +
> + if (run_command(_rm))
> + die(_("Submodule work tree '%s' contains local "
> +   "modifications; use '-f' to discard 
> them"),
> +   displaypath);
> + }
> +
> + if (!lstat(path, )) {

What is this if statement doing here?  It does not make sense,
especially without an 'else' clause on the other side, at least to
me.

At this point in the flow, the code has already determined that path
is a directory above before starting to check if it has ".git/"
immediately below it, or trying to run "git rm" in the dry run mode
to see if it yields an error, so at this point lstat() should
succeed (and would say it is a directory).  I would sort-of
understand it if this "if()" has an "else" clause to act on an
error, but that is not something the original does not do, so I am
not sure if it belongs to a "rewrite to C" patch.

> + struct strbuf sb_rm = STRBUF_INIT;
> + const char *format;
> +
> + strbuf_addstr(_rm, path);
> +
> + if (!remove_dir_recursively(_rm, 0))
> + format = _("Cleared directory '%s'\n");
> + else
> + format = _("Could not remove submodule work 
> tree '%s'\n");
> +
> + if (!(flags & OPT_QUIET))
> + printf(format, displaypath);
> +
> + mode = st.st_mode;
> +
> + strbuf_release(_rm);
> + }
> + }

If the reason is "avoid losing the original directory mode by
removing and recreating", then you should be able to do much better
by using REMOVE_DIR_KEEP_TOPLEVEL in the above "do we still 

Re: [PATCH v1 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Junio C Hamano]

Prathamesh Chavan  writes:

> +static int print_default_remote(int argc, const char **argv, const char 
> *prefix)
> +{
> + const char *remote;
> +
> + if (argc != 1)
> + die(_("submodule--helper print-default-remote takes no 
> arguments"));
> +
> + remote = get_default_remote();
> + if (remote)
> + printf("%s\n", remote);
> +
> + return 0;
> +}

This is called directly from main and return immediately after
printing, so a small leak of remote does not matter, I guess.

> +static void sync_submodule(const char *path, const char *prefix,
> +unsigned int flags)
> +{
> + const struct submodule *sub;
> + char *remote_key = NULL;
> + char *sub_origin_url, *super_config_url, *displaypath;
> + struct strbuf sb = STRBUF_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + char *sub_config_path = NULL;
> +
> + if (!is_submodule_active(the_repository, path))
> + return;
> +
> + sub = submodule_from_path(_oid, path);
> +
> + if (sub && sub->url) {
> + if (starts_with_dot_dot_slash(sub->url) || 
> starts_with_dot_slash(sub->url)) {

Not a big deal, but other codepaths seem to fold this pattern into
two lines, i.e.

if (starts_with_dot_dot_slash(sub->url) ||
starts_with_dot_slash(sub->url)) {

> + sub_origin_url = relative_url(remote_url, sub->url, 
> up_path);
> + super_config_url = relative_url(remote_url, sub->url, 
> NULL);

On this side, these two are allocated memory that need to be freed.

> + } else {
> + sub_origin_url = xstrdup(sub->url);
> + super_config_url = xstrdup(sub->url);

This side as well.

> + }
> + } else {
> + sub_origin_url = "";
> + super_config_url = "";

But not these.  You have free() of these two at the end of this
function, which will break things.

Re: [PATCH 12/26] ls-refs: introduce ls-refs server command

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:14 -0800
Brandon Williams  wrote:

> +  symrefs: In addition to the object pointed by it, show the underlying
> +ref pointed by it when showing a symbolic ref.
> +  peel: Show peeled tags.
> +  ref-pattern : When specified, only references matching the
> +  given patterns are displayed.

I notice "symrefs" being tested in patch 13 and "ref-pattern" being
tested in patch 16. Is it possible to make a test for "peel" as well?
(Or is it being tested somewhere I didn't notice?)

Me

[Leilani Buxton]

Sent from my iPhone

Re: upstreaming https://github.com/cgwalters/git-evtag ?

[Jonathan Nieder]

Hi,

Santiago Torres wrote:

>> In contrast, working on hash-function-transition.txt?  That
>> seems like it'd easily consume many person-months of work.
>> And that plan only exists post-shatter.io, whereas git-evtag
>> long predates both.
>
> I think this is partly true. A hash transition has been brought up
> multiple times pre-shattered. In my opinion shattered was a much-needed
> PR push for SHA1 deprecation. In practice, things changed very little.

Sure, the main relevant things that changed are:

 1. The sha1collisiondetection library became well known, which if
anything makes moving off of SHA-1 *less* urgent than before (but
still urgent).

and

 2. We came up with and agreed on a design for a transition off of
SHA-1 that we are (slowly but surely) executing on.  This means
it's a good time to help get it done.

>>> Personally I'd dislike to include ev-tags as it might send a signal
>>> of "papering over sha1 issues instead of fixing it".
>>
>> I don't agree.  I think it's pretty clear that a hash function transition
>> would be a huge amount of work - not least because of course
>> there are now at least two widely used implementations of git in C,
>> plus https://www.eclipse.org/jgit/ plus...
>
> I agree with Stefan here. I think it's better in the long-term to
> push for hash-agnosticity. I don't know if git-evtag is hash agnostic,
> but if it is not, then we have two transition plans to think about.

I don't think there's even a question here: Git has to transition off
of SHA-1.

In that context, Stefan's comment is a welcome one: once we've
transitioned off of SHA-1, having a separate evtag feature would make
git more complicated without any benefit to match.  To put it another
way, the gpgsig-sha256 field described in
Documentation/technical/hash-function-transition.txt provides
essentially the same functionality as an evtag.  What's missing is an
implementation of it.

I'm happy to help in any way I can (reviews, advice, etc).

[...]
> Full disclosure, I published a "competing" solution a couple of years
> ago[1] but, in my personal opinion, I think push certificates can
> achieve the same security guarantees as my system with very little
> changes.

Work to improve the usability of push certs would also be very very
welcome.

Thanks and hope that helps,
Jonathan

> [1] 
> https://www.usenix.org/conference/usenixsecurity16/technical-sessions/presentation/torres-arias

Re: [PATCH 11/26] serve: introduce git-serve

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:13 -0800
Brandon Williams  wrote:

> diff --git a/Documentation/technical/protocol-v2.txt 
> b/Documentation/technical/protocol-v2.txt
> new file mode 100644
> index 0..b87ba3816
> --- /dev/null
> +++ b/Documentation/technical/protocol-v2.txt

I'll review the documentation later, once there is some consensus that
the overall design is OK. (Or maybe there already is consensus?)

> diff --git a/builtin/serve.c b/builtin/serve.c
> new file mode 100644
> index 0..bb726786a
> --- /dev/null
> +++ b/builtin/serve.c
> @@ -0,0 +1,30 @@
> +#include "cache.h"
> +#include "builtin.h"
> +#include "parse-options.h"
> +#include "serve.h"
> +
> +static char const * const grep_usage[] = {

Should be serve_usage.

> diff --git a/serve.c b/serve.c
> new file mode 100644
> index 0..da8127775
> --- /dev/null
> +++ b/serve.c

[snip]

> +struct protocol_capability {
> + const char *name; /* capability name */

Maybe document as:

  The name of the capability. The server uses this name when advertising
  this capability, and the client uses this name to invoke the command
  corresponding to this capability.

> + /*
> +  * Function queried to see if a capability should be advertised.
> +  * Optionally a value can be specified by adding it to 'value'.
> +  */
> + int (*advertise)(struct repository *r, struct strbuf *value);

Document what happens when value is appended to. For example:

  ... If value is appended to, the server will advertise this capability
  as = instead of .

> + /*
> +  * Function called when a client requests the capability as a command.
> +  * The command request will be provided to the function via 'keys', the
> +  * capabilities requested, and 'args', the command specific parameters.
> +  *
> +  * This field should be NULL for capabilities which are not commands.
> +  */
> + int (*command)(struct repository *r,
> +struct argv_array *keys,
> +struct argv_array *args);

Looking at the code below, I see that the command is not executed unless
advertise returns true - this means that a command cannot be both
supported and unadvertised. Would this be too restrictive? For example,
this would disallow a gradual across-multiple-servers rollout in which
we allow but not advertise a capability, and then after some time,
advertise the capability.

If we change this, then the value parameter of advertise can be
mandatory instead of optional.

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Stefan Beller]

On Tue, Jan 9, 2018 at 12:12 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> And in that light, I'd like to propose a new naming scheme:
>>
>> (a) assume that we "tag" HEAD at the start of the rebase
>> (b) any abbreviation must be given as committish anchored to said ref:
>>
>> pick REBASE_HEAD~1 commit subject
>> pick REBASE_HEAD~2 distak the gostim
>> pick REBASE_HEAD~3 Document foo
>> pick REBASE_HEAD~4 Testing the star-gazer
>>
>> And as we have the full descriptiveness of the committishs there,
>> each commit can be described in a unique way using the graph relationship.
>> I am just throwing the name REBASE_HEAD out there to trigger some 
>> associations
>> ("similar to FETCH_HEAD"), but I dislike the name.
>>
>> (c) this would not solve the problem of mergy history, yet. For that we'd 
>> need
>> to introduce more keywords, that allow us to move around in the DAG,
>> such that we can reset to a specific revision or name revisions on the 
>> fly.
>> So maybe all we need is "reset", "name" (== short lived tag),
>> "merge" (rewrite parents of HEAD) to be expressive enough, but still keep
>> the line oriented interface?
>
> It is correct to say that (c) is an issue that is not solved by (b),
> but with the current scheme, the commits are internally referenced
> by full object names, and just before it is presented to the end
> users, these names are abbreviated down to unique prefix.  The
> machinery expands these abbreviated names back to the full names
> before going forward, so it is not an issue that we are creating new
> commits during the rebase.
>
> Does it make it easier to read if we used ~$n notation based on a
> fixed point, instead of shortened unique object names?  What
> improvement is (b) trying to achieve?
>

Johannes wrote:
> I think a better alternative would be to introduce a new abbreviation mode
> that is *intended* to stop caring about unique abbreviations.
>
> In web interfaces, for example, it makes tons of sense to show, say, 8
> digits in link texts and have the full name in the actual link URL.

And that is what (b) would solve, as it is shorter than the full hash and
yet exact.

(c) was mostly speculation on top of (b) if we can take it any further.

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Junio C Hamano]

Stefan Beller  writes:

> And in that light, I'd like to propose a new naming scheme:
>
> (a) assume that we "tag" HEAD at the start of the rebase
> (b) any abbreviation must be given as committish anchored to said ref:
>
> pick REBASE_HEAD~1 commit subject
> pick REBASE_HEAD~2 distak the gostim
> pick REBASE_HEAD~3 Document foo
> pick REBASE_HEAD~4 Testing the star-gazer
>
> And as we have the full descriptiveness of the committishs there,
> each commit can be described in a unique way using the graph relationship.
> I am just throwing the name REBASE_HEAD out there to trigger some associations
> ("similar to FETCH_HEAD"), but I dislike the name.
>
> (c) this would not solve the problem of mergy history, yet. For that we'd need
> to introduce more keywords, that allow us to move around in the DAG,
> such that we can reset to a specific revision or name revisions on the 
> fly.
> So maybe all we need is "reset", "name" (== short lived tag),
> "merge" (rewrite parents of HEAD) to be expressive enough, but still keep
> the line oriented interface?

It is correct to say that (c) is an issue that is not solved by (b),
but with the current scheme, the commits are internally referenced
by full object names, and just before it is presented to the end
users, these names are abbreviated down to unique prefix.  The
machinery expands these abbreviated names back to the full names
before going forward, so it is not an issue that we are creating new
commits during the rebase.

Does it make it easier to read if we used ~$n notation based on a
fixed point, instead of shortened unique object names?  What
improvement is (b) trying to achieve?

Re: [PATCH v1 0/2] Incremental rewrite of git-submodules

[Brandon Williams]

On 01/09, Prathamesh Chavan wrote:
> The patches [1] and [2] concerning the porting of submodule
> subcommands: sync and deinit were updated in accoudance with
> the changes made in one of such similar portings made earlier
> for submodule subcommand status[3]. Following are the changes
> made:

The two patches look good to me.  Thanks for continuing this work!

> 
> * It was observed that the number of params increased a lot due to flags
>   like quiet, recursive, cached, etc, and keeping in mind the future
>   subcommand's ported functions as well, a single unsigned int called
>   flags was introduced to store all of these flags, instead of having
>   parameter for each one.

This is unfortunate.  The use of a flag word or using bit-fields are
essentially equivalent so its unfortunate that the conversion to using
one or the other caused review churn.  My own preference would be to use
bit-fields ;)  I also noticed that the flags you are using start with
OPT_* which conflict with the parse-options namespace, sorry for not
catching this when a few of your older patches made it into master.
This isn't a big deal since no symbols look to collide so I am not
suggesting you change this since I would prefer to eliminate more
unnecessary review churn on this series.

> 
> * To accomodate the possiblity of a direct call to the functions
>   deinit_submodule() and sync_submodule(), callback functions were
>   introduced.
> 
> As before you can find this series at: 
> https://github.com/pratham-pc/git/commits/patch-series-2
> 
> And its build report is available at: 
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: patch-series-2
> Build #195
> 
> [1]: https://public-inbox.org/git/20170807211900.15001-6-pc44...@gmail.com/
> [2]: https://public-inbox.org/git/20170807211900.15001-7-pc44...@gmail.com/
> [3]: https://public-inbox.org/git/20171006132415.2876-4-pc44...@gmail.com/
> 
> Prathamesh Chavan (2):
>   submodule: port submodule subcommand 'sync' from shell to C
>   submodule: port submodule subcommand 'deinit' from shell to C
> 
>  builtin/submodule--helper.c | 345 
> 
>  git-submodule.sh| 112 +-
>  2 files changed, 347 insertions(+), 110 deletions(-)
> 
> -- 
> 2.14.2
> 

-- 
Brandon Williams

Re: [PATCH 6/8] Doc/gitsubmodules: improve readability of certain lines

[Kaartic Sivaraam]

On Wednesday 10 January 2018 01:01 AM, Stefan Beller wrote:
  The submodule's `$GIT_DIR/config` file would come into play when running
  `git push --recurse-submodules=check` in the superproject, as this would
 @@ -107,13 +108,13 @@ If the submodule is not yet initialized, then the 
 configuration
  inside the submodule does not exist yet, so configuration where to
  obtain the submodule from is configured here for example.

>>
>> I caught this in the context while replying. "so configuration where to
>> obtain the submodule from is configured here for example." doesn't seem
>> to read well. Maybe removing configuration from the sentence will make
>> it sound better?
>>

I'm going to make this change.


>>
 - * the `.gitmodules` file inside the superproject. Additionally to the
 -   required mapping between submodule's name and path, a project usually
 + * The `.gitmodules` file inside the superproject. Additionally, if 
 mapping
 +   is required between a submodule's name and its path, a project usually
>>>
>>> This changes meaning, originally it tries to say:
>>>
>>> * it requires mapping path <-> names.
>>
>> I get this ...
>>
>>> * but there can be more.
>>
>> ... but not this. Did the previous version really try to say this?
>> Anyways how does this sound?
> 
> Well that was me being very sloppy trying to say that there might be
> submodule..{url, ignored, shallow} settings which just happen to
> be there.
> 
>>   * The `.gitmodules` file inside the superproject. A project usually
>> uses this file to suggest defaults for the upstream collection
>> of repositories for the mapping that is required between a
>> submodule's name and its path.
>>
>> I think it conveys the "it requires mapping path <-> names." correctly
>> but doesn't convey the "but there can be more." part. I'm not sure how
>> to get that into the sentence, correctly.
> 
> I did not consider that part the important part, hence my sloppiness.
> Sorry for the confusion.
> 
> My main point was to say that the mapping is the important part and
> must be found in the .gitmodules file, otherwise we do not consider
> it a submodule (for whatever "it" is, maybe a gitlink at a path=name).
> 

So, I'm going to use the version that I specified above as I think it
seems to convey that clearly (at least to me),

The `.gitmodules` file inside the superproject. A project usually
uses this file to suggest defaults for the upstream collection
of repositories for the mapping that is required between a
submodule's name and its path.

Let me know, if there are issues.

Thanks,
Kaartic



signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH 00/18] Multi-pack index (MIDX)

[Stefan Beller]

On Tue, Jan 9, 2018 at 5:05 AM, Johannes Schindelin
 wrote:
> Hi Peff,
>
> On Tue, 9 Jan 2018, Jeff King wrote:
>
>> On Mon, Jan 08, 2018 at 02:43:00PM +0100, Johannes Schindelin wrote:
>>
>> > Take the interactive rebase for example. It generates todo lists with
>> > abbreviated commit names, for readability (and it is *really* important to
>> > keep this readable). As we expect new objects to be introduced by the
>> > interactive rebase, we convert that todo list to unabbreviated commit
>> > names before executing the interactive rebase.
>> >
>> > Your idea (to not care about unambiguous abbreviations) would break that.
>>
>> I think that could be easily worked around for rebase by asking git to
>> check ambiguity during the conversion.
>
> Sure.
>
> It also points to a flaw in your reasoning, and you should take my example
> further: previously, we guaranteed unique abbreviations, and who is to say
> that there is no script out there relying on that guarantee? You?
>
>> But I agree it's a potential problem for other scripts that we might not
>> have control over. I hadn't really intended this to be the default
>> behavior (my patch was just trying to show the direction). But it does
>> make for a pretty awful interface if callers have to opt into it
>> manually ("git log --oneline --no-really-go-fast").
>
> Exactly.
>
>> I am a bit curious if there's a bounded probability that people would
>> find acceptable for Git to give an ambiguous abbreviation. We already
>> accept 1 in 2^160, of course. But would, e.g., one in a million be OK?
>
> What is that going to solve?
>
> I think a better alternative would be to introduce a new abbreviation mode
> that is *intended* to stop caring about unique abbreviations.
>
> In web interfaces, for example, it makes tons of sense to show, say, 8
> digits in link texts and have the full name in the actual link URL.
>
> Currently, we have no way to tell Git: look, I want to have a short label
> for this, but I do not really care that this is unambiguous, I just want
> something to talk about.
>
> (And you are correct, of course, that the uniqueness of abbreviations will
> no longer be guaranteed for partial clones, and it is already not
> guaranteed for shallow clones, and it will only be possible to guarantee
> this, ever, for one particular repository at a particular time.)
>
> I am just hesitant to change things that would break existing setups.
>
> Just to throw this out there: --abbrev=8! would be one possible convention
> to state "I want exactly 8 hex digits, don't bother checking for
> uniqueness".
>
> Not sure how urgent such a feature is.

You seem to have spent some time on rebase, including lamenting on the
in-expressiveness of the instruction sheet (c.f. "rebase a mergy history sucks")

And in that light, I'd like to propose a new naming scheme:

(a) assume that we "tag" HEAD at the start of the rebase
(b) any abbreviation must be given as committish anchored to said ref:

pick REBASE_HEAD~1 commit subject
pick REBASE_HEAD~2 distak the gostim
pick REBASE_HEAD~3 Document foo
pick REBASE_HEAD~4 Testing the star-gazer

And as we have the full descriptiveness of the committishs there,
each commit can be described in a unique way using the graph relationship.
I am just throwing the name REBASE_HEAD out there to trigger some associations
("similar to FETCH_HEAD"), but I dislike the name.

(c) this would not solve the problem of mergy history, yet. For that we'd need
to introduce more keywords, that allow us to move around in the DAG,
such that we can reset to a specific revision or name revisions on the fly.
So maybe all we need is "reset", "name" (== short lived tag),
"merge" (rewrite parents of HEAD) to be expressive enough, but still keep
the line oriented interface?

Stefan

Re: [PATCH] doc/read-tree: remove obsolete remark

[Junio C Hamano]

"Andreas G. Schacker"  writes:

> Earlier versions of `git read-tree` required the `--prefix` option value
> to end with a slash. This restriction was eventually lifted without a
> corresponding amendment to the documentation.
>
> Signed-off-by: Andreas G. Schacker 
> ---

Thanks.  "read-tree --prefix=foo- $tree" is allowed these days,
indeed.  Will queue.

What is curious is that 34110cd4 ("Make 'unpack_trees()' have a
separate source and destination index", 2008-03-06) seems to be the
one that removed the check, and the removed contents do not seem to
have much in common with the stated goal of the commit.



>  Documentation/git-read-tree.txt | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
> index 72bd809fb..f2a07d54d 100644
> --- a/Documentation/git-read-tree.txt
> +++ b/Documentation/git-read-tree.txt
> @@ -81,12 +81,11 @@ OPTIONS
>  * when both sides add a path identically.  The resolution
>is to add that path.
>  
> ---prefix=/::
> +--prefix=::
>   Keep the current index contents, and read the contents
>   of the named tree-ish under the directory at ``.
>   The command will refuse to overwrite entries that already
> - existed in the original index file. Note that the `/`
> - value must end with a slash.
> + existed in the original index file.
>  
>  --exclude-per-directory=::
>   When running the command with `-u` and `-m` options, the

Re: [PATCH 4/8] Doc/gitsubmodules: avoid abbreviations

[Kaartic Sivaraam]

On Wednesday 10 January 2018 12:56 AM, Stefan Beller wrote:
> On Tue, Jan 9, 2018 at 8:06 AM, Kaartic Sivaraam
>  wrote:
>> On Tuesday 09 January 2018 12:15 AM, Stefan Beller wrote:

 - * The command line for those commands that support taking submodule 
 specs.
>>>
>>> ++ The command line for those commands that support taking submodules
>>> as part of their pathspecs[1].
>>> ++
>>> ++[1] pathspec is an official term according to `man gitglossary`.
>>>
>>> Maybe?
>>>
>>
>> Yeah, I actually did think 'specification' wasn't a the best fit for
>> this (should have mentioned that somewhere) Now, the real term came out :)
>>
>> Just to be sure, that "[1] pathspec ..." part goes to the end of the
>> document doesn't it?
> 
> The [1] part was just to highlight it for the sake of this discussion;
> I would not include it into the man page.
> 

That '++' before it made me think otherwise :)



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 6/8] Doc/gitsubmodules: improve readability of certain lines

[Stefan Beller]

>>>  The submodule's `$GIT_DIR/config` file would come into play when running
>>>  `git push --recurse-submodules=check` in the superproject, as this would
>>> @@ -107,13 +108,13 @@ If the submodule is not yet initialized, then the 
>>> configuration
>>>  inside the submodule does not exist yet, so configuration where to
>>>  obtain the submodule from is configured here for example.
>>>
>
> I caught this in the context while replying. "so configuration where to
> obtain the submodule from is configured here for example." doesn't seem
> to read well. Maybe removing configuration from the sentence will make
> it sound better?
>
>
>>> - * the `.gitmodules` file inside the superproject. Additionally to the
>>> -   required mapping between submodule's name and path, a project usually
>>> + * The `.gitmodules` file inside the superproject. Additionally, if mapping
>>> +   is required between a submodule's name and its path, a project usually
>>
>> This changes meaning, originally it tries to say:
>>
>> * it requires mapping path <-> names.
>
> I get this ...
>
>> * but there can be more.
>
> ... but not this. Did the previous version really try to say this?
> Anyways how does this sound?

Well that was me being very sloppy trying to say that there might be
submodule..{url, ignored, shallow} settings which just happen to
be there.

>   * The `.gitmodules` file inside the superproject. A project usually
> uses this file to suggest defaults for the upstream collection
> of repositories for the mapping that is required between a
> submodule's name and its path.
>
> I think it conveys the "it requires mapping path <-> names." correctly
> but doesn't convey the "but there can be more." part. I'm not sure how
> to get that into the sentence, correctly.

I did not consider that part the important part, hence my sloppiness.
Sorry for the confusion.

My main point was to say that the mapping is the important part and
must be found in the .gitmodules file, otherwise we do not consider
it a submodule (for whatever "it" is, maybe a gitlink at a path=name).

Stefan

Re: merge-base --is-ancestor A B is unreasonably slow with unrelated history B

[Derrick Stolee]

On 1/9/2018 10:17 AM, Ævar Arnfjörð Bjarmason wrote:

This is a pathological case I don't have time to dig into right now:

 git branch -D orphan;
 git checkout --orphan orphan &&
 git reset --hard &&
 touch foo &&
 git add foo &&
 git commit -m"foo" &&
 time git merge-base --is-ancestor master orphan

This takes around 5 seconds on linux.git to return 1. Which is around
the same time it takes to run current master against the first commit in
linux.git:

 git merge-base --is-ancestor 1da177e4c3f4 master

This is obviously a pathological case, but maybe we should work slightly
harder on the RHS of and discover that it itself is an orphan commit.

I ran into this while writing a hook where we'd like to do:

 git diff $master...topic

Or not, depending on if the topic is an orphan or just something
recently branched off, figured I could use --is-ancestor as on
optimization, and then discovered it's not much of an optimization.


Ævar,

This is the same performance problem that we are trying to work around 
with Jeff's "Add --no-ahead-behind to status" patch [1]. For commits 
that are far apart, many commits need to be parsed. I think the right 
solution is to create a serialized commit graph that stores the 
adjacency information of the commits and can create commit structs 
quickly. This requires storing the commit id, commit date, parents, and 
root tree id to satisfy the needs of parse_commit_gently(). Once the 
framework for this data is constructed, it is simple to add generation 
numbers to that data and start consuming them in other algorithms (by 
adding the field to 'struct commit').


I'm working on such a patch right now, but it will be a few weeks before 
I'm ready.


Thanks,
-Stolee

[1] v5 of --no-ahead-behind 
https://public-inbox.org/git/20180109185018.69164-1-...@jeffhostetler.com/T/#t


[2] v4 of --no-ahead-behind 
https://public-inbox.org/git/nycvar.qro.7.76.6.1801091744540...@zvavag-6oxh6da.rhebcr.pbec.zvpebfbsg.pbz/T/#t

Re: [PATCH 3/8] Doc/gitsubmodules: specify how submodules help in reduced size

[Kaartic Sivaraam]

On Wednesday 10 January 2018 12:31 AM, Stefan Beller wrote:
> On Tue, Jan 9, 2018 at 8:01 AM, Kaartic Sivaraam
>  wrote:
>> On Tuesday 09 January 2018 12:08 AM, Stefan Beller wrote:
 diff --git a/Documentation/gitsubmodules.txt 
 b/Documentation/gitsubmodules.txt
 index cb795c6b6..3f73983d5 100644
 --- a/Documentation/gitsubmodules.txt
 +++ b/Documentation/gitsubmodules.txt
 @@ -63,6 +63,9 @@ Submodules can be used for at least two different use 
 cases:
  * Transfer size:
In its current form Git requires the whole working tree present. It
does not allow partial trees to be transferred in fetch or clone.
 +  If you have your project as multiple repositories tied together as
 +  submodules in a superproject, you can avoid fetching the working
 +  trees of the repositories you are not interested in.
>>>
>>> You do not fetch a working tree, but a whole repository?
>>>
>>
>> Maybe I misunderstood submodules when I wrote that example. Could you
>> help out with a better and precise replacement?
> 
> If your project consists of multiple repositories tied together, some 
> submodules
> may not be of interest for all users, who do not need to fetch such submodule
> repositories.
> 

OK, now I get why I couldn't get your point. I actually was thinking of
the version of the message I had tweaked for v2 when reading your
message. It doesn't have the confusing meaning. It actually reads,

   ...
   If the project you work on consists of multiple repositories tied
   together as submodules in a superproject, you can avoid fetching the
   working trees of the repositories you are not interested in.

So, my version takes the perspective of the person who gains the
advantage of having to clone unnecessary repos. And yours, the
perspective of the person who gives the consumer of the repo that
advantage. Both sound nice to me. But if mine doesn't sound nice to you,
let me know so that I could replace it with your version.


>> Just putting in some context as to why I did this change, I thought this
>> was the only thing that lacked an example and wanted to make it consistent.
> 
> Oh, sure I like the example; I was just worried about the wording, as a 
> worktree
> is part of a repository, and the repository is the whole thing. In the
> current situation
> you can only fetch all-or-nothing, specifically you cannot fetch "just
> the worktree"
> (a shallow clone/fetch is the closest to that, but that still has the
> same amount of
> information the .git dir, than in the working tree)
> 

I get that!



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 01/26] pkt-line: introduce packet_read_with_status

[Brandon Williams]

On 01/09, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:03 -0800
> Brandon Williams  wrote:
> 
> > -int packet_read(int fd, char **src_buf, size_t *src_len,
> > -   char *buffer, unsigned size, int options)
> > +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
> > size_t *src_len,
> > +   char *buffer, unsigned size, 
> > int *pktlen,
> > +   int options)
> >  {
> > -   int len, ret;
> > +   int len;
> > char linelen[4];
> >  
> > -   ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> > -   if (ret < 0)
> > -   return ret;
> > +   if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
> > +   return PACKET_READ_EOF;
> > +
> > len = packet_length(linelen);
> > if (len < 0)
> > die("protocol error: bad line length character: %.4s", linelen);
> > -   if (!len) {
> > +
> > +   if (len == 0) {
> 
> This change (replacing "!len" with "len == 0") is unnecessary, I think.
> 
> > packet_trace("", 4, 0);
> > -   return 0;
> > +   return PACKET_READ_FLUSH;
> > +   } else if (len >= 1 && len <= 3) {
> > +   die("protocol error: bad line length character: %.4s", linelen);
> > }
> 
> This seems to be more of a "bad line length" than a "bad line length
> character".

I'll make these changes, though I do think this needs to stay as a "bad
line length character" as the len could be neg which is an indication of
parsing the linelen character failed.

> 
> Also, some of the checks are redundant. Above, it is probably better to
> delete "len >= 1", and optionally write "len < 4" instead of "len <= 3"
> (to emphasize that the subtraction of 4 below does not result in a
> negative value).
> 
> > +
> > len -= 4;
> > -   if (len >= size)
> > +   if ((len < 0) || ((unsigned)len >= size))
> > die("protocol error: bad line length %d", len);
> 
> The "len < 0" check is redundant.
> 
> > -   ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
> > -   if (ret < 0)
> > -   return ret;
> > +
> > +   if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
> > +   return PACKET_READ_EOF;
> >  
> > if ((options & PACKET_READ_CHOMP_NEWLINE) &&
> > len && buffer[len-1] == '\n')

-- 
Brandon Williams

Re: [PATCH 4/8] Doc/gitsubmodules: avoid abbreviations

[Stefan Beller]

On Tue, Jan 9, 2018 at 8:06 AM, Kaartic Sivaraam
 wrote:
> On Tuesday 09 January 2018 12:15 AM, Stefan Beller wrote:
>>>
>>> - * The command line for those commands that support taking submodule specs.
>>
>> ++ The command line for those commands that support taking submodules
>> as part of their pathspecs[1].
>> ++
>> ++[1] pathspec is an official term according to `man gitglossary`.
>>
>> Maybe?
>>
>
> Yeah, I actually did think 'specification' wasn't a the best fit for
> this (should have mentioned that somewhere) Now, the real term came out :)
>
> Just to be sure, that "[1] pathspec ..." part goes to the end of the
> document doesn't it?

The [1] part was just to highlight it for the sake of this discussion;
I would not include it into the man page.

Stefan

Re: [PATCH v1 0/2] Incremental rewrite of git-submodules

[Stefan Beller]

On Tue, Jan 9, 2018 at 9:57 AM, Prathamesh Chavan  wrote:
> The patches [1] and [2] concerning the porting of submodule
> subcommands: sync and deinit were updated in accoudance with
> the changes made in one of such similar portings made earlier
> for submodule subcommand status[3]. Following are the changes
> made:
>
> * It was observed that the number of params increased a lot due to flags
>   like quiet, recursive, cached, etc, and keeping in mind the future
>   subcommand's ported functions as well, a single unsigned int called
>   flags was introduced to store all of these flags, instead of having
>   parameter for each one.
>
> * To accomodate the possiblity of a direct call to the functions
>   deinit_submodule() and sync_submodule(), callback functions were
>   introduced.
>
> As before you can find this series at:
> https://github.com/pratham-pc/git/commits/patch-series-2
>
> And its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: patch-series-2
> Build #195

Cool!

I have reviewed both patches and found them good to apply;

Thanks,
Stefan

Re: [PATCH 02/26] pkt-line: introduce struct packet_reader

[Brandon Williams]

On 01/09, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:04 -0800
> Brandon Williams  wrote:
> 
> > diff --git a/pkt-line.h b/pkt-line.h
> > index 06c468927..c446e886a 100644
> > --- a/pkt-line.h
> > +++ b/pkt-line.h
> > @@ -111,6 +111,63 @@ char *packet_read_line_buf(char **src_buf, size_t 
> > *src_len, int *size);
> >   */
> >  ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
> >  
> > +struct packet_reader {
> > +   /* source file descriptor */
> > +   int fd;
> > +
> > +   /* source buffer and its size */
> > +   char *src_buffer;
> > +   size_t src_len;
> > +
> > +   /* buffer that pkt-lines are read into and its size */
> > +   char *buffer;
> > +   unsigned buffer_size;
> 
> Is the intention to support different buffers in the future?

Potentially at some point.

> 
> [snip]
> 
> > +/*
> > + * Peek the next packet line without consuming it and return the status.
> > + * The next call to 'packet_reader_read()' will perform a read of the same 
> > line
> > + * that was peeked, consuming the line.
> > + *
> > + * Only a single line can be peeked at a time.
> 
> It is logical to me that if you peeked at a line, and then peeked at it
> again, you will get the same line - I would phrase this not as a
> restriction ("only a single line") but just as a statement of fact (e.g.
> "Peeking at the same line multiple times without an intervening
> packet_reader_read will return the same result").

Fair enough, i'll change the wording.

> 
> > + */
> > +extern enum packet_read_status packet_reader_peek(struct packet_reader 
> > *reader);
> > +
> >  #define DEFAULT_PACKET_MAX 1000
> >  #define LARGE_PACKET_MAX 65520
> >  #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)

-- 
Brandon Williams

Re: [PATCH 09/26] transport: store protocol version

[Brandon Williams]

On 01/09, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:11 -0800
> Brandon Williams  wrote:
> 
> > diff --git a/transport.c b/transport.c
> > index 63c3dbab9..2378dcb38 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -118,6 +118,7 @@ struct git_transport_data {
> > struct child_process *conn;
> > int fd[2];
> > unsigned got_remote_heads : 1;
> > +   enum protocol_version version;
> 
> Should this be initialized to protocol_unknown_version? Right now, as
> far as I can tell, it is zero-initialized, which means protocol_v0.

I don't think it matters as the value isn't used until after the
version has already been discovered.

-- 
Brandon Williams

Re: merge-base --is-ancestor A B is unreasonably slow with unrelated history B

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> This is obviously a pathological case, but maybe we should work slightly
> harder on the RHS of and discover that it itself is an orphan commit.

In order to discover a commit is an orphan, you'd need to prove not
just that it does not reach the main part of the history (which is
cheap---its parenthood network would be quite limited and traversing
all of it is not that expensive) but the other way around, i.e. the
main part of the history would not reach it.

Do you have a cheaper way to do the latter than a full traveral of
the main history?  If not, then the cost similar to "git log master"
is expected.

Re: [PATCH 0/4] "Fast-"track Git GUI changes

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>> As it seems to be impossible to get the attention of the Git GUI
>> maintainer these "days" (I have opened Pull Requests on October 16th
>> 2016 that have not even been looked at), let's just side-step that
>> contribution path which seems to be dormant.
>
> Good to see that finally somebody else steps up after I did the same
> for a few times recently.
>
>> These fixes have been in Git for Windows for various amounts of time.
>>
>> Note: there are more Git GUI changes in Git for Windows, I only
>> accumulated the ones I deem wort considering for inclusion into v2.16.0,
>> still.
>
> Thanks.  I am not sure if it is too late for 2.16, as these are not
> fixes for regression during this cycle, though.

Heh, I changed my mind.  

Just like I pretended to be interim maintainer of Git-GUI for the
past two times, you are doing the same this time and I even agreed
that it was a good thing that you volunteered to pretend as one.

So let's follow through the pretence to its conclusion and merge
these directly to 'master'.

Thanks.

Re: [PATCH 07/26] connect: convert get_remote_heads to use struct packet_reader

[Brandon Williams]

On 01/09, Jonathan Tan wrote:
> On Tue,  2 Jan 2018 16:18:09 -0800
> Brandon Williams  wrote:
> 
> > -   while ((len = read_remote_ref(in, _buf, _len, ))) {
> > +   while (state != EXPECTING_DONE) {
> > +   switch (packet_reader_read()) {
> > +   case PACKET_READ_EOF:
> > +   die_initial_contact(1);
> > +   case PACKET_READ_NORMAL:
> > +   len = reader.pktlen;
> > +   if (len > 4 && skip_prefix(packet_buffer, "ERR ", ))
> 
> This should be a field in reader, not the global packet_buffer, I think.

Thanks for catching that.

> 
> Also, I did a search of usages of packet_buffer, and there are just a
> few of them - it might be worthwhile to eliminate it, and have each
> component using it allocate its own buffer. But this can be done in a
> separate patch set.

I'll go through and eliminate the references to packet_buffer by passing
in the buffer explicitly.

> 
> > @@ -269,6 +284,8 @@ struct ref **get_remote_heads(int in, char *src_buf, 
> > size_t src_len,
> > if (process_shallow(len, shallow_points))
> > break;
> > die("protocol error: unexpected '%s'", packet_buffer);
> 
> Here too.

-- 
Brandon Williams

Re: [PATCH 3/8] Doc/gitsubmodules: specify how submodules help in reduced size

[Stefan Beller]

On Tue, Jan 9, 2018 at 8:01 AM, Kaartic Sivaraam
 wrote:
> On Tuesday 09 January 2018 12:08 AM, Stefan Beller wrote:
>>> diff --git a/Documentation/gitsubmodules.txt 
>>> b/Documentation/gitsubmodules.txt
>>> index cb795c6b6..3f73983d5 100644
>>> --- a/Documentation/gitsubmodules.txt
>>> +++ b/Documentation/gitsubmodules.txt
>>> @@ -63,6 +63,9 @@ Submodules can be used for at least two different use 
>>> cases:
>>>  * Transfer size:
>>>In its current form Git requires the whole working tree present. It
>>>does not allow partial trees to be transferred in fetch or clone.
>>> +  If you have your project as multiple repositories tied together as
>>> +  submodules in a superproject, you can avoid fetching the working
>>> +  trees of the repositories you are not interested in.
>>
>> You do not fetch a working tree, but a whole repository?
>>
>
> Maybe I misunderstood submodules when I wrote that example. Could you
> help out with a better and precise replacement?

If your project consists of multiple repositories tied together, some submodules
may not be of interest for all users, who do not need to fetch such submodule
repositories.

> Just putting in some context as to why I did this change, I thought this
> was the only thing that lacked an example and wanted to make it consistent.

Oh, sure I like the example; I was just worried about the wording, as a worktree
is part of a repository, and the repository is the whole thing. In the
current situation
you can only fetch all-or-nothing, specifically you cannot fetch "just
the worktree"
(a shallow clone/fetch is the closest to that, but that still has the
same amount of
information the .git dir, than in the working tree)

Re: [PATCH 0/4] "Fast-"track Git GUI changes

[Junio C Hamano]

Johannes Schindelin  writes:

> As it seems to be impossible to get the attention of the Git GUI
> maintainer these "days" (I have opened Pull Requests on October 16th
> 2016 that have not even been looked at), let's just side-step that
> contribution path which seems to be dormant.

Good to see that finally somebody else steps up after I did the same
for a few times recently.

> These fixes have been in Git for Windows for various amounts of time.
>
> Note: there are more Git GUI changes in Git for Windows, I only
> accumulated the ones I deem wort considering for inclusion into v2.16.0,
> still.

Thanks.  I am not sure if it is too late for 2.16, as these are not
fixes for regression during this cycle, though.

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

[Junio C Hamano]

Lars Schneider  writes:

>> On 14 Dec 2017, at 00:00, Junio C Hamano  wrote:
>> 
>> Here are the topics that have been cooking.  Commits prefixed with
>> '-' are only in 'pu' (proposed updates) while commits prefixed with
>> '+' are in 'next'.  The ones marked with '.' do not appear in any of
>> the integration branches, but I am still holding onto them.
>> 
>> You can find the changes described here in the integration branches
>> of the repositories listed at
>> 
>>http://git-blame.blogspot.com/p/git-public-repositories.html
>> ...
>> 
>> * jk/progress-delay-fix (2017-12-04) 2 commits
>>  (merged to 'next' on 2017-12-05 at 8e62c2b18b)
>> + progress: drop delay-threshold code
>> + progress: set default delay threshold to 100%, not 0%
>> 
>> A regression in the progress eye-candy was fixed.
>
> Hi Junio,
>
> this fixes a bug that affects the Git LFS community (not only
> eye-candy). Would it be possible to get this into Git 2.15.2?

The topic is in 'master' and is a candidate to eventually hit the
'maint' track.  I do not know if 2.15.2 is warranted, though, as we
are getting closer to 2.16 and supposed to be concentrating more on
regression fixes.

Thanks.

Re: [PATCH] bisect: debug: convert struct object to object_id

[Junio C Hamano]

Yasushi SHOJI  writes:

> The commit f2fd0760f62e79609fef7bfd7ecebb002e8e4ced converted struct
> object to object_id but a debug function show_list(), which is
> ifdef'ed to noop, in bisect.c wasn't.
>
> So fix it.
>
> Signed-off-by: Yasushi SHOJI 
> ---

Thanks.  That's quite an old breakage ;-)



>  bisect.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 3756f127b..0dd0f289a 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -132,7 +132,7 @@ static void show_list(const char *debug, int counted, int 
> nr,
>   unsigned flags = commit->object.flags;
>   enum object_type type;
>   unsigned long size;
> - char *buf = read_sha1_file(commit->object.sha1, , );
> + char *buf = read_sha1_file(commit->object.oid.hash, , 
> );
>   const char *subject_start;
>   int subject_len;
>  
> @@ -144,10 +144,10 @@ static void show_list(const char *debug, int counted, 
> int nr,
>   fprintf(stderr, "%3d", weight(p));
>   else
>   fprintf(stderr, "---");
> - fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.sha1));
> + fprintf(stderr, " %.*s", 8, 
> sha1_to_hex(commit->object.oid.hash));
>   for (pp = commit->parents; pp; pp = pp->next)
>   fprintf(stderr, " %.*s", 8,
> - sha1_to_hex(pp->item->object.sha1));
> + sha1_to_hex(pp->item->object.oid.hash));
>  
>   subject_len = find_commit_subject(buf, _start);
>   if (subject_len)

Re:

[Emile Kenold]

-- 
My name is Mrs. Emile Kenold from London. I was diagnosed of lung
cancer which had damaged my liver and my health is no longer
responding to medical treatments.

I have made up my mind to give a charity donation of $11 Million to
you and i pray you will be sincere to use this money for charity work
according to my will, to help orphans, widows and also build schools
for less privilege ones, please i need your sincere and urgent
response to entrust this money to you due to my current health
condition.

Regards
Emile.

[PATCH v5 4/4] status: support --no-ahead-behind in long format

[Jeff Hostetler]

From: Jeff Hostetler 

Teach long (normal) status format to respect the --no-ahead-behind
parameter and skip the possibly expensive ahead/behind computation
between the branch and the upstream.

Signed-off-by: Jeff Hostetler 
---
 builtin/checkout.c   |  2 +-
 remote.c | 18 +-
 remote.h |  3 ++-
 t/t6040-tracking-info.sh | 29 +
 wt-status.c  |  2 +-
 5 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd..655dac2 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -605,7 +605,7 @@ static void report_tracking(struct branch_info *new)
struct strbuf sb = STRBUF_INIT;
struct branch *branch = branch_get(new->name);
 
-   if (!format_tracking_info(branch, ))
+   if (!format_tracking_info(branch, , AHEAD_BEHIND_FULL))
return;
fputs(sb.buf, stdout);
strbuf_release();
diff --git a/remote.c b/remote.c
index 2486afb..e668091 100644
--- a/remote.c
+++ b/remote.c
@@ -2066,15 +2066,16 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
 /*
  * Return true when there is anything to report, otherwise false.
  */
-int format_tracking_info(struct branch *branch, struct strbuf *sb)
+int format_tracking_info(struct branch *branch, struct strbuf *sb,
+enum ahead_behind_flags abf)
 {
-   int ours, theirs;
+   int ours, theirs, sti;
const char *full_base;
char *base;
int upstream_is_gone = 0;
 
-   if (stat_tracking_info(branch, , , _base,
-  AHEAD_BEHIND_FULL) < 0) {
+   sti = stat_tracking_info(branch, , , _base, abf);
+   if (sti < 0) {
if (!full_base)
return 0;
upstream_is_gone = 1;
@@ -2088,10 +2089,17 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
if (advice_status_hints)
strbuf_addstr(sb,
_("  (use \"git branch --unset-upstream\" to 
fixup)\n"));
-   } else if (!ours && !theirs) {
+   } else if (!sti) {
strbuf_addf(sb,
_("Your branch is up to date with '%s'.\n"),
base);
+   } else if (abf == AHEAD_BEHIND_QUICK) {
+   strbuf_addf(sb,
+   _("Your branch and '%s' refer to different 
commits.\n"),
+   base);
+   if (advice_status_hints)
+   strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
+   "git status --ahead-behind");
} else if (!theirs) {
strbuf_addf(sb,
Q_("Your branch is ahead of '%s' by %d commit.\n",
diff --git a/remote.h b/remote.h
index 27feb63..b2fa5cc 100644
--- a/remote.h
+++ b/remote.h
@@ -265,7 +265,8 @@ enum ahead_behind_flags {
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
   const char **upstream_name, enum ahead_behind_flags abf);
-int format_tracking_info(struct branch *branch, struct strbuf *sb);
+int format_tracking_info(struct branch *branch, struct strbuf *sb,
+enum ahead_behind_flags abf);
 
 struct ref *get_local_heads(void);
 /*
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 0190220..716283b 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -160,6 +160,35 @@ test_expect_success 'status -s -b --no-ahead-behind 
(diverged from upstream)' '
 '
 
 cat >expect <<\EOF
+On branch b1
+Your branch and 'origin/master' have diverged,
+and have 1 and 1 different commits each, respectively.
+EOF
+
+test_expect_success 'status --long --branch' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status --long -b | head -3
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
+On branch b1
+Your branch and 'origin/master' refer to different commits.
+EOF
+
+test_expect_success 'status --long --branch --no-ahead-behind' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status --long -b --no-ahead-behind | head -2
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index a4d3470..98d0501 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1006,7 +1006,7 @@ static void wt_longstatus_print_tracking(struct wt_status 
*s)
if (!skip_prefix(s->branch, "refs/heads/", _name))
return;
branch = branch_get(branch_name);
-   if (!format_tracking_info(branch, ))
+   if (!format_tracking_info(branch, , s->ahead_behind_flags))
   

[PATCH v5 2/4] status: add --[no-]ahead-behind to status and commit for V2 format.

[Jeff Hostetler]

From: Jeff Hostetler 

Teach "git status" and "git commit" to accept "--no-ahead-behind"
and "--ahead-behind" arguments to request quick or full ahead/behind
reporting.

When "--no-ahead-behind" is given, the existing porcelain V2 line
"branch.ab +x -y" is replaced with a new "branch.ab +? -?" line.
This indicates that the branch and its upstream are or are not equal
without the expense of computing the full ahead/behind values.

Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt |  5 
 builtin/commit.c |  7 +
 remote.c |  2 ++
 remote.h |  5 ++--
 t/t7064-wtstatus-pv2.sh  | 62 
 wt-status.c  | 30 ++---
 wt-status.h  |  2 ++
 7 files changed, 102 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a..603bf40 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -111,6 +111,11 @@ configuration variable documented in linkgit:git-config[1].
without options are equivalent to 'always' and 'never'
respectively.
 
+--ahead-behind::
+--no-ahead-behind::
+   Display or do not display detailed ahead/behind counts for the
+   branch relative to its upstream branch.  Defaults to true.
+
 ...::
See the 'pathspec' entry in linkgit:gitglossary[7].
 
diff --git a/builtin/commit.c b/builtin/commit.c
index be370f6..cfe7c62 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1137,6 +1137,9 @@ static void finalize_deferred_config(struct wt_status *s)
s->show_branch = status_deferred_config.show_branch;
if (s->show_branch < 0)
s->show_branch = 0;
+
+   if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+   s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
@@ -1351,6 +1354,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 N_("show branch information")),
OPT_BOOL(0, "show-stash", _stash,
 N_("show stash information")),
+   OPT_BOOL(0, "ahead-behind", _behind_flags,
+N_("compute full ahead/behind values")),
{ OPTION_CALLBACK, 0, "porcelain", _format,
  N_("version"), N_("machine-readable output"),
  PARSE_OPT_OPTARG, opt_parse_porcelain },
@@ -1628,6 +1633,8 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
OPT_SET_INT(0, "short", _format, N_("show status 
concisely"),
STATUS_FORMAT_SHORT),
OPT_BOOL(0, "branch", _branch, N_("show branch 
information")),
+   OPT_BOOL(0, "ahead-behind", _behind_flags,
+N_("compute full ahead/behind values")),
OPT_SET_INT(0, "porcelain", _format,
N_("machine-readable output"), 
STATUS_FORMAT_PORCELAIN),
OPT_SET_INT(0, "long", _format,
diff --git a/remote.c b/remote.c
index 23177f5..2486afb 100644
--- a/remote.c
+++ b/remote.c
@@ -2028,6 +2028,8 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
return 0;
if (abf == AHEAD_BEHIND_QUICK)
return 1;
+   if (abf != AHEAD_BEHIND_FULL)
+   BUG("stat_tracking_info: invalid abf '%d'", abf);
 
/* Run "rev-list --left-right ours...theirs" internally... */
argv_array_push(, ""); /* ignored */
diff --git a/remote.h b/remote.h
index 00932f5..27feb63 100644
--- a/remote.h
+++ b/remote.h
@@ -257,8 +257,9 @@ enum match_refs_flags {
 
 /* Flags for --ahead-behind option. */
 enum ahead_behind_flags {
-   AHEAD_BEHIND_QUICK = 0,  /* just eq/neq reporting */
-   AHEAD_BEHIND_FULL  = 1,  /* traditional a/b reporting */
+   AHEAD_BEHIND_UNSPECIFIED = -1,
+   AHEAD_BEHIND_QUICK   =  0,  /* just eq/neq reporting */
+   AHEAD_BEHIND_FULL=  1,  /* traditional a/b reporting */
 };
 
 /* Reporting of tracking info */
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index e319fa2..8f79532 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -390,6 +390,68 @@ test_expect_success 'verify upstream fields in branch 
header' '
)
 '
 
+test_expect_success 'verify --[no-]ahead-behind with V2 format' '
+   git checkout master &&
+   test_when_finished "rm -rf sub_repo" &&
+   git clone . sub_repo &&
+   (
+   ## Confirm local master tracks remote master.
+   cd sub_repo &&
+   HUF=$(git rev-parse HEAD) &&
+
+   # Confirm --no-ahead-behind reports traditional branch.ab with 
0/0 for equal branches.
+   cat >expect <<-EOF &&
+   # 

[PATCH v5 3/4] status: update short status to respect --no-ahead-behind

[Jeff Hostetler]

From: Jeff Hostetler 

Teach "git status --short --branch" to respect "--no-ahead-behind"
parameter to skip computing ahead/behind counts for the branch and
its upstream and just report '[different]'.

Signed-off-by: Jeff Hostetler 
---
 t/t6040-tracking-info.sh | 13 +
 wt-status.c  | 11 +++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 8f17fd9..0190220 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -147,6 +147,19 @@ test_expect_success 'status -s -b (diverged from 
upstream)' '
 '
 
 cat >expect <<\EOF
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
+   (
+   cd test &&
+   git checkout b1 >/dev/null &&
+   git status -s -b --no-ahead-behind | head -1
+   ) >actual &&
+   test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index 3fcab57..a4d3470 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1766,7 +1766,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
const char *base;
char *short_base;
const char *branch_name;
-   int num_ours, num_theirs;
+   int num_ours, num_theirs, sti;
int upstream_is_gone = 0;
 
color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## ");
@@ -1792,8 +1792,9 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
 
color_fprintf(s->fp, branch_color_local, "%s", branch_name);
 
-   if (stat_tracking_info(branch, _ours, _theirs, ,
-  AHEAD_BEHIND_FULL) < 0) {
+   sti = stat_tracking_info(branch, _ours, _theirs, ,
+s->ahead_behind_flags);
+   if (sti < 0) {
if (!base)
goto conclude;
 
@@ -1805,12 +1806,14 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
color_fprintf(s->fp, branch_color_remote, "%s", short_base);
free(short_base);
 
-   if (!upstream_is_gone && !num_ours && !num_theirs)
+   if (!upstream_is_gone && !sti)
goto conclude;
 
color_fprintf(s->fp, header_color, " [");
if (upstream_is_gone) {
color_fprintf(s->fp, header_color, LABEL(N_("gone")));
+   } else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK) {
+   color_fprintf(s->fp, header_color, LABEL(N_("different")));
} else if (!num_ours) {
color_fprintf(s->fp, header_color, LABEL(N_("behind ")));
color_fprintf(s->fp, branch_color_remote, "%d", num_theirs);
-- 
2.9.3

[PATCH v5 1/4] stat_tracking_info: return +1 when branches not equal

[Jeff Hostetler]

From: Jeff Hostetler 

Extend stat_tracking_info() to return +1 when branches are not equal and to
take a new "enum ahead_behind_flags" argument to allow skipping the (possibly
expensive) ahead/behind computation.

This will be used in the next commit to allow "git status" to avoid full
ahead/behind calculations for performance reasons.

Signed-off-by: Jeff Hostetler 
---
 ref-filter.c |  8 
 remote.c | 34 +-
 remote.h |  8 +++-
 wt-status.c  |  6 --
 4 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e728b15..23bcdc4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1238,8 +1238,8 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
if (atom->u.remote_ref.option == RR_REF)
*s = show_ref(>u.remote_ref.refname, refname);
else if (atom->u.remote_ref.option == RR_TRACK) {
-   if (stat_tracking_info(branch, _ours,
-  _theirs, NULL)) {
+   if (stat_tracking_info(branch, _ours, _theirs,
+  NULL, AHEAD_BEHIND_FULL) < 0) {
*s = xstrdup(msgs.gone);
} else if (!num_ours && !num_theirs)
*s = "";
@@ -1256,8 +1256,8 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
free((void *)to_free);
}
} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
-   if (stat_tracking_info(branch, _ours,
-  _theirs, NULL))
+   if (stat_tracking_info(branch, _ours, _theirs,
+  NULL, AHEAD_BEHIND_FULL) < 0)
return;
 
if (!num_ours && !num_theirs)
diff --git a/remote.c b/remote.c
index b220f0d..23177f5 100644
--- a/remote.c
+++ b/remote.c
@@ -1977,16 +1977,23 @@ int ref_newer(const struct object_id *new_oid, const 
struct object_id *old_oid)
 }
 
 /*
- * Compare a branch with its upstream, and save their differences (number
- * of commits) in *num_ours and *num_theirs. The name of the upstream branch
- * (or NULL if no upstream is defined) is returned via *upstream_name, if it
- * is not itself NULL.
+ * Lookup the upstream branch for the given branch and if present, optionally
+ * compute the commit ahead/behind values for the pair.
+ *
+ * If abf is AHEAD_BEHIND_FULL, compute the full ahead/behind and return the
+ * counts in *num_ours and *num_theirs.  If abf is AHEAD_BEHIND_QUICK, skip
+ * the (potentially expensive) a/b computation (*num_ours and *num_theirs are
+ * set to zero).
+ *
+ * The name of the upstream branch (or NULL if no upstream is defined) is
+ * returned via *upstream_name, if it is not itself NULL.
  *
  * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
- * upstream defined, or ref does not exist), 0 otherwise.
+ * upstream defined, or ref does not exist).  Returns 0 if the commits are
+ * identical.  Returns 1 if commits are different.
  */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
-  const char **upstream_name)
+  const char **upstream_name, enum ahead_behind_flags abf)
 {
struct object_id oid;
struct commit *ours, *theirs;
@@ -2014,11 +2021,13 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
if (!ours)
return -1;
 
+   *num_theirs = *num_ours = 0;
+
/* are we the same? */
-   if (theirs == ours) {
-   *num_theirs = *num_ours = 0;
+   if (theirs == ours)
return 0;
-   }
+   if (abf == AHEAD_BEHIND_QUICK)
+   return 1;
 
/* Run "rev-list --left-right ours...theirs" internally... */
argv_array_push(, ""); /* ignored */
@@ -2034,8 +2043,6 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
die("revision walk setup failed");
 
/* ... and count the commits on each side. */
-   *num_ours = 0;
-   *num_theirs = 0;
while (1) {
struct commit *c = get_revision();
if (!c)
@@ -2051,7 +2058,7 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs,
clear_commit_marks(theirs, ALL_REV_FLAGS);
 
argv_array_clear();
-   return 0;
+   return 1;
 }
 
 /*
@@ -2064,7 +2071,8 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
char *base;
int upstream_is_gone = 0;
 
-   if (stat_tracking_info(branch, , , _base) < 0) {
+   if (stat_tracking_info(branch, , , _base,
+  AHEAD_BEHIND_FULL) < 0) {
if (!full_base)
return 0;

[PATCH v5 0/4] Add --no-ahead-behind to status

[Jeff Hostetler]

From: Jeff Hostetler 

This is version 5 of my patch series to avoid expensive ahead/behind
calculations in status.  This version completely removes the config
setting and is just the --[no-]ahead-behind command line argument.

Internally (inside MSFT) we have had further discussions in this
area and identified 2 somewhat independent needs:

[1] The first is to just be able to turn off the a/b calculation when
the invoker does not need the result at all.

[2] The second is to greatly speed up or limit the a/b calculation on
extremely large repositories.

In earlier versions of this patch series, there were several
discussions of a limited mode that would look for no more than n
commits or milliseconds.  Also discussed were some problems that
such limiting will have and it was decided to be not worth the effort.

We have been discussing some ways to speed up the calculation on the
client and have tentatively scheduled this shortly.  Hopefully, this
will eliminate the performance problems and reduce the likelyhood
that anyone would need to set a config setting to change the default
behavior (in either porcelain or non-porcelain) formats.

So with that in mind, this version elimates the config setting so that
we have don't to carry forward a soon-to-be-obsolete setting.

All that remains in this version is the command line argument to turn
on/off the a/b calculation.

Jeff Hostetler (4):
  stat_tracking_info: return +1 when branches not equal
  status: add --[no-]ahead-behind to status and commit for V2 format.
  status: update short status to respect --no-ahead-behind
  status: support --no-ahead-behind in long format

 Documentation/git-status.txt |  5 
 builtin/checkout.c   |  2 +-
 builtin/commit.c |  7 +
 ref-filter.c |  8 +++---
 remote.c | 50 +++
 remote.h | 12 +++--
 t/t6040-tracking-info.sh | 42 ++
 t/t7064-wtstatus-pv2.sh  | 62 
 wt-status.c  | 41 -
 wt-status.h  |  2 ++
 10 files changed, 196 insertions(+), 35 deletions(-)

-- 
2.9.3

Re: [PATCH 0/8] Doc/submodules: a few updates

[Stefan Beller]

On Tue, Jan 9, 2018 at 9:06 AM, Kaartic Sivaraam
 wrote:
> On Tuesday 09 January 2018 12:38 AM, Stefan Beller wrote:
>> On Sat, Jan 6, 2018 at 10:46 AM, Kaartic Sivaraam
>>  wrote:
>>
>> While small patches are really appreciated for code (bisect, automated
>> testing, and
>> the general difficulty to reason about code, as a very small change
>> may affect the whole
>> code base), I am not sure if they benefit in documentation.
>> Documentation is a rather
>> local human readable thing, so by changing one sentence we don't
>> affect the understanding
>> of documentation at a completely unrelated place.
>>
>> Also it helps to read more than just sentence fragments, i.e. I tried
>> looking at the
>> whole paragraph for review. May I suggest to squash them all and
>> resend as one patch?
>>
>
> I wouldn't mind that. I thought it might be easy to find to find the
> parts I changed when the patches are small. So, I sent them without
> squashing them together. In case you feel it's not worth, let me know so
> I'll squash them in.
>
> BTW, in case I did squash them in, would it be nice to keep the commit
> subjects of the current patch series as bullet points in the unified
> commit message?

Sure.

>> I wonder if this indicates a lack of documentation when the active
>> flags were introduced.
>> They are found in 'man git config', but maybe we need to spell them
>> out explicitly
>> in the submodule related docs.
>>
>
> Possibly. So, why not in Documentation/gitsubmodules! Here's a replaced
> version of that paragraph,
>
> * The configuration file `$GIT_DIR/config` in the superproject.
>Typically this file is used to specify whether the submodule
>is recursed into at all via the `active` flag for example. A
>submodule is considered active if `submodule..url` is set
>or if the submodules path is present in `submodule.active` or
>if `submodule..url` is set.

I wonder if we'd want to give an example later, and first describe the
mechanics precisely:

   The configuration file `$GIT_DIR/config` in the superproject.
Git only recurses into active submodules. A submodule is
considered active (a) if `submodule..active` is set
or (b) if the submodules path is matches the pathsepc in
`submodule.active` or (c) if `submodule..url` is set.
(c) is a historical artefact and will be ignored if the pathspec
set in (b) excludes the submodule. For example:

[submodule "foo"]
active = false
url = https://example.org/foo
[submodule "bar"]
active = true
url = https://example.org/bar
[submodule "baz"]
url = https://example.org/baz

In the above config only the submodule bar and baz are active,
bar due to (a) and baz due to (c). Another example

[submodule "foo"]
active = true
url = https://example.org/foo
[submodule "bar"]
url = https://example.org/bar
[submodule "baz"]
url = https://example.org/baz
[submodule "bob"]
ignore = true
[submodule]
active = b*
active = (:exclude) baz

In here all submodules except baz (foo, bar, bob) are active.
'foo' due to its own active flag and all the others due to the
submodule active pathspec, which specifies that any submodule
starting with 'b' except 'baz' are also active, no matter if the .url field
is present.

>>> 2.
>>>
>>>  man git submodule
>>>
>>>update
>>>...
>>>
>>>checkout
>>>
>>>
>>>If --force is specified, the submodule will be checked out 
>>> (using git checkout --force if appropriate), even if the commit
>>>specified in the index of the containing repository already 
>>> matches the commit checked out in the submodule.
>>>
>>> I'm not sure this is conveying all the information it should be conveying.
>>> It seems to making the user wonder, "How at all does 'git submodule update 
>>> --force'
>>> differs from 'git submodule update'?" also "using git checkout --force if 
>>> appropriate"
>>> seems to be invoking all sorts confusion as "appropriate" is superfluous.
>>
>> When "submodule update" is invoked with the `--force` flag, that flag is 
>> passed
>> on to the 'checkout' operation. If you do not give the --force, then
>> the checkout
>> will also be done without --force.
>>
>
> If that's the case then shouldn't the "if appropriate" part of "(using
> git checkout --force if appropriate)" be dropped? That seems to make it
> clear, at least for me. Or is intended as '--force' will not be passed
> to git checkout all the time?
>

Yes, essentially we only pass the force flag when we were given the force flag
("when appropriate" :) Not sure how to say that otherwise. But dropping sounds
good)

Stefan

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Junio C Hamano]

Elijah Newren  writes:

> Hi,
>
> On Tue, Jan 9, 2018 at 11:19 AM, Junio C Hamano  wrote:
>
>> > I haven't come up with an addition to the test suite, but I suspect
>> > this change is conceptually wrong.  What if a call to this function
>> > is made during a recursive, inner merge?
>
> Eek, good catch.
>
>>  merge-recursive.c  |  2 +-
>>  t/t3030-merge-recursive.sh | 50 
>> ++
>>  2 files changed, 51 insertions(+), 1 deletion(-)
>>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 780f81a8bd..0fc580d8ca 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,
>> if (oid_eq(>object.oid, >object.oid)) {
>> struct strbuf sb = STRBUF_INIT;
>>
>> -   if (index_has_changes()) {
>> +   if (!o->call_depth && index_has_changes()) {
>> err(o, _("Dirty index: cannot merge (dirty: %s)"),
>> sb.buf);
>> return 0;
>
> Yep, looks good to me; sorry for overlooking this.
>
> Elijah

Thanks.  The breakage is already in 'master' so this fix needs to be
fast-tracked.

Re: [PATCH 09/26] transport: store protocol version

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:11 -0800
Brandon Williams  wrote:

> diff --git a/transport.c b/transport.c
> index 63c3dbab9..2378dcb38 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -118,6 +118,7 @@ struct git_transport_data {
>   struct child_process *conn;
>   int fd[2];
>   unsigned got_remote_heads : 1;
> + enum protocol_version version;

Should this be initialized to protocol_unknown_version? Right now, as
far as I can tell, it is zero-initialized, which means protocol_v0.

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Elijah Newren]

Hi,

On Tue, Jan 9, 2018 at 11:19 AM, Junio C Hamano  wrote:

> > I haven't come up with an addition to the test suite, but I suspect
> > this change is conceptually wrong.  What if a call to this function
> > is made during a recursive, inner merge?

Eek, good catch.

>  merge-recursive.c  |  2 +-
>  t/t3030-merge-recursive.sh | 50 
> ++
>  2 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 780f81a8bd..0fc580d8ca 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,
> if (oid_eq(>object.oid, >object.oid)) {
> struct strbuf sb = STRBUF_INIT;
>
> -   if (index_has_changes()) {
> +   if (!o->call_depth && index_has_changes()) {
> err(o, _("Dirty index: cannot merge (dirty: %s)"),
> sb.buf);
> return 0;

Yep, looks good to me; sorry for overlooking this.

Elijah

Re: [PATCH 07/26] connect: convert get_remote_heads to use struct packet_reader

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:09 -0800
Brandon Williams  wrote:

> - while ((len = read_remote_ref(in, _buf, _len, ))) {
> + while (state != EXPECTING_DONE) {
> + switch (packet_reader_read()) {
> + case PACKET_READ_EOF:
> + die_initial_contact(1);
> + case PACKET_READ_NORMAL:
> + len = reader.pktlen;
> + if (len > 4 && skip_prefix(packet_buffer, "ERR ", ))

This should be a field in reader, not the global packet_buffer, I think.

Also, I did a search of usages of packet_buffer, and there are just a
few of them - it might be worthwhile to eliminate it, and have each
component using it allocate its own buffer. But this can be done in a
separate patch set.

> @@ -269,6 +284,8 @@ struct ref **get_remote_heads(int in, char *src_buf, 
> size_t src_len,
>   if (process_shallow(len, shallow_points))
>   break;
>   die("protocol error: unexpected '%s'", packet_buffer);

Here too.

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Eric Sunshine]

On Tue, Jan 9, 2018 at 1:19 PM, Junio C Hamano  wrote:
> When merging another branch into ours, if their tree is the same as
> the common ancestor's, we can declare that our tree represents the
> result of three-way merge.  In such a case, the recursive merge
> backend incorrectly used to create a commit out of our index, even
> when the index has changes.
>
> A recent fix attempted to prevent this by adding a comparison
> between "our" tree and the index, but forgot that this check must be
> restricted only to the outermost merge.  Inner merges performed by
> the recursive backend across merge bases are by definition made from
> scratch without having any local changes added to the index.  The
> call to index_has_changes() during an inner merge is working on the
> index that has no relation to the merge being performed, preventing
> legitimate merges from getting carried out.
>
> Fix it by limiting the check to the outermost merge.
>
> Signed-off-by: Junio C Hamano 
> ---
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> @@ -678,4 +678,54 @@ test_expect_success 'merge-recursive remembers the names 
> of all base trees' '
> +test_expect_success 'merge-recursive internal merge resolves to the 
> sameness' '
> +   git reset --hard HEAD &&
> +
> +   # We are going to create a history leading to two criss-cross
> +   # branches A and B.  The common ancestor at the bottom, O0,
> +   # has two childs O1 and O2, both of which will be merge base

s/childs/children,/

> +   # between A and B, like so:
> +   #
> +   #   O1---A
> +   #  /  \ /
> +   #O0.
> +   #  \  / \
> +   #   O2---B
> +   #
> +   # The recently added "check to see if the index is different from
> +   # the tree into which something else is getting merged into and

Too many "into"s: s/merged into/merged/

> +   # reject" check must NOT kick in when an inner merge between O1
> +   # and O2 is made.  Both O1 and O2 happen to have the same tree
> +   # as O0 in this test to trigger the bug---whether the inner merge
> +   # is made by merging O2 into O1 or O1 into O2, their common ancestor
> +   # O0 and the branch being merged have the same tree, and the code
> +   # before fix will incorrectly try to look at the index.

Nit: Does "code before fix" belong in this comment? It sounds more
like something you'd say in the commit message.

> +
> +   echo "zero" >file &&
> +   git add file &&
> +   test_tick &&
> +   git commit -m "O0" &&
> +   O0=$(git rev-parse HEAD) &&
> +
> +   test_tick &&
> +   git commit --allow-empty -m "O2" &&

s/O2/O1/

> +   O1=$(git rev-parse HEAD) &&
> +
> +   git reset --hard $O0 &&
> +   test_tick &&
> +   git commit --allow-empty -m "O2" &&
> +   O2=$(git rev-parse HEAD) &&
> +
> +   test_tick &&
> +   git merge -s ours $O1 &&
> +   B=$(git rev-parse HEAD) &&
> +
> +   git reset --hard $O1 &&
> +   test_tick &&
> +   git merge -s ours $O2 &&
> +   A=$(git rev-parse HEAD) &&
> +
> +   git merge $B
> +'

Re: A cry for help, please don't ignore!

[Sandra Younes]

Good Day,

Forgive my indignation if this message comes to you as a surprise and may 
offend your personality for contacting you without your prior consent and 
writing through this channel.

I came across your name and contact on the course of my personal searching when 
i was searching for a foreign reliable partner. I was assured of your 
capability and reliability after going true your profile.

I'm (Miss. Sandra) from Benghazi libya, My father of blessed memory by name 
late General Abdel Fattah Younes who was shot death by Islamist-linked militia 
within the anti-Gaddafi forces on 28th July, 2011 and after two days later my 
mother with my two brothers was killed one early morning by the rebels as 
result of civil war that is going on in my country Libya, then after the burial 
of my parents, my uncles conspired and sold my father's properties and left 
nothing for me. On a faithful morning, I opened my father's briefcase and 
discover a document which he has deposited ($6.250M USD) in a bank in a Turkish 
Bank which has a small branch in Canada with my name as the legitimate/next of 
kin. Meanwhile i have located the bank,and have also discussed the possiblity 
of transfering the fund. My father left a clause to the bank that i must 
introduce a trusted foreign partner who would be my trustee to help me invest 
this fund; hence the need for your assistance,i request that you be my ru
 stee and assist me in thi

You will also be responsible for the investment and management of the fund for 
me and also you will help me get a good school where i will further my 
education.
I agreed to give you 40% of the $6.250M once the transfer is done. this is my 
true life story, I will be glad to receive your respond soonest for more 
details to enable us start and champion the transfer less than 14 banking days 
as i was informed by the bank manager.

Thanks for giving me your attention,

Yours sincerely,
Miss. Sandra Younes

Re: [PATCH] merge-recursive: do not look at the index during recursive merge

[Junio C Hamano]

Junio C Hamano  writes:

> + ...
> + test_tick &&
> + git commit --allow-empty -m "O2" &&
> + O1=$(git rev-parse HEAD) &&
> +
> + git reset --hard $O0 &&
> + test_tick &&
> + git commit --allow-empty -m "O2" &&
> + O2=$(git rev-parse HEAD) &&

Does not affect the validity of the test at all, but the log message
of the $O1 should be made with -m "O1", not with -m "O2".  That fix
will be in the version I'll be queuing.

> +
> + test_tick &&
> + git merge -s ours $O1 &&
> + B=$(git rev-parse HEAD) &&
> +
> + git reset --hard $O1 &&
> + test_tick &&
> + git merge -s ours $O2 &&
> + A=$(git rev-parse HEAD) &&
> +
> + git merge $B
> +'
> +
>  test_done

RE: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

Apologies: I'm trying out a new mailer - it did not end well. Git 2.12.3 is
not able to connect to mail email system without throwing Auth fails.

Sadly,
Randall

[PATCH] merge-recursive: do not look at the index during recursive merge

[Junio C Hamano]

When merging another branch into ours, if their tree is the same as
the common ancestor's, we can declare that our tree represents the
result of three-way merge.  In such a case, the recursive merge
backend incorrectly used to create a commit out of our index, even
when the index has changes.

A recent fix attempted to prevent this by adding a comparison
between "our" tree and the index, but forgot that this check must be
restricted only to the outermost merge.  Inner merges performed by
the recursive backend across merge bases are by definition made from
scratch without having any local changes added to the index.  The
call to index_has_changes() during an inner merge is working on the
index that has no relation to the merge being performed, preventing
legitimate merges from getting carried out.

Fix it by limiting the check to the outermost merge.

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

Junio C Hamano  writes:

> Elijah Newren  writes:
>
>> diff --git a/merge-recursive.c b/merge-recursive.c
>> index 2ecf495cc2..780f81a8bd 100644
>> --- a/merge-recursive.c
>> +++ b/merge-recursive.c
>> @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o,
>>  }
>>  
>>  if (oid_eq(>object.oid, >object.oid)) {
>> +struct strbuf sb = STRBUF_INIT;
>> +
>> +if (index_has_changes()) {
>> +err(o, _("Dirty index: cannot merge (dirty: 
%s)"),
>> +sb.buf);
>> +return 0;
>> +}
>>  output(o, 0, _("Already up to date!"));
>>  *result = head;
>>  return 1;
>
> I haven't come up with an addition to the test suite, but I suspect
> this change is conceptually wrong.  What if a call to this function
> is made during a recursive, inner merge?

Now I have.

 merge-recursive.c  |  2 +-
 t/t3030-merge-recursive.sh | 50 ++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 780f81a8bd..0fc580d8ca 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1954,7 +1954,7 @@ int merge_trees(struct merge_options *o,
if (oid_eq(>object.oid, >object.oid)) {
struct strbuf sb = STRBUF_INIT;
 
-   if (index_has_changes()) {
+   if (!o->call_depth && index_has_changes()) {
err(o, _("Dirty index: cannot merge (dirty: %s)"),
sb.buf);
return 0;
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 9a893b5fe7..12e3b1392d 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -678,4 +678,54 @@ test_expect_success 'merge-recursive remembers the names 
of all base trees' '
test_cmp expect actual
 '
 
+test_expect_success 'merge-recursive internal merge resolves to the sameness' '
+   git reset --hard HEAD &&
+
+   # We are going to create a history leading to two criss-cross
+   # branches A and B.  The common ancestor at the bottom, O0,
+   # has two childs O1 and O2, both of which will be merge base
+   # between A and B, like so:
+   #
+   #   O1---A
+   #  /  \ /
+   #O0.
+   #  \  / \
+   #   O2---B
+   #
+   # The recently added "check to see if the index is different from
+   # the tree into which something else is getting merged into and
+   # reject" check must NOT kick in when an inner merge between O1
+   # and O2 is made.  Both O1 and O2 happen to have the same tree
+   # as O0 in this test to trigger the bug---whether the inner merge
+   # is made by merging O2 into O1 or O1 into O2, their common ancestor
+   # O0 and the branch being merged have the same tree, and the code
+   # before fix will incorrectly try to look at the index.
+
+   echo "zero" >file &&
+   git add file &&
+   test_tick &&
+   git commit -m "O0" &&
+   O0=$(git rev-parse HEAD) &&
+
+   test_tick &&
+   git commit --allow-empty -m "O2" &&
+   O1=$(git rev-parse HEAD) &&
+
+   git reset --hard $O0 &&
+   test_tick &&
+   git commit --allow-empty -m "O2" &&
+   O2=$(git rev-parse HEAD) &&
+
+   test_tick &&
+   git merge -s ours $O1 &&
+   B=$(git rev-parse HEAD) &&
+
+   git reset --hard $O1 &&
+   test_tick &&
+   git merge -s ours $O2 &&
+   A=$(git rev-parse HEAD) &&
+
+   git merge $B
+'
+
 test_done
-- 
2.16.0-rc1-164-gb9fca19b00

[PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Randall S. Becker]

This patch create a configuration variable PATH_MAX that
corresponds with the value in limits.h. The value of PATH_MAX,
if supplied, is added to BASIC_CFLAGS and will validate with
limits.h. PATH_MAX is also added to GIT-BUILD-OPTIONS and is
available in the git test suite.

This patch also creates a test_expected_success_cond, taking a
single function as first argument. In the t0001-init.sh case,
subtest 34 this function is test_path_max_is_sane, although any
0/1 returning function can be used. The prototype allows the long base
path test to be skipped if PATH_MAX is less than 2048 bytes.

Signed-off-by: Randall S. Becker 
---
 Makefile|  9 +
 config.mak.uname|  1 +
 t/t0001-init.sh |  2 +-
 t/test-lib-functions.sh | 31 +++
 t/test-lib.sh   | 42 ++
 5 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 5543dd2..c9b96a6 100644
--- a/Makefile
+++ b/Makefile
@@ -151,6 +151,9 @@ all::
 # in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
 # wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
 #
+# Define PATH_MAX to limit the size of paths used by git and test scripts.
+# This value should be consistent with limits.h
+#
 # Define NEEDS_CRYPTO_WITH_SSL if you need -lcrypto when using -lssl
(Darwin).
 #
 # Define NEEDS_SSL_WITH_CRYPTO if you need -lssl when using -lcrypto
(Darwin).
@@ -1431,6 +1434,9 @@ ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
BASIC_CFLAGS += -DSHA1_MAX_BLOCK_SIZE="$(SHA1_MAX_BLOCK_SIZE)"
 endif
+ifdef PATH_MAX
+   BASIC_CFLAGS += -DPATH_MAX="$(PATH_MAX)"
+endif
 ifdef NO_PERL_MAKEMAKER
export NO_PERL_MAKEMAKER
 endif
@@ -2283,6 +2289,9 @@ endif
 ifdef TEST_GIT_INDEX_VERSION
@echo TEST_GIT_INDEX_VERSION=\''$(subst ','\'',$(subst
','\'',$(TEST_GIT_INDEX_VERSION)))'\' >>$@+
 endif
+ifdef PATH_MAX
+   @echo PATH_MAX=\''$(subst ','\'',$(subst ','\'',$(PATH_MAX)))'\' >>$@+
+endif
@if cmp $@+ $@ >/dev/null 2>&1; then $(RM) $@+; else mv $@+ $@; fi

 ### Detect Python interpreter path changes
diff --git a/config.mak.uname b/config.mak.uname
index 3721cea..06ee503 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -442,6 +442,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
# Missdetected, hence commented out, see below.
#NO_CURL = YesPlease
# Added manually, see above.
+   PATH_MAX = 1024
NEEDS_SSL_WITH_CURL = YesPlease
NEEDS_CRYPTO_WITH_SSL = YesPlease
HAVE_DEV_TTY = YesPlease
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index c4814d2..58dad87 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -315,7 +315,7 @@ test_expect_success 'init with separate gitdir' '
test_path_is_dir realgitdir/refs
 '

-test_expect_success 'init in long base path' '
+test_expect_success_cond 'test_path_max_is_sane' 'init in long base path' '
# exceed initial buffer size of strbuf_getcwd()
component=123456789abcdef &&
test_when_finished "chmod 0700 $component; rm -rf $component" &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 50a9a1d..67e24e9 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -430,6 +430,24 @@ test_expect_success () {
test_finish_
 }

+test_expect_success_cond () {
+   test_start_
+   test "$#" = 3 && { test_cond_func=$1; shift; } ||
+   error "bug in the test script: not parameters to 
test-expect-success-cond"
+   export test_cond_func
+   if ! test_skip_cond "$@"
+   then
+   say >&3 "expecting success: $2"
+   if test_run_ "$2"
+   then
+   test_ok_ "$1"
+   else
+   test_failure_ "$@"
+   fi
+   fi
+   test_finish_
+}
+
 # test_external runs external test scripts that provide continuous
 # test output about their progress, and succeeds/fails on
 # zero/non-zero exit code.  It outputs the test output on stdout even
@@ -536,6 +554,19 @@ test_path_is_dir () {
fi
 }

+test_path_max_is_sane() {
+   if test -z "$PATH_MAX"
+   then
+   retval=1
+   elif test $PATH_MAX -ge 2048
+   then
+   retval=1
+   else
+   retval=0
+   fi
+   return "$retval"
+}
+
 # Check if the directory exists and is empty as expected, barf otherwise.
 test_dir_is_empty () {
test_path_is_dir "$1" &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 30eb743..8d16e9e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -702,6 +702,48 @@ test_skip () {
esac
 }

+test_skip_cond () {
+   to_skip=
+   skipped_reason=
+   if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
+   then
+   to_skip=t
+   skipped_reason="GIT_SKIP_TESTS"
+   fi
+   if test -z "$to_skip" && test -n 

Re: [PATCH 02/26] pkt-line: introduce struct packet_reader

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:04 -0800
Brandon Williams  wrote:

> diff --git a/pkt-line.h b/pkt-line.h
> index 06c468927..c446e886a 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -111,6 +111,63 @@ char *packet_read_line_buf(char **src_buf, size_t 
> *src_len, int *size);
>   */
>  ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
>  
> +struct packet_reader {
> + /* source file descriptor */
> + int fd;
> +
> + /* source buffer and its size */
> + char *src_buffer;
> + size_t src_len;
> +
> + /* buffer that pkt-lines are read into and its size */
> + char *buffer;
> + unsigned buffer_size;

Is the intention to support different buffers in the future?

[snip]

> +/*
> + * Peek the next packet line without consuming it and return the status.
> + * The next call to 'packet_reader_read()' will perform a read of the same 
> line
> + * that was peeked, consuming the line.
> + *
> + * Only a single line can be peeked at a time.

It is logical to me that if you peeked at a line, and then peeked at it
again, you will get the same line - I would phrase this not as a
restriction ("only a single line") but just as a statement of fact (e.g.
"Peeking at the same line multiple times without an intervening
packet_reader_read will return the same result").

> + */
> +extern enum packet_read_status packet_reader_peek(struct packet_reader 
> *reader);
> +
>  #define DEFAULT_PACKET_MAX 1000
>  #define LARGE_PACKET_MAX 65520
>  #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)

Re: upstreaming https://github.com/cgwalters/git-evtag ?

[Santiago Torres]

> > See Documentation/technical/hash-function-transition.txt
> > for how to do it.
> 
> evtag took me a day or two to write initially and doesn't
> impose any requirements on users except a small additional
> bit of software.

I agree that, in nature it shouldn't be difficult, but I also think that
things usually take longer when you try to minimize code reuse and
streamline the system's design.

> In contrast, working on hash-function-transition.txt?  That
> seems like it'd easily consume many person-months of work.
> And that plan only exists post-shatter.io, whereas git-evtag
> long predates both.

I think this is partly true. A hash transition has been brought up
multiple times pre-shattered. In my opinion shattered was a much-needed
PR push for SHA1 deprecation. In practice, things changed very little.

> > Personally I'd dislike to include ev-tags as it might send a signal
> > of "papering over sha1 issues instead of fixing it".
> 
> I don't agree.  I think it's pretty clear that a hash function transition
> would be a huge amount of work - not least because of course
> there are now at least two widely used implementations of git in C,
> plus https://www.eclipse.org/jgit/ plus...

I agree with Stefan here. I think it's better in the long-term to
push for hash-agnosticity. I don't know if git-evtag is hash agnostic,
but if it is not, then we have two transition plans to think about.

> 
> > push certificates are somewhat underdocumented, see the
> 
> Why not call them "git signed pushes"?  Junio's post
> even says "the signed push".

A signed push creates a push certificate.
> 
> And I just looked at this a little bit more but I'm not sure I
> see how this covers the same goal as evtags;

Correct me if I'm wrong (it's been a couple of years) but last time I
read about git evtags, they basically did the following:

1. Create a signed tag.
2. Create a signed statement of all the references.
3. Create a checksum of the checked out code on the tag.
4. Create a tarball of it.

I think 1) is already happening, 2) is very similar information to the
one contained in a push certificate. I don't know how necessary are 3)
and 4), but that's just my very opinionated take on it.

Full disclosure, I published a "competing" solution a couple of years
ago[1] but, in my personal opinion, I think push certificates can
achieve the same security guarantees as my system with very little
changes.

Cheers!
-Santiago.

[1] 
https://www.usenix.org/conference/usenixsecurity16/technical-sessions/presentation/torres-arias


signature.asc
Description: PGP signature

Re: [PATCH 01/26] pkt-line: introduce packet_read_with_status

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:03 -0800
Brandon Williams  wrote:

> -int packet_read(int fd, char **src_buf, size_t *src_len,
> - char *buffer, unsigned size, int options)
> +enum packet_read_status packet_read_with_status(int fd, char **src_buffer, 
> size_t *src_len,
> + char *buffer, unsigned size, 
> int *pktlen,
> + int options)
>  {
> - int len, ret;
> + int len;
>   char linelen[4];
>  
> - ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> - if (ret < 0)
> - return ret;
> + if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
> + return PACKET_READ_EOF;
> +
>   len = packet_length(linelen);
>   if (len < 0)
>   die("protocol error: bad line length character: %.4s", linelen);
> - if (!len) {
> +
> + if (len == 0) {

This change (replacing "!len" with "len == 0") is unnecessary, I think.

>   packet_trace("", 4, 0);
> - return 0;
> + return PACKET_READ_FLUSH;
> + } else if (len >= 1 && len <= 3) {
> + die("protocol error: bad line length character: %.4s", linelen);
>   }

This seems to be more of a "bad line length" than a "bad line length
character".

Also, some of the checks are redundant. Above, it is probably better to
delete "len >= 1", and optionally write "len < 4" instead of "len <= 3"
(to emphasize that the subtraction of 4 below does not result in a
negative value).

> +
>   len -= 4;
> - if (len >= size)
> + if ((len < 0) || ((unsigned)len >= size))
>   die("protocol error: bad line length %d", len);

The "len < 0" check is redundant.

> - ret = get_packet_data(fd, src_buf, src_len, buffer, len, options);
> - if (ret < 0)
> - return ret;
> +
> + if (get_packet_data(fd, src_buffer, src_len, buffer, len, options) < 0)
> + return PACKET_READ_EOF;
>  
>   if ((options & PACKET_READ_CHOMP_NEWLINE) &&
>   len && buffer[len-1] == '\n')

[PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into four
functions: module_deinit(), for_each_listed_submodule(),
deinit_submodule() and deinit_submodule_cb().

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 153 
 git-submodule.sh|  55 +---
 2 files changed, 154 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index dd7737acd..54b0e46fc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -20,6 +20,7 @@
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
 #define OPT_RECURSIVE (1 << 2)
+#define OPT_FORCE (1 << 3)
 
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
  void *cb_data);
@@ -908,6 +909,157 @@ static int module_sync(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct deinit_cb {
+   const char *prefix;
+   unsigned int flags;
+};
+#define DEINIT_CB_INIT { NULL, 0 }
+
+static void deinit_submodule(const char *path, const char *prefix,
+unsigned int flags)
+{
+   const struct submodule *sub;
+   char *displaypath = NULL;
+   struct child_process cp_config = CHILD_PROCESS_INIT;
+   struct strbuf sb_config = STRBUF_INIT;
+   char *sub_git_dir = xstrfmt("%s/.git", path);
+   mode_t mode = 0777;
+
+   sub = submodule_from_path(_oid, path);
+
+   if (!sub || !sub->name)
+   goto cleanup;
+
+   displaypath = get_submodule_displaypath(path, prefix);
+
+   /* remove the submodule work tree (unless the user already did it) */
+   if (is_directory(path)) {
+   struct stat st;
+   /*
+* protect submodules containing a .git directory
+* NEEDSWORK: automatically call absorbgitdirs before
+* warning/die.
+*/
+   if (is_directory(sub_git_dir))
+   die(_("Submodule work tree '%s' contains a .git "
+ "directory use 'rm -rf' if you really want "
+ "to remove it including all of its history"),
+ displaypath);
+
+   if (!(flags & OPT_FORCE)) {
+   struct child_process cp_rm = CHILD_PROCESS_INIT;
+   cp_rm.git_cmd = 1;
+   argv_array_pushl(_rm.args, "rm", "-qn",
+path, NULL);
+
+   if (run_command(_rm))
+   die(_("Submodule work tree '%s' contains local "
+ "modifications; use '-f' to discard 
them"),
+ displaypath);
+   }
+
+   if (!lstat(path, )) {
+   struct strbuf sb_rm = STRBUF_INIT;
+   const char *format;
+
+   strbuf_addstr(_rm, path);
+
+   if (!remove_dir_recursively(_rm, 0))
+   format = _("Cleared directory '%s'\n");
+   else
+   format = _("Could not remove submodule work 
tree '%s'\n");
+
+   if (!(flags & OPT_QUIET))
+   printf(format, displaypath);
+
+   mode = st.st_mode;
+
+   strbuf_release(_rm);
+   }
+   }
+
+   if (mkdir(path, mode))
+   die_errno(_("could not create empty submodule directory %s"),
+ displaypath);
+
+   cp_config.git_cmd = 1;
+   argv_array_pushl(_config.args, "config", "--get-regexp", NULL);
+   argv_array_pushf(_config.args, "submodule.%s\\.", sub->name);
+
+   /* remove the .git/config entries (unless the user already did it) */
+   if (!capture_command(_config, _config, 0) && sb_config.len) {
+   char *sub_key = xstrfmt("submodule.%s", sub->name);
+   /*
+* remove the whole section so we have a clean state when
+* the user later decides to init this submodule again
+*/
+   git_config_rename_section_in_file(NULL, sub_key, NULL);
+   if (!(flags & OPT_QUIET))
+   printf(_("Submodule '%s' (%s) unregistered for path 
'%s'\n"),
+sub->name, sub->url, displaypath);
+   free(sub_key);
+   }
+
+cleanup:
+   free(displaypath);
+   free(sub_git_dir);
+   strbuf_release(_config);
+}
+
+static void deinit_submodule_cb(const struct cache_entry *list_item,
+   

[PATCH v1 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan]

Port the submodule subcommand 'sync' from shell to C using the same
mechanism as that used for porting submodule subcommand 'status'.
Hence, here the function cmd_sync() is ported from shell to C.
This is done by introducing four functions: module_sync(),
sync_submodule(), sync_submodule_cb() and print_default_remote().

The function print_default_remote() is introduced for getting
the default remote as stdout.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 192 
 git-submodule.sh|  57 +
 2 files changed, 193 insertions(+), 56 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a5c4a8a69..dd7737acd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -50,6 +50,20 @@ static char *get_default_remote(void)
return ret;
 }
 
+static int print_default_remote(int argc, const char **argv, const char 
*prefix)
+{
+   const char *remote;
+
+   if (argc != 1)
+   die(_("submodule--helper print-default-remote takes no 
arguments"));
+
+   remote = get_default_remote();
+   if (remote)
+   printf("%s\n", remote);
+
+   return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
return str[0] == '.' && is_dir_sep(str[1]);
@@ -358,6 +372,25 @@ static void module_list_active(struct module_list *list)
*list = active_modules;
 }
 
+static char *get_up_path(const char *path)
+{
+   int i;
+   struct strbuf sb = STRBUF_INIT;
+
+   for (i = count_slashes(path); i; i--)
+   strbuf_addstr(, "../");
+
+   /*
+* Check if 'path' ends with slash or not
+* for having the same output for dir/sub_dir
+* and dir/sub_dir/
+*/
+   if (!is_dir_sep(path[strlen(path) - 1]))
+   strbuf_addstr(, "../");
+
+   return strbuf_detach(, NULL);
+}
+
 static int module_list(int argc, const char **argv, const char *prefix)
 {
int i;
@@ -718,6 +751,163 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct sync_cb {
+   const char *prefix;
+   unsigned int flags;
+};
+
+#define SYNC_CB_INIT { NULL, 0 }
+
+static void sync_submodule(const char *path, const char *prefix,
+  unsigned int flags)
+{
+   const struct submodule *sub;
+   char *remote_key = NULL;
+   char *sub_origin_url, *super_config_url, *displaypath;
+   struct strbuf sb = STRBUF_INIT;
+   struct child_process cp = CHILD_PROCESS_INIT;
+   char *sub_config_path = NULL;
+
+   if (!is_submodule_active(the_repository, path))
+   return;
+
+   sub = submodule_from_path(_oid, path);
+
+   if (sub && sub->url) {
+   if (starts_with_dot_dot_slash(sub->url) || 
starts_with_dot_slash(sub->url)) {
+   char *remote_url, *up_path;
+   char *remote = get_default_remote();
+   strbuf_addf(, "remote.%s.url", remote);
+
+   if (git_config_get_string(sb.buf, _url))
+   remote_url = xgetcwd();
+
+   up_path = get_up_path(path);
+   sub_origin_url = relative_url(remote_url, sub->url, 
up_path);
+   super_config_url = relative_url(remote_url, sub->url, 
NULL);
+
+   free(remote);
+   free(up_path);
+   free(remote_url);
+   } else {
+   sub_origin_url = xstrdup(sub->url);
+   super_config_url = xstrdup(sub->url);
+   }
+   } else {
+   sub_origin_url = "";
+   super_config_url = "";
+   }
+
+   displaypath = get_submodule_displaypath(path, prefix);
+
+   if (!(flags & OPT_QUIET))
+   printf(_("Synchronizing submodule url for '%s'\n"),
+displaypath);
+
+   strbuf_reset();
+   strbuf_addf(, "submodule.%s.url", sub->name);
+   if (git_config_set_gently(sb.buf, super_config_url))
+   die(_("failed to register url for submodule path '%s'"),
+ displaypath);
+
+   if (!is_submodule_populated_gently(path, NULL))
+   goto cleanup;
+
+   prepare_submodule_repo_env(_array);
+   cp.git_cmd = 1;
+   cp.dir = path;
+   argv_array_pushl(, "submodule--helper",
+"print-default-remote", NULL);
+
+   strbuf_reset();
+   if (capture_command(, , 0))
+   die(_("failed to get the default remote for submodule '%s'"),
+ path);
+
+   strbuf_strip_suffix(, "\n");
+   remote_key = xstrfmt("remote.%s.url", sb.buf);
+
+   strbuf_reset();
+   

[PATCH v1 0/2] Incremental rewrite of git-submodules

[Prathamesh Chavan]

The patches [1] and [2] concerning the porting of submodule
subcommands: sync and deinit were updated in accoudance with
the changes made in one of such similar portings made earlier
for submodule subcommand status[3]. Following are the changes
made:

* It was observed that the number of params increased a lot due to flags
  like quiet, recursive, cached, etc, and keeping in mind the future
  subcommand's ported functions as well, a single unsigned int called
  flags was introduced to store all of these flags, instead of having
  parameter for each one.

* To accomodate the possiblity of a direct call to the functions
  deinit_submodule() and sync_submodule(), callback functions were
  introduced.

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

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

[1]: https://public-inbox.org/git/20170807211900.15001-6-pc44...@gmail.com/
[2]: https://public-inbox.org/git/20170807211900.15001-7-pc44...@gmail.com/
[3]: https://public-inbox.org/git/20171006132415.2876-4-pc44...@gmail.com/

Prathamesh Chavan (2):
  submodule: port submodule subcommand 'sync' from shell to C
  submodule: port submodule subcommand 'deinit' from shell to C

 builtin/submodule--helper.c | 345 
 git-submodule.sh| 112 +-
 2 files changed, 347 insertions(+), 110 deletions(-)

-- 
2.14.2

Re: [PATCH 00/26] protocol version 2

[Jonathan Tan]

On Tue,  2 Jan 2018 16:18:02 -0800
Brandon Williams  wrote:

> The following patches extend what I sent out as an WIP
> (https://public-inbox.org/git/20171204235823.63299-1-bmw...@google.com/) and
> implement protocol version 2.

Summarizing (for myself) the rationale for protocol version 2:

The existing protocol has a few pain points: (a) limit on the length of
the capability line (the capability line can be used to include
additional parameters in a backwards-compatible way), (b) difficulty in
creating proxies because of inconsistent flush semantics, and (c) the
need to implement clients twice - once for HTTP and once for
connect-supporting transports. To which we can add another: (d) if we
want to support something entirely new (for example, a server-side "git
log"), we will need a new protocol anyway.

The new functionality introduced in this patch set is probably best done
using a new protocol. If it were done using the existing protocol (by
adding a parameter in the capabilities line), we would still run into
(a) and (c), so we might as well introduce the new protocol now.

Some of the above points are repeats from my previous e-mail:
https://public-inbox.org/git/20171110121347.1f7c184c543622b60164e...@google.com/

> Some changes from that series are as follows:
>  * Lots of various cleanup on the ls-refs and fetch command code, both server
>and client.
>  * Fetch command now supports a stateless-rpc mode which enables communicating
>with a half-duplex connection.

Good to hear about fetch support.

>  * Introduce a new remote-helper command 'connect-half-duplex' which is
>implemented by remote-curl (the http remote-helper).  This allows for a
>client to establish a half-duplex connection and use remote-curl as a proxy
>to wrap requests in http before sending them to the remote end and
>unwrapping the responses and sending them back to the client's stdin.

I'm not sure about the "half-duplex" name - it is half-duplex in that
each side must terminate their communications with a flush, but not
half-duplex in that request-response pairs can overlap each other (e.g.
during negotation during fetch - there is an optimization in which the
client tries to keep two requests pending at a time). I think that the
idea we want to communicate is that requests and responses are always
packetized, stateless, and always happen as a pair.

I wonder if "stateless-connect" is a better keyword - it makes sense to
me (once described) that "stateless" implies that the client sends
everything the server needs at once (thus, in a packet), the server
sends everything the client needs back at once (thus, in a packet), and
then the client must not assume any state-storing on the part of the
server or transport.

>  * The transport code is refactored for ls-remote, fetch, and push to provide 
> a
>list of ref-patterns (based on the refspec being used) when requesting refs
>from the remote end.  This allows the ls-refs code to send this list of
>patterns so the remote end and filter the refs it sends back.

Briefly looking at the implementation, the client seems to incur an
extra roundtrip when using ls-remote (and others) with a v2-supporting
server. I initially didn't like this, but upon further reflection, this
is probably fine for now. The client can be upgraded later, and I think
that clients will eventually want to query git-serve directly for
"ls-refs" first, and then fall back to v0 for ancient servers, instead
of checking git-upload-pack first (as in this patch set) - so, the
support for "ls-refs" here won't be carried forward merely for backwards
compatibility, but will eventually be actively used.

As for the decision to use a new endpoint "git-serve" instead of reusing
"git-upload-pack" (or "git-receive-pack"), reusing the existing one
might allow some sort of optimization later in which the first
"git-upload-pack" query immediately returns with the v2 answer (instead
of redirecting the client to "git-serve"), but this probably doesn't
matter in practice (as I stated above, I think that eventually clients
will query git-serve first).

> This series effectively implements protocol version 2 for listing a remotes
> refs (ls-remote) as well as for fetch for the builtin transports (ssh, git,
> file) and for the http/https transports.  Push is not implemented yet and
> doesn't need to be implemented at the same time as fetch since the
> receive-pack code can default to using protocol v0 when v2 is requested by the
> client.

Agreed - push can be done later.

Re: [PATCH 0/8] Doc/submodules: a few updates

[Kaartic Sivaraam]

On Tuesday 09 January 2018 12:38 AM, Stefan Beller wrote:
> On Sat, Jan 6, 2018 at 10:46 AM, Kaartic Sivaraam
>  wrote:
> 
> While small patches are really appreciated for code (bisect, automated
> testing, and
> the general difficulty to reason about code, as a very small change
> may affect the whole
> code base), I am not sure if they benefit in documentation.
> Documentation is a rather
> local human readable thing, so by changing one sentence we don't
> affect the understanding
> of documentation at a completely unrelated place.
> 
> Also it helps to read more than just sentence fragments, i.e. I tried
> looking at the
> whole paragraph for review. May I suggest to squash them all and
> resend as one patch?
> 

I wouldn't mind that. I thought it might be easy to find to find the
parts I changed when the patches are small. So, I sent them without
squashing them together. In case you feel it's not worth, let me know so
I'll squash them in.

BTW, in case I did squash them in, would it be nice to keep the commit
subjects of the current patch series as bullet points in the unified
commit message?


> 
>>
>> I based these patches on top of 'master'.
> 
> I am not aware of other submodule patches affecting documentation in 
> master..pu,
> so this should be easy to merge.
> 
>>
>> Apart from the changes, I saw a few things that needed 
>> improvement/clarification
>> but wasn't able to do that myself due to my limited knowledge of submodules. 
>> They
>> are listed below. I'll add in patches for them if they are correctly 
>> clarified.
>>
>>
>> 1.
>>
>>  man gitsubmodules
>>
>>·   The configuration file $GIT_DIR/config in the superproject. 
>> Typical configuration at this place is controlling if a submodule is
>>recursed into at all via the active flag for example.
>>
>>If the submodule is not yet initialized, then the configuration 
>> inside the submodule does not exist yet, so configuration where to
>>obtain the submodule from is configured here for example.
>>
>> What's the "active flag" mentioned above? Also I find the phrase "is 
>> recursed into at all"
>> to be a little slippery. How could it be improved?
> 
> There are multiple ways to indicate if a submodule is "active", i.e. if Git is
> supposed to pay attentio. Historically we had to set the
> submodule..url flag in the config, but last year Brandon added
> submodule.active as well as submodule..active which supersede
> the .url flag.
> 
> (See is_submodule_active() in submodule.c to see the definitive answer to
> "should Git pay attention?")
> https://github.com/git/git/blob/master/submodule.c#L224
> 

Thanks for the info!


> I wonder if this indicates a lack of documentation when the active
> flags were introduced.
> They are found in 'man git config', but maybe we need to spell them
> out explicitly
> in the submodule related docs.
> 

Possibly. So, why not in Documentation/gitsubmodules! Here's a replaced
version of that paragraph,

* The configuration file `$GIT_DIR/config` in the superproject.
   Typically this file is used to specify whether the submodule
   is recursed into at all via the `active` flag for example. A
   submodule is considered active if `submodule..url` is set
   or if the submodules path is present in `submodule.active` or
   if `submodule..url` is set.


>> 2.
>>
>>  man git submodule
>>
>>update
>>...
>>
>>checkout
>>
>>
>>If --force is specified, the submodule will be checked out 
>> (using git checkout --force if appropriate), even if the commit
>>specified in the index of the containing repository already 
>> matches the commit checked out in the submodule.
>>
>> I'm not sure this is conveying all the information it should be conveying.
>> It seems to making the user wonder, "How at all does 'git submodule update 
>> --force'
>> differs from 'git submodule update'?" also "using git checkout --force if 
>> appropriate"
>> seems to be invoking all sorts confusion as "appropriate" is superfluous.
> 
> When "submodule update" is invoked with the `--force` flag, that flag is 
> passed
> on to the 'checkout' operation. If you do not give the --force, then
> the checkout
> will also be done without --force.
> 

If that's the case then shouldn't the "if appropriate" part of "(using
git checkout --force if appropriate)" be dropped? That seems to make it
clear, at least for me. Or is intended as '--force' will not be passed
to git checkout all the time?

>>
>> How could these confusions be clarified?
> 
> I tried giving an alternative snippet above, not sure how else to tell.
> 



-- 
Kaartic

Quote: "Be creative. Be adventurous. Be original. And above all else, be
young." - Wonder Woman



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v4 0/4] Add --no-ahead-behind to status

[Johannes Schindelin]

Hi Stolee,

On Tue, 9 Jan 2018, Derrick Stolee wrote:

> On 1/9/2018 8:15 AM, Johannes Schindelin wrote:
> >
> > On Tue, 9 Jan 2018, Jeff King wrote:
> >
> > > But I don't think you can approximate both ahead and behind together
> > > without finding the actual merge base.
> > >
> > > But even still, finding small answers quickly and accurately and
> > > punting to "really far, I didn't bother to compute it" on the big
> > > ones would be an improvement over always punting.
> >
> > Indeed. The longer I think about it, the more I like the "100+ commits
> > apart" idea.
> 
> Again, I strongly suggest we drop this approach because it will be more pain
> than it is worth.

So what you are saying is if there is a commit graph with *heavy* clock
skew, you might overestimate how many commits apart the tips are.

I say that this is striking the balance between correctness and usability
on the *wrong* side.

Sure, it might be wrong if your commit graph suffers heavily from clock
skew. In most cases, you still get a pretty darn useful hint where you're
at.

The alternative would be *not* to show any useful hint in most cases, i.e.
when you did not find all merge bases within  commits. I would really
hate it if Git spent so much time and did not even give me a hint. Totally
unsatisfying user experience.

Ciao,
Johannes

Re: [PATCH 6/8] Doc/gitsubmodules: improve readability of certain lines

[Kaartic Sivaraam]

On Tuesday 09 January 2018 12:19 AM, Stefan Beller wrote:
> On Sat, Jan 6, 2018 at 10:46 AM, Kaartic Sivaraam
>  wrote:
>>
>> - * The command line for those commands that support taking submodule
>> -   specifications. Most commands have a boolean flag '--recurse-submodules
>> -   whether to recurse into submodules. Examples are `ls-files` or 
>> `checkout`.
>> + * The command line arguments of those commands that support taking 
>> submodule
>> +   specifications. Most commands have a boolean flag '--recurse-submodules'
>> +   which specify whether they should recurse into submodules. Examples are
>> +   `ls-files` or `checkout`.
>> Some commands take enums, such as `fetch` and `push`, where you can
>> specify how submodules are affected.
>>
>> @@ -90,8 +91,8 @@ Submodule operations can be configured using the following 
>> mechanisms
>>  For example an effect from the submodule's `.gitignore` file
>>  would be observed when you run `git status --ignore-submodules=none` in
>>  the superproject. This collects information from the submodule's working
>> -directory by running `status` in the submodule, which does pay attention
>> -to its `.gitignore` file.
>> +directory by running `status` in the submodule while paying attention
>> +to the `.gitignore` file of the submodule.
> 
> Both are grammatically correct and expressive, thanks!
>

You're welcome!


>>  +
> 
> Extra spurious line?
>

No. That's a "real" plus in the document that's usually present between
paragraphs :) I think I now get why Junio suggests people to review
patches in context (possibly, by applying them) ;)


>>  The submodule's `$GIT_DIR/config` file would come into play when running
>>  `git push --recurse-submodules=check` in the superproject, as this would
>> @@ -107,13 +108,13 @@ If the submodule is not yet initialized, then the 
>> configuration
>>  inside the submodule does not exist yet, so configuration where to
>>  obtain the submodule from is configured here for example.
>>

I caught this in the context while replying. "so configuration where to
obtain the submodule from is configured here for example." doesn't seem
to read well. Maybe removing configuration from the sentence will make
it sound better?


>> - * the `.gitmodules` file inside the superproject. Additionally to the
>> -   required mapping between submodule's name and path, a project usually
>> + * The `.gitmodules` file inside the superproject. Additionally, if mapping
>> +   is required between a submodule's name and its path, a project usually
> 
> This changes meaning, originally it tries to say:
> 
> * it requires mapping path <-> names.

I get this ...

> * but there can be more.
> 

... but not this. Did the previous version really try to say this?
Anyways how does this sound?

  * The `.gitmodules` file inside the superproject. A project usually
uses this file to suggest defaults for the upstream collection
of repositories for the mapping that is required between a
submodule's name and its path.

I think it conveys the "it requires mapping path <-> names." correctly
but doesn't convey the "but there can be more." part. I'm not sure how
to get that into the sentence, correctly.



signature.asc
Description: OpenPGP digital signature

Re: rebase preserve-merges: incorrect merge commits

[Johannes Schindelin]

Hi Matwey,

On Tue, 9 Jan 2018, Matwey V. Kornilov wrote:

> 2018-01-09 16:25 GMT+03:00 Johannes Schindelin :
> > Hi Matwey,
> >
> > On Tue, 9 Jan 2018, Matwey V. Kornilov wrote:
> >
> >> 2018-01-08 22:36 GMT+03:00 Johannes Schindelin 
> >> :
> >> >
> >> > On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
> >> >
> >> >> 2018-01-08 19:32 GMT+03:00 Johannes Schindelin 
> >> >> :
> >> >> >
> >> >> > On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
> >> >> >
> >> >> >> 2018-01-08 17:42 GMT+03:00 Matwey V. Kornilov 
> >> >> >> :
> >> >> >> > 2018-01-08 16:56 GMT+03:00 Johannes Schindelin 
> >> >> >> > :
> >> >> >> >> Hi Matwey,
> >> >> >> >>
> >> >> >> >> On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
> >> >> >> >>
> >> >> >> >>> I think that rebase preserve-merges algorithm needs further
> >> >> >> >>> improvements. Probably, you already know it.
> >> >> >> >>
> >> >> >> >> Yes. preserve-merges is a fundamentally flawed design.
> >> >> >> >>
> >> >> >> >> Please have a look here:
> >> >> >> >>
> >> >> >> >> https://github.com/git/git/pull/447
> >> >> >> >>
> >> >> >> >> Since we are in a feature freeze in preparation for v2.16.0, I 
> >> >> >> >> will
> >> >> >> >> submit these patch series shortly after v2.16.0 is released.
> >> >> >> >>
> >> >> >> >>> As far as I understand the root cause of this that when new merge
> >> >> >> >>> commit is created by rebase it is done simply by git merge
> >> >> >> >>> $new_parents without taking into account any actual state of the
> >> >> >> >>> initial merge commit.
> >> >> >> >>
> >> >> >> >> Indeed. preserve-merges does not allow commits to be reordered. 
> >> >> >> >> (Actually,
> >> >> >> >> it *does* allow it, but then fails to handle it correctly.) We 
> >> >> >> >> even have
> >> >> >> >> test cases that mark this as "known breakage".
> >> >> >> >>
> >> >> >> >> But really, I do not think it is worth trying to fix the broken 
> >> >> >> >> design.
> >> >> >> >> Better to go with the new recreate-merges. (I am biased, of 
> >> >> >> >> course,
> >> >> >> >> because I invented recreate-merges. But then, I also invented
> >> >> >> >> preserve-merges, so ...)
> >> >> >> >
> >> >> >> > Well. I just checked --recreate-merges=no-rebase-cousins from the 
> >> >> >> > PR
> >> >> >> > and found that it produces the same wrong result in my test 
> >> >> >> > example.
> >> >> >> > The topology is reproduced correctly, but merge-commit content is
> >> >> >> > broken.
> >> >> >> > I did git rebase --recreate-merges=no-rebase-cousins --onto 
> >> >> >> > abc-0.1 v0.1 abc-0.2
> >> >> >>
> >> >> >> Indeed, exactly as you still say in the documentation: "Merge 
> >> >> >> conflict
> >> >> >> resolutions or manual amendments to merge commits are not preserved."
> >> >> >> My initial point is that they have to be preserved. Probably in
> >> >> >> recreate-merges, if preserve-merges is discontinued.
> >> >> >
> >> >> > Ah, but that is consistent with how non-merge-preserving rebase 
> >> >> > works: the
> >> >> > `pick` commands *also* do not record merge conflict resolution...
> >> >> >
> >> >>
> >> >> I am sorry, didn't get it. When I do non-merge-preserving rebase
> >> >> --interactive there is no way to `pick' merge-commit at all.
> >> >
> >> > Right, but you can `pick` commits and you can get merge conflicts. And 
> >> > you
> >> > need to resolve those merge conflicts and those merge conflict 
> >> > resolutions
> >> > are not preserved for future interactive rebases, unless you use `rerere`
> >> > (in which case it also extends to `pick`ing merge commits in
> >> > merge-preserving mode).
> >>
> >> Are you talking about merge conflicts arising due to commits reordering?
> >
> > Merge conflicts can arise from commit reordering, and they can also arise
> > from commits introduced in "upstream" in the meantime.
> 
> Then I am totally agree with you.
> But initially I said about conflict resolutions and amendments already
> contained in existing merge-commits. While rerere can at least learn
> conflict resolutions from existing merge-commits, rerere cannot learn
> and recover manual amendments.

Great, so the information is all there and you can implement it? :-)

Ciao,
Johannes

Re: [PATCH 4/8] Doc/gitsubmodules: avoid abbreviations

[Kaartic Sivaraam]

On Tuesday 09 January 2018 12:15 AM, Stefan Beller wrote:
>>
>> - * The command line for those commands that support taking submodule specs.
> 
> ++ The command line for those commands that support taking submodules
> as part of their pathspecs[1].
> ++
> ++[1] pathspec is an official term according to `man gitglossary`.
> 
> Maybe?
> 

Yeah, I actually did think 'specification' wasn't a the best fit for
this (should have mentioned that somewhere) Now, the real term came out :)

Just to be sure, that "[1] pathspec ..." part goes to the end of the
document doesn't it?


>> -   Most commands have a boolean flag '--recurse-submodules' whether to
>> -   recurse into submodules. Examples are `ls-files` or `checkout`.
>> + * The command line for those commands that support taking submodule
>> +   specifications. Most commands have a boolean flag '--recurse-submodules
>> +   whether to recurse into submodules. Examples are `ls-files` or 
>> `checkout`.
>> Some commands take enums, such as `fetch` and `push`, where you can
>> specify how submodules are affected.
>>
>> --
>> 2.16.0.rc0.223.g4a4ac8367
>>




signature.asc
Description: OpenPGP digital signature

Re: [PATCH 3/8] Doc/gitsubmodules: specify how submodules help in reduced size

[Kaartic Sivaraam]

On Tuesday 09 January 2018 12:08 AM, Stefan Beller wrote:
>> diff --git a/Documentation/gitsubmodules.txt 
>> b/Documentation/gitsubmodules.txt
>> index cb795c6b6..3f73983d5 100644
>> --- a/Documentation/gitsubmodules.txt
>> +++ b/Documentation/gitsubmodules.txt
>> @@ -63,6 +63,9 @@ Submodules can be used for at least two different use 
>> cases:
>>  * Transfer size:
>>In its current form Git requires the whole working tree present. It
>>does not allow partial trees to be transferred in fetch or clone.
>> +  If you have your project as multiple repositories tied together as
>> +  submodules in a superproject, you can avoid fetching the working
>> +  trees of the repositories you are not interested in.
> 
> You do not fetch a working tree, but a whole repository?
> 

Maybe I misunderstood submodules when I wrote that example. Could you
help out with a better and precise replacement?

Just putting in some context as to why I did this change, I thought this
was the only thing that lacked an example and wanted to make it consistent.


-- 
Kaartic



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/8] Doc/gitsubmodules: clearly specify advantage of submodule

[Kaartic Sivaraam]

>>content that is not compressed by delta computation between trees.
>> -  However you can also use submodules to e.g. hold large binary assets
>> +  Therefore you can use submodules to hold large binary assets
> 
> If this improves readability by a lot, I'd be all for it. But this use
> case is just
> exemplary. There are also cases of submodules that do not contain big files,
> but e.g. have a lengthy history with lots of small files.
> So I don't know, as I would want to keep emphasized that this is just
> an example.
> 

You're right I actually screwed up by emphasizing that was the only use
case it was envisioned for. Fixed it by replacing "Therefore" with " For
example, ..."

Thanks,
Kaartic



signature.asc
Description: OpenPGP digital signature

[PATCH] doc/read-tree: remove obsolete remark

[Andreas G. Schacker]

Earlier versions of `git read-tree` required the `--prefix` option value
to end with a slash. This restriction was eventually lifted without a
corresponding amendment to the documentation.

Signed-off-by: Andreas G. Schacker 
---
 Documentation/git-read-tree.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-read-tree.txt b/Documentation/git-read-tree.txt
index 72bd809fb..f2a07d54d 100644
--- a/Documentation/git-read-tree.txt
+++ b/Documentation/git-read-tree.txt
@@ -81,12 +81,11 @@ OPTIONS
 * when both sides add a path identically.  The resolution
   is to add that path.
 
---prefix=/::
+--prefix=::
Keep the current index contents, and read the contents
of the named tree-ish under the directory at ``.
The command will refuse to overwrite entries that already
-   existed in the original index file. Note that the `/`
-   value must end with a slash.
+   existed in the original index file.
 
 --exclude-per-directory=::
When running the command with `-u` and `-m` options, the
-- 
2.15.1

merge-base --is-ancestor A B is unreasonably slow with unrelated history B

[Ævar Arnfjörð Bjarmason]

This is a pathological case I don't have time to dig into right now:

git branch -D orphan;
git checkout --orphan orphan &&
git reset --hard &&
touch foo &&
git add foo &&
git commit -m"foo" &&
time git merge-base --is-ancestor master orphan

This takes around 5 seconds on linux.git to return 1. Which is around
the same time it takes to run current master against the first commit in
linux.git:

git merge-base --is-ancestor 1da177e4c3f4 master

This is obviously a pathological case, but maybe we should work slightly
harder on the RHS of and discover that it itself is an orphan commit.

I ran into this while writing a hook where we'd like to do:

git diff $master...topic

Or not, depending on if the topic is an orphan or just something
recently branched off, figured I could use --is-ancestor as on
optimization, and then discovered it's not much of an optimization.

Re: [PATCH v4 0/4] Add --no-ahead-behind to status

[Jeff Hostetler]

On 1/9/2018 9:29 AM, Derrick Stolee wrote:

On 1/9/2018 8:15 AM, Johannes Schindelin wrote:

Hi Peff,

On Tue, 9 Jan 2018, Jeff King wrote:


On Mon, Jan 08, 2018 at 03:04:20PM -0500, Jeff Hostetler wrote:


I was thinking about something similar to the logic we use today
about whether to start reporting progress on other long commands.
That would mean you could still get the ahead/behind values if you
aren't that far behind but would only get "different" if that
calculation gets too expensive (which implies the actual value isn't
going to be all that helpful anyway).

After a off-line conversation with the others I'm going to look into
a version that is limited to n commits rather than be completely on or
off.  I think if you are for example less than 100 a/b then those numbers
have value; if you are past n, then they have much less value.

I'd rather do it by a fixed limit than by time to ensure that output
is predictable on graph shape and not on system load.

I like this direction a lot. I had hoped we could say "100+ commits
ahead",

How about "100+ commits apart" instead?


Unfortunately, we can _never_ guarantee more than 1 commit ahead/behind unless 
we walk to the merge base (or have generation numbers). For example, suppose 
the 101st commit in each history has a parent that in the recent history of the 
other commit. (There must be merge commits to make this work without creating 
cycles, but the ahead/behind counts could be much lower than the number of 
walked commits.)




but I don't think we can do so accurately without generation numbers.

Even with generation numbers, it is not possible to say whether two given
commits reflect diverging branches or have an ancestor-descendant
relationship (or in graph speak: whether they are comparable).


If you walk commits using a priority queue where the priority is the generation 
number, then you can know that you have walked all reachable commits with 
generation greater than X, so you know among those commits which are comparable 
or not.

For this to work accurately, you must walk from both tips to a generation lower 
than each. It does not help the case where one branch is 100,000+ commits 
ahead, where most of those commits have higher generation number than the 
behind commit.


It could potentially make it possible to cut off the commit traversal, but
I do not even see how that would be possible.

The only thing you could say for sure is that two different commits with
the same generation number are for sure "uncomparable", i.e. reflect
diverging branches.


E.g., the case I mentioned at the bottom of this mail:

   https://public-inbox.org/git/20171224143318.gc23...@sigill.intra.peff.net/

I haven't thought too hard on it, but I suspect with generation numbers
you could bound it and at least say "100+ ahead" or "100+ behind".

If you have walked 100 commits and still have not found a merge base,
there is no telling whether one start point is the ancestor of the other.
All you can say is that there are more than 100 commits in the
"difference".

You would not even be able to say that the *shortest* path between those
two start points is longer than 100 commits, you can construct
pathological DAGs pretty easily.

Even if you had generation numbers, and one commit's generation number
was, say, 17, and the other one's was 17,171, you could not necessarily
assume that the 17 one is the ancestor of the 17,171 one, all you can say
that it is not possible the other way round.


This is why we cannot _always_ use generation numbers, but they do help in some 
cases.


But I don't think you can approximate both ahead and behind together
without finding the actual merge base.

But even still, finding small answers quickly and accurately and punting
to "really far, I didn't bother to compute it" on the big ones would be
an improvement over always punting.

Indeed. The longer I think about it, the more I like the "100+ commits
apart" idea.



Again, I strongly suggest we drop this approach because it will be more pain 
than it is worth.


Good comments all.  Thanks!
I will leave the patch series as it is with the existing on/off setting
and call it quits.

Thanks,
Jeff

[PATCH 0/4] "Fast-"track Git GUI changes

[Johannes Schindelin]

As it seems to be impossible to get the attention of the Git GUI
maintainer these "days" (I have opened Pull Requests on October 16th
2016 that have not even been looked at), let's just side-step that
contribution path which seems to be dormant.

These fixes have been in Git for Windows for various amounts of time.

Note: there are more Git GUI changes in Git for Windows, I only
accumulated the ones I deem wort considering for inclusion into v2.16.0,
still.


Johannes Schindelin (4):
  git gui: fix staging a second line to a 1-line file
  git-gui: avoid exception upon Ctrl+T in an empty list
  git-gui: fix exception when trying to stage with empty file list
  git-gui: allow Ctrl+T to toggle multiple paths

 git-gui/git-gui.sh   | 27 ++-
 git-gui/lib/diff.tcl |  1 +
 2 files changed, 27 insertions(+), 1 deletion(-)


base-commit: 36438dc19dd2a305dddebd44bf7a65f1a220075b
Published-As: https://github.com/dscho/git/releases/tag/misc-git-gui-stuff-v1
Fetch-It-Via: git fetch https://github.com/dscho/git misc-git-gui-stuff-v1
-- 
2.15.1.windows.2.395.g5bb0817ee52

[PATCH 2/4] git-gui: avoid exception upon Ctrl+T in an empty list

[Johannes Schindelin]

Previously unstaged files can be staged by clicking on them and then
pressing Ctrl+T. Conveniently, the next unstaged file is selected
automatically so that the unstaged files can be staged by repeatedly
pressing Ctrl+T.

When a user hits Ctrl+T one time too many, though, Git GUI used to throw
this exception:

expected number but got ""
expected number but got ""
while executing
"expr {int([lindex [$w tag ranges in_diff] 0])}"
(procedure "toggle_or_diff" line 13)
invoked from within
"toggle_or_diff toggle .vpane.files.workdir.list "
(command bound to event)

Let's just avoid that by skipping the operation when there are no more
files to stage.

This fixes https://github.com/git-for-windows/git/issues/1060

Signed-off-by: Johannes Schindelin 
---
 git-gui/git-gui.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index ed24aa9d2f1..3c085cddc61 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2504,6 +2504,10 @@ proc toggle_or_diff {mode w args} {
if {$last_clicked ne {}} {
set lno [lindex $last_clicked 1]
} else {
+   if {[llength $file_lists($w)] == 0} {
+   set last_clicked {}
+   return
+   }
set lno [expr {int([lindex [$w tag ranges in_diff] 0])}]
}
if {$mode eq "toggle"} {
-- 
2.15.1.windows.2.395.g5bb0817ee52

[PATCH 4/4] git-gui: allow Ctrl+T to toggle multiple paths

[Johannes Schindelin]

It is possible to select multiple files in the "Unstaged Changes" and
the "Staged Changes" lists. But when hitting Ctrl+T, surprisingly only
one entry is handled, not all selected ones.

Let's just use the same code path as for the "Stage To Commit" and the
"Unstage From Commit" menu items.

This fixes https://github.com/git-for-windows/git/issues/1012

Signed-off-by: Johannes Schindelin 
---
 git-gui/git-gui.sh | 13 +
 1 file changed, 13 insertions(+)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index ca2cdebdc4f..91c00e64893 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2501,6 +2501,19 @@ proc toggle_or_diff {mode w args} {
set pos [split [$w index @$x,$y] .]
foreach {lno col} $pos break
} else {
+   if {$mode eq "toggle"} {
+   if {$w eq $ui_workdir} {
+   do_add_selection
+   set last_clicked {}
+   return
+   }
+   if {$w eq $ui_index} {
+   do_unstage_selection
+   set last_clicked {}
+   return
+   }
+   }
+
if {$last_clicked ne {}} {
set lno [lindex $last_clicked 1]
} else {
-- 
2.15.1.windows.2.395.g5bb0817ee52

[PATCH 3/4] git-gui: fix exception when trying to stage with empty file list

[Johannes Schindelin]

If there is nothing to stage, there is nothing to stage. Let's not try
to, even if the file list contains nothing at all.

This fixes https://github.com/git-for-windows/git/issues/1075

Signed-off-by: Johannes Schindelin 
---
 git-gui/git-gui.sh | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 3c085cddc61..ca2cdebdc4f 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -2504,7 +2504,9 @@ proc toggle_or_diff {mode w args} {
if {$last_clicked ne {}} {
set lno [lindex $last_clicked 1]
} else {
-   if {[llength $file_lists($w)] == 0} {
+   if {![info exists file_lists]
+   || ![info exists file_lists($w)]
+   || [llength $file_lists($w)] == 0} {
set last_clicked {}
return
}
@@ -2518,7 +2520,13 @@ proc toggle_or_diff {mode w args} {
}
}
 
-   set path [lindex $file_lists($w) [expr {$lno - 1}]]
+   if {![info exists file_lists]
+   || ![info exists file_lists($w)]
+   || [llength $file_lists($w)] < $lno - 1} {
+   set path {}
+   } else {
+   set path [lindex $file_lists($w) [expr {$lno - 1}]]
+   }
if {$path eq {}} {
set last_clicked {}
return
-- 
2.15.1.windows.2.395.g5bb0817ee52

[PATCH 1/4] git gui: fix staging a second line to a 1-line file

[Johannes Schindelin]

When a 1-line file is augmented by a second line, and the user tries to
stage that single line via the "Stage Line" context menu item, we do not
want to see "apply: corrupt patch at line 5".

The reason for this error was that the hunk header looks like this:

@@ -1 +1,2 @@

but the existing code expects the original range always to contain a
comma. This problem is easily fixed by cutting the string "1 +1,2"
(that Git GUI formerly mistook for the starting line) at the space.

This fixes https://github.com/git-for-windows/git/issues/515

Signed-off-by: Johannes Schindelin 
---
 git-gui/lib/diff.tcl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index 4cae10a4c7f..68c4a6c7366 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -698,6 +698,7 @@ proc apply_range_or_line {x y} {
set hh [$ui_diff get $i_l "$i_l + 1 lines"]
set hh [lindex [split $hh ,] 0]
set hln [lindex [split $hh -] 1]
+   set hln [lindex [split $hln " "] 0]
 
# There is a special situation to take care of. Consider this
# hunk:
-- 
2.15.1.windows.2.395.g5bb0817ee52

Re: [PATCH v4 0/4] Add --no-ahead-behind to status

[Derrick Stolee]

On 1/9/2018 8:15 AM, Johannes Schindelin wrote:

Hi Peff,

On Tue, 9 Jan 2018, Jeff King wrote:


On Mon, Jan 08, 2018 at 03:04:20PM -0500, Jeff Hostetler wrote:


I was thinking about something similar to the logic we use today
about whether to start reporting progress on other long commands.
That would mean you could still get the ahead/behind values if you
aren't that far behind but would only get "different" if that
calculation gets too expensive (which implies the actual value isn't
going to be all that helpful anyway).

After a off-line conversation with the others I'm going to look into
a version that is limited to n commits rather than be completely on or
off.  I think if you are for example less than 100 a/b then those numbers
have value; if you are past n, then they have much less value.

I'd rather do it by a fixed limit than by time to ensure that output
is predictable on graph shape and not on system load.

I like this direction a lot. I had hoped we could say "100+ commits
ahead",

How about "100+ commits apart" instead?


Unfortunately, we can _never_ guarantee more than 1 commit ahead/behind 
unless we walk to the merge base (or have generation numbers). For 
example, suppose the 101st commit in each history has a parent that in 
the recent history of the other commit. (There must be merge commits to 
make this work without creating cycles, but the ahead/behind counts 
could be much lower than the number of walked commits.)





but I don't think we can do so accurately without generation numbers.

Even with generation numbers, it is not possible to say whether two given
commits reflect diverging branches or have an ancestor-descendant
relationship (or in graph speak: whether they are comparable).


If you walk commits using a priority queue where the priority is the 
generation number, then you can know that you have walked all reachable 
commits with generation greater than X, so you know among those commits 
which are comparable or not.


For this to work accurately, you must walk from both tips to a 
generation lower than each. It does not help the case where one branch 
is 100,000+ commits ahead, where most of those commits have higher 
generation number than the behind commit.



It could potentially make it possible to cut off the commit traversal, but
I do not even see how that would be possible.

The only thing you could say for sure is that two different commits with
the same generation number are for sure "uncomparable", i.e. reflect
diverging branches.


E.g., the case I mentioned at the bottom of this mail:

   https://public-inbox.org/git/20171224143318.gc23...@sigill.intra.peff.net/

I haven't thought too hard on it, but I suspect with generation numbers
you could bound it and at least say "100+ ahead" or "100+ behind".

If you have walked 100 commits and still have not found a merge base,
there is no telling whether one start point is the ancestor of the other.
All you can say is that there are more than 100 commits in the
"difference".

You would not even be able to say that the *shortest* path between those
two start points is longer than 100 commits, you can construct
pathological DAGs pretty easily.

Even if you had generation numbers, and one commit's generation number
was, say, 17, and the other one's was 17,171, you could not necessarily
assume that the 17 one is the ancestor of the 17,171 one, all you can say
that it is not possible the other way round.


This is why we cannot _always_ use generation numbers, but they do help 
in some cases.



But I don't think you can approximate both ahead and behind together
without finding the actual merge base.

But even still, finding small answers quickly and accurately and punting
to "really far, I didn't bother to compute it" on the big ones would be
an improvement over always punting.

Indeed. The longer I think about it, the more I like the "100+ commits
apart" idea.



Again, I strongly suggest we drop this approach because it will be more 
pain than it is worth.


Thanks,
-Stolee

Re: rebase preserve-merges: incorrect merge commits

[Matwey V. Kornilov]

2018-01-09 16:25 GMT+03:00 Johannes Schindelin :
> Hi Matwey,
>
> On Tue, 9 Jan 2018, Matwey V. Kornilov wrote:
>
>> 2018-01-08 22:36 GMT+03:00 Johannes Schindelin :
>> >
>> > On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
>> >
>> >> 2018-01-08 19:32 GMT+03:00 Johannes Schindelin 
>> >> :
>> >> >
>> >> > On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
>> >> >
>> >> >> 2018-01-08 17:42 GMT+03:00 Matwey V. Kornilov 
>> >> >> :
>> >> >> > 2018-01-08 16:56 GMT+03:00 Johannes Schindelin 
>> >> >> > :
>> >> >> >> Hi Matwey,
>> >> >> >>
>> >> >> >> On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
>> >> >> >>
>> >> >> >>> I think that rebase preserve-merges algorithm needs further
>> >> >> >>> improvements. Probably, you already know it.
>> >> >> >>
>> >> >> >> Yes. preserve-merges is a fundamentally flawed design.
>> >> >> >>
>> >> >> >> Please have a look here:
>> >> >> >>
>> >> >> >> https://github.com/git/git/pull/447
>> >> >> >>
>> >> >> >> Since we are in a feature freeze in preparation for v2.16.0, I will
>> >> >> >> submit these patch series shortly after v2.16.0 is released.
>> >> >> >>
>> >> >> >>> As far as I understand the root cause of this that when new merge
>> >> >> >>> commit is created by rebase it is done simply by git merge
>> >> >> >>> $new_parents without taking into account any actual state of the
>> >> >> >>> initial merge commit.
>> >> >> >>
>> >> >> >> Indeed. preserve-merges does not allow commits to be reordered. 
>> >> >> >> (Actually,
>> >> >> >> it *does* allow it, but then fails to handle it correctly.) We even 
>> >> >> >> have
>> >> >> >> test cases that mark this as "known breakage".
>> >> >> >>
>> >> >> >> But really, I do not think it is worth trying to fix the broken 
>> >> >> >> design.
>> >> >> >> Better to go with the new recreate-merges. (I am biased, of course,
>> >> >> >> because I invented recreate-merges. But then, I also invented
>> >> >> >> preserve-merges, so ...)
>> >> >> >
>> >> >> > Well. I just checked --recreate-merges=no-rebase-cousins from the PR
>> >> >> > and found that it produces the same wrong result in my test example.
>> >> >> > The topology is reproduced correctly, but merge-commit content is
>> >> >> > broken.
>> >> >> > I did git rebase --recreate-merges=no-rebase-cousins --onto abc-0.1 
>> >> >> > v0.1 abc-0.2
>> >> >>
>> >> >> Indeed, exactly as you still say in the documentation: "Merge conflict
>> >> >> resolutions or manual amendments to merge commits are not preserved."
>> >> >> My initial point is that they have to be preserved. Probably in
>> >> >> recreate-merges, if preserve-merges is discontinued.
>> >> >
>> >> > Ah, but that is consistent with how non-merge-preserving rebase works: 
>> >> > the
>> >> > `pick` commands *also* do not record merge conflict resolution...
>> >> >
>> >>
>> >> I am sorry, didn't get it. When I do non-merge-preserving rebase
>> >> --interactive there is no way to `pick' merge-commit at all.
>> >
>> > Right, but you can `pick` commits and you can get merge conflicts. And you
>> > need to resolve those merge conflicts and those merge conflict resolutions
>> > are not preserved for future interactive rebases, unless you use `rerere`
>> > (in which case it also extends to `pick`ing merge commits in
>> > merge-preserving mode).
>>
>> Are you talking about merge conflicts arising due to commits reordering?
>
> Merge conflicts can arise from commit reordering, and they can also arise
> from commits introduced in "upstream" in the meantime.

Then I am totally agree with you.
But initially I said about conflict resolutions and amendments already
contained in existing merge-commits. While rerere can at least learn
conflict resolutions from existing merge-commits, rerere cannot learn
and recover manual amendments.

>
> Ciao,
> Johannes



-- 
With best regards,
Matwey V. Kornilov

Re: rebase preserve-merges: incorrect merge commits

[Johannes Schindelin]

Hi Matwey,

On Tue, 9 Jan 2018, Matwey V. Kornilov wrote:

> 2018-01-08 22:36 GMT+03:00 Johannes Schindelin :
> >
> > On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
> >
> >> 2018-01-08 19:32 GMT+03:00 Johannes Schindelin 
> >> :
> >> >
> >> > On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
> >> >
> >> >> 2018-01-08 17:42 GMT+03:00 Matwey V. Kornilov 
> >> >> :
> >> >> > 2018-01-08 16:56 GMT+03:00 Johannes Schindelin 
> >> >> > :
> >> >> >> Hi Matwey,
> >> >> >>
> >> >> >> On Mon, 8 Jan 2018, Matwey V. Kornilov wrote:
> >> >> >>
> >> >> >>> I think that rebase preserve-merges algorithm needs further
> >> >> >>> improvements. Probably, you already know it.
> >> >> >>
> >> >> >> Yes. preserve-merges is a fundamentally flawed design.
> >> >> >>
> >> >> >> Please have a look here:
> >> >> >>
> >> >> >> https://github.com/git/git/pull/447
> >> >> >>
> >> >> >> Since we are in a feature freeze in preparation for v2.16.0, I will
> >> >> >> submit these patch series shortly after v2.16.0 is released.
> >> >> >>
> >> >> >>> As far as I understand the root cause of this that when new merge
> >> >> >>> commit is created by rebase it is done simply by git merge
> >> >> >>> $new_parents without taking into account any actual state of the
> >> >> >>> initial merge commit.
> >> >> >>
> >> >> >> Indeed. preserve-merges does not allow commits to be reordered. 
> >> >> >> (Actually,
> >> >> >> it *does* allow it, but then fails to handle it correctly.) We even 
> >> >> >> have
> >> >> >> test cases that mark this as "known breakage".
> >> >> >>
> >> >> >> But really, I do not think it is worth trying to fix the broken 
> >> >> >> design.
> >> >> >> Better to go with the new recreate-merges. (I am biased, of course,
> >> >> >> because I invented recreate-merges. But then, I also invented
> >> >> >> preserve-merges, so ...)
> >> >> >
> >> >> > Well. I just checked --recreate-merges=no-rebase-cousins from the PR
> >> >> > and found that it produces the same wrong result in my test example.
> >> >> > The topology is reproduced correctly, but merge-commit content is
> >> >> > broken.
> >> >> > I did git rebase --recreate-merges=no-rebase-cousins --onto abc-0.1 
> >> >> > v0.1 abc-0.2
> >> >>
> >> >> Indeed, exactly as you still say in the documentation: "Merge conflict
> >> >> resolutions or manual amendments to merge commits are not preserved."
> >> >> My initial point is that they have to be preserved. Probably in
> >> >> recreate-merges, if preserve-merges is discontinued.
> >> >
> >> > Ah, but that is consistent with how non-merge-preserving rebase works: 
> >> > the
> >> > `pick` commands *also* do not record merge conflict resolution...
> >> >
> >>
> >> I am sorry, didn't get it. When I do non-merge-preserving rebase
> >> --interactive there is no way to `pick' merge-commit at all.
> >
> > Right, but you can `pick` commits and you can get merge conflicts. And you
> > need to resolve those merge conflicts and those merge conflict resolutions
> > are not preserved for future interactive rebases, unless you use `rerere`
> > (in which case it also extends to `pick`ing merge commits in
> > merge-preserving mode).
> 
> Are you talking about merge conflicts arising due to commits reordering?

Merge conflicts can arise from commit reordering, and they can also arise
from commits introduced in "upstream" in the meantime.

Ciao,
Johannes

Re: [PATCH v4 0/4] Add --no-ahead-behind to status

[Johannes Schindelin]

Hi Peff,

On Tue, 9 Jan 2018, Jeff King wrote:

> On Mon, Jan 08, 2018 at 03:04:20PM -0500, Jeff Hostetler wrote:
> 
> > > I was thinking about something similar to the logic we use today
> > > about whether to start reporting progress on other long commands.
> > > That would mean you could still get the ahead/behind values if you
> > > aren't that far behind but would only get "different" if that
> > > calculation gets too expensive (which implies the actual value isn't
> > > going to be all that helpful anyway).
> > 
> > After a off-line conversation with the others I'm going to look into
> > a version that is limited to n commits rather than be completely on or
> > off.  I think if you are for example less than 100 a/b then those numbers
> > have value; if you are past n, then they have much less value.
> > 
> > I'd rather do it by a fixed limit than by time to ensure that output
> > is predictable on graph shape and not on system load.
> 
> I like this direction a lot. I had hoped we could say "100+ commits
> ahead",

How about "100+ commits apart" instead?

> but I don't think we can do so accurately without generation numbers.

Even with generation numbers, it is not possible to say whether two given
commits reflect diverging branches or have an ancestor-descendant
relationship (or in graph speak: whether they are comparable).

It could potentially make it possible to cut off the commit traversal, but
I do not even see how that would be possible.

The only thing you could say for sure is that two different commits with
the same generation number are for sure "uncomparable", i.e. reflect
diverging branches.

> E.g., the case I mentioned at the bottom of this mail:
> 
>   https://public-inbox.org/git/20171224143318.gc23...@sigill.intra.peff.net/
> 
> I haven't thought too hard on it, but I suspect with generation numbers
> you could bound it and at least say "100+ ahead" or "100+ behind".

If you have walked 100 commits and still have not found a merge base,
there is no telling whether one start point is the ancestor of the other.
All you can say is that there are more than 100 commits in the
"difference".

You would not even be able to say that the *shortest* path between those
two start points is longer than 100 commits, you can construct
pathological DAGs pretty easily.

Even if you had generation numbers, and one commit's generation number
was, say, 17, and the other one's was 17,171, you could not necessarily
assume that the 17 one is the ancestor of the 17,171 one, all you can say
that it is not possible the other way round.

> But I don't think you can approximate both ahead and behind together
> without finding the actual merge base.
> 
> But even still, finding small answers quickly and accurately and punting
> to "really far, I didn't bother to compute it" on the big ones would be
> an improvement over always punting.

Indeed. The longer I think about it, the more I like the "100+ commits
apart" idea.

Ciao,
Dscho