[PATCH 2/3] subtree: fix "git subtree split --rejoin"

[David Aguilar]

"git merge" in v2.9 prevents merging unrelated histories.

"git subtree split --rejoin" creates unrelated histories when
creating a split repo from a raw sub-directory that did not
originate from an invocation of "git subtree add".

Restore the original behavior by passing --allow-unrelated-histories
when merging subtrees.  This ensures that the synthetic history
created by "git subtree split" can be merged.

Add a test to ensure that this feature works as advertised.

Reported-by: Brett Cundal 
Helped-by: Johannes Schindelin 
Signed-off-by: David Aguilar 
---
This is a "re-implementation" of Brett's original RFC patch.
I preferred adding a new line (rather than modifying the existing line)
so I have no problem signing off on this being a distinct patch
authored by me.

 contrib/subtree/git-subtree.sh |  1 +
 contrib/subtree/t/t7900-subtree.sh | 16 
 2 files changed, 17 insertions(+)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 7a39b30..b567eae 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -662,6 +662,7 @@ cmd_split()
debug "Merging split branch into HEAD..."
latest_old=$(cache_get latest_old)
git merge -s ours \
+   --allow-unrelated-histories \
-m "$(rejoin_msg "$dir" $latest_old $latest_new)" \
$latest_new >&2 || exit $?
fi
diff --git a/contrib/subtree/t/t7900-subtree.sh 
b/contrib/subtree/t/t7900-subtree.sh
index 431a2fe..e179b29 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -347,6 +347,22 @@ test_expect_success 'split sub dir/ with --rejoin' '
  '
 
 next_test
+test_expect_success 'split sub dir/ with --rejoin from scratch' '
+   subtree_test_create_repo "$subtree_test_count" &&
+   test_create_commit "$subtree_test_count" main1 &&
+   (
+   cd "$subtree_test_count" &&
+   mkdir "sub dir" &&
+   echo file >"sub dir"/file &&
+   git add "sub dir/file" &&
+   git commit -m"sub dir file" &&
+   split_hash=$(git subtree split --prefix="sub dir" --rejoin) &&
+   git subtree split --prefix="sub dir" --rejoin &&
+   check_equal "$(last_commit_message)" "Split '\''sub dir/'\'' 
into commit '\''$split_hash'\''"
+   )
+ '
+
+next_test
 test_expect_success 'split sub dir/ with --rejoin and --message' '
subtree_test_create_repo "$subtree_test_count" &&
subtree_test_create_repo "$subtree_test_count/sub proj" &&
-- 
2.9.2.466.g8c6d1f9.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] t7900-subtree.sh: fix quoting and broken && chains

[David Aguilar]

Allow whitespace in arguments to subtree_test_create_repo.
Add missing && chains.

Signed-off-by: David Aguilar 
---
 contrib/subtree/t/t7900-subtree.sh | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/subtree/t/t7900-subtree.sh 
b/contrib/subtree/t/t7900-subtree.sh
index 3bf96a9..431a2fe 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -16,16 +16,16 @@ export TEST_DIRECTORY
 
 subtree_test_create_repo()
 {
-   test_create_repo "$1"
+   test_create_repo "$1" &&
(
-   cd $1
+   cd "$1" &&
git config log.date relative
)
 }
 
 create()
 {
-   echo "$1" >"$1"
+   echo "$1" >"$1" &&
git add "$1"
 }
 
@@ -73,10 +73,10 @@ join_commits()
 test_create_commit() (
repo=$1
commit=$2
-   cd "$repo"
-   mkdir -p $(dirname "$commit") \
+   cd "$repo" &&
+   mkdir -p "$(dirname "$commit")" \
|| error "Could not create directory for commit"
-   echo "$commit" >"$commit"
+   echo "$commit" >"$commit" &&
git add "$commit" || error "Could not add commit"
git commit -m "$commit" || error "Could not commit"
 )
-- 
2.9.2.466.g8c6d1f9.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] subtree: adjust style to match CodingGuidelines

[David Aguilar]

Prefer "test" over "[ ... ]", use double-quotes around variables, break
long lines, and properly indent "case" statements.

Signed-off-by: David Aguilar 
---
 contrib/subtree/git-subtree.sh | 544 ++---
 1 file changed, 341 insertions(+), 203 deletions(-)

diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index b567eae..084c884 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -4,8 +4,9 @@
 #
 # Copyright (C) 2009 Avery Pennarun 
 #
-if [ $# -eq 0 ]; then
-set -- -h
+if test $# -eq 0
+then
+   set -- -h
 fi
 OPTS_SPEC="\
 git subtree add   --prefix= 
@@ -50,87 +51,145 @@ prefix=
 
 debug()
 {
-   if [ -n "$debug" ]; then
-   printf "%s\n" "$*" >&2
+   if test -n "$debug"
+   then
+   printf "%s\n" "$@" >&2
fi
 }
 
 say()
 {
-   if [ -z "$quiet" ]; then
-   printf "%s\n" "$*" >&2
+   if test -z "$quiet"
+   then
+   printf "%s\n" "$@" >&2
fi
 }
 
 progress()
 {
-   if [ -z "$quiet" ]; then
-   printf "%s\r" "$*" >&2
+   if test -z "$quiet"
+   then
+   printf "%s\r" "$@" >&2
fi
 }
 
 assert()
 {
-   if "$@"; then
-   :
-   else
+   if ! "$@"
+   then
die "assertion failed: " "$@"
fi
 }
 
 
-#echo "Options: $*"
-
-while [ $# -gt 0 ]; do
+while test $# -gt 0
+do
opt="$1"
shift
+
case "$opt" in
-   -q) quiet=1 ;;
-   -d) debug=1 ;;
-   --annotate) annotate="$1"; shift ;;
-   --no-annotate) annotate= ;;
-   -b) branch="$1"; shift ;;
-   -P) prefix="${1%/}"; shift ;;
-   -m) message="$1"; shift ;;
-   --no-prefix) prefix= ;;
-   --onto) onto="$1"; shift ;;
-   --no-onto) onto= ;;
-   --rejoin) rejoin=1 ;;
-   --no-rejoin) rejoin= ;;
-   --ignore-joins) ignore_joins=1 ;;
-   --no-ignore-joins) ignore_joins= ;;
-   --squash) squash=1 ;;
-   --no-squash) squash= ;;
-   --) break ;;
-   *) die "Unexpected option: $opt" ;;
+   -q)
+   quiet=1
+   ;;
+   -d)
+   debug=1
+   ;;
+   --annotate)
+   annotate="$1"
+   shift
+   ;;
+   --no-annotate)
+   annotate=
+   ;;
+   -b)
+   branch="$1"
+   shift
+   ;;
+   -P)
+   prefix="${1%/}"
+   shift
+   ;;
+   -m)
+   message="$1"
+   shift
+   ;;
+   --no-prefix)
+   prefix=
+   ;;
+   --onto)
+   onto="$1"
+   shift
+   ;;
+   --no-onto)
+   onto=
+   ;;
+   --rejoin)
+   rejoin=1
+   ;;
+   --no-rejoin)
+   rejoin=
+   ;;
+   --ignore-joins)
+   ignore_joins=1
+   ;;
+   --no-ignore-joins)
+   ignore_joins=
+   ;;
+   --squash)
+   squash=1
+   ;;
+   --no-squash)
+   squash=
+   ;;
+   --)
+   break
+   ;;
+   *)
+   die "Unexpected option: $opt"
+   ;;
esac
 done
 
 command="$1"
 shift
+
 case "$command" in
-   add|merge|pull) default= ;;
-   split|push) default="--default HEAD" ;;
-   *) die "Unknown command '$command'" ;;
+add|merge|pull)
+   default=
+   ;;
+split|push)
+   default="--default HEAD"
+   ;;
+*)
+   die "Unknown command '$command'"
+   ;;
 esac
 
-if [ -z "$prefix" ]; then
+if test -z "$prefix"
+then
die "You must provide the --prefix option."
 fi
 
 case "$command" in
-   add) [ -e "$prefix" ] && 
-   die "prefix '$prefix' already exists." ;;
-   *)   [ -e "$prefix" ] || 
-   die "'$prefix' does not exist; use 'git subtree add'" ;;
+add)
+   test -e "$prefix" && die "prefix '$prefix' already exists."
+   ;;
+*)
+   test -e "$prefix" ||
+   die "'$prefix' does not exist; use 'git subtree add'"
+   ;;
 esac
 
 dir="$(dirname "$prefix/.")"
 
-if [ "$command" != "pull" -a "$command" != "add" -a "$command" != "push" ]; 
then
+if test "$command" != "pull" &&
+   test "$command" != "add" &&
+   test "$command" != "push"
+then
revs=$(git rev-parse $default --revs-only "$@") || exit $?
dirs="$(git rev-parse --no-revs --no-flags "$@")" || exit $?
-   if [ -n "$dirs" ]; then
+   if test -n "$dirs"
+   then
die "Error: Use --prefix instead of bare filenames."
fi
 fi
@@ -139,22 

Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Torsten Bögershausen]

On 07/25/2016 06:53 PM, Junio C Hamano wrote:

Pranit Bauva  writes:


>>> +enum terms_defined {
>>> +   TERM_BAD = 1,
>>> +   TERM_GOOD = 2,
>>> +   TERM_NEW = 4,
>>> +   TERM_OLD = 8
>>> +};
>>> +

>>
>> What does TERM stand for  ?

The terms (words) used to denote the newer and the older parts of
the history.  Traditionally, as a regression-hunting tool (i.e. it
used to work, where did I break it?), we called older parts of the
history "good" and newer one "bad", but as people gained experience
with the tool, it was found that the pair of words was error-prone
to use for an opposite use case "I do not recall fixing it, but it
seems to have been fixed magically, when did that happen?", and a
more explicit "new" and "old" were introduced.


Thanks for the explanation.
Is there any risk that a more generic term like "TERM_BAD" may collide
with some other definition some day ?

Would it make sense to call it GIT_BISECT_TERM_BAD, GBS_TERM_BAD,
BIS_TERM_BAD or something more unique ?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug] [Git Gui] git-gui commit gpg signing problem

[Peter van der Does]

It seems that git gui does not adhere to the config settings

git config commit.gpgsign true
git config --global user.signingkey 

Commits done through git gui are not signed while done through cli they
are signed

Peter van der Does
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] worktree: add per-worktree config files

[Stefan Beller]

On Wed, Jul 20, 2016 at 10:24 AM, Nguyễn Thái Ngọc Duy
 wrote:
> A new repo extension is added, worktreeConfig. When it is present:
>
>  - Repository config reading by default includes $GIT_DIR/config _and_
>$GIT_DIR/config.worktree. "config" file remains shared in multiple
>worktree setup.
>
>  - The special treatment for core.bare and core.worktree, to stay
>effective only in main worktree, is gone. These config files are
>supposed to be in config.worktree.
>
> This extension is most useful in multiple worktree setup because you
> now have an option to store per-worktree config (which is either
> .git/config.worktree for main worktree, or
> .git/worktrees/xx/config.worktree for linked ones).
>
> This extension can be used in single worktree mode, even though it's
> pretty much useless (but this can happen after you remove all linked
> worktrees and move back to single worktree).
>
> "git config" reads from both "config" and "config.worktree" by default
> (i.e. without either --user, --file...) when this extension is
> present. Default writes still go to "config", not "config.worktree". A
> new option --worktree is added for that (*).
>
> Since a new repo extension is introduced, existing git binaries should
> refuse to access to the repo (both from main and linked worktrees). So
> they will not misread the config file (i.e. skip the config.worktree
> part). They may still accidentally write to the config file anyway if
> they use with "git config --file ".
>
> This design places a bet on the assumption that the majority of config
> variables are shared so it is the default mode. A safer move would be
> default writes go to per-worktree file, so that accidental changes are
> isolated.
>
> (*) "git config --worktree" points back to "config" file when this
> extension is not present so that it works in any setup.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

I like the user facing design, but how am I supposed to use it internally?

Say I want to read a value preferably from the worktree I'd do a
/*
 * maybe I don't even have to set it to 1 as
 * the user is supposed to do that?
 */
repository_format_worktree_config = 1;
git_config_get_{string,bool,int} (... as usual ...)

and if I want to read the value globally I would set the variable to 0
and read? (I would need to restore it, so I'll have a temporary variable
to keep the original value of repository_format_worktree_config)

Thanks,
Stefan


> ---
>  Documentation/config.txt   | 11 -
>  Documentation/git-config.txt   | 26 
>  Documentation/git-worktree.txt | 31 ++
>  Documentation/gitrepository-layout.txt |  8 
>  builtin/config.c   | 18 +++-
>  cache.h|  2 +
>  config.c   |  7 
>  environment.c  |  1 +
>  setup.c|  5 ++-
>  t/t2028-worktree-config.sh (new +x)| 77 
> ++
>  10 files changed, 175 insertions(+), 11 deletions(-)
>  create mode 100755 t/t2028-worktree-config.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 16dc22d..7d64da0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2,8 +2,9 @@ CONFIGURATION FILE
>  --
>
>  The Git configuration file contains a number of variables that affect
> -the Git commands' behavior. The `.git/config` file in each repository
> -is used to store the configuration for that repository, and
> +the Git commands' behavior. The files `.git/config` and optionally
> +`config.worktree` (see `extensions.worktreeConfig` below) are each
> +repository is used to store the configuration for that repository, and
>  `$HOME/.gitconfig` is used to store a per-user configuration as
>  fallback values for the `.git/config` file. The file `/etc/gitconfig`
>  can be used to store a system-wide default configuration.
> @@ -264,6 +265,12 @@ advice.*::
> show directions on how to proceed from the current state.
>  --
>
> +extensions.worktreeConfig::
> +   If set, by default "git config" reads from both "config" and
> +   "config.worktree" file in that order. In multiple working
> +   directory mode, "config" file is shared while
> +   "config.worktree" is per-working directory.
> +
>  core.fileMode::
> Tells Git if the executable bit of files in the working tree
> is to be honored.
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index f163113..9dfdb6a 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -47,13 +47,15 @@ checks or transformations are performed on the value.
>
>  When reading, the values are read from the system, global and
>  repository local configuration files by default, and options
> -`--system`, `--global`, `--local` and 

Re: [PATCH] Documentation, git submodule: Note about --reference

[Stefan Beller]

On Mon, Jul 25, 2016 at 5:43 PM, Junio C Hamano  wrote:
> On Mon, Jul 25, 2016 at 5:38 PM, Stefan Beller  wrote:
>> On Tue, Jul 19, 2016 at 4:45 PM, Stefan Beller  wrote:
>>> Signed-off-by: Stefan Beller 
>>> ---
>>
>> Any comment here?
>
> I personally found it Meh in the sense that those who read
> and followed the previous note would find it adding no new
> information, and to those who don't bother the additional
> note would not help very much because they would not
> understand (and more importantly, would not care) where
> that "this affects ALL submodules" comes from, or why
> that leads to "so it is only useful...".
>
> But it may be just me.

Ok, I guess we can just drop it then.

Thanks,
Stefan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation, git submodule: Note about --reference

[Stefan Beller]

On Tue, Jul 19, 2016 at 4:45 PM, Stefan Beller  wrote:
> Signed-off-by: Stefan Beller 
> ---

Any comment here?

>
>  Is it too obvious?
>  I was approached off list and this was only obvious after some discussion,
>  so I think it is a valid warning.
>
>  On the other hand this might show that we want to get worktree working with
>  submodules.
>
>  Documentation/git-submodule.txt | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index bf3bb37..dcbd460 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -373,6 +373,10 @@ the submodule itself.
>  +
>  *NOTE*: Do *not* use this option unless you have read the note
>  for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
> ++
> +*NOTE*: This gives the same reference to all submodules, so it is only useful
> +if you are tracking different versions of a project in submodules instead
> +of different projects.
>
>  --recursive::
> This option is only valid for foreach, update, status and sync 
> commands.
> --
> 2.9.2.800.g213104a
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation, git submodule: Note about --reference

[Junio C Hamano]

On Mon, Jul 25, 2016 at 5:38 PM, Stefan Beller  wrote:
> On Tue, Jul 19, 2016 at 4:45 PM, Stefan Beller  wrote:
>> Signed-off-by: Stefan Beller 
>> ---
>
> Any comment here?

I personally found it Meh in the sense that those who read
and followed the previous note would find it adding no new
information, and to those who don't bother the additional
note would not help very much because they would not
understand (and more importantly, would not care) where
that "this affects ALL submodules" comes from, or why
that leads to "so it is only useful...".

But it may be just me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH]submodule deinit: remove outdated comment

[Stefan Beller]

Signed-off-by: Stefan Beller 
---

  This is logically part of origin/sb/submodule-deinit-all, but this change
  failed to be there on time.
  
  As I am touching code in the near vincinity, I noticed.
  I do not include this in that series to come as I want to keep that
  series focused, so I send this separately.
  
  Thanks,
  Stefan

 git-submodule.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 42868df..a5e0ad6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -391,8 +391,6 @@ cmd_init()
 #
 # Unregister submodules from .git/config and remove their work tree
 #
