Re: submodule network operations [WAS: Re: [RFC/PATCH 0/4] working tree operations: support superprefix]

[Stefan Beller]

>> Between the init and the update step you can modify the URLs.
>> These commands are just a repetition from the first email, but the
>> git commands can be viewed as moving from one state to another
>> for submodules; submodules itself can be seen as a state machine
>> according to that proposed documentation. Maybe such a state machine
>> makes it easier to understand for some people.
>
>
> "Between the init and the update step you can modify the URLs."  Yes I can
> and have to... wish it was not this way.

So how would yo u rather want to do it?
look at the .gitmodules file beforehand and then run a "submodule update" ?
Or a thing like

git -c url.https://internal.insteadOf git://github.com/ \
-c submodule.record-rewritten-urls submodule update

(no need for init there as theoretically there is not
need for such an intermediate step)


>> [remote "origin"]
>>url = https://github.com/..
>> [remote "inhouse"]
>>url = https://inhouse.corp/..
>>
>> But where do we clone it from?
>> (Or do we just do a "git init" on that submodule and fetch
>> from both remotes? in which order?)
>
> origin by default and inhouse if specified. There is already a implied
> default (origin). The idea was not to do both but rather what is specified.
> Origin and inhouse are just names for remotes. If one wanted a
> "--all-remotes" could pull from everywhere in the Ether if feature was to be
> implemented.

How is origin implied to be the default?
Should there be an order (e.g. if you cannot find it at inhouse get it
from github,
if they are down, get it from kernel.org)

Re: [PATCH] git-prompt.sh: add submodule indicator

[Stefan Beller]

On Thu, Jan 19, 2017 at 4:07 PM, Benjamin Fuchs  wrote:
> I expirienced that working with submodules can be confusing. This indicator
> will make you notice very easy when you switch into a submodule.
> The new prompt will look like this: (sub:master)
> Adding a new optional env variable for the new feature.
>
> Signed-off-by: Benjamin Fuchs 

Thanks for making submodules better :)
Relevant tangent, just posted today:
https://public-inbox.org/git/20170119193023.26837-1-sbel...@google.com/T/#u

> ---
>  contrib/completion/git-prompt.sh | 37 -
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-prompt.sh 
> b/contrib/completion/git-prompt.sh
> index 97eacd7..4c82e7f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -93,6 +93,10 @@
>  # directory is set up to be ignored by git, then set
>  # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
>  # repository level by setting bash.hideIfPwdIgnored to "false".
> +#
> +# If you would like __git_ps1 to indicate that you are in a submodule,
> +# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
> +# the branch name.
>
>  # check whether printf supports -v
>  __git_printf_supports_v=
> @@ -284,6 +288,32 @@ __git_eread ()
> test -r "$f" && read "$@" <"$f"
>  }
>
> +# __git_is_submodule
> +# Based on:
> +# 
> http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
> +__git_is_submodule ()
> +{
> +   local git_dir parent_git module_name path
> +   # Find the root of this git repo, then check if its parent dir is 
> also a repo
> +   git_dir="$(git rev-parse --show-toplevel)"
> +   module_name="$(basename "$git_dir")"
> +   parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> 
> /dev/null)"

I wonder if we want to have better plumbing commands for submodules
here as well:
Here we only check if we have an embedded git repository, which may not be a
submodule. It could be a standalone repo that just happens to be inside another.
It could be a fake submodule [1], though I think the last time I
brought these up,
the upstream Git community considered these fake submodules are bug not worth
fixing.

And this doesn't cover the case that I addressed in the RFC-ish patch above:
  $ git submodule deinit --all
  $ cd  && git status
  # in an ideal world this tells you:
  #  "You are in an un-populated submodule. To change the submodule state..."

So I guess this is a good first approximation that actually gets most
of the cases right,
thereby helping a lot of people. But I wonder about these cornercases as well?

[1] 
debuggable.com/posts/git-fake-submodules:4b563ee4-f3cc-4061-967e-0e48cbdd56cb

Thanks,
Stefan

Re: Git: new feature suggestion

[Linus Torvalds]

On Thu, Jan 19, 2017 at 1:48 PM, Jakub Narębski  wrote:
> W dniu 19.01.2017 o 19:39, Linus Torvalds pisze:
>>
>> You can do it in tig, but I suspect a more graphical tool might be better.
>
> Well, we do have "git gui blame".

Does that actually work for people? Because it really doesn't for me.

And I'm not just talking about the aesthetics of the thing, but the
whole experience, and the whole "dig into parent" which just gives me
an error message.

Linus

[PATCH] git-prompt.sh: add submodule indicator

[Benjamin Fuchs]

I expirienced that working with submodules can be confusing. This indicator
will make you notice very easy when you switch into a submodule.
The new prompt will look like this: (sub:master)
Adding a new optional env variable for the new feature.

Signed-off-by: Benjamin Fuchs 
---
 contrib/completion/git-prompt.sh | 37 -
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 97eacd7..4c82e7f 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -93,6 +93,10 @@
 # directory is set up to be ignored by git, then set
 # GIT_PS1_HIDE_IF_PWD_IGNORED to a nonempty value. Override this on the
 # repository level by setting bash.hideIfPwdIgnored to "false".
+#
+# If you would like __git_ps1 to indicate that you are in a submodule,
+# set GIT_PS1_SHOWSUBMODULE. In this case a "sub:" will be added before
+# the branch name.
 
 # check whether printf supports -v
 __git_printf_supports_v=
@@ -284,6 +288,32 @@ __git_eread ()
test -r "$f" && read "$@" <"$f"
 }
 
+# __git_is_submodule
+# Based on:
+# 
http://stackoverflow.com/questions/7359204/git-command-line-know-if-in-submodule
+__git_is_submodule ()
+{
+   local git_dir parent_git module_name path
+   # Find the root of this git repo, then check if its parent dir is also 
a repo
+   git_dir="$(git rev-parse --show-toplevel)"
+   module_name="$(basename "$git_dir")"
+   parent_git="$(cd "$git_dir/.." && git rev-parse --show-toplevel 2> 
/dev/null)"
+   if [[ -n $parent_git ]]; then
+   # List all the submodule paths for the parent repo
+   while read path
+   do
+   if [[ "$path" != "$module_name" ]]; then continue; fi
+   if [[ -d "$git_dir/../$path" ]];then return 0; fi
+   done < <(cd $parent_git && git submodule --quiet foreach 'echo 
$path' 2> /dev/null)
+fi
+return 1
+}
+
+__git_ps1_submodule ()
+{
+   __git_is_submodule && printf "sub:"
+}
+
 # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
 # when called from PS1 using command substitution
 # in this mode it prints text to add to bash PS1 prompt (includes branch name)
@@ -513,8 +543,13 @@ __git_ps1 ()
b="\${__git_ps1_branch_name}"
fi
 
+   local sub=""
+   if [ -n "${GIT_PS1_SHOWSUBMODULE}" ]; then
+   sub="$(__git_ps1_submodule)"
+   fi
+
local f="$w$i$s$u"
-   local gitstring="$c$b${f:+$z$f}$r$p"
+   local gitstring="$c$sub$b${f:+$z$f}$r$p"
 
if [ $pcmode = yes ]; then
if [ "${__git_printf_supports_v-}" != yes ]; then
-- 
2.7.4

Re: [RFC for GIT] pull-request: add praise to people doing QA

[Jacob Keller]

On Thu, Jan 19, 2017 at 4:06 PM, Joe Perches 
wrote:>> This sounds interesting to me! When I have some more time to
take a
>> look at this i might see if I can revive it.
>
> Can the terminology please be standardized to what
> was once called bylines?
>
> https://patchwork.kernel.org/patch/9307703/
>

I am fairly certain we've settled on "trailers" at this point. I don't
have an objection to either name, but most of the code today uses
trailers.

Thanks,
Jake

Re: [RFC for GIT] pull-request: add praise to people doing QA

[Joe Perches]

On Thu, 2017-01-19 at 15:42 -0800, Jacob Keller wrote:
> On Thu, Jan 19, 2017 at 1:20 PM, Jeff King  wrote:
> > On Thu, Jan 19, 2017 at 09:43:45PM +0100, Wolfram Sang wrote:
> > 
> > > > As to the implementation, I am wondering if we can make this somehow
> > > > work well with the "trailers" code we already have, instead of
> > > > inventing yet another parser of trailers.
> > > > 
> > > > In its current shape, "interpret-trailers" focuses on "editing" an
> > > > existing commit log message to tweak the trailer lines.  That mode
> > > > of operation would help amending and rebasing, and to do that it
> > > > needs to parse the commit log message, identify trailer blocks,
> > > > parse out each trailer lines, etc.
> > > > 
> > > > There is no fundamental reason why its output must be an edited
> > > > original commit log message---it should be usable as a filter that
> > > > picks trailer lines of the selected trailer type, like "Tested-By",
> > > > etc.
> > > 
> > > I didn't know about trailers before. As I undestand it, I could use
> > > "Tested-by" as the key, and the commit subject as the value. This list
> > > then could be parsed and brought into proper output shape. It would
> > > simplify the subject parsing, but most things my AWK script currently
> > > does would still need to stay or to be reimplemented (extracting names
> > > from tags, creating arrays of tags given by $name). Am I correct?
> > > 
> > > All under the assumption that trailers work on a range of commits. I
> > > have to admit that adding this to git is beyond my scope.
> > 
> > This sounds a lot like the shortlog-trailers work I did about a year
> > ago:
> > 
> >   http://public-inbox.org/git/20151229073832.gn8...@sigill.intra.peff.net/
> > 
> >   http://public-inbox.org/git/20151229075013.ga9...@sigill.intra.peff.net/
> > 
> > Nobody seemed to really find it useful, so I didn't pursue it.
> > 
> > Some of the preparatory patches in that series bit-rotted in the
> > meantime, but you can play with a version based on v2.7.0 by fetching
> > the "shortlog-trailers-historical" branch from
> > https://github.com/peff/git.git.
> > 
> > And then things like:
> > 
> >   git shortlog --ident=tested-by --format='...tested a patch by %an'
> > 
> > work (and you can put whatever commit items you want into the --format,
> > including just dumping the hash if you want to do more analysis).
> > 
> > -Peff
> 
> This sounds interesting to me! When I have some more time to take a
> look at this i might see if I can revive it.

Can the terminology please be standardized to what
was once called bylines?

https://patchwork.kernel.org/patch/9307703/

Post-merge hook

[Ilesh Mistry]

Hello

I have been looking around for this, but I can't seem to find any examples of 
how to use the git post-merge hook. Can you help me please?

I want to do something really simple in the sense of pulling locally and once 
pulled and merged any conflicts (if any) then run a command line code. But run 
this code after any merge conflicts are resolved.

Can you help me by pointing me into the right direction on how I can resolve 
this please?

Thanks
Ilesh

Sent from my iPhone

Ilesh Mistry
Kentico Technical Architect
T 01572 822 278

www.mmtdigital.co.uk 

RAR Digital Awards Winner 2016: Best Web Development Agency in the UK
RAR Digital Awards Winner 2016: Best Software Development Agency in the UK
RAR Digital Awards Winner 2016: Best Analytics Agency in the UK
RAR Digital Awards Winner 2015: Best Web Design Agency in the UK
Econsultancy 2016 Top 100 Agency
The Drum Digital Census 2015: Elite Agency Top 10


Participation in this email correspondence denotes acceptance of
terms and conditions available on request or from our website,
unless a separate contract has been agreed.

Registered Office: 1A Uppingham Gate, Ayston Road, Uppingham, Rutland, LE15 9NY
Company Number: 3681297
VAT Registration: 638 5654 05

