Re: [PATCH 0/8] consistent "-h" handling in builtins

[Junio C Hamano]

The series was pretty straight-forward.  Thanks for working on this.

Re: [PATCH 1/8] am: handle "-h" argument earlier

[Junio C Hamano]

Jeff King  writes:

> We can't easily move that setup to after the parse_options()
> call; the point is to set up defaults that are overwritten
> by the option parsing. Instead, we'll detect the "-h" case
> early and show the usage then. This matches the behavior of
> other builtins which have a similar setup-ordering issue
> (e.g., git-branch).

Thanks.  And this structure of the series is very much appreciated.


> Signed-off-by: Jeff King 
> ---
>  builtin/am.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/am.c b/builtin/am.c
> index 0f63dcab1..5ee146bfb 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char 
> *prefix)
>   OPT_END()
>   };
>  
> + if (argc == 2 && !strcmp(argv[1], "-h"))
> + usage_with_options(usage, options);
> +
>   git_config(git_am_config, NULL);
>  
>   am_state_init();

Re: [PATCHv2 0/8] A reroll of sb/submodule-blanket-recursive

[Junio C Hamano]

Stefan Beller  writes:

> v2:
> * A reroll of sb/submodule-blanket-recursive.
> * This requires ab/grep-preparatory-cleanup 

2/8 seems to be more stale than sb/checkout-recurse-submodules that
was merged at f1101cef to 'master'.  I'll try to merge Ævar's series
to 'master' before that merge, queue these patches and see if the
resulting history is too messy.

Thanks.

[PATCH 8/8] t0012: test "-h" with builtins

[Jeff King]

Since commit 99caeed05 (Let 'git  -h' show usage
without a git dir, 2009-11-09), the git wrapper handles "-h"
specially, skipping any repository setup but still calling
the builtin's cmd_foo() function. This means that every
cmd_foo() must be ready to handle this case, but we don't
have any systematic tests. This led to "git am -h" being
broken for some time without anybody noticing.

This patch just tests that "git foo -h" works for every
builtin, where we see a 129 exit code (the normal code for
our usage() helper), and that the word "usage" appears in
the output.

Signed-off-by: Jeff King 
---
 t/t0012-help.sh | 12 
 1 file changed, 12 insertions(+)

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 8faba2e8b..487b92a5d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
test_i18ncmp expect actual
 "
 
+test_expect_success 'generate builtin list' '
+   git --list-builtins >builtins
+'
+
+while read builtin
+do
+   test_expect_success "$builtin can handle -h" '
+   test_expect_code 129 git $builtin -h >output 2>&1 &&
+   test_i18ngrep usage output
+   '
+done 

[PATCH 7/8] git: add hidden --list-builtins option

[Jeff King]

It can be useful in the test suite to be able to iterate
over the list of builtins. We could do this with some
Makefile magic. But since the authoritative list is in the
commands array inside git.c, and since this could also be
handy for debugging, let's add a hidden command-line option
to dump that list.

Signed-off-by: Jeff King 
---
The forward declaration here is necessary because of the function
ordering. We could push handle_options() much lower in the file, but I
didn't think it was worth the churn.

 git.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/git.c b/git.c
index 8ff44f081..1b8b7f51a 100644
--- a/git.c
+++ b/git.c
@@ -26,6 +26,8 @@ static const char *env_names[] = {
 static char *orig_env[4];
 static int save_restore_env_balance;
 
+static void list_builtins(void);
+
 static void save_env_before_alias(void)
 {
int i;
@@ -232,6 +234,9 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
}
(*argv)++;
(*argc)--;
+   } else if (!strcmp(cmd, "--list-builtins")) {
+   list_builtins();
+   exit(0);
} else {
fprintf(stderr, "Unknown option: %s\n", cmd);
usage(git_usage_string);
@@ -529,6 +534,13 @@ int is_builtin(const char *s)
return !!get_builtin(s);
 }
 