-# $@ = requested paths (use '.' to deinit all submodules)
-#
 cmd_deinit()
 {
# parse $args after "submodule ... deinit".
-- 
2.9.2.368.g08bb350

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup

[Stefan Beller]

On Fri, Jul 22, 2016 at 10:42 AM, Duy Nguyen  wrote:
> On Fri, Jul 22, 2016 at 7:25 PM, Stefan Beller  wrote:
>> On Fri, Jul 22, 2016 at 10:09 AM, Duy Nguyen  wrote:
>>>
>>> I just quickly glanced through the rest of this mail because, as a
>>> submodule ignorant, it's just mumbo jumbo to me. But what I see here
>>> is, there may be problems if we choose to share some submodule info,
>>> but I haven't seen any good thing from sharing any submodule info at
>>> all.
>>
>> Okay. :(
>
> Didn't mean to make you feel sad :)

I was using the :( a bit carelessly here. I was quite surprised that you
"haven't seen any good thing from sharing any submodule info at all."

So what is the design philosophy in worktrees? How much independence does
one working tree have?

So here is what I did:
 *  s/git submodule init/git submodule update --init/
 * added a test_pause to the last test on the last line
 * Then:

$ find . |grep da5e6058
./addtest/.git/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066
./addtest/.git/worktrees/super-elsewhere/modules/submod/objects/08/da5e6058267d6be703ae058d173ce38ed53066
./addtest/.git/worktrees/super-elsewhere/modules/submod2/objects/08/da5e6058267d6be703ae058d173ce38ed53066
./.git/objects/08/da5e6058267d6be703ae058d173ce38ed53066

The last entry is the "upstream" for the addtest clone, so that is fine.
However inside the ./addtest/ (and its worktrees, which currently are
embedded in there?) we only want to have one object store for a given
submodule?

>
>> I assume the sharing is beneficial. (As a work-tree ignorant) I thought
>> we had this main work tree, which also holds the repository, whereas
>> the other working trees have a light weight implementation (e.g. just
>> a .git file pointing back to the main working tree/git dir).
>
> The main worktree is special for historical reason. But from the user
> point of view (and even developer's at a certain level) they should be
> treated equally. Think of it like cloning the same repo multiple
> times. Only now you save disk space because there's only one object
> database.

That's what we want for submodules too, see above?

>
>> So in a way my mental model is more like the config sharing here
>> You can configure things in ~/.gitconfig for example that have effects
>> on more than one repo. Similarly you would want to configure things
>> in one repo, that has effect on more than one working tree?
>>
>> And my assumption was to have the repository specific parts be shared,
>> whereas the working tree specific things should not be shared.
>
> I think that's a good assumption. Although I would rather be not
> sharing by default and let the user initiate it when they want to
> share something. Like ~/..gitconfig, we never write anything there
> unless the user asks us to explicitly (with git config --user).
> Accidental share could have negative effect.

Okay, got it.

>
>>> I can imagine long term you may want to just clone a submodule repo
>>> once and share it across worktrees that use it, so maybe it's just me
>>> not seeing things and this may be a step towards that.
>>
>> Just as Junio put it:
>>> I agree that when a top-level superproject has multiple worktrees
>>> these multiple worktrees may want to have the same submodule in
>>> different states, but I'd imagine that they want to share the same
>>> physical repository (i.e. $GIT_DIR/modules/$name of the primary
>>> worktree of the superproject)---is everybody involved in the
>>> discussion share this assumption?
>>
>> I agree with that as well.
>
> Yeah. We have a long way to go though. As I see it, you may need ref
> namespace as well (so they look like separate clones), which has never
> been used on the client side before. Either that or odb alternates...
>
>>> And because I have not heard any bad thing about the new config
>>> design, I'm going to drop submodule patches from this series and focus
>>> on polishing config stuff.
>>
>> Oh, sorry for not focusing on that part. The design of git config --worktree
>> is sound IMO.
>
> This makes me happy (I know other people can still find flaws in it,
> and I'm ok with that). This config split thing has been wrecking my
> brain for a long time, find the the "right" way to do with minimum
> impacts :)

After playing with this series a bit more, I actually like the UI as it is an
easy mental model "submodules behave completely independent".

However in 3/4 you said:

+ - `submodule.*` in current state should not be shared because the
+   information is tied to a particular version of .gitmodules in a
+   working directory.

This is already a problem with say different branches/versions.
That has been solved by duplicating that information to .git/config
as a required step. (I don't like that approach, as it is super confusing
IMHO)

+
+ - `remote.*` added by submodules may be per working directory as
+   well, unless you are sure remotes from all possible 

What's cooking in git.git (Jul 2016, #07; Mon, 25)

[Junio C Hamano]

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

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

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

--
[Graduated to "master"]

* ew/autoconf-pthread (2016-07-18) 1 commit
  (merged to 'next' on 2016-07-19 at 146e249)
 + configure.ac: stronger test for pthread linkage

 Existing autoconf generated test for the need to link with pthread
 library did not check all the functions from pthread libraries;
 recent FreeBSD has some functions in libc but not others, and we
 mistakenly thought linking with libc is enough when it is not.


* jc/doc-diff-filter-exclude (2016-07-14) 1 commit
  (merged to 'next' on 2016-07-19 at 0c8aa92)
 + diff: document diff-filter exclusion

 Belated doc update for a feature added in v1.8.5.


* jc/renormalize-merge-kill-safer-crlf (2016-07-12) 2 commits
  (merged to 'next' on 2016-07-13 at c243dd6)
 + merge: avoid "safer crlf" during recording of merge results
 + convert: unify the "auto" handling of CRLF
 (this branch is tangled with tb/convert-peek-in-index.)

 "git merge" with renormalization did not work well with
 merge-recursive, due to "safer crlf" conversion kicking in when it
 shouldn't.


* jk/push-scrub-url (2016-07-20) 2 commits
  (merged to 'next' on 2016-07-20 at 22bb7ed)
 + t5541: fix url scrubbing test when GPG is not set
  (merged to 'next' on 2016-07-19 at 6ada3f1)
 + push: anonymize URL in status output

 "git fetch http://user:pass@host/repo...; scrubbed the userinfo
 part, but "git push" didn't.


* js/fsck-name-object (2016-07-18) 4 commits
  (merged to 'next' on 2016-07-19 at 6f39d2f)
 + fsck: optionally show more helpful info for broken links
 + fsck: give the error function a chance to see the fsck_options
 + fsck_walk(): optionally name objects on the go
 + fsck: refactor how to describe objects

 When "git fsck" reports a broken link (e.g. a tree object contains
 a blob that does not exist), both containing object and the object
 that is referred to were reported with their 40-hex object names.
 The command learned the "--name-objects" option to show the path to
 the containing object from existing refs (e.g. "HEAD~24^2:file.txt").


* js/ignore-space-at-eol (2016-07-11) 2 commits
  (merged to 'next' on 2016-07-13 at 429dd83)
 + diff: fix a double off-by-one with --ignore-space-at-eol
 + diff: demonstrate a bug with --patience and --ignore-space-at-eol

 An age old bug that caused "git diff --ignore-space-at-eol"
 misbehave has been fixed.


* ls/travis-enable-httpd-tests (2016-07-12) 1 commit
  (merged to 'next' on 2016-07-13 at 06fa12e)
 + travis-ci: enable web server tests t55xx on Linux

 Allow http daemon tests in Travis CI tests.


* mh/blame-worktree (2016-07-18) 2 commits
  (merged to 'next' on 2016-07-19 at 4c39534)
 + t/t8003-blame-corner-cases.sh: Use here documents
 + blame: allow to blame paths freshly added to the index

 "git blame file" allowed the lineage of lines in the uncommitted,
 unadded contents of "file" to be inspected, but it refused when
 "file" did not appear in the current commit.  When "file" was
 created by renaming an existing file (but the change has not been
 committed), this restriction was unnecessarily tight.


* mh/ref-iterators (2016-06-20) 13 commits
  (merged to 'next' on 2016-07-13 at a5b4e62)
 + for_each_reflog(): reimplement using iterators
 + dir_iterator: new API for iterating over a directory tree
 + for_each_reflog(): don't abort for bad references
 + do_for_each_ref(): reimplement using reference iteration
 + refs: introduce an iterator interface
 + ref_resolves_to_object(): new function
 + entry_resolves_to_object(): rename function from ref_resolves_to_object()
 + get_ref_cache(): only create an instance if there is a submodule
 + remote rm: handle symbolic refs correctly
 + delete_refs(): add a flags argument
 + refs: use name "prefix" consistently
 + do_for_each_ref(): move docstring to the header file
 + refs: remove unnecessary "extern" keywords
 (this branch is used by mh/ref-store; uses mh/split-under-lock; is tangled 
with mh/update-ref-errors.)

 The API to iterate over all the refs (i.e. for_each_ref(), etc.)
 has been revamped.


* mh/split-under-lock (2016-06-13) 33 commits
  (merged to 'next' on 2016-07-13 at aa598af)
 + lock_ref_sha1_basic(): only handle REF_NODEREF mode
 + commit_ref_update(): remove the flags parameter
 + lock_ref_for_update(): don't resolve symrefs
 + lock_ref_for_update(): don't re-read non-symbolic references
 + refs: resolve symbolic refs first
 + ref_transaction_update(): check refname_is_safe() at a minimum
 + unlock_ref(): move definition higher in the file
 + lock_ref_for_update(): new function
 

Re: [PATCH v2 7/8] status: update git-status.txt for --porcelain=v2

[Jakub Narębski]

W dniu 2016-07-25 o 21:25, Jeff Hostetler pisze:

> +Porcelain Format Version 2
> +~~
> +
> +Version 2 format adds more detailed information about the state of
> +the worktree and the changed items.

I think it should be "and changed items", but I am not a native speaker.

> +If `--branch` is given, a header line showing branch tracking information
> +is printed.  This line begins with "### branch: ".  Fields are separated
> +by a single space.
> +
> +FieldMeaning
> +
> + | (initial)Current commit
> + | (detached)Current branch
> +   Upstream branch, if set
> ++ Ahead count, if upstream present
> +-Behind count, if upstream present
> +
> +
> +A series of lines are then displayed for the tracked entries.
> +Ordinary changed entries have the following format; the first
> +character is a 'c' to distinguish them from unmerged entries.

It would be nice (though not necessary) to have an example, either
here or at the end.

> +
> +cR [\t]
> +
> +Field   Meaning
> +
> +A 2 character field containing the staged and
> +unstaged XY values described in the short format,
> +with unchanged indicated by a "." rather than
> +a space.
> +   A 4 character field describing the submodule state.
> +"N..." when the entry is not a submodule.
> +"S" when the entry is a submodule.
> + is "C" if the commit changed; otherwise ".".
> + is "M" if it has tracked changes; otherwise ".".
> + is "U" if there are untracked changes; otherwise ".".
> +The 6 character octal file modes for head, index,
> +and worktree.

I think it might be more readable to be explicit: "for HEAD (),
index (), and worktree ()."

> +The head and index SHA1 values.
> +R   The rename percentage score.

I assume this would be C copy detection heuristics percentage
score in case of copy detection, and B break percentage score
in case of breaking change into addition and deletion of file.
Or am I confused?


> +  The current pathname. It is C-Quoted if it contains
> +special control characters.

I assume that "\t" tab character between  and  is here
to be able to not C-Quote sane filenames with internal whitespace,
isn't it?

> +   The original path. This is only present for staged renames.
> +It is C-Quoted if necessary.

I assume that "C-Quoted if necessary" is the same as "C-Quoted if
it contains special control characters"; also: '"' quote character,
'\' backlash escape character and "\t" horizontal tab are not control
characters per se., but still need C-Quoting.  The rules are the same
as for the rest of Git, e.g. for `git diff`, isn't it?

> +
> +
> +Unmerged entries have the following format; the first character is
> +a "u" to distinguish from ordinary changed entries.
> +
> +u 
> +
> +Field   Meaning
> +
> +A 2 character field describing the conflict type
> +as described in the short format.
> +   A 4 character field describing the submodule state
> +as described above.
> +The 6 character octal file modes for the stage 1,
> +stage 2, stage 3, and worktree.

Errr... the pattern has only _3_ character octal modes,   .
A question: what happens during octopus merge?

> +For regular entries, these are the head, index, and
> +worktree modes; the fourth is zero.

This is remnant of the previous version of "v2" format, isn't it?

> +The stage 1, stage 2, and stage 3 SHA1 values.
> +  The current pathname. It is C-Quoted if necessary.
> +
> +
> +A series of lines are then displayed for untracked and ignored entries.
> +
> + 
> +
> +Where  is "?" for untracked entries and "!" for ignored entries.

I assume that here also  is C-Quoted if necessary.

> +
> +When the `-z` option is given, a NUL (zero) byte follows each pathname;
> +serving as both a separator and line termination. No pathname quoting
> +or backslash escaping is performed. All fields are output in the same
> +order.
> +
>  CONFIGURATION
>  -
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/16] Report bugs consistently

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jul 25, 2016 at 02:44:25PM -0700, Junio C Hamano wrote:
>
>> > diff --git a/imap-send.c b/imap-send.c
>> > index db0fafe..67d67f8 100644
>> > --- a/imap-send.c
>> > +++ b/imap-send.c
>> > @@ -506,12 +506,12 @@ static char *next_arg(char **s)
>> >  
>> >  static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
>> >  {
>> > -  int ret;
>> > +  int ret = -1;
>> >va_list va;
>> >  
>> >va_start(va, fmt);
>> >if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= 
>> > (unsigned)blen)
>> > -  die("Fatal: buffer too small. Please report a bug.");
>> > +  die("BUG: buffer too small (%d < %d)", ret, blen);
>> >va_end(va);
>> >return ret;
>> >  }
>> 
>> If "you gave me this size but you need at least this much" is truly
>> worth reporting, then this is misleading (ret is shown as -1 but you
>> do not even know how much is necessary).  In any case, this should
>> be done as a separate step anyway.
>
> Hrm, isn't "ret" going to be the necessary size? According to the
> standard, it should tell us how many bytes were needed, not "-1" (this
> is the "your vsnprintf is broken" case handled by the strbuf code).

Yes.  If blen <= 0, we do not even do vsnprintf() and that is why
Dscho added "int ret = -1" initialization; otherwise his new die()
would end up referencing uninitialized ret.

> I do think the numbers are reversed, though. It should be "blen < ret".

That, too ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[Junio C Hamano]

John Keeping  writes:

> Allow the empty string to stand in for the null SHA-1 when pushing a new
> branch, like we do when deleting branches.
>
> This means that the following command ensures that `new-branch` is
> created on the remote (that is, is must not already exist):
>
>   git push --force-with-lease=new-branch: origin new-branch
>
> Signed-off-by: John Keeping 
> ---
> New in v2.
>
>  Documentation/git-push.txt |  3 ++-
>  remote.c   |  2 ++
>  t/t5533-push-cas.sh| 12 
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index bf7c9a2..927a034 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current 
> value to be
>  the same as the specified value `` (which is allowed to be
>  different from the remote-tracking branch we have for the refname,
>  or we do not even have to have such a remote-tracking branch when
> -this form is used).
> +this form is used).  If `` is the empty string, then the named ref
> +must not already exist.
>  +
>  Note that all forms other than `--force-with-lease=:`
>  that specifies the expected current value of the ref explicitly are
> diff --git a/remote.c b/remote.c
> index a326e4e..af94892 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, 
> const char *arg, int unse
>   entry = add_cas_entry(cas, arg, colon - arg);
>   if (!*colon)
>   entry->use_tracking = 1;
> + else if (!colon[1])
> + memset(entry->expect, 0, sizeof(entry->expect));

hashclr()?

>   else if (get_sha1(colon + 1, entry->expect))
>   return error("cannot parse expected object name '%s'", colon + 
> 1);
>   return 0;
> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index c732012..5e7f6e9 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -191,4 +191,16 @@ test_expect_success 'cover everything with default 
> force-with-lease (allowed)' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> + setup_srcdst_basic &&
> + (
> + cd dst &&
> + git branch branch master &&
> + git push --force-with-lease=branch: origin branch
> + ) &&
> + git ls-remote dst refs/heads/branch >expect &&
> + git ls-remote src refs/heads/branch >actual &&
> + test_cmp expect actual
> +'
> +
>  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

[Jeff King]

On Mon, Jul 25, 2016 at 12:34:04PM +0200, Jan Smets wrote:

> I have always assumed the post-receive hook to be executed whenever a commit
> is "accepted" by the (gitolite) server. That does not seem to be true any
> more.

Generally, yeah, I would expect that to be the case, too.

> Since 9658846 is appears that, when a client bails out, the pre-receive hook
> continues to run and the commit is written to the repository, but no
> post-receive hook is executed. No signal of any kind is received in the
> hook, not even a sig pipe when the post- hook is writing to stdout whilst
> the client has disconnected.

I see. The problem is that cmd_receive_pack() does this:

execute_commands(commands, unpack_status, );
if (pack_lockfile)
unlink_or_warn(pack_lockfile);
if (report_status)
report(commands, unpack_status);
run_receive_hook(commands, "post-receive", 1);
run_update_post_hook(commands);

It reports the status to the client, and _then_ runs the post-receive
hook. But if that reporting fails (either because of an error, or if we
just get SIGPIPE because the client hung up), then we don't actually run
the hooks.

Leaving 9658846 out of it entirely, it is always going to be racy
whether we notice that the client hung up during the pre-receive step.
E.g.:

  - your pre-receive might not write any output, so the muxer has
nothing to write to the other side, and we never notice that the
connection closed until we write the status out in report()

  - if NO_PTHREADS is set, the sideband muxer runs in a sub-process, not
a sub-thread. And thus we don't know of a hangup except by checking
the result of finish_async(), which we never do.

  - the client could hang up just _after_ we've written the pre-receive
output, but before report() is called, so there's nothing to notice
until we're in report()

So I think 9658846 just made that race a bit longer, because it means
that a write error in the sideband muxer during the pre-receive hook
means we return an error via finish_async() rather than unceremoniously
calling exit() from a sub-thread. So we have a longer period during
which we might actually finish off execute_commands() but not make it
out of report().

And the real solution is to make cmd_receive_pack() more robust, and try
harder to run the hooks even when the client hangs up or we have some
other reporting error (because getting data back to the user is only one
use of post-receive hooks; they are also used to queue jobs or do
maintenance).

But that's a bit tricky, as it requires report() to ignore SIGPIPE, and
to stop using write_or_die() or any other functions that can exit (some
of which happen at a lower level). Plus if a client does hangup, we
don't want our hook to die with SIGPIPE either, so we'd want to proxy
the data into /dev/null.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] push: allow pushing new branches with --force-with-lease

[Junio C Hamano]

John Keeping  writes:

> On Mon, Jul 25, 2016 at 10:28:01AM -0700, Junio C Hamano wrote:
>> John Keeping  writes:
>> 
>> > If there is no upstream information for a branch, it is likely that it
>> > is newly created and can safely be pushed under the normal fast-forward
>> > rules.  Relax the --force-with-lease check so that we do not reject
>> > these branches immediately but rather attempt to push them as new
>> > branches, using the null SHA-1 as the expected value.
>> >
>> > In fact, it is already possible to push new branches using the explicit
>> > --force-with-lease=: syntax, so all we do here is make
>> > this behaviour the default if no explicit "expect" value is specified.
>> 
>> I like the loss of an extra field from "struct ref".
>> 
>> I suspect that the if/else cascade in the loop in apply_cas() can
>> also be taught that ':' followed by an empty string asks to check
>> that the target ref does not exist, in order to make it a bit more
>> useful for folks who do not rely on the "use the last observed
>> status of the tracking branch".
>> 
>> That would make the "explicit" test much less cumbersome to read.
>
> Yes, that's nicer and it mirrors the syntax for deleting a remote
> branch.
>
> I've pulled it out as a preparatory step because I like the fact that
> the "explicit" test passes even before the patch that is the main point
> of the series.

Ah, our mails crossed ;-)

Thanks, I'll read these three patches.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 11/16] am -3: use merge_recursive() directly again

[Junio C Hamano]

Johannes Schindelin  writes:

> Note: the code now calls merge_recursive_generic() again. Unlike
> merge_trees() and merge_recursive(), this function returns 0 upon success,
> as most of Git's functions. Therefore, the error value -1 naturally is
> handled correctly, and we do not have to take care of it specifically.

I've finished reading through up to this point and I'd stop for
now.

Some of the patches I didn't look beyond the context presented in
the patches, so it is very possible that I missed leaks caused by
early returns and things like that, but I didn't see anything
glaringly wrong.  Looks very promising.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/16] Report bugs consistently

[Jeff King]

On Mon, Jul 25, 2016 at 02:44:25PM -0700, Junio C Hamano wrote:

> > diff --git a/imap-send.c b/imap-send.c
> > index db0fafe..67d67f8 100644
> > --- a/imap-send.c
> > +++ b/imap-send.c
> > @@ -506,12 +506,12 @@ static char *next_arg(char **s)
> >  
> >  static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
> >  {
> > -   int ret;
> > +   int ret = -1;
> > va_list va;
> >  
> > va_start(va, fmt);
> > if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= 
> > (unsigned)blen)
> > -   die("Fatal: buffer too small. Please report a bug.");
> > +   die("BUG: buffer too small (%d < %d)", ret, blen);
> > va_end(va);
> > return ret;
> >  }
> 
> If "you gave me this size but you need at least this much" is truly
> worth reporting, then this is misleading (ret is shown as -1 but you
> do not even know how much is necessary).  In any case, this should
> be done as a separate step anyway.

Hrm, isn't "ret" going to be the necessary size? According to the
standard, it should tell us how many bytes were needed, not "-1" (this
is the "your vsnprintf is broken" case handled by the strbuf code).

I do think the numbers are reversed, though. It should be "blen < ret".

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: allow pushing new branches with --force-with-lease

[Junio C Hamano]

Junio C Hamano  writes:

> I suspect that the if/else cascade in the loop in apply_cas() can
> also be taught that ':' followed by an empty string asks to check
> that the target ref does not exist, in order to make it a bit more
> useful for folks who do not rely on the "use the last observed
> status of the tracking branch".
>
> That would make the "explicit" test much less cumbersome to read.

In other words, something like this, perhaps?

 remote.c|  2 ++
 t/t5533-push-cas.sh | 12 
 2 files changed, 14 insertions(+)

diff --git a/remote.c b/remote.c
index b35ffd9..55812d8 100644
--- a/remote.c
+++ b/remote.c
@@ -2303,6 +2303,8 @@ int parse_push_cas_option(struct push_cas_option *cas, 
const char *arg, int unse
entry = add_cas_entry(cas, arg, colon - arg);
if (!*colon)
entry->use_tracking = 1;
+   else if (!colon[1])
+   hashclr(entry->expect);
else if (get_sha1(colon + 1, entry->expect))
return error("cannot parse expected object name '%s'", colon + 
1);
return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 4276b1b..04f4636 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -215,6 +215,18 @@ test_expect_success 'new branch covered by 
force-with-lease (explicit)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (less cumbersome)' 
'
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch another master &&
+   git push --force-with-lease=another: origin another
+   ) &&
+   git ls-remote dst refs/heads/another >expect &&
+   git ls-remote src refs/heads/another >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'new branch already exists' '
setup_srcdst_basic &&
(
-- 
2.9.2-629-gdd92683

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pack-objects: break out of want_object loop early

[Jeff King]

On Mon, Jul 25, 2016 at 02:52:24PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >   if (!*found_pack) {
> > ... first find! fill in found pack, etc ...
> >   }
> >   if (exclude)
> > return 1;
> >   if (incremental)
> > return 0;
> >   if (!ignore_packed_keep && !local)
> > break; /* effectively return 1, but I think the break is more clear */
> >   if (local && !p->pack_local)
> > return 0;
> >   if (ignore_packed_keep && p->pack_local && p->pack_keep)
> > return 0;
> >
> > which just bumps it up. I don't think there is a way to make it more
> > elegant, e.g., by only checking ignore_packed_keep once, because we have
> > to distinguish between each condition being set independently, or the
> > case where neither is set.
> >
> > So I stuck the new check at the end, because to me logically it was "can
> > we break out of the loop instead of looking at p->next". But I agree it
> > would be equivalent to place it before the related checks, and I don't
> > mind doing that if you think it's more readable.
> 
> I do not mind too much about having to check two bools twice.  But
> given that the reason why I was confused was because I didn't see
> why we need to pass the two "return 0" conditions at least once
> before we decide that we do not need the "return 0" thing at all,
> and started constructing a case where this might break by writing
> "Suppose you have two packs, one remote and one local in packed_git
> list in this order, and ..." before I realized that the new "early
> break" can be hoisted up like the above, I definitely feel that "we
> found one, and we aren't conditionally pretending that this thing
> does not need to be packed at all, so return early and say we want
> to pack it" is easier to understand before the two existing "if"
> statements.

Ah, right. Now you had me second-guessing for a moment that there might
be a bad case in hoisting it up where we would want to return 0 but
would break out early to the "return 1".

But it cannot be the case, because the break is mutually exclusive with
the two conditions.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 06/16] merge_recursive: abort properly upon errors