Re: [PATCH v4 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

On Thu, Jan 19, 2017 at 1:06 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Jacob Keller 
>>
>> Teach git name-rev to take multiple --refs stored as a string list of
>> patterns. The list of patterns will be matched inclusively, and each ref
>> only needs to match one pattern to be included. A ref will only be
>> excluded if it does not match any of the given patterns. Additionally,
>> if any of the patterns would allow abbreviation, then we will abbreviate
>> the ref, even if another pattern is more strict and would not have
>> allowed abbreviation on its own.
>>
>> Add tests and documentation for this change. The tests expected output
>> is dynamically generated, but this is in order to avoid hard-coding
>> a commit object id in the test results (as the expected output is to
>> simply leave the commit object unnamed).
>
> Makes sense.
>
> I do not see anything that requires "... generated, but" there,
> though, as if it is a bad thing to do to prepare expected output
> dynamically.  I'd just reword "generated.  This is..." to make it
> neutral.

To clarify my earlier comment, I think the SQUASH?? you queued looks great.

Thanks,
Jake

Re: [RFC for GIT] pull-request: add praise to people doing QA

[Jacob Keller]

On Thu, Jan 19, 2017 at 1:20 PM, Jeff King  wrote:
> On Thu, Jan 19, 2017 at 09:43:45PM +0100, Wolfram Sang wrote:
>
>> > As to the implementation, I am wondering if we can make this somehow
>> > work well with the "trailers" code we already have, instead of
>> > inventing yet another parser of trailers.
>> >
>> > In its current shape, "interpret-trailers" focuses on "editing" an
>> > existing commit log message to tweak the trailer lines.  That mode
>> > of operation would help amending and rebasing, and to do that it
>> > needs to parse the commit log message, identify trailer blocks,
>> > parse out each trailer lines, etc.
>> >
>> > There is no fundamental reason why its output must be an edited
>> > original commit log message---it should be usable as a filter that
>> > picks trailer lines of the selected trailer type, like "Tested-By",
>> > etc.
>>
>> I didn't know about trailers before. As I undestand it, I could use
>> "Tested-by" as the key, and the commit subject as the value. This list
>> then could be parsed and brought into proper output shape. It would
>> simplify the subject parsing, but most things my AWK script currently
>> does would still need to stay or to be reimplemented (extracting names
>> from tags, creating arrays of tags given by $name). Am I correct?
>>
>> All under the assumption that trailers work on a range of commits. I
>> have to admit that adding this to git is beyond my scope.
>
> This sounds a lot like the shortlog-trailers work I did about a year
> ago:
>
>   http://public-inbox.org/git/20151229073832.gn8...@sigill.intra.peff.net/
>
>   http://public-inbox.org/git/20151229075013.ga9...@sigill.intra.peff.net/
>
> Nobody seemed to really find it useful, so I didn't pursue it.
>
> Some of the preparatory patches in that series bit-rotted in the
> meantime, but you can play with a version based on v2.7.0 by fetching
> the "shortlog-trailers-historical" branch from
> https://github.com/peff/git.git.
>
> And then things like:
>
>   git shortlog --ident=tested-by --format='...tested a patch by %an'
>
> work (and you can put whatever commit items you want into the --format,
> including just dumping the hash if you want to do more analysis).
>
> -Peff

This sounds interesting to me! When I have some more time to take a
look at this i might see if I can revive it.

Thanks,
Jake

What's cooking in git.git (Jan 2017, #03; Thu, 19)

[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.

The sixth batch of topics have been merged to 'master'.  This batch
is fairly large, and regression-hunting is very much appreciated.

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"]

* bw/grep-recurse-submodules (2016-12-22) 12 commits
  (merged to 'next' on 2016-12-22 at 1ede815b8d)
 + grep: search history of moved submodules
 + grep: enable recurse-submodules to work on  objects
 + grep: optionally recurse into submodules
 + grep: add submodules as a grep source type
 + submodules: load gitmodules file from commit sha1
 + submodules: add helper to determine if a submodule is initialized
 + submodules: add helper to determine if a submodule is populated
  (merged to 'next' on 2016-12-22 at fea8fa870f)
 + real_path: canonicalize directory separators in root parts
 + real_path: have callers use real_pathdup and strbuf_realpath
 + real_path: create real_pathdup
 + real_path: convert real_path_internal to strbuf_realpath
 + real_path: resolve symlinks by hand
 (this branch is tangled with bw/realpath-wo-chdir.)

 "git grep" has been taught to optionally recurse into submodules.


* bw/pathspec-cleanup (2017-01-08) 16 commits
  (merged to 'next' on 2017-01-10 at 79291ff506)
 + pathspec: rename prefix_pathspec to init_pathspec_item
 + pathspec: small readability changes
 + pathspec: create strip submodule slash helpers
 + pathspec: create parse_element_magic helper
 + pathspec: create parse_long_magic function
 + pathspec: create parse_short_magic function
 + pathspec: factor global magic into its own function
 + pathspec: simpler logic to prefix original pathspec elements
 + pathspec: always show mnemonic and name in unsupported_magic
 + pathspec: remove unused variable from unsupported_magic
 + pathspec: copy and free owned memory
 + pathspec: remove the deprecated get_pathspec function
 + ls-tree: convert show_recursive to use the pathspec struct interface
 + dir: convert fill_directory to use the pathspec struct interface
 + dir: remove struct path_simplify
 + mv: remove use of deprecated 'get_pathspec()'
 (this branch is used by sb/pathspec-errors.)

 Code clean-up in the pathspec API.


* bw/realpath-wo-chdir (2017-01-09) 7 commits
  (merged to 'next' on 2017-01-10 at ed315a40c8)
 + real_path: set errno when max number of symlinks is exceeded
 + real_path: prevent redefinition of MAXSYMLINKS
  (merged to 'next' on 2016-12-22 at fea8fa870f)
 + real_path: canonicalize directory separators in root parts
 + real_path: have callers use real_pathdup and strbuf_realpath
 + real_path: create real_pathdup
 + real_path: convert real_path_internal to strbuf_realpath
 + real_path: resolve symlinks by hand
 (this branch is tangled with bw/grep-recurse-submodules.)

 The implementation of "real_path()" was to go there with chdir(2)
 and call getcwd(3), but this obviously wouldn't be usable in a
 threaded environment.  Rewrite it to manually resolve relative
 paths including symbolic links in path components.


* dt/disable-bitmap-in-auto-gc (2016-12-29) 2 commits
  (merged to 'next' on 2017-01-10 at 9f4e89e15d)
 + repack: die on incremental + write-bitmap-index
 + auto gc: don't write bitmaps for incremental repacks

 It is natural that "git gc --auto" may not attempt to pack
 everything into a single pack, and there is no point in warning
 when the user has configured the system to use the pack bitmap,
 leading to disabling further "gc".


* jk/archive-zip-userdiff-config (2017-01-07) 1 commit
  (merged to 'next' on 2017-01-10 at ac42e4958c)
 + archive-zip: load userdiff config

 "git archive" did not read the standard configuration files, and
 failed to notice a file that is marked as binary via the userdiff
 driver configuration.


* jk/blame-fixes (2017-01-07) 3 commits
  (merged to 'next' on 2017-01-10 at 18f909da61)
 + blame: output porcelain "previous" header for each file
 + blame: handle --no-abbrev
 + blame: fix alignment with --abbrev=40

 "git blame --porcelain" misidentified the "previous" 
 pair (aka "source") when contents came from two or more files.


* jk/execv-dashed-external (2017-01-09) 3 commits
  (merged to 'next' on 2017-01-10 at 117b506cb0)
 + execv_dashed_external: wait for child on signal death
 + execv_dashed_external: stop exiting with negative code
 + execv_dashed_external: use child_process struct

 Typing ^C to pager, which usually does not kill it, killed Git and
 took the pager down as a collateral damage in certain process-tree
 structure.  This has been fixed.


* jk/rebase-i-squash-count-fix (2017-01-07) 1 commit
  

Re: [PATCH v4 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

On Thu, Jan 19, 2017 at 1:06 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Jacob Keller 
>>
>> Teach git name-rev to take multiple --refs stored as a string list of
>> patterns. The list of patterns will be matched inclusively, and each ref
>> only needs to match one pattern to be included. A ref will only be
>> excluded if it does not match any of the given patterns. Additionally,
>> if any of the patterns would allow abbreviation, then we will abbreviate
>> the ref, even if another pattern is more strict and would not have
>> allowed abbreviation on its own.
>>
>> Add tests and documentation for this change. The tests expected output
>> is dynamically generated, but this is in order to avoid hard-coding
>> a commit object id in the test results (as the expected output is to
>> simply leave the commit object unnamed).
>
> Makes sense.
>
> I do not see anything that requires "... generated, but" there,
> though, as if it is a bad thing to do to prepare expected output
> dynamically.  I'd just reword "generated.  This is..." to make it
> neutral.

Makes sense. I was commenting this way since I was requested that
someone on the list preferred non-dynamic test expectations.

Thanks,
Jake

Re: submodule network operations [WAS: Re: [RFC/PATCH 0/4] working tree operations: support superprefix]

[Brian J. Davis]

On 1/17/2017 12:43 PM, Stefan Beller wrote:

On Sun, Jan 15, 2017 at 1:02 PM, Brian J. Davis  wrote:


Technically it is submodule..url instead of
submodule..url. The name is usually the path initially, and once
you move the submodule, only the path changes, the name is supposed to
be constant and stay the same.

I am not certain what is meant by this.  All I know is I can use my
"directory_to_checkout" above to place in tree relative from root the
project any where in the tree not already tracked by git.  You state name
instead of path, but it allows path correct? Either that or I have gone off
reservation with my use of git for years now. Maybe this is a deviation from
how it is documented/should work and how it actually works?  It works great
how I use it.

Yes name can equal the path (and usually does). This is a minor detail
that is only relevant for renaming submodules, so ... maybe let's not
focus on it too much. :)



but if say I want to pull from some server 2 and do a

git submodule update --init --recursive

That is why the "git submodule init" exists at all.

  git submodule init
  $EDIT .git/config # change submodule..url to server2
  git submodule update # that uses the adapted url and
  # creates the submodule repository.

  # From now on the submodule is on its own.
  cd  && git config --list
  # prints an "origin" remote and a lot more

For a better explanation, I started a documentation series, see

https://github.com/gitster/git/commit/e2b51b9df618ceeff7c4ec044e20f5ce9a87241e

(It is not finished, but that is supposed to explain this exact pain
point in the
STATES section, feel free to point out missing things or what is hard
to understand)

I am not sure I got much out of the STATES section regarding my problem.

Your original problem as far as I understand is this:

   You have a project with submodules.
   The submodules are described in the .gitmodules file.
   But the URL is pointing to an undesired location.
   You want to rewrite the URLs before actually cloning the submodules.

And to solve this problem we need to perform multiple steps:

   # --no is the default, just for clarity here:
   git clone  --no-recurse-submodules
   # The submodules are now in the *uninitialized* state

   git submodule init
   # the submodules are in the initialized state

   git submodule update
   # submodules are populated, i.e. cloned from
   # the configured URLs and put into the working tree at
   # the appropriate path.

Between the init and the update step you can modify the URLs.
These commands are just a repetition from the first email, but the
git commands can be viewed as moving from one state to another
for submodules; submodules itself can be seen as a state machine
according to that proposed documentation. Maybe such a state machine
makes it easier to understand for some people.


"Between the init and the update step you can modify the URLs."  Yes I can and 
have to... wish it was not this way.


what I would get is from someserver1 not someserver2 as there is no
"origin"
support for submodules.  Is this correct?  If so can origin support be
added
to submodules?

Can you explain more in detail what you mean by origin support?

Yes so when we do a:

git push origin master

origin is of course the Remote (Remotes
https://git-scm.com/book/en/v2/Git-Basics-Working-with-Remotes)

So I best use terminology "Remotes" support.  Git push supports remotes, but
git submodules does not.  The basic idea is this:

If Git allowed multiple submodule
(https://git-scm.com/book/en/v2/Git-Tools-Submodules) Remotes to be
specified say as an example:

git submodule add [remote] [url]

git submodule add origin https://github.com/chaconinc/DbConnector
git submodule add indhouse https://indhouse .corp/chaconinc/DbConnector

Where:

[submodule "DbConnector"]
 path = DbConnector
 url = https://github.com/chaconinc/DbConnector

Could then change to:

[submodule "DbConnector"]
 path = DbConnector
 remote.origin.url = https://github.com/chaconinc/DbConnector
 remote.origin.url = https://indhouse .corp/chaconinc/DbConnector

here I assume there is a typo and the second remote.origin.url should be
remote.inhouse.url ?

yes second should have read remote.inhouse.url:

[submodule "DbConnector"]
path = DbConnector
remote.origin.url = https://github.com/chaconinc/DbConnector
remote.inhouse.url = https://indhouse.corp/chaconinc/DbConnector



Then it should be possible to get:

git submodules update --init --recursive

which would setup the submodule with both

[remote "origin"]
   url = https://github.com/..
[remote "inhouse"]
   url = https://inhouse.corp/..

But where do we clone it from?
(Or do we just do a "git init" on that submodule and fetch
from both remotes? in which order?)
origin by default and inhouse if specified. There is already a implied 
default (origin). The idea was not to do both but rather what is 
specified.  Origin 

Re: grep open pull requests

[Jeff King]

On Thu, Jan 19, 2017 at 03:12:53PM -0700, Jack Bates wrote:

> Cool, thanks for all your help! "git log --cherry-pick" works quite well.
> One thing: I expected the following to be equivalent, but found that they're
> not. Is that by accident or design?
> 
>   $ git rev-list --cherry-pick --right-only master...refs/pull/1112/head
>   $ git rev-list --cherry-pick master..refs/pull/1112/head

It's by design. The "left" and "right" notions are defined only for a
three-dot symmetric difference.

In the first command you've asked git to look at commits on _both_
master and the PR, down to their merge base. It marks the tips with a
"left" and "right" bit, and then those bits propagate down.

In the second command, you've only asked for the PR commits, and there
is no left/right bit at all. So --cherry-pick is doing nothing, as it
has no "left" commits to compare to.

-Peff

Re: grep open pull requests

[Jack Bates]

On 19/01/17 12:02 PM, Jeff King wrote:

It's much trickier to find from the git topology whether a particular
history contains rebased versions of commits.  You can look at the
--cherry options to "git log", which use patch-ids to try to equate
commits. Something like:

  git for-each-ref --format='%(refname)' 'refs/pull/*/head' |
  while read refname; do
if test -z "$(git rev-list --right-only --cherry-pick -1 
origin...$refname)
then
echo "$refname: not merged"
fi
  done

That's obviously much less efficient than `--no-merged`, but it should
generally work. The exception is if the rebase changed the commit
sufficiently that its patch-id may have changed.


Cool, thanks for all your help! "git log --cherry-pick" works quite 
well. One thing: I expected the following to be equivalent, but found 
that they're not. Is that by accident or design?


  $ git rev-list --cherry-pick --right-only master...refs/pull/1112/head
  $ git rev-list --cherry-pick master..refs/pull/1112/head


I think that's probably the best answer to your "unmerged" question,
too. Ask the API which PRs are unmerged, and then do whatever git-level
analysis you want based on that.


Right, that makes sense. Thanks again!

Re: [RESEND PATCHv2] contrib: remove git-convert-objects

[Stefan Beller]

On Thu, Jan 19, 2017 at 12:57 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> git-convert-objects, originally named git-convert-cache was used in
>> early 2005 to convert to a new repository format, e.g. adding an author
>> date.
>
> I think this description is not wrong per-se but misses the much
> more important point.  In the very early days of Git, the objects
> were named after SHA-1 of deflated loose object representation,
> which meant that tweak in zlib or change of compression level would
> give the same object different names X-<.  This program was to
> convert an ancient history with these objects and rewrite them to
> match the new object naming scheme where the name comes from a hash
> of the inflated representation.

ok, in case I reroll again, I'll fixup the message.

>
>> By now the need for conversion of the very early repositories is less
>> relevant, we no longer need to keep it in contrib; remove it.
>
> I am not sure if removal of it matters, and I suspect that we saw no
> reaction from anybody because nobody thought it deserves the
> brain-cycle to decide whether to remove it.  I dunno.

I do think removing this would improve contrib/, not just because
it would better align with contribs mission statement in its README, but
also for other reasons. Why would a user look into contrib/ at all?
* to find interesting contemporary bits and pieces
* if they want to find old stuff for educational purposes, they ought to
  be looking into contrib/examples instead.

So maybe instead of this patch, just move it to the examples section?
(That way we archive the same goal: a cleaner, fresher contrib/
that doesn't look as stale)

Thanks,
Stefan

Re: [RFC for GIT] pull-request: add praise to people doing QA

[Wolfram Sang]

> > I didn't know about trailers before. As I undestand it, I could use
> > "Tested-by" as the key, and the commit subject as the value. This list
> > then could be parsed and brought into proper output shape. It would
> > simplify the subject parsing, but most things my AWK script currently
> > does would still need to stay or to be reimplemented (extracting names
> > from tags, creating arrays of tags given by $name). Am I correct?
> 
> That is not exactly what I had in mind.  I was wondering if we can
> do without any external script, implementing the logic you added
> inside shortlog with an extra option that triggers the whole thing,
> which may call into the same trailers API as used by the
> interpret-trailers command to do the parsing and picking out parts.

Sorry for being unclear. That's what I meant with "or to be
reimplemented". I should have added "in C".

I am afraid this also requires more time than I am willing to
spend on this issue. Seems my hack is going to stay for a while here.

However, thank you for your time and assisting me with pointers!



signature.asc
Description: PGP signature

Re: Git: new feature suggestion

[Stefan Beller]

On Thu, Jan 19, 2017 at 1:51 PM, Joao Pinto  wrote:
> Às 7:16 PM de 1/19/2017, Linus Torvalds escreveu:
>> On Thu, Jan 19, 2017 at 10:54 AM, Joao Pinto  wrote:
>>>
>>> I am currently facing some challenges in one of Linux subsystems where a 
>>> rename
>>> of a set of folders and files would be the perfect scenario for future
>>> development, but the suggestion is not accepted, not because it's not 
>>> correct,
>>> but because it makes the maintainer life harder in backporting bug fixes 
>>> and new
>>> features to older kernel versions and because it is not easy to follow the
>>> renamed file/folder history from the kernel.org history logs.
>>
>> Honestly, that's less of a git issue, and more of a "patch will not
>> apply across versions" issue.
>>
>> No amount of rename detection will ever fix that, simply because the
>> rename hadn't even _happened_ in the old versions that things get
>> backported to.
>>
>> ("git cherry-pick" can do a merge resolution and thus do "backwards"
>> renaming too, so tooling can definitely help, but it still ends up
>> meaning that even trivial patches are no longer the _same_ trivial
>> patch across versions).
>>
>> So renaming things increases maintainer workloads in those situations
>> regardless of any tooling issues.
>>
>> (You may also be referring to the mellanox mess, where this issue is
>> very much exacerbated by having different groups working on the same
>> thing, and maintainers having very much a "I will not take _anything_
>> from any of the groups that makes my life more complicated" model,
>> because those groups fucked up so much in the past).
>>
>> In other words, quite often issues are about workflows rather than
>> tools. The networking layer probably has more of this, because David
>> actually does the backports himself, so he _really_ doesn't want to
>> complicate things.
>
> I totally understand David' side! Synopsys is a well-known IP Vendor, and for 
> a
> long time its focus was the IP only. Knowadays the strategy has changed and
> Synopsys is very keen to help in Open Source, namelly Linux, developing the
> drivers for new IP Cores and participating in the improvement of existing 
> ones.
> I am part of the team that has that job.
>
> In USB and PCI subystems developers created common Synopsys drivers (focused 
> on
> the HW IP) and so today they are massively used by all the SoC that use 
> Synopsys
> IP.
>
> In the network subsystem, there are some drivers that target the same IP but
> were made by different companies. stmmac is an excelent driver for Synopsys 
> MAC
> 10/100/1000/QOS IPs, but there was another driver made by AXIS driver that 
> also
> targeted the QOS IP. We detected that issue and merged the AXIS specific 
> driver
> ops to stmmac, and nowadays, AXIS uses stmmac. So less drivers to maintain!
>
> The idea that was rejected consisted of renaming stmicro/stmmac to dwc/stmmac
> and to have dwc (designware controllers) as the official driver spot for
> Synopsys Ethernet IPs.
> There is another example of duplication, which is AMD' and Samsung' XGMAC
> driver, targeting the same Synopsys XGMAC IP.
>
> I am giving this examples because although the refactor adds work for
> backporting, it reduces the maintenance since we would have less duplicated
> drivers as we have today.

This sounds as if the code in question would only receive backports
for a specific
time (determined by HW lifecycle, maintenance life cycle and such).

So I wonder if this could be solved by not just renaming but
additionally adding a
symbolic link, such that the files in question seem to appear twice on
the file system.
Then backports ought to be applicable (hoping git-am doesn't choke on symlinks),
and after a while once the there no backports any more (due to life
cycle reasons),
remove the link?

This also sounds like a kind of problem, that others have run into before,
how did they solve it?

Thanks,
Stefan

>
> Thanks,
> Joao
>
>
>>Linus
>>
>

Re: merge maintaining history

[Philip Oakley]

From: "David J. Bakeman" 

On 01/14/2017 10:24 PM, Jacob Keller wrote:
On Fri, Jan 13, 2017 at 6:01 PM, David J. Bakeman  
wrote:

History

git cloned a remote repository and made many changes pushing them all to
said repository over many months.

The powers that be then required me to move project to new repository
server did so by pushing local version to new remote saving all history!

Now have to merge back to original repository(which has undergone many
changes since I split off) but how do I do that without loosing the
history of all the commits since the original move?  Note I need to push
changes to files that are already in existence.  I found on the web a
bunch of ways to insert a whole new directory structure into an existing
repository but as I said I need to do it on top of existing files.  Of
course I can copy all the files from my local working repository to the
cloned remote repository and commit any changes but I loose all the
history that way.

Thanks.

If I understand it.. you have two remotes now:

The "origin" remote, which was the original remote you started with.

You have now a "new" remote which you created and pushed to.

So you want to merge the "new" history into the original tree now, so
you checkout the original tree, then "git merge /"
and then fix up any conflicts, and then git commit to create a merge
commit that has the new history. Then you could push that to both
trees.

I would want a bit more information about your setup before providing
actual commands.

Thanks I think that's close but it's a little more complicated I think
:<(  I don't know if this diagram will work but lets try.

original A->B->C->D->E->F
\
first branch  b->c->d->e

new repo e->f->g->h

Now I need to merge h to F without loosing b through h hopefully.  Yes e
was never merged back to the original repo and it's essentially gone now
so I can't just merge to F or can I?


a quick bikeshed..

You have both repositories, so nothing is lost.

Do note that 'e' commit sha1 in the first branch will be different from the 
sha1 of 'e' commit in the new repo, but both should have the same top level 
tree if they are truly identical.


You can fetch both repositories into a single common repo so you will have 
an --orphan branch of the new repo.


Consider adding a --allow-empty commit e' to the original repo to note what 
happens next (and how to do it), which is to use the replace mechanism to 
bridge the gap between the e of the old repo and the e of the new repo by 
making e of the new repo replace the e' you just created.


This will make it look like 'h' is the natural development of 'a'. Even if e 
& e were not tree-same, you will be able to see a diff. You can now merge 
'h' onto 'F' in whatever way you find appropriate to give the right view of 
the development.


When you push this back upstream, note that the 'replace' is local, so 
upstream sees a gap, but that commit e' has the instructions to rebuild the 
link, should others require it (you may have to push the e' commit 
separately).


--
Philip 

Re: Git: new feature suggestion

[Joao Pinto]

Às 7:16 PM de 1/19/2017, Linus Torvalds escreveu:
> On Thu, Jan 19, 2017 at 10:54 AM, Joao Pinto  wrote:
>>
>> I am currently facing some challenges in one of Linux subsystems where a 
>> rename
>> of a set of folders and files would be the perfect scenario for future
>> development, but the suggestion is not accepted, not because it's not 
>> correct,
>> but because it makes the maintainer life harder in backporting bug fixes and 
>> new
>> features to older kernel versions and because it is not easy to follow the
>> renamed file/folder history from the kernel.org history logs.
> 
> Honestly, that's less of a git issue, and more of a "patch will not
> apply across versions" issue.
> 
> No amount of rename detection will ever fix that, simply because the
> rename hadn't even _happened_ in the old versions that things get
> backported to.
> 
> ("git cherry-pick" can do a merge resolution and thus do "backwards"
> renaming too, so tooling can definitely help, but it still ends up
> meaning that even trivial patches are no longer the _same_ trivial
> patch across versions).
> 
> So renaming things increases maintainer workloads in those situations
> regardless of any tooling issues.
> 
> (You may also be referring to the mellanox mess, where this issue is
> very much exacerbated by having different groups working on the same
> thing, and maintainers having very much a "I will not take _anything_
> from any of the groups that makes my life more complicated" model,
> because those groups fucked up so much in the past).
> 
> In other words, quite often issues are about workflows rather than
> tools. The networking layer probably has more of this, because David
> actually does the backports himself, so he _really_ doesn't want to
> complicate things.

I totally understand David' side! Synopsys is a well-known IP Vendor, and for a
long time its focus was the IP only. Knowadays the strategy has changed and
Synopsys is very keen to help in Open Source, namelly Linux, developing the
drivers for new IP Cores and participating in the improvement of existing ones.
I am part of the team that has that job.

In USB and PCI subystems developers created common Synopsys drivers (focused on
the HW IP) and so today they are massively used by all the SoC that use Synopsys
IP.

In the network subsystem, there are some drivers that target the same IP but
were made by different companies. stmmac is an excelent driver for Synopsys MAC
10/100/1000/QOS IPs, but there was another driver made by AXIS driver that also
targeted the QOS IP. We detected that issue and merged the AXIS specific driver
ops to stmmac, and nowadays, AXIS uses stmmac. So less drivers to maintain!

The idea that was rejected consisted of renaming stmicro/stmmac to dwc/stmmac
and to have dwc (designware controllers) as the official driver spot for
Synopsys Ethernet IPs.
There is another example of duplication, which is AMD' and Samsung' XGMAC
driver, targeting the same Synopsys XGMAC IP.

I am giving this examples because although the refactor adds work for
backporting, it reduces the maintenance since we would have less duplicated
drivers as we have today.

Thanks,
Joao


>Linus
> 

Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Thu, Jan 19, 2017 at 01:45:29PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I'm trying to figure out why "fetch --multiple" wouldn't just take a url
> > in the first place. I guess it is because multiple fetch is useless
> > without refspecs (since otherwise you're just writing to FETCH_HEAD,
> > which gets immediately overwritten).
> 
> This is probably a tangent, if FETCH_HEAD is overwritten, wouldn't
> that be a bug in the implementation of --multiple?  I somehow
> thought we had an option to tell second and subsequent "fetch" to
> append to FETCH_HEAD instead of overwriting it.

Maybe. I was just speculating on the reason.

I just disabled that check and tried it with two local repos:

  git init
  git fetch --multiple /path/to/one /path/to/two
  cat .git/FETCH_HEAD

and indeed it does seem to append.

The logic comes from 9c4a036b3 (Teach the --all option to 'git fetch',
2009-11-09), but it does not seem to give any reasoning. Nor could I
find anything on the mailing list. 

-Peff

Re: Git: new feature suggestion

[Jakub Narębski]

W dniu 19.01.2017 o 19:39, Linus Torvalds pisze:
> On Wed, Jan 18, 2017 at 10:33 PM, Konstantin Khomoutov
>  wrote:
>>
>> Still, I welcome you to read the sort-of "reference" post by Linus
>> Torvalds [1] in which he explains the reasoning behind this approach
>> implemented in Git.
> 
> It's worth noting that that discussion was from some _very_ early days
> in git (one week into the whole thing), when none of those
> visualization tools were actually implemented.
> 
> Even now, ten years after the fact, plain git doesn't actually do what
> I outlined. Yes, "git blame -Cw" works fairly well, and is in general
> better than the traditional per-file "annotate". And yes, "git log
> --follow" does another (small) part of the outlined thing, but is
> really not very powerful.

It is really a pity that "git log --follow" is so limited; it's
development stopped at early 'good enough' implementation.

For example "git log --follow gitweb/gitweb.perl" would not show
the whole history of a file (which was once independent project),
and "git log --follow" doesn't work for directories or multiple
files.

> 
> Some tools on top of git do more, but I think in general this is an
> area that could easily be improved upon. For example, the whole
> iterative and interactive drilling down in history of a particular
> file is very inconvenient to do with "git blame" (you find a commit
> that change the area in some way that you don't find interesting, so
> then you have to restart git blame with the parent of that
> unintersting commit).
> 
> You can do it in tig, but I suspect a more graphical tool might be better.

Well, we do have "git gui blame".

[...]
-- 
Jakub Narębski

Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Junio C Hamano]

Jeff King  writes:

> I'm trying to figure out why "fetch --multiple" wouldn't just take a url
> in the first place. I guess it is because multiple fetch is useless
> without refspecs (since otherwise you're just writing to FETCH_HEAD,
> which gets immediately overwritten).

This is probably a tangent, if FETCH_HEAD is overwritten, wouldn't
that be a bug in the implementation of --multiple?  I somehow
thought we had an option to tell second and subsequent "fetch" to
append to FETCH_HEAD instead of overwriting it.

Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

Hi Peff,

On Thu, 19 Jan 2017, Jeff King wrote:

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c85f3471d..9024cfffa 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1014,9 +1014,9 @@ static int add_remote_or_group(const char *name, struct 
> string_list *list)
>   git_config(get_remote_group, );
>   if (list->nr == prev_nr) {
>   struct remote *remote;
> - if (!remote_is_configured(name))
> - return 0;
>   remote = remote_get(name);
> + if (!remote->fetch_refspec_nr)
> + return 0;

Please note that it is legitimate to fetch from foreign vcs, in which case
the fetch refspecs may not be required (or even make sense).

> It's outside the scope of your patches, so I think we are OK to just
> ignore it. But if you want to pursue it, it avoids having to add the
> extra parameter to remote_is_configured().

Sure, it would avoid that. But that parameter makes semantic sense: some
callers may want to have all remotes, even those configured via the
command-line, and others really are only interested in knowing whether the
current repository config already has settings for a remote of the given
name.

> > Many thanks to Jeff King whose tireless review helped with settling for
> > nothing less than the current strategy.
> 
> Just how I wanted to be immortalized in git's commit history. ;)

You are welcome ;-)

Ciao,
Johannes

Re: merge maintaining history

[Junio C Hamano]

"David J. Bakeman"  writes:

>> So you want to merge the "new" history into the original tree now, so
>> you checkout the original tree, then "git merge /"
>> and then fix up any conflicts, and then git commit to create a merge
>> commit that has the new history. Then you could push that to both
>> trees.
>>
>> I would want a bit more information about your setup before providing
>> actual commands.
>
> Thanks I think that's close but it's a little more complicated I think
> :<(  I don't know if this diagram will work but lets try.
>
> original A->B->C->D->E->F
>  \
> first branch  b->c->d->e
>
> new repo e->f->g->h
>
> Now I need to merge h to F without loosing b through h hopefully.  Yes e
> was never merged back to the original repo and it's essentially gone now
> so I can't just merge to F or can I?

With the picture, I think you mean 'b' is forked from 'B' and the
first branch built 3 more commits on top, leading to 'e'.

You say "new repo" has 'e' thru 'h', and I take it to mean you
started developing on top of the history that leads to 'e' you built
in the first branch, and "new repo" has the resulting history that
leads to 'h'.

Unless you did something exotic and non-standard, commit 'e' in "new
repo" would be exactly the same as 'e' sitting on the tip of the
"first branch", so the picture would be more like:

> original A->B->C->D->E->F
>  \
> first branch  b->c->d->e
> \
> new repo f->g->h

no?  Then merging 'h' into 'F' will pull everything you did since
you diverged from the history that leads to 'F', resulting in a
history of this shape:

> original A->B->C->D->E->F--M
>  \/
> first branch  b->c->d->e /
> \   /
> new repo f->g->h

If on the other hand you did something non-standard and exotic to
rewrite 'e' at the end of "first branch" and make a different commit
that does not even have any parent in "new repo", and the history of
"new repo" originates in such a commit that is not 'e', things will
become messy.  But I didn't think I read you did anything unusual so
a simple "git checkout F && git merge h" should give you what you
want.

[PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

One of the really nice features of the ~/.gitconfig file is that users
can override defaults by their own preferred settings for all of their
repositories.

One such default that some users like to override is whether the
"origin" remote gets auto-pruned or not. The user would simply call

git config --global remote.origin.prune true

and from now on all "origin" remotes would be pruned automatically when
fetching into the local repository.

There is just one catch: now Git thinks that the "origin" remote is
configured, even if the repository config has no [remote "origin"]
section at all, as it does not realize that the "prune" setting was
configured globally and that there really is no "origin" remote
configured in this repository.

That is a problem e.g. when renaming a remote to a new name, when Git
may be fooled into thinking that there is already a remote of that new
name.

Let's fix this by paying more attention to *where* the remote settings
came from: if they are configured in the local repository config, we
must not overwrite them. If they were configured elsewhere, we cannot
overwrite them to begin with, as we only write the repository config.

There is only one caller of remote_is_configured() (in `git fetch`) that
may want to take remotes into account even if they were configured
outside the repository config; all other callers essentially try to
prevent the Git command from overwriting settings in the repository
config.

To accommodate that fact, the remote_is_configured() function now
requires a parameter that states whether the caller is interested in all
remotes, or only in those that were configured in the repository config.

Many thanks to Jeff King whose tireless review helped with settling for
nothing less than the current strategy.

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

Signed-off-by: Johannes Schindelin 
---
 builtin/fetch.c   |  2 +-
 builtin/remote.c  | 14 +++---
 remote.c  | 12 ++--
 remote.h  |  4 ++--
 t/t5505-remote.sh |  2 +-
 5 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f1570e3464..b5ad09d046 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1177,7 +1177,7 @@ static int add_remote_or_group(const char *name, struct 
string_list *list)
git_config(get_remote_group, );
if (list->nr == prev_nr) {
struct remote *remote = remote_get(name);
-   if (!remote_is_configured(remote))
+   if (!remote_is_configured(remote, 0))
return 0;
string_list_append(list, remote->name);
}
diff --git a/builtin/remote.c b/builtin/remote.c
index e52cf3925b..5339ed6ad1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -186,7 +186,7 @@ static int add(int argc, const char **argv)
url = argv[1];
 
remote = remote_get(name);
-   if (remote_is_configured(remote))
+   if (remote_is_configured(remote, 1))
die(_("remote %s already exists."), name);
 
strbuf_addf(, "refs/heads/test:refs/remotes/%s/test", name);
@@ -618,14 +618,14 @@ static int mv(int argc, const char **argv)
rename.remote_branches = _branches;
 
oldremote = remote_get(rename.old);
-   if (!remote_is_configured(oldremote))
+   if (!remote_is_configured(oldremote, 1))
die(_("No such remote: %s"), rename.old);
 
if (!strcmp(rename.old, rename.new) && oldremote->origin != 
REMOTE_CONFIG)
return migrate_file(oldremote);
 
newremote = remote_get(rename.new);
-   if (remote_is_configured(newremote))
+   if (remote_is_configured(newremote, 1))
die(_("remote %s already exists."), rename.new);
 
strbuf_addf(, "refs/heads/test:refs/remotes/%s/test", rename.new);
@@ -753,7 +753,7 @@ static int rm(int argc, const char **argv)
usage_with_options(builtin_remote_rm_usage, options);
 
remote = remote_get(argv[1]);
-   if (!remote_is_configured(remote))
+   if (!remote_is_configured(remote, 1))
die(_("No such remote: %s"), argv[1]);
 
known_remotes.to_delete = remote;
@@ -1415,7 +1415,7 @@ static int set_remote_branches(const char *remotename, 
const char **branches,
strbuf_addf(, "remote.%s.fetch", remotename);
 
remote = remote_get(remotename);
-   if (!remote_is_configured(remote))
+   if (!remote_is_configured(remote, 1))
die(_("No such remote '%s'"), remotename);
 
if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
@@ -1469,7 +1469,7 @@ static int get_url(int argc, const char **argv)
remotename = argv[0];
 
remote = remote_get(remotename);
-   if (!remote_is_configured(remote))
+   if (!remote_is_configured(remote, 1))
die(_("No such remote '%s'"), remotename);
 
url_nr = 0;
@@ 

Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Thu, Jan 19, 2017 at 10:20:02PM +0100, Johannes Schindelin wrote:

> There is only one caller of remote_is_configured() (in `git fetch`) that
> may want to take remotes into account even if they were configured
> outside the repository config; all other callers essentially try to
> prevent the Git command from overwriting settings in the repository
> config.
> 
> To accommodate that fact, the remote_is_configured() function now
> requires a parameter that states whether the caller is interested in all
> remotes, or only in those that were configured in the repository config.

Just to make sure I understand the issue, the problem is that:

  git config --global remote.foo.url whatever
  git fetch --multiple foo bar

would not work without this part of the patch?

I'm trying to figure out why "fetch --multiple" wouldn't just take a url
in the first place. I guess it is because multiple fetch is useless
without refspecs (since otherwise you're just writing to FETCH_HEAD,
which gets immediately overwritten). I wonder if that test really should
be:

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c85f3471d..9024cfffa 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1014,9 +1014,9 @@ static int add_remote_or_group(const char *name, struct 
string_list *list)
git_config(get_remote_group, );
if (list->nr == prev_nr) {
struct remote *remote;
-   if (!remote_is_configured(name))
-   return 0;
remote = remote_get(name);
+   if (!remote->fetch_refspec_nr)
+   return 0;
string_list_append(list, remote->name);
}
return 1;

It's outside the scope of your patches, so I think we are OK to just
ignore it. But if you want to pursue it, it avoids having to add the
extra parameter to remote_is_configured().

> Many thanks to Jeff King whose tireless review helped with settling for
> nothing less than the current strategy.

Just how I wanted to be immortalized in git's commit history. ;)

>  builtin/fetch.c   |  2 +-
>  builtin/remote.c  | 14 +++---
>  remote.c  | 12 ++--
>  remote.h  |  4 ++--
>  t/t5505-remote.sh |  2 +-
>  5 files changed, 21 insertions(+), 13 deletions(-)

Patch itself looks OK to me.

-Peff

Re: [RFC] stash --continue

[Johannes Schindelin]

Hi Marc,

On Thu, 19 Jan 2017, Marc Branchaud wrote:

> On 2017-01-19 10:49 AM, Johannes Schindelin wrote:
> >
> > On Wed, 18 Jan 2017, Marc Branchaud wrote:
> >
> > > On 2017-01-18 11:34 AM, Johannes Schindelin wrote:
> > > >
> > > > On Wed, 18 Jan 2017, Marc Branchaud wrote:
> > > >
> > > > > On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
> > > > >
> > > > > > On Mon, 16 Jan 2017, Stephan Beyer wrote:
> > > > > >
> > > > > > > a git-newbie-ish co-worker uses git-stash sometimes. Last
> > > > > > > time he used "git stash pop", he got into a merge conflict.
> > > > > > > After he resolved the conflict, he did not know what to do
> > > > > > > to get the repository into the wanted state. In his case, it
> > > > > > > was only "git add " followed by a "git
> > > > > > > reset" and a "git stash drop", but there may be more
> > > > > > > involved cases when your index is not clean before "git
> > > > > > > stash pop" and you want to have your index as before.
> > > > > > >
> > > > > > > This led to the idea to have something like "git stash
> > > > > > > --continue"[1]
> > > > > >
> > > > > > More like "git stash pop --continue". Without the "pop"
> > > > > > command, it does not make too much sense.
> > > > >
> > > > > Why not?  git should be able to remember what stash command
> > > > > created the conflict.  Why should I have to?  Maybe the fire
> > > > > alarm goes off right when I run the stash command, and by the
> > > > > time I get back to it I can't remember which operation I did.
> > > > > It would be nice to be able to tell git to "just finish off (or
> > > > > abort) the stash operation, whatever it was".
> > > >
> > > > That reeks of a big potential for confusion.
> > > >
> > > > Imagine for example a total Git noob who calls `git stash list`,
> > > > scrolls two pages down, then hits `q` by mistake. How would you
> > > > explain to that user that `git stash --continue` does not continue
> > > > showing the list at the third page?
> > >
> > > Sorry, but I have trouble taking that example seriously.  It assumes
> > > such a level of "noobness" that the user doesn't even understand how
> > > standard command output paging works, not just with git but with any
> > > shell command.
> >
> > Yeah, well, I thought you understood what I meant.
> >
> > The example was the best I could come up with quickly, and it only
> > tried to show that there are *other* stash operations that one might
> > perceive to happen at the same time as the "pop" operation, so your
> > flimsical comment "why not continue the latest operation" may very
> > well be ambiguous.
> >
> > And if it is not ambiguous in "stash", it certainly will be in other
> > Git operations. And therefore, having a DWIM in "stash" to allow
> > "--continue" without any specific subcommand, but not having it in
> > other Git commands, is just a very poor user interface design. It is
> > prone to confuse users, which is always a hallmark of a bad user
> > interface.
> 
> Please don't underestimate the power of syntactic consistency in helping
> users achieve their goals.  Having some commands use "git foo
> --continue" while others use "git foo bar --continue" *will* confuse
> people, regardless of how logical the reasons for those differences.

But that ship has already sailed!

By your reasoning, it was a mistake to introduce subcommands such as `git
stash pop` in the first place.

> But in the case of stash, I still don't see the utility in having
> operation-specific continuation.

Stephan already gave one good example where you want it: if `git stash
pop` fails, you may want to continue by *not* dropping the stash via `git
stash apply --continue`.

> Consider the following sequence (as you say, this doesn't work yet, but
> making it work seems reasonable):
> 
>   git stash pop  # creates some conflicts
>   git stash apply stash@{4} # creates some other conflicts
>   # (User resolves the conflicts created by the pop.)
>   git stash pop --continue

Yes, that would make sense: the `pop` would actually drop the stash entry.

> Given the description of the original proposal (do "git reset; git stash
> drop"), what's the state of the index and the working tree?
> 
> In particular, what has the user gained by continuing just that pop?

That it was completed.

> Another thing to ask is, how common is such a scenario likely to be?

We have millions of users. Even a 0.001% chance of anything happening will
bite users, who will then come back to bite us.

Let's just not go into the "how likely is this" game.

> I suggest that it will be far more common for users to resolve all the
> conflicts and then want to continue all their interrupted stash
> operations.  If so, fussily forcing them to explicitly continue the pop
> and the apply is just a waste.

No, it is not a waste, as we require the user nothing else than to be
precise. Do you want to continue the "stash pop"? Okay, then, call "git
stash pop --continue".

> [... a lot of stuff 

[PATCH v2 0/2] Fix remote_is_configured()

[Johannes Schindelin]

A surprising behavior triggered the bug report in
https://github.com/git-for-windows/git/issues/888: the mere existence of
the config setting "remote.origin.prune" (in this instance, configured
via ~/.gitconfig so that it applies to all repositories) fooled `git
remote rename  ` into believing that the  remote
is already there.

This patch pair demonstrates the problem, and then fixes it (along with
potential similar problems, such as setting an HTTP proxy for remotes of
a given name via ~/.gitconfig).

Changes since v1:

 - overhauled the strategy to fix the problem: instead of looking at the
   config key to determine whether it configures a remote or not, we now
   look at the config_source: if the remote setting was configured in
   .git/config, `git remote ` must not overwrite it. If it was
   configured elsewhere, `git remote` cannot overwrite any setting, as
   it only touches .git/config.


Johannes Schindelin (2):
  remote rename: demonstrate a bogus "remote exists" bug
  Be more careful when determining whether a remote was configured

 builtin/fetch.c   |  2 +-
 builtin/remote.c  | 14 +++---
 remote.c  | 12 ++--
 remote.h  |  4 ++--
 t/t5505-remote.sh |  7 +++
 5 files changed, 27 insertions(+), 12 deletions(-)


base-commit: ffac48d093d4b518a0cc0e8bf1b7cb53e0c3d7a2
Published-As: https://github.com/dscho/git/releases/tag/rename-remote-v2
Fetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v2

Interdiff vs v1:

 diff --git a/builtin/fetch.c b/builtin/fetch.c
 index f1570e3464..b5ad09d046 100644
 --- a/builtin/fetch.c
 +++ b/builtin/fetch.c
 @@ -1177,7 +1177,7 @@ static int add_remote_or_group(const char *name, struct 
string_list *list)
git_config(get_remote_group, );
if (list->nr == prev_nr) {
struct remote *remote = remote_get(name);
 -  if (!remote_is_configured(remote))
 +  if (!remote_is_configured(remote, 0))
return 0;
string_list_append(list, remote->name);
}
 diff --git a/builtin/remote.c b/builtin/remote.c
 index e52cf3925b..5339ed6ad1 100644
 --- a/builtin/remote.c
 +++ b/builtin/remote.c
 @@ -186,7 +186,7 @@ static int add(int argc, const char **argv)
url = argv[1];
  
remote = remote_get(name);
 -  if (remote_is_configured(remote))
 +  if (remote_is_configured(remote, 1))
die(_("remote %s already exists."), name);
  
strbuf_addf(, "refs/heads/test:refs/remotes/%s/test", name);
 @@ -618,14 +618,14 @@ static int mv(int argc, const char **argv)
rename.remote_branches = _branches;
  
oldremote = remote_get(rename.old);
 -  if (!remote_is_configured(oldremote))
 +  if (!remote_is_configured(oldremote, 1))
die(_("No such remote: %s"), rename.old);
  
if (!strcmp(rename.old, rename.new) && oldremote->origin != 
REMOTE_CONFIG)
return migrate_file(oldremote);
  
newremote = remote_get(rename.new);
 -  if (remote_is_configured(newremote))
 +  if (remote_is_configured(newremote, 1))
die(_("remote %s already exists."), rename.new);
  
strbuf_addf(, "refs/heads/test:refs/remotes/%s/test", rename.new);
 @@ -753,7 +753,7 @@ static int rm(int argc, const char **argv)
usage_with_options(builtin_remote_rm_usage, options);
  
remote = remote_get(argv[1]);
 -  if (!remote_is_configured(remote))
 +  if (!remote_is_configured(remote, 1))
die(_("No such remote: %s"), argv[1]);
  
known_remotes.to_delete = remote;
 @@ -1415,7 +1415,7 @@ static int set_remote_branches(const char *remotename, 
const char **branches,
strbuf_addf(, "remote.%s.fetch", remotename);
  
remote = remote_get(remotename);
 -  if (!remote_is_configured(remote))
 +  if (!remote_is_configured(remote, 1))
die(_("No such remote '%s'"), remotename);
  
if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
 @@ -1469,7 +1469,7 @@ static int get_url(int argc, const char **argv)
remotename = argv[0];
  
remote = remote_get(remotename);
 -  if (!remote_is_configured(remote))
 +  if (!remote_is_configured(remote, 1))
die(_("No such remote '%s'"), remotename);
  
url_nr = 0;
 @@ -1537,7 +1537,7 @@ static int set_url(int argc, const char **argv)
oldurl = newurl;
  
remote = remote_get(remotename);
 -  if (!remote_is_configured(remote))
 +  if (!remote_is_configured(remote, 1))
die(_("No such remote '%s'"), remotename);
  
if (push_mode) {
 diff --git a/remote.c b/remote.c
 index 298f2f93fa..8524135de4 100644
 --- a/remote.c
 +++ b/remote.c
 @@ -255,7 +255,7 @@ static void read_remotes_file(struct remote *remote)
  
if (!f)
return;
 -  remote->configured = 1;
 +  remote->configured_in_repo = 1;
  

Re: [RFC for GIT] pull-request: add praise to people doing QA

[Jeff King]

On Thu, Jan 19, 2017 at 09:43:45PM +0100, Wolfram Sang wrote:

> > As to the implementation, I am wondering if we can make this somehow
> > work well with the "trailers" code we already have, instead of
> > inventing yet another parser of trailers.  
> > 
> > In its current shape, "interpret-trailers" focuses on "editing" an
> > existing commit log message to tweak the trailer lines.  That mode
> > of operation would help amending and rebasing, and to do that it
> > needs to parse the commit log message, identify trailer blocks,
> > parse out each trailer lines, etc.  
> > 
> > There is no fundamental reason why its output must be an edited
> > original commit log message---it should be usable as a filter that
> > picks trailer lines of the selected trailer type, like "Tested-By",
> > etc.
> 
> I didn't know about trailers before. As I undestand it, I could use
> "Tested-by" as the key, and the commit subject as the value. This list
> then could be parsed and brought into proper output shape. It would
> simplify the subject parsing, but most things my AWK script currently
> does would still need to stay or to be reimplemented (extracting names
> from tags, creating arrays of tags given by $name). Am I correct?
> 
> All under the assumption that trailers work on a range of commits. I
> have to admit that adding this to git is beyond my scope.

This sounds a lot like the shortlog-trailers work I did about a year
ago:

  http://public-inbox.org/git/20151229073832.gn8...@sigill.intra.peff.net/

  http://public-inbox.org/git/20151229075013.ga9...@sigill.intra.peff.net/

Nobody seemed to really find it useful, so I didn't pursue it.

Some of the preparatory patches in that series bit-rotted in the
meantime, but you can play with a version based on v2.7.0 by fetching
the "shortlog-trailers-historical" branch from
https://github.com/peff/git.git.

And then things like:

  git shortlog --ident=tested-by --format='...tested a patch by %an'

work (and you can put whatever commit items you want into the --format,
including just dumping the hash if you want to do more analysis).

-Peff

Re: [RFC for GIT] pull-request: add praise to people doing QA

[Junio C Hamano]

Wolfram Sang  writes:

> I didn't know about trailers before. As I undestand it, I could use
> "Tested-by" as the key, and the commit subject as the value. This list
> then could be parsed and brought into proper output shape. It would
> simplify the subject parsing, but most things my AWK script currently
> does would still need to stay or to be reimplemented (extracting names
> from tags, creating arrays of tags given by $name). Am I correct?

That is not exactly what I had in mind.  I was wondering if we can
do without any external script, implementing the logic you added
inside shortlog with an extra option that triggers the whole thing,
which may call into the same trailers API as used by the
interpret-trailers command to do the parsing and picking out parts.

[PATCH v2 1/2] remote rename: demonstrate a bogus "remote exists" bug

[Johannes Schindelin]

Some users like to set `remote.origin.prune = true` in their ~/.gitconfig
so that all of their repositories use that default.

However, our code is ill-prepared for this, mistaking that single entry to
mean that there is already a remote of the name "origin", even if there is
not.

This patch adds a test case demonstrating this issue.

Reported by Andrew Arnott.

Signed-off-by: Johannes Schindelin 
---
 t/t5505-remote.sh | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8198d8eb05..2c03f44c85 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -764,6 +764,13 @@ test_expect_success 'rename a remote with name prefix of 
other remote' '
)
 '
 
+test_expect_failure 'rename succeeds with existing remote..prune' '
+   git clone one four.four &&
+   test_when_finished git config --global --unset remote.upstream.prune &&
+   git config --global remote.upstream.prune true &&
+   git -C four.four remote rename origin upstream
+'
+
 cat >remotes_origin <

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

Hi Peff,

On Thu, 19 Jan 2017, Jeff King wrote:

> diff --git a/remote.c b/remote.c
> index 298f2f93f..720d616be 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char 
> *value, void *cb)
>   }
>   remote = make_remote(name, namelen);
>   remote->origin = REMOTE_CONFIG;
> + if (current_config_scope() == CONFIG_SCOPE_REPO)
> + remote->configured = 1;
>   if (!strcmp(subkey, "mirror"))
>   remote->mirror = git_config_bool(key, value);
>   else if (!strcmp(subkey, "skipdefaultupdate"))
> 
> That doesn't make your test pass, but I think that is only because your
> test is not covering the interesting case (it puts the new config in the
> repo config, not in ~/.gitconfig).
> 
> What do you think?

Heh. After skimming the first three paragraphs of your mail, I had a
similar idea and implemented it. It works great!

v2 coming right up,
Johannes

Re: [PATCH v4 3/5] name-rev: add support to exclude refs by pattern match

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> Extend git-name-rev to support excluding refs which match shell patterns
> using --exclude. These patterns can be used to limit the scope of refs
> by excluding any ref that matches one of the --exclude patterns. A ref
> will only be used for naming when it matches at least one --ref pattern

s/--ref pattern/--refs pattern/

> but does not match any of the --exclude patterns. Thus, --exlude

s/--exlude/--exclude/

> patterns are given precedence over --ref patterns.

s/--ref pattern/--refs pattern/

No need to resend.  Thanks.

Re: [PATCH v4 2/5] name-rev: extend --refs to accept multiple patterns

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> Teach git name-rev to take multiple --refs stored as a string list of
> patterns. The list of patterns will be matched inclusively, and each ref
> only needs to match one pattern to be included. A ref will only be
> excluded if it does not match any of the given patterns. Additionally,
> if any of the patterns would allow abbreviation, then we will abbreviate
> the ref, even if another pattern is more strict and would not have
> allowed abbreviation on its own.
>
> Add tests and documentation for this change. The tests expected output
> is dynamically generated, but this is in order to avoid hard-coding
> a commit object id in the test results (as the expected output is to
> simply leave the commit object unnamed).

Makes sense.  

I do not see anything that requires "... generated, but" there,
though, as if it is a bad thing to do to prepare expected output
dynamically.  I'd just reword "generated.  This is..." to make it
neutral.

Re: [PATCH v4 2/5] name-rev: extend --refs to accept multiple patterns

[Junio C Hamano]

Jacob Keller  writes:

> + if (data->ref_filters.nr) {
> + struct string_list_item *item;
> + int matched = 0;
> +
> + /* See if any of the patterns match. */
> + for_each_string_list_item(item, >ref_filters) {
> + /* Check every pattern even after we found a match so
> +  * that we can determine when we should abbreviate the
> +  * output. We will abbreviate the output when any of
> +  * the patterns match a subpath, even if one of the
> +  * patterns matches fully.
> +  */

This describe "what" we do, which we can read from the code.  What I
asked you to mention was "why", which cannot be read from the code.

/*
 * Check all patterns even after finding a match, 
 * so that we can see if a match with a subpath exists.
 * When a user asked for 'refs/tags/v*' and 'v1.*', both
 * of which match, the user is showing her willingness
 * to accept a shortened output by having the 'v1.*' in
 * the acceptable refnames, so we shouldn't stop when seeing
 * 'refs/tags/v1.4' matches 'refs/tags/v*'.  We should show
 * it as 'v1.4'.
 */

or something like that, perhaps?

Re: [RESEND PATCHv2] contrib: remove git-convert-objects

[Junio C Hamano]

Stefan Beller  writes:

> git-convert-objects, originally named git-convert-cache was used in
> early 2005 to convert to a new repository format, e.g. adding an author
> date.

I think this description is not wrong per-se but misses the much
more important point.  In the very early days of Git, the objects
were named after SHA-1 of deflated loose object representation,
which meant that tweak in zlib or change of compression level would
give the same object different names X-<.  This program was to
convert an ancient history with these objects and rewrite them to
match the new object naming scheme where the name comes from a hash
of the inflated representation.

> By now the need for conversion of the very early repositories is less
> relevant, we no longer need to keep it in contrib; remove it.

I am not sure if removal of it matters, and I suspect that we saw no
reaction from anybody because nobody thought it deserves the
brain-cycle to decide whether to remove it.  I dunno.

Re: [RFC for GIT] pull-request: add praise to people doing QA

[Wolfram Sang]

> So the idea is to have list of those whose names appear on
> Reviewed-by: and Tested-by: collected and listed after the list of
> commit titles and author names.  I personally do not see much
> downsides in doing so, but I do not consume that many PRs myself, so
> let's hear from those who actually do process many of them.

Sadly, no further responses so far. Let's see if they will come if I
keep posting my pull requests with that information attached.

> As to the implementation, I am wondering if we can make this somehow
> work well with the "trailers" code we already have, instead of
> inventing yet another parser of trailers.  
> 
> In its current shape, "interpret-trailers" focuses on "editing" an
> existing commit log message to tweak the trailer lines.  That mode
> of operation would help amending and rebasing, and to do that it
> needs to parse the commit log message, identify trailer blocks,
> parse out each trailer lines, etc.  
> 
> There is no fundamental reason why its output must be an edited
> original commit log message---it should be usable as a filter that
> picks trailer lines of the selected trailer type, like "Tested-By",
> etc.

I didn't know about trailers before. As I undestand it, I could use
"Tested-by" as the key, and the commit subject as the value. This list
then could be parsed and brought into proper output shape. It would
simplify the subject parsing, but most things my AWK script currently
does would still need to stay or to be reimplemented (extracting names
from tags, creating arrays of tags given by $name). Am I correct?

All under the assumption that trailers work on a range of commits. I
have to admit that adding this to git is beyond my scope.

Thanks for looking into it,

   Wolfram

[PATCH v6 3/3] Retire the scripted difftool

[Johannes Schindelin]

It served its purpose, but now we have a builtin difftool. Time for the
Perl script to enjoy Florida.

Signed-off-by: Johannes Schindelin 
---
 .gitignore |  1 -
 Makefile   |  1 -
 builtin/difftool.c | 41 --
 .../examples/git-difftool.perl |  0
 git.c  |  7 +-
 t/t7800-difftool.sh| 94 +++---
 6 files changed, 47 insertions(+), 97 deletions(-)
 rename git-legacy-difftool.perl => contrib/examples/git-difftool.perl (100%)

diff --git a/.gitignore b/.gitignore
index ae025b..6722f78f9a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,7 +76,6 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
-/git-legacy-difftool
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index 8cf5bef034..e9aa6ae57c 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,6 @@ SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
 SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-legacy-difftool.perl
 SCRIPT_PERL += git-archimport.perl
 SCRIPT_PERL += git-cvsexportcommit.perl
 SCRIPT_PERL += git-cvsimport.perl
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 2115e548a5..42ad9e804a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -616,30 +616,6 @@ static int run_file_diff(int prompt, const char *prefix,
exit(ret);
 }
 
-/*
- * NEEDSWORK: this function can go once the legacy-difftool Perl script is
- * retired.
- *
- * We intentionally avoid reading the config directly here, to avoid messing up
- * the GIT_* environment variables when we need to fall back to exec()ing the
- * Perl script.
- */
-static int use_builtin_difftool(void) {
-   struct child_process cp = CHILD_PROCESS_INIT;
-   struct strbuf out = STRBUF_INIT;
-   int ret;
-
-   argv_array_pushl(,
-"config", "--bool", "difftool.usebuiltin", NULL);
-   cp.git_cmd = 1;
-   if (capture_command(, , 6))
-   return 0;
-   strbuf_trim();
-   ret = !strcmp("true", out.buf);
-   strbuf_release();
-   return ret;
-}
-
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
@@ -671,23 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   /*
-* NEEDSWORK: Once the builtin difftool has been tested enough
-* and git-legacy-difftool.perl is retired to contrib/, this preamble
-* can be removed.
-*/
-   if (!use_builtin_difftool()) {
-   const char *path = mkpath("%s/git-legacy-difftool",
- git_exec_path());
-
-   if (sane_execvp(path, (char **)argv) < 0)
-   die_errno("could not exec %s", path);
-
-   return 0;
-   }
-   prefix = setup_git_directory();
-   trace_repo_setup(prefix);
-   setup_work_tree();
/* NEEDSWORK: once we no longer spawn anything, remove this */
setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
1);
diff --git a/git-legacy-difftool.perl b/contrib/examples/git-difftool.perl
similarity index 100%
rename from git-legacy-difftool.perl
rename to contrib/examples/git-difftool.perl
diff --git a/git.c b/git.c
index c58181e5ef..bd4d668a21 100644
--- a/git.c
+++ b/git.c
@@ -424,12 +424,7 @@ static struct cmd_struct commands[] = {
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
{ "diff-index", cmd_diff_index, RUN_SETUP },
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
-   /*
-* NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
-* builtin/difftool.c has been removed, this entry should be changed to
-* RUN_SETUP | NEED_WORK_TREE
-*/
-   { "difftool", cmd_difftool },
+   { "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..aa0ef02597 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,10 +23,8 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
 }
 
-# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
-
 # Create a file on master and change it on branch
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
echo master >file &&
git add file &&
git commit -m "added file" &&
@@ -38,7 +36,7 @@ test_expect_success PERL 'setup' '
 '
 
 # Configure a custom difftool..cmd and use it

Re: [PATCH v5 3/3] Retire the scripted difftool

[Johannes Schindelin]

Hi Junio,

On Thu, 19 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Yep, as Git for Windows v2.11.0 is yesteryear's news, it was probably
> > obvious to you that I simply failed to spot and fix that oversight.
> 
> OK, if you want to tweak log message of either 2/3 or 3/3 to correct,
> there is still time, as they are still outside 'next'.  

Sent out v6 with the commit message reworded.

Ciao,
Johannes

Re: problem with insider build for windows and git

[Junio C Hamano]

Johannes Schindelin  writes:

>> are there any other topic that are already in 'master' that should go to
>> 2.11.x track?
>
> Personally, I would have merged 'nd/config-misc-fixes' into `maint`, I
> guess, and 'jc/abbrev-autoscale-config', and probably also 'jc/latin-1'.

The "almost ready" pushout were merging the ones that have been in
'master' for weeks (including that mingw-isatty topic).  These three
are still on radar, but they were too young and that was the only
reason why they were not included in the batch.

> The 'rj/git-version-gen-do-not-force-abbrev' topic would be another
> candidate for inclusion. The 'ah/grammos' strikes me as obvious `maint`
> material, as well as 'ew/svn-fixes'. 

I am holding back rj/git-version-gen-do-not-force-abbrev from 2.11.x
before 2.12 is released because I am a bit reluctant to tweak the
release infractructure in 'maint', before the same tweak hits
'master' and produces a release.

The second one will involve translators and that is why it is not
marked for back-merging in the draft release notes.

I agree that the svn thing should have been merged to 'maint' in
that batch, but I missed it.

> Having said that, these are the topics that *I* would merge into `maint`
> if I maintained Git. I don't, so this is just my opinion, man [*1*].

Yes, your opinion was exactly what was requested, and you gave one
;-)

[RESEND PATCHv2] contrib: remove git-convert-objects

[Stefan Beller]

git-convert-objects, originally named git-convert-cache was used in
early 2005 to convert to a new repository format, e.g. adding an author
date.

By now the need for conversion of the very early repositories is less
relevant, we no longer need to keep it in contrib; remove it.

Signed-off-by: Stefan Beller 
---

v2:
  no changes since v1, but as there were no replies nor do I find this
  patch cooking, I decided to resend.
  
Thanks,
Stefan

 contrib/convert-objects/convert-objects.c   | 329 
 contrib/convert-objects/git-convert-objects.txt |  29 ---
 2 files changed, 358 deletions(-)
 delete mode 100644 contrib/convert-objects/convert-objects.c
 delete mode 100644 contrib/convert-objects/git-convert-objects.txt

diff --git a/contrib/convert-objects/convert-objects.c 
b/contrib/convert-objects/convert-objects.c
deleted file mode 100644
index f3b57bf1d2..00
--- a/contrib/convert-objects/convert-objects.c
+++ /dev/null
@@ -1,329 +0,0 @@
-#include "cache.h"
-#include "blob.h"
-#include "commit.h"
-#include "tree.h"
-
-struct entry {
-   unsigned char old_sha1[20];
-   unsigned char new_sha1[20];
-   int converted;
-};
-
-#define MAXOBJECTS (100)
-
-static struct entry *convert[MAXOBJECTS];
-static int nr_convert;
-
-static struct entry * convert_entry(unsigned char *sha1);
-
-static struct entry *insert_new(unsigned char *sha1, int pos)
-{
-   struct entry *new = xcalloc(1, sizeof(struct entry));
-   hashcpy(new->old_sha1, sha1);
-   memmove(convert + pos + 1, convert + pos, (nr_convert - pos) * 
sizeof(struct entry *));
-   convert[pos] = new;
-   nr_convert++;
-   if (nr_convert == MAXOBJECTS)
-   die("you're kidding me - hit maximum object limit");
-   return new;
-}
-
-static struct entry *lookup_entry(unsigned char *sha1)
-{
-   int low = 0, high = nr_convert;
-
-   while (low < high) {
-   int next = (low + high) / 2;
-   struct entry *n = convert[next];
-   int cmp = hashcmp(sha1, n->old_sha1);
-   if (!cmp)
-   return n;
-   if (cmp < 0) {
-   high = next;
-   continue;
-   }
-   low = next+1;
-   }
-   return insert_new(sha1, low);
-}
-
-static void convert_binary_sha1(void *buffer)
-{
-   struct entry *entry = convert_entry(buffer);
-   hashcpy(buffer, entry->new_sha1);
-}
-
-static void convert_ascii_sha1(void *buffer)
-{
-   unsigned char sha1[20];
-   struct entry *entry;
-
-   if (get_sha1_hex(buffer, sha1))
-   die("expected sha1, got '%s'", (char *) buffer);
-   entry = convert_entry(sha1);
-   memcpy(buffer, sha1_to_hex(entry->new_sha1), 40);
-}
-
-static unsigned int convert_mode(unsigned int mode)
-{
-   unsigned int newmode;
-
-   newmode = mode & S_IFMT;
-   if (S_ISREG(mode))
-   newmode |= (mode & 0100) ? 0755 : 0644;
-   return newmode;
-}
-
-static int write_subdirectory(void *buffer, unsigned long size, const char 
*base, int baselen, unsigned char *result_sha1)
-{
-   char *new = xmalloc(size);
-   unsigned long newlen = 0;
-   unsigned long used;
-
-   used = 0;
-   while (size) {
-   int len = 21 + strlen(buffer);
-   char *path = strchr(buffer, ' ');
-   unsigned char *sha1;
-   unsigned int mode;
-   char *slash, *origpath;
-
-   if (!path || strtoul_ui(buffer, 8, ))
-   die("bad tree conversion");
-   mode = convert_mode(mode);
-   path++;
-   if (memcmp(path, base, baselen))
-   break;
-   origpath = path;
-   path += baselen;
-   slash = strchr(path, '/');
-   if (!slash) {
-   newlen += sprintf(new + newlen, "%o %s", mode, path);
-   new[newlen++] = '\0';
-   hashcpy((unsigned char *)new + newlen, (unsigned char 
*) buffer + len - 20);
-   newlen += 20;
-
-   used += len;
-   size -= len;
-   buffer = (char *) buffer + len;
-   continue;
-   }
-
-   newlen += sprintf(new + newlen, "%o %.*s", S_IFDIR, (int)(slash 
- path), path);
-   new[newlen++] = 0;
-   sha1 = (unsigned char *)(new + newlen);
-   newlen += 20;
-
-   len = write_subdirectory(buffer, size, origpath, 
slash-origpath+1, sha1);
-
-   used += len;
-   size -= len;
-   buffer = (char *) buffer + len;
-   }
-
-   write_sha1_file(new, newlen, tree_type, result_sha1);
-   free(new);
-   return used;
-}
-
-static void convert_tree(void *buffer, unsigned long size, unsigned char 

[PATCH v6 1/3] difftool: add a skeleton for the upcoming builtin

[Johannes Schindelin]

This adds a builtin difftool that still falls back to the legacy Perl
version, which has been renamed to `legacy-difftool`.

The idea is that the new, experimental, builtin difftool immediately hands
off to the legacy difftool for now, unless the config variable
difftool.useBuiltin is set to true.

This feature flag will be used in the upcoming Git for Windows v2.11.0
release, to allow early testers to opt-in to use the builtin difftool and
flesh out any bugs.

Signed-off-by: Johannes Schindelin 
---
 .gitignore|  1 +
 Makefile  |  3 +-
 builtin.h |  1 +
 builtin/difftool.c| 63 +++
 git-difftool.perl => git-legacy-difftool.perl |  0
 git.c |  6 +++
 t/t7800-difftool.sh   |  2 +
 7 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => git-legacy-difftool.perl (100%)

diff --git a/.gitignore b/.gitignore
index 6722f78f9a..ae025b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-difftool
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index d861bd9985..8cf5bef034 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,7 @@ SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
 SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-difftool.perl
+SCRIPT_PERL += git-legacy-difftool.perl
 SCRIPT_PERL += git-archimport.perl
 SCRIPT_PERL += git-cvsexportcommit.perl
 SCRIPT_PERL += git-cvsimport.perl
@@ -883,6 +883,7 @@ BUILTIN_OBJS += builtin/diff-files.o
 BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
+BUILTIN_OBJS += builtin/difftool.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index b9122bc5f4..67f80519da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,6 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const 
char *prefix);
 extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
 extern int cmd_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_difftool(int argc, const char **argv, const char *prefix);
 extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/difftool.c b/builtin/difftool.c
new file mode 100644
index 00..53870bbaf7
--- /dev/null
+++ b/builtin/difftool.c
@@ -0,0 +1,63 @@
+/*
+ * "git difftool" builtin command
+ *
+ * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
+ * git-difftool--helper script.
+ *
+ * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+ * The GIT_DIFF* variables are exported for use by git-difftool--helper.
+ *
+ * Any arguments that are unknown to this script are forwarded to 'git diff'.
+ *
+ * Copyright (C) 2016 Johannes Schindelin
+ */
+#include "builtin.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+
+/*
+ * NEEDSWORK: this function can go once the legacy-difftool Perl script is
+ * retired.
+ *
+ * We intentionally avoid reading the config directly here, to avoid messing up
+ * the GIT_* environment variables when we need to fall back to exec()ing the
+ * Perl script.
+ */
+static int use_builtin_difftool(void) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+
+   argv_array_pushl(,
+"config", "--bool", "difftool.usebuiltin", NULL);
+   cp.git_cmd = 1;
+   if (capture_command(, , 6))
+   return 0;
+   strbuf_trim();
+   ret = !strcmp("true", out.buf);
+   strbuf_release();
+   return ret;
+}
+
+int cmd_difftool(int argc, const char **argv, const char *prefix)
+{
+   /*
+* NEEDSWORK: Once the builtin difftool has been tested enough
+* and git-legacy-difftool.perl is retired to contrib/, this preamble
+* can be removed.
+*/
+   if (!use_builtin_difftool()) {
+   const char *path = mkpath("%s/git-legacy-difftool",
+ git_exec_path());
+
+   if (sane_execvp(path, (char **)argv) < 0)
+   die_errno("could not exec %s", path);
+
+   return 0;
+   }
+   prefix = setup_git_directory();
+   trace_repo_setup(prefix);
+   setup_work_tree();
+
+   die("TODO");
+}
diff --git a/git-difftool.perl b/git-legacy-difftool.perl
similarity index 100%
rename from git-difftool.perl

[PATCH v6 0/3] Turn the difftool into a builtin

[Johannes Schindelin]

This patch series converts the difftool from a Perl script into a
builtin, for three reasons:

1. Perl is really not native on Windows. Not only is there a performance
   penalty to be paid just for running Perl scripts, we also have to deal
   with the fact that users may have different Perl installations, with
   different options, and some other Perl installation may decide to set
   PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
   have to use because almost all other Perl distributions lack the
   Subversion bindings we need for `git svn`).

2. As the Perl script uses Unix-y paths that are not native to Windows,
   the Perl interpreter has to go through a POSIX emulation layer (the
   MSYS2 runtime). This means that paths have to be converted from
   Unix-y paths to Windows-y paths (and vice versa) whenever crossing
   the POSIX emulation barrier, leading to quite possibly surprising path
   translation errors.

3. Perl makes for a rather large reason that Git for Windows' installer
   weighs in with >30MB. While one Perl script less does not relieve us
   of that burden, it is one step in the right direction.

Changes since v5:

- reworded the commit message of 2/3 to account for the change in v4
  where we no longer keep both scripted and builtin difftool working
  (with the switch difftool.useBuiltin deciding which one is used).


Johannes Schindelin (3):
  difftool: add a skeleton for the upcoming builtin
  difftool: implement the functionality in the builtin
  Retire the scripted difftool

 Makefile   |   2 +-
 builtin.h  |   1 +
 builtin/difftool.c | 692 +
 .../examples/git-difftool.perl |   0
 git.c  |   1 +
 t/t7800-difftool.sh|  92 +--
 6 files changed, 741 insertions(+), 47 deletions(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => contrib/examples/git-difftool.perl (100%)


base-commit: ffac48d093d4b518a0cc0e8bf1b7cb53e0c3d7a2
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v6
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v6

-- 
2.11.0.windows.3

[PATCH v6 2/3] difftool: implement the functionality in the builtin

[Johannes Schindelin]

This patch gives life to the skeleton added in the previous patch.

The motivation for converting the difftool is that Perl scripts are not at
all native on Windows, and that `git difftool` therefore is pretty slow on
that platform, when there is no good reason for it to be slow.

In addition, Perl does not really have access to Git's internals. That
means that any script will always have to jump through unnecessary
hoops, and it will often need to perform unnecessary work (e.g. when
reading the entire config every time `git config` is called to query a
single config value).

The current version of the builtin difftool does not, however, make full
use of the internals but instead chooses to spawn a couple of Git
processes, still, to make for an easier conversion. There remains a lot
of room for improvement, left later.

Note: to play it safe, the original difftool is still called unless the
config setting difftool.useBuiltin is set to true.

The reason: this new, experimental, builtin difftool was shipped as part
of Git for Windows v2.11.0, to allow for easier large-scale testing, but
of course as an opt-in feature.

The speedup is actually more noticable on Linux than on Windows: a quick
test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s)
(real/user/sys) in a Linux VM, down from  (6.529s/3.112s/0.644s), while on
Windows, it is (36.064s/2.730s/7.194s), down from (47.637s/2.407s/6.863s).
The culprit is most likely the overhead incurred from *still* having to
shell out to mergetool-lib.sh and difftool--helper.sh.

Still, it is an improvement.

Signed-off-by: Johannes Schindelin 
---
 builtin/difftool.c | 672 -
 1 file changed, 671 insertions(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 53870bbaf7..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -11,9 +11,610 @@
  *
  * Copyright (C) 2016 Johannes Schindelin
  */
+#include "cache.h"
 #include "builtin.h"
 #include "run-command.h"
 #include "exec_cmd.h"
+#include "parse-options.h"
+#include "argv-array.h"
+#include "strbuf.h"
+#include "lockfile.h"
+#include "dir.h"
+
+static char *diff_gui_tool;
+static int trust_exit_code;
+
+static const char *const builtin_difftool_usage[] = {
+   N_("git difftool [] [ []] [--] [...]"),
+   NULL
+};
+
+static int difftool_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "diff.guitool")) {
+   diff_gui_tool = xstrdup(value);
+   return 0;
+   }
+
+   if (!strcmp(var, "difftool.trustexitcode")) {
+   trust_exit_code = git_config_bool(var, value);
+   return 0;
+   }
+
+   return git_default_config(var, value, cb);
+}
+
+static int print_tool_help(void)
+{
+   const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
+   return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int parse_index_info(char *p, int *mode1, int *mode2,
+   struct object_id *oid1, struct object_id *oid2,
+   char *status)
+{
+   if (*p != ':')
+   return error("expected ':', got '%c'", *p);
+   *mode1 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   *mode2 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   if (get_oid_hex(++p, oid1))
+   return error("expected object ID, got '%s'", p + 1);
+   p += GIT_SHA1_HEXSZ;
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   if (get_oid_hex(++p, oid2))
+   return error("expected object ID, got '%s'", p + 1);
+   p += GIT_SHA1_HEXSZ;
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   *status = *++p;
+   if (!*status)
+   return error("missing status");
+   if (p[1] && !isdigit(p[1]))
+   return error("unexpected trailer: '%s'", p + 1);
+   return 0;
+}
+
+/*
+ * Remove any trailing slash from $workdir
+ * before starting to avoid double slashes in symlink targets.
+ */
+static void add_path(struct strbuf *buf, size_t base_len, const char *path)
+{
+   strbuf_setlen(buf, base_len);
+   if (buf->len && buf->buf[buf->len - 1] != '/')
+   strbuf_addch(buf, '/');
+   strbuf_addstr(buf, path);
+}
+
+/*
+ * Determine whether we can simply reuse the file in the worktree.
+ */
+static int use_wt_file(const char *workdir, const char *name,
+  struct object_id *oid)
+{
+   struct strbuf buf = STRBUF_INIT;
+   struct stat st;
+   int use = 0;
+
+   strbuf_addstr(, workdir);
+   add_path(, buf.len, name);
+
+   if (!lstat(buf.buf, ) && !S_ISLNK(st.st_mode)) {
+   struct object_id wt_oid;
+   int fd = open(buf.buf, O_RDONLY);

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jan 18, 2017 at 05:22:40PM +0100, Johannes Schindelin wrote:
>
>> > > I want to err on the side of caution. That's why.
>> > 
>> > I guess I just don't see why changing the behavior with respect to
>> > "prune" or "proxy" is any less conservative than changing the one for
>> > "refspec".
>> 
> I think _this_ is a much better way of framing the problem. It is not
> about which keys are set, but about _where_ they are set. IOW, a
> reasonable rule would be: if there is any remote.*.X in the repo config,
> then git-remote should consider it a configured repo. And otherwise, no
> matter what is in ~/.gitconfig or elsewhere, git-remote should proceed
> as if it doesn't exist (and repo-level config can take precedence over
> config defined elsewhere).
>
> I.e., something like this:
>
> diff --git a/remote.c b/remote.c
> index 298f2f93f..720d616be 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char 
> *value, void *cb)
>   }
>   remote = make_remote(name, namelen);
>   remote->origin = REMOTE_CONFIG;
> + if (current_config_scope() == CONFIG_SCOPE_REPO)
> + remote->configured = 1;
>   if (!strcmp(subkey, "mirror"))
>   remote->mirror = git_config_bool(key, value);
>   else if (!strcmp(subkey, "skipdefaultupdate"))
>
> That doesn't make your test pass, but I think that is only because your
> test is not covering the interesting case (it puts the new config in the
> repo config, not in ~/.gitconfig).
>
> What do you think?
>
>> Originally, I would even have put the "vcs" into that set, as I could see
>> a legitimate use case for users to configure "remote.svn.vcs = vcs" in
>> ~/.gitconfig. But the regression test suite specifically tests for that
>> case, and I trust that there was a good reason, even if Thomas did not
>> describe that good reason in the commit message nor in any reply to this
>> patch pair.
>
> The config-scope thing above would allow "remote.svn.vcs" in
> ~/.gitconfig. But I don't think the test script actually checks that; it
> checks for the repo-level config. And we would continue to do the right
> thing there.

I am not "you" you are addressing to, but I think tying it to where
the variable came from makes quite sense.  

Because it makes it no longer possible to just inspect the
configured result to answer "is the remote configured?",
introduction of the configured field also needs to be preserved from
the original by Dscho, so does reading from historical non-config
sources like $GIT_DIR/remotes/*, which are by definition
per-repository thing.

IOW, with this tweak (and not setting ->configured based on what
keys are set), I think Dscho's patch makes sense.

Re: [PATCHv3 1/4] cache.h: document index_name_pos

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>  cache.h | 19 +++
>  1 file changed, 19 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 1b67f078dd..1469ddeafe 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -575,7 +575,26 @@ extern int verify_path(const char *path);
>  extern int index_dir_exists(struct index_state *istate, const char *name, 
> int namelen);
>  extern void adjust_dirname_case(struct index_state *istate, char *name);
>  extern struct cache_entry *index_file_exists(struct index_state *istate, 
> const char *name, int namelen, int igncase);
> +
> +/*
> + * Searches for an entry defined by name and namelen in the given index.
> + * If the return value is positive (including 0) it is the position of an
> + * exact match. If the return value is negative, the negated value minus 1
> + * is the position where the entry would be inserted.

So if the return value is -3, you negate it to get 3 and then
subtract 1 to get 2.  The function is telling you that "e" will
sit at active_cache[2] in the following example.

Which is correct.

> + * Example: The current index consists of these files and its stages:
> + *
> + *   b#0, d#0, f#1, f#3
> + *
> + * index_name_pos(, "a", 1) -> -1
> + * index_name_pos(, "b", 1) ->  0
> + * index_name_pos(, "c", 1) -> -2
> + * index_name_pos(, "d", 1) ->  1
> + * index_name_pos(, "e", 1) -> -3
> + * index_name_pos(, "f", 1) -> -3
> + * index_name_pos(, "g", 1) -> -5
> + */

This time the counting seems correct.

Thanks.

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Thu, Jan 19, 2017 at 12:12:47PM -0800, Junio C Hamano wrote:

> > The config-scope thing above would allow "remote.svn.vcs" in
> > ~/.gitconfig. But I don't think the test script actually checks that; it
> > checks for the repo-level config. And we would continue to do the right
> > thing there.
> 
> I am not "you" you are addressing to, but I think tying it to where
> the variable came from makes quite sense.  
> 
> Because it makes it no longer possible to just inspect the
> configured result to answer "is the remote configured?",
> introduction of the configured field also needs to be preserved from
> the original by Dscho, so does reading from historical non-config
> sources like $GIT_DIR/remotes/*, which are by definition
> per-repository thing.
> 
> IOW, with this tweak (and not setting ->configured based on what
> keys are set), I think Dscho's patch makes sense.

Yeah, worry if that wasn't clear: the hunk I posted was a just a
partial. The actual thing I built and ran against the test suite was
exactly as you described.

-Peff

Re: The design of refs backends, linked worktrees and submodules

[Johannes Schindelin]

Hi,

On Thu, 19 Jan 2017, Michael Haggerty wrote:

> On 01/19/2017 12:55 PM, Duy Nguyen wrote:
> > I've started working on fixing the "git gc" issue with multiple
> > worktrees, which brings me back to this. Just some thoughts. Comments
> > are really appreciated.
> > 
> > In the current code, files backend has special cases for both
> > submodules (explicitly) and linked worktrees (hidden behind git_path).
> 
> There is another terrible hack also needed to implement linked
> worktrees, namely that the `refs/bisect/` hierarchy is manually inserted
> into the `ref_cache`, because otherwise it wouldn't be noticed when
> iterating over loose references via `readdir()`.
> 
> Other similar hacks would be required if other reference subtrees are
> declared to be per-worktree.
> 
> > But if a backend has to handle this stuff, all future backends have to
> > too. Which does not sound great. Imagine we have "something" in
> > addition to worktrees and submodules in future, then all backends have
> > to learn about it.
> 
> Agreed, the status quo is not pretty.
> 
> I kindof think that it would have been a better design to store the
> references for all linked worktrees in the main repository's ref-store.
> For example, the "bisect" refs for a worktree named "" could have
> been stored under "refs/worktrees//bisect/*".

That strikes me as a good design, indeed. It addresses very explicitly the
root cause of the worktree problems: Git's source code was developed for
years with the assumption that there is a 1:1 mapping between ref names
and SHA-1s in each repository, and the way worktrees were implemented
broke that assumption.

So introducing a new refs/ namespace -- that is visible to all other
worktrees -- would have addressed that problem.

This, BTW, is related to my concerns about introducing a "shadow" config
layer for worktrees: I still think it would be a bad idea, and very likely
to cause regressions in surprising ways, to allow such config "overlays"
per-worktree, as Git's current code's assumption is that there is only one
config per repository, and that it can, say, set one config setting to
match another (which in the per-worktree case would possibly hold true in
only one worktree only). Instead, introducing a new "namespace" in the
(single) config similar to refs/worktrees/ could address that
problem preemptively.

Ciao,
Johannes

Re: [PATCH v5 0/4] Per-worktree config file support

[Stefan Beller]

On Thu, Jan 19, 2017 at 4:09 AM, Duy Nguyen  wrote:
> On Wed, Jan 11, 2017 at 12:01 AM, Stefan Beller  wrote:
>> On Tue, Jan 10, 2017 at 3:25 AM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> Let's get this rolling again. To refresh your memory because it's half
>>> a year since v4 [1], this is about letting each worktree in multi
>>> worktree setup has their own config settings. The most prominent ones
>>> are core.worktree, used by submodules, and core.sparseCheckout.
>>
>> Thanks for getting this rolling again.
>>
>>>
>>> This time I'm not touching submodules at all. I'm leaving it in the
>>> good hands of "submodule people". All I'm providing is mechanism. How
>>> you use it is up to you. So the series benefits sparse checkout users
>>> only.
>>
>> As one of the "submodule people", I have no complaints here.
>>
>>>
>>> Not much has changed from v4, except that the migration to new config
>>> layout is done automatically _update_ a config variable with "git
>>> config --worktree".
>>>
>>> I think this one is more or less ready. I have an RFC follow-up patch
>>> about core.bare, but that could be handled separately.
>>
>> I have read through the series and think the design is sound for worktrees
>> (though I have little knowledge about them).
>
> Submodules and multi worktrees start to look very similar, the more I
> think about it. Well, except that multi worktree does not separate odb
> and config files, maybe.

Similar to worktrees submodules can appear and disappear without
affecting the project/main tree. (though the mechanism is different,
for submodules, you'd checkout a version that doesn't have the submodule,
whereas for worktrees the user explicitely says: "I don't want to see this
worktree any more")

> And we have already seen both have a need to
> share code (like the moving .git dir operation). I suspect I'll learn
> more about submodules along the way, and you worktrees ;-)

I sure hope so.

>
>> Now even further:
>>
>> So to build on top of this series, I'd like to make submodules usable
>> with worktrees (i.e. shared object store, only clone/fetch once and
>> all worktrees
>> benefit from it), the big question is how to get the underlying data
>> model right.
>>
>> Would a submodule go into the superprojects
>>
>> .git/worktrees//modules//
>>
>> or rather
>>
>> .git/modules/worktrees/
>>
>> Or both (one of them being a gitlink file pointing at the other?)
>>
>> I have not made up my mind, as I haven't laid out all cases that are
>> relevant here.
>
> I would go with a conservative step first, keep submodules
> per-worktree. After it's sorted out. You can change the layout (e.g.
> as a config extension). The latter probably has some complication (but
> yeah sharing would be a big plus).

The sharing is what we are asked for as it would "make
submodules usable" (compared to the repo tool, which
doesn't have object sharing AFAIK). ;)

Currently I am leaning to put the worktree directory first and the
submodules within, i.e.

.git/worktrees//modules//

but in that directory, we'd only have the per-worktree
specific stuff, the object store would live with the superprojects
main worktree, i.e. at .git/modules/ we'd have
the main git dir for the submodule.

Thanks,
Stefan

> --
> Duy

Re: problem with insider build for windows and git

[Johannes Schindelin]

Hi Junio,

On Wed, 18 Jan 2017, Junio C Hamano wrote:

> Aside from the "ouch, one topic has merged earlier iteration, that
> was merged to 'master', also now merged to 'maint', and we need to
> follow up on both" you sent out earlier,

I know of one more "ouch" moment where my latest iterations did not get
picked up: my latest version of the "Avoid a segmentation fault with
renaming merges" patch did not output an error message in case of !nce
because the code flow will result in more appropriate error messages later
anyway. I did not provide a follow-up patch for that because the current
version in `maint` is not wrong per se.

> are there any other topic that are already in 'master' that should go to
> 2.11.x track?

Personally, I would have merged 'nd/config-misc-fixes' into `maint`, I
guess, and 'jc/abbrev-autoscale-config', and probably also 'jc/latin-1'.
The 'rj/git-version-gen-do-not-force-abbrev' topic would be another
candidate for inclusion. The 'ah/grammos' strikes me as obvious `maint`
material, as well as 'ew/svn-fixes'. I have no opinion on the p4 topics
(five, by my counting), as I have no experience with (or for that matter,
need for) Perforce, but Lars might have a strong opinion on those.

Having said that, these are the topics that *I* would merge into `maint`
if I maintained Git. I don't, so this is just my opinion, man [*1*].

Ciao,
Johannes

Footnote *1*: While you read that last part of the sentence, imagine me in
slippers and a bathrobe, with a White Russian in my left hand for which I
used milk instead of cream (for the White Russian, that is, not for my
left hand).

Re: The design of refs backends, linked worktrees and submodules

[Junio C Hamano]

Duy Nguyen  writes:

> ... A bit worried about transactions though,
> because I think per-repo and per-worktree updates will be separated in
> two transactions. But that's probably ok because future backends, like
> lmdb, will have to go through the same route anyway.

If we have per-worktree ref-store, what can be done as a useful
thing by future backends like lmdb may be to use the same single
database to store shared and per-worktree refs, similar to the way
Michael mentioned in his response to your message I am responding
to, i.e.

I kindof think that it would have been a better design to store the
references for all linked worktrees in the main repository's ref-store.
For example, the "bisect" refs for a worktree named "" could have
been stored under "refs/worktrees//bisect/*".

The current design is heavily influenced by how "contrib/workdir"
lays things out, for the latter of which I take the blame X-<, but
perhaps the files backend can be retrofitted to use that layout in
the longer term?

Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

Junio C Hamano  writes:

>> Since there's only one line that cares about the result of "colors",
>> maybe it would be less confusing to do:
>>
>>   if (!git_config_get-string("log.graphcolors", )) {
>>  ... parse, etc ...
>>  graph_set_column_colors(colors.argv, colors.argc - 1);
>>   } else {
>>  graph_set_column_colors(column_colors_ansi,
>>  column_colors_ansi_max);
>>   }
>
> Yes, that would be much much less confusing.  By doing so, the cover
> letter can lose "pushing the limits of abuse", too ;-).

Will queue with a squashable change.

Thanks.

 graph.c | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/graph.c b/graph.c
index 822d40ae20..00aeee36d8 100644
--- a/graph.c
+++ b/graph.c
@@ -229,22 +229,20 @@ struct git_graph *graph_init(struct rev_info *opt)
struct git_graph *graph = xmalloc(sizeof(struct git_graph));
 
if (!column_colors) {
-   struct argv_array ansi_colors = {
-   column_colors_ansi,
-   column_colors_ansi_max + 1
-   };
-   struct argv_array *colors = _colors;
char *string;
-
-   if (!git_config_get_string("log.graphcolors", )) {
+   if (git_config_get_string("log.graphcolors", )) {
+   /* not configured -- use default */
+   graph_set_column_colors(column_colors_ansi,
+   column_colors_ansi_max);
+   } else {
static struct argv_array custom_colors = 
ARGV_ARRAY_INIT;
argv_array_clear(_colors);
parse_graph_colors_config(_colors, string);
free(string);
-   colors = _colors;
+   /* graph_set_column_colors takes a max-index, not a 
count */
+   graph_set_column_colors(custom_colors.argv,
+   custom_colors.argc - 1);
}
-   /* graph_set_column_colors takes a max-index, not a count */
-   graph_set_column_colors(colors->argv, colors->argc - 1);
}
 
graph->commit = NULL;

[RFC/PATCH] Disallow commands from within unpopulated submodules.

[Stefan Beller]

Consider you have a submodule at path "sub". What should happen in case
you run a command such as "git -C sub add ." ?

Here is what currently happens:
1) The submodule is populated, i.e. there is a .git (file/dir) inside
   "sub". This is equivalent of running "git add ." in the submodule and
   it behaves as you would expect, adding all files to the index.
2) The submodule is not populated or even not initialized.
   For quite some time we got
   $ git -C sub add .
   git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= 
item->len && item->prefix <= item->len' failed.
   Aborted (core dumped)
   (This is fixed by another patch in flight to not assert,
but rather die with a better message instead; but that patch is
merely a fix of a corner case in the pathspec code.)

While 1) is rather uncontroversial, there are multiple things the user
may have intended with this command in 2):
 * add the submodule to the superproject
 * add all files inside the sub/ directory to the submodule or
   superproject.
