[PATCH] submodule operations: tighten pathspec errors

[Stefan Beller]

when the pathspec did not match any path, error out.
Add the `--error-unmatch` switch to use the old behavior.

Signed-off-by: Stefan Beller 
---

This was taken from the Left Over Bits:
   git submodule $cmd $pathspec may want to error out when the $pathspec does
   not match any submodules. There must be an --unmatch-ok option to override
   this safety, though. Cf. $gmane/289535
   
It's a first initial version with no tests (and probably conflicting with
some topics in flight), but I was curious how involved this issue actually is,
so I took a stab at implementing it.

I was debating if inside git-submodules.sh we want to pass around a variable
or instead export an environment variable GIT_SUBMODULE_UNMATCH, such that we
do not give it as an argument to the submodule--helper builtin.

(This version is developed on top of the sb/submodule-deinit-all)
   
 Documentation/git-submodule.txt | 15 ++-
 builtin/submodule--helper.c | 19 +++
 git-submodule.sh| 41 ++---
 3 files changed, 51 insertions(+), 24 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index ad85183..ceacc02 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -11,16 +11,16 @@ SYNOPSIS
 [verse]
 'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
  [--reference ] [--depth ] [--]  
[]
-'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
-'git submodule' [--quiet] init [--] [...]
-'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
+'git submodule' [--quiet] status [--error-unmatch] [--cached] [--recursive] 
[--] [...]
+'git submodule' [--quiet] init [--error-unmatch] [--] [...]
+'git submodule' [--quiet] deinit [--error-unmatch] [-f|--force] (--all|[--] 
...)
+'git submodule' [--quiet] update [--error-unmatch] [--init] [--remote] 
[-N|--no-fetch]
  [-f|--force] [--rebase|--merge] [--reference ]
  [--depth ] [--recursive] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
-'git submodule' [--quiet] sync [--recursive] [--] [...]
+'git submodule' [--quiet] sync [--error-unmatch] [--recursive] [--] [...]
 
 
 DESCRIPTION
@@ -260,6 +260,11 @@ OPTIONS
The name of the branch is recorded as `submodule..branch` in
`.gitmodules` for `update --remote`.
 
+--error-unmatch::
+   If the pathspec included a specification that did not match,
+   the usual operation is to error out. This switch suppresses
+   error reporting and continues the operation.
+
 -f::
 --force::