[Junio C Hamano]

Johannes Schindelin  writes:

> There are a couple of places where return values indicating errors
> are ignored. Let's teach them manners.

That is because the return value never indicated errors before this
series, isn't it?  A true error used to be expressed by dying, and
the return value indicating "cleanliness" of the merge were
deliberately ignored.

The world order changed by previous patches in this series and the
callers need to be updated to take the new kind of return values
into account.  That is not teaching them manners ;-)

> Signed-off-by: Johannes Schindelin 
> ---
>  merge-recursive.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index dc3182b..2d4cb80 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1949,8 +1949,9 @@ int merge_recursive(struct merge_options *o,
>   saved_b2 = o->branch2;
>   o->branch1 = "Temporary merge branch 1";
>   o->branch2 = "Temporary merge branch 2";
> - merge_recursive(o, merged_common_ancestors, iter->item,
> - NULL, _common_ancestors);
> + if (merge_recursive(o, merged_common_ancestors, iter->item,
> + NULL, _common_ancestors) < 0)
> + return -1;
>   o->branch1 = saved_b1;
>   o->branch2 = saved_b2;
>   o->call_depth--;

This hunk feels somewhat wrong as-is.

There is a comment before the pre-context explaining why cleanness
flag is ignored.  It needs to be updated.  We still do not care
about cleanliness, i.e. 0=clean, 1=merged with conflict, but we now
can get negative values so we need to reject and return early if
this call indicates an error.

Thee other two hunks make sense.

Thanks.

> @@ -1966,6 +1967,8 @@ int merge_recursive(struct merge_options *o,
>   o->ancestor = "merged common ancestors";
>   clean = merge_trees(o, h1->tree, h2->tree, 
> merged_common_ancestors->tree,
>   );
> + if (clean < 0)
> + return clean;
>  
>   if (o->call_depth) {
>   *result = make_virtual_commit(mrtree, "merged tree");
> @@ -2022,6 +2025,9 @@ int merge_recursive_generic(struct merge_options *o,
>   hold_locked_index(lock, 1);
>   clean = merge_recursive(o, head_commit, next_commit, ca,
>   result);
> + if (clean < 0)
> + return clean;
> +
>   if (active_cache_changed &&
>   write_locked_index(_index, lock, COMMIT_LOCK))
>   return error(_("Unable to write index."));
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pack-objects: break out of want_object loop early

[Jeff King]

On Mon, Jul 25, 2016 at 12:56:23PM -0700, Junio C Hamano wrote:

> > This function loops through each existing packfile, looking
> > for the object. When we find it, we mark the pack/offset
> > combo for later use. However, we can't just return "yes, we
> > want it" at that point. If --honor-pack-keep is in effect,
> > we must keep looking to find it in _all_ packs, to make sure
> > none of them has a .keep. Likewise, if --local is in effect,
> > we must make sure it is not present in any local pack.
> 
> s/any local pack/any non-local pack/, no?

Oops, yeah.

> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index a2f8cfd..55ef5a8 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -981,6 +981,8 @@ static int want_object_in_pack(const unsigned char 
> > *sha1,
> > return 0;
> > if (ignore_packed_keep && p->pack_local && p->pack_keep)
> > return 0;
> > +   if (!ignore_packed_keep && !local)
> > +   break;
> > }
> > }
> 
> OK, so in this loop, we may return "false" (meaning, we do not want
> to pack the object) if "local" (do not pack objects that appear in
> non-local packs) or "ignore_packed_keep" (do not pack objects that
> appear in locally kept packs) are in effect, but if neither of the
> options is set, we know that one of the preconditions ("local" or
> "ignore_packed_keep") for these two "reject by returning false" if
> statements would never trigger for any pack on packed_git list, so
> it is safe to break out and return the one that we have found.

Correct.

> If that is what is going on, I would have expected to see this early
> break before these "we found that this is available in borrowed pack
> and we are only packing local" and "we ignore objects in locally
> kept packs" checks return false.
> 
> Or am I not following the logic in the loop correctly?

Yeah, I think that would work. It has to come after "did we find this in
the pack", obviously. And it has to come after the other unrelated
checks ("are we just finding it to exclude?" and "are we
incremental?"). But you could do:

  if (!*found_pack) {
... first find! fill in found pack, etc ...
  }
  if (exclude)
return 1;
  if (incremental)
return 0;
  if (!ignore_packed_keep && !local)
break; /* effectively return 1, but I think the break is more clear */
  if (local && !p->pack_local)
return 0;
  if (ignore_packed_keep && p->pack_local && p->pack_keep)
return 0;

which just bumps it up. I don't think there is a way to make it more
elegant, e.g., by only checking ignore_packed_keep once, because we have
to distinguish between each condition being set independently, or the
case where neither is set.

So I stuck the new check at the end, because to me logically it was "can
we break out of the loop instead of looking at p->next". But I agree it
would be equivalent to place it before the related checks, and I don't
mind doing that if you think it's more readable.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

On Mon, Jul 25, 2016 at 10:28:01AM -0700, Junio C Hamano wrote:
> John Keeping  writes:
> 
> > If there is no upstream information for a branch, it is likely that it
> > is newly created and can safely be pushed under the normal fast-forward
> > rules.  Relax the --force-with-lease check so that we do not reject
> > these branches immediately but rather attempt to push them as new
> > branches, using the null SHA-1 as the expected value.
> >
> > In fact, it is already possible to push new branches using the explicit
> > --force-with-lease=: syntax, so all we do here is make
> > this behaviour the default if no explicit "expect" value is specified.
> 
> I like the loss of an extra field from "struct ref".
> 
> I suspect that the if/else cascade in the loop in apply_cas() can
> also be taught that ':' followed by an empty string asks to check
> that the target ref does not exist, in order to make it a bit more
> useful for folks who do not rely on the "use the last observed
> status of the tracking branch".
> 
> That would make the "explicit" test much less cumbersome to read.

Yes, that's nicer and it mirrors the syntax for deleting a remote
branch.

I've pulled it out as a preparatory step because I like the fact that
the "explicit" test passes even before the patch that is the main point
of the series.

> > +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> > +   setup_srcdst_basic &&
> > +   (
> > +   cd dst &&
> > +   git branch branch master &&
> > +   git push 
> > --force-with-lease=branch: origin 
> > branch
> > +   ) &&

John Keeping (3):
  Documentation/git-push: fix placeholder formatting
  push: add shorthand for --force-with-lease branch creation
  push: allow pushing new branches with --force-with-lease

 Documentation/git-push.txt |  5 +++--
 remote.c   |  9 +
 remote.h   |  1 -
 t/t5533-push-cas.sh| 38 ++
 4 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] Documentation/git-push: fix placeholder formatting

[John Keeping]

Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping 
---
New in v2.

 Documentation/git-push.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 93c3527..bf7c9a2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -198,7 +198,7 @@ branch we have for it.
 +
 `--force-with-lease=:` will protect the named ref (alone),
 if it is going to be updated, by requiring its current value to be
-the same as the specified value  (which is allowed to be
+the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
 this form is used).
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

[John Keeping]

Allow the empty string to stand in for the null SHA-1 when pushing a new
branch, like we do when deleting branches.

This means that the following command ensures that `new-branch` is
created on the remote (that is, is must not already exist):

git push --force-with-lease=new-branch: origin new-branch

Signed-off-by: John Keeping 
---
New in v2.

 Documentation/git-push.txt |  3 ++-
 remote.c   |  2 ++
 t/t5533-push-cas.sh| 12 
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index bf7c9a2..927a034 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current 
value to be
 the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
-this form is used).
+this form is used).  If `` is the empty string, then the named ref
+must not already exist.
 +
 Note that all forms other than `--force-with-lease=:`
 that specifies the expected current value of the ref explicitly are
diff --git a/remote.c b/remote.c
index a326e4e..af94892 100644
--- a/remote.c
+++ b/remote.c
@@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, 
const char *arg, int unse
entry = add_cas_entry(cas, arg, colon - arg);
if (!*colon)
entry->use_tracking = 1;
+   else if (!colon[1])
+   memset(entry->expect, 0, sizeof(entry->expect));
else if (get_sha1(colon + 1, entry->expect))
return error("cannot parse expected object name '%s'", colon + 
1);
return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..5e7f6e9 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,16 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch: origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=: syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping 
---
Changes in v2:
- The "explicit" test was previously in this patch but is now added in
  patch 2/3.

 remote.c|  7 +++
 remote.h|  1 -
 t/t5533-push-cas.sh | 26 ++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index af94892..20e174d 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int 
send_mirror,
 * branch.
 */
if (ref->expect_old_sha1) {
-   if (ref->expect_old_no_trackback ||
-   oidcmp(>old_oid, >old_oid_expect))
+   if (oidcmp(>old_oid, >old_oid_expect))
reject_reason = REF_STATUS_REJECT_STALE;
else
/* If the ref isn't stale then force the 
update. */
@@ -2345,7 +2344,7 @@ static void apply_cas(struct push_cas_option *cas,
if (!entry->use_tracking)
hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
else if (remote_tracking(remote, ref->name, 
>old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(>old_oid_expect, 0, 
sizeof(ref->old_oid_expect));
return;
}
 
@@ -2355,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
 
ref->expect_old_sha1 = 1;
if (remote_tracking(remote, ref->name, >old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect));
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   expect_old_no_trackback:1,
deletion:1,
matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 5e7f6e9..5f29664 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,6 +191,18 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'new branch covered by force-with-lease (explicit)' '
setup_srcdst_basic &&
(
@@ -203,4 +215,18 @@ test_expect_success 'new branch covered by 
force-with-lease (explicit)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch already exists' '
+   setup_srcdst_basic &&
+   (
+   cd src &&
+   git checkout -b branch master &&
+   test_commit c
+   ) &&
+   (
+   cd dst &&
+   git branch branch master &&
+   test_must_fail git push --force-with-lease=branch origin branch
+   )
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pack-objects: break out of want_object loop early

[Junio C Hamano]

Jeff King  writes:

>   if (!*found_pack) {
> ... first find! fill in found pack, etc ...
>   }
>   if (exclude)
>   return 1;
>   if (incremental)
>   return 0;
>   if (!ignore_packed_keep && !local)
>   break; /* effectively return 1, but I think the break is more clear */
>   if (local && !p->pack_local)
>   return 0;
>   if (ignore_packed_keep && p->pack_local && p->pack_keep)
>   return 0;
>
> which just bumps it up. I don't think there is a way to make it more
> elegant, e.g., by only checking ignore_packed_keep once, because we have
> to distinguish between each condition being set independently, or the
> case where neither is set.
>
> So I stuck the new check at the end, because to me logically it was "can
> we break out of the loop instead of looking at p->next". But I agree it
> would be equivalent to place it before the related checks, and I don't
> mind doing that if you think it's more readable.

I do not mind too much about having to check two bools twice.  But
given that the reason why I was confused was because I didn't see
why we need to pass the two "return 0" conditions at least once
before we decide that we do not need the "return 0" thing at all,
and started constructing a case where this might break by writing
"Suppose you have two packs, one remote and one local in packed_git
list in this order, and ..." before I realized that the new "early
break" can be hoisted up like the above, I definitely feel that "we
found one, and we aren't conditionally pretending that this thing
does not need to be packed at all, so return early and say we want
to pack it" is easier to understand before the two existing "if"
statements.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/16] Report bugs consistently

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/imap-send.c b/imap-send.c
> index db0fafe..67d67f8 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -506,12 +506,12 @@ static char *next_arg(char **s)
>  
>  static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
>  {
> - int ret;
> + int ret = -1;
>   va_list va;
>  
>   va_start(va, fmt);
>   if (blen <= 0 || (unsigned)(ret = vsnprintf(buf, blen, fmt, va)) >= 
> (unsigned)blen)
> - die("Fatal: buffer too small. Please report a bug.");
> + die("BUG: buffer too small (%d < %d)", ret, blen);
>   va_end(va);
>   return ret;
>  }

If "you gave me this size but you need at least this much" is truly
worth reporting, then this is misleading (ret is shown as -1 but you
do not even know how much is necessary).  In any case, this should
be done as a separate step anyway.

All the other hunks looked sensible.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 01/16] Verify that `git pull --rebase` shows the helpful advice when failing

[Junio C Hamano]

Johannes Schindelin  writes:

> +test_expect_success '--rebase with conflicts shows advice' '
> + test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
> + git checkout -b seq &&
> + printf "1\\n2\\n3\\n4\\n5\\n" >seq.txt &&

Make this more readble by using test-write-lines, perhaps?

> + git add seq.txt &&
> + test_tick &&
> + git commit -m "Add seq.txt" &&
> + printf "6\\n" >>seq.txt &&
> + test_tick &&
> + git commit -m "Append to seq.txt" seq.txt &&
> + git checkout -b with-conflicts HEAD^ &&
> + printf "conflicting\\n" >>seq.txt &&
> + test_tick &&
> + git commit -m "Create conflict" seq.txt &&
> + test_must_fail git pull --rebase . seq 2>err >out &&
> + grep "When you have resolved this problem" out
> +'
> +test_expect_success 'failed --rebase shows advice' '

Need a blank line before this one.

> + test_when_finished "git rebase --abort; git checkout -f to-rebase" &&
> + git checkout -b diverging &&
> + test_commit attributes .gitattributes "* text=auto" attrs &&
> + sha1="$(printf "1\\r\\n" | git hash-object -w --stdin)" &&
> + git update-index --cacheinfo 0644 $sha1 file &&
> + git commit -m v1-with-cr &&
> + git checkout -f -b fails-to-rebase HEAD^ &&

It is unclear what the "-f" is for; is it attempting to clean up a
potential mess previous steps might have left?  We didn't have it in
the previous test above.

> + test_commit v2-without-cr file "2" file2-lf &&
> + test_must_fail git pull --rebase . diverging 2>err >out &&
> + grep "When you have resolved this problem" out
> +'
> +
>  test_expect_success '--rebase fails with multiple branches' '
>   git reset --hard before-rebase &&
>   test_must_fail git pull --rebase . copy master 2>err &&

Not worth a reroll but after this series settles we would probably
want to address some of the above up with a follow-up clean-up patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Christian Couder]

On Mon, Jul 25, 2016 at 6:53 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>>> And why are the defines 1,2,4,8 ?
>>> It looks as if a #define bitmap may be a better choice here ?
>>> How do we do these kind of bit-wise opions otherwise ?
>
> We might want to ask if these should even be bitwise option.  A word
> with individually controllable bits (i.e. "flag word") makes sense
> only when the bits within it are largely independent.  But the code
> does this pretty much upfront:
>
 +   if (term_defined != 0 && term_defined != TERM_BAD &&
 +   term_defined != TERM_GOOD && term_defined != TERM_NEW &&
 +   term_defined != TERM_OLD)
 +   die(_("only one option among --term-bad, --term-good, "
 + "--term-new and --term-old can be used."));
>
> which is a very strong indication that these bits are not.
>
> I suspect that OPTION_CMDMODE would be a better choice to group
> these four options and mark them mutually incompatible automatically
> than OPT_BIT?

I must say that Pranit used that at one point, but it felt weird to me
to use that for things that is not really a command.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/8] Add configuration options for split-index

[Christian Couder]

On Mon, Jul 25, 2016 at 6:04 PM, Duy Nguyen  wrote:
>
> Hmm.. can you do the counting separately? A shared cache_entry must
> have its field "index" greater than zero. By counting the number of
> entries whose index is zero (i.e. not shared) against the total number
> of real (*) entries, you should have a decent estimate when to split.
> Then you can do exactly what "git update-index --no-split-index" and
> "git update-index --split-index" sequence does, but in write_index().
> It's easier than messing inside split-index.c. If we hit performance
> problem, then we can look into changing split-index.c

Yeah, thanks for the suggestion. I will try it.

> (*) remember that some entries may be marked CE_REMOVE, which are dead
> entries and should not be counted because they will never be written
> down on disk.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit

[Eric Wong]

Junio C Hamano  wrote:
> Eric Wong  writes:
> 
> >> Also, doesn't it break "git rebase" (non-interactive), or anything
> >> that internally runs format-patch to individual files and then runs
> >> am on each of them, anything that knows that each output file from
> >> format-patch corresponds to a single change and there is no need to
> >> split, badly if we do this unconditionally?
> >
> > Yes, rebase should probably unescape is_from_line matches.
> 
> This shouldn't matter for "git rebase", as it only reads from the
> mbox "From  " line to learn the
> original commit and extract the log message directly from there.

OK.

> But a third-party script that wants to read format-patch output
> would be forced to upgrade, which is a pain if we make this change
> unconditionally.

I suppose unconditionally having mailsplit unescape
">From ...  $DATE" lines might be acceptable?

It'll still propagate these mistakes to older versions of git,
but those installations will eventually become fewer.

> >> IOW, shouldn't this be an optional feature to format-patch that
> >> is triggered by passing a new command line option that currently
> >> nobody is passing?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] config.mak.uname: correct perl path on FreeBSD

[Junio C Hamano]

Eric Wong  writes:

> Junio C Hamano  wrote:
>> Duy Nguyen  writes:
>> 
>> > On Mon, Jul 25, 2016 at 6:56 PM, Junio C Hamano  wrote:
>> >> On Mon, Jul 25, 2016 at 9:21 AM, Nguyễn Thái Ngọc Duy  
>> >> wrote:
>> >>> It looks the the symlink /usr/bin/perl (to /usr/local/bin/perl) has
>> >>> been removed at least on FreeBSD 10.3. See [1] for more information.
>> >>>
>> >>> [1] 
>> >>> https://svnweb.freebsd.org/ports/head/UPDATING?r1=386270=386269=386270_format=c
>
> Ah, I missed that.  I guess that explains why nobody complained
> about the problem sooner.
>
>> >>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> >>> ---
>> >>>  Tested with fbsd 10.3, kvm image. But I suppose it's the same as real
>> >>>  fbsd.
>> >>
>> >> Thanks; and we know that older (but not too old that we no longer care 
>> >> about)
>> >> FreeBSD all have /usr/local/bin/perl?
>> >
>> > I'm no fbsd expert but from the first sentence in [1] "Perl has been
>> > removed from base more than ten years ago..." I would assume it meant
>> > "... removed from base, _to_ ports system" which means /usr/local for
>> > all package installation (for ten years for perl). So I think we are
>> > good.
>> 
>> I guess we didn't follow through
>> 
>> http://public-inbox.org/git/%3C20160720025630.GA71874%40plume%3E/
>> 
>> and allowed the thread to drift into a tangent?
>
> +Cc Dscho
>
> I've been meaning to followup on that, but had connectivity
> problems to my VM last week.  I still prefer we use numeric
> comparisons for version numbers since numbers are... numeric.
> IOW, I prefer we go with my original patch.

I tend to agree with you if we have to do "systems older than this
should use /usr/bin, others should use /usr/local/bin", but this
different incarnation of the same topic seems to claim that older
ones had /usr/local/bin forever anyway, and that was what made the
patch interesting.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] config.mak.uname: correct perl path on FreeBSD

[Eric Wong]

Junio C Hamano  wrote:
> Duy Nguyen  writes:
> 
> > On Mon, Jul 25, 2016 at 6:56 PM, Junio C Hamano  wrote:
> >> On Mon, Jul 25, 2016 at 9:21 AM, Nguyễn Thái Ngọc Duy  
> >> wrote:
> >>> It looks the the symlink /usr/bin/perl (to /usr/local/bin/perl) has
> >>> been removed at least on FreeBSD 10.3. See [1] for more information.
> >>>
> >>> [1] 
> >>> https://svnweb.freebsd.org/ports/head/UPDATING?r1=386270=386269=386270_format=c

Ah, I missed that.  I guess that explains why nobody complained
about the problem sooner.

> >>> Signed-off-by: Nguyễn Thái Ngọc Duy 
> >>> ---
> >>>  Tested with fbsd 10.3, kvm image. But I suppose it's the same as real
> >>>  fbsd.
> >>
> >> Thanks; and we know that older (but not too old that we no longer care 
> >> about)
> >> FreeBSD all have /usr/local/bin/perl?
> >
> > I'm no fbsd expert but from the first sentence in [1] "Perl has been
> > removed from base more than ten years ago..." I would assume it meant
> > "... removed from base, _to_ ports system" which means /usr/local for
> > all package installation (for ten years for perl). So I think we are
> > good.
> 
> I guess we didn't follow through
> 
> http://public-inbox.org/git/%3C20160720025630.GA71874%40plume%3E/
> 
> and allowed the thread to drift into a tangent?

+Cc Dscho