It is unclear what the user intended, so rather error out instead.

Now let's ask the same question for "git -C sub status ." (which is a
command that is only reading and not writing to the repository)

1) If the submodule is populated, the user clearly intended to know
   more about the submodules status
2) It is unclear if the user wanted to learn about the submodules state
   (So ideally: "The submodule 'sub' is not initialized. To init ...")
   or the status check should be applied to the superproject instead.

Avoid the confusion in 2) as well and just error out for now. Later on
we may want to add another flag to git.c to allow commands to be run
inside unpopulated submodules and each command reacts appropriately.

Signed-off-by: Stefan Beller 
---

This is the next logical step after sb/pathspec-errors (pathspec:
give better message for submodule related pathspec error). If you are in
a path that is clearly a submodules, I would expect that most users would
expect the git operation to apply to the submodule.  In case of unpopulated
submodules, this is not the case though, but we apply the operation to the
superproject, which may be wrong or confusing.  Hence just error out for now.
Later we may want to add a flag that allows specific commands to run in such
a setup (e.g. git status could give a fancier message than a die(..)).

I marked this as RFC
* to request for comments if this is a good idea from a UI-perspective
* because I did not adapt any test for this patch. (A lot of submodule tests
  seem to break with this; From a cursory read of those tests, I'd rather
  blame the tests for being sloppy than this patch damaging user expectations)