+static void list_builtins(void)
+{
+   int i;
+   for (i = 0; i < ARRAY_SIZE(commands); i++)
+   printf("%s\n", commands[i].cmd);
+}
+
 #ifdef STRIP_EXTENSION
 static void strip_extension(const char **argv)
 {
-- 
2.13.0.613.g11c956365

[PATCH 6/8] version: convert to parse-options

[Jeff King]

The "git version" command didn't traditionally accept any
options, and in fact ignores any you give it. When we added
simple option parsing for "--build-options" in 6b9c38e14, we
didn't improve this; we just loop over the arguments and
pick out the one we recognize.

Instead, let's move to a real parsing loop, complain about
nonsense options, and recognize conventions like "-h".

Signed-off-by: Jeff King 
---
I assume nobody was running "git version --foobar" and expecting it to
work. I guess we could also complain about "git version foobar" (no
dashes), but this patch doesn't. Mainly I wanted the auto-generated
options list.

 help.c | 25 -
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/help.c b/help.c
index a07f01e6f..41017e88e 100644
--- a/help.c
+++ b/help.c
@@ -8,6 +8,7 @@
 #include "column.h"
 #include "version.h"
 #include "refs.h"
+#include "parse-options.h"
 
 void add_cmdname(struct cmdnames *cmds, const char *name, int len)
 {
@@ -424,16 +425,30 @@ const char *help_unknown_cmd(const char *cmd)
 
 int cmd_version(int argc, const char **argv, const char *prefix)
 {
+   int build_options = 0;
+   const char * const usage[] = {
+   N_("git version []"),
+   NULL
+   };
+   struct option options[] = {
+   OPT_BOOL(0, "build-options", _options,
+"also print build options"),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options, usage, 0);
+
/*
 * The format of this string should be kept stable for compatibility
 * with external projects that rely on the output of "git version".
+*
+* Always show the version, even if other options are given.
 */
printf("git version %s\n", git_version_string);
-   while (*++argv) {
-   if (!strcmp(*argv, "--build-options")) {
-   printf("sizeof-long: %d\n", (int)sizeof(long));
-   /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
-   }
+
+   if (build_options) {
+   printf("sizeof-long: %d\n", (int)sizeof(long));
+   /* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
}
return 0;
 }
-- 
2.13.0.613.g11c956365

[PATCH 5/8] submodule--helper: show usage for "-h"

[Jeff King]

Normal users shouldn't ever call submodule--helper, but it
doesn't hurt to give them a normal usage message if they try
"-h".

Signed-off-by: Jeff King 
---
The usage message isn't that helpful _either_, and I admit
my ulterior motive is just to make the test at the end of
this series pass. :)

I was tempted to actually dump the names from the commands
array to stdout, but this command really is an internal
helper. It's probably not worth spending time on such
niceties.

 builtin/submodule--helper.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 566a5b6a6..a78b003c2 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1222,9 +1222,8 @@ static struct cmd_struct commands[] = {
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
 {
int i;
-   if (argc < 2)
-   die(_("submodule--helper subcommand must be "
- "called with a subcommand"));
+   if (argc < 2 || !strcmp(argv[1], "-h"))
+   usage("git submodule--helper ");
 
for (i = 0; i < ARRAY_SIZE(commands); i++) {
if (!strcmp(argv[1], commands[i].cmd)) {
-- 
2.13.0.613.g11c956365

[PATCH 4/8] remote-{ext,fd}: print usage message on invalid arguments

[Jeff King]

We just say "Expected two arguments" when we get a different
number of arguments, but we can be slightly friendlier.
People shouldn't generally be running remote helpers
themselves, but curious users might say "git remote-ext -h".

Signed-off-by: Jeff King 
---
According to remote-curl.c, we should actually handle the
1-argument case, too. I didn't dig into that because it's
orthogonal to this series, and it's not clear that anybody
cares.

 builtin/remote-ext.c | 5 -
 builtin/remote-fd.c  | 5 -
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 11b48bfb4..bfb21ba7d 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -3,6 +3,9 @@
 #include "run-command.h"
 #include "pkt-line.h"
 
+static const char usage_msg[] =
+   "git remote-ext  ";
+
 /*
  * URL syntax:
  * 'command [arg1 [arg2 [...]]]'   Invoke command with given arguments.
@@ -193,7 +196,7 @@ static int command_loop(const char *child)
 int cmd_remote_ext(int argc, const char **argv, const char *prefix)
 {
if (argc != 3)
-   die("Expected two arguments");
+   usage(usage_msg);
 
return command_loop(argv[2]);
 }
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index 08d7121b6..91dfe07e0 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -1,6 +1,9 @@
 #include "builtin.h"
 #include "transport.h"
 
+static const char usage_msg[] =
+   "git remote-fd  ";
+
 /*
  * URL syntax:
  * 'fd::[/]'Read/write socket pair
@@ -57,7 +60,7 @@ int cmd_remote_fd(int argc, const char **argv, const char 
*prefix)
char *end;
 
if (argc != 3)
-   die("Expected two arguments");
+   usage(usage_msg);
 
input_fd = (int)strtoul(argv[2], , 10);
 
-- 
2.13.0.613.g11c956365

[PATCH 3/8] upload-archive: handle "-h" option early

[Jeff King]

Normally upload-archive forks off upload-archive--writer to
do the real work, and relays any errors back over the
sideband channel. This is a good thing when the command is
properly invoked remotely via ssh or git-daemon. But it's
confusing to curious users who try "git upload-archive -h".

Let's catch this invocation early and give a real usage
message, rather than spewing "-h does not appear to be a git
repository" amidst packet-lines. The chance of a false
positive due to a real client asking for the repo "-h" is
quite small.

Likewise, we'll catch "-h" in upload-archive--writer. People
shouldn't be invoking it manually, but it doesn't hurt to
give a sane message if they do.

Signed-off-by: Jeff King 
---
 builtin/upload-archive.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index cde06977b..84532ae9a 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -22,7 +22,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, 
const char *prefix)
struct argv_array sent_argv = ARGV_ARRAY_INIT;
const char *arg_cmd = "argument ";
 
-   if (argc != 2)
+   if (argc != 2 || !strcmp(argv[1], "-h"))
usage(upload_archive_usage);
 
if (!enter_repo(argv[1], 0))
@@ -76,6 +76,9 @@ int cmd_upload_archive(int argc, const char **argv, const 
char *prefix)
 {
struct child_process writer = { argv };
 
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage(upload_archive_usage);
+
/*
 * Set up sideband subprocess.
 *
-- 
2.13.0.613.g11c956365

[PATCH 2/8] credential: handle invalid arguments earlier

[Jeff King]

The git-credential command only takes one argument: the
operation to perform. If we don't have one, we complain
immediately. But if we have one that we don't recognize, we
don't notice until after we've read the credential from
stdin. This is likely to confuse a user invoking "git
credential -h", as the program will hang waiting for their
input before showing anything.

Let's detect this case early. Likewise, we never noticed
when there are extra arguments beyond the one we're
expecting. Let's catch this with the same conditional.

Note that we don't need to handle "--help" similarly,
because the git wrapper does this before even calling
cmd_credential().

Signed-off-by: Jeff King 
---
 builtin/credential.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/credential.c b/builtin/credential.c
index 0412fa00f..879acfbcd 100644
--- a/builtin/credential.c
+++ b/builtin/credential.c
@@ -10,9 +10,9 @@ int cmd_credential(int argc, const char **argv, const char 
*prefix)
const char *op;
struct credential c = CREDENTIAL_INIT;
 
-   op = argv[1];
-   if (!op)
+   if (argc != 2 || !strcmp(argv[1], "-h"))
usage(usage_msg);
+   op = argv[1];
 
if (credential_read(, stdin) < 0)
die("unable to read credential from stdin");
-- 
2.13.0.613.g11c956365

[PATCH 1/8] am: handle "-h" argument earlier

[Jeff King]

If the user provides "-h" on the command line, then our
parse_options() invocation will show a usage message and
quit. But if "-h" is the only argument, the git wrapper
behaves specially: it ignores our RUN_SETUP flag and calls
cmd_am() without having done repository setup at all.  This
is due to 99caeed05 (Let 'git  -h' show usage
without a git dir, 2009-11-09).

Before cmd_am() calls parse_options(), though, it runs a few
other setup functions. One of these is am_state_init(),
which uses git_pathdup() to set up the default rebase-apply
path. But calling git_pathdup() when we haven't done
repository setup will fall back to using ".git". That's
mostly harmless (since we won't use the value anyway), but
is forbidden since b1ef400eec ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20), and we now BUG().

We can't easily move that setup to after the parse_options()
call; the point is to set up defaults that are overwritten
by the option parsing. Instead, we'll detect the "-h" case
early and show the usage then. This matches the behavior of
other builtins which have a similar setup-ordering issue
(e.g., git-branch).

Signed-off-by: Jeff King 
---
 builtin/am.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/am.c b/builtin/am.c
index 0f63dcab1..5ee146bfb 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage_with_options(usage, options);
+
git_config(git_am_config, NULL);
 
am_state_init();
-- 
2.13.0.613.g11c956365

[PATCH 0/8] consistent "-h" handling in builtins

[Jeff King]

On Mon, May 29, 2017 at 11:32:50AM -0400, Jeff King wrote:

> I'll try to put together patches in the next day or so. Comments welcome
> in the meantime.

So here they are. For those just joining us, the immediate problem is
that "git am -h" is broken (whether you're in a repo or not). That's
fixed by the first patch, which can go to "maint" separately (although
the rest are pretty unadventurous).

The rest of it cleans up "-h" handling for a variety of builtin
commands, and then adds a test to make sure they all behave sanely
(which unsurprisingly is how I found all the problems fixed by the
earlier patches).

  [1/8]: am: handle "-h" argument earlier
  [2/8]: credential: handle invalid arguments earlier
  [3/8]: upload-archive: handle "-h" option early
  [4/8]: remote-{ext,fd}: print usage message on invalid arguments
  [5/8]: submodule--helper: show usage for "-h"
  [6/8]: version: convert to parse-options
  [7/8]: git: add hidden --list-builtins option
  [8/8]: t0012: test "-h" with builtins

 builtin/am.c|  3 +++
 builtin/credential.c|  4 ++--
 builtin/remote-ext.c|  5 -
 builtin/remote-fd.c |  5 -
 builtin/submodule--helper.c |  5 ++---
 builtin/upload-archive.c|  5 -
 git.c   | 12 
 help.c  | 25 -
 t/t0012-help.sh | 12 
 9 files changed, 63 insertions(+), 13 deletions(-)

-Peff

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Mon, 29 May 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > diff --git a/sequencer.c b/sequencer.c
>> > index 130cc868e51..88819a1a2a9 100644
>> > --- a/sequencer.c
>> > +++ b/sequencer.c
>> > @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
>> > ignore_footer, unsigned flag)
>> >  
>> >strbuf_release();
>> >  }
>> > +
>> > +int sequencer_make_script(int keep_empty, FILE *out,
>> > +  int argc, const char **argv)
>> > +{
>> > +  char *format = NULL;
>> > +  struct pretty_print_context pp = {0};
>> > +  struct strbuf buf = STRBUF_INIT;
>> > +  struct rev_info revs;
>> > +  struct commit *commit;
>> > +
>> > +  init_revisions(, NULL);
>> > +  revs.verbose_header = 1;
>> > +  revs.max_parents = 1;
>> > +  revs.cherry_pick = 1;
>> > +  revs.limited = 1;
>> > +  revs.reverse = 1;
>> > +  revs.right_only = 1;
>> > +  revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
>> > +  revs.topo_order = 1;
>> 
>> cf. 
>> 
>> This is still futzing below the API implementation detail
>> unnecessarily.
>
> You still ask me to pass options in plain text that has to be parsed at
> run-time, rather than compile-time-verifiable flags.

Absolutely.  

We do not want these implementation details to code that does not
implement command line parsing.  This one is not parsing anybody's
set of options and should not be mucking with the low level
implementation details.

See 66b2ed09 ("Fix "log" family not to be too agressive about
showing notes", 2010-01-20) and poinder.  Back then, nobody outside
builtin/log.c and revision.c (these are the two primary things that
implement command line parsing; "log.c" needs access to the low
level details because it wants to establish custom default that is
applied before it reads options given by the end-user) mucked
directly with verbose_header, which allowed the addition of
"pretty_given" to be limited only to these places that actually do
the parsing.  If the above patch to sequencer.c existed before
66b2ed09, it would have required unnecessary change to tweak
"pretty_given" in there too when 66b2ed09 was done.  That is forcing
a totally unnecesary work.  And there is no reason to expect that
the kind of change 66b2ed09 made to the general command line parsing
will not happen in the future.  Adding more code like the above that
knows the implementation detail is unwarranted, when you can just
ask the existing command line parser to set them for you.

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Johannes Sixt  writes:

>> Doesn't this need test_i18ngrep?:
>
> Good catch! It would be this one in warn_on_inaccessible:
>
>>  wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
>
> But actually, I'm more worried about the unholy mix of
> one-test-first-then-skip_all-later that occurs in this test script (I
> do not mean the skip_all that is visible in the context, there are
> others later). I think there was some buzz recently that prove only
> understands a summary line that reads "1..0", but here we would see
> "1..1". What to do? Reorganize the test script? Dscho, any ideas?

For now I've queued this on top of 1/2, so that suggestions are not
lost, and then tweaked 2/2 (as context for the patch to the test
changes).  

Either an ack or a reroll is appreciated (I do not think we'd
terribly mind if this test were added to another script, or if this
test were skipped when UNC path cannot be determined even though it
does not need that prereq.  Also UNC_PATH can become prereq that is
tested by individual test in this script and the new test can be
added without requiring that prereq).

Thanks.

 t/t5580-clone-push-unc.sh | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..944730cddc 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,12 +8,6 @@ if ! test_have_prereq MINGW; then
test_done
 fi
 
-test_expect_failure 'remote nick cannot contain backslashes' '
-   BACKSLASHED="$(pwd | tr / )" &&
-   git ls-remote "$BACKSLASHED" >out 2>err &&
-   ! grep "unable to access" err
-'
-
 UNCPATH="$(pwd)"
 case "$UNCPATH" in
 [A-Z]:*)
@@ -51,4 +45,10 @@ test_expect_success push '
test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
 '
 
+test_expect_failure 'remote nick cannot contain backslashes' '
+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   test_i18ngrep ! "unable to access" err
+'
+
 test_done
-- 
2.13.0-493-g9105ebc082

Re: [PATCH] completion: Add completions for git config commit

[Junio C Hamano]

Will queue; thanks.

Re: [PATCH] doc: Improve description for rev-parse --short

[Junio C Hamano]

Andreas Heiduk  writes:

> First: `git rev-parse --short` without a number does use a fixed default but
> `core.abbrev` which in turn uses `find_unique_abbrev` internally.

... hence the value gives mere minumum.  I like your updated text that
clarifies this point.

> Second: `--short` implies `--verify` since the beginning (d50125085a), so
> it cannot be used for bulk-shortening ids unfortunately.

The fact you have to say "Nth:" hints that this is better done as
two patch series.  Then you can avoid saying a vague "Improve" on
the subject, that leaves the "git shortlog" readers wondering what
exactly you improved.

> Signed-off-by: Andreas Heiduk 
> ---
>  Documentation/config.txt| 1 +
>  Documentation/git-rev-parse.txt | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e0b9fd0bc..158cb588b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -862,6 +862,7 @@ core.abbrev::
>   computed based on the approximate number of packed objects
>   in your repository, which hopefully is enough for
>   abbreviated object names to stay unique for some time.
> + The minimum length is 4.
>  
>  add.ignoreErrors::
>  add.ignore-errors (deprecated)::
> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index c40c47044..7a7421c8e 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -140,7 +140,9 @@ can be used.
>  --short=number::
>   Instead of outputting the full SHA-1 values of object names try to
>   abbreviate them to a shorter unique name. When no length is specified
> - 7 is used. The minimum length is 4.
> + the effective value of the configuration variable `core.abbrev` (see
> + linkgit:git-config[1]) is used.  The minimum length is 4.  The length
> + may be exceeded to ensure unique object names.  Implies `--verify`.

"Implies --verify" is less important than the fact that multiple
object names cannot be given from the end-users' (and readers')
point of view, no?  The sentence in the pre-context still hints
(incorrectly) that we might take multiple names---that would want to
be corrected, no?

Let me try.

--short[=length]::
Take a single object name, and output a prefix of the object
name whose length is at least the specified length and
sufficient to ensure uniqueness of the name.  The minimum
length is 4.  When no length is given, the effective value
of the `core.abbrev` configuration variable is used.

Thanks.

>  
>  --symbolic::
>   Usually the object names are output in SHA-1 form (with

Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King]

On Tue, May 30, 2017 at 12:53:47PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:
> >
> >> Unfortunately, putting the default refspec into this temporary
> >> configuration environment breaks a few submodule tests
> >> (t5614-clone-submodules or t5614-clone-submodules-shallow (it's got
> >> renamed between this topic and master), t7407-submodule-foreach,
> >> t7410-submodule-checkout-to), because it "leaks" to the submodule
> >> environment.
> >
> > Doh, of course. I didn't think of that. That's probably a bad direction,
> > then, as there's no "just for this process" global config.
> 
> Yuck, right.  So... do we have a further plan for this topic, or are
> the patches more or less good as they are?

I was hoping we'd see one more iteration with the abstracted function
for adding to the "struct remote". Gábor, let me know if you're too sick
of the topic, and I can try to re-spin it.

-Peff

Re: [PATCHv3 1/4] clone: respect additional configured fetch refspecs during initial fetch

[Junio C Hamano]

Jeff King  writes:

> On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:
>
>> Unfortunately, putting the default refspec into this temporary
>> configuration environment breaks a few submodule tests
>> (t5614-clone-submodules or t5614-clone-submodules-shallow (it's got
>> renamed between this topic and master), t7407-submodule-foreach,
>> t7410-submodule-checkout-to), because it "leaks" to the submodule
>> environment.
>
> Doh, of course. I didn't think of that. That's probably a bad direction,
> then, as there's no "just for this process" global config.

Yuck, right.  So... do we have a further plan for this topic, or are
the patches more or less good as they are?

Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol

[Junio C Hamano]

Junio C Hamano  writes:

> If you are depending on a single topic in 'next', it is better to
> build on the tip of that topic, not on 'next', if you can figure out
> where the tip is.  In practice, while we are exchanging patches via
> e-mail, there should be no noticeable difference either way [*1*],
> ...

And the forgotten foot note would have said something like this:

 *1* If you are depending only on a single topic in 'next', and
 other topics in 'next' do not interfere with your work, then by
 definition, your patches that apply cleanly on 'next' ought to
 apply cleanly on the tip of that single topic.

;-)

Re: FORMAT_PATCH_NAME_MAX increase

[Junio C Hamano]

Laszlo Ersek  writes:

> would it be possible to
>
> - increase the FORMAT_PATCH_NAME_MAX macro from 64 to, say, 128?
>
> - Or else to introduce a new git-config knob for it?

It's open source, so both are "possible", but you are interested in
learning if these are acceptable project-wide.

> I have a small review-helper / interdiff script that matches patches
> from adjacent versions of a series against each other, based on subject
> line. (Using the formatted file name with the patch number stripped.)
> The project in question uses long common prefixes in subjects, and the
> current limit of 64 does not always ensure unicity (again, with the
> number stripped).

The original motivation of the design is that leading numbers are to
ensure they are unique, and munging and truncating of title are to
ensure that they are safe on the filesystem while being identifiable
for eyeballing.  And the current truncation limit may not work well
for certain projects, failing the "identifiabl" goal.

But I do not think that is true only for your custom script.  For
example, with such a project, output from "git shortlog" or the
labels "gitk" gives to commits would not be very useful, because 2/3
of your display are filled with and wasted by the same long prefix
to the left.  The real issue is not that one particular "length
limit" used by format-patch is too short; it is that the project
convention wastes too many bytes for common things that can be left
unsaid, and if we come up with a general way to address that, we'd
make all of Git that summarises commits more useful, not just
format-patch.

So I think lengthening FORMAT_PATCH_NAME_MAX, whether it is done
unconditionally or conditionally, is rather an ad-hoc hack than
a real improvement.

I cannot offhand guess what other places would suffer from such a
project convention, because I do not work with such a project, but
you may be able to come up with a list of various places in Git
where the commit titles are used, and that if there were a mechanism
to take these commit titles, pass them to your cutomization script,
which abbreviates these "long common prefixes" to a shorter string,
and to use the output from that script instead of the actual commit
title, it would help all of these places.  The list of commits "git
merge" records when merging a side branch may benefit from the same
mechanism.

For example, if the titles of your commit title look like this:

Subject: RedHat Kernel Maintenance: ipv4: add reference count
Subject: RedHat Kernel Maintenance: net: llc: add lock_sock
...

your custom script may be

#!/bin/sh
exec sed -e "s/^RedHat Kernel Maintenance: /RHKM-/"
... there may be more common patterns to shorten ...

and the resulting output from format-patch might become

0001-RHKM-ipv4-add-reference-count.patch
0002-RHKM-net-llc-add-lock_sock.patch

while "git shortlog" output and "gitk" display may also be helped
by the same mechanism.

It's OK for the _initial_ application of such a mechanism were to
affect the names of files format-patch creates and nothing else.
Interested parties can then use the same mechanism in other programs
(like "gitk" and "git shortlog"), as long as that mechansim is
designed well.  And then the end users need to learn that mechanism
only once.

Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol

[Junio C Hamano]

Lars Schneider  writes:

>> That's right. There might be some code sharing opportunity with Ben's
>> code that is already in "next":
>> https://github.com/git/git/blob/next/convert.c#L660-L677
>> 
>> Would it be useful for you if I send v5 with the changes rebased 
>> onto "next"?
>
> Hi Junio,
>
> sorry for bugging you again, but Ben's topic did not make it to "master"
> today. Is it OK if I rebase my topic onto "next" and resend?

Sorry, your earlier question was lost in the noise and I should have
picked it up during my last sweep of leftover bits.

If you are depending on a single topic in 'next', it is better to
build on the tip of that topic, not on 'next', if you can figure out
where the tip is.  In practice, while we are exchanging patches via
e-mail, there should be no noticeable difference either way [*1*],
but once you start throwing a complex and long series, you may want
to publish it to a public repository for reviewers and the
maintainer to pull, and that workflow might give us an easier way to
review, but a topic based on 'next' will never have a chance to be
pulled to be merged for real, as merging its tip to 'master' means
it will bring all other junk that may not ready.  So if you anticipate
that to happen someday, practicing to build on things that are only
needed (e.g. if you depend on two topics, you may start by merging
them on top of 'master' and then building your change on top) is a
good idea.

No matter what you do, please mention on top of what you built your
work.

Thanks.

Re: [RFC/PATCH] WIP: add deprecation & experimental process/interface

[Junio C Hamano]

Duy Nguyen  writes:

>>  * Add a new config variable `core.version`. E.g. `core.version =
>>2.14.0` With this the user can specify that they'd like
>>new/experimental features introduced in that version (and below),
>>as well as immediately getting new deprecations added in that
>>version as errors.
>
> We have extensions.* for this purpose (or close to this purpose). I
> think it's more flexible to go with extensions.* instead of a single
> "core.version". extensions.* are non-optional though (if a git binary
> does not understand it, the repo can't be accessed). So it's more
> about fundamental experiments (like sha256 transition). I'm guessing
> we can have a "soft" extensions (warn if not understand, instead of
> die), like what we have in $GIT_DIR/index.
>
> Deprecation via extension.* though may be unintuitive. But I think
> something along that line (e.g. deprecation.*) might work.

The difference/orthogonal-ness of what Ævar wants to do and what the
extension mechanism wants to do has been covered by others correctly
so I won't repeat.  But I do agree with you that "run at this
version level" is probably less useful than a la carte "I want to
enroll in this and that experiment but not the other one".

I also agree with Peff that we should name it as distinctively as
the names used by extensions mechaism---we must strongly discourage
the latter from being futzed by the end users, while this opt-in
thing is very much open to them.

Re: mergetool: what to do about deleting precious files?

[Junio C Hamano]

"Philip Oakley"  writes:

> If I now understand correctly, the merge process flow is:
>
> * canonicalise content (eol, smudge-clean, $id, renormalise, etc)
> * diff the content (internal, or GIT_EXTERNAL_DIFF)
> * apply the diff
> * if conflicts, only then use merge-driver/tool
>
> Would that be a correct interpretation?

Not quite.  There are a lot more going on before any of those steps:

 * Find the common ancestor commit (which could be many).

 * Walk the three trees (the common ancestor's, ours and theirs) in
   parallel, noticing what happened to each path.  Depending on what
   happened to the path in each branch, the merge may or may not
   "conflict" (e.g. when both sides added exactly the same contents
   to the same path, they are not counted as conflicting.  when we
   removed while they modified, they show as conflicting).

 * For paths that are conflicting, feed the canonicalized content of
   the versions from common, ours and theirs to the file-level merge
   driver.  The builtin file-level merge driver takes two xdiff (one
   between ancestor and ours, the other between ancestore and
   theirs) and reconciles them to produce the result.  But that is
   irrelevant in the context of "custom merge driver"; the builtin
   one is skipped altogether and the custom contents merge driver
   the user specified via the attributes is used instead.

Notice that the second step above has no customization knobs.  Any
path the second step deems not to conflict is "merged cleanly"
without even triggering the "oops, ours and theirs did conflicting
changes, to the content; let's see how the final content should look
like" (aka the third step).  This is *not* because "Git knows the
best"; it is merely that nobody felt the need for a mechanism to
allow customizing the second step.

And that is why I said you need a new customization mechanism if you
want to affect the outcome of the scenario that started this thread.

Re: What's cooking in git.git (May 2017, #08; Mon, 29)

[Junio C Hamano]

Samuel Lijin  writes:

> "git status --ignored" previously did not list ignored files in
> untracked directories without -uall, contrary to the documented
> behavior of the --ignored flag (that all ignored files would be
> listed). This has also been corrected.

It's a balancing act to decide what level of details when
summarizing what individual topics are about in the release notes.
I still think "clean -d" fix is the primary effect to the users, but
the change to "status -i" is also visible, so I do not mind spending
four more lines in the release notes to lengthen the description of
this topic.

Thanks.

[PATCH 2/2] treewide: use is_missing_file_error() where ENOENT and ENOTDIR are checked

[Junio C Hamano]

Using the is_missing_file_error() helper introduced in the previous
step, update all hits from

  $ git grep -e ENOENT --and -e ENOTDIR

There are codepaths that only check ENOENT, and it is possible that
some of them should be checking both.  Updating them is kept out of
this step deliberately, as we do not want to change behaviour in this
step.

Signed-off-by: Junio C Hamano 
---
 apply.c| 2 +-
 builtin/rm.c   | 2 +-
 builtin/update-index.c | 2 +-
 diff-lib.c | 2 +-
 dir.c  | 2 +-
 setup.c| 2 +-
 sha1_name.c| 4 ++--
 wrapper.c  | 4 ++--
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/apply.c b/apply.c
index 0e2caeab9c..59bb3497de 100644
--- a/apply.c
+++ b/apply.c
@@ -3741,7 +3741,7 @@ static int check_to_create(struct apply_state *state,
return 0;
 
return EXISTS_IN_WORKTREE;
-   } else if ((errno != ENOENT) && (errno != ENOTDIR)) {
+   } else if (!is_missing_file_error(errno)) {
return error_errno("%s", new_name);
}
return 0;
diff --git a/builtin/rm.c b/builtin/rm.c
index fb79dcab18..30c4332c68 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -129,7 +129,7 @@ static int check_local_mod(struct object_id *head, int 
index_only)
ce = active_cache[pos];
 
if (lstat(ce->name, ) < 0) {
-   if (errno != ENOENT && errno != ENOTDIR)
+   if (!is_missing_file_error(errno))
warning_errno(_("failed to stat '%s'"), 
ce->name);
/* It already vanished from the working tree */
continue;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index d530e89368..4e9402984a 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -253,7 +253,7 @@ static int remove_one_path(const char *path)
  */
 static int process_lstat_error(const char *path, int err)
 {
-   if (err == ENOENT || err == ENOTDIR)
+   if (is_missing_file_error(err))
return remove_one_path(path);
return error("lstat(\"%s\"): %s", path, strerror(err));
 }
diff --git a/diff-lib.c b/diff-lib.c
index 52447466b5..88fc71e89e 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -29,7 +29,7 @@
 static int check_removed(const struct cache_entry *ce, struct stat *st)
 {
if (lstat(ce->name, st) < 0) {
-   if (errno != ENOENT && errno != ENOTDIR)
+   if (!is_missing_file_error(errno))
return -1;
return 1;
}
diff --git a/dir.c b/dir.c
index aeeb5ce104..98efe9d1c5 100644
--- a/dir.c
+++ b/dir.c
@@ -2235,7 +2235,7 @@ int remove_path(const char *name)
 {
char *slash;
 
-   if (unlink(name) && errno != ENOENT && errno != ENOTDIR)
+   if (unlink(name) && !is_missing_file_error(errno))
return -1;
 
slash = strrchr(name, '/');
diff --git a/setup.c b/setup.c
index 8f64fbdfb2..bb6a2c1beb 100644
--- a/setup.c
+++ b/setup.c
@@ -147,7 +147,7 @@ int check_filename(const char *prefix, const char *arg)
name = arg;
if (!lstat(name, ))
return 1; /* file exists */
-   if (errno == ENOENT || errno == ENOTDIR)
+   if (is_missing_file_error(errno))
return 0; /* file does not exist */
die_errno("failed to stat '%s'", arg);
 }
diff --git a/sha1_name.c b/sha1_name.c
index 26ceec1d79..af7500037d 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1406,7 +1406,7 @@ static void diagnose_invalid_sha1_path(const char *prefix,
if (file_exists(filename))
die("Path '%s' exists on disk, but not in '%.*s'.",
filename, object_name_len, object_name);
-   if (errno == ENOENT || errno == ENOTDIR) {
+   if (is_missing_file_error(errno)) {
char *fullname = xstrfmt("%s%s", prefix, filename);
 
if (!get_tree_entry(tree_sha1, fullname,
@@ -1471,7 +1471,7 @@ static void diagnose_invalid_index_path(int stage,
 
if (file_exists(filename))
die("Path '%s' exists on disk, but not in the index.", 
filename);
-   if (errno == ENOENT || errno == ENOTDIR)
+   if (is_missing_file_error(errno))
die("Path '%s' does not exist (neither on disk nor in the 
index).",
filename);
 
diff --git a/wrapper.c b/wrapper.c
index 0542fc7582..2fbbd81359 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -583,8 +583,8 @@ void warn_on_inaccessible(const char *path)
 
 static int access_error_is_ok(int err, unsigned flag)
 {
-   return err == ENOENT || err == ENOTDIR ||
-   ((flag & ACCESS_EACCES_OK) && err == EACCES);
+   return (is_missing_file_error(err) ||
+   ((flag & ACCESS_EACCES_OK) && err == EACCES));
 }
 
 int access_or_warn(const char *path, int mode, unsigned flag)
-- 

[PATCH 1/2] compat-util: is_missing_file_error()

[Junio C Hamano]

Our code often opens a path to an optional file, to work on its
contents when we can successfully open it.  We can ignore a failure
to open if such an optional file does not exist, but we do want to
report a failure in opening for other reasons (e.g. we got an I/O
error, or the file is there, but we lack the permission to open).

The exact errors we need to ignore are ENOENT (obviously) and
ENOTDIR (less obvious).  Instead of repeating comparison of errno
with these two constants, introduce a helper function to do so.

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

 * Now this is independent of nd/fopen-errors topic.  We could tweak
   nd/fopen-errors topic to use this later, after both of these two
   topics have graduated.

 git-compat-util.h | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 8a4a3f85e7..8a3c680626 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1115,6 +1115,21 @@ struct tm *git_gmtime_r(const time_t *, struct tm *);
 #define getc_unlocked(fh) getc(fh)
 #endif
 
+/*
+ * Our code often opens a path to an optional file, to work on its
+ * contents when we can successfully open it.  We can ignore a failure
+ * to open if such an optional file does not exist, but we do want to
+ * report a failure in opening for other reasons (e.g. we got an I/O
+ * error, or the file is there, but we lack the permission to open).
+ *
+ * Call this function after seeing an error from open() or fopen() to
+ * see if the errno indicates a missing file that we can safely ignore.
+ */
+static inline int is_missing_file_error(int errno_)
+{
+   return (errno_ == ENOENT || errno_ == ENOTDIR);
+}
+
 extern int cmd_main(int, const char **);
 
 #endif
-- 
2.13.0-496-ga689fffbe2

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Johannes Sixt  writes:

> I would prefer to catch the case in the compatibility layer. Here is
> a two patch series that would replace your 12/13 and 13/13.

Thanks.  It is good that I can drop that last one.  Will replace
(with a SQUASH??? for 1/2).

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Ramsay Jones  writes:

> See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
> which is currently merged to the 'next' branch (merge 03b8a61e47 of the
> 'jc/skip-test-in-the-middle' branch).
>
> (see also http://testanything.org)
>
> If you look at http://testanything.org//tap-specification.html, it shows
> that you are allowed to annotate a plan of '1..0' with a SKIP directive
> to explain why no tests in a file were run. However, a plan with '1..n'
> (for any n > 0) must not have any annotation. (Back in 2012, when I wrote
> commit bf4b721932, I found much better documentation than the above!)
>
> So, after commit c7018be509, you can now use the 'skip_all' facility
> after having run some tests; it now converts that into an 'SKIP comment'
> just before the test plan, effectively skipping the remainder of the
> tests in the file. (since we are using a 'trailing plan', and have thus
> not declared how many tests we had intended to run, we can output an
> accurate plan).

Yes, but I consider that c7018be509 is an ugly workaround, not a
part of a properly designed framework.  Unless it is absolutely
necessary to run some tests before we may conditionally want to say
"skip_all/test_done", we should strive to add tests _after_ these
conditional skil_all/test_done is done.

In this case, I do not see there is a strong reason why the new test
must come before the "setup" test.  Sure, it does not use UNCPATH so
the new test may be able to run even when the current path cannot be
spelled as UNC, but that is a natural fallout of (ab)using the test
script that was meant for UNC testing for something else, so I think
a proper way out would be either (1) to use a separate script, if
the new test wants to run whether UNC path can be determined, or (2)
just accept the fact that the new test will only be run when UNC
paths are tested.  Relying on the hack c7018be509 did is much less
appealing.

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Junio C Hamano]

Johannes Sixt  writes:

> Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt  wrote:
>>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>>> index b195f71ea9..fd719a209e 100755
>>> --- a/t/t5580-clone-push-unc.sh
>>> +++ b/t/t5580-clone-push-unc.sh
>>> @@ -1,13 +1,19 @@
>>>   #!/bin/sh
>>>
>>> -test_description='various UNC path tests (Windows-only)'
>>> +test_description='various Windows-only path tests'
>>>   . ./test-lib.sh
>>>
>>>   if ! test_have_prereq MINGW; then
>>> -   skip_all='skipping UNC path tests, requires Windows'
>>> +   skip_all='skipping Windows-only path tests'
>>>  test_done
>>>   fi
>>>
>>> +test_expect_failure 'remote nick cannot contain backslashes' '
>>> +   BACKSLASHED="$(pwd | tr / )" &&
>>> +   git ls-remote "$BACKSLASHED" >out 2>err &&
>>> +   ! grep "unable to access" err
>>> +'
>>
>> Doesn't this need test_i18ngrep?:
>
> Good catch! It would be this one in warn_on_inaccessible:
>
>>  wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
>
> But actually, I'm more worried about the unholy mix of
> one-test-first-then-skip_all-later that occurs in this test script (I
> do not mean the skip_all that is visible in the context, there are
> others later). I think there was some buzz recently that prove only
> understands a summary line that reads "1..0", but here we would see
> "1..1". What to do? Reorganize the test script? Dscho, any ideas?

Put this new test after the other skip_all/test_done and you'd be
fine, I think.  It should come after the "setup" test anyway, no?

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Sahil Dua]

On Mon, May 29, 2017 at 10:50 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Mon, May 29, 2017 at 10:41 PM, Sahil Dua  wrote:
>> On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason
>>  wrote:
>>> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua  wrote:
 New feature - copying a branch along with its config section.

 Aim is to have an option -c for copying a branch just like -m option for
 renaming a branch.

 This commit adds a few basic tests for getting any suggestions/feedback
 about expected behavior for this new feature.

 Signed-off-by: Sahil Dua 
 ---
  t/t3200-branch.sh | 53 
 +
  1 file changed, 53 insertions(+)

 diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
 index fe62e7c775da..2c95ed6ebf3c 100755
 --- a/t/t3200-branch.sh
 +++ b/t/t3200-branch.sh
 @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, 
 too' '
 test_must_fail git config branch.s/s/dummy
  '

 +test_expect_success 'git branch -c dumps usage' '
 +   test_expect_code 128 git branch -c 2>err &&
 +   test_i18ngrep "branch name required" err
 +'
 +
 +git config branch.d.dummy Hello
 +
 +test_expect_success 'git branch -c d e should work' '
 +   git branch -l d &&
 +   git reflog exists refs/heads/d &&
 +   git branch -c d e &&
 +   git reflog exists refs/heads/d &&
 +   git reflog exists refs/heads/e
 +'
 +
 +test_expect_success 'config information was copied, too' '
 +   test $(git config branch.e.dummy) = Hello &&
 +   test $(git config branch.d.dummy) = Hello
 +'
 +
 +git config branch.f/f.dummy Hello
 +
 +test_expect_success 'git branch -c f/f g/g should work' '
 +   git branch -l f/f &&
 +   git reflog exists refs/heads/f/f &&
 +   git branch -c f/f g/g &&
 +   git reflog exists refs/heads/f/f &&
 +   git reflog exists refs/heads/g/g
 +'
 +
 +test_expect_success 'config information was copied, too' '
 +   test $(git config branch.f/f.dummy) = Hello &&
 +   test $(git config branch.g/g.dummy) = Hello
 +'
 +
 +test_expect_success 'git branch -c m2 m2 should work' '
 +   git branch -l m2 &&
 +   git reflog exists refs/heads/m2 &&
 +   git branch -c m2 m2 &&
 +   git reflog exists refs/heads/m2
 +'
 +
 +test_expect_success 'git branch -c a a/a should fail' '
 +   git branch -l a &&
 +   git reflog exists refs/heads/a &&
 +   test_must_fail git branch -c a a/a
 +'
 +
 +test_expect_success 'git branch -c b/b b should fail' '
 +   git branch -l b/b &&
 +   test_must_fail git branch -c b/b b
 +'
 +
  test_expect_success 'deleting a symref' '
 git branch target &&
 git symbolic-ref refs/heads/symref refs/heads/target &&

>>>
>>> This looks good to me. Comments, in no particular order:
>>>
>>> * There should be a test for `git branch -c `, i.e. that
>>> should implicitly copy from HEAD just like `git branch -m `
>>> does. However, when looking at this I can see there's actually no test
>>> for one-argument `git branch -m`, i.e. if you apply this:
>>>
>>> --- a/builtin/branch.c
>>> +++ b/builtin/branch.c
>>> @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const
>>> char *prefix)
>>> } else if (rename) {
>>> if (!argc)
>>> die(_("branch name required"));
>>> -   else if (argc == 1)
>>> -   rename_branch(head, argv[0], rename > 1);
>>> else if (argc == 2)
>>> rename_branch(argv[0], argv[1], rename > 1);
>>> else
>>>
>>> The only test that fails is a `git branch -M master`, i.e.
>>> one-argument -M is tested for, but not -m, same codepath though, but
>>> while you're at it a patch/series like this could start by adding a
>>> one-arg -m test, then follow-up by copying that for the new `-c`.
>>>
>>
>> Thanks for the suggestion. Yes, I will add one-arg test for -c. Is it
>> ok to send a different patch for adding a one-arg test for existing -m
>> option?
>
> Yeah, it makes sense to just make the first patch in the series be
> some cleanup / improvement of the existing tests, which the subsequent
> tests for -c then make use of / copy. It could even be sent on its
> own, but probably makes sense to just bundle them up. Up to you
> though, in this case you won't need patch A for patch B to work, so
> the that's one argument against bundling them up. Personally I'd do it
> if I was hacking this just because it's more convenient to keep track
> of fewer things.
>

Got it. I will submit a 

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Ramsay Jones]

On 29/05/17 22:02, Johannes Sixt wrote:
> Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:
>> On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt  wrote:
>>> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
>>> index b195f71ea9..fd719a209e 100755
>>> --- a/t/t5580-clone-push-unc.sh
>>> +++ b/t/t5580-clone-push-unc.sh
>>> @@ -1,13 +1,19 @@
>>>   #!/bin/sh
>>>
>>> -test_description='various UNC path tests (Windows-only)'
>>> +test_description='various Windows-only path tests'
>>>   . ./test-lib.sh
>>>
>>>   if ! test_have_prereq MINGW; then
>>> -   skip_all='skipping UNC path tests, requires Windows'
>>> +   skip_all='skipping Windows-only path tests'
>>>  test_done
>>>   fi
>>>
>>> +test_expect_failure 'remote nick cannot contain backslashes' '
>>> +   BACKSLASHED="$(pwd | tr / )" &&
>>> +   git ls-remote "$BACKSLASHED" >out 2>err &&
>>> +   ! grep "unable to access" err
>>> +'
>>
>> Doesn't this need test_i18ngrep?:
> 
> Good catch! It would be this one in warn_on_inaccessible:
> 
>>  wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
> 
> But actually, I'm more worried about the unholy mix of 
> one-test-first-then-skip_all-later that occurs in this test script (I do not 
> mean the skip_all that is visible in the context, there are others later). I 
> think there was some buzz recently that prove only understands a summary line 
> that reads "1..0", but here we would see "1..1". What to do? Reorganize the 
> test script? Dscho, any ideas?

See commit c7018be509 ("test: allow skipping the remainder", 18-05-2017)
which is currently merged to the 'next' branch (merge 03b8a61e47 of the
'jc/skip-test-in-the-middle' branch).

(see also http://testanything.org)

If you look at http://testanything.org//tap-specification.html, it shows
that you are allowed to annotate a plan of '1..0' with a SKIP directive
to explain why no tests in a file were run. However, a plan with '1..n'
(for any n > 0) must not have any annotation. (Back in 2012, when I wrote
commit bf4b721932, I found much better documentation than the above!)

So, after commit c7018be509, you can now use the 'skip_all' facility
after having run some tests; it now converts that into an 'SKIP comment'
just before the test plan, effectively skipping the remainder of the
tests in the file. (since we are using a 'trailing plan', and have thus
not declared how many tests we had intended to run, we can output an
accurate plan).

ATB,
Ramsay Jones

Re: Error with Templates: Could not find templates on cloning but on creating

[Johannes Sixt]

Am 29.05.2017 um 13:20 schrieb Mathias Artus:

Hi,
Today i've tried to set up a template directory. I added in the system wide 
gitconfig the following lines:

[init]
templatedir = "//OurServer/SomeDirectory/GitTemplate"

Where //Ourserver is a Network Path.
With this line i can create a new Repository and the template gets copied. But 
when i clone a repo the following error shows up and the template doesn't get 
copied:
templates not found /OurServer/SomeDirectory/GitTemplate

I Recognized that one slash was missing. Hence i added one:
[init]
templatedir = "///OurServer/SomeDirectory/GitTemplate"

Fine, cloning works after that, but creating a new repository then shows up a 
Warning:
templates not found ///OurServer/SomeDirectory/GitTemplate

Is that a known bug or is it my Failure?

I use git 2.13 on windows 7


I cannot reproduce. I'm on Windows 8.1, but I wouldn't expect that to 
make a difference. Are you using Cygwin's git by any chance?


-- Hannes

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:

On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt  wrote:

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b195f71ea9..fd719a209e 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -1,13 +1,19 @@
  #!/bin/sh

-test_description='various UNC path tests (Windows-only)'
+test_description='various Windows-only path tests'
  . ./test-lib.sh

  if ! test_have_prereq MINGW; then
-   skip_all='skipping UNC path tests, requires Windows'
+   skip_all='skipping Windows-only path tests'
 test_done
  fi

+test_expect_failure 'remote nick cannot contain backslashes' '
+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   ! grep "unable to access" err
+'


Doesn't this need test_i18ngrep?:


Good catch! It would be this one in warn_on_inaccessible:


 wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);


But actually, I'm more worried about the unholy mix of 
one-test-first-then-skip_all-later that occurs in this test script (I do 
not mean the skip_all that is visible in the context, there are others 
later). I think there was some buzz recently that prove only understands 
a summary line that reads "1..0", but here we would see "1..1". What to 
do? Reorganize the test script? Dscho, any ideas?


-- Hannes

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 10:41 PM, Sahil Dua  wrote:
> On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua  wrote:
>>> New feature - copying a branch along with its config section.
>>>
>>> Aim is to have an option -c for copying a branch just like -m option for
>>> renaming a branch.
>>>
>>> This commit adds a few basic tests for getting any suggestions/feedback
>>> about expected behavior for this new feature.
>>>
>>> Signed-off-by: Sahil Dua 
>>> ---
>>>  t/t3200-branch.sh | 53 
>>> +
>>>  1 file changed, 53 insertions(+)
>>>
>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>>> index fe62e7c775da..2c95ed6ebf3c 100755
>>> --- a/t/t3200-branch.sh
>>> +++ b/t/t3200-branch.sh
>>> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, 
>>> too' '
>>> test_must_fail git config branch.s/s/dummy
>>>  '
>>>
>>> +test_expect_success 'git branch -c dumps usage' '
>>> +   test_expect_code 128 git branch -c 2>err &&
>>> +   test_i18ngrep "branch name required" err
>>> +'
>>> +
>>> +git config branch.d.dummy Hello
>>> +
>>> +test_expect_success 'git branch -c d e should work' '
>>> +   git branch -l d &&
>>> +   git reflog exists refs/heads/d &&
>>> +   git branch -c d e &&
>>> +   git reflog exists refs/heads/d &&
>>> +   git reflog exists refs/heads/e
>>> +'
>>> +
>>> +test_expect_success 'config information was copied, too' '
>>> +   test $(git config branch.e.dummy) = Hello &&
>>> +   test $(git config branch.d.dummy) = Hello
>>> +'
>>> +
>>> +git config branch.f/f.dummy Hello
>>> +
>>> +test_expect_success 'git branch -c f/f g/g should work' '
>>> +   git branch -l f/f &&
>>> +   git reflog exists refs/heads/f/f &&
>>> +   git branch -c f/f g/g &&
>>> +   git reflog exists refs/heads/f/f &&
>>> +   git reflog exists refs/heads/g/g
>>> +'
>>> +
>>> +test_expect_success 'config information was copied, too' '
>>> +   test $(git config branch.f/f.dummy) = Hello &&
>>> +   test $(git config branch.g/g.dummy) = Hello
>>> +'
>>> +
>>> +test_expect_success 'git branch -c m2 m2 should work' '
>>> +   git branch -l m2 &&
>>> +   git reflog exists refs/heads/m2 &&
>>> +   git branch -c m2 m2 &&
>>> +   git reflog exists refs/heads/m2
>>> +'
>>> +
>>> +test_expect_success 'git branch -c a a/a should fail' '
>>> +   git branch -l a &&
>>> +   git reflog exists refs/heads/a &&
>>> +   test_must_fail git branch -c a a/a
>>> +'
>>> +
>>> +test_expect_success 'git branch -c b/b b should fail' '
>>> +   git branch -l b/b &&
>>> +   test_must_fail git branch -c b/b b
>>> +'
>>> +
>>>  test_expect_success 'deleting a symref' '
>>> git branch target &&
>>> git symbolic-ref refs/heads/symref refs/heads/target &&
>>>
>>
>> This looks good to me. Comments, in no particular order:
>>
>> * There should be a test for `git branch -c `, i.e. that
>> should implicitly copy from HEAD just like `git branch -m `
>> does. However, when looking at this I can see there's actually no test
>> for one-argument `git branch -m`, i.e. if you apply this:
>>
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const
>> char *prefix)
>> } else if (rename) {
>> if (!argc)
>> die(_("branch name required"));
>> -   else if (argc == 1)
>> -   rename_branch(head, argv[0], rename > 1);
>> else if (argc == 2)
>> rename_branch(argv[0], argv[1], rename > 1);
>> else
>>
>> The only test that fails is a `git branch -M master`, i.e.
>> one-argument -M is tested for, but not -m, same codepath though, but
>> while you're at it a patch/series like this could start by adding a
>> one-arg -m test, then follow-up by copying that for the new `-c`.
>>
>
> Thanks for the suggestion. Yes, I will add one-arg test for -c. Is it
> ok to send a different patch for adding a one-arg test for existing -m
> option?

Yeah, it makes sense to just make the first patch in the series be
some cleanup / improvement of the existing tests, which the subsequent
tests for -c then make use of / copy. It could even be sent on its
own, but probably makes sense to just bundle them up. Up to you
though, in this case you won't need patch A for patch B to work, so
the that's one argument against bundling them up. Personally I'd do it
if I was hacking this just because it's more convenient to keep track
of fewer things.

>> * We should have a -C to force -c just like -M forces -m, i.e. a test
>> where one-arg `git branch -C alreadyexists` clobbers alreadyexists,
>> and `git branch -C source alreadyexists` does the same for two-arg.
>>
> Yes, I missed this. I 

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Sahil Dua]

On Mon, May 29, 2017 at 1:30 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Mon, May 29, 2017 at 12:56 AM, Sahil Dua  wrote:
>> New feature - copying a branch along with its config section.
>>
>> Aim is to have an option -c for copying a branch just like -m option for
>> renaming a branch.
>>
>> This commit adds a few basic tests for getting any suggestions/feedback
>> about expected behavior for this new feature.
>>
>> Signed-off-by: Sahil Dua 
>> ---
>>  t/t3200-branch.sh | 53 +
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index fe62e7c775da..2c95ed6ebf3c 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, 
>> too' '
>> test_must_fail git config branch.s/s/dummy
>>  '
>>
>> +test_expect_success 'git branch -c dumps usage' '
>> +   test_expect_code 128 git branch -c 2>err &&
>> +   test_i18ngrep "branch name required" err
>> +'
>> +
>> +git config branch.d.dummy Hello
>> +
>> +test_expect_success 'git branch -c d e should work' '
>> +   git branch -l d &&
>> +   git reflog exists refs/heads/d &&
>> +   git branch -c d e &&
>> +   git reflog exists refs/heads/d &&
>> +   git reflog exists refs/heads/e
>> +'
>> +
>> +test_expect_success 'config information was copied, too' '
>> +   test $(git config branch.e.dummy) = Hello &&
>> +   test $(git config branch.d.dummy) = Hello
>> +'
>> +
>> +git config branch.f/f.dummy Hello
>> +
>> +test_expect_success 'git branch -c f/f g/g should work' '
>> +   git branch -l f/f &&
>> +   git reflog exists refs/heads/f/f &&
>> +   git branch -c f/f g/g &&
>> +   git reflog exists refs/heads/f/f &&
>> +   git reflog exists refs/heads/g/g
>> +'
>> +
>> +test_expect_success 'config information was copied, too' '
>> +   test $(git config branch.f/f.dummy) = Hello &&
>> +   test $(git config branch.g/g.dummy) = Hello
>> +'
>> +
>> +test_expect_success 'git branch -c m2 m2 should work' '
>> +   git branch -l m2 &&
>> +   git reflog exists refs/heads/m2 &&
>> +   git branch -c m2 m2 &&
>> +   git reflog exists refs/heads/m2
>> +'
>> +
>> +test_expect_success 'git branch -c a a/a should fail' '
>> +   git branch -l a &&
>> +   git reflog exists refs/heads/a &&
>> +   test_must_fail git branch -c a a/a
>> +'
>> +
>> +test_expect_success 'git branch -c b/b b should fail' '
>> +   git branch -l b/b &&
>> +   test_must_fail git branch -c b/b b
>> +'
>> +
>>  test_expect_success 'deleting a symref' '
>> git branch target &&
>> git symbolic-ref refs/heads/symref refs/heads/target &&
>>
>
> This looks good to me. Comments, in no particular order:
>
> * There should be a test for `git branch -c `, i.e. that
> should implicitly copy from HEAD just like `git branch -m `
> does. However, when looking at this I can see there's actually no test
> for one-argument `git branch -m`, i.e. if you apply this:
>
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -699,8 +699,6 @@ int cmd_branch(int argc, const char **argv, const
> char *prefix)
> } else if (rename) {
> if (!argc)
> die(_("branch name required"));
> -   else if (argc == 1)
> -   rename_branch(head, argv[0], rename > 1);
> else if (argc == 2)
> rename_branch(argv[0], argv[1], rename > 1);
> else
>
> The only test that fails is a `git branch -M master`, i.e.
> one-argument -M is tested for, but not -m, same codepath though, but
> while you're at it a patch/series like this could start by adding a
> one-arg -m test, then follow-up by copying that for the new `-c`.
>

Thanks for the suggestion. Yes, I will add one-arg test for -c. Is it
ok to send a different patch for adding a one-arg test for existing -m
option?

> * We should have a -C to force -c just like -M forces -m, i.e. a test
> where one-arg `git branch -C alreadyexists` clobbers alreadyexists,
> and `git branch -C source alreadyexists` does the same for two-arg.
>
Yes, I missed this. I will add -C option too.

> * I know this is just something you're copying, but this `git branch
> -l ` use gets me every time "wait how does listing it help isn't
> that always succeeding ... damnit it's --create-reflog not --list, got
> me again" :)
>

Yes, it was confusing to me too in the beginning. I will use --create-reflog.

> Just noting it in case it confuses other reviewers who are skimming
> this. Might be worth it to just use --create-reflog for new tests
> instead of the non-obvious -l whatever the existing tests in the file
> do, or maybe I'm the only one confused by this :)
>
> * When you use `git branch -m` it adds a note to the reflog, your
> patch should have a test like the 

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt  wrote:
> Am 26.05.2017 um 05:35 schrieb Junio C Hamano:
>> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
>> on the filesystem, Windows (correctly) fails to open it but sets
>> EINVAL to errno because the pathname has characters that cannot be
>> stored in its filesystem.
>>
>> As this is an expected failure, teach is_missing_file_error() helper
>> about this case.
>>
>> This is RFC, as there may be a case where we get EINVAL from
>> open/fopen for reasons other than "the filesystem does not like this
>> pathname" that may be worth reporting to the user, and this change
>> is sweeping such an error under the rug.
>>
>> Signed-off-by: Junio C Hamano 
>> ---
>>   wrapper.c | 4 
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index f1c87ec7ea..74aa3b7803 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>>* see if the errno indicates a missing file that we can safely ignore.
>>*/
>>   static int is_missing_file_error(int errno_) {
>> +#ifdef GIT_WINDOWS_NATIVE
>> + if (errno_ == EINVAL)
>> + return 1;
>> +#endif
>>   return (errno_ == ENOENT || errno_ == ENOTDIR);
>>   }
>
> I would prefer to catch the case in the compatibility layer. Here is
> a two patch series that would replace your 12/13 and 13/13.
>
>  8< 
> From: Johannes Schindelin 
> Subject: mingw: verify that paths are not mistaken for remote nicknames
>
> This added test case simply verifies that users will not be bothered
> with bogus complaints à la
>
> warning: unable to access '.git/remotes/D:\repo': Invalid argument
>
> when fetching from a Windows path (in this case, D:\repo).
>
> [j6t: mark the new test as test_expect_failure]
>
> Signed-off-by: Johannes Schindelin 
> Signed-off-by: Johannes Sixt 
> ---
>  t/t5580-clone-push-unc.sh | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
> index b195f71ea9..fd719a209e 100755
> --- a/t/t5580-clone-push-unc.sh
> +++ b/t/t5580-clone-push-unc.sh
> @@ -1,13 +1,19 @@
>  #!/bin/sh
>
> -test_description='various UNC path tests (Windows-only)'
> +test_description='various Windows-only path tests'
>  . ./test-lib.sh
>
>  if ! test_have_prereq MINGW; then
> -   skip_all='skipping UNC path tests, requires Windows'
> +   skip_all='skipping Windows-only path tests'
> test_done
>  fi
>
> +test_expect_failure 'remote nick cannot contain backslashes' '
> +   BACKSLASHED="$(pwd | tr / )" &&
> +   git ls-remote "$BACKSLASHED" >out 2>err &&
> +   ! grep "unable to access" err
> +'

Doesn't this need test_i18ngrep?:

wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);
wrapper.c:602:  die_errno(_("unable to access '%s'"), path);

Or is it one of these, which isn't translated:

http-push.c:1531:   error("unable to access '%s': %s",
url, curl_errorstr);
remote-curl.c:319:  die("unable to access '%s': %s",
url.buf, curl_errorstr);


> +
>  UNCPATH="$(pwd)"
>  case "$UNCPATH" in
>  [A-Z]:*)
> --
> 2.13.0.55.g17b7d13330

[GSoC] Update: Week 2

[Prathamesh Chavan]

SUMMARY OF MY PROJECT:

Git submodule subcommands are currently implemented by using shell script
'git-submodule.sh'. There are several reasons why we'll prefer not to
use the shell script. My project intends to convert the subcommands into
C code, thus making them builtins. This will increase Git's portability
and hence the efficiency of working with the git-submodule commands.
Link to the complete proposal: [1]

Mentors:
Stefan Beller 
Christian Couder 

UPDATES:

As planned for the second week, I continued working on completing the porting
of submodule subcommand foreach[2][3][4] and status.[5][6] An updated version
of these was added to the mailing list as well.

For the submodule-status, I have implemented the suggestions received on the
previous patch. But for submodule-foreach, still, some issues are left to be
solved.

Apart from this, in this week, porting of submodule subcommand sync was  also
carried out. But instead of adding anymore floating patches on the  mailing
list, I have started discussing the patch with my mentors itself, so that on
the mailing list, the focus would remain with the ported submodule subcommands
status and foreach patches.

I have also taken up with the submodule subcommand summary for porting.

PLAN FOR WEEK-3 (30 May 2017 to 5 June 2017):

As suggested by my mentors, in this week, instead of adding more floating
patches to the mailing list and porting more submodule subcommand, I would
like to polish the existing patches and try to resolve the issues they
currently have, eventually aiming for getting them merged.

Also, since I have also completed porting of submodule subcommand sync, after
reviewing the patches with mentors I'll soon be posting it on the  mailing
list.

Additionally, I will also try to complete porting of submodule-subcommand
summary in this week itself.

[1]: 
https://docs.google.com/document/d/1krxVLooWl--75Pot3dazhfygR3wCUUWZWzTXtK1L-xU/
[2]: https://public-inbox.org/git/20170526151713.10974-1-pc44...@gmail.com/
[3]: https://public-inbox.org/git/20170526151713.10974-2-pc44...@gmail.com/
[4]: https://public-inbox.org/git/20170526151713.10974-3-pc44...@gmail.com/
[5]: https://public-inbox.org/git/20170521122711.22021-1-pc44...@gmail.com/
[6]: https://public-inbox.org/git/20170521122711.22021-2-pc44...@gmail.com/

[PATCH 2/2] mingw_fopen: report ENOENT for invalid file names

[Johannes Sixt]

On Windows, certain characters are prohibited in file names, most
prominently the colon. When fopen() is called with such an invalid file
name, the underlying Windows API actually reports a particular error,
but since there is no suitable errno value, this error is translated
to EINVAL. Detect the case and report ENOENT instead.

Signed-off-by: Johannes Sixt 
---
 compat/mingw.c| 2 ++
 t/t5580-clone-push-unc.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 62109cc4e6..ce6fe8f46b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -423,6 +423,8 @@ FILE *mingw_fopen (const char *filename, const char 
*otype)

return NULL;
}
file = _wfopen(wfilename, wotype);
+   if (!file && GetLastError() == ERROR_INVALID_NAME)
+   errno = ENOENT;
if (file && hide && set_hidden_flag(wfilename, 1))
warning("could not mark '%s' as hidden.", filename);
return file;
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..93ce99ba3c 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,7 +8,7 @@ if ! test_have_prereq MINGW; then
test_done
 fi

-test_expect_failure 'remote nick cannot contain backslashes' '
+test_expect_success 'remote nick cannot contain backslashes' '
BACKSLASHED="$(pwd | tr / )" &&
git ls-remote "$BACKSLASHED" >out 2>err &&
! grep "unable to access" err
--
2.13.0.55.g17b7d13330

[PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 26.05.2017 um 05:35 schrieb Junio C Hamano:
> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
> on the filesystem, Windows (correctly) fails to open it but sets
> EINVAL to errno because the pathname has characters that cannot be
> stored in its filesystem.
> 
> As this is an expected failure, teach is_missing_file_error() helper
> about this case.
> 
> This is RFC, as there may be a case where we get EINVAL from
> open/fopen for reasons other than "the filesystem does not like this
> pathname" that may be worth reporting to the user, and this change
> is sweeping such an error under the rug.
> 
> Signed-off-by: Junio C Hamano 
> ---
>   wrapper.c | 4 
>   1 file changed, 4 insertions(+)
> 
> diff --git a/wrapper.c b/wrapper.c
> index f1c87ec7ea..74aa3b7803 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>* see if the errno indicates a missing file that we can safely ignore.
>*/
>   static int is_missing_file_error(int errno_) {
> +#ifdef GIT_WINDOWS_NATIVE
> + if (errno_ == EINVAL)
> + return 1;
> +#endif
>   return (errno_ == ENOENT || errno_ == ENOTDIR);
>   }

I would prefer to catch the case in the compatibility layer. Here is
a two patch series that would replace your 12/13 and 13/13.

 8< 
From: Johannes Schindelin 
Subject: mingw: verify that paths are not mistaken for remote nicknames

This added test case simply verifies that users will not be bothered
with bogus complaints à la

warning: unable to access '.git/remotes/D:\repo': Invalid argument

when fetching from a Windows path (in this case, D:\repo).

[j6t: mark the new test as test_expect_failure]

Signed-off-by: Johannes Schindelin 
Signed-off-by: Johannes Sixt 
---
 t/t5580-clone-push-unc.sh | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b195f71ea9..fd719a209e 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -1,13 +1,19 @@
 #!/bin/sh
 
-test_description='various UNC path tests (Windows-only)'
+test_description='various Windows-only path tests'
 . ./test-lib.sh
 
 if ! test_have_prereq MINGW; then
-   skip_all='skipping UNC path tests, requires Windows'
+   skip_all='skipping Windows-only path tests'
test_done
 fi
 
+test_expect_failure 'remote nick cannot contain backslashes' '
+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   ! grep "unable to access" err
+'
+
 UNCPATH="$(pwd)"
 case "$UNCPATH" in
 [A-Z]:*)
-- 
2.13.0.55.g17b7d13330

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Sahil Dua]

Thanks, Junio for raising all these important questions.

Indeed, showing tests in order to explain my thinking about the
feature was a bad idea. I realise that I should have explained the
feature first instead of getting the tests reviewed without any
elaboration of the intentions. I will explain it now.

My definition of "copy" for this feature is "copying from A to create
B, keeping A intact". That means "copy branch A to B" should do
whatever "move branch A to B" does except it shouldn't delete A and
should keep A unchanged.

1. When a branch topic-2 is created by copying from branch topic-1,
topic-2 branch reflog should now contain the all the entries of
topic-1 branch (before copying) followed by "Copied from topic-1".
[This is debatable though, I want inputs/suggestions about this.]

2. Copying a branch should also copy the git config section for that
branch. This means if topic-2 branch is created from topic-1,
"branch.topic-2.remote" should now be same as "branch.topic-1.remote",
if set.

3. "git push" to copied branch for example - topic-2 should push a new
branch with the same name in the remote repo. That means if topic-1
was previously pushed and a new branch topic-2 is copied from topic-1,
"git push" on topic-2 branch won't push to the same branch as "git
push on topic-1 branch would.

4. "git branch -c new-branch" should copy the currently checked out
branch and create a new branch with name "new-branch".

On Mon, May 29, 2017 at 4:09 AM, Junio C Hamano  wrote:
>
> Sahil Dua  writes:
>
> > New feature - copying a branch along with its config section.
>
> That's an unusual non-sentence (without a verb) in our commit message.

Indeed terrible. I will remove it.

>
>
> > Aim is to have an option -c for copying a branch just like -m option for
> > renaming a branch.
>
> What should it copy literally, and what should it copy with
> adjustment (and adjusting what), and what should it refrain from
> copying?
>
> For example, when you create a branch topic-2 by copying from a
> branch topic-1, do you copy the reflog in such a way that topic-2's
> reflog contains all the entries of topic-1 before the copy happens,
> capped with a new (and not shared with topic-1) entry that says
> "Copied from topic-1"?  When topic-1 is set to pull from a custom
> upstream 'upstream' (i.e. not "origin")'s 'trunk' branch, by setting
> 'branch.topic-2.remote' to "upstream", i.e. the same as the value of
> 'branch.topic-1.remote' and 'branch.topic-2.merge' to "trunk",
> i.e. the same as 'branch.topic-1.merge'?  Should a "git push" while
> topic-2 is checked out push to the same remote as a "git push" would
> push to when topic-1 is checked out?  Should they update the same
> branch at the remote?  Why and/or why not? [*1*]
>
> > This commit adds a few basic tests for getting any suggestions/feedback
> > about expected behavior for this new feature.
>
> Writing tests is a good way to prepare for an implementation, which
> must be done according to the design, but that happens after the
> design is polished and judged to be a good one.  While soliciting
> comments on the design from others, tests are a bit too low level to
> express what you are aiming at.  It is a bit unhelpful to those who
> may want to help to figure out answers to questions like the ones in
> the previos paragraph (the one that begins with "For example,...")
> by telling them to "read my intention from these tests", and when
> you want as wide an input as possible, that is counterproductive for
> your objective ;-).
>
> > Signed-off-by: Sahil Dua 
> > ---
> >  t/t3200-branch.sh | 53 
> > +
> >  1 file changed, 53 insertions(+)
> >
> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> > index fe62e7c775da..2c95ed6ebf3c 100755
> > --- a/t/t3200-branch.sh
> > +++ b/t/t3200-branch.sh
> > @@ -341,6 +341,59 @@ test_expect_success 'config information was renamed, 
> > too' '
> >   test_must_fail git config branch.s/s/dummy
> >  '
> >
> > +test_expect_success 'git branch -c dumps usage' '
> > + test_expect_code 128 git branch -c 2>err &&
> > + test_i18ngrep "branch name required" err
> > +'
>
> OK.  Do we want to support a single-name format (i.e. copy from the
> current)?
>
>
>
> > +git config branch.d.dummy Hello
>
> Write your tests to do as little outside test_expct_success block as
> possible, and do a set-up close to where it matters.
>

Regarding putting code outside test_expect_success block, I wrote this
to keep tests consistent within the same file as I saw other test
doing it this way. How can I improve this?

Here, I needed to set branch config before I copy so that I can
confirm that the new "copied" branch has the same config as well. How
can I improve this and take it as close to where-it-matters as
possible?

>
> > +test_expect_success 'git branch -c d e should work' '
> > + git branch -l d &&
> > + git 

Re: [PATCH v3 4/4] stash: implement builtin stash

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 8:18 PM, Joel Teichroeb  wrote:
> Once I have all those leaks fixed, is there a way to make sure I'm not
> missing any? I tried using valgrind with leak-check enabled, but there
> are too many leaks from other git commands.

I just used:

valgrind --leak-check=full ./git-stash list

And then skimmed things that mentioned stash.c in the pager. There
might be some better way to do this (e.g. instrument the test suite to
run valgrind for all commands and summarize that somehow), but I don't
know how to do that offhand...

Re: [PATCH v3 4/4] stash: implement builtin stash

[Joel Teichroeb]

Once I have all those leaks fixed, is there a way to make sure I'm not
missing any? I tried using valgrind with leak-check enabled, but there
are too many leaks from other git commands.

Joel

Re: [Bug] setup_git_env called without repository

[Jeff King]

On Mon, May 29, 2017 at 03:01:11PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Mon, May 29, 2017 at 1:45 PM, Zero King  wrote:
> > After upgrading to Git 2.13.0, I'm seeing the following error message
> > when running `git am -h`.
> >
> >$ git am -h
> >fatal: BUG: setup_git_env called without repository
> >
> > And with Git built from the next branch:
> >
> >$ git am -h
> >BUG: environment.c:172: setup_git_env called without repository
> >Abort trap: 6
> 
> Jeff, bisects to your b1ef400eec ("setup_git_env: avoid blind
> fall-back to ".git"", 2016-10-20).

Well, yes. But that commit is just making it easier to notice existing
violations of the setup_git_env() rules. The interesting thing is where
the violation comes from. :)

In this case, the "am" builtin uses git_pathdup() and git_config() to
set up some defaults before calling parse_options(), which is where we
handle "-h". Normally that's fine, because it uses the RUN_SETUP flag to
tell the git wrapper to barf if we're not in a repository.

But when the wrapper sees that there is a single command-line argument
and that it's "-h", it skips all of the setup and runs the builtin's
cmd_am() function anyway, under the assumption that the builtin won't do
anything meaningful before noticing the "-h" and dying. This goes back
to Jonathan's 99caeed05 (Let 'git  -h' show usage without a git
dir, 2009-11-09).

I have mixed feelings on that commit. It's unquestionably more friendly
to make "git foo -h" work outside of a repository, rather than printing
"Not a git repository". But it does break the assumptions of the cmd_*
functions.

In this case it's fairly harmless. We'd fill in bogus values for the
am_state (a bogus git-dir, but also we'd quietly ignore any repo-level
config when we _are_ in a repo), but those aren't actually used before
we hit the "-h" handling. Conceivably a cmd_* function could show
defaults as part of "-h" output, but I don't know of any cases where
that is true.

I'd be much more worried about cases where the cmd function doesn't
handle "-h" at all, and just proceeds with a broken setup. That said,
it's been this way for almost 8 years. And I see that several commands
already have workarounds for dying early in this case (try grepping for
'"-h"'). So probably we should just follow suit, like:

diff --git a/builtin/am.c b/builtin/am.c
index 0f63dcab1..5ee146bfb 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2311,6 +2311,9 @@ int cmd_am(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
+   if (argc == 2 && !strcmp(argv[1], "-h"))
+   usage_with_options(usage, options);
+
git_config(git_am_config, NULL);
 
am_state_init();

I've used the same incantation there as in other commands; possibly
this should be abstracted to a simple:

  check_help_option(argc, argv, usage, options);

that each builtin could call.

What would be _really_ nice is if the usage/options pointers for each
builtin were made available to git.c, and then it could just call
usage_with_options() itself when it sees "-h". I think that would end up
with more boilerplate than it saves, though, as we'd have to add the
pointer to the definition of each builtin struct in git.c. Possibly it
could be helped with some macro trickery and naming conventions, but I
don't know if it's worth it.

The other thing to add is a test to make sure this doesn't pop up again
in another builtin.  We should be able to do:

  for i in $builtins
  do
test_expect_success "$i -h works" "
test_expect_code 129 $i -h
"
  done

We'd need a list of builtins; probably a "git help --list-builtins"
would be helpful there. I ran something similar by hand and "git am" is
the only command that has this problem (for now, anyway). Interestingly,
"git credential" doesn't parse the "-h" at all, and so it just hangs
waiting for input (and would misbehave by ignoring repo config in this
case!). It should be fixed, too.

I'll try to put together patches in the next day or so. Comments welcome
in the meantime.

-Peff

Re: [RFC/PATCH] WIP: add deprecation & experimental process/interface

[Jeff King]

On Mon, May 29, 2017 at 01:20:02PM +0200, Ævar Arnfjörð Bjarmason wrote:

> >>  * Add a new config variable `core.version`. E.g. `core.version =
> >>2.14.0` With this the user can specify that they'd like
> >>new/experimental features introduced in that version (and below),
> >>as well as immediately getting new deprecations added in that
> >>version as errors.
> >
> > We have extensions.* for this purpose (or close to this purpose). I
> 
> From reading repository-version.txt it seems unrelated to what I'd
> like to do. I.e. there you'd like to introduce a hard breakage and
> it's already documented that if you encounter some extensions.* keys
> you don't understand you *must not* proceed.
> 
> Whereas for this you'd like to e.g. turn on some experimental feature
> in 2.16, but if you're running a 2.14 git you'd like it to just ignore
> that config key it doesn't know about instead of git breaking.

Right. repostoryformatversion (and extensions) is about the on-disk
format of a repo. If I understand correctly, this is about the user
specifying their preferred behaviors. Which is totally orthogonal.

The former must be set in the repo-level .git/config, and would
generally be set by Git itself when it writes a repo using that feature
(e.g., upcoming ref backends).  But this new thing would likely be set
in the ~/.gitconfig explicitly by the user, when they want to change the
behavior profile.

For that reason, I'd suggest using a name that is a little different
from "core.version", since it's easily missed up with
"core.repositoryformatversion". But that's a minor detail.

-Peff

Re: [Bug] setup_git_env called without repository

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 1:45 PM, Zero King  wrote:
> After upgrading to Git 2.13.0, I'm seeing the following error message
> when running `git am -h`.
>
>$ git am -h
>fatal: BUG: setup_git_env called without repository
>
> And with Git built from the next branch:
>
>$ git am -h
>BUG: environment.c:172: setup_git_env called without repository
>Abort trap: 6

Jeff, bisects to your b1ef400eec ("setup_git_env: avoid blind
fall-back to ".git"", 2016-10-20).

Re: mergetool: what to do about deleting precious files?

[Philip Oakley]

From: "Junio C Hamano" 

"Philip Oakley"  writes:


So I do not think this is not limited to "new file".  Anything that
a tree-level three-way merge would resolve cleanly without having to
consult the content-level three-way merge will complete without
consulting the merge.ours.driver; per-file content-level three-way
merge driver (which is what merge= mechanism lets you
specify via the attributes mechanism) is not something you would
want to use for this kind of thing.  It is purely for resolving the
actual content-level conflicts.


That (that Git knows best) sounds just wrong.


Don't twist my words.  I never said Git knows best.


The part I was responding to was "would resolve cleanly without having to
consult the content-level three-way merge will complete without
consulting the merge.ours.driver".

It was that lack of consultation (by git) of the putative merge-driver that 
was being noted.


The general misunderstanding, as I now see it, is the (false) expectation 
that a merge-driver would do the whole merge process.


It took a bit of digging through the documentation for me to find out just 
what the merge process appears to be. I'm sure that it obvious to those who 
have worked with git from the beginning and the previous patch flow process, 
but the merge process wasn't obvious to me, and various blogs and SO Q on 
the issue suggest the same for many others.


If I now understand correctly, the merge process flow is:

* canonicalise content (eol, smudge-clean, $id, renormalise, etc)
* diff the content (internal, or GIT_EXTERNAL_DIFF)
* apply the diff
* if conflicts, only then use merge-driver/tool

Would that be a correct interpretation?





The user-level merge driver is a mechanism to affect conflict level
three-way merges.  The interface to the content level three-way
merge driver feeds three versions of blobs and the driver is
expected to give a merged result.  The interface as designed is
incapable of passing "here is the common ancestor", "our side is
missing" and "their side is this content".

So if we want a mechanism that can affect the outcome of tree-level
three-way merge, we need a _new_ mechanism.  The existing merge
drivers that are written by end users (at least the ones written
correctly to the spec, anyway) are not expecting to be called with
"in our tree, there is no blob here", and trying to piggyback on it
will break existing users.


Is an alternative to use the GIT_EXTERNAL_DIFF to create a nul diff, so no 
changes are applied (precious/sensitive file is left behind)? This would 
have no conflicts and no requirement for a merge-conflict driver.


--

Philip 

Re: [PATCH v4 00/10] The final building block for a faster rebase -i

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 12:51 PM, Johannes Schindelin
 wrote:
> Hi René,
>
> On Sat, 27 May 2017, René Scharfe wrote:
>
>> Am 26.05.2017 um 05:15 schrieb Liam Beguin:
>> > I tried to time the execution on an interactive rebase (on Linux) but
>> > I did not notice a significant change in speed.
>> > Do we have a way to measure performance / speed changes between version?
>>
>> Well, there's performance test script p3404-rebase-interactive.sh.  You
>> could run it e.g. like this:
>>
>>   $ (cd t/perf && ./run origin/master HEAD ./p3404*.sh)
>>
>> This would compare the performance of master with the current branch
>> you're on.  The results of p3404 are quite noisy for me on master,
>> though (saw 15% difference between runs without any code changes), so
>> take them with a bag of salt.
>
> Indeed. Our performance tests are simply not very meaningful.
>
> Part of it is the use of shell scripting (which defeats performance
> testing pretty well),

Don't the performance tests take long enough that the shellscripting
overhead gets lost in the noise? E.g. on Windows what do you get when
you run this in t/perf:

$ GIT_PERF_REPEAT_COUNT=3 GIT_PERF_MAKE_OPTS="-j6 NO_OPENSSL=Y
BLK_SHA1=Y CFLAGS=-O3" ./run v2.10.0 v2.12.0 v2.13.0 p3400-rebase.sh

I get split-index performance improving by 28% in 2.12 and 58% in
2.13, small error bars even with just 3 runs. This is on Linux, but my
sense of fork overhead on Windows is that it isn't so bad as to matter
here.

I'd also be interested to see what sort of results you get for my
"grep: add support for the PCRE v1 JIT API" patch which is in pu now,
assuming you have a PCRE newer than 8.32 or so.

> another part is that we have no performance testing
> experts among us, and failed to attract any, so not only is the repeat
> count ridiculously small, we also have no graphing worth speaking of (and
> therefore it is impossible to even see trends, which is a rather important
> visual way to verify sound performance testing).
>
> Frankly, I have no illusion about this getting fixed, ever.

I have a project on my TODO that I've been meaning to get to which
would address this. I'd be interested to know what people think about
the design:

* Run the perf tests in some more where the raw runtimes are saved away
* Have some way to dump a static html page from that with graphs over
time (with gnuplot svg?)
* Supply some config file to drive this, so you can e.g. run each
tests N times against your repo X for the last 10 versions of git.
* Since it's static HTML it would be trivial for anyone to share such
results, and e.g. setup running them in cron to regularly publish to
github pages.

> So yes, in the meantime we need to use those numbers with a considerable
> amount of skepticism.

...however, while the presentation could be improved, I've seen no
reason to think that the underlying numbers are suspect, or that the
perf framework needs to be rewritten as opposed to improved upon. If
you don't think so I'd like to know why.

[Bug] setup_git_env called without repository

[Zero King]

Hi,

After upgrading to Git 2.13.0, I'm seeing the following error message
when running `git am -h`.

   $ git am -h
   fatal: BUG: setup_git_env called without repository

And with Git built from the next branch:

   $ git am -h
   BUG: environment.c:172: setup_git_env called without repository
   Abort trap: 6

--
Best regards,
Zero King

Re: [WIP/RFC 00/23] repository object

[Duy Nguyen]

On Mon, May 29, 2017 at 6:23 PM, Ævar Arnfjörð Bjarmason
 wrote:
>>> That said, even if we never reached the point where we could handle all
>>> submodule requests in-process, I think sticking the repo-related global
>>> state in a struct certainly could not hurt general readability. So it's
>>> a good direction regardless of whether we take it all the way.
>>
>> I doubt we would reach the point where libgit.a can handle all
>> submodule operations in-process either. That would put libgit.a in a
>> direct competitor position with libgit2.
>
> Wouldn't that be a good thing? We already have some users (e.g.
> Microsoft) choosing not to use libgit and instead use git.git because
> the latter is generally more mature, if git.git gains more libgit
> features without harming other things it'll be more useful to more
> people.

Whether it's a good thing might depend on view point. Similar to linux
kernel, we might want some freedom to break ABI and refactor the code.
But I think it's too early  to discuss this right now. There's lots of
work (the "die()" minefield comes to mind) before we reach the point
where libgit.a may be good enough for external use.
-- 
Duy

Error with Templates: Could not find templates on cloning but on creating

[Mathias Artus]

Hi,
Today i've tried to set up a template directory. I added in the system wide 
gitconfig the following lines:

[init]
templatedir = "//OurServer/SomeDirectory/GitTemplate"

Where //Ourserver is a Network Path.
With this line i can create a new Repository and the template gets copied. But 
when i clone a repo the following error shows up and the template doesn't get 
copied:
templates not found /OurServer/SomeDirectory/GitTemplate

I Recognized that one slash was missing. Hence i added one:
[init]
templatedir = "///OurServer/SomeDirectory/GitTemplate"

Fine, cloning works after that, but creating a new repository then shows up a 
Warning:
templates not found ///OurServer/SomeDirectory/GitTemplate

Is that a known bug or is it my Failure?

I use git 2.13 on windows 7


Kind regards
Mathias Artus

Re: [PATCH v4 10/10] rebase -i: rearrange fixup/squash lines using the rebase--helper

[Johannes Schindelin]

Hi Liam,

On Thu, 25 May 2017, Liam Beguin wrote:

> Johannes Schindelin  writes:
> [...]
> > +   if (rearranged) {
> > +   struct strbuf buf = STRBUF_INIT;
> > +
> > +   for (i = 0; i < todo_list.nr; i++) {
> > +   enum todo_command command = todo_list.items[i].command;
> > +   int cur = i;
> > +
> > +   /*
> > +* Initially, all commands are 'pick's. If it is a
> > +* fixup or a squash now, we have rearranged it.
> > +*/
> > +   if (is_fixup(command))
> > +   continue;
> > +
> > +   while (cur >= 0) {
> > +   int offset = todo_list.items[cur].offset_in_buf;
> > +   int end_offset = cur + 1 < todo_list.nr ?
> > +   todo_list.items[cur + 1].offset_in_buf :
> > +   todo_list.buf.len;
> > +   char *bol = todo_list.buf.buf + offset;
> > +   char *eol = todo_list.buf.buf + end_offset;
> 
> I got a little confused with these offsets. I know it was part of a
> previous series, but maybe we could add a description to the fields
> of `struct todo_list` and `struct todo_item`.

You mean "offset_in_buf"?

Sure, I can add a comment there. I would like to keep it out of this patch
series, though, as the purpose of it is to accelerate rebase -i by moving
logic from the (slow) shell script code to the (decently fast) C code.

Ciao,
Johannes

Re: [WIP/RFC 00/23] repository object

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 12:36 PM, Duy Nguyen  wrote:
> On Tue, May 23, 2017 at 2:35 AM, Jeff King  wrote:
>> On Thu, May 18, 2017 at 04:21:11PM -0700, Brandon Williams wrote:
>>
>>> When I first started working on the git project I found it very difficult to
>>> understand parts of the code base because of the inherently global nature of
>>> our code.  It also made working on submodules very difficult.  Since we can
>>> only open up a single repository per process, you need to launch a child
>>> process in order to process a submodule.  But you also need to be able to
>>> communicate other stateful information to the children processes so that the
>>> submodules know how best to format their output or match against a
>>> pathspec...it ends up feeling like layering on hack after hack.  What I 
>>> would
>>> really like to do, is to have the ability to have a repository object so 
>>> that I
>>> can open a submodule in-process.
>>
>> We could always buy in fully to the multi-process model and just
>> implement a generic RPC protocol between the parent and submodule gits.
>> Does CORBA still exist?
>>
>> (No, I am not serious about any of that).
>
> CORBA or not, submodule IPC is a real pain. That was what I felt
> reading the super-prefix changes a few weeks ago. Some operations
> might benefit from staying in the same process, but probably not all
> (and we lose process protection, which sometimes is a good thing)
>
>>> This is still very much in a WIP state, though it does pass all tests.  What
>>> I'm hoping for here is to get a discussion started about the feasibility of 
>>> a
>>> change like this and hopefully to get the ball rolling.  Is this a 
>>> direction we
>>> want to move in?  Is it worth the pain?
>>
>> I think the really painful part is going to be all of the system calls
>> that rely on global state provided by the OS. Like, say, every
>> filesystem call that expects to find working tree files without
>> prepending the working tree path.
>>
>> That said, even if we never reached the point where we could handle all
>> submodule requests in-process, I think sticking the repo-related global
>> state in a struct certainly could not hurt general readability. So it's
>> a good direction regardless of whether we take it all the way.
>
> I doubt we would reach the point where libgit.a can handle all
> submodule operations in-process either. That would put libgit.a in a
> direct competitor position with libgit2.

Wouldn't that be a good thing? We already have some users (e.g.
Microsoft) choosing not to use libgit and instead use git.git because
the latter is generally more mature, if git.git gains more libgit
features without harming other things it'll be more useful to more
people.

Re: [PATCH v4 05/10] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Johannes Schindelin]

Hi Liam,

On Thu, 25 May 2017, Liam Beguin wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> > index 821058d452d..9444c8d6c60 100644
> > --- a/builtin/rebase--helper.c
> > +++ b/builtin/rebase--helper.c
> > @@ -24,6 +24,10 @@ int cmd_rebase__helper(int argc, const char **argv, 
> > const char *prefix)
> > ABORT),
> > OPT_CMDMODE(0, "make-script", ,
> > N_("make rebase script"), MAKE_SCRIPT),
> > +   OPT_CMDMODE(0, "shorten-sha1s", ,
> > +   N_("shorten SHA-1s in the todo list"), SHORTEN_SHA1S),
> > +   OPT_CMDMODE(0, "expand-sha1s", ,
> > +   N_("expand SHA-1s in the todo list"), EXPAND_SHA1S),
> 
> Since work is being done to convert to `struct object_id` would it
> not be best to use a more generic name instead of 'sha1'?
> maybe something like {shorten,expand}-hashs

Good point. You suggest the use of "ids" later, and I think that is an
even better name: what we try to do here is to expand/reduce the commit
*identifiers*. The fact that they are hexadecimal representations of
hashes is an implementation detail.

> > diff --git a/sequencer.c b/sequencer.c
> > index 88819a1a2a9..201d45b1677 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2437,3 +2437,60 @@ int sequencer_make_script(int keep_empty, FILE *out,
> > strbuf_release();
> > return 0;
> >  }
> > +
> > +
> > +int transform_todo_ids(int shorten_sha1s)
> > +{
> > +   const char *todo_file = rebase_path_todo();
> > +   struct todo_list todo_list = TODO_LIST_INIT;
> > +   int fd, res, i;
> > +   FILE *out;
> > +
> > +   strbuf_reset(_list.buf);
> > +   fd = open(todo_file, O_RDONLY);
> > +   if (fd < 0)
> > +   return error_errno(_("could not open '%s'"), todo_file);
> > +   if (strbuf_read(_list.buf, fd, 0) < 0) {
> > +   close(fd);
> > +   return error(_("could not read '%s'."), todo_file);
> > +   }
> > +   close(fd);
> > +
> > +   res = parse_insn_buffer(todo_list.buf.buf, _list);
> > +   if (res) {
> > +   todo_list_release(_list);
> > +   return error(_("unusable instruction sheet: '%s'"), todo_file);
> 
> As you pointed out last time, the name of the "todo script" can be a
> source of confusion. The migration to C could be a good opportunity to
> clarify this.

True. This was simply a copy-edited part, and I should have caught that.

> I don't know which is the preferred name but we could go with
> "todo list" as it is the most common across the code base.

Yep, my next iteration will use that term.

> > +   }
> > +
> > +   out = fopen(todo_file, "w");
> > +   if (!out) {
> > +   todo_list_release(_list);
> > +   return error(_("unable to open '%s' for writing"), todo_file);
> > +   }
> > +   for (i = 0; i < todo_list.nr; i++) {
> > +   struct todo_item *item = todo_list.items + i;
> > +   int bol = item->offset_in_buf;
> > +   const char *p = todo_list.buf.buf + bol;
> > +   int eol = i + 1 < todo_list.nr ?
> > +   todo_list.items[i + 1].offset_in_buf :
> > +   todo_list.buf.len;
> > +
> > +   if (item->command >= TODO_EXEC && item->command != TODO_DROP)
> > +   fwrite(p, eol - bol, 1, out);
> > +   else {
> > +   const char *sha1 = shorten_sha1s ?
> > +   short_commit_name(item->commit) :
> > +   oid_to_hex(>commit->object.oid);
> 
> We could also use 'hash' or 'ids' here instead of 'sha1'.

Absolutely!

Thank you,
Johannes

Re: [RFC/PATCH] WIP: add deprecation & experimental process/interface

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 12:23 PM, Duy Nguyen  wrote:
> On Sat, May 27, 2017 at 6:10 PM, Ævar Arnfjörð Bjarmason
>  wrote:
>> This is the WIP start of a deprecation & experimental interface to
>> git. The goal is to formalize the workflow around deprecating
>> features, or around introducing new experimental features.
>>
>> This is much more idea than code at the moment, but included is an
>> example showing how :/ might be deprecated[1] (let's not discuss /if/
>> we should do that here, this is just an example).
>>
>> The plan, subject to RFC feedback is to:
>>
>>  * Add a new config variable `core.version`. E.g. `core.version =
>>2.14.0` With this the user can specify that they'd like
>>new/experimental features introduced in that version (and below),
>>as well as immediately getting new deprecations added in that
>>version as errors.
>
> We have extensions.* for this purpose (or close to this purpose). I

>From reading repository-version.txt it seems unrelated to what I'd
like to do. I.e. there you'd like to introduce a hard breakage and
it's already documented that if you encounter some extensions.* keys
you don't understand you *must not* proceed.

Whereas for this you'd like to e.g. turn on some experimental feature
in 2.16, but if you're running a 2.14 git you'd like it to just ignore
that config key it doesn't know about instead of git breaking.

> think it's more flexible to go with extensions.* instead of a single
> "core.version". extensions.* are non-optional though (if a git binary

I'd like there to be both, and the experimental() function would
define this on the source level, i.e. the call would include a
corresponding version where it was introduced, and a config key to
toggle it individually.

The reason to have both is that you can just upgrade git and say "I'm
not concerned about backcompat here, please give me all the latest
features" without having to exhaustively hunt down the list of things
we're shipping with that version.

> does not understand it, the repo can't be accessed). So it's more
> about fundamental experiments (like sha256 transition). I'm guessing
> we can have a "soft" extensions (warn if not understand, instead of
> die), like what we have in $GIT_DIR/index.

> Deprecation via extension.* though may be unintuitive. But I think
> something along that line (e.g. deprecation.*) might work.

Re: git worktrees must exist even if locked

[Duy Nguyen]

On Thu, May 11, 2017 at 3:24 AM, taylor, david  wrote:
> The Git documentation in describing worktrees says that one reason
> why you might want to lock a worktree is to prevent it from being pruned
> if it is on a removable media that isn't currently mounted.
>
> So, my expectation was that if the worktree is inaccessible (and locked), Git
> would pretend that there is no worktree by that name.
>
> In reality, if you have such a worktree, Git gets an error.
>
>  On local systems, /home is local to a machine; home directories are 
> elsewhere.
> Home directories are NFS mounted; /home is not.
>
> . create a repository in /my/home/dir/my-repo.git with
>
> git clone --bare 
>
> . create an empty directory /home/somedir/worktree-tests
>
> . use 'git worktree add' to add /home/somedir/worktree-tests/
>   as a worktree on branch .  It gets populated with the correct
>   content.
>
> . lock it using'git worktree lock'
>
> So far, so good.  Now, go to a different computer -- one on which
> /home/somedir/worktree-tests does not exist (and therefore
> /home/somedir/worktree-tests/ does not exist).
>
> . cd /my/home/dir/my-repo.git
>
> Now, try issuing Git commands.  Many will fail.
>
>   git fetch ==> fails:
>
>   fatal: Invalid path '/home/somedir/worktree-tests': No such file or 
> directory
>
>   git status ==> fails -- same error as above
>   git help worktree ==> fails -- same error as above

FWIW I couldn't reproduce this. The fact that "git help" also fails
suggests this is triggered by some early setup code, which narrows
down the starting point to strbuf_realpath (that can print "Invalid
path", the other call in read-cache.c involves adding index entries
and can be ignored). But I fail to see how early setup code needs to
look at any worktree at all, especially when you issue command
standing from my-repo.git (i.e. bare repo setup, even current worktree
is ignored). An strace output, if possible, might help pinpoint the
problem.
-- 
Duy

Re: [PATCH v4 4/4] convert: add "status=delayed" to filter process protocol

[Lars Schneider]

> On 23 May 2017, at 10:43, Lars Schneider  wrote:
> 
> 
>> On 23 May 2017, at 07:22, Junio C Hamano  wrote:
>> 
>> Lars Schneider  writes:
>> 
> + sigchain_pop(SIGPIPE);
> +
> + if (err || errno == EPIPE) {
 
 This looks strange, at first glance.
 Do we set errno to 0 before ?
 Or is there a trick that EPIPE can only be reached,
 if it is "our" error ?
>>> 
>>> You are right and I'll fix it! 
>>> Thanks for reminding me! 
>>> Peff also noticed that some time ago:
>>> http://public-inbox.org/git/20170411200520.oivytvlzkdu7b...@sigill.intra.peff.net/
>> 
>> Ben Peart's bp/sub-process-convert-filter topic also had the same
>> EPIPE issues in its earlier incarnation, IIRC.  I haven't looked at
>> this topic for some time, but I wonder if we can share code with it.
> 
> That's right. There might be some code sharing opportunity with Ben's
> code that is already in "next":
> https://github.com/git/git/blob/next/convert.c#L660-L677
> 
> Would it be useful for you if I send v5 with the changes rebased 
> onto "next"?

Hi Junio,

sorry for bugging you again, but Ben's topic did not make it to "master"
today. Is it OK if I rebase my topic onto "next" and resend?

Thanks,
Lars

Re: [RFC/PATCH] WIP: add deprecation & experimental process/interface

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 3:09 AM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason   writes:
>
>> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
>> index 4f94fc7574..c76bbedf86 100755
>> --- a/GIT-VERSION-GEN
>> +++ b/GIT-VERSION-GEN
>> @@ -37,4 +37,5 @@ fi
>>  test "$VN" = "$VC" || {
>>   echo >&2 "GIT_VERSION = $VN"
>>   echo "GIT_VERSION = $VN" >$GVF
>> + echo "GIT_VERSION_INT = $(echo $VN | sed -e 
>> 's/^\([0-9]*\)\.\([0-9]*\)\..*/\1\2/')" >>$GVF
>>  }
>
> Unlike Perl's v1.2.3.4 notation, this forces us worry when we go
> from v2.99.0 to v2.100.0 and eventually to v3.0, no?

Yeah it's just a dirty hack to get that WIP working, although at this
rate it'll take us ~20 years to reach 3.0 if we go up to 99, and this
would purely be internal to the codebase.

I think it make sense for core.version to be e.g. 2.13, and parsed
internally to 2013, then we have room to go to 2.999 or ~200 years at
the current dev pace.

>> + } else if (1) {
>> + /*
>> +  * TODO: Instead of `if 1` we should check a
>> +  * core.version variable here.
>> +  *
>> +  * I.e. if set to core.version=2.13 the user is opting
>> +  * in to get deprecations set at dep_at right away,
>> +  * and also perhaps experimental features from a
>> +  * sister experimental() interface.
>> +  */
>
> This essentially forces us to always read _some_ configuration.
> Some commands are meant to work outside repositories, so those who
> want to affect them needs to write core.version in their global
> configuration.  Some low-level plumbing commands may want to do
> absolute minimum without configurablity.

Doesn't making sure that those codepaths just don't call
experimental() or deprecate() solve that issue? Presumably if
something is such low-level plumbing that it can't call deprecate() or
experimental() we'd just create a new incompatible command under a
different name if we'd like to change it.

Or are there some edge cases I'm missing?

> I am not saying that it is absolutely a bad design decision to force
> us to read some configuration (yet); it's just that it is something
> that we have to keep in mind and always think about the
> ramifications of.

*Nod*. It's definitely a bit of a chicken & egg problem, especially if
we ever wanted to have experimental or deprecated config-parsing
directives, but for most parts of the codebase it should be fine.

>> + die(_("Early bird deprecation error: %s"), message);
>> + }
>> +}

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Liam,

On Thu, 25 May 2017, Liam Beguin wrote:

> Johannes Schindelin  writes:
>
> > diff --git a/sequencer.c b/sequencer.c
> > index 130cc868e51..88819a1a2a9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
> > ignore_footer, unsigned flag)
> >  
> > strbuf_release();
> >  }
> > +
> > +int sequencer_make_script(int keep_empty, FILE *out,
> > +   int argc, const char **argv)
> > +{
> > +   char *format = NULL;
> > +   struct pretty_print_context pp = {0};
> > +   struct strbuf buf = STRBUF_INIT;
> > +   struct rev_info revs;
> > +   struct commit *commit;
> > +
> > +   init_revisions(, NULL);
> > +   revs.verbose_header = 1;
> > +   revs.max_parents = 1;
> > +   revs.cherry_pick = 1;
> > +   revs.limited = 1;
> > +   revs.reverse = 1;
> > +   revs.right_only = 1;
> > +   revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > +   revs.topo_order = 1;
> > +
> > +   revs.pretty_given = 1;
> > +   git_config_get_string("rebase.instructionFormat", );
> > +   if (!format || !*format) {
> > +   free(format);
> > +   format = xstrdup("%s");
> > +   }
> > +   get_commit_format(format, );
> > +   free(format);
> > +   pp.fmt = revs.commit_format;
> > +   pp.output_encoding = get_log_output_encoding();
> > +
> > +   if (setup_revisions(argc, argv, , NULL) > 1)
> > +   return error(_("make_script: unhandled options"));
> > +
> > +   if (prepare_revision_walk() < 0)
> > +   return error(_("make_script: error preparing revisions"));
> > +
> > +   while ((commit = get_revision())) {
> > +   strbuf_reset();
> > +   if (!keep_empty && is_original_commit_empty(commit))
> > +   strbuf_addf(, "%c ", comment_line_char);
> 
> I've never had to use empty commits before, but while testing this, I
> noticed that `git rebase -i --keep-empty` behaves differently if using
> the --root option instead of a branch or something like 'HEAD~10'.
> I also tested this on v2.13.0 and the behavior is the same.

FWIW the terminology "empty commit" is a pretty poor naming choice. This
is totally not your fault at all. I just wish we had a much more intuitive
name to describe a commit that does not introduce any changes to the tree.

And yes, doing this with --root is a bit of a hack. That's because --root
is a bit of a hack.

I am curious, though, as to the exact differences you experienced. I mean,
it could be buggy behavior that needs to be fixed (independently of this
patch series, of course).

Ciao,
Johannes

Re: [PATCH v4 00/10] The final building block for a faster rebase -i

[Johannes Schindelin]

Hi Liam,

On Thu, 25 May 2017, Liam Beguin wrote:

> Johannes Schindelin  writes:
> > This patch series reimplements the expensive pre- and post-processing of
> > the todo script in C.
> >
> > [...]

I see that you used git-send-email to send this. It did look a bit funny
not to have "Re: " prefixed subjects, so at first I thought you simply
re-sent my patch series. But I guess we have no convenient way to perform
patch review, therefore I don't fault you...

Thanks for reviewing!
Johannes

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Johannes Schindelin]

Hi Junio,

On Mon, 29 May 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > diff --git a/sequencer.c b/sequencer.c
> > index 130cc868e51..88819a1a2a9 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
> > ignore_footer, unsigned flag)
> >  
> > strbuf_release();
> >  }
> > +
> > +int sequencer_make_script(int keep_empty, FILE *out,
> > +   int argc, const char **argv)
> > +{
> > +   char *format = NULL;
> > +   struct pretty_print_context pp = {0};
> > +   struct strbuf buf = STRBUF_INIT;
> > +   struct rev_info revs;
> > +   struct commit *commit;
> > +
> > +   init_revisions(, NULL);
> > +   revs.verbose_header = 1;
> > +   revs.max_parents = 1;
> > +   revs.cherry_pick = 1;
> > +   revs.limited = 1;
> > +   revs.reverse = 1;
> > +   revs.right_only = 1;
> > +   revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> > +   revs.topo_order = 1;
> 
> cf. 
> 
> This is still futzing below the API implementation detail
> unnecessarily.

You still ask me to pass options in plain text that has to be parsed at
run-time, rather than compile-time-verifiable flags.

I am quite puzzled that you keep asking me to make my code sloppy.

Ciao,
Dscho

Re: [PATCH v4 00/10] The final building block for a faster rebase -i

[Johannes Schindelin]

Hi René,

On Sat, 27 May 2017, René Scharfe wrote:

> Am 26.05.2017 um 05:15 schrieb Liam Beguin:
> > I tried to time the execution on an interactive rebase (on Linux) but
> > I did not notice a significant change in speed.
> > Do we have a way to measure performance / speed changes between version?
> 
> Well, there's performance test script p3404-rebase-interactive.sh.  You
> could run it e.g. like this:
> 
>   $ (cd t/perf && ./run origin/master HEAD ./p3404*.sh)
> 
> This would compare the performance of master with the current branch
> you're on.  The results of p3404 are quite noisy for me on master,
> though (saw 15% difference between runs without any code changes), so
> take them with a bag of salt.

Indeed. Our performance tests are simply not very meaningful.

Part of it is the use of shell scripting (which defeats performance
testing pretty well), another part is that we have no performance testing
experts among us, and failed to attract any, so not only is the repeat
count ridiculously small, we also have no graphing worth speaking of (and
therefore it is impossible to even see trends, which is a rather important
visual way to verify sound performance testing).

Frankly, I have no illusion about this getting fixed, ever.

So yes, in the meantime we need to use those numbers with a considerable
amount of skepticism.

Ciao,
Dscho

Re: [WIP/RFC 00/23] repository object

[Duy Nguyen]

On Tue, May 23, 2017 at 2:35 AM, Jeff King  wrote:
> On Thu, May 18, 2017 at 04:21:11PM -0700, Brandon Williams wrote:
>
>> When I first started working on the git project I found it very difficult to
>> understand parts of the code base because of the inherently global nature of
>> our code.  It also made working on submodules very difficult.  Since we can
>> only open up a single repository per process, you need to launch a child
>> process in order to process a submodule.  But you also need to be able to
>> communicate other stateful information to the children processes so that the
>> submodules know how best to format their output or match against a
>> pathspec...it ends up feeling like layering on hack after hack.  What I would
>> really like to do, is to have the ability to have a repository object so 
>> that I
>> can open a submodule in-process.
>
> We could always buy in fully to the multi-process model and just
> implement a generic RPC protocol between the parent and submodule gits.
> Does CORBA still exist?
>
> (No, I am not serious about any of that).

CORBA or not, submodule IPC is a real pain. That was what I felt
reading the super-prefix changes a few weeks ago. Some operations
might benefit from staying in the same process, but probably not all
(and we lose process protection, which sometimes is a good thing)

>> This is still very much in a WIP state, though it does pass all tests.  What
>> I'm hoping for here is to get a discussion started about the feasibility of a
>> change like this and hopefully to get the ball rolling.  Is this a direction 
>> we
>> want to move in?  Is it worth the pain?
>
> I think the really painful part is going to be all of the system calls
> that rely on global state provided by the OS. Like, say, every
> filesystem call that expects to find working tree files without
> prepending the working tree path.
>
> That said, even if we never reached the point where we could handle all
> submodule requests in-process, I think sticking the repo-related global
> state in a struct certainly could not hurt general readability. So it's
> a good direction regardless of whether we take it all the way.

I doubt we would reach the point where libgit.a can handle all
submodule operations in-process either. That would put libgit.a in a
direct competitor position with libgit2. I do hope though that having
clearer/modular data structure will improve readability, not hurt it
(e.g. you see the data model and could largely guess how the code
interacts before digging deep in).
-- 
Duy

Re: [RFC/PATCH] WIP: add deprecation & experimental process/interface

[Duy Nguyen]

On Sat, May 27, 2017 at 6:10 PM, Ævar Arnfjörð Bjarmason
 wrote:
> This is the WIP start of a deprecation & experimental interface to
> git. The goal is to formalize the workflow around deprecating
> features, or around introducing new experimental features.
>
> This is much more idea than code at the moment, but included is an
> example showing how :/ might be deprecated[1] (let's not discuss /if/
> we should do that here, this is just an example).
>
> The plan, subject to RFC feedback is to:
>
>  * Add a new config variable `core.version`. E.g. `core.version =
>2.14.0` With this the user can specify that they'd like
>new/experimental features introduced in that version (and below),
>as well as immediately getting new deprecations added in that
>version as errors.

We have extensions.* for this purpose (or close to this purpose). I
think it's more flexible to go with extensions.* instead of a single
"core.version". extensions.* are non-optional though (if a git binary
does not understand it, the repo can't be accessed). So it's more
about fundamental experiments (like sha256 transition). I'm guessing
we can have a "soft" extensions (warn if not understand, instead of
die), like what we have in $GIT_DIR/index.

Deprecation via extension.* though may be unintuitive. But I think
something along that line (e.g. deprecation.*) might work.
-- 
Duy

Re: git worktrees must exist even if locked

[Duy Nguyen]

On Tue, May 16, 2017 at 5:43 AM, Junio C Hamano  wrote:
> "taylor, david"  writes:
>
>> The original report was against Git v2.12.2.  I have since tried v2.12.3, 
>> v2.13.0,
>> and the next branch.  All exhibit the same symptoms.
>>
>> Even if you ignore the original scenario for creating the problem, if I do a 
>> 'rm -rf' or 'mv'
>> of a tree that contains within it worktrees, that should not break the use 
>> of Git with
>> worktrees that live elsewhere nor commands that don't require a repository.
>
> Duy, any ideas?

We are supposed to tolerate missing worktrees if locked. I'm guessing
that lots of changes in get_worktrees() lately may perhaps forget
about this and be too strict on locked worktrees.
-- 
Duy

_3gрaвcтвуйтe! Bac интeрecуют kлиeнтckиe бaзы gaнных?

[fyod.marcke...@yandex.ru]

_3qpавсmвyйme! Bас инmepeсyюm kлиeнmсkиe 6азы qанныx?

Re: ~Setting Up Charity Foundation !.

[.Sarah Edward J]

`._..

Glad to write to you this message,

I seek for your kind help in setting up a charitable organization to
help the less privileged  people and also the elderly people under
your care,

I was diagnosed of breast cancer and the doctors told me that i may
not live long due to the bad stage of my cancer because of this i
decided to use my late husband wealth to help the poor,

I want to use my late husband wealth of $3,000,000.00 to set up a
charity foundation to help the needy and the less privileged ones in
your country under your care, Can you help to build this project in
your country?

All i need from you is your sincerity to use this funds to do this
project as i desired to use it for less privileged ones and orphanage
homes,

We are the one who will make the world a better place when we help the
needy sincerely,

Reply and let me know your opinion in doing this noble work,

Remain blessed,

Mrs Sarah Edwad

FORMAT_PATCH_NAME_MAX increase

[Laszlo Ersek]

Hi,

would it be possible to

- increase the FORMAT_PATCH_NAME_MAX macro from 64 to, say, 128?

- Or else to introduce a new git-config knob for it?

I have a small review-helper / interdiff script that matches patches
from adjacent versions of a series against each other, based on subject
line. (Using the formatted file name with the patch number stripped.)
The project in question uses long common prefixes in subjects, and the
current limit of 64 does not always ensure unicity (again, with the
number stripped).

Thank you,
Laszlo

Re: [PATCH v4 00/10] The final building block for a faster rebase -i

[Junio C Hamano]

Johannes Schindelin  writes:

> This patch series reimplements the expensive pre- and post-processing of
> the todo script in C.
>
> And it concludes the work I did to accelerate rebase -i.
>

I took another look at the series (as "What's cooking" report was
listing this in the "Needs review" state).  Except for an inssue
that I already pointed out in an earlier review, I didn't spot
anything glaringly wrong in this v4 round.

Thanks.

Odp: Re: Dzial Kredytowy

[BSN Capital]

Witam,

Czy potrzebujesz zatwierdzony biznes i prywatnego kredytu / Finansowanie w 
maksymalnej wysokosci 3% w skali roku? Skontaktuj sie z nami po wiecej 
szczególów w razie zainteresowania.

Dziekuje.
BSN Capital Partners Ltd (London)
Zarzadzanie.

--
Angielska wersja
--

Do you need an approved business and personal Loan/Funding at a maximum rate of 
3% per Annum? Contact us today for more details if interested.

Thank you.
BSN Capital Partners Ltd (London)
Management.

Re: What's cooking in git.git (May 2017, #08; Mon, 29)

[Samuel Lijin]

On Mon, May 29, 2017 at 2:23 AM, Junio C Hamano  wrote:
> * sl/clean-d-ignored-fix (2017-05-24) 6 commits
>   (merged to 'next' on 2017-05-29 at 837c255ae8)
>  + clean: teach clean -d to preserve ignored paths
>  + dir: expose cmp_name() and check_contains()
>  + dir: hide untracked contents of untracked dirs
>  + dir: recurse into untracked dirs for ignored files
>  + t7061: status --ignored should search untracked dirs
>  + t7300: clean -d should skip dirs with ignored files
>
>  "git clean -d" used to clean directories that has ignored files,
>  even though the command should not lose ignored ones without "-x".
>  This has been corrected.
>
>  Will merge to 'master'.

I noticed the merge commit message (and branch name) don't make
mention of the changes to git status --ignored. I forgot to mention it
when the last cooking email when out, but was that intended?

I think at the very least, if the branch name isn't updated to reflect
that status --ignored is being changed, something like the following
should be appended to the merge commit message:

"git status --ignored" previously did not list ignored files in
untracked directories without -uall, contrary to the documented
behavior of the --ignored flag (that all ignored files would be
listed). This has also been corrected.

Re: [PATCH] wildmatch test: remove redundant duplicate test

[Ævar Arnfjörð Bjarmason]

On Mon, May 29, 2017 at 2:54 AM, Junio C Hamano  wrote:
> Thanks.  Did you run "sort | uniq -c" on it or something ;-)?

I've been writing a new backend for wildmatch(). Was wondering what
the difference in these two failing tests was, turns out there was
none.

> Will apply.

Thanks.

Re: [PATCH v3 4/4] stash: implement builtin stash

[Junio C Hamano]

Joel Teichroeb  writes:

> +int untracked_files(struct strbuf *out, int include_untracked,
> + const char **argv)

Does this need to be public?  

For a caller that wants to learn if there is _any_ untracked file,
having a strbuf that holds all output is overkill.  For a caller
that wants to learn what are the untracked paths, having a strbuf
that it needs to parse is not all that helpful.  Only for a caller
that does an unusual thing, namely, just grab the output and throw
it at another command as input, without checking what the output was
itself at all, would benefit.

So the interface to this function doesn't look like a very good one
if this wants to be a public helper.  Perhaps mark it as "static"?

> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> + cp.git_cmd = 1;
> + argv_array_push(, "ls-files");
> + argv_array_push(, "-o");
> + argv_array_push(, "-z");
> + if (include_untracked != 2)
> + argv_array_push(, "--exclude-standard");

This magic number "2" will be hard to grok by future readers.

> + argv_array_push(, "--");
> + if (argv)
> + argv_array_pushv(, argv);
> + return pipe_command(, NULL, 0, out, 0, NULL, 0);
> +}
> +
> +static int check_no_changes(const char *prefix, int include_untracked,
> + const char **argv)
> +{
> + struct argv_array args1;
> + struct argv_array args2;
> + struct strbuf out = STRBUF_INIT;
> +
> + argv_array_init();
> + argv_array_push(, "diff-index");
> + argv_array_push(, "--quiet");
> + argv_array_push(, "--cached");
> + argv_array_push(, "HEAD");
> + argv_array_push(, "--ignore-submodules");
> + argv_array_push(, "--");
> + if (argv)
> + argv_array_pushv(, argv);
> + argv_array_init();
> + argv_array_push(, "diff-files");
> + argv_array_push(, "--quiet");
> + argv_array_push(, "--ignore-submodules");
> + argv_array_push(, "--");
> + if (argv)
> + argv_array_pushv(, argv);

> + if (include_untracked)
> + untracked_files(, include_untracked, argv);
> + return cmd_diff_index(args1.argc, args1.argv, prefix) == 0 &&
> + cmd_diff_files(args2.argc, args2.argv, prefix) == 0 &&
> + (!include_untracked || out.len == 0);
> +}

Possible leak of out.buf here.

> +
> +static int get_stash_info(struct stash_info *info, const char *commit)
> +{

Judging from the callers of get_stash_info(), nobody passes a
"commit" as parameter; as far as they are concerned, they are
dealing with the name of one item in the stash (stash-id?).  They do
not care the fact that the item happens to be implemented as a
commit object.

Perhaps rename "commit" (parameter) to "stash_id" or something.

> + struct strbuf w_commit_rev = STRBUF_INIT;
> + struct strbuf b_commit_rev = STRBUF_INIT;
> + struct strbuf i_commit_rev = STRBUF_INIT;
> + struct strbuf u_commit_rev = STRBUF_INIT;
> + struct strbuf w_tree_rev = STRBUF_INIT;
> + struct strbuf b_tree_rev = STRBUF_INIT;
> + struct strbuf i_tree_rev = STRBUF_INIT;
> + struct strbuf u_tree_rev = STRBUF_INIT;
> + struct strbuf commit_buf = STRBUF_INIT;
> + struct strbuf symbolic = STRBUF_INIT;
> + struct strbuf out = STRBUF_INIT;
> + struct object_context unused;
> + char *str;
> + int ret;
> + const char *REV = commit;

Variables and field names that are all caps become misleading.
Avoid them.

> + struct child_process cp = CHILD_PROCESS_INIT;
> + info->is_stash_ref = 0;
> +
> + if (commit == NULL) {
> + strbuf_addf(_buf, "%s@{0}", ref_stash);
> + REV = commit_buf.buf;
> + } else if (strlen(commit) < 3) {
> + strbuf_addf(_buf, "%s@{%s}", ref_stash, commit);
> + REV = commit_buf.buf;
> + }
> + info->REV = REV;
> +
> + strbuf_addf(_commit_rev, "%s", REV);
> + strbuf_addf(_commit_rev, "%s^1", REV);
> + strbuf_addf(_commit_rev, "%s^2", REV);
> + strbuf_addf(_commit_rev, "%s^3", REV);
> + strbuf_addf(_tree_rev, "%s:", REV);
> + strbuf_addf(_tree_rev, "%s^1:", REV);
> + strbuf_addf(_tree_rev, "%s^2:", REV);
> + strbuf_addf(_tree_rev, "%s^3:", REV);
> +
> +
> + ret = (
> + get_sha1_with_context(w_commit_rev.buf, 0, info->w_commit.hash, 
> ) == 0 &&
> + get_sha1_with_context(b_commit_rev.buf, 0, info->b_commit.hash, 
> ) == 0 &&
> + get_sha1_with_context(i_commit_rev.buf, 0, info->i_commit.hash, 
> ) == 0 &&
> + get_sha1_with_context(w_tree_rev.buf, 0, info->w_tree.hash, 
> ) == 0 &&
> + get_sha1_with_context(b_tree_rev.buf, 0, info->b_tree.hash, 
> ) == 0 &&
> + get_sha1_with_context(i_tree_rev.buf, 0, info->i_tree.hash, 
> ) == 0);

Hmph.  What's the reason to use get_sha1_with_context() if you
declare the context is unused?  Wouldn't plain-vanilla get_sha1()
work better?

Re: [PATCH v3 3/4] close the index lock when not writing the new index

[Junio C Hamano]

Joel Teichroeb  writes:

> Signed-off-by: Joel Teichroeb 
> ---

The title says what the patch does; it does not explain why it is a
good change.  Lockfiles will be closed automatically when we exit
anyway, so one can argue that the current code is good.

If you are planning to add more code to these "missing" else clauses
in future steps in this series, this change makes sort-of sense.  If
that is the case, please say that in your log message.

>  builtin/add.c | 3 ++-
>  builtin/mv.c  | 8 +---
>  builtin/rm.c  | 3 ++-
>  merge-recursive.c | 8 +---
>  4 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 9f53f020d0..6b04eb2c71 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -461,7 +461,8 @@ int cmd_add(int argc, const char **argv, const char 
> *prefix)
>   if (active_cache_changed) {
>   if (write_locked_index(_index, _file, COMMIT_LOCK))
>   die(_("Unable to write new index file"));
> - }
> + } else
> + rollback_lock_file(_file);

I think Ævar already pointed out the style issue, i.e.

if (a-c-c) {
if (w-l-i())
die(...);
} else {
rollback_lock_file(_file);
}

The same for all other hunks in this patch.

Thanks.

Re: [PATCH v3 2/4] stash: add test for stashing in a detached state

[Junio C Hamano]

Joel Teichroeb  writes:

> Signed-off-by: Joel Teichroeb 
> ---
>  t/t3903-stash.sh | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aaae221304..b86851ef46 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -808,6 +808,17 @@ test_expect_success 'create with multiple arguments for 
> the message' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'create in a detached state' '
> + test_when_finished "git checkout master" &&
> + git checkout HEAD~1 &&
> + >foo &&
> + git add foo &&
> + STASH_ID=$(git stash create) &&
> + echo "WIP on (no branch): 47d5e0e initial" >expect &&

Is it easy to generate this (especially 47d5e0e) at runtime?  I know
that earlier tests in this script are brittle and will break when
object names change by using test vectors that hardcode object names
a bit too much, but if we can avoid making it worse, let's try to do
so for future developers who will have to do the work of adjusting
the tests for possible changes to the object name hashing algorithm.

> + git show --pretty=%s -s ${STASH_ID} >actual &&
> + test_cmp expect actual
> +'
> +
>  test_expect_success 'stash --  stashes and restores the file' '
>   >foo &&
>   >bar &&

Re: [PATCH v3 1/4] stash: add test for stash create with no files

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> +   git stash create > actual &&
>> +   test $(cat actual | wc -l) -eq 0
> ...
> Although I wonder in this case whether you don't actually mean:
>
> [...]>actual &&
> ! test -s actual
>
> I.e. isn't the test that there should be no output, not that there
> shouldn't be a \n there?

Good suggestion.  There is "test_must_be_empty" you may want to use.

Also, we do not leave SP between the redirection operator and the
filename.  Your example ">actual" is good; the original goes against
our style.

Thanks.

What's cooking in git.git (May 2017, #08; Mon, 29)

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

A bit more topics are now in 'master'.  One unfortunate thing is
that the SHA1 breakage in 2.13 for big-endian platforms were lost in
the noise with excitement felt by some subset of contributors with
the possible use of submodules.  The first step in the series is
neutral to the excitement, and should be fast-tracked down to
'maint' soonish.

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

* bc/object-id (2017-05-08) 53 commits
  (merged to 'next' on 2017-05-20 at e7372733fb)
 + object: convert parse_object* to take struct object_id
 + tree: convert parse_tree_indirect to struct object_id
 + sequencer: convert do_recursive_merge to struct object_id
 + diff-lib: convert do_diff_cache to struct object_id
 + builtin/ls-tree: convert to struct object_id
 + merge: convert checkout_fast_forward to struct object_id
 + sequencer: convert fast_forward_to to struct object_id
 + builtin/ls-files: convert overlay_tree_on_cache to object_id
 + builtin/read-tree: convert to struct object_id
 + sha1_name: convert internals of peel_onion to object_id
 + upload-pack: convert remaining parse_object callers to object_id
 + revision: convert remaining parse_object callers to object_id
 + revision: rename add_pending_sha1 to add_pending_oid
 + http-push: convert process_ls_object and descendants to object_id
 + refs/files-backend: convert many internals to struct object_id
 + refs: convert struct ref_update to use struct object_id
 + ref-filter: convert some static functions to struct object_id
 + Convert struct ref_array_item to struct object_id
 + Convert the verify_pack callback to struct object_id
 + Convert lookup_tag to struct object_id
 + log-tree: convert to struct object_id
 + Convert lookup_tree to struct object_id
 + builtin/reflog: convert tree_is_complete to take struct object_id
 + tree: convert read_tree_1 to use struct object_id internally
 + Convert lookup_blob to struct object_id
 + Convert remaining callers of lookup_blob to object_id
 + builtin/unpack-objects: convert to struct object_id
 + pack: convert struct pack_idx_entry to struct object_id
 + Convert lookup_commit* to struct object_id
 + Convert remaining callers of lookup_commit_reference* to object_id
 + builtin/tag: convert to struct object_id
 + sequencer: convert some functions to struct object_id
 + shallow: convert shallow registration functions to object_id
 + revision: convert prepare_show_merge to struct object_id
 + notes-utils: convert internals to struct object_id
 + http-push: convert some static functions to struct object_id
 + tag: convert parse_tag_buffer to struct object_id
 + builtin/verify-commit: convert to struct object_id
 + reflog_expire: convert to struct object_id
 + parse-options-cb: convert to struct object_id
 + notes-cache: convert to struct object_id
 + submodule: convert merge_submodule to use struct object_id
 + fast-import: convert to struct object_id
 + fast-import: convert internal structs to struct object_id
 + builtin/rev-parse: convert to struct object_id
 + builtin/blame: convert static function to struct object_id
 + branch: convert to struct object_id
 + bundle: convert to struct object_id
 + builtin/prune: convert to struct object_id
 + builtin/name-rev: convert to struct object_id
 + Convert struct cache_tree to use struct object_id
 + Clean up outstanding object_id transforms.
 + fetch-pack: convert to struct object_id
 (this branch is used by mh/packed-ref-store-prep.)

 Conversion from uchar[20] to struct object_id continues.


* bm/interpret-trailers-cut-line-is-eom (2017-05-18) 1 commit
  (merged to 'next' on 2017-05-23 at 3e5ff08517)
 + interpret-trailers: honor the cut line

 "git interpret-trailers", when used as GIT_EDITOR for "git commit
 -v", looked for and appended to a trailer block at the very end,
 i.e. at the end of the "diff" output.  The command has been
 corrected to pay attention to the cut-mark line "commit -v" adds to
 the buffer---the real trailer block should appear just before it.


* bw/dir-c-stops-relying-on-the-index (2017-05-06) 14 commits
  (merged to 'next' on 2017-05-20 at 1f1b764ec8)
 + dir: convert fill_directory to take an index
 + dir: convert read_directory to take an index
 + dir: convert read_directory_recursive to take an index
 + dir: convert open_cached_dir to take an index
 + dir: convert is_excluded to take an index
 + dir: convert prep_exclude to take an index
 + dir: convert add_excludes to take an index
 + dir: convert is_excluded_from_list to take an index
 + dir: convert last_exclude_matching_from_list to 

Re: [PATCH v4 02/10] rebase -i: generate the script via rebase--helper

[Junio C Hamano]

Johannes Schindelin  writes:

> diff --git a/sequencer.c b/sequencer.c
> index 130cc868e51..88819a1a2a9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2388,3 +2388,52 @@ void append_signoff(struct strbuf *msgbuf, int 
> ignore_footer, unsigned flag)
>  
>   strbuf_release();
>  }
> +
> +int sequencer_make_script(int keep_empty, FILE *out,
> + int argc, const char **argv)
> +{
> + char *format = NULL;
> + struct pretty_print_context pp = {0};
> + struct strbuf buf = STRBUF_INIT;
> + struct rev_info revs;
> + struct commit *commit;
> +
> + init_revisions(, NULL);
> + revs.verbose_header = 1;
> + revs.max_parents = 1;
> + revs.cherry_pick = 1;
> + revs.limited = 1;
> + revs.reverse = 1;
> + revs.right_only = 1;
> + revs.sort_order = REV_SORT_IN_GRAPH_ORDER;
> + revs.topo_order = 1;

cf. 

This is still futzing below the API implementation detail
unnecessarily.