I've been meaning to followup on that, but had connectivity
problems to my VM last week.  I still prefer we use numeric
comparisons for version numbers since numbers are... numeric.
IOW, I prefer we go with my original patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Lars Schneider]

On 25 Jul 2016, at 01:22, Jakub Narębski  wrote:

> W dniu 2016-07-25 o 00:36, Ramsay Jones pisze:
>> On 24/07/16 18:16, Lars Schneider wrote:
>>> On 23 Jul 2016, at 01:19, Ramsay Jones  wrote:
 On 22/07/16 16:49, larsxschnei...@gmail.com wrote:
> [...]
> This patch adds the filter..useProtocol option which, if enabled,
> keeps the external filter process running and processes all blobs with
> the following protocol over stdin/stdout.
> 
> 1. Git starts the filter on first usage and expects a welcome message
> with protocol version number:
>   Git <-- Filter: "git-filter-protocol\n"
>   Git <-- Filter: "version 1"
 
 Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
 interaction is fully defined, I guess it doesn't matter).
>>> 
>>> It was a conscious decision to have the `filter` talk first. My reasoning 
>>> was:
>>> 
>>> (1) I want a reliable way to distinguish the existing filter protocol 
>>> ("single-shot 
>>> invocation") from the new one ("long running"). I don't think there would 
>>> be a
>>> situation where the existing protocol would talk first. Therefore the users 
>>> would
>>> not accidentally mix them with a possibly half working, undetermined, 
>>> outcome.
>> 
>> If an 'single-shot' filter were incorrectly configured, instead of a new 
>> one, then
>> the interaction could last a little while - since it would result in 
>> deadlock! ;-)
>> 
>> [If Git talks first instead, configuring a 'single-shot' filter _may_ still 
>> result
>> in a deadlock - depending on pipe size, etc.]
> 
> Would it be possible to do an equivalent of sending empty file to the filter?
> If it is misconfigured old-style script, it would exit after possibly empty
> output; if not, we would start new-style interaction.

I think we would need to close the pipe to communicate "end" to the filter, no?
I would prefer to define the protocol explicitly as this is clearly easier.


> 
> This should be, if we agree that detecting misconfigured filters is a good
> thing, tested.
> 
>>> 
>>> (2) In the future we could extend the pipe protocol (see $gmane/297994, 
>>> it's very
>>> interesting). A filter could check Git's version and then pick the most 
>>> appropriate
>>> filter protocol on startup.

Thanks,
Lars--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/8] status: print branch info with --porcelain=v2 --branch

[Junio C Hamano]

Jeff Hostetler  writes:

>  /*
> + * Print branch and tracking header for porcelain v2 output.
> + * This is printed when the '--branch' parameter is given.
> + *
> + *"## branch:  [ [ + -]]"

Just FYI, the only reason why the original "short" output gives this
information on a single line with ## is because it was added as an
afterthought to then-existing short/porcelain format that used two
letter prefix and one-line-per-path format, and the expectation was
that the existing parsers for the output would ignore unknown status
letters (namely "##").  Because you are inventing a new format that
needs to be parsed by a brand new parser _anyway_, you do not have
to mimic that old convention which was a workaround, and use a
notation that mixes better with the other lines you show under
the --porcelain=v2 option.

Of course if you like "## branch:" output and think it is pleasing
to see, that is fine as well.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Lars Schneider]

On 25 Jul 2016, at 00:36, Ramsay Jones  wrote:

> On 24/07/16 18:16, Lars Schneider wrote:
>> 
>> On 23 Jul 2016, at 01:19, Ramsay Jones  wrote:
>> 
>>> On 22/07/16 16:49, larsxschnei...@gmail.com wrote:
 From: Lars Schneider 
 
 Git's clean/smudge mechanism invokes an external filter process for every
 single blob that is affected by a filter. If Git filters a lot of blobs
 then the startup time of the external filter processes can become a
 significant part of the overall Git execution time.
 
 This patch adds the filter..useProtocol option which, if enabled,
 keeps the external filter process running and processes all blobs with
 the following protocol over stdin/stdout.
 
 1. Git starts the filter on first usage and expects a welcome message
 with protocol version number:
Git <-- Filter: "git-filter-protocol\n"
Git <-- Filter: "version 1"
>>> 
>>> Hmm, I was a bit surprised to see a 'filter' talk first (but so long as the
>>> interaction is fully defined, I guess it doesn't matter).
>>> 
>>> [If you wanted to check for a version, you could add a "version" command
>>> instead, just like "clean" and "smudge".]
>> 
>> It was a conscious decision to have the `filter` talk first. My reasoning 
>> was:
>> 
>> (1) I want a reliable way to distinguish the existing filter protocol 
>> ("single-shot 
>> invocation") from the new one ("long running"). I don't think there would be 
>> a
>> situation where the existing protocol would talk first. Therefore the users 
>> would
>> not accidentally mix them with a possibly half working, undetermined, 
>> outcome.
> 
> If an 'single-shot' filter were incorrectly configured, instead of a new one, 
> then
> the interaction could last a little while - since it would result in 
> deadlock! ;-)
> 
> [If Git talks first instead, configuring a 'single-shot' filter _may_ still 
> result
> in a deadlock - depending on pipe size, etc.]

Do you think this is an issue that needs to be addressed in the first version?
If yes, I would probably look into "select" to specify a timeout for the filter.
However, wouldn't the current "single-shot" clean/smudge filter block in the 
same way if they don't write anything?


>> (2) In the future we could extend the pipe protocol (see $gmane/297994, it's 
>> very
>> interesting). A filter could check Git's version and then pick the most 
>> appropriate
>> filter protocol on startup.
>> 
>> 
>>> [...]
 +static struct cmd2process *start_protocol_filter(const char *cmd)
 +{
 +  int ret = 1;
 +  struct cmd2process *entry = NULL;
 +  struct child_process *process = NULL;
 +  struct strbuf nbuf = STRBUF_INIT;
 +  struct string_list split = STRING_LIST_INIT_NODUP;
 +  const char *argv[] = { NULL, NULL };
 +  const char *header = "git-filter-protocol\nversion";
 +
 +  entry = xmalloc(sizeof(*entry));
 +  hashmap_entry_init(entry, strhash(cmd));
 +  entry->cmd = cmd;
 +  process = >process;
 +
 +  child_process_init(process);
 +  argv[0] = cmd;
 +  process->argv = argv;
 +  process->use_shell = 1;
 +  process->in = -1;
 +  process->out = -1;
 +
 +  if (start_command(process)) {
 +  error("cannot fork to run external persistent filter '%s'", 
 cmd);
 +  return NULL;
 +  }
 +  strbuf_reset();
 +
 +  sigchain_push(SIGPIPE, SIG_IGN);
 +  ret &= strbuf_read_once(, process->out, 0) > 0;
>>> 
>>> Hmm, how much will be read into nbuf by this single call?
>>> Since strbuf_read_once() makes a single call to xread(), with
>>> a len argument that will probably be 8192, you can not really
>>> tell how much it will read, in general. (xread() does not
>>> guarantee how many bytes it will read.)
>>> 
>>> In particular, it could be less than strlen(header).
>> 
>> As mentioned to Torsten in $gmane/300156, I will add a newline
>> and then read until I find the second newline. That should solve
>> the problem, right?
>> 
>> (You wrote in $gmane/300119 that I should ignore your email but
>> I think you have a valid point here ;-)
> 
> Heh, as I said, it was late and I was trying to do several things
> at once. (I am updating 3 installations of Linux Mint 17.3 to Linux
> Mint 18 - I decided to do a complete re-install, since I needed to
> change partition sizes anyway. I have only just got email back up ...)
> 
> I stopped commenting on the patch early but, after sending the first
> email, I decided to scan the rest of your patch before going to bed
> and noticed something which would invalidate my comments ...
> 
>> 
>> 
 [...]
 +  sigchain_push(SIGPIPE, SIG_IGN);
 +  switch (entry->protocol) {
 +  case 1:
 +  if (fd >= 0 && !src) {
 +  ret &= fstat(fd, ) != -1;
 +  len = 

Re: [PATCH v2 5/8] status: print per-file porcelain v2 status data

[Junio C Hamano]

Jeff Hostetler  writes:

> +static void wt_porcelain_v2_print(struct wt_status *s);
> +

There is no point in this forward declaration, if you just place the
implementation of these functions here, no?

> +/*
> + * Print porcelain v2 info for tracked entries with changes.
> + */
> +static void wt_porcelain_v2_print_changed_entry(
> + struct string_list_item *it,
> + struct wt_status *s)
> +{
> +...
> + fprintf(s->fp, "%c %s %s %06o %06o %06o %s %s R%d %s",

It is misleading to always say R in the output when there is no
rename, isn't it?

> +  * Note that this is a last-one-wins for each the individual
> +  * stage [123] columns in the event of multiple cache rows
> +  * for a stage.

Just FYI, the usual lingo we use for that is "multiple cache entries
for the same stage", I would think.

> +  */
> + memset(stages, 0, sizeof(stages));
> + sum = 0;
> + pos = cache_name_pos(it->string, strlen(it->string));
> + assert(pos < 0);
> + pos = -pos-1;
> + while (pos < active_nr) {
> + ce = active_cache[pos++];
> + stage = ce_stage(ce);
> + if (strcmp(ce->name, it->string) || !stage)
> + break;
> + stages[stage - 1].mode = ce->ce_mode;
> + hashcpy(stages[stage - 1].oid.hash, ce->sha1);
> + sum++;
> + }
> + if (!sum)
> + die("BUG: unmerged entry without any stages");

Hmm, we seem to already have d->stagemask; if you call that variable
"sum" anyway, perhaps its computation can be more like

sum |= 1 << (stage - 1);

so that you can compare it with d->stagemask for this sanity check?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Lars Schneider]

On 24 Jul 2016, at 23:30, Jakub Narębski  wrote:

> W dniu 2016-07-24 o 22:14, Jakub Narębski pisze:
>> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
> 
>>> I agree that the name is not ideal. "UseProtocol" as it is would be a 
>>> boolean. 
>>> I thought about "persistent" but this name wouldn't convey the scope of the 
>>> persistency ("persistent for one Git operation" vs. "persistent for many 
>>> Git 
>>> operations"). What do you think about the protocol as int version idea
>>> described in $gmane/300155 ?
>> 
>> You mean the `protocol` as a config variable name (fully name being
>> `filter..protocol`), being integer-valued, isn't it? Wouldn't
>> `protocolVersion` be a more explicit?
> 
> Just throwing out further ideas:
> 
> Perhaps make `persistent` string-valued variable, with the only value
> supported for now, namely "per-process" / "operation"?
> 
> Perhaps require for `pidfile` to be present for it to be daemon,
> that is persist for possibly many Git operations. Or allow "daemon"
> or "server" value for `persistent`, then?

I like the direction of this idea. What if we use a string-valued 
"filter..protocol" with the following options:

"simple" / "invocation-per-file" / << empty >> --> current clean/smudge behavior
"invocation-per-process" --> new, proposed behavior

If necessary this could be enhanced in the future to support even a "daemon"
mode (with a pidfile config).

Thanks,
Lars

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/8] status: per-file data collection for --porcelain=v2

[Junio C Hamano]

Jeff Hostetler  writes:

> +static void aux_updated_entry_porcelain_v2(
> + struct wt_status *s,
> + struct wt_status_change_data *d,
> + struct diff_filepair *p)
> +{
> + switch (p->status) {
> + case DIFF_STATUS_ADDED:
> + /* {mode,sha1}_head are zero for an add. */
> + d->aux.porcelain_v2.mode_index = p->two->mode;

I doubt that it makes sense in the longer term to have a new "aux"
field.  Why isn't it part of the wt_status_change_data structure?
For that matter, why should these new functions have both "aux" and
"v2" in their names?

Imagine what should happen when somebody wants to add --porcelain=v3
format in 6 months.  Why must "v3" be treated differently from "v1"
and in a way close to "v2"?  Why shouldn't all the three be treated
in a similar way that "v1" has already?

> + oidcpy(>aux.porcelain_v2.oid_index, >two->oid);
> + break;
> +
> + case DIFF_STATUS_DELETED:
> + d->aux.porcelain_v2.mode_head = p->one->mode;
> + oidcpy(>aux.porcelain_v2.oid_head, >one->oid);
> + /* {mode,oid}_index are zero for a delete. */
> + break;
> +
> + case DIFF_STATUS_RENAMED:
> + d->aux.porcelain_v2.rename_score = p->score * 100 / MAX_SCORE;

I have a slight aversion against losing the precision in a helper
function like this that does not do the actual output, but it
probably is OK.

Don't we have copy detection score that is computed exactly the same
way for DIFF_STATUS_COPIED case, too?

For readability, unless a case arm is completely empty, we should
have
/* fallthru */

comment where "break;" would go for a normal case arm.

> + case DIFF_STATUS_COPIED:
> + case DIFF_STATUS_MODIFIED:
> + case DIFF_STATUS_TYPE_CHANGED:
> + case DIFF_STATUS_UNMERGED:
> + d->aux.porcelain_v2.mode_head = p->one->mode;
> + d->aux.porcelain_v2.mode_index = p->two->mode;
> + oidcpy(>aux.porcelain_v2.oid_head, >one->oid);
> + oidcpy(>aux.porcelain_v2.oid_index, >two->oid);
> + break;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Lars Schneider]

On 24 Jul 2016, at 22:14, Jakub Narębski  wrote:

> W dniu 2016-07-24 o 20:36, Lars Schneider pisze:
>> On 23 Jul 2016, at 02:11, Jakub Narębski  wrote:
>>> W dniu 2016-07-22 o 17:49, larsxschnei...@gmail.com pisze:
 From: Lars Schneider 
>>> 
>>> Nb. this line is only needed if you want author name and/or date
>>> different from the email sender, or if you have sender line misconfigured
>>> (e.g. lacking the human readable name).
>> 
>> I use "git format-patch" to generate these emails:
>> 
>> git format-patch --cover-letter --subject-prefix="PATCH ..." -M $BASE -o 
>> $OUTPUT
>> 
>> How would I disable this line? (I already checked the man page to no avail).
> 
> If you are using `git send-email` or equivalent, I think it is
> stripped automatically if it is not needed (in you case it was,
> because Sender was lacking human readable name... at least I think
> it was because of what my email reader inserted as reply line).
> If you are using an ordinary email client, you need to remove it
> yourself, if needed.

Weird. I am sending the patches with this command:

git send-email mystuff/* --to=git@vger.kernel.org --cc=...

Maybe I need to try the "--suppress-from" switch?!


>> Plus, what does "Nb" stand for? :-)
> 
> Nb. (or N.b.) stands for "nota bene", which I meant do denote
> as a note on a certain side aspect; I'll switch to "note", or
> "BTW" / "by the way".

OK, thanks for the explanation :-)


 Git's clean/smudge mechanism invokes an external filter process for every
 single blob that is affected by a filter. If Git filters a lot of blobs
 then the startup time of the external filter processes can become a
 significant part of the overall Git execution time.
>>> 
>>> Do I understand it correctly (from the commit description) that with
>>> this new option you start one filter process for the whole life of
>>> the Git command (e.g. `git add .`), which may perform more than one
>>> cleanup, but do not create a daemon or daemon-like process which
>>> would live for several commands invocations?
>> 
>> Correct!
> 
> It would be nice to make it more obvious.

OK, I will try in v2.

> 
 This patch adds the filter..useProtocol option which, if enabled,
 keeps the external filter process running and processes all blobs with
 the following protocol over stdin/stdout.
>>> 
>>> I agree with Junio that the name "useProtocol" is bad, and not quite
>>> right. Perhaps "persistent" would be better? Also, what is the value
>>> of `filter..useProtocol`: boolean? or a script name?
> 
> As you see I was not sure if `useProtocol` was boolean or a script name,
> which means that it should be stated more explicitly.  Of course this
> would end to not matter if the way new protocol is used were changed.
> 
>> I agree that the name is not ideal. "UseProtocol" as it is would be a 
>> boolean. 
>> I thought about "persistent" but this name wouldn't convey the scope of the 
>> persistency ("persistent for one Git operation" vs. "persistent for many Git 
>> operations"). What do you think about the protocol as int version idea
>> described in $gmane/300155 ?
> 
> You mean the `protocol` as a config variable name (fully name being
> `filter..protocol`), being integer-valued, isn't it? Wouldn't
> `protocolVersion` be a more explicit?

Yes, but based on your other feedback I plan to use this variable differently
anyways.


>>> I also agree that we might wat to be able to keep clean and smudge
>>> filters separate, but be able to run a single program if they are
>>> both the same. I think there is a special case for filter unset,
>>> and/or filter being "cat" -- we would want to keep that.
>> 
>> Since 1a8630d there is a more efficient way to unset a filter ;-)
>> Can you think of other cases where the separation would be useful?
> 
> I can't think of any, but it doesn't mean that it does not exist.
> It also does not mean that you need to consider situation that may
> not happen. Covering one-way filters, like "indent" filter for `clean`,
> should be enough... they do work with your proposal, don't they?

This should work right now but it would be a bit inefficient (the filter
would just pass the data unchanged through the smudge command). I plan to
add a "capabilities" flag to the protocol. Then you can define only
the "clean" capability and nothing or the current filter mechanism 
would happen for smudge (I will make a test case to demonstrate that
behavior in v2).


>>> My proposal is to use `filter..persistent` as an addition
>>> to 'clean' and 'smudge' variables, with the following possible
>>> values:
>>> 
>>> * none (the default)
>>> * clean
>>> * smudge
>>> * both
>> 
>> That could work. However, I am not convinced, yet, that separate
>> filters are an actual use case.
> 
> YAGNI (You Ain't Gonna Need It), right.

That will work in v2.


>>> I assume that either Git would have to start multiple filter
>>> commands for 

Re: [PATCH v2 2/8] status: cleanup API to wt_status_print

[Junio C Hamano]

Jeff Hostetler  writes:

> Refactor the API between builtin/commit.c and wt-status.[ch].
> Hide details of the various wt_*status_print() routines inside
> wt-status.c behind a single (new) wt_status_print() routine
> and eliminate the switch statements from builtin/commit.c
>
> This will allow us to more easily add new status formats
> and isolate that within wt-status.c
>
> Signed-off-by: Jeff Hostetler 
> ---

The division of labour between wt-status.c and cmd_status() that was
originally envisioned was to have status computation in the former
and presentation in the latter, but that was broken and more output
is made from the latter these days, so I think it may make sense to
admit that the original vision did not work out well, and this step
is a good move forward to shift the boundary.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pack-objects: break out of want_object loop early

[Junio C Hamano]

Jeff King  writes:

> When pack-objects collects the list of objects to pack
> (either from stdin, or via its internal rev-list), it
> filters each one through want_object_in_pack().
>
> This function loops through each existing packfile, looking
> for the object. When we find it, we mark the pack/offset
> combo for later use. However, we can't just return "yes, we
> want it" at that point. If --honor-pack-keep is in effect,
> we must keep looking to find it in _all_ packs, to make sure
> none of them has a .keep. Likewise, if --local is in effect,
> we must make sure it is not present in any local pack.

s/any local pack/any non-local pack/, no?

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index a2f8cfd..55ef5a8 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -981,6 +981,8 @@ static int want_object_in_pack(const unsigned char *sha1,
>   return 0;
>   if (ignore_packed_keep && p->pack_local && p->pack_keep)
>   return 0;
> + if (!ignore_packed_keep && !local)
> + break;
>   }
>   }

OK, so in this loop, we may return "false" (meaning, we do not want
to pack the object) if "local" (do not pack objects that appear in
non-local packs) or "ignore_packed_keep" (do not pack objects that
appear in locally kept packs) are in effect, but if neither of the
options is set, we know that one of the preconditions ("local" or
"ignore_packed_keep") for these two "reject by returning false" if
statements would never trigger for any pack on packed_git list, so
it is safe to break out and return the one that we have found.

If that is what is going on, I would have expected to see this early
break before these "we found that this is available in borrowed pack
and we are only packing local" and "we ignore objects in locally
kept packs" checks return false.

Or am I not following the logic in the loop correctly?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/8] status: support --porcelain[=]

[Jakub Narębski]

W dniu 2016-07-25 o 21:25, Jeff Hostetler pisze:
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -144,6 +144,21 @@ static struct strbuf message = STRBUF_INIT;
>  
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
>  
> +static int opt_parse_porcelain(const struct option *opt, const char *arg, 
> int unset)
> +{
> + enum wt_status_format *value = (enum wt_status_format *)opt->value;
> + if (unset)
> + *value = STATUS_FORMAT_NONE;
> + else if (!arg)
> + *value = STATUS_FORMAT_PORCELAIN;
> + else if (!strcmp(arg, "v1"))
> + *value = STATUS_FORMAT_PORCELAIN;
> + else
> + die("unsupported porcelain version");
> +
> + return 0;
> +}

Presumably it is not something hard to find, but perhaps it
would be better to print the version that were given (for
example "1" instead of "v1")?

-- 
Jakub Narębski

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/8] status: cleanup API to wt_status_print

[Jeff Hostetler]

Refactor the API between builtin/commit.c and wt-status.[ch].
Hide details of the various wt_*status_print() routines inside
wt-status.c behind a single (new) wt_status_print() routine
and eliminate the switch statements from builtin/commit.c

This will allow us to more easily add new status formats
and isolate that within wt-status.c

Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c | 51 +--
 wt-status.c  | 25 ++---
 wt-status.h  | 16 
 3 files changed, 43 insertions(+), 49 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index b80273b..a792deb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -142,14 +142,7 @@ static int show_ignored_in_status, have_option_m;
 static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
-static enum status_format {
-   STATUS_FORMAT_NONE = 0,
-   STATUS_FORMAT_LONG,
-   STATUS_FORMAT_SHORT,
-   STATUS_FORMAT_PORCELAIN,
-
-   STATUS_FORMAT_UNSPECIFIED
-} status_format = STATUS_FORMAT_UNSPECIFIED;
+static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
@@ -500,24 +493,11 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   s->status_format = status_format;
+   s->ignore_submodule_arg = ignore_submodule_arg;
 
wt_status_collect(s);
-
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print(s);
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print(s);
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   wt_longstatus_print(s);
-   break;
-   }
+   wt_status_print(s);
 
return s->commitable;
 }
@@ -1099,7 +1079,7 @@ static const char *read_commit_message(const char *name)
  * is not in effect here.
  */
 static struct status_deferred_config {
-   enum status_format status_format;
+   enum wt_status_format status_format;
int show_branch;
 } status_deferred_config = {
STATUS_FORMAT_UNSPECIFIED,
@@ -1381,6 +1361,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
s.ignore_submodule_arg = ignore_submodule_arg;
+   s.status_format = status_format;
+   s.verbose = verbose;
+
wt_status_collect();
 
if (0 <= fd)
@@ -1389,23 +1372,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
if (s.relative_paths)
s.prefix = prefix;
 
-   switch (status_format) {
-   case STATUS_FORMAT_SHORT:
-   wt_shortstatus_print();
-   break;
-   case STATUS_FORMAT_PORCELAIN:
-   wt_porcelain_print();
-   break;
-   case STATUS_FORMAT_UNSPECIFIED:
-   die("BUG: finalize_deferred_config() should have been called");
-   break;
-   case STATUS_FORMAT_NONE:
-   case STATUS_FORMAT_LONG:
-   s.verbose = verbose;
-   s.ignore_submodule_arg = ignore_submodule_arg;
-   wt_longstatus_print();
-   break;
-   }
+   wt_status_print();
return 0;
 }
 
diff --git a/wt-status.c b/wt-status.c
index b9a58fd..a9031e4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1447,7 +1447,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
show_bisect_in_progress(s, state, state_color);
 }
 
-void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print(struct wt_status *s)
 {
const char *branch_color = color(WT_STATUS_ONBRANCH, s);
const char *branch_status_color = color(WT_STATUS_HEADER, s);
@@ -1714,7 +1714,7 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
-void wt_shortstatus_print(struct wt_status *s)
+static void wt_shortstatus_print(struct wt_status *s)
 {
int i;
 
@@ -1746,7 +1746,7 @@ void wt_shortstatus_print(struct wt_status *s)
}
 }
 
-void wt_porcelain_print(struct wt_status *s)
+static void wt_porcelain_print(struct wt_status *s)
 {
s->use_color = 0;
s->relative_paths = 0;
@@ -1754,3 +1754,22 @@ void wt_porcelain_print(struct wt_status *s)
s->no_gettext = 1;
wt_shortstatus_print(s);
 }
+
+void wt_status_print(struct wt_status *s)
+{
+   switch (s->status_format) {
+   case STATUS_FORMAT_SHORT:
+   wt_shortstatus_print(s);
+   

[PATCH v2 3/8] status: support --porcelain[=]

[Jeff Hostetler]

Update --porcelain argument to take optional version parameter
to allow multiple porcelain formats to be supported in the future.

The token "v1" is the default value and indicates the traditional
porcelain format.

Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt |  7 +--
 builtin/commit.c | 21 ++---
 t/t7060-wtstatus.sh  | 21 +
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index e1e8f57..6b1454b 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,11 +32,14 @@ OPTIONS
 --branch::
Show the branch and tracking info even in short-format.
 
---porcelain::
+--porcelain[=]::
Give the output in an easy-to-parse format for scripts.
This is similar to the short output, but will remain stable
across Git versions and regardless of user configuration. See
below for details.
++
+The version parameter is used to specify the format version.
+This is optional and defaults to the original version 'v1' format.
 
 --long::
Give the output in the long-format. This is the default.
@@ -96,7 +99,7 @@ configuration variable documented in linkgit:git-config[1].
 
 -z::
Terminate entries with NUL, instead of LF.  This implies
-   the `--porcelain` output format if no other format is given.
+   the `--porcelain=v1` output format if no other format is given.
 
 --column[=]::
 --no-column::
diff --git a/builtin/commit.c b/builtin/commit.c
index a792deb..e6bbb12 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -144,6 +144,21 @@ static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
+static int opt_parse_porcelain(const struct option *opt, const char *arg, int 
unset)
+{
+   enum wt_status_format *value = (enum wt_status_format *)opt->value;
+   if (unset)
+   *value = STATUS_FORMAT_NONE;
+   else if (!arg)
+   *value = STATUS_FORMAT_PORCELAIN;
+   else if (!strcmp(arg, "v1"))
+   *value = STATUS_FORMAT_PORCELAIN;
+   else
+   die("unsupported porcelain version");
+
+   return 0;
+}
+
 static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 {
struct strbuf *buf = opt->value;
@@ -1316,9 +1331,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
N_("show status concisely"), STATUS_FORMAT_SHORT),
OPT_BOOL('b', "branch", _branch,
 N_("show branch information")),
-   OPT_SET_INT(0, "porcelain", _format,
-   N_("machine-readable output"),
-   STATUS_FORMAT_PORCELAIN),
+   { OPTION_CALLBACK, 0, "porcelain", _format,
+ N_("version"), N_("machine-readable output"),
+ PARSE_OPT_OPTARG, opt_parse_porcelain },
OPT_SET_INT(0, "long", _format,
N_("show status in long format (default)"),
STATUS_FORMAT_LONG),
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 44bf1d8..00e0ceb 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -228,4 +228,25 @@ test_expect_success 'status --branch with detached HEAD' '
test_i18ncmp expected actual
 '
 
+## Duplicate the above test and verify --porcelain=v1 arg parsing.
+test_expect_success 'status --porcelain=v1 --branch with detached HEAD' '
+   git reset --hard &&
+   git checkout master^0 &&
+   git status --branch --porcelain=v1 >actual &&
+   cat >expected <<-EOF &&
+   ## HEAD (no branch)
+   ?? .gitconfig
+   ?? actual
+   ?? expect
+   ?? expected
+   ?? mdconflict/
+   EOF
+   test_i18ncmp expected actual
+'
+
+## Verify parser error on invalid --porcelain argument.
+test_expect_success 'status --porcelain=bogus' '
+   test_must_fail git status --porcelain=bogus
+'
+
 test_done
-- 
2.8.0.rc4.17.gac42084.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 8/8] status: tests for --porcelain=v2

[Jeff Hostetler]

Unit tests for porcelain v2 status format.

Signed-off-by: Jeff Hostetler 
---
 t/t7064-wtstatus-pv2.sh | 542 
 1 file changed, 542 insertions(+)
 create mode 100755 t/t7064-wtstatus-pv2.sh

diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
new file mode 100755
index 000..7a7f777
--- /dev/null
+++ b/t/t7064-wtstatus-pv2.sh
@@ -0,0 +1,542 @@
+#!/bin/sh
+
+test_description='git status --porcelain=v2
+
+This test exercises porcelain V2 output for git status.'
+
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   test_tick &&
+   git config --local core.autocrlf false &&
+   echo x >file_x &&
+   echo y >file_y &&
+   echo z >file_z &&
+   mkdir dir1 &&
+   echo a >dir1/file_a &&
+   echo b >dir1/file_b
+'
+
+
+##
+## Confirm VVP output prior to initial commit.
+##
+
+test_expect_success pre_initial_commit_0 '
+   cat >expected <<-EOF &&
+   ## branch: (initial) master
+   ? actual
+   ? dir1/
+   ? expected
+   ? file_x
+   ? file_y
+   ? file_z
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=normal 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success pre_initial_commit_1 '
+   git add file_x file_y file_z dir1 &&
+   SHA_A=`git hash-object -t blob -- dir1/file_a` &&
+   SHA_B=`git hash-object -t blob -- dir1/file_b` &&
+   SHA_X=`git hash-object -t blob -- file_x` &&
+   SHA_Y=`git hash-object -t blob -- file_y` &&
+   SHA_Z=`git hash-object -t blob -- file_z` &&
+   SHA_ZERO= &&
+
+   cat >expected <<-EOF &&
+   ## branch: (initial) master
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_A R0 dir1/file_a
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_B R0 dir1/file_b
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_X R0 file_x
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_Y R0 file_y
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_Z R0 file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+## Try -z on the above
+test_expect_success pre_initial_commit_2 '
+   cat >expected.lf <<-EOF &&
+   ## branch: (initial) master
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_A R0 dir1/file_a
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_B R0 dir1/file_b
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_X R0 file_x
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_Y R0 file_y
+   c A. N... 00 100644 100644 $SHA_ZERO $SHA_Z R0 file_z
+   ? actual
+   ? expected
+   EOF
+   perl -pe y/\\012/\\000/ expected &&
+   rm expected.lf &&
+
+   git status -z --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+##
+## Create first commit. Confirm commit sha in new track header.
+## Then make some changes on top of it.
+##
+
+test_expect_success initial_commit_0 '
+   git commit -m initial &&
+   H0=`git rev-parse HEAD` &&
+   cat >expected <<-EOF &&
+   ## branch: $H0 master
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_1 '
+   echo x >>file_x &&
+   SHA_X1=`git hash-object -t blob -- file_x` &&
+   rm file_z &&
+   H0=`git rev-parse HEAD` &&
+
+   cat >expected <<-EOF &&
+   ## branch: $H0 master
+   c .M N... 100644 100644 100644 $SHA_X $SHA_X R0 file_x
+   c .D N... 100644 100644 00 $SHA_Z $SHA_Z R0 file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_2 '
+   git add file_x &&
+   git rm file_z &&
+   H0=`git rev-parse HEAD` &&
+
+   cat >expected <<-EOF &&
+   ## branch: $H0 master
+   c M. N... 100644 100644 100644 $SHA_X $SHA_X1 R0 file_x
+   c D. N... 100644 00 00 $SHA_Z $SHA_ZERO R0 file_z
+   ? actual
+   ? expected
+   EOF
+
+   git status --porcelain=v2 --branch --ignored --untracked-files=all 
>actual &&
+   test_cmp expected actual
+'
+
+
+test_expect_success initial_commit_3 '
+   git mv file_y renamed_y &&
+   H0=`git rev-parse HEAD` &&
+
+   cat >expected.q <<-EOF &&
+   ## branch: $H0 master
+   c M. N... 100644 100644 100644 $SHA_X $SHA_X1 R0 file_x
+ 

[PATCH v2 5/8] status: print per-file porcelain v2 status data

[Jeff Hostetler]

Print per-file information in porcelain v2 format.

Signed-off-by: Jeff Hostetler 
---
 wt-status.c | 285 +++-
 1 file changed, 284 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 54aedc1..ffdfe11 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1864,6 +1864,8 @@ static void wt_porcelain_print(struct wt_status *s)
wt_shortstatus_print(s);
 }
 
+static void wt_porcelain_v2_print(struct wt_status *s);
+
 void wt_status_print(struct wt_status *s)
 {
switch (s->status_format) {
@@ -1874,7 +1876,7 @@ void wt_status_print(struct wt_status *s)
wt_porcelain_print(s);
break;
case STATUS_FORMAT_PORCELAIN_V2:
-   /* TODO */
+   wt_porcelain_v2_print(s);
break;
case STATUS_FORMAT_UNSPECIFIED:
die("BUG: finalize_deferred_config() should have been called");
@@ -1885,3 +1887,284 @@ void wt_status_print(struct wt_status *s)
break;
}
 }
+
+/*
+ * Convert various submodule status values into a
+ * string of characters in the buffer provided.
+ */
+static void wt_porcelain_v2_submodule_state(
+   struct wt_status_change_data *d,
+   char sub[5])
+{
+   if (S_ISGITLINK(d->aux.porcelain_v2.mode_head) ||
+   S_ISGITLINK(d->aux.porcelain_v2.mode_index) ||
+   S_ISGITLINK(d->aux.porcelain_v2.mode_worktree)) {
+   sub[0] = 'S';
+   sub[1] = d->new_submodule_commits ? 'C' : '.';
+   sub[2] = (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED) ? 'M' 
: '.';
+   sub[3] = (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ? 'U' 
: '.';
+   } else {
+   sub[0] = 'N';
+   sub[1] = '.';
+   sub[2] = '.';
+   sub[3] = '.';
+   }
+   sub[4] = 0;
+}
+
+/*
+ * Fix-up changed entries before we print them.
+ */
+static void wt_porcelain_v2_fix_up_changed(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+
+   if (!d->index_status) {
+   /*
+* This entry is unchanged in the index (relative to the head).
+* Therefore, the collect_updated_cb was never called for this
+* entry (during the head-vs-index scan) and so the head column
+* fields were never set.
+*
+* We must have data for the index column (from the
+* index-vs-worktree scan (otherwise, this entry should not be
+* in the list of changes)).
+*
+* Copy index column fields to the head column, so that our
+* output looks complete.
+*/
+   assert(d->aux.porcelain_v2.mode_head == 0);
+   d->aux.porcelain_v2.mode_head = d->aux.porcelain_v2.mode_index;
+   oidcpy(>aux.porcelain_v2.oid_head, 
>aux.porcelain_v2.oid_index);
+   }
+
+   if (!d->worktree_status) {
+   /*
+* This entry is unchanged in the worktree (relative to the 
index).
+* Therefore, the collect_changed_cb was never called for this 
entry
+* (during the index-vs-worktree scan) and so the worktree 
column
+* fields were never set.
+*
+* We must have data for the index column (from the 
head-vs-index
+* scan).
+*
+* Copy the index column fields to the worktree column so that
+* our output looks complete.
+*
+* Note that we only have a mode field in the worktree column
+* because the scan code tries really hard to not have to 
compute it.
+*/
+   assert(d->aux.porcelain_v2.mode_worktree == 0);
+   d->aux.porcelain_v2.mode_worktree = 
d->aux.porcelain_v2.mode_index;
+   }
+}
+
+/*
+ * Print porcelain v2 info for tracked entries with changes.
+ */
+static void wt_porcelain_v2_print_changed_entry(
+   struct string_list_item *it,
+   struct wt_status *s)
+{
+   struct wt_status_change_data *d = it->util;
+   struct strbuf buf_current = STRBUF_INIT;
+   struct strbuf buf_rename_src = STRBUF_INIT;
+   const char *path_current = NULL;
+   const char *path_rename_src = NULL;
+   char key[3];
+   char submodule_token[5];
+   char changed_prefix = 'c';
+   char sep_char, eol_char;
+
+   wt_porcelain_v2_fix_up_changed(it, s);
+   wt_porcelain_v2_submodule_state(d, submodule_token);
+
+   key[0] = d->index_status ? d->index_status : '.';
+   key[1] = d->worktree_status ? d->worktree_status : '.';
+   key[2] = 0;
+
+   if (s->null_termination) {
+   /*
+* In -z mode, we DO NOT C-Quote 

[PATCH v2 1/8] status: rename long-format print routines

[Jeff Hostetler]

Renamed the various wt_status_print*() routines to be
wt_longstatus_print*() to make it clear that these
routines are only concerned with the normal/long
status output.

This will hopefully reduce confusion as other status
formats are added in the future.

Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |   4 +-
 wt-status.c  | 110 +++
 wt-status.h  |   2 +-
 3 files changed, 58 insertions(+), 58 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1f6dbcd..b80273b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -515,7 +515,7 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
break;
case STATUS_FORMAT_NONE:
case STATUS_FORMAT_LONG:
-   wt_status_print(s);
+   wt_longstatus_print(s);
break;
}
 
@@ -1403,7 +1403,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
case STATUS_FORMAT_LONG:
s.verbose = verbose;
s.ignore_submodule_arg = ignore_submodule_arg;
-   wt_status_print();
+   wt_longstatus_print();
break;
}
return 0;
diff --git a/wt-status.c b/wt-status.c
index de62ab2..b9a58fd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -139,7 +139,7 @@ void wt_status_prepare(struct wt_status *s)
s->display_comment_prefix = 0;
 }
 
-static void wt_status_print_unmerged_header(struct wt_status *s)
+static void wt_longstatus_print_unmerged_header(struct wt_status *s)
 {
int i;
int del_mod_conflict = 0;
@@ -191,7 +191,7 @@ static void wt_status_print_unmerged_header(struct 
wt_status *s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(struct wt_status *s)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -207,9 +207,9 @@ static void wt_status_print_cached_header(struct wt_status 
*s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_dirty_header(struct wt_status *s,
-int has_deleted,
-int has_dirty_submodules)
+static void wt_longstatus_print_dirty_header(struct wt_status *s,
+int has_deleted,
+int has_dirty_submodules)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -226,9 +226,9 @@ static void wt_status_print_dirty_header(struct wt_status 
*s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_other_header(struct wt_status *s,
-const char *what,
-const char *how)
+static void wt_longstatus_print_other_header(struct wt_status *s,
+const char *what,
+const char *how)
 {
const char *c = color(WT_STATUS_HEADER, s);
status_printf_ln(s, c, "%s:", what);
@@ -238,7 +238,7 @@ static void wt_status_print_other_header(struct wt_status 
*s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_status_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(struct wt_status *s)
 {
status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -304,8 +304,8 @@ static int maxwidth(const char *(*label)(int), int minval, 
int maxval)
return result;
 }
 
-static void wt_status_print_unmerged_data(struct wt_status *s,
- struct string_list_item *it)
+static void wt_longstatus_print_unmerged_data(struct wt_status *s,
+ struct string_list_item *it)
 {
const char *c = color(WT_STATUS_UNMERGED, s);
struct wt_status_change_data *d = it->util;
@@ -331,9 +331,9 @@ static void wt_status_print_unmerged_data(struct wt_status 
*s,
strbuf_release();
 }
 
-static void wt_status_print_change_data(struct wt_status *s,
-   int change_type,
-   struct string_list_item *it)
+static void wt_longstatus_print_change_data(struct wt_status *s,
+   int change_type,
+   struct string_list_item *it)
 {
struct wt_status_change_data *d = it->util;
const char *c = color(change_type, s);
@@ -378,7 +378,7 @@ static void wt_status_print_change_data(struct wt_status *s,
status = d->worktree_status;
break;
default:
-   die("BUG: unhandled change_type %d in 
wt_status_print_change_data",
+   die("BUG: unhandled change_type %d in 
wt_longstatus_print_change_data",
change_type);
}
 
@@ 

[PATCH v2 6/8] status: print branch info with --porcelain=v2 --branch

[Jeff Hostetler]

Expand porcelain v2 output to include branch and tracking
branch information.  This includes the commit SHA, the branch,
the upstream branch, and the ahead and behind counts.

Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |  5 
 wt-status.c  | 86 
 wt-status.h  |  1 +
 3 files changed, 92 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5b9efd2..ebb43dd 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -510,6 +510,8 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->fp = fp;
s->nowarn = nowarn;
s->is_initial = get_sha1(s->reference, sha1) ? 1 : 0;
+   if (!s->is_initial)
+   hashcpy(s->sha1_commit, sha1);
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
 
@@ -1378,6 +1380,9 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
fd = hold_locked_index(_lock, 0);
 
s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
+   if (!s.is_initial)
+   hashcpy(s.sha1_commit, sha1);
+
s.ignore_submodule_arg = ignore_submodule_arg;
s.status_format = status_format;
s.verbose = verbose;
diff --git a/wt-status.c b/wt-status.c
index ffdfe11..39eef4b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1889,6 +1889,88 @@ void wt_status_print(struct wt_status *s)
 }
 
 /*
+ * Print branch and tracking header for porcelain v2 output.
+ * This is printed when the '--branch' parameter is given.
+ *
+ *"## branch:  [ [ + -]]"
+ *
+ *   ::= the current commit hash or the the literal
+ *   "(initial)" to indicate an initialized repo
+ *   with no commits.
+ *
+ * ::=  the current branch name or
+ *   "(detached)" literal when detached head or
+ *   "(unknown)" when something is wrong.
+ *
+ * ::= the upstream branch name, when set.
+ *
+ *::= integer ahead value, when upstream set
+ *   and commit is present.
+ *
+ *   ::= integer behind value, when upstream set
+ *   and commit is present.
+ *
+ *
+ * The end-of-line is defined by the -z flag.
+ *
+ *  ::= NUL when -z,
+ *   LF when NOT -z.
+ *
+ */
+static void wt_porcelain_v2_print_tracking(struct wt_status *s)
+{
+   struct branch *branch;
+   const char *base;
+   const char *branch_name;
+   struct wt_status_state state;
+   int ab_info, nr_ahead, nr_behind;
+
+   memset(, 0, sizeof(state));
+   wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD"));
+
+   fprintf(s->fp, "## branch:");
+   fprintf(s->fp, " %s",
+   (s->is_initial ? "(initial)" : sha1_to_hex(s->sha1_commit)));
+
+   if (!s->branch)
+   fprintf(s->fp, " %s", "(unknown)");
+   else {
+   if (!strcmp(s->branch, "HEAD")) {
+   fprintf(s->fp, " %s", "(detached)");
+   if (state.rebase_in_progress || 
state.rebase_interactive_in_progress)
+   branch_name = state.onto;
+   else if (state.detached_from)
+   branch_name = state.detached_from;
+   else
+   branch_name = "";
+   } else {
+   branch_name = NULL;
+   skip_prefix(s->branch, "refs/heads/", _name);
+   fprintf(s->fp, " %s", branch_name);
+   }
+
+   /* Lookup stats on the upstream tracking branch, if set. */
+   branch = branch_get(branch_name);
+   base = NULL;
+   ab_info = (stat_tracking_info(branch, _ahead, _behind, 
) == 0);
+   if (base) {
+   base = shorten_unambiguous_ref(base, 0);
+   fprintf(s->fp, " %s", base);
+   free((char *)base);
+
+   if (ab_info)
+   fprintf(s->fp, " +%d -%d", nr_ahead, nr_behind);
+   }
+   }
+
+   fprintf(s->fp, "%c", (s->null_termination ? '\0' : '\n'));
+
+   free(state.branch);
+   free(state.onto);
+   free(state.detached_from);
+}
+
+/*
  * Convert various submodule status values into a
  * string of characters in the buffer provided.
  */
@@ -2132,6 +2214,7 @@ static void wt_porcelain_v2_print_other(
 /*
  * Print porcelain V2 status.
  *
+ * []
  * []*
  * []*
  * []*
@@ -2144,6 +2227,9 @@ void wt_porcelain_v2_print(struct wt_status *s)
struct string_list_item *it;
int i;
 
+   if (s->show_branch)
+   wt_porcelain_v2_print_tracking(s);
+
for (i = 0; i < s->change.nr; i++) {

[PATCH v2 4/8] status: per-file data collection for --porcelain=v2

[Jeff Hostetler]

The output of `git status --porcelain` leaves out many details
about the current status that clients might like to have. This
can force them to be less efficient as they may need to launch
secondary commands (and try to match the logic within git) to
accumulate this extra information.  For example, a GUI IDE might
need the file mode to display the correct icon for a changed item.

Signed-off-by: Jeff Hostetler 
---
 builtin/commit.c |   3 ++
 wt-status.c  | 114 ++-
 wt-status.h  |  17 +
 3 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e6bbb12..5b9efd2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -153,6 +153,8 @@ static int opt_parse_porcelain(const struct option *opt, 
const char *arg, int un
*value = STATUS_FORMAT_PORCELAIN;
else if (!strcmp(arg, "v1"))
*value = STATUS_FORMAT_PORCELAIN;
+   else if (!strcmp(arg, "v2"))
+   *value = STATUS_FORMAT_PORCELAIN_V2;
else
die("unsupported porcelain version");
 
@@ -1104,6 +1106,7 @@ static struct status_deferred_config {
 static void finalize_deferred_config(struct wt_status *s)
 {
int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
+  status_format != STATUS_FORMAT_PORCELAIN_V2 
&&
   !s->null_termination);
 
if (s->null_termination) {
diff --git a/wt-status.c b/wt-status.c
index a9031e4..54aedc1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -406,6 +406,110 @@ static void wt_longstatus_print_change_data(struct 
wt_status *s,
strbuf_release();
 }
 
+static void aux_updated_entry_porcelain_v2(
+   struct wt_status *s,
+   struct wt_status_change_data *d,
+   struct diff_filepair *p)
+{
+   switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   /* {mode,sha1}_head are zero for an add. */
+   d->aux.porcelain_v2.mode_index = p->two->mode;
+   oidcpy(>aux.porcelain_v2.oid_index, >two->oid);
+   break;
+
+   case DIFF_STATUS_DELETED:
+   d->aux.porcelain_v2.mode_head = p->one->mode;
+   oidcpy(>aux.porcelain_v2.oid_head, >one->oid);
+   /* {mode,oid}_index are zero for a delete. */
+   break;
+
+   case DIFF_STATUS_RENAMED:
+   d->aux.porcelain_v2.rename_score = p->score * 100 / MAX_SCORE;
+   case DIFF_STATUS_COPIED:
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   case DIFF_STATUS_UNMERGED:
+   d->aux.porcelain_v2.mode_head = p->one->mode;
+   d->aux.porcelain_v2.mode_index = p->two->mode;
+   oidcpy(>aux.porcelain_v2.oid_head, >one->oid);
+   oidcpy(>aux.porcelain_v2.oid_index, >two->oid);
+   break;
+
+   case DIFF_STATUS_UNKNOWN:
+   die("BUG: index status unknown");
+   break;
+   }
+}
+
+/* Save aux info for a head-vs-index change. */
+static void aux_updated_entry(
+   struct wt_status *s,
+   struct wt_status_change_data *d,
+   struct diff_filepair *p)
+{
+   if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
+   aux_updated_entry_porcelain_v2(s, d, p);
+}
+
+static void aux_changed_entry_porcelain_v2(
+   struct wt_status *s,
+   struct wt_status_change_data *d,
+   const struct diff_filepair *p)
+{
+   switch (p->status) {
+   case DIFF_STATUS_ADDED:
+   die("BUG: worktree status add???");
+   break;
+
+   case DIFF_STATUS_DELETED:
+   d->aux.porcelain_v2.mode_index = p->one->mode;
+   oidcpy(>aux.porcelain_v2.oid_index, >one->oid);
+   /* mode_worktree is zero for a delete. */
+   break;
+
+   case DIFF_STATUS_MODIFIED:
+   case DIFF_STATUS_TYPE_CHANGED:
+   case DIFF_STATUS_UNMERGED:
+   d->aux.porcelain_v2.mode_index = p->one->mode;
+   d->aux.porcelain_v2.mode_worktree = p->two->mode;
+   oidcpy(>aux.porcelain_v2.oid_index, >one->oid);
+   break;
+
+   case DIFF_STATUS_UNKNOWN:
+   die("BUG: worktree status unknown???");
+   break;
+   }
+}
+
+/* Save aux info for an index-vs-worktree change. */
+static void aux_changed_entry(
+   struct wt_status *s,
+   struct wt_status_change_data *d,
+   struct diff_filepair *p)
+{
+   if (s->status_format == STATUS_FORMAT_PORCELAIN_V2)
+   aux_changed_entry_porcelain_v2(s, d, p);
+}
+
+static void aux_initial_entry_porcelain_v2(
+   struct wt_status *s,
+   struct wt_status_change_data *d,
+   const struct cache_entry *ce)
+{
+   d->aux.porcelain_v2.mode_index = ce->ce_mode;
+   hashcpy(d->aux.porcelain_v2.oid_index.hash, ce->sha1);
+}
+
+static void 

[PATCH v2 7/8] status: update git-status.txt for --porcelain=v2

[Jeff Hostetler]

Update status manpage to include information about
porcelain v2 format.

Signed-off-by: Jeff Hostetler 
---
 Documentation/git-status.txt | 83 ++--
 1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 6b1454b..e64b1b8 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -185,10 +185,10 @@ If -b is used the short-format status is preceded by a 
line
 
 ## branchname tracking info
 
-Porcelain Format
-
+Porcelain Format Version 1
+~~
 
-The porcelain format is similar to the short format, but is guaranteed
+Version 1 porcelain format is similar to the short format, but is guaranteed
 not to change in a backwards-incompatible way between Git versions or
 based on user configuration. This makes it ideal for parsing by scripts.
 The description of the short format above also describes the porcelain
@@ -210,6 +210,83 @@ field from the first filename).  Third, filenames 
containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Porcelain Format Version 2
+~~
+
+Version 2 format adds more detailed information about the state of
+the worktree and the changed items.
+
+If `--branch` is given, a header line showing branch tracking information
+is printed.  This line begins with "### branch: ".  Fields are separated
+by a single space.
+
+FieldMeaning
+
+ | (initial)Current commit
+ | (detached)Current branch
+   Upstream branch, if set
++ Ahead count, if upstream present
+-Behind count, if upstream present
+
+
+A series of lines are then displayed for the tracked entries.
+Ordinary changed entries have the following format; the first
+character is a 'c' to distinguish them from unmerged entries.
+
+cR [\t]
+
+Field   Meaning
+
+A 2 character field containing the staged and
+unstaged XY values described in the short format,
+with unchanged indicated by a "." rather than
+a space.
+   A 4 character field describing the submodule state.
+"N..." when the entry is not a submodule.
+"S" when the entry is a submodule.
+ is "C" if the commit changed; otherwise ".".
+ is "M" if it has tracked changes; otherwise ".".
+ is "U" if there are untracked changes; otherwise ".".
+The 6 character octal file modes for head, index,
+and worktree.
+The head and index SHA1 values.
+R   The rename percentage score.
+  The current pathname. It is C-Quoted if it contains
+special control characters.
+   The original path. This is only present for staged renames.
+It is C-Quoted if necessary.
+
+
+Unmerged entries have the following format; the first character is
+a "u" to distinguish from ordinary changed entries.
+
+u 
+
+Field   Meaning
+
+A 2 character field describing the conflict type
+as described in the short format.
+   A 4 character field describing the submodule state
+as described above.
+The 6 character octal file modes for the stage 1,
+stage 2, stage 3, and worktree.
+For regular entries, these are the head, index, and
+worktree modes; the fourth is zero.
+The stage 1, stage 2, and stage 3 SHA1 values.
+  The current pathname. It is C-Quoted if necessary.
+
+
+A series of lines are then displayed for untracked and ignored entries.
+
+ 
+
+Where  is "?" for untracked entries and "!" for ignored entries.
+
+When the `-z` option is given, a NUL (zero) byte follows each pathname;
+serving as both a separator and line termination. No pathname quoting
+or backslash escaping is performed. All fields are output in the same
+order.
+
 CONFIGURATION
 -
 
-- 
2.8.0.rc4.17.gac42084.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/8] status: V2 porcelain status

[Jeff Hostetler]

This patch series adds porcelain V2 format to status.
This provides detailed information about file changes
and about the current branch.

The new output is accessed via:
git status --porcelain=v2 [--branch]

This patch series hopefully addresses all of the
comments from the previous series.  The first 2
commits move the choice of output routines into
wt-status.c and cleanup the API from builtin/commit.c.
The command line parameter is "v2" to make it easier
to define other formats and/or JSON output later if
we want.  Detail lines for ordinary changes and
unmerged changes are now completely separate and have
a unique prefix key (and are grouped by type).  The
unit tests have been converted to use heredoc's.

I removed the v2 argument from git commit --porcelain
since it didn't really fit here.

Jeff Hostetler (8):
  status: rename long-format print routines
  status: cleanup API to wt_status_print
  status: support --porcelain[=]
  status: per-file data collection for --porcelain=v2
  status: print per-file porcelain v2 status data
  status: print branch info with --porcelain=v2 --branch
  status: update git-status.txt for --porcelain=v2
  status: tests for --porcelain=v2

 Documentation/git-status.txt |  90 ++-
 builtin/commit.c |  78 +++---
 t/t7060-wtstatus.sh  |  21 ++
 t/t7064-wtstatus-pv2.sh  | 542 +
 wt-status.c  | 616 +++
 wt-status.h  |  32 ++-
 6 files changed, 1269 insertions(+), 110 deletions(-)
 create mode 100755 t/t7064-wtstatus-pv2.sh

-- 
2.8.0.rc4.17.gac42084.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too

[Jeff King]

On Mon, Jul 25, 2016 at 02:40:25PM -0400, Jeff King wrote:

> > @@ -1052,6 +1053,9 @@ static int add_object_entry_from_bitmap(const 
> > unsigned char *sha1,
> >  {
> > uint32_t index_pos;
> >  
> > +   if (local && has_loose_object_nonlocal(sha1))
> > +   return 0;
> > +
> > if (have_duplicate_entry(sha1, 0, _pos))
> > return 0;
> 
> Hrm. Adding entries from the bitmap should ideally be very fast, but
> here we're introducing extra lookups in the object database. I guess it
> only kicks in when --local is given, though, which most bitmap-using
> paths would not do.
> 
> But is this check enough? The non-bitmap code path calls
> want_object_in_pack, which checks not only loose objects, but also
> non-local packs, and .keep.
> 
> Those don't kick in for your use case. I wonder if we should simply have
> something like:
> 
>   if (local || ignore_packed_keep)
>   use_bitmap_index = 0;
> 
> and just skip bitmaps for those cases. That's easy to reason about, and
> I don't think anybody would care (your use case does not, and the repack
> use case is already not going to use bitmaps).

BTW, I thought we had more optimizations in this area, but I realized
that I had never sent them to the list. I just did, and you may want to
take a peek at:

  http://thread.gmane.org/gmane.comp.version-control.git/300218

I doubt it will speed up your case much (unless you really do have tons
of packs in your extraction). And I think it is still worth doing
disabling I showed above, even with the optimizations, just because it's
easier to reason about.

So I _think_ those optimizations are orthogonal to what we're discussing
here, but I wanted to point you at them just in case.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] pack-objects: compute local/ignore_pack_keep early

[Jeff King]

In want_object_in_pack(), we can exit early from our loop if
neither "local" nor "ignore_pack_keep" are set. If they are,
however, we must examine each pack to see if it has the
object and is non-local or has a ".keep".

It's quite common for there to be no non-local or .keep
packs at all, in which case we know ahead of time that
looking further will be pointless. We can pre-compute this
by simply iterating over the list of packs ahead of time,
and dropping the flags if there are no packs that could
match.

Another similar strategy would be to modify the loop in
want_object_in_pack() to notice that we have already found
the object once, and that we are looping only to check for
"local" and "keep" attributes. If a pack has neither of
those, we can skip the call to find_pack_entry_one(), which
is the expensive part of the loop.

This has two advantages:

  - it isn't all-or-nothing; we still get some improvement
when there's a small number of kept or non-local packs,
and a large number of non-kept local packs

  - it eliminates any possible race where we add new
non-local or kept packs after our initial scan. In
practice, I don't think this race matters; we already
cache the packed_git information, so somebody who adds a
new pack or .keep file after we've started will not be
noticed at all, unless we happen to need to call
reprepare_packed_git() because a lookup fails.

In other words, we're already racy, and the race is not
a big deal (losing the race means we might include an
object in the pack that would not otherwise be, which is
an acceptable outcome).

However, it also has a disadvantage: we still loop over the
rest of the packs for each object to check their flags. This
is much less expensive than doing the object lookup, but
still not free. So if we wanted to implement that strategy
to cover the non-all-or-nothing cases, we could do so in
addition to this one (so you get the most speedup in the
all-or-nothing case, and the best we can do in the other
cases). But given that the all-or-nothing case is likely the
most common, it is probably not worth the trouble, and we
can revisit this later if evidence points otherwise.

Signed-off-by: Jeff King 
---
 builtin/pack-objects.c | 25 -
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 55ef5a8..c5e343c 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -46,6 +46,7 @@ static int keep_unreachable, unpack_unreachable, include_tag;
 static unsigned long unpack_unreachable_expiration;
 static int pack_loose_unreachable;
 static int local;
+static int have_non_local_packs;
 static int incremental;
 static int ignore_packed_keep;
 static int allow_ofs_delta;
@@ -981,7 +982,7 @@ static int want_object_in_pack(const unsigned char *sha1,
return 0;
if (ignore_packed_keep && p->pack_local && p->pack_keep)
return 0;
-   if (!ignore_packed_keep && !local)
+   if (!ignore_packed_keep && (!local || 
!have_non_local_packs))
break;
}
}
@@ -2785,6 +2786,28 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
progress = 2;
 
