Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 02:26:02AM -0500, Jeff King wrote:

> You'll notice that it actually calls wait() on the pager. That's due to
> a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
> IIRC was addressing a very similar problem. We want to stop feeding the
> pager when we get a signal, but we don't want the main process to
> actually exit, or the pager loses the controlling terminal.
> 
> In our new scenario we have an extra process, though. The git-log child
> will wait on the pager, but the parent process can't. It doesn't know
> about it. I think that it in turn needs to wait on the child when it
> dies, and then the whole chain will stand still until the pager exits.

And here's a patch to do that. It seems to work.

I'll sleep on it and then write up a commit message tomorrow if it still
makes sense.

diff --git a/run-command.c b/run-command.c
index ca905a9e80..db47c429b7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
 
 static void cleanup_children(int sig, int in_signal)
 {
+   struct child_to_clean *children_to_wait_for = NULL;
+
while (children_to_clean) {
struct child_to_clean *p = children_to_clean;
children_to_clean = p->next;
@@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
}
 
kill(p->pid, sig);
+   p->next = children_to_wait_for;
+   children_to_wait_for = p;
+   }
+
+   while (children_to_wait_for) {
+   struct child_to_clean *p = children_to_wait_for;
+   children_to_wait_for = p->next;
+
+   while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
+   ; /* spin waiting for process exit or error */
+
if (!in_signal)
free(p);
}

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 01:47:52AM -0500, Jeff King wrote:

> > > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > > Ctrl-c. The expected behavior is that nothing happens. The actual 
> > > behavior is
> > > that the pager exits.
> > 
> > That's weird. I can't reproduce at all here. But I also can't think of a
> > thing that Git could do that would impact the behavior. For example:
> 
> I take it back. I could not reproduce under my normal config, which sets
> the pager to "diff-highlight | less". But if I drop that config, I can
> reproduce easily. And bisect it to that same commit, 86d26f240f
> (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
> 2015-12-20).

I made a little progress with strace. Prior to 86d26f240f, we would run
the "log" builtin directly inside the same executable. After it, we exec
a separate process, and the process tree looks like:

  |   `-bash,10123
  |   `-git,10125 l
  |   `-git-log,10127
  |   `-less,10128

Now if I strace all of those, I see (I've reordered and snipped a bit
for clarity):

  $ strace -p 10125 -p 10127 -p 10128
  strace: Process 10125 attached
  strace: Process 10127 attached
  strace: Process 10128 attached
  [pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 

  [pid 10128] read(5,  
  [pid 10125] wait4(10127,  

The main git process is waiting for the child to finish, the child is
blocked writing to the pager, and the pager is waiting for input from
the terminal (fd 5).

So I hit ^C:

  [pid 10128] <... read resumed> 0x7ffd39153b57, 1) = ? ERESTARTSYS (To be 
restarted if SA_RESTART is set)
  [pid 10127] <... write resumed> )   = ? ERESTARTSYS (To be restarted if 
SA_RESTART is set)
  [pid 10125] <... wait4 resumed> 0x7ffe88d0a560, 0, NULL) = ? ERESTARTSYS (To 
be restarted if SA_RESTART is set)
  [pid 10128] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
  [pid 10127] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---
  [pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_KERNEL} ---

Everybody gets the signal...

  [pid 10127] write(1, "\n\33[33mcommit bae73e80d48ace1faa3"..., 1481 


The blocked writer will resume its write() until it returns. This is the
same as it would get under the older code, too (after write() returns it
will handle the signal and die).

  [pid 10125] kill(10127, SIGINT 
  [pid 10125] <... kill resumed> )= 0

The parent git process tries to propagate the signal to the child.
Unnecessary in this instance, but helpful when the signal is only
delivered to the parent. This is due to 

  [pid 10128] rt_sigaction(SIGINT, {sa_handler=0x558dd1af0300, sa_mask=[INT], 
sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040},  
  [pid 10128] <... rt_sigaction resumed> {sa_handler=0x558dd1af0300, 
sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7f03fc8c2040}, 8) 
= 0
  [pid 10128] rt_sigprocmask(SIG_SETMASK, [],  
  [pid 10128] <... rt_sigprocmask resumed> NULL, 8) = 0
  [pid 10128] write(1, "\7\r\33[K:\33[K", 9 
  [pid 10128] <... write resumed> )   = 9
  [pid 10128] read(5,  

And here's the pager handling the signal, and going back to waiting for
terminal input.

  [pid 10125] rt_sigaction(SIGINT, {sa_handler=SIG_DFL, sa_mask=[INT], 
sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040},  
  [pid 10125] <... rt_sigaction resumed> {sa_handler=0x55aec373a6a0, 
sa_mask=[INT], sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fbe2b4c1040}, 8) 
= 0
  [pid 10125] rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [INT], 8) = 0
  [pid 10125] getpid( 
  [pid 10125] <... getpid resumed> )  = 10125
  [pid 10125] gettid()= 10125
  [pid 10125] tgkill(10125, 10125, SIGINT) = 0
  [pid 10125] rt_sigprocmask(SIG_SETMASK, [INT], NULL, 8) = 0
  [pid 10125] rt_sigreturn({mask=[]}) = 61
  [pid 10125] --- SIGINT {si_signo=SIGINT, si_code=SI_TKILL, si_pid=10125, 
si_uid=1000} ---
  [pid 10125] +++ killed by SIGINT +++

The parent process pops the signal handler and propagates to itself,
dying.

At this point things pause, and nothing happens. But as soon as I hit a
key, the pager dies:

  [pid 10128] <... read resumed> "\r", 1) = 1
  [pid 10128] write(1, "\r\33[K", 4)  = 4
  [pid 10128] write(1, " \"Another round of MIPS fixe"..., 50) = 50
  [pid 10128] read(5, 0x7ffd39153b57, 1)  = -1 EIO (Input/output error)
  [pid 10128] write(1, "\r\33[K\33[?1l\33>", 11) = 11
  [pid 10128] fsync(5)= -1 EINVAL (Invalid argument)
  [pid 10128] ioctl(5, TCGETS, {B38400 opost isig -icanon -echo ...}) = 0
  [pid 10128] ioctl(5, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost isig icanon 
echo ...}) = -1 EIO (Input/output error)
  [pid 10128] exit_group(1)   = ?
  [pid 10128] +++ exited with 1 +++

The key thing is the EIO we get reading from the terminal. I think
because the head of the process group died, we lost our controlling
terminal.

And then naturally the git-log process gets 

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Fri, Jan 06, 2017 at 01:40:32AM -0500, Jeff King wrote:

> On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:
> 
> > I'm experiencing an issue when using aliases for commands that open the 
> > pager.
> > When I press Ctrl-c from the pager, it exits. This does not happen when I
> > don't use an alias and did not happen before. It causes problems because
> > Ctrl-c is also used for other things, such as canceling a search that hasn't
> > completed.
> > 
> > To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> > Ctrl-c. The expected behavior is that nothing happens. The actual behavior 
> > is
> > that the pager exits.
> 
> That's weird. I can't reproduce at all here. But I also can't think of a
> thing that Git could do that would impact the behavior. For example:

I take it back. I could not reproduce under my normal config, which sets
the pager to "diff-highlight | less". But if I drop that config, I can
reproduce easily. And bisect it to that same commit, 86d26f240f
(setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE when ..,
2015-12-20).

-Peff

Re: Regression: Ctrl-c from the pager in an alias exits it

[Jeff King]

On Thu, Jan 05, 2017 at 03:25:29PM +0100, Trygve Aaberge wrote:

> I'm experiencing an issue when using aliases for commands that open the pager.
> When I press Ctrl-c from the pager, it exits. This does not happen when I
> don't use an alias and did not happen before. It causes problems because
> Ctrl-c is also used for other things, such as canceling a search that hasn't
> completed.
> 
> To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
> Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
> that the pager exits.