Thanks,
Stefan

 git.c   |  3 +++
 submodule.c | 36 
 submodule.h |  1 +
 3 files changed, 40 insertions(+)

diff --git a/git.c b/git.c
index bbaa949e9c..ca53b61474 100644
--- a/git.c
+++ b/git.c
@@ -2,6 +2,7 @@
 #include "exec_cmd.h"
 #include "help.h"
 #include "run-command.h"
+#include "submodule.h"
 
 const char git_usage_string[] =
"git [--version] [--help] [-C ] [-c name=value]\n"
@@ -364,6 +365,8 @@ static int run_builtin(struct cmd_struct *p, int argc, 
const char **argv)
if (prefix)
die("can't use --super-prefix from a subdirectory");
}
+   if (prefix)
+   check_prefix_inside_submodule(prefix);
 
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
diff --git a/submodule.c b/submodule.c
index 73521cdbb2..357a22b804 100644
--- a/submodule.c
+++ b/submodule.c
@@ -495,6 +495,42 @@ void set_config_fetch_recurse_submodules(int value)
config_fetch_recurse_submodules = value;
 }
 
+/* check if the given prefix is inside an uninitialized submodule */
+void check_prefix_inside_submodule(const char *prefix)
+{
+   const char *work_tree = get_git_work_tree();
+   if (work_tree) {
+   int pos;
+   const struct cache_entry *in_submodule = NULL;
+
+   if (read_cache() < 0)
+   die("index file corrupt");
+   pos = cache_name_pos(prefix, strlen(prefix));
+   /*
+* gitlinks are recored with no ending '/' in the index,
+* but the prefix has an ending '/', so we will never find
+* an exact match, but always the position where we'd
+* insert the prefix.
+*/
+   if (pos < 0) {
+   const struct cache_entry *ce;
+   int len = strlen(prefix);
+   /* Check the previous position */
+   pos = (-1 - pos) - 1;
+   ce = active_cache[pos];
+   if (ce->ce_namelen < len)
+   len = ce->ce_namelen;
+   if (!memcmp(ce->name, prefix, len))
+  

Re: Git: new feature suggestion

[Joao Pinto]

Hi Linus,

Às 6:39 PM de 1/19/2017, Linus Torvalds escreveu:
> On Wed, Jan 18, 2017 at 10:33 PM, Konstantin Khomoutov
>  wrote:
>>
>> Still, I welcome you to read the sort-of "reference" post by Linus
>> Torvalds [1] in which he explains the reasoning behind this approach
>> implemented in Git.
> 
> It's worth noting that that discussion was from some _very_ early days
> in git (one week into the whole thing), when none of those
> visualization tools were actually implemented.
> 
> Even now, ten years after the fact, plain git doesn't actually do what
> I outlined. Yes, "git blame -Cw" works fairly well, and is in general
> better than the traditional per-file "annotate". And yes, "git log
> --follow" does another (small) part of the outlined thing, but is
> really not very powerful.
> 
> Some tools on top of git do more, but I think in general this is an
> area that could easily be improved upon. For example, the whole
> iterative and interactive drilling down in history of a particular
> file is very inconvenient to do with "git blame" (you find a commit
> that change the area in some way that you don't find interesting, so
> then you have to restart git blame with the parent of that
> unintersting commit).
> 
> You can do it in tig, but I suspect a more graphical tool might be better.
> 
> .. and we still end up having a lot of things where we simply just
> work with pathnames. For example, when doing merges, it' absolutely
> _wonderful_ doing
> 
>gitk --merge 
> 
> to see what happened to that filename that has a conflict during the
> merge. But it's all based on the whole-file changes, and sometimes
> you'd like to see just the commits that generate one particular
> conflict (in the kernel, things like the MAINTAINERS file can have
> quite a lot of changes, but they are all pretty idnependent, and what
> you want to see is just "changes to this area").
> 
> We do have the "-L" flag to git log, but it doesn't actually work for
> this particular case because of limitations.
> 
> So what I'm trying to say is that the argument from 10+ years ago that
> "you can do better with intelligent tools after-the-fact" is very much
> true, but it's also true that we don't actually have all those
> intelligent tools, and this is an area that could still be improved
> upon. Some of them are actually available as add-ons in various
> graphical IDE's that use git.
> 
>  Linus
> 

I am currently facing some challenges in one of Linux subsystems where a rename
of a set of folders and files would be the perfect scenario for future
development, but the suggestion is not accepted, not because it's not correct,
but because it makes the maintainer life harder in backporting bug fixes and new
features to older kernel versions and because it is not easy to follow the
renamed file/folder history from the kernel.org history logs.

Like nature shows us, the ability to adapt is the key for survival, so Linux
would gain a lot with some new features in git that can make maintainers life
easier. Assisted-backporting would be an excellent feature for them.

Did you ever thought about optimization backport operations through git or by an
add-on to it?

I am available to help if this feature makes sense for git users.

Thanks,
Joao

Re: [PATCH v3 14/21] read-cache: touch shared index files when used

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Jan 9, 2017 at 9:34 PM, Junio C Hamano  wrote:
>> Duy Nguyen  writes:
>>
>>> On Sun, Jan 8, 2017 at 4:46 AM, Junio C Hamano  wrote:
 Christian Couder  writes:

> So what should we do if freshen_file() returns 0 which means that the
> freshening failed?

 You tell me ;-)  as you are the one who is proposing this feature.
>>>
>>> My answer is, we are not worse than freshening loose objects case
>>> (especially since I took the idea from there).
>>
>> I do not think so, unfortunately.  Loose object files with stale
>> timestamps are not removed as long as objects are still reachable.
>
> But there are plenty of unreachable loose objects, added in index,
> then got replaced with new versions. cache-tree can create loose trees
> too and it's been run more often, behind user's back, to take
> advantage of the shortcut in unpack-trees.

I am not sure if I follow.  Aren't objects reachable from the
cache-tree in the index protected from gc?

Not that I think freshening would actually fail in a repository
where you can actually write into to update the index or its refs to
make a difference (iow, even if we make it die() loudly when shared
index cannot be "touched" because we are paranoid, no real life
usage will trigger that die(), and if a repository does trigger the
die(), I think you would really want to know about it).

Re: Git: new feature suggestion

[Linus Torvalds]

On Thu, Jan 19, 2017 at 10:54 AM, Joao Pinto  wrote:
>
> I am currently facing some challenges in one of Linux subsystems where a 
> rename
> of a set of folders and files would be the perfect scenario for future
> development, but the suggestion is not accepted, not because it's not correct,
> but because it makes the maintainer life harder in backporting bug fixes and 
> new
> features to older kernel versions and because it is not easy to follow the
> renamed file/folder history from the kernel.org history logs.

Honestly, that's less of a git issue, and more of a "patch will not
apply across versions" issue.

No amount of rename detection will ever fix that, simply because the
rename hadn't even _happened_ in the old versions that things get
backported to.

("git cherry-pick" can do a merge resolution and thus do "backwards"
renaming too, so tooling can definitely help, but it still ends up
meaning that even trivial patches are no longer the _same_ trivial
patch across versions).

So renaming things increases maintainer workloads in those situations
regardless of any tooling issues.

(You may also be referring to the mellanox mess, where this issue is
very much exacerbated by having different groups working on the same
thing, and maintainers having very much a "I will not take _anything_
from any of the groups that makes my life more complicated" model,
because those groups fucked up so much in the past).

In other words, quite often issues are about workflows rather than
tools. The networking layer probably has more of this, because David
actually does the backports himself, so he _really_ doesn't want to
complicate things.

   Linus

Re: [PATCH] clear_delta_base_cache(): don't modify hashmap while iterating

[Junio C Hamano]

Jeff King  writes:

> Thanks. Here it is rolled up with a commit message.
>
> -- >8 --
> Subject: clear_delta_base_cache(): don't modify hashmap while iterating
>
> Removing entries while iterating causes fast-import to
> access an already-freed `struct packed_git`, leading to
> various confusing errors.
>
> What happens is that clear_delta_base_cache() drops the
> whole contents of the cache by iterating over the hashmap,
> calling release_delta_base_cache() on each entry. That
> function removes the item from the hashmap. The hashmap code
> may then shrink the table, but the hashmap_iter struct
> retains an offset from the old table.
>
> As a result, the next call to hashmap_iter_next() may claim
> that the iteration is done, even though some items haven't
> been visited.
>
> The only caller of clear_delta_base_cache() is fast-import,
> which wants to clear the cache because it is discarding the
> packed_git struct for its temporary pack. So by failing to
> remove all of the entries, we still have references to the
> freed packed_git.
>
> To make things even more confusing, this doesn't seem to
> trigger with the test suite, because it depends on
> complexities like the size of the hash table, which entries
> got cleared, whether we try to access them before they're
> evicted from the cache, etc.
>
> So I've been able to identify the problem with large
> imports like freebsd's svn import, or a fast-export of
> linux.git. But nothing that would be reasonable to run as
> part of the normal test suite.
>
> We can fix this easily by iterating over the lru linked list
> instead of the hashmap. They both contain the same entries,
> and we can use the "safe" variant of the list iterator,
> which exists for exactly this case.
>
> Let's also add a warning to the hashmap API documentation to
> reduce the chances of getting bit by this again.
>
> Reported-by: Ulrich Spörlein 
> Signed-off-by: Jeff King 
> ---

Makes sense.  Thanks, both, for reporting, finding and fixing.

Will apply.

>  Documentation/technical/api-hashmap.txt | 4 +++-
>  sha1_file.c | 9 -
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/technical/api-hashmap.txt 
> b/Documentation/technical/api-hashmap.txt
> index 28f5a8b71..a3f020cd9 100644
> --- a/Documentation/technical/api-hashmap.txt
> +++ b/Documentation/technical/api-hashmap.txt
> @@ -188,7 +188,9 @@ Returns the removed entry, or NULL if not found.
>  `void *hashmap_iter_next(struct hashmap_iter *iter)`::
>  `void *hashmap_iter_first(struct hashmap *map, struct hashmap_iter *iter)`::
>  
> - Used to iterate over all entries of a hashmap.
> + Used to iterate over all entries of a hashmap. Note that it is
> + not safe to add or remove entries to the hashmap while
> + iterating.
>  +
>  `hashmap_iter_init` initializes a `hashmap_iter` structure.
>  +
> diff --git a/sha1_file.c b/sha1_file.c
> index 1eb47f611..d20714d6b 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2342,11 +2342,10 @@ static inline void release_delta_base_cache(struct 
> delta_base_cache_entry *ent)
>  
>  void clear_delta_base_cache(void)
>  {
> - struct hashmap_iter iter;
> - struct delta_base_cache_entry *entry;
> - for (entry = hashmap_iter_first(_base_cache, );
> -  entry;
> -  entry = hashmap_iter_next()) {
> + struct list_head *lru, *tmp;
> + list_for_each_safe(lru, tmp, _base_cache_lru) {
> + struct delta_base_cache_entry *entry =
> + list_entry(lru, struct delta_base_cache_entry, lru);
>   release_delta_base_cache(entry);
>   }
>  }

Re: grep open pull requests

[Jeff King]

On Thu, Jan 19, 2017 at 11:12:03AM -0700, Jack Bates wrote:

> I have a couple questions around grepping among open pull requests.
> 
> First, "git for-each-ref --no-merged": When I run the following,
> it lists refs/pull/1112/head, even though #1112 was merged in commit
> ced4da1. I guess this is because the tip of refs/pull/1112/head is 107fc59,
> not ced4da1?

Right. Git's `--no-merged` is about commit ancestry.

> This maybe shows my lack of familiarity with Git details,
> but superficially the two commits appear identical -- [1] and [2] --
> same parent, etc. Nonetheless they have different SHA-1s.
> I'm not sure why that is -- I didn't merge the commit --
> but regardless, GitHub somehow still connects ced4da1 with #1112.

The commits differ only in the committer timestamp. Try:

  diff -u <(git cat-file commit 107fc5910) \
  <(git cat-file commit ced4da132)

Is it possible that somebody cherry-pick or rebased it? Looking at the
history of apache/trafficserver, I don't see any merges, which implies
to me that the project is using a rebase workflow to merge PRs.

It's much trickier to find from the git topology whether a particular
history contains rebased versions of commits.  You can look at the
--cherry options to "git log", which use patch-ids to try to equate
commits. Something like:

  git for-each-ref --format='%(refname)' 'refs/pull/*/head' |
  while read refname; do
if test -z "$(git rev-list --right-only --cherry-pick -1 
origin...$refname)
then
echo "$refname: not merged"
fi
  done

That's obviously much less efficient than `--no-merged`, but it should
generally work. The exception is if the rebase changed the commit
sufficiently that its patch-id may have changed.

> So my question is, how are they doing that,

I suspect the answer is "somebody clicked the rebase button GitHub"
which simultaneously did the rebase and marked the PR as merged.

> Lastly, a question more about GitHub than Git, but: Given the way GitHub is
> setup, I hope I can get a list of unmerged pull requests from Git alone. Can
> you think of a way to list *open* pull requests,
> or is that status only available out of band?

That information isn't reflected in the git topology. It's in GitHub's
database. You can ask the API:

  https://developer.github.com/v3/

There are libraries to help with that:

  https://developer.github.com/libraries/

I think that's probably the best answer to your "unmerged" question,
too. Ask the API which PRs are unmerged, and then do whatever git-level
analysis you want based on that.

-Peff

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Brandon Williams]

On 01/19, Stefan Hajnoczi wrote:
> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
> 
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t:test-lib.sh:   setup_malloc_check () {
>   $ git show v2.9.3:t:test-lib.sh
>   fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
> 
> This patch attempts to use the correct delimiter:
> 
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t/test-lib.sh:   setup_malloc_check () {
>   $ git show v2.9.3:t/test-lib.sh
>   (success)
> 
> This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> it as a path because it contains colons.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  builtin/grep.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 3643d8a..06f8b47 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct 
> pathspec *pathspec,
>  
>   /* Add a delimiter if there isn't one already */
>   if (name[len - 1] != '/' && name[len - 1] != ':') {
> - strbuf_addch(, ':');
> + /* rev: or rev:path/ */
> + strbuf_addch(, strchr(name, ':') ? '/' : 
> ':');

As Jeff mentioned it may be better to base which character gets appended
by checking the obj->type field like this maybe:

diff --git a/builtin/grep.c b/builtin/grep.c
index 69dab5dc5..9dfe11dc7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -495,7 +495,8 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
/* Add a delimiter if there isn't one already */
if (name[len - 1] != '/' && name[len - 1] != ':') {
/* rev: or rev:path/ */
-   strbuf_addch(, strchr(name, ':') ? '/' : 
':');
+   char del = obj->type == OBJ_COMMIT ? ':' : '/';
+   strbuf_addch(, del);
}
}
init_tree_desc(, data, size);

-- 
Brandon Williams

Re: [RFC 2/2] grep: use '/' delimiter for paths

[Junio C Hamano]

Stefan Hajnoczi  writes:

> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
>
>   $ git grep malloc v2.9.3:t
>   v2.9.3:t:test-lib.sh:   setup_malloc_check () {
>   $ git show v2.9.3:t:test-lib.sh
>   fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'

I am slightly less negative on this compared to 1/2, but not by a
big margin.  The user wanted to feed a subtree to the command,
instead of doing the more natural

$ git grep -e malloc v2.9.3 -- t

So again, "contains a colon character" is not coming from what Git
does, but the user gave Git "a colon character" when she didn't have
to.

Having said that, if we wanted to start ignoring what the end-user
said in the initial input like 1/2 and 2/2 does (i.e. "this specific
tree object" as an input), I think the approach to solve for 1/2 and
2/2 should be the same.  I think we should decide to do a slash
instead of a colon, not because v2.9.3: has colon at the end and
v2.9.3:t has colon in it, but because both of these are both bare
tree objects.  The patches presented does not seem to base their
decision on the actual object type but on the textual input, which
seems wrong.

Re: Git: new feature suggestion

[Linus Torvalds]

On Wed, Jan 18, 2017 at 10:33 PM, Konstantin Khomoutov
 wrote:
>
> Still, I welcome you to read the sort-of "reference" post by Linus
> Torvalds [1] in which he explains the reasoning behind this approach
> implemented in Git.

It's worth noting that that discussion was from some _very_ early days
in git (one week into the whole thing), when none of those
visualization tools were actually implemented.

Even now, ten years after the fact, plain git doesn't actually do what
I outlined. Yes, "git blame -Cw" works fairly well, and is in general
better than the traditional per-file "annotate". And yes, "git log
--follow" does another (small) part of the outlined thing, but is
really not very powerful.

Some tools on top of git do more, but I think in general this is an
area that could easily be improved upon. For example, the whole
iterative and interactive drilling down in history of a particular
file is very inconvenient to do with "git blame" (you find a commit
that change the area in some way that you don't find interesting, so
then you have to restart git blame with the parent of that
unintersting commit).

You can do it in tig, but I suspect a more graphical tool might be better.

.. and we still end up having a lot of things where we simply just
work with pathnames. For example, when doing merges, it' absolutely
_wonderful_ doing

   gitk --merge 

to see what happened to that filename that has a conflict during the
merge. But it's all based on the whole-file changes, and sometimes
you'd like to see just the commits that generate one particular
conflict (in the kernel, things like the MAINTAINERS file can have
quite a lot of changes, but they are all pretty idnependent, and what
you want to see is just "changes to this area").

We do have the "-L" flag to git log, but it doesn't actually work for
this particular case because of limitations.

So what I'm trying to say is that the argument from 10+ years ago that
"you can do better with intelligent tools after-the-fact" is very much
true, but it's also true that we don't actually have all those
intelligent tools, and this is an area that could still be improved
upon. Some of them are actually available as add-ons in various
graphical IDE's that use git.

 Linus

Re: [RFC 1/2] grep: only add delimiter if there isn't one already

[Junio C Hamano]

Stefan Hajnoczi  writes:

> git-grep(1) output does not follow git's own syntax:
>
>   $ git grep malloc v2.9.3:
>   v2.9.3::Makefile:   COMPAT_CFLAGS += -Icompat/nedmalloc
>   $ git show v2.9.3::Makefile
>   fatal: Path ':Makefile' does not exist in 'v2.9.3'
>
> This patch avoids emitting the unnecessary ':' delimiter if the name
> already ends with ':' or '/':
>
>   $ git grep malloc v2.9.3:
>   v2.9.3:Makefile:   COMPAT_CFLAGS += -Icompat/nedmalloc
>   $ git show v2.9.3:Makefile
>   (succeeds)

I am mildly negative on this one.  I suspect that the above example
is a made-up one and you might have a more compelling real-world use
case in mind, but at least the above does not convince me.

The end-user explicitly gave the extra ':' because she wanted to
feed the tree object, not a tag that leads to the tree, for some
reason.  I think the output should respect that and show how she
spelled the starting point.  IOW, it is not "':' added unnecessarily
by Git".  It is ':' added by the end-user for whatever reason.

>
> Signed-off-by: Stefan Hajnoczi 
> ---
>  builtin/grep.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 462e607..3643d8a 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -490,7 +490,11 @@ static int grep_object(struct grep_opt *opt, const 
> struct pathspec *pathspec,
>   strbuf_init(, PATH_MAX + len + 1);
>   if (len) {
>   strbuf_add(, name, len);
> - strbuf_addch(, ':');
> +
> + /* Add a delimiter if there isn't one already */
> + if (name[len - 1] != '/' && name[len - 1] != ':') {
> + strbuf_addch(, ':');
> + }
>   }
>   init_tree_desc(, data, size);
>   hit = grep_tree(opt, pathspec, , , base.len,

Re: Git: new feature suggestion

[Joao Pinto]

Hi,

Às 6:33 AM de 1/19/2017, Konstantin Khomoutov escreveu:
> On Wed, 18 Jan 2017 10:40:52 +
> Joao Pinto  wrote:
> 
> [...]
>> I have seen a lot of Linux developers avoid this re-organization
>> operations because they would lose the renamed file history, because
>> a new log is created for the new file, even if it is a renamed
>> version of itself. I am sending you this e-mail to suggest the
>> creation of a new feature in Git: when renamed, a file or folder
>> should inherit his parent’s log and a “rename: …” would be
>> automatically created or have some kind of pointer to its “old” form
>> to make history analysis easier.
> 
> Git does not record renames because of its stance that what matters is
> code _of the whole project_ as opposed to its location in a particular
> file.
> 
> Hence with regard to renames Git "works backwards" by detecting them
> dynamically while traversing the history (such as with `git log`
> etc).  This detection uses certain heuristics which can be controlled
> with knobs pointed to by Stefan Beller.
> 
> Still, I welcome you to read the sort-of "reference" post by Linus
> Torvalds [1] in which he explains the reasoning behind this approach
> implemented in Git.  IMO, understanding the reasoning behind the idea
> is much better than just mechanically learning how to use it.
> 
> The whole thread (esp. Torvalds' replies) is worth reading, but that
> particular mail summarizes the whole thing very well.
> 
> (The reference link to it used to be [2], but Gmane is not fully
> recovered to be able to display it.)
> 
> 1. 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__public-2Dinbox.org_git_Pine.LNX.4.58.0504150753440.7211-40ppc970.osdl.org_=DwIDaQ=DPL6_X_6JkXFx7AXWqB0tg=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0=X0bQCOGTuZF-uq6smPwJDw4Q47qHgjWaewgTHCbhMnM=97U97toe9A6XOAJxbhxvWeYpzl-wPw9QvlhQfAEUTdI=
>  
> 2. 
> https://urldefense.proofpoint.com/v2/url?u=http-3A__thread.gmane.org_gmane.comp.version-2Dcontrol.git_27_focus-3D217=DwIDaQ=DPL6_X_6JkXFx7AXWqB0tg=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0=X0bQCOGTuZF-uq6smPwJDw4Q47qHgjWaewgTHCbhMnM=agYFOBCbLeaKAB6frWWzcwHkZyrMZLW4ExgDxzQyVlI=
>  
> 

Thank you very much for the info!

Joao

Re: [RFC] stash --continue

[Marc Branchaud]

On 2017-01-19 10:49 AM, Johannes Schindelin wrote:

Hi Marc,

On Wed, 18 Jan 2017, Marc Branchaud wrote:


On 2017-01-18 11:34 AM, Johannes Schindelin wrote:


On Wed, 18 Jan 2017, Marc Branchaud wrote:


On 2017-01-16 05:54 AM, Johannes Schindelin wrote:


On Mon, 16 Jan 2017, Stephan Beyer wrote:


a git-newbie-ish co-worker uses git-stash sometimes. Last time
he used "git stash pop", he got into a merge conflict. After he
resolved the conflict, he did not know what to do to get the
repository into the wanted state. In his case, it was only "git
add " followed by a "git reset" and a "git stash
drop", but there may be more involved cases when your index is
not clean before "git stash pop" and you want to have your index
as before.

This led to the idea to have something like "git stash
--continue"[1]


More like "git stash pop --continue". Without the "pop" command,
it does not make too much sense.


Why not?  git should be able to remember what stash command created
the conflict.  Why should I have to?  Maybe the fire alarm goes off
right when I run the stash command, and by the time I get back to it
I can't remember which operation I did.  It would be nice to be able
to tell git to "just finish off (or abort) the stash operation,
whatever it was".


That reeks of a big potential for confusion.

Imagine for example a total Git noob who calls `git stash list`,
scrolls two pages down, then hits `q` by mistake. How would you
explain to that user that `git stash --continue` does not continue
showing the list at the third page?


Sorry, but I have trouble taking that example seriously.  It assumes
such a level of "noobness" that the user doesn't even understand how
standard command output paging works, not just with git but with any
shell command.


Yeah, well, I thought you understood what I meant.

The example was the best I could come up with quickly, and it only tried
to show that there are *other* stash operations that one might perceive
to happen at the same time as the "pop" operation, so your flimsical
comment "why not continue the latest operation" may very well be
ambiguous.

And if it is not ambiguous in "stash", it certainly will be in other Git
operations. And therefore, having a DWIM in "stash" to allow "--continue"
without any specific subcommand, but not having it in other Git commands,
is just a very poor user interface design. It is prone to confuse users,
which is always a hallmark of a bad user interface.


Please don't underestimate the power of syntactic consistency in helping 
users achieve their goals.  Having some commands use "git foo 
--continue" while others use "git foo bar --continue" *will* confuse 
people, regardless of how logical the reasons for those differences.


But in the case of stash, I still don't see the utility in having 
operation-specific continuation.  Consider the following sequence (as 
you say, this doesn't work yet, but making it work seems reasonable):


git stash pop  # creates some conflicts
git stash apply stash@{4} # creates some other conflicts
# (User resolves the conflicts created by the pop.)
git stash pop --continue

Given the description of the original proposal (do "git reset; git stash 
drop"), what's the state of the index and the working tree?


In particular, what has the user gained by continuing just that pop?

Another thing to ask is, how common is such a scenario likely to be?  I 
suggest that it will be far more common for users to resolve all the 
conflicts and then want to continue all their interrupted stash 
operations.  If so, fussily forcing them to explicitly continue the pop 
and the apply is just a waste.



Hence my objection to "git stash --continue". No argument in favor of "git
stash --continue" I heard so far comes even close to being convincing.


Well, what about the potential for a slippery slope?  If the user is 
forced to be specific about continuing either a pop or an apply, why 
wouldn't git allow them to be specific about *which* pop or apply they 
want to continue?  Consider another hypothetical scenario:


git stash pop  # creates some conflicts
git stash pop  # creates some more conflicts
git stash pop  # creates even more conflicts
# (User resolves the conflicts created by second pop.)
git stash pop --continue
# Oops, there's still some unresovled pops!

Obviously the user isn't ready to finish off all the pops, so they'll 
want some way to specify which pop to continue.  Dealing with that just 
feels like a lot of work for minimal benefit.



Even worse: `git stash` (without arguments) defaults to the `save`
operation, so any user who does not read the documentation (and who
does?) would assume that `git stash --continue` *also* implies `save`.


Like the first example, your user is trying to "continue" a command that
is already complete.


Says who? You may understand the semantics better than other users, but
who are you to 

Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST

[Junio C Hamano]

Jacob Keller  writes:

> On Wed, Jan 18, 2017 at 11:45 AM, Junio C Hamano  wrote:
>>
>> I do not know if it is clear enough that 'option' in the last
>> sentence is a placeholder.  I then wondered if spelling it as
>> `--no-` would make it a bit clearer, but that is ugly.
>
> To be fair, this is exactly how the rest of the doc spells these
> things, so I would rather be consistent with the doc as is, and a
> future patch could clean this up. See OPT_SET_INT, for an example of
> `--no-option`.

Ah, OK, then I have no issues with it.

> Maybe we can rephrase this "The list is reset via `--no-option`"?

I think I saw that in your latest interdiff and I think it is good.

Thanks.

Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

Jeff King  writes:

> On Thu, Jan 19, 2017 at 06:41:23PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> +static void parse_graph_colors_config(struct argv_array *colors, const char 
>> *string)
>> +{
>> +const char *end, *start;
>> +
>> +start = string;
>> +end = string + strlen(string);
>> +while (start < end) {
>> +const char *comma = strchrnul(start, ',');
>> +char color[COLOR_MAXLEN];
>> +
>> +if (!color_parse_mem(start, comma - start, color))
>> +argv_array_push(colors, color);
>> +else
>> +warning(_("ignore invalid color '%.*s' in 
>> log.graphColors"),
>> +(int)(comma - start), start);
>> +start = comma + 1;
>> +}
>> +argv_array_push(colors, GIT_COLOR_RESET);
>> +}
>
> This looks good.
>
>> @@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)
>>  {
>>  struct git_graph *graph = xmalloc(sizeof(struct git_graph));
>>  
>> -if (!column_colors)
>> -graph_set_column_colors(column_colors_ansi,
>> -column_colors_ansi_max);
>> +if (!column_colors) {
>> +struct argv_array ansi_colors = {
>> +column_colors_ansi,
>> +column_colors_ansi_max + 1
>> +};
>
> Hrm. At first I thought this would cause memory corrution, because your
> argv_array_clear() would try to free() the non-heap array you've stuffed
> inside. But you only clear the custom_colors array which actually is
> dynamically allocated. This outer one is just here to give uniform
> access:
>
>> +struct argv_array *colors = _colors;
>> +char *string;
>> +
>> +if (!git_config_get_string("log.graphcolors", )) {
>> +static struct argv_array custom_colors = 
>> ARGV_ARRAY_INIT;
>> +argv_array_clear(_colors);
>> +parse_graph_colors_config(_colors, string);
>> +free(string);
>> +colors = _colors;
>> +}
>> +/* graph_set_column_colors takes a max-index, not a count */
>> +graph_set_column_colors(colors->argv, colors->argc - 1);
>> +}
>
> Since there's only one line that cares about the result of "colors",
> maybe it would be less confusing to do:
>
>   if (!git_config_get-string("log.graphcolors", )) {
>   ... parse, etc ...
>   graph_set_column_colors(colors.argv, colors.argc - 1);
>   } else {
>   graph_set_column_colors(column_colors_ansi,
>   column_colors_ansi_max);
>   }

Yes, that would be much much less confusing.  By doing so, the cover
letter can lose "pushing the limits of abuse", too ;-).

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Wed, Jan 18, 2017 at 05:22:40PM +0100, Johannes Schindelin wrote:

> > > I want to err on the side of caution. That's why.
> > 
> > I guess I just don't see why changing the behavior with respect to
> > "prune" or "proxy" is any less conservative than changing the one for
> > "refspec".
> 
> Let's take a step back and consider the problem I try to solve, okay?

OK. Though after reading your message over several times, I think I may
have confused things by raising two separate issues.

So let me re-state my issues here for clarity:

  1. I'm concerned that a setting for remote..X would fail to apply
 to a bare "git fetch " if "X" is considered as "not really"
 configuring a remote.

  2. I'm concerned that splitting the remote.*.X keys into two classes:
 "really configures" and "does not really configure" creates a
 confusing interface for users. Some keys are OK to set in your
 ~/.gitconfig and others are not.

Let's talk about concern 1 first.  Based on your analysis in this
message, it looks like it _isn't_ a problem with your patch (because
is_configured is never used for applying values, only for add/del/rename
types of operations in remote.c).

So good. Thanks for addressing my concern. And that makes your
"conservative" comment make more sense; the idea is that you are not
breaking anything, but just loosening selectively for some values of
"X".

I think there are still some weird corner cases even for "prune",
though. E.g., try:

  git init
  git remote add foo whatever
  git config remote.foo.prune true
  git config remote.other.prune false

So now we have:

[remote "foo"]
url = whatever
fetch = +refs/heads/*:refs/remotes/foo/*
prune = true
[remote "other"]
prune = false

Now try "git remote rename foo other". With current versions of git,
it's disallowed. With your patch, I get:

[remote "other"]
url = whatever
prune = true
[remote "other"]
prune = false
fetch = +refs/heads/*:refs/remotes/other/*

The old value of remote.other.prune is overriding the one we tried to
move into place.

In your motivating example, the old value is in ~/.gitconfig, so it
automatically takes lower precedence, and everything just works (I think
it would also just work if "other" had been defined originally _before_
foo in the config file).

I think you could fix it by having git-remote teach the "not really"
config values (like "prune") to overwrite any existing value when
rewriting the config. I think this just needs the multi_replace flag set
when calling git_config_set_multivar().

That raises questions about what should happen when multi-value keys
like "refspec" would be set (if we were to add them to the "not really"
set). Should they be overwritten, or merged? And in that sense, your
patch lets you punt on those issues.


I still think my second concern is valid. It's made worse by your patch
(if only because everything was disallowed before, and now some things
are and some things aren't). If this is a first step towards a final
state where the rules are simple, then starting conservatively makes
sense. And until we get there, the answer "yes, it should, but nobody
has worked out the semantics yet; care to make a patch?" is an OK one.
But it sounds like you do not ever want to loosen the "refspec" case.

I don't think that's ideal, but at the very least if that's the end
state, the list of "OK to use in ~/.gitconfig" keys should probably be
documented.

Reading your message, though, I still wonder if we can do better...

> The problem is that `git remote rename  ` refuses to do its job
> if it detects a configured ``. And it detects a configured ``
> even in cases where there is not *really* any remote configured.

I'd add to this that "git remote add " should work in a similar way
(that was the one that I think people often ran into with
remote.origin.fetch refspecs).

> The example use case is to configure `remote.origin.prune = true` in
> ~/.gitconfig, i.e. changing Git's default for all "origin" remotes in all
> of the user's repositories.
> 
> Now, the *real* fix would be to detect whether the remote was "configured"
> in the current repository, or in ~/.gitconfig. But that may not even be
> desirable, as we would want a more general fix for the question: can `git
> remote` rename a given remote to a new name, i.e. is that new name already
> taken?

I think _this_ is a much better way of framing the problem. It is not
about which keys are set, but about _where_ they are set. IOW, a
reasonable rule would be: if there is any remote.*.X in the repo config,
then git-remote should consider it a configured repo. And otherwise, no
matter what is in ~/.gitconfig or elsewhere, git-remote should proceed
as if it doesn't exist (and repo-level config can take precedence over
config defined elsewhere).

I.e., something like this:

diff 

Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()

[Junio C Hamano]

Jeff King  writes:

> On Thu, Jan 19, 2017 at 06:41:22PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> Normally color_parse_mem() is called from config parser which trims the
>> leading spaces already. The new caller in the next patch won't. Let's be
>> tidy and trim leading spaces too (we already trim trailing spaces before
>> comma).
>
> What comma? I don't think that exists until the next patch. :)
>
> I think just trimming from the front is OK, though, because
> color_parse_mem() trims trailing whitespace after a word. So either you
> have a word and we will trim after it, or you do not (in which case
> this will trim everything and hit the !len case you added).
>
> So maybe a better commit message is just:
>
>   Normally color_parse_mem() is called from config parser which trims
>   the leading spaces already. The new caller in the next patch won't.
>   Let's be tidy and trim leading spaces too (we already trim trailing
>   spaces after a word).
>
> -Peff

Yeah, my reading stuttered at the same place in the original, and
your rewrite looks a lot more sensible.

Re: [RFC 0/2] grep: make output consistent with revision syntax

[Brandon Williams]

On 01/19, Jeff King wrote:
> On Thu, Jan 19, 2017 at 03:03:45PM +, Stefan Hajnoczi wrote:
> 
> > git-grep(1)'s output is not consistent with git-rev-parse(1) revision 
> > syntax.
> > 
> > This means you cannot take "rev:path/to/file.c: foo();" output from 
> > git-grep(1)
> > and expect "git show rev:path/to/file.c" to work.  See the individual 
> > patches
> > for examples of command-lines that produce invalid output.
> 
> I think this is a good goal.

I agree.

> I couldn't immediately think of any cases where your patches would
> misbehave, but my initial thought was that the "/" versus ":"
> distinction is about whether the initial object is a tree or a commit.

I think this is also the case, I couldn't think of another case where
this decision wasn't based on if the object is a tree or a commit.
Interestingly enough I don't think we have any tests that exist that
test the formatting of grep's output when given a tree object since the
test suite still passes with these changes in. Which means this fix
should probably include a couple tests to ensure there's no regression
in the future.

-- 
Brandon Williams

Re: Git: new feature suggestion

[Junio C Hamano]

Konstantin Khomoutov  writes:

> Still, I welcome you to read the sort-of "reference" post by Linus
> Torvalds [1] in which he explains the reasoning behind this approach
> implemented in Git.  IMO, understanding the reasoning behind the idea
> is much better than just mechanically learning how to use it.
>
> The whole thread (esp. Torvalds' replies) is worth reading, but that
> particular mail summarizes the whole thing very well.
>
> (The reference link to it used to be [2], but Gmane is not fully
> recovered to be able to display it.)
>
> 1. 
> http://public-inbox.org/git/pine.lnx.4.58.0504150753440.7...@ppc970.osdl.org/
> 2. http://thread.gmane.org/gmane.comp.version-control.git/27/focus=217

Indeed.  Thanks for providing a link to it here ;-)

The message is the most important one in the early history of Git,
and it still is one of the most important messages in the Git
mailing-list archive.  "git log -S" was designed to take a
block of text (even though people misuse it and feed a single line
to it) exactly because it wanted to serve the "tracking when that
file+line changed" part in that vision.  The rename detection in
"diff" was meant to be used on the commit "git log -S" finds
to see if the found change came from another file so that the user
can decide that "digging further" part needs to be done for another
file.  "git blame" with -M and -C options were done to mostly
automate the "drilling down" process that finds the last commit that
touched each line in the above process, and when used with tools
like "tig", you can even peel one commit back and "zoom down" if the
found commit is an uninteresting one (e.g. a change with only code
formatting).

One thing that is still missing in the current version of Git,
compared to the "ideal SCM" the message envisioned, is the part that
notices: "oops, that line didn't even exist in the previous version,
BUT I FOUND FIVE PLACES that matched almost perfectly in the same
diff, and here they are".

grep open pull requests

[Jack Bates]

I have a couple questions around grepping among open pull requests.

First, "git for-each-ref --no-merged": When I run the following,
it lists refs/pull/1112/head, even though #1112 was merged in commit 
ced4da1. I guess this is because the tip of refs/pull/1112/head is 
107fc59, not ced4da1?


This maybe shows my lack of familiarity with Git details,
but superficially the two commits appear identical -- [1] and [2] --
same parent, etc. Nonetheless they have different SHA-1s.
I'm not sure why that is -- I didn't merge the commit --
but regardless, GitHub somehow still connects ced4da1 with #1112.

So my question is, how are they doing that,
and why can't "git for-each-ref --no-merged" likewise
connect ced4da1 with refs/pull/1112/head?

  $ git clone \
  >   --bare \
  >   --config remote.origin.fetch=+refs/pull/*/head:refs/pull/*/head \
  >   https://github.com/apache/trafficserver.git
  $ cd trafficserver.git
  $ git for-each-ref --no-merged


More generally, what I want is to periodically list open pull requests 
that add or modify lines containing the string "memset". So far I have 
the following in a Makefile. Can you recommend any improvements?


.PHONY: all
all: trafficserver.git
cd trafficserver.git && git fetch
	cd trafficserver.git && git for-each-ref --format '%(refname)' 
refs/pull --no-merged | \

  while read refname; do \
	git log -p "master..$$refname" | grep -q '^+.*memset' && echo 
"$$refname"; \

  done

trafficserver.git:
git clone \
  --bare \
  --config remote.origin.fetch=+refs/pull/*/head:refs/pull/*/head \
  https://github.com/apache/trafficserver.git


Lastly, a question more about GitHub than Git, but: Given the way GitHub 
is setup, I hope I can get a list of unmerged pull requests from Git 
alone. Can you think of a way to list *open* pull requests,

or is that status only available out of band?

Thanks!


[1] 
https://github.com/apache/trafficserver/commit/107fc59104cce2a4b527f04e7ac86695c98b568c
[2] 
https://github.com/apache/trafficserver/commit/ced4da13279f834c381925f2ecd1649bfb459e8b

Re: [PATCH v5 3/3] Retire the scripted difftool

[Junio C Hamano]

Johannes Schindelin  writes:

>> Ok, I was mostly reacting to 2/3 while I am reading it:
>> 
>> The reason: this new, experimental, builtin difftool will be shipped as
>> part of Git for Windows v2.11.0, to allow for easier large-scale
>> testing, but of course as an opt-in feature.
>> 
>> as there is no longer an opportunity to participate in this opt-in
>> testing, unless 3/3 is special cased and delayed.
>
> Yep, as Git for Windows v2.11.0 is yesteryear's news, it was probably
> obvious to you that I simply failed to spot and fix that oversight.

OK, if you want to tweak log message of either 2/3 or 3/3 to
correct, there is still time, as they are still outside 'next'.  

I am hoping we can merge it and others to 'next' by the end of the
week, though.

Re: [PATCH] log: new option decorate reflog of remote refs

[Jeff King]

On Thu, Jan 19, 2017 at 07:26:30PM +0700, Nguyễn Thái Ngọc Duy wrote:

> This is most useful when you fork your branches off a remote ref and
> rely on ref decoration to show your fork points in `git log`. Then you
> do a "git fetch" and suddenly the remote decoration is gone because
> remote refs are moved forward. With this, we can still see something
> like "origin/foo@{1}"
> 
> This is for remote refs only because based on my experience, docorating
> local reflog is just too noisy. You will most likely see HEAD@{1},
> HEAD@{2} and so on. We can add that as a separate option in future if we
> see a need for it.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  I've been using this for many weeks and it has proven its usefulness
>  (to me). Looks like good material to send upstream.

I think it's a neat idea, but the actual option:

> +--decorate-remote-reflog[=]::
> + Decorate `` most recent reflog entries on remote refs, up
> + to the specified number of entries. By default, only the most
> + recent reflog entry is decorated.

seems weirdly limited and non-orthogonal. What happens when somebody
wants to decorate other reflogs besides refs/remotes?

We already have very flexible ref-selectors like --remotes, --branches,
etc. The generalization of this would perhaps be something like:

  git log --decorate-reflog --remotes --branches

where "--decorate-reflog" applies to the next ref selector and then is
reset, the same way --exclude is. And it includes those refs _only_ for
decoration, not for traversal. So you could do:

  git log --decorate-reflog --remotes --remotes

if you wanted to see use those as traversal roots, too (if this is
common, it might even merit another option for "decorate and show").

That's just off the top of my head, so maybe there are issues. I was
just surprised to see the "-remote" part in your option name.

-Peff

Re: git fast-import crashing on big imports

[Ulrich Spörlein]

2017-01-18 22:51 GMT+01:00 Jeff King :
>
> On Wed, Jan 18, 2017 at 03:27:04PM -0500, Jeff King wrote:
>
> > On Wed, Jan 18, 2017 at 09:20:07PM +0100, Ulrich Spörlein wrote:
> >
> > > I found your commit via bisect in case you were wondering. Thanks for
> > > picking this up.
> >
> > Still downloading. However, just looking at the code, the obvious
> > culprit would be clear_delta_base_cache(), which is called from
> > literally nowhere except fast-import, and then only when checkpointing.

Sorry for the bad URL, pesky last minute changes ...

> Hmm. I haven't reproduced your exact issue, but I was able to produce
> some hijinks in that function.
>
> The problem is that the hashmap_iter interface is unreliable if entries
> are added or removed from the map during the iteration.
>
> I suspect the patch below may fix things for you. It works around it by
> walking over the lru list (either is fine, as they both contain all
> entries, and since we're clearing everything, we don't care about the
> order).

Confirmed. With the patch applied, I can import the whole 55G in one go
without any crashes or aborts. Thanks much!

>
> ---
>  sha1_file.c | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 1eb47f611..d20714d6b 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2342,11 +2342,10 @@ static inline void release_delta_base_cache(struct 
> delta_base_cache_entry *ent)
>
>  void clear_delta_base_cache(void)
>  {
> -   struct hashmap_iter iter;
> -   struct delta_base_cache_entry *entry;
> -   for (entry = hashmap_iter_first(_base_cache, );
> -entry;
> -entry = hashmap_iter_next()) {
> +   struct list_head *lru, *tmp;
> +   list_for_each_safe(lru, tmp, _base_cache_lru) {
> +   struct delta_base_cache_entry *entry =
> +   list_entry(lru, struct delta_base_cache_entry, lru);
> release_delta_base_cache(entry);
> }
>  }
> --
> 2.11.0.698.gd6b48ab4c
>
>
>
>
> >
> > -Peff

Re: [RFC 0/2] grep: make output consistent with revision syntax

[Jeff King]

On Thu, Jan 19, 2017 at 03:03:45PM +, Stefan Hajnoczi wrote:

> git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.
> 
> This means you cannot take "rev:path/to/file.c: foo();" output from 
> git-grep(1)
> and expect "git show rev:path/to/file.c" to work.  See the individual patches
> for examples of command-lines that produce invalid output.

I think this is a good goal.

I couldn't immediately think of any cases where your patches would
misbehave, but my initial thought was that the "/" versus ":"
distinction is about whether the initial object is a tree or a commit.

You do still have to special case the root tree (so "v2.9.3:" does not
get any delimiter). I think "ends in a colon" is actually a reasonable
way of determining that.

> This series is an incomplete attempt at solving the issue.  I'm not familiar
> enough with the git codebase to propose a better solution.  Perhaps someone is
> interested in a proper fix?

Are there cases you know that aren't covered by your patches?

-Peff

Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST

[Johannes Schindelin]

Hi Junio,

On Wed, 18 Jan 2017, Junio C Hamano wrote:

> "Philip Oakley"  writes:
> 
> >>> +`OPT_STRING_LIST(short, long, , arg_str, description)`::
> >>> + Introduce an option with string argument.
> >>> + The string argument is stored as an element in `` which must be a
> >>> + struct string_list. Reset the list using `--no-option`.
> >>> +
> >>
> >> I do not know if it is clear enough that 'option' in the last
> >> sentence is a placeholder.  I then wondered if spelling it as
> >> `--no-` would make it a bit clearer, but that is ugly.
> >
> > Bikeshedding:: `--no-` maybe, i.e. just surround the option
> > word with the angle brackets to indicate it is to be replaced by the
> > real option's name.
> 
> Yeah, I bikeshedded that myself, and rejected it because there is no
>  mentioned anywhere in the enumeration header.

As I pointed out in a previous review round: the surrounding test uses
--no-option (I agree that it is tedious to go back to the original code
for review, rather than a mere patch review that lacks context, but I
suspected that Jake did not come up with that `--no-option` himself), so
by our own recommendation (imitate the surrounding, existing code/text)
Jake did exactly the right thing:

$ git grep -e --no-option upstream/master -- 
Documentation/technical/api-parse-options.txt
upstream/master:Documentation/technical/api-parse-options.txt:  `--option` and 
set to zero with `--no-option`.
upstream/master:Documentation/technical/api-parse-options.txt:  (even if 
initially negative), and `--no-option` resets it to
upstream/master:Documentation/technical/api-parse-options.txt:  zero. To 
determine if `--option` or `--no-option` was encountered at
upstream/master:Documentation/technical/api-parse-options.txt:  `--no-option` 
was seen.
upstream/master:Documentation/technical/api-parse-options.txt:  reset to zero 
with `--no-option`.

Ciao,
Johannes

Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Jeff King]

On Thu, Jan 19, 2017 at 06:41:23PM +0700, Nguyễn Thái Ngọc Duy wrote:

> +static void parse_graph_colors_config(struct argv_array *colors, const char 
> *string)
> +{
> + const char *end, *start;
> +
> + start = string;
> + end = string + strlen(string);
> + while (start < end) {
> + const char *comma = strchrnul(start, ',');
> + char color[COLOR_MAXLEN];
> +
> + if (!color_parse_mem(start, comma - start, color))
> + argv_array_push(colors, color);
> + else
> + warning(_("ignore invalid color '%.*s' in 
> log.graphColors"),
> + (int)(comma - start), start);
> + start = comma + 1;
> + }
> + argv_array_push(colors, GIT_COLOR_RESET);
> +}

This looks good.

> @@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)
>  {
>   struct git_graph *graph = xmalloc(sizeof(struct git_graph));
>  
> - if (!column_colors)
> - graph_set_column_colors(column_colors_ansi,
> - column_colors_ansi_max);
> + if (!column_colors) {
> + struct argv_array ansi_colors = {
> + column_colors_ansi,
> + column_colors_ansi_max + 1
> + };

Hrm. At first I thought this would cause memory corrution, because your
argv_array_clear() would try to free() the non-heap array you've stuffed
inside. But you only clear the custom_colors array which actually is
dynamically allocated. This outer one is just here to give uniform
access:

> + struct argv_array *colors = _colors;
> + char *string;
> +
> + if (!git_config_get_string("log.graphcolors", )) {
> + static struct argv_array custom_colors = 
> ARGV_ARRAY_INIT;
> + argv_array_clear(_colors);
> + parse_graph_colors_config(_colors, string);
> + free(string);
> + colors = _colors;
> + }
> + /* graph_set_column_colors takes a max-index, not a count */
> + graph_set_column_colors(colors->argv, colors->argc - 1);
> + }

Since there's only one line that cares about the result of "colors",
maybe it would be less confusing to do:

  if (!git_config_get-string("log.graphcolors", )) {
... parse, etc ...
graph_set_column_colors(colors.argv, colors.argc - 1);
  } else {
graph_set_column_colors(column_colors_ansi,
column_colors_ansi_max);
  }

-Peff

Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()

[Jeff King]

On Thu, Jan 19, 2017 at 06:41:22PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Normally color_parse_mem() is called from config parser which trims the
> leading spaces already. The new caller in the next patch won't. Let's be
> tidy and trim leading spaces too (we already trim trailing spaces before
> comma).

What comma? I don't think that exists until the next patch. :)

I think just trimming from the front is OK, though, because
color_parse_mem() trims trailing whitespace after a word. So either you
have a word and we will trim after it, or you do not (in which case
this will trim everything and hit the !len case you added).

So maybe a better commit message is just:

  Normally color_parse_mem() is called from config parser which trims
  the leading spaces already. The new caller in the next patch won't.
  Let's be tidy and trim leading spaces too (we already trim trailing
  spaces after a word).

-Peff

GOOD NEWS

[Notburga (Sr. Notburga) Maringele]

You are a recipient to Mrs Julie Leach Donation of $2 million USD.  
Contact (julieleach...@gmail.com) for claims.

Did you get my message this time?

[Quatif Group of Companies ..]

-- 
Dear Friend,

I would like to discuss a very important issue with you. I am writing to
find out if this is your valid email. Please, let me know if this email is
valid

Kind regards
Adrien Saif
Attorney to Quatif Group of Companies

Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0

[Jeff King]

On Thu, Jan 19, 2017 at 06:41:21PM +0700, Nguyễn Thái Ngọc Duy wrote:

> In this code we want to match the word "reset". If len is zero,
> strncasecmp() will return zero and we incorrectly assume it's "reset" as
> a result.

This is probably a good idea. This _is_ user-visible, so it's possible
somebody was using empty config as a synonym for "reset". But since it
was never documented, I feel like relying on that is somewhat crazy.

-Peff

Re: [PATCH v5 3/3] Retire the scripted difftool

[Johannes Schindelin]

Hi Junio,

On Wed, 18 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Hi Junio,
> >
> > On Tue, 17 Jan 2017, Junio C Hamano wrote:
> >
> >> Johannes Schindelin  writes:
> >> 
> >> > It served its purpose, but now we have a builtin difftool. Time for the
> >> > Perl script to enjoy Florida.
> >> >
> >> > Signed-off-by: Johannes Schindelin 
> >> > ---
> >> 
> >> The endgame makes a lot of sense.  Both in the cover letter and in
> >> the previous patch you talk about having both in the released
> >> version, so do you want this step to proceed slower than the other
> >> two?
> >
> > I did proceed that slowly. Already three Git for Windows versions have
> > been released with both.
> >
> > But I submitted this iteration with this patch, so my intent is clearly to
> > retire the Perl script.
> 
> Ok, I was mostly reacting to 2/3 while I am reading it:
> 
> The reason: this new, experimental, builtin difftool will be shipped as
> part of Git for Windows v2.11.0, to allow for easier large-scale
> testing, but of course as an opt-in feature.
> 
> as there is no longer an opportunity to participate in this opt-in
> testing, unless 3/3 is special cased and delayed.

Yep, as Git for Windows v2.11.0 is yesteryear's news, it was probably
obvious to you that I simply failed to spot and fix that oversight.

Ciao,
Johannes

[PATCH] clear_delta_base_cache(): don't modify hashmap while iterating

[Jeff King]

On Thu, Jan 19, 2017 at 03:03:46PM +0100, Ulrich Spörlein wrote:

> > I suspect the patch below may fix things for you. It works around it by
> > walking over the lru list (either is fine, as they both contain all
> > entries, and since we're clearing everything, we don't care about the
> > order).
> 
> Confirmed. With the patch applied, I can import the whole 55G in one go
> without any crashes or aborts. Thanks much!

Thanks. Here it is rolled up with a commit message.

-- >8 --
Subject: clear_delta_base_cache(): don't modify hashmap while iterating

Removing entries while iterating causes fast-import to
access an already-freed `struct packed_git`, leading to
various confusing errors.

What happens is that clear_delta_base_cache() drops the
whole contents of the cache by iterating over the hashmap,
calling release_delta_base_cache() on each entry. That
function removes the item from the hashmap. The hashmap code
may then shrink the table, but the hashmap_iter struct
retains an offset from the old table.

As a result, the next call to hashmap_iter_next() may claim
that the iteration is done, even though some items haven't
been visited.

The only caller of clear_delta_base_cache() is fast-import,
which wants to clear the cache because it is discarding the
packed_git struct for its temporary pack. So by failing to
remove all of the entries, we still have references to the
freed packed_git.

To make things even more confusing, this doesn't seem to
trigger with the test suite, because it depends on
complexities like the size of the hash table, which entries
got cleared, whether we try to access them before they're
evicted from the cache, etc.

So I've been able to identify the problem with large
imports like freebsd's svn import, or a fast-export of
linux.git. But nothing that would be reasonable to run as
part of the normal test suite.

We can fix this easily by iterating over the lru linked list
instead of the hashmap. They both contain the same entries,
and we can use the "safe" variant of the list iterator,
which exists for exactly this case.

Let's also add a warning to the hashmap API documentation to
reduce the chances of getting bit by this again.

Reported-by: Ulrich Spörlein 
Signed-off-by: Jeff King 
---
 Documentation/technical/api-hashmap.txt | 4 +++-
 sha1_file.c | 9 -
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/technical/api-hashmap.txt 
b/Documentation/technical/api-hashmap.txt
index 28f5a8b71..a3f020cd9 100644
--- a/Documentation/technical/api-hashmap.txt
+++ b/Documentation/technical/api-hashmap.txt
@@ -188,7 +188,9 @@ Returns the removed entry, or NULL if not found.
 `void *hashmap_iter_next(struct hashmap_iter *iter)`::
 `void *hashmap_iter_first(struct hashmap *map, struct hashmap_iter *iter)`::
 
-   Used to iterate over all entries of a hashmap.
+   Used to iterate over all entries of a hashmap. Note that it is
+   not safe to add or remove entries to the hashmap while
+   iterating.
 +
 `hashmap_iter_init` initializes a `hashmap_iter` structure.
 +
diff --git a/sha1_file.c b/sha1_file.c
index 1eb47f611..d20714d6b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2342,11 +2342,10 @@ static inline void release_delta_base_cache(struct 
delta_base_cache_entry *ent)
 
 void clear_delta_base_cache(void)
 {
-   struct hashmap_iter iter;
-   struct delta_base_cache_entry *entry;
-   for (entry = hashmap_iter_first(_base_cache, );
-entry;
-entry = hashmap_iter_next()) {
+   struct list_head *lru, *tmp;
+   list_for_each_safe(lru, tmp, _base_cache_lru) {
+   struct delta_base_cache_entry *entry =
+   list_entry(lru, struct delta_base_cache_entry, lru);
release_delta_base_cache(entry);
}
 }
-- 
2.11.0.747.g2c5918130

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Junio,

On Wed, 18 Jan 2017, Junio C Hamano wrote:

> Johannes Sixt  writes:
> 
> > Am 17.01.2017 um 20:17 schrieb Junio C Hamano:
> >> So... can we move this forward?
> >
> > I have no objects anymore.
> 
> Alright, thanks.  
> 
> Dscho what's your assessment?

I still think it will be a problem. I'll address that problem when I get
bug reports, to unblock you.

Ciao,
Johannes

Re: [RFC] stash --continue

[Johannes Schindelin]

Hi Stephan,

On Wed, 18 Jan 2017, Stephan Beyer wrote:

> PPS: Any opinions about the mentioned "backwards-compatibility" issue
> that people are then forced to finish their commits with "--continue"
> instead of "git reset" or "git commit"?

Maybe you could make it so that "git reset" and "git commit" would still
work as before, and reset the state so that "git stash pop --continue"
could complain that there is nothing to continue?

Similar to how we remove CHERRY_PICK_HEAD during a `git reset --hard`,
maybe?

Ciao,
Johannes

Re: [RFC] stash --continue

[Johannes Schindelin]

Hi Marc,

On Wed, 18 Jan 2017, Marc Branchaud wrote:

> On 2017-01-18 11:34 AM, Johannes Schindelin wrote:
> >
> > On Wed, 18 Jan 2017, Marc Branchaud wrote:
> >
> > > On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
> > >
> > > > On Mon, 16 Jan 2017, Stephan Beyer wrote:
> > > >
> > > > > a git-newbie-ish co-worker uses git-stash sometimes. Last time
> > > > > he used "git stash pop", he got into a merge conflict. After he
> > > > > resolved the conflict, he did not know what to do to get the
> > > > > repository into the wanted state. In his case, it was only "git
> > > > > add " followed by a "git reset" and a "git stash
> > > > > drop", but there may be more involved cases when your index is
> > > > > not clean before "git stash pop" and you want to have your index
> > > > > as before.
> > > > >
> > > > > This led to the idea to have something like "git stash
> > > > > --continue"[1]
> > > >
> > > > More like "git stash pop --continue". Without the "pop" command,
> > > > it does not make too much sense.
> > >
> > > Why not?  git should be able to remember what stash command created
> > > the conflict.  Why should I have to?  Maybe the fire alarm goes off
> > > right when I run the stash command, and by the time I get back to it
> > > I can't remember which operation I did.  It would be nice to be able
> > > to tell git to "just finish off (or abort) the stash operation,
> > > whatever it was".
> >
> > That reeks of a big potential for confusion.
> >
> > Imagine for example a total Git noob who calls `git stash list`,
> > scrolls two pages down, then hits `q` by mistake. How would you
> > explain to that user that `git stash --continue` does not continue
> > showing the list at the third page?
> 
> Sorry, but I have trouble taking that example seriously.  It assumes
> such a level of "noobness" that the user doesn't even understand how
> standard command output paging works, not just with git but with any
> shell command.

Yeah, well, I thought you understood what I meant.

The example was the best I could come up with quickly, and it only tried
to show that there are *other* stash operations that one might perceive
to happen at the same time as the "pop" operation, so your flimsical
comment "why not continue the latest operation" may very well be
ambiguous.

And if it is not ambiguous in "stash", it certainly will be in other Git
operations. And therefore, having a DWIM in "stash" to allow "--continue"
without any specific subcommand, but not having it in other Git commands,
is just a very poor user interface design. It is prone to confuse users,
which is always a hallmark of a bad user interface.

Hence my objection to "git stash --continue". No argument in favor of "git
stash --continue" I heard so far comes even close to being convincing.

> > Even worse: `git stash` (without arguments) defaults to the `save`
> > operation, so any user who does not read the documentation (and who
> > does?) would assume that `git stash --continue` *also* implies `save`.
> 
> Like the first example, your user is trying to "continue" a command that
> is already complete.

Says who? You may understand the semantics better than other users, but
who are you to judge?

But that's besides the point.

My point (which you did not quite understand) was that it can be ambiguous
what to continue when looking at *all* Git commands. To special-case "git
stash"'s user interface makes things more confusing, and therefore less
usable for everyone.

And even with `git stash apply`, you could construct a very plausible
scenario (which does not work yet, but we may want to make it work): if
`git stash apply` causes conflicts, and `git stash apply stash@{1}`
conflicts in a *different* set of files, why don't we allow the second
operation to succeed (adding its conflicts)?

That example is like `git cherry-pick -n` with two different commits, both
of which conflict with the current worktree, but in different files. Both
cherry-picks would do their job if called after one another, and the
result is a worktree with the *combined* conflicts. That is a legitimate
use case (which I happened to *actually* perform just the other day).

If we fix "git stash" (and there is no reason we should not), it would
also allow "git stash pop; git stash pop" to work with two stashes that
both conflict with the current worktree, just in different files.

So I challenge you to get less hung up on the *exact* example I present,
and to try to see through the example what the issue is that I am trying
to get at.

> > If that was not enough, there would still be the overall design of
> > Git's user interface. You can call it confusing, inconsistent, with a
> > lot of room for improvement, and you would be correct. But none of
> > Git's commands has a `--continue` option that remembers the latest
> > subcommand and continues that. To introduce that behavior in `git
> > stash` would disimprove the situation.
> 
> I think it's more the case that none of the 

[RFC 1/2] grep: only add delimiter if there isn't one already

[Stefan Hajnoczi]

git-grep(1) output does not follow git's own syntax:

  $ git grep malloc v2.9.3:
  v2.9.3::Makefile:   COMPAT_CFLAGS += -Icompat/nedmalloc
  $ git show v2.9.3::Makefile
  fatal: Path ':Makefile' does not exist in 'v2.9.3'

This patch avoids emitting the unnecessary ':' delimiter if the name
already ends with ':' or '/':

  $ git grep malloc v2.9.3:
  v2.9.3:Makefile:   COMPAT_CFLAGS += -Icompat/nedmalloc
  $ git show v2.9.3:Makefile
  (succeeds)

Signed-off-by: Stefan Hajnoczi 
---
 builtin/grep.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 462e607..3643d8a 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -490,7 +490,11 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
strbuf_init(, PATH_MAX + len + 1);
if (len) {
strbuf_add(, name, len);
-   strbuf_addch(, ':');
+
+   /* Add a delimiter if there isn't one already */
+   if (name[len - 1] != '/' && name[len - 1] != ':') {
+   strbuf_addch(, ':');
+   }
}
init_tree_desc(, data, size);
hit = grep_tree(opt, pathspec, , , base.len,
-- 
2.9.3

[RFC 2/2] grep: use '/' delimiter for paths

[Stefan Hajnoczi]

If the tree contains a sub-directory then git-grep(1) output contains a
colon character instead of a path separator:

  $ git grep malloc v2.9.3:t
  v2.9.3:t:test-lib.sh: setup_malloc_check () {
  $ git show v2.9.3:t:test-lib.sh
  fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'

This patch attempts to use the correct delimiter:

  $ git grep malloc v2.9.3:t
  v2.9.3:t/test-lib.sh: setup_malloc_check () {
  $ git show v2.9.3:t/test-lib.sh
  (success)

This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
it as a path because it contains colons.

Signed-off-by: Stefan Hajnoczi 
---
 builtin/grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 3643d8a..06f8b47 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct 
pathspec *pathspec,
 
/* Add a delimiter if there isn't one already */
if (name[len - 1] != '/' && name[len - 1] != ':') {
-   strbuf_addch(, ':');
+   /* rev: or rev:path/ */
+   strbuf_addch(, strchr(name, ':') ? '/' : 
':');
}
}
init_tree_desc(, data, size);
-- 
2.9.3

[RFC 0/2] grep: make output consistent with revision syntax

[Stefan Hajnoczi]

git-grep(1)'s output is not consistent with git-rev-parse(1) revision syntax.

This means you cannot take "rev:path/to/file.c: foo();" output from git-grep(1)
and expect "git show rev:path/to/file.c" to work.  See the individual patches
for examples of command-lines that produce invalid output.

This series is an incomplete attempt at solving the issue.  I'm not familiar
enough with the git codebase to propose a better solution.  Perhaps someone is
interested in a proper fix?

Stefan Hajnoczi (2):
  grep: only add delimiter if there isn't one already
  grep: use '/' delimiter for paths

 builtin/grep.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.9.3

Re: The design of refs backends, linked worktrees and submodules

[Michael Haggerty]

On 01/19/2017 12:55 PM, Duy Nguyen wrote:
> I've started working on fixing the "git gc" issue with multiple
> worktrees, which brings me back to this. Just some thoughts. Comments
> are really appreciated.
> 
> In the current code, files backend has special cases for both
> submodules (explicitly) and linked worktrees (hidden behind git_path).

There is another terrible hack also needed to implement linked
worktrees, namely that the `refs/bisect/` hierarchy is manually inserted
into the `ref_cache`, because otherwise it wouldn't be noticed when
iterating over loose references via `readdir()`.

Other similar hacks would be required if other reference subtrees are
declared to be per-worktree.

> But if a backend has to handle this stuff, all future backends have to
> too. Which does not sound great. Imagine we have "something" in
> addition to worktrees and submodules in future, then all backends have
> to learn about it.

Agreed, the status quo is not pretty.

I kindof think that it would have been a better design to store the
references for all linked worktrees in the main repository's ref-store.
For example, the "bisect" refs for a worktree named "" could have
been stored under "refs/worktrees//bisect/*". Then either:

* teach the associated tools to read/write references there directly
(probably with DWIM rules to make command-line use easier), or
* treat these references as if they were actually at a standard place
like `refs/worktree/bisect/*`; i.e., users would need to know that they
were per-worktree references, but wouldn't need to worry about the true
locations, or
* treat these references as if they were actually in their traditional
locations (though it is not obvious how this scheme could be expanded to
cover new per-worktree references).

> So how about we move worktree and submodule support back to front-end?
> 
> The backend deals with refs, period. The file-based backend will be
> given a directory where refs live in and it work on that. Backends do
> not use git_path(). Backends do not care about $GIT_DIR. Access to odb
> (e.g. sha-1 validation) if needed is abstracted out via a set of
> callbacks. This allows submodules to give access to submodule's
> separate odb. And it's getting close to the "struct repository"
> mentioned somewhere in refs "TODO" comments, even though we are
> nowhere close to introducing that struct.

This is a topic that I have thought a lot about. I definitely like this
direction. In fact I've dabbled around with some first steps; see branch
`submodule-hash` in my fork on GitHub [1]. That branch associates a
`ref_store` more closely with the directory where the references are
stored, as opposed to having a 1:1 relationship between `ref_store`s and
submodules.

I would like to see the separation of a concept "iterate over all
reachability roots" that is independent of other ref iteration. Then it
wouldn't have to include reference names, except basically for use in
error messages. So for linked worktrees, in place of the reference name
it might emit a string like ":". (Of course it would
get its information by iterating over all of the linked reference stores
using their reference iteration APIs.)

> For that to work, I'll probably need to add a "composite" ref_store
> that combines two file-based backends (for per-repo and per-worktree
> refs) to represent a unified ref store. I think your work on ref
> iterator makes way for that. A bit worried about transactions though,
> because I think per-repo and per-worktree updates will be separated in
> two transactions. But that's probably ok because future backends, like
> lmdb, will have to go through the same route anyway.

Yes, that was the main motivation for the ref-iterator work.

Regarding transactions, the commit pointed at by branch
`split-transaction` in my fork shows how I think the
`transaction_commit()` method could be split into two parts,
`transaction_prepare()` and `transaction_finish()`. The idea would be
that the driver function, `ref_transaction_commit()`, calls
`transaction_prepare()` for each `ref_store` involved in the
transaction, passing each one the reference updates for references that
live in that reference store. Those methods would verify that the part
of the transaction that lives in that ref-store "should" go through,
without actually committing anything. Then `transaction_finish()` would
be called on each ref store, and that method would commit the updates.
You probably couldn't get a bulletproof kind of compound transaction out
of this (e.g., if the computer's power goes out, one `ref_store`'s
updates might be committed but another one's not). But it would probably
be good enough to cover everyday reasons for transaction failures, like
pre-checksums not matching up.

Let me braindump some more information about this topic. A files backend
for a repository (ignoring submodules for the moment) currently consists
of five interacting parts, each of which looks a lot like a ref-store
itself:

Re: merge maintaining history

[David J. Bakeman]

On 01/14/2017 10:24 PM, Jacob Keller wrote:
> On Fri, Jan 13, 2017 at 6:01 PM, David J. Bakeman  wrote:
>> History
>>
>> git cloned a remote repository and made many changes pushing them all to
>> said repository over many months.
>>
>> The powers that be then required me to move project to new repository
>> server did so by pushing local version to new remote saving all history!
>>
>> Now have to merge back to original repository(which has undergone many
>> changes since I split off) but how do I do that without loosing the
>> history of all the commits since the original move?  Note I need to push
>> changes to files that are already in existence.  I found on the web a
>> bunch of ways to insert a whole new directory structure into an existing
>> repository but as I said I need to do it on top of existing files.  Of
>> course I can copy all the files from my local working repository to the
>> cloned remote repository and commit any changes but I loose all the
>> history that way.
>>
>> Thanks.
> If I understand it.. you have two remotes now:
>
> The "origin" remote, which was the original remote you started with.
>
> You have now a "new" remote which you created and pushed to.
>
> So you want to merge the "new" history into the original tree now, so
> you checkout the original tree, then "git merge /"
> and then fix up any conflicts, and then git commit to create a merge
> commit that has the new history. Then you could push that to both
> trees.
>
> I would want a bit more information about your setup before providing
> actual commands.
Thanks I think that's close but it's a little more complicated I think
:<(  I don't know if this diagram will work but lets try.

original A->B->C->D->E->F
 \
first branch  b->c->d->e

new repo e->f->g->h

Now I need to merge h to F without loosing b through h hopefully.  Yes e
was never merged back to the original repo and it's essentially gone now
so I can't just merge to F or can I?
 
>
> Thanks,
> Jake
>

<>

[PATCH] log: new option decorate reflog of remote refs

[Nguyễn Thái Ngọc Duy]

This is most useful when you fork your branches off a remote ref and
rely on ref decoration to show your fork points in `git log`. Then you
do a "git fetch" and suddenly the remote decoration is gone because
remote refs are moved forward. With this, we can still see something
like "origin/foo@{1}"

This is for remote refs only because based on my experience, docorating
local reflog is just too noisy. You will most likely see HEAD@{1},
HEAD@{2} and so on. We can add that as a separate option in future if we
see a need for it.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I've been using this for many weeks and it has proven its usefulness
 (to me). Looks like good material to send upstream.

 Documentation/git-log.txt |  5 +
 builtin/log.c | 10 +-
 log-tree.c| 43 +++
 log-tree.h|  2 +-
 pretty.c  |  4 ++--
 revision.c|  2 +-
 6 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 32246fd..f5ee575 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -38,6 +38,11 @@ OPTIONS
are shown as if 'short' were given, otherwise no ref names are
shown. The default option is 'short'.
 
+--decorate-remote-reflog[=]::
+   Decorate `` most recent reflog entries on remote refs, up
+   to the specified number of entries. By default, only the most
+   recent reflog entry is decorated.
+
 --source::
Print out the ref name given on the command line by which each
commit was reached.
diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc..c208703 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -36,6 +36,7 @@ static int default_follow;
 static int default_show_signature;
 static int decoration_style;
 static int decoration_given;
+static int decorate_remote_reflog;
 static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static const char *fmt_pretty;
@@ -141,6 +142,10 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
OPT_BOOL(0, "use-mailmap", , N_("Use mail map file")),
{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, N_("decorate 
options"),
  PARSE_OPT_OPTARG, decorate_callback},
+   { OPTION_INTEGER, 0, "decorate-remote-reflog",
+ _remote_reflog, N_("n"),
+ N_("decorate the last  reflog entries of remote refs"),
+ PARSE_OPT_OPTARG | PARSE_OPT_NONEG, NULL, 1 },
OPT_CALLBACK('L', NULL, _cb, "n,m:file",
 N_("Process line range n,m in file, counting from 
1"),
 log_line_range_callback),
@@ -195,9 +200,12 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
rev->abbrev_commit = 0;
}
 
+   if (decorate_remote_reflog > 0 && !decoration_style)
+   decoration_style = DECORATE_SHORT_REFS;
if (decoration_style) {
rev->show_decorations = 1;
-   load_ref_decorations(decoration_style);
+   load_ref_decorations(decoration_style,
+decorate_remote_reflog);
}
 
if (rev->line_level_traverse)
diff --git a/log-tree.c b/log-tree.c
index 8c24157..3d85ebc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -88,14 +88,37 @@ const struct name_decoration *get_name_decoration(const 
struct object *obj)
return lookup_decoration(_decoration, obj);
 }
 
+struct reflog_cb {
+   int type;
+   int count;
+   int nth;
+   const char *refname;
+};
+
+static int add_nth_reflog(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
+{
+   struct reflog_cb *cb = cb_data;
+   struct commit *commit;
+
+   commit = lookup_commit(nsha1);
+   if (commit && cb->nth) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_addf(, "%s@{%d}", cb->refname, cb->nth);
+   add_name_decoration(cb->type, sb.buf, >object);
+   strbuf_release();
+   }
+   cb->nth++;
+   return cb->nth >= cb->count;
+}
+
 static int add_ref_decoration(const char *refname, const struct object_id *oid,
  int flags, void *cb_data)
 {
struct object *obj;
enum decoration_type type = DECORATION_NONE;
 
-   assert(cb_data == NULL);
-
if (starts_with(refname, git_replace_ref_base)) {
struct object_id original_oid;
if (!check_replace_refs)
@@ -135,6 +158,17 @@ static int add_ref_decoration(const char *refname, const 
struct object_id *oid,
parse_object(obj->oid.hash);

Re: [PATCH v3 14/21] read-cache: touch shared index files when used

[Duy Nguyen]

On Mon, Jan 9, 2017 at 9:34 PM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> On Sun, Jan 8, 2017 at 4:46 AM, Junio C Hamano  wrote:
>>> Christian Couder  writes:
>>>
 So what should we do if freshen_file() returns 0 which means that the
 freshening failed?
>>>
>>> You tell me ;-)  as you are the one who is proposing this feature.
>>
>> My answer is, we are not worse than freshening loose objects case
>> (especially since I took the idea from there).
>
> I do not think so, unfortunately.  Loose object files with stale
> timestamps are not removed as long as objects are still reachable.

But there are plenty of unreachable loose objects, added in index,
then got replaced with new versions. cache-tree can create loose trees
too and it's been run more often, behind user's back, to take
advantage of the shortcut in unpack-trees.

> For the base/shared index file, the timestamp is the only thing that
> protects them from pruning, unless it is serving as the base file
> for the currently active $GIT_DIR/index that is split.
-- 
Duy

Re: [PATCH v5 0/4] Per-worktree config file support

[Duy Nguyen]

On Wed, Jan 11, 2017 at 12:01 AM, Stefan Beller  wrote:
> On Tue, Jan 10, 2017 at 3:25 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> Let's get this rolling again. To refresh your memory because it's half
>> a year since v4 [1], this is about letting each worktree in multi
>> worktree setup has their own config settings. The most prominent ones
>> are core.worktree, used by submodules, and core.sparseCheckout.
>
> Thanks for getting this rolling again.
>
>>
>> This time I'm not touching submodules at all. I'm leaving it in the
>> good hands of "submodule people". All I'm providing is mechanism. How
>> you use it is up to you. So the series benefits sparse checkout users
>> only.
>
> As one of the "submodule people", I have no complaints here.
>
>>
>> Not much has changed from v4, except that the migration to new config
>> layout is done automatically _update_ a config variable with "git
>> config --worktree".
>>
>> I think this one is more or less ready. I have an RFC follow-up patch
>> about core.bare, but that could be handled separately.
>
> I have read through the series and think the design is sound for worktrees
> (though I have little knowledge about them).

Submodules and multi worktrees start to look very similar, the more I
think about it. Well, except that multi worktree does not separate odb
and config files, maybe. And we have already seen both have a need to
share code (like the moving .git dir operation). I suspect I'll learn
more about submodules along the way, and you worktrees ;-)

> Now even further:
>
> So to build on top of this series, I'd like to make submodules usable
> with worktrees (i.e. shared object store, only clone/fetch once and
> all worktrees
> benefit from it), the big question is how to get the underlying data
> model right.
>
> Would a submodule go into the superprojects
>
> .git/worktrees//modules//
>
> or rather
>
> .git/modules/worktrees/
>
> Or both (one of them being a gitlink file pointing at the other?)
>
> I have not made up my mind, as I haven't laid out all cases that are
> relevant here.

I would go with a conservative step first, keep submodules
per-worktree. After it's sorted out. You can change the layout (e.g.
as a config extension). The latter probably has some complication (but
yeah sharing would be a big plus).
-- 
Duy

Re: [PATCH/RFC 5/4] Redefine core.bare in multiple working tree setting

[Duy Nguyen]

Thanks. I'll shelve this for now, maybe sleep on it for a while. The
series is complete without this patch by the way, if you want to pick
it up.

On Fri, Jan 13, 2017 at 6:08 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> With per-worktree configuration in place, core.bare is moved to main
>> worktree's private config file. But it does not really make sense
>> because this is about _repository_. Instead we could leave core.bare in
>> the per-repo config and change/extend its definition from:
>>
>>If true this repository is assumed to be 'bare' and has no working
>>directory associated with it.
>>
>> to
>>
>>If true this repository is assumed to be 'bare' and has no _main_
>>working directory associated with it.
>>
>> In other words, linked worktrees are not covered by core.bare. This
>> definition is the same as before when it comes to single worktree setup.
>
> Up to this point, I think it is not _wrong_ per-se, but it does not
> say anything about secondary worktrees.  Some may have their own
> working tree, others may be bare, and there is no way for programs
> to discover if a particular secondary worktree has or lacks its own
> working tree.
>
> Granted, "git worktree" porcelain may be incapable of creating a
> secondary worktree without a working tree, but I think the
> underlying repository layout still is capable of expressing such a
> secondary worktree.
>
> So there still is something else necessary, I suspect, to make the
> definition complete.  Perhaps core.bare should be set in
> per-worktree configuration for all worktrees including the primary
> one, and made the definition/explanation of core.bare to be
> "definition of this variable, if done, must be done in per-worktree
> config file.  If set to true, the worktree is 'bare' and has no
> working directory associated with it"?  That makes things even more
> equal, as there is truly no "special one" at that point.
>
> I dunno.



-- 
Duy

The design of refs backends, linked worktrees and submodules

[Duy Nguyen]

I've started working on fixing the "git gc" issue with multiple
worktrees, which brings me back to this. Just some thoughts. Comments
are really appreciated.

In the current code, files backend has special cases for both
submodules (explicitly) and linked worktrees (hidden behind git_path).
But if a backend has to handle this stuff, all future backends have to
too. Which does not sound great. Imagine we have "something" in
addition to worktrees and submodules in future, then all backends have
to learn about it.

So how about we move worktree and submodule support back to front-end?

The backend deals with refs, period. The file-based backend will be
given a directory where refs live in and it work on that. Backends do
not use git_path(). Backends do not care about $GIT_DIR. Access to odb
(e.g. sha-1 validation) if needed is abstracted out via a set of
callbacks. This allows submodules to give access to submodule's
separate odb. And it's getting close to the "struct repository"
mentioned somewhere in refs "TODO" comments, even though we are
nowhere close to introducing that struct.

How does that sound? In particular, is there anything wrong, or
unrealistic, with that?

For that to work, I'll probably need to add a "composite" ref_store
that combines two file-based backends (for per-repo and per-worktree
refs) to represent a unified ref store. I think your work on ref
iterator makes way for that. A bit worried about transactions though,
because I think per-repo and per-worktree updates will be separated in
two transactions. But that's probably ok because future backends, like
lmdb, will have to go through the same route anyway.
-- 
Duy

[PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Nguyễn Thái Ngọc Duy]

If you have a 256 colors terminal (or one with true color support), then
the predefined 12 colors seem limited. On the other hand, you don't want
to draw graph lines with every single color in this mode because the two
colors could look extremely similar. This option allows you to hand pick
the colors you want.

Even with standard terminal, if your background color is neither black
or white, then the graph line may match your background and become
hidden. You can exclude your background color (or simply the colors you
hate) with this.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt |  4 
 graph.c  | 42 +++---
 t/t4202-log.sh   | 22 ++
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..33a007b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2003,6 +2003,10 @@ log.follow::
i.e. it cannot be used to follow multiple files and does not work well
on non-linear history.
 
+log.graphColors::
+   A list of colors, separated by commas, that can be used to draw
+   history lines in `git log --graph`.
+
 log.showRoot::
If true, the initial commit will be shown as a big creation event.
This is equivalent to a diff against an empty tree.
diff --git a/graph.c b/graph.c
index dd17201..822d40a 100644
--- a/graph.c
+++ b/graph.c
@@ -4,6 +4,7 @@
 #include "graph.h"
 #include "diff.h"
 #include "revision.h"
+#include "argv-array.h"
 
 /* Internal API */
 
@@ -62,6 +63,26 @@ enum graph_state {
 static const char **column_colors;
 static unsigned short column_colors_max;
 
+static void parse_graph_colors_config(struct argv_array *colors, const char 
*string)
+{
+   const char *end, *start;
+
+   start = string;
+   end = string + strlen(string);
+   while (start < end) {
+   const char *comma = strchrnul(start, ',');
+   char color[COLOR_MAXLEN];
+
+   if (!color_parse_mem(start, comma - start, color))
+   argv_array_push(colors, color);
+   else
+   warning(_("ignore invalid color '%.*s' in 
log.graphColors"),
+   (int)(comma - start), start);
+   start = comma + 1;
+   }
+   argv_array_push(colors, GIT_COLOR_RESET);
+}
+
 void graph_set_column_colors(const char **colors, unsigned short colors_max)
 {
column_colors = colors;
@@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)
 {
struct git_graph *graph = xmalloc(sizeof(struct git_graph));
 
-   if (!column_colors)
-   graph_set_column_colors(column_colors_ansi,
-   column_colors_ansi_max);
+   if (!column_colors) {
+   struct argv_array ansi_colors = {
+   column_colors_ansi,
+   column_colors_ansi_max + 1
+   };
+   struct argv_array *colors = _colors;
+   char *string;
+
+   if (!git_config_get_string("log.graphcolors", )) {
+   static struct argv_array custom_colors = 
ARGV_ARRAY_INIT;
+   argv_array_clear(_colors);
+   parse_graph_colors_config(_colors, string);
+   free(string);
+   colors = _colors;
+   }
+   /* graph_set_column_colors takes a max-index, not a count */
+   graph_set_column_colors(colors->argv, colors->argc - 1);
+   }
 
graph->commit = NULL;
graph->revs = opt;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e2db47c..0aeabed 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' '
test_cmp expect actual
 '
 
+cat > expect.colors <<\EOF
+*   Merge branch 'side'
+|\
+| * side-2
+| * side-1
+* | Second
+* | sixth
+* | fifth
+* | fourth
+|/
+* third
+* second
+* initial
+EOF
+
+test_expect_success 'log --graph with merge with log.graphColors' '
+   test_config log.graphColors ",, blue,invalid-color, cyan, red  , " &&
+   git log --color=always --graph --date-order --pretty=tformat:%s |
+   test_decode_color | sed "s/ *\$//" >actual &&
+   test_cmp expect.colors actual
+'
+
 test_expect_success 'log --raw --graph -m with merge' '
git log --raw --graph --oneline -m master | head -n 500 >actual &&
grep "initial" actual
-- 
2.8.2.524.g6ff3d78

[PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()

[Nguyễn Thái Ngọc Duy]

Normally color_parse_mem() is called from config parser which trims the
leading spaces already. The new caller in the next patch won't. Let's be
tidy and trim leading spaces too (we already trim trailing spaces before
comma).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 color.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/color.c b/color.c
index a9eadd1..7bb4a96 100644
--- a/color.c
+++ b/color.c
@@ -207,10 +207,15 @@ int color_parse_mem(const char *value, int value_len, 
char *dst)
struct color fg = { COLOR_UNSPECIFIED };
struct color bg = { COLOR_UNSPECIFIED };
 
+   while (len > 0 && isspace(*ptr)) {
+   ptr++;
+   len--;
+   }
+
if (!len)
return -1;
 
-   if (!strncasecmp(value, "reset", len)) {
+   if (!strncasecmp(ptr, "reset", len)) {
xsnprintf(dst, end - dst, GIT_COLOR_RESET);
return 0;
}
-- 
2.8.2.524.g6ff3d78

[PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0

[Nguyễn Thái Ngọc Duy]

In this code we want to match the word "reset". If len is zero,
strncasecmp() will return zero and we incorrectly assume it's "reset" as
a result.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 color.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/color.c b/color.c
index 81c2676..a9eadd1 100644
--- a/color.c
+++ b/color.c
@@ -207,6 +207,9 @@ int color_parse_mem(const char *value, int value_len, char 
*dst)
struct color fg = { COLOR_UNSPECIFIED };
struct color bg = { COLOR_UNSPECIFIED };
 
+   if (!len)
+   return -1;
+
if (!strncasecmp(value, "reset", len)) {
xsnprintf(dst, end - dst, GIT_COLOR_RESET);
return 0;
-- 
2.8.2.524.g6ff3d78

[PATCH v5 0/3] nd/log-graph-configurable-colors

[Nguyễn Thái Ngọc Duy]

v5 moves space trimming to color_parse_mem() from read_graph_colors_config,
which is renamed to parse_graph... because the config reading is moved
back to graph_init.

I think it looks better, but we may be pushing the limits of
argv_array's abuse.

Nguyễn Thái Ngọc Duy (3):
  color.c: fix color_parse_mem() with value_len == 0
  color.c: trim leading spaces in color_parse_mem()
  log --graph: customize the graph lines with config log.graphColors

 Documentation/config.txt |  4 
 color.c  | 10 +-
 graph.c  | 42 +++---
 t/t4202-log.sh   | 22 ++
 4 files changed, 74 insertions(+), 4 deletions(-)

-- 
2.8.2.524.g6ff3d78