prepare_packed_git();
+   if (ignore_packed_keep) {
+   struct packed_git *p;
+   for (p = packed_git; p; p = p->next)
+   if (p->pack_local && p->pack_keep)
+   break;
+   if (!p) /* no keep-able packs found */
+   ignore_packed_keep = 0;
+   }
+   if (local) {
+   /*
+* unlike ignore_packed_keep above, we do not want to
+* unset "local" based on looking at packs, as it
+* also covers non-local objects
+*/
+   struct packed_git *p;
+   for (p = packed_git; p; p = p->next) {
+   if (!p->pack_local) {
+   have_non_local_packs = 1;
+   break;
+   }
+   }
+   }
 
if (progress)
progress_state = start_progress(_("Counting objects"), 0);
-- 
2.9.2.512.g8a06708
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] pack-objects: break out of want_object loop early

[Jeff King]

When pack-objects collects the list of objects to pack
(either from stdin, or via its internal rev-list), it
filters each one through want_object_in_pack().

This function loops through each existing packfile, looking
for the object. When we find it, we mark the pack/offset
combo for later use. However, we can't just return "yes, we
want it" at that point. If --honor-pack-keep is in effect,
we must keep looking to find it in _all_ packs, to make sure
none of them has a .keep. Likewise, if --local is in effect,
we must make sure it is not present in any local pack.

As a result, the sum effort of these calls is effectively
O(nr_objects * nr_packs). In an ordinary repository, we have
only a handful of packs, and this doesn't make a big
difference. But in pathological cases, it can slow the
counting phase to a crawl.

This patch notices the case that we have neither "--local"
nor "--honor-pack-keep" in effect and breaks out of the loop
early, after finding the first instance. Note that our worst
case is still "objects * packs" (i.e., we might find each
object in the last pack we look in), but in practice we will
often break out early. On an "average" repo, my git.git with
8 packs, this shows a modest 2% (a few dozen milliseconds)
improvement in the counting-objects phase of "git
pack-objects --all 
---
 builtin/pack-objects.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index a2f8cfd..55ef5a8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -981,6 +981,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return 0;
if (ignore_packed_keep && p->pack_local && p->pack_keep)
return 0;
+   if (!ignore_packed_keep && !local)
+   break;
}
}
 
-- 
2.9.2.512.g8a06708

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] speed up "Counting objects" when there are many packs

[Jeff King]

We sometimes see cases at GitHub where repository maintenance has fallen
behind, and you get a large number of packs. The solution is to repack,
but that process is itself made a lot slower by the number of packs.

We've experimented a bit with fast "just cat all the packfiles together"
type approaches, but they have some downsides, so I have nothing to show
there yet.

However, there are a few easy optimizations we can do to cut out some
unnecessary computation in common cases (e.g., when you have no .keep
files and when you have no upstream alternates storage). Both of these
patches have been in production at GitHub for about 6 months.

  [1/2]: pack-objects: break out of want_object loop early
  [2/2]: pack-objects: compute local/ignore_pack_keep early

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too

[Jeff King]

On Wed, Jul 13, 2016 at 01:52:17PM +0300, Kirill Smelkov wrote:

> > So I think if you were to repeatedly "git repack -adb" over time, you
> > would get worse and worse ordering as objects are added to the
> > repository.
> 
> Jeff, first of all thanks for clarifying.
> 
> So it is not-yet-packed-objects which make packing with bitmap less
> efficient. I was originally keeping in mind fresh repacked repository
> with just built bitmap index and for that case extracting pack with
> bitmap index seems to be just ok, but the more not-yet-packed objects we
> have the worse the result can be.

Right. So I think your scheme is fine as long as you are doing your
regular "pack all into one" repacks with a real walk, and then
"branching" off of that with one-off bitmap-computed packs into files
(even if you later take a bunch of those files and pull them into a
single bitmapped, as long as that final "all into one" does the walk).

Or I guess another way to think about it would be that if you're
computing bitmaps, you'd want to do the actual traversal.

> Yes, it also make sense. I saw write_reused_pack() in upstream git just
> copy raw bytes from original to destination pack. You mentioned you have
> something better for pack reuse - in your patch queue, in two words, is
> it now reusing pack based on object, not raw bytes, or is it something
> else?
> 
> In other words in which way it works better? (I'm just curious here as
> it is interesting to know)

The problem with the existing pack-reuse code is that it doesn't kick in
often enough. I think it looks to see that the client wants some
percentage of the pack (e.g., 90%), and then just sends the whole
beginning. This works especially badly if you have a bunch of related
repositories packed together (e.g., all of the forks of torvalds/linux
on GitHub), because you'll never hit 90% of that big pack; it has too
much unrelated cruft, even if most of the stuff you want _is_ at the
beginning. And "percent of pack" is not really a useful metric anyway.