This option is only valid for add, deinit and update commands.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 5295b72..91c49ec 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -19,7 +19,8 @@ struct module_list {
 static int module_list_compute(int argc, const char **argv,
   const char *prefix,
   struct pathspec *pathspec,
-  struct module_list *list)
+  struct module_list *list,
+  int unmatch)
 {
int i, result = 0;
char *ps_matched = NULL;
@@ -36,10 +37,9 @@ static int module_list_compute(int argc, const char **argv,
 
for (i = 0; i < active_nr; i++) {
const struct cache_entry *ce = active_cache[i];
-
-   if (!match_pathspec(pathspec, ce->name, ce_namelen(ce),
-   0, ps_matched, 1) ||
-   !S_ISGITLINK(ce->ce_mode))
+   if (!S_ISGITLINK(ce->ce_mode) ||
+   !match_pathspec(pathspec, ce->name, ce_namelen(ce),
+   0, ps_matched, 1))
continue;
 
ALLOC_GROW(list->entries, list->nr + 1, list->alloc);
@@ -53,7 +53,9 @@ static int module_list_compute(int argc, const char **argv,
i++;
}
 
-   if (ps_matched && report_path_error(ps_matched, pathspec, prefix))
+   if (!unmatch &&
+   ps_matched &&
+   report_path_error(ps_matched, pathspec, prefix))
result = -1;
 
free(ps_matched);
@@ -63,11 +65,12 @@ static int module_list_compute(int argc, const char **argv,
 
 static int module_list(int argc, const char **argv, const char *prefix)
 {
-   int i;
+   int i, unmatch = 0;
struct pathspec pathspec;
struct module_list list = MODULE_LIST_INIT;
 
struct option module_list_options[] = {
+   OPT_BOOL(0, "unmatch", , N_("Do not report error if no 
matches are found")),
OPT_STRING(0, "prefix", ,
   

[PATCH 0/2] Persistent submodule pathspec specification

[Stefan Beller]

This was part of the former series 'submodule groups'.
However the labeling was ripped out and goes in its own series
sb/pathspec-label.

First we introduce a switch `--init-default-path` for `git submodule update`
which will read the pathspec to initialize the submodules not from the command
line but from `submodule.defaultUpdatePath`, which can be configured 
permanently.

The second patch utilizes this by having `clone` set that config option
and using that new option when calling to update the submodules.

Thanks,
Stefan

Stefan Beller (2):
  submodule update: add `--init-default-path` switch
  clone: add --init-submodule= switch

 6 files changed, 216 insertions(+), 14 deletions(-)
 Documentation/config.txt|   5 ++
 Documentation/git-clone.txt |  25 +---
 Documentation/git-submodule.txt |  11 +++-
 builtin/clone.c |  34 +-
 git-submodule.sh|  21 ++-
 t/t7400-submodule-basic.sh  | 134 

-- 
2.8.3.396.g0eed146

--
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] clone: add --init-submodule= switch

[Stefan Beller]

The new switch passes the pathspec to `git submodule update --init`
which is called after the actual clone is done.

Additionally this configures the submodule.defaultUpdatePath to
be the given pathspec, such that any future invocation of
`git submodule update --init-default-paths` will keep up
with the pathspec.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt | 25 +-
 builtin/clone.c | 34 +--
 t/t7400-submodule-basic.sh  | 81 +
 3 files changed, 130 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 1b15cd7..33e5894 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,8 +14,9 @@ SYNOPSIS
  [-o ] [-b ] [-u ] [--reference ]
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
- [--jobs ] [--]  []
+ [--recursive | --recurse-submodules] [--jobs ]
+ [--init-submodule ] [--] 
+ []
 
 DESCRIPTION
 ---
@@ -207,12 +208,20 @@ objects from the source repository into a pack in the 
cloned repository.
 
 --recursive::
 --recurse-submodules::
-   After the clone is created, initialize all submodules within,
-   using their default settings. This is equivalent to running
-   `git submodule update --init --recursive` immediately after
-   the clone is finished. This option is ignored if the cloned
-   repository does not have a worktree/checkout (i.e. if any of
-   `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+   After the clone is created, initialize and clone all submodules
+   within, using their default settings. This is equivalent to
+   running `git submodule update --recursive --init `
+   immediately after the clone is finished. This option is ignored
+   if the cloned repository does not have a worktree/checkout (i.e.
+   if any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+
+--init-submodule::
+   After the clone is created, initialize and clone specified
+   submodules within, using their default settings. It is possible
+   to give multiple specifications by giving this argument multiple
+   times. This is equivalent to configure `submodule.defaultUpdateGroup`
+   and then running `git submodule update --init-default-path`
+   immediately after the clone is finished.
 
 --[no-]shallow-submodules::
All submodules which are cloned will be shallow with a depth of 1.
diff --git a/builtin/clone.c b/builtin/clone.c
index 5f867e6..56967f9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -53,6 +53,16 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list init_submodules;
+
+static int init_submodules_cb(const struct option *opt, const char *arg, int 
unset)
+{
+   if (unset)
+   return -1;
+
+   string_list_append((struct string_list *)opt->value, arg);
+   return 0;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -103,6 +113,9 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_CALLBACK(0, "init-submodule", _submodules, N_(""),
+   N_("clone specific submodules. Pass ultiple times for 
complex pathspecs"),
+   init_submodules_cb),
OPT_END()
 };
 
@@ -734,14 +747,22 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive) {
+   if (!err && (option_recursive || init_submodules.nr > 0)) {
struct argv_array args = ARGV_ARRAY_INIT;
-   argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
+   argv_array_pushl(, "submodule", "update", NULL);
+
+   if (init_submodules.nr > 0)
+   argv_array_pushf(, "--init-default-path");
+   else
+   argv_array_pushf(, "--init");
 
if (option_shallow_submodules == 1
|| (option_shallow_submodules == -1 && option_depth))
argv_array_push(, "--depth=1");
 
+   if (option_recursive)
+   argv_array_pushf(, "--recursive");
+
if (max_jobs != -1)
argv_array_pushf(, "--jobs=%d", max_jobs);
 
@@ -883,6 +904,15 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
option_no_checkout = 1;
}
 
+  

[PATCH 1/2] submodule update: add `--init-default-path` switch

[Stefan Beller]

The new switch `--init-default-path` initializes the submodules which are
configured in `submodule.defaultUpdatePath` instead of those given as
command line arguments before updating. In the first implementation this
is made incompatible with further command line arguments as it is
unclear what the user means by

git submodule update --init --init-default-path 

This new switch allows to record more complex patterns as it saves
retyping them whenever you invoke update.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt|  5 
 Documentation/git-submodule.txt | 11 -
 git-submodule.sh| 21 +---
 t/t7400-submodule-basic.sh  | 53 +
 4 files changed, 86 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e4cd291..121b236 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2786,6 +2786,11 @@ submodule.fetchJobs::
in parallel. A value of 0 will give some reasonable default.
If unset, it defaults to 1.
 
+submodule.defaultUpdatePath::
+   Specifies a set of submodules to initialize when calling
+   `git submodule --init-default-group` by using the pathspec
+   syntax.
+
 tag.forceSignAnnotated::
A boolean to specify whether annotated tags created should be GPG 
signed.
If `--annotate` is specified on the command line, it takes
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 9226c43..000231e 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -14,7 +14,7 @@ SYNOPSIS
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
+'git submodule' [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch]
  [-f|--force] [--rebase|--merge] [--reference ]
  [--depth ] [--recursive] [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
@@ -193,6 +193,10 @@ If the submodule is not yet initialized, and you just want 
to use the
 setting as stored in .gitmodules, you can automatically initialize the
 submodule with the `--init` option.
 
+You can configure a set of submodules using pathspec syntax in
+submodule.defaultUpdatePath you can use `--init-default-path` to initialize
+those before updating.
+
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 --
@@ -360,6 +364,11 @@ the submodule itself.
Initialize all submodules for which "git submodule init" has not been
called so far before updating.
 
+--init-default-path::
+   This option is only valid for the update command.
+   Initialize all submodules configured in "`submodule.defaultUpdatePath`"
+   that have not been updated before.
+
 --name::
This option is only valid for the add command. It sets the submodule's
name to the given string instead of defaulting to its path. The name
diff --git a/git-submodule.sh b/git-submodule.sh
index 5a4dec0..3d12145 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--reference ] 
[--recursive] [--] [...]
+   or: $dashless [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] [--reference 
] [--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -528,7 +528,12 @@ cmd_update()
GIT_QUIET=1
;;
-i|--init)
-   init=1
+   test -z $init || test $init = by_args || die "$(gettext 
"Only one of --init or --init-default-path may be used.")"
+   init=by_args
+   ;;
+   --init-default-path)
+   test -z $init || test $init = by_config || die 
"$(gettext "Only one of --init or --init-default-path may be used.")"
+   init=by_config
;;
--remote)
remote=1
@@ -591,7 +596,17 @@ cmd_update()
 
if test -n "$init"
then
-   cmd_init "--" "$@" || return
+   if test "$init" = "by_config"
+   then
+   if test $# 

Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy

[Junio C Hamano]

Mike Hommey  writes:

> On Fri, May 20, 2016 at 02:48:28PM -0700, Junio C Hamano wrote:
>> So, even if we agree that per-port behaviour is not something we
>> would use if we were building the system without any existing users
>> today, I do not think we want "git now fails with an error" at all.
>> It goes against the approach Git takes to give smooth transtion to
>> users when we must break backward compatibility.
>
> I don't disagree. I went with a hard fail because it was easier. I'm
> not too keen blocking this series on this transition happening. So I'll
> try to finish this series without this change, and we can separate this
> transition discussion from the rest of the connect.c changes.

Yeah, I was thinking about the same thing as a short-term
direction.

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] userdiff: add built-in pattern for CSS

[Junio C Hamano]

William Duclot  writes:

> CSS is widely used, motivating it being included as a built-in pattern.
>
> It must be noted that the word_regex for CSS (i.e. the regex defining
> what is a word in the language) does not consider '.' and '#' characters
> (in CSS selectors) to be part of the word. This behavior is documented
> by the test t/t4018/css-rule.
> The logic behind this behavior is the following: identifiers in CSS
> selectors are identifiers in a HTML/XML document. Therefore, the '.'/'#'
> character are not part of the identifier, but an indicator of the nature
> of the identifier in HTML/XML (class or id). Diffing ".class1" and
> ".class2" must show that the class name is changed, but we still are
> selecting a class.

In other words, if "div#foo" changed to "span#bar", word-diff would
say that "div changed to span, # didn't change and foo changed to
bar".

Which makes sense to me.

The above is not a request to change anything; just me thinking
aloud to see if I agree with the reasoning.

> diff --git a/userdiff.c b/userdiff.c
> index 6bf2505..0f9cfbe 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -148,6 +148,14 @@ PATTERNS("csharp",
>"[a-zA-Z_][a-zA-Z0-9_]*"
>"|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
>"|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->"),
> +PATTERNS("css",
> +  "^([^,{}]+)((,[^}]*\\{)|([ \t]*\\{))$",
> +  /* -- */
> +  /* This regex comes from W3C CSS specs. Should theoretically also 
> allow ISO 10646 characters U+00A0 and higher,
> +   * this not handled in this regex. */
> +  "-?[_a-zA-F][-_a-zA-F0-9]*" /* identifiers */
> +  "|-?[0-9]+|\\#[0-9a-fA-F]+" /* numbers */
> +),

Style:

/*
 * This regex comes from ...
 * ...
 * but they are not handled with this regex.
 */

I wonder if IPATTERN() may make it easier to express the above.

Also, a-zA-F (twice seen in "identifiers" section) looks somewhat
suspicious.  a-fA-F or a-zA-Z I would understand, and I suspect this
is a misspelled form of the latter.
--
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 v6 2/9] connect: only match the host with core.gitProxy

[Mike Hommey]

On Fri, May 20, 2016 at 02:48:28PM -0700, Junio C Hamano wrote:
> So, even if we agree that per-port behaviour is not something we
> would use if we were building the system without any existing users
> today, I do not think we want "git now fails with an error" at all.
> It goes against the approach Git takes to give smooth transtion to
> users when we must break backward compatibility.

I don't disagree. I went with a hard fail because it was easier. I'm
not too keen blocking this series on this transition happening. So I'll
try to finish this series without this change, and we can separate this
transition discussion from the rest of the connect.c changes.

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: [PATCH v6 1/9] connect: call get_host_and_port() earlier

[Mike Hommey]

On Fri, May 20, 2016 at 03:20:23PM -0700, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> >> Can never happen because?
> >> 
> >>   !*port means get_host_and_port() made the "port" pointer point at
> >>   a NUL byte.  That does not happen because the only case port is
> >>   moved by that function is to have it point at a byte after we
> >>   found ':', and the "port" string is a decimal integer whose value
> >>   is between 0 and 65535, so there is no way port points at an empty
> >>   string.
> >> 
> >> OK.
> >
> > Do you want me to add this to the commit message in a possible v7?
> 
> No.
> 
> I was merely thinking aloud to see if "in a case that never can
> happen" is sufficient decsription.  I think it is ;-)
> 
> >> This looks strange
> > v3 of this series did remove this get_port(), and broke the
> > '[host:port]:path' syntax as a consequence. The reason this happens is
> > that get_host_and_port, in that case, is called with [host:port], sees
> > the square brackets, and searches the port *after* the closing bracket,
> > because the usual case where square brackets appear is ipv6 addresses,
> > which contain colons, and the brackets in that case are used to separate
> > the host and the port.
> >
> > In that case, get_host_and_port returns "host:port" and null.
> 
> Doesn't that indicate that this codepath deserves some in-code
> comment?

What would be the usual way you'd do this? separate patch? or just doing
it in one of the patches that touches the surrounding code?

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: [PATCH v6 1/9] connect: call get_host_and_port() earlier

[Junio C Hamano]

Mike Hommey  writes:

>> Can never happen because?
>> 
>>   !*port means get_host_and_port() made the "port" pointer point at
>>   a NUL byte.  That does not happen because the only case port is
>>   moved by that function is to have it point at a byte after we
>>   found ':', and the "port" string is a decimal integer whose value
>>   is between 0 and 65535, so there is no way port points at an empty
>>   string.
>> 
>> OK.
>
> Do you want me to add this to the commit message in a possible v7?

No.

I was merely thinking aloud to see if "in a case that never can
happen" is sufficient decsription.  I think it is ;-)

>> This looks strange
> v3 of this series did remove this get_port(), and broke the
> '[host:port]:path' syntax as a consequence. The reason this happens is
> that get_host_and_port, in that case, is called with [host:port], sees
> the square brackets, and searches the port *after* the closing bracket,
> because the usual case where square brackets appear is ipv6 addresses,
> which contain colons, and the brackets in that case are used to separate
> the host and the port.
>
> In that case, get_host_and_port returns "host:port" and null.

Doesn't that indicate that this codepath deserves some in-code
comment?
--
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

What's cooking in git.git (May 2016, #07; Fri, 20)

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

Almost all the topics marked as "will merge" in this issue should be
merged before 2.9-rc0 (scheduled for coming Monday).  There are a
few very interesting topics that we will be cooking throughout the
pre-release freeze in 'next', and I am already looking forward to
the cycle after this upcoming release ;-).

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

* jk/push-client-deadlock-fix (2016-05-11) 2 commits
  (merged to 'next' on 2016-05-11 at 8f4abf9)
 + Windows: only add a no-op pthread_sigmask() when needed
  (merged to 'next' on 2016-05-06 at e91626c)
 + Windows: add pthread_sigmask() that does nothing

 Some Windows SDK lacks pthread_sigmask() implementation and fails
 to compile the recently updated "git push" codepath that uses it.

--
[New Topics]

* jk/cat-file-buffered-batch-all (2016-05-18) 2 commits
 - cat-file: default to --buffer when --batch-all-objects is used
 - cat-file: avoid noop calls to sha1_object_info_extended

 "git cat-file --batch-all" has been sped up, by taking advantage
 of the fact that it does not have to read a list of objects, in two
 ways.

 Will merge to 'next'.


* ah/no-verify-signature-with-pull-rebase (2016-05-20) 1 commit
 - pull: warn on --verify-signatures with --rebase

 "git pull --rebase --verify-signature" learned to warn the user
 that "--verify-signature" is a no-op.

 Will merge to 'next'.


* ak/t0008-ksh88-workaround (2016-05-20) 1 commit
 - t0008: 4 tests fail with ksh88

 Test portability workaround.

 Will merge to 'next'.

--
[Stalled]


* sb/bisect (2016-04-15) 22 commits
 - SQUASH???
 - bisect: get back halfway shortcut
 - bisect: compute best bisection in compute_relevant_weights()
 - bisect: use a bottom-up traversal to find relevant weights
 - bisect: prepare for different algorithms based on find_all
 - bisect: rename count_distance() to compute_weight()
 - bisect: make total number of commits global
 - bisect: introduce distance_direction()
 - bisect: extract get_distance() function from code duplication
 - bisect: use commit instead of commit list as arguments when appropriate
 - bisect: replace clear_distance() by unique markers
 - bisect: use struct node_data array instead of int array
 - bisect: get rid of recursion in count_distance()
 - bisect: make algorithm behavior independent of DEBUG_BISECT
 - bisect: make bisect compile if DEBUG_BISECT is set
 - bisect: plug the biggest memory leak
 - bisect: add test for the bisect algorithm
 - t6030: generalize test to not rely on current implementation
 - t: use test_cmp_rev() where appropriate
 - t/test-lib-functions.sh: generalize test_cmp_rev
 - bisect: allow 'bisect run' if no good commit is known
 - bisect: write about `bisect next` in documentation

 The internal algorithm used in "git bisect" to find the next commit
 to check has been optimized greatly.

 Expecting a reroll.
 ($gmane/291163)


* nd/shallow-deepen (2016-04-13) 26 commits
 - fetch, upload-pack: --deepen=N extends shallow boundary by N commits
 - upload-pack: add get_reachable_list()
 - upload-pack: split check_unreachable() in two, prep for get_reachable_list()
 - t5500, t5539: tests for shallow depth excluding a ref
 - clone: define shallow clone boundary with --shallow-exclude
 - fetch: define shallow boundary with --shallow-exclude
 - upload-pack: support define shallow boundary by excluding revisions
 - refs: add expand_ref()
 - t5500, t5539: tests for shallow depth since a specific date
 - clone: define shallow clone boundary based on time with --shallow-since
 - fetch: define shallow boundary with --shallow-since
 - upload-pack: add deepen-since to cut shallow repos based on time
 - shallow.c: implement a generic shallow boundary finder based on rev-list
 - fetch-pack: use a separate flag for fetch in deepening mode
 - fetch-pack.c: mark strings for translating
 - fetch-pack: use a common function for verbose printing
 - fetch-pack: use skip_prefix() instead of starts_with()
 - upload-pack: move rev-list code out of check_non_tip()
 - upload-pack: tighten number parsing at "deepen" lines
 - upload-pack: use skip_prefix() instead of starts_with()
 - upload-pack: move "unshallow" sending code out of deepen()
 - upload-pack: remove unused variable "backup"
 - upload-pack: move "shallow" sending code out of deepen()
 - upload-pack: move shallow deepen code out of receive_needs()
 - transport-helper.c: refactor set_helper_option()
 - remote-curl.c: convert fetch_git() to use 

Re: [PATCH v6 1/9] connect: call get_host_and_port() earlier

[Mike Hommey]

On Fri, May 20, 2016 at 01:58:26PM -0700, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> > Currently, get_host_and_port() is called in git_connect() for the ssh
> > protocol, and in git_tcp_connect_sock() for the git protocol. Instead
> > of doing this, just call it from a single place, right after
> > parse_connect_url(), and pass the host and port separately to
> > git_*_connect() functions.
> >
> > We however preserve hostandport, at least for now.
> >
> > Note that in git_tcp_connect_sock, the port was set to "" in a
> > case that never can happen, so that code path was removed.
> 
> Can never happen because?
> 
>   !*port means get_host_and_port() made the "port" pointer point at
>   a NUL byte.  That does not happen because the only case port is
>   moved by that function is to have it point at a byte after we
>   found ':', and the "port" string is a decimal integer whose value
>   is between 0 and 65535, so there is no way port points at an empty
>   string.
> 
> OK.

Do you want me to add this to the commit message in a possible v7?

> >  struct child_process *git_connect(int fd[2], const char *url,
> >   const char *prog, int flags)
> > ...
> > @@ -683,6 +681,8 @@ struct child_process *git_connect(int fd[2], const char 
> > *url,
> > signal(SIGCHLD, SIG_DFL);
> >  
> > protocol = parse_connect_url(url, , );
> > +   host = xstrdup(hostandport);
> > +   get_host_and_port(, );
> > if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
> > printf("Diag: url=%s\n", url ? url : "NULL");
> > printf("Diag: protocol=%s\n", prot_name(protocol));
> > ...
> > @@ -737,22 +737,20 @@ struct child_process *git_connect(int fd[2], const 
> > char *url,
> > if (protocol == PROTO_SSH) {
> > const char *ssh;
> > int putty = 0, tortoiseplink = 0;
> > -   char *ssh_host = hostandport;
> > -   const char *port = NULL;
> > transport_check_allowed("ssh");
> > -   get_host_and_port(_host, );
> >  
> > if (!port)
> > -   port = get_port(ssh_host);
> > +   port = get_port(host);
> 
> This looks strange.  We asked get_host_and_port() to split
> hostandport into host and port already, and learned that port is
> empty, i.e. it wasn't : where  is a decimal
> integer between 0 and 65535.  It could have been ":" in which
> case get_host_and_port() would have turned that colon into NUL.
> 
> Would there be a case where this get_port() call does anthing
> useful?

v3 of this series did remove this get_port(), and broke the
'[host:port]:path' syntax as a consequence. The reason this happens is
that get_host_and_port, in that case, is called with [host:port], sees
the square brackets, and searches the port *after* the closing bracket,
because the usual case where square brackets appear is ipv6 addresses,
which contain colons, and the brackets in that case are used to separate
the host and the port.

In that case, get_host_and_port returns "host:port" and null.

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] A part of an edge from an octopus merge gets colored, even with --color=never

[Noam Postavsky]

On Tue, May 17, 2016 at 3:55 PM, Jeff King  wrote:
>> (It's actually the first one which triggers). I'm not familiar enough
>> with the code to know whether the col_num computation is bogus, or
>> whether we needed to earlier increase the size of the "new_columns"
>> field.
>
> And unsurprisingly, reverting 339c17bc7690b5436ac61c996cede3d52c85b50d
> seems to fix this (author cc'd). It's the extra "commit_index" addition
> that causes the problem. But I'm still not sure what the correct
> solution is.

Looking at the coloured output, for some octopus merges where the
first parent edge immediately merges into the next column to the left,
col_num should be decremented by 1 (otherwise the colour of the "-."
doesn't match the rest of that edge).

| | *-.
| | |\ \
| |/ / /

For the other case where the first parent edge stays straight, the
current col_num computation is correct.

| *-.
| |\ \
| | | *

I'm not sure how to distinguish these cases in the code though. Is it
enough to just compare against graph->num_new_columns?
--
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] pull: warn on --verify-signatures with --rebase

[Junio C Hamano]

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 v6 2/9] connect: only match the host with core.gitProxy

[Junio C Hamano]

Mike Hommey  writes:

> Currently, core.gitProxy doesn't actually match purely on domain names
> as documented: it also matches ports.
>
> So a core.gitProxy value like "script for kernel.org" doesn't make the
> script called for an url like git://kernel.org:port/path, while it is
> called for git://kernel.org/path.

Let's recap.  When you are accessing "git://kernel.org:9418/path"

 - "script1 for kernel.org" does not match, which may be
   counter-intuitive;

 - "script2 for kernel.org:9418" matches and gets used.

Isn't that what the current code does?  If so, I suspect that
allowing the first one to match may be an improvement with slight
backward incompatibility that we agreed that we do not mind.

Note for those other than Mike: The reason why we do not
care about the breakage of the backward compatibility is
because it would only matter when you configured both
"script1 for kernel.org" and "script2 for kernel.org:9418",
so that URL can be used to differenciate which proxy script
to use.  In such a configuration, we used to call script2,
but "fixing" it would make it call script1.  Given that the
proxy script takes the port number as its parameter, this
can be worked around easily by introducing a new script0
that switches between script1 and script2 based on the port
number parameter.  IOW, we accept the compatibility breakage
because adjusting is easy.

We of course need to warn the user if we do this.  That is

 - If the URL we access has :, and we apply -only
   core.gitProxy entry to it, and

 - If the user has core.gitProxy entry for :,

we definitely need to warn so that the user is told that the "easy
adjustment" is necessary.

Thinking aloud a bit more, if the user has "script1 for kernel.org"
without "script2 for kernel.org:9418", and relied on the fact that
having :9418 that is not necessary (as it is the default port) in
the URL lets her bypass the proxy script, the user also needs to be
told that we have changed the rule of the game and her configuration
needs to be updated.  So the above condition to warn would need to
be loosened, i.e. "if the URL being accessed has :, and if we
apply gitProxy specified with only " we need to warn.

A proper transition plan is necessary; if the long term ideal is to
use "script1 for kernel.org" for "git://kernel.org:9418/path", we do
not want to keep giving this warning forever.

And if we need a transition plan _anyway_, we probably should have a
period during which those who have been relying on the fact that
"script2 for kernel.org:9418" was a supported way to specify proxy
for "git://kernel.org:9418/path" would get a warning but will still
use "script2 for kernel.org:9418" as a valid core.gitProxy
specification.

> This per-port behavior seems like an oversight rather than a deliberate
> choice, so, make git://kernel.org:port/path call the gitProxy script in
> the case described above.

So, even if we agree that per-port behaviour is not something we
would use if we were building the system without any existing users
today, I do not think we want "git now fails with an error" at all.
It goes against the approach Git takes to give smooth transtion to
users when we must break backward compatibility.

> However, if people have been relying on this behavior, git now fails
> with an error message inviting for a configuration change.
--
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] t0008: 4 tests fail with ksh88

[Christian Couder]

On Fri, May 20, 2016 at 6:10 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
> From: Armin Kunaschik 
> Date: Fri, 20 May 2016 16:31:30 +0200
> Subject: [PATCH] t0008: 4 tests fail with ksh88
>
> In t0008, we have
>
> cat <<-EOF
> ...
> a/b/.gitignore:8:!on*   "a/b/one\"three"
> ...
> EOF
>
> ane expect that the backslash-dq is passed through literally.

s/ane/and/

> ksh88 eats \ and generates a wrong expect data to compare with.
>
> Using \\" works this around without breaking other POSIX shells
> (which collapse backslash-backslash to a single backslash), and
> ksh88 does so, 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 v6 1/9] connect: call get_host_and_port() earlier

[Junio C Hamano]

Mike Hommey  writes:

> Currently, get_host_and_port() is called in git_connect() for the ssh
> protocol, and in git_tcp_connect_sock() for the git protocol. Instead
> of doing this, just call it from a single place, right after
> parse_connect_url(), and pass the host and port separately to
> git_*_connect() functions.
>
> We however preserve hostandport, at least for now.
>
> Note that in git_tcp_connect_sock, the port was set to "" in a
> case that never can happen, so that code path was removed.

Can never happen because?

  !*port means get_host_and_port() made the "port" pointer point at
  a NUL byte.  That does not happen because the only case port is
  moved by that function is to have it point at a byte after we
  found ':', and the "port" string is a decimal integer whose value
  is between 0 and 65535, so there is no way port points at an empty
  string.

OK.

>  struct child_process *git_connect(int fd[2], const char *url,
> const char *prog, int flags)
> ...
> @@ -683,6 +681,8 @@ struct child_process *git_connect(int fd[2], const char 
> *url,
>   signal(SIGCHLD, SIG_DFL);
>  
>   protocol = parse_connect_url(url, , );
> + host = xstrdup(hostandport);
> + get_host_and_port(, );
>   if ((flags & CONNECT_DIAG_URL) && (protocol != PROTO_SSH)) {
>   printf("Diag: url=%s\n", url ? url : "NULL");
>   printf("Diag: protocol=%s\n", prot_name(protocol));
> ...
> @@ -737,22 +737,20 @@ struct child_process *git_connect(int fd[2], const char 
> *url,
>   if (protocol == PROTO_SSH) {
>   const char *ssh;
>   int putty = 0, tortoiseplink = 0;
> - char *ssh_host = hostandport;
> - const char *port = NULL;
>   transport_check_allowed("ssh");
> - get_host_and_port(_host, );
>  
>   if (!port)
> - port = get_port(ssh_host);
> + port = get_port(host);

This looks strange.  We asked get_host_and_port() to split
hostandport into host and port already, and learned that port is
empty, i.e. it wasn't : where  is a decimal
integer between 0 and 65535.  It could have been ":" in which
case get_host_and_port() would have turned that colon into NUL.

Would there be a case where this get_port() call does anthing
useful?
--
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] pull: warn on --verify-signatures with --rebase

[Alexander 'z33ky' Hirsch]

Previously git-pull silently ignored the --verify-signatures option for
--rebase, potentially leaving users in the believe that the rebase
operation would check for valid GPG signatures.

Implementing --verify-signatures for git-rebase was talked about, but
doubts for a valid workflow rose up.
Since you usually merge other's branches into your branch you might have
an interest that their side has a valid GPG signature.
Rebasing, on the other hand, is you building something on top of
another's branch, essentially giving you the power to keep things sane.
If a valid use case is found, where --verify-signatures for git-rebase
looks sensible, that feature may be added then.

A warning was chosen in favor of emitting an error to prevent potential
breakage. A warning keeps things running, scripts for instance, while
still informing users about possibly unexpected behavior.

Signed-off-by: Alexander 'z33ky' Hirsch <1ze...@gmail.com>
---

The warning message was changed to make it clear that the pull (and
rebase) operation still proceeds.

And the commit message was amended with more reasoning about the change
and why alternative approaches were not used.

 builtin/pull.c  |  3 +++
 t/t5520-pull.sh | 16 
 2 files changed, 19 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 1d7333c..b03bc39 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -815,6 +815,9 @@ static int run_rebase(const unsigned char *curr_head,
argv_array_push(, "--no-autostash");
else if (opt_autostash == 1)
argv_array_push(, "--autostash");
+   if (opt_verify_signatures &&
+   strcmp(opt_verify_signatures, "--verify-signatures") == 0)
+   warning(_("ignoring --verify-signatures for rebase"));
 
argv_array_push(, "--onto");
argv_array_push(, sha1_to_hex(merge_head));
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 739c089..3159956 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -341,6 +341,22 @@ test_expect_success 'branch.to-rebase.rebase should 
override pull.rebase' '
test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success "pull --rebase warns on --verify-signatures" '
+   git reset --hard before-rebase &&
+   git pull --rebase --verify-signatures . copy 2>err &&
+   test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+   test new = "$(git show HEAD:file2)" &&
+   test_i18ngrep "ignoring --verify-signatures for rebase" err
+'
+
+test_expect_success "pull --rebase does not warn on --no-verify-signatures" '
+   git reset --hard before-rebase &&
+   git pull --rebase --no-verify-signatures . copy 2>err &&
+   test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+   test new = "$(git show HEAD:file2)" &&
+   test_i18ngrep ! "verify-signatures" err
+'
+
 # add a feature branch, keep-merge, that is merged into master, so the
 # test can try preserving the merge commit (or not) with various
 # --rebase flags/pull.rebase settings.
-- 
2.8.2

--
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 v6 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Junio C Hamano]

Elia Pinto  writes:

> diff --git a/http.c b/http.c
> index df6dd01..ba32bac 100644
> --- a/http.c
> +++ b/http.c
> @@ -11,6 +11,7 @@
>  #include "gettext.h"
>  #include "transport.h"
>  
> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
>  #if LIBCURL_VERSION_NUM >= 0x070a08
>  long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
>  #else
> @@ -477,6 +478,125 @@ static void set_curl_keepalive(CURL *c)
>  }
>  #endif
>  
> +static void curl_dump_header(const char *text, unsigned char *ptr, size_t 
> size, int nopriv_header)
> +{
> + struct strbuf out = STRBUF_INIT;
> + const char *header;
> + struct strbuf **header_list, **ptr_list;
> +
> + strbuf_addf(, "%s, %10.10ld bytes (0x%8.8lx)\n",
> + text, (long)size, (long)size);
> + trace_strbuf(_curl, );
> + strbuf_reset();
> + strbuf_add(,ptr,size);
> + header_list = strbuf_split_max(, '\n', 0);
> +
> + for (ptr_list = header_list; *ptr_list; ptr_list++) {
> + /*
> +  * if we are called with nopriv_header substitute a dummy value
> +  * in the Authorization or Proxy-Authorization http header if
> +  * present.
> +  */
> + if (nopriv_header &&
> + (skip_prefix((*ptr_list)->buf , "Authorization:", )
> + || skip_prefix((*ptr_list)->buf , "Proxy-Authorization:", 
> ))) { 
> + /* The first token is the type, which is OK to log */
> + while (isspace(*header))
> + header++;
> + while (*header && !isspace(*header))
> + header++;
> + /* Everything else is opaque and possibly sensitive */
> + strbuf_setlen((*ptr_list),  header - (*ptr_list)->buf );
> + strbuf_addstr((*ptr_list), " ");
> + }
> + strbuf_insert((*ptr_list), 0, text, strlen(text));
> + strbuf_insert((*ptr_list), strlen(text), ": ", 2);
> + strbuf_rtrim((*ptr_list));
> + strbuf_addch((*ptr_list), '\n');
> + trace_strbuf(_curl, (*ptr_list));

This funny indentation makes me wonder why you didn't make it into a
helper function.  If it would require too many parameters, I could
understand the aversion, as it would end up looking like:

for (header = headers; *header; header++) {
if (hide_sensitive_header)
redact_sensitive_header(header, , , ,
, ,
, , );
strbuf_insert(*header, 0, text, strlen(text));
strbuf_insert(*header, strlen(text), ": ", 2);
...
trace_strbuf(_curl, *header);
}

but I think redact_sensitive_header() helper would only need to take
a single strbuf, from the look of your actual implementation.

Yes, I am also suggesting to rename header_list and ptr_list to
headers and header, respectively.  header_list may be an OK name (as
it is "a list, each element of which is a header"), but ptr_list is
a poor name--"a pointer into a list" is a much less interesting thing
for the reader of the code to learn from the name than it represents
"one header".

And your "const char *header" does not have to be here (it would be
an implementation detail of redact_sensitive_header() function), so
such a renaming would not cause conflicts in variable names.

> + }
> + strbuf_list_free(header_list);
> + strbuf_release();
> +}
> +static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)

A blank line, between the end of previous function body and the
begining of this function, is missing.

> +{
> + size_t i;
> + struct strbuf out = STRBUF_INIT;
> + unsigned int width = 80;

In a few places Git limits the width of the output, like using function
name in hunk header lines and drawing diffstat in "diff --stat", we
do default to limit the total width to 80 display columns.

Given that this routine prefixes each and every line with a short
heading like "=> Send header: " that costs at around 15-20 columns,
and the loop we see below only counts the true payload without
counting the heading, you would probably want to reduce this
somewhat so that the whole thing would fit within 80 columns.

> + strbuf_addf(, "%s, %10.10ld bytes (0x%8.8lx)\n",
> + text, (long)size, (long)size);
> + trace_strbuf(_curl, );
> +
> + for (i = 0; i < size; i += width) {
> + size_t w;
> +
> + strbuf_reset();
> + strbuf_addf(, "%s: ", text);
> + for (w = 0; (w < width) && (i + w < size); w++) {
> + strbuf_addch(, (ptr[i + w] >= 0x20)
> + && (ptr[i + w] < 0x80) ? ptr[i + w] : '.');

Please think twice to make sure a long expression that has to span
multiple lines is cut at a sensible place.  This cuts at the worst
place where the syntactic element that binds strongest sits in the
expression.

The weakest 

Re: [PATCHv9] pathspec: allow querying for attributes

[Junio C Hamano]

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

[PATCHv9] pathspec: allow querying for attributes

[Stefan Beller]

The pathspec mechanism is extended via the new
":(attr:eol=input)pattern/to/match" syntax to filter paths so that it
requires paths to not just match the given pattern but also have the
specified attrs attached for them to be chosen.

Signed-off-by: Stefan Beller 
---

Junio, please replace the last patch of sb/pathspec-label with this one.

diff to last round:
#   diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
#   index e06520b..181f52e 100644
#   --- a/Documentation/glossary-content.txt
#   +++ b/Documentation/glossary-content.txt
#   @@ -389,7 +389,7 @@ After `attr:` comes a space separated list of 
"attribute
#requirements", all of which must be met in order for the
#path to be considered a match; this is in addition to the
#usual non-magic pathspec pattern matching.
#   -
#   ++
#Each of the attribute requirements for the path takes one of
#these forms:
#
#   diff --git a/pathspec.c b/pathspec.c
#   index 693a5e7..0a02255 100644
#   --- a/pathspec.c
#   +++ b/pathspec.c
#   @@ -115,19 +115,19 @@ static void parse_pathspec_attr_match(struct 
pathspec_item *item, const char *va
#   const char *attr = si->string;
#   struct attr_match *am = >attr_match[j];
#
#   -   attr_len = strcspn(attr, "=");
#   switch (*attr) {
#   case '!':
#   am->match_mode = MATCH_UNSPECIFIED;
#   attr++;
#   -   attr_len--;
#   +   attr_len = strlen(attr);
#   break;
#   case '-':
#   am->match_mode = MATCH_UNSET;
#   attr++;
#   -   attr_len--;
#   +   attr_len = strlen(attr);
#   break;
#   default:
#   +   attr_len = strcspn(attr, "=");
#   if (attr[attr_len] != '=')
#   am->match_mode = MATCH_SET;
#   else {
#   diff --git a/t/t6134-pathspec-with-labels.sh 
b/t/t6134-pathspec-with-labels.sh
#   index 5da1a63..a5c9632 100755
#   --- a/t/t6134-pathspec-with-labels.sh
#   +++ b/t/t6134-pathspec-with-labels.sh
#   @@ -40,8 +40,7 @@ test_expect_success 'setup a tree' '
#'
#
#test_expect_success 'pathspec with no attr' '
#   -   test_must_fail git ls-files ":(attr:)" 2>actual &&
#   -   test_i18ngrep fatal actual
#   +   test_must_fail git ls-files ":(attr:)"
#'
#
#test_expect_success 'pathspec with labels and non existent 
.gitattributes' '
#   @@ -156,8 +155,12 @@ test_expect_success 'check label excluding other 
labels' '
#'
#
#test_expect_success 'abort on giving invalid label on the command 
line' '
#   -   test_must_fail git ls-files . ":(attr:☺)" 2>actual &&
#   -   test_i18ngrep "fatal" actual
#   +   test_must_fail git ls-files . ":(attr:☺)"
#   +'
#   +
#   +test_expect_success 'abort on asking for wrong magic' '
#   +   test_must_fail git ls-files . ":(attr:-label=foo)" &&
#   +   test_must_fail git ls-files . ":(attr:!label=foo)"
#'
#
#test_done

 Documentation/glossary-content.txt |  20 +
 dir.c  |  35 
 pathspec.c | 101 +-
 pathspec.h |  16 
 t/t6134-pathspec-with-labels.sh| 166 +
 5 files changed, 334 insertions(+), 4 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index cafc284..181f52e 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -384,6 +384,26 @@ full pathname may have special meaning:
 +
 Glob magic is incompatible with literal magic.
 
+attr;;
+After `attr:` comes a space separated list of "attribute
+requirements", all of which must be met in order for the
+path to be considered a match; this is in addition to the
+usual non-magic pathspec pattern matching.
++
+Each of the attribute requirements for the path takes one of
+these forms:
+
+- "`ATTR`" requires that the attribute `ATTR` must be set.
+
+- "`-ATTR`" requires that the attribute `ATTR` must be unset.
+
+- "`ATTR=VALUE`" requires that the attribute `ATTR` must be
+  set to the string `VALUE`.
+
+- "`!ATTR`" requires that the attribute `ATTR` must be
+  unspecified.
++
+
 exclude;;
After a path matches any non-exclude pathspec, it will be run
through all exclude pathspec (magic 

Re: run-command: output owner picking strategy

[Junio C Hamano]

Stefan Beller  writes:

> I choose "as much live output" as an approximation of "least amount buffered
> over time, i.e. if you were to integrate the buffer size over time
> that should be
> minimized. (c.f. users waiting for output: http://imgur.com/gallery/lhjhbB9)
> I am not sure if that is ultimate thing to optimize for though.

It might be interesting to optimize to minimize the time the user
needs to stare an inactive screen, i.e. make sure there always is
something flowing.  That may involve throttling the output from a
buffer if that is the only buffer with any output, the foreground
thing hasn't made any output yet, and expected to stay silent for a
while, etc.



--
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: run-command: output owner picking strategy

[Stefan Beller]

On Fri, May 20, 2016 at 11:29 AM, William Duclot
 wrote:
>> When running in parallel we already may be out of order
>> (relative to serial processing). See the second example in the
>> commit message to produce a different order.
>
> Right, I could (should) have understood that by myself.
>
>> Consider we scheduled tasks to be run in 3 parallel processes:
>> (As we NEEDSWORK comment only addresses the ouput selection,
>> let's assume this is a fixes schedule, which we cannot alter.
>> Which is true if we only change the code you quoted. That picks
>> the process to output.)
>>
>> [...]
>
>> The output is produced by the current algorithm:
>> (1) Start with process 1 (A) whose output will be live
>> (2) Once A is done, flush all other done things, (B)
>> (3) live output will be round robin, so process 2 (D)
>> (4) Once D is done, flush all other done things (C, F, E)
>> in order of who finshed first
>>
>>
>> (1) is uncontroversial. We have no information about tasks A,B,C,
>> so pick a random candidate. We hardcoded process 1 for now.
>>
>> (2) also uncontroversial IMHO. There is not much we can do different.
>
> Agreed
>
>> (3) is what this NEEDSWORK comment is about. Instead of outputting D
>> we might have choosen C. (for $REASONS, e.g.: C is running longer than
>> D already, so we expect it to finish sooner, by assuming
>> any task takes the same expected time to finish. And as C
>> is expected to finish earlier than D, we may have smoother
>> output. "Less buffered bursts")
>>
>> [...]
>>
>> This seems to be better than the current behavior as we have more
>> different tasks with "live" output, i.e. you see stuff moving.
>> I made up the data to make the point though. We would need to use
>> live data and experiment with different strategies to find a
>> good/better solution.
>
> We should probably settle on what is the behavior we want to obtain,
> before trying to find a strategy to implement (or approximate) it:
> - Do we want to be as close as possible to a serial processing output?
> - Do we want to see as much live output as possible?
>
> I do not think that being close to serial processing is a relevant
> behavior: we applied an arbitrary order to tasks when naming them for
> explanations (A, B, C...), but the tasks aren't really sorted in any
> way (and that's why the parallelization is relevant).Neither the user
> nor git have any interest in getting these ouputs in a specific order.

IIRC In serial processing the output was according to the sort order
within the tree. I agree that this sorting property is of no value to the user.

>
> Therefore, a "as much live output as possible" behavior would be more
> sensible.

I choose "as much live output" as an approximation of "least amount buffered
over time, i.e. if you were to integrate the buffer size over time
that should be
minimized. (c.f. users waiting for output: http://imgur.com/gallery/lhjhbB9)
I am not sure if that is ultimate thing to optimize for though.

> But I wonder: is there a worthy benefit in optimizing the
> output owner strategy?

Eventually there are more users than just submodules for this
parallel processing machinery, I would hope. They would also benefit
of a good fundament?

> I'm not used to working with submodules, but I
> don't think that having a great number of submodules is a common thing.

(Not yet, because of the chicken and egg problem: submodule UI is not
as polished because very few people use it. And few people use it because
of confusing UI. ;)

At his GitMerge2016 talk, Lars Schneider proposed a guideline to
not use more than 25 submodules as it "doesn't scale" IIRC.
And that resentment seems to be all over the place.

> Basically: we could solve a problem, but is there a problem?
> I'm not trying to bury this NEEDSWORK, I'd be happy to look into it if
> need be!

Well Git as a community doesn't ask you to solve any problems. ;)
So if you have fun thinking about scheduling problems (and as you do
it as part of a university project, if Matthieu is happy about this problem
also), go for it :)

If you find another more "interesting" problem (either as defined by
you personal interests or by the possible impact or by possible grading),
choose that?

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: [PATCHv9 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Stefan Beller  writes:

> I checked and it looks wrong. the "exclude" section is indented below
> the new attr section
> ...
> I can resend with your proposed fixes as well.

If you do so, please make sure that the way tests check for error
condition are consistent.  I personally do not think it adds any
value to grep for "fatal", but I do not mind if you adjusted them to
go the same test_i18ngrep route if you think it does (and if you
agree that grepping is not necessary, please do not forget to update
the tests you had that use test_i18ngrep).

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: run-command: output owner picking strategy

[William Duclot]

> When running in parallel we already may be out of order
> (relative to serial processing). See the second example in the
> commit message to produce a different order.

Right, I could (should) have understood that by myself.
 
> Consider we scheduled tasks to be run in 3 parallel processes:
> (As we NEEDSWORK comment only addresses the ouput selection,
> let's assume this is a fixes schedule, which we cannot alter.
> Which is true if we only change the code you quoted. That picks
> the process to output.)
> 
> [...]
 
> The output is produced by the current algorithm:
> (1) Start with process 1 (A) whose output will be live
> (2) Once A is done, flush all other done things, (B)
> (3) live output will be round robin, so process 2 (D)
> (4) Once D is done, flush all other done things (C, F, E)
> in order of who finshed first
> 
> 
> (1) is uncontroversial. We have no information about tasks A,B,C,
> so pick a random candidate. We hardcoded process 1 for now.
> 
> (2) also uncontroversial IMHO. There is not much we can do different.

Agreed
 
> (3) is what this NEEDSWORK comment is about. Instead of outputting D
> we might have choosen C. (for $REASONS, e.g.: C is running longer than
> D already, so we expect it to finish sooner, by assuming
> any task takes the same expected time to finish. And as C
> is expected to finish earlier than D, we may have smoother
> output. "Less buffered bursts")
> 
> [...]
> 
> This seems to be better than the current behavior as we have more
> different tasks with "live" output, i.e. you see stuff moving.
> I made up the data to make the point though. We would need to use
> live data and experiment with different strategies to find a
> good/better solution.

We should probably settle on what is the behavior we want to obtain, 
before trying to find a strategy to implement (or approximate) it:
- Do we want to be as close as possible to a serial processing output? 
- Do we want to see as much live output as possible?

I do not think that being close to serial processing is a relevant 
behavior: we applied an arbitrary order to tasks when naming them for
explanations (A, B, C...), but the tasks aren't really sorted in any
way (and that's why the parallelization is relevant).Neither the user
nor git have any interest in getting these ouputs in a specific order.

Therefore, a "as much live output as possible" behavior would be more
sensible. But I wonder: is there a worthy benefit in optimizing the
output owner strategy? I'm not used to working with submodules, but I
don't think that having a great number of submodules is a common thing.
Basically: we could solve a problem, but is there a problem?
I'm not trying to bury this NEEDSWORK, I'd be happy to look into it if
need be!
--
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 19/21] t9003: become resilient to GETTEXT_POISON

[Vasco Almeida]

Às 17:59 de 20-05-2016, Junio C Hamano escreveu:
> As long as translators do not translate "Did you mean..." to begin a
> line with a tab (which I do not think there is any reason to), it is
> going to work reliably to grep for "^ lgf$" here, and it will catch
> such a potential future bug under GETTEXT_POISON build.

Translations don't affect the tests since they're run with C locale.
I think all tests source test-lib.sh which does:

# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
LANG=C
LC_ALL=C
PAGER=cat
TZ=UTC
export LANG LC_ALL PAGER TZ

So, I'll remove sed command and use grep the way you said it, in the
next re-roll.
--
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: [PATCHv9 4/4] pathspec: allow querying for attributes

[Stefan Beller]

On Fri, May 20, 2016 at 11:15 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Stefan Beller  writes:
>>
>>> +attr;;
>>> +After `attr:` comes a space separated list of "attribute
>>> +...
>>> ++
>>
>> The text looks OK, but does it format well?
>
> I didn't check this, but the remainder would look like this
> squashable patch.

I checked and it looks wrong. the "exclude" section is indented below
the new attr section

fix is:
--8<--
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index e06520b..181f52e 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -389,7 +389,7 @@ After `attr:` comes a space separated list of "attribute
 requirements", all of which must be met in order for the
 path to be considered a match; this is in addition to the
 usual non-magic pathspec pattern matching.
-
++
 Each of the attribute requirements for the path takes one of
 these forms:
--8<--

I can resend with your proposed fixes as well.

Thanks,
Stefan



>
> You seem to i18ngrep for "fatal" but we are using test_must_fail for
> the exit status, so I am not sure if that adds much value, so the
> additional tests here do nto use that pattern.
>
> diff --git a/pathspec.c b/pathspec.c
> index 693a5e7..0a02255 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -115,19 +115,19 @@ static void parse_pathspec_attr_match(struct 
> pathspec_item *item, const char *va
> const char *attr = si->string;
> struct attr_match *am = >attr_match[j];
>
> -   attr_len = strcspn(attr, "=");
> switch (*attr) {
> case '!':
> am->match_mode = MATCH_UNSPECIFIED;
> attr++;
> -   attr_len--;
> +   attr_len = strlen(attr);
> break;
> case '-':
> am->match_mode = MATCH_UNSET;
> attr++;
> -   attr_len--;
> +   attr_len = strlen(attr);
> break;
> default:
> +   attr_len = strcspn(attr, "=");
> if (attr[attr_len] != '=')
> am->match_mode = MATCH_SET;
> else {
> diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
> index 5da1a63..060047a 100755
> --- a/t/t6134-pathspec-with-labels.sh
> +++ b/t/t6134-pathspec-with-labels.sh
> @@ -160,4 +160,9 @@ test_expect_success 'abort on giving invalid label on the 
> command line' '
> test_i18ngrep "fatal" actual
>  '
>
> +test_expect_success 'abort on asking for wrong magic' '
> +   test_must_fail git ls-files . ":(attr:-label=foo)" &&
> +   test_must_fail git ls-files . ":(attr:!label=foo)"
> +'
> +
>  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: [PATCHv8 0/5] pathspec magic extension to search for attributes

[Junio C Hamano]

Stefan Beller  writes:

> As a user I'd prefer to be exposed to as few concepts as possible,
> and adding yet another concept of sparseness is not a good thing
> IMHO, so I'll try to keep it simple there.

Yes, and as a user, I'd prefer if I (and all the users) do not have
to even worry about the "sparse" hack.

If you are using submodules, it was in its design from day one that
it is not unsual for many of your submodules to be uninitialized
(but still present).  And that is a pretty normal state.  I do not
think we want to expose users to the "sparse" hack only to use
submodules effectively.

--
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: [PATCHv9 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>> +attr;;
>> +After `attr:` comes a space separated list of "attribute
>> +...
>> ++
>
> The text looks OK, but does it format well?

I didn't check this, but the remainder would look like this
squashable patch.

You seem to i18ngrep for "fatal" but we are using test_must_fail for
the exit status, so I am not sure if that adds much value, so the
additional tests here do nto use that pattern.

diff --git a/pathspec.c b/pathspec.c
index 693a5e7..0a02255 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -115,19 +115,19 @@ static void parse_pathspec_attr_match(struct 
pathspec_item *item, const char *va
const char *attr = si->string;
struct attr_match *am = >attr_match[j];
 
-   attr_len = strcspn(attr, "=");
switch (*attr) {
case '!':
am->match_mode = MATCH_UNSPECIFIED;
attr++;
-   attr_len--;
+   attr_len = strlen(attr);
break;
case '-':
am->match_mode = MATCH_UNSET;
attr++;
-   attr_len--;
+   attr_len = strlen(attr);
break;
default:
+   attr_len = strcspn(attr, "=");
if (attr[attr_len] != '=')
am->match_mode = MATCH_SET;
else {
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
index 5da1a63..060047a 100755
--- a/t/t6134-pathspec-with-labels.sh
+++ b/t/t6134-pathspec-with-labels.sh
@@ -160,4 +160,9 @@ test_expect_success 'abort on giving invalid label on the 
command line' '
test_i18ngrep "fatal" actual
 '
 
+test_expect_success 'abort on asking for wrong magic' '
+   test_must_fail git ls-files . ":(attr:-label=foo)" &&
+   test_must_fail git ls-files . ":(attr:!label=foo)"
+'
+
 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: [PATCHv8 0/5] pathspec magic extension to search for attributes

[Stefan Beller]

On Fri, May 20, 2016 at 10:00 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Right. But upon finding the new name for clone, I wondered why
>> this has to be submodule specific. The attr pathspecs are also working
>> with any other files. So if you don't use submodules, I think it would be
>> pretty cool to have a
>>
>> git clone --sparse-checkout=Documentation/ ...
>
> It would be cool, but arent' "sparse" and the various existing
> status "submodule" has very different things?

Yes they are. In one of the various "submodule groups" series I
proposed a "defaultGroup" which allows commands to ignore
some submodules. That was conceptually the very same as a
"sparse checkout, just for submodules", i.e. the submodule is
initialized and has a directory as a place holder, but most commands
ignore its existence.

We decided that was a bad thing, so now I think of a light weight
"submodule.updateGroup" which holds a pathspec and is only
used for "submodule update" commands that have no explicit
pathspec given. (That setting would be set via "git clone
--submodule-pathspec ")

>
>  - A submodule can be uninitialized, in which case you do get an empty
>directory but you do not see .git in it.
>
>  - A path can be excluded by the sparse checkout mechanism, in which
>case you do not get _anything_ in the filesystem.

Yes, but isn't that one of the minor issues?

>
> So "git clone --sparse-exclude=Documentation/" that does not waste
> diskspace for Documentation/ directory may be an interesting thing
> to have, and "git clone --sparse-exclude=submodule-dir/" that does
> not even create submodule-dir/ directory may also be, but the latter
> is quite different from a submodule that is not initialied in a
> superproject that does not use any "sparse" mechanism.
>
> Besides, I think (improved) submodule mechanism would be a good way
> forward for scalability, and "sparse" hack is not (primarily because
> it still populates the index fully with 5 million entries even when
> your attention is narrowed only to a handful of directories with
> 2000 leaf entries; this misdesign requires other ugly hacks to be
> piled on, like untracked cache and split index).
>
> I do not think we want "submodule" to be tied to and dependent on
> the latter.

Ok I just wanted to probe how much resistance I get here as an
indicator of how much more work that would be.

Besides I think (improved) sparse mechanism would be a good way
to not confuse users between submodule scalability and single
repo scalability. ;)

We don't have to keep 5 million things in the index there, but we can
stop on the tree/directory level, i.e. if a whole directory is excluded
That's all we'd need to keep a record of, no?

As a user I'd prefer to be exposed to as few concepts as possible,
and adding yet another concept of sparseness is not a good thing
IMHO, so I'll try to keep it simple there.

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 19/21] t9003: become resilient to GETTEXT_POISON

[Junio C Hamano]

Junio C Hamano  writes:

> Vasco Almeida  writes:
>
>> Alternatively, we could leave sed alone as it were before this patch and
>> use test_i18ngrep instead of grep to fake success under GETTEXT_POISON.
>> I think I prefer this way. What do you think?
>
> That is equivalent to saying that "we would translate 'lgf' to
> end-user's language", which does not make much sense to me.
>
> Wouldn't the introductory explanation, up to "Did you mean this?",
> be the only thing that is translated?

I just checked the code ;-)  We do this:

fprintf_ln(stderr,
   Q_("\nDid you mean this?",
  "\nDid you mean one of these?",
   n));

for (i = 0; i < n; i++)
fprintf(stderr, "\t%s\n", main_cmds.names[i]->name);

Using test_i18ngrep would mean we would not be able to catch a
potential future bug that does an equivalent of

for (i = 0; i < n; i++)
fprintf(stderr, "\t%s\n", _(main_cmds.names[i]->name));

in the loop, i.e. marking something that is not meant to be
translated as translatable.

As long as translators do not translate "Did you mean..." to begin a
line with a tab (which I do not think there is any reason to), it is
going to work reliably to grep for "^ lgf$" here, and it will catch
such a potential future bug under GETTEXT_POISON build.



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

BowlerStudio: using GIT as a file system for distributed robots

[Kevin Harrington]

I have spent the past year building an new open source robotics
development platform and wanted to show you fine folks working on Git:

BowlerStudio: https://github.com/NeuronRobotics/BowlerStudio

I started working a set of robotics control libraries back in 2009 and
have deployed them as the control framework for MRI compatible robots.

Over the past year i began developing a set of tutorials for building
robots using my framework. In this process i build a small scripting
engine that uses Git as its file system. This allows scripts to
hyperlink to each other based on the Git repository they live in.

The platform supports Java/Groovy, Clojure, and Python scripts, and
these scripts use an "Object in Object out" version of the Pipe
pattern for interfacing. Since the JVM provides the common runtime,
they can pass memory references back and forth in the pipe pattern as
opposed to parsing and building stings. This lets any of the languages
trivially interoperate together. If a script you want is in a
different langauge hosted on a different Git repo, no problem just
call it in line and the compiled and typed object is returned.

Under the hood, JGIT, the same library eclipse uses to provide Git
support, is the library that manages the local caches of files pulled
from the Git repo. The scripting engine has features to commit changes
and push them upstream. In the Tutorials, you look at spike examples
in the WebBrowser (using GitHub Gist to JS embed the code), JGIT takes
the Git URL extracted from the Gist, caches the repo to the disk, and
makes it availible to the new user to run. From there the user can
fork, modify, and publish changes to the demo code. In this way, all
examples are executable right out of the built in browser for the
fastest startup time with new code examples.

The Application is used to connect cameras, robot controllers,
kinematics libraries, CSG Cad engine, a physics engine and a few AI
libraries all in a common runtime with a shallow on-ramp for even
novice programmers.

TL;DR Kerbal Space Program meets ROS on the JVM using Git as a filesystem
--
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 19/21] t9003: become resilient to GETTEXT_POISON

[Junio C Hamano]

Vasco Almeida  writes:

> Alternatively, we could leave sed alone as it were before this patch and
> use test_i18ngrep instead of grep to fake success under GETTEXT_POISON.
> I think I prefer this way. What do you think?

That is equivalent to saying that "we would translate 'lgf' to
end-user's language", which does not make much sense to me.

Wouldn't the introductory explanation, up to "Did you mean this?",
be the only thing that is translated?
--
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 v6 2/2] convert: ce_compare_data() checks for a sha1 of a path

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> To compare a file in working tree with the index, convert_to_git() is used,
> the result is hashed and the hash value compared with ce->sha1.
>
> Deep down would_convert_crlf_at_commit() is invoked, to check if CRLF
> are converted or not: When a CRLF had been in the index before, CRLF in
> the working tree are not converted.
>
> While in a merge, a file name in the working tree has different blobs
> in the index with different hash values.
> Forwarding ce->sha1 from ce_compare_data() into crlf_to_git() makes sure
> the would_convert_crlf_at_commit() looks at the appropriate blob.
>
> Forward sha1 from ce_compare_data() into convert_to_git().
> All other callers use NULL, and the sha1 it is determined from path using
> get_sha1_from_cache(path), this is the same handling as before.
>
> Re-order the arguments for convert_to_git() according to their importance:
>   `src`, `len` and `dst` are the place in memory, where the conversion is done
>   `path` is the file name to look up the attributes.
>   `sha1` is needed by the "new safer autocrlf handling".
>   `checksafe` determines, if a warning is printed or an error is raised.
>
> In the same spirit, forward the sha1 into would_convert_to_git().
>
> While at it, rename has_cr_in_index() into blob_has_cr()
>
> Signed-off-by: Torsten Bögershausen 


> Changes sinve v6:
>  decrease the messiness with 12 %

What does that mean

>  convert_to_git() has a re-ordered parameter list.

That is not what I suggested, though.   being
the first three would be fine, and that would be in line with
helpers ${frotz}_to_git() that are used from there.

The primary thing that made me worried was a new parameter with a
bland name "sha1" whose purpose is unclear was added near the
beginning, leading readers to confusion.

Whether you keep  at the beginning of move it to later
together with , helpers like crlf_to_git() need to be updated
to match.  E.g. I would say that this

> - ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
> + ret |= crlf_to_git(path, sha1, src, len, dst, ca.crlf_action, 
> checksafe);

would want to be ordered more like this:

ret |= crlf_to_git(path, src, len, dst,
   ca.crlf_action, checksafe, index_blob_sha1);

if you choose to keep the first four intact for convert_to_git(),
that is.

>  Describe whats going on better in the commit msg.

The suggestion to rename the parameter was to allow readers of the
code to immediately know what kind of SHA1 it is.  They cannot
afford to run "git blame" every time to find the commit to read the
commit log message; for a public function like convert_to_git(),
in-code comment is also necessary.

--
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 19/21] t9003: become resilient to GETTEXT_POISON

[Vasco Almeida]

Às 16:39 de 20-05-2016, Junio C Hamano escreveu:
> We want to see the string appear after "Did you mean this?" and we
> do not want to be fooled by a future change in the early part of the
> message, which may contain a substring l-g-f that does not have
> anything to do with the alias we are looking for.
> 
> And the way you express "I do not care anything above this line" is
> to say "sed -e '1,/^that line/d'".
> 
> Of course, if you use this with POISON, you'd need to consider that
> "Did you mean this" would not be a good marker to identify where the
> introductory text we want to ignore ends.  You'd need to find a
> different mechanism to exclude the introductory text if you want to
> retain the future-proofing the existing "sed -e" gave us.
> 
> Perhaps discarding up to the first blank line (i.e. assuming that we
> would not remove that blank, and also assuming that we will not
> rephrase "Did you mean this?") may be a good alternative.
> 
> Or assuming that the explanatory text would not begin its lines with
> a tab, i.e.
> 
>   grep '^ lgf$' actual
> 
> (the space between '^' and 'l' above is a TAB) without using
> test_i18ngrep?
> 
> I think I like that the best among what I can think of offhand.
> 
> 
Alternatively, we could leave sed alone as it were before this patch and
use test_i18ngrep instead of grep to fake success under GETTEXT_POISON.
I think I prefer this way. What do you think?
--
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 19/21] t9003: become resilient to GETTEXT_POISON

[Eric Sunshine]

On Fri, May 20, 2016 at 12:39 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> Indeed, the sed seems superfluous. The output of the test command is:
>>
>> git: 'lfg' is not a git command. See 'git --help'.
>>
>> Did you mean this?
>> lgf
>>
>> And, the grep'd string, "lgf" only appears once, so grep alone should
>> be sufficient to verify expected behavior.
>
> Perhaps discarding up to the first blank line (i.e. assuming that we
> would not remove that blank, and also assuming that we will not
> rephrase "Did you mean this?") may be a good alternative.
>
> Or assuming that the explanatory text would not begin its lines with
> a tab, i.e.
>
> grep '^ lgf$' actual
>
> (the space between '^' and 'l' above is a TAB) without using
> test_i18ngrep?
>
> I think I like that the best among what I can think of offhand.

Yep, I also considered both of these approaches and favored the latter, 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

[PATCH v6 1/2] read-cache: factor out get_sha1_from_index() helper

[tboegi]

From: Torsten Bögershausen 

Factor out the retrieval of the sha1 for a given path in
read_blob_data_from_index() into the function get_sha1_from_index().

This will be used in the next commit, when convert.c can do the
analyze for "text=auto" without slurping the whole blob into memory
at once.

Add a wrapper definition get_sha1_from_cache().

Signed-off-by: Torsten Bögershausen 
---
 cache.h  |  3 +++
 read-cache.c | 29 ++---
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 160f8e3..15a2a10 100644
--- a/cache.h
+++ b/cache.h
@@ -379,6 +379,7 @@ extern void free_name_hash(struct index_state *istate);
 #define unmerge_cache_entry_at(at) unmerge_index_entry_at(_index, at)
 #define unmerge_cache(pathspec) unmerge_index(_index, pathspec)
 #define read_blob_data_from_cache(path, sz) 
read_blob_data_from_index(_index, (path), (sz))
+#define get_sha1_from_cache(path)  get_sha1_from_index (_index, (path))
 #endif
 
 enum object_type {
@@ -1038,6 +1039,8 @@ static inline void *read_sha1_file(const unsigned char 
*sha1, enum object_type *
return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
 }
 
+const unsigned char *get_sha1_from_index(struct index_state *istate, const 
char *path);
+
 /*
  * This internal function is only declared here for the benefit of
  * lookup_replace_object().  Please do not call it directly.
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..a3ef967 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2263,13 +2263,27 @@ int index_name_is_other(const struct index_state 
*istate, const char *name,
 
 void *read_blob_data_from_index(struct index_state *istate, const char *path, 
unsigned long *size)
 {
-   int pos, len;
+   const unsigned char *sha1;
unsigned long sz;
enum object_type type;
void *data;
 
-   len = strlen(path);
-   pos = index_name_pos(istate, path, len);
+   sha1 = get_sha1_from_index(istate, path);
+   if (!sha1)
+   return NULL;
+   data = read_sha1_file(sha1, , );
+   if (!data || type != OBJ_BLOB) {
+   free(data);
+   return NULL;
+   }
+   if (size)
+   *size = sz;
+   return data;
+}
+
+const unsigned char *get_sha1_from_index(struct index_state *istate, const 
char *path)
+{
+   int pos = index_name_pos(istate, path, strlen(path));
if (pos < 0) {
/*
 * We might be in the middle of a merge, in which
@@ -2285,14 +2299,7 @@ void *read_blob_data_from_index(struct index_state 
*istate, const char *path, un
}
if (pos < 0)
return NULL;
-   data = read_sha1_file(istate->cache[pos]->sha1, , );
-   if (!data || type != OBJ_BLOB) {
-   free(data);
-   return NULL;
-   }
-   if (size)
-   *size = sz;
-   return data;
+   return (istate->cache[pos]->sha1);
 }
 
 void stat_validity_clear(struct stat_validity *sv)
-- 
2.0.0.rc1.6318.g0c2c796

--
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 v6 2/2] convert: ce_compare_data() checks for a sha1 of a path

[tboegi]

From: Torsten Bögershausen 

To compare a file in working tree with the index, convert_to_git() is used,
the result is hashed and the hash value compared with ce->sha1.

Deep down would_convert_crlf_at_commit() is invoked, to check if CRLF
are converted or not: When a CRLF had been in the index before, CRLF in
the working tree are not converted.

While in a merge, a file name in the working tree has different blobs
in the index with different hash values.
Forwarding ce->sha1 from ce_compare_data() into crlf_to_git() makes sure
the would_convert_crlf_at_commit() looks at the appropriate blob.

Forward sha1 from ce_compare_data() into convert_to_git().
All other callers use NULL, and the sha1 it is determined from path using
get_sha1_from_cache(path), this is the same handling as before.

Re-order the arguments for convert_to_git() according to their importance:
  `src`, `len` and `dst` are the place in memory, where the conversion is done
  `path` is the file name to look up the attributes.
  `sha1` is needed by the "new safer autocrlf handling".
  `checksafe` determines, if a warning is printed or an error is raised.

In the same spirit, forward the sha1 into would_convert_to_git().

While at it, rename has_cr_in_index() into blob_has_cr()

Signed-off-by: Torsten Bögershausen 
Changes sinve v6:
 decrease the messiness with 12 %
 convert_to_git() has a re-ordered parameter list.
 Describe whats going on better in the commit msg.
 Cleanup: 0 -> SAFE_CRLF_FALSE at some places
---
 builtin/apply.c |  3 ++-
 builtin/blame.c |  2 +-
 cache.h |  1 +
 combine-diff.c  |  4 +++-
 convert.c   | 38 +-
 convert.h   | 20 ++--
 diff.c  |  3 ++-
 dir.c   |  2 +-
 read-cache.c|  4 +++-
 sha1_file.c | 17 +
 10 files changed, 65 insertions(+), 29 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 8e4da2e..c01654a 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2140,7 +2140,8 @@ static int read_old_data(struct stat *st, const char 
*path, struct strbuf *buf)
case S_IFREG:
if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
return error(_("unable to open or read %s"), path);
-   convert_to_git(path, buf->buf, buf->len, buf, 0);
+   convert_to_git(buf->buf, buf->len, buf,
+  path, NULL, SAFE_CRLF_FALSE);
return 0;
default:
return -1;
diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0..4a01e20 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2377,7 +2377,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
if (strbuf_read(, 0, 0) < 0)
die_errno("failed to read from stdin");
}
-   convert_to_git(path, buf.buf, buf.len, , 0);
+   convert_to_git(buf.buf, buf.len, , path, NULL, SAFE_CRLF_FALSE);
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
diff --git a/cache.h b/cache.h
index 15a2a10..868599e 100644
--- a/cache.h
+++ b/cache.h
@@ -605,6 +605,7 @@ extern int ie_modified(const struct index_state *, const 
struct cache_entry *, s
 
 #define HASH_WRITE_OBJECT 1
 #define HASH_FORMAT_CHECK 2
+#define HASH_USE_SHA_NOT_PATH 4
 extern int index_fd(unsigned char *sha1, int fd, struct stat *st, enum 
object_type type, const char *path, unsigned flags);
 extern int index_path(unsigned char *sha1, const char *path, struct stat *st, 
unsigned flags);
 
diff --git a/combine-diff.c b/combine-diff.c
index 0e1d4b0..cac4c81 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1053,7 +1053,9 @@ static void show_patch_diff(struct combine_diff_path 
*elem, int num_parent,
if (is_file) {
struct strbuf buf = STRBUF_INIT;
 
-   if (convert_to_git(elem->path, result, len, 
, safe_crlf)) {
+   if (convert_to_git(result, len, ,
+  elem->path, NULL,
+  safe_crlf)) {
free(result);
result = strbuf_detach(, );
result_size = len;
diff --git a/convert.c b/convert.c
index f524b8d..a58bb26 100644
--- a/convert.c
+++ b/convert.c
@@ -217,21 +217,26 @@ static void check_safe_crlf(const char *path, enum 
crlf_action crlf_action,
}
 }
 
-static int has_cr_in_index(const char *path)
+static int blob_has_cr(const unsigned char *sha1)
 {
unsigned long sz;
void *data;
-   int has_cr;
-
-   data = read_blob_data_from_cache(path, );
+   int has_cr = 0;
+   enum object_type type;
+   if (!sha1)
+   

Re: run-command: output owner picking strategy

[Stefan Beller]

On Fri, May 20, 2016 at 6:11 AM, William Duclot
 wrote:
> Hi,
> I stumbled upon this piece of code (run-command.c:pp_collect_finish()), 
> picking the owner
> of the output amongst parallel processes (introduced by Stephan Beller in 
> commit c553c72eed64b5f7316ce227f6d5d783eae6f2ed)
>
> /*
>  * Pick next process to output live.
>  * NEEDSWORK:
>  * For now we pick it randomly by doing a round
>  * robin. Later we may want to pick the one with
>  * the most output or the longest or shortest
>  * running process time.
>  */
> for (i = 0; i < n; i++)
>if (pp->children[(pp->output_owner + i) % n].state == GIT_CP_WORKING)
>   break;
> pp->output_owner = (pp->output_owner + i) % n;
>
>
> Would it be useful to improve this round-robin into something smarter (as 
> stated by the NEEDSWORK)? It seems to be only used for submodules fetch/clone.
>
> The options would be (as said in the comment):
> 1 - pick the process with the longest running process time
> 2 - pick the process with the shortest running process time
> 3 - pick the process for which the output buffer is the longest
>
> But with one of those strategies, wouldn't we lose the advantage of having 
> the same output order as a non-parallelized version? Cf the commit message.
>
> What do you think ?

When running in parallel we already may be out of order
(relative to serial processing). See the second example in the
commit message to produce a different order.

Consider we scheduled tasks to be run in 3 parallel processes:
(As we NEEDSWORK comment only addresses the ouput selection,
let's assume this is a fixes schedule, which we cannot alter.
Which is true if we only change the code you quoted. That picks
the process to output.)


process 1: |---A---||-E--|

process 2: |-B-||---D|

process 3: |---C---||-F-|

output order:A, B, D, C, F, E
timing:|---A---|{B}|--D|{C,F,E}

All outputs with {braces} are instant from buffers,
and output surrounded by |--dashes--| are live.

The output is produced by the current algorithm:
(1) Start with process 1 (A) whose output will be live
(2) Once A is done, flush all other done things, (B)
(3) live output will be round robin, so process 2 (D)
(4) Once D is done, flush all other done things (C, F, E)
in order of who finshed first


(1) is uncontroversial. We have no information about tasks A,B,C,
so pick a random candidate. We hardcoded process 1 for now.

(2) also uncontroversial IMHO. There is not much we can do different.

(3) is what this NEEDSWORK comment is about. Instead of outputting D
we might have choosen C. (for $REASONS, e.g.: C is running longer than
D already, so we expect it to finish sooner, by assuming
any task takes the same expected time to finish. And as C
is expected to finish earlier than D, we may have smoother
output. "Less buffered bursts")

Then the output looks like this:

output order: A B C D F E
timing:|---A---|{B}|-C-||-D-|{F,E}
(Same notation as above)

This seems to be better than the current behavior as we have more
different tasks with "live" output, i.e. you see stuff moving.
I made up the data to make the point though. We would need to use
live data and experiment with different strategies to find a
good/better solution.

Another way to do output would be to not round robin but keep outputting
process 1 live (different than current behavior, and not optimal IMHO)

output order: A B E C D F
timing:|---A---|{B}|-E|{C,D,F}
(Same notation as above)

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: [PATCHv8 0/5] pathspec magic extension to search for attributes

[Junio C Hamano]

Stefan Beller  writes:

> Right. But upon finding the new name for clone, I wondered why
> this has to be submodule specific. The attr pathspecs are also working
> with any other files. So if you don't use submodules, I think it would be
> pretty cool to have a
>
> git clone --sparse-checkout=Documentation/ ...

It would be cool, but arent' "sparse" and the various existing
status "submodule" has very different things?

 - A submodule can be uninitialized, in which case you do get an empty
   directory but you do not see .git in it.

 - A path can be excluded by the sparse checkout mechanism, in which
   case you do not get _anything_ in the filesystem.

So "git clone --sparse-exclude=Documentation/" that does not waste
diskspace for Documentation/ directory may be an interesting thing
to have, and "git clone --sparse-exclude=submodule-dir/" that does
not even create submodule-dir/ directory may also be, but the latter
is quite different from a submodule that is not initialied in a
superproject that does not use any "sparse" mechanism.

Besides, I think (improved) submodule mechanism would be a good way
forward for scalability, and "sparse" hack is not (primarily because
it still populates the index fully with 5 million entries even when
your attention is narrowed only to a handful of directories with
2000 leaf entries; this misdesign requires other ugly hacks to be
piled on, like untracked cache and split index).

I do not think we want "submodule" to be tied to and dependent on
the latter.
--
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] upload-pack.c: use of parse-options API

[Junio C Hamano]

Matthieu Moy  writes:

>> Using "format-patch --patience" or some other diff option, and pick
>> the best one to give to "send-email" would indeed be a way to do so.
>
> It's a matter of taste. My flow is "send-email-only", I do as much as
> possible in-tree, and when I notice something to do during "git send-email",
> I just abort and retry. So "Oops, looks ugly, I'll try with another
> option" is OK in this flow.

Ah, I didn't consider "final-review in send-email and aborting".

I agree that it is just the matter of preference, if it is easy to
review everything that you would be sending out, and decide to
abort.  I just thought that final review would be cumbersome in that
flow.

--
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 19/21] t9003: become resilient to GETTEXT_POISON

[Junio C Hamano]

Eric Sunshine  writes:

> [cc:+junio]
>
> Indeed, the sed seems superfluous. The output of the test command is:
>
> git: 'lfg' is not a git command. See 'git --help'.
>
> Did you mean this?
> lgf
>
> And, the grep'd string, "lgf" only appears once, so grep alone should
> be sufficient to verify expected behavior.

We want to see the string appear after "Did you mean this?" and we
do not want to be fooled by a future change in the early part of the
message, which may contain a substring l-g-f that does not have
anything to do with the alias we are looking for.

And the way you express "I do not care anything above this line" is
to say "sed -e '1,/^that line/d'".

Of course, if you use this with POISON, you'd need to consider that
"Did you mean this" would not be a good marker to identify where the
introductory text we want to ignore ends.  You'd need to find a
different mechanism to exclude the introductory text if you want to
retain the future-proofing the existing "sed -e" gave us.

Perhaps discarding up to the first blank line (i.e. assuming that we
would not remove that blank, and also assuming that we will not
rephrase "Did you mean this?") may be a good alternative.

Or assuming that the explanatory text would not begin its lines with
a tab, i.e.

grep '^ lgf$' actual

(the space between '^' and 'l' above is a TAB) without using
test_i18ngrep?

I think I like that the best among what I can think of offhand.


--
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] t0008: 4 tests fail with ksh88

[Armin Kunaschik]

On Fri, May 20, 2016 at 6:10 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>>> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
>>> index 89544dd..b425f3a 100755
>>> --- a/t/t0008-ignores.sh
>>> +++ b/t/t0008-ignores.sh
>>> @@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
>>> a/b/.gitignore:8:!on*   a/b/one
>>> a/b/.gitignore:8:!on*   a/b/one one
>
> The patch was whitespace-damaged and didn't apply, so I had to redo
> it from scratch while updating the log message.  If this looks good
> to you, there is no need to resend.

Thanks. Looks like even Google Mail interface in plain text mode eats
white spaces :-(
--
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: Odd Difference Between Windows Git and Standard Git

[Junio C Hamano]

Junio C Hamano  writes:

> Thanks for asking a great question.  I somehow expected that we
> probe in init-db.c::create_default_files() for this when we probe
> for case sensitivity, symlinks, etc., but apparently we don't.

Ah, we do probe by using "config" as a guinea pig file.

Of course, if you are doing network mount between systems with and
without filemode support, the result would depend on where you did
the "git init", so that would not help.

Which means that other probed things like symlink support and case
sensitivity are likely to be wrong in the .git/config that the user
may want to fix.


--
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: Odd Difference Between Windows Git and Standard Git

[Torsten Bögershausen]

On 20.05.16 17:23, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
 What does
 git diff
 say ?
>>>
>>> Great question. For all the unexpected files it says the
>>> same thing:
>>>
>>> old mode 100755
>>> new mode 100644
>>
>> So the solution is to run
>> git config  core.filemode false
> 
> Thanks for asking a great question.  I somehow expected that we
> probe in init-db.c::create_default_files() for this when we probe
> for case sensitivity, symlinks, etc., but apparently we don't.
> 
> I guess we don't because on some filesystems we can't.  IIRC, it
> goes something like: chmod immediately followed by lstat pretends
> that change to the executable bit stuck, until the in-core buffer at
> the vfs layer is flushed to the disk platter that holds the
> filesystem without any notion of executable bit.

We do the probing, when the repo is cloned, and then never again.
If I clone the repo under Windows, the probing gives core.filemod false.
If I clone under Linux, the probing gives core.filemod true.

Unfortunatly Git for Windows looks at core.filemode, when looking
at the working tree, even if the stat() implementation never detects
the executable bit.

A fix could look like this:
 
static int git_default_core_config(const char *var, const char *value)
{
/* This needs a better name */
if (!strcmp(var, "core.filemode")) {
#ifndef GIT_WINDOWS_NATIVE
trust_executable_bit = git_config_bool(var, value);
#endif
return 0;
}



--
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] t0008: 4 tests fail with ksh88

[Armin Kunaschik]

On Fri, May 20, 2016 at 5:16 PM, Junio C Hamano  wrote:
> Armin Kunaschik  writes:
>
>> From: Armin Kunaschik 
>>
>> \" in the test t0008 is not treated the same way in bash and in ksh.
>
> Could you refrain from singling out "bash"?  We don't write for
> "bash" specifically (and the test I ran are with "dash" before I
> push things out).
I can name it "other shells" if this is more comfortable. But I tested
this only with bash and ksh88 on AIX.

> Ideally, if you can try ksh93 and if you find out that ksh93 works,
> then the above can be made in line with your "Subject" to mark ksh88
> as broken (as opposed to other POSIX shells)?  That would help us by
> reminding that running test fine with ksh93 is not a sufficient
> check to make sure we didn't break ksh88 users.
>
>> In ksh the \ disappears and generates false expect data to
>> compare with.
>> Using \\" works portable, the same way in bash and in ksh and
>> is less ambigous.
>
> All of the above would need s/ksh/&88/g; I'd think.  I just tried
>
> make SHELL_PATH=/bin/ksh93
> cd t && /bin/ksh93 t0008-*.sh
>
> and this patch is not necessary for ksh93.
Yes, the patch is not necessary with ksh93 on AIX, but it works :-)
The patch is targeting "ksh" on AIX (which actually is a ksh88).

In the discussion Jeff took a look into the POSIX specification
and described the behavior like this:


I think either is reasonable (there is no need to backslash-escape a
double-quote inside a here-doc, but one assumes that backslash would
generally have its usual behavior). I'm not quite sure how to interpret
POSIX here (see below), but it seems clear that spelling it with two
backslashes as you suggest is the best bet.


I'd not declare ksh88 on AIX broken just because of this ambiguity
since it is not 100% clear in the POSIX description.

Armin
--
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] t0008: 4 tests fail with ksh88

[Junio C Hamano]

Junio C Hamano  writes:

>> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
>> index 89544dd..b425f3a 100755
>> --- a/t/t0008-ignores.sh
>> +++ b/t/t0008-ignores.sh
>> @@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
>> a/b/.gitignore:8:!on*   a/b/one
>> a/b/.gitignore:8:!on*   a/b/one one

The patch was whitespace-damaged and didn't apply, so I had to redo
it from scratch while updating the log message.  If this looks good
to you, there is no need to resend.

Thanks.

-- >8 --
From: Armin Kunaschik 
Date: Fri, 20 May 2016 16:31:30 +0200
Subject: [PATCH] t0008: 4 tests fail with ksh88

In t0008, we have

cat <<-EOF
...
a/b/.gitignore:8:!on*   "a/b/one\"three"
...
EOF

ane expect that the backslash-dq is passed through literally.

ksh88 eats \ and generates a wrong expect data to compare with.

Using \\" works this around without breaking other POSIX shells
(which collapse backslash-backslash to a single backslash), and
ksh88 does so, too.

It makes it easier to read, too, because the reason why we are
writing backslash there is *not* because we think dq is special and
want to quote it (if that were the case we would have two more
backslashes on that line).  It is simply because we want a single
literal backslash there.  Since backslash is treated specially in
unquoted here-document, explicitly doubling it to quote it expresses
our intent better than relying on the character that immediately
comes after it (i.e. '"') not being a special character.

Signed-off-by: Armin Kunaschik 
Acked-by: Jeff King 
Signed-off-by: Junio C Hamano 
---
 t/t0008-ignores.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 89544dd..b425f3a 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
a/b/.gitignore:8:!on*   a/b/one
a/b/.gitignore:8:!on*   a/b/one one
a/b/.gitignore:8:!on*   a/b/one two
-   a/b/.gitignore:8:!on*   "a/b/one\"three"
+   a/b/.gitignore:8:!on*   "a/b/one\\"three"
a/b/.gitignore:9:!two   a/b/two
a/.gitignore:1:two* a/b/twooo
$global_excludes:2:!globaltwo   globaltwo
@@ -686,7 +686,7 @@ cat <<-EOF >expected-all
a/b/.gitignore:8:!on*   b/one
a/b/.gitignore:8:!on*   b/one one
a/b/.gitignore:8:!on*   b/one two
-   a/b/.gitignore:8:!on*   "b/one\"three"
+   a/b/.gitignore:8:!on*   "b/one\\"three"
a/b/.gitignore:9:!two   b/two
::  b/not-ignored
a/.gitignore:1:two* b/twooo
-- 
2.8.3-625-g89e3711

--
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: t0008 test fails with ksh

[Junio C Hamano]

Jeff King  writes:

>   ... However,
>   the double-quote character ( '"' ) shall not be treated specially
>   within a here-document, except when the double-quote appears within
>   "$()", "``", or "${}".
>
> So OK, that sounds like ksh is doing the right thing. But what's that
> "specially" in the last sentence?

I would say: Just like \X is passed thru as-is without losing \, \"
is passed thru without losing \, because " is not special, just like
X is not special.

> Anyway, it doesn't really matter what the standard says. We can spell
> this in a less ambiguous way, and it does not hurt too much to do so.

Yes.
--
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: Odd Difference Between Windows Git and Standard Git

[Torsten Bögershausen]

On 20.05.16 16:28, Jon Forrest wrote:
> 
> 
> On 5/20/2016 7:19 AM, Torsten Bögershausen wrote:
> 
>>> Great question. For all the unexpected files it says the
>>> same thing:
>>>
>>> old mode 100755
>>> new mode 100644
>>
>> So the solution is to run
>> git config  core.filemode false
> 
> This worked perfectly!
> 
> I wonder if this should be the default for Git for Windows.

It is.
But you need to clone the repo under Windows.

I probably submit a patch some day, that core.filemode will be ignored
under Windows.


--
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: more novice-friendly behaviour of `git add -p`

[enrico]

Junio C Hamano  pobox.com> writes:

> 
> enrico  gmail.com> writes:
> 
> > Hello all,
> > I have encountered a couple of non-necessary difficulties when editing a
> > patch during a `git add -p`.
> >
> > Firstly, the help message says
> > "To remove '-' lines, make them ' ' lines (context)."
> > which is a bit confusing because that "them" refers to '-', not to 'lines'.
> 
> I think that sentence refers to a line line this in a patch:
> 
> -This is what the line used to be
> 
> as a '-'-line.  A line that does not change between preimage and
> postimage have SP instead of '-' at the beginning, and the sentence
> seems to refer to it as a ' '-line.  So from that reading, "turning
> '-'-lines that you do not want to loes into ' '-lines" is perfectly
> sensible phrasing.

I agree it is, and that little dash would definitely make the message less
ambiguous.
Git has a way to "explain itself" to its users so that they can become
better as they use it, and these sort of messages play a very important part
in this learning process.

> 
> In any case, "edit" is about giving a low-level access and precise
> control to people who are familiar with (1) what each line of "diff"
> output means and (2) what is done to them by "patch" (rather, in
> Git's context, "apply").
> 
> I agree with you that "edit" mode is a too-advanced tool for those
> who are not comfortable with these two things.  A solution would
> however not be to modify "edit" mode (which would affect those who
> are prepared to and want to use the "low-level access and precise
> control" to their advantage), but to introduce an easier-to-use,
> and perhaps a bit limited for safety, mode for those who are not the
> target audience for "edit" mode.
> 
> The "split" subcommand to split the hunk before applying was an
> attempt to go in that direction; it never allows you the user to
> make an arbitrary change to corrupt the patch and make it unusable.
> Perhaps you can mimick its spirit and come up with a new "guarded
> edit" command?
>

I am not sure we are talking about the same issue. I am not pointing out
that git is unsafe to less-than-very-expert users.
Much more trivially, I am saying that the current behaviour of the "edit"
mode, when coupled with hunk splitting, is needlessly frustrating (because
of the issue described in the link I provided in my previous message).
That's why I would argue that git would help wanna-be-experts better if
it told them, in some way, that editing after splitting is generally a bad idea.
Advanced users would not be bothered by this
warning/lack-of-edit-after-splitting because, I think, they don't do it
anyway. They already know it
is a pain, so they either split or edit.

--
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: Odd Difference Between Windows Git and Standard Git

[Junio C Hamano]

Torsten Bögershausen  writes:

>>> What does
>>> git diff
>>> say ?
>> 
>> Great question. For all the unexpected files it says the
>> same thing:
>> 
>> old mode 100755
>> new mode 100644
>
> So the solution is to run
> git config  core.filemode false

Thanks for asking a great question.  I somehow expected that we
probe in init-db.c::create_default_files() for this when we probe
for case sensitivity, symlinks, etc., but apparently we don't.

I guess we don't because on some filesystems we can't.  IIRC, it
goes something like: chmod immediately followed by lstat pretends
that change to the executable bit stuck, until the in-core buffer at
the vfs layer is flushed to the disk platter that holds the
filesystem without any notion of executable 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: [PATCH] t0008: 4 tests fail with ksh88

[Junio C Hamano]

Armin Kunaschik  writes:

> From: Armin Kunaschik 
>
> \" in the test t0008 is not treated the same way in bash and in ksh.

Could you refrain from singling out "bash"?  We don't write for
"bash" specifically (and the test I ran are with "dash" before I
push things out).

Ideally, if you can try ksh93 and if you find out that ksh93 works,
then the above can be made in line with your "Subject" to mark ksh88
as broken (as opposed to other POSIX shells)?  That would help us by
reminding that running test fine with ksh93 is not a sufficient
check to make sure we didn't break ksh88 users.

> In ksh the \ disappears and generates false expect data to
> compare with.
> Using \\" works portable, the same way in bash and in ksh and
> is less ambigous.

All of the above would need s/ksh/&88/g; I'd think.  I just tried

make SHELL_PATH=/bin/ksh93
cd t && /bin/ksh93 t0008-*.sh

and this patch is not necessary for ksh93.

> Acked-by: Jeff King 

I didn't see him acking this exact version, so if you didn't include
this line here, I would have missed it.  Thanks.

> Signed-off-by: Armin Kunaschik 
> ---
> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
> index 89544dd..b425f3a 100755
> --- a/t/t0008-ignores.sh
> +++ b/t/t0008-ignores.sh
> @@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
> a/b/.gitignore:8:!on*   a/b/one
> a/b/.gitignore:8:!on*   a/b/one one
> a/b/.gitignore:8:!on*   a/b/one two
> -   a/b/.gitignore:8:!on*   "a/b/one\"three"
> +   a/b/.gitignore:8:!on*   "a/b/one\\"three"
> a/b/.gitignore:9:!two   a/b/two
> a/.gitignore:1:two* a/b/twooo
> $global_excludes:2:!globaltwo   globaltwo
> @@ -686,7 +686,7 @@ cat <<-EOF >expected-all
> a/b/.gitignore:8:!on*   b/one
> a/b/.gitignore:8:!on*   b/one one
> a/b/.gitignore:8:!on*   b/one two
> -   a/b/.gitignore:8:!on*   "b/one\"three"
> +   a/b/.gitignore:8:!on*   "b/one\\"three"
> a/b/.gitignore:9:!two   b/two
> ::  b/not-ignored
> a/.gitignore:1:two* b/twooo
--
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 for Windows 2.8.3

[Johannes Schindelin]

Dear Git users,

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

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

Changes since Git for Windows v2.8.2 (May 3rd 2016)

New Features

  ??? Comes with Git v2.8.3.

Filename | SHA-256
 | ---
Git-2.8.3-64-bit.exe | 
5b26be59b9e289351338befe7f2f185011bc057c63515afb9d29d3744cc68e6b
Git-2.8.3-32-bit.exe | 
428a1765cfbadd88b767b779823dfeae134dcbe43170740b088ffad2cdb4be4b
PortableGit-2.8.3-64-bit.7z.exe | 
5db28a49b014e99435b9a238527a6efeaecb6648eb803a451357505407bc297c
PortableGit-2.8.3-32-bit.7z.exe | 
de52d070219e9c4ec1db179f2adbf4b760686c3180608f0382a1f8c7031e72ad
Git-2.8.3-64-bit.tar.bz2 | 
7fb5237c6ed2fe379bdec02b35649573e928c23b196773fde952f9ae45aec345
Git-2.8.3-32-bit.tar.bz2 | 
b70218f9ab677a63d366dfa89ae5bdbee34849529d9f2ce4a09d91f7669a0b44

Ciao,
Johannes

Re: more novice-friendly behaviour of `git add -p`

[Junio C Hamano]

enrico  writes:

> Hello all,
> I have encountered a couple of non-necessary difficulties when editing a
> patch during a `git add -p`.
>
> Firstly, the help message says
> "To remove '-' lines, make them ' ' lines (context)."
> which is a bit confusing because that "them" refers to '-', not to 'lines'.

I think that sentence refers to a line line this in a patch:

-This is what the line used to be

as a '-'-line.  A line that does not change between preimage and
postimage have SP instead of '-' at the beginning, and the sentence
seems to refer to it as a ' '-line.  So from that reading, "turning
'-'-lines that you do not want to loes into ' '-lines" is perfectly
sensible phrasing.

In any case, "edit" is about giving a low-level access and precise
control to people who are familiar with (1) what each line of "diff"
output means and (2) what is done to them by "patch" (rather, in
Git's context, "apply").

I agree with you that "edit" mode is a too-advanced tool for those
who are not comfortable with these two things.  A solution would
however not be to modify "edit" mode (which would affect those who
are prepared to and want to use the "low-level access and precise
control" to their advantage), but to introduce an easier-to-use,
and perhaps a bit limited for safety, mode for those who are not the
target audience for "edit" mode.

The "split" subcommand to split the hunk before applying was an
attempt to go in that direction; it never allows you the user to
make an arbitrary change to corrupt the patch and make it unusable.
Perhaps you can mimick its spirit and come up with a new "guarded
edit" 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

[PATCH] t0008: 4 tests fail with ksh88

[Armin Kunaschik]

From: Armin Kunaschik 

\" in the test t0008 is not treated the same way in bash and in ksh.
In ksh the \ disappears and generates false expect data to
compare with.
Using \\" works portable, the same way in bash and in ksh and
is less ambigous.

Acked-by: Jeff King 
Signed-off-by: Armin Kunaschik 
---
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 89544dd..b425f3a 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh
@@ -605,7 +605,7 @@ cat <<-EOF >expected-verbose
a/b/.gitignore:8:!on*   a/b/one
a/b/.gitignore:8:!on*   a/b/one one
a/b/.gitignore:8:!on*   a/b/one two
-   a/b/.gitignore:8:!on*   "a/b/one\"three"
+   a/b/.gitignore:8:!on*   "a/b/one\\"three"
a/b/.gitignore:9:!two   a/b/two
a/.gitignore:1:two* a/b/twooo
$global_excludes:2:!globaltwo   globaltwo
@@ -686,7 +686,7 @@ cat <<-EOF >expected-all
a/b/.gitignore:8:!on*   b/one
a/b/.gitignore:8:!on*   b/one one
a/b/.gitignore:8:!on*   b/one two
-   a/b/.gitignore:8:!on*   "b/one\"three"
+   a/b/.gitignore:8:!on*   "b/one\\"three"
a/b/.gitignore:9:!two   b/two
::  b/not-ignored
a/.gitignore:1:two* b/twooo
--
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: Odd Difference Between Windows Git and Standard Git

[Jon Forrest]

On 5/20/2016 7:19 AM, Torsten Bögershausen wrote:


Great question. For all the unexpected files it says the
same thing:

old mode 100755
new mode 100644


So the solution is to run
git config  core.filemode false


This worked perfectly!

I wonder if this should be the default for Git for Windows.

Thanks for the quick (and correct) response.

Jon Forrest

--
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: [Opinion gathering] Git remote whitelist/blacklist

[Randall S. Becker]

On May 20, 2016 10:22 AM, Francois Beutin wrote:
> We (Ensimag students) plan to implement the "remote whitelist/blacklist"
> feature described in the SoC 2016 ideas, but first I would like to be sure we
> agree on what exactly this feature would be, and that the community sees an
> interest in it.
> 
> The general idea is to add a way to prevent accidental push to the wrong
> repository, we see two ways to do it:
> First solution:
>  - a whitelist: Git will accept a push to a repository in it
>  - a blacklist: Git will refuse a push to a repository in it
>  - a default policy
> 
> Second solution:
>  - a default policy
>  - a list of repository not following the default policy
> 
> The new options in config if we implement the first solution:
> 
> [remote]
>   # List of repository that will be allowed/denied with
>   # a whitelist/blacklist
>   whitelisted = "http://git-hosting.org;
>   blacklisted = "http://git-hosting2.org;
> 
>   # What is displayed when the user attempts a push on an
>   # unauthorised repository? (this option overwrites
>   # the default message)
>   denymessage = "message"
> 
>   # What git should do if the user attempts a push on an
>   # unauthorised repository (reject or warn and
>   # ask the user)?
>   denypolicy = reject(default)/warning
> 
>   # How should unknown repositories be treated?
>   defaultpolicy = allow(default)/deny
> 
> 
> Some concrete usage example:
> 
>  - A beginner is working on company code, to prevent him from
>   accidentally pushing the code on a public repository, the
>   company (or him) can do:
> git config --global remote.defaultpolicy "deny"
> git config --global remote.denymessage "Not the company's server!"
> git config --global remote.denypolicy "reject"
> git config --global remote.whitelisted "http://company-server.com;
> 
> 
>  - A regular git user fears that he might accidentally push sensible
>   code to a public repository he often uses for free-time
>   projects, he can do:
> git config remote.defaultpolicy "allow"   #not really needed
> git config remote.denymessage "Are you sure it is the good server?"
> git config remote.denypolicy "warning"
> git config remote.blacklisted "http://github/personnalproject;
> 
> 
> We would like to gather opinions about this before starting to
>   implement it, is there any controversy? Do you prefer the
>   first or second solution (or none)? Do you find the option's
>   names accurate?

How would this feature be secure and made reliably consistent in managing the 
policies (I do like storing the lists separate from the repository, btw)? My 
concern is that by using git config, a legitimate clone can be made of a 
repository with these attributes, then the attributes overridden by local 
config on the clone turning the policy off, changing the remote, and thereby 
allowing a push to an unauthorized destination (example: one on the originally 
intended blacklist). It is unclear to me how a policy manager would keep track 
of this or even know this happened and prevent policies from being bypassed - 
could you clarify this for the requirements?

Cheers,
Randall

-- Brief whoami: NonStop developer since approximately 
UNIX(421664400)/NonStop(2112884442)
-- In my real life, I talk 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 v6 05/17] ref-filter: move get_head_description() from branch.c

[Karthik Nayak]

>
> Note that this expects that va/i18n-misc-updates topic, which
> corrects the translator instruction around here, is already applied.
>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 7d3af1c..fcb3353 100644
>> ...
>> +char *get_head_description(void)
>> +{
>> + struct strbuf desc = STRBUF_INIT;
>> + struct wt_status_state state;
>> + memset(, 0, sizeof(state));
>> + wt_status_get_state(, 1);
>> + if (state.rebase_in_progress ||
>> + state.rebase_interactive_in_progress)
>> + strbuf_addf(, _("(no branch, rebasing %s)"),
>> + state.branch);
>> + else if (state.bisect_in_progress)
>> + strbuf_addf(, _("(no branch, bisect started on %s)"),
>> + state.branch);
>> + else if (state.detached_from) {
>> + /* TRANSLATORS: make sure these match _("HEAD detached at ")
>> +and _("HEAD detached from ") in wt-status.c */
>> + if (state.detached_at)
>> + strbuf_addf(, _("(HEAD detached at %s)"),
>> + state.detached_from);
>> + else
>> + strbuf_addf(, _("(HEAD detached from %s)"),
>> + state.detached_from);
>> + }
>
> ... but the change is apparently lost.
>
> It is a good lesson not to blindly rebase things on 'next', which
> would have unrelated changes.  If you needed es/test-gpg-tags topic
> for the test script change, check out 'master', merge that single
> topic, and then rebase the series on top of the result.
>

Lesson learned. Will do that from now on :)

-- 
Regards,
Karthik Nayak
--
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: Odd Difference Between Windows Git and Standard Git

[Torsten Bögershausen]

On 20.05.16 15:48, Jon Forrest wrote:
> 
> 
> On 5/20/2016 6:19 AM, Torsten Bögershausen wrote:
>> On 20.05.16 03:48, Jon Forrest wrote:
>>> I'm running Git version 2.8.2 built from source on Ubuntu 16.04.
>>> I'm using a repository that's stored on Dropbox. I'm the only person
>>> accessing this repo. Everything works great.
>>>
>>> For reasons unrelated to Git, I decided to try Git for Windows,
>>> so I installed "git version 2.8.2.windows.1" on Windows 10.
>>> When I run 'git status' on Ubuntu the list I see is exactly what
>>> I expect. However, when I run 'git status' on the
>>> same Dropbox repo on Windows, I see what I expect plus I'm told
>>> that every .pdf file and some .png files are modified.
>> To bring at least a little light into the story:
>>
>> What does
>> git diff
>> say ?
> 
> Great question. For all the unexpected files it says the
> same thing:
> 
> old mode 100755
> new mode 100644

So the solution is to run
git config  core.filemode false



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

[Opinion gathering] Git remote whitelist/blacklist

[Francois Beutin]

Hi everyone,

We (Ensimag students) plan to implement the
"remote whitelist/blacklist" feature described in the SoC 2016 ideas,
but first I would like to be sure we agree on what exactly this
feature would be, and that the community sees an interest in it.

The general idea is to add a way to prevent accidental push to the
wrong repository, we see two ways to do it:
First solution:
 - a whitelist: Git will accept a push to a repository in it
 - a blacklist: Git will refuse a push to a repository in it
 - a default policy

Second solution:
 - a default policy
 - a list of repository not following the default policy

The new options in config if we implement the first solution:

[remote]
# List of repository that will be allowed/denied with
# a whitelist/blacklist
whitelisted = "http://git-hosting.org;
blacklisted = "http://git-hosting2.org;

# What is displayed when the user attempts a push on an
# unauthorised repository? (this option overwrites
# the default message)
denymessage = "message"

# What git should do if the user attempts a push on an
# unauthorised repository (reject or warn and
# ask the user)?
denypolicy = reject(default)/warning

# How should unknown repositories be treated?
defaultpolicy = allow(default)/deny


Some concrete usage example:

 - A beginner is working on company code, to prevent him from
accidentally pushing the code on a public repository, the
company (or him) can do:
git config --global remote.defaultpolicy "deny"
git config --global remote.denymessage "Not the company's server!"
git config --global remote.denypolicy "reject"
git config --global remote.whitelisted "http://company-server.com;


 - A regular git user fears that he might accidentally push sensible
code to a public repository he often uses for free-time
projects, he can do:
git config remote.defaultpolicy "allow" #not really needed
git config remote.denymessage "Are you sure it is the good server?"
git config remote.denypolicy "warning"
git config remote.blacklisted "http://github/personnalproject;


We would like to gather opinions about this before starting to
implement it, is there any controversy? Do you prefer the
first or second solution (or none)? Do you find the option's
names accurate?
--
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: Odd Difference Between Windows Git and Standard Git

[Jon Forrest]

On 5/20/2016 6:19 AM, Torsten Bögershausen wrote:

On 20.05.16 03:48, Jon Forrest wrote:

I'm running Git version 2.8.2 built from source on Ubuntu 16.04.
I'm using a repository that's stored on Dropbox. I'm the only person
accessing this repo. Everything works great.

For reasons unrelated to Git, I decided to try Git for Windows,
so I installed "git version 2.8.2.windows.1" on Windows 10.
When I run 'git status' on Ubuntu the list I see is exactly what
I expect. However, when I run 'git status' on the
same Dropbox repo on Windows, I see what I expect plus I'm told
that every .pdf file and some .png files are modified.

To bring at least a little light into the story:

What does
git diff
say ?


Great question. For all the unexpected files it says the
same thing:

old mode 100755
new mode 100644


What does
git config -l | grep core
say ?


core.symlinks=false
core.autocrlf=true
core.fscache=true
core.editor=vim
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true


And what does
git ls-files --eol
say?


The same thing for all the unexpected files,
which is:

i/-text w/-text attr/

I'm running the above commands on Windows.

The result of the 2nd question on Ubuntu is:

core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true

and the 3rd (for the expected files) is:

i/lfw/lfattr/

Jon


--
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] userdiff: add built-in pattern for CSS

[William Duclot]

CSS is widely used, motivating it being included as a built-in pattern.

It must be noted that the word_regex for CSS (i.e. the regex defining
what is a word in the language) does not consider '.' and '#' characters
(in CSS selectors) to be part of the word. This behavior is documented
by the test t/t4018/css-rule.
The logic behind this behavior is the following: identifiers in CSS
selectors are identifiers in a HTML/XML document. Therefore, the '.'/'#'
character are not part of the identifier, but an indicator of the nature
of the identifier in HTML/XML (class or id). Diffing ".class1" and
".class2" must show that the class name is changed, but we still are
selecting a class.

Signed-off-by: William Duclot 
Signed-off-by: Matthieu Moy 
---
 Documentation/gitattributes.txt |  2 ++
 t/t4018-diff-funcname.sh|  1 +
 t/t4018/css-rule|  4 
 t/t4034-diff-words.sh   |  1 +
 t/t4034/css/expect  | 16 
 t/t4034/css/post|  9 +
 t/t4034/css/pre |  9 +
 userdiff.c  |  8 
 8 files changed, 50 insertions(+)
 create mode 100644 t/t4018/css-rule
 create mode 100644 t/t4034/css/expect
 create mode 100644 t/t4034/css/post
 create mode 100644 t/t4034/css/pre

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e3b1de8..81f60ad 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -525,6 +525,8 @@ patterns are available:
 
 - `csharp` suitable for source code in the C# language.
 
+- `css` suitable for source code in the CSS language.
+
 - `fortran` suitable for source code in the Fortran language.
 
 - `fountain` suitable for Fountain documents.
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 67373dc..1795ffc 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -30,6 +30,7 @@ diffpatterns="
bibtex
cpp
csharp
+   css
fortran
fountain
html
diff --git a/t/t4018/css-rule b/t/t4018/css-rule
new file mode 100644
index 000..84ed754
--- /dev/null
+++ b/t/t4018/css-rule
@@ -0,0 +1,4 @@
+RIGHT label.control-label {
+margin-top: 10px!important;
+border : 10px ChangeMe #C6C6C6;
+}
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index f2f55fc..912df91 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -302,6 +302,7 @@ test_language_driver ada
 test_language_driver bibtex
 test_language_driver cpp
 test_language_driver csharp
+test_language_driver css
 test_language_driver fortran
 test_language_driver html
 test_language_driver java
diff --git a/t/t4034/css/expect b/t/t4034/css/expect
new file mode 100644
index 000..b025d88
--- /dev/null
+++ b/t/t4034/css/expect
@@ -0,0 +1,16 @@
+diff --git a/pre b/post
+index 735f301..bdf6a90 100644
+--- a/pre
 b/post
+@@ -1,9 +1,9 @@
+.class-formother-form label.control-label {
+margin-top: 1015px!important;
+border : 10px dasheddotted #C6C6C6;
+}
+#CC
+padding-bottom#CB
+margin-left
+150pxem
+10px
+!important
+divli.class#id
diff --git a/t/t4034/css/post b/t/t4034/css/post
new file mode 100644
index 000..bdf6a90
--- /dev/null
+++ b/t/t4034/css/post
@@ -0,0 +1,9 @@
+.other-form label.control-label {
+margin-top: 15px!important;
+border : 10px dotted #C6C6C6;
+}
+#CB
+margin-left
+150em
+10px
+li.class#id
diff --git a/t/t4034/css/pre b/t/t4034/css/pre
new file mode 100644
index 000..735f301
--- /dev/null
+++ b/t/t4034/css/pre
@@ -0,0 +1,9 @@
+.class-form label.control-label {
+margin-top: 10px!important;
+border : 10px dashed #C6C6C6;
+}
+#CC
+padding-bottom
+150px
+10px!important
+div.class#id
diff --git a/userdiff.c b/userdiff.c
index 6bf2505..0f9cfbe 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -148,6 +148,14 @@ PATTERNS("csharp",
 "[a-zA-Z_][a-zA-Z0-9_]*"
 "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
 "|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->"),
+PATTERNS("css",
+"^([^,{}]+)((,[^}]*\\{)|([ \t]*\\{))$",
+/* -- */
+/* This regex comes from W3C CSS specs. Should theoretically also 
allow ISO 10646 characters U+00A0 and higher,
+ * this not handled in this regex. */
+"-?[_a-zA-F][-_a-zA-F0-9]*" /* identifiers */
+"|-?[0-9]+|\\#[0-9a-fA-F]+" /* numbers */
+),
 { "default", NULL, -1, { NULL, 0 } },
 };
 #undef PATTERNS
-- 
2.8.2.403.gdf0b511.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: Odd Difference Between Windows Git and Standard Git

[Torsten Bögershausen]

On 20.05.16 03:48, Jon Forrest wrote:
> I'm running Git version 2.8.2 built from source on Ubuntu 16.04.
> I'm using a repository that's stored on Dropbox. I'm the only person
> accessing this repo. Everything works great.
> 
> For reasons unrelated to Git, I decided to try Git for Windows,
> so I installed "git version 2.8.2.windows.1" on Windows 10.
> When I run 'git status' on Ubuntu the list I see is exactly what
> I expect. However, when I run 'git status' on the
> same Dropbox repo on Windows, I see what I expect plus I'm told
> that every .pdf file and some .png files are modified.
To bring at least a little light into the story:

What does 
git diff
say ?

What does 
git config -l | grep core
say ?

And what does
git ls-files --eol
say?


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

more novice-friendly behaviour of `git add -p`

[enrico]

Hello all,
I have encountered a couple of non-necessary difficulties when editing a
patch during a `git add -p`.

Firstly, the help message says
"To remove '-' lines, make them ' ' lines (context)."
which is a bit confusing because that "them" refers to '-', not to 'lines'.
I spent a good half hour changing '-' lines to lines containing a single
white space but git was not very happy about it.
I would suggest to change that line with
"To remove '-' lines, change '-' into ' ' (for context)"

Secondly, as discussed here
(http://git.661346.n2.nabble.com/git-add-patch-bug-with-split-edit-td2171634.html)
and in numerous stackoverflow questions, the behaviour of the "edit" (e)
option during an interactive add is a bit...bizarre: it requires the user to
do a lot of gymnastic if (s)he is editing a hunk after having used the split
(s) option, and nine times out of ten the patch will not apply cleanly.
I would suggest to change the behaviour of the interactive add to only allow
edits when the hunk has not been split (possibly with a one-line explanation
for why editing is not possible appearing when inside a split hunk). Since
editing is more powerful than splitting this would not result in a loss of
generality, but, in my humble opinion, in a much nicer experience for
novices and experts alike.

Best regards,
enrico

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

run-command: output owner picking strategy

[William Duclot]

Hi,
I stumbled upon this piece of code (run-command.c:pp_collect_finish()), picking 
the owner of the output amongst parallel processes (introduced by Stephan 
Beller in commit c553c72eed64b5f7316ce227f6d5d783eae6f2ed)

/*
 * Pick next process to output live.
 * NEEDSWORK:
 * For now we pick it randomly by doing a round
 * robin. Later we may want to pick the one with
 * the most output or the longest or shortest
 * running process time.
 */
for (i = 0; i < n; i++)
   if (pp->children[(pp->output_owner + i) % n].state == GIT_CP_WORKING)
  break;
pp->output_owner = (pp->output_owner + i) % n;


Would it be useful to improve this round-robin into something smarter (as 
stated by the NEEDSWORK)? It seems to be only used for submodules fetch/clone.

The options would be (as said in the comment):
1 - pick the process with the longest running process time
2 - pick the process with the shortest running process time
3 - pick the process for which the output buffer is the longest

But with one of those strategies, wouldn't we lose the advantage of having the 
same output order as a non-parallelized version? Cf the commit message.

What do you think ? 
--
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/4] bundle v3: the beginning

[Christian Couder]

I am responding to this 2+ month old email because I am investigating
adding an alternate object store at the same level as loose and packed
objects. This alternate object store could be used for large files. I
am working on this for GitLab. (Yeah, I am working, as a freelance,
for both Booking.com and GitLab these days.)

On Wed, Mar 2, 2016 at 9:32 PM, Junio C Hamano  wrote:
> The bundle v3 format introduces an ability to have the bundle header
> (which describes what references in the bundled history can be
> fetched, and what objects the receiving repository must have in
> order to unbundle it successfully) in one file, and the bundled pack
> stream data in a separate file.
>
> A v3 bundle file begins with a line with "# v3 git bundle", followed
> by zero or more "extended header" lines, and an empty line, finally
> followed by the list of prerequisites and references in the same
> format as v2 bundle.  If it uses the "split bundle" feature, there
> is a "data: $URL" extended header line, and nothing follows the list
> of prerequisites and references.  Also, "sha1: " extended header
> line may exist to help validating that the pack stream data matches
> the bundle header.
>
> A typical expected use of a split bundle is to help initial clone
> that involves a huge data transfer, and would go like this:
>
>  - Any repository people would clone and fetch from would regularly
>be repacked, and it is expected that there would be a packfile
>without prerequisites that holds all (or at least most) of the
>history of it (call it pack-$name.pack).
>
>  - After arranging that packfile to be downloadable over popular
>transfer methods used for serving static files (such as HTTP or
>HTTPS) that are easily resumable as $URL/pack-$name.pack, a v3
>bundle file (call it $name.bndl) can be prepared with an extended
>header "data: $URL/pack-$name.pack" to point at the download
>location for the packfile, and be served at "$URL/$name.bndl".
>
>  - An updated Git client, when trying to "git clone" from such a
>repository, may be redirected to $URL/$name.bndl", which would be
>a tiny text file (when split bundle feature is used).
>
>  - The client would then inspect the downloaded $name.bndl, learn
>that the corresponding packfile exists at $URL/pack-$name.pack,
>and downloads it as pack-$name.pack, until the download succeeds.
>This can easily be done with "wget --continue" equivalent over an
>unreliable link.  The checksum recorded on the "sha1: " header
>line is expected to be used by this downloader (not written yet).

I wonder if this mechanism could also be used or extended to clone and
fetch an alternate object database.

In [1], [2] and [3], and this was also discussed during the
Contributor Summit last month, Peff says that he started working on
alternate object database support a long time ago, and that the hard
part is a protocol extension to tell remotes that you can access some
objects in a different way.

If a Git client would download a "$name.bndl" v3 bundle file that
would have a "data: $URL/alt-odb-$name.odb" extended header, the Git
client would just need to download "$URL/alt-odb-$name.odb" and use
the alternate object database support on this file.

This way it would know all it has to know to access the objects in the
alternate database. The alternate object database may not contain the
real objects, if they are too big for example, but just files that
describe how to get the real objects.

>  - After fully downloading $name.bndl and pack-$name.pack and
>storing them next to each other, the client would clone from the
>$name.bndl; this would populate the newly created repository with
>reasonably recent history.
>
>  - Then the client can issue "git fetch" against the original
>repository to obtain the most recent part of the history created
>since the bundle was made.

[1] http://thread.gmane.org/gmane.comp.version-control.git/206886/focus=207040
[2] http://thread.gmane.org/gmane.comp.version-control.git/247171
[3] http://thread.gmane.org/gmane.comp.version-control.git/202902/focus=203020
--
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 v6 0/2] Implement the GIT_TRACE_CURL environment variable

[Elia Pinto]

This is the sixty version but in reality is the complete rewriting of the 
patches discussed here
(here called V1)

$gmane/290520
$gmane/290521

*Changes from V5
($gmane/293236)

- don't export curl_trace anymore. Define it static
- fix a minor cleanup (style) in setup_curl_trace
- and, finally, i rewrote completely curl_dump, separating it
into two functions (one for http header and one for the http data),
hoping for a coherent implementation with curl --ascii-trace output
but easier to read than the previous implementation that
used a hard-to-read double-loop.

 
*Changes from V4
($gmane/292867)

-  add a better abstraction with the routine setup_curl_trace
-  curl_dump : drop the noex parameter, define nopriv boolean as int
-  use decimal constant where appropiate
-  fix multi-line comment
-  redo the authorization header skip with a replace of possible sensitive data.
   We prefer to print only:
   09:00:53.238330 http.c:534  => Send header: Authorization:  

   intested of
   09:00:53.238330 http.c:534  => Send header: Authorization:  
Basic(o other scheme) 
   as it was done in the original proposed suggestion by Jeff King.
   This is because i think it's better not to print even the authorization 
scheme.
   We add also the (previously missing) proxy-authorization case
-  curl_dump: fix strbuf memory leak

as suggested by Jeff King
($gmane/292891)
($gmane/292892)

In this series i keep the original curl_dump parsing code, even though it is
objectively difficult to read. This is because the same code is used internally 
by curl
to do "ascii-trace" and is also reported in the libcurl code examples and test.
I think this may make maintenance of code easier in the future (libcurl
new dev, new features and so on)

Of course if the maintainer (or other) believes it is really necessary
to rewrite the above code to accept the patches i will do.

*Changes from V3
($gmane/292040)

- add missing static to curl_dump
- reorder the patch order
- tried to fix all (but i am not sure) the problems reported by Julio 
($gmane/292055)
- * squash the documentation with the http.c commit.
  * in the trace prefix each line to indicate it is about sending a header, and 
drop the
initial hex count
  * auto-censor Authorization headers by default

as suggested by Jeff King ($gmane/292074)

*Changes from V2
($gmane/291868)

- fix garbage comment in http.c (i am very sorry for that)
- add final '.' to the commit message for imap-send.c and to other commit 
messages
- typofix double ; in http.c
- merge the nice cleanup and code refactoring of Ramsay Jones (Thank you very 
much !!)
- squash the previous commit 2/4

*Changes from V1

- introduced GIT_TRACE_CURL variable with its documentation
- changed the name of the temporary variable "i" in "w" in the helper routine
- used the c escape sequences instead of the hex equivalent
- dropped the previous GIT_DEBUG_CURL env var
- curl_dump and curl_trace factored out to a shared implementation
in http.c




Elia Pinto (2):
  http.c: implement the GIT_TRACE_CURL environment variable
  imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

 Documentation/git.txt |   8 
 http.c| 123 +-
 http.h|   2 +
 imap-send.c   |   1 +
 4 files changed, 132 insertions(+), 2 deletions(-)

-- 
2.8.2.435.g7c6234f.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 v6 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable

[Elia Pinto]

Permit the use of the GIT_TRACE_CURL environment variable calling
the setup_curl_trace http.c helper routine.

Helped-by: Torsten Bögershausen 
Helped-by: Ramsay Jones 
Helped-by: Junio C Hamano 
Helped-by: Eric Sunshine 
Helped-by: Jeff King 
Signed-off-by: Elia Pinto 
---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/imap-send.c b/imap-send.c
index 938c691..50377c5 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1443,6 +1443,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 
if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
+   setup_curl_trace(curl);
 
return curl;
 }
-- 
2.8.2.435.g7c6234f.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 v6 1/2] http.c: implement the GIT_TRACE_CURL environment variable

[Elia Pinto]

Implement the GIT_TRACE_CURL environment variable to allow a
greater degree of detail of GIT_CURL_VERBOSE, in particular
the complete transport header and all the data payload exchanged.
It might be useful if a particular situation could require a more
thorough debugging analysis. Document the new GIT_TRACE_CURL
environment variable.

Helped-by: Torsten Bögershausen 
Helped-by: Ramsay Jones 
Helped-by: Junio C Hamano 
Helped-by: Eric Sunshine 
Helped-by: Jeff King 
Signed-off-by: Elia Pinto 
---
 Documentation/git.txt |   8 
 http.c| 124 +-
 http.h|   2 +
 3 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index dd6dbf7..a46a356 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1077,6 +1077,14 @@ of clones and fetches.
cloning of shallow repositories.
See 'GIT_TRACE' for available trace output options.
 
+'GIT_TRACE_CURL'::
+   Enables a curl full trace dump of all incoming and outgoing data,
+   including descriptive information, of the git transport protocol.
+   This is similar to doing curl --trace-ascii on the command line.
+   This option overrides setting the GIT_CURL_VERBOSE environment
+   variable.
+   See 'GIT_TRACE' for available trace output options.
+
 'GIT_LITERAL_PATHSPECS'::
Setting this variable to `1` will cause Git to treat all
pathspecs literally, rather than as glob patterns. For example,
diff --git a/http.c b/http.c
index df6dd01..ba32bac 100644
--- a/http.c
+++ b/http.c
@@ -11,6 +11,7 @@
 #include "gettext.h"
 #include "transport.h"
 
+static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);
 #if LIBCURL_VERSION_NUM >= 0x070a08
 long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;
 #else
@@ -477,6 +478,125 @@ static void set_curl_keepalive(CURL *c)
 }
 #endif
 
+static void curl_dump_header(const char *text, unsigned char *ptr, size_t 
size, int nopriv_header)
+{
+   struct strbuf out = STRBUF_INIT;
+   const char *header;
+   struct strbuf **header_list, **ptr_list;
+
+   strbuf_addf(, "%s, %10.10ld bytes (0x%8.8lx)\n",
+   text, (long)size, (long)size);
+   trace_strbuf(_curl, );
+   strbuf_reset();
+   strbuf_add(,ptr,size);
+   header_list = strbuf_split_max(, '\n', 0);
+
+   for (ptr_list = header_list; *ptr_list; ptr_list++) {
+   /*
+* if we are called with nopriv_header substitute a dummy value
+* in the Authorization or Proxy-Authorization http header if
+* present.
+*/
+   if (nopriv_header &&
+   (skip_prefix((*ptr_list)->buf , "Authorization:", )
+   || skip_prefix((*ptr_list)->buf , "Proxy-Authorization:", 
))) { 
+   /* The first token is the type, which is OK to log */
+   while (isspace(*header))
+   header++;
+   while (*header && !isspace(*header))
+   header++;
+   /* Everything else is opaque and possibly sensitive */
+   strbuf_setlen((*ptr_list),  header - (*ptr_list)->buf );
+   strbuf_addstr((*ptr_list), " ");
+   }
+   strbuf_insert((*ptr_list), 0, text, strlen(text));
+   strbuf_insert((*ptr_list), strlen(text), ": ", 2);
+   strbuf_rtrim((*ptr_list));
+   strbuf_addch((*ptr_list), '\n');
+   trace_strbuf(_curl, (*ptr_list));
+   }
+   strbuf_list_free(header_list);
+   strbuf_release();
+}
+static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)
+{
+   size_t i;
+   struct strbuf out = STRBUF_INIT;
+   unsigned int width = 80;
+
+   strbuf_addf(, "%s, %10.10ld bytes (0x%8.8lx)\n",
+   text, (long)size, (long)size);
+   trace_strbuf(_curl, );
+
+   for (i = 0; i < size; i += width) {
+   size_t w;
+
+   strbuf_reset();
+   strbuf_addf(, "%s: ", text);
+   for (w = 0; (w < width) && (i + w < size); w++) {
+   strbuf_addch(, (ptr[i + w] >= 0x20)
+   && (ptr[i + w] < 0x80) ? ptr[i + w] : '.');
+   }
+   strbuf_addch(, '\n');
+   trace_strbuf(_curl, );
+   }
+   strbuf_release();
+}
+
+static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t 
size, void *userp)
+{
+   const char *text;
+   int nopriv_header = 0;  /*
+* default: there are no sensitive data
+* in the trace to be skipped
+*/
+
+   switch (type) {
+   case CURLINFO_TEXT:
+   trace_printf_key(_curl, "== Info: %s", data);
+   default:/* we ignore 

Re: [PATCH v6 3/3] bisect--helper: `write_terms` shell function in C

[Pranit Bauva]

Hey Eric,

On Mon, May 16, 2016 at 12:58 PM, Eric Sunshine  wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva  wrote:
>> Reimplement the `write_terms` shell function in C and add a `write-terms`
>> subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
>> remove the subcommand `--check-term-format` as it can now be called from
>> inside the function write_terms() C implementation.
>>
>> Using `--write-terms` subcommand is a temporary measure to port shell
>> function to C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by some
>> other method.
>>
>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -56,18 +56,41 @@ static int check_term_format(const char *term, const 
>> char *orig_term)
>> +int write_terms(const char *bad, const char *good)
>
> s/^/static/

Sure. Will include this in re-roll.

>> +{
>> +   struct strbuf content = STRBUF_INIT;
>> +   FILE *fp;
>> +   int res;
>> +
>> +   if (!strcmp(bad, good))
>> +   return error(_("please use two different terms"));
>> +
>> +   if (check_term_format(bad, "bad") || check_term_format(good, "good"))
>> +   return -1;
>> +
>> +   strbuf_addf(, "%s\n%s\n", bad, good);
>
> What's the point of the strbuf when you could more easily emit this
> same content directly to the file via:
>
> fprintf(fp, "%s\n%s\n", bad, good);

fprintf seems way better and it is also extensively used in git's
source code. Will update.

>> +   fp = fopen(".git/BISECT_TERMS", "w");
>
> Hardcoding ".git/" is wrong for a variety of reasons: It won't be
> correct with linked worktrees, or when GIT_DIR is pointing elsewhere,
> or when ".git" is a symbolic link, etc. Check out the get_git_dir(),
> get_git_common_dir(), etc. functions in cache.h instead.
>
>> +   if (!fp){
>
> Style: space before '{'

Sorry. Might have missed this. Will include this in re-roll

>> +   strbuf_release();
>> +   die_errno(_("could not open the file to read terms"));
>
> "read terms"? I thought this was writing.

Ya. It should be "write terms".

> Is dying here correct? I thought we established previously that you
> should be reporting failure via normal -1 return value rather than
> dying. Indeed, you're doing so below when strbuf_write() fails.

I was confused about this. I thought not able to open a file is an
exceptional situation (because it has never happened with me) and thus
I used die(). I will use error() as further justified by Johannes.

>> +   }
>> +   res = strbuf_write(, fp);
>> +   fclose(fp);
>> +   strbuf_release();
>> +   return (res < 0) ? -1 : 0;
>> +}
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>
> Style: insert blank line between functions

Sure.

Regards,
Pranit Bauva
--
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] bisect--helper: `bisect_log` shell function in C

[Pranit Bauva]

Hey Eric,

On Mon, May 16, 2016 at 1:06 PM, Eric Sunshine  wrote:
> On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva  wrote:
>> bisect--helper: `bisect_log` shell function in C
>
> Do you need to insert "rewrite" or "reimplement" in the subject?
>
>> Reimplement the `bisect_log` shell function in C and add a
>> `--bisect-log` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-log` subcommand is a temporary measure to port shell
>> function to C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by some
>> other method.
>>
>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
>> +int bisect_log(void)
>
> s/^/static/

Will include this in the re-roll

>> +{
>> +   struct strbuf buf = STRBUF_INIT;
>> +
>> +   if (strbuf_read_file(, ".git/BISECT_LOG", 256) < 0)
>
> As mentioned in my review of the "write-terms" rewrite, hardcoding
> ".git/" here is wrong for a variety of reasons. See get_git_dir(),
> get_git_common_dir(), etc. in cache.h instead.

Thanks. I will have a look into this.

>> +   return error(_("We are not bisecting."));
>> +
>> +   printf("%s", buf.buf);
>> +   strbuf_release();
>> +
>> +   return 0;
>> +}
>> +
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> @@ -109,6 +127,8 @@ int cmd_bisect__helper(int argc, const char **argv, 
>> const char *prefix)
>> if (argc != 2)
>> die(_("--write-terms requires two arguments"));
>> return write_terms(argv[0], argv[1]);
>> +   case BISECT_LOG:
>
> Shouldn't you be die()ing here with an appropriate error message if
> argc is not 0?

I think it would be better to check for argc != 0 and die
appropriately. Will include this in the re-roll.

>> +   return bisect_log();
>> default:
>> die("BUG: unknown subcommand '%d'", cmdmode);
>> }

Regards,
Pranit Bauva
--
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] bisect--helper: `bisect_voc` shell function in C

[Pranit Bauva]

Hey Johannes,

On Mon, May 16, 2016 at 12:10 PM, Johannes Schindelin
 wrote:
> Hi Pranit,
>
> On Sat, 14 May 2016, Pranit Bauva wrote:
>
>> Reimplement the `bisect_voc` shell function in C. This is a too small
>> function to be called as a subcommand though the working of this
>> function has been tested by calling it as a subcommand.
>
> This leaves me puzzled as to what this patch is supposed to do. Maybe
> rename this function to have a more intuitive name, and then throw in a
> patch that makes use of the function?

Are you suggesting to first have an introductory patch which will
rename the function in the shell script and then this patch which will
convert the "new" shell function to C? I can do this. I have to think
of a nice name. How does 'good_or_bad" sound?

Regards,
Pranit Bauva
--
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 v6 1/3] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Pranit Bauva]

Hey Eric,

On Mon, May 16, 2016 at 12:37 PM, Eric Sunshine  wrote:
> On Thu, May 12, 2016 at 1:32 AM, Pranit Bauva  wrote:
>> `--next-all` is meant to be used as a subcommand to support multiple
>> "operation mode" though the current implementation does not contain any
>> other subcommand along side with `--next-all` but further commits will
>> include some more subcommands.
>>
>> Signed-off-by: Pranit Bauva 
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const 
>> char *prefix)
>> -   if (!next_all)
>> +   if (!cmdmode)
>> usage_with_options(git_bisect_helper_usage, options);
>>
>> -   /* next-all */
>> -   return bisect_next_all(prefix, no_checkout);
>> +   switch (cmdmode) {
>> +   case NEXT_ALL:
>> +   return bisect_next_all(prefix, no_checkout);
>> +   default:
>> +   die("BUG: unknown subcommand '%d'", cmdmode);
>> +   }
>> +   return 0;
>
> What happens if you remove this useless 'return 0'? Does the (or some)
> compiler incorrectly complain about it falling off the end of the
> function without returning a value?

I tried removing it. It works fine with gcc and clang. You can see the
build on travis-CI[1]. I am not sure of other compilers and also don't
know a way to test it either. You could use my branch on github[2] if
you want to test it on other compilers. I think its better to keep the
return 0 if we aren't sure whether it would work on every compiler.

[1]: https://travis-ci.org/pranitbauva1997/git/builds/131622175
[2]: https://github.com/pranitbauva1997/git/tree/return-try

Regards,
Pranit Bauva
--
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 v5 0/2] bisect--helper: rewrite of check-term-format()

[Pranit Bauva]

Hey Eric,
Sorry for the late reply. I was on vacation.

On Mon, May 16, 2016 at 6:05 AM, Eric Sunshine  wrote:
> On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva  wrote:
>> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva  wrote:
>>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano  wrote:
 Pranit Bauva  writes:
> I completely missed your point and you want me to go the Eric Sunshine's 
> way?

 I am neutral.

 When I read your response to Eric's "top down" suggestion, I didn't
 quite get much more than "I started going this way, and I do not
 want to change to the other direction.", which was what I had the
 most trouble with.  If your justification for the approach to start
 from building a tiny bottom layer that will need to be dismantled
 soon and repeat that (which sounds somewhat wasteful) were more
 convincing, I may have felt differently.
>>>
>>> Sorry if it seemed that "I have done quite some work and I don't want
>>> to scrape it off and redo everything". This isn't a case for me. I
>>> think of this as just a small part in the process of learning and my
>>> efforts would be completely wasted as I can still reuse the methods I
>>
>> efforts would **not** be completely wasted
>>
>>> wrote. This is still open for a "philosophical" discussion. I am
>>> assuming 1e1ea69fa4e is how Eric is suggesting.
>
> Speaking of 1e1ea69 (pull: implement skeletal builtin pull,
> 2015-06-14), one of the (numerous) things Paul Tan did which impressed
> me was to formally measure test suite coverage of the commands he was
> converting to C, and then improve coverage where it was lacking. That
> approach increases confidence in the conversion far more than fallible
> human reviews do.
>
> Setting aside the top-down vs. bottom-up discussion, as a reviewer
> (and user) I'd be far more interested in seeing you spend a good
> initial chunk of your project emulating Paul's approach to measuring
> and improving test coverage (though I don't know how your GSoC mentors
> feel about that).

Just adding to the points mentioned by Christian.

I had initially planned to first improve test coverage and then start
with function conversion as I mentioned in my introductory mail[1]. I
also pointed out that I did some work searching about the tools to
test coverage (kcov as Matthieu) suggested and I found that it is not
that easy to set it up. Then Christian pointed it out (privately) that
I can do this afterwards too before the code is finally merged. And
also I am trying to see the test coverage as and when I am converting
each function.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/292308

Regards,
Pranit Bauva
--
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] upload-pack.c: use of parse-options API

[Matthieu Moy]

Junio Hamano wrote:
> Matthieu Moy  writes:
> 
> > antoine.qu...@ensimag.grenoble-inp.fr wrote:
> >> come from the commit (gmane/131517)
> >
> > Please, use a real commit id instead of a Gmane link.
> >
> > We don't know how long Gmane will remain up, but a self
> > reference from Git's history to itself will always remain valid.
> >
> > The following alias is handy for this:
> >
> > [alias]
> > whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short
> >
> > In your case it would allow writing:
> >
> > Description for --stateless-rpc and --advertise-refs is taken
> > from commit 42526b4 (Add stateless RPC options to upload-pack,
> > receive-pack, 2009-10-30).
> 
> Good suggestion; a real question may be how you went from
> $gmane/131517 to 42526b4 (I vaguely recall that you have and publish
> a database of sort; this would be a good place to advertise it ;-).

I don't (I think someone has, but I'm not this someone).

I just "git log --grep"-ed the subject line in Git's history.

> quit after a single request/response exchange

Seems much clearer to me.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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] upload-pack.c: use of parse-options API

[Matthieu Moy]

Junio C Hamano wrote:
> Encouraging use of "send-email" with "--patience" is a losing
> approach if your goal is to present more reviewable diff, isn't it?
> Using "send-email" as if it is a front-end of "format-patch" means
> you lose the opportunity for the final proof-reading (not just
> finding typoes in the message, but the shape of diff, like you are
> pointing out).
> 
> Using "format-patch --patience" or some other diff option, and pick
> the best one to give to "send-email" would indeed be a way to do so.

It's a matter of taste. My flow is "send-email-only", I do as much as
possible in-tree, and when I notice something to do during "git send-email",
I just abort and retry. So "Oops, looks ugly, I'll try with another
option" is OK in this flow.

> What is happening is that Antoine's patch (which is slightly
> different from what you quoted above) has trailing whitespace after
> "setup_path();"

Nice catch!

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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