That's weird. I can't reproduce at all here. But I also can't think of a
thing that Git could do that would impact the behavior. For example:

  1. Git should never kill() the pager; in fact it waits for it to
 finish.

  2. Git can impact the pager environment and command line options, but
 those haven't changed anytime recently (and besides, I'm not sure
 there even is an option to convince less to die).

  3. Git can impact the set of blocked signals that the pager sees, but
 I think less would set up its own handler for SIGINT anyway.

I suppose it's possible that your shell or another program (tmux,
maybe?) catches the SIGINT and does a process-group kill. But then I
don't know why it would matter that you're using an alias. The process
tree without an alias:

  |-xterm,21861,peff
  |   `-bash,21863
  |   `-git,22376 log
  |   `-less,22377

and with:

  |-xterm,21861,peff
  |   `-bash,21863
  |   `-git,22391 l
  |   `-git-log,22393
  |   `-less,22394

are pretty similar.

Puzzling.

-Peff

Re: [PATCH] branch_get_push: do not segfault when HEAD is detached

[Jeff King]

On Thu, Jan 05, 2017 at 11:56:23PM -0500, Kyle Meyer wrote:

> Move the detached HEAD check from branch_get_push_1() to
> branch_get_push() to avoid setting branch->push_tracking_ref when
> branch is NULL.

Yep, I think this is the right fix.

> diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
> index 7214f5b33..90c639ae1 100755
> --- a/t/t1514-rev-parse-push.sh
> +++ b/t/t1514-rev-parse-push.sh
> @@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
>   resolve topic@{push} refs/remotes/origin/magic/topic
>  '
>  
> +test_expect_success 'resolving @{push} fails with a detached HEAD' '
> + git checkout HEAD^{} &&
> + test_when_finished "git checkout -" &&
> + test_must_fail git rev-parse @{push}
> +'

Looks good. Thanks.

-Peff

PS Looks like this is your first patch. Welcome. :)

[PATCH] branch_get_push: do not segfault when HEAD is detached

[Kyle Meyer]

Move the detached HEAD check from branch_get_push_1() to
branch_get_push() to avoid setting branch->push_tracking_ref when
branch is NULL.

Signed-off-by: Kyle Meyer 
---
 remote.c  | 6 +++---
 t/t1514-rev-parse-push.sh | 6 ++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index ad6c5424e..d5eaec737 100644
--- a/remote.c
+++ b/remote.c
@@ -1716,9 +1716,6 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
 {
struct remote *remote;
 
-   if (!branch)
-   return error_buf(err, _("HEAD does not point to a branch"));
-
remote = remote_get(pushremote_for_branch(branch, NULL));
if (!remote)
return error_buf(err,
@@ -1778,6 +1775,9 @@ static const char *branch_get_push_1(struct branch 
*branch, struct strbuf *err)
 
 const char *branch_get_push(struct branch *branch, struct strbuf *err)
 {
+   if (!branch)
+   return error_buf(err, _("HEAD does not point to a branch"));
+
if (!branch->push_tracking_ref)
branch->push_tracking_ref = branch_get_push_1(branch, err);
return branch->push_tracking_ref;
diff --git a/t/t1514-rev-parse-push.sh b/t/t1514-rev-parse-push.sh
index 7214f5b33..90c639ae1 100755
--- a/t/t1514-rev-parse-push.sh
+++ b/t/t1514-rev-parse-push.sh
@@ -60,4 +60,10 @@ test_expect_success '@{push} with push refspecs' '
resolve topic@{push} refs/remotes/origin/magic/topic
 '
 
+test_expect_success 'resolving @{push} fails with a detached HEAD' '
+   git checkout HEAD^{} &&
+   test_when_finished "git checkout -" &&
+   test_must_fail git rev-parse @{push}
+'
+
 test_done
-- 
2.11.0

[PATCH 3/3] blame: output porcelain "previous" header for each file

[Jeff King]

It's possible for content currently found in one file to
have originated in two separate files, each of which may
have been modified in some single older commit.  The
--porcelain output generates an incorrect "previous" header
in this case, whereas --line-porcelain gets it right.  The
problem is that the porcelain output tries to omit repeated
details of commits, and treats "previous" as a property of
the commit, when it is really a property of the blamed block
of lines.

Let's look at an example. In a case like this, you might see
this output from --line-porcelain:

  SOME_SHA1 1 1 1
  author ...
  committer ...
  previous SOME_SHA1^ file_one
  filename file_one
  ...some line content...
  SOME_SHA1 2 1 1
  author ...
  committer ...
  previous SOME_SHA1^ file_two
  filename file_two
  ...some different content

The "filename" fields tell us that the two lines are from
two different files. But notice that the filename also
appears in the "previous" field, which tells us where to
start a re-blame. The second content line never appeared in
file_one at all, so we would obviously need to re-blame from
file_two (or possibly even some other file, if had just been
renamed to file_two in SOME_SHA1).

So far so good. Now here's what --porcelain looks like:

  SOME_SHA1 1 1 1
  author ...
  committer ...
  previous SOME_SHA1^ file_one
  filename file_one
  ...some line content...
  SOME_SHA1 2 1 1
  filename file_two
  ...some different content

We've dropped the author and committer fields from the
second line, as they would just be repeats.  But we can't
omit "filename", because it depends on the actual block of
blamed lines, not just the commit. This is handled by
emit_porcelain_details(), which will show the filename
either if it is the first mention of the commit _or_ if the
commit has multiple paths in it.

But we don't give "previous" the same handling. It's written
inside emit_one_suspect_detail(), which bails early if we've
already seen that commit. And so the output above is wrong;
a reader would assume that the correct place to re-blame
line two is from file_one, but that's obviously nonsense.

Let's treat "previous" the same as "filename", and show it
fresh whenever we know we are in a confusing case like this.

Signed-off-by: Jeff King 
---
I'm assuming that the parent sha1 for a "previous" field will always be
the same for a given commit. So we really only need to care about
reprinting when we know there are multiple paths, as this patch does
(i.e., treat it exactly the same as "filename"). If I'm wrong, then
there's probably another corner case that this doesn't handle. I
couldn't think of a way to trigger such a setup, though.

 builtin/blame.c |  23 +
 t/t8011-blame-split-file.sh | 117 
 2 files changed, 131 insertions(+), 9 deletions(-)
 create mode 100755 t/t8011-blame-split-file.sh

diff --git a/builtin/blame.c b/builtin/blame.c
index c6170fed81..3aae19a0f9 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1700,13 +1700,23 @@ static void get_commit_info(struct commit *commit,
 }
 
 /*
+ * Write out any suspect information which depends on the path. This must be
+ * handled separately from emit_one_suspect_detail(), because a given commit
+ * may have changes in multiple paths. So this needs to appear each time
+ * we mention a new group.
+ *
  * To allow LF and other nonportable characters in pathnames,
  * they are c-style quoted as needed.
  */
-static void write_filename_info(const char *path)
+static void write_filename_info(struct origin *suspect)
 {
+   if (suspect->previous) {
+   struct origin *prev = suspect->previous;
+   printf("previous %s ", oid_to_hex(>commit->object.oid));
+   write_name_quoted(prev->path, stdout, '\n');
+   }
printf("filename ");
-   write_name_quoted(path, stdout, '\n');
+   write_name_quoted(suspect->path, stdout, '\n');
 }
 
 /*
@@ -1735,11 +1745,6 @@ static int emit_one_suspect_detail(struct origin 
*suspect, int repeat)
printf("summary %s\n", ci.summary.buf);
if (suspect->commit->object.flags & UNINTERESTING)
printf("boundary\n");
-   if (suspect->previous) {
-   struct origin *prev = suspect->previous;
-   printf("previous %s ", oid_to_hex(>commit->object.oid));
-   write_name_quoted(prev->path, stdout, '\n');
-   }
 
commit_info_destroy();
 
@@ -1760,7 +1765,7 @@ static void found_guilty_entry(struct blame_entry *ent,
   oid_to_hex(>commit->object.oid),
   ent->s_lno + 1, ent->lno + 1, ent->num_lines);
emit_one_suspect_detail(suspect, 0);
-   write_filename_info(suspect->path);
+   write_filename_info(suspect);
maybe_flush_or_die(stdout, "stdout");
}
pi->blamed_lines += 

[PATCH 1/3] blame: fix alignment with --abbrev=40

[Jeff King]

The blame command internally adds 1 to any requested sha1
abbreviation length, and then subtracts it when outputting a
boundary commit. This lets regular and boundary sha1s line
up visually, but it misses one corner case.

When the requested length is 40, we bump the value to 41.
But since we only have 40 characters, that's all we can show
(fortunately the truncation is done by a printf precision
field, so it never tries to read past the end of the
buffer).  So a normal sha1 shows 40 hex characters, and a
boundary sha1 shows "^" plus 40 hex characters. The result
is misaligned.

The "-l" option to show long sha1s gets around this by
skipping the "abbrev" variable entirely and just always
using GIT_SHA1_HEXSZ.  This avoids the "+1" issue, but it
does mean that boundary commits only have 39 characters
printed.  This is somewhat odd, but it does look good
visually: the results are aligned and left-justified. The
alternative would be to allocate an extra column that would
contain either an extra space or the "^" boundary marker.

As this is by definition the human-readable view, it's
probably not that big a deal either way (and of course
--porcelain, etc, correctly produce correct 40-hex sha1s).
But for consistency, this patch teaches --abbrev=40 to
produce the same output as "-l" (always left-aligned, with
40-hex for normal sha1s, and "^" plus 39-hex for
boundaries).

Signed-off-by: Jeff King 
---
I mostly didn't explore the extra-column solution out of a sense of
inertia. The "-l" option has behaved this way for years. But it was also
out of laziness, as I doubt anybody cares too much, and it would require
a fair bit of special-casing in the printing routines.

 builtin/blame.c  |  2 +-
 t/t8002-blame.sh | 28 
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4ddfadb71f..1d312542de 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2656,7 +2656,7 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
} else if (show_progress < 0)
show_progress = isatty(2);
 
-   if (0 < abbrev)
+   if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
/* one more abbrev length is needed for the boundary commit */
abbrev++;
 
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index ab79de9544..c6347ad8fd 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -86,4 +86,32 @@ test_expect_success 'blame with showEmail config true' '
test_cmp expected_n result
 '
 
+test_expect_success 'set up abbrev tests' '
+   test_commit abbrev &&
+   sha1=$(git rev-parse --verify HEAD) &&
+   check_abbrev () {
+   expect=$1; shift
+   echo $sha1 | cut -c 1-$expect >expect &&
+   git blame "$@" abbrev.t >actual &&
+   perl -lne "/[0-9a-f]+/ and print \$&" actual.sha &&
+   test_cmp expect actual.sha
+   }
+'
+
+test_expect_success 'blame --abbrev= works' '
+   # non-boundary commits get +1 for alignment
+   check_abbrev 31 --abbrev=30 HEAD &&
+   check_abbrev 30 --abbrev=30 ^HEAD
+'
+
+test_expect_success 'blame -l aligns regular and boundary commits' '
+   check_abbrev 40 -l HEAD &&
+   check_abbrev 39 -l ^HEAD
+'
+
+test_expect_success 'blame --abbrev=40 behaves like -l' '
+   check_abbrev 40 --abbrev=40 HEAD &&
+   check_abbrev 39 --abbrev=40 ^HEAD
+'
+
 test_done
-- 
2.11.0.519.g31435224cf

[PATCH 2/3] blame: handle --no-abbrev

[Jeff King]

You can already ask blame for full sha1s with "-l" or with
"--abbrev=40". But for consistency with other parts of Git,
we should support "--no-abbrev".

Worse, blame already accepts --no-abbrev, but it's totally
broken. When we see --no-abbrev, the abbrev variable is set
to 0, which is then used as a printf precision. For regular
sha1s, that means we print nothing at all (which is very
wrong). For boundary commits we decrement it to "-1", which
printf interprets as "no limit" (which is almost correct,
except it misses the 39-length magic explained in the
previous commit).

Let's detect --no-abbrev and behave as if --abbrev=40 was
given.

Signed-off-by: Jeff King 
---
I also wondered if we needed to clamp this within MINIMUM_ABBREV, but
that is done for us already by the parseopt handler.

 builtin/blame.c  | 2 ++
 t/t8002-blame.sh | 4 
 2 files changed, 6 insertions(+)

diff --git a/builtin/blame.c b/builtin/blame.c
index 1d312542de..c6170fed81 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2659,6 +2659,8 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
if (0 < abbrev && abbrev < GIT_SHA1_HEXSZ)
/* one more abbrev length is needed for the boundary commit */
abbrev++;
+   else if (!abbrev)
+   abbrev = GIT_SHA1_HEXSZ;
 
if (revs_file && read_ancestry(revs_file))
die_errno("reading graft file '%s' failed", revs_file);
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index c6347ad8fd..380e1c1054 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -114,4 +114,8 @@ test_expect_success 'blame --abbrev=40 behaves like -l' '
check_abbrev 39 --abbrev=40 ^HEAD
 '
 
+test_expect_success '--no-abbrev works like --abbrev=40' '
+   check_abbrev 40 --no-abbrev
+'
+
 test_done
-- 
2.11.0.519.g31435224cf

[PATCH 0/3] fixing some random blame corner cases

[Jeff King]

Here are three fixes for some fairly obscure corner cases. I haven't
actually seen these in the wild. I came up with the final one while
discussing a hypothetical with somebody, then ran across the middle one
while trying to write a test for the third, which made me scratch my
head enough to yield the first one. Classic yak-shaving.

One other thing that surprised me while writing blame tests is that
"--root" is not the default for git-blame (though it has been for many
years in git-log). I'm not sure if it would be a good idea to change it,
or if blame is too plumbing-ish to allow that.

  [1/3]: blame: fix alignment with --abbrev=40
  [2/3]: blame: handle --no-abbrev
  [3/3]: blame: output porcelain "previous" header for each file

 builtin/blame.c |  27 ++
 t/t8002-blame.sh|  32 
 t/t8011-blame-split-file.sh | 117 
 3 files changed, 166 insertions(+), 10 deletions(-)
 create mode 100755 t/t8011-blame-split-file.sh

-Peff

Re: [PATCH v2 2/4] t7610: make tests more independent and debuggable

[Stefan Beller]

On Thu, Jan 5, 2017 at 5:09 PM, Richard Hansen  wrote:
> Reduce how much a test can interfere other tests:

A bullet point list as an unordered list often indicates that you're
doing multiple
things at once, possibly unrelated, so they could go into different patches. ;)

While telling you to make even more commits: you may also want to make
a patch with an entry to the .mailmap file (assuming you're the same
Richard Hansen that contributed from rhan...@bbn.com);
Welcome to Google!

>
>   * Move setup code that multiple tests depend on to the 'setup' test
> case.
>   * Run 'git reset --hard' after every test (pass or fail) to clean up
> in case the test fails and leaves the repository in a strange
> state.
>   * If the repository must be in a particular state (beyond what is
> already done by the 'setup' test case) before the test can run,
> make the necessary repository changes in the test script even if
> it means duplicating some lines of code from the previous test
> case.
>   * Never assume that a particular commit is checked out.
>   * Always work on a test-specific branch when the test might create a
> commit.  This is not always necessary for correctness, but it
> improves debuggability by ensuring a commit created by test #N
> shows up on the testN branch, not the branch for test #N-1.




> @@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
>  '
>
>  test_expect_success 'mergetool crlf' '
> +   test_when_finished "git reset --hard" &&
> test_config core.autocrlf true &&
> git checkout -b test$test_count branch1 &&
> test_must_fail git merge master >/dev/null 2>&1 &&
> @@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
> git submodule update -N &&
> test "$(cat submod/bar)" = "master submodule" &&
> git commit -m "branch1 resolved with mergetool - autocrlf" &&

> -   test_config core.autocrlf false &&
> -   git reset --hard
> +   test_config core.autocrlf false
>  '

This is the nit that led me to writing this email in the first place:
test_config is a function that sets a configuration for a single test only,
so it makes no sense as the last statement of a test. (In its implementation
it un-configures with test_when_finished)

So I think we do not want to add it back, but rather remove this
test_config statement.

But to do this we need to actually be careful with the order of the newly
added test_when_finished "git reset --hard" and  test_config core.autocrlf true,
which uses test_when_finished internally.

The order seems correct to me, as the reset would be executed after the
"test_config core.autocrlf true" is un-configured.

[PATCH v2 0/4] fix mergetool+rerere+subdir regression

[Richard Hansen]

If rerere is enabled, no pathnames are given, and mergetool is run
from a subdirectory, mergetool always prints "No files need merging".
Fix the bug.

This regression was introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Changes since v1:
  * Patch 2/4 was reworked to improve the commit message, improve test
case independence even further, and use 'test_when_finished "git
reset --hard"' instead of a plain 'git reset --hard'.

Richard Hansen (4):
  t7610: update branch names to match test number
  t7610: make tests more independent and debuggable
  t7610: add test case for rerere+mergetool+subdir bug
  mergetool: fix running in subdir when rerere enabled

 git-mergetool.sh |   1 +
 t/t7610-mergetool.sh | 251 ++-
 2 files changed, 147 insertions(+), 105 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 2/4] t7610: make tests more independent and debuggable

[Richard Hansen]

Reduce how much a test can interfere other tests:

  * Move setup code that multiple tests depend on to the 'setup' test
case.
  * Run 'git reset --hard' after every test (pass or fail) to clean up
in case the test fails and leaves the repository in a strange
state.
  * If the repository must be in a particular state (beyond what is
already done by the 'setup' test case) before the test can run,
make the necessary repository changes in the test script even if
it means duplicating some lines of code from the previous test
case.
  * Never assume that a particular commit is checked out.
  * Always work on a test-specific branch when the test might create a
commit.  This is not always necessary for correctness, but it
improves debuggability by ensuring a commit created by test #N
shows up on the testN branch, not the branch for test #N-1.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 146 +--
 1 file changed, 84 insertions(+), 62 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..2c06cf73f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -55,6 +55,22 @@ test_expect_success 'setup' '
git rm file12 &&
git commit -m "branch1 changes" &&
 
+   git checkout -b delete-base branch1 &&
+   mkdir -p a/a &&
+   (echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
+   git add a/a/file.txt &&
+   git commit -m"base file" &&
+   git checkout -b move-to-b delete-base &&
+   mkdir -p b/b &&
+   git mv a/a/file.txt b/b/file.txt &&
+   (echo one; echo two; echo 4) >b/b/file.txt &&
+   git commit -a -m"move to b" &&
+   git checkout -b move-to-c delete-base &&
+   mkdir -p c/c &&
+   git mv a/a/file.txt c/c/file.txt &&
+   (echo one; echo two; echo 3) >c/c/file.txt &&
+   git commit -a -m"move to c" &&
+
git checkout -b stash1 master &&
echo stash1 change file11 >file11 &&
git add file11 &&
@@ -86,6 +102,23 @@ test_expect_success 'setup' '
git rm file11 &&
git commit -m "master updates" &&
 
+   git clean -fdx &&
+   git checkout -b order-file-start master &&
+   echo start >a &&
+   echo start >b &&
+   git add a b &&
+   git commit -m start &&
+   git checkout -b order-file-side1 order-file-start &&
+   echo side1 >a &&
+   echo side1 >b &&
+   git add a b &&
+   git commit -m side1 &&
+   git checkout -b order-file-side2 order-file-start &&
+   echo side2 >a &&
+   echo side2 >b &&
+   git add a b &&
+   git commit -m side2 &&
+
git config merge.tool mytool &&
git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
git config mergetool.mytool.trustExitCode true &&
@@ -94,6 +127,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -112,6 +146,7 @@ test_expect_success 'custom mergetool' '
 '
 
 test_expect_success 'mergetool crlf' '
+   test_when_finished "git reset --hard" &&
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -129,11 +164,11 @@ test_expect_success 'mergetool crlf' '
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
git commit -m "branch1 resolved with mergetool - autocrlf" &&
-   test_config core.autocrlf false &&
-   git reset --hard
+   test_config core.autocrlf false
 '
 
 test_expect_success 'mergetool in subdir' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -145,8 +180,13 @@ test_expect_success 'mergetool in subdir' '
 '
 
 test_expect_success 'mergetool on file in parent dir' '
+   test_when_finished "git reset --hard" &&
+   git checkout -b test$test_count branch1 &&
+   git submodule update -N &&
(
cd subdir &&
+   test_must_fail git merge master >/dev/null 2>&1 &&
+   ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 
2>&1 ) &&
( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
@@ -161,6 +201,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -169,11 

[PATCH v2 4/4] mergetool: fix running in subdir when rerere enabled

[Richard Hansen]

If rerere is enabled and no pathnames are given, run cd_to_toplevel
before running 'git diff --name-only' so that 'git diff --name-only'
sees the pathnames emitted by 'git rerere remaining'.

Also run cd_to_toplevel before running 'git rerere remaining' in case
'git rerere remaining' is ever changed to print pathnames relative to
the current directory rather than to $GIT_WORK_TREE.

This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Signed-off-by: Richard Hansen 
---
 git-mergetool.sh | 1 +
 t/t7610-mergetool.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..67ea0d6db 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -456,6 +456,7 @@ main () {
 
if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
+   cd_to_toplevel
set -- $(git rerere remaining)
if test $# -eq 0
then
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 1cef1ec2e..5d76772cf 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -231,7 +231,7 @@ test_expect_success 'mergetool merges all from subdir 
(rerere disabled)' '
)
 '
 
-test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+test_expect_success 'mergetool merges all from subdir (rerere enabled)' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled true &&
-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2 1/4] t7610: update branch names to match test number

[Richard Hansen]

Rename the testNN branches so that NN matches the test number.  This
should make it easier to troubleshoot test issues.  Use $test_count to
keep this future-proof.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 82 ++--
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6d9f21511..14090739f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,7 +94,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
-   git checkout -b test1 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -113,7 +113,7 @@ test_expect_success 'custom mergetool' '
 
 test_expect_success 'mergetool crlf' '
test_config core.autocrlf true &&
-   git checkout -b test2 branch1 &&
+   git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
@@ -134,7 +134,7 @@ test_expect_success 'mergetool crlf' '
 '
 
 test_expect_success 'mergetool in subdir' '
-   git checkout -b test3 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -161,7 +161,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
-   git checkout -b test4 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -192,7 +192,7 @@ test_expect_success 'mergetool merges all from subdir' '
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
-   git checkout -b test5 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
@@ -233,7 +233,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
 test_expect_success 'mergetool takes partial path' '
git reset --hard &&
test_config rerere.enabled false &&
-   git checkout -b test12 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
 
@@ -308,12 +308,12 @@ test_expect_success 'mergetool keeps tempfiles when 
aborting delete/delete' '
 '
 
 test_expect_success 'deleted vs modified submodule' '
-   git checkout -b test6 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
git rm --cached submod &&
git commit -m "Submodule deleted from branch" &&
-   git checkout -b test6.a test6 &&
+   git checkout -b test$test_count.a test$test_count &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
@@ -329,7 +329,7 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by keeping module" &&
 
mv submod submod-movedaside &&
-   git checkout -b test6.b test6 &&
+   git checkout -b test$test_count.b test$test_count &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -343,9 +343,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
 
mv submod-movedaside submod &&
-   git checkout -b test6.c master &&
+   git checkout -b test$test_count.c master &&
git submodule update -N &&
-   test_must_fail git merge test6 &&
+   test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -359,9 +359,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
mv submod.orig submod &&
 
-   git checkout -b test6.d master &&
+   git checkout -b test$test_count.d master &&
git submodule update -N &&
-   test_must_fail git merge test6 &&
+   test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 

[PATCH v2 3/4] t7610: add test case for rerere+mergetool+subdir bug

[Richard Hansen]

If rerere is enabled and mergetool is run from a subdirectory,
mergetool always prints "No files need merging".  Add an expected
failure test case for this situation.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 2c06cf73f..1cef1ec2e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -213,7 +213,7 @@ test_expect_success 'mergetool skips autoresolved' '
test "$output" = "No files need merging"
 '
 
-test_expect_success 'mergetool merges all from subdir' '
+test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
@@ -231,6 +231,25 @@ test_expect_success 'mergetool merges all from subdir' '
)
 '
 
+test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+   test_when_finished "git reset --hard" &&
+   git checkout -b test$test_count branch1 &&
+   test_config rerere.enabled true &&
+   rm -rf .git/rr-cache &&
+   (
+   cd subdir &&
+   test_must_fail git merge master &&
+   ( yes "r" | git mergetool ../submod ) &&
+   ( yes "d" "d" | git mergetool --no-prompt ) &&
+   test "$(cat ../file1)" = "master updated" &&
+   test "$(cat ../file2)" = "master new" &&
+   test "$(cat file3)" = "master new sub" &&
+   ( cd .. && git submodule update -N ) &&
+   test "$(cat ../submod/bar)" = "master submodule" &&
+   git commit -m "branch2 resolved by mergetool from subdir"
+   )
+'
+
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_when_finished "git reset --hard" &&
test_config rerere.enabled true &&
-- 
2.11.0.390.gc69c2f50cf-goog



smime.p7s
Description: S/MIME Cryptographic Signature

[PATCHv6 1/2] submodule tests: don't use itself as a submodule

[Stefan Beller]

In reality nobody would run "git submodule add ./. "
to add the repository to itself as a submodule as this comes with some
nasty surprises, such as infinite recursion when cloning that repository.
However we do this all the time in the test suite, because most of the
time this was the most convenient way to test a very specific thing
for submodule behavior.

This provides an easier way to have submodules in tests, by just setting
TEST_CREATE_SUBMODULE to a non empty string, similar to
TEST_NO_CREATE_REPO.

Make use of it in those tests that add a submodule from ./. except for
the occurrence in create_lib_submodule_repo as there it seems we craft
a repository deliberately for both inside as well as outside use.

The name "pretzel.[non]bare" was chosen deliberate to not introduce
more strings to the test suite containing "sub[module]" as searching for
"sub" already yields a lot of hits from different contexts. "pretzel"
doesn't occur in the test suite yet, so it is a good candidate for
a potential remote for a submodule.

Signed-off-by: Stefan Beller 
---
 t/lib-submodule-update.sh |  2 ++
 t/t7001-mv.sh |  5 +++--
 t/t7507-commit-verbose.sh |  4 +++-
 t/t7800-difftool.sh   |  4 +++-
 t/test-lib-functions.sh   | 16 
 5 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 79cdd34a54..58d76d9df8 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -44,6 +44,8 @@ create_lib_submodule_repo () {
git branch "no_submodule" &&
 
git checkout -b "add_sub1" &&
+   # Adding the repo itself as a submodule is a hack.
+   # Do not imitate this.
git submodule add ./. sub1 &&
git config -f .gitmodules submodule.sub1.ignore all &&
git config submodule.sub1.ignore all &&
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index e365d1ff77..6cb32f3a3a 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='git mv in subdirs'
+TEST_CREATE_SUBMODULE=yes
 . ./test-lib.sh
 
 test_expect_success \
@@ -288,12 +289,12 @@ rm -f moved symlink
 test_expect_success 'setup submodule' '
git commit -m initial &&
git reset --hard &&
-   git submodule add ./. sub &&
+   git submodule add ./pretzel.bare sub &&
echo content >file &&
git add file &&
git commit -m "added sub and file" &&
mkdir -p deep/directory/hierarchy &&
-   git submodule add ./. deep/directory/hierarchy/sub &&
+   git submodule add ./pretzel.bare deep/directory/hierarchy/sub &&
git commit -m "added another submodule" &&
git branch submodule
 '
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index ed2653d46f..d269900afa 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 
 test_description='verbose commit template'
+TEST_CREATE_SUBMODULE=yes
 . ./test-lib.sh
 
 write_script "check-for-diff" <<\EOF &&
@@ -74,11 +75,12 @@ test_expect_success 'diff in message is retained with -v' '
 
 test_expect_success 'submodule log is stripped out too with -v' '
git config diff.submodule log &&
-   git submodule add ./. sub &&
+   git submodule add ./pretzel.bare sub &&
git commit -m "sub added" &&
(
cd sub &&
echo "more" >>file &&
+   git add file &&
git commit -a -m "submodule commit"
) &&
(
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461a..d13a5d0453 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -7,6 +7,7 @@ test_description='git-difftool
 
 Testing basic diff tool invocation
 '
+TEST_CREATE_SUBMODULE=Yes
 
 . ./test-lib.sh
 
@@ -534,7 +535,8 @@ test_expect_success PERL 'difftool --no-symlinks detects 
conflict ' '
 '
 
 test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
-   git submodule add ./. submod/ule &&
+   git submodule add ./pretzel.bare submod/ule &&
+   test_commit -C submod/ule second_commit &&
test_config -C submod/ule diff.tool checktrees &&
test_config -C submod/ule difftool.checktrees.cmd '\''
test -d "$LOCAL" && test -d "$REMOTE" && echo good
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 579e812506..aa327a7dff 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -800,6 +800,22 @@ test_create_repo () {
error "cannot run git init -- have you built things yet?"
mv .git/hooks .git/hooks-disabled
) || exit
+   if test -n "$TEST_CREATE_SUBMODULE"
+   then
+   (
+   cd "$repo"
+   TEST_CREATE_SUBMODULE=
+   export TEST_CREATE_SUBMODULE
+   test_create_repo 

[PATCHv6 0/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

v6:
* rebased on top of origin/bw/pathspec-cleanup, resolving conflicts.
  (Additionally needs merging with origin/sb/submodule-embed-gitdir to have 
  6f94351b0, test-lib-functions.sh: teach test_commit -C )
* reworded comments and commit message
* do not reuse the strip_submodule_slash_expensive function, but have
  a dedicated die_inside_submodule_path function.

v5:
* was just resending the latest patch, which turns out to be in conflict with
  origin/bw/pathspec-cleanup

v4:
> It MIGHT be a handy hack when writing a test, but let's stop doing
> that insanity.  No sane project does that in real life, doesn't it?

> Create a subdirectory, make it a repository, have a commit there and
> bind that as our own submodule.  That would be a more normal way to
> start your own superproject and its submodule pair if they originate
> together at the same place.

This comes as an extra patch before the actual fix.
The actual fixing patch was reworded borrowing some words from Jeff.
As this makes use of "test_commit -C", it goes on top of 
sb/submodule-embed-gitdir

v3:
more defensive and with tests.


Stefan Beller (2):
  submodule tests: don't use itself as a submodule
  pathspec: give better message for submodule related pathspec error

 pathspec.c   | 31 ++-
 t/lib-submodule-update.sh|  2 ++
 t/t6134-pathspec-in-submodule.sh | 33 +
 t/t7001-mv.sh|  5 +++--
 t/t7507-commit-verbose.sh|  4 +++-
 t/t7800-difftool.sh  |  4 +++-
 t/test-lib-functions.sh  | 16 
 7 files changed, 82 insertions(+), 13 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

-- 
2.11.0.31.g919a8d0.dirty

[PATCHv6 2/2] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

Every once in a while someone complains to the mailing list to have
run into this weird assertion[1]. The usual response from the mailing
list is link to old discussions[2], and acknowledging the problem
stating it is known.

This patch accomplishes two things:

  1. Switch assert() to die("BUG") to give a more readable message.

  2. Take one of the cases where we hit a BUG and turn it into a normal
 "there was something wrong with the input" message.

     This assertion triggered for cases where there wasn't a programming
     bug, but just bogus input. In particular, if the user asks for a
     pathspec that is inside a submodule, we shouldn't assert() or
     die("BUG"); we should tell the user their request is bogus.

 The only reason we did not check for it, is the expensive nature
 of such a check, so callers avoid setting the flag
 PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due
 to bogus input, the expense of CPU cycles spent outweighs the user
 wondering what went wrong, so run that check unconditionally before
 dying with a more generic error message.

Note: There is a case (e.g. "git -C submodule add .") in which we call
strip_submodule_slash_expensive, as git-add requests it via the flag
PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to
trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,
such that we executed

if (item->nowildcard_len < prefixlen)
item->nowildcard_len = prefixlen;

and prefixlen was not adapted (e.g. it was computed from "submodule/")
So in the die_inside_submodule_path function we also need handle paths,
that were stripped before, i.e. are the exact submodule path. This
is why the conditions in die_inside_submodule_path are slightly
different than in strip_submodule_slash_expensive.

[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] 
http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html

Helped-by: Jeff King 
Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
 pathspec.c   | 35 +--
 t/t6134-pathspec-in-submodule.sh | 33 +
 2 files changed, 66 insertions(+), 2 deletions(-)
 create mode 100755 t/t6134-pathspec-in-submodule.sh

diff --git a/pathspec.c b/pathspec.c
index d4efcf6662..42cd83c235 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct 
pathspec_item *item)
}
 }
 
+static void die_inside_submodule_path(struct pathspec_item *item)
+{
+   int i;
+
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+   int ce_len = ce_namelen(ce);
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   if (item->len < ce_len ||
+   !(item->match[ce_len] == '/' || item->match[ce_len] == 
'\0') ||
+   memcmp(ce->name, item->match, ce_len))
+   continue;
+
+   die(_("Pathspec '%s' is in submodule '%.*s'"),
+   item->original, ce_len, ce->name);
+   }
+}
+
 /*
  * Perform the initialization of a pathspec_item based on a pathspec element.
  */
@@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, 
unsigned flags,
}
 
/* sanity checks, pathspec matchers assume these are sane */
-   assert(item->nowildcard_len <= item->len &&
-  item->prefix <= item->len);
+   if (item->nowildcard_len > item->len ||
+   item->prefix > item->len) {
+   /*
+* This case can be triggered by the user pointing us to a
+* pathspec inside a submodule, which is an input error.
+* Detect that here and complain, but fallback in the
+* non-submodule case to a BUG, as we have no idea what
+* would trigger that.
+*/
+   die_inside_submodule_path(item);
+   die ("BUG: item->nowildcard_len > item->len || item->prefix > 
item->len)");
+   }
 }
 
 static int pathspec_item_cmp(const void *a_, const void *b_)
diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh
new file mode 100755
index 00..2900d8d06e
--- /dev/null
+++ b/t/t6134-pathspec-in-submodule.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test case exclude pathspec'
+
+TEST_CREATE_SUBMODULE=yes
+. ./test-lib.sh
+
+test_expect_success 'setup a submodule' '
+   git submodule add ./pretzel.bare sub &&
+   git commit -a -m "add submodule" &&
+   git submodule deinit --all
+'
+
+cat 

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Jeff King]

On Thu, Jan 05, 2017 at 11:05:29AM +0100, Lars Schneider wrote:

> > The git-scm.com site uses asciidoctor, too, and I think I have seen some
> > oddness with the rendering though. So in general I am in favor of making
> > things work under both asciidoc and asciidoctor.
> 
> I am not familiar with both tools but it sounds to me as if "asciidoctor"
> is kind of the "lowest common denominator". Is this true? If yes, would it
> make sense to switch TravisCI [1] to asciidocter if this change gets merged?

I don't think that's quite true.

The two programs produce different output for certain inputs. We tend to
see the cases where asciidoc produces the desired output and asciidoctor
doesn't, because traditionally the documentation was written _for_
asciidoc. So whenever asciidoctor diverges, it looks like a bug.

If people start developing and checking their work using asciidoctor[1],
then we'll see bugs going in the other direction.

As far as CI goes, I am not altogether convinced of the usefulness of
building the documentation. It's very expensive, and the failure mode is
rarely "whoops, running `make doc` failed". It's almost always that the
output looks subtly wrong, but that's very hard to check automatically.

[1] I think we've also traditionally considered asciidoc to be the
definitive toolchain, and people using asciidoctor are free to
submit patches to make things work correctly in both places. I'm not
opposed to changing that attitude, as it seems like asciidoctor is
faster and more actively maintained these days. But I suspect our
build chain would need some improvements. Last time I tried building
with AsciiDoctor it involved a lot manual tweaking of Makefile
variables. It sounds like Dscho is doing it regularly, though. It
should probably work out of the box (with something like
USE_ASCIIDOCTOR=Yes) if we expect people to actually rely on it.

Re: core.sshCommand and url.*.insteadOf for submodules

[Stefan Beller]

On Thu, Jan 5, 2017 at 9:02 AM, Jeff King  wrote:
> On Thu, Jan 05, 2017 at 05:53:30AM -0800, Stefan Beller wrote:
>
>> > My scenario is as follows: I use 2 SSH keys for GitHub, for private and
>> > work-related repositories. My default key is my private key. So when I
>> > clone a work repository and try getting the submodules, `git submodule
>> > update --init` fails. This is also the case when setting
>> > `core.sshCommand` and `url.*.insteadOf` (useful for substituting
>> > "github.com" by some ~/.ssh/config'ured host).
>>
>> which is why e.g.
>> git config --global url.https://github.com/.insteadOf git://github.com/
>> is not your preferred way here.
>>
>> There was some discussion a couple of weeks ago, which settings
>> should be kept when recursing into submodules, Jacob and Jeff cc'd.
>
> The only discussion I recall was from last May. But that was about "-c"
> config on the command-line, and the end decision was that we pass it all
> down to submodules, per 89044baa8b (submodule: stop sanitizing config
> options, 2016-05-04).

Oh, yeah that was the difference.

>
> I think the problem here is more about propagating options from the
> superproject's repo-level config into the submodules. AFAIK we do not do
> that at all, but I may have missed some patches in that area.

AFAIK there were no such patches yet.

>
> Another approach would be conditional config includes based on the repo
> path. With the patches discussed in [1], you could do something like:
>
>   git config --global include./path/to/work/repos.path .gitconfig-work
>   git config -f ~/.gitconfig-work url.foo.insteadOf bar

Or maybe we could specialize these patches to allow
includes from specific other repos, i.e. superproject(s) or worktrees.

>
> -Peff
>
> [1] http://public-inbox.org/git/20160626070617.30211-1-pclo...@gmail.com/

Re: core.sshCommand and url.*.insteadOf for submodules

[Jeff King]

On Thu, Jan 05, 2017 at 05:53:30AM -0800, Stefan Beller wrote:

> > My scenario is as follows: I use 2 SSH keys for GitHub, for private and
> > work-related repositories. My default key is my private key. So when I
> > clone a work repository and try getting the submodules, `git submodule
> > update --init` fails. This is also the case when setting
> > `core.sshCommand` and `url.*.insteadOf` (useful for substituting
> > "github.com" by some ~/.ssh/config'ured host).
> 
> which is why e.g.
> git config --global url.https://github.com/.insteadOf git://github.com/
> is not your preferred way here.
> 
> There was some discussion a couple of weeks ago, which settings
> should be kept when recursing into submodules, Jacob and Jeff cc'd.

The only discussion I recall was from last May. But that was about "-c"
config on the command-line, and the end decision was that we pass it all
down to submodules, per 89044baa8b (submodule: stop sanitizing config
options, 2016-05-04).

I think the problem here is more about propagating options from the
superproject's repo-level config into the submodules. AFAIK we do not do
that at all, but I may have missed some patches in that area.

Another approach would be conditional config includes based on the repo
path. With the patches discussed in [1], you could do something like:

  git config --global include./path/to/work/repos.path .gitconfig-work
  git config -f ~/.gitconfig-work url.foo.insteadOf bar

-Peff

[1] http://public-inbox.org/git/20160626070617.30211-1-pclo...@gmail.com/

Re: [PATCH 2/4] t7610: make tests more independent and debuggable

[Richard Hansen]

On 2017-01-04 15:27, Stefan Beller wrote:

On Tue, Jan 3, 2017 at 4:50 PM, Richard Hansen  wrote:

If a test fails it might leave the repository in a strange state.  Add
'git reset --hard' at the beginning of each test to increase the odds
of passing when an earlier test fails.


So each test is cleaning up the previous test, which *may* confuse
a reader ("how is the reset --hard relevant for this test? Oooh it's
just a cleanup").

We could put it another way by having each test itself make clean
up after itself via

  test_when_finished "git reset --hard" &&
  ..

at the beginning of each test.
This would produce the same order of operations, i.e. a
reset run between each test, but semantically tells the reader
that the reset is part of the current test cleaning up after itself,
as "reset" is operation for this particular test to cleanup.
Does that make sense?


I like that idea; thanks for the suggestion.  I'll cook up a reroll.



Also use test-specific branches to avoid interfering with later tests
and to make the tests easier to debug.


That sounds great!
Though in the code I only spot one occurrence for

+   git checkout -b test$test_count branch1 &&

so maybe that could be part of the first patch in the series?


There are two; the other is buried in the change for the 'mergetool on 
file in parent dir' test.


Thanks for the review,
Richard




Thanks,
Stefan

Re: [PATCH 2/4] t7610: make tests more independent and debuggable

[Richard Hansen]

On 2017-01-05 07:20, Simon Ruderich wrote:

On Tue, Jan 03, 2017 at 07:50:40PM -0500, Richard Hansen wrote:

[snip]
@@ -145,8 +148,13 @@ test_expect_success 'mergetool in subdir' '
 '

 test_expect_success 'mergetool on file in parent dir' '
+   git reset --hard &&
+   git checkout -b test$test_count branch1 &&
+   git submodule update -N &&
(
cd subdir &&
+   test_must_fail git merge master >/dev/null 2>&1 &&
+   ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&


This change seems unrelated to the changes mentioned in the
commit message. Was it intended?


Yes, that is intentional; without this change, the test depends on the 
successful completion of the previous test.  I'll improve the commit 
message.


Thanks,
Richard



Regards
Simon


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH 2/4] t7610: make tests more independent and debuggable

[Richard Hansen]

On 2017-01-04 15:27, Stefan Beller wrote:

On Tue, Jan 3, 2017 at 4:50 PM, Richard Hansen  wrote:

If a test fails it might leave the repository in a strange state.  Add
'git reset --hard' at the beginning of each test to increase the odds
of passing when an earlier test fails.


So each test is cleaning up the previous test, which *may* confuse
a reader ("how is the reset --hard relevant for this test? Oooh it's
just a cleanup").

We could put it another way by having each test itself make clean
up after itself via

  test_when_finished "git reset --hard" &&
  ..

at the beginning of each test.
This would produce the same order of operations, i.e. a
reset run between each test, but semantically tells the reader
that the reset is part of the current test cleaning up after itself,
as "reset" is operation for this particular test to cleanup.
Does that make sense?


I like that idea; thanks for the suggestion.  I'll cook up a reroll.



Also use test-specific branches to avoid interfering with later tests
and to make the tests easier to debug.


That sounds great!
Though in the code I only spot one occurrence for

+   git checkout -b test$test_count branch1 &&

so maybe that could be part of the first patch in the series?


There are two; the other is buried in the change for the 'mergetool on 
file in parent dir' test.  I'll improve the commit message to make it 
more clear.


Thanks for the review,
Richard




Thanks,
Stefan


smime.p7s
Description: S/MIME Cryptographic Signature

Regression: Ctrl-c from the pager in an alias exits it

[Trygve Aaberge]

I'm experiencing an issue when using aliases for commands that open the pager.
When I press Ctrl-c from the pager, it exits. This does not happen when I
don't use an alias and did not happen before. It causes problems because
Ctrl-c is also used for other things, such as canceling a search that hasn't
completed.

To reproduce, create e.g. the alias `l = log` and run `git l`. Then press
Ctrl-c. The expected behavior is that nothing happens. The actual behavior is
that the pager exits.

I bisected the repo, and found that the commit 86d26f240 [0] introduced the
issue.

[0]: 86d26f240 (setup.c: re-fix d95138e (setup: set env $GIT_WORK_TREE
when .. - 2015-12-20)

-- 
Trygve Aaberge

Re: git branch -D doesn't work with deleted worktree

[Stefan Beller]

On Thu, Jan 5, 2017 at 2:06 AM, Roland Illig  wrote:
> Git 2.11.0 gives a wrong error message after the following commands:
>
> $ git init
> $ echo hello >file
> $ git add file
> $ git commit -m "message"
> $ git worktree add ../worktree
> $ rm -rf ../worktree
> $ git br -D worktree
> error: Cannot delete branch 'worktree' checked out at '../worktree'
>
> Since ../worktree has been deleted, there cannot be anything checked out at 
> that location.
>
> In my opinion, deleting the branch should just work. Especially since I used 
> the -D option and the "git worktree" documentation says "When you are done 
> with a linked working tree you can simply delete it."
>
> Regards,
> Roland
>

Re: core.sshCommand and url.*.insteadOf for submodules

[Stefan Beller]

On Thu, Jan 5, 2017 at 2:09 AM, Stefan Schindler  wrote:
> Hello mailing list,
>
> it seems like that the `core.sshCommand` and `url.*.insteadOf`
> configuration settings do not apply to `git submodule update --init`
> (and probably related) calls.
>
> Is this intentional?

The original design of submodules was to have a submodule to be a
standalone repository, such that e.g. its options are read from its own
config file. So the original vision was to decouple the init and clone of the
submodule to allow the user to change the settings:

git submodule init
# copies the submodule..URL from .gitmodules to .git/config
# user realizes that the URL is not a good idea, such that
git  config submodule..url http://${company-mirror}/submodule
# now the url is fixed so
git submodule update

I guess it could be a good idea to propagate some settings from the
superproject to the submodules when they are cloned.

>
> My scenario is as follows: I use 2 SSH keys for GitHub, for private and
> work-related repositories. My default key is my private key. So when I
> clone a work repository and try getting the submodules, `git submodule
> update --init` fails. This is also the case when setting
> `core.sshCommand` and `url.*.insteadOf` (useful for substituting
> "github.com" by some ~/.ssh/config'ured host).
>

which is why e.g.
git config --global url.https://github.com/.insteadOf git://github.com/
is not your preferred way here.

There was some discussion a couple of weeks ago, which settings
should be kept when recursing into submodules, Jacob and Jeff cc'd.

> Greetings,
> Stefan Schindler

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Johannes Schindelin]

Hi Lars,

On Thu, 5 Jan 2017, Lars Schneider wrote:

> > On 04 Jan 2017, at 09:08, Jeff King  wrote:
> > 
> > On Mon, Jan 02, 2017 at 05:03:57PM +0100, Johannes Schindelin wrote:
> > 
> >> From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= 
> >> 
> >> The `user-manual.txt` is designed as a `book` but the `Makefile`
> >> wants to build it as an `article`. This seems to be a problem when
> >> building the documentation with `asciidoctor`. Furthermore the parts
> >> *Git Glossary* and *Appendix B* had no subsections which is not
> >> allowed when building with `asciidoctor`. So lets add a *dummy*
> >> section.
> > 
> > The git-scm.com site uses asciidoctor, too, and I think I have seen
> > some oddness with the rendering though. So in general I am in favor of
> > making things work under both asciidoc and asciidoctor.
> 
> I am not familiar with both tools but it sounds to me as if
> "asciidoctor" is kind of the "lowest common denominator". Is this true?
> If yes, would it make sense to switch TravisCI [1] to asciidocter if
> this change gets merged?

It is true that asciidoc typically parses whatever asciidoctor parses,
but not vice versa.

In that light, I would love to see our Travis runs to switch to
asciidoctor.

For the record, this is my local config.mak in the asciidoctor worktree:

-- snip --
ASCIIDOC=asciidoctor
ASCIIDOC_HTML=html5
ASCIIDOC_DOCBOOK=docbook45
ASCIIDOC_EXTRA="-alitdd=&\#45;&\#45;"
ASCIIDOC_CONF=-I"/mingw64/lib/asciidoctor-extensions" -rman-inline-macro
-- snap --

Please note that the extensions are required to build correctly (and we
require this patch, too, unfortunately:
https://github.com/git-for-windows/MINGW-packages/blob/master/mingw-w64-asciidoctor-extensions/0001-man-inline-macro-enable-linkgit-syntax.patch).

Ciao,
Dscho

Re: [PATCH 2/4] t7610: make tests more independent and debuggable

[Simon Ruderich]

On Tue, Jan 03, 2017 at 07:50:40PM -0500, Richard Hansen wrote:
> [snip]
> @@ -145,8 +148,13 @@ test_expect_success 'mergetool in subdir' '
>  '
>
>  test_expect_success 'mergetool on file in parent dir' '
> + git reset --hard &&
> + git checkout -b test$test_count branch1 &&
> + git submodule update -N &&
>   (
>   cd subdir &&
> + test_must_fail git merge master >/dev/null 2>&1 &&
> + ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&

This change seems unrelated to the changes mentioned in the
commit message. Was it intended?

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9


signature.asc
Description: PGP signature

Refreshing index timestamps without reading content

[Quentin Casasnovas]

Hi guys,

Apologies if this is documented somewhere, I have fairly bad search vudu
skills.

I'm looking for a way to cause a full refresh of the index without causing
any read of the files, basically telling git "trust me, all worktree files
are matching the index, but their stat information have changed".  I have
read about the update-index --assume-unchanged and --skip-worktree flags in
the documentation, but these do not cause any index refresh - rather, they
fake that the respective worktree files are matching the index until you
remove those assume-unchanged/skip-worktree bits.

This might sound like a really weird thing to do, but I do have a use case
for it - we have some build farm setup where the resulting objects of a
compilation are stored on a shared server.  The source files are not stored
on the shared server, but locally on each of the build server (as to
decrease network load and make good use of local storage as caches).

We then use an onion filesystem to mount the compiled objects on top of the
local sources - and change the modification time of the source to be older
than the object files, so that on subsequent builds, make does not rebuild
the whole world.

This works fine except for one thing, after changing the mtime of the
source files, the first subsequent git command needing to compare the tree
with the index will take a LONG time since it will read all of the object
content:

  cd linux-2.6

  # Less than a second  when the index is up to date
  time git status > /dev/null
  git status 0.06s user 0.09s system 172% cpu 0.087 total
  ~~~

  # Change the mtime..
  git ls-tree -r --name-only HEAD | xargs -n 1024 touch

  # Now 30s..
  time git status > /dev/null
  git status  2.73s user 1.79s system 13% cpu 32.453 total
  

The timing information above was captured on my laptop SSD and the penalty
is obviously much higher on spinning disks - especially when this operation
is done on *hundreds* of different work tree in parallel, all hosted on the
same filesystem (it can take tens of minutes!).

Is there any way to tell git, after the git ls-tree command above, to
refresh its stat cache information and trust us that the file content has
not changed, as to avoid any useless file read (though it will obviously
will have to stat all of them, but that's not something we can really
avoid)

If not, I am willing to implement a --assume-content-unchanged to the git
update-index if you guys don't see something fundamentally wrong with this
approach.

Thanks for any hints you can give! :)

Q


signature.asc
Description: Digital signature

core.sshCommand and url.*.insteadOf for submodules

[Stefan Schindler]

Hello mailing list,

it seems like that the `core.sshCommand` and `url.*.insteadOf`
configuration settings do not apply to `git submodule update --init`
(and probably related) calls.

Is this intentional?

My scenario is as follows: I use 2 SSH keys for GitHub, for private and
work-related repositories. My default key is my private key. So when I
clone a work repository and try getting the submodules, `git submodule
update --init` fails. This is also the case when setting
`core.sshCommand` and `url.*.insteadOf` (useful for substituting
"github.com" by some ~/.ssh/config'ured host).

Greetings,
Stefan Schindler

git branch -D doesn't work with deleted worktree

[Roland Illig]

Git 2.11.0 gives a wrong error message after the following commands:
 
$ git init
$ echo hello >file
$ git add file
$ git commit -m "message"
$ git worktree add ../worktree
$ rm -rf ../worktree
$ git br -D worktree
error: Cannot delete branch 'worktree' checked out at '../worktree'

Since ../worktree has been deleted, there cannot be anything checked out at 
that location.

In my opinion, deleting the branch should just work. Especially since I used 
the -D option and the "git worktree" documentation says "When you are done with 
a linked working tree you can simply delete it."

Regards,
Roland

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Lars Schneider]

> On 04 Jan 2017, at 09:08, Jeff King  wrote:
> 
> On Mon, Jan 02, 2017 at 05:03:57PM +0100, Johannes Schindelin wrote:
> 
>> From: =?UTF-8?q?=EB=A7=88=EB=88=84=EC=97=98?= 
>> 
>> The `user-manual.txt` is designed as a `book` but the `Makefile` wants
>> to build it as an `article`. This seems to be a problem when building
>> the documentation with `asciidoctor`. Furthermore the parts *Git
>> Glossary* and *Appendix B* had no subsections which is not allowed when
>> building with `asciidoctor`. So lets add a *dummy* section.
> 
> The git-scm.com site uses asciidoctor, too, and I think I have seen some
> oddness with the rendering though. So in general I am in favor of making
> things work under both asciidoc and asciidoctor.

I am not familiar with both tools but it sounds to me as if "asciidoctor"
is kind of the "lowest common denominator". Is this true? If yes, would it
make sense to switch TravisCI [1] to asciidocter if this change gets merged?

- Lars

[1] https://github.com/git/git/blob/master/.travis.yml#L48

Re re between individual finance

[Mrs.l.pascal]

Hello;

Mr. I turn to you, I am an individual who provides loans of money to 
all
people in need for financing of all people projects. For more 
information

please contact me mail:lambackpas...@gmx.fr



Hola; Señor. Me dirijo a usted, soy una persona que ofrece préstamos de
dinero a todas las personas necesitan para financiación de todas las
personas. Para más información por favor éntreme en contacto con
mail:lambackpas...@gmx.fr