So the better scheme is more like:

  1. Generate the bitmap of objects to send using reachability bitmaps.

  2. Do a quick scan of their content in the packfile to see which can
 be reused verbatim. If they're base objects, we can send them
 as-is. If they're deltas, we can send them if their base is going
 to be sent. This fills in another bitmap of "reusable" objects.

 After a long string of unusable objects, you can give up and set
 the rest of the bitmap to zeroes.

  3. Walk the "reuse" bitmap and send out the objects more-or-less
 verbatim. You do have make adjustments to delta-base-offsets for
 any "holes" (so if an object's entry says "my base is 500 bytes
 back", but you omitted some objects in between, you have to adjust
 that offset).

The upside is that you can send out those objects without even making a
"struct object_entry" for them, which drastically reduces the memory
requirements for serving a clone. Any objects which didn't get marked
for reuse just get handled in the usual way (so stuff that was not close
by in the pack, or stuff that was pushed since your last big repack).

The downside is that because those objects aren't in our normal packing
list, they're not available as delta bases for the new objects we _do_
send. So it can make the resulting pack a little bit bigger.

> Yes, for packing it is only hash which is used. And I assume name-hash
> for bitmap is not enabled by default for compatibility with JGit code.
>
> It would make sense to me to eventually enable name-hash bitmap
> extension by default, as packing result is much better with it. And
> those who care about compatibility with JGit can just turn it off in
> their git config.

Correct, the defaults are for JGit compatibility. If you are not using
JGit, you should have it on all the time.  We went with the conservative
default, but as more people using regular Git bitmaps, it would probably
be good to make them less arcane and confusing to use.

> > As I understand your use case, it is OK to do the less careful things.
> > It's just that pack-objects until now has been split into two modes:
> > packing to a file is careful, and packing to stdout is less so. And you
> > want to pack to a file in the non-careful mode.
> 
> Yes, it should be ok, as after repository extraction git-backup
> verifies rev-list for all refs
> 
> https://lab.nexedi.com/kirr/git-backup/blob/7fcb8c67/git-backup.go#L855
> 
> And if an object is missing - e.g. a blob - rev-list complains:
> 
> fatal: missing blob object '980a0d5f19a64b4b30a87d4206aade58726b60e3'
> 
> though it does not catch blob corruptions.

Right, that makes sense. Even the pack-to-disk code invoked by
git-repack is not foolproof for blob corruptions. It is only checking a
crc, not the full sha1. So it's better than nothing, but not as careful
as a full index-pack.

> Probably pack.useBitmaps is of no use in normal situation, but for
> debugging 

Re: [PATCH] config.mak.uname: correct perl path on FreeBSD

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Jul 25, 2016 at 6:56 PM, Junio C Hamano  wrote:
>> On Mon, Jul 25, 2016 at 9:21 AM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> It looks the the symlink /usr/bin/perl (to /usr/local/bin/perl) has
>>> been removed at least on FreeBSD 10.3. See [1] for more information.
>>>
>>> [1] 
>>> https://svnweb.freebsd.org/ports/head/UPDATING?r1=386270=386269=386270_format=c
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>>> ---
>>>  Tested with fbsd 10.3, kvm image. But I suppose it's the same as real
>>>  fbsd.
>>
>> Thanks; and we know that older (but not too old that we no longer care about)
>> FreeBSD all have /usr/local/bin/perl?
>
> I'm no fbsd expert but from the first sentence in [1] "Perl has been
> removed from base more than ten years ago..." I would assume it meant
> "... removed from base, _to_ ports system" which means /usr/local for
> all package installation (for ten years for perl). So I think we are
> good.

I guess we didn't follow through

http://public-inbox.org/git/%3C20160720025630.GA71874%40plume%3E/

and allowed the thread to drift into a tangent?


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] mailinfo: extract is_from_line from mailsplit

[Junio C Hamano]

Andreas Schwab  writes:

>> +colon = line + len - 2;
>> +line += 5;
>> +for (;;) {
>> +if (colon < line)
>> +return 0;
>> +if (*--colon == ':')
>> +break;
>> +}
>> +
>> +if (!isdigit(colon[-4]) ||
>> +!isdigit(colon[-2]) ||
>> +!isdigit(colon[-1]) ||
>> +!isdigit(colon[ 1]) ||
>> +!isdigit(colon[ 2]))
>> +return 0;
>> +
>> +/* year */
>> +if (strtol(colon+3, NULL, 10) <= 90)
>> +return 0;
>> +
>> +/* Ok, close enough */
>> +return 1;
>> +}
>
> Should this be made more strict, like by checking for a space before the
> year?

The function seems to judge the line to be "close enough" by
checking these places (please view with fixed-width font ;-):

From me Mon Jul 25 01:23:45 2016
x   x x 

We only see if the year part is more than 90 but we do not even
ensure that it is at the end of the line without any further
garbage, which I think is probably OK for the purpose of "close
enough".

We also do not bother rejecting a single-digit hour, or there is a
colon between the hour and minute part.

I would say requiring SP between the year and the second, and
requiring colon between hour and minute, would probably be a good
change, i.e. something like this:

if (!isdigit(colon[-4]) ||
colon[-3] != ':' ||
!isdigit(colon[-2]) ||
!isdigit(colon[-1]) ||
!isdigit(colon[ 1]) ||
!isdigit(colon[ 2]) ||
colon[3] != ' ')

would be safe enough without increasing the false negatives too
much.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i18n: notes: mark comment for translation

[Junio C Hamano]

Vasco Almeida  writes:

>  static const char note_template[] =
> - "\nWrite/edit the notes for the following object:\n";
> + N_("Write/edit the notes for the following object:");
>  
>  struct note_data {
>   int given;
> @@ -179,7 +179,8 @@ static void prepare_note_data(const unsigned char 
> *object, struct note_data *d,
>   copy_obj_to_fd(fd, old_note);
>  
>   strbuf_addch(, '\n');
> - strbuf_add_commented_lines(, note_template, 
> strlen(note_template));
> + strbuf_addch(, '\n');
> + strbuf_add_commented_lines(, _(note_template), 
> strlen(_(note_template)));

I do not quite understand why you want the blank lines surrounding
the message outside add_commented_lines() call.  I think the intent
is to produce

#
# Write/edit the notes for the following object:
#

with the single call.  If you pushed the newlines outside the
message, wouldn't you end up having this instead ( denoting an
extra empty line each before and after the message)?


# Write/edit the notes for the following object:


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] git svn: migrate tests to use lib-httpd

[Junio C Hamano]

Eric Wong  writes:

> This allows us to use common test infrastructure and parallelize
> the tests.  For now, GIT_SVN_TEST_HTTPD=true needs to be set to
> enable the SVN HTTP tests because we reuse the same test cases
> for both file:// and http:// SVN repositories.  SVN_HTTPD_PORT
> is no longer honored.
>
> Tested under Apache 2.2 and 2.4 on Debian 7.x (wheezy) and
> 8.x (jessie), respectively.
>
> Cc: Clemens Buchacher 
> Cc: Michael J Gruber 
> Signed-off-by: Eric Wong 
> ---
>  t/lib-git-svn.sh  | 91 
> +--
>  t/lib-httpd.sh|  8 ++-
>  t/lib-httpd/apache.conf   |  4 +-
>  t/t9115-git-svn-dcommit-funky-renames.sh  |  7 ++-
>  t/t9118-git-svn-funky-branch-names.sh |  2 +-
>  t/t9120-git-svn-clone-with-percent-escapes.sh |  2 +-
>  t/t9142-git-svn-shallow-clone.sh  |  2 +-
>  7 files changed, 30 insertions(+), 86 deletions(-)

Nice code reduction ;-)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] git subcommand to check if branch is up-to-date with upstream

[Jakub Narębski]

W dniu 2016-07-25 o 18:58, Junio C Hamano pisze:
> Sidhant Sharma  writes:
> 
>> I was wondering if it would be a good idea to have a command to check if a
>> push or pull is required. Perhaps it can also suggest if changes are
>> fast-forward or the branches (local and remote) have diverged.
> 
> Doesn't "branch -v" give that information these days?  You'd need to
> "fetch" first to get the up-to-date worldview before running it, of
> course.

You need "branch -v -v". For current branch, you can simply run "git checkout".
All this is the information for end user, not scripts.

$ git branch -v -v
* gitweb-docs   4ebf58d [origin/master: ahead 1] gitweb(1): Document query 
parameters
  master08bb350 [origin/master] Sixth batch of topics for 2.10

$ git checkout
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)

-- 
Jakub Narębski


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit

[Junio C Hamano]

Eric Wong  writes:

>> Also, doesn't it break "git rebase" (non-interactive), or anything
>> that internally runs format-patch to individual files and then runs
>> am on each of them, anything that knows that each output file from
>> format-patch corresponds to a single change and there is no need to
>> split, badly if we do this unconditionally?
>
> Yes, rebase should probably unescape is_from_line matches.

This shouldn't matter for "git rebase", as it only reads from the
mbox "From  " line to learn the
original commit and extract the log message directly from there.

But a third-party script that wants to read format-patch output
would be forced to upgrade, which is a pain if we make this change
unconditionally.

>> IOW, shouldn't this be an optional feature to format-patch that
>> is triggered by passing a new command line option that currently
>> nobody is passing?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: "git log --format='format:%+s%+b'" doesn't insert newline before body

[Jakub Narębski]

W dniu 2016-07-25 o 09:33, Johannes Schindelin pisze:

> Therefore "%s%n%+b" *might* do what you intended (I would not
> know, because that information was missing from the report).

Shouldn't it be "%s%n%n%-b" or "%s%n%-b"?

-- 
Jakub Narębski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] config.mak.uname: correct perl path on FreeBSD

[Duy Nguyen]

On Mon, Jul 25, 2016 at 6:56 PM, Junio C Hamano  wrote:
> On Mon, Jul 25, 2016 at 9:21 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> It looks the the symlink /usr/bin/perl (to /usr/local/bin/perl) has
>> been removed at least on FreeBSD 10.3. See [1] for more information.
>>
>> [1] 
>> https://svnweb.freebsd.org/ports/head/UPDATING?r1=386270=386269=386270_format=c
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  Tested with fbsd 10.3, kvm image. But I suppose it's the same as real
>>  fbsd.
>
> Thanks; and we know that older (but not too old that we no longer care about)
> FreeBSD all have /usr/local/bin/perl?

I'm no fbsd expert but from the first sentence in [1] "Perl has been
removed from base more than ten years ago..." I would assume it meant
"... removed from base, _to_ ports system" which means /usr/local for
all package installation (for ten years for perl). So I think we are
good.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push: allow pushing new branches with --force-with-lease

[Junio C Hamano]

John Keeping  writes:

> If there is no upstream information for a branch, it is likely that it
> is newly created and can safely be pushed under the normal fast-forward
> rules.  Relax the --force-with-lease check so that we do not reject
> these branches immediately but rather attempt to push them as new
> branches, using the null SHA-1 as the expected value.
>
> In fact, it is already possible to push new branches using the explicit
> --force-with-lease=: syntax, so all we do here is make
> this behaviour the default if no explicit "expect" value is specified.

I like the loss of an extra field from "struct ref".

I suspect that the if/else cascade in the loop in apply_cas() can
also be taught that ':' followed by an empty string asks to check
that the target ref does not exist, in order to make it a bit more
useful for folks who do not rely on the "use the last observed
status of the tracking branch".

That would make the "explicit" test much less cumbersome to read.


> +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> + setup_srcdst_basic &&
> + (
> + cd dst &&
> + git branch branch master &&
> + git push 
> --force-with-lease=branch: origin 
> branch
> + ) &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] A Change to Commit IDs Too Ridiculous to Consider?

[Junio C Hamano]

Junio C Hamano  writes:

> Jon Forrest  writes:
>
>> Sure. Take a look at the 2nd or 3rd chapter of Pro Git Reedited, 2nd
>> Edition (or just Pro Git 2nd Edition - it doesn't matter). You see
>> lots of output showing 'git commit' commands and the commit IDs that
>> result. I suspect you'd see the same in almost any book about Git.
>
> I would think that the early-stage learners are better served that
> it is the norm, not anything strange, that the commit object name
> would be different when you do two identical sequence from scratch.

Ehh, sent without proofreading.  The above should say "... are
better served if the book taught that it is the norm, ...".

> Forcing them to know GIT_*_DATE variables, just to give them an
> impression as if setting of them is part of any normal workflow (or
> more importantly, stable commit IDs made by different people at
> different times is something expected), is doing them double
> disservice, IMHO.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] git subcommand to check if branch is up-to-date with upstream

[Junio C Hamano]

Sidhant Sharma  writes:

> I was wondering if it would be a good idea to have a command to check if a
> push or pull is required. Perhaps it can also suggest if changes are
> fast-forward or the branches (local and remote) have diverged.

Doesn't "branch -v" give that information these days?  You'd need to
"fetch" first to get the up-to-date worldview before running it, of
course.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] config.mak.uname: correct perl path on FreeBSD

[Junio C Hamano]

On Mon, Jul 25, 2016 at 9:21 AM, Nguyễn Thái Ngọc Duy  wrote:
> It looks the the symlink /usr/bin/perl (to /usr/local/bin/perl) has
> been removed at least on FreeBSD 10.3. See [1] for more information.
>
> [1] 
> https://svnweb.freebsd.org/ports/head/UPDATING?r1=386270=386269=386270_format=c
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Tested with fbsd 10.3, kvm image. But I suppose it's the same as real
>  fbsd.

Thanks; and we know that older (but not too old that we no longer care about)
FreeBSD all have /usr/local/bin/perl?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 12/12] bisect--helper: `get_terms` & `bisect_terms` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

>>> +enum terms_defined {
>>> +   TERM_BAD = 1,
>>> +   TERM_GOOD = 2,
>>> +   TERM_NEW = 4,
>>> +   TERM_OLD = 8
>>> +};
>>> +
>>
>> What does TERM stand for  ?

The terms (words) used to denote the newer and the older parts of
the history.  Traditionally, as a regression-hunting tool (i.e. it
used to work, where did I break it?), we called older parts of the
history "good" and newer one "bad", but as people gained experience
with the tool, it was found that the pair of words was error-prone
to use for an opposite use case "I do not recall fixing it, but it
seems to have been fixed magically, when did that happen?", and a
more explicit "new" and "old" were introduced.

>> And why are the defines 1,2,4,8 ?
>> It looks as if a #define bitmap may be a better choice here ?
>> How do we do these kind of bit-wise opions otherwise ?

We might want to ask if these should even be bitwise option.  A word
with individually controllable bits (i.e. "flag word") makes sense
only when the bits within it are largely independent.  But the code
does this pretty much upfront:

>>> +   if (term_defined != 0 && term_defined != TERM_BAD &&
>>> +   term_defined != TERM_GOOD && term_defined != TERM_NEW &&
>>> +   term_defined != TERM_OLD)
>>> +   die(_("only one option among --term-bad, --term-good, "
>>> + "--term-new and --term-old can be used."));

which is a very strong indication that these bits are not.

I suspect that OPTION_CMDMODE would be a better choice to group
these four options and mark them mutually incompatible automatically
than OPT_BIT?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Complex gitweb URL

[Junio C Hamano]

Jakub Narębski  writes:

> Subject: [PATCH] gitweb(1): Document query parameters
>
> The gitweb manpage includes description of gitweb URL structure,
> but it was limited to passing parameters in the path part of URL
> ('path info'), and it didn't include explanation of query parameters.
> ---

I think this weather-balloon patch adds the missing info at the most
natural place in the flow of the document.  An earlier part talks
about a GitWeb URL having , , ,  and
followed by what is called  after a question mark '?',
but what  part means and how to formulate it are left
unsaid.

By the way, the title of the section 'Actions, and URLs' needs to
lose the comma, I would think.

>  Documentation/gitweb.txt | 37 +
>  1 file changed, 37 insertions(+)
>
> diff --git a/Documentation/gitweb.txt b/Documentation/gitweb.txt
> index 96156e5..301003f 100644
> --- a/Documentation/gitweb.txt
> +++ b/Documentation/gitweb.txt
> @@ -395,6 +395,43 @@ atom::
>   Generates an RSS (or Atom) feed of changes to repository.
>  
>  
> +Query parameters:
> +~
> +In some cases gitweb cannot pass a parameter in path part of URL, for
> +example if a filename contains double dots ('..') inside.  All parameters
> +that cannot be passed in that way are put as a query parameter, that is
> +after '?'.

Until this point, the reader hasn't even been told about "filename"
is one of the potential things that can be given to "control the
behaviour of the action", and is left wondering if she has a
filename that does not have double-dots, how it can be passed "in
path part of URL" after reading the first sentence.  Then the second
sentence adds more to the mystery by starting with "All parameters
that cannot be passed" "in that way": what are the possible things
that the users may want to pass?  what exactly is "in that way",
which probably means "encoded as part of the ", but it is
unclear how that encoding works (is it just to "concatenate"?).

Or is "filename in the revision" the ONLY thing the above paragraph
talks about, and is the ONLY purpose of the " part" to
represent that "filename in the revision" in the URL?

.../gitweb.cgi///:/?

The above syntax (taken from the first paragraph of the section)
suggests that  cannot be used to hold filename with a colon,
too.  Perhaps "for example if a filename contains double-dots" needs
to become an exhaustive description somewhere, i.e. "(see below for
the exact rules)" added to that sentence?

> +By default gitweb would generate links using query parameters, unless
> +the original URL was using path part, or gitweb was configured to use
> +pathinfo with the 'pathinfo' feature. All gitweb installations recognize
> +URL in either format, so you can use one that is better for you when
> +interacting with gitweb (handcrafting or editing URLs, or creating
> +a program interacting with gitweb installation).
> +
> +Here is the list of some of query parameters, together with their
> +long names (which should help remembering the short name of
> +each parameter):
> +
> +'a' (action)::
> + The action that will be run.
> +
> +'p' (project)::
> + The project repository that will be displayed.
> +
> +'h' (hash)::
> + The object id of displayed object (commit, tag, tree, blob).
> + In case of files 'hb' (hash_base) and 'f' (filename) might be
> + used instead.
> +
> +'hp' (hash_parent)::
> +'fp' (file_parent)::
> +'hpb' (hash_parent_base)::
> +  Those parameters define the second object for diff-like actions,
> +  the object gitweb is comparing againts.

There is no mention of "filename" or "path" here, which further
confuses a reader who was told in the first paragraph that these
query-parameters are used when  part cannot hold a certain
filename.  Also it is unclear if  corresponds to 'h'ash
in the above.

> +
> +
> +
>  WEBSERVER CONFIGURATION
>  ---
>  This section explains how to configure some common webservers to run gitweb. 
> In
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] config.mak.uname: correct perl path on FreeBSD

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

It looks the the symlink /usr/bin/perl (to /usr/local/bin/perl) has
been removed at least on FreeBSD 10.3. See [1] for more information.

[1] 
https://svnweb.freebsd.org/ports/head/UPDATING?r1=386270=386269=386270_format=c

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Tested with fbsd 10.3, kvm image. But I suppose it's the same as real
 fbsd.

 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index a88f139..4cd62bd 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -203,6 +203,7 @@ ifeq ($(uname_S),FreeBSD)
NO_STRTOUMAX = YesPlease
endif
PYTHON_PATH = /usr/local/bin/python
+   PERL_PATH = /usr/local/bin/perl
HAVE_PATHS_H = YesPlease
GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
HAVE_BSD_SYSCTL = YesPlease
-- 
2.9.1.566.gbd532d4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-prompt.sh incompatible with non-basic global grep.patternType

[Junio C Hamano]

Eric Sunshine  writes:

>> Later when the grep API was used in revision graversal machinery,
>
> s/graversal/traversal/

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t5510: become resilient to GETTEXT_POISON

[Duy Nguyen]

On Mon, Jul 25, 2016 at 5:16 PM, Junio C Hamano  wrote:
> On Mon, Jul 25, 2016 at 2:31 AM, Vasco Almeida  wrote:
>> Replace gettext poison text with appropriate values to be able to cut
>> the right output of git fetch command for comparison.
>
> Hmm, as these tests are _all_ about human-readable output, it probably is
> sufficient to skip them using prerequiste, I would think. We do not want each
> individual test to have too intimate knowledge on how POISON strings look
> like.

Yeah these tests are about alignment,  they are probably useless
anyway after the text is poisoned (and has the same length).
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/8] Add configuration options for split-index

[Duy Nguyen]

On Sat, Jul 23, 2016 at 6:11 PM, Christian Couder
 wrote:
> Ok, I started working on automatically pushing back all changes to the
> shared index when the percentage of entries in linked vs shared
> indexes is greater than 25% (maybe I will make it configurable later).
> It is very basic and doesn't work well for now (for one thing it is
> missing added entries), see:
>
> https://github.com/chriscool/git/commits/config-split-index8
>
> Basically I would like a way to count then entries that are only in
> the linked index without modifying them to be safe, but I have a hard
> time seeing how I could modify prepare_to_write_split_index() to get
> that.

Hmm.. can you do the counting separately? A shared cache_entry must
have its field "index" greater than zero. By counting the number of
entries whose index is zero (i.e. not shared) against the total number
of real (*) entries, you should have a decent estimate when to split.
Then you can do exactly what "git update-index --no-split-index" and
"git update-index --split-index" sequence does, but in write_index().
It's easier than messing inside split-index.c. If we hit performance
problem, then we can look into changing split-index.c

(*) remember that some entries may be marked CE_REMOVE, which are dead
entries and should not be counted because they will never be written
down on disk.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Duy Nguyen]

On Sun, Jul 24, 2016 at 9:11 PM, Lars Schneider
 wrote:
>
> On 23 Jul 2016, at 10:14, Eric Wong  wrote:
>
>> larsxschnei...@gmail.com wrote:
>>> Please note that the protocol filters do not support stream processing
>>> with this implemenatation because the filter needs to know the length of
>>> the result in advance. A protocol version 2 could address this in a
>>> future patch.
>>
>> Would it be prudent to reuse pkt-line for this?
>
> Peff suggested that, too, in $gmane/299902.

And I was about to suggest the same too, until I saw his patch then
stopped. Having a common way to split a byte stream to a packet stream
could be a good thing.

> However, this would make the protocol a bit more complicated

For high level scripting languages, pkt-line is dead simple. If your
scripts are in sh then it could get a bit ugly, but I'm thinking of a
small utility to make shell scripting pkt-line easier anyway (it goes
back to the idea of rewriting index-helper as a script, which I might
do).

> and it wouldn't buy us anything for Git
> large file processing filters (my main motivation for this patch) as these
> filters can't leverage streaming anyways.

This is a good point. How are you planning to do it? Unless streaming
is done entirely in kernel (sendfie() and friends, which is not all
positive), I think you can still stream and wrap/unwrap pkt-line just
fine.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/4] submodule: support running in multiple worktree setup

[Junio C Hamano]

Stefan Beller  writes:

> On Fri, Jul 22, 2016 at 9:55 AM, Junio C Hamano  wrote:
>>
>>  * submodule.$name.update, submodule.$name.ignore,
>>submodule.$name.branch, etc. would need to be all different among
>>worktrees of the superproject, as that is the whole point of
>>being able to work on separate branches of the superproject in
>>separate worktrees.
>
> What do you mean by "would need". The ability to be different or rather
> the veto of an 'inheritance' of defaults from the repository configuration?

They have to be able to represent different settings per worktree
that checks out different branches/commits of superproject.  They
may happen to be set the same, but they do not have to be.

Is what I meant.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t5510: become resilient to GETTEXT_POISON

[Junio C Hamano]

On Mon, Jul 25, 2016 at 2:31 AM, Vasco Almeida  wrote:
> Replace gettext poison text with appropriate values to be able to cut
> the right output of git fetch command for comparison.

Hmm, as these tests are _all_ about human-readable output, it probably is
sufficient to skip them using prerequiste, I would think. We do not want each
individual test to have too intimate knowledge on how POISON strings look
like.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] A Change to Commit IDs Too Ridiculous to Consider?

[Junio C Hamano]

Jon Forrest  writes:

> Sure. Take a look at the 2nd or 3rd chapter of Pro Git Reedited, 2nd
> Edition (or just Pro Git 2nd Edition - it doesn't matter). You see
> lots of output showing 'git commit' commands and the commit IDs that
> result. I suspect you'd see the same in almost any book about Git.

I would think that the early-stage learners are better served that
it is the norm, not anything strange, that the commit object name
would be different when you do two identical sequence from scratch.
Forcing them to know GIT_*_DATE variables, just to give them an
impression as if setting of them is part of any normal workflow (or
more importantly, stable commit IDs made by different people at
different times is something expected), is doing them double
disservice, IMHO.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[Junio C Hamano]

Heiko Voigt  writes:

> Thats a good idea. Can send a patch, but I noticed that your change is
> already in next and my change has not been picked up. So I am unsure if
> I should base my patches on next or if Junio wants to keep your change?

When you noticed something you want to touch up in 'next' but not in
'master', please try to find which topic the target of your
touching-up comes from.

git log --first-parent  master..next

I often use --oneline for , because I am reasonably
familiar with the topics in 'next', but people may start from "git
blame" output, finding the commit that left the text you are
touching up, then where it was merged to 'next'.  That would give
you

commit 59dbd5834007c1e4dff25770da095babcb69a009
Merge: 679f992 508a285
Author: Junio C Hamano 
Date:   Tue Jul 19 13:43:44 2016 -0700

Merge branch 'rs/submodule-config-code-cleanup' into next

Code cleanup.

* rs/submodule-config-code-cleanup:
  submodule-config: use explicit empty string instead of strbuf in 
config_from()

The second parent 508a285c (submodule-config: use explicit empty
string instead of strbuf in config_from(), 2016-07-19) is a good
place to base your patch.

Thanks.



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule-config: use explicit empty string instead of strbuf in config_from()

[Heiko Voigt]

Hi,

On Thu, Jul 21, 2016 at 08:57:03PM +0200, René Scharfe wrote:
> >diff --git a/submodule-config.c b/submodule-config.c
> >index 077db40..dccea59 100644
> >--- a/submodule-config.c
> >+++ b/submodule-config.c

[...]

> >@@ -431,14 +432,19 @@ static const struct submodule *config_from(struct 
> >submodule_cache *cache,
> > submodule = cache_lookup_path(cache, sha1, key);
> > break;
> > }
> >-if (submodule)
> >+if (submodule) {
> >+strbuf_release();
> > return submodule;
> >+}
> >
> > config = read_sha1_file(sha1, , _size);
> >-if (!config)
> >+if (!config) {
> >+strbuf_release();
> > return NULL;
> >+}
> >
> > if (type != OBJ_BLOB) {
> >+strbuf_release();
> > free(config);
> > return NULL;
> > }
> 
> A separate patch could combine the previous two conditionals; free(NULL) is
> allowed.

Thats a good idea. Can send a patch, but I noticed that your change is
already in next and my change has not been picked up. So I am unsure if
I should base my patches on next or if Junio wants to keep your change?

Cheers Heiko
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] git subcommand to check if branch is up-to-date with upstream

[Sidhant Sharma]

Hi,
I was wondering if it would be a good idea to have a command to check if a
push or pull is required. Perhaps it can also suggest if changes are
fast-forward or the branches (local and remote) have diverged.
The reason I feel the need of such a command is when I need to check if my
branch is up-to-date with the remote, and when I need to know if my local
has diverged. Currently I use a script based on this stackoverflow answer[1]
Not an extremely useful tool, but I thought it'll be good to have it.

Warm regards,
Sidhant Sharma


[1] http://stackoverflow.com/a/3278427/5211579
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: "git log --format='format:%+s%+b'" doesn't insert newline before body

[Ilya Tumaykin]

On Monday 25 July 2016 10:06:28 Johannes Schindelin wrote:
> Hi Ilya,
> 
> On Mon, 25 Jul 2016, Ilya Tumaykin wrote:
> > On Monday 25 July 2016 09:33:00 Johannes Schindelin wrote:
> > > On Sun, 24 Jul 2016, Ilya Tumaykin wrote:
> > > [...]
> > > 
> > > > $ git --no-pager log -1 --format='format:%+s%+b'
> > > > 
> > > > Actual results:
> > > > ```
> > > > 
> > > > This is subject
> > > > And this is body
> > > > ```
> > 
> > According to 'git-log' man page '%+b' should insert linefeed "immediately
> > before the expansion if and only if the placeholder expands to a non-empty
> > string." Here "%b" expands to a non-empty string, thus I expect a linefeed
> > before it. Or am I misinterpreting man page somehow?
> 
> The line break is there: after the subject. The misinterpretation is most
> likely the assumption that the new-line "character" is part of the commit
> subject; It is not.

I see. Thank you very much for the explanation. This does help me.

-- 
Best regards.
Ilya Tumaykin.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Client exit whilst running pre-receive hook : commit accepted but no post-receive hook ran

[Jan Smets]

Hi

I have always assumed the post-receive hook to be executed whenever a 
commit is "accepted" by the (gitolite) server. That does not seem to be 
true any more.


Since 9658846 is appears that, when a client bails out, the pre-receive 
hook continues to run and the commit is written to the repository, but 
no post-receive hook is executed. No signal of any kind is received in 
the hook, not even a sig pipe when the post- hook is writing to stdout 
whilst the client has disconnected.



commit 9658846ce3d379b9ff8010a2ed326fcafc10eb82
Author: Jeff King 
Date:   Wed Feb 24 02:40:16 2016 -0500

write_or_die: handle EPIPE in async threads

diff --git a/write_or_die.c b/write_or_die.c
...
 static void check_pipe(int err)
 {
if (err == EPIPE) {
+   if (in_async())
+   async_exit(141);



Please keep me in CC as I am not subscribed to the list.

Thanks
Jan



The pre-receive hook from my quick testing => press Ctrl-C on the client 
when it is busy processing the 'sleep 5'
In my testing I was committing/pushing 32MB+ binary files that take some 
time to process.


#!/bin/bash
trap 'echo TRAP >> /tmp/gittest/log' 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 
16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31

IN=$(cat /dev/stdin)

echo -n $(date) >> /tmp/gittest/log
echo " : PRE START"  >> /tmp/gittest/log

for i in $(seq 1 10); do
  echo This is the pre-receive hook $i; sleep 0.1
done

# give time for client to ctrl-c out
sleep 5

echo -n $(date) >> /tmp/gittest/log
echo " : PRE END"  >> /tmp/gittest/log

# This should result in a sigpipe? but it isn't.
echo "Done !"
echo "Done !"

# no exit code -> accept commit



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] t5510: become resilient to GETTEXT_POISON

[Vasco Almeida]

Replace gettext poison text with appropriate values to be able to cut
the right output of git fetch command for comparison.

The first gettext poison falls from the previous line into the next
because the poison does not add a newline, so we must replace it with
nothing.

Signed-off-by: Vasco Almeida 
---
 t/t5510-fetch.sh | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 6bd4853..b261223 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -694,7 +694,10 @@ test_expect_success 'fetch aligned output' '
(
cd full-output &&
git -c fetch.output=full fetch origin 2>&1 | \
-   grep -e "->" | cut -c 22- >../actual
+   grep -e "->" | \
+   sed -e "/master/ s/# GETTEXT POISON #//" \
+   -e "/tag/ s/# GETTEXT POISON #/[new tag]/" 
| \
+   cut -c 22- >../actual
) &&
cat >expect <<-\EOF &&
master   -> origin/master
@@ -709,7 +712,10 @@ test_expect_success 'fetch compact output' '
(
cd compact &&
git -c fetch.output=compact fetch origin 2>&1 | \
-   grep -e "->" | cut -c 22- >../actual
+   grep -e "->" | \
+   sed -e "/master/ s/# GETTEXT POISON #//" \
+   -e "/extraa/ s/# GETTEXT POISON #/[new tag]
/" | \
+   cut -c 22- >../actual
) &&
cat >expect <<-\EOF &&
master -> origin/*
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] format-patch: escape "From " lines recognized by mailsplit

[Eric Wong]

Junio C Hamano  wrote:
> Junio C Hamano  writes:
> > Eric Wong  writes:
> >
> >> Users have mistakenly copied "From " lines into commit messages
> >> in the past, and will certainly make the same mistakes in the
> >> future.  Since not everyone uses mboxrd, yet, we should at least
> >> prevent miss-split mails by always escaping "From " lines based
> >> on the check used by mailsplit.
> >>
> >> mailsplit will not perform unescaping by default, yet, as it
> >> could cause further invocations of format-patch from old
> >> versions of git to generate bad output.  Propagating the mboxo
> >> escaping is preferable to miss-split patches.  Unescaping may
> >> still be performed via "--mboxrd".
> >
> > As a tool to produce mbox file, quoting like this in format-patch
> > output may make sense, I would think, but shouldn't send-email undo
> > this when sending individual patches?
> 
> Also, doesn't it break "git rebase" (non-interactive), or anything
> that internally runs format-patch to individual files and then runs
> am on each of them, anything that knows that each output file from
> format-patch corresponds to a single change and there is no need to
> split, badly if we do this unconditionally?

Yes, rebase should probably unescape is_from_line matches.

Anything which spawns an editor should probably warn/reprompt
users on is_from_line() matches, too, to prevent user errors
from sneaking in.

> IOW, shouldn't this be an optional feature to format-patch that is
> triggered by passing a new command line option that currently nobody
> is passing?

I added --pretty=mboxrd as the optional feature for this reason.
It'll take a while for people to start using it (or perhaps make
it the default in git 3.0).
In the meantime, I would prefer extra ">" being injected rather
than breaking mailsplit completely.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[ANNOUNCE] git-cinnabar 0.4.0 beta 2

[Mike Hommey]

Git-cinnabar is a git remote helper to interact with mercurial
repositories. It allows to clone, pull and push from/to mercurial remote
repositories, using git.

Code on https://github.com/glandium/git-cinnabar
This release on
https://github.com/glandium/git-cinnabar/releases/tag/0.4.0b2

[ Previous announcements:
  http://marc.info/?l=git=146762932928309
  http://marc.info/?l=git=146179749105388
  http://marc.info/?l=git=145294370431454
  http://marc.info/?l=git=145284823007354
  http://marc.info/?l=git=142837367709781 (...)]

What's new since 0.4.0b1?

- Some more bug fixes.
- Updated git to 2.9.2 for cinnabar-helper.
- Now supports `git push --dry-run`.
- Added a new `git cinnabar fetch` command to fetch a specific revision
  that is not necessarily a head.
- Some improvements to the experimental native wire protocol support.

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: "git log --format='format:%+s%+b'" doesn't insert newline before body

[Johannes Schindelin]

Hi Ilya,

On Mon, 25 Jul 2016, Ilya Tumaykin wrote:

> On Monday 25 July 2016 09:33:00 Johannes Schindelin wrote:
> > 
> > On Sun, 24 Jul 2016, Ilya Tumaykin wrote:
> > [...]
> > > $ git --no-pager log -1 --format='format:%+s%+b'
> > > 
> > > Actual results:
> > > ```
> > > 
> > > This is subject
> > > And this is body
> > > ```
> 
> According to 'git-log' man page '%+b' should insert linefeed "immediately 
> before the expansion if and only if the placeholder expands to a non-empty 
> string." Here "%b" expands to a non-empty string, thus I expect a linefeed 
> before it. Or am I misinterpreting man page somehow?

The line break is there: after the subject. The misinterpretation is most
likely the assumption that the new-line "character" is part of the commit
subject; It is not.

> > Unless you somehow allow empty commit messages (Git does not, unless
> > you play games with low-level commands), the second '+' is
> > unnecessarily conditional. Therefore "%s%n%+b" *might* do what you
> > intended (I would not know, because that information was missing from
> > the report).
> 
> I want to display '%B', but add colours to '%s'. Thus I have to
> separately include '%s' and '%b', and not just '%B'. I was using
> '%+s%n%+b' with extra colour formatting as a workaround for some time
> now.

Okay. Hopefully the explanation above clarifies that this is not a
work-around, but the correct solution.

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: "git log --format='format:%+s%+b'" doesn't insert newline before body

[Ilya Tumaykin]

On Monday 25 July 2016 09:33:00 Johannes Schindelin wrote:
> Hi Ilya,
> 
> On Sun, 24 Jul 2016, Ilya Tumaykin wrote:
> > Steps to reproduce:
> > $ git init
> > $ >123
> > $ git add 123
> > $ git commit -v -m 'This is subject' -m 'And this is body'
> > $ git --no-pager log -1 --format='format:%+s%+b'
> > 
> > Actual results:
> > ```
> > 
> > This is subject
> > And this is body
> > ```
> > 
> > Expected results:
> > ```
> > 
> > This is subject
> > 
> > And this is body
> > ```
> 
> The empty line between commit subject and body is neither part of the
> subject nor of the body. That means that the above-mentioned expectation
> was incorrect.

According to 'git-log' man page '%+b' should insert linefeed "immediately 
before the expansion if and only if the placeholder expands to a non-empty 
string." Here "%b" expands to a non-empty string, thus I expect a linefeed 
before it. Or am I misinterpreting man page somehow?

> Unless you somehow allow empty commit messages (Git does not, unless you
> play games with low-level commands), the second '+' is unnecessarily
> conditional. Therefore "%s%n%+b" *might* do what you intended (I would not
> know, because that information was missing from the report).

I want to display '%B', but add colours to '%s'. Thus I have to separately 
include '%s' and '%b', and not just '%B'. I was using '%+s%n%+b' with extra 
colour formatting as a workaround for some time now.

> Ciao,
> Johannes

-- 
Best regards.
Ilya Tumaykin.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: "git log --format='format:%+s%+b'" doesn't insert newline before body

[Johannes Schindelin]

Hi Ilya,

On Sun, 24 Jul 2016, Ilya Tumaykin wrote:

> Steps to reproduce:
> $ git init
> $ >123
> $ git add 123
> $ git commit -v -m 'This is subject' -m 'And this is body'
> $ git --no-pager log -1 --format='format:%+s%+b'
> 
> Actual results:
> ```
> 
> This is subject
> And this is body
> ```
> 
> Expected results:
> ```
> 
> This is subject
> 
> And this is body
> ```

The empty line between commit subject and body is neither part of the
subject nor of the body. That means that the above-mentioned expectation
was incorrect.

Unless you somehow allow empty commit messages (Git does not, unless you
play games with low-level commands), the second '+' is unnecessarily
conditional. Therefore "%s%n%+b" *might* do what you intended (I would not
know, because that information was missing from the report).

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/3] convert: add filter..useProtocol option

[Eric Wong]

Lars Schneider  wrote:
> On 23 Jul 2016, at 10:14, Eric Wong  wrote:
> > larsxschnei...@gmail.com wrote:
> >> +static struct cmd2process *start_protocol_filter(const char *cmd)
> >> +{
> >> +  int ret = 1;
> >> +  struct cmd2process *entry = NULL;
> >> +  struct child_process *process = NULL;
> > 
> > These are unconditionally set below, so initializing to NULL
> > may hide future bugs.
> 
> OK. I thought it is generally a good thing to initialize a pointer with 
> NULL. Can you explain to me how this might hide future bugs?
> I will remove the initialization.

Compilers complain about uninitialized variables.  Blindly
setting them to NULL can allow them to be dereferenced;
triggering segfaults; especially if it's passed to a different
compilation unit the compiler can't see.

> >> +static int apply_protocol_filter(const char *path, const char *src, 
> >> size_t len,
> >> +  int fd, struct strbuf *dst, 
> >> const char *cmd,
> >> +  const char *filter_type)
> >> +{



> >> +  if (fd >= 0 && !src) {
> >> +  ret &= fstat(fd, ) != -1;
> >> +  len = fileStat.st_size;
> > 
> > There's a truncation bug when sizeof(size_t) < sizeof(off_t)
> 
> OK. What would you suggest to do in that case? Should we just let the
> filter fail? Is there anything else we could do?

Anything which refers to something on disk (or will eventually
stored there, such as blobs) should evolve towards off_t rather
than size_t.  We just discovered a bunch of 32-bit truncation
bugs the other week:

https://public-inbox.org/git/1466807902.28869.8.ca...@gmail.com/

If the protocol/ABI is frozen, it should probably fail;
and a 64-bit-off_t version for 32-bit systems should be defined.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ANN] Pro Git Reedited 2nd Edition

[Johannes Schindelin]

Hi Jon,

On Sun, 24 Jul 2016, Jon Forrest wrote:

> On 7/24/2016 11:27 AM, Jakub Narębski wrote:
> 
> > All right. One issue I have after browsing through changes is that
> > description of changes and their granularity is severely lacking.  "A
> > few more very minor changes.", "More piddly changes.", "should have
> > included this in last commit" are not good commit messages.
> 
> You're absolutely right. I probably should have squashed those commits.
> Those comments aren't really intended for public consumption.
> Since I made many changes per commit, I really couldn't give an
> instructive commit message.
> 
> Now that this edition is done, I plan on following good commit
> practices in the future.

For the record: I found it a good practice to clean up my commits (read:
reword commit messages, split/join commits) *even after* pushing them to a
public repository.

Interactive rebase to the rescue.

That practice is not solely for others' benefit, by the way.

You will most likely find that it not only structures your work better,
you will also find that it turns up things that were forgotten and still
need to be addressed.

Ciao,
Johannes

Re: Bug:

[Beat Bolli]

Ilya Tumaykin  gmail.com> writes:

> $ git --no-pager log -1 --format='format:%+s%+b'
> Please fix.

Simply use `git --no-pager log -1 --format='format:%+s%n%+b'`

The message body excludes the empty line preceding it.





--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html